OCPBUGS-86564: Use stable username hash for user-settings ConfigMap names#16512
OCPBUGS-86564: Use stable username hash for user-settings ConfigMap names#16512pedjak wants to merge 1 commit into
Conversation
openshift-ci-robot
added
jira/severity-moderate
|
@pedjak: This pull request references Jira Issue OCPBUGS-86564, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughUser settings ConfigMap resource names transition from UID-based to username-hash-based identification. A shared ChangesUser Settings Hash-based Naming
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pedjak The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/usersettings/handlers.go`:
- Around line 119-122: The AlreadyExists branch in the Create path (detected
with apierrors.IsAlreadyExists(err)) currently returns immediately after
logging, which prevents retrying cleanup of legacy UID-based resources when a
migration source is present; update the handler in handlers.go so that when
apierrors.IsAlreadyExists(err) and the migration source is detected you do not
early-return from the create flow but instead trigger/reenqueue the old-resource
cleanup logic (the same cleanup used elsewhere for migration), then perform the
ConfigMap fetch via
h.internalProxiedClient.CoreV1().ConfigMaps(namespace).Get(ctx,
userSettingMeta.getConfigMapName(), meta.GetOptions{}) to return the existing
data; apply the same change to the analogous block around lines 127-129 to
ensure cleanup retries run before returning existing resources.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 93878d20-787d-4d4d-8036-040b1a96227a
📒 Files selected for processing (9)
frontend/packages/console-shared/src/hooks/__tests__/useGetUserSettingConfigMap.spec.tsfrontend/packages/console-shared/src/hooks/__tests__/useUserPreference.spec.tsfrontend/packages/console-shared/src/hooks/useGetUserSettingConfigMap.tsfrontend/packages/console-shared/src/hooks/useUserPreference.tsfrontend/packages/console-shared/src/utils/user-settings.tspkg/usersettings/handlers.gopkg/usersettings/helpers.gopkg/usersettings/helpers_test.gopkg/usersettings/types.go
|
/jira refresh |
|
@pedjak: This pull request references Jira Issue OCPBUGS-86564, which is invalid:
Comment DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@pedjak: This pull request references Jira Issue OCPBUGS-86564, which is invalid:
Comment DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@pedjak: This pull request references Jira Issue OCPBUGS-86564, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Actionable comments posted: 0 |
|
@pedjak: This pull request references Jira Issue OCPBUGS-86564, which is valid. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/usersettings/helpers.go (1)
24-26: ⚡ Quick winUse
sha256.Sum256instead of ignoringWrite's return values.Line 25 drops the
Writeresults entirely. For a one-shot hash,sha256.Sum256is simpler and avoids the repo's Go error-handling violation.Suggested change
- sha256Hash := sha256.New() - sha256Hash.Write([]byte(name)) - resourceIdentifier = hex.EncodeToString(sha256Hash.Sum(nil)) + sum := sha256.Sum256([]byte(name)) + resourceIdentifier = hex.EncodeToString(sum[:])As per coding guidelines, "Never ignore error returns".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/usersettings/helpers.go` around lines 24 - 26, The code builds a SHA256 by calling sha256.New(), then sha256Hash.Write([]byte(name)) and ignores its return values; replace this one-shot hashing with sha256.Sum256 to avoid ignoring Write results: compute sum := sha256.Sum256([]byte(name)) and set resourceIdentifier = hex.EncodeToString(sum[:]); update the block that currently declares sha256Hash and calls Write/Sum to use sha256.Sum256 instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/usersettings/handlers.go`:
- Around line 95-97: The code currently returns early when the ConfigMap exists
(the call to h.internalProxiedClient.CoreV1().ConfigMaps(namespace).Get
returning nil error), which skips reconciling RBAC; change the flow in the
handler function that calls userSettingMeta.getConfigMapName() so that after
detecting an existing ConfigMap you still proceed to ensure the associated Role
and RoleBinding are created/updated (reconcile RBAC) before returning;
specifically, remove the early return and invoke the same Role/RoleBinding
reconciliation logic used for the POST path (or call the
reconcileRoles/reconcileRoleBinding helper if present) so RBAC is repaired even
when the ConfigMap already exists.
---
Nitpick comments:
In `@pkg/usersettings/helpers.go`:
- Around line 24-26: The code builds a SHA256 by calling sha256.New(), then
sha256Hash.Write([]byte(name)) and ignores its return values; replace this
one-shot hashing with sha256.Sum256 to avoid ignoring Write results: compute sum
:= sha256.Sum256([]byte(name)) and set resourceIdentifier =
hex.EncodeToString(sum[:]); update the block that currently declares sha256Hash
and calls Write/Sum to use sha256.Sum256 instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 67e66cc3-d342-4e2c-a8e3-dd47488fffa9
📒 Files selected for processing (10)
frontend/packages/console-shared/src/hooks/__tests__/useGetUserSettingConfigMap.spec.tsfrontend/packages/console-shared/src/hooks/__tests__/useUserPreference.spec.tsfrontend/packages/console-shared/src/hooks/useGetUserSettingConfigMap.tsfrontend/packages/console-shared/src/hooks/useUserPreference.tsfrontend/packages/console-shared/src/utils/user-settings.tspkg/usersettings/handlers.gopkg/usersettings/handlers_test.gopkg/usersettings/helpers.gopkg/usersettings/helpers_test.gopkg/usersettings/types.go
🚧 Files skipped from review as they are similar to previous changes (7)
- pkg/usersettings/types.go
- frontend/packages/console-shared/src/utils/user-settings.ts
- frontend/packages/console-shared/src/hooks/tests/useUserPreference.spec.ts
- frontend/packages/console-shared/src/hooks/useGetUserSettingConfigMap.ts
- frontend/packages/console-shared/src/hooks/useUserPreference.ts
- pkg/usersettings/helpers_test.go
- frontend/packages/console-shared/src/hooks/tests/useGetUserSettingConfigMap.spec.ts
| existing, err := h.internalProxiedClient.CoreV1().ConfigMaps(namespace).Get(ctx, userSettingMeta.getConfigMapName(), meta.GetOptions{}) | ||
| if err == nil { | ||
| return existing, nil |
There was a problem hiding this comment.
Don't short-circuit before RBAC is reconciled.
Returning at Line 97 skips Role/RoleBinding creation entirely. If the hashed ConfigMap already exists but either RBAC object was removed or never created, this POST path no longer repairs access and the user can be left with an unreadable/unwritable settings ConfigMap.
Suggested direction
- existing, err := h.internalProxiedClient.CoreV1().ConfigMaps(namespace).Get(ctx, userSettingMeta.getConfigMapName(), meta.GetOptions{})
- if err == nil {
- return existing, nil
- }
- if !apierrors.IsNotFound(err) {
+ existing, err := h.internalProxiedClient.CoreV1().ConfigMaps(namespace).Get(ctx, userSettingMeta.getConfigMapName(), meta.GetOptions{})
+ configMapExists := err == nil
+ if err != nil && !apierrors.IsNotFound(err) {
return nil, err
}
@@
- configMap := createConfigMap(userSettingMeta)
- if migrateFrom != nil {
+ configMap := createConfigMap(userSettingMeta)
+ if !configMapExists && migrateFrom != nil {
configMap.Data = migrateFrom.Data
}
@@
+ if configMapExists {
+ return existing, nil
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/usersettings/handlers.go` around lines 95 - 97, The code currently
returns early when the ConfigMap exists (the call to
h.internalProxiedClient.CoreV1().ConfigMaps(namespace).Get returning nil error),
which skips reconciling RBAC; change the flow in the handler function that calls
userSettingMeta.getConfigMapName() so that after detecting an existing ConfigMap
you still proceed to ensure the associated Role and RoleBinding are
created/updated (reconcile RBAC) before returning; specifically, remove the
early return and invoke the same Role/RoleBinding reconciliation logic used for
the POST path (or call the reconcileRoles/reconcileRoleBinding helper if
present) so RBAC is repaired even when the ConfigMap already exists.
There was a problem hiding this comment.
The Role and RoleBinding are always created before the ConfigMap in createUserSettings, so if the ConfigMap exists, RBAC was necessarily already set up in a prior call. The only way RBAC could be missing with the ConfigMap present is manual admin deletion — that's not a scenario this code path should self-heal from. Skipping this one.
There was a problem hiding this comment.
@pedjak, that's a fair point — the creation order (Role → RoleBinding → ConfigMap) provides a strong invariant: a successfully created ConfigMap implies RBAC was established in the same call. Treating manual admin deletion as a self-heal scenario would also risk re-instating access that an admin intentionally removed. Acknowledged, skipping this one.
pedjak
force-pushed
the
OCPBUGS-86564
branch
2 times, most recently
from
3003f2c to
91ca2db
Compare
|
/retest |
1 similar comment
|
/retest |
|
@pedjak: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
…ames The UID from SelfSubjectReview is an identity UID that can differ across identity providers for the same user, causing duplicate ConfigMaps. Use SHA256(username) instead, which is stable across IDPs. On first login post-upgrade, old ConfigMaps are discovered via OwnerReference (not by computing old names), the most recently created one is migrated, and all old resources are cleaned up. If the new ConfigMap already exists, the handler short-circuits without listing. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/packages/console-shared/src/hooks/useUserPreference.ts (1)
74-79: 💤 Low valueWiden
hashUsernameForSettings’nameparameter to matchUserInfo.username?: string.
usernamecan beundefined(UserInfo.username?: string), buthashUsernameForSettingscurrently typesname: stringeven though it already guardsif (!name) return nullat runtime—so widen the signature toname: string | undefined(orname?: string) for correct type alignment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/packages/console-shared/src/hooks/useUserPreference.ts` around lines 74 - 79, The function signature for hashUsernameForSettings is too narrow: it currently declares name: string but callers like useUserPreference (in the selector using getUser()?.username) can pass undefined; widen the parameter to accept undefined (e.g., name?: string or name: string | undefined) and keep the existing runtime guard (if (!name) return null) intact; update the function declaration for hashUsernameForSettings (and any exported type references) so TypeScript matches the runtime behavior and the call site in useUserPreference no longer requires a cast.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@frontend/packages/console-shared/src/hooks/useUserPreference.ts`:
- Around line 74-79: The function signature for hashUsernameForSettings is too
narrow: it currently declares name: string but callers like useUserPreference
(in the selector using getUser()?.username) can pass undefined; widen the
parameter to accept undefined (e.g., name?: string or name: string | undefined)
and keep the existing runtime guard (if (!name) return null) intact; update the
function declaration for hashUsernameForSettings (and any exported type
references) so TypeScript matches the runtime behavior and the call site in
useUserPreference no longer requires a cast.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 16ee83a5-a7dc-4862-add5-9a237a13a4cd
📒 Files selected for processing (10)
frontend/packages/console-shared/src/hooks/__tests__/useGetUserSettingConfigMap.spec.tsfrontend/packages/console-shared/src/hooks/__tests__/useUserPreference.spec.tsfrontend/packages/console-shared/src/hooks/useGetUserSettingConfigMap.tsfrontend/packages/console-shared/src/hooks/useUserPreference.tsfrontend/packages/console-shared/src/utils/user-settings.tspkg/usersettings/handlers.gopkg/usersettings/handlers_test.gopkg/usersettings/helpers.gopkg/usersettings/helpers_test.gopkg/usersettings/types.go
✅ Files skipped from review due to trivial changes (1)
- frontend/packages/console-shared/src/hooks/tests/useUserPreference.spec.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/usersettings/helpers_test.go
- pkg/usersettings/helpers.go
- frontend/packages/console-shared/src/hooks/useGetUserSettingConfigMap.ts
- pkg/usersettings/handlers_test.go
- frontend/packages/console-shared/src/hooks/tests/useGetUserSettingConfigMap.spec.ts
- pkg/usersettings/handlers.go
2 participants
Analysis / Root cause:
The
SelfSubjectReviewAPI returns the identity UID, not the stable Userresource UID. With
mappingMethod: addand multiple identity providers(e.g. LDAP + OpenID), the same user receives different identity UIDs
across logins, causing a new
user-settings-*ConfigMap (plus Role andRoleBinding) to be created each time. A customer observed ~21,594
ConfigMaps for ~3,869 users.
Introduced in commit a186aa1 which migrated from the OCP User API
(
Users().Get("~")) toSelfSubjectReview.https://redhat.atlassian.net/browse/OCPBUGS-86564
Solution description:
pkg/usersettings/): ChangeResourceIdentifierfromUIDtoSHA256(username). The username is stable across identityproviders for the same OpenShift user. UID is still used in
OwnerReferences for garbage collection.
not yet exist, the backend lists all ConfigMaps in the namespace and
finds old ones via OwnerReference (matching
Kind: Userand theusername). If multiple old ConfigMaps exist (e.g. from different
identity provider UIDs), data is migrated from the most recently
created one. All old ConfigMaps and their associated Roles and
RoleBindings are cleaned up. If the new ConfigMap already exists,
the handler short-circuits without listing.
Best-effort — errors are logged but never block creation.
console-shared/src/hooks/): Removeuidfrom theConfigMap name priority chain so the frontend watches the same
hash-based name the backend creates. Extract duplicated
hashNameOrKubeadmininto a sharedhashUsernameForSettingsutility.Screenshots / screen recording:
Test setup:
Cluster with multiple identity providers configured with
mappingMethod: add(e.g. LDAP + OpenID).Test cases:
user-settings-{SHA256(username)}verify old settings are preserved in the new ConfigMap and old
resources are cleaned up
the most recent one is migrated and all old ones are removed
kube:admin(no UID) still usesuser-settings-kubeadmingo test ./pkg/usersettings/...cd frontend && yarn test packages/console-shared/src/hooks/__tests__/useUserPreference.spec.ts packages/console-shared/src/hooks/__tests__/useGetUserSettingConfigMap.spec.tsBrowser conformance:
Summary by CodeRabbit
New Features
Bug Fixes
Tests