From d1916ce1788dc4eb85c7d204d2d3622a4947c13b Mon Sep 17 00:00:00 2001 From: Hamed Karbasi Date: Sat, 21 Oct 2023 17:29:40 +0330 Subject: [PATCH 01/36] fix assert step in step 6 --- testing/e2e/04-validation-webhook.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testing/e2e/04-validation-webhook.yaml b/testing/e2e/04-validation-webhook.yaml index 1f9e2ac..a20e07e 100644 --- a/testing/e2e/04-validation-webhook.yaml +++ b/testing/e2e/04-validation-webhook.yaml @@ -9,6 +9,6 @@ commands: ignoreFailure: true assert: - s3bucket-ok.yaml - - 01-assert.yaml + - 02-assert.yaml error: - s3bucket-wrong-s3userref.yaml From cf0cffb0dd4e10b87690de4a3c2ae57cbbd5a720 Mon Sep 17 00:00:00 2001 From: Hamed Karbasi Date: Mon, 23 Oct 2023 14:48:34 +0330 Subject: [PATCH 02/36] add subUsers creation and remove --- api/v1alpha1/s3userclaim_types.go | 6 ++++ api/v1alpha1/zz_generated.deepcopy.go | 10 ++++++ .../bases/s3.snappcloud.io_s3userclaims.yaml | 8 +++++ .../controllers/s3userclaim/provisioner.go | 35 +++++++++++++++++++ 4 files changed, 59 insertions(+) diff --git a/api/v1alpha1/s3userclaim_types.go b/api/v1alpha1/s3userclaim_types.go index ea00f0b..4ddccc3 100644 --- a/api/v1alpha1/s3userclaim_types.go +++ b/api/v1alpha1/s3userclaim_types.go @@ -34,6 +34,9 @@ type S3UserClaimSpec struct { // +kubebuilder:validation:Optional // +kubebuilder:default:={"maxSize":"5368709120", "maxObjects":"1000", "maxBuckets": 2} Quota *UserQuota `json:"quota,omitempty"` + + // +kubebuilder:validation:Optional + SubUsers []string `json:"subUsers,omitempty"` } // S3UserClaimStatus defines the observed state of S3UserClaim @@ -43,6 +46,9 @@ type S3UserClaimStatus struct { // +kubebuilder:validation:Optional S3UserName string `json:"s3UserName,omitempty"` + + // +kubebuilder:validation:Optional + SubUsers []string `json:"subUsers,omitempty"` } // +kubebuilder:object:root=true diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 1a0ef88..082cf70 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -209,6 +209,11 @@ func (in *S3UserClaimSpec) DeepCopyInto(out *S3UserClaimSpec) { *out = new(UserQuota) (*in).DeepCopyInto(*out) } + if in.SubUsers != nil { + in, out := &in.SubUsers, &out.SubUsers + *out = make([]string, len(*in)) + copy(*out, *in) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new S3UserClaimSpec. @@ -229,6 +234,11 @@ func (in *S3UserClaimStatus) DeepCopyInto(out *S3UserClaimStatus) { *out = new(UserQuota) (*in).DeepCopyInto(*out) } + if in.SubUsers != nil { + in, out := &in.SubUsers, &out.SubUsers + *out = make([]string, len(*in)) + copy(*out, *in) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new S3UserClaimStatus. diff --git a/config/crd/bases/s3.snappcloud.io_s3userclaims.yaml b/config/crd/bases/s3.snappcloud.io_s3userclaims.yaml index bdcb9bc..d1a14fc 100644 --- a/config/crd/bases/s3.snappcloud.io_s3userclaims.yaml +++ b/config/crd/bases/s3.snappcloud.io_s3userclaims.yaml @@ -86,6 +86,10 @@ spec: type: string s3UserClass: type: string + subUsers: + items: + type: string + type: array required: - adminSecret - readonlySecret @@ -116,6 +120,10 @@ spec: type: object s3UserName: type: string + subUsers: + items: + type: string + type: array type: object type: object served: true diff --git a/internal/controllers/s3userclaim/provisioner.go b/internal/controllers/s3userclaim/provisioner.go index 80e71e0..fd68e70 100644 --- a/internal/controllers/s3userclaim/provisioner.go +++ b/internal/controllers/s3userclaim/provisioner.go @@ -30,6 +30,7 @@ func (r *Reconciler) Provision(ctx context.Context) (ctrl.Result, error) { r.ensureCephUser, r.ensureCephUserQuota, r.ensureReadonlySubuser, + r.ensureOtherSubusers, // retrieve the ceph user to have keys of subuser at hand r.retrieveCephUser, r.ensureAdminSecret, @@ -131,6 +132,40 @@ func (r *Reconciler) ensureReadonlySubuser(ctx context.Context) (*ctrl.Result, e return subreconciler.ContinueReconciling() } +func (r *Reconciler) ensureOtherSubusers(ctx context.Context) (*ctrl.Result, error) { + subUsers := r.s3UserClaim.Spec.SubUsers + // Create a hashmap for subUsers available in the instance spec + subUserSet := make(map[string]bool) + // Create subUsers and continue if they are existed + for _, subUser := range subUsers { + subUserSet[subUser] = true + desiredSubuser := admin.SubuserSpec{ + Name: subUser, + Access: admin.SubuserAccessNone, + KeyType: pointer.String(consts.CephKeyTypeS3), + } + switch err := r.rgwClient.CreateSubuser(ctx, admin.User{ID: r.cephUserFullId}, desiredSubuser); { + case err.Error() == admin.ErrSubuserExists.Error(): + r.logger.Info(fmt.Sprintf("Subuser '%s' exists for User %s", subUser, r.cephUserFullId)) + case err != nil: + r.logger.Error(err, "failed to create subUser") + return subreconciler.Requeue() + } + } + + // Remove subUsers which are not available in the spec but are in the ceph subUsers. + for _, cephSubUser := range r.cephUser.Subusers { + if !subUserSet[cephSubUser.Name] { + err := r.rgwClient.RemoveSubuser(ctx, admin.User{ID: r.cephUserFullId}, cephSubUser) + if err != nil { + r.logger.Error(err, "Failed to remove subUser") + return subreconciler.Requeue() + } + } + } + + return subreconciler.ContinueReconciling() +} func (r *Reconciler) retrieveCephUser(ctx context.Context) (*ctrl.Result, error) { retrievedUser, err := r.rgwClient.GetUser(ctx, admin.User{ID: r.cephUserFullId}) if err != nil { From 5113b108e1db88b6ace1d755f3c3558021aad844 Mon Sep 17 00:00:00 2001 From: Hamed Karbasi Date: Mon, 23 Oct 2023 14:48:55 +0330 Subject: [PATCH 03/36] add teststep for adding subUsers to e2e --- testing/e2e/02-assert.yaml | 32 +++++++++++++++++++++++++++- testing/e2e/02-create-userclaim.yaml | 4 +++- 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/testing/e2e/02-assert.yaml b/testing/e2e/02-assert.yaml index 89a7b95..6638822 100644 --- a/testing/e2e/02-assert.yaml +++ b/testing/e2e/02-assert.yaml @@ -10,7 +10,9 @@ status: maxSize: 5k maxObjects: "500" maxBuckets: 5 - + subUsers: + - subUser1 + - subUser2 --- apiVersion: s3.snappcloud.io/v1alpha1 @@ -56,7 +58,35 @@ metadata: name: s3userclaim-sample type: Opaque +# SubUser1 +--- +apiVersion: v1 +kind: Secret +metadata: + name: s3userclaim-sample-subUser1 + namespace: s3-test + ownerReferences: + - apiVersion: s3.snappcloud.io/v1alpha1 + blockOwnerDeletion: true + controller: true + kind: S3UserClaim + name: s3userclaim-sample +type: Opaque +# SubUser2 +--- +apiVersion: v1 +kind: Secret +metadata: + name: s3userclaim-sample-subUser2 + namespace: s3-test + ownerReferences: + - apiVersion: s3.snappcloud.io/v1alpha1 + blockOwnerDeletion: true + controller: true + kind: S3UserClaim + name: s3userclaim-sample +type: Opaque # Extra user --- diff --git a/testing/e2e/02-create-userclaim.yaml b/testing/e2e/02-create-userclaim.yaml index 0a389f7..62055a3 100644 --- a/testing/e2e/02-create-userclaim.yaml +++ b/testing/e2e/02-create-userclaim.yaml @@ -12,7 +12,9 @@ spec: maxSize: 5000 maxObjects: 500 maxBuckets: 5 - + subUsers: + - subUser1 + - subUser2 --- # Extra user apiVersion: s3.snappcloud.io/v1alpha1 From 00882004e32a66138d19c47e83207dafe1809ac4 Mon Sep 17 00:00:00 2001 From: Hamed Karbasi Date: Mon, 23 Oct 2023 17:28:45 +0330 Subject: [PATCH 04/36] add ensure subUsersSecret and fix subUser recreation --- .../controllers/s3userclaim/provisioner.go | 65 ++++++++++++++----- pkg/consts/consts.go | 3 + 2 files changed, 50 insertions(+), 18 deletions(-) diff --git a/internal/controllers/s3userclaim/provisioner.go b/internal/controllers/s3userclaim/provisioner.go index fd68e70..be6b97b 100644 --- a/internal/controllers/s3userclaim/provisioner.go +++ b/internal/controllers/s3userclaim/provisioner.go @@ -35,6 +35,7 @@ func (r *Reconciler) Provision(ctx context.Context) (ctrl.Result, error) { r.retrieveCephUser, r.ensureAdminSecret, r.ensureReadonlySecret, + r.ensureOtherSubusersSecret, r.ensureS3User, r.updateS3UserClaimStatus, r.addCleanupFinalizer, @@ -133,30 +134,41 @@ func (r *Reconciler) ensureReadonlySubuser(ctx context.Context) (*ctrl.Result, e } func (r *Reconciler) ensureOtherSubusers(ctx context.Context) (*ctrl.Result, error) { - subUsers := r.s3UserClaim.Spec.SubUsers - // Create a hashmap for subUsers available in the instance spec - subUserSet := make(map[string]bool) - // Create subUsers and continue if they are existed - for _, subUser := range subUsers { - subUserSet[subUser] = true + specSubUsers := r.s3UserClaim.Spec.SubUsers + // Create a hashmap to move all spec and ceph subUsers to it + subUserSet := make(map[string]string) + + // Tag specSubUsers with "create" + for _, subUser := range specSubUsers { + subUserSet[subUser] = consts.SubUserTagCreate + } + + // Tag cephSubUsers as remove if they are not already in the hashmap and remove them otherwise + // since they are already available on ceph and not needed to created. + for _, subUsersSpec := range r.cephUser.Subusers { + _, exists := subUserSet[subUsersSpec.Name] + if exists { + delete(subUserSet, subUsersSpec.Name) + } else { + subUserSet[subUsersSpec.Name] = consts.SubUserTagRemove + } + } + + // Iterate over the subUsers hashmap and create or remove subUsers according to their tags. + for subUser, tag := range subUserSet { desiredSubuser := admin.SubuserSpec{ Name: subUser, Access: admin.SubuserAccessNone, KeyType: pointer.String(consts.CephKeyTypeS3), } - switch err := r.rgwClient.CreateSubuser(ctx, admin.User{ID: r.cephUserFullId}, desiredSubuser); { - case err.Error() == admin.ErrSubuserExists.Error(): - r.logger.Info(fmt.Sprintf("Subuser '%s' exists for User %s", subUser, r.cephUserFullId)) - case err != nil: - r.logger.Error(err, "failed to create subUser") - return subreconciler.Requeue() - } - } + if tag == consts.SubUserTagCreate { - // Remove subUsers which are not available in the spec but are in the ceph subUsers. - for _, cephSubUser := range r.cephUser.Subusers { - if !subUserSet[cephSubUser.Name] { - err := r.rgwClient.RemoveSubuser(ctx, admin.User{ID: r.cephUserFullId}, cephSubUser) + if err := r.rgwClient.CreateSubuser(ctx, admin.User{ID: r.cephUserFullId}, desiredSubuser); err != nil { + r.logger.Error(err, "failed to create subUser") + return subreconciler.Requeue() + } + } else { + err := r.rgwClient.RemoveSubuser(ctx, admin.User{ID: r.cephUserFullId}, desiredSubuser) if err != nil { r.logger.Error(err, "Failed to remove subUser") return subreconciler.Requeue() @@ -195,6 +207,23 @@ func (r *Reconciler) ensureReadonlySecret(ctx context.Context) (*ctrl.Result, er return r.ensureSecret(ctx, assembledSecret) } +func (r *Reconciler) ensureOtherSubusersSecret(ctx context.Context) (*ctrl.Result, error) { + for _, subUser := range r.s3UserClaim.Spec.SubUsers { + cephSubUserFullId := fmt.Sprintf("%s:%s", r.cephUserFullId, subUser) + SubUserSecretName := fmt.Sprintf("%s-%s", r.s3UserClaim.Name, subUser) + assembledSecret, err := r.assembleCephUserSecret(cephSubUserFullId, SubUserSecretName) + if err != nil { + r.logger.Error(err, "failed to assemble readonly secret") + return subreconciler.Requeue() + } + result, err := r.ensureSecret(ctx, assembledSecret) + if result != nil || err != nil { + return result, err + } + } + return subreconciler.ContinueReconciling() +} + func (r *Reconciler) ensureS3User(ctx context.Context) (*ctrl.Result, error) { existingS3User := &s3v1alpha1.S3User{} diff --git a/pkg/consts/consts.go b/pkg/consts/consts.go index 839076d..764e805 100644 --- a/pkg/consts/consts.go +++ b/pkg/consts/consts.go @@ -34,4 +34,7 @@ const ( DeletionPolicyDelete = "delete" DeletionPolicyRetain = "retain" + + SubUserTagCreate = "create" + SubUserTagRemove = "remove" ) From b248d4e16cd865c0d2dc333c9a078728bbee578d Mon Sep 17 00:00:00 2001 From: Hamed Karbasi Date: Mon, 23 Oct 2023 17:30:02 +0330 Subject: [PATCH 05/36] add subUsers to status --- internal/controllers/s3userclaim/provisioner.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/controllers/s3userclaim/provisioner.go b/internal/controllers/s3userclaim/provisioner.go index be6b97b..0123599 100644 --- a/internal/controllers/s3userclaim/provisioner.go +++ b/internal/controllers/s3userclaim/provisioner.go @@ -263,6 +263,7 @@ func (r *Reconciler) updateS3UserClaimStatus(ctx context.Context) (*ctrl.Result, status := s3v1alpha1.S3UserClaimStatus{ Quota: r.s3UserClaim.Spec.Quota, S3UserName: r.s3UserName, + SubUsers: r.s3UserClaim.Spec.SubUsers, } if !apiequality.Semantic.DeepEqual(r.s3UserClaim.Status, status) { From de5251bb47087d34167166fbd0f8e23e786685c4 Mon Sep 17 00:00:00 2001 From: Hamed Karbasi Date: Mon, 23 Oct 2023 18:28:22 +0330 Subject: [PATCH 06/36] fix subUser spell --- testing/e2e/02-create-userclaim.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/testing/e2e/02-create-userclaim.yaml b/testing/e2e/02-create-userclaim.yaml index 62055a3..fe543a0 100644 --- a/testing/e2e/02-create-userclaim.yaml +++ b/testing/e2e/02-create-userclaim.yaml @@ -13,8 +13,8 @@ spec: maxObjects: 500 maxBuckets: 5 subUsers: - - subUser1 - - subUser2 + - subuser1 + - subuser2 --- # Extra user apiVersion: s3.snappcloud.io/v1alpha1 From 5c2c252065264da664bb1f560f1ef2ed3f6e146d Mon Sep 17 00:00:00 2001 From: Hamed Karbasi Date: Mon, 23 Oct 2023 22:26:42 +0330 Subject: [PATCH 07/36] fix create subuser bug --- internal/controllers/s3userclaim/provisioner.go | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/internal/controllers/s3userclaim/provisioner.go b/internal/controllers/s3userclaim/provisioner.go index 0123599..cd55d99 100644 --- a/internal/controllers/s3userclaim/provisioner.go +++ b/internal/controllers/s3userclaim/provisioner.go @@ -140,17 +140,21 @@ func (r *Reconciler) ensureOtherSubusers(ctx context.Context) (*ctrl.Result, err // Tag specSubUsers with "create" for _, subUser := range specSubUsers { - subUserSet[subUser] = consts.SubUserTagCreate + cephSubUserFullId := fmt.Sprintf("%s:%s", r.cephUserFullId, subUser) + subUserSet[cephSubUserFullId] = consts.SubUserTagCreate } + // Add read-only subUser to subUsers to prevent if from removing + subUserSet[r.readonlyCephUserFullId] = consts.SubUserTagCreate + // Tag cephSubUsers as remove if they are not already in the hashmap and remove them otherwise // since they are already available on ceph and not needed to created. - for _, subUsersSpec := range r.cephUser.Subusers { - _, exists := subUserSet[subUsersSpec.Name] + for _, cephsubUser := range r.cephUser.Subusers { + _, exists := subUserSet[cephsubUser.Name] if exists { - delete(subUserSet, subUsersSpec.Name) + delete(subUserSet, cephsubUser.Name) } else { - subUserSet[subUsersSpec.Name] = consts.SubUserTagRemove + subUserSet[cephsubUser.Name] = consts.SubUserTagRemove } } @@ -162,13 +166,14 @@ func (r *Reconciler) ensureOtherSubusers(ctx context.Context) (*ctrl.Result, err KeyType: pointer.String(consts.CephKeyTypeS3), } if tag == consts.SubUserTagCreate { - + r.logger.Info(fmt.Sprintf("Create subUser: %s", subUser)) if err := r.rgwClient.CreateSubuser(ctx, admin.User{ID: r.cephUserFullId}, desiredSubuser); err != nil { r.logger.Error(err, "failed to create subUser") return subreconciler.Requeue() } } else { err := r.rgwClient.RemoveSubuser(ctx, admin.User{ID: r.cephUserFullId}, desiredSubuser) + r.logger.Info(fmt.Sprintf("Remove subUser: %s", subUser)) if err != nil { r.logger.Error(err, "Failed to remove subUser") return subreconciler.Requeue() From 333de3ee54c78bbfce7a6f440d826bab4afe9b88 Mon Sep 17 00:00:00 2001 From: Hamed Karbasi Date: Sat, 28 Oct 2023 14:41:03 +0330 Subject: [PATCH 08/36] add initSpecBasedVars --- internal/controllers/s3userclaim/handler.go | 14 +++++++++++++- internal/controllers/s3userclaim/provisioner.go | 6 +++--- testing/e2e/02-assert.yaml | 8 ++++---- 3 files changed, 20 insertions(+), 8 deletions(-) diff --git a/internal/controllers/s3userclaim/handler.go b/internal/controllers/s3userclaim/handler.go index e80f762..3c7f30f 100644 --- a/internal/controllers/s3userclaim/handler.go +++ b/internal/controllers/s3userclaim/handler.go @@ -50,6 +50,7 @@ type Reconciler struct { cephTenant string cephUserId string cephUserFullId string + cephSubUserFullIdMap map[string][]string cephDisplayName string s3UserName string readonlyCephUserId string @@ -104,7 +105,8 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu return r.Cleanup(ctx) } } - + // Initialize the spec-based variables which need the reconciler to already have fetched the CRD + r.initSpecBasedVars() return r.Provision(ctx) } @@ -129,3 +131,13 @@ func (r *Reconciler) initVars(req ctrl.Request) { r.s3UserName = fmt.Sprintf("%s.%s", req.Namespace, req.Name) } + +func (r *Reconciler) initSpecBasedVars() { + // init subUser CephSubUserFullIDs and corresponding secrets + for _, subUser := range r.s3UserClaim.Spec.SubUsers { + // First item is the cephSubUserFullId + r.cephSubUserFullIdMap[subUser][0] = fmt.Sprintf("%s:%s", r.cephUserFullId, subUser) + // Second item is the SubUserSecretName + r.cephSubUserFullIdMap[subUser][1] = fmt.Sprintf("%s-%s", r.s3UserClaim.Name, subUser) + } +} diff --git a/internal/controllers/s3userclaim/provisioner.go b/internal/controllers/s3userclaim/provisioner.go index cd55d99..36bb2d5 100644 --- a/internal/controllers/s3userclaim/provisioner.go +++ b/internal/controllers/s3userclaim/provisioner.go @@ -140,7 +140,7 @@ func (r *Reconciler) ensureOtherSubusers(ctx context.Context) (*ctrl.Result, err // Tag specSubUsers with "create" for _, subUser := range specSubUsers { - cephSubUserFullId := fmt.Sprintf("%s:%s", r.cephUserFullId, subUser) + cephSubUserFullId := r.cephSubUserFullIdMap[subUser][0] subUserSet[cephSubUserFullId] = consts.SubUserTagCreate } @@ -214,8 +214,8 @@ func (r *Reconciler) ensureReadonlySecret(ctx context.Context) (*ctrl.Result, er func (r *Reconciler) ensureOtherSubusersSecret(ctx context.Context) (*ctrl.Result, error) { for _, subUser := range r.s3UserClaim.Spec.SubUsers { - cephSubUserFullId := fmt.Sprintf("%s:%s", r.cephUserFullId, subUser) - SubUserSecretName := fmt.Sprintf("%s-%s", r.s3UserClaim.Name, subUser) + cephSubUserFullId := r.cephSubUserFullIdMap[subUser][0] + SubUserSecretName := r.cephSubUserFullIdMap[subUser][1] assembledSecret, err := r.assembleCephUserSecret(cephSubUserFullId, SubUserSecretName) if err != nil { r.logger.Error(err, "failed to assemble readonly secret") diff --git a/testing/e2e/02-assert.yaml b/testing/e2e/02-assert.yaml index 6638822..e60ad2b 100644 --- a/testing/e2e/02-assert.yaml +++ b/testing/e2e/02-assert.yaml @@ -11,8 +11,8 @@ status: maxObjects: "500" maxBuckets: 5 subUsers: - - subUser1 - - subUser2 + - subuser1 + - subuser2 --- apiVersion: s3.snappcloud.io/v1alpha1 @@ -63,7 +63,7 @@ type: Opaque apiVersion: v1 kind: Secret metadata: - name: s3userclaim-sample-subUser1 + name: s3userclaim-sample-subuser1 namespace: s3-test ownerReferences: - apiVersion: s3.snappcloud.io/v1alpha1 @@ -78,7 +78,7 @@ type: Opaque apiVersion: v1 kind: Secret metadata: - name: s3userclaim-sample-subUser2 + name: s3userclaim-sample-subuser2 namespace: s3-test ownerReferences: - apiVersion: s3.snappcloud.io/v1alpha1 From 6fc674c604a3b23158366ea9cbf577aba27178a6 Mon Sep 17 00:00:00 2001 From: Hamed Karbasi Date: Sat, 28 Oct 2023 17:17:28 +0330 Subject: [PATCH 09/36] complete test step for subuser creation and deletion --- testing/e2e/03-assert.yaml | 2 +- testing/e2e/05-assert.yaml | 16 +++++++++++----- testing/e2e/05-create-subuser.yaml | 16 ++++++++++++++++ testing/e2e/06-assert.yaml | 6 ++++++ testing/e2e/06-delete-subuser.yaml | 15 +++++++++++++++ testing/e2e/06-errors.yaml | 14 ++++---------- testing/e2e/07-assert.yaml | 9 +++++++++ ...lete-bucket.yaml => 07-delete-bucket.yaml} | 0 testing/e2e/07-errors.yaml | 11 +++-------- ...serclaim.yaml => 08-delete-userclaim.yaml} | 0 .../e2e/{05-errors.yaml => 08-errors.yaml} | 10 ++++------ ....yaml => 09-delete-extra-user-bucket.yaml} | 0 testing/e2e/09-errors.yaml | 19 +++++++++++++++++++ testing/e2e/set-aws-secret.sh | 13 ++++++++++--- 14 files changed, 98 insertions(+), 33 deletions(-) create mode 100644 testing/e2e/05-create-subuser.yaml create mode 100644 testing/e2e/06-assert.yaml create mode 100644 testing/e2e/06-delete-subuser.yaml create mode 100644 testing/e2e/07-assert.yaml rename testing/e2e/{05-delete-bucket.yaml => 07-delete-bucket.yaml} (100%) rename testing/e2e/{06-delete-userclaim.yaml => 08-delete-userclaim.yaml} (100%) rename testing/e2e/{05-errors.yaml => 08-errors.yaml} (51%) rename testing/e2e/{07-delete-extra-user-bucket.yaml => 09-delete-extra-user-bucket.yaml} (100%) create mode 100644 testing/e2e/09-errors.yaml diff --git a/testing/e2e/03-assert.yaml b/testing/e2e/03-assert.yaml index bbdc45c..ebecb56 100644 --- a/testing/e2e/03-assert.yaml +++ b/testing/e2e/03-assert.yaml @@ -24,7 +24,7 @@ kind: TestAssert timeout: 30 commands: # Configure the aws user credentials using created secret -- command: testing/e2e/set-aws-secret.sh +- command: testing/e2e/set-aws-secret.sh ceph-test s3-sample-admin-secret # Check if the buckets are existed on s3 - script: aws --endpoint-url "http://127.0.0.1:8000" s3api list-buckets --profile ceph-test | grep s3bucket-sample-delete - script: aws --endpoint-url "http://127.0.0.1:8000" s3api list-buckets --profile ceph-test | grep s3bucket-sample-retain diff --git a/testing/e2e/05-assert.yaml b/testing/e2e/05-assert.yaml index 573d84c..0014315 100644 --- a/testing/e2e/05-assert.yaml +++ b/testing/e2e/05-assert.yaml @@ -1,9 +1,15 @@ ---- +# Assert creating the secret +apiVersion: v1 +kind: Secret +metadata: + name: s3userclaim-sample-subuser1 + namespace: s3-test +type: Opaque +--- +# Assert that this user would see the buckets apiVersion: kuttl.dev/v1beta1 kind: TestAssert -timeout: 30 +timeout: 5 commands: - # Check if the buckets are existed on s3 -- script: aws --endpoint-url "http://127.0.0.1:8000" s3api list-buckets --profile ceph-test | grep s3bucket-sample-delete | [ $(wc -c) -eq 0 ] -- script: aws --endpoint-url "http://127.0.0.1:8000" s3api list-buckets --profile ceph-test | grep s3bucket-sample-retain \ No newline at end of file +- command: testing/e2e/set-aws-secret.sh ceph-subuser1 s3userclaim-sample-subuser1 diff --git a/testing/e2e/05-create-subuser.yaml b/testing/e2e/05-create-subuser.yaml new file mode 100644 index 0000000..073f873 --- /dev/null +++ b/testing/e2e/05-create-subuser.yaml @@ -0,0 +1,16 @@ +apiVersion: s3.snappcloud.io/v1alpha1 +kind: S3UserClaim +metadata: + name: s3userclaim-sample + namespace: s3-test +spec: + s3UserClass: ceph-default + readonlySecret: s3-sample-readonly-secret + adminSecret: s3-sample-admin-secret + quota: + maxSize: 5000 + maxObjects: 500 + maxBuckets: 5 + subUsers: + - subuser1 + - subuser2 \ No newline at end of file diff --git a/testing/e2e/06-assert.yaml b/testing/e2e/06-assert.yaml new file mode 100644 index 0000000..f9924dd --- /dev/null +++ b/testing/e2e/06-assert.yaml @@ -0,0 +1,6 @@ +# Assert that this user would see the buckets +apiVersion: kuttl.dev/v1beta1 +kind: TestAssert +timeout: 5 +commands: +- script: aws --endpoint-url "http://127.0.0.1:8000" s3api list-buckets --profile ceph-subuser1 && exit 1 || exit 0 \ No newline at end of file diff --git a/testing/e2e/06-delete-subuser.yaml b/testing/e2e/06-delete-subuser.yaml new file mode 100644 index 0000000..644524f --- /dev/null +++ b/testing/e2e/06-delete-subuser.yaml @@ -0,0 +1,15 @@ +apiVersion: s3.snappcloud.io/v1alpha1 +kind: S3UserClaim +metadata: + name: s3userclaim-sample + namespace: s3-test +spec: + s3UserClass: ceph-default + readonlySecret: s3-sample-readonly-secret + adminSecret: s3-sample-admin-secret + quota: + maxSize: 5000 + maxObjects: 500 + maxBuckets: 5 + subUsers: + - subuser2 \ No newline at end of file diff --git a/testing/e2e/06-errors.yaml b/testing/e2e/06-errors.yaml index 2bf2bec..95848f4 100644 --- a/testing/e2e/06-errors.yaml +++ b/testing/e2e/06-errors.yaml @@ -1,12 +1,6 @@ -apiVersion: s3.snappcloud.io/v1alpha1 -kind: S3UserClaim +apiVersion: v1 +kind: Secret metadata: - name: s3userclaim-sample + name: s3userclaim-sample-subuser1 namespace: s3-test - ---- -apiVersion: s3.snappcloud.io/v1alpha1 -kind: S3User -metadata: - name: s3-test.s3userclaim-sample - +type: Opaque \ No newline at end of file diff --git a/testing/e2e/07-assert.yaml b/testing/e2e/07-assert.yaml new file mode 100644 index 0000000..573d84c --- /dev/null +++ b/testing/e2e/07-assert.yaml @@ -0,0 +1,9 @@ +--- + +apiVersion: kuttl.dev/v1beta1 +kind: TestAssert +timeout: 30 +commands: + # Check if the buckets are existed on s3 +- script: aws --endpoint-url "http://127.0.0.1:8000" s3api list-buckets --profile ceph-test | grep s3bucket-sample-delete | [ $(wc -c) -eq 0 ] +- script: aws --endpoint-url "http://127.0.0.1:8000" s3api list-buckets --profile ceph-test | grep s3bucket-sample-retain \ No newline at end of file diff --git a/testing/e2e/05-delete-bucket.yaml b/testing/e2e/07-delete-bucket.yaml similarity index 100% rename from testing/e2e/05-delete-bucket.yaml rename to testing/e2e/07-delete-bucket.yaml diff --git a/testing/e2e/07-errors.yaml b/testing/e2e/07-errors.yaml index f56e822..732765d 100644 --- a/testing/e2e/07-errors.yaml +++ b/testing/e2e/07-errors.yaml @@ -1,19 +1,14 @@ apiVersion: s3.snappcloud.io/v1alpha1 kind: S3Bucket metadata: - name: s3bucket-extra-delete + name: s3bucket-sample-delete namespace: s3-test --- apiVersion: s3.snappcloud.io/v1alpha1 -kind: S3UserClaim +kind: S3Bucket metadata: - name: s3userclaim-extra + name: s3bucket-sample-retain namespace: s3-test ---- -apiVersion: s3.snappcloud.io/v1alpha1 -kind: S3User -metadata: - name: s3-test.s3userclaim-extra \ No newline at end of file diff --git a/testing/e2e/06-delete-userclaim.yaml b/testing/e2e/08-delete-userclaim.yaml similarity index 100% rename from testing/e2e/06-delete-userclaim.yaml rename to testing/e2e/08-delete-userclaim.yaml diff --git a/testing/e2e/05-errors.yaml b/testing/e2e/08-errors.yaml similarity index 51% rename from testing/e2e/05-errors.yaml rename to testing/e2e/08-errors.yaml index 732765d..2bf2bec 100644 --- a/testing/e2e/05-errors.yaml +++ b/testing/e2e/08-errors.yaml @@ -1,14 +1,12 @@ apiVersion: s3.snappcloud.io/v1alpha1 -kind: S3Bucket +kind: S3UserClaim metadata: - name: s3bucket-sample-delete + name: s3userclaim-sample namespace: s3-test --- - apiVersion: s3.snappcloud.io/v1alpha1 -kind: S3Bucket +kind: S3User metadata: - name: s3bucket-sample-retain - namespace: s3-test + name: s3-test.s3userclaim-sample diff --git a/testing/e2e/07-delete-extra-user-bucket.yaml b/testing/e2e/09-delete-extra-user-bucket.yaml similarity index 100% rename from testing/e2e/07-delete-extra-user-bucket.yaml rename to testing/e2e/09-delete-extra-user-bucket.yaml diff --git a/testing/e2e/09-errors.yaml b/testing/e2e/09-errors.yaml new file mode 100644 index 0000000..f56e822 --- /dev/null +++ b/testing/e2e/09-errors.yaml @@ -0,0 +1,19 @@ +apiVersion: s3.snappcloud.io/v1alpha1 +kind: S3Bucket +metadata: + name: s3bucket-extra-delete + namespace: s3-test + +--- + +apiVersion: s3.snappcloud.io/v1alpha1 +kind: S3UserClaim +metadata: + name: s3userclaim-extra + namespace: s3-test + +--- +apiVersion: s3.snappcloud.io/v1alpha1 +kind: S3User +metadata: + name: s3-test.s3userclaim-extra \ No newline at end of file diff --git a/testing/e2e/set-aws-secret.sh b/testing/e2e/set-aws-secret.sh index 73e3a1a..57eecb6 100755 --- a/testing/e2e/set-aws-secret.sh +++ b/testing/e2e/set-aws-secret.sh @@ -1,8 +1,15 @@ #!/bin/bash # This file gets the s3user credentials from the secret and sets as the aws-cli credentials. -# Replace with your Kubernetes secret name and profile name -SECRET_NAME="s3-sample-admin-secret" -PROFILE_NAME="ceph-test" +# Usage: ./script.sh PROFILE_NAME SECRET_NAME +# Example: ./script.sh ceph-test s3-sample-admin-secret + +if [ "$#" -ne 2 ]; then + echo "Usage: $0 PROFILE_NAME SECRET_NAME" + exit 1 +fi + +PROFILE_NAME="$1" +SECRET_NAME="$2" REGION="us-east-1" # Get the access key and secret key from Kubernetes secret From 2eb728b274cccaa6ab2ad023f166db166c17ab19 Mon Sep 17 00:00:00 2001 From: Hamed Karbasi Date: Sun, 29 Oct 2023 08:55:25 +0330 Subject: [PATCH 10/36] use functions instead of map for generating subuser fullIds --- internal/controllers/s3userclaim/handler.go | 26 +++++---- .../controllers/s3userclaim/provisioner.go | 54 +++++++++++++------ 2 files changed, 55 insertions(+), 25 deletions(-) diff --git a/internal/controllers/s3userclaim/handler.go b/internal/controllers/s3userclaim/handler.go index 3c7f30f..066c816 100644 --- a/internal/controllers/s3userclaim/handler.go +++ b/internal/controllers/s3userclaim/handler.go @@ -20,6 +20,7 @@ import ( "context" "fmt" "regexp" + "strings" "github.com/ceph/go-ceph/rgw/admin" "github.com/go-logr/logr" @@ -50,7 +51,6 @@ type Reconciler struct { cephTenant string cephUserId string cephUserFullId string - cephSubUserFullIdMap map[string][]string cephDisplayName string s3UserName string readonlyCephUserId string @@ -105,8 +105,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu return r.Cleanup(ctx) } } - // Initialize the spec-based variables which need the reconciler to already have fetched the CRD - r.initSpecBasedVars() return r.Provision(ctx) } @@ -132,12 +130,20 @@ func (r *Reconciler) initVars(req ctrl.Request) { r.s3UserName = fmt.Sprintf("%s.%s", req.Namespace, req.Name) } -func (r *Reconciler) initSpecBasedVars() { - // init subUser CephSubUserFullIDs and corresponding secrets - for _, subUser := range r.s3UserClaim.Spec.SubUsers { - // First item is the cephSubUserFullId - r.cephSubUserFullIdMap[subUser][0] = fmt.Sprintf("%s:%s", r.cephUserFullId, subUser) - // Second item is the SubUserSecretName - r.cephSubUserFullIdMap[subUser][1] = fmt.Sprintf("%s-%s", r.s3UserClaim.Name, subUser) +func cephSubUserFullIdMaker(cephUserFullId string, subUser string) string { + return fmt.Sprintf("%s:%s", cephUserFullId, subUser) +} + +func subUserSecretNameMaker(s3UserClaimName string, subUser string) string { + return fmt.Sprintf("%s-%s", s3UserClaimName, subUser) +} + +func subUserNameExtractor(cephSubUserFullId string) (string, error) { + parts := strings.Split(cephSubUserFullId, ":") + if len(parts) == 2 { + // return the last part since it's the subUser name + return parts[1], nil + } else { + return "", fmt.Errorf("cannot parse the cephSubUserFullId=%s", cephSubUserFullId) } } diff --git a/internal/controllers/s3userclaim/provisioner.go b/internal/controllers/s3userclaim/provisioner.go index 36bb2d5..edfa996 100644 --- a/internal/controllers/s3userclaim/provisioner.go +++ b/internal/controllers/s3userclaim/provisioner.go @@ -136,48 +136,72 @@ func (r *Reconciler) ensureReadonlySubuser(ctx context.Context) (*ctrl.Result, e func (r *Reconciler) ensureOtherSubusers(ctx context.Context) (*ctrl.Result, error) { specSubUsers := r.s3UserClaim.Spec.SubUsers // Create a hashmap to move all spec and ceph subUsers to it - subUserSet := make(map[string]string) + subUserFullIdSet := make(map[string]string) // Tag specSubUsers with "create" for _, subUser := range specSubUsers { - cephSubUserFullId := r.cephSubUserFullIdMap[subUser][0] - subUserSet[cephSubUserFullId] = consts.SubUserTagCreate + cephSubUserFullId := cephSubUserFullIdMaker(r.cephUserFullId, subUser) + subUserFullIdSet[cephSubUserFullId] = consts.SubUserTagCreate } // Add read-only subUser to subUsers to prevent if from removing - subUserSet[r.readonlyCephUserFullId] = consts.SubUserTagCreate + subUserFullIdSet[r.readonlyCephUserFullId] = consts.SubUserTagCreate // Tag cephSubUsers as remove if they are not already in the hashmap and remove them otherwise // since they are already available on ceph and not needed to created. for _, cephsubUser := range r.cephUser.Subusers { - _, exists := subUserSet[cephsubUser.Name] + _, exists := subUserFullIdSet[cephsubUser.Name] if exists { - delete(subUserSet, cephsubUser.Name) + delete(subUserFullIdSet, cephsubUser.Name) } else { - subUserSet[cephsubUser.Name] = consts.SubUserTagRemove + subUserFullIdSet[cephsubUser.Name] = consts.SubUserTagRemove } } // Iterate over the subUsers hashmap and create or remove subUsers according to their tags. - for subUser, tag := range subUserSet { + for subUserFullId, tag := range subUserFullIdSet { desiredSubuser := admin.SubuserSpec{ - Name: subUser, + Name: subUserFullId, Access: admin.SubuserAccessNone, KeyType: pointer.String(consts.CephKeyTypeS3), } if tag == consts.SubUserTagCreate { - r.logger.Info(fmt.Sprintf("Create subUser: %s", subUser)) + // Create the subuser + r.logger.Info(fmt.Sprintf("Create subUser: %s", subUserFullId)) if err := r.rgwClient.CreateSubuser(ctx, admin.User{ID: r.cephUserFullId}, desiredSubuser); err != nil { r.logger.Error(err, "failed to create subUser") return subreconciler.Requeue() } } else { + // Delete the subuser err := r.rgwClient.RemoveSubuser(ctx, admin.User{ID: r.cephUserFullId}, desiredSubuser) - r.logger.Info(fmt.Sprintf("Remove subUser: %s", subUser)) + r.logger.Info(fmt.Sprintf("Remove subUser: %s", subUserFullId)) if err != nil { - r.logger.Error(err, "Failed to remove subUser") + r.logger.Error(err, "failed to remove subUser") return subreconciler.Requeue() } + // Extrace subUser name from the subUserFullId + subUser, err := subUserNameExtractor(subUserFullId) + if err != nil { + r.logger.Error(err, "failed to remove s3SubUserSecret") + return subreconciler.Requeue() + } + subUserSecretName := subUserSecretNameMaker(r.s3UserClaim.Name, subUser) + subUserSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: r.s3UserClaim.Namespace, + Name: subUserSecretName, + }, + } + switch err := r.Delete(ctx, subUserSecret); { + case apierrors.IsNotFound(err): + return subreconciler.ContinueReconciling() + case err != nil: + r.logger.Error(err, "failed to remove s3SubUserSecret") + return subreconciler.Requeue() + default: + return subreconciler.ContinueReconciling() + } } } @@ -214,11 +238,11 @@ func (r *Reconciler) ensureReadonlySecret(ctx context.Context) (*ctrl.Result, er func (r *Reconciler) ensureOtherSubusersSecret(ctx context.Context) (*ctrl.Result, error) { for _, subUser := range r.s3UserClaim.Spec.SubUsers { - cephSubUserFullId := r.cephSubUserFullIdMap[subUser][0] - SubUserSecretName := r.cephSubUserFullIdMap[subUser][1] + cephSubUserFullId := cephSubUserFullIdMaker(r.cephUserFullId, subUser) + SubUserSecretName := subUserSecretNameMaker(r.s3UserClaim.Name, subUser) assembledSecret, err := r.assembleCephUserSecret(cephSubUserFullId, SubUserSecretName) if err != nil { - r.logger.Error(err, "failed to assemble readonly secret") + r.logger.Error(err, "failed to assemble other subUsers secret") return subreconciler.Requeue() } result, err := r.ensureSecret(ctx, assembledSecret) From 476d5b4de41e7bd375c32f4607d58bb113b66f27 Mon Sep 17 00:00:00 2001 From: Hamed Karbasi Date: Sun, 29 Oct 2023 09:12:23 +0330 Subject: [PATCH 11/36] add e2e-test-local to Makefile --- Makefile | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index c41d1a5..b183adc 100644 --- a/Makefile +++ b/Makefile @@ -114,8 +114,16 @@ test: manifests generate fmt vet envtest setup-dev-env ## Run tests. .PHONY: e2e-test e2e-test: docker-build # Run e2e tests kubectl kuttl test -##@ Build +.PHONY: e2e-test-local +e2e-test-local: docker-build # Run e2e tests for local development purposes + kind load docker-image $(IMG) + kubectl delete pod -n s3-operator-system -l control-plane=controller-manager + kubectl delete s3b --all -A + kubectl delete s3u --all -A + kubectl kuttl test --start-kind=false + +##@ Build .PHONY: build build: manifests generate fmt vet ## Build manager binary. go build -o bin/manager main.go From 2fd5b13996904126238197bf4a3df70eda09fe64 Mon Sep 17 00:00:00 2001 From: Hamed Karbasi Date: Sun, 29 Oct 2023 09:37:18 +0330 Subject: [PATCH 12/36] remove redundant subusers in testing steps --- testing/e2e/02-assert.yaml | 32 ---------------------------- testing/e2e/02-create-userclaim.yaml | 3 --- testing/e2e/05-assert.yaml | 23 ++++++++++++++++++++ 3 files changed, 23 insertions(+), 35 deletions(-) diff --git a/testing/e2e/02-assert.yaml b/testing/e2e/02-assert.yaml index e60ad2b..bd5fd76 100644 --- a/testing/e2e/02-assert.yaml +++ b/testing/e2e/02-assert.yaml @@ -10,9 +10,6 @@ status: maxSize: 5k maxObjects: "500" maxBuckets: 5 - subUsers: - - subuser1 - - subuser2 --- apiVersion: s3.snappcloud.io/v1alpha1 @@ -58,35 +55,6 @@ metadata: name: s3userclaim-sample type: Opaque -# SubUser1 ---- -apiVersion: v1 -kind: Secret -metadata: - name: s3userclaim-sample-subuser1 - namespace: s3-test - ownerReferences: - - apiVersion: s3.snappcloud.io/v1alpha1 - blockOwnerDeletion: true - controller: true - kind: S3UserClaim - name: s3userclaim-sample -type: Opaque - -# SubUser2 ---- -apiVersion: v1 -kind: Secret -metadata: - name: s3userclaim-sample-subuser2 - namespace: s3-test - ownerReferences: - - apiVersion: s3.snappcloud.io/v1alpha1 - blockOwnerDeletion: true - controller: true - kind: S3UserClaim - name: s3userclaim-sample -type: Opaque # Extra user --- diff --git a/testing/e2e/02-create-userclaim.yaml b/testing/e2e/02-create-userclaim.yaml index fe543a0..f3a7af0 100644 --- a/testing/e2e/02-create-userclaim.yaml +++ b/testing/e2e/02-create-userclaim.yaml @@ -12,9 +12,6 @@ spec: maxSize: 5000 maxObjects: 500 maxBuckets: 5 - subUsers: - - subuser1 - - subuser2 --- # Extra user apiVersion: s3.snappcloud.io/v1alpha1 diff --git a/testing/e2e/05-assert.yaml b/testing/e2e/05-assert.yaml index 0014315..61ccd3b 100644 --- a/testing/e2e/05-assert.yaml +++ b/testing/e2e/05-assert.yaml @@ -6,6 +6,29 @@ metadata: namespace: s3-test type: Opaque +--- +# Assert creating the secret +apiVersion: v1 +kind: Secret +metadata: + name: s3userclaim-sample-subuser2 + namespace: s3-test +type: Opaque +--- +# Assert the status of s3userclaim +apiVersion: s3.snappcloud.io/v1alpha1 +kind: S3UserClaim +metadata: + name: s3userclaim-sample + namespace: s3-test +status: + quota: + maxSize: 5k + maxObjects: "500" + maxBuckets: 5 + subUsers: + - subuser1 + - subuser2 --- # Assert that this user would see the buckets apiVersion: kuttl.dev/v1beta1 From 6323355255896cf3bf916aed4eee2c3e3df7b500 Mon Sep 17 00:00:00 2001 From: Hamed Karbasi Date: Sun, 29 Oct 2023 12:30:32 +0330 Subject: [PATCH 13/36] add subUser validator to s3userclaim webhook --- api/v1alpha1/s3userclaim_webhook.go | 36 ++++++++++++++++++++++++++--- 1 file changed, 33 insertions(+), 3 deletions(-) diff --git a/api/v1alpha1/s3userclaim_webhook.go b/api/v1alpha1/s3userclaim_webhook.go index a23f6de..f101b69 100644 --- a/api/v1alpha1/s3userclaim_webhook.go +++ b/api/v1alpha1/s3userclaim_webhook.go @@ -20,6 +20,7 @@ import ( "context" goerrors "errors" "fmt" + "regexp" "time" openshiftquota "github.com/openshift/api/quota/v1" @@ -70,12 +71,19 @@ func (suc *S3UserClaim) ValidateCreate() error { s3userclaimlog.Info("validate create", "name", suc.Name) // Validate Quota - errorList := validateQuota(suc) - if len(errorList) == 0 { + allErrs := validateQuota(suc) + + // Validate subUsers + subUsersErrorList := validateSubUsers(suc.Spec.SubUsers) + + // Concatinate all errors + allErrs = append(allErrs, subUsersErrorList...) + + if len(allErrs) == 0 { return nil } - return apierrors.NewInvalid(suc.GroupVersionKind().GroupKind(), suc.Name, errorList) + return apierrors.NewInvalid(suc.GroupVersionKind().GroupKind(), suc.Name, allErrs) } func (suc *S3UserClaim) ValidateUpdate(old runtime.Object) error { @@ -99,6 +107,10 @@ func (suc *S3UserClaim) ValidateUpdate(old runtime.Object) error { errorList := validateQuota(suc) allErrs = append(allErrs, errorList...) + // Validate subUsers + subusersErrorList := validateSubUsers(suc.Spec.SubUsers) + + allErrs = append(allErrs, subusersErrorList...) if len(allErrs) == 0 { return nil } @@ -313,3 +325,21 @@ func findTeamNamespaces(ctx context.Context, team string) ([]string, error) { return namespaces, nil } + +func validateSubUsers(subUsers []string) field.ErrorList { + // Since the subUsers name is used in their secret name, they should follow the below + // regex pattern. Otherwise, the request has to be denied. + errorList := field.ErrorList{} + pattern := `^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$` + regex := regexp.MustCompile(pattern) + for _, subUser := range subUsers { + if !regex.MatchString(subUser) { + errorReason := fmt.Sprintf("subuser: %s is invalid. It must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character", subUser) + errorList = append(errorList, field.Forbidden(field.NewPath("spec").Child("subUsers"), errorReason)) + } + } + if len(errorList) == 0 { + return nil + } + return errorList +} From 04d8a31727bce4a3f8c878264c86b8609e6cd2fd Mon Sep 17 00:00:00 2001 From: Hamed Karbasi Date: Sun, 29 Oct 2023 15:03:14 +0330 Subject: [PATCH 14/36] add bucket-policy test step --- ...reate-subuser.yaml => 05-add-subuser.yaml} | 0 testing/e2e/06-add-bucket-access.yaml | 6 ++++++ testing/e2e/06-assert.yaml | 4 +++- testing/e2e/06-errors.yaml | 6 ------ testing/e2e/07-assert.yaml | 9 +++------ ...te-subuser.yaml => 07-delete-subuser.yaml} | 0 testing/e2e/07-errors.yaml | 16 ++++------------ testing/e2e/08-assert.yaml | 9 +++++++++ ...lete-bucket.yaml => 08-delete-bucket.yaml} | 0 testing/e2e/08-errors.yaml | 10 ++++++---- ...serclaim.yaml => 09-delete-userclaim.yaml} | 0 testing/e2e/09-errors.yaml | 13 +++---------- ....yaml => 10-delete-extra-user-bucket.yaml} | 0 testing/e2e/10-errors.yaml | 19 +++++++++++++++++++ testing/e2e/bucket-with-subuser-access.yaml | 11 +++++++++++ 15 files changed, 64 insertions(+), 39 deletions(-) rename testing/e2e/{05-create-subuser.yaml => 05-add-subuser.yaml} (100%) create mode 100644 testing/e2e/06-add-bucket-access.yaml delete mode 100644 testing/e2e/06-errors.yaml rename testing/e2e/{06-delete-subuser.yaml => 07-delete-subuser.yaml} (100%) create mode 100644 testing/e2e/08-assert.yaml rename testing/e2e/{07-delete-bucket.yaml => 08-delete-bucket.yaml} (100%) rename testing/e2e/{08-delete-userclaim.yaml => 09-delete-userclaim.yaml} (100%) rename testing/e2e/{09-delete-extra-user-bucket.yaml => 10-delete-extra-user-bucket.yaml} (100%) create mode 100644 testing/e2e/10-errors.yaml create mode 100644 testing/e2e/bucket-with-subuser-access.yaml diff --git a/testing/e2e/05-create-subuser.yaml b/testing/e2e/05-add-subuser.yaml similarity index 100% rename from testing/e2e/05-create-subuser.yaml rename to testing/e2e/05-add-subuser.yaml diff --git a/testing/e2e/06-add-bucket-access.yaml b/testing/e2e/06-add-bucket-access.yaml new file mode 100644 index 0000000..7d77bb0 --- /dev/null +++ b/testing/e2e/06-add-bucket-access.yaml @@ -0,0 +1,6 @@ +apiVersion: kuttl.dev/v1beta1 +kind: TestStep +commands: + - command: ./set-aws-secret.sh ceph-subuser1 s3userclaim-sample-subuser1 + - command: kubectl apply -f bucket-with-subuser-access.yaml + \ No newline at end of file diff --git a/testing/e2e/06-assert.yaml b/testing/e2e/06-assert.yaml index f9924dd..9b513b8 100644 --- a/testing/e2e/06-assert.yaml +++ b/testing/e2e/06-assert.yaml @@ -3,4 +3,6 @@ apiVersion: kuttl.dev/v1beta1 kind: TestAssert timeout: 5 commands: -- script: aws --endpoint-url "http://127.0.0.1:8000" s3api list-buckets --profile ceph-subuser1 && exit 1 || exit 0 \ No newline at end of file + # Subuser must now be able to get an object +- script: aws s3 --endpoint-url "http://127.0.0.1:8000" ls s3://s3bucket-sample-delete --profile ceph-subuser1 | grep "test-object.txt" + \ No newline at end of file diff --git a/testing/e2e/06-errors.yaml b/testing/e2e/06-errors.yaml deleted file mode 100644 index 95848f4..0000000 --- a/testing/e2e/06-errors.yaml +++ /dev/null @@ -1,6 +0,0 @@ -apiVersion: v1 -kind: Secret -metadata: - name: s3userclaim-sample-subuser1 - namespace: s3-test -type: Opaque \ No newline at end of file diff --git a/testing/e2e/07-assert.yaml b/testing/e2e/07-assert.yaml index 573d84c..f9924dd 100644 --- a/testing/e2e/07-assert.yaml +++ b/testing/e2e/07-assert.yaml @@ -1,9 +1,6 @@ ---- - +# Assert that this user would see the buckets apiVersion: kuttl.dev/v1beta1 kind: TestAssert -timeout: 30 +timeout: 5 commands: - # Check if the buckets are existed on s3 -- script: aws --endpoint-url "http://127.0.0.1:8000" s3api list-buckets --profile ceph-test | grep s3bucket-sample-delete | [ $(wc -c) -eq 0 ] -- script: aws --endpoint-url "http://127.0.0.1:8000" s3api list-buckets --profile ceph-test | grep s3bucket-sample-retain \ No newline at end of file +- script: aws --endpoint-url "http://127.0.0.1:8000" s3api list-buckets --profile ceph-subuser1 && exit 1 || exit 0 \ No newline at end of file diff --git a/testing/e2e/06-delete-subuser.yaml b/testing/e2e/07-delete-subuser.yaml similarity index 100% rename from testing/e2e/06-delete-subuser.yaml rename to testing/e2e/07-delete-subuser.yaml diff --git a/testing/e2e/07-errors.yaml b/testing/e2e/07-errors.yaml index 732765d..95848f4 100644 --- a/testing/e2e/07-errors.yaml +++ b/testing/e2e/07-errors.yaml @@ -1,14 +1,6 @@ -apiVersion: s3.snappcloud.io/v1alpha1 -kind: S3Bucket +apiVersion: v1 +kind: Secret metadata: - name: s3bucket-sample-delete + name: s3userclaim-sample-subuser1 namespace: s3-test - ---- - -apiVersion: s3.snappcloud.io/v1alpha1 -kind: S3Bucket -metadata: - name: s3bucket-sample-retain - namespace: s3-test - +type: Opaque \ No newline at end of file diff --git a/testing/e2e/08-assert.yaml b/testing/e2e/08-assert.yaml new file mode 100644 index 0000000..573d84c --- /dev/null +++ b/testing/e2e/08-assert.yaml @@ -0,0 +1,9 @@ +--- + +apiVersion: kuttl.dev/v1beta1 +kind: TestAssert +timeout: 30 +commands: + # Check if the buckets are existed on s3 +- script: aws --endpoint-url "http://127.0.0.1:8000" s3api list-buckets --profile ceph-test | grep s3bucket-sample-delete | [ $(wc -c) -eq 0 ] +- script: aws --endpoint-url "http://127.0.0.1:8000" s3api list-buckets --profile ceph-test | grep s3bucket-sample-retain \ No newline at end of file diff --git a/testing/e2e/07-delete-bucket.yaml b/testing/e2e/08-delete-bucket.yaml similarity index 100% rename from testing/e2e/07-delete-bucket.yaml rename to testing/e2e/08-delete-bucket.yaml diff --git a/testing/e2e/08-errors.yaml b/testing/e2e/08-errors.yaml index 2bf2bec..732765d 100644 --- a/testing/e2e/08-errors.yaml +++ b/testing/e2e/08-errors.yaml @@ -1,12 +1,14 @@ apiVersion: s3.snappcloud.io/v1alpha1 -kind: S3UserClaim +kind: S3Bucket metadata: - name: s3userclaim-sample + name: s3bucket-sample-delete namespace: s3-test --- + apiVersion: s3.snappcloud.io/v1alpha1 -kind: S3User +kind: S3Bucket metadata: - name: s3-test.s3userclaim-sample + name: s3bucket-sample-retain + namespace: s3-test diff --git a/testing/e2e/08-delete-userclaim.yaml b/testing/e2e/09-delete-userclaim.yaml similarity index 100% rename from testing/e2e/08-delete-userclaim.yaml rename to testing/e2e/09-delete-userclaim.yaml diff --git a/testing/e2e/09-errors.yaml b/testing/e2e/09-errors.yaml index f56e822..2bf2bec 100644 --- a/testing/e2e/09-errors.yaml +++ b/testing/e2e/09-errors.yaml @@ -1,19 +1,12 @@ -apiVersion: s3.snappcloud.io/v1alpha1 -kind: S3Bucket -metadata: - name: s3bucket-extra-delete - namespace: s3-test - ---- - apiVersion: s3.snappcloud.io/v1alpha1 kind: S3UserClaim metadata: - name: s3userclaim-extra + name: s3userclaim-sample namespace: s3-test --- apiVersion: s3.snappcloud.io/v1alpha1 kind: S3User metadata: - name: s3-test.s3userclaim-extra \ No newline at end of file + name: s3-test.s3userclaim-sample + diff --git a/testing/e2e/09-delete-extra-user-bucket.yaml b/testing/e2e/10-delete-extra-user-bucket.yaml similarity index 100% rename from testing/e2e/09-delete-extra-user-bucket.yaml rename to testing/e2e/10-delete-extra-user-bucket.yaml diff --git a/testing/e2e/10-errors.yaml b/testing/e2e/10-errors.yaml new file mode 100644 index 0000000..f56e822 --- /dev/null +++ b/testing/e2e/10-errors.yaml @@ -0,0 +1,19 @@ +apiVersion: s3.snappcloud.io/v1alpha1 +kind: S3Bucket +metadata: + name: s3bucket-extra-delete + namespace: s3-test + +--- + +apiVersion: s3.snappcloud.io/v1alpha1 +kind: S3UserClaim +metadata: + name: s3userclaim-extra + namespace: s3-test + +--- +apiVersion: s3.snappcloud.io/v1alpha1 +kind: S3User +metadata: + name: s3-test.s3userclaim-extra \ No newline at end of file diff --git a/testing/e2e/bucket-with-subuser-access.yaml b/testing/e2e/bucket-with-subuser-access.yaml new file mode 100644 index 0000000..0256781 --- /dev/null +++ b/testing/e2e/bucket-with-subuser-access.yaml @@ -0,0 +1,11 @@ +apiVersion: s3.snappcloud.io/v1alpha1 +kind: S3Bucket +metadata: + name: s3bucket-sample-delete + namespace: s3-test +spec: + s3UserRef: s3userclaim-sample + s3DeletionPolicy: delete + s3SubUserBinding: + - name: subuser1 + access: read \ No newline at end of file From 765e74ea9fcebcaa15f15dcb14e24680a4f70ede Mon Sep 17 00:00:00 2001 From: Hamed Karbasi Date: Sun, 29 Oct 2023 15:22:27 +0330 Subject: [PATCH 15/36] add s3subuserbinding type --- api/v1alpha1/s3bucket_types.go | 3 +++ api/v1alpha1/types.go | 10 +++++++++ api/v1alpha1/zz_generated.deepcopy.go | 22 ++++++++++++++++++- .../crd/bases/s3.snappcloud.io_s3buckets.yaml | 19 ++++++++++++++++ 4 files changed, 53 insertions(+), 1 deletion(-) diff --git a/api/v1alpha1/s3bucket_types.go b/api/v1alpha1/s3bucket_types.go index d80b056..f4c4484 100644 --- a/api/v1alpha1/s3bucket_types.go +++ b/api/v1alpha1/s3bucket_types.go @@ -29,6 +29,9 @@ type S3BucketSpec struct { // +kubebuilder:validation:Enum=delete;retain // +kubebuilder:default=delete S3DeletionPolicy string `json:"s3DeletionPolicy,omitempty"` + + // +kubebuilder:validation:Optional + S3SubUserBinding []SubUserBinding `json:"s3SubUserBinding,omitempty"` } // S3BucketStatus defines the observed state of S3Bucket diff --git a/api/v1alpha1/types.go b/api/v1alpha1/types.go index 9e43634..384e033 100644 --- a/api/v1alpha1/types.go +++ b/api/v1alpha1/types.go @@ -11,3 +11,13 @@ type UserQuota struct { // max number of buckets the user can create MaxBuckets int `json:"maxBuckets,omitempty"` } + +type SubUserBinding struct { + // name of the subuser + // +kubebuilder:validation:Required + Name string `json:"name"` + // access of the subuser which can be read, write or full + // +kubebuilder:default=read + // +kubebuilder:validation:Enum=read;write;full + Access string `json:"access,omitempty"` +} diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 082cf70..04e9772 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -31,7 +31,7 @@ func (in *S3Bucket) DeepCopyInto(out *S3Bucket) { *out = *in out.TypeMeta = in.TypeMeta in.ObjectMeta.DeepCopyInto(&out.ObjectMeta) - out.Spec = in.Spec + in.Spec.DeepCopyInto(&out.Spec) out.Status = in.Status } @@ -88,6 +88,11 @@ func (in *S3BucketList) DeepCopyObject() runtime.Object { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *S3BucketSpec) DeepCopyInto(out *S3BucketSpec) { *out = *in + if in.S3SubUserBinding != nil { + in, out := &in.S3SubUserBinding, &out.S3SubUserBinding + *out = make([]SubUserBinding, len(*in)) + copy(*out, *in) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new S3BucketSpec. @@ -323,6 +328,21 @@ func (in *S3UserStatus) DeepCopy() *S3UserStatus { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *SubUserBinding) DeepCopyInto(out *SubUserBinding) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new SubUserBinding. +func (in *SubUserBinding) DeepCopy() *SubUserBinding { + if in == nil { + return nil + } + out := new(SubUserBinding) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *UserQuota) DeepCopyInto(out *UserQuota) { *out = *in diff --git a/config/crd/bases/s3.snappcloud.io_s3buckets.yaml b/config/crd/bases/s3.snappcloud.io_s3buckets.yaml index fd69f0e..e6d77d9 100644 --- a/config/crd/bases/s3.snappcloud.io_s3buckets.yaml +++ b/config/crd/bases/s3.snappcloud.io_s3buckets.yaml @@ -47,6 +47,25 @@ spec: - delete - retain type: string + s3SubUserBinding: + items: + properties: + access: + default: read + description: access of the subuser which can be read, write + or full + enum: + - read + - write + - full + type: string + name: + description: name of the subuser + type: string + required: + - name + type: object + type: array s3UserRef: type: string required: From 85d6b50f4f4bfd2d8abc7f292ec465fe280ad689 Mon Sep 17 00:00:00 2001 From: Hamed Karbasi Date: Mon, 30 Oct 2023 17:40:44 +0330 Subject: [PATCH 16/36] add untested bucket policy logic --- internal/controllers/s3bucket/handler.go | 47 ++++++++++++++-- internal/controllers/s3bucket/provisioner.go | 9 +++ internal/s3_agent/s3_agent.go | 58 +++++++++++++++++++- pkg/consts/consts.go | 4 ++ 4 files changed, 113 insertions(+), 5 deletions(-) diff --git a/internal/controllers/s3bucket/handler.go b/internal/controllers/s3bucket/handler.go index 19b51dd..233f24d 100644 --- a/internal/controllers/s3bucket/handler.go +++ b/internal/controllers/s3bucket/handler.go @@ -3,6 +3,7 @@ package s3bucket import ( "context" "fmt" + "regexp" "github.com/go-logr/logr" "github.com/opdev/subreconciler" @@ -29,10 +30,14 @@ type Reconciler struct { logger logr.Logger s3Agent *s3_agent.S3Agent // reconcile specific variables - s3Bucket *s3v1alpha1.S3Bucket - s3UserRef string - s3BucketName string - rgwEndpoint string + s3Bucket *s3v1alpha1.S3Bucket + s3UserRef string + s3BucketName string + rgwEndpoint string + clusterName string + cephTenant string + cephUserFullId string + subUserAccessMap map[string]string } func NewReconciler(mgr manager.Manager, cfg *config.Config) *Reconciler { @@ -41,6 +46,7 @@ func NewReconciler(mgr manager.Manager, cfg *config.Config) *Reconciler { Client: mgr.GetClient(), scheme: mgr.GetScheme(), rgwEndpoint: cfg.Rgw.Endpoint, + clusterName: cfg.ClusterName, } } @@ -72,6 +78,8 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu r.logger.Error(err, "Failed to login on S3 with the user credentials") return subreconciler.Evaluate(subreconciler.Requeue()) } + // Initialize ceph tenant and cephFullUserId variables + r.initVars(req) // Delete event if r.s3Bucket.ObjectMeta.DeletionTimestamp != nil { return r.Cleanup(ctx) @@ -103,5 +111,36 @@ func (r *Reconciler) setS3Agent(ctx context.Context, req ctrl.Request) error { if err != nil { return err } + + readActions := []string{ + "s3:ListBucket", + "s3:GetObject", + } + writeActions := []string{ + "s3:DeleteObject", + "s3:PutObject", + } + + // TODO: Move this to operator level instead of reconciler level + r.s3Agent.BucketAccessAction = map[string][]string{ + consts.BucketAccessRead: readActions, + consts.BucketAccessWrite: append(readActions, writeActions...), + } return nil } + +func (r *Reconciler) initVars(req ctrl.Request) { + // TODO: This function is mutual with the s3userclaim handler. It should be moved to a higher layer. + // Only alphanumeric characters and underscore are allowed for tenant name + k8sNameSpecialChars := regexp.MustCompile(`[.-]`) + namespace := k8sNameSpecialChars.ReplaceAllString(req.Namespace, "_") + clusterName := k8sNameSpecialChars.ReplaceAllString(r.clusterName, "_") + r.cephTenant = fmt.Sprintf("%s__%s", clusterName, namespace) + r.cephUserFullId = fmt.Sprintf("%s$%s", r.cephTenant, r.s3UserRef) + + r.subUserAccessMap = make(map[string]string) + for _, binding := range r.s3Bucket.Spec.S3SubUserBinding { + r.subUserAccessMap[binding.Name] = binding.Access + } + +} diff --git a/internal/controllers/s3bucket/provisioner.go b/internal/controllers/s3bucket/provisioner.go index 486e249..a51ba84 100644 --- a/internal/controllers/s3bucket/provisioner.go +++ b/internal/controllers/s3bucket/provisioner.go @@ -20,6 +20,7 @@ func (r *Reconciler) Provision(ctx context.Context) (ctrl.Result, error) { // Do the actual reconcile work subrecs := []subreconciler.Fn{ r.ensureBucket, + r.ensureBucketPolicy, r.updateBucketStatus, r.addCleanupFinalizer, } @@ -41,6 +42,14 @@ func (r *Reconciler) ensureBucket(ctx context.Context) (*ctrl.Result, error) { return subreconciler.ContinueReconciling() } +func (r *Reconciler) ensureBucketPolicy(ctx context.Context) (*ctrl.Result, error) { + err := r.s3Agent.SetBucketPolicy(r.subUserAccessMap, + r.cephTenant, r.s3UserRef, r.s3BucketName) + if err != nil { + return subreconciler.Requeue() + } + return subreconciler.ContinueReconciling() +} func (r *Reconciler) updateBucketStatus(ctx context.Context) (*ctrl.Result, error) { status := s3v1alpha1.S3BucketStatus{ Ready: true, diff --git a/internal/s3_agent/s3_agent.go b/internal/s3_agent/s3_agent.go index d5d24b6..7e17c64 100644 --- a/internal/s3_agent/s3_agent.go +++ b/internal/s3_agent/s3_agent.go @@ -1,6 +1,7 @@ package s3_agent import ( + "encoding/json" "fmt" "net/http" "time" @@ -14,7 +15,8 @@ import ( // S3Agent wraps the s3.S3 structure to allow for wrapper methods type S3Agent struct { - Client *s3.S3 + Client *s3.S3 + BucketAccessAction map[string][]string } func NewS3Agent(accessKey, secretKey, endpoint string, debug bool) (*S3Agent, error) { @@ -74,3 +76,57 @@ func (s *S3Agent) DeleteBucket(name string) error { _, err := s.Client.DeleteBucket(bucketInput) return err } + +func (s *S3Agent) SetBucketPolicy(subUserAccessMap map[string]string, tenant string, + owner string, bucket string) error { + // The map of access levels to the AWS IAM names slice + accessAWSIAMMap := make(map[string][]string) + policy := map[string]interface{}{ + "Version": "2012-10-17", + "Id": "S3Policy", + } + statementSlice := []map[string]interface{}{} + for subUser, access := range subUserAccessMap { + // Create AWS IAM Name needed for the policy from the subuser name + aws_iam := fmt.Sprintf("arn:aws:iam::%s:user/%s:%s", tenant, owner, subUser) + accessAWSIAMMap[access] = append(accessAWSIAMMap[access], aws_iam) + } + + // Iterate over different levels + for access, AWS_iam := range accessAWSIAMMap { + principal := map[string][]string{} + statement := map[string]interface{}{ + "Sid": "BucketAllow", + "Effect": "Allow", + "Principal": map[string][]string{}, + "Action": []string{}, + "Resource": []string{ + fmt.Sprintf("arn:aws:s3::%s:%s", tenant, bucket), + fmt.Sprintf("arn:aws:s3::%s:%s/*", tenant, bucket), + }, + } + principal["AWS"] = AWS_iam + statement["Principal"] = principal + + if actions, exists := s.BucketAccessAction[access]; exists { + statement["Action"] = actions + } else { + return fmt.Errorf("the access %s doesn't exists", access) + } + // Append the statement + statementSlice = append(statementSlice, statement) + } + policy["Statement"] = statementSlice + policyMarshal, err := json.Marshal(policy) + + policyInput := s3.PutBucketPolicyInput{Bucket: aws.String(bucket), + Policy: aws.String(string(policyMarshal))} + if err != nil { + return err + } + _, err = s.Client.PutBucketPolicy(&policyInput) + if err != nil { + return err + } + return nil +} diff --git a/pkg/consts/consts.go b/pkg/consts/consts.go index 764e805..bcf5d54 100644 --- a/pkg/consts/consts.go +++ b/pkg/consts/consts.go @@ -37,4 +37,8 @@ const ( SubUserTagCreate = "create" SubUserTagRemove = "remove" + + // Bucket Access Levels + BucketAccessRead = "read" + BucketAccessWrite = "write" ) From 033e2868b0b14b7843e8300ada97646405b3ee7e Mon Sep 17 00:00:00 2001 From: Hamed Karbasi Date: Mon, 30 Oct 2023 18:37:02 +0330 Subject: [PATCH 17/36] revise bucket test --- api/v1alpha1/types.go | 4 ++-- config/crd/bases/s3.snappcloud.io_s3buckets.yaml | 4 +--- testing/e2e/05-assert.yaml | 1 - testing/e2e/06-add-bucket-access.yaml | 3 +++ testing/e2e/06-assert.yaml | 15 ++++++++++++--- testing/e2e/bucket-with-subuser-access.yaml | 2 ++ 6 files changed, 20 insertions(+), 9 deletions(-) diff --git a/api/v1alpha1/types.go b/api/v1alpha1/types.go index 384e033..df6a048 100644 --- a/api/v1alpha1/types.go +++ b/api/v1alpha1/types.go @@ -16,8 +16,8 @@ type SubUserBinding struct { // name of the subuser // +kubebuilder:validation:Required Name string `json:"name"` - // access of the subuser which can be read, write or full + // access of the subuser which can be read or write // +kubebuilder:default=read - // +kubebuilder:validation:Enum=read;write;full + // +kubebuilder:validation:Enum=read;write Access string `json:"access,omitempty"` } diff --git a/config/crd/bases/s3.snappcloud.io_s3buckets.yaml b/config/crd/bases/s3.snappcloud.io_s3buckets.yaml index e6d77d9..522e974 100644 --- a/config/crd/bases/s3.snappcloud.io_s3buckets.yaml +++ b/config/crd/bases/s3.snappcloud.io_s3buckets.yaml @@ -52,12 +52,10 @@ spec: properties: access: default: read - description: access of the subuser which can be read, write - or full + description: access of the subuser which can be read or write enum: - read - write - - full type: string name: description: name of the subuser diff --git a/testing/e2e/05-assert.yaml b/testing/e2e/05-assert.yaml index 61ccd3b..3259a04 100644 --- a/testing/e2e/05-assert.yaml +++ b/testing/e2e/05-assert.yaml @@ -33,6 +33,5 @@ status: # Assert that this user would see the buckets apiVersion: kuttl.dev/v1beta1 kind: TestAssert -timeout: 5 commands: - command: testing/e2e/set-aws-secret.sh ceph-subuser1 s3userclaim-sample-subuser1 diff --git a/testing/e2e/06-add-bucket-access.yaml b/testing/e2e/06-add-bucket-access.yaml index 7d77bb0..1d80ae5 100644 --- a/testing/e2e/06-add-bucket-access.yaml +++ b/testing/e2e/06-add-bucket-access.yaml @@ -2,5 +2,8 @@ apiVersion: kuttl.dev/v1beta1 kind: TestStep commands: - command: ./set-aws-secret.sh ceph-subuser1 s3userclaim-sample-subuser1 + - command: ./set-aws-secret.sh ceph-subuser2 s3userclaim-sample-subuser2 - command: kubectl apply -f bucket-with-subuser-access.yaml + # Put an object to the bucket for access testing + - command: echo "Hello, S3!" | aws s3 --endpoint-url "http://127.0.0.1:8000" cp - s3://s3bucket-sample-delete/test-object.txt --profile ceph-test \ No newline at end of file diff --git a/testing/e2e/06-assert.yaml b/testing/e2e/06-assert.yaml index 9b513b8..7f00ce4 100644 --- a/testing/e2e/06-assert.yaml +++ b/testing/e2e/06-assert.yaml @@ -3,6 +3,15 @@ apiVersion: kuttl.dev/v1beta1 kind: TestAssert timeout: 5 commands: - # Subuser must now be able to get an object -- script: aws s3 --endpoint-url "http://127.0.0.1:8000" ls s3://s3bucket-sample-delete --profile ceph-subuser1 | grep "test-object.txt" - \ No newline at end of file + # Subuser1 must be able to put an object in the bucket +- script: echo "Hello, S3!" | aws s3 --endpoint-url "http://127.0.0.1:8000" cp - s3://s3bucket-sample-delete/test-object.txt --profile ceph-subuser1 + # Subuser1 must be able to get the object from the bucket +- script: aws s3 --endpoint-url "http://127.0.0.1:8000" cp s3://s3bucket-sample-delete/test-object.txt - --profile ceph-subuser1 | grep "Hello, S3!" + # Subuser1 must be able to delete the object +- script: aws s3 --endpoint-url "http://127.0.0.1:8000" rm s3://s3bucket-sample-delete/test-object.txt --profile ceph-subuser1 + # Subuser2 must now be able to get the object +- script: aws s3 --endpoint-url "http://127.0.0.1:8000" cp s3://s3bucket-sample-delete/test-object.txt - --profile ceph-subuser2 | grep "Hello, S3!" + # Subuser1 must not be able to put an object in the bucket +- script: echo "Hello, S3!" | aws s3 --endpoint-url "http://127.0.0.1:8000" cp - s3://s3bucket-sample-delete/test-object.txt --profile ceph-subuser2 && exit 1 || exit 0 + # Subuser1 must be able to delete the object +- script: aws s3 --endpoint-url "http://127.0.0.1:8000" rm s3://s3bucket-sample-delete/test-object.txt --profile ceph-subuser2 && exit 1 || exit 0 diff --git a/testing/e2e/bucket-with-subuser-access.yaml b/testing/e2e/bucket-with-subuser-access.yaml index 0256781..ec50cf2 100644 --- a/testing/e2e/bucket-with-subuser-access.yaml +++ b/testing/e2e/bucket-with-subuser-access.yaml @@ -8,4 +8,6 @@ spec: s3DeletionPolicy: delete s3SubUserBinding: - name: subuser1 + access: write + - name: subuser2 access: read \ No newline at end of file From 199a0235953014a1466e9adf494f989fe5db3a9b Mon Sep 17 00:00:00 2001 From: Hamed Karbasi Date: Tue, 31 Oct 2023 21:28:12 +0330 Subject: [PATCH 18/36] fix testing problem --- testing/e2e/01-install-resource-quota.yaml | 2 +- testing/e2e/02-assert.yaml | 4 ++-- testing/e2e/02-create-userclaim.yaml | 2 +- testing/e2e/03-assert.yaml | 3 +++ testing/e2e/05-add-subuser.yaml | 2 +- testing/e2e/05-assert.yaml | 12 ++++++++++-- testing/e2e/06-add-bucket-access.yaml | 3 --- testing/e2e/06-assert.yaml | 10 ++++++---- testing/e2e/07-delete-subuser.yaml | 2 +- 9 files changed, 25 insertions(+), 15 deletions(-) diff --git a/testing/e2e/01-install-resource-quota.yaml b/testing/e2e/01-install-resource-quota.yaml index 5c67f03..2c639ac 100644 --- a/testing/e2e/01-install-resource-quota.yaml +++ b/testing/e2e/01-install-resource-quota.yaml @@ -6,7 +6,7 @@ spec: quota: hard: s3/objects: 5k - s3/size: 12k + s3/size: 22k s3/buckets: 5k selector: labels: diff --git a/testing/e2e/02-assert.yaml b/testing/e2e/02-assert.yaml index bd5fd76..7ad2672 100644 --- a/testing/e2e/02-assert.yaml +++ b/testing/e2e/02-assert.yaml @@ -7,7 +7,7 @@ metadata: namespace: s3-test status: quota: - maxSize: 5k + maxSize: 10k maxObjects: "500" maxBuckets: 5 --- @@ -18,7 +18,7 @@ metadata: name: s3-test.s3userclaim-sample spec: quota: - maxSize: 5k + maxSize: 10k maxObjects: "500" maxBuckets: 5 claimRef: diff --git a/testing/e2e/02-create-userclaim.yaml b/testing/e2e/02-create-userclaim.yaml index f3a7af0..a228dd3 100644 --- a/testing/e2e/02-create-userclaim.yaml +++ b/testing/e2e/02-create-userclaim.yaml @@ -9,7 +9,7 @@ spec: readonlySecret: s3-sample-readonly-secret adminSecret: s3-sample-admin-secret quota: - maxSize: 5000 + maxSize: 10000 maxObjects: 500 maxBuckets: 5 --- diff --git a/testing/e2e/03-assert.yaml b/testing/e2e/03-assert.yaml index ebecb56..d70b3c6 100644 --- a/testing/e2e/03-assert.yaml +++ b/testing/e2e/03-assert.yaml @@ -34,6 +34,9 @@ commands: - script: aws s3 --endpoint-url "http://127.0.0.1:8000" ls s3://s3bucket-sample-delete --profile ceph-test | grep "test-object.txt" # Check if the user can delete the object - script: aws s3 --endpoint-url "http://127.0.0.1:8000" rm s3://s3bucket-sample-delete/test-object.txt --profile ceph-test + # Put the object again for the next steps +- script: echo "Hello, S3!" | aws s3 --endpoint-url "http://127.0.0.1:8000" cp - s3://s3bucket-sample-delete/test-object.txt --profile ceph-test + --- # Check CR of the extra user bucket diff --git a/testing/e2e/05-add-subuser.yaml b/testing/e2e/05-add-subuser.yaml index 073f873..97c0d26 100644 --- a/testing/e2e/05-add-subuser.yaml +++ b/testing/e2e/05-add-subuser.yaml @@ -8,7 +8,7 @@ spec: readonlySecret: s3-sample-readonly-secret adminSecret: s3-sample-admin-secret quota: - maxSize: 5000 + maxSize: 10000 maxObjects: 500 maxBuckets: 5 subUsers: diff --git a/testing/e2e/05-assert.yaml b/testing/e2e/05-assert.yaml index 3259a04..07b4e84 100644 --- a/testing/e2e/05-assert.yaml +++ b/testing/e2e/05-assert.yaml @@ -23,15 +23,23 @@ metadata: namespace: s3-test status: quota: - maxSize: 5k + maxSize: 10k maxObjects: "500" maxBuckets: 5 subUsers: - subuser1 - subuser2 --- -# Assert that this user would see the buckets apiVersion: kuttl.dev/v1beta1 kind: TestAssert commands: +# Login with subuser 1 - command: testing/e2e/set-aws-secret.sh ceph-subuser1 s3userclaim-sample-subuser1 +# Subuser most be able to see the buckets +- script: aws --endpoint-url "http://127.0.0.1:8000" s3api list-buckets --profile ceph-subuser1 | grep s3bucket-sample-delete +# Subuser most not be able to get an object +- script: aws s3 --endpoint-url "http://127.0.0.1:8000" cp s3://s3bucket-sample-delete/test-object.txt - --profile ceph-subuser1 && exit 1 || exit 0 + # Subuser1 must not be able to put an object in the bucket +- script: echo "Hello, S3!" | aws s3 --endpoint-url "http://127.0.0.1:8000" cp - s3://s3bucket-sample-delete/test-object.txt --profile ceph-subuser1 && exit 1 || exit 0 + # Subuser1 must be able to delete the object +- script: aws s3 --endpoint-url "http://127.0.0.1:8000" rm s3://s3bucket-sample-delete/test-object.txt --profile ceph-subuser1 && exit 1 || exit 0 diff --git a/testing/e2e/06-add-bucket-access.yaml b/testing/e2e/06-add-bucket-access.yaml index 1d80ae5..dd16719 100644 --- a/testing/e2e/06-add-bucket-access.yaml +++ b/testing/e2e/06-add-bucket-access.yaml @@ -4,6 +4,3 @@ commands: - command: ./set-aws-secret.sh ceph-subuser1 s3userclaim-sample-subuser1 - command: ./set-aws-secret.sh ceph-subuser2 s3userclaim-sample-subuser2 - command: kubectl apply -f bucket-with-subuser-access.yaml - # Put an object to the bucket for access testing - - command: echo "Hello, S3!" | aws s3 --endpoint-url "http://127.0.0.1:8000" cp - s3://s3bucket-sample-delete/test-object.txt --profile ceph-test - \ No newline at end of file diff --git a/testing/e2e/06-assert.yaml b/testing/e2e/06-assert.yaml index 7f00ce4..86658b1 100644 --- a/testing/e2e/06-assert.yaml +++ b/testing/e2e/06-assert.yaml @@ -3,15 +3,17 @@ apiVersion: kuttl.dev/v1beta1 kind: TestAssert timeout: 5 commands: - # Subuser1 must be able to put an object in the bucket -- script: echo "Hello, S3!" | aws s3 --endpoint-url "http://127.0.0.1:8000" cp - s3://s3bucket-sample-delete/test-object.txt --profile ceph-subuser1 # Subuser1 must be able to get the object from the bucket - script: aws s3 --endpoint-url "http://127.0.0.1:8000" cp s3://s3bucket-sample-delete/test-object.txt - --profile ceph-subuser1 | grep "Hello, S3!" # Subuser1 must be able to delete the object - script: aws s3 --endpoint-url "http://127.0.0.1:8000" rm s3://s3bucket-sample-delete/test-object.txt --profile ceph-subuser1 + # Subuser1 must be able to put an object in the bucket +- script: echo "Hello, S3!" | aws s3 --endpoint-url "http://127.0.0.1:8000" cp - s3://s3bucket-sample-delete/test-object.txt --profile ceph-subuser1 # Subuser2 must now be able to get the object - script: aws s3 --endpoint-url "http://127.0.0.1:8000" cp s3://s3bucket-sample-delete/test-object.txt - --profile ceph-subuser2 | grep "Hello, S3!" - # Subuser1 must not be able to put an object in the bucket + # Subuser2 must not be able to put an object in the bucket - script: echo "Hello, S3!" | aws s3 --endpoint-url "http://127.0.0.1:8000" cp - s3://s3bucket-sample-delete/test-object.txt --profile ceph-subuser2 && exit 1 || exit 0 - # Subuser1 must be able to delete the object + # Subuser2 must be able to delete the object - script: aws s3 --endpoint-url "http://127.0.0.1:8000" rm s3://s3bucket-sample-delete/test-object.txt --profile ceph-subuser2 && exit 1 || exit 0 + # Empty the bucket +- script: aws s3 --endpoint-url "http://127.0.0.1:8000" rm s3://s3bucket-sample-delete/test-object.txt --profile ceph-subuser1 diff --git a/testing/e2e/07-delete-subuser.yaml b/testing/e2e/07-delete-subuser.yaml index 644524f..229a46d 100644 --- a/testing/e2e/07-delete-subuser.yaml +++ b/testing/e2e/07-delete-subuser.yaml @@ -8,7 +8,7 @@ spec: readonlySecret: s3-sample-readonly-secret adminSecret: s3-sample-admin-secret quota: - maxSize: 5000 + maxSize: 10000 maxObjects: 500 maxBuckets: 5 subUsers: From 5eb0228963bb9d90b12e1f1aed70f383aed5260b Mon Sep 17 00:00:00 2001 From: Hamed Karbasi Date: Fri, 3 Nov 2023 18:13:00 +0330 Subject: [PATCH 19/36] user pointer for s3userclaim webhook --- api/v1alpha1/s3userclaim_webhook.go | 46 +++++++++++------------------ 1 file changed, 17 insertions(+), 29 deletions(-) diff --git a/api/v1alpha1/s3userclaim_webhook.go b/api/v1alpha1/s3userclaim_webhook.go index f101b69..dbe5227 100644 --- a/api/v1alpha1/s3userclaim_webhook.go +++ b/api/v1alpha1/s3userclaim_webhook.go @@ -69,15 +69,13 @@ var _ webhook.Validator = &S3UserClaim{} func (suc *S3UserClaim) ValidateCreate() error { s3userclaimlog.Info("validate create", "name", suc.Name) + allErrs := field.ErrorList{} // Validate Quota - allErrs := validateQuota(suc) + validateQuota(suc, &allErrs) // Validate subUsers - subUsersErrorList := validateSubUsers(suc.Spec.SubUsers) - - // Concatinate all errors - allErrs = append(allErrs, subUsersErrorList...) + validateSubUsers(suc.Spec.SubUsers, &allErrs) if len(allErrs) == 0 { return nil @@ -104,13 +102,11 @@ func (suc *S3UserClaim) ValidateUpdate(old runtime.Object) error { } // Validate Quota - errorList := validateQuota(suc) - allErrs = append(allErrs, errorList...) + validateQuota(suc, &allErrs) // Validate subUsers - subusersErrorList := validateSubUsers(suc.Spec.SubUsers) + validateSubUsers(suc.Spec.SubUsers, &allErrs) - allErrs = append(allErrs, subusersErrorList...) if len(allErrs) == 0 { return nil } @@ -140,35 +136,32 @@ func (suc *S3UserClaim) ValidateDelete() error { return nil } -func validateQuota(suc *S3UserClaim) field.ErrorList { +func validateQuota(suc *S3UserClaim, allErrsPointer *field.ErrorList) { ctx, cancel := context.WithTimeout(context.Background(), ValidationTimeout) defer cancel() - errorList := field.ErrorList{} - quotaFieldPath := field.NewPath("spec").Child("quota") // TODO(therealak12): refactor the code as there are similarities between two quota validator functions switch err := validateAgainstNamespaceQuota(ctx, suc); { case err == consts.ErrExceededNamespaceQuota: - errorList = append(errorList, field.Forbidden(quotaFieldPath, err.Error())) + *allErrsPointer = append(*allErrsPointer, field.Forbidden(quotaFieldPath, err.Error())) case err != nil: s3userclaimlog.Error(err, "failed to validate against cluster quota") - errorList = append(errorList, field.InternalError(quotaFieldPath, fmt.Errorf(consts.ContactCloudTeamErrMessage))) + *allErrsPointer = append(*allErrsPointer, field.InternalError(quotaFieldPath, fmt.Errorf(consts.ContactCloudTeamErrMessage))) } switch err := validateAgainstClusterQuota(ctx, suc); { case err == consts.ErrExceededClusterQuota: - errorList = append(errorList, field.Forbidden(quotaFieldPath, err.Error())) + *allErrsPointer = append(*allErrsPointer, field.Forbidden(quotaFieldPath, err.Error())) case goerrors.Is(err, consts.ErrClusterQuotaNotDefined): - errorList = append(errorList, field.Forbidden(quotaFieldPath, err.Error())) + *allErrsPointer = append(*allErrsPointer, field.Forbidden(quotaFieldPath, err.Error())) case err != nil: s3userclaimlog.Error(err, "failed to validate against cluster quota") - errorList = append(errorList, field.InternalError(quotaFieldPath, fmt.Errorf(consts.ContactCloudTeamErrMessage))) + *allErrsPointer = append(*allErrsPointer, field.InternalError(quotaFieldPath, fmt.Errorf(consts.ContactCloudTeamErrMessage))) } - return errorList } func validateAgainstNamespaceQuota(ctx context.Context, suc *S3UserClaim) error { @@ -292,10 +285,10 @@ func findTeam(ctx context.Context, suc *S3UserClaim) (string, error) { return "", fmt.Errorf("failed to get namespace, %w", err) } - labels := ns.ObjectMeta.Labels - if labels == nil { - labels = map[string]string{} - } + // labels := ns.ObjectMeta.Labels + // if labels == nil { + // labels = map[string]string{} + // } team, ok := ns.ObjectMeta.Labels[consts.LabelTeam] if !ok { @@ -326,20 +319,15 @@ func findTeamNamespaces(ctx context.Context, team string) ([]string, error) { return namespaces, nil } -func validateSubUsers(subUsers []string) field.ErrorList { +func validateSubUsers(subUsers []string, allErrsPointer *field.ErrorList) { // Since the subUsers name is used in their secret name, they should follow the below // regex pattern. Otherwise, the request has to be denied. - errorList := field.ErrorList{} pattern := `^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$` regex := regexp.MustCompile(pattern) for _, subUser := range subUsers { if !regex.MatchString(subUser) { errorReason := fmt.Sprintf("subuser: %s is invalid. It must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character", subUser) - errorList = append(errorList, field.Forbidden(field.NewPath("spec").Child("subUsers"), errorReason)) + *allErrsPointer = append(*allErrsPointer, field.Forbidden(field.NewPath("spec").Child("subUsers"), errorReason)) } } - if len(errorList) == 0 { - return nil - } - return errorList } From 8d71e84b355619b2fcb304234fcc470367be343c Mon Sep 17 00:00:00 2001 From: Hamed Karbasi Date: Fri, 3 Nov 2023 18:46:23 +0330 Subject: [PATCH 20/36] add update bucket status for success and failure deletion --- api/v1alpha1/s3bucket_types.go | 6 ++++++ api/v1alpha1/zz_generated.deepcopy.go | 7 ++++++- .../crd/bases/s3.snappcloud.io_s3buckets.yaml | 19 +++++++++++++++++++ internal/controllers/s3bucket/cleaner.go | 2 ++ internal/controllers/s3bucket/provisioner.go | 13 ++++++++++--- 5 files changed, 43 insertions(+), 4 deletions(-) diff --git a/api/v1alpha1/s3bucket_types.go b/api/v1alpha1/s3bucket_types.go index f4c4484..a046f6f 100644 --- a/api/v1alpha1/s3bucket_types.go +++ b/api/v1alpha1/s3bucket_types.go @@ -39,6 +39,12 @@ type S3BucketStatus struct { // +kubebuilder:validation:Optional // +kubebuilder:default=false Ready bool `json:"ready,omitempty"` + + // +kubebuilder:validation:Optional + Reason string `json:"reason,omitempty"` + + // +kubebuilder:validation:Optional + S3SubUserBinding []SubUserBinding `json:"s3SubUserBinding,omitempty"` } //+kubebuilder:object:root=true diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 04e9772..324899c 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -32,7 +32,7 @@ func (in *S3Bucket) DeepCopyInto(out *S3Bucket) { out.TypeMeta = in.TypeMeta in.ObjectMeta.DeepCopyInto(&out.ObjectMeta) in.Spec.DeepCopyInto(&out.Spec) - out.Status = in.Status + in.Status.DeepCopyInto(&out.Status) } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new S3Bucket. @@ -108,6 +108,11 @@ func (in *S3BucketSpec) DeepCopy() *S3BucketSpec { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *S3BucketStatus) DeepCopyInto(out *S3BucketStatus) { *out = *in + if in.S3SubUserBinding != nil { + in, out := &in.S3SubUserBinding, &out.S3SubUserBinding + *out = make([]SubUserBinding, len(*in)) + copy(*out, *in) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new S3BucketStatus. diff --git a/config/crd/bases/s3.snappcloud.io_s3buckets.yaml b/config/crd/bases/s3.snappcloud.io_s3buckets.yaml index 522e974..dc8e11f 100644 --- a/config/crd/bases/s3.snappcloud.io_s3buckets.yaml +++ b/config/crd/bases/s3.snappcloud.io_s3buckets.yaml @@ -75,6 +75,25 @@ spec: ready: default: false type: boolean + reason: + type: string + s3SubUserBinding: + items: + properties: + access: + default: read + description: access of the subuser which can be read or write + enum: + - read + - write + type: string + name: + description: name of the subuser + type: string + required: + - name + type: object + type: array type: object type: object served: true diff --git a/internal/controllers/s3bucket/cleaner.go b/internal/controllers/s3bucket/cleaner.go index e45ffc7..cec79d3 100644 --- a/internal/controllers/s3bucket/cleaner.go +++ b/internal/controllers/s3bucket/cleaner.go @@ -44,6 +44,8 @@ func (r *Reconciler) removeOrRetainBucket(ctx context.Context) (*ctrl.Result, er } } r.logger.Error(err, "failed to remove the bucket") + // update bucket status with failure reason; e.g. Bucket is not empty + r.updateBucketStatus(ctx, false, err.Error(), r.s3Bucket.Spec.S3SubUserBinding) return subreconciler.Requeue() } return subreconciler.ContinueReconciling() diff --git a/internal/controllers/s3bucket/provisioner.go b/internal/controllers/s3bucket/provisioner.go index a51ba84..756da9d 100644 --- a/internal/controllers/s3bucket/provisioner.go +++ b/internal/controllers/s3bucket/provisioner.go @@ -21,7 +21,7 @@ func (r *Reconciler) Provision(ctx context.Context) (ctrl.Result, error) { subrecs := []subreconciler.Fn{ r.ensureBucket, r.ensureBucketPolicy, - r.updateBucketStatus, + r.updateBucketStatusSuccess, r.addCleanupFinalizer, } for _, subrec := range subrecs { @@ -50,9 +50,16 @@ func (r *Reconciler) ensureBucketPolicy(ctx context.Context) (*ctrl.Result, erro } return subreconciler.ContinueReconciling() } -func (r *Reconciler) updateBucketStatus(ctx context.Context) (*ctrl.Result, error) { + +func (r *Reconciler) updateBucketStatusSuccess(ctx context.Context) (*ctrl.Result, error) { + return r.updateBucketStatus(ctx, true, "", r.s3Bucket.Spec.S3SubUserBinding) +} +func (r *Reconciler) updateBucketStatus(ctx context.Context, + ready bool, reason string, s3subUserBinding []s3v1alpha1.SubUserBinding) (*ctrl.Result, error) { status := s3v1alpha1.S3BucketStatus{ - Ready: true, + Ready: ready, + Reason: reason, + S3SubUserBinding: s3subUserBinding, } if !apiequality.Semantic.DeepEqual(r.s3Bucket.Status, status) { From 6818e0d307b203c2e002f850bef325b07422f2ee Mon Sep 17 00:00:00 2001 From: Hamed Karbasi Date: Fri, 3 Nov 2023 19:28:17 +0330 Subject: [PATCH 21/36] add some comments --- internal/controllers/s3bucket/cleaner.go | 2 +- internal/controllers/s3bucket/provisioner.go | 8 +++++--- internal/controllers/s3userclaim/provisioner.go | 1 + 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/internal/controllers/s3bucket/cleaner.go b/internal/controllers/s3bucket/cleaner.go index cec79d3..ef091ef 100644 --- a/internal/controllers/s3bucket/cleaner.go +++ b/internal/controllers/s3bucket/cleaner.go @@ -45,7 +45,7 @@ func (r *Reconciler) removeOrRetainBucket(ctx context.Context) (*ctrl.Result, er } r.logger.Error(err, "failed to remove the bucket") // update bucket status with failure reason; e.g. Bucket is not empty - r.updateBucketStatus(ctx, false, err.Error(), r.s3Bucket.Spec.S3SubUserBinding) + r.updateBucketStatus(ctx, false, err.Error()) return subreconciler.Requeue() } return subreconciler.ContinueReconciling() diff --git a/internal/controllers/s3bucket/provisioner.go b/internal/controllers/s3bucket/provisioner.go index 756da9d..bbfee16 100644 --- a/internal/controllers/s3bucket/provisioner.go +++ b/internal/controllers/s3bucket/provisioner.go @@ -46,20 +46,22 @@ func (r *Reconciler) ensureBucketPolicy(ctx context.Context) (*ctrl.Result, erro err := r.s3Agent.SetBucketPolicy(r.subUserAccessMap, r.cephTenant, r.s3UserRef, r.s3BucketName) if err != nil { + r.logger.Error(err, "failed to set the bucket policy") + r.updateBucketStatus(ctx, false, err.Error()) return subreconciler.Requeue() } return subreconciler.ContinueReconciling() } func (r *Reconciler) updateBucketStatusSuccess(ctx context.Context) (*ctrl.Result, error) { - return r.updateBucketStatus(ctx, true, "", r.s3Bucket.Spec.S3SubUserBinding) + return r.updateBucketStatus(ctx, true, "") } func (r *Reconciler) updateBucketStatus(ctx context.Context, - ready bool, reason string, s3subUserBinding []s3v1alpha1.SubUserBinding) (*ctrl.Result, error) { + ready bool, reason string) (*ctrl.Result, error) { status := s3v1alpha1.S3BucketStatus{ Ready: ready, Reason: reason, - S3SubUserBinding: s3subUserBinding, + S3SubUserBinding: r.s3Bucket.Spec.S3SubUserBinding, } if !apiequality.Semantic.DeepEqual(r.s3Bucket.Status, status) { diff --git a/internal/controllers/s3userclaim/provisioner.go b/internal/controllers/s3userclaim/provisioner.go index edfa996..e732e3f 100644 --- a/internal/controllers/s3userclaim/provisioner.go +++ b/internal/controllers/s3userclaim/provisioner.go @@ -193,6 +193,7 @@ func (r *Reconciler) ensureOtherSubusers(ctx context.Context) (*ctrl.Result, err Name: subUserSecretName, }, } + // Delete subUser secret switch err := r.Delete(ctx, subUserSecret); { case apierrors.IsNotFound(err): return subreconciler.ContinueReconciling() From dc975918cee12a214ce0ee610e7c4665ec681bad Mon Sep 17 00:00:00 2001 From: Hamed Karbasi Date: Fri, 3 Nov 2023 19:37:28 +0330 Subject: [PATCH 22/36] update e2e test doc --- docs/E2E-TEST-WORKFLOW.md | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/docs/E2E-TEST-WORKFLOW.md b/docs/E2E-TEST-WORKFLOW.md index b477095..836a1d0 100644 --- a/docs/E2E-TEST-WORKFLOW.md +++ b/docs/E2E-TEST-WORKFLOW.md @@ -3,7 +3,7 @@ The e2e tests are performed via [Kuttle](https://kuttl.dev/). Use the bellow command to run the tests: ```bash -kubectl-kuttl test +make e2e-test ``` ## Test Workflow @@ -12,15 +12,18 @@ Here is the test workflow: | Step | Action | Assertion | | ---- | ------------------------------------------------------------ | ------------------------------------------------------------ | -| 0 | Install external-crds | No assertion | -| 0 | Install Ceph cluster | No assertion | -| 0 | Run the operator locally | No assertion | -| 1 | Create S3UserClaims | S3UserClaim CR
S3User CR
Created user on ceph-rgw | -| 2 | Create S3Buckets: One with retain and one with delete DeletionPolicy mode. | S3Bucket CR
S3Bucket on ceph-rgw via aws-cli
aws-cli with user credentials **can** create or delete objects on the bucket | -| 3 | Webhook validation: Update S3Bucket S3UserRef | Must be **denied** by the bucket validation update webhook. | -| 3 | Webhook validation: Create S3bucket with wrong S3UserRef | Must be **denied** by the bucket validation create webhook. | -| 3 | Webhook validation: Delete S3UserClaim | Must be **denied** by the user validation delete Webhook. | -| 4 | Delete S3Buckets | DeletionPolicy on delete: bucket must **be** deleted.
DeletionPolicy on retain: bucket must **not be** deleted. | -| 5 | Delete S3UserClaim for Sample user | S3UserClaim and S3User CRs are deleted. | -| 6 | Delete S3bucket and S3UserClaim for Extra user | S3bucket, S3UserClaim and S3User CRs are deleted. | -| | | | +| 0 | Install Cert Manager | check if cert manager pods are ready | +| 1 | Install Ceph cluster | Check if ceph cluster pod is ready | +| 1 | Install resource quota CRD | No assertion | +| 1 | Install the operator | Check if operator pod is ready | +| 2 | Create S3UserClaims | S3UserClaim CR
S3User CR
Created user on ceph-rgw | +| 3 | Create S3Buckets: One with retain and one with delete DeletionPolicy mode. | S3Bucket CR
S3Bucket on ceph-rgw via aws-cli
aws-cli with user credentials **can** create or delete objects on the bucket | +| 4 | Webhook validation: Update S3Bucket S3UserRef | Must be **denied** by the bucket validation update webhook. | +| 4 | Webhook validation: Create S3bucket with wrong S3UserRef | Must be **denied** by the bucket validation create webhook. | +| 4 | Webhook validation: Delete S3UserClaim | Must be **denied** by the user validation delete Webhook. | +| 5 | Add subUser | Check if subUser is added to the s3Userclaim status.
Check if subUsers secrets are created. | +| 6 | Add Bucket Access | Check if subUsers have correct access to the bucket. | +| 7 | Delete subUser | Check if deleted subUser doesn't have access to the bucket. | +| 8 | Delete S3Buckets | DeletionPolicy on delete: bucket must **be** deleted.
DeletionPolicy on retain: bucket must **not be** deleted. | +| 9 | Delete S3UserClaim for Sample user | S3UserClaim and S3User CRs are deleted. | +| 10 | Delete S3bucket and S3UserClaim for Extra user | S3bucket, S3UserClaim and S3User CRs are deleted. | From 497c3aa5854a809bc330d8a97ec1c49e42ea5b77 Mon Sep 17 00:00:00 2001 From: Hamed Karbasi Date: Sat, 4 Nov 2023 11:25:14 +0330 Subject: [PATCH 23/36] fix make test --- internal/controllers/s3userclaim/s3userclaim_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/controllers/s3userclaim/s3userclaim_test.go b/internal/controllers/s3userclaim/s3userclaim_test.go index c0c8b4d..569f4ab 100644 --- a/internal/controllers/s3userclaim/s3userclaim_test.go +++ b/internal/controllers/s3userclaim/s3userclaim_test.go @@ -203,7 +203,7 @@ var _ = Describe("S3UserClaim Controller", func() { gotCephUser, err := rgwClient.GetUser(ctx, cephUser) g.Expect(err).NotTo(HaveOccurred()) - g.Expect(len(gotCephUser.Keys)).To(BeNumerically("==", 2)) + g.Expect(len(gotCephUser.Keys)).To(BeNumerically("==", 3)) g.Expect(gotCephUser.Keys).To(ContainElement(admin.UserKeySpec{ User: readonlyCephUser.Name, AccessKey: string(readonlySecret.Data[consts.DataKeyAccessKey]), @@ -325,7 +325,7 @@ var _ = Describe("S3UserClaim Controller", func() { It("Should remove the Ceph user with buckets", func() { gotCephUser, err := rgwClient.GetUser(ctx, cephUser) Expect(err).NotTo(HaveOccurred()) - Expect(len(gotCephUser.Keys)).To(Equal(2)) + Expect(len(gotCephUser.Keys)).To(Equal(3)) var s3Keys admin.UserKeySpec if gotCephUser.Keys[0].User == cephUser.ID { s3Keys = gotCephUser.Keys[0] From 11c8cd3af6a620a0c3d3e400478ebab059c8a0b9 Mon Sep 17 00:00:00 2001 From: Hamed Karbasi Date: Sat, 4 Nov 2023 13:13:31 +0330 Subject: [PATCH 24/36] remove ensure readonly user --- .../controllers/s3userclaim/provisioner.go | 21 ------------------- .../s3userclaim/s3userclaim_test.go | 4 ++-- 2 files changed, 2 insertions(+), 23 deletions(-) diff --git a/internal/controllers/s3userclaim/provisioner.go b/internal/controllers/s3userclaim/provisioner.go index e732e3f..7a2985e 100644 --- a/internal/controllers/s3userclaim/provisioner.go +++ b/internal/controllers/s3userclaim/provisioner.go @@ -29,7 +29,6 @@ func (r *Reconciler) Provision(ctx context.Context) (ctrl.Result, error) { subrecs := []subreconciler.Fn{ r.ensureCephUser, r.ensureCephUserQuota, - r.ensureReadonlySubuser, r.ensureOtherSubusers, // retrieve the ceph user to have keys of subuser at hand r.retrieveCephUser, @@ -113,26 +112,6 @@ func (r *Reconciler) ensureCephUserQuota(ctx context.Context) (*ctrl.Result, err } } -func (r *Reconciler) ensureReadonlySubuser(ctx context.Context) (*ctrl.Result, error) { - desiredSubuser := admin.SubuserSpec{ - Name: r.readonlyCephUserId, - Access: admin.SubuserAccessRead, - KeyType: pointer.String(consts.CephKeyTypeS3), - } - - for _, subuser := range r.cephUser.Subusers { - if subuser.Name == r.readonlyCephUserFullId { - return subreconciler.ContinueReconciling() - } - } - - if err := r.rgwClient.CreateSubuser(ctx, admin.User{ID: r.cephUserFullId}, desiredSubuser); err != nil { - r.logger.Error(err, "failed to create subuser") - return subreconciler.Requeue() - } - return subreconciler.ContinueReconciling() -} - func (r *Reconciler) ensureOtherSubusers(ctx context.Context) (*ctrl.Result, error) { specSubUsers := r.s3UserClaim.Spec.SubUsers // Create a hashmap to move all spec and ceph subUsers to it diff --git a/internal/controllers/s3userclaim/s3userclaim_test.go b/internal/controllers/s3userclaim/s3userclaim_test.go index 569f4ab..c0c8b4d 100644 --- a/internal/controllers/s3userclaim/s3userclaim_test.go +++ b/internal/controllers/s3userclaim/s3userclaim_test.go @@ -203,7 +203,7 @@ var _ = Describe("S3UserClaim Controller", func() { gotCephUser, err := rgwClient.GetUser(ctx, cephUser) g.Expect(err).NotTo(HaveOccurred()) - g.Expect(len(gotCephUser.Keys)).To(BeNumerically("==", 3)) + g.Expect(len(gotCephUser.Keys)).To(BeNumerically("==", 2)) g.Expect(gotCephUser.Keys).To(ContainElement(admin.UserKeySpec{ User: readonlyCephUser.Name, AccessKey: string(readonlySecret.Data[consts.DataKeyAccessKey]), @@ -325,7 +325,7 @@ var _ = Describe("S3UserClaim Controller", func() { It("Should remove the Ceph user with buckets", func() { gotCephUser, err := rgwClient.GetUser(ctx, cephUser) Expect(err).NotTo(HaveOccurred()) - Expect(len(gotCephUser.Keys)).To(Equal(3)) + Expect(len(gotCephUser.Keys)).To(Equal(2)) var s3Keys admin.UserKeySpec if gotCephUser.Keys[0].User == cephUser.ID { s3Keys = gotCephUser.Keys[0] From 5c2e0fd32461209814c750fec8c804b9acd21c1f Mon Sep 17 00:00:00 2001 From: Hamed Karbasi Date: Sat, 4 Nov 2023 13:41:24 +0330 Subject: [PATCH 25/36] remove redundant comment in s3userclaim webhook --- api/v1alpha1/s3userclaim_webhook.go | 7 ------- config/manager/kustomization.yaml | 5 ++++- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/api/v1alpha1/s3userclaim_webhook.go b/api/v1alpha1/s3userclaim_webhook.go index dbe5227..c3c01a5 100644 --- a/api/v1alpha1/s3userclaim_webhook.go +++ b/api/v1alpha1/s3userclaim_webhook.go @@ -71,10 +71,8 @@ func (suc *S3UserClaim) ValidateCreate() error { s3userclaimlog.Info("validate create", "name", suc.Name) allErrs := field.ErrorList{} - // Validate Quota validateQuota(suc, &allErrs) - // Validate subUsers validateSubUsers(suc.Spec.SubUsers, &allErrs) if len(allErrs) == 0 { @@ -285,11 +283,6 @@ func findTeam(ctx context.Context, suc *S3UserClaim) (string, error) { return "", fmt.Errorf("failed to get namespace, %w", err) } - // labels := ns.ObjectMeta.Labels - // if labels == nil { - // labels = map[string]string{} - // } - team, ok := ns.ObjectMeta.Labels[consts.LabelTeam] if !ok { return "", fmt.Errorf("namespace %s doesn't have team label", ns.ObjectMeta.Name) diff --git a/config/manager/kustomization.yaml b/config/manager/kustomization.yaml index 2d03f76..da9b1d0 100644 --- a/config/manager/kustomization.yaml +++ b/config/manager/kustomization.yaml @@ -6,4 +6,7 @@ kind: Kustomization images: - name: controller newName: controller - newTag: latest \ No newline at end of file + newTag: latest +- name: ghcr.io/snapp-incubator/s3-operator + newName: s3-operator + newTag: latest From ed903cfa8bc744cb2cf12cb3447c64b39bc6c865 Mon Sep 17 00:00:00 2001 From: Hamed Karbasi Date: Sat, 4 Nov 2023 13:55:16 +0330 Subject: [PATCH 26/36] add optional tag marker to Access filed of SubUserBinding --- api/v1alpha1/types.go | 1 + config/manager/kustomization.yaml | 3 --- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/api/v1alpha1/types.go b/api/v1alpha1/types.go index df6a048..6088c05 100644 --- a/api/v1alpha1/types.go +++ b/api/v1alpha1/types.go @@ -17,6 +17,7 @@ type SubUserBinding struct { // +kubebuilder:validation:Required Name string `json:"name"` // access of the subuser which can be read or write + // +kubebuilder:validation:Optional // +kubebuilder:default=read // +kubebuilder:validation:Enum=read;write Access string `json:"access,omitempty"` diff --git a/config/manager/kustomization.yaml b/config/manager/kustomization.yaml index da9b1d0..1126696 100644 --- a/config/manager/kustomization.yaml +++ b/config/manager/kustomization.yaml @@ -7,6 +7,3 @@ images: - name: controller newName: controller newTag: latest -- name: ghcr.io/snapp-incubator/s3-operator - newName: s3-operator - newTag: latest From bc70bad501dccdd8e3e3e088c64bf07baf12ce49 Mon Sep 17 00:00:00 2001 From: Hamed Karbasi Date: Sat, 4 Nov 2023 14:04:24 +0330 Subject: [PATCH 27/36] rename functions to follow the verb pattern --- internal/controllers/s3userclaim/handler.go | 7 +++--- .../controllers/s3userclaim/provisioner.go | 22 +++++++++---------- 2 files changed, 14 insertions(+), 15 deletions(-) diff --git a/internal/controllers/s3userclaim/handler.go b/internal/controllers/s3userclaim/handler.go index 066c816..552f5f3 100644 --- a/internal/controllers/s3userclaim/handler.go +++ b/internal/controllers/s3userclaim/handler.go @@ -130,18 +130,17 @@ func (r *Reconciler) initVars(req ctrl.Request) { r.s3UserName = fmt.Sprintf("%s.%s", req.Namespace, req.Name) } -func cephSubUserFullIdMaker(cephUserFullId string, subUser string) string { +func generateSubUserFullId(cephUserFullId string, subUser string) string { return fmt.Sprintf("%s:%s", cephUserFullId, subUser) } -func subUserSecretNameMaker(s3UserClaimName string, subUser string) string { +func generateSubUserSecretName(s3UserClaimName string, subUser string) string { return fmt.Sprintf("%s-%s", s3UserClaimName, subUser) } -func subUserNameExtractor(cephSubUserFullId string) (string, error) { +func extractSubUserName(cephSubUserFullId string) (string, error) { parts := strings.Split(cephSubUserFullId, ":") if len(parts) == 2 { - // return the last part since it's the subUser name return parts[1], nil } else { return "", fmt.Errorf("cannot parse the cephSubUserFullId=%s", cephSubUserFullId) diff --git a/internal/controllers/s3userclaim/provisioner.go b/internal/controllers/s3userclaim/provisioner.go index 7a2985e..a3e9deb 100644 --- a/internal/controllers/s3userclaim/provisioner.go +++ b/internal/controllers/s3userclaim/provisioner.go @@ -113,13 +113,13 @@ func (r *Reconciler) ensureCephUserQuota(ctx context.Context) (*ctrl.Result, err } func (r *Reconciler) ensureOtherSubusers(ctx context.Context) (*ctrl.Result, error) { - specSubUsers := r.s3UserClaim.Spec.SubUsers + desiredSubUsers := r.s3UserClaim.Spec.SubUsers // Create a hashmap to move all spec and ceph subUsers to it subUserFullIdSet := make(map[string]string) // Tag specSubUsers with "create" - for _, subUser := range specSubUsers { - cephSubUserFullId := cephSubUserFullIdMaker(r.cephUserFullId, subUser) + for _, subUser := range desiredSubUsers { + cephSubUserFullId := generateSubUserFullId(r.cephUserFullId, subUser) subUserFullIdSet[cephSubUserFullId] = consts.SubUserTagCreate } @@ -128,12 +128,12 @@ func (r *Reconciler) ensureOtherSubusers(ctx context.Context) (*ctrl.Result, err // Tag cephSubUsers as remove if they are not already in the hashmap and remove them otherwise // since they are already available on ceph and not needed to created. - for _, cephsubUser := range r.cephUser.Subusers { - _, exists := subUserFullIdSet[cephsubUser.Name] + for _, currentSubUser := range r.cephUser.Subusers { + _, exists := subUserFullIdSet[currentSubUser.Name] if exists { - delete(subUserFullIdSet, cephsubUser.Name) + delete(subUserFullIdSet, currentSubUser.Name) } else { - subUserFullIdSet[cephsubUser.Name] = consts.SubUserTagRemove + subUserFullIdSet[currentSubUser.Name] = consts.SubUserTagRemove } } @@ -160,12 +160,12 @@ func (r *Reconciler) ensureOtherSubusers(ctx context.Context) (*ctrl.Result, err return subreconciler.Requeue() } // Extrace subUser name from the subUserFullId - subUser, err := subUserNameExtractor(subUserFullId) + subUser, err := extractSubUserName(subUserFullId) if err != nil { r.logger.Error(err, "failed to remove s3SubUserSecret") return subreconciler.Requeue() } - subUserSecretName := subUserSecretNameMaker(r.s3UserClaim.Name, subUser) + subUserSecretName := generateSubUserSecretName(r.s3UserClaim.Name, subUser) subUserSecret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Namespace: r.s3UserClaim.Namespace, @@ -218,8 +218,8 @@ func (r *Reconciler) ensureReadonlySecret(ctx context.Context) (*ctrl.Result, er func (r *Reconciler) ensureOtherSubusersSecret(ctx context.Context) (*ctrl.Result, error) { for _, subUser := range r.s3UserClaim.Spec.SubUsers { - cephSubUserFullId := cephSubUserFullIdMaker(r.cephUserFullId, subUser) - SubUserSecretName := subUserSecretNameMaker(r.s3UserClaim.Name, subUser) + cephSubUserFullId := generateSubUserFullId(r.cephUserFullId, subUser) + SubUserSecretName := generateSubUserSecretName(r.s3UserClaim.Name, subUser) assembledSecret, err := r.assembleCephUserSecret(cephSubUserFullId, SubUserSecretName) if err != nil { r.logger.Error(err, "failed to assemble other subUsers secret") From 4b272d04869cc2e1b1a1bfd41227fb86c13a1d79 Mon Sep 17 00:00:00 2001 From: Hamed Karbasi Date: Sat, 4 Nov 2023 16:02:34 +0330 Subject: [PATCH 28/36] break down syncSubusersList to smaller functions --- .../controllers/s3userclaim/provisioner.go | 134 ++++++++++-------- 1 file changed, 77 insertions(+), 57 deletions(-) diff --git a/internal/controllers/s3userclaim/provisioner.go b/internal/controllers/s3userclaim/provisioner.go index a3e9deb..41c4531 100644 --- a/internal/controllers/s3userclaim/provisioner.go +++ b/internal/controllers/s3userclaim/provisioner.go @@ -29,7 +29,7 @@ func (r *Reconciler) Provision(ctx context.Context) (ctrl.Result, error) { subrecs := []subreconciler.Fn{ r.ensureCephUser, r.ensureCephUserQuota, - r.ensureOtherSubusers, + r.syncSubusersList, // retrieve the ceph user to have keys of subuser at hand r.retrieveCephUser, r.ensureAdminSecret, @@ -112,75 +112,25 @@ func (r *Reconciler) ensureCephUserQuota(ctx context.Context) (*ctrl.Result, err } } -func (r *Reconciler) ensureOtherSubusers(ctx context.Context) (*ctrl.Result, error) { - desiredSubUsers := r.s3UserClaim.Spec.SubUsers - // Create a hashmap to move all spec and ceph subUsers to it - subUserFullIdSet := make(map[string]string) - - // Tag specSubUsers with "create" - for _, subUser := range desiredSubUsers { - cephSubUserFullId := generateSubUserFullId(r.cephUserFullId, subUser) - subUserFullIdSet[cephSubUserFullId] = consts.SubUserTagCreate - } - - // Add read-only subUser to subUsers to prevent if from removing - subUserFullIdSet[r.readonlyCephUserFullId] = consts.SubUserTagCreate +func (r *Reconciler) syncSubusersList(ctx context.Context) (*ctrl.Result, error) { - // Tag cephSubUsers as remove if they are not already in the hashmap and remove them otherwise - // since they are already available on ceph and not needed to created. - for _, currentSubUser := range r.cephUser.Subusers { - _, exists := subUserFullIdSet[currentSubUser.Name] - if exists { - delete(subUserFullIdSet, currentSubUser.Name) - } else { - subUserFullIdSet[currentSubUser.Name] = consts.SubUserTagRemove - } - } + subUserFullIdAccessMap := r.generateSubUserAccessMap(r.s3UserClaim.Spec.SubUsers, + r.cephUser.Subusers) // Iterate over the subUsers hashmap and create or remove subUsers according to their tags. - for subUserFullId, tag := range subUserFullIdSet { + for subUserFullId, tag := range subUserFullIdAccessMap { desiredSubuser := admin.SubuserSpec{ Name: subUserFullId, Access: admin.SubuserAccessNone, KeyType: pointer.String(consts.CephKeyTypeS3), } if tag == consts.SubUserTagCreate { - // Create the subuser - r.logger.Info(fmt.Sprintf("Create subUser: %s", subUserFullId)) - if err := r.rgwClient.CreateSubuser(ctx, admin.User{ID: r.cephUserFullId}, desiredSubuser); err != nil { - r.logger.Error(err, "failed to create subUser") + if err := r.generateSubUser(ctx, r.cephUserFullId, desiredSubuser); err != nil { return subreconciler.Requeue() } } else { - // Delete the subuser - err := r.rgwClient.RemoveSubuser(ctx, admin.User{ID: r.cephUserFullId}, desiredSubuser) - r.logger.Info(fmt.Sprintf("Remove subUser: %s", subUserFullId)) - if err != nil { - r.logger.Error(err, "failed to remove subUser") - return subreconciler.Requeue() - } - // Extrace subUser name from the subUserFullId - subUser, err := extractSubUserName(subUserFullId) - if err != nil { - r.logger.Error(err, "failed to remove s3SubUserSecret") - return subreconciler.Requeue() - } - subUserSecretName := generateSubUserSecretName(r.s3UserClaim.Name, subUser) - subUserSecret := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: r.s3UserClaim.Namespace, - Name: subUserSecretName, - }, - } - // Delete subUser secret - switch err := r.Delete(ctx, subUserSecret); { - case apierrors.IsNotFound(err): - return subreconciler.ContinueReconciling() - case err != nil: - r.logger.Error(err, "failed to remove s3SubUserSecret") + if err := r.RemoveSubuserAndSecret(ctx, r.cephUserFullId, desiredSubuser); err != nil { return subreconciler.Requeue() - default: - return subreconciler.ContinueReconciling() } } } @@ -385,3 +335,73 @@ func (r *Reconciler) assembleCephUserSecret(userName, secretName string) (*corev return secret, nil } + +func (r *Reconciler) generateSubUserAccessMap(desiredSubUsers []string, + currentSubUsers []admin.SubuserSpec) map[string]string { + // Create a hashmap to move all spec and ceph subUsers to it + subUserFullIdAccessMap := make(map[string]string) + + // Tag specSubUsers with "create" + for _, subUser := range desiredSubUsers { + cephSubUserFullId := generateSubUserFullId(r.cephUserFullId, subUser) + subUserFullIdAccessMap[cephSubUserFullId] = consts.SubUserTagCreate + } + + // Add read-only subUser to subUsers to prevent if from removing + subUserFullIdAccessMap[r.readonlyCephUserFullId] = consts.SubUserTagCreate + + // Tag cephSubUsers as remove if they are not already in the hashmap and remove them otherwise + // since they are already available on ceph and not needed to created. + for _, currentSubUser := range r.cephUser.Subusers { + _, exists := subUserFullIdAccessMap[currentSubUser.Name] + if exists { + delete(subUserFullIdAccessMap, currentSubUser.Name) + } else { + subUserFullIdAccessMap[currentSubUser.Name] = consts.SubUserTagRemove + } + } + return subUserFullIdAccessMap +} + +func (r *Reconciler) generateSubUser(ctx context.Context, cephUserFullId string, + desiredSubuser admin.SubuserSpec) error { + r.logger.Info(fmt.Sprintf("Create subUser: %s", desiredSubuser.Name)) + if err := r.rgwClient.CreateSubuser(ctx, admin.User{ID: cephUserFullId}, desiredSubuser); err != nil { + r.logger.Error(err, "failed to create subUser") + return err + } + return nil +} + +func (r *Reconciler) RemoveSubuserAndSecret(ctx context.Context, cephUserFullId string, + subuserToRemove admin.SubuserSpec) error { + err := r.rgwClient.RemoveSubuser(ctx, admin.User{ID: cephUserFullId}, subuserToRemove) + r.logger.Info(fmt.Sprintf("Remove subUser: %s", subuserToRemove.Name)) + if err != nil { + r.logger.Error(err, "failed to remove subUser") + return err + } + // Extrace subUser name from the subUserFullId + subUser, err := extractSubUserName(subuserToRemove.Name) + if err != nil { + r.logger.Error(err, "failed to remove s3SubUserSecret") + return err + } + // Delete subUser secret + subUserSecretName := generateSubUserSecretName(r.s3UserClaim.Name, subUser) + subUserSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: r.s3UserClaim.Namespace, + Name: subUserSecretName, + }, + } + switch err := r.Delete(ctx, subUserSecret); { + case apierrors.IsNotFound(err): + return nil + case err != nil: + r.logger.Error(err, "failed to remove s3SubUserSecret") + return err + default: + return nil + } +} From 9f4b06907e3a4442318bdae7fc131b32c46b202f Mon Sep 17 00:00:00 2001 From: Hamed Karbasi Date: Sat, 4 Nov 2023 16:15:54 +0330 Subject: [PATCH 29/36] add comment on syncSubusersList function --- config/manager/kustomization.yaml | 3 +++ internal/controllers/s3userclaim/provisioner.go | 7 +++++++ 2 files changed, 10 insertions(+) diff --git a/config/manager/kustomization.yaml b/config/manager/kustomization.yaml index 1126696..da9b1d0 100644 --- a/config/manager/kustomization.yaml +++ b/config/manager/kustomization.yaml @@ -7,3 +7,6 @@ images: - name: controller newName: controller newTag: latest +- name: ghcr.io/snapp-incubator/s3-operator + newName: s3-operator + newTag: latest diff --git a/internal/controllers/s3userclaim/provisioner.go b/internal/controllers/s3userclaim/provisioner.go index 41c4531..5637b79 100644 --- a/internal/controllers/s3userclaim/provisioner.go +++ b/internal/controllers/s3userclaim/provisioner.go @@ -112,6 +112,13 @@ func (r *Reconciler) ensureCephUserQuota(ctx context.Context) (*ctrl.Result, err } } +// syncSubusersList creates the new subusers and deletes the ones which have been removed +// from the spec. +// At first, it creates a map from subusers to a tag which can be either "create" or "remove" +// demonstrating the action that we want to be happened on the subuser: +// 1. Subusers which are in the spec list and not in the current ceph users list will be created. +// 2. Subusers which are not in the spec list but are in the current ceph users list will be removed with their secrets. +// 3. Subusers which are common in the both lists will be deleted from the map; hence, no action happens on them. func (r *Reconciler) syncSubusersList(ctx context.Context) (*ctrl.Result, error) { subUserFullIdAccessMap := r.generateSubUserAccessMap(r.s3UserClaim.Spec.SubUsers, From ff41616d3a40cc2f32c1fa40db04630d658eac28 Mon Sep 17 00:00:00 2001 From: Hamed Karbasi Date: Sat, 4 Nov 2023 16:44:30 +0330 Subject: [PATCH 30/36] use marker validation instead of webhook for subusers name in bucket --- api/v1alpha1/s3userclaim_types.go | 4 +- api/v1alpha1/s3userclaim_webhook.go | 38 +++++-------------- api/v1alpha1/types.go | 2 + api/v1alpha1/zz_generated.deepcopy.go | 4 +- .../bases/s3.snappcloud.io_s3userclaims.yaml | 2 + internal/controllers/s3userclaim/handler.go | 23 +++++------ .../controllers/s3userclaim/provisioner.go | 14 ++++++- 7 files changed, 41 insertions(+), 46 deletions(-) diff --git a/api/v1alpha1/s3userclaim_types.go b/api/v1alpha1/s3userclaim_types.go index 4ddccc3..af7937e 100644 --- a/api/v1alpha1/s3userclaim_types.go +++ b/api/v1alpha1/s3userclaim_types.go @@ -36,7 +36,7 @@ type S3UserClaimSpec struct { Quota *UserQuota `json:"quota,omitempty"` // +kubebuilder:validation:Optional - SubUsers []string `json:"subUsers,omitempty"` + SubUsers []SubUser `json:"subUsers,omitempty"` } // S3UserClaimStatus defines the observed state of S3UserClaim @@ -48,7 +48,7 @@ type S3UserClaimStatus struct { S3UserName string `json:"s3UserName,omitempty"` // +kubebuilder:validation:Optional - SubUsers []string `json:"subUsers,omitempty"` + SubUsers []SubUser `json:"subUsers,omitempty"` } // +kubebuilder:object:root=true diff --git a/api/v1alpha1/s3userclaim_webhook.go b/api/v1alpha1/s3userclaim_webhook.go index c3c01a5..e637c1c 100644 --- a/api/v1alpha1/s3userclaim_webhook.go +++ b/api/v1alpha1/s3userclaim_webhook.go @@ -20,7 +20,6 @@ import ( "context" goerrors "errors" "fmt" - "regexp" "time" openshiftquota "github.com/openshift/api/quota/v1" @@ -71,9 +70,7 @@ func (suc *S3UserClaim) ValidateCreate() error { s3userclaimlog.Info("validate create", "name", suc.Name) allErrs := field.ErrorList{} - validateQuota(suc, &allErrs) - - validateSubUsers(suc.Spec.SubUsers, &allErrs) + allErrs = validateQuota(suc, allErrs) if len(allErrs) == 0 { return nil @@ -99,11 +96,7 @@ func (suc *S3UserClaim) ValidateUpdate(old runtime.Object) error { ) } - // Validate Quota - validateQuota(suc, &allErrs) - - // Validate subUsers - validateSubUsers(suc.Spec.SubUsers, &allErrs) + allErrs = validateQuota(suc, allErrs) if len(allErrs) == 0 { return nil @@ -134,7 +127,7 @@ func (suc *S3UserClaim) ValidateDelete() error { return nil } -func validateQuota(suc *S3UserClaim, allErrsPointer *field.ErrorList) { +func validateQuota(suc *S3UserClaim, allErrs field.ErrorList) field.ErrorList { ctx, cancel := context.WithTimeout(context.Background(), ValidationTimeout) defer cancel() @@ -144,22 +137,22 @@ func validateQuota(suc *S3UserClaim, allErrsPointer *field.ErrorList) { switch err := validateAgainstNamespaceQuota(ctx, suc); { case err == consts.ErrExceededNamespaceQuota: - *allErrsPointer = append(*allErrsPointer, field.Forbidden(quotaFieldPath, err.Error())) + allErrs = append(allErrs, field.Forbidden(quotaFieldPath, err.Error())) case err != nil: s3userclaimlog.Error(err, "failed to validate against cluster quota") - *allErrsPointer = append(*allErrsPointer, field.InternalError(quotaFieldPath, fmt.Errorf(consts.ContactCloudTeamErrMessage))) + allErrs = append(allErrs, field.InternalError(quotaFieldPath, fmt.Errorf(consts.ContactCloudTeamErrMessage))) } switch err := validateAgainstClusterQuota(ctx, suc); { case err == consts.ErrExceededClusterQuota: - *allErrsPointer = append(*allErrsPointer, field.Forbidden(quotaFieldPath, err.Error())) + allErrs = append(allErrs, field.Forbidden(quotaFieldPath, err.Error())) case goerrors.Is(err, consts.ErrClusterQuotaNotDefined): - *allErrsPointer = append(*allErrsPointer, field.Forbidden(quotaFieldPath, err.Error())) + allErrs = append(allErrs, field.Forbidden(quotaFieldPath, err.Error())) case err != nil: s3userclaimlog.Error(err, "failed to validate against cluster quota") - *allErrsPointer = append(*allErrsPointer, field.InternalError(quotaFieldPath, fmt.Errorf(consts.ContactCloudTeamErrMessage))) + allErrs = append(allErrs, field.InternalError(quotaFieldPath, fmt.Errorf(consts.ContactCloudTeamErrMessage))) } - + return allErrs } func validateAgainstNamespaceQuota(ctx context.Context, suc *S3UserClaim) error { @@ -311,16 +304,3 @@ func findTeamNamespaces(ctx context.Context, team string) ([]string, error) { return namespaces, nil } - -func validateSubUsers(subUsers []string, allErrsPointer *field.ErrorList) { - // Since the subUsers name is used in their secret name, they should follow the below - // regex pattern. Otherwise, the request has to be denied. - pattern := `^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$` - regex := regexp.MustCompile(pattern) - for _, subUser := range subUsers { - if !regex.MatchString(subUser) { - errorReason := fmt.Sprintf("subuser: %s is invalid. It must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character", subUser) - *allErrsPointer = append(*allErrsPointer, field.Forbidden(field.NewPath("spec").Child("subUsers"), errorReason)) - } - } -} diff --git a/api/v1alpha1/types.go b/api/v1alpha1/types.go index 6088c05..6200b89 100644 --- a/api/v1alpha1/types.go +++ b/api/v1alpha1/types.go @@ -12,6 +12,8 @@ type UserQuota struct { MaxBuckets int `json:"maxBuckets,omitempty"` } +// +kubebuilder:validation:Pattern=^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ +type SubUser string type SubUserBinding struct { // name of the subuser // +kubebuilder:validation:Required diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 324899c..cf84046 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -221,7 +221,7 @@ func (in *S3UserClaimSpec) DeepCopyInto(out *S3UserClaimSpec) { } if in.SubUsers != nil { in, out := &in.SubUsers, &out.SubUsers - *out = make([]string, len(*in)) + *out = make([]SubUser, len(*in)) copy(*out, *in) } } @@ -246,7 +246,7 @@ func (in *S3UserClaimStatus) DeepCopyInto(out *S3UserClaimStatus) { } if in.SubUsers != nil { in, out := &in.SubUsers, &out.SubUsers - *out = make([]string, len(*in)) + *out = make([]SubUser, len(*in)) copy(*out, *in) } } diff --git a/config/crd/bases/s3.snappcloud.io_s3userclaims.yaml b/config/crd/bases/s3.snappcloud.io_s3userclaims.yaml index d1a14fc..2a14a5d 100644 --- a/config/crd/bases/s3.snappcloud.io_s3userclaims.yaml +++ b/config/crd/bases/s3.snappcloud.io_s3userclaims.yaml @@ -88,6 +88,7 @@ spec: type: string subUsers: items: + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ type: string type: array required: @@ -122,6 +123,7 @@ spec: type: string subUsers: items: + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ type: string type: array type: object diff --git a/internal/controllers/s3userclaim/handler.go b/internal/controllers/s3userclaim/handler.go index 552f5f3..fcadcc7 100644 --- a/internal/controllers/s3userclaim/handler.go +++ b/internal/controllers/s3userclaim/handler.go @@ -44,17 +44,18 @@ type Reconciler struct { rgwClient *admin.API // reconcile specific variables - clusterResourceQuota *openshiftquota.ClusterResourceQuota - s3UserClaim *s3v1alpha1.S3UserClaim - cephUser admin.User - s3UserClaimNamespace string - cephTenant string - cephUserId string - cephUserFullId string - cephDisplayName string - s3UserName string - readonlyCephUserId string - readonlyCephUserFullId string + clusterResourceQuota *openshiftquota.ClusterResourceQuota + s3UserClaim *s3v1alpha1.S3UserClaim + cephUser admin.User + s3UserClaimNamespace string + cephTenant string + cephUserId string + cephUserFullId string + cephDisplayName string + s3UserName string + readonlyCephUserId string + readonlyCephUserFullId string + desiredSubUsersStringList []string // configurations clusterName string diff --git a/internal/controllers/s3userclaim/provisioner.go b/internal/controllers/s3userclaim/provisioner.go index 5637b79..356b6a4 100644 --- a/internal/controllers/s3userclaim/provisioner.go +++ b/internal/controllers/s3userclaim/provisioner.go @@ -78,6 +78,8 @@ func (r *Reconciler) ensureCephUser(ctx context.Context) (*ctrl.Result, error) { logger.Error(err, "failed to get ceph user", "userId", desiredUser.ID) return subreconciler.Requeue() } + // retrieve desiredSubUsers as string list + r.desiredSubUsersStringList = retrieveSubUsersString(r.s3UserClaim.Spec.SubUsers) return subreconciler.ContinueReconciling() } @@ -121,7 +123,7 @@ func (r *Reconciler) ensureCephUserQuota(ctx context.Context) (*ctrl.Result, err // 3. Subusers which are common in the both lists will be deleted from the map; hence, no action happens on them. func (r *Reconciler) syncSubusersList(ctx context.Context) (*ctrl.Result, error) { - subUserFullIdAccessMap := r.generateSubUserAccessMap(r.s3UserClaim.Spec.SubUsers, + subUserFullIdAccessMap := r.generateSubUserAccessMap(r.desiredSubUsersStringList, r.cephUser.Subusers) // Iterate over the subUsers hashmap and create or remove subUsers according to their tags. @@ -174,7 +176,7 @@ func (r *Reconciler) ensureReadonlySecret(ctx context.Context) (*ctrl.Result, er } func (r *Reconciler) ensureOtherSubusersSecret(ctx context.Context) (*ctrl.Result, error) { - for _, subUser := range r.s3UserClaim.Spec.SubUsers { + for _, subUser := range r.desiredSubUsersStringList { cephSubUserFullId := generateSubUserFullId(r.cephUserFullId, subUser) SubUserSecretName := generateSubUserSecretName(r.s3UserClaim.Name, subUser) assembledSecret, err := r.assembleCephUserSecret(cephSubUserFullId, SubUserSecretName) @@ -412,3 +414,11 @@ func (r *Reconciler) RemoveSubuserAndSecret(ctx context.Context, cephUserFullId return nil } } + +func retrieveSubUsersString(desiredSubUsers []s3v1alpha1.SubUser) []string { + subUsersStringList := make([]string, len(desiredSubUsers)) + for i, desiredSubUser := range desiredSubUsers { + subUsersStringList[i] = string(desiredSubUser) + } + return subUsersStringList +} From 252d8da3d237b4040680950fa39416eb4b6cf123 Mon Sep 17 00:00:00 2001 From: Hamed Karbasi Date: Sat, 4 Nov 2023 16:55:22 +0330 Subject: [PATCH 31/36] remove map suffix --- .../controllers/s3userclaim/provisioner.go | 33 +++++++++---------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/internal/controllers/s3userclaim/provisioner.go b/internal/controllers/s3userclaim/provisioner.go index 356b6a4..14fe38e 100644 --- a/internal/controllers/s3userclaim/provisioner.go +++ b/internal/controllers/s3userclaim/provisioner.go @@ -123,11 +123,11 @@ func (r *Reconciler) ensureCephUserQuota(ctx context.Context) (*ctrl.Result, err // 3. Subusers which are common in the both lists will be deleted from the map; hence, no action happens on them. func (r *Reconciler) syncSubusersList(ctx context.Context) (*ctrl.Result, error) { - subUserFullIdAccessMap := r.generateSubUserAccessMap(r.desiredSubUsersStringList, + subUserFullIdAccess := r.generateSubUserAccess(r.desiredSubUsersStringList, r.cephUser.Subusers) - // Iterate over the subUsers hashmap and create or remove subUsers according to their tags. - for subUserFullId, tag := range subUserFullIdAccessMap { + // Iterate over the subUsers map and create or remove subUsers according to their tags. + for subUserFullId, tag := range subUserFullIdAccess { desiredSubuser := admin.SubuserSpec{ Name: subUserFullId, Access: admin.SubuserAccessNone, @@ -138,7 +138,7 @@ func (r *Reconciler) syncSubusersList(ctx context.Context) (*ctrl.Result, error) return subreconciler.Requeue() } } else { - if err := r.RemoveSubuserAndSecret(ctx, r.cephUserFullId, desiredSubuser); err != nil { + if err := r.removeSubuserAndSecret(ctx, r.cephUserFullId, desiredSubuser); err != nil { return subreconciler.Requeue() } } @@ -345,31 +345,31 @@ func (r *Reconciler) assembleCephUserSecret(userName, secretName string) (*corev return secret, nil } -func (r *Reconciler) generateSubUserAccessMap(desiredSubUsers []string, +func (r *Reconciler) generateSubUserAccess(desiredSubUsers []string, currentSubUsers []admin.SubuserSpec) map[string]string { - // Create a hashmap to move all spec and ceph subUsers to it - subUserFullIdAccessMap := make(map[string]string) + // Create a map to move all spec and ceph subUsers to it + subUserFullIdAccess := make(map[string]string) // Tag specSubUsers with "create" for _, subUser := range desiredSubUsers { cephSubUserFullId := generateSubUserFullId(r.cephUserFullId, subUser) - subUserFullIdAccessMap[cephSubUserFullId] = consts.SubUserTagCreate + subUserFullIdAccess[cephSubUserFullId] = consts.SubUserTagCreate } - // Add read-only subUser to subUsers to prevent if from removing - subUserFullIdAccessMap[r.readonlyCephUserFullId] = consts.SubUserTagCreate + // Add read-only subUser to subUsers to prevent it from removing + subUserFullIdAccess[r.readonlyCephUserFullId] = consts.SubUserTagCreate - // Tag cephSubUsers as remove if they are not already in the hashmap and remove them otherwise + // Tag cephSubUsers as remove if they are not already in the map and remove them otherwise // since they are already available on ceph and not needed to created. for _, currentSubUser := range r.cephUser.Subusers { - _, exists := subUserFullIdAccessMap[currentSubUser.Name] + _, exists := subUserFullIdAccess[currentSubUser.Name] if exists { - delete(subUserFullIdAccessMap, currentSubUser.Name) + delete(subUserFullIdAccess, currentSubUser.Name) } else { - subUserFullIdAccessMap[currentSubUser.Name] = consts.SubUserTagRemove + subUserFullIdAccess[currentSubUser.Name] = consts.SubUserTagRemove } } - return subUserFullIdAccessMap + return subUserFullIdAccess } func (r *Reconciler) generateSubUser(ctx context.Context, cephUserFullId string, @@ -382,7 +382,7 @@ func (r *Reconciler) generateSubUser(ctx context.Context, cephUserFullId string, return nil } -func (r *Reconciler) RemoveSubuserAndSecret(ctx context.Context, cephUserFullId string, +func (r *Reconciler) removeSubuserAndSecret(ctx context.Context, cephUserFullId string, subuserToRemove admin.SubuserSpec) error { err := r.rgwClient.RemoveSubuser(ctx, admin.User{ID: cephUserFullId}, subuserToRemove) r.logger.Info(fmt.Sprintf("Remove subUser: %s", subuserToRemove.Name)) @@ -390,7 +390,6 @@ func (r *Reconciler) RemoveSubuserAndSecret(ctx context.Context, cephUserFullId r.logger.Error(err, "failed to remove subUser") return err } - // Extrace subUser name from the subUserFullId subUser, err := extractSubUserName(subuserToRemove.Name) if err != nil { r.logger.Error(err, "failed to remove s3SubUserSecret") From 36842fc8d430e0e8ae969cff42d4c03ad6010ac4 Mon Sep 17 00:00:00 2001 From: Hamed Karbasi Date: Sat, 4 Nov 2023 17:20:14 +0330 Subject: [PATCH 32/36] implement bucketAccessAction as a function instead of an attribute --- internal/controllers/s3bucket/handler.go | 14 ----------- .../controllers/s3userclaim/provisioner.go | 2 +- internal/s3_agent/s3_agent.go | 23 ++++++++++++++++--- 3 files changed, 21 insertions(+), 18 deletions(-) diff --git a/internal/controllers/s3bucket/handler.go b/internal/controllers/s3bucket/handler.go index 233f24d..463409a 100644 --- a/internal/controllers/s3bucket/handler.go +++ b/internal/controllers/s3bucket/handler.go @@ -112,20 +112,6 @@ func (r *Reconciler) setS3Agent(ctx context.Context, req ctrl.Request) error { return err } - readActions := []string{ - "s3:ListBucket", - "s3:GetObject", - } - writeActions := []string{ - "s3:DeleteObject", - "s3:PutObject", - } - - // TODO: Move this to operator level instead of reconciler level - r.s3Agent.BucketAccessAction = map[string][]string{ - consts.BucketAccessRead: readActions, - consts.BucketAccessWrite: append(readActions, writeActions...), - } return nil } diff --git a/internal/controllers/s3userclaim/provisioner.go b/internal/controllers/s3userclaim/provisioner.go index 14fe38e..6e02580 100644 --- a/internal/controllers/s3userclaim/provisioner.go +++ b/internal/controllers/s3userclaim/provisioner.go @@ -356,7 +356,7 @@ func (r *Reconciler) generateSubUserAccess(desiredSubUsers []string, subUserFullIdAccess[cephSubUserFullId] = consts.SubUserTagCreate } - // Add read-only subUser to subUsers to prevent it from removing + // Add read-only subUser to subUsers to prevent removing it subUserFullIdAccess[r.readonlyCephUserFullId] = consts.SubUserTagCreate // Tag cephSubUsers as remove if they are not already in the map and remove them otherwise diff --git a/internal/s3_agent/s3_agent.go b/internal/s3_agent/s3_agent.go index 7e17c64..f1b6d8f 100644 --- a/internal/s3_agent/s3_agent.go +++ b/internal/s3_agent/s3_agent.go @@ -11,12 +11,12 @@ import ( "github.com/aws/aws-sdk-go/aws/credentials" "github.com/aws/aws-sdk-go/aws/session" "github.com/aws/aws-sdk-go/service/s3" + "github.com/snapp-incubator/s3-operator/pkg/consts" ) // S3Agent wraps the s3.S3 structure to allow for wrapper methods type S3Agent struct { - Client *s3.S3 - BucketAccessAction map[string][]string + Client *s3.S3 } func NewS3Agent(accessKey, secretKey, endpoint string, debug bool) (*S3Agent, error) { @@ -108,7 +108,8 @@ func (s *S3Agent) SetBucketPolicy(subUserAccessMap map[string]string, tenant str principal["AWS"] = AWS_iam statement["Principal"] = principal - if actions, exists := s.BucketAccessAction[access]; exists { + BucketAccessAction := generateBucketAccessAction() + if actions, exists := BucketAccessAction[access]; exists { statement["Action"] = actions } else { return fmt.Errorf("the access %s doesn't exists", access) @@ -130,3 +131,19 @@ func (s *S3Agent) SetBucketPolicy(subUserAccessMap map[string]string, tenant str } return nil } + +func generateBucketAccessAction() map[string][]string { + readActions := []string{ + "s3:ListBucket", + "s3:GetObject", + } + writeActions := []string{ + "s3:DeleteObject", + "s3:PutObject", + } + + return map[string][]string{ + consts.BucketAccessRead: readActions, + consts.BucketAccessWrite: append(readActions, writeActions...), + } +} From 26deb516a62b13d6c0b0f9c27c66a7b96ccca3d8 Mon Sep 17 00:00:00 2001 From: Hamed Karbasi Date: Sat, 4 Nov 2023 17:23:10 +0330 Subject: [PATCH 33/36] fix linting --- internal/s3_agent/s3_agent.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/s3_agent/s3_agent.go b/internal/s3_agent/s3_agent.go index f1b6d8f..06a34bb 100644 --- a/internal/s3_agent/s3_agent.go +++ b/internal/s3_agent/s3_agent.go @@ -11,6 +11,7 @@ import ( "github.com/aws/aws-sdk-go/aws/credentials" "github.com/aws/aws-sdk-go/aws/session" "github.com/aws/aws-sdk-go/service/s3" + "github.com/snapp-incubator/s3-operator/pkg/consts" ) From 3a6c1a5de39e15db63c514d62fac73ca92badce7 Mon Sep 17 00:00:00 2001 From: Hamed Karbasi Date: Sat, 4 Nov 2023 17:33:34 +0330 Subject: [PATCH 34/36] place the remove subUser log before its action --- internal/controllers/s3userclaim/provisioner.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/controllers/s3userclaim/provisioner.go b/internal/controllers/s3userclaim/provisioner.go index 6e02580..cc86b4a 100644 --- a/internal/controllers/s3userclaim/provisioner.go +++ b/internal/controllers/s3userclaim/provisioner.go @@ -384,9 +384,9 @@ func (r *Reconciler) generateSubUser(ctx context.Context, cephUserFullId string, func (r *Reconciler) removeSubuserAndSecret(ctx context.Context, cephUserFullId string, subuserToRemove admin.SubuserSpec) error { - err := r.rgwClient.RemoveSubuser(ctx, admin.User{ID: cephUserFullId}, subuserToRemove) r.logger.Info(fmt.Sprintf("Remove subUser: %s", subuserToRemove.Name)) - if err != nil { + if err := r.rgwClient.RemoveSubuser(ctx, admin.User{ID: cephUserFullId}, + subuserToRemove); err != nil { r.logger.Error(err, "failed to remove subUser") return err } From 7aa4f6e47f0967e77c213cc010699135af5a8e94 Mon Sep 17 00:00:00 2001 From: Hamed Karbasi Date: Sat, 4 Nov 2023 18:18:12 +0330 Subject: [PATCH 35/36] fix capital case letter of BucketAccessAction --- internal/s3_agent/s3_agent.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/s3_agent/s3_agent.go b/internal/s3_agent/s3_agent.go index 06a34bb..58eeed5 100644 --- a/internal/s3_agent/s3_agent.go +++ b/internal/s3_agent/s3_agent.go @@ -109,8 +109,8 @@ func (s *S3Agent) SetBucketPolicy(subUserAccessMap map[string]string, tenant str principal["AWS"] = AWS_iam statement["Principal"] = principal - BucketAccessAction := generateBucketAccessAction() - if actions, exists := BucketAccessAction[access]; exists { + bucketAccessAction := generateBucketAccessAction() + if actions, exists := bucketAccessAction[access]; exists { statement["Action"] = actions } else { return fmt.Errorf("the access %s doesn't exists", access) From 9b648c705f0ce5ea2a3dfebaa6000f5af6c8d6e4 Mon Sep 17 00:00:00 2001 From: Hamed Karbasi Date: Sun, 5 Nov 2023 11:50:27 +0330 Subject: [PATCH 36/36] convert subUser form to subuser --- api/v1alpha1/s3bucket_types.go | 4 +- api/v1alpha1/s3userclaim_types.go | 4 +- api/v1alpha1/types.go | 4 +- api/v1alpha1/zz_generated.deepcopy.go | 32 +++---- .../crd/bases/s3.snappcloud.io_s3buckets.yaml | 4 +- .../bases/s3.snappcloud.io_s3userclaims.yaml | 4 +- docs/E2E-TEST-WORKFLOW.md | 6 +- internal/controllers/s3bucket/handler.go | 8 +- internal/controllers/s3bucket/provisioner.go | 4 +- internal/controllers/s3userclaim/handler.go | 16 ++-- .../controllers/s3userclaim/provisioner.go | 96 +++++++++---------- internal/s3_agent/s3_agent.go | 6 +- pkg/consts/consts.go | 4 +- testing/e2e/05-add-subuser.yaml | 2 +- testing/e2e/05-assert.yaml | 2 +- testing/e2e/07-delete-subuser.yaml | 2 +- testing/e2e/bucket-with-subuser-access.yaml | 2 +- 17 files changed, 100 insertions(+), 100 deletions(-) diff --git a/api/v1alpha1/s3bucket_types.go b/api/v1alpha1/s3bucket_types.go index a046f6f..ad9af9c 100644 --- a/api/v1alpha1/s3bucket_types.go +++ b/api/v1alpha1/s3bucket_types.go @@ -31,7 +31,7 @@ type S3BucketSpec struct { S3DeletionPolicy string `json:"s3DeletionPolicy,omitempty"` // +kubebuilder:validation:Optional - S3SubUserBinding []SubUserBinding `json:"s3SubUserBinding,omitempty"` + S3SubuserBinding []SubuserBinding `json:"s3SubuserBinding,omitempty"` } // S3BucketStatus defines the observed state of S3Bucket @@ -44,7 +44,7 @@ type S3BucketStatus struct { Reason string `json:"reason,omitempty"` // +kubebuilder:validation:Optional - S3SubUserBinding []SubUserBinding `json:"s3SubUserBinding,omitempty"` + S3SubuserBinding []SubuserBinding `json:"s3SubuserBinding,omitempty"` } //+kubebuilder:object:root=true diff --git a/api/v1alpha1/s3userclaim_types.go b/api/v1alpha1/s3userclaim_types.go index af7937e..c9e0071 100644 --- a/api/v1alpha1/s3userclaim_types.go +++ b/api/v1alpha1/s3userclaim_types.go @@ -36,7 +36,7 @@ type S3UserClaimSpec struct { Quota *UserQuota `json:"quota,omitempty"` // +kubebuilder:validation:Optional - SubUsers []SubUser `json:"subUsers,omitempty"` + Subusers []Subuser `json:"subusers,omitempty"` } // S3UserClaimStatus defines the observed state of S3UserClaim @@ -48,7 +48,7 @@ type S3UserClaimStatus struct { S3UserName string `json:"s3UserName,omitempty"` // +kubebuilder:validation:Optional - SubUsers []SubUser `json:"subUsers,omitempty"` + Subusers []Subuser `json:"subusers,omitempty"` } // +kubebuilder:object:root=true diff --git a/api/v1alpha1/types.go b/api/v1alpha1/types.go index 6200b89..f83e732 100644 --- a/api/v1alpha1/types.go +++ b/api/v1alpha1/types.go @@ -13,8 +13,8 @@ type UserQuota struct { } // +kubebuilder:validation:Pattern=^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ -type SubUser string -type SubUserBinding struct { +type Subuser string +type SubuserBinding struct { // name of the subuser // +kubebuilder:validation:Required Name string `json:"name"` diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index cf84046..13d072a 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -88,9 +88,9 @@ func (in *S3BucketList) DeepCopyObject() runtime.Object { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *S3BucketSpec) DeepCopyInto(out *S3BucketSpec) { *out = *in - if in.S3SubUserBinding != nil { - in, out := &in.S3SubUserBinding, &out.S3SubUserBinding - *out = make([]SubUserBinding, len(*in)) + if in.S3SubuserBinding != nil { + in, out := &in.S3SubuserBinding, &out.S3SubuserBinding + *out = make([]SubuserBinding, len(*in)) copy(*out, *in) } } @@ -108,9 +108,9 @@ func (in *S3BucketSpec) DeepCopy() *S3BucketSpec { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *S3BucketStatus) DeepCopyInto(out *S3BucketStatus) { *out = *in - if in.S3SubUserBinding != nil { - in, out := &in.S3SubUserBinding, &out.S3SubUserBinding - *out = make([]SubUserBinding, len(*in)) + if in.S3SubuserBinding != nil { + in, out := &in.S3SubuserBinding, &out.S3SubuserBinding + *out = make([]SubuserBinding, len(*in)) copy(*out, *in) } } @@ -219,9 +219,9 @@ func (in *S3UserClaimSpec) DeepCopyInto(out *S3UserClaimSpec) { *out = new(UserQuota) (*in).DeepCopyInto(*out) } - if in.SubUsers != nil { - in, out := &in.SubUsers, &out.SubUsers - *out = make([]SubUser, len(*in)) + if in.Subusers != nil { + in, out := &in.Subusers, &out.Subusers + *out = make([]Subuser, len(*in)) copy(*out, *in) } } @@ -244,9 +244,9 @@ func (in *S3UserClaimStatus) DeepCopyInto(out *S3UserClaimStatus) { *out = new(UserQuota) (*in).DeepCopyInto(*out) } - if in.SubUsers != nil { - in, out := &in.SubUsers, &out.SubUsers - *out = make([]SubUser, len(*in)) + if in.Subusers != nil { + in, out := &in.Subusers, &out.Subusers + *out = make([]Subuser, len(*in)) copy(*out, *in) } } @@ -334,16 +334,16 @@ func (in *S3UserStatus) DeepCopy() *S3UserStatus { } // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *SubUserBinding) DeepCopyInto(out *SubUserBinding) { +func (in *SubuserBinding) DeepCopyInto(out *SubuserBinding) { *out = *in } -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new SubUserBinding. -func (in *SubUserBinding) DeepCopy() *SubUserBinding { +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new SubuserBinding. +func (in *SubuserBinding) DeepCopy() *SubuserBinding { if in == nil { return nil } - out := new(SubUserBinding) + out := new(SubuserBinding) in.DeepCopyInto(out) return out } diff --git a/config/crd/bases/s3.snappcloud.io_s3buckets.yaml b/config/crd/bases/s3.snappcloud.io_s3buckets.yaml index dc8e11f..0f14ff4 100644 --- a/config/crd/bases/s3.snappcloud.io_s3buckets.yaml +++ b/config/crd/bases/s3.snappcloud.io_s3buckets.yaml @@ -47,7 +47,7 @@ spec: - delete - retain type: string - s3SubUserBinding: + s3SubuserBinding: items: properties: access: @@ -77,7 +77,7 @@ spec: type: boolean reason: type: string - s3SubUserBinding: + s3SubuserBinding: items: properties: access: diff --git a/config/crd/bases/s3.snappcloud.io_s3userclaims.yaml b/config/crd/bases/s3.snappcloud.io_s3userclaims.yaml index 2a14a5d..99513ca 100644 --- a/config/crd/bases/s3.snappcloud.io_s3userclaims.yaml +++ b/config/crd/bases/s3.snappcloud.io_s3userclaims.yaml @@ -86,7 +86,7 @@ spec: type: string s3UserClass: type: string - subUsers: + subusers: items: pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ type: string @@ -121,7 +121,7 @@ spec: type: object s3UserName: type: string - subUsers: + subusers: items: pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ type: string diff --git a/docs/E2E-TEST-WORKFLOW.md b/docs/E2E-TEST-WORKFLOW.md index 836a1d0..9de3dea 100644 --- a/docs/E2E-TEST-WORKFLOW.md +++ b/docs/E2E-TEST-WORKFLOW.md @@ -21,9 +21,9 @@ Here is the test workflow: | 4 | Webhook validation: Update S3Bucket S3UserRef | Must be **denied** by the bucket validation update webhook. | | 4 | Webhook validation: Create S3bucket with wrong S3UserRef | Must be **denied** by the bucket validation create webhook. | | 4 | Webhook validation: Delete S3UserClaim | Must be **denied** by the user validation delete Webhook. | -| 5 | Add subUser | Check if subUser is added to the s3Userclaim status.
Check if subUsers secrets are created. | -| 6 | Add Bucket Access | Check if subUsers have correct access to the bucket. | -| 7 | Delete subUser | Check if deleted subUser doesn't have access to the bucket. | +| 5 | Add subuser | Check if subuser is added to the s3Userclaim status.
Check if subusers secrets are created. | +| 6 | Add Bucket Access | Check if subusers have correct access to the bucket. | +| 7 | Delete subuser | Check if deleted subuser doesn't have access to the bucket. | | 8 | Delete S3Buckets | DeletionPolicy on delete: bucket must **be** deleted.
DeletionPolicy on retain: bucket must **not be** deleted. | | 9 | Delete S3UserClaim for Sample user | S3UserClaim and S3User CRs are deleted. | | 10 | Delete S3bucket and S3UserClaim for Extra user | S3bucket, S3UserClaim and S3User CRs are deleted. | diff --git a/internal/controllers/s3bucket/handler.go b/internal/controllers/s3bucket/handler.go index 463409a..63d4202 100644 --- a/internal/controllers/s3bucket/handler.go +++ b/internal/controllers/s3bucket/handler.go @@ -37,7 +37,7 @@ type Reconciler struct { clusterName string cephTenant string cephUserFullId string - subUserAccessMap map[string]string + subuserAccessMap map[string]string } func NewReconciler(mgr manager.Manager, cfg *config.Config) *Reconciler { @@ -124,9 +124,9 @@ func (r *Reconciler) initVars(req ctrl.Request) { r.cephTenant = fmt.Sprintf("%s__%s", clusterName, namespace) r.cephUserFullId = fmt.Sprintf("%s$%s", r.cephTenant, r.s3UserRef) - r.subUserAccessMap = make(map[string]string) - for _, binding := range r.s3Bucket.Spec.S3SubUserBinding { - r.subUserAccessMap[binding.Name] = binding.Access + r.subuserAccessMap = make(map[string]string) + for _, binding := range r.s3Bucket.Spec.S3SubuserBinding { + r.subuserAccessMap[binding.Name] = binding.Access } } diff --git a/internal/controllers/s3bucket/provisioner.go b/internal/controllers/s3bucket/provisioner.go index bbfee16..49c0da7 100644 --- a/internal/controllers/s3bucket/provisioner.go +++ b/internal/controllers/s3bucket/provisioner.go @@ -43,7 +43,7 @@ func (r *Reconciler) ensureBucket(ctx context.Context) (*ctrl.Result, error) { } func (r *Reconciler) ensureBucketPolicy(ctx context.Context) (*ctrl.Result, error) { - err := r.s3Agent.SetBucketPolicy(r.subUserAccessMap, + err := r.s3Agent.SetBucketPolicy(r.subuserAccessMap, r.cephTenant, r.s3UserRef, r.s3BucketName) if err != nil { r.logger.Error(err, "failed to set the bucket policy") @@ -61,7 +61,7 @@ func (r *Reconciler) updateBucketStatus(ctx context.Context, status := s3v1alpha1.S3BucketStatus{ Ready: ready, Reason: reason, - S3SubUserBinding: r.s3Bucket.Spec.S3SubUserBinding, + S3SubuserBinding: r.s3Bucket.Spec.S3SubuserBinding, } if !apiequality.Semantic.DeepEqual(r.s3Bucket.Status, status) { diff --git a/internal/controllers/s3userclaim/handler.go b/internal/controllers/s3userclaim/handler.go index fcadcc7..205e3bb 100644 --- a/internal/controllers/s3userclaim/handler.go +++ b/internal/controllers/s3userclaim/handler.go @@ -55,7 +55,7 @@ type Reconciler struct { s3UserName string readonlyCephUserId string readonlyCephUserFullId string - desiredSubUsersStringList []string + desiredSubusersStringList []string // configurations clusterName string @@ -131,19 +131,19 @@ func (r *Reconciler) initVars(req ctrl.Request) { r.s3UserName = fmt.Sprintf("%s.%s", req.Namespace, req.Name) } -func generateSubUserFullId(cephUserFullId string, subUser string) string { - return fmt.Sprintf("%s:%s", cephUserFullId, subUser) +func generateSubuserFullId(cephUserFullId string, subuser string) string { + return fmt.Sprintf("%s:%s", cephUserFullId, subuser) } -func generateSubUserSecretName(s3UserClaimName string, subUser string) string { - return fmt.Sprintf("%s-%s", s3UserClaimName, subUser) +func generateSubuserSecretName(s3UserClaimName string, subuser string) string { + return fmt.Sprintf("%s-%s", s3UserClaimName, subuser) } -func extractSubUserName(cephSubUserFullId string) (string, error) { - parts := strings.Split(cephSubUserFullId, ":") +func extractSubuserName(cephSubuserFullId string) (string, error) { + parts := strings.Split(cephSubuserFullId, ":") if len(parts) == 2 { return parts[1], nil } else { - return "", fmt.Errorf("cannot parse the cephSubUserFullId=%s", cephSubUserFullId) + return "", fmt.Errorf("cannot parse the cephSubuserFullId=%s", cephSubuserFullId) } } diff --git a/internal/controllers/s3userclaim/provisioner.go b/internal/controllers/s3userclaim/provisioner.go index cc86b4a..12933dd 100644 --- a/internal/controllers/s3userclaim/provisioner.go +++ b/internal/controllers/s3userclaim/provisioner.go @@ -78,8 +78,8 @@ func (r *Reconciler) ensureCephUser(ctx context.Context) (*ctrl.Result, error) { logger.Error(err, "failed to get ceph user", "userId", desiredUser.ID) return subreconciler.Requeue() } - // retrieve desiredSubUsers as string list - r.desiredSubUsersStringList = retrieveSubUsersString(r.s3UserClaim.Spec.SubUsers) + // retrieve desiredSubusers as string list + r.desiredSubusersStringList = retrieveSubusersString(r.s3UserClaim.Spec.Subusers) return subreconciler.ContinueReconciling() } @@ -123,18 +123,18 @@ func (r *Reconciler) ensureCephUserQuota(ctx context.Context) (*ctrl.Result, err // 3. Subusers which are common in the both lists will be deleted from the map; hence, no action happens on them. func (r *Reconciler) syncSubusersList(ctx context.Context) (*ctrl.Result, error) { - subUserFullIdAccess := r.generateSubUserAccess(r.desiredSubUsersStringList, + subuserFullIdAccess := r.generateSubuserAccess(r.desiredSubusersStringList, r.cephUser.Subusers) - // Iterate over the subUsers map and create or remove subUsers according to their tags. - for subUserFullId, tag := range subUserFullIdAccess { + // Iterate over the subusers map and create or remove subusers according to their tags. + for subuserFullId, tag := range subuserFullIdAccess { desiredSubuser := admin.SubuserSpec{ - Name: subUserFullId, + Name: subuserFullId, Access: admin.SubuserAccessNone, KeyType: pointer.String(consts.CephKeyTypeS3), } - if tag == consts.SubUserTagCreate { - if err := r.generateSubUser(ctx, r.cephUserFullId, desiredSubuser); err != nil { + if tag == consts.SubuserTagCreate { + if err := r.generateSubuser(ctx, r.cephUserFullId, desiredSubuser); err != nil { return subreconciler.Requeue() } } else { @@ -176,12 +176,12 @@ func (r *Reconciler) ensureReadonlySecret(ctx context.Context) (*ctrl.Result, er } func (r *Reconciler) ensureOtherSubusersSecret(ctx context.Context) (*ctrl.Result, error) { - for _, subUser := range r.desiredSubUsersStringList { - cephSubUserFullId := generateSubUserFullId(r.cephUserFullId, subUser) - SubUserSecretName := generateSubUserSecretName(r.s3UserClaim.Name, subUser) - assembledSecret, err := r.assembleCephUserSecret(cephSubUserFullId, SubUserSecretName) + for _, subuser := range r.desiredSubusersStringList { + cephSubuserFullId := generateSubuserFullId(r.cephUserFullId, subuser) + SubuserSecretName := generateSubuserSecretName(r.s3UserClaim.Name, subuser) + assembledSecret, err := r.assembleCephUserSecret(cephSubuserFullId, SubuserSecretName) if err != nil { - r.logger.Error(err, "failed to assemble other subUsers secret") + r.logger.Error(err, "failed to assemble other subusers secret") return subreconciler.Requeue() } result, err := r.ensureSecret(ctx, assembledSecret) @@ -231,7 +231,7 @@ func (r *Reconciler) updateS3UserClaimStatus(ctx context.Context) (*ctrl.Result, status := s3v1alpha1.S3UserClaimStatus{ Quota: r.s3UserClaim.Spec.Quota, S3UserName: r.s3UserName, - SubUsers: r.s3UserClaim.Spec.SubUsers, + Subusers: r.s3UserClaim.Spec.Subusers, } if !apiequality.Semantic.DeepEqual(r.s3UserClaim.Status, status) { @@ -345,38 +345,38 @@ func (r *Reconciler) assembleCephUserSecret(userName, secretName string) (*corev return secret, nil } -func (r *Reconciler) generateSubUserAccess(desiredSubUsers []string, - currentSubUsers []admin.SubuserSpec) map[string]string { - // Create a map to move all spec and ceph subUsers to it - subUserFullIdAccess := make(map[string]string) +func (r *Reconciler) generateSubuserAccess(desiredSubusers []string, + currentSubusers []admin.SubuserSpec) map[string]string { + // Create a map to move all spec and ceph subusers to it + subuserFullIdAccess := make(map[string]string) - // Tag specSubUsers with "create" - for _, subUser := range desiredSubUsers { - cephSubUserFullId := generateSubUserFullId(r.cephUserFullId, subUser) - subUserFullIdAccess[cephSubUserFullId] = consts.SubUserTagCreate + // Tag specSubusers with "create" + for _, subuser := range desiredSubusers { + cephSubuserFullId := generateSubuserFullId(r.cephUserFullId, subuser) + subuserFullIdAccess[cephSubuserFullId] = consts.SubuserTagCreate } - // Add read-only subUser to subUsers to prevent removing it - subUserFullIdAccess[r.readonlyCephUserFullId] = consts.SubUserTagCreate + // Add read-only subuser to subusers to prevent removing it + subuserFullIdAccess[r.readonlyCephUserFullId] = consts.SubuserTagCreate - // Tag cephSubUsers as remove if they are not already in the map and remove them otherwise + // Tag cephSubusers as remove if they are not already in the map and remove them otherwise // since they are already available on ceph and not needed to created. - for _, currentSubUser := range r.cephUser.Subusers { - _, exists := subUserFullIdAccess[currentSubUser.Name] + for _, currentSubuser := range r.cephUser.Subusers { + _, exists := subuserFullIdAccess[currentSubuser.Name] if exists { - delete(subUserFullIdAccess, currentSubUser.Name) + delete(subuserFullIdAccess, currentSubuser.Name) } else { - subUserFullIdAccess[currentSubUser.Name] = consts.SubUserTagRemove + subuserFullIdAccess[currentSubuser.Name] = consts.SubuserTagRemove } } - return subUserFullIdAccess + return subuserFullIdAccess } -func (r *Reconciler) generateSubUser(ctx context.Context, cephUserFullId string, +func (r *Reconciler) generateSubuser(ctx context.Context, cephUserFullId string, desiredSubuser admin.SubuserSpec) error { - r.logger.Info(fmt.Sprintf("Create subUser: %s", desiredSubuser.Name)) + r.logger.Info(fmt.Sprintf("Create subuser: %s", desiredSubuser.Name)) if err := r.rgwClient.CreateSubuser(ctx, admin.User{ID: cephUserFullId}, desiredSubuser); err != nil { - r.logger.Error(err, "failed to create subUser") + r.logger.Error(err, "failed to create subuser") return err } return nil @@ -384,40 +384,40 @@ func (r *Reconciler) generateSubUser(ctx context.Context, cephUserFullId string, func (r *Reconciler) removeSubuserAndSecret(ctx context.Context, cephUserFullId string, subuserToRemove admin.SubuserSpec) error { - r.logger.Info(fmt.Sprintf("Remove subUser: %s", subuserToRemove.Name)) + r.logger.Info(fmt.Sprintf("Remove subuser: %s", subuserToRemove.Name)) if err := r.rgwClient.RemoveSubuser(ctx, admin.User{ID: cephUserFullId}, subuserToRemove); err != nil { - r.logger.Error(err, "failed to remove subUser") + r.logger.Error(err, "failed to remove subuser") return err } - subUser, err := extractSubUserName(subuserToRemove.Name) + subuser, err := extractSubuserName(subuserToRemove.Name) if err != nil { - r.logger.Error(err, "failed to remove s3SubUserSecret") + r.logger.Error(err, "failed to remove s3SubuserSecret") return err } - // Delete subUser secret - subUserSecretName := generateSubUserSecretName(r.s3UserClaim.Name, subUser) - subUserSecret := &corev1.Secret{ + // Delete subuser secret + subuserSecretName := generateSubuserSecretName(r.s3UserClaim.Name, subuser) + subuserSecret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Namespace: r.s3UserClaim.Namespace, - Name: subUserSecretName, + Name: subuserSecretName, }, } - switch err := r.Delete(ctx, subUserSecret); { + switch err := r.Delete(ctx, subuserSecret); { case apierrors.IsNotFound(err): return nil case err != nil: - r.logger.Error(err, "failed to remove s3SubUserSecret") + r.logger.Error(err, "failed to remove s3SubuserSecret") return err default: return nil } } -func retrieveSubUsersString(desiredSubUsers []s3v1alpha1.SubUser) []string { - subUsersStringList := make([]string, len(desiredSubUsers)) - for i, desiredSubUser := range desiredSubUsers { - subUsersStringList[i] = string(desiredSubUser) +func retrieveSubusersString(desiredSubusers []s3v1alpha1.Subuser) []string { + subusersStringList := make([]string, len(desiredSubusers)) + for i, desiredSubuser := range desiredSubusers { + subusersStringList[i] = string(desiredSubuser) } - return subUsersStringList + return subusersStringList } diff --git a/internal/s3_agent/s3_agent.go b/internal/s3_agent/s3_agent.go index 58eeed5..161a0f1 100644 --- a/internal/s3_agent/s3_agent.go +++ b/internal/s3_agent/s3_agent.go @@ -78,7 +78,7 @@ func (s *S3Agent) DeleteBucket(name string) error { return err } -func (s *S3Agent) SetBucketPolicy(subUserAccessMap map[string]string, tenant string, +func (s *S3Agent) SetBucketPolicy(subuserAccessMap map[string]string, tenant string, owner string, bucket string) error { // The map of access levels to the AWS IAM names slice accessAWSIAMMap := make(map[string][]string) @@ -87,9 +87,9 @@ func (s *S3Agent) SetBucketPolicy(subUserAccessMap map[string]string, tenant str "Id": "S3Policy", } statementSlice := []map[string]interface{}{} - for subUser, access := range subUserAccessMap { + for subuser, access := range subuserAccessMap { // Create AWS IAM Name needed for the policy from the subuser name - aws_iam := fmt.Sprintf("arn:aws:iam::%s:user/%s:%s", tenant, owner, subUser) + aws_iam := fmt.Sprintf("arn:aws:iam::%s:user/%s:%s", tenant, owner, subuser) accessAWSIAMMap[access] = append(accessAWSIAMMap[access], aws_iam) } diff --git a/pkg/consts/consts.go b/pkg/consts/consts.go index bcf5d54..9d70dfb 100644 --- a/pkg/consts/consts.go +++ b/pkg/consts/consts.go @@ -35,8 +35,8 @@ const ( DeletionPolicyDelete = "delete" DeletionPolicyRetain = "retain" - SubUserTagCreate = "create" - SubUserTagRemove = "remove" + SubuserTagCreate = "create" + SubuserTagRemove = "remove" // Bucket Access Levels BucketAccessRead = "read" diff --git a/testing/e2e/05-add-subuser.yaml b/testing/e2e/05-add-subuser.yaml index 97c0d26..daba7de 100644 --- a/testing/e2e/05-add-subuser.yaml +++ b/testing/e2e/05-add-subuser.yaml @@ -11,6 +11,6 @@ spec: maxSize: 10000 maxObjects: 500 maxBuckets: 5 - subUsers: + subusers: - subuser1 - subuser2 \ No newline at end of file diff --git a/testing/e2e/05-assert.yaml b/testing/e2e/05-assert.yaml index 07b4e84..eafccbd 100644 --- a/testing/e2e/05-assert.yaml +++ b/testing/e2e/05-assert.yaml @@ -26,7 +26,7 @@ status: maxSize: 10k maxObjects: "500" maxBuckets: 5 - subUsers: + subusers: - subuser1 - subuser2 --- diff --git a/testing/e2e/07-delete-subuser.yaml b/testing/e2e/07-delete-subuser.yaml index 229a46d..a19cb5f 100644 --- a/testing/e2e/07-delete-subuser.yaml +++ b/testing/e2e/07-delete-subuser.yaml @@ -11,5 +11,5 @@ spec: maxSize: 10000 maxObjects: 500 maxBuckets: 5 - subUsers: + subusers: - subuser2 \ No newline at end of file diff --git a/testing/e2e/bucket-with-subuser-access.yaml b/testing/e2e/bucket-with-subuser-access.yaml index ec50cf2..be8b5ce 100644 --- a/testing/e2e/bucket-with-subuser-access.yaml +++ b/testing/e2e/bucket-with-subuser-access.yaml @@ -6,7 +6,7 @@ metadata: spec: s3UserRef: s3userclaim-sample s3DeletionPolicy: delete - s3SubUserBinding: + s3SubuserBinding: - name: subuser1 access: write - name: subuser2