diff --git a/main.go b/main.go index 3a829ab..8378caf 100644 --- a/main.go +++ b/main.go @@ -21,8 +21,6 @@ var ( appName = "provider-cloudscale" appLongName = "Crossplane provider that deploys resources on cloudscale.ch" - - envPrefix = "" ) func init() { @@ -45,10 +43,9 @@ func newApp() (context.Context, context.CancelFunc, *cli.App) { logInstance := &atomic.Value{} logInstance.Store(logr.Discard()) app := &cli.App{ - Name: appName, - Usage: appLongName, - Version: fmt.Sprintf("%s, revision=%s, date=%s", version, commit, date), - Compiled: compilationDate(), + Name: appName, + Usage: appLongName, + Version: fmt.Sprintf("%s, revision=%s, date=%s", version, commit, date), EnableBashCompletion: true, @@ -94,25 +91,10 @@ func rootAction(hasSubcommands bool) func(context *cli.Context) error { } } -// env combines envPrefix with given suffix delimited by underscore. -func env(suffix string) string { - return envPrefix + "_" + suffix -} - -// envVars combines envPrefix with each given suffix delimited by underscore. func envVars(suffixes ...string) []string { arr := make([]string, len(suffixes)) for i := range suffixes { - arr[i] = env(suffixes[i]) + arr[i] = suffixes[i] } return arr } - -func compilationDate() time.Time { - compiled, err := time.Parse(time.RFC3339, date) - if err != nil { - // an empty Time{} causes cli.App to guess it from binary's file timestamp. - return time.Time{} - } - return compiled -} diff --git a/operator/bucketcontroller/create.go b/operator/bucketcontroller/create.go index 2ee2487..3ba4c8c 100644 --- a/operator/bucketcontroller/create.go +++ b/operator/bucketcontroller/create.go @@ -24,6 +24,7 @@ func (p *ProvisioningPipeline) Create(ctx context.Context, mg resource.Managed) pipe.WithBeforeHooks(pipelineutil.DebugLogger(pctx)). WithSteps( pipe.NewStep("create bucket", p.createS3Bucket), + pipe.NewStep("set lock", p.setLock), pipe.NewStep("emit event", p.emitCreationEvent), ) err := pipe.RunWithContext(pctx) @@ -54,6 +55,16 @@ func (p *ProvisioningPipeline) createS3Bucket(ctx *pipelineContext) error { return nil } +// setLock sets an annotation that tells the Observe func that we have successfully created the bucket. +// Without it, another resource that has the same bucket name might "adopt" the same bucket, causing 2 resources managing 1 bucket. +func (p *ProvisioningPipeline) setLock(ctx *pipelineContext) error { + if ctx.bucket.Annotations == nil { + ctx.bucket.Annotations = map[string]string{} + } + ctx.bucket.Annotations[lockAnnotation] = "claimed" + return nil +} + func (p *ProvisioningPipeline) emitCreationEvent(ctx *pipelineContext) error { p.recorder.Event(ctx.bucket, event.Event{ Type: event.TypeNormal, diff --git a/operator/bucketcontroller/observe.go b/operator/bucketcontroller/observe.go index 0456cf2..4506eb2 100644 --- a/operator/bucketcontroller/observe.go +++ b/operator/bucketcontroller/observe.go @@ -2,6 +2,7 @@ package bucketcontroller import ( "context" + "fmt" "net/http" xpv1 "github.com/crossplane/crossplane-runtime/apis/common/v1" @@ -12,6 +13,10 @@ import ( controllerruntime "sigs.k8s.io/controller-runtime" ) +var bucketExistsFn = func(ctx context.Context, mc *minio.Client, bucketName string) (bool, error) { + return mc.BucketExists(ctx, bucketName) +} + // Observe implements managed.ExternalClient. func (p *ProvisioningPipeline) Observe(ctx context.Context, mg resource.Managed) (managed.ExternalObservation, error) { log := controllerruntime.LoggerFrom(ctx) @@ -21,7 +26,7 @@ func (p *ProvisioningPipeline) Observe(ctx context.Context, mg resource.Managed) bucket := fromManaged(mg) bucketName := bucket.GetBucketName() - exists, err := s3Client.BucketExists(ctx, bucketName) + exists, err := bucketExistsFn(ctx, s3Client, bucketName) if err != nil { errResp := minio.ToErrorResponse(err) if errResp.StatusCode == http.StatusForbidden { @@ -32,10 +37,12 @@ func (p *ProvisioningPipeline) Observe(ctx context.Context, mg resource.Managed) } return managed.ExternalObservation{}, errors.Wrap(err, "cannot determine whether bucket exists") } - if exists { + if _, hasAnnotation := bucket.Annotations[lockAnnotation]; hasAnnotation && exists { bucket.Status.AtProvider.BucketName = bucketName bucket.SetConditions(xpv1.Available()) return managed.ExternalObservation{ResourceExists: true, ResourceUpToDate: true}, nil + } else if exists { + return managed.ExternalObservation{}, fmt.Errorf("bucket exists already, try changing bucket name: %s", bucketName) } return managed.ExternalObservation{}, nil } diff --git a/operator/bucketcontroller/observe_test.go b/operator/bucketcontroller/observe_test.go new file mode 100644 index 0000000..c8cd743 --- /dev/null +++ b/operator/bucketcontroller/observe_test.go @@ -0,0 +1,104 @@ +package bucketcontroller + +import ( + "context" + "net/http" + "testing" + + "github.com/crossplane/crossplane-runtime/pkg/errors" + "github.com/crossplane/crossplane-runtime/pkg/reconciler/managed" + "github.com/go-logr/logr" + "github.com/minio/minio-go/v7" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + cloudscalev1 "github.com/vshn/provider-cloudscale/apis/cloudscale/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestProvisioningPipeline_Observe(t *testing.T) { + tests := map[string]struct { + givenBucket *cloudscalev1.Bucket + bucketExists bool + returnError error + + expectedError string + expectedResult managed.ExternalObservation + expectedBucketObservation cloudscalev1.BucketObservation + }{ + "NewBucketDoesntYetExistOnCloudscale": { + givenBucket: &cloudscalev1.Bucket{Spec: cloudscalev1.BucketSpec{ForProvider: cloudscalev1.BucketParameters{ + BucketName: "my-bucket"}}, + }, + expectedResult: managed.ExternalObservation{}, + }, + "BucketExistsAndAccessibleWithOurCredentials": { + givenBucket: &cloudscalev1.Bucket{ + ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{ + lockAnnotation: "claimed", + }}, + Spec: cloudscalev1.BucketSpec{ForProvider: cloudscalev1.BucketParameters{ + BucketName: "my-bucket"}}, + }, + bucketExists: true, + expectedResult: managed.ExternalObservation{ResourceExists: true, ResourceUpToDate: true}, + expectedBucketObservation: cloudscalev1.BucketObservation{BucketName: "my-bucket"}, + }, + "NewBucketObservationThrowsGenericError": { + givenBucket: &cloudscalev1.Bucket{Spec: cloudscalev1.BucketSpec{ForProvider: cloudscalev1.BucketParameters{ + BucketName: "my-bucket"}}, + }, + returnError: errors.New("error"), + expectedResult: managed.ExternalObservation{}, + expectedError: "cannot determine whether bucket exists: error", + }, + "BucketAlreadyExistsOnCloudscale_WithoutAccess": { + givenBucket: &cloudscalev1.Bucket{Spec: cloudscalev1.BucketSpec{ForProvider: cloudscalev1.BucketParameters{ + BucketName: "my-bucket"}}, + }, + returnError: minio.ErrorResponse{StatusCode: http.StatusForbidden, Message: "Access Denied"}, + expectedResult: managed.ExternalObservation{}, + expectedError: "wrong credentials or bucket exists already, try changing bucket name: Access Denied", + }, + "BucketAlreadyExistsOnCloudscale_WithAccess_PreventAdoption": { + // this is a case where we should avoid adopting an existing bucket even if we have access. + // Otherwise, there could be multiple K8s resources that manage the same bucket. + givenBucket: &cloudscalev1.Bucket{ + Spec: cloudscalev1.BucketSpec{ForProvider: cloudscalev1.BucketParameters{ + BucketName: "my-bucket"}}, + // no bucket name in status here. + }, + bucketExists: true, + expectedResult: managed.ExternalObservation{}, + expectedError: "bucket exists already, try changing bucket name: my-bucket", + }, + "BucketAlreadyExistsOnCloudscale_InAnotherZone": { + givenBucket: &cloudscalev1.Bucket{ + Spec: cloudscalev1.BucketSpec{ForProvider: cloudscalev1.BucketParameters{ + BucketName: "my-bucket"}}, + }, + returnError: minio.ErrorResponse{StatusCode: http.StatusMovedPermanently, Message: "301 Moved Permanently"}, + expectedResult: managed.ExternalObservation{}, + expectedError: "mismatching endpointURL and region, or bucket exists already in a different region, try changing bucket name: 301 Moved Permanently", + }, + } + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + currFn := bucketExistsFn + defer func() { + bucketExistsFn = currFn + }() + bucketExistsFn = func(ctx context.Context, mc *minio.Client, bucketName string) (bool, error) { + return tc.bucketExists, tc.returnError + } + p := ProvisioningPipeline{} + result, err := p.Observe(logr.NewContext(context.Background(), logr.Discard()), tc.givenBucket) + if tc.expectedError != "" { + assert.EqualError(t, err, tc.expectedError) + return + } + require.NoError(t, err) + assert.Equal(t, tc.expectedResult, result) + assert.Equal(t, tc.expectedBucketObservation, tc.givenBucket.Status.AtProvider) + }) + } +} diff --git a/operator/bucketcontroller/pipeline.go b/operator/bucketcontroller/pipeline.go index 83c290e..76c578e 100644 --- a/operator/bucketcontroller/pipeline.go +++ b/operator/bucketcontroller/pipeline.go @@ -35,3 +35,5 @@ func NewProvisioningPipeline(kube client.Client, recorder event.Recorder, minio func fromManaged(mg resource.Managed) *cloudscalev1.Bucket { return mg.(*cloudscalev1.Bucket) } + +const lockAnnotation = cloudscalev1.Group + "/lock" diff --git a/test/controllerconfig-cloudscale.yaml b/test/controllerconfig-cloudscale.yaml index ec9ed64..a515296 100644 --- a/test/controllerconfig-cloudscale.yaml +++ b/test/controllerconfig-cloudscale.yaml @@ -5,3 +5,6 @@ metadata: name: providerconfig-cloudscale spec: imagePullPolicy: IfNotPresent + env: + - name: LOG_LEVEL + value: "1" diff --git a/test/e2e/provider/00-assert.yaml b/test/e2e/provider/00-assert.yaml index 0659453..1edd81c 100644 --- a/test/e2e/provider/00-assert.yaml +++ b/test/e2e/provider/00-assert.yaml @@ -5,8 +5,7 @@ kind: TestAssert apiVersion: cloudscale.crossplane.io/v1 kind: ObjectsUser metadata: - annotations: - kuttl: '00-install' + name: e2e-test-objectsuser spec: deletionPolicy: Delete forProvider: diff --git a/test/e2e/provider/00-install-user.yaml b/test/e2e/provider/00-install-user.yaml index d468f43..cddcb50 100644 --- a/test/e2e/provider/00-install-user.yaml +++ b/test/e2e/provider/00-install-user.yaml @@ -2,8 +2,6 @@ apiVersion: cloudscale.crossplane.io/v1 kind: ObjectsUser metadata: - annotations: - kuttl: '00-install' name: e2e-test-objectsuser spec: forProvider: diff --git a/test/e2e/provider/01-assert.yaml b/test/e2e/provider/01-assert.yaml index 9cd47da..ddb9158 100644 --- a/test/e2e/provider/01-assert.yaml +++ b/test/e2e/provider/01-assert.yaml @@ -6,7 +6,8 @@ apiVersion: cloudscale.crossplane.io/v1 kind: Bucket metadata: annotations: - kuttl: '01-install' + cloudscale.crossplane.io/lock: claimed + name: e2e-test-bucket spec: deletionPolicy: Delete forProvider: diff --git a/test/e2e/provider/01-install-bucket.yaml b/test/e2e/provider/01-install-bucket.yaml index e04175b..a9e5039 100644 --- a/test/e2e/provider/01-install-bucket.yaml +++ b/test/e2e/provider/01-install-bucket.yaml @@ -3,7 +3,7 @@ apiVersion: cloudscale.crossplane.io/v1 kind: Bucket metadata: annotations: - kuttl: '01-install' + cloudscale.crossplane.io/lock: claimed name: e2e-test-bucket spec: forProvider: