OCPBUGS-66251: OADP restore results in htpasswd no longer being applied as volume#7628
OCPBUGS-66251: OADP restore results in htpasswd no longer being applied as volume#7628mehabhalodiya wants to merge 1 commit into
Conversation
openshift-ci-robot
added
jira/severity-important
|
@mehabhalodiya: This pull request references Jira Issue OCPBUGS-66251, 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. |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request introduces significant infrastructure and API enhancements to the HyperShift project. Major changes include: new etcd backup and Azure Private Link Service CRD types; expanded platform support for Azure topology/private configurations, GCP workload identity, and AWS spot instances; enhanced validation rules across multiple CRD manifests; updated Tekton pipeline definitions with improved task bundling; extended Makefile with new test targets (envtest, backup-restore); new GitHub Actions workflows for CI/CD (lint, verify, test, docs, envtest); base container image updates; and comprehensive documentation additions including Claude skills and command guides. API schemas are tightened with stricter immutability constraints, cross-field validations, and required field changes. ✨ Finishing Touches🧪 Generate unit tests (beta)
|
openshift-ci
Bot
added
area/control-plane-operator
mehabhalodiya
changed the title
![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
🤖 Fix all issues with AI agents
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/oauth/deployment.go`:
- Around line 95-118: When identityProviders > 0, removeIDPVolumeMounts must run
regardless of whether ConvertIdentityProviders returned volumes to avoid leaving
stale mounts; change the block handling ConvertIdentityProviders so that after
calling removeIDPVolumes(deployment) and calling ConvertIdentityProviders(...)
you always call removeIDPVolumeMounts(deployment) before inspecting
volumeMountInfo.Volumes, then only append volumeMountInfo.Volumes and update
container mounts via util.UpdateContainer(ComponentName, ...) if
len(volumeMountInfo.Volumes) > 0. Ensure the same functions (removeIDPVolumes,
removeIDPVolumeMounts, ConvertIdentityProviders, util.UpdateContainer,
ComponentName, deployment) are used and no other behavior changes.
| identityProviders := configuration.OAuth.IdentityProviders | ||
| if len(identityProviders) > 0 { | ||
| // Remove any existing IDP volumes to ensure clean state after restore | ||
| // This is important for OADP restore scenarios where the deployment | ||
| // might be restored with stale volume configurations | ||
| removeIDPVolumes(deployment) | ||
|
|
||
| _, volumeMountInfo, _ := ConvertIdentityProviders(cpContext, identityProviders, providerOverrides(cpContext.HCP), cpContext.Client, deployment.Namespace) | ||
| // Ignore the error here, since we don't want to fail the deployment if the identity providers are invalid | ||
| // A condition will be set on the HC to indicate the error | ||
| if len(volumeMountInfo.Volumes) > 0 { | ||
| // Remove any existing IDP volume mounts before adding new ones | ||
| removeIDPVolumeMounts(deployment) | ||
|
|
||
| deployment.Spec.Template.Spec.Volumes = append(deployment.Spec.Template.Spec.Volumes, volumeMountInfo.Volumes...) | ||
| util.UpdateContainer(ComponentName, deployment.Spec.Template.Spec.Containers, func(c *corev1.Container) { | ||
| c.VolumeMounts = append(c.VolumeMounts, volumeMountInfo.VolumeMounts.ContainerMounts(ComponentName)...) | ||
| }) | ||
| } | ||
| } else { | ||
| // If no identity providers are configured, ensure any stale IDP volumes are removed | ||
| removeIDPVolumes(deployment) | ||
| removeIDPVolumeMounts(deployment) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the ConvertIdentityProviders function
fd -t f -e go -path "*oauth*" control-plane-operator/controllers/hostedcontrolplane/v2 | head -20Repository: openshift/hypershift
Length of output: 232
🏁 Script executed:
#!/bin/bash
# Search for ConvertIdentityProviders implementation
rg "func ConvertIdentityProviders" -A 50 --type goRepository: openshift/hypershift
Length of output: 11465
🏁 Script executed:
#!/bin/bash
# Also search for the removeIDPVolumes and removeIDPVolumeMounts functions
rg "func (.*)?removeIDPVolumes|func (.*)?removeIDPVolumeMounts" -A 15 --type goRepository: openshift/hypershift
Length of output: 3071
Fix dangling IDP mounts when volume conversion fails.
When ConvertIdentityProviders produces zero volumes (e.g., all providers fail conversion), removeIDPVolumes runs unconditionally, but removeIDPVolumeMounts is skipped because volumeMountInfo.Volumes is empty. This leaves stale mounts referencing volumes that no longer exist, creating an inconsistent pod spec. Move removeIDPVolumeMounts outside the conditional to always clean up mounts alongside volumes.
✅ Suggested adjustment
- removeIDPVolumes(deployment)
-
- _, volumeMountInfo, _ := ConvertIdentityProviders(cpContext, identityProviders, providerOverrides(cpContext.HCP), cpContext.Client, deployment.Namespace)
+ removeIDPVolumes(deployment)
+ removeIDPVolumeMounts(deployment)
+
+ _, volumeMountInfo, _ := ConvertIdentityProviders(cpContext, identityProviders, providerOverrides(cpContext.HCP), cpContext.Client, deployment.Namespace)
// Ignore the error here, since we don't want to fail the deployment if the identity providers are invalid
// A condition will be set on the HC to indicate the error
if len(volumeMountInfo.Volumes) > 0 {
- // Remove any existing IDP volume mounts before adding new ones
- removeIDPVolumeMounts(deployment)
-
deployment.Spec.Template.Spec.Volumes = append(deployment.Spec.Template.Spec.Volumes, volumeMountInfo.Volumes...)
util.UpdateContainer(ComponentName, deployment.Spec.Template.Spec.Containers, func(c *corev1.Container) {
c.VolumeMounts = append(c.VolumeMounts, volumeMountInfo.VolumeMounts.ContainerMounts(ComponentName)...)
})
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| identityProviders := configuration.OAuth.IdentityProviders | |
| if len(identityProviders) > 0 { | |
| // Remove any existing IDP volumes to ensure clean state after restore | |
| // This is important for OADP restore scenarios where the deployment | |
| // might be restored with stale volume configurations | |
| removeIDPVolumes(deployment) | |
| _, volumeMountInfo, _ := ConvertIdentityProviders(cpContext, identityProviders, providerOverrides(cpContext.HCP), cpContext.Client, deployment.Namespace) | |
| // Ignore the error here, since we don't want to fail the deployment if the identity providers are invalid | |
| // A condition will be set on the HC to indicate the error | |
| if len(volumeMountInfo.Volumes) > 0 { | |
| // Remove any existing IDP volume mounts before adding new ones | |
| removeIDPVolumeMounts(deployment) | |
| deployment.Spec.Template.Spec.Volumes = append(deployment.Spec.Template.Spec.Volumes, volumeMountInfo.Volumes...) | |
| util.UpdateContainer(ComponentName, deployment.Spec.Template.Spec.Containers, func(c *corev1.Container) { | |
| c.VolumeMounts = append(c.VolumeMounts, volumeMountInfo.VolumeMounts.ContainerMounts(ComponentName)...) | |
| }) | |
| } | |
| } else { | |
| // If no identity providers are configured, ensure any stale IDP volumes are removed | |
| removeIDPVolumes(deployment) | |
| removeIDPVolumeMounts(deployment) | |
| } | |
| identityProviders := configuration.OAuth.IdentityProviders | |
| if len(identityProviders) > 0 { | |
| // Remove any existing IDP volumes to ensure clean state after restore | |
| // This is important for OADP restore scenarios where the deployment | |
| // might be restored with stale volume configurations | |
| removeIDPVolumes(deployment) | |
| removeIDPVolumeMounts(deployment) | |
| _, volumeMountInfo, _ := ConvertIdentityProviders(cpContext, identityProviders, providerOverrides(cpContext.HCP), cpContext.Client, deployment.Namespace) | |
| // Ignore the error here, since we don't want to fail the deployment if the identity providers are invalid | |
| // A condition will be set on the HC to indicate the error | |
| if len(volumeMountInfo.Volumes) > 0 { | |
| deployment.Spec.Template.Spec.Volumes = append(deployment.Spec.Template.Spec.Volumes, volumeMountInfo.Volumes...) | |
| util.UpdateContainer(ComponentName, deployment.Spec.Template.Spec.Containers, func(c *corev1.Container) { | |
| c.VolumeMounts = append(c.VolumeMounts, volumeMountInfo.VolumeMounts.ContainerMounts(ComponentName)...) | |
| }) | |
| } | |
| } else { | |
| // If no identity providers are configured, ensure any stale IDP volumes are removed | |
| removeIDPVolumes(deployment) | |
| removeIDPVolumeMounts(deployment) | |
| } |
🤖 Prompt for AI Agents
In `@control-plane-operator/controllers/hostedcontrolplane/v2/oauth/deployment.go`
around lines 95 - 118, When identityProviders > 0, removeIDPVolumeMounts must
run regardless of whether ConvertIdentityProviders returned volumes to avoid
leaving stale mounts; change the block handling ConvertIdentityProviders so that
after calling removeIDPVolumes(deployment) and calling
ConvertIdentityProviders(...) you always call removeIDPVolumeMounts(deployment)
before inspecting volumeMountInfo.Volumes, then only append
volumeMountInfo.Volumes and update container mounts via
util.UpdateContainer(ComponentName, ...) if len(volumeMountInfo.Volumes) > 0.
Ensure the same functions (removeIDPVolumes, removeIDPVolumeMounts,
ConvertIdentityProviders, util.UpdateContainer, ComponentName, deployment) are
used and no other behavior changes.
|
/assign @jparrill |
mehabhalodiya
force-pushed
the
oadp-oauth
branch
from
cf02839 to
3f75e61
Compare
openshift-merge-robot
added
the
needs-rebase
openshift-ci
Bot
added
area/ai
openshift-ci-robot
added
the
jira/valid-bug
openshift-merge-robot
removed
the
needs-rebase
mehabhalodiya
force-pushed
the
oadp-oauth
branch
3 times, most recently
from
08f2779 to
717712a
Compare
|
/test e2e-aks |
|
/test e2e-aws |
|
/test e2e-aws-upgrade-hypershift-operator |
|
/test e2e-kubevirt-aws-ovn-reduced |
|
/test e2e-v2-aws |
|
Thanks for the PR!, one question, how are you testing this?, do you need some help to make sure this fully works? |
mehabhalodiya
force-pushed
the
oadp-oauth
branch
from
717712a to
1934f7d
Compare
|
@mehabhalodiya: This pull request references Jira Issue OCPBUGS-66251, 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 (mbhalodi@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. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7628 +/- ##
==========================================
+ Coverage 34.63% 34.75% +0.11%
==========================================
Files 767 767
Lines 93186 93196 +10
==========================================
+ Hits 32277 32389 +112
+ Misses 58236 58116 -120
- Partials 2673 2691 +18
🚀 New features to boost your workflow:
|
openshift-ci
Bot
added
needs-rebase
mehabhalodiya
force-pushed
the
oadp-oauth
branch
from
d0e1de7 to
1934f7d
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mehabhalodiya 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 |
This PR fixes an issue where OAuth identity provider (IDP) secrets are not mounted as volumes in the oauth-openshift deployment after OADP restore operations. Signed-off-by: mehabhalodiya <mehabhalodiya@gmail.com>
mehabhalodiya
force-pushed
the
oadp-oauth
branch
from
1934f7d to
c3d1589
Compare
openshift-ci
Bot
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. |
|
@mehabhalodiya: 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. |
|
/uncc @bryan-cox |
Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryAll three failures stem from two root issues: (1) a git merge conflict in Root Causeci/prow/security (FAILURE): The job failed during the git clone/checkout phase before any security scan could execute. Prow attempted to merge PR commit codecov/patch (FAILURE): Patch coverage is 21.05% with 15 lines of new/changed production code not covered by tests. Specifically:
tide (ERROR): Tide cannot merge the PR because: (1) merge conflicts exist ( Recommendations
Evidence
|
7 participants
Which issue(s) this PR fixes:
Fixes https://issues.redhat.com/browse/OCPBUGS-66251
Description
This PR fixes an issue where OAuth identity provider (IDP) secrets are not mounted as volumes in the oauth-openshift deployment after OADP restore operations.
✅ Changes Made & Tests Created
1. Fixed Error Handling (deployment.go & config.go)
The Root Cause:
The previous code was silently ignoring errors from ConvertIdentityProviders() using _, volumeMountInfo, _ := ...
When IDP conversion failed (e.g., during transient OADP restore conditions), no volumes were added
Result: HTPasswd secret was never mounted to the oauth-openshift deployment
The Fix:
This ensures:
✅ Errors are not silently ignored
✅ Reconciliation retries automatically on failure
✅ When OADP-restored secrets become available, conversion succeeds
✅ IDP volumes are properly added to the deployment
2. Comprehensive Testing
Created deployment_test.go with tests verifying:
✅ HTPasswd IDP volumes are added to deployment
Test: When HTPasswd IDP is configured, it should add IDP volumes to deployment
✅ Handles missing secrets correctly (OADP restore scenario)
Test: When HTPasswd IDP secret is missing, it should still add volume reference
✅ Supports multiple IDPs
Test: When multiple IDPs are configured, it should add all IDP volumes
✅ OADP Restore Scenario Test
Test: TestAdaptDeploymentOADPRestoreScenario
Created config_test.go verifying:
✅ OAuth config includes IDPs
3. Test Results
All tests pass successfully:
Special notes for your reviewer:
Checklist:
Summary by CodeRabbit
New Features
allocateNodeCIDRsfield.API Changes
AzurePrivateLinkServiceandHCPEtcdBackup.Infrastructure