Skip to content

OCPBUGS-66251: OADP restore results in htpasswd no longer being applied as volume #7628

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

Open
mehabhalodiya wants to merge 1 commit into
base: main
Choose a base branch
from
Open

OCPBUGS-66251: OADP restore results in htpasswd no longer being applied as volume #7628

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
17 changes: 12 additions & 5 deletions control-plane-operator/controllers/hostedcontrolplane/v2/oauth/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,9 @@ func adaptConfigMap(cpContext component.WorkloadContext, cm *corev1.ConfigMap) e
return fmt.Errorf("failed to decode existing oauth server configuration: %w", err)
}

adaptOAuthConfig(cpContext, oauthConfig)
if err := adaptOAuthConfig(cpContext, oauthConfig); err != nil {
return err
}
serializedConfig, err := util.SerializeResource(oauthConfig, api.Scheme)
if err != nil {
return fmt.Errorf("failed to serialize oauth server configuration: %w", err)
Expand All @@ -60,7 +62,7 @@ func adaptConfigMap(cpContext component.WorkloadContext, cm *corev1.ConfigMap) e
return nil
}

func adaptOAuthConfig(cpContext component.WorkloadContext, cfg *osinv1.OsinServerConfig) {
func adaptOAuthConfig(cpContext component.WorkloadContext, cfg *osinv1.OsinServerConfig) error {
configuration := cpContext.HCP.Spec.Configuration

cfg.GenericAPIServerConfig.ServingInfo.NamedCertificates = globalconfig.GetConfigNamedCertificates(configuration.GetNamedCertificates(), oauthNamedCertificateMountPathPrefix)
Expand All @@ -85,11 +87,16 @@ func adaptOAuthConfig(cpContext component.WorkloadContext, cfg *osinv1.OsinServe

// var identityProviders []osinv1.IdentityProvider
if configuration != nil && configuration.OAuth != nil {
// 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
identityProviders, _, _ := ConvertIdentityProviders(cpContext, configuration.OAuth.IdentityProviders, providerOverrides(cpContext.HCP), cpContext.Client, cpContext.HCP.Namespace)
identityProviders, _, err := ConvertIdentityProviders(cpContext, configuration.OAuth.IdentityProviders, providerOverrides(cpContext.HCP), cpContext.Client, cpContext.HCP.Namespace)
if err != nil {
// Return the error to trigger reconciliation retry. This ensures that after OADP restore,
// if IDP conversion fails due to transient issues, it will be retried until successful.
// The HCP controller separately validates IDPs and sets a ValidIDPConfiguration condition.
return fmt.Errorf("failed to convert identity providers: %w", err)
}
cfg.OAuthConfig.IdentityProviders = identityProviders
}
return nil
}

func providerOverrides(hcp *hyperv1.HostedControlPlane) map[string]*ConfigOverride {
Expand Down
196 changes: 196 additions & 0 deletions control-plane-operator/controllers/hostedcontrolplane/v2/oauth/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,196 @@
package oauth

import (
"testing"

. "github.com/onsi/gomega"

hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1"
"github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/infra"
"github.com/openshift/hypershift/support/api"
component "github.com/openshift/hypershift/support/controlplane-component"

configv1 "github.com/openshift/api/config/v1"
osinv1 "github.com/openshift/api/osin/v1"

corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

"sigs.k8s.io/controller-runtime/pkg/client/fake"
)

// TestAdaptOAuthConfigWithHTPasswd verifies that HTPasswd IDP is properly
// configured in the OAuth server config
func TestAdaptOAuthConfigWithHTPasswd(t *testing.T) {
testCases := []struct {
name string
identityProviders []configv1.IdentityProvider
idpSecrets []*corev1.Secret
expectError bool
expectedIDPCount int
}{
{
name: "When HTPasswd IDP is configured with secret, it should add IDP to config",
identityProviders: []configv1.IdentityProvider{
{
Name: "htpasswd-idp",
IdentityProviderConfig: configv1.IdentityProviderConfig{
Type: configv1.IdentityProviderTypeHTPasswd,
HTPasswd: &configv1.HTPasswdIdentityProvider{
FileData: configv1.SecretNameReference{
Name: "htpasswd-secret",
},
},
},
},
},
idpSecrets: []*corev1.Secret{
{
ObjectMeta: metav1.ObjectMeta{
Name: "htpasswd-secret",
Namespace: "test-namespace",
},
Data: map[string][]byte{
"htpasswd": []byte("admin:$apr1$xyz"),
},
},
},
expectError: false,
expectedIDPCount: 1,
},
{
name: "When multiple IDPs are configured, all should be added to config",
identityProviders: []configv1.IdentityProvider{
{
Name: "htpasswd-idp",
IdentityProviderConfig: configv1.IdentityProviderConfig{
Type: configv1.IdentityProviderTypeHTPasswd,
HTPasswd: &configv1.HTPasswdIdentityProvider{
FileData: configv1.SecretNameReference{
Name: "htpasswd-secret",
},
},
},
},
{
Name: "github-idp",
IdentityProviderConfig: configv1.IdentityProviderConfig{
Type: configv1.IdentityProviderTypeGitHub,
GitHub: &configv1.GitHubIdentityProvider{
ClientID: "test-client",
ClientSecret: configv1.SecretNameReference{
Name: "github-secret",
},
},
},
},
},
idpSecrets: []*corev1.Secret{
{
ObjectMeta: metav1.ObjectMeta{
Name: "htpasswd-secret",
Namespace: "test-namespace",
},
Data: map[string][]byte{
"htpasswd": []byte("admin:$apr1$xyz"),
},
},
{
ObjectMeta: metav1.ObjectMeta{
Name: "github-secret",
Namespace: "test-namespace",
},
Data: map[string][]byte{
"clientSecret": []byte("github-secret-value"),
},
},
},
expectError: false,
expectedIDPCount: 2,
},
{
name: "When no IDPs are configured, config should have no IDPs",
identityProviders: []configv1.IdentityProvider{},
idpSecrets: []*corev1.Secret{},
expectError: false,
expectedIDPCount: 0,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
g := NewGomegaWithT(t)

// Create fake client with IDP secrets
fakeClientBuilder := fake.NewClientBuilder().WithScheme(api.Scheme)
for _, secret := range tc.idpSecrets {
fakeClientBuilder.WithObjects(secret)
}

hcp := &hyperv1.HostedControlPlane{
ObjectMeta: metav1.ObjectMeta{
Name: "test-hcp",
Namespace: "test-namespace",
},
Spec: hyperv1.HostedControlPlaneSpec{
Configuration: &hyperv1.ClusterConfiguration{
OAuth: &configv1.OAuthSpec{
IdentityProviders: tc.identityProviders,
},
},
},
Status: hyperv1.HostedControlPlaneStatus{
ControlPlaneEndpoint: hyperv1.APIEndpoint{
Host: "api.test.example.com",
Port: 6443,
},
},
}

cpContext := component.WorkloadContext{
Client: fakeClientBuilder.Build(),
HCP: hcp,
InfraStatus: infra.InfrastructureStatus{
OAuthHost: "oauth.test.example.com",
OAuthPort: 443,
},
}

// Create a minimal OAuth config
cfg := &osinv1.OsinServerConfig{
GenericAPIServerConfig: configv1.GenericAPIServerConfig{
ServingInfo: configv1.HTTPServingInfo{},
},
OAuthConfig: osinv1.OAuthConfig{},
}

// Adapt the config
err := adaptOAuthConfig(cpContext, cfg)

if tc.expectError {
g.Expect(err).To(HaveOccurred())
return
}

g.Expect(err).ToNot(HaveOccurred())

// Verify the correct number of IDPs were added
g.Expect(len(cfg.OAuthConfig.IdentityProviders)).To(Equal(tc.expectedIDPCount),
"Expected %d identity providers but found %d", tc.expectedIDPCount, len(cfg.OAuthConfig.IdentityProviders))

// If IDPs are expected, verify they are properly configured
if tc.expectedIDPCount > 0 {
idpNames := make(map[string]bool)
for _, idp := range cfg.OAuthConfig.IdentityProviders {
idpNames[idp.Name] = true
}

// Verify all configured IDPs are present
for _, expectedIDP := range tc.identityProviders {
g.Expect(idpNames).To(HaveKey(expectedIDP.Name),
"Expected IDP %s to be in config", expectedIDP.Name)
}
}
})
}
}
10 changes: 7 additions & 3 deletions control-plane-operator/controllers/hostedcontrolplane/v2/oauth/deployment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,9 +94,13 @@ func adaptDeployment(cpContext component.WorkloadContext, deployment *appsv1.Dep

identityProviders := configuration.OAuth.IdentityProviders
if len(identityProviders) > 0 {
_, 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
_, volumeMountInfo, err := ConvertIdentityProviders(cpContext, identityProviders, providerOverrides(cpContext.HCP), cpContext.Client, deployment.Namespace)
if err != nil {
// Return the error to trigger reconciliation retry. This ensures that after OADP restore,
// if IDP conversion fails due to transient issues, it will be retried until successful.
// The HCP controller separately validates IDPs and sets a ValidIDPConfiguration condition.
return fmt.Errorf("failed to convert identity providers: %w", err)
}
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) {
Expand Down
Loading
Loading