Add EnhancedDefaulting flag to do sensible defaulting when possible #4344
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: zac-nixon 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
the
approved
|
/retest |
1 similar comment
|
/retest |
|
@zac-nixon: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| if isSubnetInLocalZoneOrOutpost { | ||
| // Skip mTLS configuration for Local Zone or Outpost subnets | ||
| l.logger.V(1).Info("skipping mutual authentication configuration as it is not supported in local zone or outpost") | ||
| if !isSecureProtocol(gwLsCfg.protocol) || lbLsCfg == nil || lbLsCfg.MutualAuthentication == nil { |
There was a problem hiding this comment.
Why are we removing the outpost/lz checks from here?
This will fail in creating listeners in outpost/lz as our CRD default is OFF (even if customer does not specify it). the drift logic in deployer only run for updates not for create. So for new listeners we will have desired mtls setting as non-nil with OFF setting, and we will fail in creating listener. we still need to check outpost/lz if we are building listener object from CRD for such cases. Correct me if I am wrong.
There was a problem hiding this comment.
It's on the lines below:
// Default to "off" mode when no explicit mTLS configuration exists
if lbLsCfg == nil || lbLsCfg.MutualAuthentication == nil {
return &elbv2model.MutualAuthenticationAttributes{
Mode: string(elbv2gw.MutualAuthenticationOffMode),
}, nil
}
I removed this block, as the new defaulting checks within the deployer package make it so we don't need to inject the off mode.
| }) | ||
| prestack = append(prestack, shieldPrestack) | ||
| break | ||
| case addon.ProvisionedCapacity: |
There was a problem hiding this comment.
I really like this. Thanks for unifying these
| // 8. desired = Other value, sdk = None. Result = drift | ||
| // 9. desired = Other value, sdk = Other value. Result = drift depending on equality. | ||
|
|
||
| desiredIsNilOrNone := lsSpec.ALPNPolicy == nil || len(lsSpec.ALPNPolicy) == 0 || lsSpec.ALPNPolicy[0] == string(elbv2model.ALPNPolicyNone) |
There was a problem hiding this comment.
can we check nil here? is it possible lsSpec.ALPNPolicy=nil so lsSpec.ALPNPolicy[0] will panic. same for sdkLS.Listener, and then sdkLS.Listener.AlpnPolicy will panic
| SubnetMappings: subnets.subnets, | ||
| SecurityGroups: securityGroupTokens, | ||
| LoadBalancerAttributes: lbModelBuilder.buildLoadBalancerAttributes(lbConf), | ||
| MinimumLoadBalancerCapacity: lbModelBuilder.buildLoadBalancerMinimumCapacity(lbConf), |
There was a problem hiding this comment.
why are we not taking MinimumLoadBalancerCapacity from load balancer spec anymore?
Issue
#4319
Description
Adds a controller flag, that will remove mTLS configuration when the annotation is removed from the annotations. This flag could potentially be moved to other APIs, like Provisioned Capacity, but those require a bigger lift. This change preserves the initial LBC logic, so no breaking API changes. See the code for the various cases that I considered.
With Gateway API, we were handling removal of mTLS configuration much better. With this change, I am going ahead and enabling EnhancedDefault behavior for Gateway API as I believe this implementation is correct and better than ignoring missing annotations. Because the deployer is smarter now, we can remove the Local Zone / Outpost check from the Gateway API model builder.
Summary of additions:
Checklist
README.md, or thedocsdirectory)BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯