diff --git a/apis/vmware/v1beta1/providerserviceaccount_types.go b/apis/vmware/v1beta1/providerserviceaccount_types.go index 7aaff8aa65..052e53331e 100644 --- a/apis/vmware/v1beta1/providerserviceaccount_types.go +++ b/apis/vmware/v1beta1/providerserviceaccount_types.go @@ -24,7 +24,13 @@ import ( // ProviderServiceAccountSpec defines the desired state of ProviderServiceAccount. type ProviderServiceAccountSpec struct { + // ClusterName is the name of the Cluster this object belongs to. + // +optional + ClusterName *string `json:"clusterName,omitempty"` + + // Deprecated: Ref will be removed in a future release. // Ref specifies the reference to the VSphereCluster for which the ProviderServiceAccount needs to be realized. + // +optional Ref *corev1.ObjectReference `json:"ref"` // Rules specifies the privileges that need to be granted to the service account. diff --git a/apis/vmware/v1beta1/zz_generated.deepcopy.go b/apis/vmware/v1beta1/zz_generated.deepcopy.go index 68f72ea580..fa46a98a16 100644 --- a/apis/vmware/v1beta1/zz_generated.deepcopy.go +++ b/apis/vmware/v1beta1/zz_generated.deepcopy.go @@ -89,6 +89,11 @@ func (in *ProviderServiceAccountList) DeepCopyObject() runtime.Object { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *ProviderServiceAccountSpec) DeepCopyInto(out *ProviderServiceAccountSpec) { *out = *in + if in.ClusterName != nil { + in, out := &in.ClusterName, &out.ClusterName + *out = new(string) + **out = **in + } if in.Ref != nil { in, out := &in.Ref, &out.Ref *out = new(v1.ObjectReference) diff --git a/config/supervisor/crd/vmware.infrastructure.cluster.x-k8s.io_providerserviceaccounts.yaml b/config/supervisor/crd/vmware.infrastructure.cluster.x-k8s.io_providerserviceaccounts.yaml index 040c8af8e6..66004710a4 100644 --- a/config/supervisor/crd/vmware.infrastructure.cluster.x-k8s.io_providerserviceaccounts.yaml +++ b/config/supervisor/crd/vmware.infrastructure.cluster.x-k8s.io_providerserviceaccounts.yaml @@ -50,9 +50,14 @@ spec: spec: description: ProviderServiceAccountSpec defines the desired state of ProviderServiceAccount. properties: + clusterName: + description: ClusterName is the name of the Cluster this object belongs + to. + type: string ref: - description: Ref specifies the reference to the VSphereCluster for - which the ProviderServiceAccount needs to be realized. + description: 'Deprecated: Ref will be removed in a future release. + Ref specifies the reference to the VSphereCluster for which the + ProviderServiceAccount needs to be realized.' properties: apiVersion: description: API version of the referent. @@ -149,7 +154,6 @@ spec: cluster that contains the generated service account token. type: string required: - - ref - rules - targetNamespace - targetSecretName diff --git a/config/webhook/manifests.yaml b/config/webhook/manifests.yaml index 7c867e393e..915e6f6b3c 100644 --- a/config/webhook/manifests.yaml +++ b/config/webhook/manifests.yaml @@ -4,6 +4,27 @@ kind: MutatingWebhookConfiguration metadata: name: mutating-webhook-configuration webhooks: +- admissionReviewVersions: + - v1beta1 + clientConfig: + service: + name: webhook-service + namespace: system + path: /mutate-vmware-infrastructure-cluster-x-k8s-io-v1beta1-providerserviceaccount + failurePolicy: Fail + matchPolicy: Equivalent + name: default.providerserviceaccount.vmware.infrastructure.cluster.x-k8s.io + rules: + - apiGroups: + - vmware.infrastructure.cluster.x-k8s.io + apiVersions: + - v1beta1 + operations: + - CREATE + - UPDATE + resources: + - providerserviceaccounts + sideEffects: None - admissionReviewVersions: - v1beta1 clientConfig: @@ -94,6 +115,27 @@ kind: ValidatingWebhookConfiguration metadata: name: validating-webhook-configuration webhooks: +- admissionReviewVersions: + - v1beta1 + clientConfig: + service: + name: webhook-service + namespace: system + path: /validate-vmware-infrastructure-cluster-x-k8s-io-v1beta1-providerserviceaccount + failurePolicy: Fail + matchPolicy: Equivalent + name: validation.providerserviceaccount.vmware.infrastructure.x-k8s.io + rules: + - apiGroups: + - vmware.infrastructure.cluster.x-k8s.io + apiVersions: + - v1beta1 + operations: + - CREATE + - UPDATE + resources: + - providerserviceaccounts + sideEffects: None - admissionReviewVersions: - v1beta1 clientConfig: diff --git a/controllers/serviceaccount_controller.go b/controllers/serviceaccount_controller.go index 32e028b580..98438fa69e 100644 --- a/controllers/serviceaccount_controller.go +++ b/controllers/serviceaccount_controller.go @@ -582,9 +582,17 @@ func (r *ServiceAccountReconciler) getProviderServiceAccounts(ctx context.Contex if pSvcAccount.DeletionTimestamp != nil { continue } - ref := pSvcAccount.Spec.Ref - if ref != nil && ref.Name == clusterCtx.VSphereCluster.Name { - pSvcAccounts = append(pSvcAccounts, pSvcAccount) + // take ClusterName as higher precedence temporally before Ref was deprecated. + if pSvcAccount.Spec.ClusterName != nil && *pSvcAccount.Spec.ClusterName != "" { + clusterName := *pSvcAccount.Spec.ClusterName + if clusterName == clusterCtx.VSphereCluster.Name { + pSvcAccounts = append(pSvcAccounts, pSvcAccount) + } + } else if pSvcAccount.Spec.Ref != nil { //nolint:staticcheck + ref := pSvcAccount.Spec.Ref //nolint:staticcheck + if ref.Name == clusterCtx.VSphereCluster.Name { + pSvcAccounts = append(pSvcAccounts, pSvcAccount) + } } } return pSvcAccounts, nil @@ -678,11 +686,20 @@ func (r *ServiceAccountReconciler) providerServiceAccountToVSphereCluster(_ cont } func toVSphereClusterRequest(providerServiceAccount *vmwarev1.ProviderServiceAccount) []reconcile.Request { - vsphereClusterRef := providerServiceAccount.Spec.Ref - if vsphereClusterRef == nil || vsphereClusterRef.Name == "" { - return nil - } - return []reconcile.Request{ - {NamespacedName: client.ObjectKey{Namespace: providerServiceAccount.Namespace, Name: vsphereClusterRef.Name}}, + // take ClusterName as higher precedence temporally before Ref was deprecated. + if providerServiceAccount.Spec.ClusterName != nil && *providerServiceAccount.Spec.ClusterName != "" { + clusterName := *providerServiceAccount.Spec.ClusterName + return []reconcile.Request{ + {NamespacedName: client.ObjectKey{Namespace: providerServiceAccount.Namespace, Name: clusterName}}, + } + } else if providerServiceAccount.Spec.Ref != nil { //nolint:staticcheck + vsphereClusterRef := providerServiceAccount.Spec.Ref //nolint:staticcheck + if vsphereClusterRef.Name != "" { + return []reconcile.Request{ + {NamespacedName: client.ObjectKey{Namespace: providerServiceAccount.Namespace, Name: vsphereClusterRef.Name}}, + } + } } + + return nil } diff --git a/controllers/serviceaccount_controller_suite_test.go b/controllers/serviceaccount_controller_suite_test.go index 4dc971f622..e915dba5d9 100644 --- a/controllers/serviceaccount_controller_suite_test.go +++ b/controllers/serviceaccount_controller_suite_test.go @@ -221,7 +221,7 @@ func getTestProviderServiceAccount(namespace string, vSphereCluster *vmwarev1.VS Controller: &truePointer, }, } - pSvcAccount.Spec.Ref = &corev1.ObjectReference{ + pSvcAccount.Spec.Ref = &corev1.ObjectReference{ //nolint:staticcheck Name: vSphereCluster.Name, } } diff --git a/internal/test/helpers/envtest.go b/internal/test/helpers/envtest.go index 6ea9ce1605..816a514b9a 100644 --- a/internal/test/helpers/envtest.go +++ b/internal/test/helpers/envtest.go @@ -179,6 +179,10 @@ func NewTestEnvironment(ctx context.Context) *TestEnvironment { return err } + if err := (&webhooks.ProviderServiceAccountWebhook{}).SetupWebhookWithManager(mgr); err != nil { + return err + } + return (&webhooks.VSphereFailureDomainWebhook{}).SetupWebhookWithManager(mgr) } diff --git a/internal/webhooks/providerserviceaccount.go b/internal/webhooks/providerserviceaccount.go new file mode 100644 index 0000000000..af5580ed33 --- /dev/null +++ b/internal/webhooks/providerserviceaccount.go @@ -0,0 +1,98 @@ +/* +Copyright 2024 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package webhooks + +import ( + "context" + "fmt" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/validation/field" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/webhook" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + vmwarev1 "sigs.k8s.io/cluster-api-provider-vsphere/apis/vmware/v1beta1" +) + +// +kubebuilder:webhook:verbs=create;update,path=/validate-vmware-infrastructure-cluster-x-k8s-io-v1beta1-providerserviceaccount,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=vmware.infrastructure.cluster.x-k8s.io,resources=providerserviceaccounts,versions=v1beta1,name=validation.providerserviceaccount.vmware.infrastructure.x-k8s.io,sideEffects=None,admissionReviewVersions=v1beta1 +// +kubebuilder:webhook:verbs=create;update,path=/mutate-vmware-infrastructure-cluster-x-k8s-io-v1beta1-providerserviceaccount,mutating=true,failurePolicy=fail,matchPolicy=Equivalent,groups=vmware.infrastructure.cluster.x-k8s.io,resources=providerserviceaccounts,versions=v1beta1,name=default.providerserviceaccount.vmware.infrastructure.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1beta1 + +// ProviderServiceAccountWebhook implements a validation and defaulting webhook for ProviderServiceAccount. +type ProviderServiceAccountWebhook struct{} + +var _ webhook.CustomValidator = &ProviderServiceAccountWebhook{} +var _ webhook.CustomDefaulter = &ProviderServiceAccountWebhook{} + +func (webhook *ProviderServiceAccountWebhook) SetupWebhookWithManager(mgr ctrl.Manager) error { + return ctrl.NewWebhookManagedBy(mgr). + For(&vmwarev1.ProviderServiceAccount{}). + WithValidator(webhook). + WithDefaulter(webhook). + Complete() +} + +// Default implements webhook.Defaulter so a webhook will be registered for the type. +func (webhook *ProviderServiceAccountWebhook) Default(_ context.Context, _ runtime.Object) error { + return nil +} + +// ValidateCreate implements webhook.Validator so a webhook will be registered for the type. +func (webhook *ProviderServiceAccountWebhook) ValidateCreate(_ context.Context, raw runtime.Object) (admission.Warnings, error) { + var allErrs field.ErrorList + + obj, ok := raw.(*vmwarev1.ProviderServiceAccount) + if !ok { + return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a ProviderServiceAccount but got a %T", raw)) + } + + spec := obj.Spec + if spec.Ref == nil && (spec.ClusterName == nil || *spec.ClusterName == "") { //nolint:staticcheck + allErrs = append(allErrs, field.Forbidden(field.NewPath("spec"), "should specify Ref or ClusterName")) + if spec.ClusterName != nil && *spec.ClusterName == "" { + allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "ClusterName"), "ClusterName should not be empty")) + } + } + + return nil, aggregateObjErrors(obj.GroupVersionKind().GroupKind(), obj.Name, allErrs) +} + +// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type. +func (webhook *ProviderServiceAccountWebhook) ValidateUpdate(_ context.Context, _ runtime.Object, newRaw runtime.Object) (admission.Warnings, error) { + var allErrs field.ErrorList + + newTyped, ok := newRaw.(*vmwarev1.ProviderServiceAccount) + if !ok { + return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a ProviderServiceAccount but got a %T", newRaw)) + } + + spec := newTyped.Spec + if spec.Ref == nil && (spec.ClusterName == nil || *spec.ClusterName == "") { //nolint:staticcheck + allErrs = append(allErrs, field.Forbidden(field.NewPath("spec"), "should specify Ref or ClusterName")) + if spec.ClusterName != nil && *spec.ClusterName == "" { + allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "ClusterName"), "ClusterName should not be empty")) + } + } + + return nil, aggregateObjErrors(newTyped.GroupVersionKind().GroupKind(), newTyped.Name, allErrs) +} + +// ValidateDelete implements webhook.Validator so a webhook will be registered for the type. +func (webhook *ProviderServiceAccountWebhook) ValidateDelete(_ context.Context, _ runtime.Object) (admission.Warnings, error) { + return nil, nil +} diff --git a/internal/webhooks/providerserviceaccount_test.go b/internal/webhooks/providerserviceaccount_test.go new file mode 100644 index 0000000000..1bc7e988cb --- /dev/null +++ b/internal/webhooks/providerserviceaccount_test.go @@ -0,0 +1,138 @@ +/* +Copyright 2024 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package webhooks + +import ( + "context" + "testing" + + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" + + vmwarev1 "sigs.k8s.io/cluster-api-provider-vsphere/apis/vmware/v1beta1" +) + +var ( + testClusterName = "test-cluster-name" + targetNamespace = "test-target-namespace" + targetSecretName = "test-target-secret-name" +) + +func TestProviderServiceAccount_ValidateCreate(t *testing.T) { + g := NewWithT(t) + tests := []struct { + name string + providerServiceAccount *vmwarev1.ProviderServiceAccount + wantErr bool + }{ + { + name: "successful ProviderServiceAccount creation with Ref specified ", + providerServiceAccount: createProviderServiceAccount(true, false), + wantErr: false, + }, + + { + name: "successful ProviderServiceAccount creation with ClusterName specified ", + providerServiceAccount: createProviderServiceAccount(false, true), + wantErr: false, + }, + + { + name: "ProviderServiceAccount creation should specify at least Ref or ClusterName ", + providerServiceAccount: createProviderServiceAccount(false, false), + wantErr: true, + }, + { + name: "successful ProviderServiceAccount creation both Ref and ClusterName specified ", + providerServiceAccount: createProviderServiceAccount(true, true), + wantErr: false, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + webhook := &ProviderServiceAccountWebhook{} + _, err := webhook.ValidateCreate(context.Background(), tc.providerServiceAccount) + if tc.wantErr { + g.Expect(err).To(HaveOccurred()) + } else { + g.Expect(err).NotTo(HaveOccurred()) + } + }) + } +} + +func TestProviderServiceAccount_ValidateUpdate(t *testing.T) { + g := NewWithT(t) + + tests := []struct { + name string + oldProviderServiceAccount *vmwarev1.ProviderServiceAccount + providerServiceAccount *vmwarev1.ProviderServiceAccount + wantErr bool + }{ + { + name: "ClusterName can be added during ProviderServiceAccount update", + oldProviderServiceAccount: createProviderServiceAccount(true, false), + providerServiceAccount: createProviderServiceAccount(true, true), + wantErr: false, + }, + { + name: "Ref can be removed during ProviderServiceAccount update", + oldProviderServiceAccount: createProviderServiceAccount(true, true), + providerServiceAccount: createProviderServiceAccount(false, true), + wantErr: false, + }, + { + name: "ProviderServiceAccount update should specify at least Ref or ClusterName ", + oldProviderServiceAccount: createProviderServiceAccount(true, false), + providerServiceAccount: createProviderServiceAccount(false, false), + wantErr: true, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + webhook := &ProviderServiceAccountWebhook{} + _, err := webhook.ValidateUpdate(context.Background(), tc.oldProviderServiceAccount, tc.providerServiceAccount) + if tc.wantErr { + g.Expect(err).To(HaveOccurred()) + } else { + g.Expect(err).NotTo(HaveOccurred()) + } + }) + } +} + +func createProviderServiceAccount(withRef, withClusterName bool) *vmwarev1.ProviderServiceAccount { + providerServiceAccount := &vmwarev1.ProviderServiceAccount{ + Spec: vmwarev1.ProviderServiceAccountSpec{ + Rules: []rbacv1.PolicyRule{}, + TargetNamespace: targetNamespace, + TargetSecretName: targetSecretName, + }, + } + + if withRef { + providerServiceAccount.Spec.Ref = &corev1.ObjectReference{} //nolint:staticcheck + } + + if withClusterName { + providerServiceAccount.Spec.ClusterName = &testClusterName + } + + return providerServiceAccount +} diff --git a/main.go b/main.go index e8eb6420e5..fe27c376b4 100644 --- a/main.go +++ b/main.go @@ -377,6 +377,9 @@ func setupVAPIControllers(ctx context.Context, controllerCtx *capvcontext.Contro } func setupSupervisorControllers(ctx context.Context, controllerCtx *capvcontext.ControllerManagerContext, mgr ctrlmgr.Manager, tracker *remote.ClusterCacheTracker) error { + if err := (&webhooks.ProviderServiceAccountWebhook{}).SetupWebhookWithManager(mgr); err != nil { + return err + } if err := controllers.AddClusterControllerToManager(ctx, controllerCtx, mgr, true, concurrency(vSphereClusterConcurrency)); err != nil { return err }