)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":36412,"name":"Scott Davidson","email":"sdavidson327@gmail.com","username":"sd109"},"change_message_id":"b6e9902f712b59c2a156bc3af8aec324f92a486a","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"041a628f_c099f255","updated":"2025-07-15 15:45:55.000000000","message":"Thanks for the fix, just a couple of minor suggestions","commit_id":"e74fdf1e220939c7146fb51219854df795984f16"}],"magnum_capi_helm/common/app_creds.py":[{"author":{"_account_id":36412,"name":"Scott Davidson","email":"sdavidson327@gmail.com","username":"sd109"},"change_message_id":"b6e9902f712b59c2a156bc3af8aec324f92a486a","unresolved":true,"context_lines":[{"line_number":52,"context_line":"def _create_app_cred(context, cluster):"},{"line_number":53,"context_line":"    if not set(CONF.capi_helm.required_user_roles).issubset("},{"line_number":54,"context_line":"        set(context.roles)"},{"line_number":55,"context_line":"    ):"},{"line_number":56,"context_line":"        raise PolicyNotAuthorized("},{"line_number":57,"context_line":"            message\u003d("},{"line_number":58,"context_line":"                f\"Unauthorized - required roles: \""}],"source_content_type":"text/x-python","patch_set":1,"id":"a5b0d2ba_1be66a7b","line":55,"updated":"2025-07-15 15:45:55.000000000","message":"Nit: You could use set(a) \u003c set(b) here instead of the issubset method.","commit_id":"e74fdf1e220939c7146fb51219854df795984f16"},{"author":{"_account_id":38122,"name":"Jasleen","display_name":"Jasleen Kaur","email":"jasleen@stackhpc.com","username":"JasleenKaur"},"change_message_id":"6c2d3e22d2cc22e627ed5c58df6a0f9ed7adc259","unresolved":false,"context_lines":[{"line_number":52,"context_line":"def _create_app_cred(context, cluster):"},{"line_number":53,"context_line":"    if not set(CONF.capi_helm.required_user_roles).issubset("},{"line_number":54,"context_line":"        set(context.roles)"},{"line_number":55,"context_line":"    ):"},{"line_number":56,"context_line":"        raise PolicyNotAuthorized("},{"line_number":57,"context_line":"            message\u003d("},{"line_number":58,"context_line":"                f\"Unauthorized - required roles: \""}],"source_content_type":"text/x-python","patch_set":1,"id":"2775cedd_163b5027","line":55,"in_reply_to":"a5b0d2ba_1be66a7b","updated":"2025-07-15 16:03:24.000000000","message":"Done","commit_id":"e74fdf1e220939c7146fb51219854df795984f16"},{"author":{"_account_id":36412,"name":"Scott Davidson","email":"sdavidson327@gmail.com","username":"sd109"},"change_message_id":"b6e9902f712b59c2a156bc3af8aec324f92a486a","unresolved":true,"context_lines":[{"line_number":55,"context_line":"    ):"},{"line_number":56,"context_line":"        raise PolicyNotAuthorized("},{"line_number":57,"context_line":"            message\u003d("},{"line_number":58,"context_line":"                f\"Unauthorized - required roles: \""},{"line_number":59,"context_line":"                f\"{CONF.capi_helm.required_user_roles}\""},{"line_number":60,"context_line":"            )"},{"line_number":61,"context_line":"        )"}],"source_content_type":"text/x-python","patch_set":1,"id":"e2ef7330_db2220fd","line":58,"updated":"2025-07-15 15:45:55.000000000","message":"This line doesn\u0027t need to be an f-string","commit_id":"e74fdf1e220939c7146fb51219854df795984f16"},{"author":{"_account_id":38122,"name":"Jasleen","display_name":"Jasleen Kaur","email":"jasleen@stackhpc.com","username":"JasleenKaur"},"change_message_id":"6c2d3e22d2cc22e627ed5c58df6a0f9ed7adc259","unresolved":false,"context_lines":[{"line_number":55,"context_line":"    ):"},{"line_number":56,"context_line":"        raise PolicyNotAuthorized("},{"line_number":57,"context_line":"            message\u003d("},{"line_number":58,"context_line":"                f\"Unauthorized - required roles: \""},{"line_number":59,"context_line":"                f\"{CONF.capi_helm.required_user_roles}\""},{"line_number":60,"context_line":"            )"},{"line_number":61,"context_line":"        )"}],"source_content_type":"text/x-python","patch_set":1,"id":"04b3734c_46abf465","line":58,"in_reply_to":"e2ef7330_db2220fd","updated":"2025-07-15 16:03:24.000000000","message":"Done","commit_id":"e74fdf1e220939c7146fb51219854df795984f16"},{"author":{"_account_id":14394,"name":"Dale Smith","email":"dale@catalystcloud.nz","username":"dalees"},"change_message_id":"e4b5518c4e8e17b4acc1ba7eb0efed4f83071883","unresolved":true,"context_lines":[{"line_number":63,"context_line":"        user\u003dcluster.user_id,"},{"line_number":64,"context_line":"        name\u003df\"magnum-{cluster.uuid}\","},{"line_number":65,"context_line":"        description\u003df\"Magnum cluster ({cluster.uuid})\","},{"line_number":66,"context_line":"        # roles\u003droles,"},{"line_number":67,"context_line":"    )"},{"line_number":68,"context_line":"    return {"},{"line_number":69,"context_line":"        \"clouds\": {"}],"source_content_type":"text/x-python","patch_set":3,"id":"6d165504_6c6d4881","line":66,"updated":"2025-07-16 03:57:57.000000000","message":"Query: Would it be reasonable to include these `required_user_roles` here, and limit the roles in the App Cred?\n\nIt\u0027s broader than the problem you\u0027re trying to solve, but may benefit by limiting the scope of the app cred to only those necessary.","commit_id":"af23f9515ee0880d15e1c3c009ee9f2862203b25"},{"author":{"_account_id":38122,"name":"Jasleen","display_name":"Jasleen Kaur","email":"jasleen@stackhpc.com","username":"JasleenKaur"},"change_message_id":"34b6b6a7c3db56460f32f363136dd27a67abb73d","unresolved":false,"context_lines":[{"line_number":63,"context_line":"        user\u003dcluster.user_id,"},{"line_number":64,"context_line":"        name\u003df\"magnum-{cluster.uuid}\","},{"line_number":65,"context_line":"        description\u003df\"Magnum cluster ({cluster.uuid})\","},{"line_number":66,"context_line":"        # roles\u003droles,"},{"line_number":67,"context_line":"    )"},{"line_number":68,"context_line":"    return {"},{"line_number":69,"context_line":"        \"clouds\": {"}],"source_content_type":"text/x-python","patch_set":3,"id":"6661f9f7_ac365981","line":66,"in_reply_to":"6d165504_6c6d4881","updated":"2025-07-17 15:09:52.000000000","message":"Sure, Added the logic to limit the roles for app cred.","commit_id":"af23f9515ee0880d15e1c3c009ee9f2862203b25"}],"magnum_capi_helm/conf.py":[{"author":{"_account_id":14394,"name":"Dale Smith","email":"dale@catalystcloud.nz","username":"dalees"},"change_message_id":"e4b5518c4e8e17b4acc1ba7eb0efed4f83071883","unresolved":true,"context_lines":[{"line_number":132,"context_line":"    ),"},{"line_number":133,"context_line":"    cfg.ListOpt("},{"line_number":134,"context_line":"        \"required_user_roles\","},{"line_number":135,"context_line":"        default\u003d[\"load-balancer_member\"],"},{"line_number":136,"context_line":"        help\u003d("},{"line_number":137,"context_line":"            \"A comma-separated list of roles required for \""},{"line_number":138,"context_line":"            \"a user to create a workload cluster.\""}],"source_content_type":"text/x-python","patch_set":3,"id":"b2bc4e90_7c274363","line":135,"updated":"2025-07-16 03:57:57.000000000","message":"This doesn\u0027t seem like enough to create a cluster, and doesn\u0027t match keystone default roles[1]. Shouldn\u0027t `member` be included here to allow creation of instances?\n\n\n[1] https://docs.openstack.org/keystone/latest/admin/service-api-protection.html#member","commit_id":"af23f9515ee0880d15e1c3c009ee9f2862203b25"},{"author":{"_account_id":38122,"name":"Jasleen","display_name":"Jasleen Kaur","email":"jasleen@stackhpc.com","username":"JasleenKaur"},"change_message_id":"34b6b6a7c3db56460f32f363136dd27a67abb73d","unresolved":false,"context_lines":[{"line_number":132,"context_line":"    ),"},{"line_number":133,"context_line":"    cfg.ListOpt("},{"line_number":134,"context_line":"        \"required_user_roles\","},{"line_number":135,"context_line":"        default\u003d[\"load-balancer_member\"],"},{"line_number":136,"context_line":"        help\u003d("},{"line_number":137,"context_line":"            \"A comma-separated list of roles required for \""},{"line_number":138,"context_line":"            \"a user to create a workload cluster.\""}],"source_content_type":"text/x-python","patch_set":3,"id":"87c63f3f_68331e0e","line":135,"in_reply_to":"b2bc4e90_7c274363","updated":"2025-07-17 15:09:52.000000000","message":"Done","commit_id":"af23f9515ee0880d15e1c3c009ee9f2862203b25"},{"author":{"_account_id":22629,"name":"Michal Nasiadka","email":"mnasiadka@gmail.com","username":"mnasiadka"},"change_message_id":"a20237ef745268da74b452d12c068dc41cfba1c2","unresolved":true,"context_lines":[{"line_number":130,"context_line":"            \"generated application credentials.\""},{"line_number":131,"context_line":"        ),"},{"line_number":132,"context_line":"    ),"},{"line_number":133,"context_line":"    cfg.ListOpt("},{"line_number":134,"context_line":"        \"required_user_roles\","},{"line_number":135,"context_line":"        default\u003d[\"member\",\"load-balancer_member\"],"},{"line_number":136,"context_line":"        help\u003d("}],"source_content_type":"text/x-python","patch_set":4,"id":"96e42d65_7b9ef231","line":133,"updated":"2025-07-16 14:18:48.000000000","message":"Any pointer why this should be configurable?","commit_id":"b4c71938c6461e6745cac7163791cad472c6b414"},{"author":{"_account_id":22629,"name":"Michal Nasiadka","email":"mnasiadka@gmail.com","username":"mnasiadka"},"change_message_id":"c67a31bbe9b4542334ac00916ba585ae5e0a4fd4","unresolved":true,"context_lines":[{"line_number":130,"context_line":"            \"generated application credentials.\""},{"line_number":131,"context_line":"        ),"},{"line_number":132,"context_line":"    ),"},{"line_number":133,"context_line":"    cfg.ListOpt("},{"line_number":134,"context_line":"        \"required_user_roles\","},{"line_number":135,"context_line":"        default\u003d[\"member\",\"load-balancer_member\"],"},{"line_number":136,"context_line":"        help\u003d("}],"source_content_type":"text/x-python","patch_set":4,"id":"39c2e64f_6a1c4d6b","line":133,"in_reply_to":"092a87a2_c9368ed2","updated":"2025-07-17 13:46:17.000000000","message":"Well, I still think a better approach is to finally start checking the status of things on CAPI mgmt cluster side and fail the cluster creation with some meaningful information.\nI understand it\u0027s not a small code journey - but with this approach I have a feeling we\u0027re going to make people rather unhappy, than happy from that new check.","commit_id":"b4c71938c6461e6745cac7163791cad472c6b414"},{"author":{"_account_id":36412,"name":"Scott Davidson","email":"sdavidson327@gmail.com","username":"sd109"},"change_message_id":"faa9f2525bc353efb185e0654c8abfe9d1f58dac","unresolved":true,"context_lines":[{"line_number":130,"context_line":"            \"generated application credentials.\""},{"line_number":131,"context_line":"        ),"},{"line_number":132,"context_line":"    ),"},{"line_number":133,"context_line":"    cfg.ListOpt("},{"line_number":134,"context_line":"        \"required_user_roles\","},{"line_number":135,"context_line":"        default\u003d[\"member\",\"load-balancer_member\"],"},{"line_number":136,"context_line":"        help\u003d("}],"source_content_type":"text/x-python","patch_set":4,"id":"527016b2_67d7351a","line":133,"in_reply_to":"39c2e64f_6a1c4d6b","updated":"2025-07-17 15:29:36.000000000","message":"Yes I\u0027m also keen to start checking resource status on the CAPI side so that users have more visibility but I think we need [1] to be merged into Magnum core first.\n\nThe problem is that in this particular case, if we don\u0027t block cluster creation before it reaches the CAPI mgmt cluster then cluster deletion also doesn\u0027t work because the cluster create fails due to missing LB permissions then, when the user tries to delete the cluster, that also fails because CAPI tries to list LBs to check if it needs to clean them up and then fails to list due to policy error so cluster gets stuck in DELETE_IN_PROGRESS state instead and the operator needs to do some manual clean up on the management cluster.\n\nPersonally, I think requiring admins to set a capi-helm config option in magnum.cfg if they\u0027re using non-standard role mappings is better than asking them to manually clean up any clusters manually when a user forgets to use the correct permissions...\n\nOther option would be to see if the CAPO folks would be willing to handle HTTP 403 from LB endpoint as unrecoverable error rather than continuing with reconcile loop but that doesn\u0027t seem like an ideal solution either since 403 errors could sometimes be temporary in general.\n\n[1] https://review.opendev.org/c/openstack/magnum/+/948681","commit_id":"b4c71938c6461e6745cac7163791cad472c6b414"},{"author":{"_account_id":36412,"name":"Scott Davidson","email":"sdavidson327@gmail.com","username":"sd109"},"change_message_id":"0d05b8db1d726c14ffff0a1183ed0f259c4fe08f","unresolved":true,"context_lines":[{"line_number":130,"context_line":"            \"generated application credentials.\""},{"line_number":131,"context_line":"        ),"},{"line_number":132,"context_line":"    ),"},{"line_number":133,"context_line":"    cfg.ListOpt("},{"line_number":134,"context_line":"        \"required_user_roles\","},{"line_number":135,"context_line":"        default\u003d[\"member\",\"load-balancer_member\"],"},{"line_number":136,"context_line":"        help\u003d("}],"source_content_type":"text/x-python","patch_set":4,"id":"9accf903_bbbdebe4","line":133,"in_reply_to":"527016b2_67d7351a","updated":"2025-07-22 13:09:53.000000000","message":"I\u0027ve proposed a couple of alternative patches to add a workaround on the CAPI/CAPO side instead:\n- https://github.com/kubernetes-sigs/cluster-api-provider-openstack/pull/2629\n- https://github.com/azimuth-cloud/cluster-api-janitor-openstack/pull/218\n\nIf they\u0027re both accepted then it will at least allow the user to delete clusters that are missing LB permissions but they still won\u0027t be able to see why the cluster was stuck in a CREATING state because the status_reason field on the Magnum object will remain empty.\n\nHopefully we can improve the error status messages on the Magnum side once CAPO gets better at reporting non-fatal errors: https://github.com/kubernetes-sigs/cluster-api-provider-openstack/issues/2379","commit_id":"b4c71938c6461e6745cac7163791cad472c6b414"},{"author":{"_account_id":36412,"name":"Scott Davidson","email":"sdavidson327@gmail.com","username":"sd109"},"change_message_id":"9853836ca186ca70d501a246f0dc3625f68877f0","unresolved":true,"context_lines":[{"line_number":130,"context_line":"            \"generated application credentials.\""},{"line_number":131,"context_line":"        ),"},{"line_number":132,"context_line":"    ),"},{"line_number":133,"context_line":"    cfg.ListOpt("},{"line_number":134,"context_line":"        \"required_user_roles\","},{"line_number":135,"context_line":"        default\u003d[\"member\",\"load-balancer_member\"],"},{"line_number":136,"context_line":"        help\u003d("}],"source_content_type":"text/x-python","patch_set":4,"id":"f3dfa276_75e0b024","line":133,"in_reply_to":"96e42d65_7b9ef231","updated":"2025-07-16 15:14:29.000000000","message":"Seems like the safer option in case operators are defining custom roles / policies on their cloud?","commit_id":"b4c71938c6461e6745cac7163791cad472c6b414"},{"author":{"_account_id":36412,"name":"Scott Davidson","email":"sdavidson327@gmail.com","username":"sd109"},"change_message_id":"58c391e96aea308de21c571ebf4d6e4225cf1f79","unresolved":true,"context_lines":[{"line_number":130,"context_line":"            \"generated application credentials.\""},{"line_number":131,"context_line":"        ),"},{"line_number":132,"context_line":"    ),"},{"line_number":133,"context_line":"    cfg.ListOpt("},{"line_number":134,"context_line":"        \"required_user_roles\","},{"line_number":135,"context_line":"        default\u003d[\"member\",\"load-balancer_member\"],"},{"line_number":136,"context_line":"        help\u003d("}],"source_content_type":"text/x-python","patch_set":4,"id":"092a87a2_c9368ed2","line":133,"in_reply_to":"d6fd73d8_e1f2ce39","updated":"2025-07-17 13:40:54.000000000","message":"I guess the default required roles should be whatever the majority of clouds use then, but I don\u0027t know how common is to override these roles so not sure what the safest default is.\n\nThe problem with failing cluster creation any later than this is that once the driver has done a helm install onto the management cluster, CAPI takes over and tries to create the LB so at that point we can\u0027t fail the creation from within Magnum. \n\nWe could try listing LBs during the cluster creation to make sure we have the right permissions, rather than assuming certain keystone policy config, if you think that is a better approach?","commit_id":"b4c71938c6461e6745cac7163791cad472c6b414"},{"author":{"_account_id":22629,"name":"Michal Nasiadka","email":"mnasiadka@gmail.com","username":"mnasiadka"},"change_message_id":"a83b9877186baf124eff8ad2dfdac5e7c22c3137","unresolved":true,"context_lines":[{"line_number":130,"context_line":"            \"generated application credentials.\""},{"line_number":131,"context_line":"        ),"},{"line_number":132,"context_line":"    ),"},{"line_number":133,"context_line":"    cfg.ListOpt("},{"line_number":134,"context_line":"        \"required_user_roles\","},{"line_number":135,"context_line":"        default\u003d[\"member\",\"load-balancer_member\"],"},{"line_number":136,"context_line":"        help\u003d("}],"source_content_type":"text/x-python","patch_set":4,"id":"d6fd73d8_e1f2ce39","line":133,"in_reply_to":"f3dfa276_75e0b024","updated":"2025-07-16 17:22:04.000000000","message":"Ah, these are keystone roles - so that\u0027s going to break the majority of clouds, which override Octavia policy and don\u0027t use load-balancer_member.\nWouldn\u0027t it be better to fail the cluster creation early when we observe that we can\u0027t create an LB?","commit_id":"b4c71938c6461e6745cac7163791cad472c6b414"}]}