✨ Supporting externally managed Control Plane #4438

prometherion · 2023-08-04T13:15:16Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Cluster API allows defining externally managed control plane for the given cluster, the current code-base doesn't consider this since it nevertheless performs the following actions:

reconciliation of a Load Balancer, although already provisioned by the ControlPlane provider
blocking the EC2 creation since it requires an ELB DNS name

Which issue(s) this PR fixes:
Fixes #4437

Special notes for your reviewer:

Checklist:

squashed commits
includes documentation
adds unit tests
adds or updates e2e tests

Release note:

A new enum value, `disabled`, is added for the `AWSCluster.spec.controlPlaneLoadBalancer.loadBalancerType` field, which skips the reconciliation of the load balancer for the given cluster, useful for clusters which are consuming an externally managed Control Plane.

k8s-ci-robot · 2023-08-04T13:15:24Z

Welcome @prometherion!

It looks like this is your first PR to kubernetes-sigs/cluster-api-provider-aws 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api-provider-aws has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2023-08-04T13:15:25Z

Hi @prometherion. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pkg/cloud/scope/machine.go

vincepri · 2023-08-07T16:44:35Z

/ok-to-test

controllers/awscluster_controller.go

controllers/awsmachine_controller.go

prometherion · 2023-08-10T16:58:03Z

If the proposed approach is ok I can work on proper unit-tests for this.

controllers/awsmachine_controller.go

prometherion · 2024-02-03T15:56:34Z

/retest

richardcase · 2024-02-04T12:02:36Z

/milestone v2.4.0

richardcase · 2024-02-04T12:08:15Z

Until #4733 merges

/hold

But this looks good to me, pending any changes required after 4733.

Signed-off-by: Dario Tranchitella <[email protected]>

prometherion · 2024-02-04T16:21:12Z

/test pull-cluster-api-provider-aws-e2e
/test pull-cluster-api-provider-aws-e2e-eks

prometherion · 2024-02-04T18:28:01Z

Thanks, @richardcase, rebased as requested.

The failed test seems unrelated to the changes we introduced.

richardcase · 2024-02-07T08:36:46Z

Thanks @prometherion

/unhold

richardcase · 2024-02-07T08:37:28Z

/test pull-cluster-api-provider-aws-e2e

faiq · 2024-02-07T16:00:20Z

/lgtm

faiq · 2024-02-07T16:00:49Z

the tests are all passing, and the code looks good. i think this is ready to merge

richardcase · 2024-02-04T12:06:49Z

controllers/awsmachine_controller.go

@@ -1178,3 +1186,22 @@ func (r *AWSMachineReconciler) ensureInstanceMetadataOptions(ec2svc services.EC2

 	return ec2svc.ModifyInstanceMetadataOptions(instance.ID, machine.Spec.InstanceMetadataOptions)
 }
+
+// +kubebuilder:rbac:groups=controlplane.cluster.x-k8s.io,resources=*,verbs=get;list;watch


I'm undecided whether we should have all these in one place.

TBH, this wildcard makes obsolete the following ones:

cluster-api-provider-aws/controllers/rosacluster_controller.go

Line 55 in 6193d17

// +kubebuilder:rbac:groups=controlplane.cluster.x-k8s.io,resources=rosacontrolplanes;rosacontrolplanes/status,verbs=get;list;watch

cluster-api-provider-aws/bootstrap/eks/controllers/eksconfig_controller.go

Line 62 in 6193d17

// +kubebuilder:rbac:groups=controlplane.cluster.x-k8s.io,resources=awsmanagedcontrolplanes,verbs=get;list;watch

cluster-api-provider-aws/controllers/awsmanagedcluster_controller.go

Line 54 in 6193d17

// +kubebuilder:rbac:groups=controlplane.cluster.x-k8s.io,resources=awsmanagedcontrolplanes;awsmanagedcontrolplanes/status,verbs=get;list;watch

cluster-api-provider-aws/controllers/rosacluster_controller.go

Line 55 in 6193d17

// +kubebuilder:rbac:groups=controlplane.cluster.x-k8s.io,resources=rosacontrolplanes;rosacontrolplanes/status,verbs=get;list;watch

...and many other lines.

Although we could optimize the role rules, I think these are more intuitive for developers, since it's pretty straightforward trying to understand the objects controlled by the various controllers.

Let's move these to a single place above

richardcase · 2024-02-08T15:55:03Z

api/v1beta2/awscluster_webhook.go

@@ -298,5 +298,49 @@ func (r *AWSCluster) validateControlPlaneLBs() field.ErrorList {
 		}
 	}

+	if r.Spec.ControlPlaneLoadBalancer.LoadBalancerType == LoadBalancerTypeDisabled {
+		if r.Spec.ControlPlaneLoadBalancer.Name != nil {


I'm wondering if we need a func Empty() bool on the load balancer instead of checking each sub field. But lets discuss that separately.

We still would need the check of each subfield, unfortunately. We could slender the webhook file, of course, but from a UX perspective, knowing which field is breaking the validation would be helpful.

richardcase · 2024-02-08T15:56:06Z

/approve

k8s-ci-robot · 2024-02-08T15:56:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: richardcase

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [richardcase]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

vincepri · 2024-02-26T19:15:32Z

api/v1beta2/awscluster_types.go

@@ -229,7 +230,7 @@ type AWSLoadBalancerSpec struct {

 	// LoadBalancerType sets the type for a load balancer. The default type is classic.


Could we add a comment on what each value for this does and what the new value is useful for?

vincepri · 2024-02-26T19:18:22Z

api/v1beta2/awscluster_webhook.go

@@ -298,5 +298,49 @@ func (r *AWSCluster) validateControlPlaneLBs() field.ErrorList {
 		}
 	}

+	if r.Spec.ControlPlaneLoadBalancer.LoadBalancerType == LoadBalancerTypeDisabled {


Are we validating somewhere that the type cannot be changed once created? How are we dealing right now if a user creates the type = disabled and then wants alb and vice-versa?

vincepri · 2024-02-26T19:19:29Z

api/v1beta2/awscluster_webhook.go

+		if r.Spec.ControlPlaneLoadBalancer.CrossZoneLoadBalancing {
+			allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "controlPlaneLoadBalancer", "crossZoneLoadBalancing"), r.Spec.ControlPlaneLoadBalancer.CrossZoneLoadBalancing, "cross-zone load balancing cannot be set if the LoadBalancer reconciliation is disabled"))
+		}
+
+		if len(r.Spec.ControlPlaneLoadBalancer.Subnets) > 0 {
+			allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "controlPlaneLoadBalancer", "subnets"), r.Spec.ControlPlaneLoadBalancer.Subnets, "subnets cannot be set if the LoadBalancer reconciliation is disabled"))
+		}
+
+		if r.Spec.ControlPlaneLoadBalancer.HealthCheckProtocol != nil {
+			allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "controlPlaneLoadBalancer", "healthCheckProtocol"), r.Spec.ControlPlaneLoadBalancer.HealthCheckProtocol, "healthcheck protocol cannot be set if the LoadBalancer reconciliation is disabled"))
+		}
+
+		if len(r.Spec.ControlPlaneLoadBalancer.AdditionalSecurityGroups) > 0 {
+			allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "controlPlaneLoadBalancer", "additionalSecurityGroups"), r.Spec.ControlPlaneLoadBalancer.AdditionalSecurityGroups, "additional Security Groups cannot be set if the LoadBalancer reconciliation is disabled"))
+		}
+
+		if len(r.Spec.ControlPlaneLoadBalancer.AdditionalListeners) > 0 {
+			allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "controlPlaneLoadBalancer", "additionalListeners"), r.Spec.ControlPlaneLoadBalancer.AdditionalListeners, "cannot set additional listeners if the LoadBalancer reconciliation is disabled"))
+		}
+
+		if len(r.Spec.ControlPlaneLoadBalancer.IngressRules) > 0 {
+			allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "controlPlaneLoadBalancer", "ingressRules"), r.Spec.ControlPlaneLoadBalancer.IngressRules, "ingress rules cannot be set if the LoadBalancer reconciliation is disabled"))
+		}
+
+		if r.Spec.ControlPlaneLoadBalancer.PreserveClientIP {
+			allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "controlPlaneLoadBalancer", "preserveClientIP"), r.Spec.ControlPlaneLoadBalancer.PreserveClientIP, "cannot preserve client IP if the LoadBalancer reconciliation is disabled"))
+		}
+
+		if r.Spec.ControlPlaneLoadBalancer.DisableHostsRewrite {
+			allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "controlPlaneLoadBalancer", "disableHostsRewrite"), r.Spec.ControlPlaneLoadBalancer.DisableHostsRewrite, "cannot disable hosts rewrite if the LoadBalancer reconciliation is disabled"))
+		}
+	}
+
+	for _, rule := range r.Spec.ControlPlaneLoadBalancer.IngressRules {
+		if (rule.CidrBlocks != nil || rule.IPv6CidrBlocks != nil) && (rule.SourceSecurityGroupIDs != nil || rule.SourceSecurityGroupRoles != nil) {
+			allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "controlPlaneLoadBalancer", "ingressRules"), r.Spec.ControlPlaneLoadBalancer.IngressRules, "CIDR blocks and security group IDs or security group roles cannot be used together"))
+		}
+	}


We should open an issue to track for the next API version to rework this, it seems to me that we're going around the current API structure to disable the control plane management

vincepri · 2024-02-26T19:20:33Z

controllers/awscluster_controller.go

+func (r *AWSClusterReconciler) reconcileLoadBalancer(clusterScope *scope.ClusterScope, awsCluster *infrav1.AWSCluster) (*time.Duration, error) {
+	retryAfterDuration := 15 * time.Second


Could we rework this to just return an error and leave the retryAfterDuration outside?

vincepri · 2024-02-26T19:20:53Z

controllers/awscluster_controller.go

+func (r *AWSClusterReconciler) checkForExternalControlPlaneLoadBalancer(clusterScope *scope.ClusterScope, awsCluster *infrav1.AWSCluster) *time.Duration {
+	requeueAfterPeriod := 15 * time.Second
+


Same as above, the retry after should be outside in a caller

vincepri · 2024-02-26T19:21:53Z

controllers/awsmachine_controller.go

+	cp, err := r.getControlPlane(ctx, log, cluster)
+	if err != nil {
+		return ctrl.Result{}, err
+	}


It seems we're missing that the control plane is an optional reference, and this is going to panic when nil

Potentially could be solved with #4817.

vincepri · 2024-02-26T19:22:05Z

controllers/awsmachine_controller.go

@@ -1178,3 +1186,22 @@ func (r *AWSMachineReconciler) ensureInstanceMetadataOptions(ec2svc services.EC2

 	return ec2svc.ModifyInstanceMetadataOptions(instance.ID, machine.Spec.InstanceMetadataOptions)
 }
+
+// +kubebuilder:rbac:groups=controlplane.cluster.x-k8s.io,resources=*,verbs=get;list;watch


Let's move these to a single place above

vincepri · 2024-02-26T19:23:14Z

pkg/cloud/scope/machine.go

@@ -371,6 +377,10 @@ func (m *MachineScope) IsEKSManaged() bool {
 	return m.InfraCluster.InfraCluster().GetObjectKind().GroupVersionKind().Kind == ekscontrolplanev1.AWSManagedControlPlaneKind
 }

+func (m *MachineScope) IsControlPlaneExternallyManaged() bool {


A bit confusing on why this is needed, the control plane reference can be anything, including a new CRD; why do we need to mark it as externally managed?

…ues/4437" This reverts commit 6193d17, reversing changes made to 6ffb575.

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority labels Aug 4, 2023

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 4, 2023

k8s-ci-robot requested review from dthorsen and shivi28 August 4, 2023 13:15

prometherion commented Aug 4, 2023

View reviewed changes

pkg/cloud/scope/machine.go Show resolved Hide resolved

prometherion mentioned this pull request Aug 4, 2023

Supporting AWS CAPI clusters clastix/cluster-api-control-plane-provider-kamaji#46

Closed

prometherion force-pushed the issues/4437 branch 2 times, most recently from 58dca4e to e36b61e Compare August 4, 2023 16:03

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 7, 2023

vincepri reviewed Aug 7, 2023

View reviewed changes

controllers/awscluster_controller.go Outdated Show resolved Hide resolved

controllers/awscluster_controller.go Outdated Show resolved Hide resolved

controllers/awscluster_controller.go Outdated Show resolved Hide resolved

vincepri reviewed Aug 7, 2023

View reviewed changes

controllers/awsmachine_controller.go Outdated Show resolved Hide resolved

prometherion force-pushed the issues/4437 branch from 41f95ad to db16163 Compare August 8, 2023 14:59

prometherion requested a review from vincepri August 10, 2023 16:58

prometherion force-pushed the issues/4437 branch 3 times, most recently from 369c894 to 64fca56 Compare August 11, 2023 11:44

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 5, 2023

Skarlso reviewed Sep 21, 2023

View reviewed changes

controllers/awsmachine_controller.go Show resolved Hide resolved

prometherion force-pushed the issues/4437 branch from 64fca56 to fe32fd4 Compare October 2, 2023 12:23

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 2, 2023

prometherion force-pushed the issues/4437 branch from fe32fd4 to a4b2f1e Compare October 2, 2023 12:23

k8s-ci-robot added this to the v2.4.0 milestone Feb 4, 2024

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 4, 2024

prometherion force-pushed the issues/4437 branch from 3bb5d3a to 6b4fec7 Compare February 4, 2024 15:26

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 4, 2024

prometherion added 3 commits February 4, 2024 16:54

feat: allowing nodes creation when cp is externally managed

f7d3575

Signed-off-by: Dario Tranchitella <[email protected]>

feat: disabled load balancer enum for externally managed LBs

4ee9c54

Signed-off-by: Dario Tranchitella <[email protected]>

chore(log): capitalising debug message

fc3cb07

Signed-off-by: Dario Tranchitella <[email protected]>

prometherion force-pushed the issues/4437 branch from 6b4fec7 to fc3cb07 Compare February 4, 2024 15:55

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 7, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 7, 2024

richardcase reviewed Feb 8, 2024

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 8, 2024

k8s-ci-robot merged commit 6193d17 into kubernetes-sigs:main Feb 8, 2024
21 checks passed

prometherion deleted the issues/4437 branch February 9, 2024 10:42

vincepri reviewed Feb 26, 2024

View reviewed changes

nrb added a commit to nrb/cluster-api-provider-aws that referenced this pull request Feb 26, 2024

Revert "Merge pull request kubernetes-sigs#4438 from prometherion/iss…

768d360

…ues/4437" This reverts commit 6193d17, reversing changes made to 6ffb575.

nrb mentioned this pull request Feb 26, 2024

🐛 Revert "Supporting externally managed Control Plane" #4821

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Supporting externally managed Control Plane #4438

✨ Supporting externally managed Control Plane #4438

prometherion commented Aug 4, 2023 •

edited

Loading

k8s-ci-robot commented Aug 4, 2023

k8s-ci-robot commented Aug 4, 2023

vincepri commented Aug 7, 2023

prometherion commented Aug 10, 2023

prometherion commented Feb 3, 2024

richardcase commented Feb 4, 2024

richardcase commented Feb 4, 2024

prometherion commented Feb 4, 2024

prometherion commented Feb 4, 2024

richardcase commented Feb 7, 2024

richardcase commented Feb 7, 2024

faiq commented Feb 7, 2024

faiq commented Feb 7, 2024

richardcase Feb 4, 2024

prometherion Feb 9, 2024

vincepri Feb 26, 2024

richardcase Feb 8, 2024

prometherion Feb 9, 2024

richardcase commented Feb 8, 2024

k8s-ci-robot commented Feb 8, 2024

vincepri Feb 26, 2024

vincepri Feb 26, 2024

vincepri Feb 26, 2024

vincepri Feb 26, 2024

vincepri Feb 26, 2024

vincepri Feb 26, 2024

prometherion Feb 26, 2024

vincepri Feb 26, 2024

vincepri Feb 26, 2024

		@@ -229,7 +230,7 @@ type AWSLoadBalancerSpec struct {

		// LoadBalancerType sets the type for a load balancer. The default type is classic.

		func (r AWSClusterReconciler) reconcileLoadBalancer(clusterScope scope.ClusterScope, awsCluster infrav1.AWSCluster) (time.Duration, error) {
		retryAfterDuration := 15 * time.Second

		func (r AWSClusterReconciler) checkForExternalControlPlaneLoadBalancer(clusterScope scope.ClusterScope, awsCluster infrav1.AWSCluster) time.Duration {
		requeueAfterPeriod := 15 * time.Second

✨ Supporting externally managed Control Plane #4438

✨ Supporting externally managed Control Plane #4438

Conversation

prometherion commented Aug 4, 2023 • edited Loading

k8s-ci-robot commented Aug 4, 2023

k8s-ci-robot commented Aug 4, 2023

vincepri commented Aug 7, 2023

prometherion commented Aug 10, 2023

prometherion commented Feb 3, 2024

richardcase commented Feb 4, 2024

richardcase commented Feb 4, 2024

prometherion commented Feb 4, 2024

prometherion commented Feb 4, 2024

richardcase commented Feb 7, 2024

richardcase commented Feb 7, 2024

faiq commented Feb 7, 2024

faiq commented Feb 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardcase commented Feb 8, 2024

k8s-ci-robot commented Feb 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prometherion commented Aug 4, 2023 •

edited

Loading