From a4618b8d7bae4cca74bbf33fd095c51f782bf5de Mon Sep 17 00:00:00 2001 From: Sedef Savas Date: Wed, 6 Oct 2021 10:55:41 -0700 Subject: [PATCH] Fix for internet-facing ELB scheme --- api/v1alpha3/awscluster_types.go | 2 +- api/v1alpha3/types.go | 4 ++ api/v1alpha4/awscluster_types.go | 2 +- api/v1beta1/awscluster_webhook.go | 12 ++-- api/v1beta1/awscluster_webhook_test.go | 58 +++++++++++++++++++ api/v1beta1/defaults.go | 7 +++ api/v1beta1/network_types.go | 3 + ...tructure.cluster.x-k8s.io_awsclusters.yaml | 2 + ....cluster.x-k8s.io_awsclustertemplates.yaml | 1 + pkg/cloud/services/elb/loadbalancer.go | 8 +++ 10 files changed, 93 insertions(+), 6 deletions(-) diff --git a/api/v1alpha3/awscluster_types.go b/api/v1alpha3/awscluster_types.go index 6b6c8f779d..eb21758865 100644 --- a/api/v1alpha3/awscluster_types.go +++ b/api/v1alpha3/awscluster_types.go @@ -148,7 +148,7 @@ type Bastion struct { type AWSLoadBalancerSpec struct { // Scheme sets the scheme of the load balancer (defaults to internet-facing) // +kubebuilder:default=internet-facing - // +kubebuilder:validation:Enum=internet-facing;internal + // +kubebuilder:validation:Enum=internet-facing;Internet-facing;internal // +optional Scheme *ClassicELBScheme `json:"scheme,omitempty"` diff --git a/api/v1alpha3/types.go b/api/v1alpha3/types.go index 80fd2f9f69..d0a65b1c4b 100644 --- a/api/v1alpha3/types.go +++ b/api/v1alpha3/types.go @@ -96,6 +96,10 @@ var ( ClassicELBSchemeInternal = ClassicELBScheme("internal") ) +func (e ClassicELBScheme) String() string { + return string(e) +} + // ClassicELBProtocol defines listener protocols for a classic load balancer. type ClassicELBProtocol string diff --git a/api/v1alpha4/awscluster_types.go b/api/v1alpha4/awscluster_types.go index ba12ff0a55..7100a0f7d8 100644 --- a/api/v1alpha4/awscluster_types.go +++ b/api/v1alpha4/awscluster_types.go @@ -148,7 +148,7 @@ type Bastion struct { type AWSLoadBalancerSpec struct { // Scheme sets the scheme of the load balancer (defaults to internet-facing) // +kubebuilder:default=internet-facing - // +kubebuilder:validation:Enum=internet-facing;internal + // +kubebuilder:validation:Enum=internet-facing;Internet-facing;internal // +optional Scheme *ClassicELBScheme `json:"scheme,omitempty"` diff --git a/api/v1beta1/awscluster_webhook.go b/api/v1beta1/awscluster_webhook.go index 397562097f..779795b101 100644 --- a/api/v1beta1/awscluster_webhook.go +++ b/api/v1beta1/awscluster_webhook.go @@ -95,10 +95,14 @@ func (r *AWSCluster) ValidateUpdate(old runtime.Object) error { // If old scheme was not nil, the new scheme should be the same. existingLoadBalancer := oldC.Spec.ControlPlaneLoadBalancer.DeepCopy() if !reflect.DeepEqual(existingLoadBalancer.Scheme, newLoadBalancer.Scheme) { - allErrs = append(allErrs, - field.Invalid(field.NewPath("spec", "controlPlaneLoadBalancer", "scheme"), - r.Spec.ControlPlaneLoadBalancer.Scheme, "field is immutable"), - ) + // Only allow changes from Internet-facing scheme to internet-facing. + if !(existingLoadBalancer.Scheme.String() == ClassicELBSchemeIncorrectInternetFacing.String() && + newLoadBalancer.Scheme.String() == ClassicELBSchemeInternetFacing.String()) { + allErrs = append(allErrs, + field.Invalid(field.NewPath("spec", "controlPlaneLoadBalancer", "scheme"), + r.Spec.ControlPlaneLoadBalancer.Scheme, "field is immutable"), + ) + } } } diff --git a/api/v1beta1/awscluster_webhook_test.go b/api/v1beta1/awscluster_webhook_test.go index 6034af3ab1..367595919c 100644 --- a/api/v1beta1/awscluster_webhook_test.go +++ b/api/v1beta1/awscluster_webhook_test.go @@ -19,12 +19,14 @@ package v1beta1 import ( "context" "testing" + "time" . "github.com/onsi/gomega" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" utildefaulting "sigs.k8s.io/cluster-api/util/defaulting" + "sigs.k8s.io/controller-runtime/pkg/client" ) func TestAWSClusterDefault(t *testing.T) { @@ -36,12 +38,50 @@ func TestAWSClusterDefault(t *testing.T) { } func TestAWSCluster_ValidateCreate(t *testing.T) { + unsupportedIncorrectScheme := ClassicELBScheme("any-other-scheme") + tests := []struct { name string cluster *AWSCluster wantErr bool + expect func(t *testing.T, res *AWSLoadBalancerSpec) }{ // The SSHKeyName tests were moved to sshkeyname_test.go + { + name: "Default nil scheme to `internet-facing`", + cluster: &AWSCluster{ + Spec: AWSClusterSpec{}, + }, + expect: func(t *testing.T, res *AWSLoadBalancerSpec) { + if res.Scheme.String() != ClassicELBSchemeInternetFacing.String() { + t.Error("Expected internet-facing defaulting for nil loadbalancer schemes") + } + }, + wantErr: false, + }, + { + name: "Internet-facing ELB scheme is defaulted to internet-facing during creation", + cluster: &AWSCluster{ + Spec: AWSClusterSpec{ + ControlPlaneLoadBalancer: &AWSLoadBalancerSpec{Scheme: &ClassicELBSchemeIncorrectInternetFacing}, + }, + }, + expect: func(t *testing.T, res *AWSLoadBalancerSpec) { + if res.Scheme.String() != ClassicELBSchemeInternetFacing.String() { + t.Error("Expected internet-facing defaulting for supported incorrect scheme: Internet-facing") + } + }, + wantErr: false, + }, + { + name: "Supported schemes are 'internet-facing, Internet-facing, internal, or nil', rest will be rejected", + cluster: &AWSCluster{ + Spec: AWSClusterSpec{ + ControlPlaneLoadBalancer: &AWSLoadBalancerSpec{Scheme: &unsupportedIncorrectScheme}, + }, + }, + wantErr: true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -54,6 +94,24 @@ func TestAWSCluster_ValidateCreate(t *testing.T) { if err := testEnv.Create(ctx, cluster); (err != nil) != tt.wantErr { t.Errorf("ValidateCreate() error = %v, wantErr %v", err, tt.wantErr) } + + if tt.wantErr { + return + } + + c := &AWSCluster{} + key := client.ObjectKey{ + Name: cluster.Name, + Namespace: "default", + } + + g := NewWithT(t) + g.Eventually(func() bool { + err := testEnv.Get(ctx, key, c) + return err == nil + }, 10*time.Second).Should(Equal(true)) + + tt.expect(t, c.Spec.ControlPlaneLoadBalancer) }) } } diff --git a/api/v1beta1/defaults.go b/api/v1beta1/defaults.go index 6ae73172e9..aee55fda55 100644 --- a/api/v1beta1/defaults.go +++ b/api/v1beta1/defaults.go @@ -59,6 +59,13 @@ func SetDefaults_AWSClusterSpec(s *AWSClusterSpec) { //nolint:golint,stylecheck Name: AWSClusterControllerIdentityName, } } + + // If ELB scheme is set to Internet-facing due to an API bug in versions > v0.6.6 and v0.7.0, default it to internet-facing. + if s.ControlPlaneLoadBalancer == nil { + s.ControlPlaneLoadBalancer = &AWSLoadBalancerSpec{Scheme: &ClassicELBSchemeInternetFacing} + } else if s.ControlPlaneLoadBalancer.Scheme != nil && s.ControlPlaneLoadBalancer.Scheme.String() == ClassicELBSchemeIncorrectInternetFacing.String() { + s.ControlPlaneLoadBalancer.Scheme = &ClassicELBSchemeInternetFacing + } } // SetDefaults_Labels is used to default cluster scope resources for clusterctl move. diff --git a/api/v1beta1/network_types.go b/api/v1beta1/network_types.go index 98af1c4aed..362943ba34 100644 --- a/api/v1beta1/network_types.go +++ b/api/v1beta1/network_types.go @@ -42,6 +42,9 @@ var ( // ClassicELBSchemeInternal defines an internal-only facing // load balancer internal to an ELB. ClassicELBSchemeInternal = ClassicELBScheme("internal") + + // ClassicELBSchemeIncorrectInternetFacing was inaccurately used to define an internet-facing LB in v0.6 releases > v0.6.6 and v0.7.0 release. + ClassicELBSchemeIncorrectInternetFacing = ClassicELBScheme("Internet-facing") ) func (e ClassicELBScheme) String() string { diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml index 2adcf1ff8f..bcbf92c5e9 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml @@ -143,6 +143,7 @@ spec: to internet-facing) enum: - internet-facing + - Internet-facing - internal type: string subnets: @@ -891,6 +892,7 @@ spec: to internet-facing) enum: - internet-facing + - Internet-facing - internal type: string subnets: diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclustertemplates.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclustertemplates.yaml index 686166a28a..37729a5a83 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclustertemplates.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclustertemplates.yaml @@ -131,6 +131,7 @@ spec: (defaults to internet-facing) enum: - internet-facing + - Internet-facing - internal type: string subnets: diff --git a/pkg/cloud/services/elb/loadbalancer.go b/pkg/cloud/services/elb/loadbalancer.go index 64cc244b12..654473b4ae 100644 --- a/pkg/cloud/services/elb/loadbalancer.go +++ b/pkg/cloud/services/elb/loadbalancer.go @@ -316,6 +316,14 @@ func (s *Service) getAPIServerClassicELBSpec() (*infrav1.ClassicELB, error) { } securityGroupIDs = append(securityGroupIDs, s.scope.SecurityGroups()[infrav1.SecurityGroupAPIServerLB].ID) + // If ELB scheme is set to Internet-facing due to an API bug in versions > v0.6.6 and v0.7.0, change it to internet-facing and patch. + if s.scope.ControlPlaneLoadBalancerScheme().String() == infrav1.ClassicELBSchemeIncorrectInternetFacing.String() { + s.scope.ControlPlaneLoadBalancer().Scheme = &infrav1.ClassicELBSchemeInternetFacing + if err := s.scope.PatchObject(); err != nil { + return nil, err + } + } + res := &infrav1.ClassicELB{ Name: elbName, Scheme: s.scope.ControlPlaneLoadBalancerScheme(),