Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ Add support for customizing additional provider deployments #538

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions api/v1alpha1/provider_conversion.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ func (src *BootstrapProvider) ConvertTo(dstRaw conversion.Hub) error {
}

dst.Spec.ManifestPatches = restored.Spec.ManifestPatches
dst.Spec.AdditionalDeployments = restored.Spec.AdditionalDeployments

return nil
}
Expand Down Expand Up @@ -107,6 +108,7 @@ func (src *ControlPlaneProvider) ConvertTo(dstRaw conversion.Hub) error {
}

dst.Spec.ManifestPatches = restored.Spec.ManifestPatches
dst.Spec.AdditionalDeployments = restored.Spec.AdditionalDeployments

return nil
}
Expand Down Expand Up @@ -168,6 +170,7 @@ func (src *CoreProvider) ConvertTo(dstRaw conversion.Hub) error {
}

dst.Spec.ManifestPatches = restored.Spec.ManifestPatches
dst.Spec.AdditionalDeployments = restored.Spec.AdditionalDeployments

return nil
}
Expand Down Expand Up @@ -229,6 +232,7 @@ func (src *InfrastructureProvider) ConvertTo(dstRaw conversion.Hub) error {
}

dst.Spec.ManifestPatches = restored.Spec.ManifestPatches
dst.Spec.AdditionalDeployments = restored.Spec.AdditionalDeployments

return nil
}
Expand Down
1 change: 1 addition & 0 deletions api/v1alpha1/zz_generated.conversion.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

18 changes: 18 additions & 0 deletions api/v1alpha2/provider_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,24 @@ type ProviderSpec struct {
// This should be an inline yaml blob-string https://datatracker.ietf.org/doc/html/rfc7396
// +optional
ManifestPatches []string `json:"manifestPatches,omitempty"`

// AdditionalDeployments is a map of additional deployments that the provider
// should manage. The key is the name of the deployment and the value is the
// DeploymentSpec.
// +optional
AdditionalDeployments map[string]AdditionalDeployments `json:"additionalDeployments,omitempty"`
}

// AdditionalDeployments defines the properties that can be enabled on the controller
// manager and deployment for the provider if the provider is managing additional deployments.
type AdditionalDeployments struct {
// Manager defines the properties that can be enabled on the controller manager for the additional provider deployment.
// +optional
Manager *ManagerSpec `json:"manager,omitempty"`

// Deployment defines the properties that can be enabled on the deployment for the additional provider deployment.
// +optional
Deployment *DeploymentSpec `json:"deployment,omitempty"`
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexander-demicev not sure if this is the best way to do it, but I refactored the code to include an AdditionalDeployments field that maps the name of the deployment to the Manager and Deployment spec, where the additional deployments can be customized. Is this a good way to go about it?

}

// ConfigmapReference contains enough information to locate the configmap.
Expand Down
32 changes: 32 additions & 0 deletions api/v1alpha2/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1,393 changes: 1,393 additions & 0 deletions config/crd/bases/operator.cluster.x-k8s.io_addonproviders.yaml

Large diffs are not rendered by default.

1,393 changes: 1,393 additions & 0 deletions config/crd/bases/operator.cluster.x-k8s.io_bootstrapproviders.yaml

Large diffs are not rendered by default.

1,393 changes: 1,393 additions & 0 deletions config/crd/bases/operator.cluster.x-k8s.io_controlplaneproviders.yaml

Large diffs are not rendered by default.

1,393 changes: 1,393 additions & 0 deletions config/crd/bases/operator.cluster.x-k8s.io_coreproviders.yaml

Large diffs are not rendered by default.

1,393 changes: 1,393 additions & 0 deletions config/crd/bases/operator.cluster.x-k8s.io_infrastructureproviders.yaml

Large diffs are not rendered by default.

1,393 changes: 1,393 additions & 0 deletions config/crd/bases/operator.cluster.x-k8s.io_ipamproviders.yaml

Large diffs are not rendered by default.

1,393 changes: 1,393 additions & 0 deletions config/crd/bases/operator.cluster.x-k8s.io_runtimeextensionproviders.yaml

Large diffs are not rendered by default.

16 changes: 15 additions & 1 deletion hack/charts/cluster-api-operator/templates/infra.yaml
Original file line number Diff line number Diff line change
@@ -1,3 +1,13 @@
{{- define "recursivePrinter" }}
{{- range $key, $value := . }}
{{- if kindIs "map" $value }}
{{ $key }}:
{{- include "recursivePrinter" $value | indent 2 }}
{{- else }}
{{ $key }}: {{ $value }}
{{- end }}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function allows the chart to print out additionalDeployments correctly in yaml format as both deployments and manager has nested fields.

{{- end }}
{{- end }}
# Infrastructure providers
{{- if .Values.infrastructure }}
{{- $infrastructures := split ";" .Values.infrastructure }}
Expand Down Expand Up @@ -40,7 +50,7 @@ metadata:
"helm.sh/hook": "post-install"
"helm.sh/hook-weight": "2"
"argocd.argoproj.io/sync-wave": "2"
{{- if or $infrastructureVersion $.Values.configSecret.name $.Values.manager }}
{{- if or $infrastructureVersion $.Values.configSecret.name $.Values.manager $.Values.additionalDeployments }}
spec:
{{- end }}
{{- if $infrastructureVersion }}
Expand All @@ -66,5 +76,9 @@ spec:
namespace: {{ $.Values.configSecret.namespace }}
{{- end }}
{{- end }}
{{- if $.Values.additionalDeployments }}
additionalDeployments:
{{- include "recursivePrinter" $.Values.additionalDeployments | indent 2 }}
{{- end }}
{{- end }}
{{- end }}
54 changes: 34 additions & 20 deletions internal/controller/component_customizer.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,25 +73,41 @@ func customizeObjectsFn(provider operatorv1.GenericProvider) func(objs []unstruc
}))
}

//nolint:nestif
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if a nolint is allowed here. I'll have to create a few helper functions if not. lmk which approach is better!

if o.GetKind() == deploymentKind {
d := &appsv1.Deployment{}
if err := scheme.Scheme.Convert(&o, d, nil); err != nil {
return nil, err
}

providerDeployment := provider.GetSpec().Deployment
providerManager := provider.GetSpec().Manager

// If there are multiple deployments, check if we specify customizations for those deployments.
// 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
// and the deployment name doesn't follow "ca*-controller-manager" pattern, or the provider
// doesn't specify customizations for the deployment.
// 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
additionalDeployments := provider.GetSpec().AdditionalDeployments
// Skip the deployment if there are no additional deployments specified.
if additionalDeployments == nil {
results = append(results, o)
continue
}

additionalProviderCustomization, ok := additionalDeployments[o.GetName()]
if !ok {
results = append(results, o)
continue
}

providerDeployment = additionalProviderCustomization.Deployment
providerManager = additionalProviderCustomization.Manager
}

d := &appsv1.Deployment{}
if err := scheme.Scheme.Convert(&o, d, nil); err != nil {
return nil, err
}

if err := customizeDeployment(provider.GetSpec(), d); err != nil {
if err := customizeDeployment(providerDeployment, providerManager, d); err != nil {
return nil, err
}

Expand All @@ -108,28 +124,26 @@ 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 {
func customizeDeployment(dSpec *operatorv1.DeploymentSpec, mSpec *operatorv1.ManagerSpec, d *appsv1.Deployment) error {
// Customize deployment spec first.
if pSpec.Deployment != nil {
customizeDeploymentSpec(pSpec, d)
if dSpec != nil {
customizeDeploymentSpec(*dSpec, d)
}

// Run the customizeManagerContainer after, so it overrides anything in the deploymentSpec.
if pSpec.Manager != nil {
if mSpec != nil {
container := findManagerContainer(&d.Spec)
if container == nil {
return fmt.Errorf("cannot find %q container in deployment %q", managerContainerName, d.Name)
}

customizeManagerContainer(pSpec.Manager, container)
customizeManagerContainer(mSpec, container)
}

return nil
}

func customizeDeploymentSpec(pSpec operatorv1.ProviderSpec, d *appsv1.Deployment) {
dSpec := pSpec.Deployment

func customizeDeploymentSpec(dSpec operatorv1.DeploymentSpec, d *appsv1.Deployment) {
if dSpec.Replicas != nil {
d.Spec.Replicas = ptr.To(int32(*dSpec.Replicas))
}
Expand Down
51 changes: 46 additions & 5 deletions internal/controller/component_customizer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -557,10 +557,7 @@ func TestCustomizeDeployment(t *testing.T) {
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
deployment := managerDepl.DeepCopy()
if err := customizeDeployment(operatorv1.ProviderSpec{
Deployment: tc.inputDeploymentSpec,
Manager: tc.inputManagerSpec,
}, deployment); err != nil {
if err := customizeDeployment(tc.inputDeploymentSpec, tc.inputManagerSpec, deployment); err != nil {
t.Error(err)
}

Expand All @@ -575,6 +572,7 @@ func TestCustomizeMultipleDeployment(t *testing.T) {
tests := []struct {
name string
nonManagerDeploymentName string
shouldCustomize bool
}{
{
name: "name without suffix and prefix",
Expand All @@ -588,6 +586,11 @@ func TestCustomizeMultipleDeployment(t *testing.T) {
name: "name with suffix",
nonManagerDeploymentName: "non-manager-controller-manager",
},
{
name: "name with azureserviceoperator controller-manager",
nonManagerDeploymentName: "azureserviceoperator-controller-manager",
shouldCustomize: true,
},
{
name: "empty name",
nonManagerDeploymentName: "",
Expand All @@ -603,6 +606,17 @@ func TestCustomizeMultipleDeployment(t *testing.T) {
},
Spec: appsv1.DeploymentSpec{
Replicas: ptr.To(int32(3)),
Template: corev1.PodTemplateSpec{
Spec: corev1.PodSpec{
Containers: []corev1.Container{
{
Name: "manager",
Image: "registry.k8s.io/a-manager:1.6.2",
Args: []string{},
},
},
},
},
},
}

Expand All @@ -629,6 +643,23 @@ func TestCustomizeMultipleDeployment(t *testing.T) {
Deployment: &operatorv1.DeploymentSpec{
Replicas: ptr.To(10),
},
AdditionalDeployments: map[string]operatorv1.AdditionalDeployments{
"azureserviceoperator-controller-manager": {
Deployment: &operatorv1.DeploymentSpec{
Containers: []operatorv1.ContainerSpec{
{
Name: "manager",
Args: map[string]string{
"--crd-pattern": ".*",
},
},
},
},
Manager: &operatorv1.ManagerSpec{
Verbosity: 1,
},
},
},
},
},
}
Expand Down Expand Up @@ -657,8 +688,18 @@ func TestCustomizeMultipleDeployment(t *testing.T) {
t.Errorf("expected 10 replicas, got %d", *managerDepl.Spec.Replicas)
}

if tc.shouldCustomize {
// non-manager container should have been customized
container := findManagerContainer(&nonManagerDepl.Spec)
if container == nil {
t.Error("expected container to be found")
} else if container.Args != nil && container.Args[0] != "--crd-pattern=.*" {
t.Errorf("expected --crd-pattern=.*, got %s", container.Args[0])
}
}

// non-manager deployment should not have been customized
if *nonManagerDepl.Spec.Replicas != 3 {
if *nonManagerDepl.Spec.Replicas != 3 && !tc.shouldCustomize {
t.Errorf("expected 3 replicas, got %d", *nonManagerDepl.Spec.Replicas)
}
})
Expand Down
Loading