Skip to content

Commit 39cde7f

Browse files
authored
Merge pull request #441 from kubernetes-sigs/fix-attributes
Fixed attribute logic
2 parents d10d11a + e1a2a5b commit 39cde7f

File tree

9 files changed

+212
-246
lines changed

9 files changed

+212
-246
lines changed

docs/ingress-resources.md

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,6 @@ Required annotations are:
5454
alb.ingress.kubernetes.io/load-balancer-attributes
5555
alb.ingress.kubernetes.io/backend-protocol
5656
alb.ingress.kubernetes.io/certificate-arn
57-
alb.ingress.kubernetes.io/connection-idle-timeout
5857
alb.ingress.kubernetes.io/healthcheck-interval-seconds
5958
alb.ingress.kubernetes.io/healthcheck-path
6059
alb.ingress.kubernetes.io/healthcheck-port
@@ -81,8 +80,6 @@ Optional annotations are:
8180
8281
- **certificate-arn**: Enables HTTPS and uses the certificate defined, based on arn, stored in your [AWS Certificate Manager](https://aws.amazon.com/certificate-manager).
8382
84-
- **connection-idle-timeout**: Sets the connection idle timeout setting for the ALB. This is a global setting for the ALB.
85-
8683
- **healthcheck-interval-seconds**: The approximate amount of time, in seconds, between health checks of an individual target. The default is 15 seconds.
8784
8885
- **healthcheck-path**: The ping path that is the destination on the targets for health checks. The default is /.

pkg/alb/lb/loadbalancer.go

Lines changed: 70 additions & 132 deletions
Large diffs are not rendered by default.

pkg/alb/lb/types.go

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,6 @@ type options struct {
4444
}
4545

4646
type opts struct {
47-
idleTimeout *int64
4847
ports portList
4948
inboundCidrs util.Cidrs
5049
webACLId *string
@@ -61,7 +60,6 @@ const (
6160
schemeModified
6261
attributesModified
6362
managedSecurityGroupsModified
64-
connectionIdleTimeoutModified
6563
ipAddressTypeModified
6664
webACLAssociationModified
6765
)

pkg/alb/rs/rule_test.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -354,6 +354,8 @@ func TestRuleReconcile(t *testing.T) {
354354
}
355355

356356
func genTG(arn, svcname string) *tg.TargetGroup {
357+
albelbv2.ELBV2svc = mockedELBV2{}
358+
357359
t, _ := tg.NewCurrentTargetGroup(&tg.NewCurrentTargetGroupOptions{
358360
ALBNamePrefix: "pfx",
359361
LoadBalancerID: "nnnnn",
@@ -755,3 +757,7 @@ func (m mockedELBV2) ModifyRule(input *elbv2.ModifyRuleInput) (*elbv2.ModifyRule
755757
func (m mockedELBV2) DeleteRule(input *elbv2.DeleteRuleInput) (*elbv2.DeleteRuleOutput, error) {
756758
return &m.DeleteRuleOutput, m.DeleteRuleError
757759
}
760+
761+
func (m mockedELBV2) DescribeTargetGroupAttributesFiltered(s *string) (albelbv2.TargetGroupAttributes, error) {
762+
return nil, nil
763+
}

pkg/alb/tg/targetgroup.go

Lines changed: 23 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -121,12 +121,18 @@ func NewCurrentTargetGroup(o *NewCurrentTargetGroupOptions) (*TargetGroup, error
121121
return nil, fmt.Errorf("The Target Group %s does not have the proper tags, can't import: %s", *o.TargetGroup.TargetGroupArn, err.Error())
122122
}
123123

124+
attrs, err := albelbv2.ELBV2svc.DescribeTargetGroupAttributesFiltered(o.TargetGroup.TargetGroupArn)
125+
if err != nil {
126+
return nil, fmt.Errorf("Failed to retrieve attributes for Target Group. Error: %s", err.Error())
127+
}
128+
124129
return &TargetGroup{
125-
ID: id,
126-
SvcName: svcName,
127-
logger: o.Logger,
128-
tags: tags{current: tgTags},
129-
tg: tg{current: o.TargetGroup},
130+
ID: id,
131+
SvcName: svcName,
132+
logger: o.Logger,
133+
attributes: attributes{current: attrs},
134+
tags: tags{current: tgTags},
135+
tg: tg{current: o.TargetGroup},
130136
}, nil
131137
}
132138

@@ -234,7 +240,8 @@ func (t *TargetGroup) create(rOpts *ReconcileOptions) error {
234240
func (t *TargetGroup) modify(mods tgChange, rOpts *ReconcileOptions) error {
235241
desired := t.tg.desired
236242
if mods&paramsModified != 0 {
237-
in := &elbv2.ModifyTargetGroupInput{
243+
t.logger.Infof("Modifying target group parameters.")
244+
o, err := albelbv2.ELBV2svc.ModifyTargetGroup(&elbv2.ModifyTargetGroupInput{
238245
HealthCheckIntervalSeconds: desired.HealthCheckIntervalSeconds,
239246
HealthCheckPath: desired.HealthCheckPath,
240247
HealthCheckPort: desired.HealthCheckPort,
@@ -244,13 +251,11 @@ func (t *TargetGroup) modify(mods tgChange, rOpts *ReconcileOptions) error {
244251
Matcher: desired.Matcher,
245252
TargetGroupArn: t.CurrentARN(),
246253
UnhealthyThresholdCount: desired.UnhealthyThresholdCount,
247-
}
248-
o, err := albelbv2.ELBV2svc.ModifyTargetGroup(in)
254+
})
249255
if err != nil {
250256
rOpts.Eventf(api.EventTypeWarning, "ERROR", "Error modifying target group %s: %s", t.ID, err.Error())
251-
t.logger.Errorf("Failed TargetGroup modification. ARN: %s | Error: %s.",
257+
return fmt.Errorf("Failed TargetGroup modification. ARN: %s | Error: %s",
252258
*t.CurrentARN(), err.Error())
253-
return err
254259
}
255260
t.tg.current = o.TargetGroups[0]
256261
// AmazonAPI doesn't return an empty HealthCheckPath.
@@ -259,46 +264,45 @@ func (t *TargetGroup) modify(mods tgChange, rOpts *ReconcileOptions) error {
259264

260265
// check/change tags
261266
if mods&tagsModified != 0 {
267+
t.logger.Infof("Modifying target group tags.")
262268
if err := albelbv2.ELBV2svc.UpdateTags(t.CurrentARN(), t.tags.current, t.tags.desired); err != nil {
263269
rOpts.Eventf(api.EventTypeWarning, "ERROR", "Error changing tags on target group %s: %s", t.ID, err.Error())
264-
t.logger.Errorf("Failed TargetGroup modification. Unable to modify tags. ARN: %s | Error: %s.",
270+
return fmt.Errorf("Failed TargetGroup modification. Unable to modify tags. ARN: %s | Error: %s",
265271
*t.CurrentARN(), err.Error())
266-
return err
267272
}
268273
t.tags.current = t.tags.desired
269274
}
270275

271276
if mods&targetsModified != 0 {
277+
t.logger.Infof("Modifying target group targets.")
272278
additions := util.Difference(t.targets.desired, t.targets.current)
273279
removals := util.Difference(t.targets.current, t.targets.desired)
274280

275281
// check/change targets
276282
if len(additions) > 0 {
277283
if err := t.registerTargets(additions, rOpts); err != nil {
278284
rOpts.Eventf(api.EventTypeWarning, "ERROR", "Error adding targets to target group %s: %s", t.ID, err.Error())
279-
t.logger.Infof("Failed TargetGroup modification. Unable to add targets: %s.", err.Error())
280-
return err
285+
return fmt.Errorf("Failed TargetGroup modification. Unable to add targets: %s", err.Error())
281286
}
282287
}
283288
if len(removals) > 0 {
284289
if err := t.deregisterTargets(removals, rOpts); err != nil {
285290
rOpts.Eventf(api.EventTypeWarning, "ERROR", "Error removing targets from target group %s: %s", t.ID, err.Error())
286-
t.logger.Infof("Failed TargetGroup modification. Unable to remove targets: %s.", err.Error())
287-
return err
291+
return fmt.Errorf("Failed TargetGroup modification. Unable to remove targets: %s", err.Error())
288292
}
289293
}
290294
t.targets.current = t.targets.desired
291295
}
292296

293297
if mods&attributesModified != 0 {
298+
t.logger.Infof("Modifying target group attributes.")
294299
aOpts := &elbv2.ModifyTargetGroupAttributesInput{
295300
Attributes: t.attributes.desired,
296301
TargetGroupArn: t.CurrentARN(),
297302
}
298303
if _, err := albelbv2.ELBV2svc.ModifyTargetGroupAttributes(aOpts); err != nil {
299304
rOpts.Eventf(api.EventTypeWarning, "ERROR", "Error modifying attributes in target group %s: %s", t.ID, err.Error())
300-
t.logger.Infof("Failed TargetGroup modification. Unable to change attributes: %s.", err.Error())
301-
return err
305+
return fmt.Errorf("Failed TargetGroup modification. Unable to change attributes: %s", err.Error())
302306
}
303307
t.attributes.current = t.attributes.desired
304308
}
@@ -367,15 +371,12 @@ func (t *TargetGroup) needsModification() tgChange {
367371
changes |= tagsModified
368372
}
369373

370-
if !reflect.DeepEqual(t.attributes.current.Sorted(), t.attributes.desired.Sorted()) {
374+
if !reflect.DeepEqual(t.attributes.current.Filtered().Sorted(), t.attributes.desired.Filtered().Sorted()) {
371375
t.logger.Debugf("Attributes need to be changed")
372376
changes |= attributesModified
373377
}
374378

375379
return changes
376-
// These fields require a rebuild and are enforced via TG name hash
377-
// Port *int64 `min:"1" type:"integer"`
378-
// Protocol *string `type:"string" enum:"ProtocolEnum"`
379380
}
380381

381382
// Registers Targets (ec2 instances) to TargetGroup, must be called when Current != Desired

pkg/annotations/annotations.go

Lines changed: 3 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,6 @@ var cache = ccache.New(ccache.Configure())
2727
const (
2828
backendProtocolKey = "alb.ingress.kubernetes.io/backend-protocol"
2929
certificateArnKey = "alb.ingress.kubernetes.io/certificate-arn"
30-
connectionIdleTimeoutKey = "alb.ingress.kubernetes.io/connection-idle-timeout"
3130
webACLIdKey = "alb.ingress.kubernetes.io/web-acl-id"
3231
webACLIdAltKey = "alb.ingress.kubernetes.io/waf-acl-id"
3332
healthcheckIntervalSecondsKey = "alb.ingress.kubernetes.io/healthcheck-interval-seconds"
@@ -61,7 +60,6 @@ const (
6160
type Annotations struct {
6261
BackendProtocol *string
6362
CertificateArn *string
64-
ConnectionIdleTimeout *int64
6563
WebACLId *string
6664
HealthcheckIntervalSeconds *int64
6765
HealthcheckPath *string
@@ -82,7 +80,7 @@ type Annotations struct {
8280
TargetGroupAttributes albelbv2.TargetGroupAttributes
8381
SslPolicy *string
8482
VPCID *string
85-
LoadBalancerAttributes []*elbv2.LoadBalancerAttribute
83+
LoadBalancerAttributes albelbv2.LoadBalancerAttributes
8684
}
8785

8886
type PortData struct {
@@ -139,7 +137,6 @@ func (vf *ValidatingAnnotationFactory) ParseAnnotations(opts *ParseAnnotationsOp
139137
a := new(Annotations)
140138
for _, err := range []error{
141139
a.setBackendProtocol(annotations),
142-
a.setConnectionIdleTimeout(annotations),
143140
a.setCertificateArn(annotations, vf.validator),
144141
a.setHealthcheckIntervalSeconds(annotations),
145142
a.setHealthcheckPath(annotations),
@@ -171,7 +168,6 @@ func (vf *ValidatingAnnotationFactory) ParseAnnotations(opts *ParseAnnotationsOp
171168
}
172169

173170
func (a *Annotations) setLoadBalancerAttributes(annotations map[string]string) error {
174-
var attrs []*elbv2.LoadBalancerAttribute
175171
var badAttrs []string
176172
v, ok := annotations[loadbalancerAttributesKey]
177173
if !ok {
@@ -189,12 +185,8 @@ func (a *Annotations) setLoadBalancerAttributes(annotations map[string]string) e
189185
badAttrs = append(badAttrs, *rawAttr)
190186
continue
191187
}
192-
attrs = append(attrs, &elbv2.LoadBalancerAttribute{
193-
Key: aws.String(parts[0]),
194-
Value: aws.String(parts[1]),
195-
})
188+
a.LoadBalancerAttributes.Set(parts[0], parts[1])
196189
}
197-
a.LoadBalancerAttributes = attrs
198190

199191
if len(badAttrs) > 0 {
200192
return fmt.Errorf("Unable to parse `%s` into Key=Value pair(s)", strings.Join(badAttrs, ", "))
@@ -224,22 +216,6 @@ func (a *Annotations) setCertificateArn(annotations map[string]string, validator
224216
return nil
225217
}
226218

227-
func (a *Annotations) setConnectionIdleTimeout(annotations map[string]string) error {
228-
i, err := strconv.ParseInt(annotations[connectionIdleTimeoutKey], 10, 64)
229-
if err != nil {
230-
if annotations[connectionIdleTimeoutKey] != "" {
231-
return err
232-
}
233-
return nil
234-
}
235-
// aws only accepts a range of 1-3600 seconds
236-
if i < 1 || i > 3600 {
237-
return fmt.Errorf("Invalid connection idle timeout provided must be within 1-3600 seconds. Was: %d", i)
238-
}
239-
a.ConnectionIdleTimeout = aws.Int64(i)
240-
return nil
241-
}
242-
243219
func (a *Annotations) setHealthcheckIntervalSeconds(annotations map[string]string) error {
244220
i, err := strconv.ParseInt(annotations[healthcheckIntervalSecondsKey], 10, 64)
245221
if err != nil {
@@ -677,16 +653,11 @@ func (a *Annotations) setTags(annotations map[string]string) error {
677653
}
678654
return nil
679655
}
656+
680657
func (a *Annotations) setTargetGroupAttributes(annotations map[string]string) error {
681658
var badAttrs []string
682659
rawAttrs := util.NewAWSStringSlice(annotations[targetGroupAttributesKey])
683660

684-
a.TargetGroupAttributes.Set("deregistration_delay.timeout_seconds", "300")
685-
a.TargetGroupAttributes.Set("slow_start.duration_seconds", "0")
686-
a.TargetGroupAttributes.Set("stickiness.enabled", "false")
687-
a.TargetGroupAttributes.Set("stickiness.lb_cookie.duration_seconds", "86400")
688-
a.TargetGroupAttributes.Set("stickiness.type", "lb_cookie")
689-
690661
for _, rawAttr := range rawAttrs {
691662
parts := strings.Split(*rawAttr, "=")
692663
switch {

pkg/annotations/annotations_test.go

Lines changed: 1 addition & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -184,21 +184,6 @@ func TestHealthcheckSecondsValidation(t *testing.T) {
184184
}
185185
}
186186

187-
// Should fail when idle timeout is not in range 1-3600. Should succeed otherwise.
188-
func TestConnectionIdleTimeoutValidation(t *testing.T) {
189-
a := &Annotations{}
190-
191-
err := a.setConnectionIdleTimeout(map[string]string{connectionIdleTimeoutKey: "15"})
192-
if err != nil || a.ConnectionIdleTimeout == aws.Int64(0) {
193-
t.Error("Failed to set connection idle timeout when value was correct.")
194-
}
195-
196-
err = a.setConnectionIdleTimeout(map[string]string{connectionIdleTimeoutKey: "3700"})
197-
if err == nil {
198-
t.Error("Succeeded setting connection idle timeout when value was incorrect")
199-
}
200-
}
201-
202187
func TestSetLoadBalancerAttributes(t *testing.T) {
203188
var tests = []struct {
204189
annotations map[string]string
@@ -299,7 +284,7 @@ func TestSetTargetGroupAttributes(t *testing.T) {
299284
annotations := &Annotations{}
300285
attributes := map[string]string{targetGroupAttributesKey: "deregistration_delay.timeout_seconds=60,stickiness.enabled=true"}
301286
err := annotations.setTargetGroupAttributes(attributes)
302-
if err != nil || len(annotations.TargetGroupAttributes) != 5 {
287+
if err != nil || len(annotations.TargetGroupAttributes) != 2 {
303288
t.Errorf("setTargetGroupAttributes - number of attributes incorrect")
304289
}
305290

0 commit comments

Comments
 (0)