Passing security groups by specifying more options in addition to UUIDs on ports #1246

Xenwar · 2022-05-20T08:55:14Z

With OpenStackMachineTemplate it is possible to specify security groups at both instance and port levels. However, both levels do not have same format, with the instance level having more options. This PR moves the port level format closer to that of the instance level, as shown below.

Specifying security groups at port level:

spec:
  template:
    spec:
      ports:
      - description: "Port 1 with security group in UUID format"
        securityGroups: 
        - "2f6584db-5138-453b-b47b-99696cb11f0f"

Specifying security groups at port level:

spec:
  template:
    spec:
        securityGroups:
        - name: "capo-port-instance-level-sg"
           uuid: ....
           filter: .....

As can be seen above, the instance level setting has more options. This PR the securityGroupsFilters field. The original field port.securityGroups is kept for backward compatibility. However, the old field is redundant due to the securityGroupsFilters already containing the UUID field. Therefore, removing it is more preferable.

Fixes #1245

Special notes for your reviewer:

netlify · 2022-05-20T08:55:16Z

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

Name	Link
🔨 Latest commit	`a14dcd6`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/628e03417319ea000826fb37
😎 Deploy Preview	https://deploy-preview-1246--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.

Xenwar · 2022-05-20T09:08:38Z

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

Xenwar · 2022-05-20T11:03:02Z

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

Xenwar · 2022-05-21T18:11:30Z

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

apricote · 2022-05-23T07:25:54Z

hack/ci/cloud-init/controller.yaml.tpl

@@ -153,6 +153,8 @@
    # Adjust the CPU quota
    openstack quota set --cores 32 demo
    openstack quota set --secgroups 50 demo
+    openstack quota set --secgroups 100 demo


I think you can remove the previous line openstack quota set --secgroups 50 demo, because it will get overridden by this one.

thanks, will do that after the current test.

apricote · 2022-05-23T08:12:42Z

pkg/cloud/services/networking/port.go

+				if sg == "" {
+					continue
+				}
+				*securityGroups = append(*securityGroups, sg)


How will this handle duplicate security groups? E.g. if the same uuid is specified in portOpts.SecurityGroups and portOpts.SecurityGroupFilters

Tried duplicates, and it does fail, no check on that.
Thanks, will add the logic to remove the duplicates.

Done: now duplicates are removed

securityGroups field

securityGroupFilter field

A combination of the two.

Xenwar · 2022-05-23T12:33:56Z

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

Xenwar · 2022-05-23T17:19:27Z

/hold We might need to consider deprecating the port.securityGroup in *[]string format

jichenjc · 2022-05-25T03:45:54Z

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

jichenjc · 2022-05-25T05:21:35Z

pkg/cloud/services/networking/port.go

-
+		securityGroups, err = s.CollectPortSecurityGroups(eventObject, portOpts.SecurityGroups, portOpts.SecurityGroupFilters)
+		if err != nil {
+			return nil, fmt.Errorf("multiple ports found with name \"%s\"", portName)


should we only log the err itself here?

The error message is wrong, will remove the text.
However, we need to return the error as it tells the absence of the security groups specified in the manifests.

apricote · 2022-05-25T07:25:44Z

api/v1alpha4/conversion_test.go

@@ -244,6 +244,7 @@ func TestFuzzyConversion(t *testing.T) {
 						v1alpha5PortOpts.Network = nil
 					}
 				}
+				v1alpha5PortOpts.SecurityGroupFilters = nil


@bavarianbidi is introducing utilconversion.(Un)MarshalData from Cluster API in #1247 to avoid losing data on a round-trip conversion to v1alpha4 and back to alpha5.

https://github.com/kubernetes-sigs/cluster-api-provider-openstack/pull/1247/files#diff-9a53c6afb893cfd6bb4f50185dea035075cb0ef25fce713667c270d5c9668a31R41

I think it makes sense to also apply this here, though I am not sure about the best process for this, some options:

wait until ✨ Feature: restrict API Server LB access via IPs #1247 is merged and then rebase ontop

extract the round-trip setup into a seperate PR and rebase this ontop

continue as is and do this in a follow up PR once ✨ Feature: restrict API Server LB access via IPs #1247 is merged

I think doing it in a follow up PR could be good as I am planning the deprecate the securityGroups field. The reason being, the uuid it provides is also provided by the new introduced securityGroupFilter field.

I'm also fine doing a separate PR as it seems that all conversation unit tests has to be refactored. Already though about but didn't had much time to start the discussion.

apricote · 2022-05-25T07:53:51Z

pkg/cloud/services/networking/port.go

+			continue
+		}
+		uids[sg] = 1
+		securityGroupCount++


securityGroupCount is initialized to the number of SGs returned from the filter (line 271). Here you increment it by one for every single security group returned by either the filters, or the explicit id list. In effect, you are double counting the security groups from the filter.

This can be fixed by initializing securityGroupCount := 0 in line 271.

This does not introduce any impacting bugs, only the slice for distinctSecurityGroupIDs will have a backing array that is too large.

True, but even with securityGroupCount := 0 at 271, we will still have extra space consumed.
The reason is allSecurityGroupIDs could still contain duplicates.

I will add an extra loop to count distinct values.

Added securityGroupCount := 0 at 271, change the incremental to conditional, thanks.

apricote · 2022-05-25T10:29:51Z

/lgtm

jichenjc · 2022-05-25T10:31:18Z

/approve

k8s-ci-robot · 2022-05-25T10:31:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jichenjc, Xenwar

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:

~~OWNERS~~ [jichenjc]

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

Xenwar · 2022-05-25T11:05:42Z

/hold cancel
deprecating the port.securityGroup and comment #1246 (comment), will addressed on a separated PR.

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 20, 2022

k8s-ci-robot requested review from apricote and mdbooth May 20, 2022 08:55

Xenwar mentioned this pull request May 20, 2022

Specify security groups by more options in addition to UUIDs at Port level #1245

Closed

Xenwar force-pushed the sg-by-name branch from 766a490 to e422860 Compare May 21, 2022 18:10

Xenwar force-pushed the sg-by-name branch 6 times, most recently from 465ea32 to 634a5dd Compare May 23, 2022 07:26

apricote reviewed May 23, 2022

View reviewed changes

Xenwar force-pushed the sg-by-name branch 2 times, most recently from 61f52c6 to 50d5ec0 Compare May 23, 2022 11:33

Xenwar force-pushed the sg-by-name branch 3 times, most recently from 9e87d0d to cb0a4f3 Compare May 23, 2022 16:36

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

Xenwar force-pushed the sg-by-name branch 2 times, most recently from ac66f4e to 12c733f Compare May 24, 2022 19:38

jichenjc reviewed May 25, 2022

View reviewed changes

Xenwar force-pushed the sg-by-name branch from 12c733f to ea45539 Compare May 25, 2022 05:40

apricote reviewed May 25, 2022

View reviewed changes

Passing security groups by filter instead of ID on ports

a14dcd6

Xenwar force-pushed the sg-by-name branch from ea45539 to a14dcd6 Compare May 25, 2022 10:21

k8s-ci-robot assigned apricote May 25, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 25, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 25, 2022

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

k8s-ci-robot merged commit a22f805 into kubernetes-sigs:main May 25, 2022

Xenwar mentioned this pull request May 25, 2022

Deprecating/Replacing securityGroups field from OpenStackMachineTemplate ports #1251

Closed

Xenwar mentioned this pull request Jun 9, 2022

Deprecating and/or replacing securityGroups field from OpenStackMachineTemplate #1257

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Passing security groups by specifying more options in addition to UUIDs on ports #1246

Passing security groups by specifying more options in addition to UUIDs on ports #1246

Xenwar commented May 20, 2022

netlify bot commented May 20, 2022 •

edited

Loading

Xenwar commented May 20, 2022

Xenwar commented May 20, 2022

Xenwar commented May 21, 2022

apricote May 23, 2022

Xenwar May 23, 2022

Xenwar May 25, 2022

apricote May 23, 2022

Xenwar May 23, 2022

Xenwar May 25, 2022

Xenwar commented May 23, 2022

Xenwar commented May 23, 2022

jichenjc commented May 25, 2022

jichenjc May 25, 2022

Xenwar May 25, 2022

apricote May 25, 2022

Xenwar May 25, 2022

bavarianbidi May 25, 2022

apricote May 25, 2022

Xenwar May 25, 2022

Xenwar May 25, 2022

apricote commented May 25, 2022

jichenjc commented May 25, 2022

k8s-ci-robot commented May 25, 2022

Xenwar commented May 25, 2022

Passing security groups by specifying more options in addition to UUIDs on ports #1246

Passing security groups by specifying more options in addition to UUIDs on ports #1246

Conversation

Xenwar commented May 20, 2022

netlify bot commented May 20, 2022 • edited Loading

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

Xenwar commented May 20, 2022

Xenwar commented May 20, 2022

Xenwar commented May 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xenwar commented May 23, 2022

Xenwar commented May 23, 2022

jichenjc commented May 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apricote commented May 25, 2022

jichenjc commented May 25, 2022

k8s-ci-robot commented May 25, 2022

Xenwar commented May 25, 2022

netlify bot commented May 20, 2022 •

edited

Loading