-
Notifications
You must be signed in to change notification settings - Fork 588
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
✨ Supporting externally managed Control Plane #4438
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -298,5 +298,49 @@ func (r *AWSCluster) validateControlPlaneLBs() field.ErrorList { | |
} | ||
} | ||
|
||
if r.Spec.ControlPlaneLoadBalancer.LoadBalancerType == LoadBalancerTypeDisabled { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we validating somewhere that the |
||
if r.Spec.ControlPlaneLoadBalancer.Name != nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm wondering if we need a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "controlPlaneLoadBalancer", "name"), r.Spec.ControlPlaneLoadBalancer.Name, "cannot configure a name if the LoadBalancer reconciliation is disabled")) | ||
} | ||
|
||
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")) | ||
} | ||
} | ||
Comment on lines
+306
to
+343
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
||
return allErrs | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -266,6 +266,45 @@ func (r *AWSClusterReconciler) reconcileDelete(ctx context.Context, clusterScope | |
return nil | ||
} | ||
|
||
func (r *AWSClusterReconciler) reconcileLoadBalancer(clusterScope *scope.ClusterScope, awsCluster *infrav1.AWSCluster) (*time.Duration, error) { | ||
retryAfterDuration := 15 * time.Second | ||
Comment on lines
+269
to
+270
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we rework this to just return an |
||
if clusterScope.AWSCluster.Spec.ControlPlaneLoadBalancer.LoadBalancerType == infrav1.LoadBalancerTypeDisabled { | ||
clusterScope.Debug("load balancer reconciliation shifted to external provider, checking external endpoint") | ||
|
||
return r.checkForExternalControlPlaneLoadBalancer(clusterScope, awsCluster), nil | ||
} | ||
|
||
elbService := r.getELBService(clusterScope) | ||
|
||
if err := elbService.ReconcileLoadbalancers(); err != nil { | ||
clusterScope.Error(err, "failed to reconcile load balancer") | ||
conditions.MarkFalse(awsCluster, infrav1.LoadBalancerReadyCondition, infrav1.LoadBalancerFailedReason, infrautilconditions.ErrorConditionAfterInit(clusterScope.ClusterObj()), err.Error()) | ||
return nil, err | ||
} | ||
|
||
if awsCluster.Status.Network.APIServerELB.DNSName == "" { | ||
conditions.MarkFalse(awsCluster, infrav1.LoadBalancerReadyCondition, infrav1.WaitForDNSNameReason, clusterv1.ConditionSeverityInfo, "") | ||
clusterScope.Info("Waiting on API server ELB DNS name") | ||
return &retryAfterDuration, nil | ||
} | ||
|
||
clusterScope.Debug("Looking up IP address for DNS", "dns", awsCluster.Status.Network.APIServerELB.DNSName) | ||
if _, err := net.LookupIP(awsCluster.Status.Network.APIServerELB.DNSName); err != nil { | ||
clusterScope.Error(err, "failed to get IP address for dns name", "dns", awsCluster.Status.Network.APIServerELB.DNSName) | ||
conditions.MarkFalse(awsCluster, infrav1.LoadBalancerReadyCondition, infrav1.WaitForDNSNameResolveReason, clusterv1.ConditionSeverityInfo, "") | ||
clusterScope.Info("Waiting on API server ELB DNS name to resolve") | ||
return &retryAfterDuration, nil | ||
} | ||
conditions.MarkTrue(awsCluster, infrav1.LoadBalancerReadyCondition) | ||
|
||
awsCluster.Spec.ControlPlaneEndpoint = clusterv1.APIEndpoint{ | ||
Host: awsCluster.Status.Network.APIServerELB.DNSName, | ||
Port: clusterScope.APIServerPort(), | ||
} | ||
|
||
return nil, nil | ||
} | ||
|
||
func (r *AWSClusterReconciler) reconcileNormal(clusterScope *scope.ClusterScope) (reconcile.Result, error) { | ||
clusterScope.Info("Reconciling AWSCluster") | ||
|
||
|
@@ -280,7 +319,6 @@ func (r *AWSClusterReconciler) reconcileNormal(clusterScope *scope.ClusterScope) | |
} | ||
|
||
ec2Service := r.getEC2Service(clusterScope) | ||
elbService := r.getELBService(clusterScope) | ||
networkSvc := r.getNetworkService(*clusterScope) | ||
sgService := r.getSecurityGroupService(*clusterScope) | ||
s3Service := s3.NewService(clusterScope) | ||
|
@@ -310,37 +348,17 @@ func (r *AWSClusterReconciler) reconcileNormal(clusterScope *scope.ClusterScope) | |
} | ||
} | ||
|
||
if err := elbService.ReconcileLoadbalancers(); err != nil { | ||
clusterScope.Error(err, "failed to reconcile load balancer") | ||
conditions.MarkFalse(awsCluster, infrav1.LoadBalancerReadyCondition, infrav1.LoadBalancerFailedReason, infrautilconditions.ErrorConditionAfterInit(clusterScope.ClusterObj()), err.Error()) | ||
if requeueAfter, err := r.reconcileLoadBalancer(clusterScope, awsCluster); err != nil { | ||
return reconcile.Result{}, err | ||
} else if requeueAfter != nil { | ||
return reconcile.Result{RequeueAfter: *requeueAfter}, err | ||
} | ||
|
||
if err := s3Service.ReconcileBucket(); err != nil { | ||
conditions.MarkFalse(awsCluster, infrav1.S3BucketReadyCondition, infrav1.S3BucketFailedReason, clusterv1.ConditionSeverityError, err.Error()) | ||
return reconcile.Result{}, errors.Wrapf(err, "failed to reconcile S3 Bucket for AWSCluster %s/%s", awsCluster.Namespace, awsCluster.Name) | ||
} | ||
|
||
if awsCluster.Status.Network.APIServerELB.DNSName == "" { | ||
conditions.MarkFalse(awsCluster, infrav1.LoadBalancerReadyCondition, infrav1.WaitForDNSNameReason, clusterv1.ConditionSeverityInfo, "") | ||
clusterScope.Info("Waiting on API server ELB DNS name") | ||
return reconcile.Result{RequeueAfter: 15 * time.Second}, nil | ||
} | ||
|
||
clusterScope.Debug("looking up IP address for DNS", "dns", awsCluster.Status.Network.APIServerELB.DNSName) | ||
if _, err := net.LookupIP(awsCluster.Status.Network.APIServerELB.DNSName); err != nil { | ||
clusterScope.Error(err, "failed to get IP address for dns name", "dns", awsCluster.Status.Network.APIServerELB.DNSName) | ||
conditions.MarkFalse(awsCluster, infrav1.LoadBalancerReadyCondition, infrav1.WaitForDNSNameResolveReason, clusterv1.ConditionSeverityInfo, "") | ||
clusterScope.Info("Waiting on API server ELB DNS name to resolve") | ||
return reconcile.Result{RequeueAfter: 15 * time.Second}, nil | ||
} | ||
conditions.MarkTrue(awsCluster, infrav1.LoadBalancerReadyCondition) | ||
|
||
awsCluster.Spec.ControlPlaneEndpoint = clusterv1.APIEndpoint{ | ||
Host: awsCluster.Status.Network.APIServerELB.DNSName, | ||
Port: clusterScope.APIServerPort(), | ||
} | ||
|
||
for _, subnet := range clusterScope.Subnets().FilterPrivate() { | ||
found := false | ||
for _, az := range awsCluster.Status.Network.APIServerELB.AvailabilityZones { | ||
|
@@ -447,3 +465,29 @@ func (r *AWSClusterReconciler) requeueAWSClusterForUnpausedCluster(_ context.Con | |
} | ||
} | ||
} | ||
|
||
func (r *AWSClusterReconciler) checkForExternalControlPlaneLoadBalancer(clusterScope *scope.ClusterScope, awsCluster *infrav1.AWSCluster) *time.Duration { | ||
requeueAfterPeriod := 15 * time.Second | ||
|
||
Comment on lines
+469
to
+471
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above, the retry after should be outside in a caller |
||
switch { | ||
case len(awsCluster.Spec.ControlPlaneEndpoint.Host) == 0 && awsCluster.Spec.ControlPlaneEndpoint.Port == 0: | ||
clusterScope.Info("AWSCluster control plane endpoint is still non-populated") | ||
conditions.MarkFalse(awsCluster, infrav1.LoadBalancerReadyCondition, infrav1.WaitForExternalControlPlaneEndpointReason, clusterv1.ConditionSeverityInfo, "") | ||
|
||
return &requeueAfterPeriod | ||
case len(awsCluster.Spec.ControlPlaneEndpoint.Host) == 0: | ||
clusterScope.Info("AWSCluster control plane endpoint host is still non-populated") | ||
conditions.MarkFalse(awsCluster, infrav1.LoadBalancerReadyCondition, infrav1.WaitForExternalControlPlaneEndpointReason, clusterv1.ConditionSeverityInfo, "") | ||
|
||
return &requeueAfterPeriod | ||
case awsCluster.Spec.ControlPlaneEndpoint.Port == 0: | ||
clusterScope.Info("AWSCluster control plane endpoint port is still non-populated") | ||
conditions.MarkFalse(awsCluster, infrav1.LoadBalancerReadyCondition, infrav1.WaitForExternalControlPlaneEndpointReason, clusterv1.ConditionSeverityInfo, "") | ||
|
||
return &requeueAfterPeriod | ||
default: | ||
conditions.MarkTrue(awsCluster, infrav1.LoadBalancerReadyCondition) | ||
|
||
return nil | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add a comment on what each value for this does and what the new value is useful for?