Skip to content

worker: prevent SSL policy update loop on shared load balancers #788

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.

Sign up for GitHub

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

Merged
MustafaSaber merged 1 commit into from
Apr 23, 2026
Merged

worker: prevent SSL policy update loop on shared load balancers #788

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 4 additions & 2 deletions aws/adapter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{}
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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,
Expand Down
25 changes: 21 additions & 4 deletions aws/cf.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -29,6 +30,7 @@ type Stack struct {
Scheme string
SecurityGroup string
SSLPolicy string
SSLPolicyIsExplicit bool
IpAddressType string
LoadBalancerType string
HTTP2 bool
Expand Down Expand Up @@ -184,6 +186,7 @@ type stackSpec struct {
deregistrationDelayTimeoutSeconds uint
controllerID string
sslPolicy string
sslPolicyIsExplicit bool
ipAddressType string
targetGroupIPAddressType string
loadbalancerType string
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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)
Expand All @@ -491,6 +503,10 @@ func mapToManagedStack(stack *types.Stack) *Stack {
if key == ingressOwnerTag {
ownerIngress = value
}

if key == sslPolicyExplicitTag && value == "true" {
sslPolicyIsExplicit = true
}
}

http2 := true
Expand All @@ -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,
Expand Down
111 changes: 111 additions & 0 deletions aws/cf_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
12 changes: 8 additions & 4 deletions aws/fake/cf.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ type CFClient struct {
templateCreationHistory []string
paramCreationHistory [][]types.Parameter
tagCreationHistory [][]types.Tag
tagUpdateHistory [][]types.Tag
Outputs CFOutputs
}

Expand All @@ -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{}
}

Expand Down Expand Up @@ -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 {
Expand All @@ -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),
Expand Down
34 changes: 20 additions & 14 deletions worker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ type loadBalancer struct {
clusterLocal bool
securityGroup string
sslPolicy string
sslPolicyIsExplicit bool
ipAddressType string
wafWebACLID string
certTTL time.Duration
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
},
)
}
Expand Down Expand Up @@ -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)
Expand All @@ -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 {
Expand Down
Loading
Loading