⚠️ Remove PortOpts.SecurityGroups #1291

lentzi90 · 2022-07-04T07:02:21Z

This replaces #1257.

What this PR does / why we need it:

Deprecating and replacing PortOpts.SecurityGroups of *[]string format field from OpenStackMachineTemplate ports and replace is with *[]SecurityGroupParam format. The PortOpts.SecurityGroupFilters of type []SecurityGroupParam is removed.

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 #1251

Special notes for your reviewer:

~~I opened a new PR instead of rebasing the existing since there are some issues and I'm unsure how to solve them. This way it will be easier to compare the rebased and original code side by side.~~

~~We need to provide a conversion function for the PortOpts.SecurityGroups field that changes from *[]string in v1alpha5 to *[]SecurityGroupParam in v1alpha6.~~
The same conversion is needed from v1alpha4 to v1alpha6 and this is where I'm having issues. The required name of the conversion function becomes the same for both v1alpha4 and v1alpha5 because it goes from *[]string which doesn't have a version.
```
Convert_Slice_string_To_Slice_v1alpha6_SecurityGroupParam
```
~~If I add this conversion function to both v1alpha4 and v1alpha5, the generator complains that there is a duplicate.~~
If I instead add the conversion in only one place (e.g. v1alpha5), then the generator will be happy and import the conversion function in the other API version as well! But the compiler does not like this at all. It gets an import error.
```
could not import ./api/v1alpha4 (no required module provides package "./api/v1alpha4")
```

This wasn't a problem in the original PR since it only needed the conversion from v1alpha4 -> v1alpha5 (so no duplicate function name or import needed).
If you have any suggestions on how to solve this, please let me know.

Edit: I think I figured it out! By adding the conversion function in v1alpha6 instead of v1alpha5 and/or v1alpha4, I avoid the issue.

TODOs:

squashed commits
if necessary:
- includes documentation
- adds unit tests

/hold

netlify · 2022-07-04T07:02:25Z

✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

Name	Link
🔨 Latest commit	`20cd37a`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/64182cfb22118800086f6930
😎 Deploy Preview	https://deploy-preview-1291--kubernetes-sigs-cluster-api-openstack.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

k8s-ci-robot · 2022-07-04T07:02:29Z

Hi @lentzi90. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

jichenjc · 2022-07-05T00:41:04Z

/ok-to-test

api/v1alpha5/zz_generated.conversion.go

lentzi90 · 2022-07-05T11:10:14Z

api/v1alpha6/types.go

-	SecurityGroupFilters []SecurityGroupParam `json:"securityGroupFilters,omitempty"`
-	AllowedAddressPairs  []AddressPair        `json:"allowedAddressPairs,omitempty"`
+	// The names of the security groups to assign to the port
+	SecurityGroups      *[]SecurityGroupParam `json:"securityGroups,omitempty"`


Please check this carefully.
I have copied it from #1257 but I'm unsure if it should be *[]SecurityGroupParam or []SecurityGroupParam. Note that the SecurityGroupFilters used to be []SecurityGroupParam but now SecurityGroups is *[]SecurityGroupParam.

mdbooth

This looks good to me with some caveats.

We need to decide if it's useful to specify an explicitly empty list of SecurityGroups.
Double check we didn't break selection by filter.

and a big one:

I would really like to see us test this with envtests rather than e2e. It's a perfect candidate.

mdbooth · 2022-08-10T12:41:52Z

pkg/cloud/services/networking/port.go

+			}
+			if len(portSecurityGroups) > 0 {
+				securityGroups = &portSecurityGroups
+			}
 		}
 		// inherit port security groups from the instance if not explicitly specified
 		if securityGroups == nil || len(*securityGroups) == 0 {


The len(*securityGroups) == 0 check looks out of place. The only reason I can think of to specify SecurityGroups as *[]foo rather than []foo is to allow the user to differentiate between no value (i.e. use the default) and an explicitly empty list. Combining those 2 cases in this check removes this benefit.

If we're changing this we should decide if we really need this distinction. If not we should simplify the type to []foo to remove the landmine.

Not sure I understand. But I'll give it a go. So the securityGroups == nil is for checking if the portOps didn't have any security groups specified, right? Then the len(*securityGroups) == 0 would mean that we check if the portOps had an explicit empty list of security groups?

If this is the case, then I agree that combining them is point less. And it makes the check do a different thing than the comment above implies. So the actual current behavior if to inherit the security groups from the instance, no matter if the portOpts had an empty list or nothing specified at all.
I would be happy to simplify this if we get consensus.

A quick check revealed that there are no unit tests that check this at least. Could be something in the e2e tests of course but that seems unlikely.

pkg/cloud/services/networking/securitygroups.go

lentzi90 · 2022-08-16T13:47:28Z

pkg/cloud/services/networking/securitygroups.go

-		if len(SGList) == 0 {
-			return nil, fmt.Errorf("security group %s not found", sg.Name)
-		}


Note this change in behavior for GetSecurityGroups!
It used to return an error if none were found. Now it simply returns an empty slice.
If we want to keep the current behavior, we will need a way to tell the difference between "unable to get security groups" (e.g. network errors) and "these params doesn't match anything". Otherwise we have no way of knowing if we should create a new port or retry again later.

lentzi90 · 2022-08-16T13:50:31Z

/retest

k8s-triage-robot · 2022-12-20T11:01:09Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

lentzi90 · 2022-12-20T11:39:19Z

/remove-lifecycle stale

lentzi90 · 2022-12-21T06:20:24Z

/test pull-cluster-api-provider-openstack-e2e-full-test

lentzi90 · 2022-12-21T07:00:34Z

/test pull-cluster-api-provider-openstack-e2e-full-test

This turns PortOpts.SecurityGroups in v1alpha6 into *[]SecurityGroupParam instead of *[]string and removes the SecurityGroupFilters field which would contain the same information otherwise. Co-authored-by: Anwar Hassen <anwar.hassen@est.tech>

k8s-ci-robot · 2023-03-08T11:12:29Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: lentzi90
Once this PR has been reviewed and has the lgtm label, please assign tobiasgiese for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

This adds conversion tests especially for PortOpts. Both "normal" tests and fuzzy tests are added for v1alpha6. Conversions and tests are updated also for older API versions.

k8s-ci-robot · 2023-03-21T23:35:40Z

PR needs rebase.

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/test-infra repository.

k8s-ci-robot · 2023-03-27T14:58:55Z

@lentzi90: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-provider-openstack-e2e-test	`20cd37a`	link	true	`/test pull-cluster-api-provider-openstack-e2e-test`

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/test-infra repository. I understand the commands that are listed here.

lentzi90 · 2023-03-27T18:27:04Z

Replaced by #1516 which just drops securityGroups instead of renaming securityGroupFilters to securityGroups and dropping the old securityGroups.

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 4, 2022

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 4, 2022

k8s-ci-robot requested review from seanschneeweiss and tobiasgiese July 4, 2022 07:02

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jul 4, 2022

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 5, 2022

lentzi90 mentioned this pull request Jul 5, 2022

How to handle conversion from unversioned to versioned type? kubernetes/code-generator#142

Closed

mdbooth mentioned this pull request Jul 5, 2022

Deprecating and/or replacing securityGroups field from OpenStackMachineTemplate #1257

Closed

lentzi90 commented Jul 5, 2022

View reviewed changes

api/v1alpha5/zz_generated.conversion.go Outdated Show resolved Hide resolved

lentzi90 force-pushed the lentzi90/deprecate-security-group-filters branch 2 times, most recently from a9e43c9 to 70546e0 Compare July 5, 2022 11:06

lentzi90 commented Jul 5, 2022

View reviewed changes

mdbooth requested changes Aug 10, 2022

View reviewed changes

lentzi90 force-pushed the lentzi90/deprecate-security-group-filters branch from 70546e0 to 8498a17 Compare August 16, 2022 13:41

lentzi90 commented Aug 16, 2022

View reviewed changes

lentzi90 force-pushed the lentzi90/deprecate-security-group-filters branch from 8498a17 to 815c661 Compare August 17, 2022 06:11

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 16, 2022

lentzi90 force-pushed the lentzi90/deprecate-security-group-filters branch from 815c661 to 8934598 Compare September 21, 2022 10:23

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 21, 2022

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 20, 2022

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 20, 2022

lentzi90 force-pushed the lentzi90/deprecate-security-group-filters branch from 8934598 to de155f5 Compare December 20, 2022 13:30

lentzi90 force-pushed the lentzi90/deprecate-security-group-filters branch from de155f5 to 852fb61 Compare December 20, 2022 13:56

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 3, 2023

⚠ Remove PortOpts.SecurityGroupFilters

1ef5ee1

This turns PortOpts.SecurityGroups in v1alpha6 into *[]SecurityGroupParam instead of *[]string and removes the SecurityGroupFilters field which would contain the same information otherwise. Co-authored-by: Anwar Hassen <anwar.hassen@est.tech>

lentzi90 force-pushed the lentzi90/deprecate-security-group-filters branch from 852fb61 to 1ef5ee1 Compare March 8, 2023 11:12

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 8, 2023

Add conversion tests

20cd37a

This adds conversion tests especially for PortOpts. Both "normal" tests and fuzzy tests are added for v1alpha6. Conversions and tests are updated also for older API versions.

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 20, 2023

lentzi90 mentioned this pull request Mar 20, 2023

🌱 Add fuzzy conversion tests for v1alpha6 #1509

Merged

3 tasks

lentzi90 changed the title ~~⚠️ Remove PortOpts.SecurityGroupFilters~~ ⚠️ Remove PortOpts.SecurityGroups Mar 21, 2023

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 21, 2023

mdbooth mentioned this pull request Mar 22, 2023

🌱 Un-pointer PortOpts.SecurityGroups #1511

Merged

lentzi90 mentioned this pull request Mar 27, 2023

⚠️ Remove PortOpts.SecurityGroups #1516

Merged

3 tasks

lentzi90 closed this Mar 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚠️ Remove PortOpts.SecurityGroups #1291

⚠️ Remove PortOpts.SecurityGroups #1291

lentzi90 commented Jul 4, 2022 •

edited

netlify bot commented Jul 4, 2022 •

edited

k8s-ci-robot commented Jul 4, 2022

jichenjc commented Jul 5, 2022

lentzi90 Jul 5, 2022

mdbooth left a comment

mdbooth Aug 10, 2022

lentzi90 Aug 16, 2022

lentzi90 Aug 16, 2022

lentzi90 Aug 16, 2022

lentzi90 commented Aug 16, 2022

k8s-triage-robot commented Dec 20, 2022

lentzi90 commented Dec 20, 2022

lentzi90 commented Dec 21, 2022

lentzi90 commented Dec 21, 2022

k8s-ci-robot commented Mar 8, 2023

k8s-ci-robot commented Mar 21, 2023

k8s-ci-robot commented Mar 27, 2023

lentzi90 commented Mar 27, 2023

⚠️ Remove PortOpts.SecurityGroups #1291

⚠️ Remove PortOpts.SecurityGroups #1291

Conversation

lentzi90 commented Jul 4, 2022 • edited

netlify bot commented Jul 4, 2022 • edited

✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

k8s-ci-robot commented Jul 4, 2022

jichenjc commented Jul 5, 2022

lentzi90 Jul 5, 2022

Choose a reason for hiding this comment

mdbooth left a comment

Choose a reason for hiding this comment

mdbooth Aug 10, 2022

Choose a reason for hiding this comment

lentzi90 Aug 16, 2022

Choose a reason for hiding this comment

lentzi90 Aug 16, 2022

Choose a reason for hiding this comment

lentzi90 Aug 16, 2022

Choose a reason for hiding this comment

lentzi90 commented Aug 16, 2022

k8s-triage-robot commented Dec 20, 2022

lentzi90 commented Dec 20, 2022

lentzi90 commented Dec 21, 2022

lentzi90 commented Dec 21, 2022

k8s-ci-robot commented Mar 8, 2023

k8s-ci-robot commented Mar 21, 2023

k8s-ci-robot commented Mar 27, 2023

lentzi90 commented Mar 27, 2023

lentzi90 commented Jul 4, 2022 •

edited

netlify bot commented Jul 4, 2022 •

edited