Skip to content

OCPBUGS-34027: Fix cleanup of orphaned OIDC resources in guest cluster #7143

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
bryan-cox wants to merge 5 commits into from
Closed

OCPBUGS-34027: Fix cleanup of orphaned OIDC resources in guest cluster #7143

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1fe0ad9
fix(hcco): cleanup orphaned OIDC resources when config is removed
bryan-cox Oct 31, 2025
6003474
fix(hcco): prevent OIDC resource cleanup race condition
bryan-cox Nov 10, 2025
a448d9c
fix(globalconfig): clear Authentication spec when OIDC config is removed
bryan-cox Nov 17, 2025
68494f3
fix(OCPBUGS-34027): Explicitly set IntegratedOAuth type when removing…
bryan-cox Nov 20, 2025
2041250
test(globalconfig): add unit tests for Authentication reconciliation
bryan-cox Nov 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
104 changes: 103 additions & 1 deletion control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,11 @@ const (
ConfigNamespace = "openshift-config"
ConfigManagedNamespace = "openshift-config-managed"
CloudProviderCMName = "cloud-provider-config"
awsCredentialsTemplate = `[default]
// OIDCProviderManagedLabel is used to identify ConfigMaps and Secrets in the guest cluster
// that are managed by the OIDC provider reconciliation logic. Resources with this label
// will be subject to cleanup when they are no longer referenced in the HCP configuration.
OIDCProviderManagedLabel = "hypershift.openshift.io/oidc-provider-managed"
awsCredentialsTemplate = `[default]
role_arn = %s
web_identity_token_file = /var/run/secrets/openshift/serviceaccount/token
sts_regional_endpoints = regional
Expand Down Expand Up @@ -1238,13 +1242,20 @@ func (r *reconciler) reconcileIngressController(ctx context.Context, hcp *hyperv

func (r *reconciler) reconcileAuthOIDC(ctx context.Context, hcp *hyperv1.HostedControlPlane) error {
var errs []error

// Track expected resources that should exist based on current HCP configuration
expectedConfigMaps := sets.New[string]()
expectedSecrets := sets.New[string]()
Comment on lines +1246 to +1248

@coderabbitai coderabbitai Bot Nov 10, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 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:

  1. Upgrade to this version with existing OIDC resources (no labels yet)
  2. Immediately remove OIDC config from HCP spec before reconciliation runs
  3. Cleanup runs but can't find old resources (no labels)
  4. 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.go

Length 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:

  1. The resources won't acquire the label
  2. List() with client.HasLabels{OIDCProviderManagedLabel} won't find them
  3. 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.


if !util.HCPOAuthEnabled(hcp) &&
len(hcp.Spec.Configuration.Authentication.OIDCProviders) != 0 {

// Copy issuer CA configmap into openshift-config namespace
provider := hcp.Spec.Configuration.Authentication.OIDCProviders[0]
if provider.Issuer.CertificateAuthority.Name != "" {
name := provider.Issuer.CertificateAuthority.Name
expectedConfigMaps.Insert(name)

var src corev1.ConfigMap
err := r.cpClient.Get(ctx, client.ObjectKey{Namespace: hcp.Namespace, Name: name}, &src)
if err != nil {
Expand All @@ -1260,7 +1271,11 @@ func (r *reconciler) reconcileAuthOIDC(ctx context.Context, hcp *hyperv1.HostedC
if dest.Data == nil {
dest.Data = map[string]string{}
}
if dest.Labels == nil {
dest.Labels = map[string]string{}
}
dest.Data["ca-bundle.crt"] = src.Data["ca-bundle.crt"]
dest.Labels[OIDCProviderManagedLabel] = "true"
return nil
})
if err != nil {
Expand All @@ -1273,6 +1288,8 @@ func (r *reconciler) reconcileAuthOIDC(ctx context.Context, hcp *hyperv1.HostedC
if len(hcp.Spec.Configuration.Authentication.OIDCProviders[0].OIDCClients) > 0 {
for _, oidcClient := range hcp.Spec.Configuration.Authentication.OIDCProviders[0].OIDCClients {
if oidcClient.ClientSecret.Name != "" {
expectedSecrets.Insert(oidcClient.ClientSecret.Name)

var src corev1.Secret
err := r.cpClient.Get(ctx, client.ObjectKey{Namespace: hcp.Namespace, Name: oidcClient.ClientSecret.Name}, &src)
if err != nil {
Expand All @@ -1293,17 +1310,102 @@ func (r *reconciler) reconcileAuthOIDC(ctx context.Context, hcp *hyperv1.HostedC
if dest.Data == nil {
dest.Data = map[string][]byte{}
}
if dest.Labels == nil {
dest.Labels = map[string]string{}
}
dest.Data["clientSecret"] = src.Data["clientSecret"]
dest.Labels[OIDCProviderManagedLabel] = "true"
return nil
})
if err != nil {
errs = append(errs, fmt.Errorf("failed to reconcile OIDCClient secret %s: %w", dest.Name, err))
}
}
}
}
}

// Clean up orphaned OIDC resources that are no longer referenced in the HCP configuration
if err := r.cleanupOrphanedOIDCResources(ctx, expectedConfigMaps, expectedSecrets); err != nil {
errs = append(errs, err)
}

return utilerrors.NewAggregate(errs)
}

// cleanupOrphanedOIDCResources removes ConfigMaps and Secrets in the openshift-config namespace
// that were previously created for OIDC authentication but are no longer referenced in the
// HCP configuration. This prevents orphaned resources from accumulating when OIDC providers
// are removed or their configurations change.
//
// IMPORTANT: This function only deletes resources after verifying that the guest cluster's
// Authentication resource no longer references OIDC providers. This prevents a race condition
// where resources are deleted while components still expect them to exist.
func (r *reconciler) cleanupOrphanedOIDCResources(ctx context.Context, expectedConfigMaps, expectedSecrets sets.Set[string]) error {
log := ctrl.LoggerFrom(ctx)
var errs []error

// Check if the Authentication resource in the guest cluster still has OIDC clients configured.
// If it does, we should NOT delete the resources yet to avoid breaking the cluster.
// The Authentication status is managed by the authentication-operator and kube-apiserver,
// and we need to wait for them to finish processing the configuration change before cleanup.
auth := &configv1.Authentication{
ObjectMeta: metav1.ObjectMeta{
Name: "cluster",
},
}
if err := r.client.Get(ctx, client.ObjectKeyFromObject(auth), auth); err != nil {
if !apierrors.IsNotFound(err) {
log.Error(err, "failed to get authentication resource, skipping cleanup to be safe")
return fmt.Errorf("failed to get authentication resource: %w", err)
}
// Authentication resource doesn't exist, safe to clean up
} else if len(auth.Status.OIDCClients) > 0 {
// Authentication status still shows OIDC clients - the authentication-operator
// hasn't finished processing the removal yet. Skip cleanup to avoid breaking the cluster.
log.Info("skipping OIDC resource cleanup: authentication status still shows OIDC clients",
"oidcClientCount", len(auth.Status.OIDCClients))
return nil
}

// Safe to proceed with cleanup - Authentication resource either doesn't exist or has no OIDC clients

// Clean up orphaned ConfigMaps
configMapList := &corev1.ConfigMapList{}
if err := r.client.List(ctx, configMapList, client.InNamespace(ConfigNamespace), client.HasLabels{OIDCProviderManagedLabel}); err != nil {
errs = append(errs, fmt.Errorf("failed to list OIDC managed configmaps: %w", err))
} else {
for i := range configMapList.Items {
cm := &configMapList.Items[i]
if !expectedConfigMaps.Has(cm.Name) {
log.Info("deleting orphaned OIDC configmap", "name", cm.Name, "namespace", cm.Namespace)
if err := r.client.Delete(ctx, cm); err != nil {
if !apierrors.IsNotFound(err) {
errs = append(errs, fmt.Errorf("failed to delete orphaned OIDC configmap %s: %w", cm.Name, err))
}
}
}
}
}

// Clean up orphaned Secrets
secretList := &corev1.SecretList{}
if err := r.client.List(ctx, secretList, client.InNamespace(ConfigNamespace), client.HasLabels{OIDCProviderManagedLabel}); err != nil {
errs = append(errs, fmt.Errorf("failed to list OIDC managed secrets: %w", err))
} else {
for i := range secretList.Items {
secret := &secretList.Items[i]
if !expectedSecrets.Has(secret.Name) {
log.Info("deleting orphaned OIDC secret", "name", secret.Name, "namespace", secret.Namespace)
if err := r.client.Delete(ctx, secret); err != nil {
if !apierrors.IsNotFound(err) {
errs = append(errs, fmt.Errorf("failed to delete orphaned OIDC secret %s: %w", secret.Name, err))
}
}
}
}
}

return utilerrors.NewAggregate(errs)
}

Expand Down
Loading