)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":27900,"name":"Artem Goncharov","email":"artem.goncharov@gmail.com","username":"gtema"},"change_message_id":"a81cdf40b766ecfc9d090053982837193e783220","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"4a937262_ffc6ee03","updated":"2024-01-31 10:48:56.000000000","message":"This all feels totally screwed for me. At the moment specifying project_id is not really having the effect people may be expecting.\n\nWhen a session token is retrieved (authorized) it is bound to a specific project. An attempt to use this token to modify some other project is expected to fail from token scope verification perspective. I see now more and more projects trying to create workarounds for that (i.e. Designate adding new header to operate in other project).\nNow back here: nobody should actually specify project_id at all. This is how it was designed initially (from overall OpenStack auth perspective). Instead resource is created in the project for which the auth token is valid. That would also mean that project_id would not be present in the network search (different story is whether user token is giving privileges to search in other project). In order to also prevent search failure you should be passing network id and not name (but even that may fail depending on auth policies).\n\nI do not think that the proposed change is dangerous, it is not doing things how they were designed to be (and as such required).\nIdea of the test is good, but even that demonstrates potentially wrong usage pattern since it uses \"almighty\" access: end user is not capable of creating another project and put network there with the normal Keystone auth ways (normal user may not be even allowed to list projects). Only admin role user would be able to do so.\n\nIf you have problem with the module right now, can you try getting rid of specifying project?","commit_id":"21b58223c37dd68f9ad8e784cb518d95ffc12ec8"},{"author":{"_account_id":14826,"name":"Mark Goddard","email":"markgoddard86@gmail.com","username":"mgoddard"},"change_message_id":"116cfd62738abea10b653b7437f7c6e75d8f6b46","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":4,"id":"ebd4236c_78a6f41f","in_reply_to":"4a937262_ffc6ee03","updated":"2024-02-26 12:09:27.000000000","message":"Hi Artem, sorry for the slow reply. I\u0027ll try to address your various concerns.\n\nFirst of all, in regards to the main feature of the change, searching networks outside of the project by name:\n\nTypically an external network is a shared network, and often not in the same project as the routers that use it as a gateway. It is not a good user experience to force users to use a UUID to reference a shared external network in another project. It either requires baking a UUID into Ansible config, or using some additional tasks to query the network\u0027s UUID.\n\nNext, the subject of the project_id parameter:\n\nFor non-admin users, you are right. A non-admin user should not be able to create or modify resources in a different project to the one their token is scoped to. \n\nThere is a valid use case where resources for multiple projects are pre-provisioned. Ideally this would not require an admin user, but OpenStack RBAC is what it is. We typically do this using this project: https://github.com/stackhpc/openstack-config, often wrapped in some CI pipeline. Removing the project_id parameter would be harmful to our (and probably many other people\u0027s) use case.\n\nFinally, the subject of use of the admin user in tests:\n\nAFAICT all of the router CI tests currently use an admin user. Where the resources span multiple projects this is necessary. It would be better if the actual test were performed by a non-admin user, but this is not the case. The proposed test follows the same pattern.","commit_id":"21b58223c37dd68f9ad8e784cb518d95ffc12ec8"},{"author":{"_account_id":14826,"name":"Mark Goddard","email":"markgoddard86@gmail.com","username":"mgoddard"},"change_message_id":"f735c0fcf8433e93c0433b3dcac33d2ebd6a2a0b","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":4,"id":"209f2ef7_63096e3f","in_reply_to":"85fbfb28_14fd9f97","updated":"2024-03-19 15:03:01.000000000","message":"\u003e \u003e\u003e It is not a good user experience to force users to use a UUID to reference a shared external network in another project.\n\u003e \n\u003e - if you would be using TF you would first invoke data_source modules to find all resources you need. I am talking here about the same approach\n\nThis change does not prevent that approach, it just makes a name-based approach possible too.\n\n\u003e - imagine performance impact when the search goes through all available resources (just accidentially recently I found an abnormal behavior in Neutron that i.e. listing ports in big project takes 10s with admin creds and 5min with regular user creds. This is caused by inefficient policies evaluation). Your change basically assumes 2 API calls with 1st known to always fail (in the shared net usecase)\n\nWe\u0027re not talking about listing all networks, only querying a network by its name. That should be fast, whether global or project-scoped.\n\nThe first call only fails in the case where the network is specified by name and is not in the same project.\n\nOverall it seems you\u0027re set against this approach. Which is a shame because it fixes a regression in a feature that you may not like, but does exist. The fix also doesn\u0027t have negative impact on the currently working paths.","commit_id":"21b58223c37dd68f9ad8e784cb518d95ffc12ec8"},{"author":{"_account_id":27900,"name":"Artem Goncharov","email":"artem.goncharov@gmail.com","username":"gtema"},"change_message_id":"d7229ae40aa29277dea621e5e21430dded18b835","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":4,"id":"85fbfb28_14fd9f97","in_reply_to":"ebd4236c_78a6f41f","updated":"2024-03-14 10:42:26.000000000","message":"\u003e\u003e It is not a good user experience to force users to use a UUID to reference a shared external network in another project.\n\n- if you would be using TF you would first invoke data_source modules to find all resources you need. I am talking here about the same approach\n- imagine performance impact when the search goes through all available resources (just accidentially recently I found an abnormal behavior in Neutron that i.e. listing ports in big project takes 10s with admin creds and 5min with regular user creds. This is caused by inefficient policies evaluation). Your change basically assumes 2 API calls with 1st known to always fail (in the shared net usecase)\n- we introduced a disaster into the OpenStack APIs by the authz scope. That means that user requests access to a certain project. That also means that all resources are going to be created in this project. This means you should not be ever specifying project_id. The only exception here is an admin which bypasses all sort of sanity and rules.\n\nWe all created a huge problem by introducing semi-comfort interfaces across the code base of the client tooling. It is neither complete nor standardized nor efficient. It is just a pain and causes too many bugs (like here some could suggest introducing a new parameter \"network_project_id\"). This is all going wrong direction and I am instead looking into a different direction with auto-generated modules from OpenAPI specs (but you can imagine, that such sort of additional lookups are just not possible to be described reasonably). This way we would expose modules with everything what API can do and let user (or some additional \"usability\" modules) to orchestrate things properly.\n\nYou would be much more efficient if you do first a lookup to find required resource (you can also cache it by any available means) and use it in multiple places across the playbook overall playing a huge role in the overall performance reducing amount of API roundtrips.\n\n\u003e\u003e There is a valid use case where resources for multiple projects are pre-provisioned.\n\nThis is exactly the case where you would heavily benefit from doing a lookup once and reusing the value. You would not even need to drop project_id specification in the playbooks you refer to.\n\n\u003e\u003e Finally, the subject of use of the admin user in tests:\n\nYou refer to the tests currently requiring admin creds. Right. This is what I hate about those. Most of the tests were created for working in default devstack without deeper analysis of auth situation in OpenStack (and how it is different in real clouds). I personally see a lot of super experiences cores across multiple projects that do not fully understand authz concepts in OpenStack. It\u0027s time to start addressing real issues and not add workarounds.\n\nOverall we should not extend projects into direction they follow right now when we clearly see issues with the direction. It only makes any rework attempt more complex.","commit_id":"21b58223c37dd68f9ad8e784cb518d95ffc12ec8"},{"author":{"_account_id":14826,"name":"Mark Goddard","email":"markgoddard86@gmail.com","username":"mgoddard"},"change_message_id":"73147d5eaf2f6051812682fbb132447a66d01f29","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"a71348a3_0a0cbcf0","updated":"2024-03-14 09:44:32.000000000","message":"@Artem any thoughts on my response?","commit_id":"cfd2d1f773a011da21382cc335befc8f52193489"},{"author":{"_account_id":27900,"name":"Artem Goncharov","email":"artem.goncharov@gmail.com","username":"gtema"},"change_message_id":"a20cc02ba358e1556ad2fc66cc072557bbde2450","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"14d21727_4c271f16","updated":"2024-03-19 16:21:45.000000000","message":"I am not against of fixing regression, but against of restoring undesired and unrecommended usage patterns","commit_id":"cfd2d1f773a011da21382cc335befc8f52193489"},{"author":{"_account_id":14826,"name":"Mark Goddard","email":"markgoddard86@gmail.com","username":"mgoddard"},"change_message_id":"99d781ce11b7a1ebd63434b8a5515d7b4454a8a2","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"a478a690_e14f6265","updated":"2024-03-20 13:35:29.000000000","message":"Thank you for approving.","commit_id":"cfd2d1f773a011da21382cc335befc8f52193489"},{"author":{"_account_id":27900,"name":"Artem Goncharov","email":"artem.goncharov@gmail.com","username":"gtema"},"change_message_id":"d7229ae40aa29277dea621e5e21430dded18b835","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"da40f60c_b33e1eb0","updated":"2024-03-14 10:42:26.000000000","message":"sorry, was long time willing to respond but as usual there are always more important things","commit_id":"cfd2d1f773a011da21382cc335befc8f52193489"}],"ci/roles/router/tasks/main.yml":[{"author":{"_account_id":13252,"name":"Dr. Jens Harbott","display_name":"Jens Harbott (frickler)","email":"frickler@offenerstapel.de","username":"jrosenboom"},"change_message_id":"bcac04054f97ee1f5b7c37ba7589650e795f38be","unresolved":true,"context_lines":[{"line_number":332,"context_line":"     state: present"},{"line_number":333,"context_line":"     name: \"{{ external_network_name }}\""},{"line_number":334,"context_line":"     external: true"},{"line_number":335,"context_line":"     project: admin"},{"line_number":336,"context_line":""},{"line_number":337,"context_line":"- name: Create subnet5"},{"line_number":338,"context_line":"  openstack.cloud.subnet:"}],"source_content_type":"text/x-yaml","patch_set":3,"id":"fcaf77c8_b84e60c3","line":335,"updated":"2024-01-19 06:30:21.000000000","message":"Why is this needed?","commit_id":"0763056f6bc931c9af9a86b693bd79da10f3eb2a"},{"author":{"_account_id":14826,"name":"Mark Goddard","email":"markgoddard86@gmail.com","username":"mgoddard"},"change_message_id":"54131574517aceeddbe10c2705179d865b21c153","unresolved":false,"context_lines":[{"line_number":332,"context_line":"     state: present"},{"line_number":333,"context_line":"     name: \"{{ external_network_name }}\""},{"line_number":334,"context_line":"     external: true"},{"line_number":335,"context_line":"     project: admin"},{"line_number":336,"context_line":""},{"line_number":337,"context_line":"- name: Create subnet5"},{"line_number":338,"context_line":"  openstack.cloud.subnet:"}],"source_content_type":"text/x-yaml","patch_set":3,"id":"bdfb3c1d_88dd7f92","line":335,"in_reply_to":"fcaf77c8_b84e60c3","updated":"2024-01-19 10:41:32.000000000","message":"I was trying to test attaching to an external gateway in another project, assuming that the CI would be using a non-admin account for testing where possible. However, that assumption looks to be incorrect. I\u0027ll try to add an explicit regression test.","commit_id":"0763056f6bc931c9af9a86b693bd79da10f3eb2a"}],"plugins/modules/router.py":[{"author":{"_account_id":13252,"name":"Dr. Jens Harbott","display_name":"Jens Harbott (frickler)","email":"frickler@offenerstapel.de","username":"jrosenboom"},"change_message_id":"bcac04054f97ee1f5b7c37ba7589650e795f38be","unresolved":true,"context_lines":[{"line_number":622,"context_line":"            if not network:"},{"line_number":623,"context_line":"                # Fall back to a global search for the network."},{"line_number":624,"context_line":"                network \u003d self.conn.network.find_network(network_name_or_id,"},{"line_number":625,"context_line":"                                                         ignore_missing\u003dFalse)"},{"line_number":626,"context_line":""},{"line_number":627,"context_line":"        # Validate and cache the subnet IDs so we can avoid duplicate checks"},{"line_number":628,"context_line":"        # and expensive API calls."}],"source_content_type":"text/x-python","patch_set":3,"id":"74df22a6_a59d161f","line":625,"updated":"2024-01-19 06:30:21.000000000","message":"Can you add some explicit test for this case to make sure it doesn\u0027t regress again?","commit_id":"0763056f6bc931c9af9a86b693bd79da10f3eb2a"},{"author":{"_account_id":14826,"name":"Mark Goddard","email":"markgoddard86@gmail.com","username":"mgoddard"},"change_message_id":"7ad77dad552162b33f71b77816800b29e029dfa3","unresolved":false,"context_lines":[{"line_number":622,"context_line":"            if not network:"},{"line_number":623,"context_line":"                # Fall back to a global search for the network."},{"line_number":624,"context_line":"                network \u003d self.conn.network.find_network(network_name_or_id,"},{"line_number":625,"context_line":"                                                         ignore_missing\u003dFalse)"},{"line_number":626,"context_line":""},{"line_number":627,"context_line":"        # Validate and cache the subnet IDs so we can avoid duplicate checks"},{"line_number":628,"context_line":"        # and expensive API calls."}],"source_content_type":"text/x-python","patch_set":3,"id":"19e45606_a3bb0970","line":625,"in_reply_to":"74df22a6_a59d161f","updated":"2024-01-19 10:44:46.000000000","message":"Done","commit_id":"0763056f6bc931c9af9a86b693bd79da10f3eb2a"}]}