Skip to content
Draft
Show file tree
Hide file tree
Changes from 1 commit
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
Prev Previous commit
feat(auth): validate external claims sourcing configuration
Reject external claims sourcing unless the target control plane is
5.1+ and the CPO feature set enables it. Resolve the target version
before validation and report lookup failures through cluster status.

Add shared HO/CPO checks for source URLs, authentication settings,
CEL expressions, duplicates, and list limits. Use local POC checks
until the webhook validator's Kubernetes dependencies are compatible.
  • Loading branch information
liouk committed Sep 8, 2026
commit ec00b33731f09f534f15dd1720b6a290416c9875
7 changes: 7 additions & 0 deletions control-plane-operator/featuregates/featuregates.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,13 @@ func Gate() featuregate.FeatureGate {
return globalGate
}

// EnabledForFeatureSet checks the CPO's feature definitions for the given feature
// set without changing the process-global gate. Unknown features or feature sets
// return false.
func EnabledForFeatureSet(feature featuregate.Feature, featureSet configv1.FeatureSet) bool {
return allFeatures.EnabledForFeatureSet(feature, featureSet)
}

// ConfigureFeatureSet is used to configure the feature gates based on the provided featureSet.
// The provided featureSet must be a known feature set name.
// ConfigureFeatureSet should only be called once on startup.
Expand Down
38 changes: 38 additions & 0 deletions control-plane-operator/featuregates/featuregates_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
package featuregates

import (
"testing"

. "github.com/onsi/gomega"

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

"k8s.io/component-base/featuregate"
)

func TestEnabledForFeatureSet(t *testing.T) {
testCases := []struct {
name string
feature featuregate.Feature
featureSet configv1.FeatureSet
enabled bool
}{
{"When external claims sourcing is queried in TechPreview it should be enabled", ExternalOIDCExternalClaimsSourcing, configv1.TechPreviewNoUpgrade, true},
{"When external claims sourcing is queried in Default it should be disabled", ExternalOIDCExternalClaimsSourcing, configv1.Default, false},
{"When UID and extra mappings are queried in Default it should be enabled", ExternalOIDCWithUIDAndExtraClaimMappings, configv1.Default, true},
{"When the feature is unknown it should be disabled", "Unknown", configv1.TechPreviewNoUpgrade, false},
{"When the feature set is unknown it should be disabled", ExternalOIDCExternalClaimsSourcing, "Unknown", false},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
g := NewWithT(t)
gate := Gate()
globalEnabled := gate.Enabled(ExternalOIDCExternalClaimsSourcing)
g.Expect(EnabledForFeatureSet(tc.feature, tc.featureSet)).To(Equal(tc.enabled))
g.Expect(Gate()).To(BeIdenticalTo(gate))
g.Expect(Gate().Enabled(ExternalOIDCExternalClaimsSourcing)).To(Equal(globalEnabled))
})
}
}
164 changes: 164 additions & 0 deletions hypershift-operator/controllers/hostedcluster/authentication_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,164 @@
package hostedcluster

import (
"errors"
"testing"
"time"

. "github.com/onsi/gomega"

hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1"
"github.com/openshift/hypershift/api/util/ipnet"
"github.com/openshift/hypershift/hypershift-operator/controllers/manifests"
"github.com/openshift/hypershift/hypershift-operator/controllers/manifests/controlplaneoperator"
"github.com/openshift/hypershift/support/api"
"github.com/openshift/hypershift/support/releaseinfo"
"github.com/openshift/hypershift/support/releaseinfo/testutils"
"github.com/openshift/hypershift/support/upsert"

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

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

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

"github.com/blang/semver"
"go.uber.org/mock/gomock"
)

func TestValidateOCPConfigurationsExternalClaimsSources(t *testing.T) {
t.Parallel()
for _, tc := range []struct {
name string
version string
featureSet configv1.FeatureSet
externalClaims bool
wantError bool
}{
{name: "When targeting 4.23 it should reject external claims", version: "4.23.0", featureSet: configv1.TechPreviewNoUpgrade, externalClaims: true, wantError: true},
{name: "When targeting 4.24 it should not alias it to 5.1", version: "4.24.0", featureSet: configv1.TechPreviewNoUpgrade, externalClaims: true, wantError: true},
{name: "When targeting 5.0 it should reject external claims", version: "5.0.9", featureSet: configv1.TechPreviewNoUpgrade, externalClaims: true, wantError: true},
{name: "When targeting 5.1 with the gate it should accept external claims", version: "5.1.0", featureSet: configv1.TechPreviewNoUpgrade, externalClaims: true},
{name: "When targeting a 5.1 prerelease it should accept external claims", version: "5.1.0-rc.1", featureSet: configv1.TechPreviewNoUpgrade, externalClaims: true},
{name: "When targeting a 5.1 patch it should accept external claims", version: "5.1.3", featureSet: configv1.TechPreviewNoUpgrade, externalClaims: true},
{name: "When targeting a later minor it should accept external claims", version: "5.2.0", featureSet: configv1.TechPreviewNoUpgrade, externalClaims: true},
{name: "When targeting a later major it should accept external claims", version: "6.0.0", featureSet: configv1.TechPreviewNoUpgrade, externalClaims: true},
{name: "When using the default feature set it should reject external claims", version: "5.1.0", externalClaims: true, wantError: true},
{name: "When using an unknown feature set it should reject external claims", version: "5.1.0", featureSet: "unknown", externalClaims: true, wantError: true},
{name: "When no external claims are configured it should accept ordinary OIDC on older releases", version: "4.22.0"},
} {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
g := NewWithT(t)
authn := &configv1.AuthenticationSpec{
Type: configv1.AuthenticationTypeOIDC,
OIDCProviders: []configv1.OIDCProvider{{
Name: "issuer",
Issuer: configv1.TokenIssuer{
URL: "https://issuer.example.com",
Audiences: []configv1.TokenAudience{"client"},
},
ClaimMappings: configv1.TokenClaimMappings{
Username: configv1.UsernameClaimMapping{Claim: "sub", PrefixPolicy: configv1.NoPrefix},
},
}},
}
if tc.externalClaims {
// Use the second provider to verify the field path identifies the offending provider.
provider := *authn.OIDCProviders[0].DeepCopy()
provider.Name = "external"
provider.Issuer.URL = "https://external.example.com"
provider.ExternalClaimsSources = []configv1.ExternalClaimsSource{{
URL: configv1.SourceURL{Hostname: "claims.example.com", PathExpression: "'/userinfo'"},
Mappings: []configv1.SourcedClaimMapping{{Name: "groups", Expression: "response.groups"}},
}}
authn.OIDCProviders = append(authn.OIDCProviders, provider)
}
hc := &hyperv1.HostedCluster{Spec: hyperv1.HostedClusterSpec{
Configuration: &hyperv1.ClusterConfiguration{Authentication: authn},
}}
r := &HostedClusterReconciler{FeatureSet: tc.featureSet, Client: fake.NewClientBuilder().WithScheme(api.Scheme).Build()}
err := r.validateOCPConfigurations(t.Context(), hc, r.Client, semver.MustParse(tc.version))
if tc.wantError {
g.Expect(err).To(MatchError(ContainSubstring("spec.configuration.authentication.oidcProviders[1].externalClaimsSources: Forbidden: requires control plane version 5.1 or later")))
} else {
g.Expect(err).NotTo(HaveOccurred())
}
})
}

t.Run("When authentication is absent it should skip authentication validation", func(t *testing.T) {
t.Parallel()
g := NewWithT(t)
r := &HostedClusterReconciler{Client: fake.NewClientBuilder().WithScheme(api.Scheme).Build()}
hc := &hyperv1.HostedCluster{}
g.Expect(r.validateOCPConfigurations(t.Context(), hc, r.Client, semver.Version{})).To(Succeed())
hc.Spec.Configuration = &hyperv1.ClusterConfiguration{}
g.Expect(r.validateOCPConfigurations(t.Context(), hc, r.Client, semver.Version{})).To(Succeed())
})
}

func TestReconcileAuthenticationReleaseLookupFailure(t *testing.T) {
t.Parallel()
for _, tc := range []struct {
name string
version string
err error
}{
{name: "When the release lookup fails it should persist unknown validation and retry", err: errors.New("registry unavailable")},
{name: "When the release version is malformed it should persist unknown validation and retry", version: "invalid-version"},
} {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
g := NewWithT(t)
hc := &hyperv1.HostedCluster{
ObjectMeta: metav1.ObjectMeta{Namespace: "clusters", Name: "example", Generation: 2},
Spec: hyperv1.HostedClusterSpec{
Release: hyperv1.Release{Image: "worker-release"},
ControlPlaneRelease: &hyperv1.Release{Image: "control-plane-release"},
PullSecret: corev1.LocalObjectReference{Name: "pull-secret"},
Networking: hyperv1.ClusterNetworking{
ClusterNetwork: []hyperv1.ClusterNetworkEntry{{CIDR: *ipnet.MustParseCIDR("172.16.1.0/24")}},
},
},
Status: hyperv1.HostedClusterStatus{Conditions: []metav1.Condition{{
Type: string(hyperv1.ValidHostedClusterConfiguration), Status: metav1.ConditionTrue, ObservedGeneration: 1,
}}},
}
hcp := controlplaneoperator.HostedControlPlane(manifests.HostedControlPlaneNamespace(hc.Namespace, hc.Name), hc.Name)
hcp.Spec.ReleaseImage = "previous-release"
originalHCP := hcp.DeepCopy()
pullSecret := &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{Namespace: hc.Namespace, Name: hc.Spec.PullSecret.Name},
Data: map[string][]byte{corev1.DockerConfigJsonKey: []byte("{}")},
}
c := fake.NewClientBuilder().WithScheme(api.Scheme).WithObjects(hc, hcp, pullSecret).WithStatusSubresource(hc).Build()
provider := releaseinfo.NewMockProviderWithOpenShiftImageRegistryOverrides(gomock.NewController(t))
var release *releaseinfo.ReleaseImage
if tc.version != "" {
release = testutils.InitReleaseImageOrDie(tc.version)
}
provider.EXPECT().Lookup(gomock.Any(), "control-plane-release", []byte("{}")).Return(release, tc.err)
r := &HostedClusterReconciler{
Client: c,
RegistryProvider: fakeReleaseProvider{releaseProvider: provider},
createOrUpdate: func(ctrl.Request) upsert.CreateOrUpdateFN { return ctrl.CreateOrUpdate },
now: func() metav1.Time { return metav1.NewTime(time.Now()) },
}
_, err := r.Reconcile(t.Context(), ctrl.Request{NamespacedName: client.ObjectKeyFromObject(hc)})
g.Expect(err).To(MatchError(ContainSubstring("failed to resolve control plane release version")))
g.Expect(c.Get(t.Context(), client.ObjectKeyFromObject(hc), hc)).To(Succeed())
condition := meta.FindStatusCondition(hc.Status.Conditions, string(hyperv1.ValidHostedClusterConfiguration))
g.Expect(condition).NotTo(BeNil())
g.Expect(condition.Status).To(Equal(metav1.ConditionUnknown))
g.Expect(condition.ObservedGeneration).To(Equal(hc.Generation))
g.Expect(condition.Message).To(Equal(err.Error()))
g.Expect(c.Get(t.Context(), client.ObjectKeyFromObject(hcp), hcp)).To(Succeed())
g.Expect(hcp.Spec).To(Equal(originalHCP.Spec))
})
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import (
capimanagerv2 "github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/v2/capi_manager"
capiproviderv2 "github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/v2/capi_provider"
cpov2 "github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/v2/controlplaneoperator"
cpoFeaturegates "github.com/openshift/hypershift/control-plane-operator/featuregates"
"github.com/openshift/hypershift/control-plane-pki-operator/certificates"
"github.com/openshift/hypershift/hypershift-operator/controllers/hostedcluster/internal/platform"
platformaws "github.com/openshift/hypershift/hypershift-operator/controllers/hostedcluster/internal/platform/aws"
Expand Down Expand Up @@ -653,6 +654,27 @@ func (r *HostedClusterReconciler) reconcile(ctx context.Context, req ctrl.Reques
releaseProvider := r.RegistryProvider.GetReleaseProvider()
registryClientImageMetadataProvider := r.RegistryProvider.GetMetadataProvider()

// Resolve the target control plane version before validating its configuration.
releaseImage, err := r.lookupReleaseImage(ctx, hcluster, releaseProvider)
var releaseImageVersion semver.Version
if err == nil {
releaseImageVersion, err = semver.Parse(releaseImage.Version())
}
if err != nil {
err = fmt.Errorf("failed to resolve control plane release version: %w", err)
meta.SetStatusCondition(&hcluster.Status.Conditions, metav1.Condition{
Type: string(hyperv1.ValidHostedClusterConfiguration),
ObservedGeneration: hcluster.Generation,
Status: metav1.ConditionUnknown,
Reason: hyperv1.InvalidImageReason,
Message: err.Error(),
})
if statusErr := r.Client.Status().Update(ctx, hcluster); statusErr != nil {
return ctrl.Result{}, errors.Join(err, fmt.Errorf("failed to update status: %w", statusErr))
}
return ctrl.Result{}, err
}

pullSecretBytes, err := hyperutil.GetPullSecretBytes(ctx, r.Client, hcluster)
if err != nil {
return ctrl.Result{}, err
Expand Down Expand Up @@ -995,7 +1017,7 @@ func (r *HostedClusterReconciler) reconcile(ctx context.Context, req ctrl.Reques
Type: string(hyperv1.ValidHostedClusterConfiguration),
ObservedGeneration: hcluster.Generation,
}
if err := r.validateConfigAndClusterCapabilities(ctx, hcluster); err != nil {
if err := r.validateConfigAndClusterCapabilities(ctx, hcluster, releaseImageVersion); err != nil {
condition.Status = metav1.ConditionFalse
condition.Message = err.Error()
condition.Reason = hyperv1.InvalidConfigurationReason
Expand Down Expand Up @@ -1215,10 +1237,6 @@ func (r *HostedClusterReconciler) reconcile(ctx context.Context, req ctrl.Reques

hcluster.Status.PayloadArch = payloadArch

releaseImage, err := r.lookupReleaseImage(ctx, hcluster, releaseProvider)
if err != nil {
return ctrl.Result{}, fmt.Errorf("failed to lookup release image: %w", err)
}
// Set Progressing condition
{
condition := metav1.Condition{
Expand Down Expand Up @@ -1780,13 +1798,6 @@ func (r *HostedClusterReconciler) reconcile(ctx context.Context, req ctrl.Reques
}
}

// Get release image version
var releaseImageVersion semver.Version
releaseImageVersion, err = semver.Parse(releaseImage.Version())
if err != nil {
return ctrl.Result{}, fmt.Errorf("failed to parse release image version: %w", err)
}

// Reconcile the HostedControlPlane
isAutoscalingNeeded, err := r.isAutoscalingNeeded(ctx, hcluster)
if err != nil {
Expand Down Expand Up @@ -3659,7 +3670,7 @@ func (r *HostedClusterReconciler) reconcileClusterPrometheusRBAC(ctx context.Con
return nil
}

func (r *HostedClusterReconciler) validateConfigAndClusterCapabilities(ctx context.Context, hc *hyperv1.HostedCluster) error {
func (r *HostedClusterReconciler) validateConfigAndClusterCapabilities(ctx context.Context, hc *hyperv1.HostedCluster, releaseVersion semver.Version) error {
var errs []error
for _, svc := range hc.Spec.Services {
if svc.Type == hyperv1.Route && !r.ManagementClusterCapabilities.Has(capabilities.CapabilityRoute) {
Expand Down Expand Up @@ -3711,7 +3722,7 @@ func (r *HostedClusterReconciler) validateConfigAndClusterCapabilities(ctx conte
errs = append(errs, err...)
}

if err := r.validateOCPConfigurations(ctx, hc, r.Client); err != nil {
if err := r.validateOCPConfigurations(ctx, hc, r.Client, releaseVersion); err != nil {
errs = append(errs, err)
}

Expand Down Expand Up @@ -4056,11 +4067,24 @@ func (r *HostedClusterReconciler) validateNetworks(hc *hyperv1.HostedCluster) er
//
// TODO: Add validation for other OpenShift components (e.g. OAuth, Ingress, etc.)
// Jira: https://issues.redhat.com/browse/CNTRLPLANE-382
func (r *HostedClusterReconciler) validateOCPConfigurations(ctx context.Context, hc *hyperv1.HostedCluster, client client.Client) error {
func (r *HostedClusterReconciler) validateOCPConfigurations(ctx context.Context, hc *hyperv1.HostedCluster, client client.Client, releaseVersion semver.Version) error {
var errs field.ErrorList
errs = append(errs, validations.ValidateOCPAPIServerSANs(ctx, hc, client)...)

if hc.Spec.Configuration != nil && hc.Spec.Configuration.Authentication != nil {
// Feature support follows the minor release, including its prereleases.
release := semver.Version{Major: releaseVersion.Major, Minor: releaseVersion.Minor}
for i, provider := range hc.Spec.Configuration.Authentication.OIDCProviders {
if len(provider.ExternalClaimsSources) == 0 {
continue
}
if release.LT(semver.Version{Major: 5, Minor: 1}) || !cpoFeaturegates.EnabledForFeatureSet(cpoFeaturegates.ExternalOIDCExternalClaimsSourcing, r.FeatureSet) {
errs = append(errs, field.Forbidden(
field.NewPath("spec", "configuration", "authentication", "oidcProviders").Index(i).Child("externalClaimsSources"),
fmt.Sprintf("requires control plane version 5.1 or later and feature gate %s enabled in the CPO feature set (target version: %s, feature set: %q)", cpoFeaturegates.ExternalOIDCExternalClaimsSourcing, releaseVersion, r.FeatureSet),
))
}
}
err := supportvalidations.ValidateAuthenticationSpec(ctx, client, hc.Spec.Configuration.Authentication, hc.Namespace, []string{hc.Spec.IssuerURL})
if err != nil {
fieldErr := &field.Error{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2320,7 +2320,7 @@ func TestValidateConfigAndClusterCapabilities(t *testing.T) {
r.KubevirtInfraClients = kvinfra.NewMockKubevirtInfraClientMap(r.Client, tc.infraKubeVirtVersion, tc.infraK8sVersion)

ctx := t.Context()
actual := r.validateConfigAndClusterCapabilities(ctx, tc.hostedCluster)
actual := r.validateConfigAndClusterCapabilities(ctx, tc.hostedCluster, semver.MustParse("5.1.0"))
if diff := cmp.Diff(actual, tc.expectedResult, equateErrorMessage); diff != "" {
t.Errorf("actual validation result differs from expected: %s", diff)
}
Expand Down
8 changes: 8 additions & 0 deletions pkg/featuregates/featuregates.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,14 @@ func (fsaf FeatureSetAwareFeatures) AddFeature(feature *Feature) {
}
}

// EnabledForFeatureSet reports whether a feature is enabled in the given feature
// set without constructing or changing a feature gate. Unknown features or feature
// sets return false.
func (fsaf FeatureSetAwareFeatures) EnabledForFeatureSet(feature featuregate.Feature, featureSet configv1.FeatureSet) bool {
features := fsaf[featureSet]
return features != nil && features.Enabled.Has(feature)
}

// FeatureGatesForFeatureSet returns the featuregate.MutableFeatureGate corresponding to the provided featureSet.
// If the provided featureSet is unknown, an error will be returned.
// If the provided featureSet is known, the featuregate.MutableFeatureGate will be returned where
Expand Down
15 changes: 15 additions & 0 deletions pkg/featuregates/featuregates_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ package featuregates_test
import (
"testing"

. "github.com/onsi/gomega"

"github.com/openshift/hypershift/pkg/featuregates"

configv1 "github.com/openshift/api/config/v1"
Expand Down Expand Up @@ -86,12 +88,25 @@ func TestCreatingFeatureGates(t *testing.T) {

for feat, expectedEnabledState := range expectedFeatureState {
assert.Equal(t, fg.Enabled(feat), expectedEnabledState, "actual featuregate enabled state does not match expected", "featureset", fs, "featuregate", feat)
NewWithT(t).Expect(features.EnabledForFeatureSet(feat, fs)).To(Equal(expectedEnabledState))
}
}
})
}
}

func TestEnabledForFeatureSetUnknownInputs(t *testing.T) {
features := featuregates.NewFeatureSetAwareFeatures()
features.AddFeature(featuregates.NewFeature("Foo", featuregates.WithEnableForFeatureSets(configv1.Default)))

t.Run("When the feature is unknown it should return false", func(t *testing.T) {
NewWithT(t).Expect(features.EnabledForFeatureSet("Unknown", configv1.Default)).To(BeFalse())
})
t.Run("When the feature set is unknown it should return false", func(t *testing.T) {
NewWithT(t).Expect(features.EnabledForFeatureSet("Foo", "Unknown")).To(BeFalse())
})
}

func TestConfiguringUnknownFeatureSetErrors(t *testing.T) {
features := featuregates.NewFeatureSetAwareFeatures()
_, err := features.FeatureGatesForFeatureSet("FooBar")
Expand Down
Loading