OCPBUGS-74511: remove RouteExternalCertificate feature gate

pkg/manifestclienttest/testdata/must-gather-01/cluster-scoped-resources/config.openshift.io/featuregates.yaml

@@ -59,7 +59,6 @@ items:
       - name: MixedCPUsAllocation
       - name: NetworkLiveMigration
       - name: NodeSwap
-      - name: RouteExternalCertificate
       - name: SigstoreImageVerification
       - name: VSphereControlPlaneMachineSet
       - name: VSphereStaticIPs

pkg/manifestclienttest/testdata/must-gather-01/cluster-scoped-resources/config.openshift.io/featuregates/cluster.yaml

@@ -56,7 +56,6 @@ status:
     - name: MixedCPUsAllocation
     - name: NetworkLiveMigration
     - name: NodeSwap
-    - name: RouteExternalCertificate
     - name: SigstoreImageVerification
     - name: VSphereControlPlaneMachineSet
     - name: VSphereStaticIPs

pkg/manifestclienttest/testdata/no-namespaces/cluster-scoped-resources/config.openshift.io/featuregates/cluster.yaml

@@ -56,7 +56,6 @@ status:
     - name: MixedCPUsAllocation
     - name: NetworkLiveMigration
     - name: NodeSwap
-    - name: RouteExternalCertificate
     - name: SigstoreImageVerification
     - name: VSphereControlPlaneMachineSet
     - name: VSphereStaticIPs

pkg/route/common.go

@@ -12,12 +12,3 @@ import (
 type SubjectAccessReviewCreator interface {
 	Create(ctx context.Context, sar *authorizationv1.SubjectAccessReview, opts metav1.CreateOptions) (*authorizationv1.SubjectAccessReview, error)
 }
-
-// RouteValidationOptions used to tweak how/what fields are validated. These
-// options are propagated by the apiserver.
-type RouteValidationOptions struct {
-
-	// AllowExternalCertificates option is set when the RouteExternalCertificate
-	// feature gate is enabled.
-	AllowExternalCertificates bool
-}

pkg/route/hostassignment/assignment.go

@@ -37,15 +37,15 @@ type HostnameGenerator interface {
 // AllocateHost allocates a host name ONLY if the route doesn't specify a subdomain wildcard policy and
 // the host name on the route is empty and an allocator is configured.
 // It must first allocate the shard and may return an error if shard allocation fails.
-func AllocateHost(ctx context.Context, route *routev1.Route, sarc route.SubjectAccessReviewCreator, routeAllocator HostnameGenerator, opts route.RouteValidationOptions) field.ErrorList {
+func AllocateHost(ctx context.Context, route *routev1.Route, sarc route.SubjectAccessReviewCreator, routeAllocator HostnameGenerator) field.ErrorList {
 	hostSet := len(route.Spec.Host) > 0
 	certSet := route.Spec.TLS != nil &&
 		(len(route.Spec.TLS.CACertificate) > 0 ||
 			len(route.Spec.TLS.Certificate) > 0 ||
 			len(route.Spec.TLS.DestinationCACertificate) > 0 ||
 			len(route.Spec.TLS.Key) > 0)
 
-	if opts.AllowExternalCertificates && route.Spec.TLS != nil && route.Spec.TLS.ExternalCertificate != nil {
+	if route.Spec.TLS != nil && route.Spec.TLS.ExternalCertificate != nil {
 		certSet = certSet || len(route.Spec.TLS.ExternalCertificate.Name) > 0
 	}
 
@@ -103,7 +103,7 @@ func AllocateHost(ctx context.Context, route *routev1.Route, sarc route.SubjectA
 	return nil
 }
 
-func hasCertificateInfo(tls *routev1.TLSConfig, opts route.RouteValidationOptions) bool {
+func hasCertificateInfo(tls *routev1.TLSConfig) bool {
 	if tls == nil {
 		return false
 	}
@@ -112,7 +112,7 @@ func hasCertificateInfo(tls *routev1.TLSConfig, opts route.RouteValidationOption
 		len(tls.CACertificate) > 0 ||
 		len(tls.DestinationCACertificate) > 0
 
-	if opts.AllowExternalCertificates && tls.ExternalCertificate != nil {
+	if tls.ExternalCertificate != nil {
 		hasInfo = hasInfo || len(tls.ExternalCertificate.Name) > 0
 	}
 	return hasInfo
@@ -122,11 +122,11 @@ func hasCertificateInfo(tls *routev1.TLSConfig, opts route.RouteValidationOption
 // Note: If (newer/updated) route uses externalCertificate, this function always returns true, as we cannot definitively verify if
 // the content of the referenced secret has been modified. Even if the secret name remains the same,
 // we must assume that the secret content is changed, necessitating authorization.
-func certificateChangeRequiresAuth(route, older *routev1.Route, opts route.RouteValidationOptions) bool {
+func certificateChangeRequiresAuth(route, older *routev1.Route) bool {
 	switch {
 	case route.Spec.TLS != nil && older.Spec.TLS != nil:
 		a, b := route.Spec.TLS, older.Spec.TLS
-		if !hasCertificateInfo(a, opts) {
+		if !hasCertificateInfo(a) {
 			// removing certificate info is allowed
 			return false
 		}
@@ -136,16 +136,14 @@ func certificateChangeRequiresAuth(route, older *routev1.Route, opts route.Route
 			a.DestinationCACertificate != b.DestinationCACertificate ||
 			a.Key != b.Key
 
-		if opts.AllowExternalCertificates {
-			if route.Spec.TLS.ExternalCertificate != nil {
-				certChanged = true
-			}
+		if route.Spec.TLS.ExternalCertificate != nil {
+			certChanged = true
 		}
 
 		return certChanged
 	case route.Spec.TLS != nil:
 		// using any default certificate is allowed
-		return hasCertificateInfo(route.Spec.TLS, opts)
+		return hasCertificateInfo(route.Spec.TLS)
 	default:
 		// all other cases we are not adding additional certificate info
 		return false
@@ -177,10 +175,10 @@ func validateImmutableField(newVal, oldVal interface{}, fldPath *field.Path, err
 // since we cannot verify state of external secret object.
 // Due to this it proceeds with the assumption that the certificate has changed
 // when the route has externalCertificate set.
-func ValidateHostUpdate(ctx context.Context, route, older *routev1.Route, sarc route.SubjectAccessReviewCreator, opts route.RouteValidationOptions) field.ErrorList {
+func ValidateHostUpdate(ctx context.Context, route, older *routev1.Route, sarc route.SubjectAccessReviewCreator) field.ErrorList {
 	hostChanged := route.Spec.Host != older.Spec.Host
 	subdomainChanged := route.Spec.Subdomain != older.Spec.Subdomain
-	certChanged := certificateChangeRequiresAuth(route, older, opts)
+	certChanged := certificateChangeRequiresAuth(route, older)
 	if !hostChanged && !certChanged && !subdomainChanged {
 		return nil
 	}
@@ -251,14 +249,12 @@ func ValidateHostUpdate(ctx context.Context, route, older *routev1.Route, sarc r
 			errs = append(errs, validateImmutableField(route.Spec.TLS.DestinationCACertificate, older.Spec.TLS.DestinationCACertificate, field.NewPath("spec", "tls", "destinationCACertificate"), routeTLSPermissionErrMsg)...)
 			errs = append(errs, validateImmutableField(route.Spec.TLS.Key, older.Spec.TLS.Key, field.NewPath("spec", "tls", "key"), routeTLSPermissionErrMsg)...)
 
-			if opts.AllowExternalCertificates {
-				if route.Spec.TLS.ExternalCertificate == nil || older.Spec.TLS.ExternalCertificate == nil {
-					errs = append(errs, validateImmutableField(route.Spec.TLS.ExternalCertificate, older.Spec.TLS.ExternalCertificate, field.NewPath("spec", "tls", "externalCertificate"), routeTLSPermissionErrMsg)...)
-				} else {
-					// since the state of the external secret cannot be verified, return error (even when secret name remains unchanged)
-					// without performing immutability checks, if externalCertificate is set.
-					errs = append(errs, field.Invalid(field.NewPath("spec", "tls", "externalCertificate"), route.Spec.TLS.ExternalCertificate, routeTLSPermissionErrMsg))
-				}
+			if route.Spec.TLS.ExternalCertificate == nil || older.Spec.TLS.ExternalCertificate == nil {
+				errs = append(errs, validateImmutableField(route.Spec.TLS.ExternalCertificate, older.Spec.TLS.ExternalCertificate, field.NewPath("spec", "tls", "externalCertificate"), routeTLSPermissionErrMsg)...)
+			} else {
+				// since the state of the external secret cannot be verified, return error (even when secret name remains unchanged)
+				// without performing immutability checks, if externalCertificate is set.
+				errs = append(errs, field.Invalid(field.NewPath("spec", "tls", "externalCertificate"), route.Spec.TLS.ExternalCertificate, routeTLSPermissionErrMsg))
 			}
 			return errs
 		}

pkg/route/hostassignment/assignment_test.go

@@ -12,7 +12,6 @@ import (
 	"k8s.io/apiserver/pkg/endpoints/request"
 
 	routev1 "github.com/openshift/api/route/v1"
-	"github.com/openshift/library-go/pkg/route"
 )
 
 type testAllocator struct {
@@ -53,9 +52,6 @@ func TestHostWithWildcardPolicies(t *testing.T) {
 		expected          string
 		expectedSubdomain string
 
-		// field for externalCertificate
-		opts route.RouteValidationOptions
-
 		errs  int
 		allow bool
 	}{
@@ -139,8 +135,6 @@ func TestHostWithWildcardPolicies(t *testing.T) {
 			wildcardPolicy: routev1.WildcardPolicyNone,
 			allow:          false,
 			errs:           1,
-
-			opts: route.RouteValidationOptions{AllowExternalCertificates: true},
 		},
 		{
 			name:           "tls-permission-allowed-external-certificate",
@@ -149,8 +143,6 @@ func TestHostWithWildcardPolicies(t *testing.T) {
 			wildcardPolicy: routev1.WildcardPolicyNone,
 			allow:          true,
 			errs:           0,
-
-			opts: route.RouteValidationOptions{AllowExternalCertificates: true},
 		},
 		{
 			name:           "no-host-but-allowed",
@@ -293,7 +285,6 @@ func TestHostWithWildcardPolicies(t *testing.T) {
 			allow:          false,
 			errs:           1, // AllocateHost() -> routeHostPermissionErrMsg
 
-			opts: route.RouteValidationOptions{AllowExternalCertificates: true},
 		},
 		{
 			name:           "create-with-external-certificate-allowed",
@@ -303,8 +294,6 @@ func TestHostWithWildcardPolicies(t *testing.T) {
 			wildcardPolicy: routev1.WildcardPolicyNone,
 			allow:          true,
 			errs:           0,
-
-			opts: route.RouteValidationOptions{AllowExternalCertificates: true},
 		},
 		{
 			name:           "external-certificate-added-from-no-certificate-denied",
@@ -316,8 +305,6 @@ func TestHostWithWildcardPolicies(t *testing.T) {
 			wildcardPolicy: routev1.WildcardPolicyNone,
 			allow:          false,
 			errs:           1,
-
-			opts: route.RouteValidationOptions{AllowExternalCertificates: true},
 		},
 		{
 			name:           "external-certificate-added-from-no-certificate-allowed",
@@ -329,8 +316,6 @@ func TestHostWithWildcardPolicies(t *testing.T) {
 			wildcardPolicy: routev1.WildcardPolicyNone,
 			allow:          true,
 			errs:           0,
-
-			opts: route.RouteValidationOptions{AllowExternalCertificates: true},
 		},
 		{
 			name:           "external-certificate-added-from-nil-tls-denied",
@@ -342,8 +327,6 @@ func TestHostWithWildcardPolicies(t *testing.T) {
 			wildcardPolicy: routev1.WildcardPolicyNone,
 			allow:          false,
 			errs:           1,
-
-			opts: route.RouteValidationOptions{AllowExternalCertificates: true},
 		},
 		{
 			name:           "external-certificate-added-from-nil-tls-allowed",
@@ -355,8 +338,6 @@ func TestHostWithWildcardPolicies(t *testing.T) {
 			wildcardPolicy: routev1.WildcardPolicyNone,
 			allow:          true,
 			errs:           0,
-
-			opts: route.RouteValidationOptions{AllowExternalCertificates: true},
 		},
 		{
 			name:           "external-certificate-removed-and-set-no-certificate-denied",
@@ -369,8 +350,6 @@ func TestHostWithWildcardPolicies(t *testing.T) {
 			allow:          false,
 			// removing certificate info is allowed
 			errs: 0,
-
-			opts: route.RouteValidationOptions{AllowExternalCertificates: true},
 		},
 		{
 			name:           "external-certificate-removed-and-set-no-certificate-allowed",
@@ -383,8 +362,6 @@ func TestHostWithWildcardPolicies(t *testing.T) {
 			allow:          true,
 			// removing certificate info is allowed
 			errs: 0,
-
-			opts: route.RouteValidationOptions{AllowExternalCertificates: true},
 		},
 		{
 			name:           "external-certificate-removed-and-set-nil-tls-denied",
@@ -397,8 +374,6 @@ func TestHostWithWildcardPolicies(t *testing.T) {
 			allow:          false,
 			// removing certificate info is allowed
 			errs: 0,
-
-			opts: route.RouteValidationOptions{AllowExternalCertificates: true},
 		},
 		{
 			name:           "external-certificate-removed-and-set-nil-tls-allowed",
@@ -411,8 +386,6 @@ func TestHostWithWildcardPolicies(t *testing.T) {
 			allow:          true,
 			// removing certificate info is allowed
 			errs: 0,
-
-			opts: route.RouteValidationOptions{AllowExternalCertificates: true},
 		},
 		{
 			name:           "external-certificate-changed-to-certificate-denied",
@@ -424,8 +397,6 @@ func TestHostWithWildcardPolicies(t *testing.T) {
 			wildcardPolicy: routev1.WildcardPolicyNone,
 			allow:          false,
 			errs:           2,
-
-			opts: route.RouteValidationOptions{AllowExternalCertificates: true},
 		},
 		{
 			name:           "external-certificate-changed-to-certificate-allowed",
@@ -437,8 +408,6 @@ func TestHostWithWildcardPolicies(t *testing.T) {
 			wildcardPolicy: routev1.WildcardPolicyNone,
 			allow:          true,
 			errs:           0,
-
-			opts: route.RouteValidationOptions{AllowExternalCertificates: true},
 		},
 		{
 			name:           "certificate-changed-to-external-certificate-denied",
@@ -450,8 +419,6 @@ func TestHostWithWildcardPolicies(t *testing.T) {
 			wildcardPolicy: routev1.WildcardPolicyNone,
 			allow:          false,
 			errs:           2,
-
-			opts: route.RouteValidationOptions{AllowExternalCertificates: true},
 		},
 		{
 			name:           "certificate-changed-to-external-certificate-allowed",
@@ -463,8 +430,6 @@ func TestHostWithWildcardPolicies(t *testing.T) {
 			wildcardPolicy: routev1.WildcardPolicyNone,
 			allow:          true,
 			errs:           0,
-
-			opts: route.RouteValidationOptions{AllowExternalCertificates: true},
 		},
 		{
 			name:     "certificate-changed-to-external-certificate-denied-and-featuregate-is-not-set",
@@ -478,8 +443,6 @@ func TestHostWithWildcardPolicies(t *testing.T) {
 			wildcardPolicy: routev1.WildcardPolicyNone,
 			allow:          false,
 			errs:           0,
-
-			opts: route.RouteValidationOptions{AllowExternalCertificates: false},
 		},
 		{
 			name:     "certificate-changed-to-external-certificate-allowed-but-featuregate-is-not-set",
@@ -493,8 +456,6 @@ func TestHostWithWildcardPolicies(t *testing.T) {
 			wildcardPolicy: routev1.WildcardPolicyNone,
 			allow:          true,
 			errs:           0,
-
-			opts: route.RouteValidationOptions{AllowExternalCertificates: false},
 		},
 		{
 			name:           "external-certificate-changed-denied",
@@ -506,8 +467,6 @@ func TestHostWithWildcardPolicies(t *testing.T) {
 			wildcardPolicy: routev1.WildcardPolicyNone,
 			allow:          false,
 			errs:           1,
-
-			opts: route.RouteValidationOptions{AllowExternalCertificates: true},
 		},
 		{
 			name:           "external-certificate-changed-allowed",
@@ -519,8 +478,6 @@ func TestHostWithWildcardPolicies(t *testing.T) {
 			wildcardPolicy: routev1.WildcardPolicyNone,
 			allow:          true,
 			errs:           0,
-
-			opts: route.RouteValidationOptions{AllowExternalCertificates: true},
 		},
 		{
 			name:           "external-certificate-unchanged-denied",
@@ -533,8 +490,6 @@ func TestHostWithWildcardPolicies(t *testing.T) {
 			allow:          false,
 			// permission is required even if referenced secret name remains unchanged
 			errs: 1,
-
-			opts: route.RouteValidationOptions{AllowExternalCertificates: true},
 		},
 		{
 			name:           "external-certificate-unchanged-allowed",
@@ -546,8 +501,6 @@ func TestHostWithWildcardPolicies(t *testing.T) {
 			wildcardPolicy: routev1.WildcardPolicyNone,
 			allow:          true,
 			errs:           0,
-
-			opts: route.RouteValidationOptions{AllowExternalCertificates: true},
 		},
 		{
 			name:           "ca-certificate-unchanged",
@@ -702,9 +655,9 @@ func TestHostWithWildcardPolicies(t *testing.T) {
 						},
 					},
 				}
-				errs = ValidateHostUpdate(ctx, route, oldRoute, &testSAR{allow: tc.allow}, tc.opts)
+				errs = ValidateHostUpdate(ctx, route, oldRoute, &testSAR{allow: tc.allow})
 			} else {
-				errs = AllocateHost(ctx, route, &testSAR{allow: tc.allow}, testAllocator{}, tc.opts)
+				errs = AllocateHost(ctx, route, &testSAR{allow: tc.allow}, testAllocator{})
 			}
 
 			if route.Spec.Host != tc.expected {