)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":7973,"name":"Douglas Mendizábal","email":"dmendiza@redhat.com","username":"dougmendizabal"},"change_message_id":"37b3d86934971bcd4117c9d1b1d2496aba6bb2ee","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"fe9e62a9_3398d512","updated":"2022-01-21 16:55:38.000000000","message":"Is this ready to move from PoC to actual implementation?","commit_id":"db7758a1456f7a3d4034aa0d1c11c19aed29e52f"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"3a36d1c941da6b035bbd935295bdd9a72b866eab","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"a273ab03_df2a667c","in_reply_to":"33564d49_1121b183","updated":"2022-01-24 14:20:48.000000000","message":"Also the associated spec is not merged yet, Once the spec approved I will move the code/open it for review.\n\n\nhttps://review.opendev.org/c/openstack/keystone-specs/+/818603","commit_id":"db7758a1456f7a3d4034aa0d1c11c19aed29e52f"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"047b44c80bd53452f93c25a8e8f078c413e745e4","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"33564d49_1121b183","in_reply_to":"fe9e62a9_3398d512","updated":"2022-01-21 17:03:52.000000000","message":"Ok to move it as it is well tested locally, I think it will need release note and proper commit message, so if anyone is interested can own this or I can push it next week.\n\nThank you!","commit_id":"db7758a1456f7a3d4034aa0d1c11c19aed29e52f"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"922b8b7c67850491aeb8781080a92d63e336b248","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"9345e6ae_0cd1eb2e","updated":"2023-05-24 13:53:07.000000000","message":"Abhishek answered my question, this LGTM.","commit_id":"f13ef5db3f2456e227cd0be32679c78725a61495"},{"author":{"_account_id":7973,"name":"Douglas Mendizábal","email":"dmendiza@redhat.com","username":"dougmendizabal"},"change_message_id":"594d98e86ef8f8ceb135841d48489d056cf0cd03","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"3df2ab49_0fcebae5","updated":"2023-08-28 19:53:35.000000000","message":"Finally had some time to test this out.  In my devstack env, `keystone-manage bootstrap` is not setting up the role implications correctly during an upgrade.\n\nI expect the entries in implied_role after an upgrade with this patch to be:\n\nadmin -\u003e manager\nmanager -\u003e member\nmember -\u003e reader\n\nWhich are all there, but there is an extra entry where admin -\u003e member, which is left over from the previous version.  I think we should clean that up as it may cause issues.","commit_id":"f13ef5db3f2456e227cd0be32679c78725a61495"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"21786ac497909bc38f7412c48cc02ed3a228971d","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"725ad7dd_bc4896eb","updated":"2022-03-28 16:02:10.000000000","message":"How are we handling the situation where a \u0027manager\u0027 role already exists in the deployment?  The spec says we\u0027re supposed to handle it gracefully and give the existing role precedence, but I don\u0027t see any way to do that (give existing role precedence) and also have the new role named \u0027manager\u0027.  I think we have to refrain from doing anything if a \u0027manager\u0027 role already exists, and let the operator migrate the local \u0027manager\u0027 role (and its occurrence in all policy files) to something else.  The other thing an operator could do is just never upgrade and always use their local policy files.  I don\u0027t really have any helpful suggestions other than that we need to be ready to handle this case.","commit_id":"f13ef5db3f2456e227cd0be32679c78725a61495"},{"author":{"_account_id":16465,"name":"Kristi Nikolla","email":"knikolla@bu.edu","username":"knikolla"},"change_message_id":"535d17272f0b4b1b636b25aa181b569b3b6c3a53","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"13ade8f8_3ffff412","updated":"2022-02-11 16:44:40.000000000","message":"Looks good but needs a release note. ","commit_id":"f13ef5db3f2456e227cd0be32679c78725a61495"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"7d01601be729f1260d3ac226ab722b016a8000d4","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"87b0a081_e9d746a7","in_reply_to":"725ad7dd_bc4896eb","updated":"2023-05-23 17:25:22.000000000","message":"Brian,\n\nAFAIK, if you see line #177 (https://review.opendev.org/c/openstack/keystone/+/822601/2/keystone/cmd/bootstrap.py#177) in this patch, it ensures the manager role exists or not, if it exists then it will use the same role and creates implied role. If that is also existed then it will skip the same with Info log \"Implied role where %s implies %s exists, skipping creation.\" (see line #179)\n\nI think it is handle effectively here, there might be some corner cases but as per our spec I think we are good here.","commit_id":"f13ef5db3f2456e227cd0be32679c78725a61495"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"922b8b7c67850491aeb8781080a92d63e336b248","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"c95ee461_d3f0c87f","in_reply_to":"87b0a081_e9d746a7","updated":"2023-05-24 13:53:07.000000000","message":"Thanks for pointing that out!  You\u0027re right that we\u0027re giving the current role precedence in that we\u0027re re-using it; I think I was reading too much into the spec.  I\u0027m still a bit worried about what happens if the existing role has been used in a manner unlike what we\u0027re using it for, but I guess that\u0027s a different issue that can be dealt with in the \u0027upgrade\u0027 section of the release notes.","commit_id":"f13ef5db3f2456e227cd0be32679c78725a61495"},{"author":{"_account_id":8556,"name":"Ghanshyam Maan","display_name":"Ghanshyam Maan","email":"gmaan.os14@gmail.com","username":"ghanshyam"},"change_message_id":"6ac87ee041ea1d8cf42724ce2055e03361175e9a","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"e234f746_fdac369a","updated":"2023-08-31 17:17:34.000000000","message":"1 comment/question inline","commit_id":"bbbbd10e482a67b53187a93daa58f8bee770852f"},{"author":{"_account_id":7414,"name":"David Wilde","email":"dwilde@redhat.com","username":"d34dh0r53"},"change_message_id":"5f5bdeaae24d454929d53f80be036fee6b642ed4","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"809507d0_a5fb0859","updated":"2023-09-13 12:17:18.000000000","message":"recheck","commit_id":"bbbbd10e482a67b53187a93daa58f8bee770852f"},{"author":{"_account_id":7414,"name":"David Wilde","email":"dwilde@redhat.com","username":"d34dh0r53"},"change_message_id":"bfb43f82e347b3a34b3619bc9b58c84bde57ebea","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"4b110d39_a24dae8d","updated":"2023-09-06 14:10:04.000000000","message":"recheck - k2kfailure","commit_id":"bbbbd10e482a67b53187a93daa58f8bee770852f"},{"author":{"_account_id":8556,"name":"Ghanshyam Maan","display_name":"Ghanshyam Maan","email":"gmaan.os14@gmail.com","username":"ghanshyam"},"change_message_id":"047bed7fbe947801a448a2608ee5828d4b9ab2f3","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"8bb39569_e53bebbc","updated":"2023-08-31 18:11:02.000000000","message":"thanks Douglas for changes and clarification. lgtm","commit_id":"bbbbd10e482a67b53187a93daa58f8bee770852f"}],"keystone/cmd/bootstrap.py":[{"author":{"_account_id":7973,"name":"Douglas Mendizábal","email":"dmendiza@redhat.com","username":"dougmendizabal"},"change_message_id":"9d1a84ec7bb32460194e42711fc6493b1a7955e7","unresolved":true,"context_lines":[{"line_number":181,"context_line":"    def _bootstrap_admin_role(self):"},{"line_number":182,"context_line":"        role \u003d self._ensure_role_exists(self.admin_role_name)"},{"line_number":183,"context_line":"        self.admin_role_id \u003d role[\u0027id\u0027]"},{"line_number":184,"context_line":"        self._ensure_implied_role(self.admin_role_id, self.manager_role_id)"},{"line_number":185,"context_line":""},{"line_number":186,"context_line":"    def _bootstrap_admin_user(self):"},{"line_number":187,"context_line":"        # NOTE(morganfainberg): Do not create the user if it already exists."}],"source_content_type":"text/x-python","patch_set":2,"id":"24f82f04_5506325b","line":184,"range":{"start_line":184,"start_character":54,"end_line":184,"end_character":74},"updated":"2022-02-11 16:45:07.000000000","message":"I need to check whether this will work on an upgrade.  That is to say, whether the implied role will be updated from member -\u003e manager when this is run on an existing deployment.","commit_id":"f13ef5db3f2456e227cd0be32679c78725a61495"},{"author":{"_account_id":8556,"name":"Ghanshyam Maan","display_name":"Ghanshyam Maan","email":"gmaan.os14@gmail.com","username":"ghanshyam"},"change_message_id":"daa8b81156fa8147598028f134bb7c683e1e3c16","unresolved":true,"context_lines":[{"line_number":181,"context_line":"    def _bootstrap_admin_role(self):"},{"line_number":182,"context_line":"        role \u003d self._ensure_role_exists(self.admin_role_name)"},{"line_number":183,"context_line":"        self.admin_role_id \u003d role[\u0027id\u0027]"},{"line_number":184,"context_line":"        self._ensure_implied_role(self.admin_role_id, self.manager_role_id)"},{"line_number":185,"context_line":""},{"line_number":186,"context_line":"    def _bootstrap_admin_user(self):"},{"line_number":187,"context_line":"        # NOTE(morganfainberg): Do not create the user if it already exists."}],"source_content_type":"text/x-python","patch_set":2,"id":"9ed3714a_824a2a2f","line":184,"range":{"start_line":184,"start_character":54,"end_line":184,"end_character":74},"in_reply_to":"24f82f04_5506325b","updated":"2023-06-15 17:05:35.000000000","message":"I did not get the comment. you mean member will have manager power in implied roles? \n\nWe want manager to implied of member permission not vice versa right?","commit_id":"f13ef5db3f2456e227cd0be32679c78725a61495"},{"author":{"_account_id":7973,"name":"Douglas Mendizábal","email":"dmendiza@redhat.com","username":"dougmendizabal"},"change_message_id":"34b4ee87abbf8a055e96a37223b451205d659da5","unresolved":true,"context_lines":[{"line_number":181,"context_line":"    def _bootstrap_admin_role(self):"},{"line_number":182,"context_line":"        role \u003d self._ensure_role_exists(self.admin_role_name)"},{"line_number":183,"context_line":"        self.admin_role_id \u003d role[\u0027id\u0027]"},{"line_number":184,"context_line":"        self._ensure_implied_role(self.admin_role_id, self.manager_role_id)"},{"line_number":185,"context_line":""},{"line_number":186,"context_line":"    def _bootstrap_admin_user(self):"},{"line_number":187,"context_line":"        # NOTE(morganfainberg): Do not create the user if it already exists."}],"source_content_type":"text/x-python","patch_set":2,"id":"c03393da_e832eba9","line":184,"range":{"start_line":184,"start_character":54,"end_line":184,"end_character":74},"in_reply_to":"9ed3714a_824a2a2f","updated":"2023-08-30 20:06:05.000000000","message":"Fixed the data inconsistency during an upgrade in the latest patch.","commit_id":"f13ef5db3f2456e227cd0be32679c78725a61495"},{"author":{"_account_id":7973,"name":"Douglas Mendizábal","email":"dmendiza@redhat.com","username":"dougmendizabal"},"change_message_id":"8f62faaf652fa671955f8428df82a1894087bc3a","unresolved":false,"context_lines":[{"line_number":181,"context_line":"    def _bootstrap_admin_role(self):"},{"line_number":182,"context_line":"        role \u003d self._ensure_role_exists(self.admin_role_name)"},{"line_number":183,"context_line":"        self.admin_role_id \u003d role[\u0027id\u0027]"},{"line_number":184,"context_line":"        self._ensure_implied_role(self.admin_role_id, self.manager_role_id)"},{"line_number":185,"context_line":""},{"line_number":186,"context_line":"    def _bootstrap_admin_user(self):"},{"line_number":187,"context_line":"        # NOTE(morganfainberg): Do not create the user if it already exists."}],"source_content_type":"text/x-python","patch_set":2,"id":"10340ad8_a8a1e016","line":184,"range":{"start_line":184,"start_character":54,"end_line":184,"end_character":74},"in_reply_to":"c03393da_e832eba9","updated":"2023-08-30 20:06:28.000000000","message":"Done","commit_id":"f13ef5db3f2456e227cd0be32679c78725a61495"},{"author":{"_account_id":8556,"name":"Ghanshyam Maan","display_name":"Ghanshyam Maan","email":"gmaan.os14@gmail.com","username":"ghanshyam"},"change_message_id":"6ac87ee041ea1d8cf42724ce2055e03361175e9a","unresolved":true,"context_lines":[{"line_number":190,"context_line":"        role \u003d self._ensure_role_exists(self.admin_role_name)"},{"line_number":191,"context_line":"        self.admin_role_id \u003d role[\u0027id\u0027]"},{"line_number":192,"context_line":"        self._ensure_implied_role(self.admin_role_id, self.manager_role_id)"},{"line_number":193,"context_line":"        # NOTE(dmendiza): deployments older than 2023.2 did not have a"},{"line_number":194,"context_line":"        # \"manager\" role, so we need to clean up the old admin -\u003e member"},{"line_number":195,"context_line":"        # implied role"},{"line_number":196,"context_line":"        try:"},{"line_number":197,"context_line":"            PROVIDERS.role_api.delete_implied_role(self.admin_role_id,"},{"line_number":198,"context_line":"                                                   self.member_role_id)"},{"line_number":199,"context_line":"        except exception.ImpliedRoleNotFound:"},{"line_number":200,"context_line":"            pass"},{"line_number":201,"context_line":""},{"line_number":202,"context_line":"    def _bootstrap_admin_user(self):"},{"line_number":203,"context_line":"        # NOTE(morganfainberg): Do not create the user if it already exists."}],"source_content_type":"text/x-python","patch_set":4,"id":"c2f3e1d2_3cb202a0","line":200,"range":{"start_line":193,"start_character":0,"end_line":200,"end_character":16},"updated":"2023-08-31 17:17:34.000000000","message":"I am little confused on this. so if old deployment does not have manager role then:\n\n1. does running this new bootstrap on old deployment with no manager role fail the implied role of admin-\u003emanager? because no manager role.\n\n2. old deployment with no manager role we need to keep admin-\u003emember implied role right? but it seems L197 deleting that\n\nOR we do not expect people to run the latest bootstrap on old deployment.  if so then I got your point and  we are good here,","commit_id":"bbbbd10e482a67b53187a93daa58f8bee770852f"},{"author":{"_account_id":8556,"name":"Ghanshyam Maan","display_name":"Ghanshyam Maan","email":"gmaan.os14@gmail.com","username":"ghanshyam"},"change_message_id":"047bed7fbe947801a448a2608ee5828d4b9ab2f3","unresolved":false,"context_lines":[{"line_number":190,"context_line":"        role \u003d self._ensure_role_exists(self.admin_role_name)"},{"line_number":191,"context_line":"        self.admin_role_id \u003d role[\u0027id\u0027]"},{"line_number":192,"context_line":"        self._ensure_implied_role(self.admin_role_id, self.manager_role_id)"},{"line_number":193,"context_line":"        # NOTE(dmendiza): deployments older than 2023.2 did not have a"},{"line_number":194,"context_line":"        # \"manager\" role, so we need to clean up the old admin -\u003e member"},{"line_number":195,"context_line":"        # implied role"},{"line_number":196,"context_line":"        try:"},{"line_number":197,"context_line":"            PROVIDERS.role_api.delete_implied_role(self.admin_role_id,"},{"line_number":198,"context_line":"                                                   self.member_role_id)"},{"line_number":199,"context_line":"        except exception.ImpliedRoleNotFound:"},{"line_number":200,"context_line":"            pass"},{"line_number":201,"context_line":""},{"line_number":202,"context_line":"    def _bootstrap_admin_user(self):"},{"line_number":203,"context_line":"        # NOTE(morganfainberg): Do not create the user if it already exists."}],"source_content_type":"text/x-python","patch_set":4,"id":"415e57d5_bd669b0e","line":200,"range":{"start_line":193,"start_character":0,"end_line":200,"end_character":16},"in_reply_to":"0f54b448_b0da8b72","updated":"2023-08-31 18:11:02.000000000","message":"I see, thanks for detail explanation. basically we do create the manager role always with this bootstrap so anyone running new bootstrap on old get manager role. its just we need to cleanup admin-\u003emember implied role if exist. Got it.","commit_id":"bbbbd10e482a67b53187a93daa58f8bee770852f"},{"author":{"_account_id":7973,"name":"Douglas Mendizábal","email":"dmendiza@redhat.com","username":"dougmendizabal"},"change_message_id":"853dac6f132d0b5133918a83b8c8938448c61ce8","unresolved":false,"context_lines":[{"line_number":190,"context_line":"        role \u003d self._ensure_role_exists(self.admin_role_name)"},{"line_number":191,"context_line":"        self.admin_role_id \u003d role[\u0027id\u0027]"},{"line_number":192,"context_line":"        self._ensure_implied_role(self.admin_role_id, self.manager_role_id)"},{"line_number":193,"context_line":"        # NOTE(dmendiza): deployments older than 2023.2 did not have a"},{"line_number":194,"context_line":"        # \"manager\" role, so we need to clean up the old admin -\u003e member"},{"line_number":195,"context_line":"        # implied role"},{"line_number":196,"context_line":"        try:"},{"line_number":197,"context_line":"            PROVIDERS.role_api.delete_implied_role(self.admin_role_id,"},{"line_number":198,"context_line":"                                                   self.member_role_id)"},{"line_number":199,"context_line":"        except exception.ImpliedRoleNotFound:"},{"line_number":200,"context_line":"            pass"},{"line_number":201,"context_line":""},{"line_number":202,"context_line":"    def _bootstrap_admin_user(self):"},{"line_number":203,"context_line":"        # NOTE(morganfainberg): Do not create the user if it already exists."}],"source_content_type":"text/x-python","patch_set":4,"id":"0f54b448_b0da8b72","line":200,"range":{"start_line":193,"start_character":0,"end_line":200,"end_character":16},"in_reply_to":"c2f3e1d2_3cb202a0","updated":"2023-08-31 18:07:07.000000000","message":"A1: No, executing `keystone-manage bootstrap` with this patch creates the \"manager\" role first before adding the admin-\u003emanager implied role.\n\nA2: We delete the direct implied role, but we\u0027ll still have the transitive implied role there.  In other words, before this patch, `keystone-manage bootstrap` creates the following implied roles:\n\n    admin  -\u003e member\n    member -\u003e reader\n\nHere, admin also implies reader even though there is no direct mapping from admin -\u003e reader.  This is because implied roles cascade down.\n\nWith this patch on a new install we get:\n\n    admin   -\u003e manager\n    manager -\u003e member\n    member  -\u003e reader\n    \nJust like admin implied reader before, admin still implies reader now, and it also implies member because the implications cascade down.  The issue is that running bootstrap with the previous patchset on an old deployment still kept the previous entries in the database, so, on an upgrade we would end up with:\n\n    admin   -\u003e manager\n    admin   -\u003e member\n    manager -\u003e member\n    member  -\u003e reader\n\nIn this case, admin implies member twice.  Once directly, and again through the manager role.  So we delete the admin-\u003emember implied role, because it was put there before this patch.  I hope that makes it clearer.","commit_id":"bbbbd10e482a67b53187a93daa58f8bee770852f"}]}