diff --git a/Containerfile.control-plane b/Containerfile.control-plane index 04e8820426fa..33da61a23cd5 100644 --- a/Containerfile.control-plane +++ b/Containerfile.control-plane @@ -22,6 +22,7 @@ LABEL io.openshift.hypershift.control-plane-operator-manages-ignition-server=tru LABEL io.openshift.hypershift.control-plane-operator-manages.cluster-machine-approver=true LABEL io.openshift.hypershift.control-plane-operator-manages.cluster-autoscaler=true LABEL io.openshift.hypershift.control-plane-operator-manages.decompress-decode-config=true +LABEL io.openshift.hypershift.control-plane-operator-manages.rendered-trust-bundle-validation=true LABEL io.openshift.hypershift.control-plane-operator-creates-aws-sg=true LABEL io.openshift.hypershift.control-plane-operator-applies-management-kas-network-policy-label=true LABEL io.openshift.hypershift.restricted-psa=true diff --git a/Dockerfile b/Dockerfile index cfde0c7b28a0..8e3a853697ad 100644 --- a/Dockerfile +++ b/Dockerfile @@ -28,6 +28,7 @@ LABEL io.openshift.hypershift.control-plane-operator-manages-ignition-server=tru LABEL io.openshift.hypershift.control-plane-operator-manages.cluster-machine-approver=true LABEL io.openshift.hypershift.control-plane-operator-manages.cluster-autoscaler=true LABEL io.openshift.hypershift.control-plane-operator-manages.decompress-decode-config=true +LABEL io.openshift.hypershift.control-plane-operator-manages.rendered-trust-bundle-validation=true LABEL io.openshift.hypershift.control-plane-operator-creates-aws-sg=true LABEL io.openshift.hypershift.control-plane-operator-applies-management-kas-network-policy-label=true LABEL io.openshift.hypershift.restricted-psa=true diff --git a/Dockerfile.control-plane b/Dockerfile.control-plane index fdea2e18c84a..8e94db6eadad 100644 --- a/Dockerfile.control-plane +++ b/Dockerfile.control-plane @@ -22,6 +22,7 @@ LABEL io.openshift.hypershift.control-plane-operator-manages-ignition-server=tru LABEL io.openshift.hypershift.control-plane-operator-manages.cluster-machine-approver=true LABEL io.openshift.hypershift.control-plane-operator-manages.cluster-autoscaler=true LABEL io.openshift.hypershift.control-plane-operator-manages.decompress-decode-config=true +LABEL io.openshift.hypershift.control-plane-operator-manages.rendered-trust-bundle-validation=true LABEL io.openshift.hypershift.control-plane-operator-creates-aws-sg=true LABEL io.openshift.hypershift.control-plane-operator-applies-management-kas-network-policy-label=true LABEL io.openshift.hypershift.restricted-psa=true diff --git a/Dockerfile.dev b/Dockerfile.dev index 3789aaaabe4d..b305e461d5bd 100644 --- a/Dockerfile.dev +++ b/Dockerfile.dev @@ -35,6 +35,7 @@ LABEL io.openshift.hypershift.control-plane-operator-manages-ignition-server=tru LABEL io.openshift.hypershift.control-plane-operator-manages.cluster-machine-approver=true LABEL io.openshift.hypershift.control-plane-operator-manages.cluster-autoscaler=true LABEL io.openshift.hypershift.control-plane-operator-manages.decompress-decode-config=true +LABEL io.openshift.hypershift.control-plane-operator-manages.rendered-trust-bundle-validation=true LABEL io.openshift.hypershift.control-plane-operator-creates-aws-sg=true LABEL io.openshift.hypershift.control-plane-operator-applies-management-kas-network-policy-label=true LABEL io.openshift.hypershift.restricted-psa=true diff --git a/docs/content/reference/aggregated-docs.md b/docs/content/reference/aggregated-docs.md index 26646c2c6767..ba36d4ec0127 100644 --- a/docs/content/reference/aggregated-docs.md +++ b/docs/content/reference/aggregated-docs.md @@ -69200,7 +69200,38 @@ Changes to the following fields alter the configuration hash that the controller - **`HostedCluster.spec.pullSecret`** — a change in the **name** of the referenced Secret triggers a rollout. Changing the content of the Secret without changing the name does not trigger a rollout. -- **`HostedCluster.spec.additionalTrustBundle`** — same behavior as `pullSecret`: only a change in the referenced ConfigMap **name** triggers a rollout. +- **`HostedCluster.spec.additionalTrustBundle`** — changing the `ca-bundle.crt` content triggers a rollout across all NodePools. Switching to another ConfigMap with identical content does not itself trigger a rollout. + +- **`HostedCluster.spec.configuration.proxy.trustedCA`** — changing the referenced ConfigMap **name** or the `ca-bundle.crt` content triggers a rollout across all NodePools. + +Trust-bundle reconciliation requires a CPO/ignition-server image advertising +`io.openshift.hypershift.control-plane-operator-manages.rendered-trust-bundle-validation`. +The ignition server checks the hash of the actual combined additional and proxy CA +bundle rendered for the MCO before caching a payload. A delayed ConfigMap update +therefore delays payload generation instead of installing stale certificates. +Older CPO images need this validation backported (and the capability label added); +upgrading only the HyperShift Operator is insufficient. With an unsupported CPO, +trust-bundle reconciliation stops before bootstrap changes or old-token cleanup. + +During formula-only migration, Karpenter retains its existing bootstrap/drift +identity and persists a separate versioned content baseline on its durable +NodeClass resource. Later reconciles compare content against that baseline, +including after a controller restart. A subsequent CA change produces a new +identity and token. Regular NodePools persist their baseline on the NodePool; +the migration comparison is independent of the infrastructure platform. + +!!! note "HyperShift Operator upgrade and trust-bundle hashing" + + When upgrading to a HyperShift Operator that hashes trust-bundle ConfigMap **content** + (instead of only the ConfigMap name), NodePools that already reference a trust bundle do + **not** roll solely because of that formula change. On the first reconcile after upgrade, + the controller seeds the versioned rollout baseline + (`nodePoolCurrentRolloutConfigV2`) and records `nodePoolConfigHashVersion=v2`. + It retains the completed payload hash and the bootstrap Secret references. + CA edits made before this first reconcile are absorbed into the baseline. + Subsequent CA-content or other rollout-relevant spec changes trigger a rollout; + management-only image changes do not. In-progress rollouts retain their deployed + target, while a newer release request can still supersede it. - **`HostedCluster.spec.imageContentSources`** — changes to image content source policies managed at the HostedCluster level produce an additional core ignition config that alters the configuration hash. @@ -69337,7 +69368,8 @@ These conditions are set to `True` while the corresponding rollout is in progres | `NodePool.spec.config` | Yes | The changed NodePool | | `NodePool.spec.tuningConfig` | Yes | The changed NodePool | | `HostedCluster.spec.pullSecret` (name change) | Yes | All NodePools | -| `HostedCluster.spec.additionalTrustBundle` (name change) | Yes | All NodePools | +| `HostedCluster.spec.additionalTrustBundle` (content change) | Yes | All NodePools | +| `HostedCluster.spec.configuration.proxy.trustedCA` (name or content change) | Yes | All NodePools | | `HostedCluster.spec.imageContentSources` | Yes | All NodePools | | `HostedCluster.spec.configuration.proxy` | Yes | All NodePools | | `HostedCluster.spec.configuration.image` | Yes | All NodePools | diff --git a/docs/content/reference/nodepool-rollouts.md b/docs/content/reference/nodepool-rollouts.md index 4917cdbfa0f4..7a18348afb6e 100644 --- a/docs/content/reference/nodepool-rollouts.md +++ b/docs/content/reference/nodepool-rollouts.md @@ -34,7 +34,38 @@ Changes to the following fields alter the configuration hash that the controller - **`HostedCluster.spec.pullSecret`** — a change in the **name** of the referenced Secret triggers a rollout. Changing the content of the Secret without changing the name does not trigger a rollout. -- **`HostedCluster.spec.additionalTrustBundle`** — same behavior as `pullSecret`: only a change in the referenced ConfigMap **name** triggers a rollout. +- **`HostedCluster.spec.additionalTrustBundle`** — changing the `ca-bundle.crt` content triggers a rollout across all NodePools. Switching to another ConfigMap with identical content does not itself trigger a rollout. + +- **`HostedCluster.spec.configuration.proxy.trustedCA`** — changing the referenced ConfigMap **name** or the `ca-bundle.crt` content triggers a rollout across all NodePools. + +Trust-bundle reconciliation requires a CPO/ignition-server image advertising +`io.openshift.hypershift.control-plane-operator-manages.rendered-trust-bundle-validation`. +The ignition server checks the hash of the actual combined additional and proxy CA +bundle rendered for the MCO before caching a payload. A delayed ConfigMap update +therefore delays payload generation instead of installing stale certificates. +Older CPO images need this validation backported (and the capability label added); +upgrading only the HyperShift Operator is insufficient. With an unsupported CPO, +trust-bundle reconciliation stops before bootstrap changes or old-token cleanup. + +During formula-only migration, Karpenter retains its existing bootstrap/drift +identity and persists a separate versioned content baseline on its durable +NodeClass resource. Later reconciles compare content against that baseline, +including after a controller restart. A subsequent CA change produces a new +identity and token. Regular NodePools persist their baseline on the NodePool; +the migration comparison is independent of the infrastructure platform. + +!!! note "HyperShift Operator upgrade and trust-bundle hashing" + + When upgrading to a HyperShift Operator that hashes trust-bundle ConfigMap **content** + (instead of only the ConfigMap name), NodePools that already reference a trust bundle do + **not** roll solely because of that formula change. On the first reconcile after upgrade, + the controller seeds the versioned rollout baseline + (`nodePoolCurrentRolloutConfigV2`) and records `nodePoolConfigHashVersion=v2`. + It retains the completed payload hash and the bootstrap Secret references. + CA edits made before this first reconcile are absorbed into the baseline. + Subsequent CA-content or other rollout-relevant spec changes trigger a rollout; + management-only image changes do not. In-progress rollouts retain their deployed + target, while a newer release request can still supersede it. - **`HostedCluster.spec.imageContentSources`** — changes to image content source policies managed at the HostedCluster level produce an additional core ignition config that alters the configuration hash. @@ -171,7 +202,8 @@ These conditions are set to `True` while the corresponding rollout is in progres | `NodePool.spec.config` | Yes | The changed NodePool | | `NodePool.spec.tuningConfig` | Yes | The changed NodePool | | `HostedCluster.spec.pullSecret` (name change) | Yes | All NodePools | -| `HostedCluster.spec.additionalTrustBundle` (name change) | Yes | All NodePools | +| `HostedCluster.spec.additionalTrustBundle` (content change) | Yes | All NodePools | +| `HostedCluster.spec.configuration.proxy.trustedCA` (name or content change) | Yes | All NodePools | | `HostedCluster.spec.imageContentSources` | Yes | All NodePools | | `HostedCluster.spec.configuration.proxy` | Yes | All NodePools | | `HostedCluster.spec.configuration.image` | Yes | All NodePools | diff --git a/hack/tools/hypershiftlinter/analyzers/e2eutilallowlist/allowlist.go b/hack/tools/hypershiftlinter/analyzers/e2eutilallowlist/allowlist.go index e6c5921e6ec8..4ad65451a1b5 100644 --- a/hack/tools/hypershiftlinter/analyzers/e2eutilallowlist/allowlist.go +++ b/hack/tools/hypershiftlinter/analyzers/e2eutilallowlist/allowlist.go @@ -722,26 +722,6 @@ var allowlist = map[string]map[string][]string{ "WaitForReadyNodesByNodePool", "WithTimeout", }, - "NodePoolTrustBundleTest/It(\"should propagate and remove additional trust bundle to/from the hosted cluster\")#1": { - "Condition", - "Condition.Status", - "Condition.Type", - "ConditionPredicate", - "EventuallyNotFound", - "EventuallyObject", - "NameGenerator.GenerateName", - "Predicate", - "SimpleNameGenerator", - "UpdateObject", - "Version418", - "Version422", - "WaitForReadyNodesByNodePool", - "WithInterval", - "WithTimeout", - }, - "NodePoolTrustBundleTest/It(\"should propagate and remove additional trust bundle to/from the hosted cluster\")#1/DeferCleanup()#3": { - "UpdateObject", - }, "buildMachineConfigVerificationDaemonSet": { "NameGenerator.GenerateName", "SimpleNameGenerator", diff --git a/hypershift-operator/controllers/nodepool/annotation_seeding_test.go b/hypershift-operator/controllers/nodepool/annotation_seeding_test.go index dfa5ebae6c15..5f6cd0fba007 100644 --- a/hypershift-operator/controllers/nodepool/annotation_seeding_test.go +++ b/hypershift-operator/controllers/nodepool/annotation_seeding_test.go @@ -14,6 +14,7 @@ import ( ) func TestSeedRolloutAnnotation(t *testing.T) { + t.Run("When trust bundle hashing changes, it should preserve migration and subsequent rollout behavior", testSeedRolloutAnnotationTrustBundleMigration) config := &rolloutConfig{ releaseImage: &releaseinfo.ReleaseImage{ImageStream: &imagev1.ImageStream{ObjectMeta: metav1.ObjectMeta{Name: "4.18.5"}}}, } @@ -96,6 +97,7 @@ func TestSeedRolloutAnnotation(t *testing.T) { deployedBootstrapHash: tc.deployedHash, } seedRolloutAnnotation(token) + tc.expected[nodePoolAnnotationConfigHashVersion] = CurrentConfigHashVersion NewWithT(t).Expect(nodePool.Annotations).To(Equal(tc.expected)) }) } diff --git a/hypershift-operator/controllers/nodepool/capi_test.go b/hypershift-operator/controllers/nodepool/capi_test.go index 4e5c7ee26994..30659e4e68fe 100644 --- a/hypershift-operator/controllers/nodepool/capi_test.go +++ b/hypershift-operator/controllers/nodepool/capi_test.go @@ -3005,7 +3005,7 @@ func TestPropagateVersionAndTemplate(t *testing.T) { rc := &rolloutConfig{ rolloutMcoRawConfig: "user-config", pullSecretName: "pull", - additionalTrustBundleName: "trust", + additionalTrustBundleHash: "trust", rolloutGlobalConfig: "global", mcoRawConfig: "old-haproxy\n---\nuser-config", haproxyRawConfig: "old-haproxy", @@ -3241,7 +3241,7 @@ func TestPropagateVersionAndTemplateToMachineSet(t *testing.T) { rc := &rolloutConfig{ rolloutMcoRawConfig: "user-config", pullSecretName: "pull", - additionalTrustBundleName: "trust", + additionalTrustBundleHash: "trust", rolloutGlobalConfig: "global", mcoRawConfig: "old-haproxy\n---\nuser-config", haproxyRawConfig: "old-haproxy", @@ -3869,7 +3869,7 @@ func TestReconcileMachineSetStatus(t *testing.T) { rc := &rolloutConfig{ rolloutMcoRawConfig: "user-config", pullSecretName: "pull", - additionalTrustBundleName: "trust", + additionalTrustBundleHash: "trust", rolloutGlobalConfig: "global", mcoRawConfig: "old-haproxy\n---\nuser-config", haproxyRawConfig: "old-haproxy", diff --git a/hypershift-operator/controllers/nodepool/conditions.go b/hypershift-operator/controllers/nodepool/conditions.go index 78547abed6b7..f9b29fc915a4 100644 --- a/hypershift-operator/controllers/nodepool/conditions.go +++ b/hypershift-operator/controllers/nodepool/conditions.go @@ -2,6 +2,7 @@ package nodepool import ( "context" + coreerrors "errors" "fmt" "net/netip" "strconv" @@ -428,14 +429,13 @@ func (r *NodePoolReconciler) validMachineConfigCondition(ctx context.Context, no } _, err = NewConfigGenerator(ctx, r.Client, hcluster, nodePool, releaseImage, haproxyRawConfig, controlPlaneNamespace, resolvedRHELStream) if err != nil { - SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{ - Type: hyperv1.NodePoolValidMachineConfigConditionType, - Status: corev1.ConditionFalse, - Reason: hyperv1.NodePoolValidationFailedReason, - Message: err.Error(), - ObservedGeneration: nodePool.Generation, - }) - return &ctrl.Result{}, fmt.Errorf("failed to generate config: %w", err) + result, condErr := setValidMachineConfigConditionForError(nodePool, err) + if condErr == nil { + // Trust-bundle ConfigMap problems soft-fail; ValidMachineConfig=False already + // surfaces the problem and ConfigMap watches requeue when restored. + log.Info("HostedCluster trust bundle ConfigMap is missing or invalid", "error", err.Error()) + } + return result, condErr } SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{ @@ -448,6 +448,32 @@ func (r *NodePoolReconciler) validMachineConfigCondition(ctx context.Context, no return nil, nil } +// setValidMachineConfigConditionForError sets ValidMachineConfig=False for a config generation error. +// Missing or malformed HostedCluster trust-bundle ConfigMaps soft-fail (nil error) so transient +// delete/recreate windows do not hard-fail NodePool reconciliation. Other config errors still +// return an error for exponential backoff. +func setValidMachineConfigConditionForError(nodePool *hyperv1.NodePool, err error) (*ctrl.Result, error) { + reason := hyperv1.NodePoolValidationFailedReason + var trustBundleErr *TrustBundleConfigError + softFail := coreerrors.As(err, &trustBundleErr) + if softFail && apierrors.IsNotFound(err) { + reason = hyperv1.NodePoolNotFoundReason + } + + SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{ + Type: hyperv1.NodePoolValidMachineConfigConditionType, + Status: corev1.ConditionFalse, + Reason: reason, + Message: err.Error(), + ObservedGeneration: nodePool.Generation, + }) + if softFail { + // Reconciling won't solve the input problem; a ConfigMap create/update will requeue. + return &ctrl.Result{}, nil + } + return &ctrl.Result{}, fmt.Errorf("failed to generate config: %w", err) +} + func (r *NodePoolReconciler) updatingConfigCondition(ctx context.Context, nodePool *hyperv1.NodePool, hcluster *hyperv1.HostedCluster) (*ctrl.Result, error) { log := ctrl.LoggerFrom(ctx) token, err := r.token(ctx, hcluster, nodePool) diff --git a/hypershift-operator/controllers/nodepool/conditions_test.go b/hypershift-operator/controllers/nodepool/conditions_test.go index 83ea417d58d1..3c2abde11044 100644 --- a/hypershift-operator/controllers/nodepool/conditions_test.go +++ b/hypershift-operator/controllers/nodepool/conditions_test.go @@ -19,7 +19,9 @@ import ( "github.com/openshift/api/image/docker10" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/utils/ptr" "sigs.k8s.io/cluster-api/api/core/v1beta2" @@ -29,6 +31,60 @@ import ( "github.com/blang/semver" ) +func TestSetValidMachineConfigConditionForError(t *testing.T) { + testCases := []struct { + name string + err error + expectedReason string + expectError bool + }{ + { + name: "When trust bundle ConfigMap is not found, it should soft-fail with NotFound reason", + err: &TrustBundleConfigError{err: fmt.Errorf("cannot get ConfigMap clusters/user-ca: %w", + apierrors.NewNotFound(schema.GroupResource{Resource: "configmaps"}, "user-ca"))}, + expectedReason: hyperv1.NodePoolNotFoundReason, + expectError: false, + }, + { + name: "When trust bundle ConfigMap is missing ca-bundle.crt, it should soft-fail with ValidationFailed reason", + err: &TrustBundleConfigError{err: fmt.Errorf("ConfigMap clusters/user-ca missing %q key", + "ca-bundle.crt")}, + expectedReason: hyperv1.NodePoolValidationFailedReason, + expectError: false, + }, + { + name: "When a non-trust-bundle config error occurs, it should hard-fail with ValidationFailed reason", + err: fmt.Errorf("expected 3 core ignition configs, found 0"), + expectedReason: hyperv1.NodePoolValidationFailedReason, + expectError: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + nodePool := &hyperv1.NodePool{ + ObjectMeta: metav1.ObjectMeta{Name: "workers", Namespace: "clusters", Generation: 2}, + } + + result, err := setValidMachineConfigConditionForError(nodePool, tc.err) + g.Expect(result).NotTo(BeNil()) + if tc.expectError { + g.Expect(err).To(HaveOccurred()) + } else { + g.Expect(err).NotTo(HaveOccurred()) + } + + condition := FindStatusCondition(nodePool.Status.Conditions, hyperv1.NodePoolValidMachineConfigConditionType) + g.Expect(condition).NotTo(BeNil()) + g.Expect(condition.Status).To(Equal(corev1.ConditionFalse)) + g.Expect(condition.Reason).To(Equal(tc.expectedReason)) + g.Expect(condition.Message).To(Equal(tc.err.Error())) + g.Expect(condition.ObservedGeneration).To(Equal(nodePool.Generation)) + }) + } +} + func TestIgnitionEndpointAvailableCondition(t *testing.T) { const ignitionHostname = "ignition-example.service.hypershift.example.com" diff --git a/hypershift-operator/controllers/nodepool/config.go b/hypershift-operator/controllers/nodepool/config.go index 3d9a3a70a41c..c70b723b2fa6 100644 --- a/hypershift-operator/controllers/nodepool/config.go +++ b/hypershift-operator/controllers/nodepool/config.go @@ -18,6 +18,7 @@ import ( "github.com/openshift/hypershift/support/api" "github.com/openshift/hypershift/support/backwardcompat" "github.com/openshift/hypershift/support/capabilities" + "github.com/openshift/hypershift/support/certs" "github.com/openshift/hypershift/support/globalconfig" "github.com/openshift/hypershift/support/netutil" "github.com/openshift/hypershift/support/releaseinfo" @@ -56,17 +57,21 @@ type ConfigGenerator struct { // This field is intentionally outside rolloutConfig because it does not // participate in the config hash that drives rollouts. resolvedRHELStreamForBootImage string + // renderedTrustBundleHash is computed from the same source snapshot as rollout hashes. + // It validates the exact aggregate passed to the MCO, including bundle removal. + renderedTrustBundleHash string *rolloutConfig } // rolloutConfig is the canonical source for input that produces a unique hash id and causes a NodePool rollout. // This can be grouped by two categories of input based on how it's consumed by the MCO: -// - Some fields from spec like hostedCluster.Spec.Config, pullSecretName, additionalTrustBundleName... +// - Some fields from spec like hostedCluster.Spec.Config, pullSecretName, additionalTrustBundleHash, proxyTrustedCAHash... // - The mcoRawConfig, which is an MCO consumable version of NodePool.spec.config, tuneConfig and any hypershift core machineConfig. type rolloutConfig struct { releaseImage *releaseinfo.ReleaseImage pullSecretName string - additionalTrustBundleName string + additionalTrustBundleHash string + proxyTrustedCAHash string // globalConfig represents input from hostedCluster.spec.config that requires a NodePool rollout. globalConfig string // rolloutGlobalConfig is the global config derived only from user-set spec fields @@ -165,8 +170,15 @@ func NewConfigGenerator(ctx context.Context, client client.Client, hostedCluster }, } - if hostedCluster.Spec.AdditionalTrustBundle != nil { - cg.rolloutConfig.additionalTrustBundleName = hostedCluster.Spec.AdditionalTrustBundle.Name + additionalTrustBundleHash, proxyTrustedCAHash, err := rolloutTrustBundleHashes(ctx, client, hostedCluster) + if err != nil { + return nil, err + } + cg.rolloutConfig.additionalTrustBundleHash = additionalTrustBundleHash + cg.rolloutConfig.proxyTrustedCAHash = proxyTrustedCAHash + cg.renderedTrustBundleHash, err = renderedTrustBundleHash(ctx, client, hostedCluster, additionalTrustBundleHash, proxyTrustedCAHash) + if err != nil { + return nil, err } mcoRawConfig, rolloutMcoRawConfig, err := cg.generateMCORawConfig(ctx, hostedCluster.Spec.Capabilities) @@ -190,11 +202,10 @@ func (cg *ConfigGenerator) CompressedAndEncoded() (*bytes.Buffer, error) { return supportutil.CompressAndEncode([]byte(cg.mcoRawConfig)) } -// Hash returns a unique hash id for any NodePool API input that requires a NodePool rollout, i.e. the rolloutConfig struct. -// TODO(alberto): hash the struct directly instead of the string representation field by field. -// This is kept like this for now to contain the scope of the refactor and avoid backward compatibility issues. +// Hash identifies a rendered payload. RolloutHash, not this full hash, decides +// whether workers need a rollout. func (cg *ConfigGenerator) Hash() string { - return supportutil.HashSimple(cg.mcoRawConfig + cg.releaseImage.Version() + cg.pullSecretName + cg.additionalTrustBundleName + cg.globalConfig + cg.rhelStream) + return cg.hashAtVersion(CurrentConfigHashVersion) } // HashWithOutVersion is like Hash but doesn't compute the release version. @@ -202,7 +213,125 @@ func (cg *ConfigGenerator) Hash() string { // TODO(alberto): This was left inconsistent in https://github.com/openshift/hypershift/pull/3795/files. It should also contain cg.globalConfig. // This is kept like this for now to contain the scope of the refactor and avoid backward compatibility issues. func (cg *ConfigGenerator) HashWithoutVersion() string { - return supportutil.HashSimple(cg.mcoRawConfig + cg.pullSecretName + cg.additionalTrustBundleName + cg.rhelStream) + return cg.hashWithoutVersionAtVersion(CurrentConfigHashVersion) +} + +func (cg *ConfigGenerator) hashAtVersion(version string) string { + switch version { + case ConfigHashVersionV1: + atbName := additionalTrustBundleName(cg.hostedCluster) + return supportutil.HashSimple(cg.mcoRawConfig + cg.releaseImage.Version() + cg.pullSecretName + atbName + cg.globalConfig + cg.rhelStream) + case ConfigHashVersionV2: + return supportutil.HashSimple(cg.mcoRawConfig + cg.releaseImage.Version() + cg.pullSecretName + cg.additionalTrustBundleHash + cg.proxyTrustedCAHash + cg.globalConfig + cg.rhelStream) + default: + return cg.hashAtVersion(CurrentConfigHashVersion) + } +} + +func (cg *ConfigGenerator) hashWithoutVersionAtVersion(version string) string { + switch version { + case ConfigHashVersionV1: + atbName := additionalTrustBundleName(cg.hostedCluster) + return supportutil.HashSimple(cg.mcoRawConfig + cg.pullSecretName + atbName + cg.rhelStream) + case ConfigHashVersionV2: + return supportutil.HashSimple(cg.mcoRawConfig + cg.pullSecretName + cg.additionalTrustBundleHash + cg.proxyTrustedCAHash + cg.rhelStream) + default: + return cg.hashWithoutVersionAtVersion(CurrentConfigHashVersion) + } +} + +func additionalTrustBundleName(hc *hyperv1.HostedCluster) string { + if hc != nil && hc.Spec.AdditionalTrustBundle != nil { + return hc.Spec.AdditionalTrustBundle.Name + } + return "" +} + +// TrustBundleConfigError indicates a HostedCluster-referenced trust-bundle ConfigMap +// (additionalTrustBundle or proxy.trustedCA) is missing or malformed. Callers should +// surface this as ValidMachineConfig=False and soft-fail reconcile so transient +// ConfigMap delete/recreate windows do not hard-fail the NodePool reconcile loop. +type TrustBundleConfigError struct { + err error +} + +func (e *TrustBundleConfigError) Error() string { + return e.err.Error() +} + +func (e *TrustBundleConfigError) Unwrap() error { + return e.err +} + +func rolloutTrustBundleHashes(ctx context.Context, c client.Client, hc *hyperv1.HostedCluster) (additionalTrustBundleHash, proxyTrustedCAHash string, err error) { + if hc.Spec.AdditionalTrustBundle != nil { + additionalTrustBundleHash, err = configMapCABundleHash(ctx, c, hc.Namespace, hc.Spec.AdditionalTrustBundle.Name) + if err != nil { + return "", "", err + } + } + if hc.Spec.Configuration != nil && hc.Spec.Configuration.Proxy != nil && hc.Spec.Configuration.Proxy.TrustedCA.Name != "" { + proxyTrustedCAHash, err = configMapCABundleHash(ctx, c, hc.Namespace, hc.Spec.Configuration.Proxy.TrustedCA.Name) + if err != nil { + return "", "", err + } + } + return additionalTrustBundleHash, proxyTrustedCAHash, nil +} + +// getConfigMapWithCABundle fetches a ConfigMap that must contain certs.UserCABundleMapKey. +// Used by both rollout content hashing and getAdditionalTrustBundle. +func getConfigMapWithCABundle(ctx context.Context, c client.Client, namespace, name string) (*corev1.ConfigMap, error) { + cm := &corev1.ConfigMap{} + if err := c.Get(ctx, client.ObjectKey{Namespace: namespace, Name: name}, cm); err != nil { + return nil, err + } + if _, ok := cm.Data[certs.UserCABundleMapKey]; !ok { + return nil, fmt.Errorf("ConfigMap %s/%s missing %q key", namespace, name, certs.UserCABundleMapKey) + } + return cm, nil +} + +func configMapCABundleHash(ctx context.Context, c client.Client, namespace, name string) (string, error) { + cm, err := getConfigMapWithCABundle(ctx, c, namespace, name) + if err != nil { + return "", &TrustBundleConfigError{err: fmt.Errorf("cannot get ConfigMap %s/%s: %w", namespace, name, err)} + } + return supportutil.HashSimple(cm.Data[certs.UserCABundleMapKey]), nil +} + +func renderedTrustBundleHash(ctx context.Context, c client.Client, hc *hyperv1.HostedCluster, additionalHash, proxyHash string) (string, error) { + refs := []struct{ name, hash string }{{additionalTrustBundleName(hc), additionalHash}} + if hc.Spec.Configuration != nil && hc.Spec.Configuration.Proxy != nil { + refs = append(refs, struct{ name, hash string }{hc.Spec.Configuration.Proxy.TrustedCA.Name, proxyHash}) + } + var bundle string + for _, ref := range refs { + if ref.name == "" { + continue + } + cm, err := getConfigMapWithCABundle(ctx, c, hc.Namespace, ref.name) + if err != nil { + return "", &TrustBundleConfigError{err: err} + } + data := cm.Data[certs.UserCABundleMapKey] + if supportutil.HashSimple(data) != ref.hash { + return "", &TrustBundleConfigError{err: fmt.Errorf("ConfigMap %s/%s changed while computing trust bundle hashes; retrying", hc.Namespace, ref.name)} + } + // Match CPO's additional-then-proxy concatenation, including duplicate refs. + bundle += data + } + return supportutil.HashSimple(bundle), nil +} + +func hostedClusterReferencesConfigMap(hc *hyperv1.HostedCluster, configMapName string) bool { + if hc.Spec.AdditionalTrustBundle != nil && hc.Spec.AdditionalTrustBundle.Name == configMapName { + return true + } + if hc.Spec.Configuration != nil && hc.Spec.Configuration.Proxy != nil && hc.Spec.Configuration.Proxy.TrustedCA.Name == configMapName { + return true + } + return false } // RolloutHash returns a hash derived only from spec-driven inputs that require node replacement. @@ -210,14 +339,14 @@ func (cg *ConfigGenerator) HashWithoutVersion() string { // are excluded, so they do not trigger Replace or InPlace rollouts. Endpoint-affecting // connectivity changes (Public/Private, API server address/port) are included. func (cg *ConfigGenerator) RolloutHash() string { - return supportutil.HashSimple(cg.rolloutMcoRawConfig + cg.releaseImage.Version() + cg.pullSecretName + cg.additionalTrustBundleName + cg.rolloutGlobalConfig + cg.rhelStream + cg.endpointConfig) + return supportutil.HashSimple(cg.rolloutMcoRawConfig + cg.releaseImage.Version() + cg.pullSecretName + cg.additionalTrustBundleHash + cg.proxyTrustedCAHash + cg.rolloutGlobalConfig + cg.rhelStream + cg.endpointConfig) } // RolloutHashWithoutVersion is like RolloutHash but excludes the release version. // Used to detect config-only changes for the UpdatingConfig condition, // separate from version changes which have their own condition. func (cg *ConfigGenerator) RolloutHashWithoutVersion() string { - return supportutil.HashSimple(cg.rolloutMcoRawConfig + cg.pullSecretName + cg.additionalTrustBundleName + cg.rolloutGlobalConfig + cg.rhelStream + cg.endpointConfig) + return supportutil.HashSimple(cg.rolloutMcoRawConfig + cg.pullSecretName + cg.additionalTrustBundleHash + cg.proxyTrustedCAHash + cg.rolloutGlobalConfig + cg.rhelStream + cg.endpointConfig) } func (cg *ConfigGenerator) Version() string { diff --git a/hypershift-operator/controllers/nodepool/config_hash_migration_live_test.go b/hypershift-operator/controllers/nodepool/config_hash_migration_live_test.go new file mode 100644 index 000000000000..cadded89fd78 --- /dev/null +++ b/hypershift-operator/controllers/nodepool/config_hash_migration_live_test.go @@ -0,0 +1,113 @@ +//go:build live + +package nodepool + +import ( + "context" + "fmt" + "os" + "testing" + + . "github.com/onsi/gomega" + + hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" + haproxy "github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/apiserver-haproxy" + "github.com/openshift/hypershift/pkg/manifests" + "github.com/openshift/hypershift/support/api" + fakereleaseprovider "github.com/openshift/hypershift/support/releaseinfo/fake" + "github.com/openshift/hypershift/support/thirdparty/library-go/pkg/image/dockerv1client" + fakeimagemetadataprovider "github.com/openshift/hypershift/support/util/fakeimagemetadataprovider" + + docker10 "github.com/openshift/api/image/docker10" + + appsv1 "k8s.io/api/apps/v1" + "k8s.io/apimachinery/pkg/types" + + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/config" +) + +// TestLiveComputeLegacyHash prints legacy and content-based hashes for a live cluster. +// Run: KUBECONFIG=... go test -tags live ./hypershift-operator/controllers/nodepool/ -run TestLiveComputeLegacyHash -v +func TestLiveComputeLegacyHash(t *testing.T) { + g := NewWithT(t) + + ns := envOr("LIVE_NAMESPACE", "clusters") + hcName := envOr("LIVE_HC", "rfe8751-migrate") + npName := envOr("LIVE_NP", "rfe8751-migrate-ap-south-1a") + + cfg, err := config.GetConfig() + g.Expect(err).NotTo(HaveOccurred()) + + c, err := client.New(cfg, client.Options{Scheme: api.Scheme}) + g.Expect(err).NotTo(HaveOccurred()) + + ctx := context.Background() + hc := &hyperv1.HostedCluster{} + g.Expect(c.Get(ctx, types.NamespacedName{Namespace: ns, Name: hcName}, hc)).To(Succeed()) + np := &hyperv1.NodePool{} + g.Expect(c.Get(ctx, types.NamespacedName{Namespace: ns, Name: npName}, np)).To(Succeed()) + + version := np.Status.Version + if version == "" { + version = "4.21.0" + } + releaseImage, err := (&fakereleaseprovider.FakeReleaseProvider{Version: version}).Lookup(ctx, hc.Spec.Release.Image, nil) + g.Expect(err).NotTo(HaveOccurred()) + + osStreamsEnabled := false + resolvedRHELStream, err := GetRHELStreamForBootImage(ctx, c, np, releaseImage, osStreamsEnabled) + g.Expect(err).NotTo(HaveOccurred()) + + hoImage := os.Getenv("HO_IMAGE") + if hoImage == "" { + deploy := &appsv1.Deployment{} + g.Expect(c.Get(ctx, types.NamespacedName{Namespace: "hypershift", Name: "operator"}, deploy)).To(Succeed()) + hoImage = deploy.Spec.Template.Spec.Containers[0].Image + } + + r := &NodePoolReconciler{ + Client: c, + HypershiftOperatorImage: hoImage, + ReleaseProvider: &fakereleaseprovider.FakeReleaseProvider{ + Version: version, + Components: map[string]string{"hypershift": hoImage}, + }, + ImageMetadataProvider: &fakeimagemetadataprovider.FakeRegistryClientImageMetadataProvider{ + Result: &dockerv1client.DockerImageConfig{ + Config: &docker10.DockerConfig{ + Labels: map[string]string{ + haproxy.ControlPlaneOperatorSkipsHAProxyConfigGenerationLabel: "true", + }, + }, + }, + }, + } + haproxyRawConfig, err := r.generateHAProxyRawConfig(ctx, np, hc, releaseImage) + g.Expect(err).NotTo(HaveOccurred()) + + cg, err := NewConfigGenerator(ctx, c, hc, np, releaseImage, haproxyRawConfig, manifests.HostedControlPlaneNamespace(ns, hcName), resolvedRHELStream) + g.Expect(err).NotTo(HaveOccurred()) + + legacyHWV := cg.hashWithoutVersionAtVersion(ConfigHashVersionV1) + legacyH := cg.hashAtVersion(ConfigHashVersionV1) + newHWV := cg.HashWithoutVersion() + newH := cg.Hash() + + fmt.Printf("LIVE_LEGACY_HWV=%s\n", legacyHWV) + fmt.Printf("LIVE_LEGACY_H=%s\n", legacyH) + fmt.Printf("LIVE_NEW_HWV=%s\n", newHWV) + fmt.Printf("LIVE_NEW_H=%s\n", newH) + fmt.Printf("LIVE_CURRENT=%s\n", np.Annotations[nodePoolAnnotationCurrentConfig]) + fmt.Printf("LIVE_CURRENT_VERSION=%s\n", np.Annotations[nodePoolAnnotationCurrentConfigVersion]) + fmt.Printf("LIVE_CONFIG_HASH_VERSION=%s\n", np.Annotations[nodePoolAnnotationConfigHashVersion]) + fmt.Printf("LIVE_ROLLOUT_BASELINE=%s\n", np.Annotations[nodePoolAnnotationCurrentRolloutConfig]) + fmt.Printf("LIVE_ROLLOUT_TARGET=%s\n", cg.RolloutHashWithoutVersion()) +} + +func envOr(key, def string) string { + if v := os.Getenv(key); v != "" { + return v + } + return def +} diff --git a/hypershift-operator/controllers/nodepool/config_hash_version.go b/hypershift-operator/controllers/nodepool/config_hash_version.go new file mode 100644 index 000000000000..33e996612015 --- /dev/null +++ b/hypershift-operator/controllers/nodepool/config_hash_version.go @@ -0,0 +1,16 @@ +package nodepool + +// The v2 rollout baseline is stored separately from the legacy name-based +// rollout annotations. Upstream's seedRolloutAnnotation and token state machine +// adopt existing bootstrap identities once, including in-flight rollouts, before +// tracking subsequent CA content changes. Never rewrite a completed payload hash +// merely to migrate a rollout formula: it identifies the secrets still in use. +const ( + // ConfigHashVersionV1 is the legacy name-based payload hash formula. + ConfigHashVersionV1 = "v1" + // ConfigHashVersionV2 includes trust-bundle content in payload and rollout hashes. + ConfigHashVersionV2 = "v2" + // CurrentConfigHashVersion identifies the active trust-bundle hash formula. + CurrentConfigHashVersion = ConfigHashVersionV2 + nodePoolAnnotationConfigHashVersion = "hypershift.openshift.io/nodePoolConfigHashVersion" +) diff --git a/hypershift-operator/controllers/nodepool/config_hash_version_test.go b/hypershift-operator/controllers/nodepool/config_hash_version_test.go new file mode 100644 index 000000000000..4eb8e10055ef --- /dev/null +++ b/hypershift-operator/controllers/nodepool/config_hash_version_test.go @@ -0,0 +1,58 @@ +package nodepool + +import ( + "testing" + + . "github.com/onsi/gomega" + + hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" + "github.com/openshift/hypershift/support/releaseinfo" + supportutil "github.com/openshift/hypershift/support/util" + + imageapi "github.com/openshift/api/image/v1" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestHashAtVersion(t *testing.T) { + t.Run("When references have identical content, it should retain the v2 hash", testHashAtVersionContentEquivalentReferences) + mcoRawConfig := "mco-config" + pullSecretName := "pull-secret" + atbName := "user-ca" + atbContentHash := supportutil.HashSimple("bundle-content") + proxyContentHash := supportutil.HashSimple("proxy-ca-content") + globalConfig := "global-config" + rhelStream := "" + version := "4.18.0" + + cg := &ConfigGenerator{ + hostedCluster: &hyperv1.HostedCluster{ + Spec: hyperv1.HostedClusterSpec{ + AdditionalTrustBundle: &corev1.LocalObjectReference{Name: atbName}, + PullSecret: corev1.LocalObjectReference{Name: pullSecretName}, + }, + }, + rolloutConfig: &rolloutConfig{ + releaseImage: &releaseinfo.ReleaseImage{ + ImageStream: &imageapi.ImageStream{ + ObjectMeta: metav1.ObjectMeta{Name: version}, + }, + }, + pullSecretName: pullSecretName, + additionalTrustBundleHash: atbContentHash, + proxyTrustedCAHash: proxyContentHash, + globalConfig: globalConfig, + mcoRawConfig: mcoRawConfig, + rhelStream: rhelStream, + }, + } + + g := NewWithT(t) + g.Expect(cg.hashWithoutVersionAtVersion(ConfigHashVersionV1)).To(Equal( + supportutil.HashSimple(mcoRawConfig + pullSecretName + atbName + rhelStream))) + g.Expect(cg.hashAtVersion(ConfigHashVersionV1)).To(Equal( + supportutil.HashSimple(mcoRawConfig + version + pullSecretName + atbName + globalConfig + rhelStream))) + g.Expect(cg.hashWithoutVersionAtVersion(ConfigHashVersionV2)).To(Equal(cg.HashWithoutVersion())) + g.Expect(cg.hashAtVersion(ConfigHashVersionV2)).To(Equal(cg.Hash())) +} diff --git a/hypershift-operator/controllers/nodepool/config_test.go b/hypershift-operator/controllers/nodepool/config_test.go index 2c61ab8f04c9..53934d0d39c4 100644 --- a/hypershift-operator/controllers/nodepool/config_test.go +++ b/hypershift-operator/controllers/nodepool/config_test.go @@ -16,6 +16,7 @@ import ( cpomanifests "github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/manifests" sharedingress "github.com/openshift/hypershift/hypershift-operator/controllers/sharedingress" api "github.com/openshift/hypershift/support/api" + "github.com/openshift/hypershift/support/certs" "github.com/openshift/hypershift/support/releaseinfo" supportutil "github.com/openshift/hypershift/support/util" @@ -418,8 +419,8 @@ spec: }, { name: "When additionalTrustBundle is specified it should be included in rolloutConfig", - expectedHash: "632801f8", - expectedHashWithoutVersion: "71375893", + expectedHash: "1d823653", + expectedHashWithoutVersion: "7b5702f4", nodePool: &hyperv1.NodePool{ ObjectMeta: metav1.ObjectMeta{ Namespace: "test", @@ -442,6 +443,15 @@ spec: TokenSecretConfigKey: machineConfig, }, }, + &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "additional-trust-bundle", + Namespace: "test", + }, + Data: map[string]string{ + certs.UserCABundleMapKey: "test-ca-bundle", + }, + }, }, expectedMCORawConfig: machineConfigDefaulted, releaseImage: &releaseinfo.ReleaseImage{ @@ -506,7 +516,7 @@ spec: } g.Expect(cg.pullSecretName).To(Equal(tc.hostedCluster.Spec.PullSecret.Name)) if tc.hostedCluster.Spec.AdditionalTrustBundle != nil { - g.Expect(cg.additionalTrustBundleName).To(Equal("additional-trust-bundle")) + g.Expect(cg.additionalTrustBundleHash).To(Equal(supportutil.HashSimple("test-ca-bundle"))) } if tc.hostedCluster.Spec.Configuration != nil { @@ -609,83 +619,102 @@ func TestHash(t *testing.T) { baseCaseMCORawConfig := "test config" baseCaseReleaseVersion := "4.7.0" baseCasePullSecretName := "pull-secret" - baseCaseAdditionalTrustBundleName := "trust-bundle" + baseCaseAdditionalTrustBundleHash := "trust-bundle-hash" + baseCaseProxyTrustedCAHash := "" baseCaseGlobalConfig := "global config" - baseCaseHash := "bb196408" + baseCaseHash := "bf2008f3" testCases := []struct { name string mcoRawConfig string releaseVersion string pullSecretName string - additionalTrustBundleName string + additionalTrustBundleHash string + proxyTrustedCAHash string globalConfig string rhelStream string expected string }{ { - name: "When base case inputs are used, it should produce the base case hash", + name: "When base rollout config is used, it should produce the expected hash", mcoRawConfig: baseCaseMCORawConfig, releaseVersion: baseCaseReleaseVersion, pullSecretName: baseCasePullSecretName, - additionalTrustBundleName: baseCaseAdditionalTrustBundleName, + additionalTrustBundleHash: baseCaseAdditionalTrustBundleHash, + proxyTrustedCAHash: baseCaseProxyTrustedCAHash, globalConfig: baseCaseGlobalConfig, expected: baseCaseHash, }, { - name: "When a different version is used, it should change the hash", + name: "When release version differs, it should change the hash", mcoRawConfig: baseCaseMCORawConfig, releaseVersion: "4.8.0", pullSecretName: baseCasePullSecretName, - additionalTrustBundleName: baseCaseAdditionalTrustBundleName, + additionalTrustBundleHash: baseCaseAdditionalTrustBundleHash, + proxyTrustedCAHash: baseCaseProxyTrustedCAHash, globalConfig: baseCaseGlobalConfig, - expected: "27bb7699", + expected: "8c635f74", }, { - name: "When a different mcoRawConfig is used, it should change the hash", + name: "When mcoRawConfig differs, it should change the hash", mcoRawConfig: "different", releaseVersion: baseCaseReleaseVersion, pullSecretName: baseCasePullSecretName, - additionalTrustBundleName: baseCaseAdditionalTrustBundleName, + additionalTrustBundleHash: baseCaseAdditionalTrustBundleHash, + proxyTrustedCAHash: baseCaseProxyTrustedCAHash, globalConfig: baseCaseGlobalConfig, - expected: "25f99ac5", + expected: "1a019fa0", }, { - name: "When a different pullSecretName is used, it should change the hash", + name: "When pullSecretName differs, it should change the hash", mcoRawConfig: baseCaseMCORawConfig, releaseVersion: baseCaseReleaseVersion, pullSecretName: "different", - additionalTrustBundleName: baseCaseAdditionalTrustBundleName, + additionalTrustBundleHash: baseCaseAdditionalTrustBundleHash, + proxyTrustedCAHash: baseCaseProxyTrustedCAHash, globalConfig: baseCaseGlobalConfig, - expected: "d0d6f6e9", + expected: "338f02c4", }, { - name: "When a different trust-bundle is used, it should change the hash", + name: "When additionalTrustBundleHash differs, it should change the hash", mcoRawConfig: baseCaseMCORawConfig, releaseVersion: baseCaseReleaseVersion, pullSecretName: baseCasePullSecretName, - additionalTrustBundleName: "different", + additionalTrustBundleHash: "different", + proxyTrustedCAHash: baseCaseProxyTrustedCAHash, globalConfig: baseCaseGlobalConfig, expected: "42d42744", }, { - name: "When a different globalConfig is used, it should change the hash", + name: "When proxyTrustedCAHash differs, it should change the hash", + mcoRawConfig: baseCaseMCORawConfig, + releaseVersion: baseCaseReleaseVersion, + pullSecretName: baseCasePullSecretName, + additionalTrustBundleHash: baseCaseAdditionalTrustBundleHash, + proxyTrustedCAHash: "different", + globalConfig: baseCaseGlobalConfig, + expected: "42a01974", + }, + { + name: "When globalConfig differs, it should change the hash", mcoRawConfig: baseCaseMCORawConfig, releaseVersion: baseCaseReleaseVersion, pullSecretName: baseCasePullSecretName, - additionalTrustBundleName: baseCaseAdditionalTrustBundleName, + additionalTrustBundleHash: baseCaseAdditionalTrustBundleHash, + proxyTrustedCAHash: baseCaseProxyTrustedCAHash, globalConfig: "different", - expected: "e916ddfe", + expected: "52c0d495", }, { name: "When rhelStream is a non-default stream, it should change the hash", mcoRawConfig: baseCaseMCORawConfig, releaseVersion: baseCaseReleaseVersion, pullSecretName: baseCasePullSecretName, - additionalTrustBundleName: baseCaseAdditionalTrustBundleName, + additionalTrustBundleHash: baseCaseAdditionalTrustBundleHash, + proxyTrustedCAHash: baseCaseProxyTrustedCAHash, globalConfig: baseCaseGlobalConfig, rhelStream: "rhel-10", - expected: "2dbbd41b", + expected: "4702e8a6", }, } @@ -703,17 +732,17 @@ func TestHash(t *testing.T) { rolloutConfig: &rolloutConfig{ mcoRawConfig: tc.mcoRawConfig, pullSecretName: tc.pullSecretName, - additionalTrustBundleName: tc.additionalTrustBundleName, + additionalTrustBundleHash: tc.additionalTrustBundleHash, + proxyTrustedCAHash: tc.proxyTrustedCAHash, globalConfig: tc.globalConfig, rhelStream: tc.rhelStream, releaseImage: releaseImage, }, } - hash := cg.Hash() g.Expect(hash).ToNot(BeEmpty()) g.Expect(hash).To(Equal(tc.expected)) - if tc.name != "When base case inputs are used, it should produce the base case hash" { + if tc.name != "When base rollout config is used, it should produce the expected hash" { g.Expect(hash).ToNot(Equal(baseCaseHash)) } }) @@ -724,72 +753,90 @@ func TestHashWithoutVersion(t *testing.T) { baseCaseMCORawConfig := "test config" baseCaseReleaseVersion := "4.7.0" baseCasePullSecretName := "pull-secret" - baseCaseAdditionalTrustBundleName := "trust-bundle" + baseCaseAdditionalTrustBundleHash := "trust-bundle-hash" + baseCaseProxyTrustedCAHash := "" baseCaseGlobalConfig := "global config" - baseCaseHash := "85234650" + baseCaseHash := "7d347237" testCases := []struct { name string mcoRawConfig string releaseVersion string pullSecretName string - additionalTrustBundleName string + additionalTrustBundleHash string + proxyTrustedCAHash string globalConfig string rhelStream string expected string }{ { - name: "When base case inputs are used, it should produce the base case hash", + name: "When base rollout config is used, it should produce the expected hash", mcoRawConfig: baseCaseMCORawConfig, releaseVersion: baseCaseReleaseVersion, pullSecretName: baseCasePullSecretName, - additionalTrustBundleName: baseCaseAdditionalTrustBundleName, + additionalTrustBundleHash: baseCaseAdditionalTrustBundleHash, + proxyTrustedCAHash: baseCaseProxyTrustedCAHash, globalConfig: baseCaseGlobalConfig, expected: baseCaseHash, }, { - name: "When a different version is used, it should not change the hash", + name: "When release version differs, it should not change the hash", mcoRawConfig: baseCaseMCORawConfig, releaseVersion: "4.8.0", pullSecretName: baseCasePullSecretName, - additionalTrustBundleName: baseCaseAdditionalTrustBundleName, + additionalTrustBundleHash: baseCaseAdditionalTrustBundleHash, + proxyTrustedCAHash: baseCaseProxyTrustedCAHash, globalConfig: baseCaseGlobalConfig, expected: baseCaseHash, }, { - name: "When a different mcoRawConfig is used, it should change the hash", + name: "When mcoRawConfig differs, it should change the hash", mcoRawConfig: "different", releaseVersion: baseCaseReleaseVersion, pullSecretName: baseCasePullSecretName, - additionalTrustBundleName: baseCaseAdditionalTrustBundleName, + additionalTrustBundleHash: baseCaseAdditionalTrustBundleHash, + proxyTrustedCAHash: baseCaseProxyTrustedCAHash, globalConfig: baseCaseGlobalConfig, - expected: "5ea671c5", + expected: "c3130a68", }, { - name: "When a different pullSecretName is used, it should change the hash", + name: "When pullSecretName differs, it should change the hash", mcoRawConfig: baseCaseMCORawConfig, releaseVersion: baseCaseReleaseVersion, pullSecretName: "different", - additionalTrustBundleName: baseCaseAdditionalTrustBundleName, + additionalTrustBundleHash: baseCaseAdditionalTrustBundleHash, + proxyTrustedCAHash: baseCaseProxyTrustedCAHash, globalConfig: baseCaseGlobalConfig, - expected: "f6e82eb7", + expected: "ede3a372", }, { - name: "When a different trust-bundle is used, it should change the hash", + name: "When additionalTrustBundleHash differs, it should change the hash", mcoRawConfig: baseCaseMCORawConfig, releaseVersion: baseCaseReleaseVersion, pullSecretName: baseCasePullSecretName, - additionalTrustBundleName: "different", + additionalTrustBundleHash: "different", + proxyTrustedCAHash: baseCaseProxyTrustedCAHash, globalConfig: baseCaseGlobalConfig, expected: "935c3492", }, + { + name: "When proxyTrustedCAHash differs, it should change the hash", + mcoRawConfig: baseCaseMCORawConfig, + releaseVersion: baseCaseReleaseVersion, + pullSecretName: baseCasePullSecretName, + additionalTrustBundleHash: baseCaseAdditionalTrustBundleHash, + proxyTrustedCAHash: "different", + globalConfig: baseCaseGlobalConfig, + expected: "727bd5c8", + }, { // TODO(alberto): This was left inconsistent in https://github.com/openshift/hypershift/pull/3795/files. It should also contain cg.globalConfig. // This is kept like this for now to contain the scope of the refactor and avoid backward compatibility issues. - name: "When a different globalConfig is used, it should NOT change the hash", + name: "When globalConfig differs, it should not change the hash", mcoRawConfig: baseCaseMCORawConfig, releaseVersion: baseCaseReleaseVersion, pullSecretName: baseCasePullSecretName, - additionalTrustBundleName: baseCaseAdditionalTrustBundleName, + additionalTrustBundleHash: baseCaseAdditionalTrustBundleHash, + proxyTrustedCAHash: baseCaseProxyTrustedCAHash, globalConfig: "different", expected: baseCaseHash, }, @@ -798,10 +845,11 @@ func TestHashWithoutVersion(t *testing.T) { mcoRawConfig: baseCaseMCORawConfig, releaseVersion: baseCaseReleaseVersion, pullSecretName: baseCasePullSecretName, - additionalTrustBundleName: baseCaseAdditionalTrustBundleName, + additionalTrustBundleHash: baseCaseAdditionalTrustBundleHash, + proxyTrustedCAHash: baseCaseProxyTrustedCAHash, globalConfig: baseCaseGlobalConfig, rhelStream: "rhel-10", - expected: "671fe083", + expected: "9240abaa", }, } @@ -819,7 +867,8 @@ func TestHashWithoutVersion(t *testing.T) { rolloutConfig: &rolloutConfig{ mcoRawConfig: tc.mcoRawConfig, pullSecretName: tc.pullSecretName, - additionalTrustBundleName: tc.additionalTrustBundleName, + additionalTrustBundleHash: tc.additionalTrustBundleHash, + proxyTrustedCAHash: tc.proxyTrustedCAHash, globalConfig: tc.globalConfig, rhelStream: tc.rhelStream, releaseImage: releaseImage, @@ -837,7 +886,7 @@ func TestRolloutHash(t *testing.T) { baseCaseUserConfig := "test config" baseCaseReleaseVersion := "4.7.0" baseCasePullSecretName := "pull-secret" - baseCaseAdditionalTrustBundleName := "trust-bundle" + baseCaseAdditionalTrustBundleHash := "trust-bundle" baseCaseGlobalConfig := "global config" baseCaseRolloutGlobalConfig := "rollout global config" baseCaseHAProxyRawConfig := "haproxy static pod manifest with image digest" @@ -849,7 +898,7 @@ func TestRolloutHash(t *testing.T) { mcoRawConfig: baseCaseHAProxyRawConfig + "\n---\n" + baseCaseUserConfig, rolloutMcoRawConfig: baseCaseUserConfig, pullSecretName: baseCasePullSecretName, - additionalTrustBundleName: baseCaseAdditionalTrustBundleName, + additionalTrustBundleHash: baseCaseAdditionalTrustBundleHash, globalConfig: baseCaseGlobalConfig, rolloutGlobalConfig: baseCaseRolloutGlobalConfig, haproxyRawConfig: baseCaseHAProxyRawConfig, @@ -895,8 +944,8 @@ func TestRolloutHash(t *testing.T) { expectChanged: true, }, { - name: "When the additionalTrustBundleName changes it should change the RolloutHash", - modify: func(rc *rolloutConfig) { rc.additionalTrustBundleName = "different-trust-bundle" }, + name: "When the additionalTrustBundleHash changes it should change the RolloutHash", + modify: func(rc *rolloutConfig) { rc.additionalTrustBundleHash = "different-trust-bundle" }, expectChanged: true, }, { @@ -1238,7 +1287,7 @@ func TestIsOutdated(t *testing.T) { rolloutConfig: &rolloutConfig{ rolloutMcoRawConfig: "config", pullSecretName: "pull", - additionalTrustBundleName: "trust", + additionalTrustBundleHash: "trust", rolloutGlobalConfig: "global", releaseImage: &releaseinfo.ReleaseImage{ ImageStream: &imageapi.ImageStream{ diff --git a/hypershift-operator/controllers/nodepool/config_trustbundle_test.go b/hypershift-operator/controllers/nodepool/config_trustbundle_test.go new file mode 100644 index 000000000000..791880bc4cd9 --- /dev/null +++ b/hypershift-operator/controllers/nodepool/config_trustbundle_test.go @@ -0,0 +1,261 @@ +package nodepool + +import ( + "errors" + "testing" + + . "github.com/onsi/gomega" + + hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" + api "github.com/openshift/hypershift/support/api" + "github.com/openshift/hypershift/support/certs" + supportutil "github.com/openshift/hypershift/support/util" + + configv1 "github.com/openshift/api/config/v1" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + crclient "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/reconcile" +) + +func TestHostedClusterReferencesConfigMap(t *testing.T) { + testCases := []struct { + name string + hostedCluster *hyperv1.HostedCluster + configMapName string + expected bool + }{ + { + name: "When ConfigMap is referenced as additionalTrustBundle, it should return true", + hostedCluster: &hyperv1.HostedCluster{ + Spec: hyperv1.HostedClusterSpec{ + AdditionalTrustBundle: &corev1.LocalObjectReference{Name: "user-ca"}, + }, + }, + configMapName: "user-ca", + expected: true, + }, + { + name: "When ConfigMap is referenced as proxy trustedCA, it should return true", + hostedCluster: &hyperv1.HostedCluster{ + Spec: hyperv1.HostedClusterSpec{ + Configuration: &hyperv1.ClusterConfiguration{ + Proxy: &configv1.ProxySpec{ + TrustedCA: configv1.ConfigMapNameReference{Name: "proxy-ca"}, + }, + }, + }, + }, + configMapName: "proxy-ca", + expected: true, + }, + { + name: "When ConfigMap is not referenced by HostedCluster, it should return false", + hostedCluster: &hyperv1.HostedCluster{ + Spec: hyperv1.HostedClusterSpec{ + AdditionalTrustBundle: &corev1.LocalObjectReference{Name: "user-ca"}, + }, + }, + configMapName: "other-ca", + expected: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + g.Expect(hostedClusterReferencesConfigMap(tc.hostedCluster, tc.configMapName)).To(Equal(tc.expected)) + }) + } +} + +func TestRolloutTrustBundleHashes(t *testing.T) { + testCases := []struct { + name string + hostedCluster *hyperv1.HostedCluster + objects []crclient.Object + expectedAdditionalTrustBundle string + expectedProxyTrustedCA string + expectError bool + expectTrustBundleError bool + }{ + { + name: "When additionalTrustBundle content changes, it should produce a different hash", + hostedCluster: &hyperv1.HostedCluster{ + ObjectMeta: metav1.ObjectMeta{Namespace: "clusters", Name: "test"}, + Spec: hyperv1.HostedClusterSpec{ + AdditionalTrustBundle: &corev1.LocalObjectReference{Name: "user-ca"}, + }, + }, + objects: []crclient.Object{ + &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Namespace: "clusters", Name: "user-ca"}, + Data: map[string]string{certs.UserCABundleMapKey: "bundle-a"}, + }, + }, + expectedAdditionalTrustBundle: supportutil.HashSimple("bundle-a"), + }, + { + name: "When proxy trustedCA is set, it should hash the ConfigMap content", + hostedCluster: &hyperv1.HostedCluster{ + ObjectMeta: metav1.ObjectMeta{Namespace: "clusters", Name: "test"}, + Spec: hyperv1.HostedClusterSpec{ + Configuration: &hyperv1.ClusterConfiguration{ + Proxy: &configv1.ProxySpec{ + TrustedCA: configv1.ConfigMapNameReference{Name: "proxy-ca"}, + }, + }, + }, + }, + objects: []crclient.Object{ + &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Namespace: "clusters", Name: "proxy-ca"}, + Data: map[string]string{certs.UserCABundleMapKey: "proxy-bundle"}, + }, + }, + expectedProxyTrustedCA: supportutil.HashSimple("proxy-bundle"), + }, + { + name: "When referenced ConfigMap is missing ca-bundle.crt, it should return a TrustBundleConfigError", + hostedCluster: &hyperv1.HostedCluster{ + ObjectMeta: metav1.ObjectMeta{Namespace: "clusters", Name: "test"}, + Spec: hyperv1.HostedClusterSpec{ + AdditionalTrustBundle: &corev1.LocalObjectReference{Name: "user-ca"}, + }, + }, + objects: []crclient.Object{ + &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Namespace: "clusters", Name: "user-ca"}, + Data: map[string]string{"other-key": "value"}, + }, + }, + expectError: true, + expectTrustBundleError: true, + }, + { + name: "When referenced ConfigMap does not exist, it should return a TrustBundleConfigError", + hostedCluster: &hyperv1.HostedCluster{ + ObjectMeta: metav1.ObjectMeta{Namespace: "clusters", Name: "test"}, + Spec: hyperv1.HostedClusterSpec{ + AdditionalTrustBundle: &corev1.LocalObjectReference{Name: "user-ca"}, + }, + }, + expectError: true, + expectTrustBundleError: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + client := fake.NewClientBuilder().WithScheme(api.Scheme).WithObjects(tc.objects...).Build() + additionalHash, proxyHash, err := rolloutTrustBundleHashes(t.Context(), client, tc.hostedCluster) + if tc.expectError { + g.Expect(err).To(HaveOccurred()) + if tc.expectTrustBundleError { + var trustBundleErr *TrustBundleConfigError + g.Expect(errors.As(err, &trustBundleErr)).To(BeTrue()) + } + return + } + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(additionalHash).To(Equal(tc.expectedAdditionalTrustBundle)) + g.Expect(proxyHash).To(Equal(tc.expectedProxyTrustedCA)) + }) + } +} + +func TestEnqueueNodePoolsForHostedClusterReferencedConfig(t *testing.T) { + hostedClusterWithBoth := &hyperv1.HostedCluster{ + ObjectMeta: metav1.ObjectMeta{Namespace: "clusters", Name: "test-hc"}, + Spec: hyperv1.HostedClusterSpec{ + AdditionalTrustBundle: &corev1.LocalObjectReference{Name: "user-ca"}, + Configuration: &hyperv1.ClusterConfiguration{ + Proxy: &configv1.ProxySpec{ + TrustedCA: configv1.ConfigMapNameReference{Name: "proxy-ca"}, + }, + }, + }, + } + nodePool := &hyperv1.NodePool{ + ObjectMeta: metav1.ObjectMeta{Namespace: "clusters", Name: "workers"}, + Spec: hyperv1.NodePoolSpec{ + ClusterName: "test-hc", + }, + } + + testCases := []struct { + name string + hostedCluster *hyperv1.HostedCluster + configMapName string + expectEnqueue bool + }{ + { + name: "When ConfigMap is referenced as proxy.trustedCA, it should enqueue the NodePool", + hostedCluster: &hyperv1.HostedCluster{ + ObjectMeta: metav1.ObjectMeta{Namespace: "clusters", Name: "test-hc"}, + Spec: hyperv1.HostedClusterSpec{ + Configuration: &hyperv1.ClusterConfiguration{ + Proxy: &configv1.ProxySpec{ + TrustedCA: configv1.ConfigMapNameReference{Name: "proxy-ca"}, + }, + }, + }, + }, + configMapName: "proxy-ca", + expectEnqueue: true, + }, + { + name: "When ConfigMap is referenced as additionalTrustBundle, it should enqueue the NodePool", + hostedCluster: &hyperv1.HostedCluster{ + ObjectMeta: metav1.ObjectMeta{Namespace: "clusters", Name: "test-hc"}, + Spec: hyperv1.HostedClusterSpec{ + AdditionalTrustBundle: &corev1.LocalObjectReference{Name: "user-ca"}, + }, + }, + configMapName: "user-ca", + expectEnqueue: true, + }, + { + name: "When ConfigMap is not referenced by HostedCluster, it should not enqueue the NodePool", + hostedCluster: hostedClusterWithBoth, + configMapName: "unrelated-ca", + expectEnqueue: false, + }, + { + name: "When HostedCluster is not found, it should not enqueue the NodePool", + hostedCluster: nil, + configMapName: "user-ca", + expectEnqueue: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + + configMap := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Namespace: "clusters", Name: tc.configMapName}, + Data: map[string]string{certs.UserCABundleMapKey: "updated-bundle"}, + } + + builder := fake.NewClientBuilder().WithScheme(api.Scheme).WithObjects(nodePool) + if tc.hostedCluster != nil { + builder = builder.WithObjects(tc.hostedCluster) + } + reconciler := &NodePoolReconciler{ + Client: builder.Build(), + } + + requests := reconciler.enqueueNodePoolsForConfig(t.Context(), configMap) + if tc.expectEnqueue { + g.Expect(requests).To(ConsistOf(reconcile.Request{NamespacedName: crclient.ObjectKeyFromObject(nodePool)})) + } else { + g.Expect(requests).To(BeEmpty()) + } + }) + } +} diff --git a/hypershift-operator/controllers/nodepool/nodepool_controller.go b/hypershift-operator/controllers/nodepool/nodepool_controller.go index 3ee9ce4318bd..0a02467012a7 100644 --- a/hypershift-operator/controllers/nodepool/nodepool_controller.go +++ b/hypershift-operator/controllers/nodepool/nodepool_controller.go @@ -41,6 +41,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/client-go/tools/record" "k8s.io/client-go/util/workqueue" "k8s.io/utils/ptr" @@ -74,8 +75,8 @@ const ( nodePoolAnnotationCurrentConfig = "hypershift.openshift.io/nodePoolCurrentConfig" nodePoolAnnotationCurrentConfigVersion = "hypershift.openshift.io/nodePoolCurrentConfigVersion" nodePoolAnnotationTargetConfigVersion = "hypershift.openshift.io/nodePoolTargetConfigVersion" - nodePoolAnnotationCurrentRolloutConfig = "hypershift.openshift.io/nodePoolCurrentRolloutConfig" - nodePoolAnnotationInProgressRolloutConfig = "hypershift.openshift.io/nodePoolInProgressRolloutConfig" + nodePoolAnnotationCurrentRolloutConfig = "hypershift.openshift.io/nodePoolCurrentRolloutConfigV2" + nodePoolAnnotationInProgressRolloutConfig = "hypershift.openshift.io/nodePoolInProgressRolloutConfigV2" nodePoolAnnotationUpgradeInProgressTrue = "hypershift.openshift.io/nodePoolUpgradeInProgressTrue" nodePoolAnnotationUpgradeInProgressFalse = "hypershift.openshift.io/nodePoolUpgradeInProgressFalse" nodePoolAnnotationMaxUnavailable = "hypershift.openshift.io/nodePoolMaxUnavailable" @@ -83,7 +84,7 @@ const ( // Stored on the CAPI workload's metadata with its bootstrap reference, so // request identity survives a failed subsequent NodePool patch. Keeping it // off the machine template avoids triggering a rollout when it is seeded. - nodePoolAnnotationTargetRolloutConfig = "hypershift.openshift.io/nodePoolTargetRolloutConfig" + nodePoolAnnotationTargetRolloutConfig = "hypershift.openshift.io/nodePoolTargetRolloutConfigV2" // ec2InstanceMetadataHTTPTokensAnnotation can be set to change the instance metadata options of the nodepool underlying EC2 instances // possible values are 'required' (i.e. IMDSv2) or 'optional' which is the default. @@ -128,6 +129,7 @@ type NotReadyError struct { } type CPOCapabilities struct { + RenderedTrustBundleValidation bool DecompressAndDecodeConfig bool CreateDefaultAWSSecurityGroup bool } @@ -455,12 +457,13 @@ func (r *NodePoolReconciler) reconcile(ctx context.Context, hcluster *hyperv1.Ho } } - // If reconciliation is paused we return before modifying any state capi, err := newCAPI(token, infraID) if err != nil { return ctrl.Result{}, err } capi.scaleFromZeroPlatform = r.ScaleFromZeroPlatform + + // If reconciliation is paused we return before modifying any state if isPaused, duration := reconcilerpolicy.IsReconciliationPaused(log, nodePool.Spec.PausedUntil); isPaused { if err := capi.Pause(ctx); err != nil { return ctrl.Result{}, fmt.Errorf("error pausing CAPI: %w", err) @@ -1006,6 +1009,7 @@ func seedRolloutAnnotation(token *Token) { if nodePool.Annotations == nil { nodePool.Annotations = make(map[string]string) } + nodePool.Annotations[nodePoolAnnotationConfigHashVersion] = CurrentConfigHashVersion completedHash := nodePool.Annotations[nodePoolAnnotationCurrentConfigVersion] rolloutInProgress := token.deployedBootstrapHash != "" && token.deployedBootstrapHash != completedHash hasInProgressMarker := nodePool.Annotations[nodePoolAnnotationInProgressRolloutConfig] != "" @@ -1140,6 +1144,7 @@ func (r *NodePoolReconciler) enqueueNodePoolsForConfig(ctx context.Context, obj } // Otherwise reconcile NodePools which are referencing the given ConfigMap. + seen := sets.New[string]() for key := range nodePoolList.Items { reconcileNodePool := false for _, v := range nodePoolList.Items[key].Spec.Config { @@ -1159,13 +1164,43 @@ func (r *NodePoolReconciler) enqueueNodePoolsForConfig(ctx context.Context, obj } } if reconcileNodePool { - result = append(result, - reconcile.Request{NamespacedName: client.ObjectKeyFromObject(&nodePoolList.Items[key])}, - ) + req := reconcile.Request{NamespacedName: client.ObjectKeyFromObject(&nodePoolList.Items[key])} + seen.Insert(req.String()) + result = append(result, req) } } + // Reconcile NodePools when a ConfigMap referenced by the HostedCluster changes in place. + hcCache := map[string]*hyperv1.HostedCluster{} + for key := range nodePoolList.Items { + np := &nodePoolList.Items[key] + req := reconcile.Request{NamespacedName: client.ObjectKeyFromObject(np)} + if seen.Has(req.String()) { + continue + } + hc, ok := hcCache[np.Spec.ClusterName] + if !ok { + hc = &hyperv1.HostedCluster{} + if err := r.Get(ctx, client.ObjectKey{Namespace: np.Namespace, Name: np.Spec.ClusterName}, hc); err != nil { + if !apierrors.IsNotFound(err) { + ctrl.LoggerFrom(ctx).Error(err, "Failed to get HostedCluster; skipping NodePool enqueue", + "hostedCluster", np.Spec.ClusterName, + "nodePool", client.ObjectKeyFromObject(np)) + } + // Continue so a transient error for one HostedCluster does not drop + // enqueue requests for NodePools belonging to other HostedClusters. + continue + } + hcCache[np.Spec.ClusterName] = hc + } + if !hostedClusterReferencesConfigMap(hc, cm.Name) { + continue + } + seen.Insert(req.String()) + result = append(result, req) + } + return result } @@ -1272,16 +1307,23 @@ func validateInfraID(infraID string) error { } func (r *NodePoolReconciler) detectCPOCapabilities(ctx context.Context, hostedCluster *hyperv1.HostedCluster) (*CPOCapabilities, error) { + return DetectCPOCapabilities(ctx, r.Client, hostedCluster, r.ReleaseProvider, r.ImageMetadataProvider, r.HypershiftOperatorImage) +} + +// DetectCPOCapabilities reads the capabilities of the selected CPO image for all +// bootstrap consumers, including NodePools and Karpenter NodeClasses. +func DetectCPOCapabilities(ctx context.Context, c client.Client, hostedCluster *hyperv1.HostedCluster, releaseProvider releaseinfo.Provider, imageMetadataProvider imageregistry.ImageMetadataProvider, hypershiftOperatorImage string) (*CPOCapabilities, error) { + r := &NodePoolReconciler{Client: c} pullSecretBytes, err := r.getPullSecretBytes(ctx, hostedCluster) if err != nil { return nil, err } - controlPlaneOperatorImage, err := imageregistry.GetControlPlaneOperatorImage(ctx, hostedCluster, r.ReleaseProvider, r.HypershiftOperatorImage, pullSecretBytes) + controlPlaneOperatorImage, err := imageregistry.GetControlPlaneOperatorImage(ctx, hostedCluster, releaseProvider, hypershiftOperatorImage, pullSecretBytes) if err != nil { return nil, fmt.Errorf("failed to get controlPlaneOperatorImage: %w", err) } - controlPlaneOperatorImageMetadata, err := r.ImageMetadataProvider.ImageMetadata(ctx, controlPlaneOperatorImage, pullSecretBytes) + controlPlaneOperatorImageMetadata, err := imageMetadataProvider.ImageMetadata(ctx, controlPlaneOperatorImage, pullSecretBytes) if err != nil { return nil, fmt.Errorf("failed to look up image metadata for %s: %w", controlPlaneOperatorImage, err) } @@ -1289,6 +1331,7 @@ func (r *NodePoolReconciler) detectCPOCapabilities(ctx context.Context, hostedCl imageLabels := imageregistry.ImageLabels(controlPlaneOperatorImageMetadata) result := &CPOCapabilities{} _, result.DecompressAndDecodeConfig = imageLabels[controlPlaneOperatorManagesDecompressAndDecodeConfig] + _, result.RenderedTrustBundleValidation = imageLabels["io.openshift.hypershift.control-plane-operator-manages.rendered-trust-bundle-validation"] _, result.CreateDefaultAWSSecurityGroup = imageLabels[controlPlaneOperatorCreatesDefaultAWSSecurityGroup] return result, nil @@ -1323,12 +1366,9 @@ func getPullSecretName(ctx context.Context, crclient client.Client, hostedCluste } func (r *NodePoolReconciler) getAdditionalTrustBundle(ctx context.Context, hostedCluster *hyperv1.HostedCluster) (*corev1.ConfigMap, error) { - additionalTrustBundle := &corev1.ConfigMap{} - if err := r.Client.Get(ctx, client.ObjectKey{Namespace: hostedCluster.Namespace, Name: hostedCluster.Spec.AdditionalTrustBundle.Name}, additionalTrustBundle); err != nil { - return additionalTrustBundle, fmt.Errorf("cannot get additionalTrustBundle %s/%s: %w", hostedCluster.Namespace, hostedCluster.Spec.AdditionalTrustBundle.Name, err) - } - if _, hasKey := additionalTrustBundle.Data["ca-bundle.crt"]; !hasKey { - return additionalTrustBundle, fmt.Errorf(" additionalTrustBundle %s/%s missing %q key", additionalTrustBundle.Namespace, additionalTrustBundle.Name, "ca-bundle.crt") + additionalTrustBundle, err := getConfigMapWithCABundle(ctx, r.Client, hostedCluster.Namespace, hostedCluster.Spec.AdditionalTrustBundle.Name) + if err != nil { + return &corev1.ConfigMap{}, fmt.Errorf("cannot get additionalTrustBundle %s/%s: %w", hostedCluster.Namespace, hostedCluster.Spec.AdditionalTrustBundle.Name, err) } return additionalTrustBundle, nil } diff --git a/hypershift-operator/controllers/nodepool/token.go b/hypershift-operator/controllers/nodepool/token.go index 514fd93700c6..e9d0e5a17ec2 100644 --- a/hypershift-operator/controllers/nodepool/token.go +++ b/hypershift-operator/controllers/nodepool/token.go @@ -34,19 +34,28 @@ import ( ) const ( - TokenSecretTokenGenerationTime = "hypershift.openshift.io/last-token-generation-time" - TokenSecretReleaseKey = "release" - TokenSecretReleaseVersionKey = "release-version" - TokenSecretTokenKey = "token" - TokenSecretPullSecretHashKey = "pull-secret-hash" - TokenSecretHCConfigurationHashKey = "hc-configuration-hash" - TokenSecretAdditionalTrustBundleKey = "additional-trust-bundle-hash" - TokenSecretCloudConfigHashKey = "cloud-config-hash" - TokenSecretConfigKey = "config" - TokenSecretAnnotation = "hypershift.openshift.io/ignition-config" - TokenSecretIgnitionReachedAnnotation = "hypershift.openshift.io/ignition-reached" - TokenSecretNodePoolUpgradeType = "hypershift.openshift.io/node-pool-upgrade-type" - TokenSecretOSStreamKey = "os-stream" + TokenSecretTokenGenerationTime = "hypershift.openshift.io/last-token-generation-time" + TokenSecretReleaseKey = "release" + TokenSecretReleaseVersionKey = "release-version" + TokenSecretTokenKey = "token" + TokenSecretPullSecretHashKey = "pull-secret-hash" + TokenSecretHCConfigurationHashKey = "hc-configuration-hash" + TokenSecretAdditionalTrustBundleKey = "additional-trust-bundle-hash" + // TokenSecretProxyTrustedCAHashKey tracks spec.configuration.proxy.trustedCA ConfigMap + // content, mirroring TokenSecretAdditionalTrustBundleKey. It is written unconditionally on + // every reconcile (like TokenSecretHCConfigurationHashKey/TokenSecretCloudConfigHashKey + // below) so the ignition-server token-secret controller can detect a proxy trustedCA content + // change for a Secret pair that CAPI is intentionally keeping under its existing name (see + // effectiveSecrets and the versioned rollout baseline) and invalidate its cached payload + // instead of serving stale content indefinitely until the token naturally rotates/expires. + TokenSecretProxyTrustedCAHashKey = "proxy-trusted-ca-hash" + TokenSecretRenderedTrustBundleHashKey = "rendered-trust-bundle-hash" + TokenSecretCloudConfigHashKey = "cloud-config-hash" + TokenSecretConfigKey = "config" + TokenSecretAnnotation = "hypershift.openshift.io/ignition-config" + TokenSecretIgnitionReachedAnnotation = "hypershift.openshift.io/ignition-reached" + TokenSecretNodePoolUpgradeType = "hypershift.openshift.io/node-pool-upgrade-type" + TokenSecretOSStreamKey = "os-stream" ) // Token knows how to create an UUUID token for a unique configGenerator Hash. @@ -56,12 +65,12 @@ type Token struct { upsert.CreateOrUpdateProvider cpoCapabilities *CPOCapabilities *ConfigGenerator - // TODO(alberto): we don't really support content inplace changes for fields like pull secret and AdditionalTrustBundle. - // In fact we only trigger a rollout if the .Name referenced in the field changes. - // Consider removing these hash checks and consolidate with the rolloutConfig struct input. - // This is kept like this for now to contain the scope of the refactor and avoid backward compatibility issues. + // These content hashes are stored in the token Secret for the ignition-server + // token-secret controller. They are separate from the rolloutConfig hash that + // drives NodePool rollouts (see ConfigGenerator.RolloutHash). pullSecretHash []byte additionalTrustBundleHash []byte + proxyTrustedCAHash []byte globalConfigHash []byte cloudConfigHash []byte userData *userData @@ -125,6 +134,9 @@ func NewToken(ctx context.Context, configGenerator *ConfigGenerator, cpoCapabili return nil, err } additionalTrustBundle = additionalTrustBundleCM.Data["ca-bundle.crt"] + if configGenerator.renderedTrustBundleHash != "" && supportutil.HashSimple(additionalTrustBundle) != configGenerator.additionalTrustBundleHash { + return nil, &TrustBundleConfigError{err: fmt.Errorf("additional trust bundle changed while building token; retrying")} + } } // TODO(alberto): This hash should be consolidated with configGenerator using globalConfigString as that is what configGenerator uses to create a configGenerator.Hash() and so what triggers a rollout. @@ -149,6 +161,7 @@ func NewToken(ctx context.Context, configGenerator *ConfigGenerator, cpoCapabili cpoCapabilities: cpoCapabilities, pullSecretHash: []byte(supportutil.HashSimple(pullSecretBytes)), additionalTrustBundleHash: []byte(supportutil.HashSimple(additionalTrustBundle)), + proxyTrustedCAHash: []byte(configGenerator.proxyTrustedCAHash), globalConfigHash: []byte(hcConfigurationHash), cloudConfigHash: []byte(cloudConfigHash), } @@ -310,6 +323,9 @@ func setExpirationTimestampOnToken(ctx context.Context, c client.Client, tokenSe func (t *Token) Reconcile(ctx context.Context) error { log := ctrl.LoggerFrom(ctx) + if err := t.validateTrustBundleCapabilities(ctx); err != nil { + return err + } outdated := t.isOutdated() if outdated { @@ -349,6 +365,38 @@ func (t *Token) Reconcile(ctx context.Context) error { return nil } +func (t *Token) validateTrustBundleCapabilities(ctx context.Context) error { + if t.cpoCapabilities.RenderedTrustBundleValidation { + return nil + } + // Old ignition servers ignore new Secret keys. Fail before token cleanup or + // bootstrap propagation rather than letting an unvalidated payload roll nodes. + if (len(t.additionalTrustBundleHash) > 0 && string(t.additionalTrustBundleHash) != supportutil.HashSimple("")) || len(t.proxyTrustedCAHash) > 0 { + return fmt.Errorf("trust bundle reconciliation requires a CPO image with rendered-trust-bundle-validation support") + } + // Also protect removal of the last bundle when the operator/CPO is downgraded. + // The deployed payload can differ from the completed one during a rollout. + // Check both before cleanup; a bundle may exist only in the in-flight target. + for _, hash := range []string{t.nodePool.Annotations[nodePoolAnnotationCurrentConfigVersion], t.deployedBootstrapHash} { + if hash == "" { + continue + } + previous, _ := t.secretsForHash(hash) + if err := t.Get(ctx, client.ObjectKeyFromObject(previous), previous); err != nil { + if apierrors.IsNotFound(err) { + continue + } + return err + } + for _, key := range []string{TokenSecretRenderedTrustBundleHashKey, TokenSecretAdditionalTrustBundleKey, TokenSecretProxyTrustedCAHashKey} { + if value := string(previous.Data[key]); value != "" && value != supportutil.HashSimple("") { + return fmt.Errorf("trust bundle removal requires a CPO image with rendered-trust-bundle-validation support") + } + } + } + return nil +} + // SetDeployedBootstrapHash records the full hash extracted from the CAPI // workload's current bootstrap secret reference (e.g. MachineDeployment or // MachineSet). Must be called before Reconcile() so effectiveSecrets() can @@ -653,6 +701,18 @@ func (t *Token) reconcileTokenSecret(tokenSecret *corev1.Secret) error { // Otherwise if something which does not trigger a new token generation from spec.Config changes, like .IDP, both hashes would mismatch forever. tokenSecret.Data[TokenSecretHCConfigurationHashKey] = t.globalConfigHash tokenSecret.Data[TokenSecretCloudConfigHashKey] = t.cloudConfigHash + // Also kept in sync in place (see comment above): lets the ignition-server token-secret + // controller invalidate a cached payload when proxy.trustedCA content changes for a token + // Secret that CAPI is intentionally keeping under its existing name/token (hash-formula + // migration skip, see config_hash_version.go), instead of serving stale CA content until the + // token naturally rotates or expires. + tokenSecret.Data[TokenSecretProxyTrustedCAHashKey] = t.proxyTrustedCAHash + if t.renderedTrustBundleHash != "" { + tokenSecret.Data[TokenSecretRenderedTrustBundleHashKey] = []byte(t.renderedTrustBundleHash) + } + // Retained bootstrap tokens must follow the migration baseline too. Otherwise + // a cache rebuild rejects a token whose additional bundle changed before migration. + tokenSecret.Data[TokenSecretAdditionalTrustBundleKey] = t.additionalTrustBundleHash return nil } diff --git a/hypershift-operator/controllers/nodepool/token_test.go b/hypershift-operator/controllers/nodepool/token_test.go index 88291fa9f837..4a9d0e5ae577 100644 --- a/hypershift-operator/controllers/nodepool/token_test.go +++ b/hypershift-operator/controllers/nodepool/token_test.go @@ -709,7 +709,8 @@ func TestTokenReconcile(t *testing.T) { ignitionServerCACert, }, cpoCapabilities: &CPOCapabilities{ - DecompressAndDecodeConfig: true, + DecompressAndDecodeConfig: true, + RenderedTrustBundleValidation: true, }, }, { @@ -771,7 +772,8 @@ func TestTokenReconcile(t *testing.T) { ignitionServerCACert, }, cpoCapabilities: &CPOCapabilities{ - DecompressAndDecodeConfig: true, + DecompressAndDecodeConfig: true, + RenderedTrustBundleValidation: true, }, }, } @@ -839,6 +841,7 @@ func TestTokenReconcile(t *testing.T) { g.Expect(gotTokenSecret.Data[TokenSecretPullSecretHashKey]).To(Equal(expectedPullSecretHash)) g.Expect(gotTokenSecret.Data[TokenSecretAdditionalTrustBundleKey]).To(Equal(expectedAdditionalTrustBundleHash)) g.Expect(gotTokenSecret.Data[TokenSecretHCConfigurationHashKey]).To(Equal([]byte(expectedGlobalConfig))) + g.Expect(gotTokenSecret.Data[TokenSecretProxyTrustedCAHashKey]).To(Equal([]byte(tc.configGenerator.proxyTrustedCAHash))) // Validate the os-stream key is set to the resolved RHEL stream. g.Expect(gotTokenSecret.Data[TokenSecretOSStreamKey]).To(Equal([]byte(tc.configGenerator.resolvedRHELStreamForBootImage))) @@ -936,7 +939,7 @@ func TestTokenUserDataSecret(t *testing.T) { }, }, pullSecretName: "test-pull-secret", - additionalTrustBundleName: "test-trust-bundle", + additionalTrustBundleHash: "test-trust-bundle-hash", globalConfig: "test-global-config", mcoRawConfig: "test-mco-raw-config", }, @@ -1377,6 +1380,72 @@ func TestSetKarpenterAMILabels(t *testing.T) { } } +func testReconcileTokenSecretProxyTrustedCAHash(t *testing.T) { + testCases := []struct { + name string + proxyTrustedCAHash string + }{ + { + name: "When ConfigGenerator.proxyTrustedCAHash is set, it should write it to the token Secret", + proxyTrustedCAHash: "proxy-trusted-ca-hash-value", + }, + { + name: "When ConfigGenerator.proxyTrustedCAHash is empty, it should write an empty value to the token Secret", + proxyTrustedCAHash: "", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + + token := &Token{ + ConfigGenerator: &ConfigGenerator{ + hostedCluster: &hyperv1.HostedCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-hc", + Namespace: "test-namespace", + }, + }, + nodePool: &hyperv1.NodePool{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-nodepool", + }, + Spec: hyperv1.NodePoolSpec{ + Management: hyperv1.NodePoolManagement{ + UpgradeType: hyperv1.UpgradeTypeReplace, + }, + }, + }, + rolloutConfig: &rolloutConfig{ + proxyTrustedCAHash: tc.proxyTrustedCAHash, + releaseImage: &releaseinfo.ReleaseImage{ + ImageStream: &imageapi.ImageStream{ + ObjectMeta: metav1.ObjectMeta{ + Name: "4.17", + }, + }, + }, + }, + }, + cpoCapabilities: &CPOCapabilities{DecompressAndDecodeConfig: true}, + proxyTrustedCAHash: []byte(tc.proxyTrustedCAHash), + } + + tokenSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "token-test", + Namespace: "test-namespace", + }, + } + + err := token.reconcileTokenSecret(tokenSecret) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(tokenSecret.Data[TokenSecretProxyTrustedCAHashKey]).To(Equal([]byte(tc.proxyTrustedCAHash))) + }) + } +} + func TestReconcileUserDataSecret(t *testing.T) { testCases := []struct { name string diff --git a/hypershift-operator/controllers/nodepool/trust_bundle_regression_test.go b/hypershift-operator/controllers/nodepool/trust_bundle_regression_test.go new file mode 100644 index 000000000000..3a5629b2abe0 --- /dev/null +++ b/hypershift-operator/controllers/nodepool/trust_bundle_regression_test.go @@ -0,0 +1,187 @@ +package nodepool + +import ( + "fmt" + "testing" + + hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" + "github.com/openshift/hypershift/support/releaseinfo" + "github.com/openshift/hypershift/support/thirdparty/library-go/pkg/image/dockerv1client" + "github.com/openshift/hypershift/support/util" + fakeimagemetadataprovider "github.com/openshift/hypershift/support/util/fakeimagemetadataprovider" + + configv1 "github.com/openshift/api/config/v1" + "github.com/openshift/api/image/docker10" + imageapi "github.com/openshift/api/image/v1" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +func migrationConfigFixture() *ConfigGenerator { + return &ConfigGenerator{ + hostedCluster: &hyperv1.HostedCluster{ObjectMeta: metav1.ObjectMeta{Name: "hc", Namespace: "clusters"}, Spec: hyperv1.HostedClusterSpec{AdditionalTrustBundle: &corev1.LocalObjectReference{Name: "ca"}}}, + nodePool: &hyperv1.NodePool{ObjectMeta: metav1.ObjectMeta{Name: "workers", Namespace: "clusters"}, Spec: hyperv1.NodePoolSpec{Management: hyperv1.NodePoolManagement{UpgradeType: hyperv1.UpgradeTypeReplace}}}, + controlplaneNamespace: "clusters-hc", + rolloutConfig: &rolloutConfig{ + releaseImage: &releaseinfo.ReleaseImage{ImageStream: &imageapi.ImageStream{ObjectMeta: metav1.ObjectMeta{Name: "4.21.10"}}}, + additionalTrustBundleHash: util.HashSimple("additional"), + proxyTrustedCAHash: util.HashSimple("proxy"), + }, + } +} + +func TestRenderedTrustBundleHash(t *testing.T) { + for _, tc := range []struct { + name, additional, proxy string + missing, changed bool + }{ + {"When both bundles exist, it should hash additional followed by proxy", "additional", "proxy", false, false}, + {"When no bundles exist, it should hash the empty bundle", "", "", false, false}, + {"When a bundle is missing, it should return an error", "additional", "proxy", true, false}, + {"When source content changes between reads, it should reject the inconsistent snapshot", "additional", "proxy", false, true}, + } { + t.Run(tc.name, func(t *testing.T) { + hc := &hyperv1.HostedCluster{ObjectMeta: metav1.ObjectMeta{Namespace: "clusters"}} + var objects []client.Object + var ah, ph string + if tc.additional != "" { + hc.Spec.AdditionalTrustBundle = &corev1.LocalObjectReference{Name: "additional"} + ah = util.HashSimple(tc.additional) + objects = append(objects, &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "additional", Namespace: hc.Namespace}, Data: map[string]string{"ca-bundle.crt": tc.additional}}) + } + if tc.proxy != "" { + hc.Spec.Configuration = &hyperv1.ClusterConfiguration{Proxy: &configv1.ProxySpec{TrustedCA: configv1.ConfigMapNameReference{Name: "proxy"}}} + ph = util.HashSimple(tc.proxy) + if !tc.missing { + objects = append(objects, &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "proxy", Namespace: hc.Namespace}, Data: map[string]string{"ca-bundle.crt": tc.proxy}}) + } + } + if tc.changed { + ph = util.HashSimple("old") + } + c := fake.NewClientBuilder().WithObjects(objects...).Build() + hash, err := renderedTrustBundleHash(t.Context(), c, hc, ah, ph) + if tc.missing || tc.changed { + if err == nil { + t.Fatal("expected error") + } + return + } + if err != nil { + t.Fatal(err) + } + if hash != util.HashSimple(tc.additional+tc.proxy) { + t.Fatal("unexpected aggregate hash") + } + }) + } +} + +func testHashAtVersionContentEquivalentReferences(t *testing.T) { + cg := migrationConfigFixture() + before := cg.Hash() + cg.hostedCluster.Spec.AdditionalTrustBundle.Name = "identical-ca-different-name" + if cg.Hash() != before { + t.Fatal("identical content under another name should not roll nodes") + } +} + +func TestValidateTrustBundleCapabilities(t *testing.T) { + for _, tc := range []struct { + name string + supported, current, previous, wantError, inFlight bool + }{ + {"When the CPO supports validation, it should accept trust bundles", true, true, false, false, false}, + {"When the CPO lacks validation, it should block a trust bundle before cleanup", false, true, false, true, false}, + {"When the last bundle is removed with an older CPO, it should block propagation", false, false, true, true, false}, + {"When a bundle exists only in the deployed target, it should block removal with an older CPO", false, false, true, true, true}, + {"When there are no trust bundles, it should retain old CPO compatibility", false, false, false, false, false}, + } { + t.Run(tc.name, func(t *testing.T) { + cg := migrationConfigFixture() + cg.nodePool.Annotations = map[string]string{nodePoolAnnotationCurrentConfigVersion: "legacy"} + previous := &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "token-workers-legacy", Namespace: cg.controlplaneNamespace}, Data: map[string][]byte{}} + if tc.previous { + previous.Data[TokenSecretAdditionalTrustBundleKey] = []byte(util.HashSimple("old")) + } + cg.Client = fake.NewClientBuilder().WithObjects(previous).Build() + token := &Token{ConfigGenerator: cg, cpoCapabilities: &CPOCapabilities{RenderedTrustBundleValidation: tc.supported}} + if tc.inFlight { + token.SetDeployedBootstrapHash("legacy") + cg.nodePool.Annotations[nodePoolAnnotationCurrentConfigVersion] = "completed" + } + if tc.current { + token.proxyTrustedCAHash = []byte(util.HashSimple("proxy")) + } + err := token.validateTrustBundleCapabilities(t.Context()) + if (err != nil) != tc.wantError { + t.Fatalf("error=%v, wantError=%v", err, tc.wantError) + } + }) + } +} + +func TestDetectCPOCapabilities(t *testing.T) { + for _, tc := range []struct { + name string + enabled, missingSecret, metadataError bool + }{ + {"When the selected image advertises validation, it should enable the capability", true, false, false}, + {"When the selected image lacks validation, it should leave the capability disabled", false, false, false}, + {"When the pull secret is missing, it should return an error", false, true, false}, + {"When metadata lookup fails, it should return an error", false, false, true}, + } { + t.Run(tc.name, func(t *testing.T) { + hc := &hyperv1.HostedCluster{ObjectMeta: metav1.ObjectMeta{Namespace: "clusters", Annotations: map[string]string{hyperv1.ControlPlaneOperatorImageAnnotation: "quay.io/hypershift/hypershift-operator:latest"}}, Spec: hyperv1.HostedClusterSpec{PullSecret: corev1.LocalObjectReference{Name: "pull-secret"}}} + var objects []client.Object + if !tc.missingSecret { + objects = append(objects, &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "pull-secret", Namespace: "clusters"}, Data: map[string][]byte{corev1.DockerConfigJsonKey: []byte(`{"auths":{}}`)}}) + } + labels := map[string]string{} + if tc.enabled { + labels["io.openshift.hypershift.control-plane-operator-manages.rendered-trust-bundle-validation"] = "true" + } + provider := &fakeimagemetadataprovider.FakeRegistryClientImageMetadataProvider{Result: &dockerv1client.DockerImageConfig{Config: &docker10.DockerConfig{Labels: labels}}} + if tc.metadataError { + provider.Err = fmt.Errorf("metadata unavailable") + } + caps, err := DetectCPOCapabilities(t.Context(), fake.NewClientBuilder().WithObjects(objects...).Build(), hc, nil, provider, "") + if tc.missingSecret || tc.metadataError { + if err == nil { + t.Fatal("expected error") + } + return + } + if err != nil { + t.Fatal(err) + } + if caps.RenderedTrustBundleValidation != tc.enabled { + t.Fatal("incorrect capability") + } + }) + } +} + +func TestReconcileTokenSecret(t *testing.T) { + t.Run("When the proxy bundle changes, it should update the token hash", testReconcileTokenSecretProxyTrustedCAHash) + cg := migrationConfigFixture() + cg.renderedTrustBundleHash = util.HashSimple("updated-additional" + "proxy") + token := &Token{ConfigGenerator: cg, additionalTrustBundleHash: []byte(util.HashSimple("updated-additional")), proxyTrustedCAHash: []byte(cg.proxyTrustedCAHash)} + secret := &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "token-workers-legacy", Namespace: cg.controlplaneNamespace}, Data: map[string][]byte{TokenSecretTokenKey: []byte("retained-token"), TokenSecretAdditionalTrustBundleKey: []byte(util.HashSimple("old-additional"))}} + if err := token.reconcileTokenSecret(secret); err != nil { + t.Fatal(err) + } + if string(secret.Data[TokenSecretTokenKey]) != "retained-token" { + t.Fatal("migration rotated the bootstrap identity") + } + if string(secret.Data[TokenSecretAdditionalTrustBundleKey]) != string(token.additionalTrustBundleHash) { + t.Fatal("retained token kept a stale additional trust hash") + } + if string(secret.Data[TokenSecretRenderedTrustBundleHashKey]) != cg.renderedTrustBundleHash { + t.Fatal("retained token is missing rendered bundle validation") + } +} diff --git a/hypershift-operator/controllers/nodepool/trust_bundle_rollout_migration_test.go b/hypershift-operator/controllers/nodepool/trust_bundle_rollout_migration_test.go new file mode 100644 index 000000000000..7735be809bae --- /dev/null +++ b/hypershift-operator/controllers/nodepool/trust_bundle_rollout_migration_test.go @@ -0,0 +1,136 @@ +package nodepool + +import ( + "encoding/base64" + "reflect" + "strings" + "testing" + + hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" + "github.com/openshift/hypershift/support/api" + "github.com/openshift/hypershift/support/upsert" + "github.com/openshift/hypershift/support/util" + + configv1 "github.com/openshift/api/config/v1" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/ptr" + + capiv1 "sigs.k8s.io/cluster-api/api/core/v1beta2" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + "github.com/go-logr/logr" +) + +// Exercise the production token state machine, seeding and CAPI propagation in +// sequence. Legacy rollout annotations deliberately remain present: they must +// never be mistaken for a content-aware baseline. +func testSeedRolloutAnnotationTrustBundleMigration(t *testing.T) { + for _, upgradeType := range []hyperv1.UpgradeType{hyperv1.UpgradeTypeReplace, hyperv1.UpgradeTypeInPlace} { + for _, inFlight := range []bool{false, true} { + for _, field := range []string{"additional", "proxy"} { + name := "When " + string(upgradeType) + " migrates with " + field + if inFlight { + name += " and an active rollout" + } + t.Run(name+", it should retain bootstrap until the next content edit", func(t *testing.T) { + cg := migrationConfigFixture() + np := cg.nodePool + np.Spec.Management.UpgradeType = upgradeType + np.Status.Version = cg.Version() + legacy := cg.hashAtVersion(ConfigHashVersionV1) + completed := legacy + if inFlight { + completed = "previous-completed" + } + np.Annotations = map[string]string{ + nodePoolAnnotationCurrentConfigVersion: completed, + "hypershift.openshift.io/nodePoolCurrentRolloutConfig": "legacy-name-based-baseline", + "hypershift.openshift.io/nodePoolInProgressRolloutConfig": "legacy-request", + } + // Content changed before the operator upgrade, without changing the reference. + cg.additionalTrustBundleHash = util.HashSimple("pre-upgrade-edit") + cg.renderedTrustBundleHash = util.HashSimple("pre-upgrade-editproxy") + cg.Client = fake.NewClientBuilder().WithScheme(api.Scheme).Build() + token := &Token{ConfigGenerator: cg, CreateOrUpdateProvider: upsert.New(false), + cpoCapabilities: &CPOCapabilities{DecompressAndDecodeConfig: true, RenderedTrustBundleValidation: true}, + additionalTrustBundleHash: []byte(cg.additionalTrustBundleHash), proxyTrustedCAHash: []byte(cg.proxyTrustedCAHash), + userData: &userData{ignitionServerEndpoint: "ignition.example.com", proxy: &configv1.Proxy{}}, + deployedBootstrapHash: legacy, deployedVersion: cg.Version()} + md := &capiv1.MachineDeployment{ObjectMeta: metav1.ObjectMeta{Name: np.Name, Namespace: cg.controlplaneNamespace}} + md.Spec.Template.Spec.Version = cg.Version() + md.Spec.Template.Spec.InfrastructureRef.Name = "template" + md.Spec.Template.Spec.Bootstrap.DataSecretName = ptr.To("user-data-workers-" + legacy) + ms := &capiv1.MachineSet{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{}}, Spec: capiv1.MachineSetSpec{Template: md.Spec.Template}} + before := md.Spec.Template.DeepCopy() + capi := &CAPI{Token: token} + propagate := func() bool { + tmpl := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "template"}} + if upgradeType == hyperv1.UpgradeTypeInPlace { + return capi.propagateVersionAndTemplateToMachineSet(logr.Discard(), ms, tmpl) + } + return capi.propagateVersionAndTemplate(logr.Discard(), md, tmpl) + } + if err := token.Reconcile(t.Context()); err != nil { + t.Fatal(err) + } + seedRolloutAnnotation(token) + if propagate() { + t.Fatal("formula migration changed the workload spec") + } + if !reflect.DeepEqual(*before, md.Spec.Template) || !reflect.DeepEqual(*before, ms.Spec.Template) { + t.Fatal("migration changed a machine template") + } + if token.EffectiveHash() != legacy || np.Annotations[nodePoolAnnotationCurrentConfigVersion] != completed { + t.Fatal("migration rewrote a bootstrap identity") + } + if np.Annotations[nodePoolAnnotationConfigHashVersion] != ConfigHashVersionV2 { + t.Fatal("missing version marker") + } + baseline := np.Annotations[nodePoolAnnotationCurrentRolloutConfig] + // Simulate reloading the persisted workload request and a token rotation. + token.deployedRolloutConfig = token.RolloutHash() + secret, userData := token.secretsForHash(legacy) + if err := cg.Get(t.Context(), client.ObjectKeyFromObject(secret), secret); err != nil { + t.Fatal(err) + } + secret.Data[TokenSecretTokenKey] = []byte("rotated-token") + if err := cg.Update(t.Context(), secret); err != nil { + t.Fatal(err) + } + cg.mcoRawConfig += "management-only image change" + if err := token.Reconcile(t.Context()); err != nil { + t.Fatal(err) + } + seedRolloutAnnotation(token) + if propagate() || token.EffectiveHash() != legacy { + t.Fatal("management-only change rolled the workload") + } + if err := cg.Get(t.Context(), client.ObjectKeyFromObject(userData), userData); err != nil { + t.Fatal(err) + } + if !strings.Contains(string(userData.Data["value"]), base64.StdEncoding.EncodeToString([]byte("rotated-token"))) { + t.Fatal("retained user data did not follow token rotation") + } + if field == "additional" { + cg.additionalTrustBundleHash = util.HashSimple("post-migration-edit") + } else { + cg.proxyTrustedCAHash = util.HashSimple("post-migration-edit") + } + if err := token.Reconcile(t.Context()); err != nil { + t.Fatal(err) + } + seedRolloutAnnotation(token) + if !propagate() || token.EffectiveHash() == legacy { + t.Fatal("subsequent content change did not propagate") + } + if np.Annotations[nodePoolAnnotationCurrentRolloutConfig] != baseline { + t.Fatal("unfinished rollout advanced the completed baseline") + } + }) + } + } + } +} diff --git a/ignition-server/cmd/run_local_ignitionprovider.go b/ignition-server/cmd/run_local_ignitionprovider.go index 8ad98cf0b8c1..2f36593865c1 100644 --- a/ignition-server/cmd/run_local_ignitionprovider.go +++ b/ignition-server/cmd/run_local_ignitionprovider.go @@ -113,7 +113,7 @@ func (o *RunLocalIgnitionProviderOptions) Run(ctx context.Context) error { } osStream := string(token.Data[controllers.TokenSecretOSStreamKey]) - payload, err := p.GetPayload(ctx, o.Image, config.String(), "", "", "", osStream, "") + payload, err := p.GetPayload(ctx, o.Image, config.String(), "", "", "", osStream, "", string(token.Data[controllers.TokenSecretRenderedTrustBundleHashKey])) if err != nil { return err } diff --git a/ignition-server/controllers/cache.go b/ignition-server/controllers/cache.go index 6811494189bc..1ab529841edf 100644 --- a/ignition-server/controllers/cache.go +++ b/ignition-server/controllers/cache.go @@ -32,9 +32,20 @@ type ExpiringCache struct { } type CacheValue struct { - Payload []byte - SecretName string - CloudConfigHash string + RenderedTrustBundleHash string + Payload []byte + SecretName string + CloudConfigHash string + // AdditionalTrustBundleHash and ProxyTrustedCAHash are compared against the token Secret's + // current values on every reconcile, alongside CloudConfigHash, to decide whether a cached + // payload is still valid. Without this, a trust-bundle content change for a token Secret that + // is not renamed (e.g. an in-place ca-bundle.crt edit, or a Secret pair CAPI intentionally + // keeps under its existing name across a hash-formula migration, see + // hypershift-operator/controllers/nodepool/config_hash_version.go) would never invalidate an + // already-cached payload, serving stale trust-bundle content until the token naturally + // rotates or expires (up to the cache ttl). + AdditionalTrustBundleHash string + ProxyTrustedCAHash string } type entry struct { diff --git a/ignition-server/controllers/local_ignitionprovider.go b/ignition-server/controllers/local_ignitionprovider.go index b0973c662ad5..323fa62a4210 100644 --- a/ignition-server/controllers/local_ignitionprovider.go +++ b/ignition-server/controllers/local_ignitionprovider.go @@ -241,7 +241,7 @@ func (p *LocalIgnitionProvider) fetchBootstrapKubeConfig(ctx context.Context) ([ return data, nil } -func (p *LocalIgnitionProvider) fetchAndValidateMCSConfig(ctx context.Context, hcConfigurationHash string) (*corev1.ConfigMap, error) { +func (p *LocalIgnitionProvider) fetchAndValidateMCSConfig(ctx context.Context, hcConfigurationHash, expectedBundleHash string) (*corev1.ConfigMap, error) { mcsConfig := &corev1.ConfigMap{} if err := p.Client.Get(ctx, client.ObjectKey{Namespace: p.Namespace, Name: "machine-config-server"}, mcsConfig); err != nil { return nil, fmt.Errorf("failed to get machine-config-server configmap: %w", err) @@ -255,6 +255,9 @@ func (p *LocalIgnitionProvider) fetchAndValidateMCSConfig(ctx context.Context, h if err := yaml.Unmarshal([]byte(mcsConfig.Data["user-ca-bundle-config.yaml"]), &userCaBundleConfigCM); err != nil { return nil, fmt.Errorf("failed to unmarshal user-ca-bundle-config.yaml: %w", err) } + if expectedBundleHash != "" && util.HashSimple(userCaBundleConfigCM.Data["ca-bundle.crt"]) != expectedBundleHash { + return nil, fmt.Errorf("rendered trust bundle does not match expected hash; waiting for CA propagation") + } managedTrustedBundle := &corev1.ConfigMap{} if err := p.Client.Get(ctx, client.ObjectKey{Namespace: p.Namespace, Name: managedTrustBundleName}, managedTrustedBundle); err != nil { @@ -685,7 +688,7 @@ func (p *LocalIgnitionProvider) runMCSAndFetchPayload(ctx context.Context, dirs return payload, nil } -func (p *LocalIgnitionProvider) GetPayload(ctx context.Context, releaseImage, customConfig, pullSecretHash, additionalTrustBundleHash, hcConfigurationHash, osStream, cloudConfigHash string) ([]byte, error) { +func (p *LocalIgnitionProvider) GetPayload(ctx context.Context, releaseImage, customConfig, pullSecretHash, additionalTrustBundleHash, hcConfigurationHash, osStream, cloudConfigHash, renderedTrustBundleHash string) ([]byte, error) { p.lock.Lock() defer p.lock.Unlock() @@ -717,7 +720,7 @@ func (p *LocalIgnitionProvider) GetPayload(ctx context.Context, releaseImage, cu return nil, fmt.Errorf("failed to get bootstrap kubeconfig: %w", err) } - mcsConfig, err := p.fetchAndValidateMCSConfig(ctx, hcConfigurationHash) + mcsConfig, err := p.fetchAndValidateMCSConfig(ctx, hcConfigurationHash, renderedTrustBundleHash) if err != nil { return nil, err } diff --git a/ignition-server/controllers/local_ignitionprovider_test.go b/ignition-server/controllers/local_ignitionprovider_test.go index 6ce3836b2333..a36c2f267d79 100644 --- a/ignition-server/controllers/local_ignitionprovider_test.go +++ b/ignition-server/controllers/local_ignitionprovider_test.go @@ -1156,7 +1156,7 @@ func TestFetchAndValidateMCSConfig(t *testing.T) { Namespace: "test-ns", } - result, err := provider.fetchAndValidateMCSConfig(t.Context(), tt.hcConfigurationHash) + result, err := provider.fetchAndValidateMCSConfig(t.Context(), tt.hcConfigurationHash, "") if tt.expectError != "" { g.Expect(err).To(HaveOccurred()) g.Expect(err.Error()).To(ContainSubstring(tt.expectError)) diff --git a/ignition-server/controllers/tokensecret_controller.go b/ignition-server/controllers/tokensecret_controller.go index 92c8a4d69e42..82751d191218 100644 --- a/ignition-server/controllers/tokensecret_controller.go +++ b/ignition-server/controllers/tokensecret_controller.go @@ -35,10 +35,14 @@ const ( TokenSecretPullSecretHashKey = "pull-secret-hash" TokenSecretHCConfigurationHashKey = "hc-configuration-hash" TokenSecretAdditionalTrustBundleHashKey = "additional-trust-bundle-hash" - TokenSecretCloudConfigHashKey = "cloud-config-hash" - InvalidConfigReason = "InvalidConfig" - CloudConfigPendingReason = "CloudConfigPending" - TokenSecretReasonKey = "reason" + // TokenSecretProxyTrustedCAHashKey is intentionally duplicated from nodepool/token.go + // to avoid a dependency from ignition-server -> hypershift-operator. + TokenSecretProxyTrustedCAHashKey = "proxy-trusted-ca-hash" + TokenSecretRenderedTrustBundleHashKey = "rendered-trust-bundle-hash" + TokenSecretCloudConfigHashKey = "cloud-config-hash" + InvalidConfigReason = "InvalidConfig" + CloudConfigPendingReason = "CloudConfigPending" + TokenSecretReasonKey = "reason" // TokenSecretOSStreamKey is intentionally duplicated from nodepool/token.go // to avoid a dependency from ignition-server → hypershift-operator. TokenSecretOSStreamKey = "os-stream" @@ -90,7 +94,7 @@ func NewPayloadStore() *ExpiringCache { type IgnitionProvider interface { // GetPayload returns the ignition payload content for // the provided release image and a config string containing 0..N MachineConfig yaml definitions. - GetPayload(ctx context.Context, payloadImage, config, pullSecretHash, additionalTrustBundleHash, hcConfigurationHash, osStream, cloudConfigHash string) ([]byte, error) + GetPayload(ctx context.Context, payloadImage, config, pullSecretHash, additionalTrustBundleHash, hcConfigurationHash, osStream, cloudConfigHash, renderedTrustBundleHash string) ([]byte, error) } // TokenSecretReconciler watches token Secrets @@ -234,9 +238,12 @@ func (r *TokenSecretReconciler) Reconcile(ctx context.Context, req ctrl.Request) token := string(tokenSecret.Data[TokenSecretTokenKey]) cloudConfigHash := string(tokenSecret.Data[TokenSecretCloudConfigHashKey]) + additionalTrustBundleHash := string(tokenSecret.Data[TokenSecretAdditionalTrustBundleHashKey]) + proxyTrustedCAHash := string(tokenSecret.Data[TokenSecretProxyTrustedCAHashKey]) + renderedTrustBundleHash := string(tokenSecret.Data[TokenSecretRenderedTrustBundleHashKey]) if value, ok := r.PayloadStore.Get(token); ok { - if value.CloudConfigHash != cloudConfigHash { - log.Info("Cloud config hash changed, invalidating cached payload") + if !cachedPayloadStillValid(value, cloudConfigHash, additionalTrustBundleHash, proxyTrustedCAHash, renderedTrustBundleHash) { + log.Info("Content hash changed, invalidating cached payload") r.PayloadStore.Delete(token) } else { log.Info("Payload found in cache") @@ -256,11 +263,11 @@ func (r *TokenSecretReconciler) Reconcile(ctx context.Context, req ctrl.Request) oldToken, ok := tokenSecret.Data[TokenSecretOldTokenKey] if ok { if value, ok := r.PayloadStore.Get(string(oldToken)); ok { - if value.CloudConfigHash == cloudConfigHash { + if cachedPayloadStillValid(value, cloudConfigHash, additionalTrustBundleHash, proxyTrustedCAHash, renderedTrustBundleHash) { r.PayloadStore.Set(token, value) return ctrl.Result{RequeueAfter: ttl/2 - durationDeref(timeLived)}, nil } - log.Info("Cloud config hash changed, invalidating old token cached payload") + log.Info("Content hash changed, invalidating old token cached payload") r.PayloadStore.Delete(string(oldToken)) } } @@ -287,11 +294,10 @@ func (r *TokenSecretReconciler) Reconcile(ctx context.Context, req ctrl.Request) PayloadCacheMissTotal.Inc() pullSecretHash := string(tokenSecret.Data[TokenSecretPullSecretHashKey]) hcConfigurationHash := string(tokenSecret.Data[TokenSecretHCConfigurationHashKey]) - additionalTrustBundleHash := string(tokenSecret.Data[TokenSecretAdditionalTrustBundleHashKey]) osStream := string(tokenSecret.Data[TokenSecretOSStreamKey]) payload, err := func() ([]byte, error) { start := time.Now() - payload, err := r.IgnitionProvider.GetPayload(ctx, releaseImage, config.String(), pullSecretHash, additionalTrustBundleHash, hcConfigurationHash, osStream, cloudConfigHash) + payload, err := r.IgnitionProvider.GetPayload(ctx, releaseImage, config.String(), pullSecretHash, additionalTrustBundleHash, hcConfigurationHash, osStream, cloudConfigHash, renderedTrustBundleHash) if err != nil { return nil, fmt.Errorf("error getting ignition payload: %w", err) } @@ -323,7 +329,14 @@ func (r *TokenSecretReconciler) Reconcile(ctx context.Context, req ctrl.Request) } log.Info("IgnitionProvider generated payload") - cacheValue := CacheValue{Payload: payload, SecretName: tokenSecret.Name, CloudConfigHash: cloudConfigHash} + cacheValue := CacheValue{ + Payload: payload, + SecretName: tokenSecret.Name, + CloudConfigHash: cloudConfigHash, + AdditionalTrustBundleHash: additionalTrustBundleHash, + ProxyTrustedCAHash: proxyTrustedCAHash, + RenderedTrustBundleHash: renderedTrustBundleHash, + } r.PayloadStore.Set(token, cacheValue) oldToken, ok = tokenSecret.Data[TokenSecretOldTokenKey] if ok { @@ -376,6 +389,22 @@ func getTokenTimeLived(tokenSecret *corev1.Secret, now time.Time) (*time.Duratio } // tokenIDNeedRotation returns true if a duration is longer than the ttl/2. +// cachedPayloadStillValid reports whether a cached payload is still valid for the token Secret's +// current content hashes. All three hashes are compared because any of them can change in place +// for a token Secret whose name does not change: cloudConfigHash and hcConfigurationHash are +// written unconditionally on every HyperShift Operator reconcile (see token.go), and +// additionalTrustBundleHash/proxyTrustedCAHash are written unconditionally while a Secret pair is +// intentionally kept under its existing name across a hash-formula migration (see +// hypershift-operator/controllers/nodepool/config_hash_version.go). Without checking all of them, +// a cache hit would keep serving a payload generated from stale trust-bundle content until the +// token naturally rotates or expires. +func cachedPayloadStillValid(value CacheValue, cloudConfigHash, additionalTrustBundleHash, proxyTrustedCAHash, renderedTrustBundleHash string) bool { + return value.CloudConfigHash == cloudConfigHash && + value.AdditionalTrustBundleHash == additionalTrustBundleHash && + value.ProxyTrustedCAHash == proxyTrustedCAHash && + value.RenderedTrustBundleHash == renderedTrustBundleHash +} + // This is the criteria to trigger a token ID rotation. func tokenNeedRotation(timeLived *time.Duration) bool { if timeLived == nil || *timeLived >= ttl/2 { diff --git a/ignition-server/controllers/tokensecret_controller_test.go b/ignition-server/controllers/tokensecret_controller_test.go index db1c3974364e..d0c6676e6605 100644 --- a/ignition-server/controllers/tokensecret_controller_test.go +++ b/ignition-server/controllers/tokensecret_controller_test.go @@ -30,11 +30,12 @@ var ( type fakeIgnitionProvider struct{} -func (p *fakeIgnitionProvider) GetPayload(ctx context.Context, releaseImage, config, pullSecretHash, additionalTrustBundleHash, hcConfigurationHash, osStream, cloudConfigHash string) (payload []byte, err error) { +func (p *fakeIgnitionProvider) GetPayload(ctx context.Context, releaseImage, config, pullSecretHash, additionalTrustBundleHash, hcConfigurationHash, osStream, cloudConfigHash, renderedTrustBundleHash string) (payload []byte, err error) { return []byte(fakePayload), nil } func TestReconcile(t *testing.T) { + t.Run("When trust bundle rendering is delayed, it should reject stale payloads", testReconcileRenderedTrustBundle) compressedConfig, err := util.CompressAndEncode([]byte("compressedConfig")) if err != nil { t.Fatal(err) @@ -694,7 +695,7 @@ type osStreamCapturingProvider struct { capturedOSStream string } -func (p *osStreamCapturingProvider) GetPayload(_ context.Context, _, _, _, _, _, osStream, _ string) ([]byte, error) { +func (p *osStreamCapturingProvider) GetPayload(_ context.Context, _, _, _, _, _, osStream, _, _ string) ([]byte, error) { p.capturedOSStream = osStream return []byte(fakePayload), nil } @@ -870,6 +871,107 @@ func TestCacheInvalidationOnCloudConfigHashChange(t *testing.T) { } } +func TestCacheInvalidationOnTrustBundleHashChange(t *testing.T) { + compressedConfig, err := util.CompressAndEncode([]byte("compressedConfig")) + if err != nil { + t.Fatal(err) + } + compressedConfigBytes := compressedConfig.Bytes() + + tokenID := uuid.New().String() + secretName := "test" + + tests := []struct { + name string + cachedAdditionalTrustBundle string + secretAdditionalTrustBundle string + cachedProxyTrustedCA string + secretProxyTrustedCA string + expectRegeneration bool + }{ + { + name: "When additionalTrustBundleHash and proxyTrustedCAHash match cached values, it should return cached payload", + cachedAdditionalTrustBundle: "atb-abc", + secretAdditionalTrustBundle: "atb-abc", + cachedProxyTrustedCA: "proxy-abc", + secretProxyTrustedCA: "proxy-abc", + expectRegeneration: false, + }, + { + name: "When additionalTrustBundleHash differs from cached value, it should regenerate payload", + cachedAdditionalTrustBundle: "atb-old", + secretAdditionalTrustBundle: "atb-new", + cachedProxyTrustedCA: "proxy-abc", + secretProxyTrustedCA: "proxy-abc", + expectRegeneration: true, + }, + { + name: "When only proxyTrustedCAHash differs from cached value (e.g. in-place proxy.trustedCA " + + "content change, or a Secret pair kept under its existing name across a hash-formula " + + "migration), it should regenerate payload instead of serving a stale cached payload", + cachedAdditionalTrustBundle: "atb-abc", + secretAdditionalTrustBundle: "atb-abc", + cachedProxyTrustedCA: "proxy-old", + secretProxyTrustedCA: "proxy-new", + expectRegeneration: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: secretName, + Namespace: "test", + Annotations: map[string]string{ + TokenSecretAnnotation: "true", + TokenSecretTokenGenerationTime: time.Now().Format(time.RFC3339Nano), + }, + CreationTimestamp: metav1.Now(), + }, + Data: map[string][]byte{ + TokenSecretTokenKey: []byte(tokenID), + TokenSecretReleaseKey: []byte("release"), + TokenSecretConfigKey: compressedConfigBytes, + TokenSecretAdditionalTrustBundleHashKey: []byte(tt.secretAdditionalTrustBundle), + TokenSecretProxyTrustedCAHashKey: []byte(tt.secretProxyTrustedCA), + }, + } + + callCount := 0 + provider := &countingIgnitionProvider{count: &callCount} + + r := TokenSecretReconciler{ + Client: fake.NewClientBuilder().WithObjects(secret).Build(), + IgnitionProvider: provider, + PayloadStore: NewPayloadStore(), + } + + r.PayloadStore.Set(tokenID, CacheValue{ + Payload: []byte("old-payload"), + SecretName: secretName, + AdditionalTrustBundleHash: tt.cachedAdditionalTrustBundle, + ProxyTrustedCAHash: tt.cachedProxyTrustedCA, + }) + + _, err := r.Reconcile(t.Context(), ctrl.Request{NamespacedName: client.ObjectKeyFromObject(secret)}) + g.Expect(err).ToNot(HaveOccurred()) + + if tt.expectRegeneration { + g.Expect(callCount).To(Equal(1), "expected GetPayload to be called for regeneration") + value, found := r.PayloadStore.Get(tokenID) + g.Expect(found).To(BeTrue()) + g.Expect(value.AdditionalTrustBundleHash).To(Equal(tt.secretAdditionalTrustBundle)) + g.Expect(value.ProxyTrustedCAHash).To(Equal(tt.secretProxyTrustedCA)) + } else { + g.Expect(callCount).To(Equal(0), "expected cached payload to be returned without calling GetPayload") + } + }) + } +} + func TestOldTokenFallbackWithCloudConfigHashMismatch(t *testing.T) { g := NewWithT(t) @@ -933,7 +1035,7 @@ type countingIgnitionProvider struct { count *int } -func (p *countingIgnitionProvider) GetPayload(_ context.Context, _, _, _, _, _, _, _ string) ([]byte, error) { +func (p *countingIgnitionProvider) GetPayload(_ context.Context, _, _, _, _, _, _, _, _ string) ([]byte, error) { *p.count++ return []byte("regenerated-payload"), nil } @@ -942,7 +1044,7 @@ type errorIgnitionProvider struct { err error } -func (p *errorIgnitionProvider) GetPayload(_ context.Context, _, _, _, _, _, _, _ string) ([]byte, error) { +func (p *errorIgnitionProvider) GetPayload(_ context.Context, _, _, _, _, _, _, _, _ string) ([]byte, error) { return nil, p.err } diff --git a/ignition-server/controllers/trust_bundle_regression_test.go b/ignition-server/controllers/trust_bundle_regression_test.go new file mode 100644 index 000000000000..4052264dfcc4 --- /dev/null +++ b/ignition-server/controllers/trust_bundle_regression_test.go @@ -0,0 +1,104 @@ +package controllers + +import ( + "context" + "fmt" + "testing" + "time" + + "github.com/openshift/hypershift/support/util" + + corev1 "k8s.io/api/core/v1" + 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" + "sigs.k8s.io/yaml" +) + +// Only the external MCO process is replaced. Readiness and cache reconciliation +// use the production code, including propagation of the expected bundle identity. +type validatingBundleProvider struct { + provider *LocalIgnitionProvider + calls int +} + +func (p *validatingBundleProvider) GetPayload(ctx context.Context, _, _, _, _, hcHash, _, _, bundleHash string) ([]byte, error) { + p.calls++ + cm, err := p.provider.fetchAndValidateMCSConfig(ctx, hcHash, bundleHash) + if err != nil { + return nil, err + } + bundle := &corev1.ConfigMap{} + if err := yaml.Unmarshal([]byte(cm.Data["user-ca-bundle-config.yaml"]), bundle); err != nil { + return nil, err + } + return []byte(bundle.Data["ca-bundle.crt"]), nil +} + +func TestCachedPayloadStillValid(t *testing.T) { + for _, tc := range []struct { + name string + renderedHash string + want bool + }{ + {"When the rendered bundle matches, it should reuse the payload", "expected", true}, + {"When the rendered bundle differs, it should reject the payload", "old", false}, + {"When the cached payload predates validation, it should reject the payload", "", false}, + } { + t.Run(tc.name, func(t *testing.T) { + value := CacheValue{CloudConfigHash: "cloud", AdditionalTrustBundleHash: "additional", ProxyTrustedCAHash: "proxy", RenderedTrustBundleHash: tc.renderedHash} + if got := cachedPayloadStillValid(value, "cloud", "additional", "proxy", "expected"); got != tc.want { + t.Fatalf("cache valid=%v, want %v", got, tc.want) + } + }) + } +} + +func testReconcileRenderedTrustBundle(t *testing.T) { + for _, target := range []string{"new-proxy-ca", ""} { + t.Run(fmt.Sprintf("When the proxy bundle becomes %q before rendering, it should wait and cache only matching bytes", target), func(t *testing.T) { + compressed, err := util.CompressAndEncode([]byte("config")) + if err != nil { + t.Fatal(err) + } + mcs := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "machine-config-server", Namespace: "cp"}, Data: map[string]string{"configuration-hash": "unchanged", "user-ca-bundle-config.yaml": `{"data":{"ca-bundle.crt":"old-proxy-ca"}}`}} + managed := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: managedTrustBundleName, Namespace: "cp"}, Data: map[string]string{"ca-bundle.crt": "old-proxy-ca"}} + secret := &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "token-workers-new", Namespace: "cp", CreationTimestamp: metav1.Now(), Annotations: map[string]string{TokenSecretAnnotation: "true", TokenSecretTokenGenerationTime: time.Now().Format(time.RFC3339Nano)}}, Data: map[string][]byte{ + TokenSecretTokenKey: []byte("new-token"), TokenSecretReleaseKey: []byte("release"), TokenSecretConfigKey: compressed.Bytes(), TokenSecretHCConfigurationHashKey: []byte("unchanged"), TokenSecretRenderedTrustBundleHashKey: []byte(util.HashSimple(target)), + }} + c := fake.NewClientBuilder().WithObjects(mcs, managed, secret).Build() + p := &validatingBundleProvider{provider: &LocalIgnitionProvider{Client: c, Namespace: "cp"}} + r := &TokenSecretReconciler{Client: c, IgnitionProvider: p, PayloadStore: NewPayloadStore()} + req := ctrl.Request{NamespacedName: client.ObjectKeyFromObject(secret)} + if _, err := r.Reconcile(t.Context(), req); err == nil { + t.Fatal("stale rendering must be rejected") + } + if _, found := r.PayloadStore.Get("new-token"); found { + t.Fatal("stale payload was cached") + } + mcs.Data["user-ca-bundle-config.yaml"] = fmt.Sprintf(`{"data":{"ca-bundle.crt":%q}}`, target) + managed.Data["ca-bundle.crt"] = target + if err := c.Update(t.Context(), mcs); err != nil { + t.Fatal(err) + } + if err := c.Update(t.Context(), managed); err != nil { + t.Fatal(err) + } + if _, err := r.Reconcile(t.Context(), req); err != nil { + t.Fatal(err) + } + value, found := r.PayloadStore.Get("new-token") + if !found || string(value.Payload) != target { + t.Fatalf("unexpected cached payload: %q", value.Payload) + } + if _, err := r.Reconcile(t.Context(), req); err != nil { + t.Fatal(err) + } + if p.calls != 2 { + t.Fatalf("expected rejection then successful rendering; got %d calls", p.calls) + } + }) + } +} diff --git a/karpenter-operator/controllers/karpenterignition/config_hash_baseline_test.go b/karpenter-operator/controllers/karpenterignition/config_hash_baseline_test.go new file mode 100644 index 000000000000..92341f8c5678 --- /dev/null +++ b/karpenter-operator/controllers/karpenterignition/config_hash_baseline_test.go @@ -0,0 +1,56 @@ +package karpenterignition + +import ( + "testing" + + hyperkarpenterv1 "github.com/openshift/hypershift/api/karpenter/v1" + "github.com/openshift/hypershift/support/api" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +func testUpdateConfigAnnotationsMigration(t *testing.T) { + nc := &hyperkarpenterv1.OpenshiftEC2NodeClass{ObjectMeta: metav1.ObjectMeta{Name: "workers", Annotations: map[string]string{openshiftEC2NodeClassAnnotationCurrentConfigVersion: "legacy"}}} + c := fake.NewClientBuilder().WithScheme(api.Scheme).WithObjects(nc).Build() + r := &KarpenterIgnitionReconciler{GuestClient: c} + if err := r.updateConfigAnnotations(t.Context(), nc, "legacy", "content"); err != nil { + t.Fatal(err) + } + reloaded := &hyperkarpenterv1.OpenshiftEC2NodeClass{} + if err := c.Get(t.Context(), client.ObjectKeyFromObject(nc), reloaded); err != nil { + t.Fatal(err) + } + if reloaded.Annotations[openshiftEC2NodeClassAnnotationCurrentRolloutConfig] != "content" { + t.Fatal("baseline was not persisted") + } + if reloaded.Annotations[openshiftEC2NodeClassAnnotationCurrentConfigVersion] != "legacy" { + t.Fatal("migration changed the drift identity") + } + stale := reloaded.DeepCopy() + if err := r.updateConfigAnnotations(t.Context(), reloaded, "legacy", "new-content"); err != nil { + t.Fatal(err) + } + if err := r.updateConfigAnnotations(t.Context(), stale, "legacy", "stale"); err == nil { + t.Fatal("stale writer should not overwrite the baseline") + } +} + +func TestUpdateConfigAnnotations(t *testing.T) { + t.Run("When migrating a baseline, it should preserve identity and reject stale writes", testUpdateConfigAnnotationsMigration) + nc := &hyperkarpenterv1.OpenshiftEC2NodeClass{ObjectMeta: metav1.ObjectMeta{Name: "workers"}} + c := fake.NewClientBuilder().WithScheme(api.Scheme).WithObjects(nc).Build() + r := &KarpenterIgnitionReconciler{GuestClient: c} + if err := r.updateConfigAnnotations(t.Context(), nc, "new-content", "rollout-content"); err != nil { + t.Fatal(err) + } + reloaded := &hyperkarpenterv1.OpenshiftEC2NodeClass{} + if err := c.Get(t.Context(), client.ObjectKeyFromObject(nc), reloaded); err != nil { + t.Fatal(err) + } + if reloaded.Annotations[openshiftEC2NodeClassAnnotationCurrentRolloutConfig] != "rollout-content" || reloaded.Annotations[openshiftEC2NodeClassAnnotationCurrentConfigVersion] != "new-content" { + t.Fatal("identity and baseline must advance together") + } +} diff --git a/karpenter-operator/controllers/karpenterignition/cpo_capabilities_test.go b/karpenter-operator/controllers/karpenterignition/cpo_capabilities_test.go new file mode 100644 index 000000000000..de2c87d8bfa8 --- /dev/null +++ b/karpenter-operator/controllers/karpenterignition/cpo_capabilities_test.go @@ -0,0 +1,94 @@ +package karpenterignition + +import ( + "context" + "fmt" + "reflect" + "testing" + + hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" + "github.com/openshift/hypershift/support/thirdparty/library-go/pkg/image/dockerv1client" + fakeimagemetadataprovider "github.com/openshift/hypershift/support/util/fakeimagemetadataprovider" + + "github.com/openshift/api/image/docker10" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +type recordingCPOImageProvider struct { + fakeimagemetadataprovider.FakeRegistryClientImageMetadataProvider + image string +} + +func (p *recordingCPOImageProvider) ImageMetadata(ctx context.Context, image string, pullSecret []byte) (*dockerv1client.DockerImageConfig, error) { + p.image = image + return p.FakeRegistryClientImageMetadataProvider.ImageMetadata(ctx, image, pullSecret) +} + +func TestDetectCPOCapabilities(t *testing.T) { + const runningImage = "quay.io/rhn-support-alosingh/hypershift-operator:rfe8751-review-fixes-20261013" + for _, tc := range []struct { + name string + missingImage, missingSecret, metadataError, unsupported, staleAnnotation bool + }{ + {name: "When the HCP lacks the HC override, it should inspect the running CPO image"}, + {name: "When an inherited annotation is stale, it should prefer the running CPO image", staleAnnotation: true}, + {name: "When the running image lacks validation, it should leave the capability disabled", unsupported: true}, + {name: "When the running image is empty, it should fail rather than resolve the release CPO", missingImage: true}, + {name: "When the pull secret is missing, it should return an error", missingSecret: true}, + {name: "When image metadata lookup fails, it should return an error", metadataError: true}, + } { + t.Run(tc.name, func(t *testing.T) { + hcp := &hyperv1.HostedControlPlane{ + ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "clusters-test"}, + Spec: hyperv1.HostedControlPlaneSpec{ + ReleaseImage: "quay.io/openshift-release-dev/ocp-release:5.0.0-rc.6-x86_64", + PullSecret: corev1.LocalObjectReference{Name: "pull-secret"}, + }, + } + if tc.staleAnnotation { + hcp.Annotations = map[string]string{hyperv1.ControlPlaneOperatorImageAnnotation: "quay.io/hypershift/control-plane-operator:old"} + } + hc, err := hostedClusterFromHCP(hcp, "ignition.test") + if err != nil { + t.Fatal(err) + } + originalHC, originalHCP := hc.DeepCopy(), hcp.DeepCopy() + var objects []client.Object + if !tc.missingSecret { + objects = append(objects, &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "pull-secret", Namespace: hcp.Namespace}, Data: map[string][]byte{corev1.DockerConfigJsonKey: []byte(`{"auths":{}}`)}}) + } + labels := map[string]string{} + if !tc.unsupported { + labels["io.openshift.hypershift.control-plane-operator-manages.rendered-trust-bundle-validation"] = "true" + } + provider := &recordingCPOImageProvider{FakeRegistryClientImageMetadataProvider: fakeimagemetadataprovider.FakeRegistryClientImageMetadataProvider{Result: &dockerv1client.DockerImageConfig{Config: &docker10.DockerConfig{Labels: labels}}}} + if tc.metadataError { + provider.Err = fmt.Errorf("metadata unavailable") + } + r := &KarpenterIgnitionReconciler{ManagementClient: fake.NewClientBuilder().WithObjects(objects...).Build(), ImageMetadataProvider: provider, ControlPlaneOperatorImage: runningImage} + if tc.missingImage { + r.ControlPlaneOperatorImage = "" + } + // No release provider: resolving the release instead of the running image must fail this test. + caps, err := r.detectCPOCapabilities(t.Context(), hc) + wantError := tc.missingImage || tc.missingSecret || tc.metadataError + if (err != nil) != wantError { + t.Fatalf("error=%v, wantError=%v", err, wantError) + } + if !reflect.DeepEqual(hc, originalHC) || !reflect.DeepEqual(hcp, originalHCP) { + t.Fatal("capability detection mutated the source HC or HCP") + } + if !tc.missingImage && !tc.missingSecret && provider.image != runningImage { + t.Fatalf("inspected %q, want %q", provider.image, runningImage) + } + if !wantError && caps.RenderedTrustBundleValidation == tc.unsupported { + t.Fatal("incorrect validation capability") + } + }) + } +} diff --git a/karpenter-operator/controllers/karpenterignition/karpenterignition_controller.go b/karpenter-operator/controllers/karpenterignition/karpenterignition_controller.go index 57068e10e90e..de43e4e4191d 100644 --- a/karpenter-operator/controllers/karpenterignition/karpenterignition_controller.go +++ b/karpenter-operator/controllers/karpenterignition/karpenterignition_controller.go @@ -46,25 +46,26 @@ import ( const ( openshiftEC2NodeClassAnnotationCurrentConfigVersion = "hypershift.openshift.io/nodeClassCurrentConfigVersion" - openshiftEC2NodeClassAnnotationCurrentRolloutConfig = "hypershift.openshift.io/nodeClassCurrentRolloutConfig" + openshiftEC2NodeClassAnnotationCurrentRolloutConfig = "hypershift.openshift.io/nodeClassCurrentRolloutConfigV2" // nodePoolAnnotationCurrentConfigVersion mirrors the annotation from nodepool_controller.go // It's used to track the current config version for outdated token cleanup nodePoolAnnotationCurrentConfigVersion = "hypershift.openshift.io/nodePoolCurrentConfigVersion" - nodePoolAnnotationCurrentRolloutConfig = "hypershift.openshift.io/nodePoolCurrentRolloutConfig" + nodePoolAnnotationCurrentRolloutConfig = "hypershift.openshift.io/nodePoolCurrentRolloutConfigV2" kubeletConfigFinalizer = "hypershift.openshift.io/karpenter-kubelet-config-finalizer" ) type KarpenterIgnitionReconciler struct { - ManagementClient client.Client - GuestClient client.Client - ReleaseProvider releaseinfo.Provider - VersionResolver releaseinfo.VersionResolver - ImageMetadataProvider imageregistry.ImageMetadataProvider - HypershiftOperatorImage string - IgnitionEndpoint string - Namespace string + ControlPlaneOperatorImage string + ManagementClient client.Client + GuestClient client.Client + ReleaseProvider releaseinfo.Provider + VersionResolver releaseinfo.VersionResolver + ImageMetadataProvider imageregistry.ImageMetadataProvider + HypershiftOperatorImage string + IgnitionEndpoint string + Namespace string upsert.CreateOrUpdateProvider } @@ -300,9 +301,11 @@ func (r *KarpenterIgnitionReconciler) reconcileNodeClassToken( return fmt.Errorf("failed to build config generator: %w", err) } - token, err := nodepool.NewToken(ctx, cg, &nodepool.CPOCapabilities{ - DecompressAndDecodeConfig: true, - }) + cpoCapabilities, err := r.detectCPOCapabilities(ctx, hostedCluster) + if err != nil { + return fmt.Errorf("failed to detect CPO capabilities: %w", err) + } + token, err := nodepool.NewToken(ctx, cg, cpoCapabilities) if err != nil { return fmt.Errorf("failed to create token: %w", err) } @@ -330,6 +333,15 @@ func (r *KarpenterIgnitionReconciler) reconcileNodeClassToken( token.SetDeployedBootstrapHash(currentConfigVersion) } + // Persist the v2 content baseline before refreshing retained bootstrap secrets. + // Retries must not absorb another content edit if a later write fails. + if currentConfigVersion != "" && currentRolloutConfig == "" { + currentRolloutConfig = cg.RolloutHashWithoutVersion() + if err := r.updateConfigAnnotations(ctx, openshiftEC2NodeClass, currentConfigVersion, currentRolloutConfig); err != nil { + return err + } + np.Annotations[nodePoolAnnotationCurrentRolloutConfig] = currentRolloutConfig + } if err := token.Reconcile(ctx); err != nil { return fmt.Errorf("failed to reconcile token: %w", err) } @@ -356,6 +368,21 @@ func (r *KarpenterIgnitionReconciler) reconcileNodeClassToken( return nil } +// detectCPOCapabilities uses the CPO image supplied by the running control plane. +// The synthesized HostedCluster does not carry the original HC's image override, +// so resolving from its release can select a different image from the one serving ignition. +func (r *KarpenterIgnitionReconciler) detectCPOCapabilities(ctx context.Context, hostedCluster *hyperv1.HostedCluster) (*nodepool.CPOCapabilities, error) { + if r.ControlPlaneOperatorImage == "" { + return nil, fmt.Errorf("control plane operator image is required for capability detection") + } + cluster := hostedCluster.DeepCopy() + if cluster.Annotations == nil { + cluster.Annotations = make(map[string]string) + } + cluster.Annotations[hyperv1.ControlPlaneOperatorImageAnnotation] = r.ControlPlaneOperatorImage + return nodepool.DetectCPOCapabilities(ctx, r.ManagementClient, cluster, r.ReleaseProvider, r.ImageMetadataProvider, r.HypershiftOperatorImage) +} + func (r *KarpenterIgnitionReconciler) createInMemoryNodePool( hcp *hyperv1.HostedControlPlane, openshiftEC2NodeClass *hyperkarpenterv1.OpenshiftEC2NodeClass, diff --git a/karpenter-operator/controllers/karpenterignition/karpenterignition_controller_test.go b/karpenter-operator/controllers/karpenterignition/karpenterignition_controller_test.go index 035757dfbc35..4074f14ccaa6 100644 --- a/karpenter-operator/controllers/karpenterignition/karpenterignition_controller_test.go +++ b/karpenter-operator/controllers/karpenterignition/karpenterignition_controller_test.go @@ -211,16 +211,18 @@ kind: Config`), WithStatusSubresource(&hyperkarpenterv1.OpenshiftEC2NodeClass{}). Build() + recordingProvider := &recordingCPOImageProvider{FakeRegistryClientImageMetadataProvider: *fakeImageMetadataProvider} r := &KarpenterIgnitionReconciler{ - ManagementClient: fakeManagementClient, - GuestClient: fakeGuestClient, - ReleaseProvider: mockedReleaseProvider, - VersionResolver: &fakeVersionResolver{}, - ImageMetadataProvider: fakeImageMetadataProvider, - HypershiftOperatorImage: "test-hypershift-operator-image", - IgnitionEndpoint: testIgnitionEndpoint, - Namespace: testNamespace, - CreateOrUpdateProvider: upsert.New(false), + ControlPlaneOperatorImage: "quay.io/hypershift/control-plane-operator:latest", + ManagementClient: fakeManagementClient, + GuestClient: fakeGuestClient, + ReleaseProvider: mockedReleaseProvider, + VersionResolver: &fakeVersionResolver{}, + ImageMetadataProvider: recordingProvider, + HypershiftOperatorImage: "test-hypershift-operator-image", + IgnitionEndpoint: testIgnitionEndpoint, + Namespace: testNamespace, + CreateOrUpdateProvider: upsert.New(false), } ctx := log.IntoContext(t.Context(), testr.New(t)) @@ -230,6 +232,7 @@ kind: Config`), NamespacedName: client.ObjectKey{Name: testNodeClassName}, }) g.Expect(err).NotTo(HaveOccurred()) + g.Expect(recordingProvider.image).To(Equal(r.ControlPlaneOperatorImage), "capability detection must inspect the running CPO, not the release image") expectedNodePoolName := karpenterutil.KarpenterNodePoolName(nodeClass) tokenPrefix := "token-" + expectedNodePoolName + "-" @@ -386,6 +389,45 @@ kind: Config`), // https://github.com/openshift/hypershift/blob/825484eb33d14b4ab849b428d134582320655fcf/hypershift-operator/controllers/nodepool/token.go#L197 g.Expect(len(tokenSecrets)).To(Equal(3), "should have 3 token secrets, got: %v", tokenSecrets) g.Expect(len(userDataSecrets)).To(Equal(2), "should have 2 user-data secrets, got: %v", userDataSecrets) + + for _, field := range []string{"additional", "proxy"} { + t.Run("When a legacy NodeClass migrates with "+field+" CA content, it should retain identity until the next edit", func(t *testing.T) { + g := NewWithT(t) + recordingProvider.Result.Config.Labels["io.openshift.hypershift.control-plane-operator-manages.rendered-trust-bundle-validation"] = "true" + g.Expect(fakeGuestClient.Get(ctx, client.ObjectKeyFromObject(nodeClass), nodeClass)).To(Succeed()) + retained := nodeClass.Annotations[configVersionAnnotation] + delete(nodeClass.Annotations, openshiftEC2NodeClassAnnotationCurrentRolloutConfig) + nodeClass.Annotations["hypershift.openshift.io/nodeClassCurrentRolloutConfig"] = "legacy-name-based-baseline" + g.Expect(fakeGuestClient.Update(ctx, nodeClass)).To(Succeed()) + bundle := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "migration-" + field, Namespace: testNamespace}, Data: map[string]string{"ca-bundle.crt": "pre-upgrade-content"}} + g.Expect(fakeManagementClient.Create(ctx, bundle)).To(Succeed()) + g.Expect(fakeManagementClient.Get(ctx, client.ObjectKeyFromObject(hcp), hcp)).To(Succeed()) + if field == "additional" { + hcp.Spec.AdditionalTrustBundle = &corev1.LocalObjectReference{Name: bundle.Name} + } else { + hcp.Spec.Configuration = &hyperv1.ClusterConfiguration{Proxy: &configv1.ProxySpec{TrustedCA: configv1.ConfigMapNameReference{Name: bundle.Name}}} + } + g.Expect(fakeManagementClient.Update(ctx, hcp)).To(Succeed()) + _, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(nodeClass)}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(fakeGuestClient.Get(ctx, client.ObjectKeyFromObject(nodeClass), nodeClass)).To(Succeed()) + g.Expect(nodeClass.Annotations[configVersionAnnotation]).To(Equal(retained)) + baseline := nodeClass.Annotations[openshiftEC2NodeClassAnnotationCurrentRolloutConfig] + g.Expect(baseline).NotTo(BeEmpty()) + retainedUserData := &corev1.Secret{} + g.Expect(fakeManagementClient.Get(ctx, client.ObjectKey{Namespace: testNamespace, Name: userDataPrefix + retained}, retainedUserData)).To(Succeed()) + g.Expect(string(retainedUserData.Data["value"])).To(ContainSubstring(retained)) + // Restart the controller; the durable baseline must still detect the edit. + restarted := *r + bundle.Data["ca-bundle.crt"] = "post-migration-content" + g.Expect(fakeManagementClient.Update(ctx, bundle)).To(Succeed()) + _, err = restarted.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(nodeClass)}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(fakeGuestClient.Get(ctx, client.ObjectKeyFromObject(nodeClass), nodeClass)).To(Succeed()) + g.Expect(nodeClass.Annotations[configVersionAnnotation]).NotTo(Equal(retained)) + g.Expect(nodeClass.Annotations[openshiftEC2NodeClassAnnotationCurrentRolloutConfig]).NotTo(Equal(baseline)) + }) + } } func TestReconcileVersionResolution(t *testing.T) { @@ -531,15 +573,16 @@ kind: Config`), Build() r := &KarpenterIgnitionReconciler{ - ManagementClient: fakeManagementClient, - GuestClient: fakeGuestClient, - ReleaseProvider: mockedReleaseProvider, - VersionResolver: resolver, - ImageMetadataProvider: fakeImageMetadataProvider, - HypershiftOperatorImage: "test-hypershift-operator-image", - IgnitionEndpoint: testIgnitionEndpoint, - Namespace: testNamespace, - CreateOrUpdateProvider: upsert.New(false), + ControlPlaneOperatorImage: "quay.io/hypershift/control-plane-operator:latest", + ManagementClient: fakeManagementClient, + GuestClient: fakeGuestClient, + ReleaseProvider: mockedReleaseProvider, + VersionResolver: resolver, + ImageMetadataProvider: fakeImageMetadataProvider, + HypershiftOperatorImage: "test-hypershift-operator-image", + IgnitionEndpoint: testIgnitionEndpoint, + Namespace: testNamespace, + CreateOrUpdateProvider: upsert.New(false), } ctx := log.IntoContext(t.Context(), testr.New(t)) @@ -577,15 +620,16 @@ kind: Config`), Build() r := &KarpenterIgnitionReconciler{ - ManagementClient: fakeManagementClient, - GuestClient: fakeGuestClient, - ReleaseProvider: mockedReleaseProvider, - VersionResolver: resolver, - ImageMetadataProvider: fakeImageMetadataProvider, - HypershiftOperatorImage: "test-hypershift-operator-image", - IgnitionEndpoint: testIgnitionEndpoint, - Namespace: testNamespace, - CreateOrUpdateProvider: upsert.New(false), + ControlPlaneOperatorImage: "quay.io/hypershift/control-plane-operator:latest", + ManagementClient: fakeManagementClient, + GuestClient: fakeGuestClient, + ReleaseProvider: mockedReleaseProvider, + VersionResolver: resolver, + ImageMetadataProvider: fakeImageMetadataProvider, + HypershiftOperatorImage: "test-hypershift-operator-image", + IgnitionEndpoint: testIgnitionEndpoint, + Namespace: testNamespace, + CreateOrUpdateProvider: upsert.New(false), } ctx := log.IntoContext(t.Context(), testr.New(t)) @@ -624,15 +668,16 @@ kind: Config`), Build() r := &KarpenterIgnitionReconciler{ - ManagementClient: fakeManagementClient, - GuestClient: fakeGuestClient, - ReleaseProvider: mockedReleaseProvider, - VersionResolver: resolver, - ImageMetadataProvider: fakeImageMetadataProvider, - HypershiftOperatorImage: "test-hypershift-operator-image", - IgnitionEndpoint: testIgnitionEndpoint, - Namespace: testNamespace, - CreateOrUpdateProvider: upsert.New(false), + ControlPlaneOperatorImage: "quay.io/hypershift/control-plane-operator:latest", + ManagementClient: fakeManagementClient, + GuestClient: fakeGuestClient, + ReleaseProvider: mockedReleaseProvider, + VersionResolver: resolver, + ImageMetadataProvider: fakeImageMetadataProvider, + HypershiftOperatorImage: "test-hypershift-operator-image", + IgnitionEndpoint: testIgnitionEndpoint, + Namespace: testNamespace, + CreateOrUpdateProvider: upsert.New(false), } ctx := log.IntoContext(t.Context(), testr.New(t)) @@ -685,15 +730,16 @@ kind: Config`), Build() r := &KarpenterIgnitionReconciler{ - ManagementClient: fakeManagementClient, - GuestClient: fakeGuestClient, - ReleaseProvider: mockedReleaseProvider, - VersionResolver: resolver, - ImageMetadataProvider: fakeImageMetadataProvider, - HypershiftOperatorImage: "test-hypershift-operator-image", - IgnitionEndpoint: testIgnitionEndpoint, - Namespace: testNamespace, - CreateOrUpdateProvider: upsert.New(false), + ControlPlaneOperatorImage: "quay.io/hypershift/control-plane-operator:latest", + ManagementClient: fakeManagementClient, + GuestClient: fakeGuestClient, + ReleaseProvider: mockedReleaseProvider, + VersionResolver: resolver, + ImageMetadataProvider: fakeImageMetadataProvider, + HypershiftOperatorImage: "test-hypershift-operator-image", + IgnitionEndpoint: testIgnitionEndpoint, + Namespace: testNamespace, + CreateOrUpdateProvider: upsert.New(false), } ctx := log.IntoContext(t.Context(), testr.New(t)) @@ -756,7 +802,8 @@ func TestResolveVersion(t *testing.T) { hcp := baseHCP() resolver := &fakeVersionResolver{image: "should-not-be-called"} - r := &KarpenterIgnitionReconciler{VersionResolver: resolver} + r := &KarpenterIgnitionReconciler{ + ControlPlaneOperatorImage: "quay.io/hypershift/control-plane-operator:latest", VersionResolver: resolver} ctx := log.IntoContext(t.Context(), testr.New(t)) image, err := r.resolveReleaseImage(ctx, hcp, "") @@ -771,7 +818,8 @@ func TestResolveVersion(t *testing.T) { resolvedImage := "quay.io/openshift-release-dev/ocp-release@sha256:abc123" resolver := &fakeVersionResolver{image: resolvedImage} - r := &KarpenterIgnitionReconciler{VersionResolver: resolver} + r := &KarpenterIgnitionReconciler{ + ControlPlaneOperatorImage: "quay.io/hypershift/control-plane-operator:latest", VersionResolver: resolver} ctx := log.IntoContext(t.Context(), testr.New(t)) image, err := r.resolveReleaseImage(ctx, hcp, "4.17.0") @@ -822,7 +870,8 @@ func TestResolveVersion(t *testing.T) { resolvedImage := "quay.io/openshift-release-dev/ocp-release@sha256:skewed" resolver := &fakeVersionResolver{image: resolvedImage} - r := &KarpenterIgnitionReconciler{VersionResolver: resolver} + r := &KarpenterIgnitionReconciler{ + ControlPlaneOperatorImage: "quay.io/hypershift/control-plane-operator:latest", VersionResolver: resolver} ctx := log.IntoContext(t.Context(), testr.New(t)) // validateVersion should pass (4.16.0 is valid, just outside skew policy) @@ -844,7 +893,8 @@ func TestResolveVersion(t *testing.T) { hcp := baseHCP() resolver := &fakeVersionResolver{err: fmt.Errorf("Cincinnati API unavailable")} - r := &KarpenterIgnitionReconciler{VersionResolver: resolver} + r := &KarpenterIgnitionReconciler{ + ControlPlaneOperatorImage: "quay.io/hypershift/control-plane-operator:latest", VersionResolver: resolver} ctx := log.IntoContext(t.Context(), testr.New(t)) _, err := r.resolveReleaseImage(ctx, hcp, "4.17.0") @@ -894,7 +944,8 @@ func TestUpdateVersionStatus(t *testing.T) { WithStatusSubresource(&hyperkarpenterv1.OpenshiftEC2NodeClass{}). Build() - r := &KarpenterIgnitionReconciler{GuestClient: fakeGuestClient} + r := &KarpenterIgnitionReconciler{ + ControlPlaneOperatorImage: "quay.io/hypershift/control-plane-operator:latest", GuestClient: fakeGuestClient} ctx := log.IntoContext(t.Context(), testr.New(t)) hcpImage := "quay.io/openshift-release-dev/ocp-release:4.17.0-x86_64" @@ -921,7 +972,8 @@ func TestUpdateVersionStatus(t *testing.T) { WithStatusSubresource(&hyperkarpenterv1.OpenshiftEC2NodeClass{}). Build() - r := &KarpenterIgnitionReconciler{GuestClient: fakeGuestClient} + r := &KarpenterIgnitionReconciler{ + ControlPlaneOperatorImage: "quay.io/hypershift/control-plane-operator:latest", GuestClient: fakeGuestClient} ctx := log.IntoContext(t.Context(), testr.New(t)) resolvedImage := "quay.io/openshift-release-dev/ocp-release@sha256:abc123" @@ -950,7 +1002,8 @@ func TestUpdateVersionStatus(t *testing.T) { WithStatusSubresource(&hyperkarpenterv1.OpenshiftEC2NodeClass{}). Build() - r := &KarpenterIgnitionReconciler{GuestClient: fakeGuestClient} + r := &KarpenterIgnitionReconciler{ + ControlPlaneOperatorImage: "quay.io/hypershift/control-plane-operator:latest", GuestClient: fakeGuestClient} ctx := log.IntoContext(t.Context(), testr.New(t)) resolveErr := fmt.Errorf("Cincinnati API unavailable") @@ -977,7 +1030,8 @@ func TestUpdateVersionStatus(t *testing.T) { WithStatusSubresource(&hyperkarpenterv1.OpenshiftEC2NodeClass{}). Build() - r := &KarpenterIgnitionReconciler{GuestClient: fakeGuestClient} + r := &KarpenterIgnitionReconciler{ + ControlPlaneOperatorImage: "quay.io/hypershift/control-plane-operator:latest", GuestClient: fakeGuestClient} ctx := log.IntoContext(t.Context(), testr.New(t)) hcpImage := "quay.io/openshift-release-dev/ocp-release:4.17.0-x86_64" @@ -1513,10 +1567,11 @@ func TestReconcileKubeletConfigMapOrphanCleanup(t *testing.T) { Build() r := &KarpenterIgnitionReconciler{ - ManagementClient: fakeManagementClient, - GuestClient: fakeGuestClient, - Namespace: testNamespace, - CreateOrUpdateProvider: upsert.New(false), + ControlPlaneOperatorImage: "quay.io/hypershift/control-plane-operator:latest", + ManagementClient: fakeManagementClient, + GuestClient: fakeGuestClient, + Namespace: testNamespace, + CreateOrUpdateProvider: upsert.New(false), } ctx := log.IntoContext(t.Context(), testr.New(t)) @@ -1562,10 +1617,11 @@ func TestReconcileKubeletConfigMapOrphanCleanup(t *testing.T) { Build() r := &KarpenterIgnitionReconciler{ - ManagementClient: fake.NewClientBuilder().WithScheme(scheme).WithObjects(hcp).Build(), - GuestClient: fakeGuestClient, - Namespace: testNamespace, - CreateOrUpdateProvider: upsert.New(false), + ControlPlaneOperatorImage: "quay.io/hypershift/control-plane-operator:latest", + ManagementClient: fake.NewClientBuilder().WithScheme(scheme).WithObjects(hcp).Build(), + GuestClient: fakeGuestClient, + Namespace: testNamespace, + CreateOrUpdateProvider: upsert.New(false), } ctx := log.IntoContext(t.Context(), testr.New(t)) @@ -1598,10 +1654,11 @@ func TestReconcileKubeletConfigMapOrphanCleanup(t *testing.T) { Build() r := &KarpenterIgnitionReconciler{ - ManagementClient: fakeManagementClient, - GuestClient: fakeGuestClient, - Namespace: testNamespace, - CreateOrUpdateProvider: upsert.New(false), + ControlPlaneOperatorImage: "quay.io/hypershift/control-plane-operator:latest", + ManagementClient: fakeManagementClient, + GuestClient: fakeGuestClient, + Namespace: testNamespace, + CreateOrUpdateProvider: upsert.New(false), } ctx := log.IntoContext(t.Context(), testr.New(t)) @@ -1748,15 +1805,16 @@ kind: Config`), Build() r := &KarpenterIgnitionReconciler{ - ManagementClient: fakeManagementClient, - GuestClient: fakeGuestClient, - ReleaseProvider: mockedReleaseProvider, - VersionResolver: &fakeVersionResolver{}, - ImageMetadataProvider: fakeImgProvider, - HypershiftOperatorImage: "test-hypershift-operator-image", - IgnitionEndpoint: testIgnitionEndpoint, - Namespace: testNamespace, - CreateOrUpdateProvider: upsert.New(false), + ControlPlaneOperatorImage: "quay.io/hypershift/control-plane-operator:latest", + ManagementClient: fakeManagementClient, + GuestClient: fakeGuestClient, + ReleaseProvider: mockedReleaseProvider, + VersionResolver: &fakeVersionResolver{}, + ImageMetadataProvider: fakeImgProvider, + HypershiftOperatorImage: "test-hypershift-operator-image", + IgnitionEndpoint: testIgnitionEndpoint, + Namespace: testNamespace, + CreateOrUpdateProvider: upsert.New(false), } return r, fakeGuestClient } @@ -1919,10 +1977,11 @@ kind: Config`), Build() r := &KarpenterIgnitionReconciler{ - ManagementClient: fakeManagementClient, - GuestClient: fakeGuestClient, - Namespace: testNamespace, - CreateOrUpdateProvider: upsert.New(false), + ControlPlaneOperatorImage: "quay.io/hypershift/control-plane-operator:latest", + ManagementClient: fakeManagementClient, + GuestClient: fakeGuestClient, + Namespace: testNamespace, + CreateOrUpdateProvider: upsert.New(false), } ctx := log.IntoContext(t.Context(), testr.New(t)) @@ -1976,10 +2035,11 @@ kind: Config`), Build() r := &KarpenterIgnitionReconciler{ - ManagementClient: fakeManagementClient, - GuestClient: fakeGuestClient, - Namespace: testNamespace, - CreateOrUpdateProvider: upsert.New(false), + ControlPlaneOperatorImage: "quay.io/hypershift/control-plane-operator:latest", + ManagementClient: fakeManagementClient, + GuestClient: fakeGuestClient, + Namespace: testNamespace, + CreateOrUpdateProvider: upsert.New(false), } ctx := log.IntoContext(t.Context(), testr.New(t)) @@ -2020,10 +2080,11 @@ kind: Config`), Build() r := &KarpenterIgnitionReconciler{ - ManagementClient: fakeManagementClient, - GuestClient: fakeGuestClient, - Namespace: testNamespace, - CreateOrUpdateProvider: upsert.New(false), + ControlPlaneOperatorImage: "quay.io/hypershift/control-plane-operator:latest", + ManagementClient: fakeManagementClient, + GuestClient: fakeGuestClient, + Namespace: testNamespace, + CreateOrUpdateProvider: upsert.New(false), } ctx := log.IntoContext(t.Context(), testr.New(t)) @@ -2040,7 +2101,8 @@ kind: Config`), } func TestCreateInMemoryNodePool(t *testing.T) { - r := &KarpenterIgnitionReconciler{} + r := &KarpenterIgnitionReconciler{ + ControlPlaneOperatorImage: "quay.io/hypershift/control-plane-operator:latest"} hcp := &hyperv1.HostedControlPlane{ ObjectMeta: metav1.ObjectMeta{ Name: "test-hcp", @@ -2122,8 +2184,9 @@ func TestReconcileKubeletConfigMap(t *testing.T) { Build() r := &KarpenterIgnitionReconciler{ - ManagementClient: fakeManagementClient, - CreateOrUpdateProvider: upsert.New(false), + ControlPlaneOperatorImage: "quay.io/hypershift/control-plane-operator:latest", + ManagementClient: fakeManagementClient, + CreateOrUpdateProvider: upsert.New(false), } hcp := &hyperv1.HostedControlPlane{ ObjectMeta: metav1.ObjectMeta{ @@ -2159,8 +2222,9 @@ func TestReconcileKubeletConfigMap(t *testing.T) { Build() r := &KarpenterIgnitionReconciler{ - ManagementClient: fakeManagementClient, - CreateOrUpdateProvider: upsert.New(false), + ControlPlaneOperatorImage: "quay.io/hypershift/control-plane-operator:latest", + ManagementClient: fakeManagementClient, + CreateOrUpdateProvider: upsert.New(false), } hcp := &hyperv1.HostedControlPlane{ ObjectMeta: metav1.ObjectMeta{ @@ -2219,8 +2283,9 @@ func TestReconcileKubeletConfigMap(t *testing.T) { Build() r := &KarpenterIgnitionReconciler{ - ManagementClient: fakeManagementClient, - CreateOrUpdateProvider: upsert.New(false), + ControlPlaneOperatorImage: "quay.io/hypershift/control-plane-operator:latest", + ManagementClient: fakeManagementClient, + CreateOrUpdateProvider: upsert.New(false), } hcp := &hyperv1.HostedControlPlane{ ObjectMeta: metav1.ObjectMeta{ @@ -2280,8 +2345,9 @@ func TestReconcileKubeletConfigMap(t *testing.T) { Build() r := &KarpenterIgnitionReconciler{ - ManagementClient: fakeManagementClient, - CreateOrUpdateProvider: upsert.New(false), + ControlPlaneOperatorImage: "quay.io/hypershift/control-plane-operator:latest", + ManagementClient: fakeManagementClient, + CreateOrUpdateProvider: upsert.New(false), } hcp := &hyperv1.HostedControlPlane{ ObjectMeta: metav1.ObjectMeta{ @@ -2316,8 +2382,9 @@ func TestReconcileKubeletConfigMap(t *testing.T) { Build() r := &KarpenterIgnitionReconciler{ - ManagementClient: fakeManagementClient, - CreateOrUpdateProvider: upsert.New(false), + ControlPlaneOperatorImage: "quay.io/hypershift/control-plane-operator:latest", + ManagementClient: fakeManagementClient, + CreateOrUpdateProvider: upsert.New(false), } hcp := &hyperv1.HostedControlPlane{ ObjectMeta: metav1.ObjectMeta{ @@ -2377,8 +2444,9 @@ func TestReconcileTaintConfigMap(t *testing.T) { fakeManagementClient := fake.NewClientBuilder().WithScheme(scheme).Build() r := &KarpenterIgnitionReconciler{ - ManagementClient: fakeManagementClient, - CreateOrUpdateProvider: upsert.New(false), + ControlPlaneOperatorImage: "quay.io/hypershift/control-plane-operator:latest", + ManagementClient: fakeManagementClient, + CreateOrUpdateProvider: upsert.New(false), } err := r.reconcileTaintConfigMap(ctx, hcp) @@ -2423,8 +2491,9 @@ func TestReconcileTaintConfigMap(t *testing.T) { } fakeManagementClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(existingCM).Build() r := &KarpenterIgnitionReconciler{ - ManagementClient: fakeManagementClient, - CreateOrUpdateProvider: upsert.New(false), + ControlPlaneOperatorImage: "quay.io/hypershift/control-plane-operator:latest", + ManagementClient: fakeManagementClient, + CreateOrUpdateProvider: upsert.New(false), } err := r.reconcileTaintConfigMap(ctx, hcp) diff --git a/karpenter-operator/main.go b/karpenter-operator/main.go index 5e656cb5c337..20a6dba75bc7 100644 --- a/karpenter-operator/main.go +++ b/karpenter-operator/main.go @@ -195,12 +195,13 @@ func run(ctx context.Context) error { } kir := karpenterignition.KarpenterIgnitionReconciler{ - ReleaseProvider: releaseProvider, - VersionResolver: releaseinfo.NewCincinnatiVersionResolver(), - ImageMetadataProvider: imageMetaDataProvider, - HypershiftOperatorImage: hypershiftOperatorImage, - IgnitionEndpoint: ignitionEndpoint, - Namespace: namespace, + ControlPlaneOperatorImage: controlPlaneOperatorImage, + ReleaseProvider: releaseProvider, + VersionResolver: releaseinfo.NewCincinnatiVersionResolver(), + ImageMetadataProvider: imageMetaDataProvider, + HypershiftOperatorImage: hypershiftOperatorImage, + IgnitionEndpoint: ignitionEndpoint, + Namespace: namespace, } if err := kir.SetupWithManager(mgr, managementCluster); err != nil { return fmt.Errorf("failed to setup karpenter ignition controller with manager: %w", err) diff --git a/test/e2e/v2/tests/nodepool_lifecycle_test.go b/test/e2e/v2/tests/nodepool_lifecycle_test.go index d1d59630d39a..e8469eefbfb0 100644 --- a/test/e2e/v2/tests/nodepool_lifecycle_test.go +++ b/test/e2e/v2/tests/nodepool_lifecycle_test.go @@ -26,8 +26,8 @@ import ( . "github.com/onsi/gomega" hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" - npconst "github.com/openshift/hypershift/pkg/nodepool" "github.com/openshift/hypershift/pkg/manifests" + npconst "github.com/openshift/hypershift/pkg/nodepool" hyperapi "github.com/openshift/hypershift/support/api" "github.com/openshift/hypershift/support/netutil" "github.com/openshift/hypershift/support/podspec" @@ -43,6 +43,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/client-go/util/retry" "k8s.io/utils/ptr" capiazure "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1" @@ -51,6 +52,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/apiutil" "sigs.k8s.io/yaml" + "github.com/blang/semver" ignitionapi "github.com/coreos/ignition/v2/config/v3_2/types" "github.com/google/go-cmp/cmp" ) @@ -802,211 +804,174 @@ func NodePoolMirrorConfigsTest(getTestCtx internal.TestContextGetter) { // NodePoolTrustBundleTest creates an additional trust bundle ConfigMap, updates the // HostedCluster to reference it, waits for NodePool update cycle, verifies user-ca-bundle -// exists in the hosted cluster, removes the trust bundle, verifies CPO deployment no longer -// mounts it, waits for another update cycle, and verifies user-ca-bundle is deleted (4.22+). +// exists in the hosted cluster, patches ca-bundle.crt in place (same ConfigMap name) and +// waits for another update cycle (RFE-8751), removes the trust bundle, verifies CPO +// deployment no longer mounts it, waits for another update cycle, and verifies +// user-ca-bundle is deleted (4.22+). func NodePoolTrustBundleTest(getTestCtx internal.TestContextGetter) { - It("should propagate and remove additional trust bundle to/from the hosted cluster", Label("nodepool-trust-bundle"), func() { + It("should propagate, roll on in-place content change, and remove additional trust bundle", Label("nodepool-trust-bundle"), func() { testCtx := getTestCtx() + ctx := testCtx.Context hc, err := testCtx.GetHostedCluster() Expect(err).NotTo(HaveOccurred()) - version := testCtx.SkipIfVersionBelow(e2eutil.Version418) - + version := testCtx.SkipIfVersionBelow(semver.MustParse("4.18.0")) hcClient, err := testCtx.GetHostedClusterClient(hc) Expect(err).NotTo(HaveOccurred()) - ctx := testCtx.Context - const ( - defaultPollInterval = 15 * time.Second - nodePoolConfigUpdateStartTimeout = 10 * time.Minute - nodePoolConfigUpdateFinishTimeout = 20 * time.Minute - cpoDeploymentUpdateTimeout = 10 * time.Minute - guestUserCABundlePropagationTimeout = 5 * time.Minute + defaultPollInterval = 15 * time.Second + nodePoolConfigUpdateStartTimeout = 10 * time.Minute + nodePoolConfigUpdateFinishTimeout = 20 * time.Minute + cpoDeploymentUpdateTimeout = 10 * time.Minute + hostedUserCABundlePropagationTimeout = 5 * time.Minute ) - defaultNP := getDefaultNodePool(ctx, testCtx.MgmtClient, hc) - Expect(defaultNP).NotTo(BeNil(), "default NodePool should exist") - - var oneReplica int32 = 1 - np := buildTestNodePool(defaultNP, "trust-bundle", func(pool *hyperv1.NodePool) { - pool.Spec.Replicas = &oneReplica - }) - Expect(testCtx.MgmtClient.Create(ctx, np)).To(Succeed(), "failed to create NodePool %s", np.Name) - GinkgoWriter.Printf("Created NodePool %s for trust bundle test\n", np.Name) + // Use an existing, ready NodePool as the template for the test-created pool. + pools := &hyperv1.NodePoolList{} + Expect(testCtx.MgmtClient.List(ctx, pools, crclient.InNamespace(hc.Namespace))).To(Succeed()) + var defaultNP *hyperv1.NodePool + for i := range pools.Items { + pool := &pools.Items[i] + if pool.Spec.ClusterName == hc.Name && pool.DeletionTimestamp == nil && pool.Status.Replicas > 0 { + defaultNP = pool + break + } + } + Expect(defaultNP).NotTo(BeNil(), "ready NodePool for HostedCluster %s/%s should exist", hc.Namespace, hc.Name) + np := &hyperv1.NodePool{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: hc.Name + "-trust-bundle-", + Namespace: hc.Namespace, + }, + Spec: *defaultNP.Spec.DeepCopy(), + } + np.Spec.Replicas = ptr.To[int32](1) + Expect(testCtx.MgmtClient.Create(ctx, np)).To(Succeed(), "failed to create trust bundle NodePool") DeferCleanup(func() { - cleanupNodePool(ctx, testCtx.MgmtClient, np) + Expect(crclient.IgnoreNotFound(testCtx.MgmtClient.Delete(ctx, np))).To(Succeed()) + Eventually(func() error { + return testCtx.MgmtClient.Get(ctx, crclient.ObjectKeyFromObject(np), &hyperv1.NodePool{}) + }).WithContext(ctx).WithTimeout(nodePoolUpgradeTimeout(np.Spec.Platform.Type)). + WithPolling(defaultPollInterval).Should(Satisfy(apierrors.IsNotFound), "NodePool %s/%s should be deleted", np.Namespace, np.Name) }) - e2eutil.WaitForReadyNodesByNodePool(GinkgoTB(), ctx, hcClient, np, hc.Spec.Platform.Type) + // This readiness wait is for the NodePool created by this spec, not suite startup. + nodeReadyTimeout := 45 * time.Minute + if hc.Spec.Platform.Type == hyperv1.PowerVSPlatform { + nodeReadyTimeout = 60 * time.Minute + } + Eventually(func(g Gomega) { + nodes := &corev1.NodeList{} + g.Expect(hcClient.List(ctx, nodes, crclient.MatchingLabels{hyperv1.NodePoolLabel: np.Name})).To(Succeed()) + g.Expect(nodes.Items).To(HaveLen(1), "expected one node for NodePool %s/%s", np.Namespace, np.Name) + g.Expect(nodes.Items[0].Status.Conditions).To(ContainElement(SatisfyAll( + HaveField("Type", corev1.NodeReady), HaveField("Status", corev1.ConditionTrue), + ))) + }).WithContext(ctx).WithTimeout(nodeReadyTimeout).WithPolling(3 * time.Second).Should(Succeed()) + + // Keep the condition checks identical for addition, content edit, and removal. + waitForConfig := func(g Gomega, status corev1.ConditionStatus, timeout time.Duration, description string) { + GinkgoHelper() + g.Eventually(func(pollG Gomega) { + pool := &hyperv1.NodePool{} + pollG.Expect(testCtx.MgmtClient.Get(ctx, crclient.ObjectKeyFromObject(np), pool)).To(Succeed()) + pollG.Expect(pool.Status.Conditions).To(ContainElement(SatisfyAll( + HaveField("Type", hyperv1.NodePoolUpdatingConfigConditionType), HaveField("Status", status), + ))) + if status == corev1.ConditionFalse { + pollG.Expect(pool.Status.Conditions).To(ContainElement(SatisfyAll( + HaveField("Type", hyperv1.NodePoolAllNodesHealthyConditionType), HaveField("Status", corev1.ConditionTrue), + ))) + } + }).WithContext(ctx).WithTimeout(timeout).WithPolling(defaultPollInterval).Should(Succeed(), + "NodePool %s/%s: %s", np.Namespace, np.Name, description) + } - // Create additional trust bundle ConfigMap trustBundle := &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ - Name: e2eutil.SimpleNameGenerator.GenerateName("trust-bundle-"), - Namespace: hc.Namespace, + GenerateName: "trust-bundle-", + Namespace: hc.Namespace, }, Data: map[string]string{"ca-bundle.crt": "dummy"}, } Expect(testCtx.MgmtClient.Create(ctx, trustBundle)).To(Succeed(), "failed to create trust bundle ConfigMap") DeferCleanup(func() { - if err := testCtx.MgmtClient.Delete(ctx, trustBundle); err != nil && !apierrors.IsNotFound(err) { - Expect(err).NotTo(HaveOccurred(), "cleanup: failed to delete ConfigMap %s", trustBundle.Name) - } + Expect(crclient.IgnoreNotFound(testCtx.MgmtClient.Delete(ctx, trustBundle))).To(Succeed(), + "cleanup: failed to delete ConfigMap %s", trustBundle.Name) }) - // Update HostedCluster to reference the trust bundle - GinkgoWriter.Printf("Updating HostedCluster with additional trust bundle %s\n", trustBundle.Name) - Expect(e2eutil.UpdateObject(GinkgoTB(), ctx, testCtx.MgmtClient, hc, func(obj *hyperv1.HostedCluster) { - obj.Spec.AdditionalTrustBundle = &corev1.LocalObjectReference{Name: trustBundle.Name} - })).To(Succeed(), "failed to update HostedCluster with trust bundle") - - // Defer cleanup: remove trust bundle reference from HostedCluster - DeferCleanup(func() { - currentHC := &hyperv1.HostedCluster{} - if err := testCtx.MgmtClient.Get(ctx, crclient.ObjectKeyFromObject(hc), currentHC); err != nil { - if !apierrors.IsNotFound(err) { - GinkgoWriter.Printf("Warning: cleanup: failed to get HostedCluster: %v\n", err) + // Patch only the reference and retry conflicts without overwriting other HC changes. + setTrustBundleReference := func(ref *corev1.LocalObjectReference) error { + return retry.RetryOnConflict(retry.DefaultRetry, func() error { + current := &hyperv1.HostedCluster{} + if err := testCtx.MgmtClient.Get(ctx, crclient.ObjectKeyFromObject(hc), current); err != nil { + return err } - return - } - err := e2eutil.UpdateObject(GinkgoTB(), ctx, testCtx.MgmtClient, currentHC, func(obj *hyperv1.HostedCluster) { - obj.Spec.AdditionalTrustBundle = nil + before := current.DeepCopy() + current.Spec.AdditionalTrustBundle = ref + return testCtx.MgmtClient.Patch(ctx, current, crclient.MergeFromWithOptions(before, crclient.MergeFromWithOptimisticLock{})) }) - if err != nil && !apierrors.IsNotFound(err) { - Expect(err).NotTo(HaveOccurred(), "cleanup: failed to remove additional trust bundle") - } + } + originalTrustBundle := hc.Spec.AdditionalTrustBundle + DeferCleanup(func() { + Expect(crclient.IgnoreNotFound(setTrustBundleReference(originalTrustBundle))).To(Succeed(), + "cleanup: failed to restore additional trust bundle reference") }) + Expect(setTrustBundleReference(&corev1.LocalObjectReference{Name: trustBundle.Name})).To(Succeed(), + "failed to update HostedCluster with trust bundle") + + waitForConfig(Default, corev1.ConditionTrue, nodePoolConfigUpdateStartTimeout, "begin updating") + waitForConfig(Default, corev1.ConditionFalse, nodePoolConfigUpdateFinishTimeout, "stop updating") + + userCAKey := crclient.ObjectKey{Namespace: "openshift-config", Name: "user-ca-bundle"} + Eventually(func() error { + return hcClient.Get(ctx, userCAKey, &corev1.ConfigMap{}) + }).WithContext(ctx).WithTimeout(hostedUserCABundlePropagationTimeout).WithPolling(defaultPollInterval). + Should(Succeed(), "user-ca-bundle should exist in the hosted cluster") + + // In-place content change (same ConfigMap name / HC reference) must trigger a NodePool + // config rollout (RFE-8751). Legacy HO only rolled on reference name changes. + GinkgoWriter.Printf("Patching trust bundle ConfigMap %s ca-bundle.crt in place\n", trustBundle.Name) + Expect(retry.RetryOnConflict(retry.DefaultRetry, func() error { + current := &corev1.ConfigMap{} + if err := testCtx.MgmtClient.Get(ctx, crclient.ObjectKeyFromObject(trustBundle), current); err != nil { + return err + } + before := current.DeepCopy() + current.Data["ca-bundle.crt"] = "dummy-updated-content" + return testCtx.MgmtClient.Patch(ctx, current, crclient.MergeFromWithOptions(before, crclient.MergeFromWithOptimisticLock{})) + })).To(Succeed(), "failed to patch trust bundle ConfigMap content in place") - e2eutil.EventuallyObject(GinkgoTB(), ctx, fmt.Sprintf("NodePool %s/%s to begin updating", np.Namespace, np.Name), - func(ctx context.Context) (*hyperv1.NodePool, error) { - pool := &hyperv1.NodePool{} - err := testCtx.MgmtClient.Get(ctx, crclient.ObjectKeyFromObject(np), pool) - return pool, err - }, - []e2eutil.Predicate[*hyperv1.NodePool]{ - e2eutil.ConditionPredicate[*hyperv1.NodePool](e2eutil.Condition{ - Type: hyperv1.NodePoolUpdatingConfigConditionType, - Status: metav1.ConditionTrue, - }), - }, - e2eutil.WithInterval(defaultPollInterval), e2eutil.WithTimeout(nodePoolConfigUpdateStartTimeout), - ) - - e2eutil.EventuallyObject(GinkgoTB(), ctx, fmt.Sprintf("NodePool %s/%s to stop updating", np.Namespace, np.Name), - func(ctx context.Context) (*hyperv1.NodePool, error) { - pool := &hyperv1.NodePool{} - err := testCtx.MgmtClient.Get(ctx, crclient.ObjectKeyFromObject(np), pool) - return pool, err - }, - []e2eutil.Predicate[*hyperv1.NodePool]{ - e2eutil.ConditionPredicate[*hyperv1.NodePool](e2eutil.Condition{ - Type: hyperv1.NodePoolUpdatingConfigConditionType, - Status: metav1.ConditionFalse, - }), - e2eutil.ConditionPredicate[*hyperv1.NodePool](e2eutil.Condition{ - Type: hyperv1.NodePoolAllNodesHealthyConditionType, - Status: metav1.ConditionTrue, - }), - }, - e2eutil.WithInterval(defaultPollInterval), e2eutil.WithTimeout(nodePoolConfigUpdateFinishTimeout), - ) - - // Verify user-ca-bundle exists in the hosted cluster - userCAConfigMap := &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: "user-ca-bundle", - Namespace: "openshift-config", - }, - } - e2eutil.EventuallyObject(GinkgoTB(), ctx, "user-ca-bundle to exist in hosted cluster", - func(ctx context.Context) (*corev1.ConfigMap, error) { - cm := &corev1.ConfigMap{} - err := hcClient.Get(ctx, crclient.ObjectKeyFromObject(userCAConfigMap), cm) - return cm, err - }, - []e2eutil.Predicate[*corev1.ConfigMap]{ - func(obj *corev1.ConfigMap) (bool, string, error) { return true, "exists", nil }, - }, - e2eutil.WithInterval(defaultPollInterval), e2eutil.WithTimeout(guestUserCABundlePropagationTimeout), - ) + waitForConfig(Default, corev1.ConditionTrue, nodePoolConfigUpdateStartTimeout, "begin updating after in-place trust bundle content change") + waitForConfig(Default, corev1.ConditionFalse, nodePoolConfigUpdateFinishTimeout, "stop updating after in-place trust bundle content change") - // Remove trust bundle from HostedCluster - GinkgoWriter.Printf("Removing additional trust bundle from HostedCluster\n") - Expect(e2eutil.UpdateObject(GinkgoTB(), ctx, testCtx.MgmtClient, hc, func(obj *hyperv1.HostedCluster) { - obj.Spec.AdditionalTrustBundle = nil - })).To(Succeed(), "failed to remove trust bundle from HostedCluster") + Expect(setTrustBundleReference(nil)).To(Succeed(), "failed to remove trust bundle from HostedCluster") - // Verify CPO deployment no longer mounts the trust bundle - cpNamespace := manifests.HostedControlPlaneNamespace(hc.Namespace, hc.Name) - cpoDeployment := &appsv1.Deployment{ - ObjectMeta: metav1.ObjectMeta{ - Name: "control-plane-operator", - Namespace: cpNamespace, - }, + cpoKey := crclient.ObjectKey{ + Namespace: manifests.HostedControlPlaneNamespace(hc.Namespace, hc.Name), + Name: "control-plane-operator", } - e2eutil.EventuallyObject(GinkgoTB(), ctx, "CPO deployment to stop mounting trust bundle", - func(ctx context.Context) (*appsv1.Deployment, error) { - deploy := &appsv1.Deployment{} - err := testCtx.MgmtClient.Get(ctx, crclient.ObjectKeyFromObject(cpoDeployment), deploy) - return deploy, err - }, - []e2eutil.Predicate[*appsv1.Deployment]{ - func(obj *appsv1.Deployment) (bool, string, error) { - for _, volume := range obj.Spec.Template.Spec.Volumes { - if volume.ConfigMap != nil && volume.ConfigMap.Name == "trusted-ca" { - return false, "trust bundle volume still mounted in CPO", nil - } - } - if ready := podspec.IsDeploymentReady(ctx, obj); !ready { - return false, "CPO deployment is not ready", nil - } - return true, "trust bundle volume removed from CPO", nil - }, - }, - e2eutil.WithInterval(defaultPollInterval), e2eutil.WithTimeout(cpoDeploymentUpdateTimeout), - ) - - // Wait for NodePool to cycle again - e2eutil.EventuallyObject(GinkgoTB(), ctx, fmt.Sprintf("NodePool %s/%s to begin updating after trust bundle removal", np.Namespace, np.Name), - func(ctx context.Context) (*hyperv1.NodePool, error) { - pool := &hyperv1.NodePool{} - err := testCtx.MgmtClient.Get(ctx, crclient.ObjectKeyFromObject(np), pool) - return pool, err - }, - []e2eutil.Predicate[*hyperv1.NodePool]{ - e2eutil.ConditionPredicate[*hyperv1.NodePool](e2eutil.Condition{ - Type: hyperv1.NodePoolUpdatingConfigConditionType, - Status: metav1.ConditionTrue, - }), - }, - e2eutil.WithInterval(defaultPollInterval), e2eutil.WithTimeout(nodePoolConfigUpdateStartTimeout), - ) - - e2eutil.EventuallyObject(GinkgoTB(), ctx, fmt.Sprintf("NodePool %s/%s to stop updating after trust bundle removal", np.Namespace, np.Name), - func(ctx context.Context) (*hyperv1.NodePool, error) { - pool := &hyperv1.NodePool{} - err := testCtx.MgmtClient.Get(ctx, crclient.ObjectKeyFromObject(np), pool) - return pool, err - }, - []e2eutil.Predicate[*hyperv1.NodePool]{ - e2eutil.ConditionPredicate[*hyperv1.NodePool](e2eutil.Condition{ - Type: hyperv1.NodePoolUpdatingConfigConditionType, - Status: metav1.ConditionFalse, - }), - e2eutil.ConditionPredicate[*hyperv1.NodePool](e2eutil.Condition{ - Type: hyperv1.NodePoolAllNodesHealthyConditionType, - Status: metav1.ConditionTrue, - }), - }, - e2eutil.WithInterval(defaultPollInterval), e2eutil.WithTimeout(nodePoolConfigUpdateFinishTimeout), - ) - - // Verify user-ca-bundle is deleted from the hosted cluster (4.22+) - if version.GE(e2eutil.Version422) { - e2eutil.EventuallyNotFound(GinkgoTB(), ctx, hcClient, userCAConfigMap, - e2eutil.WithInterval(10*time.Second), e2eutil.WithTimeout(5*time.Minute), - ) + Eventually(func(g Gomega) { + deploy := &appsv1.Deployment{} + g.Expect(testCtx.MgmtClient.Get(ctx, cpoKey, deploy)).To(Succeed()) + for _, volume := range deploy.Spec.Template.Spec.Volumes { + if volume.ConfigMap != nil { + g.Expect(volume.ConfigMap.Name).NotTo(Equal("trusted-ca"), "trust bundle volume still mounted in CPO") + } + } + g.Expect(podspec.IsDeploymentReady(ctx, deploy)).To(BeTrue(), "CPO deployment should be ready") + }).WithContext(ctx).WithTimeout(cpoDeploymentUpdateTimeout).WithPolling(defaultPollInterval). + Should(Succeed(), "CPO deployment should stop mounting trust bundle") + + waitForConfig(Default, corev1.ConditionTrue, nodePoolConfigUpdateStartTimeout, "begin updating after trust bundle removal") + waitForConfig(Default, corev1.ConditionFalse, nodePoolConfigUpdateFinishTimeout, "stop updating after trust bundle removal") + + if version.GE(semver.MustParse("4.22.0")) { + Eventually(func() error { + return hcClient.Get(ctx, userCAKey, &corev1.ConfigMap{}) + }).WithContext(ctx).WithTimeout(hostedUserCABundlePropagationTimeout).WithPolling(10*time.Second). + Should(Satisfy(apierrors.IsNotFound), "user-ca-bundle should be deleted from the hosted cluster") } }) } diff --git a/test/e2e/v2/tests/nodepool_rollout_control_test.go b/test/e2e/v2/tests/nodepool_rollout_control_test.go index 1b86e2975654..c78edbe22ef2 100644 --- a/test/e2e/v2/tests/nodepool_rollout_control_test.go +++ b/test/e2e/v2/tests/nodepool_rollout_control_test.go @@ -46,7 +46,7 @@ import ( ) const ( - nodePoolCurrentRolloutConfigAnnotation = "hypershift.openshift.io/nodePoolCurrentRolloutConfig" + nodePoolCurrentRolloutConfigAnnotation = "hypershift.openshift.io/nodePoolCurrentRolloutConfigV2" nodePoolCurrentConfigAnnotation = "hypershift.openshift.io/nodePoolCurrentConfig" )