feat(rbac): namespaced webhook auth and a migratable SA-UID identity - #531
feat(rbac): namespaced webhook auth and a migratable SA-UID identity#531therealdwright wants to merge 1 commit into
Conversation
|
|
jaypipes
left a comment
jaypipes
left a comment
There was a problem hiding this comment.
This is a backwards-incompatible change where the temporal-worker-controller's manager identity will change and that will unfortunately result in deadlocks where existing Temporal Worker Deployments will not be able to be managed by temporal-worker-controller until a manual temporal worker deployment set-manager-identity CLI call is made.
In addition, removing namespaces entirely from the list of resources the ClusterRole needs and changing from SubjectAccessReview to LocalSubjectAccessReview for both ClusterRole and Role will likely break all existing installations, cluster-scoped or otherwise. We would certainly need a functional test that demonstrated such a migration path would be doable with helm upgrade...
| setupLog.Error(err, "unable to fetch service account UID for controller identity suffix") | ||
| os.Exit(1) | ||
| } | ||
| if err := os.Setenv(controller.IdentitySuffixEnvKey, string(sa.UID)); err != nil { |
There was a problem hiding this comment.
Won't this break all existing installations of temporal-worker-controller in a backwards-incompatible way? The identity suffix env key will change from the namespace's UID to the service account's UID and then Worker Deployment's manager identity will change which result in all the Temporal Worker Deployments managed by the temporal-worker-controller getting into a manager-identity deadlock.
There was a problem hiding this comment.
Thanks @jaypipes , good pickup, I didn't consider the deadlock. First-time contributor here so apologies if I've approached this incorrectly.
I added an integration test that reproduces the behaviour you have describedr: once the suffix changes the controller won't promote a new version, since the deployment is still owned by the old identity. It fails on the current PR.
For the transition I tried to follow the existing getDeprecatedControllerIdentity reclaim path from when the ns-UID suffix landed (#308). The controller now also reads the namespace UID and treats the old id/<ns-uid> identity as reclaimable, so helm upgrade re-adopts existing deployments under the new id/<sa-uid> identity on its own, no manual set-manager-identity. Test passes with that in. I left the namespaces get grant in for the migration window, figured it can come out a release or two later once everything's re-adopted.
Does that match how you'd want the transition handled, and how it's been done before? Happy to redo it if there's a better way.
There was a problem hiding this comment.
On SubjectAccessReview > LocalSubjectAccessReview: I don't think it's a permanent break, but there's an transient upgrade issue. The WRT webhook is failurePolicy: Fail, so during the rolling restart the old pod loses subjectaccessreviews create once the new RBAC is applied and WRT admissions get blocked for that window. I believe it clears once the new pod is ready. If that matters I can keep both grants for one release (same migration-window idea as the identity change) and drop the cluster one in a follow-up or I can leave it if a brief gap is fine. Do you have a preference?
There was a problem hiding this comment.
Ran the helm upgrade migration on kind (released v1.8.1 then upgraded to the code/manifests in this PR):
- Pre-upgrade: WorkerDeployment
ManagerIdentity = <release>/<namespace-UID>. - After upgrade + a worker image bump: controller logged "claimed manager identity" under the new
<release>/<SA-UID>,ManagerIdentityupdated to match, and the new version promoted to current. No manualset-manager-identity, no deadlock. - RBAC (namespace-scoped):
manager-cluster-rolenow onlynamespaces get; new namespacedidentity-reader-role;localsubjectaccessreviewsin the per-namespace Role; webhook accepted a live WorkerDeployment patch.
Shrinks the cluster-scoped RBAC the controller needs in namespace-scoped mode and lays groundwork to remove the manager ClusterRole entirely in a later release, without the manager-identity deadlock raised in review. - The WorkerResourceTemplate webhook authorizes with LocalSubjectAccessReview instead of SubjectAccessReview. It is namespaced and sufficient (the webhook rejects non-namespaced kinds and always checks within the object's namespace), so the subjectaccessreviews "create" grant moves out of cluster scope into the per-namespace Role. - The controller identity suffix is now the ServiceAccount UID, read via a small namespaced identity-reader Role, instead of the Namespace UID. To avoid deadlocking Worker Deployments still claimed under the previous namespace-UID identity, the controller also reads the namespace UID and treats that legacy identity as reclaimable (mirroring the existing deprecated-identity reclaim path), adopting such deployments under the new identity on helm upgrade. The namespaces "get" grant and its minimal manager-cluster-role are retained for this migration window and can be dropped in a later release. Adds an integration test that reproduces the upgrade against a real Temporal server: a deployment claimed under the pre-upgrade identity is re-claimed and its new version promoted after the suffix changes. Signed-off-by: Daniel Wright <danielwright@bitgo.com>
48cb233 to
df76094
Compare
therealdwright
changed the title
3 participants
What was changed
Two changes toward reducing the cluster-scoped RBAC the controller needs in
namespace-scoped mode (
rbac.restrictWatchNamespaces), without breaking existing installs onupgrade:
SubjectAccessReview→LocalSubjectAccessReviewin the WorkerResourceTemplatewebhook. It is namespaced and sufficient (the webhook rejects non-namespaced kinds and
always checks within the object's namespace), so
subjectaccessreviews createmoves outof the cluster-scoped grant into the per-namespace Role.
Identity suffix from the ServiceAccount UID instead of the Namespace UID, read via a
small namespaced
identity-reader-role. A plain suffix change would deadlock existingWorker Deployments (they stay managed under the old identity — thanks @jaypipes), so the
controller also reads the namespace UID and treats the old
id/<ns-uid>identity asreclaimable, the same pattern as the existing
getDeprecatedControllerIdentityreclaimfrom Prepare to include cluster UID in CONTROLLER_IDENTITY to prevent cross-cluster conflicts #308. On
helm upgradeit re-adopts existing deployments under the newid/<sa-uid>identity automatically.
The
namespaces getgrant (a minimalmanager-cluster-role) is retained for this migrationwindow; it can be dropped in a follow-up release once deployments have been re-adopted, at
which point namespace-scoped mode no longer needs a manager ClusterRole.
Why?
Clusters whose policies forbid ClusterRole creation (common in the multi-tenant environments
namespace-scoped mode targets) currently can't install the chart. Both grants are ClusterRoles
only because their resource types are cluster-scoped, not because the controller needs
cluster-wide reach. This removes one now and sets up removing the other safely.
Checklist
Closes [Feature Request] Eliminate ClusterRole requirement in namespace-scoped mode #530
How was this tested:
TestManagerIdentitySuffixMigration) reproduces the upgradeagainst a real Temporal server (
temporaltest): it fails (deadlock) without the reclaimpath and passes with it (re-claims and promotes the new version). The full
TestIntegrationsuite passes with the change (no regression to the existingmanager-identity tests).
make manifestsis idempotent (CI rbac.yaml gate passes).helm template(namespace-scoped): the manager is a namespaced Role;manager-cluster-rolenow holds only
namespaces get; a new namespacedidentity-reader-roleis added; nosubjectaccessreviewscluster grant.Any docs updates needed?
Probably a short note in the namespace-scoped/RBAC docs about the reduced ClusterRole and
the planned follow-up removal. Happy to add.