[feat: gw api] Security Group discovery and management #4142
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
approved
| Tags: tags, | ||
| } | ||
|
|
||
| if lbModelBuilder.loadBalancerType == elbv2model.LoadBalancerTypeNetwork { |
There was a problem hiding this comment.
Can we have dedicated functions to build these LB type specific params? instead of big if blocks for better readability?
| } | ||
|
|
||
| func (lbModelBuilder *loadBalancerBuilderImpl) translateSourcePrefixEnabled(b bool) elbv2model.EnablePrefixForIpv6SourceNat { | ||
| if b { |
There was a problem hiding this comment.
NIT: Lets add proper variable name for this
| func (lbModelBuilder *loadBalancerBuilderImpl) buildLoadBalancerName(lbConf *elbv2gw.LoadBalancerConfiguration, gw *gwv1.Gateway, scheme elbv2model.LoadBalancerScheme) (string, error) { | ||
| if lbConf.Spec.LoadBalancerName != nil { | ||
| name := *lbConf.Spec.LoadBalancerName | ||
| // The name of the loadbalancer can only have up to 32 characters |
There was a problem hiding this comment.
Already added as part of crd validation. https://github.com/kubernetes-sigs/aws-load-balancer-controller/blob/main/apis/gateway/v1beta1/loadbalancerconfig_types.go#L173
There was a problem hiding this comment.
i was just being defensive but yeah fair enough the whole point of using the CRD was to avoid having duplicate validations
zac-nixon
force-pushed
the
znixon/gw-sg-creation
branch
2 times, most recently
from
47f5eb6 to
1fcf866
Compare
|
/lgtm |
k8s-ci-robot
added
lgtm
|
New changes are detected. LGTM label has been removed. |
zac-nixon
force-pushed
the
znixon/gw-sg-creation
branch
2 times, most recently
from
c4a31e6 to
61c8da1
Compare
| protocolSet = protocolSet.Insert(string(ec2types.ProtocolUdp)) | ||
| break | ||
| default: | ||
| // Ignore? Throw error? |
There was a problem hiding this comment.
I think we can ignore this one since the conversion would fail at the time of building RouteDescriptors.
| // CIDR Loop | ||
| for _, cidr := range sourceRanges { | ||
| isIPv6 := isIPv6CIDR(cidr) | ||
|
|
There was a problem hiding this comment.
Can we simplify these nested ifs to something like this for better readability?
if isIPv6 && includeIPv6 {
permissions = append(permissions, ec2model.IPPermission{
IPProtocol: protocol,
FromPort: awssdk.Int32(int32(port)),
ToPort: awssdk.Int32(int32(port)),
IPv6Range: []ec2model.IPv6Range{
{
CIDRIPv6: cidr,
},
},
})
} else {
permissions = append(permissions, ec2model.IPPermission{
IPProtocol: protocol,
FromPort: awssdk.Int32(int32(port)),
ToPort: awssdk.Int32(int32(port)),
IPRanges: []ec2model.IPRange{
{
CIDRIP: cidr,
},
},
})
}
if enableICMP && isIPv6 && includeIPv6 {
permissions = append(permissions, ec2model.IPPermission{
IPProtocol: shared_constants.ICMPV6Protocol,
FromPort: awssdk.Int32(shared_constants.ICMPV6TypeForPathMtu),
ToPort: awssdk.Int32(shared_constants.ICMPV6CodeForPathMtu),
IPv6Range: []ec2model.IPv6Range{
{
CIDRIPv6: cidr,
},
},
})
} else {
permissions = append(permissions, ec2model.IPPermission{
IPProtocol: shared_constants.ICMPV4Protocol,
FromPort: awssdk.Int32(shared_constants.ICMPV4TypeForPathMtu),
ToPort: awssdk.Int32(shared_constants.ICMPV4CodeForPathMtu),
IPRanges: []ec2model.IPRange{
{
CIDRIP: cidr,
},
},
})
}
There was a problem hiding this comment.
IMO, I don't think it's more readable. Also, it looks like there is a bug as it would always enable icmpv4 if icmp was not enabled.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: shraddhabang, 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 |
zac-nixon
force-pushed
the
znixon/gw-sg-creation
branch
from
61c8da1 to
5c1f930
Compare
Description
Adds logic to manage security groups, either via static IDs / names, or by controller managed SG. This logic combines the ingress and service paths to work for either nlb / alb.
Checklist
README.md, or thedocsdirectory)BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯