From f4698538510532a2c7ae03309f1f07bf67f13afc Mon Sep 17 00:00:00 2001 From: chaunceyjiang Date: Fri, 2 Dec 2022 22:23:46 +0800 Subject: [PATCH] 0. Introduce ClusterName field to ClusterResourceSetBinding 1. remove the Cluster owner reference from the ClusterResourceSetBinding. 2. update the clusterName field in the CRS controller to update existing CRSbindings with the new field. 3. adding a check in the validation webhook preventing the cluster name to be changed once it is set Signed-off-by: chaunceyjiang --- ...r.x-k8s.io_clusterresourcesetbindings.yaml | 4 + config/webhook/manifests.yaml | 22 +++++ exp/addons/api/v1alpha3/conversion.go | 30 ++++++- .../api/v1alpha3/zz_generated.conversion.go | 40 ++++++--- exp/addons/api/v1alpha4/conversion.go | 30 ++++++- .../api/v1alpha4/zz_generated.conversion.go | 40 ++++++--- .../clusterresourcesetbinding_types.go | 5 ++ .../clusterresourcesetbinding_webhook.go | 78 ++++++++++++++++ .../clusterresourcesetbinding_webhook_test.go | 89 +++++++++++++++++++ .../clusterresourceset_controller.go | 2 +- .../clusterresourceset_controller_test.go | 24 ++--- .../controllers/clusterresourceset_helpers.go | 36 +++++--- .../clusterresourcesetbinding_controller.go | 35 ++++++-- internal/test/envtest/environment.go | 3 + main.go | 6 ++ test/framework/ownerreference_helpers.go | 4 +- 16 files changed, 384 insertions(+), 64 deletions(-) create mode 100644 exp/addons/api/v1beta1/clusterresourcesetbinding_webhook.go create mode 100644 exp/addons/api/v1beta1/clusterresourcesetbinding_webhook_test.go diff --git a/config/crd/bases/addons.cluster.x-k8s.io_clusterresourcesetbindings.yaml b/config/crd/bases/addons.cluster.x-k8s.io_clusterresourcesetbindings.yaml index edf31d9af124..1fe1d47f60f2 100644 --- a/config/crd/bases/addons.cluster.x-k8s.io_clusterresourcesetbindings.yaml +++ b/config/crd/bases/addons.cluster.x-k8s.io_clusterresourcesetbindings.yaml @@ -272,6 +272,10 @@ spec: - clusterResourceSetName type: object type: array + clusterName: + description: 'ClusterName is the name of the Cluster this binding + applies to. Note: this field mandatory in v1beta2.' + type: string type: object type: object served: true diff --git a/config/webhook/manifests.yaml b/config/webhook/manifests.yaml index f38f2a1e673f..83174169ca30 100644 --- a/config/webhook/manifests.yaml +++ b/config/webhook/manifests.yaml @@ -410,6 +410,28 @@ webhooks: resources: - clusterresourcesets sideEffects: None +- admissionReviewVersions: + - v1 + - v1beta1 + clientConfig: + service: + name: webhook-service + namespace: system + path: /validate-addons-cluster-x-k8s-io-v1beta1-clusterresourcesetbinding + failurePolicy: Fail + matchPolicy: Equivalent + name: validation.clusterresourcesetbinding.addons.cluster.x-k8s.io + rules: + - apiGroups: + - addons.cluster.x-k8s.io + apiVersions: + - v1beta1 + operations: + - CREATE + - UPDATE + resources: + - clusterresourcesetbindings + sideEffects: None - admissionReviewVersions: - v1 - v1beta1 diff --git a/exp/addons/api/v1alpha3/conversion.go b/exp/addons/api/v1alpha3/conversion.go index 25cd0b8b5db1..e7c38ca772c6 100644 --- a/exp/addons/api/v1alpha3/conversion.go +++ b/exp/addons/api/v1alpha3/conversion.go @@ -17,9 +17,11 @@ limitations under the License. package v1alpha3 import ( + apiconversion "k8s.io/apimachinery/pkg/conversion" "sigs.k8s.io/controller-runtime/pkg/conversion" addonsv1 "sigs.k8s.io/cluster-api/exp/addons/api/v1beta1" + utilconversion "sigs.k8s.io/cluster-api/util/conversion" ) func (src *ClusterResourceSet) ConvertTo(dstRaw conversion.Hub) error { @@ -49,13 +51,31 @@ func (dst *ClusterResourceSetList) ConvertFrom(srcRaw conversion.Hub) error { func (src *ClusterResourceSetBinding) ConvertTo(dstRaw conversion.Hub) error { dst := dstRaw.(*addonsv1.ClusterResourceSetBinding) - return Convert_v1alpha3_ClusterResourceSetBinding_To_v1beta1_ClusterResourceSetBinding(src, dst, nil) + if err := Convert_v1alpha3_ClusterResourceSetBinding_To_v1beta1_ClusterResourceSetBinding(src, dst, nil); err != nil { + return err + } + // Manually restore data. + restored := &addonsv1.ClusterResourceSetBinding{} + if ok, err := utilconversion.UnmarshalData(src, restored); err != nil || !ok { + return err + } + dst.Spec.ClusterName = restored.Spec.ClusterName + return nil } func (dst *ClusterResourceSetBinding) ConvertFrom(srcRaw conversion.Hub) error { src := srcRaw.(*addonsv1.ClusterResourceSetBinding) - return Convert_v1beta1_ClusterResourceSetBinding_To_v1alpha3_ClusterResourceSetBinding(src, dst, nil) + if err := Convert_v1beta1_ClusterResourceSetBinding_To_v1alpha3_ClusterResourceSetBinding(src, dst, nil); err != nil { + return err + } + + // Preserve Hub data on down-conversion except for metadata + if err := utilconversion.MarshalData(src, dst); err != nil { + return err + } + + return nil } func (src *ClusterResourceSetBindingList) ConvertTo(dstRaw conversion.Hub) error { @@ -69,3 +89,9 @@ func (dst *ClusterResourceSetBindingList) ConvertFrom(srcRaw conversion.Hub) err return Convert_v1beta1_ClusterResourceSetBindingList_To_v1alpha3_ClusterResourceSetBindingList(src, dst, nil) } + +// Convert_v1beta1_ClusterResourceSetBindingSpec_To_v1alpha3_ClusterResourceSetBindingSpec is a conversion function. +func Convert_v1beta1_ClusterResourceSetBindingSpec_To_v1alpha3_ClusterResourceSetBindingSpec(in *addonsv1.ClusterResourceSetBindingSpec, out *ClusterResourceSetBindingSpec, s apiconversion.Scope) error { + // Spec.ClusterName does not exist in ClusterResourceSetBinding v1alpha3 API. + return autoConvert_v1beta1_ClusterResourceSetBindingSpec_To_v1alpha3_ClusterResourceSetBindingSpec(in, out, s) +} diff --git a/exp/addons/api/v1alpha3/zz_generated.conversion.go b/exp/addons/api/v1alpha3/zz_generated.conversion.go index 48b5d5349e6a..b8514b7f909b 100644 --- a/exp/addons/api/v1alpha3/zz_generated.conversion.go +++ b/exp/addons/api/v1alpha3/zz_generated.conversion.go @@ -74,11 +74,6 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } - if err := s.AddGeneratedConversionFunc((*v1beta1.ClusterResourceSetBindingSpec)(nil), (*ClusterResourceSetBindingSpec)(nil), func(a, b interface{}, scope conversion.Scope) error { - return Convert_v1beta1_ClusterResourceSetBindingSpec_To_v1alpha3_ClusterResourceSetBindingSpec(a.(*v1beta1.ClusterResourceSetBindingSpec), b.(*ClusterResourceSetBindingSpec), scope) - }); err != nil { - return err - } if err := s.AddGeneratedConversionFunc((*ClusterResourceSetList)(nil), (*v1beta1.ClusterResourceSetList)(nil), func(a, b interface{}, scope conversion.Scope) error { return Convert_v1alpha3_ClusterResourceSetList_To_v1beta1_ClusterResourceSetList(a.(*ClusterResourceSetList), b.(*v1beta1.ClusterResourceSetList), scope) }); err != nil { @@ -139,6 +134,11 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } + if err := s.AddConversionFunc((*v1beta1.ClusterResourceSetBindingSpec)(nil), (*ClusterResourceSetBindingSpec)(nil), func(a, b interface{}, scope conversion.Scope) error { + return Convert_v1beta1_ClusterResourceSetBindingSpec_To_v1alpha3_ClusterResourceSetBindingSpec(a.(*v1beta1.ClusterResourceSetBindingSpec), b.(*ClusterResourceSetBindingSpec), scope) + }); err != nil { + return err + } return nil } @@ -202,7 +202,17 @@ func Convert_v1beta1_ClusterResourceSetBinding_To_v1alpha3_ClusterResourceSetBin func autoConvert_v1alpha3_ClusterResourceSetBindingList_To_v1beta1_ClusterResourceSetBindingList(in *ClusterResourceSetBindingList, out *v1beta1.ClusterResourceSetBindingList, s conversion.Scope) error { out.ListMeta = in.ListMeta - out.Items = *(*[]v1beta1.ClusterResourceSetBinding)(unsafe.Pointer(&in.Items)) + if in.Items != nil { + in, out := &in.Items, &out.Items + *out = make([]v1beta1.ClusterResourceSetBinding, len(*in)) + for i := range *in { + if err := Convert_v1alpha3_ClusterResourceSetBinding_To_v1beta1_ClusterResourceSetBinding(&(*in)[i], &(*out)[i], s); err != nil { + return err + } + } + } else { + out.Items = nil + } return nil } @@ -213,7 +223,17 @@ func Convert_v1alpha3_ClusterResourceSetBindingList_To_v1beta1_ClusterResourceSe func autoConvert_v1beta1_ClusterResourceSetBindingList_To_v1alpha3_ClusterResourceSetBindingList(in *v1beta1.ClusterResourceSetBindingList, out *ClusterResourceSetBindingList, s conversion.Scope) error { out.ListMeta = in.ListMeta - out.Items = *(*[]ClusterResourceSetBinding)(unsafe.Pointer(&in.Items)) + if in.Items != nil { + in, out := &in.Items, &out.Items + *out = make([]ClusterResourceSetBinding, len(*in)) + for i := range *in { + if err := Convert_v1beta1_ClusterResourceSetBinding_To_v1alpha3_ClusterResourceSetBinding(&(*in)[i], &(*out)[i], s); err != nil { + return err + } + } + } else { + out.Items = nil + } return nil } @@ -234,14 +254,10 @@ func Convert_v1alpha3_ClusterResourceSetBindingSpec_To_v1beta1_ClusterResourceSe func autoConvert_v1beta1_ClusterResourceSetBindingSpec_To_v1alpha3_ClusterResourceSetBindingSpec(in *v1beta1.ClusterResourceSetBindingSpec, out *ClusterResourceSetBindingSpec, s conversion.Scope) error { out.Bindings = *(*[]*ResourceSetBinding)(unsafe.Pointer(&in.Bindings)) + // WARNING: in.ClusterName requires manual conversion: does not exist in peer-type return nil } -// Convert_v1beta1_ClusterResourceSetBindingSpec_To_v1alpha3_ClusterResourceSetBindingSpec is an autogenerated conversion function. -func Convert_v1beta1_ClusterResourceSetBindingSpec_To_v1alpha3_ClusterResourceSetBindingSpec(in *v1beta1.ClusterResourceSetBindingSpec, out *ClusterResourceSetBindingSpec, s conversion.Scope) error { - return autoConvert_v1beta1_ClusterResourceSetBindingSpec_To_v1alpha3_ClusterResourceSetBindingSpec(in, out, s) -} - func autoConvert_v1alpha3_ClusterResourceSetList_To_v1beta1_ClusterResourceSetList(in *ClusterResourceSetList, out *v1beta1.ClusterResourceSetList, s conversion.Scope) error { out.ListMeta = in.ListMeta if in.Items != nil { diff --git a/exp/addons/api/v1alpha4/conversion.go b/exp/addons/api/v1alpha4/conversion.go index 1de13aac78c0..b1863ff4eaf6 100644 --- a/exp/addons/api/v1alpha4/conversion.go +++ b/exp/addons/api/v1alpha4/conversion.go @@ -17,9 +17,11 @@ limitations under the License. package v1alpha4 import ( + apiconversion "k8s.io/apimachinery/pkg/conversion" "sigs.k8s.io/controller-runtime/pkg/conversion" addonsv1 "sigs.k8s.io/cluster-api/exp/addons/api/v1beta1" + utilconversion "sigs.k8s.io/cluster-api/util/conversion" ) func (src *ClusterResourceSet) ConvertTo(dstRaw conversion.Hub) error { @@ -49,13 +51,31 @@ func (dst *ClusterResourceSetList) ConvertFrom(srcRaw conversion.Hub) error { func (src *ClusterResourceSetBinding) ConvertTo(dstRaw conversion.Hub) error { dst := dstRaw.(*addonsv1.ClusterResourceSetBinding) - return Convert_v1alpha4_ClusterResourceSetBinding_To_v1beta1_ClusterResourceSetBinding(src, dst, nil) + if err := Convert_v1alpha4_ClusterResourceSetBinding_To_v1beta1_ClusterResourceSetBinding(src, dst, nil); err != nil { + return err + } + // Manually restore data. + restored := &addonsv1.ClusterResourceSetBinding{} + if ok, err := utilconversion.UnmarshalData(src, restored); err != nil || !ok { + return err + } + dst.Spec.ClusterName = restored.Spec.ClusterName + return nil } func (dst *ClusterResourceSetBinding) ConvertFrom(srcRaw conversion.Hub) error { src := srcRaw.(*addonsv1.ClusterResourceSetBinding) - return Convert_v1beta1_ClusterResourceSetBinding_To_v1alpha4_ClusterResourceSetBinding(src, dst, nil) + if err := Convert_v1beta1_ClusterResourceSetBinding_To_v1alpha4_ClusterResourceSetBinding(src, dst, nil); err != nil { + return err + } + + // Preserve Hub data on down-conversion except for metadata + if err := utilconversion.MarshalData(src, dst); err != nil { + return err + } + + return nil } func (src *ClusterResourceSetBindingList) ConvertTo(dstRaw conversion.Hub) error { @@ -69,3 +89,9 @@ func (dst *ClusterResourceSetBindingList) ConvertFrom(srcRaw conversion.Hub) err return Convert_v1beta1_ClusterResourceSetBindingList_To_v1alpha4_ClusterResourceSetBindingList(src, dst, nil) } + +// Convert_v1beta1_ClusterResourceSetBindingSpec_To_v1alpha4_ClusterResourceSetBindingSpec is a conversion function. +func Convert_v1beta1_ClusterResourceSetBindingSpec_To_v1alpha4_ClusterResourceSetBindingSpec(in *addonsv1.ClusterResourceSetBindingSpec, out *ClusterResourceSetBindingSpec, s apiconversion.Scope) error { + // Spec.ClusterName does not exist in ClusterResourceSetBinding v1alpha4 API. + return autoConvert_v1beta1_ClusterResourceSetBindingSpec_To_v1alpha4_ClusterResourceSetBindingSpec(in, out, s) +} diff --git a/exp/addons/api/v1alpha4/zz_generated.conversion.go b/exp/addons/api/v1alpha4/zz_generated.conversion.go index 72a5d116a2c5..3eafdc8da85a 100644 --- a/exp/addons/api/v1alpha4/zz_generated.conversion.go +++ b/exp/addons/api/v1alpha4/zz_generated.conversion.go @@ -74,11 +74,6 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } - if err := s.AddGeneratedConversionFunc((*v1beta1.ClusterResourceSetBindingSpec)(nil), (*ClusterResourceSetBindingSpec)(nil), func(a, b interface{}, scope conversion.Scope) error { - return Convert_v1beta1_ClusterResourceSetBindingSpec_To_v1alpha4_ClusterResourceSetBindingSpec(a.(*v1beta1.ClusterResourceSetBindingSpec), b.(*ClusterResourceSetBindingSpec), scope) - }); err != nil { - return err - } if err := s.AddGeneratedConversionFunc((*ClusterResourceSetList)(nil), (*v1beta1.ClusterResourceSetList)(nil), func(a, b interface{}, scope conversion.Scope) error { return Convert_v1alpha4_ClusterResourceSetList_To_v1beta1_ClusterResourceSetList(a.(*ClusterResourceSetList), b.(*v1beta1.ClusterResourceSetList), scope) }); err != nil { @@ -139,6 +134,11 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } + if err := s.AddConversionFunc((*v1beta1.ClusterResourceSetBindingSpec)(nil), (*ClusterResourceSetBindingSpec)(nil), func(a, b interface{}, scope conversion.Scope) error { + return Convert_v1beta1_ClusterResourceSetBindingSpec_To_v1alpha4_ClusterResourceSetBindingSpec(a.(*v1beta1.ClusterResourceSetBindingSpec), b.(*ClusterResourceSetBindingSpec), scope) + }); err != nil { + return err + } return nil } @@ -202,7 +202,17 @@ func Convert_v1beta1_ClusterResourceSetBinding_To_v1alpha4_ClusterResourceSetBin func autoConvert_v1alpha4_ClusterResourceSetBindingList_To_v1beta1_ClusterResourceSetBindingList(in *ClusterResourceSetBindingList, out *v1beta1.ClusterResourceSetBindingList, s conversion.Scope) error { out.ListMeta = in.ListMeta - out.Items = *(*[]v1beta1.ClusterResourceSetBinding)(unsafe.Pointer(&in.Items)) + if in.Items != nil { + in, out := &in.Items, &out.Items + *out = make([]v1beta1.ClusterResourceSetBinding, len(*in)) + for i := range *in { + if err := Convert_v1alpha4_ClusterResourceSetBinding_To_v1beta1_ClusterResourceSetBinding(&(*in)[i], &(*out)[i], s); err != nil { + return err + } + } + } else { + out.Items = nil + } return nil } @@ -213,7 +223,17 @@ func Convert_v1alpha4_ClusterResourceSetBindingList_To_v1beta1_ClusterResourceSe func autoConvert_v1beta1_ClusterResourceSetBindingList_To_v1alpha4_ClusterResourceSetBindingList(in *v1beta1.ClusterResourceSetBindingList, out *ClusterResourceSetBindingList, s conversion.Scope) error { out.ListMeta = in.ListMeta - out.Items = *(*[]ClusterResourceSetBinding)(unsafe.Pointer(&in.Items)) + if in.Items != nil { + in, out := &in.Items, &out.Items + *out = make([]ClusterResourceSetBinding, len(*in)) + for i := range *in { + if err := Convert_v1beta1_ClusterResourceSetBinding_To_v1alpha4_ClusterResourceSetBinding(&(*in)[i], &(*out)[i], s); err != nil { + return err + } + } + } else { + out.Items = nil + } return nil } @@ -234,14 +254,10 @@ func Convert_v1alpha4_ClusterResourceSetBindingSpec_To_v1beta1_ClusterResourceSe func autoConvert_v1beta1_ClusterResourceSetBindingSpec_To_v1alpha4_ClusterResourceSetBindingSpec(in *v1beta1.ClusterResourceSetBindingSpec, out *ClusterResourceSetBindingSpec, s conversion.Scope) error { out.Bindings = *(*[]*ResourceSetBinding)(unsafe.Pointer(&in.Bindings)) + // WARNING: in.ClusterName requires manual conversion: does not exist in peer-type return nil } -// Convert_v1beta1_ClusterResourceSetBindingSpec_To_v1alpha4_ClusterResourceSetBindingSpec is an autogenerated conversion function. -func Convert_v1beta1_ClusterResourceSetBindingSpec_To_v1alpha4_ClusterResourceSetBindingSpec(in *v1beta1.ClusterResourceSetBindingSpec, out *ClusterResourceSetBindingSpec, s conversion.Scope) error { - return autoConvert_v1beta1_ClusterResourceSetBindingSpec_To_v1alpha4_ClusterResourceSetBindingSpec(in, out, s) -} - func autoConvert_v1alpha4_ClusterResourceSetList_To_v1beta1_ClusterResourceSetList(in *ClusterResourceSetList, out *v1beta1.ClusterResourceSetList, s conversion.Scope) error { out.ListMeta = in.ListMeta if in.Items != nil { diff --git a/exp/addons/api/v1beta1/clusterresourcesetbinding_types.go b/exp/addons/api/v1beta1/clusterresourcesetbinding_types.go index 47f8762ec3e6..3b0ba2ddb97a 100644 --- a/exp/addons/api/v1beta1/clusterresourcesetbinding_types.go +++ b/exp/addons/api/v1beta1/clusterresourcesetbinding_types.go @@ -133,6 +133,11 @@ type ClusterResourceSetBindingSpec struct { // Bindings is a list of ClusterResourceSets and their resources. // +optional Bindings []*ResourceSetBinding `json:"bindings,omitempty"` + + // ClusterName is the name of the Cluster this binding applies to. + // Note: this field mandatory in v1beta2. + // +optional + ClusterName string `json:"clusterName,omitempty"` } // ANCHOR_END: ClusterResourceSetBindingSpec diff --git a/exp/addons/api/v1beta1/clusterresourcesetbinding_webhook.go b/exp/addons/api/v1beta1/clusterresourcesetbinding_webhook.go new file mode 100644 index 000000000000..91a5b12c49f8 --- /dev/null +++ b/exp/addons/api/v1beta1/clusterresourcesetbinding_webhook.go @@ -0,0 +1,78 @@ +/* +Copyright 2022 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 v1beta1 + +import ( + "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/cluster-api/feature" +) + +func (c *ClusterResourceSetBinding) SetupWebhookWithManager(mgr ctrl.Manager) error { + return ctrl.NewWebhookManagedBy(mgr). + For(c). + Complete() +} + +// +kubebuilder:webhook:verbs=create;update,path=/validate-addons-cluster-x-k8s-io-v1beta1-clusterresourcesetbinding,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=addons.cluster.x-k8s.io,resources=clusterresourcesetbindings,versions=v1beta1,name=validation.clusterresourcesetbinding.addons.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1;v1beta1 + +var _ webhook.Validator = &ClusterResourceSetBinding{} + +// ValidateCreate implements webhook.Validator so a webhook will be registered for the type. +func (c *ClusterResourceSetBinding) ValidateCreate() error { + return c.validate(nil) +} + +// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type. +func (c *ClusterResourceSetBinding) ValidateUpdate(old runtime.Object) error { + oldBinding, ok := old.(*ClusterResourceSetBinding) + if !ok { + return apierrors.NewBadRequest(fmt.Sprintf("expected a ClusterResourceSetBinding but got a %T", old)) + } + return c.validate(oldBinding) +} + +// ValidateDelete implements webhook.Validator so a webhook will be registered for the type. +func (c *ClusterResourceSetBinding) ValidateDelete() error { + return nil +} + +func (c *ClusterResourceSetBinding) validate(old *ClusterResourceSetBinding) error { + // NOTE: ClusterResourceSet is behind ClusterResourceSet feature gate flag; the web hook + // must prevent creating new objects in case the feature flag is disabled. + if !feature.Gates.Enabled(feature.ClusterResourceSet) { + return field.Forbidden( + field.NewPath("spec"), + "can be set only if the ClusterResourceSet feature flag is enabled", + ) + } + var allErrs field.ErrorList + if old != nil && old.Spec.ClusterName != "" && old.Spec.ClusterName != c.Spec.ClusterName { + allErrs = append(allErrs, + field.Invalid(field.NewPath("spec", "clusterName"), c.Spec.ClusterName, "field is immutable")) + } + if len(allErrs) == 0 { + return nil + } + return apierrors.NewInvalid(GroupVersion.WithKind("ClusterResourceSetBinding").GroupKind(), c.Name, allErrs) +} diff --git a/exp/addons/api/v1beta1/clusterresourcesetbinding_webhook_test.go b/exp/addons/api/v1beta1/clusterresourcesetbinding_webhook_test.go new file mode 100644 index 000000000000..0530a6183edc --- /dev/null +++ b/exp/addons/api/v1beta1/clusterresourcesetbinding_webhook_test.go @@ -0,0 +1,89 @@ +/* +Copyright 2022 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 v1beta1 + +import ( + "testing" + + . "github.com/onsi/gomega" +) + +func TestClusterResourceSetBindingClusterNameImmutable(t *testing.T) { + tests := []struct { + name string + oldClusterName string + newClusterName string + expectErr bool + }{ + { + name: "when ClusterName is empty", + oldClusterName: "", + newClusterName: "", + expectErr: false, + }, + { + name: "add ClusterName field", + oldClusterName: "", + newClusterName: "bar", + expectErr: false, + }, + { + name: "when the ClusterName has not changed", + oldClusterName: "bar", + newClusterName: "bar", + expectErr: false, + }, + { + name: "when the ClusterName has changed", + oldClusterName: "bar", + newClusterName: "different", + expectErr: true, + }, + { + name: "existing ClusterName field to be set to empty", + oldClusterName: "bar", + newClusterName: "", + expectErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + newClusterResourceSetBinding := &ClusterResourceSetBinding{ + Spec: ClusterResourceSetBindingSpec{ + ClusterName: tt.newClusterName, + }, + } + + oldClusterResourceSetBinding := &ClusterResourceSetBinding{ + Spec: ClusterResourceSetBindingSpec{ + ClusterName: tt.oldClusterName, + }, + } + + if tt.expectErr { + g.Expect(newClusterResourceSetBinding.ValidateCreate()).To(Succeed()) + g.Expect(newClusterResourceSetBinding.ValidateUpdate(oldClusterResourceSetBinding)).NotTo(Succeed()) + return + } + g.Expect(newClusterResourceSetBinding.ValidateCreate()).To(Succeed()) + g.Expect(newClusterResourceSetBinding.ValidateUpdate(oldClusterResourceSetBinding)).To(Succeed()) + }) + } +} diff --git a/exp/addons/internal/controllers/clusterresourceset_controller.go b/exp/addons/internal/controllers/clusterresourceset_controller.go index a029bface122..36dc8b12244a 100644 --- a/exp/addons/internal/controllers/clusterresourceset_controller.go +++ b/exp/addons/internal/controllers/clusterresourceset_controller.go @@ -269,7 +269,7 @@ func (r *ClusterResourceSetReconciler) ApplyClusterResourceSet(ctx context.Conte }() // Ensure that the owner references are set on the ClusterResourceSetBinding. - clusterResourceSetBinding.OwnerReferences = ensureOwnerRefs(clusterResourceSetBinding, clusterResourceSet, cluster) + clusterResourceSetBinding.OwnerReferences = ensureOwnerRefs(clusterResourceSetBinding, clusterResourceSet) errList := []error{} resourceSetBinding := clusterResourceSetBinding.GetOrCreateBinding(clusterResourceSet) diff --git a/exp/addons/internal/controllers/clusterresourceset_controller_test.go b/exp/addons/internal/controllers/clusterresourceset_controller_test.go index 60c53b2702f3..8f8bcb384f5c 100644 --- a/exp/addons/internal/controllers/clusterresourceset_controller_test.go +++ b/exp/addons/internal/controllers/clusterresourceset_controller_test.go @@ -235,7 +235,7 @@ metadata: testCluster.SetLabels(labels) g.Expect(env.Update(ctx, testCluster)).To(Succeed()) - t.Log("Verifying ClusterResourceSetBinding is created with cluster owner reference") + t.Log("Verifying ClusterResourceSetBinding is created with cluster name") g.Eventually(func() bool { binding := &addonsv1.ClusterResourceSetBinding{} clusterResourceSetBindingKey := client.ObjectKey{ @@ -247,12 +247,7 @@ metadata: return false } - return util.HasOwnerRef(binding.GetOwnerReferences(), metav1.OwnerReference{ - APIVersion: clusterv1.GroupVersion.String(), - Kind: "Cluster", - Name: testCluster.Name, - UID: testCluster.UID, - }) + return binding.Spec.ClusterName == testCluster.Name }, timeout).Should(BeTrue()) // Wait until ClusterResourceSetBinding is created for the Cluster @@ -528,8 +523,8 @@ metadata: if err != nil { return false } - return len(binding.Spec.Bindings) == 2 && len(binding.OwnerReferences) == 3 - }, timeout).Should(BeTrue(), "Expected 2 ClusterResourceSets and 3 OwnerReferences") + return len(binding.Spec.Bindings) == 2 && len(binding.OwnerReferences) == 2 + }, timeout).Should(BeTrue(), "Expected 2 ClusterResourceSets and 2 OwnerReferences") t.Log("Verifying deleted CRS is deleted from ClusterResourceSetBinding") // Delete one of the CRS instances and wait until it is removed from the binding list. @@ -544,8 +539,8 @@ metadata: if err != nil { return false } - return len(binding.Spec.Bindings) == 1 && len(binding.OwnerReferences) == 2 - }, timeout).Should(BeTrue(), "ClusterResourceSetBinding should have 1 ClusterResourceSet and 2 OwnerReferences") + return len(binding.Spec.Bindings) == 1 && len(binding.OwnerReferences) == 1 + }, timeout).Should(BeTrue(), "ClusterResourceSetBinding should have 1 ClusterResourceSet and 1 OwnerReferences") t.Log("Verifying ClusterResourceSetBinding is deleted after deleting all matching CRS objects") // Delete one of the CRS instances and wait until it is removed from the binding list. @@ -940,12 +935,7 @@ func clusterResourceSetBindingReady(env *envtest.Environment, cluster *clusterv1 return false } - return util.HasOwnerRef(binding.GetOwnerReferences(), metav1.OwnerReference{ - APIVersion: clusterv1.GroupVersion.String(), - Kind: "Cluster", - Name: cluster.Name, - UID: cluster.UID, - }) + return binding.Spec.ClusterName == cluster.Name } } diff --git a/exp/addons/internal/controllers/clusterresourceset_helpers.go b/exp/addons/internal/controllers/clusterresourceset_helpers.go index 2b25ceab3ef8..be0dcdb6d0cb 100644 --- a/exp/addons/internal/controllers/clusterresourceset_helpers.go +++ b/exp/addons/internal/controllers/clusterresourceset_helpers.go @@ -32,6 +32,7 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "k8s.io/klog/v2" "sigs.k8s.io/controller-runtime/pkg/client" @@ -120,9 +121,9 @@ func (r *ClusterResourceSetReconciler) getOrCreateClusterResourceSetBinding(ctx } clusterResourceSetBinding.Name = cluster.Name clusterResourceSetBinding.Namespace = cluster.Namespace - clusterResourceSetBinding.OwnerReferences = ensureOwnerRefs(clusterResourceSetBinding, clusterResourceSet, cluster) + clusterResourceSetBinding.OwnerReferences = ensureOwnerRefs(clusterResourceSetBinding, clusterResourceSet) clusterResourceSetBinding.Spec.Bindings = []*addonsv1.ResourceSetBinding{} - + clusterResourceSetBinding.Spec.ClusterName = cluster.Name if err := r.Client.Create(ctx, clusterResourceSetBinding); err != nil { if apierrors.IsAlreadyExists(err) { if err = r.Client.Get(ctx, clusterResourceSetBindingKey, clusterResourceSetBinding); err != nil { @@ -136,16 +137,10 @@ func (r *ClusterResourceSetReconciler) getOrCreateClusterResourceSetBinding(ctx return clusterResourceSetBinding, nil } -// ensureOwnerRefs ensures Cluster and ClusterResourceSet owner references are set on the ClusterResourceSetBinding. -func ensureOwnerRefs(clusterResourceSetBinding *addonsv1.ClusterResourceSetBinding, clusterResourceSet *addonsv1.ClusterResourceSet, cluster *clusterv1.Cluster) []metav1.OwnerReference { +// ensureOwnerRefs ensure ClusterResourceSet owner references are set on the ClusterResourceSetBinding. +func ensureOwnerRefs(clusterResourceSetBinding *addonsv1.ClusterResourceSetBinding, clusterResourceSet *addonsv1.ClusterResourceSet) []metav1.OwnerReference { ownerRefs := make([]metav1.OwnerReference, len(clusterResourceSetBinding.GetOwnerReferences())) copy(ownerRefs, clusterResourceSetBinding.GetOwnerReferences()) - ownerRefs = util.EnsureOwnerRef(ownerRefs, metav1.OwnerReference{ - APIVersion: clusterv1.GroupVersion.String(), - Kind: "Cluster", - Name: cluster.Name, - UID: cluster.UID, - }) ownerRefs = util.EnsureOwnerRef(ownerRefs, metav1.OwnerReference{ APIVersion: clusterResourceSet.GroupVersionKind().GroupVersion().String(), @@ -234,3 +229,24 @@ func normalizeData(resource *unstructured.Unstructured) ([][]byte, error) { return dataList, nil } + +func getClusterNameFromOwnerRef(obj metav1.ObjectMeta) (string, error) { + for _, ref := range obj.OwnerReferences { + if ref.Kind != "Cluster" { + continue + } + gv, err := schema.ParseGroupVersion(ref.APIVersion) + if err != nil { + return "", errors.Wrap(err, "failed to find cluster name in ownerRefs") + } + + if gv.Group != clusterv1.GroupVersion.Group { + continue + } + if ref.Name == "" { + return "", errors.New("failed to find cluster name in ownerRefs: ref name is empty") + } + return ref.Name, nil + } + return "", errors.New("failed to find cluster name in ownerRefs: no cluster ownerRef") +} diff --git a/exp/addons/internal/controllers/clusterresourcesetbinding_controller.go b/exp/addons/internal/controllers/clusterresourcesetbinding_controller.go index b0e12e3dd3f5..f1798e321a13 100644 --- a/exp/addons/internal/controllers/clusterresourcesetbinding_controller.go +++ b/exp/addons/internal/controllers/clusterresourcesetbinding_controller.go @@ -21,6 +21,7 @@ import ( "github.com/pkg/errors" apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" @@ -33,6 +34,7 @@ import ( "sigs.k8s.io/cluster-api/feature" "sigs.k8s.io/cluster-api/internal/hooks" "sigs.k8s.io/cluster-api/util" + "sigs.k8s.io/cluster-api/util/patch" "sigs.k8s.io/cluster-api/util/predicates" ) @@ -77,8 +79,33 @@ func (r *ClusterResourceSetBindingReconciler) Reconcile(ctx context.Context, req // Error reading the object - requeue the request. return ctrl.Result{}, err } - - cluster, err := util.GetOwnerCluster(ctx, r.Client, binding.ObjectMeta) + // Before 1.4 cluster name was stored as ownerRef; this code migrated this value to + // the spec.clusterName field if necessary. + clusterName := binding.Spec.ClusterName + if clusterName == "" { + // Initialize the patch helper. + patchHelper, err := patch.NewHelper(binding, r.Client) + if err != nil { + return ctrl.Result{}, errors.Wrap(err, "failed to configure the patch helper") + } + // Update the clusterName field of the existing ClusterResourceSetBindings with ownerReferences. + // More details please refer to: https://github.com/kubernetes-sigs/cluster-api/issues/7669. + if clusterName, err = getClusterNameFromOwnerRef(binding.ObjectMeta); err != nil { + return ctrl.Result{}, err + } + binding.Spec.ClusterName = clusterName + binding.OwnerReferences = util.RemoveOwnerRef(binding.OwnerReferences, metav1.OwnerReference{ + APIVersion: clusterv1.GroupVersion.String(), + Kind: "Cluster", + Name: clusterName, + }) + if err = patchHelper.Patch(ctx, binding); err != nil { + return ctrl.Result{}, errors.Wrap(err, "failed to patch ClusterResourceSetBinding to add spec.clusterName") + } + // Will enter the next Reconcile + return ctrl.Result{}, nil + } + cluster, err := util.GetClusterByName(ctx, r.Client, req.Namespace, clusterName) if err != nil { if apierrors.IsNotFound(err) { // If the owner cluster is already deleted, delete its ClusterResourceSetBinding @@ -87,10 +114,6 @@ func (r *ClusterResourceSetBindingReconciler) Reconcile(ctx context.Context, req } return ctrl.Result{}, err } - if cluster == nil { - log.Info("ownerRef not found for the ClusterResourceSetBinding") - return ctrl.Result{}, nil - } // If the owner cluster is in deletion process, delete its ClusterResourceSetBinding if !cluster.DeletionTimestamp.IsZero() { if feature.Gates.Enabled(feature.RuntimeSDK) && feature.Gates.Enabled(feature.ClusterTopology) { diff --git a/internal/test/envtest/environment.go b/internal/test/envtest/environment.go index 89def53bfaa2..32520fc4a4f8 100644 --- a/internal/test/envtest/environment.go +++ b/internal/test/envtest/environment.go @@ -298,6 +298,9 @@ func newEnvironment(uncachedObjs ...client.Object) *Environment { if err := (&addonsv1.ClusterResourceSet{}).SetupWebhookWithManager(mgr); err != nil { klog.Fatalf("unable to create webhook for crs: %+v", err) } + if err := (&addonsv1.ClusterResourceSetBinding{}).SetupWebhookWithManager(mgr); err != nil { + klog.Fatalf("unable to create webhook for ClusterResourceSetBinding: %+v", err) + } if err := (&expv1.MachinePool{}).SetupWebhookWithManager(mgr); err != nil { klog.Fatalf("unable to create webhook for machinepool: %+v", err) } diff --git a/main.go b/main.go index cb53d83bc168..5d747724523a 100644 --- a/main.go +++ b/main.go @@ -529,6 +529,12 @@ func setupWebhooks(mgr ctrl.Manager) { setupLog.Error(err, "unable to create webhook", "webhook", "ClusterResourceSet") os.Exit(1) } + // NOTE: ClusterResourceSetBinding is behind ClusterResourceSet feature gate flag; the webhook + // is going to prevent creating or updating new objects in case the feature flag is disabled + if err := (&addonsv1.ClusterResourceSetBinding{}).SetupWebhookWithManager(mgr); err != nil { + setupLog.Error(err, "unable to create webhook", "webhook", "ClusterResourceSetBinding") + os.Exit(1) + } if err := (&clusterv1.MachineHealthCheck{}).SetupWebhookWithManager(mgr); err != nil { setupLog.Error(err, "unable to create webhook", "webhook", "MachineHealthCheck") diff --git a/test/framework/ownerreference_helpers.go b/test/framework/ownerreference_helpers.go index a1c0cc1fd136..cb2c3d43b195 100644 --- a/test/framework/ownerreference_helpers.go +++ b/test/framework/ownerreference_helpers.go @@ -157,9 +157,9 @@ var ExpOwnerReferenceAssertions = map[string]func([]metav1.OwnerReference) error // ClusterResourcesSet doesn't have ownerReferences (it is a clusterctl move-hierarchy root). return hasExactOwnersByGVK(owners, []schema.GroupVersionKind{}) }, - // ClusterResourcesSetBinding has both Cluster and ClusterResourceSet set as owners on creation. + // ClusterResourcesSetBinding has ClusterResourceSet set as owners on creation. clusterResourceSetBindingKind: func(owners []metav1.OwnerReference) error { - return hasExactOwnersByGVK(owners, []schema.GroupVersionKind{clusterGVK, clusterResourceSetGVK}) + return hasExactOwnersByGVK(owners, []schema.GroupVersionKind{clusterResourceSetGVK}) }, }