diff --git a/control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/AROSwift/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_role.yaml b/control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/AROSwift/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_role.yaml index eb6ef8d3776f..4603b3c7b2f4 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/AROSwift/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_role.yaml +++ b/control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/AROSwift/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_role.yaml @@ -44,9 +44,12 @@ rules: - pods verbs: - get + - list + - watch - apiGroups: - apps resources: + - deployments - replicasets verbs: - get diff --git a/control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/GCP/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_role.yaml b/control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/GCP/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_role.yaml index eb6ef8d3776f..4603b3c7b2f4 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/GCP/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_role.yaml +++ b/control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/GCP/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_role.yaml @@ -44,9 +44,12 @@ rules: - pods verbs: - get + - list + - watch - apiGroups: - apps resources: + - deployments - replicasets verbs: - get diff --git a/control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_role.yaml b/control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_role.yaml index eb6ef8d3776f..4603b3c7b2f4 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_role.yaml +++ b/control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_role.yaml @@ -44,9 +44,12 @@ rules: - pods verbs: - get + - list + - watch - apiGroups: - apps resources: + - deployments - replicasets verbs: - get diff --git a/control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_role.yaml b/control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_role.yaml index eb6ef8d3776f..4603b3c7b2f4 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_role.yaml +++ b/control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_role.yaml @@ -44,9 +44,12 @@ rules: - pods verbs: - get + - list + - watch - apiGroups: - apps resources: + - deployments - replicasets verbs: - get diff --git a/control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_role.yaml b/control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_role.yaml index eb6ef8d3776f..4603b3c7b2f4 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_role.yaml +++ b/control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_role.yaml @@ -44,9 +44,12 @@ rules: - pods verbs: - get + - list + - watch - apiGroups: - apps resources: + - deployments - replicasets verbs: - get diff --git a/control-plane-operator/controllers/hostedcontrolplane/v2/assets/control-plane-pki-operator/role.yaml b/control-plane-operator/controllers/hostedcontrolplane/v2/assets/control-plane-pki-operator/role.yaml index 29680d33f97f..ad370da320e6 100644 --- a/control-plane-operator/controllers/hostedcontrolplane/v2/assets/control-plane-pki-operator/role.yaml +++ b/control-plane-operator/controllers/hostedcontrolplane/v2/assets/control-plane-pki-operator/role.yaml @@ -35,9 +35,12 @@ rules: - pods verbs: - get + - list + - watch - apiGroups: - apps resources: + - deployments - replicasets verbs: - get diff --git a/control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller.go b/control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller.go index 2b41a438ec13..4e64e59ffe7c 100644 --- a/control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller.go +++ b/control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "math/big" + "net" "sort" "strconv" "strings" @@ -20,6 +21,8 @@ import ( hcpmanifests "github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/manifests" "github.com/openshift/hypershift/control-plane-pki-operator/certificates" "github.com/openshift/hypershift/control-plane-pki-operator/manifests" + "github.com/openshift/hypershift/support/config" + "github.com/openshift/hypershift/support/podspec" "github.com/openshift/library-go/pkg/certs/cert-inspection/certgraphanalysis" "github.com/openshift/library-go/pkg/certs/cert-inspection/certgraphapi" @@ -54,9 +57,14 @@ type CertificateRevocationController struct { getSecret func(namespace, name string) (*corev1.Secret, error) listSecrets func(namespace string) ([]*corev1.Secret, error) getConfigMap func(namespace, name string) (*corev1.ConfigMap, error) + listPods func(namespace string, selector labels.Selector) ([]*corev1.Pod, error) // for unit testing only skipKASConnections bool + // overrideVerifyCertAgainstKASPods, when non-nil, replaces verifyCertificateAgainstAllKASPods + // for unit testing. This allows tests to control per-pod verification behavior without + // requiring real KAS pod connections. + overrideVerifyCertAgainstKASPods func(ctx context.Context, namespace string, adminCfg *rest.Config, certPEM, keyPEM []byte, verifyFunc func(ctx context.Context, client kubernetes.Interface) (bool, error)) (bool, error) } // TODO: we need some sort of time-based GC for completed CRRs @@ -85,11 +93,15 @@ func NewCertificateRevocationController( getConfigMap: func(namespace, name string) (*corev1.ConfigMap, error) { return kubeInformersForNamespaces.InformersFor(namespace).Core().V1().ConfigMaps().Lister().ConfigMaps(namespace).Get(name) }, + listPods: func(namespace string, selector labels.Selector) ([]*corev1.Pod, error) { + return kubeInformersForNamespaces.InformersFor(namespace).Core().V1().Pods().Lister().Pods(namespace).List(selector) + }, } crrInformer := hypershiftInformers.Certificates().V1alpha1().CertificateRevocationRequests().Informer() secretInformer := kubeInformersForNamespaces.InformersFor(hostedControlPlane.Namespace).Core().V1().Secrets().Informer() configMapInformer := kubeInformersForNamespaces.InformersFor(hostedControlPlane.Namespace).Core().V1().ConfigMaps().Informer() + podInformer := kubeInformersForNamespaces.InformersFor(hostedControlPlane.Namespace).Core().V1().Pods().Informer() listCRRs := func(namespace string) ([]*certificatesv1alpha1.CertificateRevocationRequest, error) { return hypershiftInformers.Certificates().V1alpha1().CertificateRevocationRequests().Lister().CertificateRevocationRequests(hostedControlPlane.Namespace).List(labels.Everything()) } @@ -98,6 +110,10 @@ func NewCertificateRevocationController( WithInformersQueueKeysFunc(enqueueCertificateRevocationRequest, crrInformer). WithInformersQueueKeysFunc(enqueueSecret(listCRRs), secretInformer). WithInformersQueueKeysFunc(enqueueConfigMap(listCRRs), configMapInformer). + // KAS pod readiness and PodIP gate certificate verification progress; wiring + // the pod informer ensures that pod transitions trigger immediate reconciliation + // instead of waiting for the next ResyncEvery or SyntheticRequeueError cycle. + WithInformersQueueKeysFunc(enqueueKASPod(listCRRs, hostedControlPlane.Namespace), podInformer). WithSync(c.syncCertificateRevocationRequest). ResyncEvery(time.Minute). ToController("CertificateRevocationController", eventRecorder.WithComponentSuffix(c.fieldManager)) @@ -247,6 +263,20 @@ func enqueueConfigMap(listCRRs func(namespace string) ([]*certificatesv1alpha1.C } } +func enqueueKASPod(listCRRs func(namespace string) ([]*certificatesv1alpha1.CertificateRevocationRequest, error), namespace string) func(obj runtime.Object) []string { + return func(obj runtime.Object) []string { + pod, ok := obj.(*corev1.Pod) + if !ok { + klog.ErrorS(fmt.Errorf("unexpected object of type %T, wanted %T", obj, &corev1.Pod{}), "could not determine queue key") + return nil + } + if !kasAppLabelSelector.Matches(labels.Set(pod.Labels)) { + return nil + } + return enqueueAll(namespace, listCRRs) + } +} + // signerClassForConfigMap determines the signer classes that the configmap contains data for. // We could use this transformation to create an index, but we expect the scale of resource // counts for this controller to be very small (maybe O(10)) and the rate of change to be @@ -535,6 +565,124 @@ func (c *CertificateRevocationController) generateNewSignerCertificate(ctx conte return false, nil, false, nil } +const perPodVerifyTimeout = 10 * time.Second + +// kasAppLabelSelector is the label selector used to find KAS pods in the hosted control plane namespace. +var kasAppLabelSelector = labels.SelectorFromSet(labels.Set{"app": hcpmanifests.KubeAPIServerServiceName}) + +// verifyCertificateAgainstAllKASPods connects to each KAS pod individually to run verifyFunc, +// ensuring that all pods (not just one behind a service load balancer) pass the verification. +// It returns true when all non-terminating ready pods pass. +func (c *CertificateRevocationController) verifyCertificateAgainstAllKASPods( + ctx context.Context, + namespace string, + adminCfg *rest.Config, + certPEM, keyPEM []byte, + verifyFunc func(ctx context.Context, client kubernetes.Interface) (bool, error), +) (bool, error) { + if c.overrideVerifyCertAgainstKASPods != nil { + return c.overrideVerifyCertAgainstKASPods(ctx, namespace, adminCfg, certPEM, keyPEM, verifyFunc) + } + + pods, err := c.listPods(namespace, kasAppLabelSelector) + if err != nil { + return false, fmt.Errorf("couldn't list KAS pods: %w", err) + } + + var readyPods []*corev1.Pod + for _, pod := range pods { + if pod.DeletionTimestamp != nil { + continue + } + if !podspec.IsPodReady(pod) || pod.Status.PodIP == "" { + // a non-terminating pod that's not ready: we can't check it yet, requeue + klog.V(4).Infof("KAS pod %s/%s not ready for verification (ready=%v, podIP=%q), requeueing", pod.Namespace, pod.Name, podspec.IsPodReady(pod), pod.Status.PodIP) + return false, nil + } + readyPods = append(readyPods, pod) + } + + if len(readyPods) == 0 { + // no pods to check yet, requeue + klog.V(4).Infof("No ready KAS pods found in namespace %s, requeueing", namespace) + return false, nil + } + + // Cross-check against the KAS Deployment's expected replica count. + // Without this, if some pods aren't visible (e.g. informer cache lag or + // a pod restarting between list and verification), we could verify only a + // subset and miss pods still serving with old trust bundles. + kasDeployment, err := c.kubeClient.AppsV1().Deployments(namespace).Get(ctx, hcpmanifests.KubeAPIServerServiceName, metav1.GetOptions{}) + if err != nil { + return false, fmt.Errorf("couldn't get KAS deployment to verify replica count: %w", err) + } + expectedReplicas := int32(1) + if kasDeployment.Spec.Replicas != nil { + expectedReplicas = *kasDeployment.Spec.Replicas + } + if int32(len(readyPods)) != expectedReplicas { + klog.V(4).Infof("KAS pod count mismatch in namespace %s: found %d ready pods, expected %d replicas, requeueing", namespace, len(readyPods), expectedReplicas) + return false, nil + } + + for _, pod := range readyPods { + port := podspec.ContainerPort(pod, "client", config.KASPodDefaultPort) + podCfg := rest.AnonymousClientConfig(adminCfg) + podCfg.Timeout = perPodVerifyTimeout + podCfg.TLSClientConfig.CertData = certPEM + podCfg.TLSClientConfig.KeyData = keyPEM + podCfg.Host = fmt.Sprintf("https://%s", net.JoinHostPort(pod.Status.PodIP, strconv.Itoa(int(port)))) + // We're connecting to the PodIP, but the serving cert is still issued for the KAS service + // name. Keep CA verification enabled and override ServerName for SNI + hostname validation. + podCfg.TLSClientConfig.ServerName = hcpmanifests.KubeAPIServerServiceName + + podClient, err := kubernetes.NewForConfig(podCfg) + if err != nil { + return false, fmt.Errorf("couldn't create client for KAS pod %s/%s: %w", pod.Namespace, pod.Name, err) + } + + podCtx, cancel := context.WithTimeout(ctx, perPodVerifyTimeout) + passed, err := verifyFunc(podCtx, podClient) + cancel() + if err != nil { + return false, fmt.Errorf("verification failed against KAS pod %s/%s: %w", pod.Namespace, pod.Name, err) + } + if !passed { + return false, nil + } + } + + return true, nil +} + +// verifyCertificateTrusted checks if a KAS pod trusts the given certificate by performing a SelfSubjectReview. +// Returns true if the pod accepts the certificate, false if unauthorized, error otherwise. +func verifyCertificateTrusted(ctx context.Context, client kubernetes.Interface) (bool, error) { + _, err := client.AuthenticationV1().SelfSubjectReviews().Create(ctx, &authenticationv1.SelfSubjectReview{}, metav1.CreateOptions{}) + if apierrors.IsUnauthorized(err) { + return false, nil + } + if err != nil { + return false, fmt.Errorf("couldn't send SSR to guest cluster: %w", err) + } + return true, nil +} + +// verifyCertificateRevoked checks if a KAS pod has revoked the given certificate by performing a SelfSubjectReview. +// Returns true if the pod rejects the certificate (Unauthorized), false if still trusted, error otherwise. +func verifyCertificateRevoked(ctx context.Context, client kubernetes.Interface) (bool, error) { + _, err := client.AuthenticationV1().SelfSubjectReviews().Create(ctx, &authenticationv1.SelfSubjectReview{}, metav1.CreateOptions{}) + if err == nil { + // pod still trusts the old cert + return false, nil + } + if !apierrors.IsUnauthorized(err) { + return false, fmt.Errorf("couldn't send SSR to guest cluster: %w", err) + } + // unauthorized means the cert is properly revoked on this pod + return true, nil +} + func (c *CertificateRevocationController) ensureNewSignerCertificatePropagated(ctx context.Context, namespace string, name string, now func() time.Time, crr *certificatesv1alpha1.CertificateRevocationRequest) (bool, *actions, bool, error) { signer, ok := secretForSignerClass(namespace, certificates.SignerClass(crr.Spec.SignerClass)) if !ok { @@ -569,6 +717,17 @@ func (c *CertificateRevocationController) ensureNewSignerCertificatePropagated(c return true, nil, false, nil } + var recorded bool + for _, condition := range crr.Status.Conditions { + if condition.Type == certificatesv1alpha1.NewCertificatesTrustedType && condition.Status == metav1.ConditionTrue { + recorded = true + break + } + } + if recorded { + return false, nil, false, nil + } + // the real gate for this phase is that KAS has loaded the updated trust bundle and now // authorizes clients using certificates signed by the new signer - it is difficult to unit-test // that, though, and it's always valid to first check that our certificates have propagated as far @@ -578,7 +737,7 @@ func (c *CertificateRevocationController) ensureNewSignerCertificatePropagated(c } // if the updated trust bundle has propagated as far as we can tell, let's go ahead and ask - // KAS to detect when it trusts the new signer + // each KAS pod individually to detect when it trusts the new signer if !c.skipKASConnections { kubeconfig := hcpmanifests.KASServiceKubeconfigSecret(namespace) kubeconfigSecret, err := c.getSecret(kubeconfig.Namespace, kubeconfig.Name) @@ -593,49 +752,51 @@ func (c *CertificateRevocationController) ensureNewSignerCertificatePropagated(c if err != nil { return true, nil, false, fmt.Errorf("couldn't load guest cluster service network kubeconfig: %w", err) } - certCfg := rest.AnonymousClientConfig(adminCfg) - certCfg.TLSClientConfig.CertData = currentCertPEM - certCfg.TLSClientConfig.KeyData = currentKeyPEM - testClient, err := kubernetes.NewForConfig(certCfg) + allTrusted, err := c.verifyCertificateAgainstAllKASPods(ctx, namespace, adminCfg, currentCertPEM, currentKeyPEM, verifyCertificateTrusted) if err != nil { - return true, nil, false, fmt.Errorf("couldn't create guest cluster client using old certificate: %w", err) - } - - _, err = testClient.AuthenticationV1().SelfSubjectReviews().Create(ctx, &authenticationv1.SelfSubjectReview{}, metav1.CreateOptions{}) - if apierrors.IsUnauthorized(err) { - // this is OK, things are just propagating still - return true, nil, true, nil // we need to synthetically re-queue since nothing about KAS loading will trigger us + return true, nil, false, err } - if err != nil { - return true, nil, false, fmt.Errorf("couldn't send SSR to guest cluster: %w", err) + if !allTrusted { + return true, nil, true, nil // pod transitions trigger reconciliation, but synthetic re-queue is still needed for KAS trust bundle reloads } - } - var recorded bool - for _, condition := range crr.Status.Conditions { - if condition.Type == certificatesv1alpha1.NewCertificatesTrustedType && condition.Status == metav1.ConditionTrue { - recorded = true - break + // Cross-check: verify the previous signer is also still trusted. + // At this stage the old signer hasn't been pruned yet, so both old and new + // should be trusted. If the old is rejected, the pod is mid-reload. + if crr.Status.PreviousSigner != nil { + oldSecret, err := c.getSecret(namespace, crr.Status.PreviousSigner.Name) + if err != nil { + return true, nil, false, err + } + oldCertPEM := oldSecret.Data[corev1.TLSCertKey] + oldKeyPEM := oldSecret.Data[corev1.TLSPrivateKeyKey] + if len(oldCertPEM) > 0 && len(oldKeyPEM) > 0 { + oldTrusted, err := c.verifyCertificateAgainstAllKASPods(ctx, namespace, adminCfg, oldCertPEM, oldKeyPEM, verifyCertificateTrusted) + if err != nil { + return true, nil, false, err + } + if !oldTrusted { + klog.V(4).Infof("KAS pods accepted new cert but rejected old signer cert for %s/%s; likely mid-reload, requeueing", namespace, name) + return true, nil, true, nil + } + } } } - if !recorded { - cfg := certificatesv1alpha1applyconfigurations.CertificateRevocationRequest(name, namespace) - cfg.Status = certificatesv1alpha1applyconfigurations.CertificateRevocationRequestStatus(). - WithRevocationTimestamp(*crr.Status.RevocationTimestamp). - WithPreviousSigner(*crr.Status.PreviousSigner). - WithConditions(conditions(crr.Status.Conditions, metav1applyconfigurations.Condition(). - WithType(certificatesv1alpha1.NewCertificatesTrustedType). - WithStatus(metav1.ConditionTrue). - WithLastTransitionTime(metav1.NewTime(now())). - WithReason(hypershiftv1beta1.AsExpectedReason). - WithMessage(fmt.Sprintf("New signer certificate %s/%s trusted.", signer.Namespace, signer.Name)), - )...) - e := event("CertificateRevocationProgressing", "New %q signer certificates valid.", crr.Spec.SignerClass) - return true, &actions{event: e, crr: cfg}, false, nil - } - return false, nil, false, nil + cfg := certificatesv1alpha1applyconfigurations.CertificateRevocationRequest(name, namespace) + cfg.Status = certificatesv1alpha1applyconfigurations.CertificateRevocationRequestStatus(). + WithRevocationTimestamp(*crr.Status.RevocationTimestamp). + WithPreviousSigner(*crr.Status.PreviousSigner). + WithConditions(conditions(crr.Status.Conditions, metav1applyconfigurations.Condition(). + WithType(certificatesv1alpha1.NewCertificatesTrustedType). + WithStatus(metav1.ConditionTrue). + WithLastTransitionTime(metav1.NewTime(now())). + WithReason(hypershiftv1beta1.AsExpectedReason). + WithMessage(fmt.Sprintf("New signer certificate %s/%s trusted.", signer.Namespace, signer.Name)), + )...) + e := event("CertificateRevocationProgressing", "New %q signer certificates valid.", crr.Spec.SignerClass) + return true, &actions{event: e, crr: cfg}, false, nil } func (c *CertificateRevocationController) generateNewLeafCertificates(ctx context.Context, namespace string, name string, now func() time.Time, crr *certificatesv1alpha1.CertificateRevocationRequest) (bool, *actions, bool, error) { @@ -813,6 +974,17 @@ func (c *CertificateRevocationController) prunePreviousSignerCertificates(ctx co } func (c *CertificateRevocationController) ensureOldSignerCertificateRevoked(ctx context.Context, namespace string, name string, now func() time.Time, crr *certificatesv1alpha1.CertificateRevocationRequest) (bool, *actions, bool, error) { + var recorded bool + for _, condition := range crr.Status.Conditions { + if condition.Type == certificatesv1alpha1.PreviousCertificatesRevokedType && condition.Status == metav1.ConditionTrue { + recorded = true + break + } + } + if recorded { + return false, nil, false, nil + } + oldCertSecret, err := c.getSecret(namespace, crr.Status.PreviousSigner.Name) if err != nil { return true, nil, false, err @@ -833,6 +1005,21 @@ func (c *CertificateRevocationController) ensureOldSignerCertificateRevoked(ctx return true, nil, false, fmt.Errorf("signer certificate %s/%s had no data for %s", oldCertSecret.Namespace, oldCertSecret.Name, corev1.TLSPrivateKeyKey) } + // Load the current (new) signer cert/key for cross-checking during per-pod verification. + signer, ok := secretForSignerClass(namespace, certificates.SignerClass(crr.Spec.SignerClass)) + if !ok { + return true, nil, false, nil + } + signerSecret, _, err := c.loadCertificateSecret(signer.Namespace, signer.Name) + if err != nil { + return true, nil, false, err + } + if signerSecret == nil { + return true, nil, false, nil + } + currentCertPEM := signerSecret.Data[corev1.TLSCertKey] + currentKeyPEM := signerSecret.Data[corev1.TLSPrivateKeyKey] + totalClientCA := manifests.TotalKASClientCABundle(namespace) totalClientTrustBundle, err := c.loadTrustBundleConfigMap(totalClientCA.Namespace, totalClientCA.Name) if err != nil { @@ -850,7 +1037,7 @@ func (c *CertificateRevocationController) ensureOldSignerCertificateRevoked(ctx } // if the updated trust bundle has propagated as far as we can tell, let's go ahead and ask - // KAS to ensure it no longer trusts the old signer + // each KAS pod individually to ensure it no longer trusts the old signer if !c.skipKASConnections { kubeconfig := hcpmanifests.KASServiceKubeconfigSecret(namespace) kubeconfigSecret, err := c.getSecret(kubeconfig.Namespace, kubeconfig.Name) @@ -865,49 +1052,43 @@ func (c *CertificateRevocationController) ensureOldSignerCertificateRevoked(ctx if err != nil { return true, nil, false, fmt.Errorf("couldn't load guest cluster service network kubeconfig: %w", err) } - certCfg := rest.AnonymousClientConfig(adminCfg) - certCfg.TLSClientConfig.CertData = oldCertPEM - certCfg.TLSClientConfig.KeyData = oldKeyPEM - testClient, err := kubernetes.NewForConfig(certCfg) + allRevoked, err := c.verifyCertificateAgainstAllKASPods(ctx, namespace, adminCfg, oldCertPEM, oldKeyPEM, verifyCertificateRevoked) if err != nil { - return true, nil, false, fmt.Errorf("couldn't create guest cluster client using old certificate: %w", err) + return true, nil, false, err + } + if !allRevoked { + return true, nil, true, nil // pod transitions trigger reconciliation, but synthetic re-queue is still needed for KAS trust bundle reloads } - _, err = testClient.AuthenticationV1().SelfSubjectReviews().Create(ctx, &authenticationv1.SelfSubjectReview{}, metav1.CreateOptions{}) - if err == nil { - // this is OK, things are just propagating still - return true, nil, true, nil // we need to synthetically re-queue since nothing about KAS loading will trigger us + // Cross-check: verify the current signer is still trusted by all pods. + // This guards against transient states where kube-apiserver rejects ALL + // certificates during a trust bundle reload, which could produce a false + // positive on the revocation check above. + allTrusted, err := c.verifyCertificateAgainstAllKASPods(ctx, namespace, adminCfg, currentCertPEM, currentKeyPEM, verifyCertificateTrusted) + if err != nil { + return true, nil, false, err } - if !apierrors.IsUnauthorized(err) { - return true, nil, false, fmt.Errorf("couldn't send SSR to guest cluster: %w", err) + if !allTrusted { + klog.V(4).Infof("KAS pods rejected old cert but also rejected current signer cert for %s/%s; likely mid-reload, requeueing", namespace, name) + return true, nil, true, nil } } - var recorded bool - for _, condition := range crr.Status.Conditions { - if condition.Type == certificatesv1alpha1.PreviousCertificatesRevokedType && condition.Status == metav1.ConditionTrue { - recorded = true - break - } - } - if !recorded { - cfg := certificatesv1alpha1applyconfigurations.CertificateRevocationRequest(name, namespace) - cfg.Status = certificatesv1alpha1applyconfigurations.CertificateRevocationRequestStatus(). - WithRevocationTimestamp(*crr.Status.RevocationTimestamp). - WithPreviousSigner(*crr.Status.PreviousSigner). - WithConditions(conditions(crr.Status.Conditions, - metav1applyconfigurations.Condition(). - WithType(certificatesv1alpha1.PreviousCertificatesRevokedType). - WithStatus(metav1.ConditionTrue). - WithLastTransitionTime(metav1.NewTime(now())). - WithReason(hypershiftv1beta1.AsExpectedReason). - WithMessage("Previous signer certificate revoked."), - )...) - e := event("CertificateRevocationComplete", "%q signer certificates revoked.", crr.Spec.SignerClass) - return true, &actions{event: e, crr: cfg}, false, nil - } - return false, nil, false, nil + cfg := certificatesv1alpha1applyconfigurations.CertificateRevocationRequest(name, namespace) + cfg.Status = certificatesv1alpha1applyconfigurations.CertificateRevocationRequestStatus(). + WithRevocationTimestamp(*crr.Status.RevocationTimestamp). + WithPreviousSigner(*crr.Status.PreviousSigner). + WithConditions(conditions(crr.Status.Conditions, + metav1applyconfigurations.Condition(). + WithType(certificatesv1alpha1.PreviousCertificatesRevokedType). + WithStatus(metav1.ConditionTrue). + WithLastTransitionTime(metav1.NewTime(now())). + WithReason(hypershiftv1beta1.AsExpectedReason). + WithMessage("Previous signer certificate revoked."), + )...) + e := event("CertificateRevocationComplete", "%q signer certificates revoked.", crr.Spec.SignerClass) + return true, &actions{event: e, crr: cfg}, false, nil } func (c *CertificateRevocationController) loadCertificateSecret(namespace, name string) (*corev1.Secret, []*x509.Certificate, error) { diff --git a/control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller_test.go b/control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller_test.go index 63b3592c2da6..d8c7b96f010b 100644 --- a/control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller_test.go +++ b/control-plane-pki-operator/certificaterevocationcontroller/certificaterevocationcontroller_test.go @@ -1,28 +1,42 @@ package certificaterevocationcontroller import ( + "context" "crypto/x509" "crypto/x509/pkix" "embed" + "fmt" "os" "path/filepath" "testing" "time" + . "github.com/onsi/gomega" + certificatesv1alpha1 "github.com/openshift/hypershift/api/certificates/v1alpha1" hypershiftv1beta1 "github.com/openshift/hypershift/api/hypershift/v1beta1" certificatesv1alpha1applyconfigurations "github.com/openshift/hypershift/client/applyconfiguration/certificates/v1alpha1" + hcpmanifests "github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/manifests" "github.com/openshift/hypershift/control-plane-pki-operator/certificates" "github.com/openshift/hypershift/control-plane-pki-operator/manifests" librarygocrypto "github.com/openshift/library-go/pkg/crypto" "github.com/openshift/library-go/pkg/operator/certrotation" + appsv1 "k8s.io/api/apps/v1" 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/labels" + "k8s.io/apimachinery/pkg/runtime" corev1applyconfigurations "k8s.io/client-go/applyconfigurations/core/v1" metav1applyconfigurations "k8s.io/client-go/applyconfigurations/meta/v1" + "k8s.io/client-go/kubernetes" + kubefake "k8s.io/client-go/kubernetes/fake" + "k8s.io/client-go/rest" + k8stesting "k8s.io/client-go/testing" + "k8s.io/client-go/tools/clientcmd" + clientcmdapi "k8s.io/client-go/tools/clientcmd/api" "k8s.io/client-go/util/cert" testingclock "k8s.io/utils/clock/testing" "k8s.io/utils/ptr" @@ -1047,6 +1061,9 @@ func TestCertificateRevocationController_processCertificateRevocationRequest(t * } return nil, apierrors.NewNotFound(corev1.SchemeGroupVersion.WithResource("configmaps").GroupResource(), name) }, + listPods: func(namespace string, selector labels.Selector) ([]*corev1.Pod, error) { + return nil, nil + }, skipKASConnections: true, } a, requeue, err := c.processCertificateRevocationRequest(t.Context(), testCase.crrNamespace, testCase.crrName, testCase.now) @@ -1076,3 +1093,859 @@ func compareActions() []cmp.Option { cmpopts.IgnoreFields(metav1applyconfigurations.ConditionApplyConfiguration{}, "ObservedGeneration"), } } + +func kasPodSpec() corev1.PodSpec { + return corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: "kube-apiserver", + Ports: []corev1.ContainerPort{{ + Name: "client", + ContainerPort: 6443, + }}, + }}, + } +} + +func fakeKubeClientWithKASDeployment(replicas int32) kubernetes.Interface { + return kubefake.NewClientset(&appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: hcpmanifests.KubeAPIServerServiceName, + Namespace: "test-ns", + }, + Spec: appsv1.DeploymentSpec{ + Replicas: ptr.To(replicas), + }, + }) +} + +func TestVerifyCertificateAgainstAllKASPods(t *testing.T) { + t.Parallel() + now := time.Now() + for _, testCase := range []struct { + name string + pods []*corev1.Pod + listPodsErr error + kasReplicas int32 + verifyFunc func(ctx context.Context, client kubernetes.Interface) (bool, error) + + expectedResult bool + expectedErr bool + expectedCalls int + }{ + { + name: "When all pods are terminating it should requeue", + pods: []*corev1.Pod{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "kas-1", + Namespace: "test-ns", + DeletionTimestamp: &metav1.Time{Time: now}, + }, + Spec: kasPodSpec(), + Status: corev1.PodStatus{PodIP: "10.0.0.1"}, + }}, + expectedResult: false, + }, + { + name: "When no pods exist it should requeue", + pods: []*corev1.Pod{}, + expectedResult: false, + }, + { + name: "When a non-terminating pod is not ready it should requeue", + pods: []*corev1.Pod{{ + ObjectMeta: metav1.ObjectMeta{Name: "kas-1", Namespace: "test-ns"}, + Spec: kasPodSpec(), + Status: corev1.PodStatus{ + PodIP: "10.0.0.1", + Conditions: []corev1.PodCondition{{ + Type: corev1.PodReady, + Status: corev1.ConditionFalse, + }}, + }, + }}, + expectedResult: false, + }, + { + name: "When a ready pod has empty PodIP it should requeue", + pods: []*corev1.Pod{{ + ObjectMeta: metav1.ObjectMeta{Name: "kas-1", Namespace: "test-ns"}, + Spec: kasPodSpec(), + Status: corev1.PodStatus{ + PodIP: "", + Conditions: []corev1.PodCondition{{ + Type: corev1.PodReady, + Status: corev1.ConditionTrue, + }}, + }, + }}, + expectedResult: false, + }, + { + name: "When a mix of terminating and not-ready pods exists it should requeue", + pods: []*corev1.Pod{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "kas-1", + Namespace: "test-ns", + DeletionTimestamp: &metav1.Time{Time: now}, + }, + Spec: kasPodSpec(), + Status: corev1.PodStatus{ + PodIP: "10.0.0.1", + Conditions: []corev1.PodCondition{{ + Type: corev1.PodReady, + Status: corev1.ConditionTrue, + }}, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{Name: "kas-2", Namespace: "test-ns"}, + Spec: kasPodSpec(), + Status: corev1.PodStatus{ + PodIP: "10.0.0.2", + Conditions: []corev1.PodCondition{{ + Type: corev1.PodReady, + Status: corev1.ConditionFalse, + }}, + }, + }, + }, + expectedResult: false, + }, + { + name: "When ready non-terminating pods exist it should call verifyFunc for each", + pods: []*corev1.Pod{ + { + ObjectMeta: metav1.ObjectMeta{Name: "kas-1", Namespace: "test-ns"}, + Spec: kasPodSpec(), + Status: corev1.PodStatus{ + PodIP: "10.0.0.1", + Conditions: []corev1.PodCondition{{ + Type: corev1.PodReady, + Status: corev1.ConditionTrue, + }}, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{Name: "kas-2", Namespace: "test-ns"}, + Spec: kasPodSpec(), + Status: corev1.PodStatus{ + PodIP: "10.0.0.2", + Conditions: []corev1.PodCondition{{ + Type: corev1.PodReady, + Status: corev1.ConditionTrue, + }}, + }, + }, + }, + kasReplicas: 2, + verifyFunc: func(_ context.Context, _ kubernetes.Interface) (bool, error) { + return true, nil + }, + expectedResult: true, + expectedCalls: 2, + }, + { + name: "When ready pod count does not match expected replicas it should requeue", + pods: []*corev1.Pod{ + { + ObjectMeta: metav1.ObjectMeta{Name: "kas-1", Namespace: "test-ns"}, + Spec: kasPodSpec(), + Status: corev1.PodStatus{ + PodIP: "10.0.0.1", + Conditions: []corev1.PodCondition{{ + Type: corev1.PodReady, + Status: corev1.ConditionTrue, + }}, + }, + }, + }, + kasReplicas: 3, + expectedResult: false, + }, + { + name: "When one pod's verifyFunc returns false it should return false", + pods: []*corev1.Pod{ + { + ObjectMeta: metav1.ObjectMeta{Name: "kas-1", Namespace: "test-ns"}, + Spec: kasPodSpec(), + Status: corev1.PodStatus{ + PodIP: "10.0.0.1", + Conditions: []corev1.PodCondition{{ + Type: corev1.PodReady, + Status: corev1.ConditionTrue, + }}, + }, + }, + }, + kasReplicas: 1, + verifyFunc: func(_ context.Context, _ kubernetes.Interface) (bool, error) { + return false, nil + }, + expectedResult: false, + expectedCalls: 1, + }, + { + name: "When listing pods fails it should return an error", + listPodsErr: fmt.Errorf("connection refused"), + expectedResult: false, + expectedErr: true, + }, + { + name: "When verifyFunc returns an error it should return an error", + pods: []*corev1.Pod{{ + ObjectMeta: metav1.ObjectMeta{Name: "kas-1", Namespace: "test-ns"}, + Spec: kasPodSpec(), + Status: corev1.PodStatus{ + PodIP: "10.0.0.1", + Conditions: []corev1.PodCondition{{ + Type: corev1.PodReady, + Status: corev1.ConditionTrue, + }}, + }, + }}, + kasReplicas: 1, + verifyFunc: func(_ context.Context, _ kubernetes.Interface) (bool, error) { + return false, fmt.Errorf("SSR failed") + }, + expectedResult: false, + expectedErr: true, + expectedCalls: 1, + }, + } { + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + c := &CertificateRevocationController{ + kubeClient: fakeKubeClientWithKASDeployment(testCase.kasReplicas), + listPods: func(namespace string, selector labels.Selector) ([]*corev1.Pod, error) { + g.Expect(namespace).To(Equal("test-ns")) + g.Expect(selector.String()).To(Equal(kasAppLabelSelector.String())) + if testCase.listPodsErr != nil { + return nil, testCase.listPodsErr + } + return testCase.pods, nil + }, + } + + // verifyFunc may not be called for all cases (e.g. when pods are terminating or not ready) + callCount := 0 + verifyFunc := testCase.verifyFunc + if verifyFunc == nil { + verifyFunc = func(_ context.Context, _ kubernetes.Interface) (bool, error) { + t.Fatal("verifyFunc should not have been called") + return false, nil + } + } else { + original := verifyFunc + verifyFunc = func(ctx context.Context, client kubernetes.Interface) (bool, error) { + callCount++ + return original(ctx, client) + } + } + + result, err := c.verifyCertificateAgainstAllKASPods(t.Context(), "test-ns", &rest.Config{}, nil, nil, verifyFunc) + if testCase.expectedErr { + g.Expect(err).To(HaveOccurred()) + } else { + g.Expect(err).ToNot(HaveOccurred()) + } + g.Expect(result).To(Equal(testCase.expectedResult)) + g.Expect(callCount).To(Equal(testCase.expectedCalls)) + }) + } +} + +func TestEnqueueKASPod(t *testing.T) { + t.Parallel() + crr := &certificatesv1alpha1.CertificateRevocationRequest{ + ObjectMeta: metav1.ObjectMeta{Name: "test-crr", Namespace: "test-ns"}, + } + listCRRs := func(namespace string) ([]*certificatesv1alpha1.CertificateRevocationRequest, error) { + return []*certificatesv1alpha1.CertificateRevocationRequest{crr}, nil + } + listCRRsErr := func(namespace string) ([]*certificatesv1alpha1.CertificateRevocationRequest, error) { + return nil, fmt.Errorf("connection refused") + } + + for _, testCase := range []struct { + name string + obj runtime.Object + listCRRs func(namespace string) ([]*certificatesv1alpha1.CertificateRevocationRequest, error) + expectedKeys int + }{ + { + name: "When pod has KAS label it should enqueue all CRRs", + obj: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "kas-1", + Namespace: "test-ns", + Labels: map[string]string{"app": "kube-apiserver"}, + }, + }, + listCRRs: listCRRs, + expectedKeys: 1, + }, + { + name: "When pod does not have KAS label it should return nil", + obj: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "etcd-0", + Namespace: "test-ns", + Labels: map[string]string{"app": "etcd"}, + }, + }, + listCRRs: listCRRs, + expectedKeys: 0, + }, + { + name: "When pod has no labels it should return nil", + obj: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "unlabeled", + Namespace: "test-ns", + }, + }, + listCRRs: listCRRs, + expectedKeys: 0, + }, + { + name: "When object is not a pod it should return nil", + obj: &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "cm"}}, + listCRRs: listCRRs, + expectedKeys: 0, + }, + { + name: "When listing CRRs fails it should return nil", + obj: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "kas-1", + Namespace: "test-ns", + Labels: map[string]string{"app": "kube-apiserver"}, + }, + }, + listCRRs: listCRRsErr, + expectedKeys: 0, + }, + } { + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + enqueue := enqueueKASPod(testCase.listCRRs, "test-ns") + keys := enqueue(testCase.obj) + g.Expect(keys).To(HaveLen(testCase.expectedKeys)) + }) + } +} + +func TestVerifyCertificateTrusted(t *testing.T) { + t.Parallel() + for _, testCase := range []struct { + name string + reactor func(action k8stesting.Action) (bool, runtime.Object, error) + expectedResult bool + expectedErr bool + }{ + { + name: "When SSR succeeds it should return true", + reactor: func(action k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, nil + }, + expectedResult: true, + }, + { + name: "When SSR returns Unauthorized it should return false", + reactor: func(action k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, apierrors.NewUnauthorized("not authorized") + }, + expectedResult: false, + }, + { + name: "When SSR returns other error it should return error", + reactor: func(action k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, fmt.Errorf("connection refused") + }, + expectedResult: false, + expectedErr: true, + }, + } { + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + fakeClient := kubefake.NewClientset() + fakeClient.PrependReactor("create", "selfsubjectreviews", testCase.reactor) + + result, err := verifyCertificateTrusted(t.Context(), fakeClient) + if testCase.expectedErr { + g.Expect(err).To(HaveOccurred()) + } else { + g.Expect(err).ToNot(HaveOccurred()) + } + g.Expect(result).To(Equal(testCase.expectedResult)) + }) + } +} + +func TestVerifyCertificateRevoked(t *testing.T) { + t.Parallel() + for _, testCase := range []struct { + name string + reactor func(action k8stesting.Action) (bool, runtime.Object, error) + expectedResult bool + expectedErr bool + }{ + { + name: "When SSR succeeds it should return false because pod still trusts the cert", + reactor: func(action k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, nil + }, + expectedResult: false, + }, + { + name: "When SSR returns Unauthorized it should return true because cert is revoked", + reactor: func(action k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, apierrors.NewUnauthorized("not authorized") + }, + expectedResult: true, + }, + { + name: "When SSR returns other error it should return error", + reactor: func(action k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, fmt.Errorf("connection refused") + }, + expectedResult: false, + expectedErr: true, + }, + } { + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + fakeClient := kubefake.NewClientset() + fakeClient.PrependReactor("create", "selfsubjectreviews", testCase.reactor) + + result, err := verifyCertificateRevoked(t.Context(), fakeClient) + if testCase.expectedErr { + g.Expect(err).To(HaveOccurred()) + } else { + g.Expect(err).ToNot(HaveOccurred()) + } + g.Expect(result).To(Equal(testCase.expectedResult)) + }) + } +} + +func makeKubeconfig(t *testing.T) []byte { + t.Helper() + kubeconfigData, err := clientcmd.Write(clientcmdapi.Config{ + Clusters: map[string]*clientcmdapi.Cluster{ + "default": { + Server: "https://kube-apiserver:6443", + InsecureSkipTLSVerify: true, + }, + }, + AuthInfos: map[string]*clientcmdapi.AuthInfo{ + "admin": {}, + }, + Contexts: map[string]*clientcmdapi.Context{ + "default": { + Cluster: "default", + AuthInfo: "admin", + }, + }, + CurrentContext: "default", + }) + if err != nil { + t.Fatalf("failed to write kubeconfig: %v", err) + } + return kubeconfigData +} + +func TestEnsureNewSignerCertificatePropagated_KASVerification(t *testing.T) { + t.Parallel() + revocationTime, err := time.Parse(time.RFC3339Nano, "2006-01-02T15:04:05.999999999Z") + if err != nil { + t.Fatalf("could not parse time: %v", err) + } + postRevocationClock := testingclock.NewFakeClock(revocationTime.Add(revocationOffset + 1*time.Hour)) + + data := pki(t, revocationTime) + + newPropagatedCRR := func() *certificatesv1alpha1.CertificateRevocationRequest { + return &certificatesv1alpha1.CertificateRevocationRequest{ + ObjectMeta: metav1.ObjectMeta{Namespace: "crr-ns", Name: "crr-name"}, + Spec: certificatesv1alpha1.CertificateRevocationRequestSpec{SignerClass: string(certificates.CustomerBreakGlassSigner)}, + Status: certificatesv1alpha1.CertificateRevocationRequestStatus{ + RevocationTimestamp: ptr.To(metav1.NewTime(revocationTime)), + PreviousSigner: &corev1.LocalObjectReference{Name: "1pfcydcz358pa1glirkmc72sdkf5zw21uam4jbnj03pw"}, + Conditions: []metav1.Condition{{ + Type: certificatesv1alpha1.RootCertificatesRegeneratedType, + Status: metav1.ConditionTrue, + LastTransitionTime: metav1.NewTime(postRevocationClock.Now()), + Reason: hypershiftv1beta1.AsExpectedReason, + Message: `Signer certificate crr-ns/customer-system-admin-signer regenerated.`, + }}, + }, + } + } + + newPropagatedSecrets := func(kubeconfigData []byte) []*corev1.Secret { + return []*corev1.Secret{{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "crr-ns", + Name: manifests.CustomerSystemAdminSigner("").Name, + }, + Data: map[string][]byte{ + corev1.TLSCertKey: data.future.raw.signerCert, + corev1.TLSPrivateKeyKey: data.future.raw.signerKey, + }, + }, { + ObjectMeta: metav1.ObjectMeta{ + Namespace: "crr-ns", + Name: "1pfcydcz358pa1glirkmc72sdkf5zw21uam4jbnj03pw", + }, + Data: map[string][]byte{ + corev1.TLSCertKey: data.original.raw.signerCert, + corev1.TLSPrivateKeyKey: data.original.raw.signerKey, + }, + }, { + ObjectMeta: metav1.ObjectMeta{ + Namespace: "crr-ns", + Name: hcpmanifests.KASServiceKubeconfigSecret("").Name, + }, + Data: map[string][]byte{ + "kubeconfig": kubeconfigData, + }, + }} + } + + newPropagatedCMs := func() []*corev1.ConfigMap { + return []*corev1.ConfigMap{{ + ObjectMeta: metav1.ObjectMeta{Namespace: "crr-ns", Name: manifests.TotalKASClientCABundle("").Name}, + Data: map[string]string{ + "ca-bundle.crt": string(data.original.raw.signerCert) + string(data.future.raw.signerCert), + }, + }} + } + + newPropagatedController := func(crr *certificatesv1alpha1.CertificateRevocationRequest, secrets []*corev1.Secret, cms []*corev1.ConfigMap) *CertificateRevocationController { + return &CertificateRevocationController{ + getCRR: func(namespace, name string) (*certificatesv1alpha1.CertificateRevocationRequest, error) { + return crr, nil + }, + getSecret: func(namespace, name string) (*corev1.Secret, error) { + for _, s := range secrets { + if s.Namespace == namespace && s.Name == name { + return s, nil + } + } + return nil, apierrors.NewNotFound(corev1.SchemeGroupVersion.WithResource("secrets").GroupResource(), name) + }, + listSecrets: func(namespace string) ([]*corev1.Secret, error) { + return secrets, nil + }, + getConfigMap: func(namespace, name string) (*corev1.ConfigMap, error) { + for _, cm := range cms { + if cm.Namespace == namespace && cm.Name == name { + return cm, nil + } + } + return nil, apierrors.NewNotFound(corev1.SchemeGroupVersion.WithResource("configmaps").GroupResource(), name) + }, + listPods: func(namespace string, selector labels.Selector) ([]*corev1.Pod, error) { + return nil, nil + }, + skipKASConnections: false, + } + } + + t.Run("When no KAS pods exist it should requeue", func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + kubeconfigData := makeKubeconfig(t) + crr := newPropagatedCRR() + secrets := newPropagatedSecrets(kubeconfigData) + cms := newPropagatedCMs() + c := newPropagatedController(crr, secrets, cms) + + _, requeue, err := c.processCertificateRevocationRequest(t.Context(), "crr-ns", "crr-name", postRevocationClock.Now) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(requeue).To(BeTrue(), "should requeue when no KAS pods exist") + }) + + t.Run("When all KAS pods accept new cert but reject old cert it should requeue", func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + kubeconfigData := makeKubeconfig(t) + crr := newPropagatedCRR() + secrets := newPropagatedSecrets(kubeconfigData) + cms := newPropagatedCMs() + c := newPropagatedController(crr, secrets, cms) + + // Override verification: new cert is trusted, but old cert is rejected (mid-reload) + c.overrideVerifyCertAgainstKASPods = func(_ context.Context, _ string, _ *rest.Config, certPEM, _ []byte, verifyFunc func(context.Context, kubernetes.Interface) (bool, error)) (bool, error) { + fakeClient := kubefake.NewClientset() + if string(certPEM) == string(data.future.raw.signerCert) { + // new cert: trusted + fakeClient.PrependReactor("create", "selfsubjectreviews", func(action k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, nil + }) + } else { + // old cert: rejected (mid-reload) + fakeClient.PrependReactor("create", "selfsubjectreviews", func(action k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, apierrors.NewUnauthorized("not authorized") + }) + } + return verifyFunc(context.Background(), fakeClient) + } + + _, requeue, err := c.processCertificateRevocationRequest(t.Context(), "crr-ns", "crr-name", postRevocationClock.Now) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(requeue).To(BeTrue(), "should requeue when KAS pods reject old cert during mid-reload") + }) + + t.Run("When all KAS pods accept both new and old certs it should mark trusted", func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + kubeconfigData := makeKubeconfig(t) + crr := newPropagatedCRR() + secrets := newPropagatedSecrets(kubeconfigData) + cms := newPropagatedCMs() + c := newPropagatedController(crr, secrets, cms) + + // Override verification: both new and old certs are trusted + c.overrideVerifyCertAgainstKASPods = func(_ context.Context, _ string, _ *rest.Config, _ []byte, _ []byte, verifyFunc func(context.Context, kubernetes.Interface) (bool, error)) (bool, error) { + fakeClient := kubefake.NewClientset() + fakeClient.PrependReactor("create", "selfsubjectreviews", func(action k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, nil + }) + return verifyFunc(context.Background(), fakeClient) + } + + a, requeue, err := c.processCertificateRevocationRequest(t.Context(), "crr-ns", "crr-name", postRevocationClock.Now) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(requeue).To(BeFalse()) + g.Expect(a).ToNot(BeNil()) + g.Expect(a.crr).ToNot(BeNil()) + // Should have set NewCertificatesTrustedType to True + var foundTrusted bool + for _, cond := range a.crr.Status.Conditions { + if cond.Type != nil && *cond.Type == certificatesv1alpha1.NewCertificatesTrustedType && + cond.Status != nil && *cond.Status == metav1.ConditionTrue { + foundTrusted = true + } + } + g.Expect(foundTrusted).To(BeTrue(), "should have set NewCertificatesTrustedType condition to True") + }) +} + +func TestEnsureOldSignerCertificateRevoked_KASVerification(t *testing.T) { + t.Parallel() + revocationTime, err := time.Parse(time.RFC3339Nano, "2006-01-02T15:04:05.999999999Z") + if err != nil { + t.Fatalf("could not parse time: %v", err) + } + postRevocationClock := testingclock.NewFakeClock(revocationTime.Add(revocationOffset + 1*time.Hour)) + + data := pki(t, revocationTime) + + newRevokedCRR := func() *certificatesv1alpha1.CertificateRevocationRequest { + return &certificatesv1alpha1.CertificateRevocationRequest{ + ObjectMeta: metav1.ObjectMeta{Namespace: "crr-ns", Name: "crr-name"}, + Spec: certificatesv1alpha1.CertificateRevocationRequestSpec{SignerClass: string(certificates.CustomerBreakGlassSigner)}, + Status: certificatesv1alpha1.CertificateRevocationRequestStatus{ + RevocationTimestamp: ptr.To(metav1.NewTime(revocationTime)), + PreviousSigner: &corev1.LocalObjectReference{Name: "1pfcydcz358pa1glirkmc72sdkf5zw21uam4jbnj03pw"}, + Conditions: []metav1.Condition{{ + Type: certificatesv1alpha1.LeafCertificatesRegeneratedType, + Status: metav1.ConditionTrue, + LastTransitionTime: metav1.NewTime(postRevocationClock.Now()), + Reason: hypershiftv1beta1.AsExpectedReason, + Message: `All leaf certificates are re-generated.`, + }, { + Type: certificatesv1alpha1.RootCertificatesRegeneratedType, + Status: metav1.ConditionTrue, + LastTransitionTime: metav1.NewTime(postRevocationClock.Now()), + Reason: hypershiftv1beta1.AsExpectedReason, + Message: `Signer certificate crr-ns/customer-system-admin-signer regenerated.`, + }, { + Type: certificatesv1alpha1.NewCertificatesTrustedType, + Status: metav1.ConditionTrue, + LastTransitionTime: metav1.NewTime(postRevocationClock.Now()), + Reason: hypershiftv1beta1.AsExpectedReason, + Message: `New signer certificate crr-ns/customer-system-admin-signer trusted.`, + }}, + }, + } + } + + newRevokedSecrets := func(kubeconfigData []byte) []*corev1.Secret { + return []*corev1.Secret{{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "crr-ns", + Name: manifests.CustomerSystemAdminSigner("").Name, + Annotations: map[string]string{certrotation.CertificateIssuer: "crr-ns_customer-break-glass-signer@1234"}, + }, + Data: map[string][]byte{ + corev1.TLSCertKey: data.future.raw.signerCert, + corev1.TLSPrivateKeyKey: data.future.raw.signerKey, + }, + }, { + ObjectMeta: metav1.ObjectMeta{ + Namespace: "crr-ns", + Name: "1pfcydcz358pa1glirkmc72sdkf5zw21uam4jbnj03pw", + }, + Data: map[string][]byte{ + corev1.TLSCertKey: data.original.raw.signerCert, + corev1.TLSPrivateKeyKey: data.original.raw.signerKey, + }, + }, { + ObjectMeta: metav1.ObjectMeta{ + Namespace: "crr-ns", + Name: manifests.CustomerSystemAdminClientCertSecret("").Name, + Annotations: map[string]string{certrotation.CertificateIssuer: "crr-ns_customer-break-glass-signer@1234"}, + }, + Data: map[string][]byte{ + corev1.TLSCertKey: data.future.raw.signedCert, + corev1.TLSPrivateKeyKey: data.future.raw.clientKey, + }, + }, { + ObjectMeta: metav1.ObjectMeta{ + Namespace: "crr-ns", + Name: hcpmanifests.KASServiceKubeconfigSecret("").Name, + }, + Data: map[string][]byte{ + "kubeconfig": kubeconfigData, + }, + }} + } + + newRevokedCMs := func() []*corev1.ConfigMap { + return []*corev1.ConfigMap{{ + ObjectMeta: metav1.ObjectMeta{Namespace: "crr-ns", Name: manifests.CustomerSystemAdminSignerCA("").Name}, + Data: map[string]string{ + "ca-bundle.crt": string(data.future.raw.signerCert), + }, + }, { + ObjectMeta: metav1.ObjectMeta{Namespace: "crr-ns", Name: manifests.TotalKASClientCABundle("").Name}, + Data: map[string]string{ + "ca-bundle.crt": string(data.future.raw.signerCert), + }, + }} + } + + newRevokedController := func(crr *certificatesv1alpha1.CertificateRevocationRequest, secrets []*corev1.Secret, cms []*corev1.ConfigMap) *CertificateRevocationController { + return &CertificateRevocationController{ + getCRR: func(namespace, name string) (*certificatesv1alpha1.CertificateRevocationRequest, error) { + return crr, nil + }, + getSecret: func(namespace, name string) (*corev1.Secret, error) { + for _, s := range secrets { + if s.Namespace == namespace && s.Name == name { + return s, nil + } + } + return nil, apierrors.NewNotFound(corev1.SchemeGroupVersion.WithResource("secrets").GroupResource(), name) + }, + listSecrets: func(namespace string) ([]*corev1.Secret, error) { + return secrets, nil + }, + getConfigMap: func(namespace, name string) (*corev1.ConfigMap, error) { + for _, cm := range cms { + if cm.Namespace == namespace && cm.Name == name { + return cm, nil + } + } + return nil, apierrors.NewNotFound(corev1.SchemeGroupVersion.WithResource("configmaps").GroupResource(), name) + }, + listPods: func(namespace string, selector labels.Selector) ([]*corev1.Pod, error) { + return nil, nil + }, + skipKASConnections: false, + } + } + + t.Run("When no KAS pods exist it should requeue", func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + kubeconfigData := makeKubeconfig(t) + crr := newRevokedCRR() + secrets := newRevokedSecrets(kubeconfigData) + cms := newRevokedCMs() + c := newRevokedController(crr, secrets, cms) + + _, requeue, err := c.processCertificateRevocationRequest(t.Context(), "crr-ns", "crr-name", postRevocationClock.Now) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(requeue).To(BeTrue(), "should requeue when no KAS pods exist") + }) + + t.Run("When all KAS pods reject old cert but also reject current cert it should requeue", func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + kubeconfigData := makeKubeconfig(t) + crr := newRevokedCRR() + secrets := newRevokedSecrets(kubeconfigData) + cms := newRevokedCMs() + c := newRevokedController(crr, secrets, cms) + + // Override verification: old cert is rejected, but current cert is also rejected (mid-reload) + c.overrideVerifyCertAgainstKASPods = func(_ context.Context, _ string, _ *rest.Config, _ []byte, _ []byte, verifyFunc func(context.Context, kubernetes.Interface) (bool, error)) (bool, error) { + fakeClient := kubefake.NewClientset() + fakeClient.PrependReactor("create", "selfsubjectreviews", func(action k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, apierrors.NewUnauthorized("not authorized") + }) + return verifyFunc(context.Background(), fakeClient) + } + + _, requeue, err := c.processCertificateRevocationRequest(t.Context(), "crr-ns", "crr-name", postRevocationClock.Now) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(requeue).To(BeTrue(), "should requeue when KAS pods reject both old and current cert during mid-reload") + }) + + t.Run("When all KAS pods reject old cert and accept current cert it should mark revoked", func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + kubeconfigData := makeKubeconfig(t) + crr := newRevokedCRR() + secrets := newRevokedSecrets(kubeconfigData) + cms := newRevokedCMs() + c := newRevokedController(crr, secrets, cms) + + // Override verification: old cert is rejected (revoked), current cert is accepted + c.overrideVerifyCertAgainstKASPods = func(_ context.Context, _ string, _ *rest.Config, certPEM, _ []byte, verifyFunc func(context.Context, kubernetes.Interface) (bool, error)) (bool, error) { + fakeClient := kubefake.NewClientset() + if string(certPEM) == string(data.original.raw.signerCert) { + // old cert: revoked (unauthorized) + fakeClient.PrependReactor("create", "selfsubjectreviews", func(action k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, apierrors.NewUnauthorized("not authorized") + }) + } else { + // current cert: trusted + fakeClient.PrependReactor("create", "selfsubjectreviews", func(action k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, nil + }) + } + return verifyFunc(context.Background(), fakeClient) + } + + a, requeue, err := c.processCertificateRevocationRequest(t.Context(), "crr-ns", "crr-name", postRevocationClock.Now) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(requeue).To(BeFalse()) + g.Expect(a).ToNot(BeNil()) + g.Expect(a.crr).ToNot(BeNil()) + // Should have set PreviousCertificatesRevokedType to True + var foundRevoked bool + for _, cond := range a.crr.Status.Conditions { + if cond.Type != nil && *cond.Type == certificatesv1alpha1.PreviousCertificatesRevokedType && + cond.Status != nil && *cond.Status == metav1.ConditionTrue { + foundRevoked = true + } + } + g.Expect(foundRevoked).To(BeTrue(), "should have set PreviousCertificatesRevokedType condition to True") + }) +} diff --git a/support/podspec/containers.go b/support/podspec/containers.go index f0792fe543ee..9670976c99d3 100644 --- a/support/podspec/containers.go +++ b/support/podspec/containers.go @@ -243,3 +243,29 @@ func EnforceRestrictedSecurityContextToContainers(podSpec *corev1.PodSpec) error } return nil } + +// IsPodReady returns true if the pod has a Ready condition with status True. +func IsPodReady(pod *corev1.Pod) bool { + if pod == nil { + return false + } + for _, condition := range pod.Status.Conditions { + if condition.Type == corev1.PodReady { + return condition.Status == corev1.ConditionTrue + } + } + return false +} + +// ContainerPort returns the container port matching the given name, or the default +// port if no matching named port is found. +func ContainerPort(pod *corev1.Pod, portName string, defaultPort int32) int32 { + for i := range pod.Spec.Containers { + for j := range pod.Spec.Containers[i].Ports { + if pod.Spec.Containers[i].Ports[j].Name == portName { + return pod.Spec.Containers[i].Ports[j].ContainerPort + } + } + } + return defaultPort +} diff --git a/support/podspec/containers_test.go b/support/podspec/containers_test.go index 7fb3536b2b4a..90d6163b1208 100644 --- a/support/podspec/containers_test.go +++ b/support/podspec/containers_test.go @@ -124,6 +124,159 @@ func TestFindVolumeMount(t *testing.T) { }) } +func TestIsPodReady(t *testing.T) { + t.Parallel() + for _, testCase := range []struct { + name string + pod *corev1.Pod + expected bool + }{ + { + name: "When pod is nil it should return false", + pod: nil, + expected: false, + }, + { + name: "When pod has Ready=True it should return true", + pod: &corev1.Pod{ + Status: corev1.PodStatus{ + Conditions: []corev1.PodCondition{{ + Type: corev1.PodReady, + Status: corev1.ConditionTrue, + }}, + }, + }, + expected: true, + }, + { + name: "When pod has Ready=False it should return false", + pod: &corev1.Pod{ + Status: corev1.PodStatus{ + Conditions: []corev1.PodCondition{{ + Type: corev1.PodReady, + Status: corev1.ConditionFalse, + }}, + }, + }, + expected: false, + }, + { + name: "When pod has no Ready condition it should return false", + pod: &corev1.Pod{ + Status: corev1.PodStatus{ + Conditions: []corev1.PodCondition{{ + Type: corev1.PodInitialized, + Status: corev1.ConditionTrue, + }}, + }, + }, + expected: false, + }, + { + name: "When pod has no conditions it should return false", + pod: &corev1.Pod{ + Status: corev1.PodStatus{}, + }, + expected: false, + }, + } { + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + g.Expect(IsPodReady(testCase.pod)).To(Equal(testCase.expected)) + }) + } +} + +func TestContainerPort(t *testing.T) { + t.Parallel() + for _, testCase := range []struct { + name string + pod *corev1.Pod + portName string + defaultPort int32 + expectedPort int32 + }{ + { + name: "When named port is found it should return the container port", + pod: &corev1.Pod{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Ports: []corev1.ContainerPort{{ + Name: "client", + ContainerPort: 6443, + }}, + }}, + }, + }, + portName: "client", + defaultPort: 8443, + expectedPort: 6443, + }, + { + name: "When named port is missing it should return the default port", + pod: &corev1.Pod{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Ports: []corev1.ContainerPort{{ + Name: "metrics", + ContainerPort: 9090, + }}, + }}, + }, + }, + portName: "client", + defaultPort: 6443, + expectedPort: 6443, + }, + { + name: "When container has no ports it should return the default port", + pod: &corev1.Pod{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: "kube-apiserver", + }}, + }, + }, + portName: "client", + defaultPort: 6443, + expectedPort: 6443, + }, + { + name: "When port is in a non-first container it should return the container port", + pod: &corev1.Pod{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "sidecar", + Ports: []corev1.ContainerPort{{ + Name: "metrics", + ContainerPort: 9090, + }}, + }, + { + Name: "kube-apiserver", + Ports: []corev1.ContainerPort{{ + Name: "client", + ContainerPort: 7443, + }}, + }, + }, + }, + }, + portName: "client", + defaultPort: 6443, + expectedPort: 7443, + }, + } { + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + g.Expect(ContainerPort(testCase.pod, testCase.portName, testCase.defaultPort)).To(Equal(testCase.expectedPort)) + }) + } +} + func TestEnforceRestrictedSecurityContextToContainers(t *testing.T) { tests := []struct { name string diff --git a/test/e2e/create_cluster_test.go b/test/e2e/create_cluster_test.go index 98eaaf3d894d..81b1124d1e21 100644 --- a/test/e2e/create_cluster_test.go +++ b/test/e2e/create_cluster_test.go @@ -24,7 +24,9 @@ import ( configv1 "github.com/openshift/api/config/v1" operatorv1 "github.com/openshift/api/operator/v1" + appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/client-go/tools/clientcmd" crclient "sigs.k8s.io/controller-runtime/pkg/client" @@ -143,6 +145,66 @@ func TestCreateCluster(t *testing.T) { Execute(&clusterOpts, globalOpts.Platform, globalOpts.ArtifactDir, "create-cluster", globalOpts.ServiceAccountSigningKey) } +// TestCreateClusterHABreakGlassCredentials exercises the break-glass credential flow +// on a HighlyAvailable control plane (3 KAS replicas). This validates that the +// CertificateRevocationController correctly verifies certificate revocation against +// each individual KAS pod rather than through the service load balancer. +func TestCreateClusterHABreakGlassCredentials(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithCancel(testContext) + defer cancel() + + clusterOpts := globalOpts.DefaultClusterOptions(t) + clusterOpts.ControlPlaneAvailabilityPolicy = string(hyperv1.HighlyAvailable) + clusterOpts.NodePoolReplicas = 0 + + e2eutil.NewHypershiftTest(t, ctx, func(t *testing.T, g Gomega, mgtClient crclient.Client, hostedCluster *hyperv1.HostedCluster) { + // Wait for guest API to be reachable (SelfSubjectReview only, no nodes needed) + _ = e2eutil.WaitForGuestClient(t, ctx, mgtClient, hostedCluster) + + // Assert that KAS deployment has 3 ready replicas to guard against false-positive passes + controlPlaneNamespace := manifests.HostedControlPlaneNamespace(hostedCluster.Namespace, hostedCluster.Name) + e2eutil.EventuallyObject(t, ctx, "KAS deployment to have 3 ready replicas", + func(ctx context.Context) (*appsv1.Deployment, error) { + deployment := &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{ + Namespace: controlPlaneNamespace, + Name: "kube-apiserver", + }} + err := mgtClient.Get(ctx, crclient.ObjectKeyFromObject(deployment), deployment) + return deployment, err + }, + []e2eutil.Predicate[*appsv1.Deployment]{ + func(deployment *appsv1.Deployment) (done bool, reasons string, err error) { + ready := deployment.Status.ReadyReplicas + return ready == 3, fmt.Sprintf("expected 3 ready replicas, got %d", ready), nil + }, + }, + ) + + t.Logf("fetching mgmt kubeconfig") + mgmtCfg, err := e2eutil.GetConfig() + g.Expect(err).NotTo(HaveOccurred(), "couldn't get mgmt kubeconfig") + mgmtCfg.QPS = -1 + mgmtCfg.Burst = -1 + + mgmtClients, err := integrationframework.NewClients(mgmtCfg) + g.Expect(err).NotTo(HaveOccurred(), "couldn't create mgmt clients") + + guestKubeConfigSecretData := e2eutil.WaitForGuestKubeConfig(t, ctx, mgtClient, hostedCluster) + + guestConfig, err := clientcmd.RESTConfigFromKubeConfig(guestKubeConfigSecretData) + g.Expect(err).NotTo(HaveOccurred(), "couldn't load guest kubeconfig") + guestConfig.QPS = -1 + guestConfig.Burst = -1 + + guestClients, err := integrationframework.NewClients(guestConfig) + g.Expect(err).NotTo(HaveOccurred(), "couldn't create guest clients") + + integration.RunTestControlPlanePKIOperatorBreakGlassCredentials(t, testContext, hostedCluster, mgmtClients, guestClients) + }).Execute(&clusterOpts, globalOpts.Platform, globalOpts.ArtifactDir, "ha-break-glass-creds", globalOpts.ServiceAccountSigningKey) +} + // TODO(alberto): rename this e2e to drop TestCreateCluster prefix after merging https://github.com/openshift/release/pull/66655 // Without the prefix, this e2e wouldn't run now. func TestCreateClusterDefaultSecurityContextUID(t *testing.T) { diff --git a/test/e2e/util/hypershift_framework.go b/test/e2e/util/hypershift_framework.go index 3b629a0882d8..4781067822c8 100644 --- a/test/e2e/util/hypershift_framework.go +++ b/test/e2e/util/hypershift_framework.go @@ -289,8 +289,7 @@ func (h *hypershiftTest) after(hostedCluster *hyperv1.HostedCluster, platform hy // so skipping until we fix it. // TODO(alberto): consider drop this gate when we fix OCPBUGS-61291. if hostedCluster.Spec.Platform.Type != hyperv1.NonePlatform { - // Private clusters may won't be reachable from the test runner; assume workers exist. - hasWorkerNodes := true + hasWorkerNodes := false if !netutil.IsPrivateHC(hostedCluster) { guestClient := WaitForGuestClient(t, t.Context(), h.client, hostedCluster) var nodeList corev1.NodeList @@ -298,6 +297,24 @@ func (h *hypershiftTest) after(hostedCluster *hyperv1.HostedCluster, platform hy t.Errorf("failed to list nodes in guest cluster: %v", err) } hasWorkerNodes = len(nodeList.Items) > 0 + } else { + // Private clusters are not reachable from the test runner; + // determine worker node expectation from NodePool replicas. + var nodePoolList hyperv1.NodePoolList + if err := h.client.List(t.Context(), &nodePoolList, crclient.InNamespace(hostedCluster.Namespace)); err != nil { + t.Errorf("failed to list NodePools: %v", err) + } + for i := range nodePoolList.Items { + np := &nodePoolList.Items[i] + if np.Spec.Replicas != nil && *np.Spec.Replicas > 0 { + hasWorkerNodes = true + break + } + if np.Spec.AutoScaling != nil && np.Spec.AutoScaling.Max > 0 { + hasWorkerNodes = true + break + } + } } ValidateHostedClusterConditions(t, t.Context(), h.client, hostedCluster, hasWorkerNodes, 10*time.Minute, h.upgradeContext) } diff --git a/test/integration/control_plane_pki_operator.go b/test/integration/control_plane_pki_operator.go index 4602f0dc954b..2d8fad06be15 100644 --- a/test/integration/control_plane_pki_operator.go +++ b/test/integration/control_plane_pki_operator.go @@ -279,12 +279,32 @@ func validateRevocation(t *testing.T, ctx context.Context, hostedCluster *hypers }, ) - t.Logf("creating a client using the a certificate from the revoked signer") - previousCertClient := clientForCertKey(t, guest.Cfg, signedCrt, key) - - t.Log("issuing SSR to confirm that we're not authorized to contact the server") - response, err := previousCertClient.AuthenticationV1().SelfSubjectReviews().Create(ctx, &authenticationv1.SelfSubjectReview{}, metav1.CreateOptions{}) - if !apierrors.IsUnauthorized(err) { - t.Fatalf("expected an unauthorized error, got %v, response %#v", err, response) + // Poll the SSR through the service LB until we get Unauthorized. + // The controller already verified per-pod that all KAS instances rejected + // the old cert before setting PreviousCertificatesRevokedType=True, and the + // old signer CA has been pruned from the trust bundle ConfigMap. However, a + // KAS pod may restart between the controller's check and this test, causing + // the LB to transiently route to a pod that hasn't finished loading the + // trust bundle yet. Polling handles this without duplicating the controller's + // per-pod logic. + // + // We set a per-request timeout on the client because KAS may stall the TLS + // handshake (rather than cleanly rejecting) while reloading its trust bundle + // after revocation. Without a request timeout the HTTP call blocks + // indefinitely and the poll timeout cannot interrupt it. + t.Log("polling SSR to confirm that the revoked certificate is rejected") + cfgWithTimeout := rest.CopyConfig(guest.Cfg) + cfgWithTimeout.Timeout = 10 * time.Second + previousCertClient := clientForCertKey(t, cfgWithTimeout, signedCrt, key) + if err := wait.PollUntilContextTimeout(ctx, 1*time.Second, 3*time.Minute, true, func(ctx context.Context) (done bool, err error) { + t.Log("attempting SSR with revoked cert") + _, err = previousCertClient.AuthenticationV1().SelfSubjectReviews().Create(ctx, &authenticationv1.SelfSubjectReview{}, metav1.CreateOptions{}) + if apierrors.IsUnauthorized(err) { + return true, nil + } + t.Logf("SSR not yet unauthorized (err=%v), retrying", err) + return false, nil + }); err != nil { + t.Fatalf("revoked certificate was not rejected within timeout: %v", err) } }