From 43bb5b99dbb9fe0b66660fc6c3cd85f1df6cdb97 Mon Sep 17 00:00:00 2001 From: Mikhail Fedosin Date: Wed, 17 Jan 2024 15:00:01 +0100 Subject: [PATCH 1/2] Revert "fix: customize only provider deployments" This reverts commit d4235636ca86b9b533906c59c9ed031d40ef783d. --- internal/controller/component_customizer.go | 9 +-------- internal/controller/component_customizer_test.go | 15 --------------- 2 files changed, 1 insertion(+), 23 deletions(-) diff --git a/internal/controller/component_customizer.go b/internal/controller/component_customizer.go index cad8a8cef..b817aa88c 100644 --- a/internal/controller/component_customizer.go +++ b/internal/controller/component_customizer.go @@ -29,10 +29,8 @@ import ( "k8s.io/client-go/kubernetes/scheme" configv1alpha1 "k8s.io/component-base/config/v1alpha1" "k8s.io/utils/pointer" - clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" - "sigs.k8s.io/cluster-api/util" - operatorv1 "sigs.k8s.io/cluster-api-operator/api/v1alpha2" + "sigs.k8s.io/cluster-api/util" ) const ( @@ -97,11 +95,6 @@ func customizeObjectsFn(provider operatorv1.GenericProvider) func(objs []unstruc // customizeDeployment customize provider deployment base on provider spec input. func customizeDeployment(pSpec operatorv1.ProviderSpec, d *appsv1.Deployment) error { - // Ensure that we customize a manager deployment. It must contain "cluster.x-k8s.io/provider" label. - if _, ok := d.GetLabels()[clusterv1.ProviderNameLabel]; !ok { - return nil - } - // Customize deployment spec first. if pSpec.Deployment != nil { customizeDeploymentSpec(pSpec, d) diff --git a/internal/controller/component_customizer_test.go b/internal/controller/component_customizer_test.go index 79567d0df..eaee35010 100644 --- a/internal/controller/component_customizer_test.go +++ b/internal/controller/component_customizer_test.go @@ -29,7 +29,6 @@ import ( "k8s.io/apimachinery/pkg/util/intstr" configv1alpha1 "k8s.io/component-base/config/v1alpha1" "k8s.io/utils/pointer" - clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" operatorv1 "sigs.k8s.io/cluster-api-operator/api/v1alpha2" ) @@ -41,9 +40,6 @@ func TestCustomizeDeployment(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "manager", Namespace: metav1.NamespaceSystem, - Labels: map[string]string{ - clusterv1.ProviderNameLabel: "infra-provider", - }, }, Spec: appsv1.DeploymentSpec{ Template: corev1.PodTemplateSpec{ @@ -87,7 +83,6 @@ func TestCustomizeDeployment(t *testing.T) { inputDeploymentSpec *operatorv1.DeploymentSpec inputManagerSpec *operatorv1.ManagerSpec expectedDeploymentSpec func(*appsv1.DeploymentSpec) (*appsv1.DeploymentSpec, bool) - noDeployementLabels bool }{ { name: "empty", @@ -555,16 +550,6 @@ func TestCustomizeDeployment(t *testing.T) { return expectedDS, reflect.DeepEqual(inputDS.Template.Spec.Containers[0], expectedDS.Template.Spec.Containers[0]) }, }, - { - name: "no provider label on the deployment", - inputDeploymentSpec: &operatorv1.DeploymentSpec{ - NodeSelector: map[string]string{"a": "b"}, - }, - expectedDeploymentSpec: func(inputDS *appsv1.DeploymentSpec) (*appsv1.DeploymentSpec, bool) { - return &managerDepl.Spec, true - }, - noDeployementLabels: true, - }, } for _, tc := range tests { From 195da273f2b9a7bca5693d0a592699f188f9499e Mon Sep 17 00:00:00 2001 From: Mikhail Fedosin Date: Wed, 17 Jan 2024 16:03:13 +0100 Subject: [PATCH 2/2] fix: don't customize non-manager deployments To prevent customiztion of non-manager deployments, we introduce a check that first verifies that there are several deployments. If there are several deployments available, we customize only those with names like "ca*-controller-manager" and skip others. --- internal/controller/component_customizer.go | 34 +++++++ .../controller/component_customizer_test.go | 96 +++++++++++++++++++ 2 files changed, 130 insertions(+) diff --git a/internal/controller/component_customizer.go b/internal/controller/component_customizer.go index b817aa88c..7b0531e9c 100644 --- a/internal/controller/component_customizer.go +++ b/internal/controller/component_customizer.go @@ -47,6 +47,8 @@ func customizeObjectsFn(provider operatorv1.GenericProvider) func(objs []unstruc return func(objs []unstructured.Unstructured) ([]unstructured.Unstructured, error) { results := []unstructured.Unstructured{} + isMultipleDeployments := isMultipleDeployments(objs) + for i := range objs { o := objs[i] @@ -72,6 +74,18 @@ func customizeObjectsFn(provider operatorv1.GenericProvider) func(objs []unstruc } if o.GetKind() == deploymentKind { + // We need to skip the deployment customization if there are several deployments available + // and the deployment name doesn't follow "ca*-controller-manager" pattern. + // Currently it is applicable only for CAPZ manifests, which contain 2 deployments: + // capz-controller-manager and azureserviceoperator-controller-manager + // This is a temporary fix until CAPI provides a contract to distinguish provider deployments. + // TODO: replace this check and just compare labels when CAPI provides the contract for that. + if isMultipleDeployments && !isProviderManagerDeploymentName(o.GetName()) { + results = append(results, o) + + continue + } + d := &appsv1.Deployment{} if err := scheme.Scheme.Convert(&o, d, nil); err != nil { return nil, err @@ -330,3 +344,23 @@ func leaderElectionArgs(lec *configv1alpha1.LeaderElectionConfiguration, args [] return args } + +// isMultipleDeployments check if there are multiple deployments in the manifests. +func isMultipleDeployments(objs []unstructured.Unstructured) bool { + var numberOfDeployments int + + for i := range objs { + o := objs[i] + + if o.GetKind() == deploymentKind { + numberOfDeployments++ + } + } + + return numberOfDeployments > 1 +} + +// isProviderManagerDeploymentName checks that the provided follows the provider manager deployment name pattern: "ca*-controller-manager". +func isProviderManagerDeploymentName(name string) bool { + return strings.HasPrefix(name, "ca") && strings.HasSuffix(name, "-controller-manager") +} diff --git a/internal/controller/component_customizer_test.go b/internal/controller/component_customizer_test.go index eaee35010..9b386811b 100644 --- a/internal/controller/component_customizer_test.go +++ b/internal/controller/component_customizer_test.go @@ -26,7 +26,9 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/client-go/kubernetes/scheme" configv1alpha1 "k8s.io/component-base/config/v1alpha1" "k8s.io/utils/pointer" @@ -568,3 +570,97 @@ func TestCustomizeDeployment(t *testing.T) { }) } } + +func TestCustomizeMultipleDeployment(t *testing.T) { + tests := []struct { + name string + nonManagerDeploymentName string + }{ + { + name: "name without suffix and prefix", + nonManagerDeploymentName: "non-manager", + }, + { + name: "name with prefix", + nonManagerDeploymentName: "ca-non-manager", + }, + { + name: "name with suffix", + nonManagerDeploymentName: "non-manager-controller-manager", + }, + { + name: "empty name", + nonManagerDeploymentName: "", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + managerDepl := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cap-controller-manager", + Namespace: metav1.NamespaceSystem, + }, + Spec: appsv1.DeploymentSpec{ + Replicas: pointer.Int32(3), + }, + } + + nonManagerDepl := managerDepl.DeepCopy() + nonManagerDepl.Name = tc.nonManagerDeploymentName + + var managerDeplRaw, nonManagerDeplRaw unstructured.Unstructured + + if err := scheme.Scheme.Convert(managerDepl, &managerDeplRaw, nil); err != nil { + t.Error(err) + } + + if err := scheme.Scheme.Convert(nonManagerDepl, &nonManagerDeplRaw, nil); err != nil { + t.Error(err) + } + + objs := []unstructured.Unstructured{managerDeplRaw, nonManagerDeplRaw} + + // We want to customize the manager deployment and leave the non-manager deployment alone. + // Replicas number will be set to 10 for the manager deployment and 3 for the non-manager deployment. + provider := operatorv1.CoreProvider{ + Spec: operatorv1.CoreProviderSpec{ + ProviderSpec: operatorv1.ProviderSpec{ + Deployment: &operatorv1.DeploymentSpec{ + Replicas: pointer.Int(10), + }, + }, + }, + } + + customizationFunc := customizeObjectsFn(&provider) + + objs, err := customizationFunc(objs) + if err != nil { + t.Error(err) + } + + if len(objs) != 2 { + t.Errorf("expected 2 objects, got %d", len(objs)) + } + + if err := scheme.Scheme.Convert(&objs[0], managerDepl, nil); err != nil { + t.Error(err) + } + + if err := scheme.Scheme.Convert(&objs[1], nonManagerDepl, nil); err != nil { + t.Error(err) + } + + // manager deployment should have been customized + if *managerDepl.Spec.Replicas != 10 { + t.Errorf("expected 10 replicas, got %d", *managerDepl.Spec.Replicas) + } + + // non-manager deployment should not have been customized + if *nonManagerDepl.Spec.Replicas != 3 { + t.Errorf("expected 3 replicas, got %d", *nonManagerDepl.Spec.Replicas) + } + }) + } +}