Merge pull request #45 from vshn/adoption

Prevent adoption of existing buckets

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
-}

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,

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
 }

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)
+		})
+	}
+}

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"

test/controllerconfig-cloudscale.yaml

@@ -5,3 +5,6 @@ metadata:
   name: providerconfig-cloudscale
 spec:
   imagePullPolicy: IfNotPresent
+  env:
+    - name: LOG_LEVEL
+      value: "1"

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:

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:

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:

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: