Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[terraform][openstack] allow disabling port_security at port level #8455

Merged

Conversation

cristicalin
Copy link
Contributor

@cristicalin cristicalin commented Jan 20, 2022

Use openstack_networking_port_v2 and openstack_networking_floatingip_associate_v2
to attach floating ips. This gives us more flexibility on disabling port security
when binding instances directly on provider networks in private cloud scenario.

What type of PR is this?

/kind bug
/kind feature

What this PR does / why we need it:
This PR allows instances created with our terraform code to be bound directly on a provider network which is a current limitation/bug when needing to disable port security. This PR has the side effect of disabling a workaround for opentelekom cloud but since I'm not able to test on that cloud I'm not sure the workaround is still needed there.

I have previously introduced the ability to disable port security at the network level (#8410) but this uncovered a bug when using it on a provider network so from this perspective it is both a bug fix and a feature at the same time.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

[OpenStack] allow disabling port security at port level

Use openstack_networking_port_v2 and openstack_networking_floatingip_associate_v2
to attach floating ips. This gives us more flexibility on disabling port security
when binding instances directly on provider networks in private cloud scenario.
@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 20, 2022
@k8s-ci-robot k8s-ci-robot requested review from bozzo and EppO January 20, 2022 15:39
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 20, 2022
@cristicalin
Copy link
Contributor Author

/cc @r0b2g1t - could you weigh in on the opentelekom impact of this proposed change since you originally added the workaround in #4321 ?

@r0b2g1t
Copy link
Contributor

r0b2g1t commented Jan 25, 2022

Hi, I haven't currently the ability to test it. From my prospective I think that should work. The OTC supports this types of resources.

One remark, I may recommend removing the tag /kind bug from this PR, it's more an improvement instead of a fix of something.

@cristicalin
Copy link
Contributor Author

Thanks @r0b2g1t for this info, in this case I think we can merge it as is.

One remark, I may recommend removing the tag /kind bug from this PR, it's more an improvement instead of a fix of something.

I had previously introduced the ability to disable port security at the network level but this uncovered a bug when using it on a provider network so from this perspective it is both a bug fix and a feature at the same time.

/cc @floryut @oomichi

@r0b2g1t
Copy link
Contributor

r0b2g1t commented Jan 25, 2022

I had previously introduced the ability to disable port security at the network level but this uncovered a bug when using it on a provider network so from this perspective it is both a bug fix and a feature at the same time.

I understand, maybe you can add this sentence to the Fix paragraph of this PR description? Thank you in advance. :-)

@oomichi
Copy link
Contributor

oomichi commented Jan 25, 2022

I am fine to go forward with this pull request.
I will +1 after updating the description as @r0b2g1t said.

@cristicalin
Copy link
Contributor Author

I updated the description but I'm somewhat confused of this request since it does not end up in the git commit message.

@r0b2g1t
Copy link
Contributor

r0b2g1t commented Jan 25, 2022

Thanks, for adding it to the description. The advantage is that it is easier for reviewers and developers who are not so deeply involved in the topic to get started and understand the history of it. That this is a fix for a bug that was triggered by the previous PR that allows port security to be switched off was not obvious to me from the beginning.

@cristicalin
Copy link
Contributor Author

/cc @floryut @oomichi , can I get a lgtm on this?

Copy link
Member

@floryut floryut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cristicalin Good job 👍

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cristicalin, floryut

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 2, 2022
@floryut floryut added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed kind/feature Categorizes issue or PR as related to a new feature. labels Feb 2, 2022
@oomichi
Copy link
Contributor

oomichi commented Feb 2, 2022

Thanks for updating.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 2, 2022
@k8s-ci-robot k8s-ci-robot merged commit 7759494 into kubernetes-sigs:master Feb 2, 2022
sakuraiyuta pushed a commit to sakuraiyuta/kubespray that referenced this pull request Apr 16, 2022
…ubernetes-sigs#8455)

Use openstack_networking_port_v2 and openstack_networking_floatingip_associate_v2
to attach floating ips. This gives us more flexibility on disabling port security
when binding instances directly on provider networks in private cloud scenario.
@oomichi oomichi mentioned this pull request May 28, 2022
LuckySB pushed a commit to southbridgeio/kubespray that referenced this pull request Jun 29, 2023
…ubernetes-sigs#8455)

Use openstack_networking_port_v2 and openstack_networking_floatingip_associate_v2
to attach floating ips. This gives us more flexibility on disabling port security
when binding instances directly on provider networks in private cloud scenario.
LuckySB pushed a commit to southbridgeio/kubespray that referenced this pull request Oct 21, 2023
…ubernetes-sigs#8455)

Use openstack_networking_port_v2 and openstack_networking_floatingip_associate_v2
to attach floating ips. This gives us more flexibility on disabling port security
when binding instances directly on provider networks in private cloud scenario.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants