Reconcile OpenStack port additional security groups - #18732
Reconcile OpenStack port additional security groups#18732rdebnath-x-ion wants to merge 1 commit into
Conversation
|
|
|
Hi @rdebnath-x-ion. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/easycla |
b4d268d to
d9236b4
Compare
The OpenStack Port task tracks kOps-managed SecurityGroups and user-supplied AdditionalSecurityGroups (by name). Two bugs prevented AdditionalSecurityGroups from being reconciled on existing ports: - newPortTaskFromCloud copied the desired AdditionalSecurityGroups into the actual state without checking the port, so drift was never detected. - RenderOpenstack had no path to apply security-group changes to an existing port, so changes were only picked up when an instance was recreated. Record an additional group in the actual state only when its ID is actually attached to the port, and update the port in place via UpdatePort with the merged managed + additional security-group IDs when the actual and desired sets differ. Add unit tests for both the read (drift detection) and write (in-place UpdatePort) paths. Fixes kubernetes#18731 Signed-off-by: Rounak Debnath <r.debnath@x-ion.de>
d9236b4 to
d518d95
Compare
|
/easycla |
|
Invalid commit message issues detected Invalid commit messagesKeywords which can automatically close issues and hashtag(#) mentions are not allowed.
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. |
1 similar comment
|
Invalid commit message issues detected Invalid commit messagesKeywords which can automatically close issues and hashtag(#) mentions are not allowed.
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. |
What this does
Fixes reconciliation of an OpenStack
Port'sAdditionalSecurityGroups(extra, kOps-unmanaged security groups attached by name).Two independent bugs are addressed:
Drift was invisible (read path).
newPortTaskFromCloudcopied the desiredAdditionalSecurityGroupsinto the actual state verbatim, without checking the port, so actual always equalled expected and a missing group was never detected. It now records an additional group in the actual state only if its ID is actually attached to the port.Changes were never applied (write path).
RenderOpenstackhad no code path to push a security-group change to an existing port — changes only took effect when an instance was recreated. It now, when the actual vs desired security groups differ, builds the merged set of managed + resolved additional security-group IDs and callsUpdatePort. Both kinds are sent together because OpenStack replaces the entire security-group set on update.Change detection compares the actual and expected tasks directly rather than inspecting
changes, becauseBuildChangescopies the expected value intochangesfor slice fields, making an emptied list indistinguishable there from an unchanged one.Testing
UpdatePortwith the merged managed + additional IDs.additionalSecurityGroupsentry is now applied to existing ports in place viaUpdatePort, without a rolling-update.Fixes #18731