Skip to content

Commit

Permalink
Merge pull request #397 from Fedosin/fix-multiple-deployments
Browse files Browse the repository at this point in the history 
🐛 don't customize non-manager deployments
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot authored Jan 18, 2024
2 parents 6f7c927 + 195da27 commit d6e5f4f
Show file tree
Hide file tree
Showing 2 changed files with 131 additions and 23 deletions.
43 changes: 35 additions & 8 deletions internal/controller/component_customizer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand All @@ -49,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]

Expand All @@ -74,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
Expand All @@ -97,11 +109,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)
Expand Down Expand Up @@ -337,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")
}
111 changes: 96 additions & 15 deletions internal/controller/component_customizer_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,11 @@ 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"
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"

operatorv1 "sigs.k8s.io/cluster-api-operator/api/v1alpha2"
)
Expand All @@ -41,9 +42,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{
Expand Down Expand Up @@ -87,7 +85,6 @@ func TestCustomizeDeployment(t *testing.T) {
inputDeploymentSpec *operatorv1.DeploymentSpec
inputManagerSpec *operatorv1.ManagerSpec
expectedDeploymentSpec func(*appsv1.DeploymentSpec) (*appsv1.DeploymentSpec, bool)
noDeployementLabels bool
}{
{
name: "empty",
Expand Down Expand Up @@ -555,16 +552,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 {
Expand All @@ -583,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)
}
})
}
}

0 comments on commit d6e5f4f

Please sign in to comment.