create_port is failing for non admin context as Nova neutronclient interactions are not using admin context consistently

I think the description is slightly wrong.

I took a look at neutron policy file and create_port can be created by any user by default.
https://github.com/openstack/neutron/blob/master/etc/policy.json#L72 and that must be the reason the code was written with a neutronclient without admin context. If the default policy.json is used (which allows all users to create port), everything is good . However, if we customize the policy.json to something like below:

"create_port": "role:member or role:im-custom-role"

For creating port in the deploy flow (which is the flow referred to here), the get_client() does not pass admin as true meaning it uses the token of the logged in user
https://github.com/openstack/nova/blob/master/nova/network/neutronv2/api.py#L749

In the same flow the client context is updated to admin to enable a neutron flow that needs admin (here admin is set to true because the default policy.json doesn't allow all roles to update ports and thus without admin context the below call fails)
https://github.com/openstack/nova/blob/master/nova/network/neutronv2/api.py#L789

        # We always need admin_client to build nw_info,
        # we sometimes need it when updating ports
        admin_client = get_client(context, admin=True)

        ordered_nets, ordered_ports, preexisting_port_ids, \
            created_port_ids = self._update_ports_for_instance(
                context, instance,
                neutron, admin_client, requests_and_created_ports, nets,
                bind_host_id, dhcp_opts, available_macs)

It doesn't make sense for nova to use neutronclient with admin in some cases and without admin in others. It might work with the default neutron policy.json but not with a custom one. The code is not written consistently.

The nova-neutronclient code must either use admin context consistently , in which case it will always use the neutron service user credentials from the nova.conf file from the [neutron] section to make calls to neutron. Or, it must always use the token of the logged-in user. The problem with using the credentials of the logged-in user is that there would be cases where the logged in user's token gets expired mid-way and the nova-neutronclient logic would a fresh authentication with keystone and the credentials used at that time would be service user credentials (as the logged in user's credentials are not available to generate a fresh token) and that would again lead to inconsistencies.

This defect must be used to decide the course of action either way and not leave things inconsistently.

I'm not sure how to proceed here and asked on the ML for advice: http://lists.openstack.org/pipermail/openstack-dev/2016-August/100610.html

The issue could be solved by having either get_client(context, admin=True) at all places or get_client(context) while getting neutron_client for nova. The call should be consistent and not having admin=True for some cases and not for others.

The issue can be summed up like this... when nova has asked neutron to create a port in previous releases, it has always used a token with the admin role. Something has gone into newton that has changed this behavior, which has the potential to break users who have customized neutron's policy.json file. Unless there's a very good reason for this change and it is appropriately documented in the release notes so that folks will know to look for and handle this on upgrades, that behavioral change needs to be reverted.

OK so here is the problem, in master (newton) it's using the non-admin port client:

https://github.com/openstack/nova/blob/master/nova/network/neutronv2/api.py#L702

That's coming from here:

https://github.com/openstack/nova/blob/master/nova/network/neutronv2/api.py#L749

Before newton, that would be the port_client here with admin=True if using the port binding extension:

https://github.com/openstack/nova/blob/stable/mitaka/nova/network/neutronv2/api.py#L564

So before newton we used an admin port client, but with the refactor changes in newton it's using the non-admin client hence the regression.

Per conversation in IRC I volunteered to work on this bug.

Fix proposed to branch: master
Review: https://review.openstack.org/357540

So this change was intentional, we need to get off the admin token.

My preferred way to fix this is to stop deciding if we should use the admin token or not based on if the port_binding extension is enabled.

There is no (sensible) Neutron deployment that would ever have port binding disabled right now, so we probably want to fix that anyway (I will work on a patch for that).

Related fix proposed to branch: master
Review: https://review.openstack.org/357726

@John we noticed this in a deployment that has port_binding extension enabled, so I'm not sure how that would be related.

Thanks John for the clarification and the update! I will work on getting the tests passing with this new change.

@auggy Is your change addressing:

So before newton we used an admin port client, but with the refactor changes in newton it's using the non-admin client hence the regression.
So we will change it all to make it consistent.

Patch in merge conflict, moving this out of In Progress

Automatically discovered version newton in description. If this is incorrect, please update the description to include 'nova version: ...'

Reviewed: https://review.openstack.org/357726
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=7f38e25e7df7be805459998c591e7bf8d900f766
Submitter: Jenkins
Branch: master

commit 7f38e25e7df7be805459998c591e7bf8d900f766
Author: John Garbutt <email address hidden>
Date: Fri Aug 19 10:17:58 2016 +0100

    Assume neutron port_binding extensions enabled

    No recent neutron deployment should ever have the port_binding extension
    missing in its list.

    It appears like this has been the case since this commit in Liberty:
    61121c5f2af27e31092db7ac6947f796198410a8

    It causing lots of confusion around when an admin_client should be used,
    among other things, so lets remove this needless complexity.

    Co-Authored-By: Augustina Ragwitz <email address hidden>
    Change-Id: I5fa73fa0610b23ef231952b2035a284819186a7c
    Related-Bug: 1608601

Change abandoned by Matt Riedemann (<email address hidden>) on branch: master
Review: https://review.opendev.org/357540
Reason: This is super old and in merge conflict so I'm going to abandon. Can be restored and rebased if someone wants to continue working on this.