🐛 Don't apply worker SG to control plane machines #1785
Conversation
k8s-ci-robot
added
the
do-not-merge/hold
✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/M
Currently, if a worker machine security group is specified but a control plane machine security group is not, the worker machine SG will be be applied to both worker *and* control plane machines. Correct this mistake. Signed-off-by: Stephen Finucane <[email protected]>
stephenfin
force-pushed
the
issues/1784
branch
from
bc7ec1c to
7e082f1
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
| if openStackCluster.Status.ControlPlaneSecurityGroup != nil { | ||
| managedSecurityGroup = openStackCluster.Status.ControlPlaneSecurityGroup.ID | ||
| } |
There was a problem hiding this comment.
I think in practise this would mean we're in some unusual state and we shouldn't reconcile at all until ControlPlaneSecurityGroup is set. However:
- This is clearly better and safer than the current code
- @EmilienM is currently working on a refactor which will be able to fix this up after the fact even if we were briefly in a weird state
So I'm happy to merge this.
| ConfigDrive: pointer.Bool(true), | ||
| ServerGroupID: serverGroupUUID, | ||
| ConfigDrive: pointer.Bool(true), | ||
| SecurityGroups: []infrav1.SecurityGroupFilter{}, |
There was a problem hiding this comment.
nit: I think you're setting SecurityGroups to an empty list to avoid appending to nil here? If so, you don't need to do that: nil is a valid empty slice. Appending to it will create a slice in the same way that a new slice is created if you exceed the capacity of the existing slice: https://go.dev/play/p/Nhln2sbvEGD
It's also safe and idiomatic to iterate over a nil slice in a for loop: https://go.dev/play/p/E5aqZHV51L0. This means, e.g. you don't need to write an additional guard like:
if (slice != nil) {
for _, elem := range slice {
...
}
}There was a problem hiding this comment.
Good to know, but the reason I did this is dumber: I had set the instance spec SecurityGroups attribute to an empty list in the test cases, and I modified the default machine to avoid having to also create a custom machine in the test case, which in turn meant I had to modify the default instance spec 😅 With the benefit of hindight, I could have simply not set SecurityGroups in those two additional test cases and thus avoided modifying anything in the defaults. lmk if you want me to respin.
There was a problem hiding this comment.
Not worth a respin just for this imho.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mdbooth, stephenfin 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
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
What this PR does / why we need it:
Currently, if a worker machine security group is specified but a control plane machine security group is not, the worker machine SG will be be applied to both worker and control plane machines. Correct this mistake.
Signed-off-by: Stephen Finucane [email protected]
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 #1784
Special notes for your reviewer:
None.
TODOs:
/hold