Only admin should update device_id

[Nachi Ueno]

For current implementation, device_id can be updated by non-admin user.
Policy check for device_id is also needed.

[dan wendlandt (danwent)]

what is meant by 'policy change for device_id is also needed'? You mean to enforce some format (e.g., that its a UUID)? I think DHCP may violate that right now.

[Salvatore Orlando (salvatore-orlando)]

Please note that device_id is set by the nova integration API, in nova.network.quantumv2.api.API, method allocate_for_instane.

The corresponding port create operation is then executed with the same context of the nova request. Is that context guaranteed to be always an admin context?

[dan wendlandt (danwent)]

salvatore, that's a very good point. It makes me wonder if it is correct for nova to always be using the user context here, or if it should be an admin user acting on behalf of the tenant. Would be good to check with Yong around his original reasoning of using the existing tenant credentials.

[dan wendlandt (danwent)]

yong, can you comment here? thanks.

[dan wendlandt (danwent)]

we're trying to sort out whether this is a blocker or not. Working through some use cases. Will update soon.

[yong sheng gong (gongysh)]

hi,
First, nova quantum API does it (attach instance) in two ways:
1. list nets, subnets, create new port with device_id. This is in the same tenant context as nova
2.attach instance to port. In this case, we also need to make sure the port belongs to the same nova tenant

our current behaviour is: tenant can only operate his own net and port.
If this is not enough for port, we can make sure port's device_id cannot be changed after first being assigned a value.

if we need admin context to do port's device id assignment, nova quantum api has the quantum user which must be a admin account, since it will list the network information for all instances hosted by that host, so we can use this account.

all in all, we can limit the only once assignment of device_id for tenant user, but allow admin user for change it without limit. This way, we don't need to modify nova code, at the same time, keep the flexibility.

[dan wendlandt (danwent)]

I believe the is a case in the existing nova code were we update the device_id on a port using a non-admin context: https://github.com/openstack/nova/blob/master/nova/network/quantumv2/api.py#L125 . This is the case where the tenant passing in an existing port-id to Nova, rather than a network-id.

[dan wendlandt (danwent)]

I believe that a user that changed with device_ids on its own ports could cause Nova to be confused about what VIFs are on a VM, reporting incorrect IPs, and potentially incorrectly deallocating ports when a VM is torn down. As long as the tenant_id of a port always matches the owner of a Nova instance though, I think this behavior is limited to a tenant confusing itself, but never being able to mess with another tenant.

[yong sheng gong (gongysh)]

So do we need to fix it?
if we need to fix it, how about my advice at floor #6?

[Salvatore Orlando (salvatore-orlando)]

We need to look back at nova and see now if we can do this operation with an admin context without adding extra chattiness on the nova-quantum interface.

This is not enough in terms of importance however IMHO to target this bug to grizzly.

[OpenStack Infra (hudson-openstack)]

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

[Salvatore Orlando (salvatore-orlando)]

Re-triaging to verify whether this can be safely and simply fixed for Havana.

[Salvatore Orlando (salvatore-orlando)]

re-triage analysis.

This could be sorted out in two ways.
1) Make the fields admin_only and fix nova.network.neutronv2.api.API in order to perform the port_update operation needed when a neutron port is passed to create server with admin context.

This is simple, but will look weird and violate the principle of performing port create operations within tenant context

2) Enforce a policy where device_id and device_owner can be updated only if they're empty. Once set, they can be updated only with admin context. This is easy to enforce by hardcoding in db_plugin, but harder to perform in an orthogonal way using the policy engine. I have tried using the field check to authorize only when tenant_id is the owner of the port and device_id is empty, but the policy is enforced on the resource with the updated attributes. the solution would involve the policy engine retrieving a context and performing a query, which sounds expensive.

If we feel we need to fix this, I can probably extend the policy engine to pass both the original and the modified target.
Otherwise let's leave this bug untargeted.

[shihanzhang (shihanzhang)]

I think the 'device_owner' also shouldn't be updated!

[Eugene Nikanorov (enikanorov)]

Lowering as this has not gotten enough attention during last 2 years.

[Salvatore Orlando (salvatore-orlando)]

This actually recently came up again: https://review.openstack.org/#/c/274021/

Not really about device_id, more about device_owner. But the problem is the same.
These two attributes should really become read only, but as they're widely used any change in the logic here requires more thought.

[Launchpad Janitor (janitor)]

[Expired for neutron because there has been no activity for 60 days.]