[WIP] ✨Disable/enable PortSecurity at port level #914

Xenwar · 2021-06-21T09:58:40Z

This PR adds the capability of enabling/disabling PortSecurity at the port level.

Example Manifest

apiVersion: infrastructure.cluster.x-k8s.io/v1alpha4
kind: OpenStackCluster
metadata:
  name: basic-8
  namespace: default
spec:
  disablePortSecurity: false

apiVersion: infrastructure.cluster.x-k8s.io/v1alpha4
kind: OpenStackMachineTemplate
metadata:
  name: basic-8-control-plane
  namespace: default
spec:
  template:
    spec:
      ports:
      - description: "primary with PortSecurity enabled, required for proper k8s cluster"
        disablePortSecurity: false
        allowedAddressPairs:
        - macAddress: "a4:b8:34:2b:ab:01"
          ipAddress: "10.10.10.10/24"
      - description: "secondary with PortSecurity disabled"
        disablePortSecurity: true
        allowedAddressPairs:
        - macAddress: "a4:b8:34:2b:ab:02"
          ipAddress: "20.20.20.20/24"

Example output:
`

(openstack) port show primary-port
+-----------------------+--------------------------------------------------------------------+
| Field                 | Value                                                              |
+-----------------------+--------------------------------------------------------------------+
| allowed_address_pairs | ip_address='10.10.10.10/24', mac_address='a4:b8:34:2b:ab:01'       |
| description           | primary with PortSecurity enabled, required for proper k8s cluster |
| port_security_enabled | True                                                               |
| security_group_ids    | 37e42488-1417-4e2c-980f-d2ef71ffbacc                               |
+-----------------------+--------------------------------------------------------------------+
(openstack) port show secondary-port
+-----------------------+--------------------------------------+
| Field                 | Value                                |
+-----------------------+--------------------------------------+
| allowed_address_pairs |                                      |
| description           | secondary with PortSecurity disabled |
| port_security_enabled | False                                |
| security_group_ids    |                                      |
+-----------------------+--------------------------------------+

`

Fixes #911
Signed-off-by: Anwar Hassen [email protected]

k8s-ci-robot · 2021-06-21T09:58:48Z

Hi @Xenwar. 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.

Xenwar · 2021-06-21T10:08:57Z

/assign @sbueringer

hidekazuna · 2021-06-23T07:32:21Z

config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclusters.yaml

@@ -1264,6 +1264,11 @@ spec:
                              type: array
                            description:
                              type: string
+                            disablePortSecurity:
+                              description: DisablePortSecurity disables the port security


Where this description come from? Description should be auto generated from a comment in api/v1alpha4/types.go.

You are right. I did not re-generate the yaml files after removing the comment.
Will add the comment back.

hidekazuna · 2021-06-23T07:58:40Z

pkg/cloud/services/compute/instance.go

@@ -505,6 +515,28 @@ func (s *Service) getOrCreatePort(eventObject runtime.Object, clusterName string
 	}

 	record.Eventf(eventObject, "SuccessfulCreatePort", "Created port %s with id %s", port.Name, port.ID)
+	if portOpts.DisablePortSecurity == true || networkDisabledPortSecurity == true {


Can't we create port with/without port security not updating after creating port?

no, we cannot.
At the port level, PortSecurity is immutable.
For the network level, there is a bug reporter (#913)

I think it is possible from gophercloud docs.

Indeed. The link you provided is more efficient as it does not need retrieving the port to update it, I will update my PR accordingly.

Thanks a lot

k8s-ci-robot · 2021-06-23T11:12:49Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Xenwar
To complete the pull request process, please ask for approval from sbueringer after the PR has been reviewed.

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

hidekazuna · 2021-06-23T12:54:46Z

@Xenwar I think we need to discuss motivation and spec more. Please see #911 #913

Xenwar · 2021-06-23T14:08:45Z

@Xenwar I think we need to discuss motivation and spec more. Please see #911 #913

Thanks, I will try it again and update the issue accordingly.

Xenwar · 2021-06-23T17:10:58Z

@hidekazuna
Having difficulty combining these two options.
The ports.Create(s.networkClient, takes only one of them. Option2 does not allow setting HostID, VNICType and Profile properties.
Any idea if combining them is possible ?


	option1 := portsbinding.CreateOptsExt{
		CreateOptsBuilder: createOpts,
		HostID:            portOpts.HostID,
		VNICType:          portOpts.VNICType,
		Profile:           nil,

	}
	option2 := portsecurity.PortCreateOptsExt{
		CreateOptsBuilder:   createOpts,
		PortSecurityEnabled: &enablePortSecurity,
	}

hidekazuna · 2021-06-24T00:17:38Z

@hidekazuna
Having difficulty combining these two options.
The ports.Create(s.networkClient, takes only one of them. Option2 does not allow setting HostID, VNICType and Profile properties.
Any idea if combining them is possible ?
	option1 := portsbinding.CreateOptsExt{
		CreateOptsBuilder: createOpts,
		HostID:            portOpts.HostID,
		VNICType:          portOpts.VNICType,
		Profile:           nil,

	}
	option2 := portsecurity.PortCreateOptsExt{
		CreateOptsBuilder:   createOpts,
		PortSecurityEnabled: &enablePortSecurity,
	}

@Xenwar Thanks investigating. I have no idea to combine them as of now.

Xenwar · 2021-06-24T06:22:29Z

@hidekazuna Thanks.

jichenjc · 2021-06-29T00:32:44Z

/ok-to-test

iamemilio · 2021-06-29T19:54:45Z

I just made a similar PR not realizing this was already out there. Lets combine efforts and share homework: #921

Rather than inheriting the port security settings from the cluster spec, I think its better to just inherit them from the network (which openstack does automatically on create) since these ports can be created on any network in theory. This is why I chose to make it *bool.

iamemilio · 2021-06-29T19:55:46Z

Another thing to watch out for is that because we typically set the security groups at the instance level, OpenStack will apply that security group to all port interfaces of that instance. This will cause ugly bugs when setting the port security on those ports.

Xenwar · 2021-06-30T08:31:10Z

I just made a similar PR not realizing this was already out there. Lets combine efforts and share homework: #921

Rather than inheriting the port security settings from the cluster spec, I think its better to just inherit them from the network (which openstack does automatically on create) since these ports can be created on any network in theory. This is why I chose to make it *bool.

Thanks, we can combine efforts.
On inheriting portSecurity from the network, my reasoning was there should be a single source of truth for the setting. So, if one changes the setting at the OSCluster spec, then it is propagated to the the Network.

Also, Network level setting is relevant when PortSecurity is disabled at the network level so the setting is disabled for each port. i.e. Ports do not inherit the setting from the network in all cases.

Xenwar · 2021-06-30T08:51:28Z

Another thing to watch out for is that because we typically set the security groups at the instance level, OpenStack will apply that security group to all port interfaces of that instance. This will cause ugly bugs when setting the port security on those ports.

We have a use case to enable/disable PortSecurity at the port(s) level. I am not quite sure yet at what point the bug you mentioned could show up, during Port creation or during an update as we have the option of specifying security groups at the port level.

We could probably open a discussion on slack on the this and the other comment.

iamemilio · 2021-06-30T17:58:32Z

/assign @iamemilio
going to assign myself to this so that I get notifications :)

Xenwar · 2021-06-30T20:55:10Z

/hold alternatives ways under discussion, #921

Xenwar · 2021-07-14T06:42:41Z

/unhold

hidekazuna · 2021-07-14T09:42:39Z

@Xenwar hold/unfold does not mean ready for review. If PR is not ready for review, rename title starting with WIP, please.

Xenwar · 2021-07-14T10:20:31Z

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

hidekazuna · 2021-07-15T13:12:43Z

docs/book/src/clusteropenstack/configuration.md

@@ -327,6 +327,13 @@ A floating IP is created and associated to the bastion host automatically, but y

 If `managedSecurityGroups: true`, security group rule opening 22/tcp is added to security groups for bastion, controller, and worker nodes respectively. Otherwise, you have to add `securityGroups` to the `bastion` in `OpenStackCluster` spec and `OpenStackMachineTemplate` spec template respectively.

+If PortSecurity disable for all ports either by setting `OpenStackCluster.spec.disablePortSecurity: true` or for each port individually, `managedSecurityGroups: false` is the only valid option. The following is not a valid coonfiguration and is not supported.


This is in "Accessing nodes through the bastion host via SSH" section, not security group section. How about moving to Ports?

Thanks, moved

Xenwar · 2021-07-15T18:37:24Z

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

Xenwar · 2021-07-26T06:24:48Z

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

Signed-off-by: Anwar Hassen <[email protected]>

Xenwar · 2021-07-29T09:22:52Z

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

k8s-ci-robot · 2021-07-29T11:22:25Z

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

Test name	Commit	Details	Rerun command
pull-cluster-api-provider-openstack-e2e-test	`3537479`	link	`/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.

Xenwar · 2021-07-30T06:54:12Z

This PR is no longer needed.
Feature is merged by #921

k8s-ci-robot · 2021-07-30T06:54:19Z

@Xenwar: 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 added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 21, 2021

k8s-ci-robot requested review from chrischdi and jichenjc June 21, 2021 09:58

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 21, 2021

k8s-ci-robot assigned sbueringer Jun 21, 2021

hidekazuna reviewed Jun 23, 2021

View reviewed changes

Xenwar force-pushed the toggle_port_level_security branch from b6347f9 to 84a4551 Compare June 23, 2021 11:12

Xenwar force-pushed the toggle_port_level_security branch from 84a4551 to 6bfa2fa Compare June 24, 2021 06:32

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 Jun 29, 2021

iamemilio mentioned this pull request Jun 30, 2021

✨ Apply Port Security to Ports #921

Merged

k8s-ci-robot assigned iamemilio Jun 30, 2021

Xenwar force-pushed the toggle_port_level_security branch from 6bfa2fa to 4db197b Compare June 30, 2021 20:24

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

Xenwar force-pushed the toggle_port_level_security branch from ac16771 to 4816166 Compare July 14, 2021 08:52

Xenwar force-pushed the toggle_port_level_security branch 2 times, most recently from 6f72039 to be7d601 Compare July 15, 2021 12:37

hidekazuna reviewed Jul 15, 2021

View reviewed changes

Xenwar force-pushed the toggle_port_level_security branch from be7d601 to 7d71d42 Compare July 15, 2021 14:20

Xenwar changed the title ~~✨ Disable/enable PortSecurity at port level~~ ✨[WIP] Disable/enable PortSecurity at port level Jul 16, 2021

Xenwar changed the title ~~✨[WIP] Disable/enable PortSecurity at port level~~ [WIP] ✨Disable/enable PortSecurity at port level Jul 16, 2021

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 16, 2021

Xenwar force-pushed the toggle_port_level_security branch from 7d71d42 to b5fe28a Compare July 16, 2021 11:22

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 16, 2021

Xenwar force-pushed the toggle_port_level_security branch 2 times, most recently from 11b6d31 to 4e9edf6 Compare July 21, 2021 10:16

macaptain mentioned this pull request Jul 22, 2021

🏃 Refactor all network client calls into networking package #950

Merged

1 task

Xenwar force-pushed the toggle_port_level_security branch 2 times, most recently from d8e2ee7 to 248dcaa Compare July 23, 2021 11:47

Disable/enable PortSecurity at port level

3537479

Signed-off-by: Anwar Hassen <[email protected]>

Xenwar force-pushed the toggle_port_level_security branch from 248dcaa to 3537479 Compare July 26, 2021 09:13

Xenwar closed this Jul 30, 2021

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] ✨Disable/enable PortSecurity at port level #914

[WIP] ✨Disable/enable PortSecurity at port level #914

Xenwar commented Jun 21, 2021 •

edited

Loading

k8s-ci-robot commented Jun 21, 2021

Xenwar commented Jun 21, 2021

hidekazuna Jun 23, 2021

Xenwar Jun 23, 2021

hidekazuna Jun 23, 2021

Xenwar Jun 23, 2021

hidekazuna Jun 23, 2021

Xenwar Jun 23, 2021

Xenwar Jun 23, 2021

k8s-ci-robot commented Jun 23, 2021

hidekazuna commented Jun 23, 2021

Xenwar commented Jun 23, 2021

Xenwar commented Jun 23, 2021 •

edited

Loading

hidekazuna commented Jun 24, 2021

Xenwar commented Jun 24, 2021

jichenjc commented Jun 29, 2021

iamemilio commented Jun 29, 2021

iamemilio commented Jun 29, 2021 •

edited

Loading

Xenwar commented Jun 30, 2021

Xenwar commented Jun 30, 2021

iamemilio commented Jun 30, 2021

Xenwar commented Jun 30, 2021

Xenwar commented Jul 14, 2021

hidekazuna commented Jul 14, 2021

Xenwar commented Jul 14, 2021

hidekazuna Jul 15, 2021

Xenwar Jul 15, 2021

Xenwar commented Jul 15, 2021

Xenwar commented Jul 26, 2021

Xenwar commented Jul 29, 2021

k8s-ci-robot commented Jul 29, 2021

Xenwar commented Jul 30, 2021

k8s-ci-robot commented Jul 30, 2021

		@@ -327,6 +327,13 @@ A floating IP is created and associated to the bastion host automatically, but y

		If `managedSecurityGroups: true`, security group rule opening 22/tcp is added to security groups for bastion, controller, and worker nodes respectively. Otherwise, you have to add `securityGroups` to the `bastion` in `OpenStackCluster` spec and `OpenStackMachineTemplate` spec template respectively.

		If PortSecurity disable for all ports either by setting `OpenStackCluster.spec.disablePortSecurity: true` or for each port individually, `managedSecurityGroups: false` is the only valid option. The following is not a valid coonfiguration and is not supported.

[WIP] ✨Disable/enable PortSecurity at port level #914

[WIP] ✨Disable/enable PortSecurity at port level #914

Conversation

Xenwar commented Jun 21, 2021 • edited Loading

k8s-ci-robot commented Jun 21, 2021

Xenwar commented Jun 21, 2021

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

k8s-ci-robot commented Jun 23, 2021

hidekazuna commented Jun 23, 2021

Xenwar commented Jun 23, 2021

Xenwar commented Jun 23, 2021 • edited Loading

hidekazuna commented Jun 24, 2021

Xenwar commented Jun 24, 2021

jichenjc commented Jun 29, 2021

iamemilio commented Jun 29, 2021

iamemilio commented Jun 29, 2021 • edited Loading

Xenwar commented Jun 30, 2021

Xenwar commented Jun 30, 2021

iamemilio commented Jun 30, 2021

Xenwar commented Jun 30, 2021

Xenwar commented Jul 14, 2021

hidekazuna commented Jul 14, 2021

Xenwar commented Jul 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xenwar commented Jul 15, 2021

Xenwar commented Jul 26, 2021

Xenwar commented Jul 29, 2021

k8s-ci-robot commented Jul 29, 2021

Xenwar commented Jul 30, 2021

k8s-ci-robot commented Jul 30, 2021

Xenwar commented Jun 21, 2021 •

edited

Loading

Xenwar commented Jun 23, 2021 •

edited

Loading

iamemilio commented Jun 29, 2021 •

edited

Loading