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.

Sign up for GitHub

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

Jump to bottom

Add port_security_enabled argument to os_port module #47715

Merged
merged 1 commit into from
Mar 7, 2019
Merged

Add port_security_enabled argument to os_port module #47715

merged 1 commit into from
Mar 7, 2019

Conversation

MaxBab
Copy link
Contributor

@MaxBab MaxBab commented Oct 27, 2018

SUMMARY

Port security could be set during creation of the port on the network.
Add port_security_enabled boolean during port creation.

Fixes: #24694

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

os_port

ANSIBLE VERSION

ansible 2.7.0
config file = /etc/ansible/ansible.cfg
configured module search path = [u'/home/stack/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
ansible python module location = /tmp/ansible_venv/lib/python2.7/site-packages/ansible
executable location = /tmp/ansible_venv/bin/ansible
python version = 2.7.5 (default, May 31 2018, 09:41:32) [GCC 4.8.5 20150623 (Red Hat 4.8.5-28)]

ADDITIONAL INFORMATION

Depends-On: https://review.openstack.org/#/c/613759

@ansibot
Copy link
Contributor

ansibot commented Oct 27, 2018

Hi @MaxBab, thank you for submitting this pull-request!

click here for bot help

@ansibot
Copy link
Contributor

ansibot commented Oct 27, 2018

cc @cloudnull @dagnello @emonty @evrardjp @juliakreger @mnaser @odyssey4me @rcarrillocruz @Shrews
click here for bot help

@ansibot ansibot added affects_2.8 This issue/PR affects Ansible v2.8 cloud community_review In order to be merged, this PR must follow the community review workflow. feature This issue/PR relates to a feature request. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. new_contributor This PR is the first contribution by a new community member. openstack support:community This issue/PR relates to code supported by the Ansible community. labels Oct 27, 2018
@opendev-zuul
Copy link

opendev-zuul bot commented Oct 27, 2018

Build succeeded (third-party-check pipeline).

@jborean93 jborean93 removed the needs_triage Needs a first human triage before being processed. label Nov 1, 2018
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Nov 9, 2018
@mnaser
Copy link
Contributor

mnaser commented Nov 12, 2018

shipit

jamescassell
jamescassell suggested changes Nov 12, 2018
View reviewed changes
lib/ansible/modules/cloud/openstack/os_port.py
@@ -291,6 +302,7 @@ def main():
device_owner=dict(default=None),
device_id=dict(default=None),
state=dict(default='present', choices=['absent', 'present']),
port_security_enabled=dict(default=None, type='bool')
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the default for this should be True, shouldn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is true.
When not providing any value regarding this argument, OpenStack automatically set it as True.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @jamescassell ,

Is it acceptable for you that the argument default is None?
I think if the argument is not used, we should rely on the neutron default behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with shipping as is, but i'm not the best person to say.

Copy link
Contributor

Choose a reason for hiding this comment

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

This can be set on OpenStack network level also. So my thought would be that the default should be None, in which case the port will inherit the setting from the network. This is similar to how OpenStackSDK itself, and "openstack port-create" CLI, behave. I guess that setting the default to True would override that always, and enable it even if it the user did not specify it and the network default was set to "False". Also, in case of port update we presumably want to leave this parameter as-is unless it is explicitly specified.

Copy link
Contributor

Choose a reason for hiding this comment

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

it should be None, to allow taking a really default from the system, and not override it

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Nov 12, 2018
@MaxBab
Copy link
Contributor Author

MaxBab commented Dec 10, 2018

Hello @emonty ,

Could you take a look at the patch, please?

Thank you.

@MaxBab
Add port_security_enabled argument to os_port module
8e37ad7 
Port security could be set during creation of the port on the network.
Add port_security_enabled boolean during port creation.
@MaxBab MaxBab force-pushed the add_port_security_enabled_to_os_port branch from 20e84fd to 8e37ad7 Compare January 24, 2019 10:40
@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. new_contributor This PR is the first contribution by a new community member. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Jan 24, 2019
@opendev-zuul
Copy link

opendev-zuul bot commented Jan 24, 2019

Build succeeded (third-party-check pipeline).

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Jan 24, 2019
@MaxBab
Copy link
Contributor Author

MaxBab commented Jan 24, 2019

Hello @mnaser @gundalow @odyssey4me @abadger ,

I rebased the patch according to the latest changes.
Could you take a look, please?

Thank you.

@mnaser
Copy link
Contributor

mnaser commented Jan 28, 2019

shipit

@ansibot ansibot added stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_review Updates were made after the last review and the last review is more than 7 days old. labels Feb 7, 2019
@MaxBab
Copy link
Contributor Author

MaxBab commented Feb 11, 2019

Hello @odyssey4me ,
Could you take a look at the patch, please?

Thanks.

@gtema gtema mentioned this pull request Feb 19, 2019
Closed
@vkhitrin
Copy link

This is a really useful change, could it please be reviewed @gundalow?

gtema
gtema approved these changes Feb 21, 2019
View reviewed changes
lib/ansible/modules/cloud/openstack/os_port.py
@@ -291,6 +302,7 @@ def main():
device_owner=dict(default=None),
device_id=dict(default=None),
state=dict(default='present', choices=['absent', 'present']),
port_security_enabled=dict(default=None, type='bool')
Copy link
Contributor

Choose a reason for hiding this comment

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

it should be None, to allow taking a really default from the system, and not override it

Yarboa pushed a commit to redhat-openstack/ansible-nfv that referenced this pull request Feb 24, 2019
@MaxBab
Add port security feature
8af4441 
Add the port security feature to the os_port module and openstack_tasks
network section.

The port security feature patch currently in review upstream.
ansible/ansible#47715

Change-Id: I93237fe4a141fe95431ffb230566ec0aca746c23
@Yarboa
Copy link

Yarboa commented Feb 28, 2019

This is a really useful change for operating cloud operations, could it please be reviewed @gundalow,
@odyssey4me ?

@odyssey4me
Copy link
Contributor

shipit

@gtema
Copy link
Contributor

gtema commented Feb 28, 2019

!needs_revision

@MaxBab
Copy link
Contributor Author

MaxBab commented Mar 7, 2019

Hello @cloudnull @dagnello @emonty @evrardjp @juliakreger @mnaser @odyssey4me @rcarrillocruz @Shrews,

Could you help me to promote this patch, please?
I got 2 shipit approvals, but the problem is that between the first and second shipit, ansibot assigned a stale_ci label.
As far as I understand, it requires additional steps to approve it.
Am I right?

The change is used by me, and I would like it to be an official part of the module.

Thank you.

@ansibot ansibot added shipit This PR is ready to be merged by Core automerge This PR was automatically merged by ansibot. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Mar 7, 2019
@ansibot ansibot merged commit bf58f84 into ansible:devel Mar 7, 2019
@MaxBab MaxBab deleted the add_port_security_enabled_to_os_port branch March 7, 2019 20:01
@ansible ansible locked and limited conversation to collaborators Jul 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jamescassell jamescassell jamescassell requested changes

@carstenkoester carstenkoester carstenkoester left review comments

@odyssey4me odyssey4me odyssey4me approved these changes

@gtema gtema gtema approved these changes

Assignees
No one assigned
Labels
affects_2.8 This issue/PR affects Ansible v2.8 automerge This PR was automatically merged by ansibot. cloud feature This issue/PR relates to a feature request. module This issue/PR relates to a module. openstack shipit This PR is ready to be merged by Core stale_review Updates were made after the last review and the last review is more than 7 days old. support:community This issue/PR relates to code supported by the Ansible community.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

os_port - allow port_security_enabled boolean flag to be controlled by the module
10 participants
@MaxBab @ansibot @mnaser @vkhitrin @Yarboa @odyssey4me @gtema @jamescassell @carstenkoester @jborean93