Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,12 @@ func adaptDeployment(cpContext component.WorkloadContext, deployment *appsv1.Dep

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't do anything. The deployment object passed to the adapt func is coming from the static manifest file https://github.com/openshift/hypershift/blob/main/control-plane-operator/controllers/hostedcontrolplane/v2/assets/oauth-openshift/deployment.yaml

the code is already idempotent

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
Expand All @@ -103,6 +109,10 @@ func adaptDeployment(cpContext component.WorkloadContext, deployment *appsv1.Dep
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)
}
}

Expand Down Expand Up @@ -143,3 +153,31 @@ func applyNamedCertificateMounts(certs []configv1.APIServerNamedServingCert, spe
}
})
}

// removeIDPVolumes removes all IDP-related volumes from the deployment.
// This ensures clean state after OADP restore where stale volumes might exist.
func removeIDPVolumes(deployment *appsv1.Deployment) {
filtered := []corev1.Volume{}
for _, vol := range deployment.Spec.Template.Spec.Volumes {
// Keep volumes that are not IDP-related (idp-secret-* or idp-cm-*)
if !strings.HasPrefix(vol.Name, "idp-secret-") && !strings.HasPrefix(vol.Name, "idp-cm-") {
filtered = append(filtered, vol)
}
}
deployment.Spec.Template.Spec.Volumes = filtered
}

// removeIDPVolumeMounts removes all IDP-related volume mounts from the oauth-openshift container.
// This ensures clean state after OADP restore where stale volume mounts might exist.
func removeIDPVolumeMounts(deployment *appsv1.Deployment) {
util.UpdateContainer(ComponentName, deployment.Spec.Template.Spec.Containers, func(c *corev1.Container) {
filtered := []corev1.VolumeMount{}
for _, vm := range c.VolumeMounts {
// Keep volume mounts that are not IDP-related (idp-secret-* or idp-cm-*)
if !strings.HasPrefix(vm.Name, "idp-secret-") && !strings.HasPrefix(vm.Name, "idp-cm-") {
filtered = append(filtered, vm)
}
}
c.VolumeMounts = filtered
})
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
package oauth

import (
"testing"

. "github.com/onsi/gomega"

appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
)

// TestRemoveIDPVolumes verifies that removeIDPVolumes filters out only IDP-related volumes
// while keeping all other volumes intact.
func TestRemoveIDPVolumes(t *testing.T) {
t.Run("When deployment has mixed IDP and non-IDP volumes, it should keep only non-IDP volumes", func(t *testing.T) {
g := NewWithT(t)

deployment := &appsv1.Deployment{
Spec: appsv1.DeploymentSpec{
Template: corev1.PodTemplateSpec{
Spec: corev1.PodSpec{
Volumes: []corev1.Volume{
{Name: "idp-secret-0-file-data"},
{Name: "idp-cm-0-ca"},
{Name: "non-idp-volume"},
},
},
},
},
}

removeIDPVolumes(deployment)

g.Expect(deployment.Spec.Template.Spec.Volumes).To(HaveLen(1))
g.Expect(deployment.Spec.Template.Spec.Volumes[0].Name).To(Equal("non-idp-volume"))
})

t.Run("When deployment has no IDP volumes, it should keep all volumes unchanged", func(t *testing.T) {
g := NewWithT(t)

deployment := &appsv1.Deployment{
Spec: appsv1.DeploymentSpec{
Template: corev1.PodTemplateSpec{
Spec: corev1.PodSpec{
Volumes: []corev1.Volume{
{Name: "config-volume"},
{Name: "audit-config"},
},
},
},
},
}

removeIDPVolumes(deployment)

g.Expect(deployment.Spec.Template.Spec.Volumes).To(HaveLen(2))
g.Expect(deployment.Spec.Template.Spec.Volumes[0].Name).To(Equal("config-volume"))
g.Expect(deployment.Spec.Template.Spec.Volumes[1].Name).To(Equal("audit-config"))
})
}

// TestRemoveIDPVolumeMounts verifies that removeIDPVolumeMounts filters out only IDP-related
// volume mounts from the oauth-openshift container while keeping all other mounts intact.
func TestRemoveIDPVolumeMounts(t *testing.T) {
t.Run("When container has mixed IDP and non-IDP volume mounts, it should keep only non-IDP mounts", func(t *testing.T) {
g := NewWithT(t)

deployment := &appsv1.Deployment{
Spec: appsv1.DeploymentSpec{
Template: corev1.PodTemplateSpec{
Spec: corev1.PodSpec{
Containers: []corev1.Container{
{
Name: ComponentName,
VolumeMounts: []corev1.VolumeMount{
{Name: "idp-secret-0-file-data"},
{Name: "idp-cm-0-ca"},
{Name: "config-volume"},
},
},
{
// Another container to ensure we only filter the oauth-openshift container
Name: "sidecar",
VolumeMounts: []corev1.VolumeMount{
{Name: "idp-secret-0-file-data"},
{Name: "sidecar-volume"},
},
},
},
},
},
},
}

removeIDPVolumeMounts(deployment)

containers := deployment.Spec.Template.Spec.Containers
g.Expect(containers).To(HaveLen(2))

// oauth-openshift container should have only non-IDP mounts
oauthMounts := containers[0].VolumeMounts
g.Expect(oauthMounts).To(HaveLen(1))
g.Expect(oauthMounts[0].Name).To(Equal("config-volume"))

// sidecar container mounts should be untouched
sidecarMounts := containers[1].VolumeMounts
g.Expect(sidecarMounts).To(HaveLen(2))
g.Expect(sidecarMounts[0].Name).To(Equal("idp-secret-0-file-data"))
g.Expect(sidecarMounts[1].Name).To(Equal("sidecar-volume"))
})

t.Run("When oauth-openshift container has no IDP mounts, it should keep all mounts unchanged", func(t *testing.T) {
g := NewWithT(t)

deployment := &appsv1.Deployment{
Spec: appsv1.DeploymentSpec{
Template: corev1.PodTemplateSpec{
Spec: corev1.PodSpec{
Containers: []corev1.Container{
{
Name: ComponentName,
VolumeMounts: []corev1.VolumeMount{
{Name: "config-volume"},
{Name: "audit-config"},
},
},
},
},
},
},
}

removeIDPVolumeMounts(deployment)

oauthMounts := deployment.Spec.Template.Spec.Containers[0].VolumeMounts
g.Expect(oauthMounts).To(HaveLen(2))
g.Expect(oauthMounts[0].Name).To(Equal("config-volume"))
g.Expect(oauthMounts[1].Name).To(Equal("audit-config"))
})
}