From 26b6b7ac9776e525d191d1d3e41c148d98628752 Mon Sep 17 00:00:00 2001 From: Grant Spence Date: Mon, 8 Jan 2024 16:05:49 -0500 Subject: [PATCH 1/5] Set Upgradeable=False if default cert uses SHA-1 If the default certificate is using SHA-1, then set Upgradeable to be False. With OpenSSL 3.0 provided by RHEL 9, the router will fail to start if the default cert provided is SHA-1. --- pkg/operator/controller/ingress/status.go | 23 +++++--- .../controller/ingress/status_test.go | 58 ++++++++++++++----- 2 files changed, 58 insertions(+), 23 deletions(-) diff --git a/pkg/operator/controller/ingress/status.go b/pkg/operator/controller/ingress/status.go index ec6073ce56..047c7115f9 100644 --- a/pkg/operator/controller/ingress/status.go +++ b/pkg/operator/controller/ingress/status.go @@ -689,15 +689,18 @@ func computeIngressEvaluationConditionsDetectedCondition(ic *operatorv1.IngressC } // checkDefaultCertificate returns an error value indicating whether the default -// certificate is safe for upgrades. In particular, if the current default -// certificate specifies a Subject Alternative Name (SAN) for the ingress -// domain, then it is safe to upgrade, and the return value is nil. Otherwise, -// if the certificate has a legacy Common Name (CN) and no SAN, then the return +// certificate is safe for upgrades. There are two scenarios we handle: +// 1.If the current default certificate specifies a legacy Common Name (CN) and no +// Subject Alternative Name (SAN) for the ingress domain, then the return // value is an error indicating that the certificate must be replaced by one -// with a SAN before upgrading is allowed. This check is necessary because -// OpenShift 4.10 and newer are built using Go 1.17, which rejects certificates -// without SANs. Note that this function only checks the validity of the -// certificate insofar as it affects upgrades. +// with a SAN. This check is necessary because OpenShift 4.10 and newer are +// built using Go 1.17, which rejects certificates without SANs. +// 2. If the current default certificate uses a SHA1-based signature algorithm, then +// the return value is an error indicating SHA1 is too weak. This check is necessary +// because OpenShift 4.16+ uses RHEL 9 and OpenSSL 3.0, which disallow SHA1. +// Note that this function only checks the validity of the certificate insofar as it +// affects upgrades. If the default certificate is safe for upgrades, then the return +// value is nil. func checkDefaultCertificate(secret *corev1.Secret, domain string) error { var certData []byte if v, ok := secret.Data["tls.crt"]; !ok { @@ -725,6 +728,10 @@ func checkDefaultCertificate(secret *corev1.Secret, domain string) error { if cert.Subject.CommonName == domain && !foundSAN { return fmt.Errorf("certificate in secret %s/%s has legacy Common Name (CN) but has no Subject Alternative Name (SAN) for domain: %s", secret.Namespace, secret.Name, domain) } + switch cert.SignatureAlgorithm { + case x509.SHA1WithRSA, x509.ECDSAWithSHA1: + return fmt.Errorf("certificate in secret %s/%s has weak SHA1 signature algorithm: %s (see https://docs.openshift.com/container-platform/4.16/release_notes/ocp-4-16-release-notes.html#ocp-4-16-sha-haproxy-support-removed_release-notes for more details)", secret.Namespace, secret.Name, cert.SignatureAlgorithm) + } } return nil diff --git a/pkg/operator/controller/ingress/status_test.go b/pkg/operator/controller/ingress/status_test.go index 9b3dbd4e47..e8c70d7dde 100644 --- a/pkg/operator/controller/ingress/status_test.go +++ b/pkg/operator/controller/ingress/status_test.go @@ -1,6 +1,8 @@ package ingress import ( + "crypto/ecdsa" + "crypto/elliptic" "crypto/rand" "crypto/rsa" "crypto/x509" @@ -2213,20 +2215,36 @@ func Test_checkZoneInConfig(t *testing.T) { } func Test_computeIngressUpgradeableCondition(t *testing.T) { - makeDefaultCertificateSecret := func(cn string, sans []string) *corev1.Secret { - key, err := rsa.GenerateKey(rand.Reader, 2048) - if err != nil { - t.Fatalf("failed to generate key: %v", err) - } - + makeDefaultCertificateSecret := func(cn string, sans []string, signatureAlgorithm x509.SignatureAlgorithm) *corev1.Secret { certTemplate := &x509.Certificate{ - SerialNumber: big.NewInt(1), - Subject: pkix.Name{CommonName: cn}, - DNSNames: sans, + SerialNumber: big.NewInt(1), + Subject: pkix.Name{CommonName: cn}, + DNSNames: sans, + SignatureAlgorithm: signatureAlgorithm, } - cert, err := x509.CreateCertificate(rand.Reader, certTemplate, certTemplate, &key.PublicKey, key) - if err != nil { - t.Fatalf("failed to generate certificate: %v", err) + + var cert []byte + switch signatureAlgorithm { + case x509.SHA1WithRSA, x509.SHA256WithRSA, x509.SHA384WithRSA, x509.SHA512WithRSA: + key, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatalf("failed to generate RSA key: %v", err) + } + cert, err = x509.CreateCertificate(rand.Reader, certTemplate, certTemplate, &key.PublicKey, key) + if err != nil { + t.Fatalf("failed to generate RSA certificate: %v", err) + } + case x509.ECDSAWithSHA1, x509.ECDSAWithSHA256, x509.ECDSAWithSHA384, x509.ECDSAWithSHA512: + key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + t.Fatalf("failed to generate ECDSA key: %v", err) + } + cert, err = x509.CreateCertificate(rand.Reader, certTemplate, certTemplate, &key.PublicKey, key) + if err != nil { + t.Fatalf("failed to generate ECDSA certificate: %v", err) + } + default: + t.Fatal("unsupported signature algorithm") } certData := pem.EncodeToMemory(&pem.Block{ @@ -2278,12 +2296,22 @@ func Test_computeIngressUpgradeableCondition(t *testing.T) { }, { description: "if the default certificate has a SAN", - secret: makeDefaultCertificateSecret(wildcardDomain, []string{wildcardDomain}), + secret: makeDefaultCertificateSecret(wildcardDomain, []string{wildcardDomain}, x509.ECDSAWithSHA256), expect: true, }, { description: "if the default certificate has no SAN", - secret: makeDefaultCertificateSecret(wildcardDomain, []string{}), + secret: makeDefaultCertificateSecret(wildcardDomain, []string{}, x509.ECDSAWithSHA256), + expect: false, + }, + { + description: "if the default certificate uses ECDSA SHA1", + secret: makeDefaultCertificateSecret(wildcardDomain, []string{wildcardDomain}, x509.ECDSAWithSHA1), + expect: false, + }, + { + description: "if the default certificate uses RSA SHA1", + secret: makeDefaultCertificateSecret(wildcardDomain, []string{wildcardDomain}, x509.SHA1WithRSA), expect: false, }, } @@ -2333,7 +2361,7 @@ func Test_computeIngressUpgradeableCondition(t *testing.T) { } secret := tc.secret if secret == nil { - secret = makeDefaultCertificateSecret("", []string{wildcardDomain}) + secret = makeDefaultCertificateSecret("", []string{wildcardDomain}, x509.SHA256WithRSA) } expectedStatus := operatorv1.ConditionFalse From 09ceb002c5d3dd82ba023d47a89b5206fa4a2e81 Mon Sep 17 00:00:00 2001 From: Grant Spence Date: Tue, 30 Jan 2024 11:33:35 -0500 Subject: [PATCH 2/5] Bump openshift/api Bump openshift/api to get new route UnservableInFutureVersions status condition type. go mod edit -replace=github.com/openshift/api=github.com/openshift/api@93d6bda14341f1d18319774367829c3278c171e7 go mod tidy go mod vendor Vendors https://github.com/openshift/api/pull/1751 --- go.mod | 2 +- go.sum | 4 +- .../v1/001-cloudprivateipconfig.crd.yaml | 2 +- .../001-cloudprivateipconfig.crd.yaml-patch | 2 +- ...01_authentication.crd-CustomNoUpgrade.yaml | 3 +- ...authentication.crd-Default-Hypershift.yaml | 3 +- ...tication.crd-Default-Hypershift.yaml-patch | 5 +- ...thentication.crd-TechPreviewNoUpgrade.yaml | 3 +- ...onfig-operator_01_network-Default.crd.yaml | 47 +++++++++++++++++++ .../config/v1/stable.network.testsuite.yaml | 10 +++- .../api/config/v1/types_authentication.go | 3 +- .../openshift/api/config/v1/types_feature.go | 2 +- .../openshift/api/config/v1/types_network.go | 1 - ...uster-network-operator_01-Default.crd.yaml | 7 +++ ...0_90_cluster_csi_driver_01_config.crd.yaml | 2 +- .../operator/v1/stable.network.testsuite.yaml | 7 +-- .../operator/v1/types_csi_cluster_driver.go | 2 +- .../api/operator/v1/types_network.go | 1 - .../openshift/api/route/v1/generated.proto | 2 +- .../route/v1/route-CustomNoUpgrade.crd.yaml | 2 +- .../v1/route-TechPreviewNoUpgrade.crd.yaml | 2 +- .../openshift/api/route/v1/route.crd.yaml | 2 +- .../openshift/api/route/v1/types.go | 6 ++- .../v1/zz_generated.swagger_doc_generated.go | 2 +- vendor/modules.txt | 4 +- 25 files changed, 96 insertions(+), 30 deletions(-) diff --git a/go.mod b/go.mod index c31dca8ec8..8ecb718df9 100644 --- a/go.mod +++ b/go.mod @@ -143,6 +143,6 @@ require ( // github.com/operator-framework/operator-sdk. replace ( bitbucket.org/ww/goautoneg => github.com/munnerz/goautoneg v0.0.0-20120707110453-a547fc61f48d - github.com/openshift/api => github.com/openshift/api v0.0.0-20240131192415-e18b9cc8aa8b + github.com/openshift/api => github.com/openshift/api v0.0.0-20240522145529-93d6bda14341 k8s.io/client-go => k8s.io/client-go v0.28.0 ) diff --git a/go.sum b/go.sum index 00417880ea..722626706b 100644 --- a/go.sum +++ b/go.sum @@ -918,8 +918,8 @@ github.com/opencontainers/runtime-spec v0.1.2-0.20190507144316-5b71a03e2700/go.m github.com/opencontainers/runtime-spec v0.1.2-0.20190618234442-a950415649c7/go.mod h1:jwyrGlmzljRJv/Fgzds9SsS/C5hL+LL3ko9hs6T5lQ0= github.com/opencontainers/runtime-spec v1.0.0/go.mod h1:jwyrGlmzljRJv/Fgzds9SsS/C5hL+LL3ko9hs6T5lQ0= github.com/opencontainers/runtime-tools v0.0.0-20181011054405-1d69bd0f9c39/go.mod h1:r3f7wjNzSs2extwzU3Y+6pKfobzPh+kKFJ3ofN+3nfs= -github.com/openshift/api v0.0.0-20240131192415-e18b9cc8aa8b h1:+oWnXf9QvOlVG4Y5NJ18iiWMbbwsp5CND+jjM3M/qRA= -github.com/openshift/api v0.0.0-20240131192415-e18b9cc8aa8b/go.mod h1:qNtV0315F+f8ld52TLtPvrfivZpdimOzTi3kn9IVbtU= +github.com/openshift/api v0.0.0-20240522145529-93d6bda14341 h1:JQpzgk+p24rkgNbNsrNR0yLm63WTKapuT60INU5BqT8= +github.com/openshift/api v0.0.0-20240522145529-93d6bda14341/go.mod h1:qNtV0315F+f8ld52TLtPvrfivZpdimOzTi3kn9IVbtU= github.com/openshift/build-machinery-go v0.0.0-20200211121458-5e3d6e570160/go.mod h1:1CkcsT3aVebzRBzVTSbiKSkJMsC/CASqxesfqEMfJEc= github.com/openshift/client-go v0.0.0-20200116152001-92a2713fa240/go.mod h1:4riOwdj99Hd/q+iAcJZfNCsQQQMwURnZV6RL4WHYS5w= github.com/openshift/client-go v0.0.0-20230120202327-72f107311084 h1:66uaqNwA+qYyQDwsMWUfjjau8ezmg1dzCqub13KZOcE= diff --git a/vendor/github.com/openshift/api/cloudnetwork/v1/001-cloudprivateipconfig.crd.yaml b/vendor/github.com/openshift/api/cloudnetwork/v1/001-cloudprivateipconfig.crd.yaml index d4e9e0b88e..41c0671c85 100644 --- a/vendor/github.com/openshift/api/cloudnetwork/v1/001-cloudprivateipconfig.crd.yaml +++ b/vendor/github.com/openshift/api/cloudnetwork/v1/001-cloudprivateipconfig.crd.yaml @@ -29,7 +29,7 @@ spec: name: anyOf: - format: ipv4 - - format: ipv6 + - pattern: ^[0-9a-f]{4}(\.[0-9a-f]{4}){7}$ type: string type: object spec: diff --git a/vendor/github.com/openshift/api/cloudnetwork/v1/001-cloudprivateipconfig.crd.yaml-patch b/vendor/github.com/openshift/api/cloudnetwork/v1/001-cloudprivateipconfig.crd.yaml-patch index 1239c05439..a11d1b82c9 100644 --- a/vendor/github.com/openshift/api/cloudnetwork/v1/001-cloudprivateipconfig.crd.yaml-patch +++ b/vendor/github.com/openshift/api/cloudnetwork/v1/001-cloudprivateipconfig.crd.yaml-patch @@ -7,4 +7,4 @@ type: string anyOf: - format: ipv4 - - format: ipv6 + - pattern: '^[0-9a-f]{4}(\.[0-9a-f]{4}){7}$' diff --git a/vendor/github.com/openshift/api/config/v1/0000_10_config-operator_01_authentication.crd-CustomNoUpgrade.yaml b/vendor/github.com/openshift/api/config/v1/0000_10_config-operator_01_authentication.crd-CustomNoUpgrade.yaml index 949938d499..69d171917c 100644 --- a/vendor/github.com/openshift/api/config/v1/0000_10_config-operator_01_authentication.crd-CustomNoUpgrade.yaml +++ b/vendor/github.com/openshift/api/config/v1/0000_10_config-operator_01_authentication.crd-CustomNoUpgrade.yaml @@ -141,7 +141,8 @@ spec: audiences: description: Audiences is an array of audiences that the token was issued for. Valid tokens must include at least one of these values in their "aud" claim. Must be set to exactly one value. type: array - maxItems: 1 + maxItems: 10 + minItems: 1 items: type: string minLength: 1 diff --git a/vendor/github.com/openshift/api/config/v1/0000_10_config-operator_01_authentication.crd-Default-Hypershift.yaml b/vendor/github.com/openshift/api/config/v1/0000_10_config-operator_01_authentication.crd-Default-Hypershift.yaml index fe60935aab..cc698b46c6 100644 --- a/vendor/github.com/openshift/api/config/v1/0000_10_config-operator_01_authentication.crd-Default-Hypershift.yaml +++ b/vendor/github.com/openshift/api/config/v1/0000_10_config-operator_01_authentication.crd-Default-Hypershift.yaml @@ -123,7 +123,8 @@ spec: items: minLength: 1 type: string - maxItems: 1 + maxItems: 10 + minItems: 1 type: array x-kubernetes-list-type: set issuerCertificateAuthority: diff --git a/vendor/github.com/openshift/api/config/v1/0000_10_config-operator_01_authentication.crd-Default-Hypershift.yaml-patch b/vendor/github.com/openshift/api/config/v1/0000_10_config-operator_01_authentication.crd-Default-Hypershift.yaml-patch index ed03e26ca4..dcc254fbd5 100644 --- a/vendor/github.com/openshift/api/config/v1/0000_10_config-operator_01_authentication.crd-Default-Hypershift.yaml-patch +++ b/vendor/github.com/openshift/api/config/v1/0000_10_config-operator_01_authentication.crd-Default-Hypershift.yaml-patch @@ -91,7 +91,8 @@ audiences: description: Audiences is an array of audiences that the token was issued for. Valid tokens must include at least one of these values in their "aud" claim. Must be set to exactly one value. type: array - maxItems: 1 + maxItems: 10 + minItems: 1 items: type: string minLength: 1 @@ -281,4 +282,4 @@ - "" - None - IntegratedOAuth - - OIDC \ No newline at end of file + - OIDC diff --git a/vendor/github.com/openshift/api/config/v1/0000_10_config-operator_01_authentication.crd-TechPreviewNoUpgrade.yaml b/vendor/github.com/openshift/api/config/v1/0000_10_config-operator_01_authentication.crd-TechPreviewNoUpgrade.yaml index 0b29bbb0bc..83a8d87d18 100644 --- a/vendor/github.com/openshift/api/config/v1/0000_10_config-operator_01_authentication.crd-TechPreviewNoUpgrade.yaml +++ b/vendor/github.com/openshift/api/config/v1/0000_10_config-operator_01_authentication.crd-TechPreviewNoUpgrade.yaml @@ -126,7 +126,8 @@ spec: items: minLength: 1 type: string - maxItems: 1 + maxItems: 10 + minItems: 1 type: array x-kubernetes-list-type: set issuerCertificateAuthority: diff --git a/vendor/github.com/openshift/api/config/v1/0000_10_config-operator_01_network-Default.crd.yaml b/vendor/github.com/openshift/api/config/v1/0000_10_config-operator_01_network-Default.crd.yaml index 4582bf8fdf..8548be6571 100644 --- a/vendor/github.com/openshift/api/config/v1/0000_10_config-operator_01_network-Default.crd.yaml +++ b/vendor/github.com/openshift/api/config/v1/0000_10_config-operator_01_network-Default.crd.yaml @@ -110,6 +110,53 @@ spec: clusterNetworkMTU: description: ClusterNetworkMTU is the MTU for inter-pod networking. type: integer + conditions: + description: 'conditions represents the observations of a network.config current state. Known .status.conditions.type are: "NetworkTypeMigrationInProgress", "NetworkTypeMigrationMTUReady", "NetworkTypeMigrationTargetCNIAvailable", "NetworkTypeMigrationTargetCNIInUse" and "NetworkTypeMigrationOriginalCNIPurged"' + type: array + items: + description: "Condition contains details for one aspect of the current state of this API Resource. --- This struct is intended for direct use as an array at the field path .status.conditions. For example, \n type FooStatus struct{ // Represents the observations of a foo's current state. // Known .status.conditions.type are: \"Available\", \"Progressing\", and \"Degraded\" // +patchMergeKey=type // +patchStrategy=merge // +listType=map // +listMapKey=type Conditions []metav1.Condition `json:\"conditions,omitempty\" patchStrategy:\"merge\" patchMergeKey:\"type\" protobuf:\"bytes,1,rep,name=conditions\"` \n // other fields }" + type: object + required: + - lastTransitionTime + - message + - reason + - status + - type + properties: + lastTransitionTime: + description: lastTransitionTime is the last time the condition transitioned from one status to another. This should be when the underlying condition changed. If that is not known, then using the time when the API field changed is acceptable. + type: string + format: date-time + message: + description: message is a human readable message indicating details about the transition. This may be an empty string. + type: string + maxLength: 32768 + observedGeneration: + description: observedGeneration represents the .metadata.generation that the condition was set based upon. For instance, if .metadata.generation is currently 12, but the .status.conditions[x].observedGeneration is 9, the condition is out of date with respect to the current state of the instance. + type: integer + format: int64 + minimum: 0 + reason: + description: reason contains a programmatic identifier indicating the reason for the condition's last transition. Producers of specific condition types may define expected values and meanings for this field, and whether the values are considered a guaranteed API. The value should be a CamelCase string. This field may not be empty. + type: string + maxLength: 1024 + minLength: 1 + pattern: ^[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?$ + status: + description: status of the condition, one of True, False, Unknown. + type: string + enum: + - "True" + - "False" + - Unknown + type: + description: type of condition in CamelCase or in foo.example.com/CamelCase. --- Many .condition.type values are consistent across resources like Available, but because arbitrary conditions can be useful (see .node.status.conditions), the ability to deconflict is important. The regex it matches is (dns1123SubdomainFmt/)?(qualifiedNameFmt) + type: string + maxLength: 316 + pattern: ^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])$ + x-kubernetes-list-map-keys: + - type + x-kubernetes-list-type: map migration: description: Migration contains the cluster network migration configuration. type: object diff --git a/vendor/github.com/openshift/api/config/v1/stable.network.testsuite.yaml b/vendor/github.com/openshift/api/config/v1/stable.network.testsuite.yaml index 7922d44812..c85d122a65 100644 --- a/vendor/github.com/openshift/api/config/v1/stable.network.testsuite.yaml +++ b/vendor/github.com/openshift/api/config/v1/stable.network.testsuite.yaml @@ -12,7 +12,7 @@ tests: apiVersion: config.openshift.io/v1 kind: Network spec: {} - - name: Should not be able to set status conditions + - name: Should be able to set status conditions initial: | apiVersion: config.openshift.io/v1 kind: Network @@ -28,4 +28,10 @@ tests: apiVersion: config.openshift.io/v1 kind: Network spec: {} - status: {} + status: + conditions: + - type: NetworkTypeMigrationInProgress + status: "False" + reason: "Reason" + message: "Message" + lastTransitionTime: "2023-10-25T12:00:00Z" diff --git a/vendor/github.com/openshift/api/config/v1/types_authentication.go b/vendor/github.com/openshift/api/config/v1/types_authentication.go index b53aff173f..62c9e7f5ae 100644 --- a/vendor/github.com/openshift/api/config/v1/types_authentication.go +++ b/vendor/github.com/openshift/api/config/v1/types_authentication.go @@ -240,7 +240,8 @@ type TokenIssuer struct { // // +listType=set // +kubebuilder:validation:Required - // +kubebuilder:validation:MaxItems=1 + // +kubebuilder:validation:MinItems=1 + // +kubebuilder:validation:MaxItems=10 // +required Audiences []TokenAudience `json:"audiences"` diff --git a/vendor/github.com/openshift/api/config/v1/types_feature.go b/vendor/github.com/openshift/api/config/v1/types_feature.go index 880d7312de..462a246960 100644 --- a/vendor/github.com/openshift/api/config/v1/types_feature.go +++ b/vendor/github.com/openshift/api/config/v1/types_feature.go @@ -188,7 +188,6 @@ var FeatureSets = map[FeatureSet]*FeatureGateEnabledDisabled{ with(metricsServer). with(installAlternateInfrastructureAWS). without(clusterAPIInstall). - with(sdnLiveMigration). with(mixedCPUsAllocation). with(managedBootImages). without(disableKubeletCloudCredentialProviders). @@ -211,6 +210,7 @@ var defaultFeatures = &FeatureGateEnabledDisabled{ externalCloudProviderExternal, privateHostedZoneAWS, buildCSIVolumes, + sdnLiveMigration, }, Disabled: []FeatureGateDescription{ disableKubeletCloudCredentialProviders, // We do not currently ship the correct config to use the external credentials provider. diff --git a/vendor/github.com/openshift/api/config/v1/types_network.go b/vendor/github.com/openshift/api/config/v1/types_network.go index 3d345b2d60..794f3db7b7 100644 --- a/vendor/github.com/openshift/api/config/v1/types_network.go +++ b/vendor/github.com/openshift/api/config/v1/types_network.go @@ -95,7 +95,6 @@ type NetworkStatus struct { // +patchStrategy=merge // +listType=map // +listMapKey=type - // +openshift:enable:FeatureSets=CustomNoUpgrade;TechPreviewNoUpgrade Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type"` } diff --git a/vendor/github.com/openshift/api/operator/v1/0000_70_cluster-network-operator_01-Default.crd.yaml b/vendor/github.com/openshift/api/operator/v1/0000_70_cluster-network-operator_01-Default.crd.yaml index 2a19187c88..a32c771b82 100644 --- a/vendor/github.com/openshift/api/operator/v1/0000_70_cluster-network-operator_01-Default.crd.yaml +++ b/vendor/github.com/openshift/api/operator/v1/0000_70_cluster-network-operator_01-Default.crd.yaml @@ -428,6 +428,13 @@ spec: description: multicast specifies whether or not the multicast configuration is migrated automatically when changing the cluster default network provider. If unset, this property defaults to 'true' and multicast configure is migrated. type: boolean default: true + mode: + description: mode indicates the mode of network migration. The supported values are "Live", "Offline" and omitted. A "Live" migration operation will not cause service interruption by migrating the CNI of each node one by one. The cluster network will work as normal during the network migration. An "Offline" migration operation will cause service interruption. During an "Offline" migration, two rounds of node reboots are required. The cluster network will be malfunctioning during the network migration. When omitted, this means no opinion and the platform is left to choose a reasonable default which is subject to change over time. The current default value is "Offline". + type: string + enum: + - Live + - Offline + - "" mtu: description: mtu contains the MTU migration configuration. Set this to allow changing the MTU values for the default network. If unset, the operation of changing the MTU for the default network will be rejected. type: object diff --git a/vendor/github.com/openshift/api/operator/v1/0000_90_cluster_csi_driver_01_config.crd.yaml b/vendor/github.com/openshift/api/operator/v1/0000_90_cluster_csi_driver_01_config.crd.yaml index 93e34a5dfc..cbd642a568 100644 --- a/vendor/github.com/openshift/api/operator/v1/0000_90_cluster_csi_driver_01_config.crd.yaml +++ b/vendor/github.com/openshift/api/operator/v1/0000_90_cluster_csi_driver_01_config.crd.yaml @@ -59,7 +59,7 @@ spec: properties: kmsKeyARN: description: kmsKeyARN sets the cluster default storage class to encrypt volumes with a user-defined KMS key, rather than the default KMS key used by AWS. The value may be either the ARN or Alias ARN of a KMS key. - pattern: ^arn:(aws|aws-cn|aws-us-gov):kms:[a-z0-9-]+:[0-9]{12}:(key|alias)\/.*$ + pattern: ^arn:(aws|aws-cn|aws-us-gov|aws-iso|aws-iso-b|aws-iso-e|aws-iso-f):kms:[a-z0-9-]+:[0-9]{12}:(key|alias)\/.*$ type: string type: object azure: diff --git a/vendor/github.com/openshift/api/operator/v1/stable.network.testsuite.yaml b/vendor/github.com/openshift/api/operator/v1/stable.network.testsuite.yaml index 47d5354280..7590f57142 100644 --- a/vendor/github.com/openshift/api/operator/v1/stable.network.testsuite.yaml +++ b/vendor/github.com/openshift/api/operator/v1/stable.network.testsuite.yaml @@ -255,11 +255,11 @@ tests: ipv6: internalMasqueradeSubnet: "abcd:eff01:2345:6789::2345:6789/20" expectedError: "Invalid value: \"string\": each segment of an IPv6 address must be a hexadecimal number between 0 and FFFF, failed on segment 2" - - name: Should not be able to create migration mode + - name: Should be able to create migration mode initial: | apiVersion: operator.openshift.io/v1 kind: Network - spec: + spec: migration: mode: Live expected: | @@ -269,7 +269,8 @@ tests: disableNetworkDiagnostics: false logLevel: Normal operatorLogLevel: Normal - migration: {} + migration: + mode: Live - name: "IPsec - Empty ipsecConfig is allowed in initial state" initial: | apiVersion: operator.openshift.io/v1 diff --git a/vendor/github.com/openshift/api/operator/v1/types_csi_cluster_driver.go b/vendor/github.com/openshift/api/operator/v1/types_csi_cluster_driver.go index 8e9853b06f..9ec7e5bed5 100644 --- a/vendor/github.com/openshift/api/operator/v1/types_csi_cluster_driver.go +++ b/vendor/github.com/openshift/api/operator/v1/types_csi_cluster_driver.go @@ -159,7 +159,7 @@ type AWSCSIDriverConfigSpec struct { // kmsKeyARN sets the cluster default storage class to encrypt volumes with a user-defined KMS key, // rather than the default KMS key used by AWS. // The value may be either the ARN or Alias ARN of a KMS key. - // +kubebuilder:validation:Pattern:=`^arn:(aws|aws-cn|aws-us-gov):kms:[a-z0-9-]+:[0-9]{12}:(key|alias)\/.*$` + // +kubebuilder:validation:Pattern:=`^arn:(aws|aws-cn|aws-us-gov|aws-iso|aws-iso-b|aws-iso-e|aws-iso-f):kms:[a-z0-9-]+:[0-9]{12}:(key|alias)\/.*$` // +optional KMSKeyARN string `json:"kmsKeyARN,omitempty"` } diff --git a/vendor/github.com/openshift/api/operator/v1/types_network.go b/vendor/github.com/openshift/api/operator/v1/types_network.go index 1bc5e77bd0..190605b852 100644 --- a/vendor/github.com/openshift/api/operator/v1/types_network.go +++ b/vendor/github.com/openshift/api/operator/v1/types_network.go @@ -157,7 +157,6 @@ type NetworkMigration struct { // An "Offline" migration operation will cause service interruption. During an "Offline" migration, two rounds of node reboots are required. The cluster network will be malfunctioning during the network migration. // When omitted, this means no opinion and the platform is left to choose a reasonable default which is subject to change over time. // The current default value is "Offline". - // +openshift:enable:FeatureSets=CustomNoUpgrade;TechPreviewNoUpgrade // +optional Mode NetworkMigrationMode `json:"mode"` } diff --git a/vendor/github.com/openshift/api/route/v1/generated.proto b/vendor/github.com/openshift/api/route/v1/generated.proto index d31fa5222e..66b35420e9 100644 --- a/vendor/github.com/openshift/api/route/v1/generated.proto +++ b/vendor/github.com/openshift/api/route/v1/generated.proto @@ -213,7 +213,7 @@ message RouteIngress { // router. message RouteIngressCondition { // Type is the type of the condition. - // Currently only Admitted. + // Currently only Admitted or UnservableInFutureVersions. optional string type = 1; // Status is the status of the condition. diff --git a/vendor/github.com/openshift/api/route/v1/route-CustomNoUpgrade.crd.yaml b/vendor/github.com/openshift/api/route/v1/route-CustomNoUpgrade.crd.yaml index 13461f6669..eeeccbc97a 100644 --- a/vendor/github.com/openshift/api/route/v1/route-CustomNoUpgrade.crd.yaml +++ b/vendor/github.com/openshift/api/route/v1/route-CustomNoUpgrade.crd.yaml @@ -344,7 +344,7 @@ spec: description: Status is the status of the condition. Can be True, False, Unknown. type: string type: - description: Type is the type of the condition. Currently only Admitted. + description: Type is the type of the condition. Currently only Admitted or UnservableInFutureVersions. type: string host: description: Host is the host string under which the route is exposed; this value is required diff --git a/vendor/github.com/openshift/api/route/v1/route-TechPreviewNoUpgrade.crd.yaml b/vendor/github.com/openshift/api/route/v1/route-TechPreviewNoUpgrade.crd.yaml index 87b617cac1..a9146d716b 100644 --- a/vendor/github.com/openshift/api/route/v1/route-TechPreviewNoUpgrade.crd.yaml +++ b/vendor/github.com/openshift/api/route/v1/route-TechPreviewNoUpgrade.crd.yaml @@ -344,7 +344,7 @@ spec: description: Status is the status of the condition. Can be True, False, Unknown. type: string type: - description: Type is the type of the condition. Currently only Admitted. + description: Type is the type of the condition. Currently only Admitted or UnservableInFutureVersions. type: string host: description: Host is the host string under which the route is exposed; this value is required diff --git a/vendor/github.com/openshift/api/route/v1/route.crd.yaml b/vendor/github.com/openshift/api/route/v1/route.crd.yaml index cda46fc33f..e126760255 100644 --- a/vendor/github.com/openshift/api/route/v1/route.crd.yaml +++ b/vendor/github.com/openshift/api/route/v1/route.crd.yaml @@ -376,7 +376,7 @@ spec: description: Status is the status of the condition. Can be True, False, Unknown. type: string type: - description: Type is the type of the condition. Currently only Admitted. + description: Type is the type of the condition. Currently only Admitted or UnservableInFutureVersions. type: string required: - status diff --git a/vendor/github.com/openshift/api/route/v1/types.go b/vendor/github.com/openshift/api/route/v1/types.go index 2de728bc00..b5a567d6a5 100644 --- a/vendor/github.com/openshift/api/route/v1/types.go +++ b/vendor/github.com/openshift/api/route/v1/types.go @@ -369,14 +369,16 @@ type RouteIngressConditionType string const ( // RouteAdmitted means the route is able to service requests for the provided Host RouteAdmitted RouteIngressConditionType = "Admitted" - // TODO: add other route condition types + // RouteUnservableInFutureVersions indicates that the route is using an unsupported + // configuration that may be incompatible with a future version of OpenShift. + RouteUnservableInFutureVersions RouteIngressConditionType = "UnservableInFutureVersions" ) // RouteIngressCondition contains details for the current condition of this route on a particular // router. type RouteIngressCondition struct { // Type is the type of the condition. - // Currently only Admitted. + // Currently only Admitted or UnservableInFutureVersions. Type RouteIngressConditionType `json:"type" protobuf:"bytes,1,opt,name=type,casttype=RouteIngressConditionType"` // Status is the status of the condition. // Can be True, False, Unknown. diff --git a/vendor/github.com/openshift/api/route/v1/zz_generated.swagger_doc_generated.go b/vendor/github.com/openshift/api/route/v1/zz_generated.swagger_doc_generated.go index 8d49587177..c65815a1cc 100644 --- a/vendor/github.com/openshift/api/route/v1/zz_generated.swagger_doc_generated.go +++ b/vendor/github.com/openshift/api/route/v1/zz_generated.swagger_doc_generated.go @@ -85,7 +85,7 @@ func (RouteIngress) SwaggerDoc() map[string]string { var map_RouteIngressCondition = map[string]string{ "": "RouteIngressCondition contains details for the current condition of this route on a particular router.", - "type": "Type is the type of the condition. Currently only Admitted.", + "type": "Type is the type of the condition. Currently only Admitted or UnservableInFutureVersions.", "status": "Status is the status of the condition. Can be True, False, Unknown.", "reason": "(brief) reason for the condition's last transition, and is usually a machine and human readable constant", "message": "Human readable message indicating details about last transition.", diff --git a/vendor/modules.txt b/vendor/modules.txt index d9503d6ab6..5e3bd5ee10 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -345,7 +345,7 @@ github.com/munnerz/goautoneg github.com/oklog/ulid # github.com/onsi/ginkgo v1.16.5 ## explicit; go 1.16 -# github.com/openshift/api v3.9.1-0.20190924102528-32369d4db2ad+incompatible => github.com/openshift/api v0.0.0-20240131192415-e18b9cc8aa8b +# github.com/openshift/api v3.9.1-0.20190924102528-32369d4db2ad+incompatible => github.com/openshift/api v0.0.0-20240522145529-93d6bda14341 ## explicit; go 1.20 github.com/openshift/api github.com/openshift/api/apiserver @@ -1262,5 +1262,5 @@ sigs.k8s.io/structured-merge-diff/v4/value ## explicit; go 1.12 sigs.k8s.io/yaml # bitbucket.org/ww/goautoneg => github.com/munnerz/goautoneg v0.0.0-20120707110453-a547fc61f48d -# github.com/openshift/api => github.com/openshift/api v0.0.0-20240131192415-e18b9cc8aa8b +# github.com/openshift/api => github.com/openshift/api v0.0.0-20240522145529-93d6bda14341 # k8s.io/client-go => k8s.io/client-go v0.28.0 From 6c4576b37e821043e9bc04914b5a9906d7b03d0e Mon Sep 17 00:00:00 2001 From: Grant Spence Date: Tue, 11 Jun 2024 13:08:18 -0400 Subject: [PATCH 3/5] Rename certificate generation E2E helper functions Rename generateClientCA and generateClientCertificate to generateCA and generateCertificate respectively to reflect the fact that they will also generate server certs in future E2E tests. --- test/e2e/client_tls_test.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/test/e2e/client_tls_test.go b/test/e2e/client_tls_test.go index 24dec9f801..bb50212abd 100644 --- a/test/e2e/client_tls_test.go +++ b/test/e2e/client_tls_test.go @@ -57,25 +57,25 @@ func TestClientTLS(t *testing.T) { t.Parallel() // We will configure the ingresscontroller to recognize certificates // signed by this CA. - ca, caKey, err := generateClientCA() + ca, caKey, err := generateCA() if err != nil { t.Fatalf("failed to generate client CA certificate: %v", err) } - validMatchingCert, validMatchingKey, err := generateClientCertificate(ca, caKey, "allowed") + validMatchingCert, validMatchingKey, err := generateCertificate(ca, caKey, "allowed") if err != nil { t.Fatalf("failed to generate first client certificate: %v", err) } - validMismatchingCert, validMismatchingKey, err := generateClientCertificate(ca, caKey, "disallowed") + validMismatchingCert, validMismatchingKey, err := generateCertificate(ca, caKey, "disallowed") if err != nil { t.Fatalf("failed to generate second client certificate: %v", err) } // The ingresscontroller will not recognize certificates signed by this // other CA. - otherCA, otherCAKey, err := generateClientCA() + otherCA, otherCAKey, err := generateCA() if err != nil { t.Fatalf("failed to generate other CA certificate: %v", err) } - invalidMatchingCert, invalidMatchingKey, err := generateClientCertificate(otherCA, otherCAKey, "allowed") + invalidMatchingCert, invalidMatchingKey, err := generateCertificate(otherCA, otherCAKey, "allowed") if err != nil { t.Fatalf("failed to generate third client certificate: %v", err) } @@ -1439,8 +1439,8 @@ func getActiveCRLs(t *testing.T, clientPod *corev1.Pod) ([]*x509.RevocationList, return crls, nil } -// generateClientCA generates and returns a CA certificate and key. -func generateClientCA() (*x509.Certificate, *rsa.PrivateKey, error) { +// generateCA generates and returns a CA certificate and key. +func generateCA() (*x509.Certificate, *rsa.PrivateKey, error) { key, err := rsa.GenerateKey(rand.Reader, 2048) if err != nil { return nil, nil, err @@ -1475,9 +1475,9 @@ func generateClientCA() (*x509.Certificate, *rsa.PrivateKey, error) { return certs[0], key, nil } -// generateClientCertificate generates and returns a client certificate and key +// generateCertificate generates and returns a certificate and key // where the certificate is signed by the provided CA certificate. -func generateClientCertificate(caCert *x509.Certificate, caKey *rsa.PrivateKey, cn string) (*x509.Certificate, *rsa.PrivateKey, error) { +func generateCertificate(caCert *x509.Certificate, caKey *rsa.PrivateKey, cn string) (*x509.Certificate, *rsa.PrivateKey, error) { key, err := rsa.GenerateKey(rand.Reader, 2048) if err != nil { return nil, nil, err From 37971411ffcf7654224436a8765b38bf8a3e04d8 Mon Sep 17 00:00:00 2001 From: Grant Spence Date: Tue, 11 Jun 2024 13:00:03 -0400 Subject: [PATCH 4/5] Add SignatureAlgorithm argument to generateCertificate Add a SignatureAlgorithm argument the generateCertificate E2E helper function so that we can support generating certificates with incompatible SHA-1 algorithms in future E2E tests. --- test/e2e/client_tls_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/e2e/client_tls_test.go b/test/e2e/client_tls_test.go index bb50212abd..cd212e42dc 100644 --- a/test/e2e/client_tls_test.go +++ b/test/e2e/client_tls_test.go @@ -61,11 +61,11 @@ func TestClientTLS(t *testing.T) { if err != nil { t.Fatalf("failed to generate client CA certificate: %v", err) } - validMatchingCert, validMatchingKey, err := generateCertificate(ca, caKey, "allowed") + validMatchingCert, validMatchingKey, err := generateCertificate(ca, caKey, "allowed", x509.SHA256WithRSA) if err != nil { t.Fatalf("failed to generate first client certificate: %v", err) } - validMismatchingCert, validMismatchingKey, err := generateCertificate(ca, caKey, "disallowed") + validMismatchingCert, validMismatchingKey, err := generateCertificate(ca, caKey, "disallowed", x509.SHA256WithRSA) if err != nil { t.Fatalf("failed to generate second client certificate: %v", err) } @@ -75,7 +75,7 @@ func TestClientTLS(t *testing.T) { if err != nil { t.Fatalf("failed to generate other CA certificate: %v", err) } - invalidMatchingCert, invalidMatchingKey, err := generateCertificate(otherCA, otherCAKey, "allowed") + invalidMatchingCert, invalidMatchingKey, err := generateCertificate(otherCA, otherCAKey, "allowed", x509.SHA256WithRSA) if err != nil { t.Fatalf("failed to generate third client certificate: %v", err) } @@ -1477,7 +1477,7 @@ func generateCA() (*x509.Certificate, *rsa.PrivateKey, error) { // generateCertificate generates and returns a certificate and key // where the certificate is signed by the provided CA certificate. -func generateCertificate(caCert *x509.Certificate, caKey *rsa.PrivateKey, cn string) (*x509.Certificate, *rsa.PrivateKey, error) { +func generateCertificate(caCert *x509.Certificate, caKey *rsa.PrivateKey, cn string, signatureAlgorithm x509.SignatureAlgorithm) (*x509.Certificate, *rsa.PrivateKey, error) { key, err := rsa.GenerateKey(rand.Reader, 2048) if err != nil { return nil, nil, err @@ -1488,7 +1488,7 @@ func generateCertificate(caCert *x509.Certificate, caKey *rsa.PrivateKey, cn str CommonName: cn, Organization: []string{"OpenShift"}, }, - SignatureAlgorithm: x509.SHA256WithRSA, + SignatureAlgorithm: signatureAlgorithm, NotBefore: time.Now().Add(-24 * time.Hour), NotAfter: time.Now().Add(24 * time.Hour), SerialNumber: big.NewInt(1), From 6778ce1a619b2f58310a037694e341e446888c55 Mon Sep 17 00:00:00 2001 From: Grant Spence Date: Tue, 11 Jun 2024 13:15:07 -0400 Subject: [PATCH 5/5] Add route upgradeable control loop for SHA-1 certs The route upgradeable control loop determines upgradeablity by searching for the UnservableInFutureVersions condition in the routes. It creates an admin-gate if the condition is found. This implemenentation targets 4.15 upgrades to 4.16 specifically and assumes any UnservableInFutureVersions route status is an upgrade blocker. Add E2E test to validate the functionality of the admin-gate as well as validating that the router adds the UnservableInFutureVersions condition for routes with SHA-1 certificates. --- pkg/operator/controller/names.go | 9 + .../controller/route-metrics/controller.go | 27 +- .../route-upgradeable/controller.go | 272 ++++++++++++++++++ pkg/operator/operator.go | 31 +- test/e2e/all_test.go | 1 + test/e2e/route_upgradeable_test.go | 99 +++++++ test/e2e/util_test.go | 26 ++ 7 files changed, 447 insertions(+), 18 deletions(-) create mode 100644 pkg/operator/controller/route-upgradeable/controller.go create mode 100644 test/e2e/route_upgradeable_test.go diff --git a/pkg/operator/controller/names.go b/pkg/operator/controller/names.go index 24923a426b..51c5857d49 100644 --- a/pkg/operator/controller/names.go +++ b/pkg/operator/controller/names.go @@ -292,3 +292,12 @@ func GatewayDNSRecordName(gateway *gatewayapiv1beta1.Gateway, host string) types Name: fmt.Sprintf("%s-%s-wildcard", gateway.Name, util.Hash(host)), } } + +// AdminGatesConfigMapName returns the namespaced name for the +// configmap that contains admin gates. +func AdminGatesConfigMapName() types.NamespacedName { + return types.NamespacedName{ + Name: "admin-gates", + Namespace: GlobalMachineSpecifiedConfigNamespace, + } +} diff --git a/pkg/operator/controller/route-metrics/controller.go b/pkg/operator/controller/route-metrics/controller.go index 227d5bd8bb..05d88630b6 100644 --- a/pkg/operator/controller/route-metrics/controller.go +++ b/pkg/operator/controller/route-metrics/controller.go @@ -39,20 +39,11 @@ var ( // New creates the route metrics controller. This is the controller // that handles all the logic for gathering and exporting // metrics related to route resources. -func New(mgr manager.Manager, namespace string) (controller.Controller, error) { - // Create a new cache to watch on Route objects from every namespace. - newCache, err := cache.New(mgr.GetConfig(), cache.Options{ - Scheme: mgr.GetScheme(), - }) - if err != nil { - return nil, err - } - // Add the cache to the manager so that the cache is started along with the other runnables. - mgr.Add(newCache) +func New(mgr manager.Manager, config Config) (controller.Controller, error) { operatorCache := mgr.GetCache() reconciler := &reconciler{ - cache: newCache, - namespace: namespace, + cache: config.Cache, + namespace: config.Namespace, routeToIngresses: make(map[types.NamespacedName]sets.String), } c, err := controller.New(controllerName, mgr, controller.Options{ @@ -74,7 +65,7 @@ func New(mgr manager.Manager, namespace string) (controller.Controller, error) { return nil, err } // add watch for changes in Route - if err := c.Watch(source.Kind(newCache, &routev1.Route{}), + if err := c.Watch(source.Kind(reconciler.cache, &routev1.Route{}), handler.EnqueueRequestsFromMapFunc(reconciler.routeToIngressController)); err != nil { return nil, err } @@ -144,6 +135,16 @@ func (r *reconciler) routeToIngressController(context context.Context, obj clien return requests } +// Config holds all the configuration that must be provided when creating the +// controller. +type Config struct { + // Namespace specifies the namespace where IngressControllers reside. + Namespace string + // Cache should be a namespace global cache since routes + // are not restricted to any particular namespace. + Cache cache.Cache +} + // reconciler handles the actual ingresscontroller reconciliation logic in response to events. type reconciler struct { cache cache.Cache diff --git a/pkg/operator/controller/route-upgradeable/controller.go b/pkg/operator/controller/route-upgradeable/controller.go new file mode 100644 index 0000000000..92c5107c58 --- /dev/null +++ b/pkg/operator/controller/route-upgradeable/controller.go @@ -0,0 +1,272 @@ +package routeupgradeable + +import ( + "context" + "fmt" + "time" + + "sigs.k8s.io/controller-runtime/pkg/cache" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller" + "sigs.k8s.io/controller-runtime/pkg/event" + "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/manager" + "sigs.k8s.io/controller-runtime/pkg/predicate" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + "sigs.k8s.io/controller-runtime/pkg/source" + + operatorv1 "github.com/openshift/api/operator/v1" + routev1 "github.com/openshift/api/route/v1" + logf "github.com/openshift/cluster-ingress-operator/pkg/log" + operatorcontroller "github.com/openshift/cluster-ingress-operator/pkg/operator/controller" + + "golang.org/x/time/rate" + corev1 "k8s.io/api/core/v1" + "k8s.io/client-go/util/workqueue" +) + +const ( + controllerName = "route_upgradeable_controller" + + // RouteConfigAdminGate415Key is the key of the admin-gate that will block upgrades from 4.15 to 4.16. + RouteConfigAdminGate415Key = "ack-4.15-route-config-not-supported-in-4.16" + + // routeConfigAdminGate415Msg is the message that will be displayed in the admin-gates configmap. + routeConfigAdminGate415Msg = "A route in this cluster contains a configuration that isn't supported in 4.16. Please review each route's UnservableInFutureVersions status condition for more information." + + // unservableInFutureVersionsIndexFieldName is the name of the index field that will list + // routes with UnservableInFutureVersions condition. + unservableInFutureVersionsIndexFieldName = "UnservableInFutureVersionsRoute" +) + +var ( + log = logf.Logger.WithName(controllerName) +) + +// New creates the route upgradeable controller. This is the controller that handles +// interpreting route status to determine if an admin gate is needed. The admin gate +// will block upgrades until an admin acks it or resolves the unsupported configuration. +func New(mgr manager.Manager, config Config) (controller.Controller, error) { + operatorCache := mgr.GetCache() + reconciler := &reconciler{ + cache: config.Cache, + client: mgr.GetClient(), + } + + c, err := controller.New(controllerName, mgr, controller.Options{ + Reconciler: reconciler, + RateLimiter: workqueue.NewMaxOfRateLimiter( + // Rate-limit to 1 update every 5 seconds per + // reconcile to avoid burning CPU if object + // updates are frequent. + workqueue.NewItemExponentialFailureRateLimiter(5*time.Second, 30*time.Second), + // 10 qps, 100 bucket size, same as DefaultControllerRateLimiter(). + &workqueue.BucketRateLimiter{Limiter: rate.NewLimiter(rate.Limit(10), 100)}, + ), + }) + if err != nil { + return nil, err + } + + // Index routes over the UnservableInFutureVersions condition so that we can + // efficiently find routes with UnservableInFutureVersions=true. + if err := reconciler.cache.IndexField(context.Background(), &routev1.Route{}, unservableInFutureVersionsIndexFieldName, client.IndexerFunc(func(o client.Object) []string { + if getRouteUnservableInFutureVersionsStatusCondition(o.(*routev1.Route)) != nil { + return []string{string(operatorv1.ConditionTrue)} + } + return nil + })); err != nil { + return nil, fmt.Errorf("failed to create index for routes: %w", err) + } + + updatedUnservableInFutureVersionsConditionFilter := predicate.Funcs{ + // Although this CreateFunc triggers for each route upon operator startup, we will rely on the + // configmap watch with the CreateFunc on adminGateFilter to trigger for the admin-gates + // configmap to ensure the initial state. + CreateFunc: func(createEvent event.CreateEvent) bool { return false }, + // The only route deletions that matter are routes that have UnservableInFutureVersions. + DeleteFunc: func(deleteEvent event.DeleteEvent) bool { + route := deleteEvent.Object.(*routev1.Route) + return getRouteUnservableInFutureVersionsStatusCondition(route) != nil + }, + // Only route updates for UnservableInFutureVersions changes matter. + UpdateFunc: func(e event.UpdateEvent) bool { + oldRoute := e.ObjectOld.(*routev1.Route) + newRoute := e.ObjectNew.(*routev1.Route) + return routeIngressConditionStatusChanged( + getRouteUnservableInFutureVersionsStatusCondition(newRoute), + getRouteUnservableInFutureVersionsStatusCondition(oldRoute), + ) + }, + // Ignore all other events. + GenericFunc: func(genericEvent event.GenericEvent) bool { return false }, + } + + adminGateFilter := predicate.Funcs{ + // Filter for only when the admin-gates configmap is created. + // This runs on operator startup, and ensures initial state for the admin-gates configmap. + CreateFunc: func(e event.CreateEvent) bool { + configmap := e.Object.(*corev1.ConfigMap) + return configmap.Name == operatorcontroller.AdminGatesConfigMapName().Name && configmap.Namespace == operatorcontroller.AdminGatesConfigMapName().Namespace + }, + // Ignore delete events because our reconcile can't do anything if + // the admin-gates configmap doesn't exist. + DeleteFunc: func(deleteEvent event.DeleteEvent) bool { return false }, + // Filter for when our specific key is updated for admin-gates configmap. + UpdateFunc: func(e event.UpdateEvent) bool { + configmapOld := e.ObjectOld.(*corev1.ConfigMap) + configmapNew := e.ObjectNew.(*corev1.ConfigMap) + if configmapOld.Name != operatorcontroller.AdminGatesConfigMapName().Name || configmapOld.Namespace != operatorcontroller.AdminGatesConfigMapName().Namespace { + return false + } + oldVal, oldExists := configmapOld.Data[RouteConfigAdminGate415Key] + newVal, newExists := configmapNew.Data[RouteConfigAdminGate415Key] + return oldExists != newExists || oldVal != newVal + }, + // Ignore all other events. + GenericFunc: func(genericEvent event.GenericEvent) bool { return false }, + } + + toAdminGateConfigMap := func(_ context.Context, _ client.Object) []reconcile.Request { + return []reconcile.Request{{ + operatorcontroller.AdminGatesConfigMapName(), + }} + } + + // Watch for routes using reconciler.cache because this cache + // includes all namespaces for watching routes. + if err := c.Watch(source.Kind(reconciler.cache, &routev1.Route{}), + handler.EnqueueRequestsFromMapFunc(toAdminGateConfigMap), + updatedUnservableInFutureVersionsConditionFilter); err != nil { + return nil, err + } + // Watch for configmap updates using the operatorCache because this cache includes + // the openshift-config-managed namespace in which our admin-gate configmap resides. + if err := c.Watch(source.Kind(operatorCache, &corev1.ConfigMap{}), + &handler.EnqueueRequestForObject{}, + adminGateFilter); err != nil { + return nil, err + } + return c, nil +} + +// reconciler handles the actual configmap reconciliation logic in response to events. +type reconciler struct { + client client.Client + cache cache.Cache +} + +// Config holds all the configuration that must be provided when creating the +// controller. +type Config struct { + // Cache should be a namespace global cache since routes + // are not restricted to any particular namespace. + Cache cache.Cache +} + +// Reconcile expects request to refer to a configmap resource. It will add +// an admin gate if any routes have the UnservableInFutureVersions status and remove the +// admin gate if no routes have the UnservableInFutureVersions status. +func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) (reconcile.Result, error) { + log.Info("reconciling", "request", request) + + unservableInFutureVersionsRoutesExists, err := r.unservableInFutureVersionsRouteExists(ctx) + if err != nil { + return reconcile.Result{}, fmt.Errorf("failed to determine if UnservableInFutureVersionsRoute routes exist: %w", err) + } + if unservableInFutureVersionsRoutesExists { + if err := r.addAdminGate(ctx); err != nil { + return reconcile.Result{}, fmt.Errorf("failed to add admin gate: %w", err) + } + } else { + if err := r.removeAdminGate(ctx); err != nil { + return reconcile.Result{}, fmt.Errorf("failed to remove admin gate: %w", err) + } + } + + return reconcile.Result{}, nil +} + +// unservableInFutureVersionsRouteExists checks if there are any routes with +// the UnservableInFutureVersions condition. It returns true if at least +// one route is found with the UnservableInFutureVersions condition. +func (r *reconciler) unservableInFutureVersionsRouteExists(ctx context.Context) (bool, error) { + // List routes using the "UnservableInFutureVersionsRoute" index which only lists UnservableInFutureVersions routes. + routeList := &routev1.RouteList{} + listOpts := client.MatchingFields(map[string]string{ + unservableInFutureVersionsIndexFieldName: string(operatorv1.ConditionTrue), + }) + if err := r.cache.List(ctx, routeList, listOpts); err != nil { + return false, fmt.Errorf("failed to list all routes: %w", err) + } + + return len(routeList.Items) != 0, nil +} + +// getRouteUnservableInFutureVersionsStatusCondition finds and returns a +// RouteIngressCondition with UnservableInFutureVersions=true for any router. +// If the UnservableInFutureVersions=true condition doesn't exist, nil is returned. +func getRouteUnservableInFutureVersionsStatusCondition(route *routev1.Route) *routev1.RouteIngressCondition { + for i := range route.Status.Ingress { + for j := range route.Status.Ingress[i].Conditions { + condition := &route.Status.Ingress[i].Conditions[j] + if condition.Type == routev1.RouteUnservableInFutureVersions && condition.Status == corev1.ConditionTrue { + return condition + } + } + } + return nil +} + +// addAdminGate adds an admin gate to block upgrades. +func (r *reconciler) addAdminGate(ctx context.Context) error { + adminGateConfigMap := &corev1.ConfigMap{} + if err := r.client.Get(ctx, operatorcontroller.AdminGatesConfigMapName(), adminGateConfigMap); err != nil { + return fmt.Errorf("failed to get configmap %s: %w", operatorcontroller.AdminGatesConfigMapName(), err) + } + + if adminGateConfigMap.Data == nil { + adminGateConfigMap.Data = map[string]string{} + } + + if val, ok := adminGateConfigMap.Data[RouteConfigAdminGate415Key]; ok && val == routeConfigAdminGate415Msg { + return nil // Exists as expected. + } + adminGateConfigMap.Data[RouteConfigAdminGate415Key] = routeConfigAdminGate415Msg + + log.Info("Adding admin gate for unservable in future versions route") + if err := r.client.Update(ctx, adminGateConfigMap); err != nil { + return fmt.Errorf("failed to update configmap %s: %w", operatorcontroller.AdminGatesConfigMapName(), err) + } + return nil +} + +// removeAdminGate removes the admin gate to unblock upgrades. +func (r *reconciler) removeAdminGate(ctx context.Context) error { + adminGateConfigMap := &corev1.ConfigMap{} + if err := r.client.Get(ctx, operatorcontroller.AdminGatesConfigMapName(), adminGateConfigMap); err != nil { + return fmt.Errorf("failed to get configmap %s: %w", operatorcontroller.AdminGatesConfigMapName(), err) + } + + if _, ok := adminGateConfigMap.Data[RouteConfigAdminGate415Key]; !ok { + // Nothing needs to be done if key doesn't exist + return nil + } + + log.Info("Removing admin gate for unservable in future versions route") + + delete(adminGateConfigMap.Data, RouteConfigAdminGate415Key) + if err := r.client.Update(ctx, adminGateConfigMap); err != nil { + return fmt.Errorf("failed to update configmap %s: %w", operatorcontroller.AdminGatesConfigMapName(), err) + } + return nil +} + +// routeIngressConditionStatusChanged detects if a RouteIngressCondition's status changed. +func routeIngressConditionStatusChanged(a, b *routev1.RouteIngressCondition) bool { + // If one is nil, then must both be nil to be unchanged. + if a == nil || b == nil { + return a != b + } + return a.Status != b.Status +} diff --git a/pkg/operator/operator.go b/pkg/operator/operator.go index 1e7601210c..bd5f980a6c 100644 --- a/pkg/operator/operator.go +++ b/pkg/operator/operator.go @@ -8,12 +8,9 @@ import ( configclient "github.com/openshift/client-go/config/clientset/versioned" configinformers "github.com/openshift/client-go/config/informers/externalversions" "github.com/openshift/library-go/pkg/operator/configobserver/featuregates" + "github.com/openshift/library-go/pkg/operator/onepodpernodeccontroller" "github.com/openshift/library-go/pkg/operator/v1helpers" - monitoringdashboard "github.com/openshift/cluster-ingress-operator/pkg/operator/controller/monitoring-dashboard" - routemetricscontroller "github.com/openshift/cluster-ingress-operator/pkg/operator/controller/route-metrics" - errorpageconfigmapcontroller "github.com/openshift/cluster-ingress-operator/pkg/operator/controller/sync-http-error-code-configmap" - "github.com/openshift/library-go/pkg/operator/onepodpernodeccontroller" corev1 "k8s.io/api/core/v1" "k8s.io/client-go/informers" "k8s.io/client-go/kubernetes" @@ -38,7 +35,11 @@ import ( ingress "github.com/openshift/cluster-ingress-operator/pkg/operator/controller/ingress" ingresscontroller "github.com/openshift/cluster-ingress-operator/pkg/operator/controller/ingress" ingressclasscontroller "github.com/openshift/cluster-ingress-operator/pkg/operator/controller/ingressclass" + monitoringdashboard "github.com/openshift/cluster-ingress-operator/pkg/operator/controller/monitoring-dashboard" + routemetricscontroller "github.com/openshift/cluster-ingress-operator/pkg/operator/controller/route-metrics" + routeupgradeablecontroller "github.com/openshift/cluster-ingress-operator/pkg/operator/controller/route-upgradeable" statuscontroller "github.com/openshift/cluster-ingress-operator/pkg/operator/controller/status" + errorpageconfigmapcontroller "github.com/openshift/cluster-ingress-operator/pkg/operator/controller/sync-http-error-code-configmap" "github.com/openshift/library-go/pkg/operator/events" "k8s.io/apimachinery/pkg/api/errors" @@ -268,8 +269,21 @@ func New(config operatorconfig.Config, kubeConfig *rest.Config) (*Operator, erro } } + // Create a new global cache to watch on Route objects from every namespace. + globalCache, err := cache.New(mgr.GetConfig(), cache.Options{ + Scheme: mgr.GetScheme(), + }) + if err != nil { + return nil, err + } + // Add the cache to the manager so that the cache is started along with the other runnables. + mgr.Add(globalCache) + // Set up the route metrics controller. - if _, err := routemetricscontroller.New(mgr, config.Namespace); err != nil { + if _, err := routemetricscontroller.New(mgr, routemetricscontroller.Config{ + Namespace: config.Namespace, + Cache: globalCache, + }); err != nil { return nil, fmt.Errorf("failed to create route metrics controller: %w", err) } @@ -310,6 +324,13 @@ func New(config operatorconfig.Config, kubeConfig *rest.Config) (*Operator, erro return nil, fmt.Errorf("failed to create gatewayapi controller: %w", err) } + // Set up the route upgradeable controller. + if _, err := routeupgradeablecontroller.New(mgr, routeupgradeablecontroller.Config{ + Cache: globalCache, + }); err != nil { + return nil, fmt.Errorf("failed to create route upgradeable controller: %w", err) + } + return &Operator{ manager: mgr, // TODO: These are only needed for the default ingress controller stuff, which diff --git a/test/e2e/all_test.go b/test/e2e/all_test.go index 14ab641b82..755b756310 100644 --- a/test/e2e/all_test.go +++ b/test/e2e/all_test.go @@ -114,5 +114,6 @@ func TestAll(t *testing.T) { t.Run("TestRouteHardStopAfterEnableOnIngressControllerHasPriorityOverIngressConfig", TestRouteHardStopAfterEnableOnIngressControllerHasPriorityOverIngressConfig) t.Run("TestHostNetworkPortBinding", TestHostNetworkPortBinding) t.Run("TestDashboardCreation", TestDashboardCreation) + t.Run("TestRouteNotUpgradeableWithSha1", TestRouteNotUpgradeableWithSha1) }) } diff --git a/test/e2e/route_upgradeable_test.go b/test/e2e/route_upgradeable_test.go new file mode 100644 index 0000000000..85465d0012 --- /dev/null +++ b/test/e2e/route_upgradeable_test.go @@ -0,0 +1,99 @@ +//go:build e2e +// +build e2e + +package e2e + +import ( + "context" + "crypto/x509" + "testing" + "time" + + routev1 "github.com/openshift/api/route/v1" + operatorcontroller "github.com/openshift/cluster-ingress-operator/pkg/operator/controller" + routeupgradeable "github.com/openshift/cluster-ingress-operator/pkg/operator/controller/route-upgradeable" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" +) + +const ( + waitForAdminGateInterval = 3 * time.Second + waitForAdminGateTimeout = 3 * time.Minute +) + +// TestRouteNotUpgradeableWithSha1 tests the route-upgradeable control loop by creating a route with a SHA1 certificate, +// expecting the Ingress Operator to create an admin gate, and update the route to SHA256 certificate to resolve the +// issue. +// +// This is a serial test because adding an admin gate could potentially interfere with other upgrade-related tests. +// Also, other tests create and delete router deployments, which would interfere with the status of the route that +// this test creates and updates. +func TestRouteNotUpgradeableWithSha1(t *testing.T) { + // Make sure admin gate doesn't exist before starting test. + if err := waitForAdminGate(t, routeupgradeable.RouteConfigAdminGate415Key, false, waitForAdminGateInterval, waitForAdminGateTimeout); err != nil { + t.Fatalf("failed to observe initial admin gate value: %v", err) + } + + // Generate SHA1 certificates + ca, caKey, err := generateCA() + if err != nil { + t.Fatalf("failed to generate client CA certificate: %v", err) + } + certSha1, keySha1, err := generateCertificate(ca, caKey, "test", x509.SHA1WithRSA) + if err != nil { + t.Fatalf("failed to generate SHA1 certificate: %v", err) + } + + t.Log("creating SHA1 route to add admin gate") + routeSha1Name := types.NamespacedName{Name: "route-sha1", Namespace: operatorcontroller.DefaultOperandNamespace} + routeSha1 := &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: routeSha1Name.Name, + Namespace: routeSha1Name.Namespace, + }, + Spec: routev1.RouteSpec{ + To: routev1.RouteTargetReference{ + Kind: "Service", + Name: "foo", + }, + TLS: &routev1.TLSConfig{ + Termination: routev1.TLSTerminationEdge, + Certificate: encodeCert(certSha1), + Key: encodeKey(keySha1), + }, + }, + } + if err := kclient.Create(context.Background(), routeSha1); err != nil { + t.Fatalf("failed to create route %s: %v", routeSha1Name, err) + } + t.Cleanup(func() { + if err := kclient.Delete(context.Background(), routeSha1); err != nil { + t.Fatalf("failed to delete route %s: %v", routeSha1Name, err) + } + }) + + // Poll for the admin gate, and wait until it is True. + if err := waitForAdminGate(t, routeupgradeable.RouteConfigAdminGate415Key, true, waitForAdminGateInterval, waitForAdminGateTimeout); err != nil { + t.Fatalf("admin gate observation error: %v", err) + } + + // Now, resolve the route's certificate problem by replacing with supported SHA256 certs. + t.Log("resolving SHA1 route issue to remove admin gate") + certSha256, keySha256, err := generateCertificate(ca, caKey, "test", x509.SHA256WithRSA) + if err != nil { + t.Fatalf("failed to generate SHA256 certificate: %v", err) + } + + if err := kclient.Get(context.Background(), routeSha1Name, routeSha1); err != nil { + t.Fatalf("failed to get route %s: %v", routeSha1Name, err) + } + routeSha1.Spec.TLS.Certificate = encodeCert(certSha256) + routeSha1.Spec.TLS.Key = encodeKey(keySha256) + if err := kclient.Update(context.Background(), routeSha1); err != nil { + t.Fatalf("failed to update route %s: %v", routeSha1Name, err) + } + if err := waitForAdminGate(t, routeupgradeable.RouteConfigAdminGate415Key, false, waitForAdminGateInterval, waitForAdminGateTimeout); err != nil { + t.Fatalf("admin gate observation error: %v", err) + } +} diff --git a/test/e2e/util_test.go b/test/e2e/util_test.go index 9b96627b4b..56f8e0d620 100644 --- a/test/e2e/util_test.go +++ b/test/e2e/util_test.go @@ -17,6 +17,7 @@ import ( configv1 "github.com/openshift/api/config/v1" operatorv1 "github.com/openshift/api/operator/v1" routev1 "github.com/openshift/api/route/v1" + operatorcontroller "github.com/openshift/cluster-ingress-operator/pkg/operator/controller" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" @@ -843,3 +844,28 @@ func createNamespace(t *testing.T, name string) *corev1.Namespace { return ns } + +// waitForAdminGate waits for an admin gate specified by adminGateKey +// to either exist or not exist based on the expectExists argument. +func waitForAdminGate(t *testing.T, adminGateKey string, expectExists bool, interval, timeout time.Duration) error { + t.Helper() + err := wait.PollUntilContextTimeout(context.Background(), interval, timeout, false, func(ctx context.Context) (bool, error) { + adminGateConfigMap := &corev1.ConfigMap{} + if err := kclient.Get(ctx, operatorcontroller.AdminGatesConfigMapName(), adminGateConfigMap); err != nil { + t.Logf("failed to get configmap %s: %v", operatorcontroller.AdminGatesConfigMapName(), err) + return false, nil + } + + if _, ok := adminGateConfigMap.Data[adminGateKey]; ok == expectExists { + return true, nil + } + + t.Logf("waiting for admin gate %q to become %t", adminGateKey, expectExists) + return false, nil + }) + if err != nil { + return fmt.Errorf("failed to observe admin gate %q as %t: %w", adminGateKey, expectExists, err) + } + t.Logf("admin gate %q is %t as expected", adminGateKey, expectExists) + return nil +}