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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 14 additions & 2 deletions lib/ansible/modules/cloud/openstack/os_port.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,11 @@
choices: [normal, direct, direct-physical, macvtap, baremetal, virtio-forwarder]
default: normal
version_added: "2.8"
port_security_enabled:
description:
- Whether to enable or disable the port security on the network.
type: bool
version_added: "2.8"
'''

EXAMPLES = '''
Expand Down Expand Up @@ -202,6 +207,10 @@
description: Type of the created port
returned: success
type: str
port_security_enabled:
description: Port security state on the network.
returned: success
type: bool
'''

from ansible.module_utils.basic import AnsibleModule
Expand All @@ -217,7 +226,8 @@ def _needs_update(module, port, cloud):
'mac_address',
'device_owner',
'device_id',
'binding:vnic_type']
'binding:vnic_type',
'port_security_enabled']
compare_dict = ['allowed_address_pairs',
'extra_dhcp_opts']
compare_list = ['security_groups']
Expand Down Expand Up @@ -283,7 +293,8 @@ def _compose_port_args(module, cloud):
'extra_dhcp_opts',
'device_owner',
'device_id',
'binding:vnic_type']
'binding:vnic_type',
'port_security_enabled']
for optional_param in optional_parameters:
if module.params[optional_param] is not None:
port_kwargs[optional_param] = module.params[optional_param]
Expand Down Expand Up @@ -319,6 +330,7 @@ def main():
vnic_type=dict(default='normal',
choices=['normal', 'direct', 'direct-physical',
'macvtap', 'baremetal', 'virtio-forwarder']),
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

)

module_kwargs = openstack_module_kwargs(
Expand Down