Created a controller to manage ALB attributes #650
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bigkraig The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
approved
k8s-ci-robot
added
size/XL
|
nice work! it's much cleaner and readable now! 👍 |
k8s-ci-robot
added
size/XXL
bigkraig
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
internal/alb/lb/attributes.go
Outdated
| ) | ||
|
|
||
| const ( | ||
| DeletionProtectionEnabledString = "deletion_protection.enabled" |
There was a problem hiding this comment.
Maybe like DeletionProtectionEnabledKey and DefaultDeletionProtectionEnabled ?
but current workings for me too 😸 , just nitpicking...
internal/alb/lb/attributes.go
Outdated
| RoutingHTTP2Enabled: RoutingHTTP2Enabled, | ||
| } | ||
| for _, attr := range attrs { | ||
| switch aws.StringValue(attr.Key) { |
There was a problem hiding this comment.
could be switch attrKey := aws.StringValue(attr.Key); attrKey{...
so avoids lot's of aws.StringValue(attr.Key) in the error report...
nitpicking too 😸
internal/alb/lb/attributes.go
Outdated
| return a, fmt.Errorf("invalid load balancer attribute value %s=%s", aws.StringValue(attr.Key), aws.StringValue(attr.Value)) | ||
| } | ||
| default: | ||
| return a, fmt.Errorf("invalid load balancer attribute %s", aws.StringValue(attr.Key)) |
There was a problem hiding this comment.
i think it should be a warning via log here. Or add another params to the NewAttribute to indicate whether we are build from user's annotation or elbv2's response.
Since AWS elbv2 might adds new default attributes to lb, and our controller will error. We should be forward compatible too
There was a problem hiding this comment.
either way the error should be pushed up to fail the reconcile and it will be added with an event and error log there
internal/alb/lb/attributes.go
Outdated
| return fmt.Errorf("failed to retrieve attributes from ELBV2 in AWS: %s", err.Error()) | ||
| } | ||
|
|
||
| current, _ := NewAttributes(raw.Attributes) |
There was a problem hiding this comment.
ignored error isn't good enough
suppose a default new attribute get added in aws and appears first in attribute list.
All the attributes we get in current will be default inited, and will cause we always modify attributes over and over...
internal/alb/lb/attributes.go
Outdated
|
|
||
| // attributesChangeSet returns a list of elbv2.LoadBalancerAttribute required to change a into b | ||
| func attributesChangeSet(a, b *Attributes) (changeSet []*elbv2.LoadBalancerAttribute, ok bool) { | ||
| if a.DeletionProtectionEnabled != b.DeletionProtectionEnabled && b.DeletionProtectionEnabled != DeletionProtectionEnabled { |
There was a problem hiding this comment.
is the default compare desired?
I mean if user changed b.DeletionProtectionEnabled to true(via anno), and then change it back to false.
The a. DeletionProtectionEnabled(on alb) will always stays in true.
If we want to model the concept of optional attribute(like unset), we should use pointers to better model it. e.g. DeletionProtectionEnabled: *bool, which is why aws using *string instead of string. Hope golang got optional[bool] :D
internal/alb/lb/attributes.go
Outdated
| }) | ||
| } | ||
|
|
||
| if len(changeSet) > 0 { |
There was a problem hiding this comment.
i think we don't need the ok here, we can test for len(changeSet) that used the result of attributesChangeSet.
which is more clear to express the logic ("if changeSet is not empty, then..")
😸
|
/lgtm |
k8s-ci-robot
added
the
lgtm
Created a controller to manage ALB attributes
* Enable setting custom objectSelector for webhook #650 * Add formatting changes * Add webhook changes to README * Read update suggestion * Update READ table header to original
* Enable setting custom objectSelector for webhook kubernetes-sigs#650 * Add formatting changes * Add webhook changes to README * Read update suggestion * Update READ table header to original
No description provided.