rbac_migrator: suppress error on role_exists #17800
Conversation
See the comment in the code change for why this is safe.
| @@ -42,7 +42,17 @@ ss::future<> rbac_migrator::do_mutate() { | |||
| auto err = co_await _controller.get_security_frontend().local().create_role( | |||
| role_name, std::move(role), model::timeout_clock::now() + 5s); | |||
|
|
|||
| if (err) { | |||
| if (err == cluster::errc::role_exists) { | |||
There was a problem hiding this comment.
Is it possible that in-between more users were created? Are migrations guaranteed to run before any other operations on the cluster can happen?
We should probably document explicitly (if not done already) that migrations must be idempotent.
There was a problem hiding this comment.
Is it possible that in-between more users were created?
Yes, and that's fine. In that case, the migration will appear to have happened at the original time (before the users were added). This role is created as a one-time convenience to showcase roles and it won't be kept up-to-date with users added after the migration.
Are migrations guaranteed to run before any other operations on the cluster can happen?
The logical version of the cluster is only incremented after the migration and the RBAC feature is only activated after the migration, so other RBAC operations guarded by the RBAC feature flag won't happen. But otherwise, operations aren't blocked by the migration as far as I can tell.
We should probably document explicitly (if not done already) that migrations must be idempotent.
I now noticed that there was already a hint about migrations executing at least once (here), but I've now added one more comment to state that do_mutate should be idempotent: 33d857f
There was a problem hiding this comment.
and it won't be kept up-to-date with users added after the migration.
wondering why are we bothering to do it at all then :) wouldn't it just confuse users more?
|
ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/47670#018ecded-bbf6-4d7e-a92e-59d8e65e6e08 ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/47670#018ece15-f6e5-49c4-a3e1-75b49f22d10d |
|
new failures in https://buildkite.com/redpanda/redpanda/builds/47670#018ecddc-1925-4895-b3e9-21c5d85ad0f5: |
If the leader running the feature migration loses leadership after
the role is created but before the feature migration is successfully
completed, the next leader will redo the feature migration. In that
case, we will get a
role_existserror, which we can safely ignore.This was discovered through the following CI failure: https://ci-artifacts.dev.vectorized.cloud/redpanda/47611/018ec8a8-1fa5-49b2-8d82-1577c14d20bf/vbuild/ducktape/results/final/report.html
Fixes https://redpandadata.atlassian.net/browse/CORE-2287
Fixes #17736
Backports Required
Release Notes