OCPBUGS-34027: Fix cleanup of orphaned OIDC resources in guest cluster - #7143
OCPBUGS-34027: Fix cleanup of orphaned OIDC resources in guest cluster#7143bryan-cox wants to merge 5 commits into
Conversation
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/severity-critical
openshift-ci-robot
added
the
jira/valid-reference
|
@bryan-cox: This pull request references Jira Issue OCPBUGS-34027, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (yingzhan@redhat.com), skipping review request. 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. |
openshift-ci-robot
added
the
jira/valid-bug
openshift-ci
Bot
added
area/control-plane-operator
WalkthroughAdds OIDC resource tracking and marking during reconciliation, implements a race-aware cleanup for orphaned OIDC ConfigMaps and Secrets in Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
/test all |
6acbfbb to
226be29
Compare
|
/test all |
|
/test e2e-aws |
|
/test e2e-aws-upgrade-hypershift-operator |
|
/auto-cc |
226be29 to
042214a
Compare
|
/lgtm |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go (1)
1242-1327: OIDC resource tracking and cleanup integration looks good.The changes properly track expected resources during reconciliation and invoke cleanup for orphaned resources. Key strengths:
- Defensive nil-checks before setting labels (lines 1270-1272, 1309-1311)
- ARO HCP day-2 secrets correctly excluded via annotation check (lines 1295-1298)
- Expected resources only tracked after successful reconciliation
Consider deferring cleanup if reconciliation errors occurred, to avoid deleting resources when the control plane is temporarily unreachable:
+ // Only cleanup if reconciliation succeeded + if len(errs) == 0 { if err := r.cleanupOrphanedOIDCResources(ctx, expectedConfigMaps, expectedSecrets); err != nil { errs = append(errs, err) } + }This would prevent transient control plane access issues from triggering resource deletion, though the current behavior is self-healing and consistent with the codebase pattern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go(5 hunks)control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Red Hat Konflux / hypershift-operator-main-on-pull-request
- GitHub Check: Red Hat Konflux / control-plane-operator-main-on-pull-request
🔇 Additional comments (3)
control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go (1)
2187-2433: Comprehensive test coverage for OIDC resource cleanup.The test covers all critical scenarios:
- Full removal when OIDC provider is deleted
- Partial removal when CA reference is removed
- Partial removal when individual OIDC clients are removed
- Preservation of resources without the managed label
- No deletions when resources remain referenced
The test structure is clear and the verification logic properly checks both deletion (via
IsNotFound) and preservation of resources.control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go (2)
97-100: Well-documented managed label constant.The constant provides a clear way to identify OIDC resources that are managed by the reconciliation logic, enabling safe cleanup of orphaned resources.
1332-1377: Clean and well-documented cleanup implementation.The function efficiently identifies and removes orphaned OIDC resources using label-based queries. The implementation correctly:
- Uses label selectors to find only managed resources
- Preserves resources in the expected set
- Handles
NotFounderrors gracefully (lines 1350, 1368)- Aggregates errors for proper reporting
- Includes clear documentation of the cleanup behavior
|
@bryan-cox: This pull request references Jira Issue OCPBUGS-34027, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (xiuwang+1@redhat.com), skipping review request. 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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go(5 hunks)control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go
🔇 Additional comments (3)
control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go (3)
97-100: LGTM: Clear documentation for the managed label.The constant is well-documented and its purpose is clear.
1348-1365: Excellent race condition safeguard.The check for
auth.Status.OIDCClientsbefore proceeding with cleanup prevents deleting resources while the authentication-operator or kube-apiserver is still processing configuration changes. This is a critical safety measure.
1270-1272: LGTM: Proper nil-safety for Labels initialization.Correctly initializes the Labels map before setting the managed label, preventing potential nil pointer dereference.
Also applies to: 1309-1311
| // Track expected resources that should exist based on current HCP configuration | ||
| expectedConfigMaps := sets.New[string]() | ||
| expectedSecrets := sets.New[string]() |
There was a problem hiding this comment.
🧩 Analysis chain
Verify backward compatibility for unlabeled resources.
The tracking and labeling logic correctly marks resources during reconciliation. However, the PR description states: "resources created before this change may lack the managed label but can be cleaned up via name matching."
The current implementation only cleans up resources with the OIDCProviderManagedLabel (see lines 1371-1372, 1389-1390 using client.HasLabels{OIDCProviderManagedLabel}). Resources created before this change won't be found by the label selector unless they're reconciled first.
Scenario to consider:
- Upgrade to this version with existing OIDC resources (no labels yet)
- Immediately remove OIDC config from HCP spec before reconciliation runs
- Cleanup runs but can't find old resources (no labels)
- Resources are orphaned
Run this script to check if there's any name-based cleanup logic for backward compatibility:
Also applies to: 1253-1253, 1270-1274, 1287-1287, 1309-1313, 1325-1327
🏁 Script executed:
#!/bin/bash
# Search for any name-based cleanup or migration logic for unlabeled OIDC resources
echo "Checking for name-based cleanup or label migration logic..."
# Look for any code that handles resources without the label
rg -nC3 "OIDCProviderManagedLabel|oidc-provider-managed" --type=go
# Check if there's any migration or backfill logic
rg -nC3 "backward.{0,20}compat|migration|backfill" --type=go -g '**/resources.go'
# Look for cleanup logic that doesn't use label selectors
ast-grep --pattern $'func $_(ctx context.Context, $$$) $_ {
$$$
client.List($$$)
$$$
ConfigNamespace
$$$
}'Length of output: 19279
The web search wasn't helpful for this specific code context. I need to look at the codebase directly to understand how the expectedConfigMaps and expectedSecrets sets are used.
Let me search properly with actual file access:
Actually, let me get the correct line ranges:
---
🏁 Script executed:
```shell
#!/bin/bash
# Look at the full reconcileAuthOIDC function to see how expectedConfigMaps and expectedSecrets are used
rg -nA100 "func \(r \*HostedClusterConfigReconciler\) reconcileAuthOIDC" control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go | head -150
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look at the full reconcileAuthOIDC function to see how expectedConfigMaps and expectedSecrets are used
rg -nA150 "reconcileAuthOIDC.*ctx.*HostedCluster" control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go | grep -A150 "func.*reconcileAuthOIDC"Length of output: 0
🏁 Script executed:
#!/bin/bash
# First, find where reconcileAuthOIDC function is defined
echo "=== Finding reconcileAuthOIDC function ==="
rg -n "func.*reconcileAuthOIDC" control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go
# Then get the function body with more context to see how expectedConfigMaps is used
echo -e "\n=== Looking for how expectedConfigMaps/expectedSecrets are used ==="
rg -n "expectedConfigMaps\|expectedSecrets" control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go -A2 -B2
# Look for the cleanup function that uses these sets
echo -e "\n=== Looking for cleanupOrphanedOIDCResources function ==="
rg -nA50 "cleanupOrphanedOIDCResources" control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.goLength of output: 3862
---
🏁 Script executed:
```shell
#!/bin/bash
# Check if expectedConfigMaps is used anywhere else besides the comparison in cleanupOrphanedOIDCResources
echo "=== All uses of expectedConfigMaps variable ==="
rg -n "expectedConfigMaps" control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go
# Check if there's any other cleanup mechanism or name-based logic
echo -e "\n=== Check for additional cleanup or fallback logic ==="
sed -n '1340,1410p' control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go
Length of output: 3622
Backward compatibility gap confirmed: cleanup only finds labeled resources.
The implementation only cleans up resources that have the OIDCProviderManagedLabel label (lines 1371, 1389). Resources created before this change won't have the label and won't be found by the client.HasLabels filters, leaving them orphaned.
The cleanup logic never performs name-based matching as mentioned in the PR description. If OIDC config is removed before old resources are reconciled and labeled:
- The resources won't acquire the label
List()withclient.HasLabels{OIDCProviderManagedLabel}won't find them- Cleanup won't delete them
Consider adding either a fallback name-based lookup or a migration mechanism to label existing resources on first reconciliation.
🤖 Prompt for AI Agents
In
control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go
around lines 1242 to 1244, the cleanup only lists resources by
OIDCProviderManagedLabel so pre-existing resources without that label are
orphaned; add a fallback name-based lookup or a one-time migration to label
legacy resources: when listing for cleanup, if the label-filtered List returns
empty or before deletion also attempt to Get/List by the expected resource names
(or namespace+name) and include those in the deletion set, or implement a
migration pass during reconciliation that finds known resource names and applies
the OIDCProviderManagedLabel so subsequent reconciles use the label-only path.
Ensure the fallback path respects existing owner references and errors, and
avoid double-deleting by deduplicating resources between label and name-based
matches.
When external authentication (OIDC providers) is configured on a HyperShift cluster, ConfigMaps and Secrets are created in the guest cluster's openshift-config namespace for CA certificates and client secrets. However, when the OIDC configuration is removed from the HostedControlPlane spec, these resources were not being deleted, leaving orphaned resources in the guest cluster. This commit adds cleanup logic to the reconcileAuthOIDC function to: - Track expected ConfigMaps and Secrets based on current HCP configuration - Add a managed label to OIDC resources for identification - Delete orphaned resources that are no longer referenced The cleanup handles several scenarios: - Complete removal of OIDC provider configuration - Removal of CA certificate reference from provider - Removal of individual OIDCClients from a provider Resources without the managed label are not affected, ensuring backwards compatibility and avoiding accidental deletion of user-created resources. Fixes: OCPBUGS-34027 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
During testing, it was discovered that the initial implementation caused a race condition where ConfigMaps and Secrets were deleted immediately when OIDC configuration was removed from the HostedControlPlane spec, but the guest cluster's Authentication resource status had not yet been updated by the authentication-operator. This caused components (console, CLI) to look for the deleted resources and report errors like: - "secret 'authid-console-openshift-console' not found" - "configmap 'keycloak-oidc-ca' not found" This commit adds a safety check to the cleanup logic: - Before deleting any resources, verify that the Authentication resource no longer has OIDC clients in its status - If OIDC clients are still present in the status, skip cleanup and let it happen on the next reconciliation loop - This ensures resources are only deleted after the authentication-operator has finished processing the configuration change The cleanup will now happen in a subsequent reconciliation after the Authentication status has been updated, preventing the cluster from entering a degraded state during the transition period. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
6bbef66 to
6003474
Compare
When OIDC configuration is removed from the HostedCluster (by setting spec.configuration to null or removing spec.configuration.authentication), the Authentication resource in the guest cluster was not being updated to remove the OIDC provider configuration. This left orphaned OIDC configuration in the Authentication spec, preventing the authentication-operator from clearing the OIDC clients from the status. This commit updates ReconcileAuthenticationConfiguration to explicitly clear the Authentication spec when configuration is removed, ensuring: 1. The Authentication resource is reset to default state (empty spec) 2. The ServiceAccountIssuer is preserved and set from the cluster's issuerURL 3. The authentication-operator can update the status to remove OIDC clients 4. The OIDC resource cleanup logic can proceed to delete orphaned resources Also fixes test errors by using the correct apierrors.IsNotFound function instead of the standard library errors package. This completes the fix for OCPBUGS-34027 by ensuring the full cleanup flow works correctly when OIDC configuration is removed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
openshift-ci
Bot
added
the
area/hypershift-operator
|
/test e2e-aws |
… OIDC When OIDC provider configuration is removed from a HostedControlPlane, the Authentication resource must explicitly fallback to IntegratedOAuth mode. Without this, the authentication-operator doesn't clear the Authentication.Status.OIDCClients field, preventing cleanup of orphaned OIDC resources. This fix ensures proper cleanup by: 1. Setting Type: IntegratedOAuth when authentication config is removed 2. Allowing the authentication-operator to detect the mode change 3. Enabling it to clear the OIDCClients status properly 4. Finally triggering the cleanup logic in hosted cluster config operator 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Update: Fixed Authentication Type IssueThe tester confirmed that secrets and configmaps were still not being cleaned up even with the race condition fix. The root cause was discovered: Problem: When OIDC configuration is removed from the HCP, we were clearing Solution: Now explicitly setting
What Changed: // Before:
authentication.Spec = configv1.AuthenticationSpec{}
// After:
authentication.Spec = configv1.AuthenticationSpec{
Type: configv1.AuthenticationTypeIntegratedOAuth,
}This ensures the cluster properly falls back to OAuth when OIDC is removed, which is the expected day-2 behavior. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
support/globalconfig/authentication_test.go (1)
11-104: Tests comprehensively cover authentication reconciliation edge casesThe table-driven tests exercise the key transitions (OIDC → none, config present but Authentication nil, OIDC explicitly provided, issuer changes) and assert the critical invariants: explicit
Type, clearedOIDCProviders, and correctServiceAccountIssuer. There are no correctness or maintainability red flags here.If
ReconcileAuthenticationConfigurationis expected to handle anilexistingAuth, consider adding a small additional case for that scenario to lock in the behavior; otherwise this looks solid as-is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
support/globalconfig/authentication_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
support/globalconfig/authentication_test.go
d9d70ee to
bf8e00d
Compare
These tests verify that ReconcileAuthenticationConfiguration correctly sets the Authentication Type field to IntegratedOAuth when OIDC config is removed. This catches the bug where Type was being set to empty string instead of an explicit IntegratedOAuth value. Key test cases: - Transition from OIDC to no OIDC (the actual bug scenario) - Type field must not be empty string - ServiceAccountIssuer is always preserved These tests fail with the old broken code and pass with the fix. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
bf8e00d to
2041250
Compare
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Moved back to draft since this is having issues in pre-merge testing. |
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. 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. |
|
@bryan-cox: The following tests 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. |
|
Stale PRs are closed after 21d of inactivity. If this PR is still relevant, comment to refresh it or remove the stale label. If this PR is safe to close now please do so with /lifecycle stale |
openshift-ci
Bot
added
the
lifecycle/stale
|
Stale PRs rot after 14d of inactivity. Mark the PR as fresh by commenting If this PR is safe to close now please do so with /lifecycle rotten |
openshift-ci
Bot
added
lifecycle/rotten
|
/close Not actively maintaining this PR |
|
@bryan-cox: Closed this PR. 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 kubernetes-sigs/prow repository. |
|
@bryan-cox: This pull request references Jira Issue OCPBUGS-34027. The bug has been updated to no longer refer to the pull request using the external bug tracker. All external bug links have been closed. The bug has been moved to the NEW state. 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. |
|
Now I have everything. Let me produce the final consolidated report. Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryAll three failures stem from the same root cause: the PR branch is stale and has unresolved merge conflicts with Root CauseAll three job failures share a single root cause: git merge conflict due to a stale PR branch.
The conflicting file ( Recommendations
Evidence
|
5 participants
What this PR does / why we need it:
This PR fixes an issue where ConfigMaps and Secrets created for external authentication (OIDC providers) in the guest cluster's
openshift-confignamespace were not being deleted when the OIDC configuration was removed from the HostedControlPlane spec.UPDATE after testing: The initial implementation caused a race condition where resources were deleted before the guest cluster's Authentication status was updated, causing components to report errors. This has been fixed with an additional safety check.
The fix:
Cleanup handles these scenarios:
Which issue(s) this PR fixes:
Fixes OCPBUGS-34027
Special notes for your reviewer:
Race Condition Fix (Critical)
The initial implementation deleted resources immediately when OIDC config was removed, but the Authentication operator hadn't finished updating the guest cluster's Authentication status yet. This caused:
secret 'authid-console-openshift-console' not foundconfigmap 'keycloak-oidc-ca' not foundSolution: Added safety check that prevents deletion until
Authentication.status.oidcClientsis empty. Resources are cleaned up on the next reconciliation loop after the Authentication operator finishes processing the change.Other Considerations
hypershift.openshift.io/hosted-cluster-sourced: "true"are preservedChecklist:
🤖 Generated with Claude Code via
/jira:solve OCPBUGS-34027 origin