🐛 port: don't add any SGs when port security is disabled #2159
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/M
✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| if ptr.Deref(portSpec.DisablePortSecurity, false) { | ||
| return nil, errors.New("security groups cannot be set when port security is disabled") | ||
| } |
There was a problem hiding this comment.
IIRC if you try to add a security group to a port with port security disabled you get an error, right? i.e. It's not just that it ignores the option, but it actually won't add the security groups?
If so, we should be able to safely add API validation for this instead, because we know there is no working configuration with it set.
i.e. We should write this as CEL instead. The tests would be in apivalidations.
There was a problem hiding this comment.
IIRC if you try to add a security group to a port with port security disabled you get an error, right? i.e. It's not just that it ignores the option, but it actually won't add the security groups?
yeah right. And good points on API validations. I'll check that.
There was a problem hiding this comment.
I've added tests in the webhook like other fields and tests in API validations. I've let this check on purpose because:
- it doesn't hurt, I think
- it's safe and it makes sense to report an error and not let it through anyway.
There was a problem hiding this comment.
Ack to leaving the check in.
I'd prefer we didn't add anything new to the webhooks, though, unless we absolutely have to. I take a look to see how easy the CEL is to write.
There was a problem hiding this comment.
I'm going to withdraw this objection. While the CEL is simple enough to write, unfortunately because it affects both the SecurityGroups and DisablePortSecurity fields it needs to be implemented on the PortOpts struct rather than just one field. Unfortunately this struct is a monster, so I wasn't able to write the rule which doesn't exceed the complexity budget.
There was a problem hiding this comment.
that was my thought when I looked at CEL and why I took the webhook way instead 😕
When port security is disabled on a port, don't add any security group to the port options. Skip the security groups when resolving the ports spec. Report an error when a port tries to be created with both security groups and disable port security to true. Adds unit tests coverage for these scenarios.
k8s-ci-robot
added
size/L
| for _, port := range newObj.Spec.Ports { | ||
| if ptr.Deref(port.DisablePortSecurity, false) && len(port.SecurityGroups) > 0 { | ||
| allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "ports"), "cannot have security groups when DisablePortSecurity is set to true")) | ||
| } | ||
| } |
There was a problem hiding this comment.
Is there any reason we can't do this in CEL? We haven't added any new logic to the webhooks in a while and I'd like to phase them out.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
if you can /approve instead so I can ask @MaysaMacedo to take a look as well. Thanks |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mdbooth 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
|
@MaysaMacedo it merged by accident but when time permits, please have a look and let me know if anything is wrong, we'll fix afterwards. Thanks |
What this PR does / why we need it:
When port security is disabled on a port, don't add any security group
to the port options.
Skip the security groups when resolving the ports spec.
Report an error when a port tries to be created with both security
groups and disable port security to true.
Adds unit tests coverage for these scenarios.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #2158