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

Let's enable enable-ml2-port-security out of the box #218

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nobuto-m
Copy link
Contributor

@nobuto-m nobuto-m commented Jul 22, 2021

enable-ml2-port-security for Yoga+

Neutron's port security extension allows operators to turn on and off
the port security on per port or per network basis. It brings
flexibility to network port management and it's expected to be available
where the extension is available, OVS/OVN deployments in mind. Let's
turn it on for new deployments.

Also, it's must-have for Octavia as per the charm deployment guide so
make sure it's enabled in the Octavia overlay bundle.

@nobuto-m nobuto-m force-pushed the enable-ml2-port-security branch from 7482080 to dc507e2 Compare July 15, 2022 17:25
@nobuto-m
Copy link
Contributor Author

The CI error is on the server side I believe.

Located charm "ceph-radosgw" in charm-hub, channel quincy/stable
ERROR cannot deploy bundle: cannot resolve charm or bundle "cinder": resolving with preferred channel: Post "https://api.charmhub.io/v2/charms/refresh": EOF

Copy link
Contributor

@wolsen wolsen left a comment

Choose a reason for hiding this comment

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

So, Octavia isn't enabled in the bundles by default - but there is a loadbalancer specific overlay in which would arguably be a better place to enable the ml2-port-security since the justification in the commit message is around Octavia Loadbalancers requiring it.

On the other hand, perhaps the default for the neutron-api charm is wrong and the ml2-port-security should be enabled by default rather than disabled by default. I get the sense that it is generally enabled more often than not, which begs the question as to whether the current default is the right choice. Which would also be better to do in the charm than in every single bundle.

Of course, there are drawbacks to changing the default as this changes some important behavior for folks who have not considered it before and they will see a behavior change in the upgrade.

So, I guess in short, I'm less of a fan in changing it in every bundle since Rocky and think this should rather be a discussion about what the right default is here or if we should consider a more targeted change towards the loadbalancer overlay.

@nobuto-m
Copy link
Contributor Author

nobuto-m commented Jul 15, 2022

Hi Billy, thanks for reviewing this. My comments are below:

So, Octavia isn't enabled in the bundles by default - but there is a loadbalancer specific overlay in which would arguably be a better place to enable the ml2-port-security since the justification in the commit message is around Octavia Loadbalancers requiring it.

Yeah, my commit message was too casual. It's a must-have for Octavia, but it should be enabled for almost all cases as you pointed out.

On the other hand, perhaps the default for the neutron-api charm is wrong and the ml2-port-security should be enabled by default rather than disabled by default. I get the sense that it is generally enabled more often than not, which begs the question as to whether the current default is the right choice. Which would also be better to do in the charm than in every single bundle.

In Canonical's field deployments, it's always enabled. The reasons the config is still false in the charm are (in my understanding):

  • port security feature is ML2 plugin dependent, it can be enabled for OVS/OVN for sure but there might be some ML2, which doesn't properly support port security yet.
  • the upgrade concern as you pointed out below. ARP spoofing protection will be enabled on upgrades, which might not be what users would expect (existing vip without using "allowed address pairs" properly may stop working).

EDIT: ARP spoofing protection is enabled even when port security extension is disabled so the statement above is probably not applicable or correct.

So, I guess in short, I'm less of a fan in changing it in every bundle since Rocky and think this should rather be a discussion about what the right default is here or if we should consider a more targeted change towards the loadbalancer overlay.

Initially I thought about it, but:

  • octavia overlay is not maintained (it's separate story)
  • considering a scenario where deploying openstack-base first then adding the Octavia overlay after some time, it changes Neutron's behavior by introducing Octavia. Which is also weird to me.

Openstack-base bundle is opinionated about the architecture (it's OVS/OVN only) so I thought we can be opinionated about ml2-port-security too.

Neutron's port security extension allows operators to turn on and off
the port security on per port or per network basis. It brings
flexibility to network port management and it's expected to be available
where the extension is available, OVS/OVN deployments in mind. Let's
turn it on for new deployments.

Also, it's must-have for Octavia as per the charm deployment guide so
make sure it's enabled in the Octavia overlay bundle.
@nobuto-m nobuto-m force-pushed the enable-ml2-port-security branch from dc507e2 to 72a5e25 Compare July 15, 2022 20:44
@nobuto-m
Copy link
Contributor Author

nobuto-m commented Jul 15, 2022

I've updated the number of files touched and the commit message for the time being.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants