From a7c99ca6a159e5529eadaa9b43f45456ef25a53b Mon Sep 17 00:00:00 2001 From: Mustafa Abdelrahman Date: Mon, 20 Apr 2026 19:00:10 +0200 Subject: [PATCH] worker: prevent SSL policy update loop on shared load balancers In https://github.com/zalando-incubator/kube-ingress-aws-controller/pull/774, update LB in-place was introduced. However, it was also introducing the following bug -> Ingress A has a custom SSL policy -> Ingress B uses the default SSL policy -> Since Ingress B doesn't have the annotation we will allow it through as `(ingress.HasSSLPolicyAnnotation && l.sslPolicy != ingress.SSLPolicy)` doesn't apply -> So we add both ingresses to LB then later override LB SSL policy with new ingress SSL policy (default one) Later `inSync` will revert to the old SSL policy and we go into infinite loop. Adding the new tag will help keep in-place updates, while blocking mismatch ingresses from landing on the same LB Signed-off-by: Mustafa Abdelrahman --- aws/adapter.go | 6 +- aws/cf.go | 25 ++++++-- aws/cf_test.go | 111 ++++++++++++++++++++++++++++++++ aws/fake/cf.go | 12 ++-- worker.go | 34 ++++++---- worker_ssl_policy_test.go | 132 +++++++++++++++++++++++++++++++++++++- worker_test.go | 72 +++++++++++++++++++++ 7 files changed, 365 insertions(+), 27 deletions(-) diff --git a/aws/adapter.go b/aws/adapter.go index 8e9ab2d5..54937a78 100644 --- a/aws/adapter.go +++ b/aws/adapter.go @@ -812,7 +812,7 @@ func (a *Adapter) UpdateTargetGroupsAndAutoScalingGroups(ctx context.Context, st // All the required resources (listeners and target group) are created in a // transactional fashion. // Failure to create the stack causes it to be deleted automatically. -func (a *Adapter) CreateStack(ctx context.Context, certificateARNs []string, scheme, securityGroup, owner, sslPolicy, ipAddressType, wafWebACLID string, cwAlarms CloudWatchAlarmList, loadBalancerType string, http2 bool) (string, error) { +func (a *Adapter) CreateStack(ctx context.Context, certificateARNs []string, scheme, securityGroup, owner, sslPolicy, ipAddressType, wafWebACLID string, cwAlarms CloudWatchAlarmList, loadBalancerType string, http2 bool, sslPolicyIsExplicit bool) (string, error) { certARNs := make(map[string]time.Time, len(certificateARNs)) for _, arn := range certificateARNs { certARNs[arn] = time.Time{} @@ -859,6 +859,7 @@ func (a *Adapter) CreateStack(ctx context.Context, certificateARNs []string, sch deregistrationDelayTimeoutSeconds: uint(a.deregistrationDelayTimeout.Seconds()), controllerID: a.controllerID, sslPolicy: sslPolicy, + sslPolicyIsExplicit: sslPolicyIsExplicit, ipAddressType: ipAddressType, targetGroupIPAddressType: a.targetGroupIPAddressType, loadbalancerType: loadBalancerType, @@ -886,7 +887,7 @@ func (a *Adapter) CreateStack(ctx context.Context, certificateARNs []string, sch return createStack(ctx, a.cloudformation, spec) } -func (a *Adapter) UpdateStack(ctx context.Context, stackName string, certificateARNs map[string]time.Time, scheme, securityGroup, owner, sslPolicy, ipAddressType, wafWebACLID string, cwAlarms CloudWatchAlarmList, loadBalancerType string, http2 bool) (string, error) { +func (a *Adapter) UpdateStack(ctx context.Context, stackName string, certificateARNs map[string]time.Time, scheme, securityGroup, owner, sslPolicy, ipAddressType, wafWebACLID string, cwAlarms CloudWatchAlarmList, loadBalancerType string, http2 bool, sslPolicyIsExplicit bool) (string, error) { if _, ok := SSLPolicies[sslPolicy]; !ok { return "", fmt.Errorf("invalid SSLPolicy '%s' defined", sslPolicy) } @@ -924,6 +925,7 @@ func (a *Adapter) UpdateStack(ctx context.Context, stackName string, certificate deregistrationDelayTimeoutSeconds: uint(a.deregistrationDelayTimeout.Seconds()), controllerID: a.controllerID, sslPolicy: sslPolicy, + sslPolicyIsExplicit: sslPolicyIsExplicit, ipAddressType: ipAddressType, targetGroupIPAddressType: a.targetGroupIPAddressType, loadbalancerType: loadBalancerType, diff --git a/aws/cf.go b/aws/cf.go index dfe4f0a9..f11377a8 100644 --- a/aws/cf.go +++ b/aws/cf.go @@ -13,10 +13,11 @@ import ( ) const ( - certificateARNTagLegacy = "ingress:certificate-arn" - certificateARNTagPrefix = "ingress:certificate-arn/" - ingressOwnerTag = "ingress:owner" - cwAlarmConfigHashTag = "cloudwatch:alarm-config-hash" + certificateARNTagLegacy = "ingress:certificate-arn" + certificateARNTagPrefix = "ingress:certificate-arn/" + ingressOwnerTag = "ingress:owner" + cwAlarmConfigHashTag = "cloudwatch:alarm-config-hash" + sslPolicyExplicitTag = "ingress:ssl-policy-explicit" ) // Stack is a simple wrapper around a CloudFormation Stack. @@ -29,6 +30,7 @@ type Stack struct { Scheme string SecurityGroup string SSLPolicy string + SSLPolicyIsExplicit bool IpAddressType string LoadBalancerType string HTTP2 bool @@ -184,6 +186,7 @@ type stackSpec struct { deregistrationDelayTimeoutSeconds uint controllerID string sslPolicy string + sslPolicyIsExplicit bool ipAddressType string targetGroupIPAddressType string loadbalancerType string @@ -294,6 +297,10 @@ func createStack(ctx context.Context, svc CloudFormationAPI, spec *stackSpec) (s params.Tags = append(params.Tags, cfTag(cwAlarmConfigHashTag, spec.cwAlarms.Hash())) } + if spec.sslPolicyIsExplicit { + params.Tags = append(params.Tags, cfTag(sslPolicyExplicitTag, "true")) + } + resp, err := svc.CreateStack(ctx, params) if err != nil { return spec.name, err @@ -368,6 +375,10 @@ func updateStack(ctx context.Context, svc CloudFormationAPI, spec *stackSpec) (s params.Tags = append(params.Tags, cfTag(cwAlarmConfigHashTag, spec.cwAlarms.Hash())) } + if spec.sslPolicyIsExplicit { + params.Tags = append(params.Tags, cfTag(sslPolicyExplicitTag, "true")) + } + if spec.stackTerminationProtection { params := &cloudformation.UpdateTerminationProtectionInput{ StackName: aws.String(spec.name), @@ -472,6 +483,7 @@ func mapToManagedStack(stack *types.Stack) *Stack { certificateARNs := make(map[string]time.Time, len(tags)) ownerIngress := "" + sslPolicyIsExplicit := false for key, value := range tags { if strings.HasPrefix(key, certificateARNTagPrefix) { arn := strings.TrimPrefix(key, certificateARNTagPrefix) @@ -491,6 +503,10 @@ func mapToManagedStack(stack *types.Stack) *Stack { if key == ingressOwnerTag { ownerIngress = value } + + if key == sslPolicyExplicitTag && value == "true" { + sslPolicyIsExplicit = true + } } http2 := true @@ -511,6 +527,7 @@ func mapToManagedStack(stack *types.Stack) *Stack { Scheme: parameters[parameterLoadBalancerSchemeParameter], SecurityGroup: parameters[parameterLoadBalancerSecurityGroupParameter], SSLPolicy: parameters[parameterListenerSslPolicyParameter], + SSLPolicyIsExplicit: sslPolicyIsExplicit, IpAddressType: parameters[parameterIpAddressTypeParameter], LoadBalancerType: parameters[parameterLoadBalancerTypeParameter], HTTP2: http2, diff --git a/aws/cf_test.go b/aws/cf_test.go index 504a78f4..9dd3a121 100644 --- a/aws/cf_test.go +++ b/aws/cf_test.go @@ -933,6 +933,117 @@ func TestShouldDelete(t *testing.T) { } +func TestSSLPolicyExplicitTag(t *testing.T) { + t.Run("createStack writes tag when sslPolicyIsExplicit=true", func(t *testing.T) { + c := &fake.CFClient{Outputs: fake.CFOutputs{ + CreateStack: fake.R(fake.MockCSOutput("stack-id"), nil), + }} + _, err := createStack(context.Background(), c, &stackSpec{ + name: "foo", + securityGroupID: "sg-1", + vpcID: "vpc-1", + sslPolicy: "ELBSecurityPolicy-TLS-1-2-2017-01", + sslPolicyIsExplicit: true, + }) + assert.NoError(t, err) + tags := convertCloudFormationTags(c.GetTagCreationHistory()[0]) + assert.Equal(t, "true", tags[sslPolicyExplicitTag], "sslPolicyExplicitTag must be written when sslPolicyIsExplicit=true") + }) + + t.Run("createStack omits tag when sslPolicyIsExplicit=false", func(t *testing.T) { + c := &fake.CFClient{Outputs: fake.CFOutputs{ + CreateStack: fake.R(fake.MockCSOutput("stack-id"), nil), + }} + _, err := createStack(context.Background(), c, &stackSpec{ + name: "foo", + securityGroupID: "sg-1", + vpcID: "vpc-1", + sslPolicy: "ELBSecurityPolicy-TLS-1-2-2017-01", + sslPolicyIsExplicit: false, + }) + assert.NoError(t, err) + tags := convertCloudFormationTags(c.GetTagCreationHistory()[0]) + _, present := tags[sslPolicyExplicitTag] + assert.False(t, present, "sslPolicyExplicitTag must NOT be written when sslPolicyIsExplicit=false") + }) + + t.Run("updateStack writes tag when sslPolicyIsExplicit=true", func(t *testing.T) { + c := &fake.CFClient{Outputs: fake.CFOutputs{ + UpdateStack: fake.R(fake.MockUSOutput("stack-id"), nil), + }} + _, err := updateStack(context.Background(), c, &stackSpec{ + name: "foo", + securityGroupID: "sg-1", + vpcID: "vpc-1", + sslPolicy: "ELBSecurityPolicy-TLS-1-2-2017-01", + sslPolicyIsExplicit: true, + }) + assert.NoError(t, err) + tags := convertCloudFormationTags(c.GetTagUpdateHistory()[0]) + assert.Equal(t, "true", tags[sslPolicyExplicitTag], "sslPolicyExplicitTag must be written on update when sslPolicyIsExplicit=true") + }) + + t.Run("updateStack omits tag when sslPolicyIsExplicit=false", func(t *testing.T) { + c := &fake.CFClient{Outputs: fake.CFOutputs{ + UpdateStack: fake.R(fake.MockUSOutput("stack-id"), nil), + }} + _, err := updateStack(context.Background(), c, &stackSpec{ + name: "foo", + securityGroupID: "sg-1", + vpcID: "vpc-1", + sslPolicy: "ELBSecurityPolicy-TLS-1-2-2017-01", + sslPolicyIsExplicit: false, + }) + assert.NoError(t, err) + tags := convertCloudFormationTags(c.GetTagUpdateHistory()[0]) + _, present := tags[sslPolicyExplicitTag] + assert.False(t, present, "sslPolicyExplicitTag must NOT be written on update when sslPolicyIsExplicit=false") + }) + + t.Run("mapToManagedStack sets SSLPolicyIsExplicit=true when tag present", func(t *testing.T) { + c := &fake.CFClient{Outputs: fake.CFOutputs{ + DescribeStacks: fake.R(&cloudformation.DescribeStacksOutput{ + Stacks: []types.Stack{ + { + StackName: aws.String("my-stack"), + StackStatus: types.StackStatusCreateComplete, + Tags: []types.Tag{ + cfTag(kubernetesCreatorTag, DefaultControllerID), + cfTag(clusterIDTagPrefix+"test-cluster", resourceLifecycleOwned), + cfTag(sslPolicyExplicitTag, "true"), + }, + }, + }, + }, nil), + }} + stacks, err := findManagedStacks(context.Background(), c, "test-cluster", DefaultControllerID) + assert.NoError(t, err) + assert.Len(t, stacks, 1) + assert.True(t, stacks[0].SSLPolicyIsExplicit, "SSLPolicyIsExplicit must be true when tag is present") + }) + + t.Run("mapToManagedStack sets SSLPolicyIsExplicit=false when tag absent", func(t *testing.T) { + c := &fake.CFClient{Outputs: fake.CFOutputs{ + DescribeStacks: fake.R(&cloudformation.DescribeStacksOutput{ + Stacks: []types.Stack{ + { + StackName: aws.String("my-stack"), + StackStatus: types.StackStatusCreateComplete, + Tags: []types.Tag{ + cfTag(kubernetesCreatorTag, DefaultControllerID), + cfTag(clusterIDTagPrefix+"test-cluster", resourceLifecycleOwned), + }, + }, + }, + }, nil), + }} + stacks, err := findManagedStacks(context.Background(), c, "test-cluster", DefaultControllerID) + assert.NoError(t, err) + assert.Len(t, stacks, 1) + assert.False(t, stacks[0].SSLPolicyIsExplicit, "SSLPolicyIsExplicit must be false when tag is absent") + }) +} + func TestNLBTargetGroupAttributes(t *testing.T) { for _, ti := range []struct { name string diff --git a/aws/fake/cf.go b/aws/fake/cf.go index 1fcc415e..16fcff6b 100644 --- a/aws/fake/cf.go +++ b/aws/fake/cf.go @@ -21,6 +21,7 @@ type CFClient struct { templateCreationHistory []string paramCreationHistory [][]types.Parameter tagCreationHistory [][]types.Tag + tagUpdateHistory [][]types.Tag Outputs CFOutputs } @@ -39,6 +40,7 @@ func (m *CFClient) GetTagCreationHistory() [][]types.Tag { func (m *CFClient) CleanCreationHistory() { m.paramCreationHistory = [][]types.Parameter{} m.tagCreationHistory = [][]types.Tag{} + m.tagUpdateHistory = [][]types.Tag{} m.templateCreationHistory = []string{} } @@ -84,10 +86,8 @@ func MockCSOutput(stackId string) *cloudformation.CreateStackOutput { } } -func (m *CFClient) UpdateStack(context.Context, *cloudformation.UpdateStackInput, ...func(*cloudformation.Options)) (*cloudformation.UpdateStackOutput, error) { - // TODO: https://github.com/zalando-incubator/kube-ingress-aws-controller/issues/653 - // Update stack needs to use different variable to register change history, - // so createStack and updateStack mocks don't mess with each other states. +func (m *CFClient) UpdateStack(_ context.Context, params *cloudformation.UpdateStackInput, _ ...func(*cloudformation.Options)) (*cloudformation.UpdateStackOutput, error) { + m.tagUpdateHistory = append(m.tagUpdateHistory, params.Tags) out, ok := m.Outputs.UpdateStack.response.(*cloudformation.UpdateStackOutput) if !ok { @@ -96,6 +96,10 @@ func (m *CFClient) UpdateStack(context.Context, *cloudformation.UpdateStackInput return out, m.Outputs.UpdateStack.err } +func (m *CFClient) GetTagUpdateHistory() [][]types.Tag { + return m.tagUpdateHistory +} + func MockUSOutput(stackId string) *cloudformation.UpdateStackOutput { return &cloudformation.UpdateStackOutput{ StackId: aws.String(stackId), diff --git a/worker.go b/worker.go index dd73f9ff..30107d70 100644 --- a/worker.go +++ b/worker.go @@ -48,6 +48,7 @@ type loadBalancer struct { clusterLocal bool securityGroup string sslPolicy string + sslPolicyIsExplicit bool ipAddressType string wafWebACLID string certTTL time.Duration @@ -130,10 +131,13 @@ func (l *loadBalancer) addIngress(certificateARNs []string, ingress *kubernetes. return false } - // settings that can be changed on an existing load balancer if it's - // NOT shared. + // settings that can be changed on an existing load balancer if it's NOT + // shared. For shared LBs the SSL policy guard only fires when at least one + // side has an explicit annotation, which prevents an unannotated ingress + // from overwriting an explicitly-set policy while still + // allowing in-place updates when both sides use the global default. if ingress.Shared && (l.securityGroup != ingress.SecurityGroup || - (ingress.HasSSLPolicyAnnotation && l.sslPolicy != ingress.SSLPolicy) || + (l.sslPolicy != ingress.SSLPolicy && (ingress.HasSSLPolicyAnnotation || l.sslPolicyIsExplicit)) || l.wafWebACLID != ingress.WAFWebACLID) { return false } @@ -394,6 +398,7 @@ func getAllLoadBalancers(certs CertificatesFinder, certTTL time.Duration, stackL shared: sl.Stack.OwnerIngress == "", securityGroup: sl.Stack.SecurityGroup, sslPolicy: sl.Stack.SSLPolicy, + sslPolicyIsExplicit: sl.Stack.SSLPolicyIsExplicit, ipAddressType: sl.Stack.IpAddressType, loadBalancerType: sl.Stack.LoadBalancerType, http2: sl.Stack.HTTP2, @@ -481,15 +486,16 @@ func matchIngressesToLoadBalancers( loadBalancers = append( loadBalancers, &loadBalancer{ - ingresses: i, - scheme: ingress.Scheme, - shared: ingress.Shared, - securityGroup: ingress.SecurityGroup, - sslPolicy: ingress.SSLPolicy, - ipAddressType: ingress.IPAddressType, - loadBalancerType: ingress.LoadBalancerType, - http2: ingress.HTTP2, - wafWebACLID: ingress.WAFWebACLID, + ingresses: i, + scheme: ingress.Scheme, + shared: ingress.Shared, + securityGroup: ingress.SecurityGroup, + sslPolicy: ingress.SSLPolicy, + sslPolicyIsExplicit: ingress.HasSSLPolicyAnnotation, + ipAddressType: ingress.IPAddressType, + loadBalancerType: ingress.LoadBalancerType, + http2: ingress.HTTP2, + wafWebACLID: ingress.WAFWebACLID, }, ) } @@ -547,7 +553,7 @@ func (w *worker) createStack(ctx context.Context, lb *loadBalancer, problems *pr log.Infof("Creating stack for certificates %q / ingress %q", certificates, lb.ingresses) - stackId, err := w.awsAdapter.CreateStack(ctx, certificates, lb.scheme, lb.securityGroup, lb.Owner(), lb.sslPolicy, lb.ipAddressType, lb.wafWebACLID, lb.cwAlarms, lb.loadBalancerType, lb.http2) + stackId, err := w.awsAdapter.CreateStack(ctx, certificates, lb.scheme, lb.securityGroup, lb.Owner(), lb.sslPolicy, lb.ipAddressType, lb.wafWebACLID, lb.cwAlarms, lb.loadBalancerType, lb.http2, lb.sslPolicyIsExplicit) if err != nil { if isAlreadyExistsError(err) { lb.stack, err = w.awsAdapter.GetStack(ctx, stackId) @@ -567,7 +573,7 @@ func (w *worker) updateStack(ctx context.Context, lb *loadBalancer, problems *pr log.Infof("Updating %q stack for %d certificates / %d ingresses", lb.scheme, len(certificates), len(lb.ingresses)) - stackId, err := w.awsAdapter.UpdateStack(ctx, lb.stack.Name, certificates, lb.scheme, lb.securityGroup, lb.Owner(), lb.sslPolicy, lb.ipAddressType, lb.wafWebACLID, lb.cwAlarms, lb.loadBalancerType, lb.http2) + stackId, err := w.awsAdapter.UpdateStack(ctx, lb.stack.Name, certificates, lb.scheme, lb.securityGroup, lb.Owner(), lb.sslPolicy, lb.ipAddressType, lb.wafWebACLID, lb.cwAlarms, lb.loadBalancerType, lb.http2, lb.sslPolicyIsExplicit) if isNoUpdatesToBePerformedError(err) { log.Debugf("Stack(%q) is already up to date", certificates) } else if err != nil { diff --git a/worker_ssl_policy_test.go b/worker_ssl_policy_test.go index d6f9af68..84c8aff6 100644 --- a/worker_ssl_policy_test.go +++ b/worker_ssl_policy_test.go @@ -19,10 +19,52 @@ func TestSSLPolicyAnnotationLoadBalancerMatching(t *testing.T) { description string }{ { - name: "shared LB - no annotation - should share despite different policy", + name: "shared LB - explicit policy on LB - no annotation on ingress - different policy - should NOT share", + existingLB: &loadBalancer{ + shared: true, + sslPolicy: "ELBSecurityPolicy-2016-08", + sslPolicyIsExplicit: true, // LB was created with an explicit annotation + ingresses: make(map[string][]*kubernetes.Ingress), + scheme: "internet-facing", + loadBalancerType: aws.LoadBalancerTypeApplication, + }, + incomingIngress: &kubernetes.Ingress{ + SSLPolicy: "ELBSecurityPolicy-TLS-1-2-2017-01", + HasSSLPolicyAnnotation: false, // Using default from command line + Shared: true, + Scheme: "internet-facing", + LoadBalancerType: aws.LoadBalancerTypeApplication, + }, + maxCerts: 10, + shouldAdd: false, + description: "Ingress without annotation must NOT share LB whose policy was set explicitly; adding it would overwrite the LB policy and cause an update loop", + }, + { + name: "shared LB - default policy on LB - no annotation on ingress - different policy - should share (allows in-place update)", + existingLB: &loadBalancer{ + shared: true, + sslPolicy: "ELBSecurityPolicy-2016-08", + sslPolicyIsExplicit: false, // LB was created with the global default + ingresses: make(map[string][]*kubernetes.Ingress), + scheme: "internet-facing", + loadBalancerType: aws.LoadBalancerTypeApplication, + }, + incomingIngress: &kubernetes.Ingress{ + SSLPolicy: "ELBSecurityPolicy-TLS-1-2-2017-01", + HasSSLPolicyAnnotation: false, // Using global default (which changed) + Shared: true, + Scheme: "internet-facing", + LoadBalancerType: aws.LoadBalancerTypeApplication, + }, + maxCerts: 10, + shouldAdd: true, + description: "When both sides use the global default and the default changes, the unannotated ingress should join the existing LB for an in-place update", + }, + { + name: "shared LB - no annotation - same policy - should share", existingLB: &loadBalancer{ shared: true, - sslPolicy: "ELBSecurityPolicy-2016-08", + sslPolicy: "ELBSecurityPolicy-TLS-1-2-2017-01", ingresses: make(map[string][]*kubernetes.Ingress), scheme: "internet-facing", loadBalancerType: aws.LoadBalancerTypeApplication, @@ -36,7 +78,7 @@ func TestSSLPolicyAnnotationLoadBalancerMatching(t *testing.T) { }, maxCerts: 10, shouldAdd: true, - description: "Ingress without annotation should share LB even with different SSL policy", + description: "Ingress without annotation should share LB when SSL policies are identical", }, { name: "shared LB - with annotation - matching policy - should share", @@ -78,6 +120,90 @@ func TestSSLPolicyAnnotationLoadBalancerMatching(t *testing.T) { shouldAdd: false, description: "Ingress with different SSL policy annotation should NOT share LB", }, + { + name: "shared LB - default policy on LB - annotated ingress - different policy - should NOT share", + existingLB: &loadBalancer{ + shared: true, + sslPolicy: "ELBSecurityPolicy-2016-08", + sslPolicyIsExplicit: false, // LB was created with the global default + ingresses: make(map[string][]*kubernetes.Ingress), + scheme: "internet-facing", + loadBalancerType: aws.LoadBalancerTypeApplication, + }, + incomingIngress: &kubernetes.Ingress{ + SSLPolicy: "ELBSecurityPolicy-TLS-1-2-2017-01", + HasSSLPolicyAnnotation: true, // Ingress has an explicit annotation + Shared: true, + Scheme: "internet-facing", + LoadBalancerType: aws.LoadBalancerTypeApplication, + }, + maxCerts: 10, + shouldAdd: false, + description: "Annotated ingress must NOT join a default-policy LB when policies differ", + }, + { + name: "shared LB - both sides explicit - same policy - should share", + existingLB: &loadBalancer{ + shared: true, + sslPolicy: "ELBSecurityPolicy-TLS-1-2-2017-01", + sslPolicyIsExplicit: true, + ingresses: make(map[string][]*kubernetes.Ingress), + scheme: "internet-facing", + loadBalancerType: aws.LoadBalancerTypeApplication, + }, + incomingIngress: &kubernetes.Ingress{ + SSLPolicy: "ELBSecurityPolicy-TLS-1-2-2017-01", + HasSSLPolicyAnnotation: true, + Shared: true, + Scheme: "internet-facing", + LoadBalancerType: aws.LoadBalancerTypeApplication, + }, + maxCerts: 10, + shouldAdd: true, + description: "Two ingresses with identical explicit SSL policy annotations should share the LB", + }, + { + name: "shared LB - both sides explicit - different policy - should NOT share", + existingLB: &loadBalancer{ + shared: true, + sslPolicy: "ELBSecurityPolicy-2016-08", + sslPolicyIsExplicit: true, + ingresses: make(map[string][]*kubernetes.Ingress), + scheme: "internet-facing", + loadBalancerType: aws.LoadBalancerTypeApplication, + }, + incomingIngress: &kubernetes.Ingress{ + SSLPolicy: "ELBSecurityPolicy-TLS-1-2-2017-01", + HasSSLPolicyAnnotation: true, + Shared: true, + Scheme: "internet-facing", + LoadBalancerType: aws.LoadBalancerTypeApplication, + }, + maxCerts: 10, + shouldAdd: false, + description: "Two ingresses with different explicit SSL policy annotations must NOT share the LB", + }, + { + name: "shared LB - both sides default - same policy - should share", + existingLB: &loadBalancer{ + shared: true, + sslPolicy: "ELBSecurityPolicy-TLS-1-2-2017-01", + sslPolicyIsExplicit: false, + ingresses: make(map[string][]*kubernetes.Ingress), + scheme: "internet-facing", + loadBalancerType: aws.LoadBalancerTypeApplication, + }, + incomingIngress: &kubernetes.Ingress{ + SSLPolicy: "ELBSecurityPolicy-TLS-1-2-2017-01", + HasSSLPolicyAnnotation: false, + Shared: true, + Scheme: "internet-facing", + LoadBalancerType: aws.LoadBalancerTypeApplication, + }, + maxCerts: 10, + shouldAdd: true, + description: "Two unannotated ingresses with the same global default policy should share the LB", + }, { name: "non-shared LB - always allows different SSL policies", existingLB: &loadBalancer{ diff --git a/worker_test.go b/worker_test.go index 44bd3341..9a0ed488 100644 --- a/worker_test.go +++ b/worker_test.go @@ -9,6 +9,7 @@ import ( "net/http/httptest" "os" "reflect" + "sort" "sync" "testing" "time" @@ -2017,6 +2018,77 @@ func TestBuildModel(t *testing.T) { } }, }, + { + title: "SSL policy update loop: unannotated ingress must not overwrite non-default policy on existing shared LB", + certs: func() CertificatesFinder { + const certARN = "arn:aws:acm:eu-central-1:123456789012:certificate/abc" + ca, err := certsfake.NewCA() + if err != nil { + panic(err) + } + cs, err := ca.NewCertificateSummary(certARN) + if err != nil { + panic(err) + } + return certsfake.NewCert([]*certs.CertificateSummary{cs}) + }(), + ingresses: []*kubernetes.Ingress{ + { + Namespace: "default", + Name: "ingress-a", + Shared: true, + Scheme: "internet-facing", + SSLPolicy: "ELBSecurityPolicy-TLS-1-2-2017-01", + HasSSLPolicyAnnotation: true, + LoadBalancerType: aws.LoadBalancerTypeApplication, + CertificateARN: "arn:aws:acm:eu-central-1:123456789012:certificate/abc", + HTTP2: true, + }, + { + Namespace: "default", + Name: "ingress-b", + Shared: true, + Scheme: "internet-facing", + SSLPolicy: "ELBSecurityPolicy-2016-08", + HasSSLPolicyAnnotation: false, + LoadBalancerType: aws.LoadBalancerTypeApplication, + CertificateARN: "arn:aws:acm:eu-central-1:123456789012:certificate/abc", + HTTP2: true, + }, + }, + stacks: []*aws.StackLBState{{ + Stack: &aws.Stack{ + Name: "kube-ingress-aws-stack-a", + Scheme: "internet-facing", + SSLPolicy: "ELBSecurityPolicy-TLS-1-2-2017-01", + SSLPolicyIsExplicit: true, // stack was created with an explicit annotation + CertificateARNs: map[string]time.Time{ + "arn:aws:acm:eu-central-1:123456789012:certificate/abc": {}, + }, + LoadBalancerType: aws.LoadBalancerTypeApplication, + HTTP2: true, + }, + }}, + validate: func(t *testing.T, lbs []*loadBalancer) { + var generatedLBs []*loadBalancer + for _, lb := range lbs { + if !lb.clusterLocal { + generatedLBs = append(generatedLBs, lb) + } + } + require.Len(t, generatedLBs, 2, "ingressA and ingressB must each get their own LB") + + sort.Slice(generatedLBs, func(i, j int) bool { + return generatedLBs[i].stack != nil && generatedLBs[j].stack == nil && generatedLBs[i].stack.Name < generatedLBs[j].stack.Name + }) + + lbA, lbB := generatedLBs[0], generatedLBs[1] + assert.Equal(t, "ELBSecurityPolicy-TLS-1-2-2017-01", lbA.sslPolicy, "existing LB must retain non-default SSL policy") + assert.Equal(t, "ELBSecurityPolicy-2016-08", lbB.sslPolicy, "new LB for ingressB must use the default SSL policy") + lbA.cwAlarms = aws.CloudWatchAlarmList{} + assert.True(t, lbA.inSync(), "existing LB must be in-sync; drift would trigger an update and restart the loop") + }, + }, { title: "with global and ingress defined WAF", ingresses: []*kubernetes.Ingress{{