From c3331917f5daaca9c10d11b528300910271239db Mon Sep 17 00:00:00 2001 From: tithakka Date: Thu, 3 Sep 2026 18:18:56 -0500 Subject: [PATCH] HYPERFLEET-1409 - feat: report status conditions on HyperFleetConfig --- api/v1alpha1/hyperfleetconfig_types.go | 54 ++++- ...perfleet.redhat.com_hyperfleetconfigs.yaml | 11 +- docs/status-conditions.md | 72 ++++++ internal/bundle/bundle.go | 9 +- internal/component/api/api.go | 121 +++++++++- internal/component/api/api_test.go | 135 ++++++++++++ .../controller/hyperfleetconfig_controller.go | 62 +++++- .../hyperfleetconfig_controller_test.go | 201 +++++++++++++++++ .../controller/hyperfleetconfig_status.go | 155 +++++++++++++ .../hyperfleetconfig_status_test.go | 207 ++++++++++++++++++ 10 files changed, 997 insertions(+), 30 deletions(-) create mode 100644 docs/status-conditions.md create mode 100644 internal/controller/hyperfleetconfig_status.go create mode 100644 internal/controller/hyperfleetconfig_status_test.go diff --git a/api/v1alpha1/hyperfleetconfig_types.go b/api/v1alpha1/hyperfleetconfig_types.go index 6074d89..441f3fc 100644 --- a/api/v1alpha1/hyperfleetconfig_types.go +++ b/api/v1alpha1/hyperfleetconfig_types.go @@ -84,8 +84,7 @@ var AllSizingProfiles = []SizingProfile{SizingProfileSmall, SizingProfileMedium, // operator-layer vocabulary describing installation health, and is distinct from // the HyperFleet API's own resource-condition vocabulary (Available/Ready/ // Reconciled/LastKnownReconciled/per-adapter; see architecture ADR-0007 and -// ADR-0008). The bundle controller (HYPERFLEET-1409) populates these; this story -// defines the schema only. +// ADR-0008). Populated by the bundle controller as of HYPERFLEET-1409. const ( // ConditionAvailable is True when the installed operand (the API) is // deployed and healthy. @@ -98,16 +97,50 @@ const ( ConditionDegraded = "Degraded" ) +// Reason strings for the operator-layer conditions above (HYPERFLEET-1409). +// These are published API vocabulary — partners may read status.conditions[].reason +// — so every writer of a condition must use one of these constants rather than an +// ad hoc string, and the set must stay documented in docs/status-conditions.md. +const ( + // ReasonDeploymentAvailable: Available=True — the operand Deployment reports + // Available and all desired replicas are ready. + ReasonDeploymentAvailable = "DeploymentAvailable" + // ReasonDeploymentUnavailable: Available=False — the operand Deployment is + // missing or has zero available replicas. + ReasonDeploymentUnavailable = "DeploymentUnavailable" + // ReasonDeploymentNotReady: Available=False — the operand Deployment exists + // with some, but not all, replicas ready. + ReasonDeploymentNotReady = "DeploymentNotReady" + // ReasonRolloutInProgress: Progressing=True — the operand Deployment has not + // finished rolling out its current generation. + ReasonRolloutInProgress = "RolloutInProgress" + // ReasonRolloutComplete: Progressing=False — the operand Deployment is fully + // rolled out and stable. + ReasonRolloutComplete = "RolloutComplete" + // ReasonAsExpected: Degraded=False — no failure signal (ClusterOperator + // convention default). + ReasonAsExpected = "AsExpected" + // ReasonReferencedSecretMissing: Degraded=True — a Secret referenced by the + // spec (database, TLS, or JWKS) does not exist in the operator's namespace. + ReasonReferencedSecretMissing = "ReferencedSecretMissing" + // ReasonReconcileError: Degraded=True — a component failed to render or + // apply, or JWKS discovery failed with no cached fallback available, during + // the most recent reconcile. + ReasonReconcileError = "ReconcileError" +) + // SecretReference references a Secret by name. Referenced Secrets must live in // the operator's own namespace: because HyperFleetConfig is cluster-scoped, no // namespace field is exposed (name-only + operator-namespace convention, decided // in the HYPERFLEET-1406 API review). // -// TODO(HYPERFLEET-1512): the operator-namespace constraint is convention-only -// today — the schema cannot enforce it (CEL sees no cross-object/namespace -// state; ADR-0019 rules out webhooks). The reconciler must enforce it (resolve -// the Secret in the operator's own namespace and surface a Degraded condition -// when it is missing) once it lands. +// The operator-namespace constraint is convention-only at the schema level — +// CEL sees no cross-object/namespace state, and ADR-0019 rules out webhooks — +// but the reconciler does resolve every reference in its own namespace +// (referencedSecretData) and surfaces ConditionDegraded / +// ReasonReferencedSecretMissing when one is absent (HYPERFLEET-1409). +// TODO(HYPERFLEET-1512): decide whether any further enforcement (e.g. rejecting +// reconciliation outright) belongs here. type SecretReference struct { // name is the name of the Secret in the operator's namespace. It must be a // valid DNS-1123 subdomain, matching what k8s.io/apimachinery/pkg/util/validation @@ -255,8 +288,9 @@ type HyperFleetConfigSpec struct { } // HyperFleetConfigStatus defines the observed state of HyperFleetConfig. It is -// populated by the bundle controller in later stories; this story defines the -// schema only. +// populated by the bundle controller after every reconcile (HYPERFLEET-1409), +// rolling up each component's health into the Available/Progressing/Degraded +// conditions. type HyperFleetConfigStatus struct { // observedGeneration is the .metadata.generation the operator last acted on. // @@ -280,6 +314,8 @@ type HyperFleetConfigStatus struct { // +kubebuilder:printcolumn:name="Bundle",type=string,JSONPath=`.spec.bundle` // +kubebuilder:printcolumn:name="Profile",type=string,JSONPath=`.spec.api.profile` // +kubebuilder:printcolumn:name="Available",type=string,JSONPath=`.status.conditions[?(@.type=="Available")].status` +// +kubebuilder:printcolumn:name="Progressing",type=string,JSONPath=`.status.conditions[?(@.type=="Progressing")].status` +// +kubebuilder:printcolumn:name="Degraded",type=string,JSONPath=`.status.conditions[?(@.type=="Degraded")].status` // +kubebuilder:printcolumn:name="Age",type=date,JSONPath=`.metadata.creationTimestamp` // HyperFleetConfig is the Schema for the hyperfleetconfigs API. It is a diff --git a/config/crd/bases/hyperfleet.redhat.com_hyperfleetconfigs.yaml b/config/crd/bases/hyperfleet.redhat.com_hyperfleetconfigs.yaml index 4fa5164..98a2acd 100644 --- a/config/crd/bases/hyperfleet.redhat.com_hyperfleetconfigs.yaml +++ b/config/crd/bases/hyperfleet.redhat.com_hyperfleetconfigs.yaml @@ -26,6 +26,12 @@ spec: - jsonPath: .status.conditions[?(@.type=="Available")].status name: Available type: string + - jsonPath: .status.conditions[?(@.type=="Progressing")].status + name: Progressing + type: string + - jsonPath: .status.conditions[?(@.type=="Degraded")].status + name: Degraded + type: string - jsonPath: .metadata.creationTimestamp name: Age type: date @@ -210,8 +216,9 @@ spec: status: description: |- HyperFleetConfigStatus defines the observed state of HyperFleetConfig. It is - populated by the bundle controller in later stories; this story defines the - schema only. + populated by the bundle controller after every reconcile (HYPERFLEET-1409), + rolling up each component's health into the Available/Progressing/Degraded + conditions. properties: conditions: description: |- diff --git a/docs/status-conditions.md b/docs/status-conditions.md new file mode 100644 index 0000000..8e2e980 --- /dev/null +++ b/docs/status-conditions.md @@ -0,0 +1,72 @@ +# Status conditions + +`HyperFleetConfig` reports its installation health via `status.conditions`. This +is deliberately a *different* vocabulary from the HyperFleet API's own +condition dialect — the two are separate layers, and both are documented here +so it's clear which one you're looking at. + +## Two layers + +- **Operator layer** (this document): `status.conditions` on the + `HyperFleetConfig` CR itself. It describes whether the operator has + successfully installed and is maintaining a healthy deployment of the + operand(s) selected by `spec.bundle` — i.e. "is the operator doing its job." + This follows the OpenShift `ClusterOperator` convention (`Available`, + `Progressing`, `Degraded`), minus `Upgradeable` (see architecture ADR-0019). +- **API layer**: conditions reported by the HyperFleet API's own resources + (`Available`, `Ready`, `Reconciled`, `LastKnownReconciled`, per-adapter + `Successful`, etc. — see architecture ADR-0007 and ADR-0008). These describe + whether the API is successfully reconciling partner-managed resources + (clusters, node pools, etc.) — i.e. "is the API doing its job." They are + unrelated to and unaffected by the operator-layer conditions here. + +A `HyperFleetConfig` can be `Available=True` (the API Deployment is healthy) +while individual API-layer resources are failing to reconcile, and vice versa: +the operator conditions say nothing about the health of resources managed +through the API. + +## Operator-layer condition types + +| Type | Meaning | +|---|---| +| `Available` | The operand (the API) is deployed and healthy. | +| `Progressing` | The operator is actively rolling out a change to the operand. | +| `Degraded` | The operator cannot reach or maintain the desired state. | + +Each condition's `observedGeneration` (and the top-level +`status.observedGeneration`) reflects the `.metadata.generation` the operator +had processed when the condition was last evaluated. `lastTransitionTime` +changes only when a condition's `status` actually flips — reconciles that +leave health unchanged do not touch it. + +If a reconcile fails before any component's health could actually be checked +(e.g. a failed OIDC discovery, or a failed apply on the first operand), +`Available` and `Progressing` are left exactly as they were last recorded +rather than being guessed — only `Degraded` and `observedGeneration` are +updated. This avoids publishing a fabricated "healthy" value alongside +`Degraded=True` when the real state was never actually observed. + +## Reason strings + +Every condition write uses one of the following reasons — no ad hoc strings. +Partners may read `status.conditions[].reason` as part of the published +contract, so this table is the source of truth; the Go constants live +alongside the condition types in `api/v1alpha1/hyperfleetconfig_types.go`. + +| Condition | Status | Reason | Meaning | +|---|---|---|---| +| Available | True | `DeploymentAvailable` | The operand Deployment reports Available; all desired replicas are ready. | +| Available | False | `DeploymentUnavailable` | The operand Deployment is missing, or has zero available replicas. | +| Available | False | `DeploymentNotReady` | The operand Deployment exists with some, but not all, replicas ready. | +| Progressing | True | `RolloutInProgress` | The operand Deployment has not finished rolling out its current generation. | +| Progressing | False | `RolloutComplete` | The operand Deployment is fully rolled out and stable. | +| Degraded | False | `AsExpected` | No failure detected (the ClusterOperator convention's default). | +| Degraded | True | `ReferencedSecretMissing` | A Secret referenced by `spec.api` (database, TLS, or JWKS) does not exist in the operator's namespace. | +| Degraded | True | `ReconcileError` | Any other error during the most recent reconcile — JWKS discovery, reading referenced Secrets, resolving bundle components, or a component's render/apply. | + +## Notes on `Available` + +The operand's readiness probe (`/readyz`) only succeeds once the API has +established a working database connection, so `Available=True` also implies +the API can reach its configured PostgreSQL database — not just that the pod +is running (`/healthz`, the liveness probe, does not check this). diff --git a/internal/bundle/bundle.go b/internal/bundle/bundle.go index 84505dd..37ab02a 100644 --- a/internal/bundle/bundle.go +++ b/internal/bundle/bundle.go @@ -37,12 +37,15 @@ import ( // // - Render is a pure function (CR → desired objects); it must not read or write // the cluster. The controller applies what it returns. -// - Conditions reports component health. It is consumed starting in -// HYPERFLEET-1409; until then the controller does not roll it into status. +// - Conditions is also cluster-access-free: it derives health from applied, the +// objects Render produced, already updated in place by apply.Objects with the +// server's current status (see internal/apply). This avoids giving every +// component its own client.Client and an extra round of API reads for state +// the controller already has. Consumed starting in HYPERFLEET-1409. type Component interface { Name() string Render(ctx context.Context, cr *hyperfleetv1alpha1.HyperFleetConfig) ([]client.Object, error) - Conditions(ctx context.Context, cr *hyperfleetv1alpha1.HyperFleetConfig) ([]metav1.Condition, error) + Conditions(ctx context.Context, cr *hyperfleetv1alpha1.HyperFleetConfig, applied []client.Object) ([]metav1.Condition, error) } // Config carries the inputs the resolver needs to construct components. diff --git a/internal/component/api/api.go b/internal/component/api/api.go index 52981af..e0bbaf9 100644 --- a/internal/component/api/api.go +++ b/internal/component/api/api.go @@ -20,6 +20,7 @@ import ( "context" "fmt" + appsv1 "k8s.io/api/apps/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" @@ -161,11 +162,117 @@ func (c *Component) resolveJWKSource(cr *hyperfleetv1alpha1.HyperFleetConfig) (u return c.ResolvedJWKSURL, "" } -// Conditions reports the component's health as metav1.Conditions. The contract is -// defined now (HYPERFLEET-1407) so it need not be reopened next story, but its -// output is not yet rolled up into status.conditions — real health derivation and -// status wiring land in HYPERFLEET-1409. Returning nil until then keeps the -// reconciler from writing status prematurely. -func (c *Component) Conditions(_ context.Context, _ *hyperfleetv1alpha1.HyperFleetConfig) ([]metav1.Condition, error) { - return nil, nil +// Conditions reports the component's health as metav1.Conditions, derived from +// the live state of the Deployment Render produced (already updated in place by +// apply.Objects with the server's current status — see bundle.Component). It +// never reads the cluster itself. +// +// Available reflects whether the Deployment is up: True only once all desired +// replicas are ready, distinguishing "not present at all" (DeploymentUnavailable) +// from "up but partially ready" (DeploymentNotReady). Progressing is derived from +// replica-count/generation lag rather than trusting the Deployment's own +// "Progressing" condition Reason verbatim, since that built-in condition stays +// True/NewReplicaSetAvailable even at steady state, which is not what +// HyperFleetConfig's Progressing means. +func (c *Component) Conditions(_ context.Context, _ *hyperfleetv1alpha1.HyperFleetConfig, applied []client.Object) ([]metav1.Condition, error) { + dep := findDeployment(applied) + return []metav1.Condition{ + availableCondition(dep), + progressingCondition(dep), + }, nil +} + +// findDeployment returns the API Deployment from applied, or nil if absent. +func findDeployment(applied []client.Object) *appsv1.Deployment { + for _, o := range applied { + if dep, ok := o.(*appsv1.Deployment); ok && dep.Name == ResourceName { + return dep + } + } + return nil +} + +// desiredReplicas returns dep.Spec.Replicas, defaulting to 1 to match +// render.go's current fixed baseline (Replicas is not yet configurable). +func desiredReplicas(dep *appsv1.Deployment) int32 { + if dep.Spec.Replicas != nil { + return *dep.Spec.Replicas + } + return 1 +} + +// availableCondition derives Available from the Deployment's replica counts. +func availableCondition(dep *appsv1.Deployment) metav1.Condition { + if dep == nil { + return metav1.Condition{ + Type: hyperfleetv1alpha1.ConditionAvailable, + Status: metav1.ConditionFalse, + Reason: hyperfleetv1alpha1.ReasonDeploymentUnavailable, + Message: "the API Deployment does not exist", + } + } + + desired := desiredReplicas(dep) + + switch { + case desired == 0: + return metav1.Condition{ + Type: hyperfleetv1alpha1.ConditionAvailable, + Status: metav1.ConditionTrue, + Reason: hyperfleetv1alpha1.ReasonDeploymentAvailable, + Message: "the API Deployment is scaled to zero", + } + case dep.Status.AvailableReplicas == 0: + return metav1.Condition{ + Type: hyperfleetv1alpha1.ConditionAvailable, + Status: metav1.ConditionFalse, + Reason: hyperfleetv1alpha1.ReasonDeploymentUnavailable, + Message: "the API Deployment has no available replicas", + } + case dep.Status.AvailableReplicas < desired: + return metav1.Condition{ + Type: hyperfleetv1alpha1.ConditionAvailable, + Status: metav1.ConditionFalse, + Reason: hyperfleetv1alpha1.ReasonDeploymentNotReady, + Message: fmt.Sprintf("the API Deployment has %d/%d replicas available", dep.Status.AvailableReplicas, desired), + } + default: + return metav1.Condition{ + Type: hyperfleetv1alpha1.ConditionAvailable, + Status: metav1.ConditionTrue, + Reason: hyperfleetv1alpha1.ReasonDeploymentAvailable, + Message: "the API Deployment is available", + } + } +} + +// progressingCondition derives Progressing from replica-count and generation +// lag: the Deployment has not caught up with its most recently observed +// generation, or not all replicas have been updated to the current template. +func progressingCondition(dep *appsv1.Deployment) metav1.Condition { + if dep == nil { + return metav1.Condition{ + Type: hyperfleetv1alpha1.ConditionProgressing, + Status: metav1.ConditionTrue, + Reason: hyperfleetv1alpha1.ReasonRolloutInProgress, + Message: "the API Deployment does not exist yet", + } + } + + desired := desiredReplicas(dep) + + if dep.Status.ObservedGeneration < dep.Generation || dep.Status.UpdatedReplicas < desired { + return metav1.Condition{ + Type: hyperfleetv1alpha1.ConditionProgressing, + Status: metav1.ConditionTrue, + Reason: hyperfleetv1alpha1.ReasonRolloutInProgress, + Message: "the API Deployment rollout has not completed", + } + } + return metav1.Condition{ + Type: hyperfleetv1alpha1.ConditionProgressing, + Status: metav1.ConditionFalse, + Reason: hyperfleetv1alpha1.ReasonRolloutComplete, + Message: "the API Deployment rollout is complete", + } } diff --git a/internal/component/api/api_test.go b/internal/component/api/api_test.go index 3c10a3d..4fa1d64 100644 --- a/internal/component/api/api_test.go +++ b/internal/component/api/api_test.go @@ -346,6 +346,141 @@ func TestRenderRoleHasNoRules(t *testing.T) { g.Expect(rb.Subjects[0].Namespace).To(Equal(testNamespace)) } +// deploymentWithStatus returns a Deployment named ResourceName with the given +// spec.replicas and status fields set, for feeding directly into Conditions +// without any cluster/client dependency. ObservedGeneration matches generation +// (the Deployment controller has caught up); the mid-rollout test overrides it +// after construction to simulate a lag. +func deploymentWithStatus(replicas, availableReplicas, updatedReplicas int32, generation int64) *appsv1.Deployment { + return &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: ResourceName, Generation: generation}, + Spec: appsv1.DeploymentSpec{Replicas: ptr.To(replicas)}, + Status: appsv1.DeploymentStatus{ + AvailableReplicas: availableReplicas, + UpdatedReplicas: updatedReplicas, + ObservedGeneration: generation, + }, + } +} + +func condMapByType(conds []metav1.Condition) map[string]metav1.Condition { + out := make(map[string]metav1.Condition, len(conds)) + for _, c := range conds { + out[c.Type] = c + } + return out +} + +func TestConditionsNoDeployment(t *testing.T) { + g := NewWithT(t) + + conds, err := New("img", testNamespace, Options{}).Conditions(context.Background(), testCR(), nil) + g.Expect(err).NotTo(HaveOccurred()) + + byType := condMapByType(conds) + g.Expect(byType[hyperfleetv1alpha1.ConditionAvailable].Status).To(Equal(metav1.ConditionFalse)) + g.Expect(byType[hyperfleetv1alpha1.ConditionAvailable].Reason).To(Equal(hyperfleetv1alpha1.ReasonDeploymentUnavailable)) + g.Expect(byType[hyperfleetv1alpha1.ConditionProgressing].Status).To(Equal(metav1.ConditionTrue)) + g.Expect(byType[hyperfleetv1alpha1.ConditionProgressing].Reason).To(Equal(hyperfleetv1alpha1.ReasonRolloutInProgress)) +} + +func TestConditionsZeroAvailableReplicas(t *testing.T) { + g := NewWithT(t) + + dep := deploymentWithStatus(1, 0, 0, 1) + conds, err := New("img", testNamespace, Options{}).Conditions(context.Background(), testCR(), []client.Object{dep}) + g.Expect(err).NotTo(HaveOccurred()) + + byType := condMapByType(conds) + g.Expect(byType[hyperfleetv1alpha1.ConditionAvailable].Status).To(Equal(metav1.ConditionFalse)) + g.Expect(byType[hyperfleetv1alpha1.ConditionAvailable].Reason).To(Equal(hyperfleetv1alpha1.ReasonDeploymentUnavailable)) +} + +func TestConditionsPartiallyReady(t *testing.T) { + g := NewWithT(t) + + // 2 desired, only 1 available: not fully Available, and still Progressing + // since not all replicas are updated. + dep := deploymentWithStatus(2, 1, 1, 1) + conds, err := New("img", testNamespace, Options{}).Conditions(context.Background(), testCR(), []client.Object{dep}) + g.Expect(err).NotTo(HaveOccurred()) + + byType := condMapByType(conds) + g.Expect(byType[hyperfleetv1alpha1.ConditionAvailable].Status).To(Equal(metav1.ConditionFalse)) + g.Expect(byType[hyperfleetv1alpha1.ConditionAvailable].Reason).To(Equal(hyperfleetv1alpha1.ReasonDeploymentNotReady)) + g.Expect(byType[hyperfleetv1alpha1.ConditionProgressing].Status).To(Equal(metav1.ConditionTrue)) +} + +func TestConditionsFullyReadyAndStable(t *testing.T) { + g := NewWithT(t) + + dep := deploymentWithStatus(1, 1, 1, 1) + conds, err := New("img", testNamespace, Options{}).Conditions(context.Background(), testCR(), []client.Object{dep}) + g.Expect(err).NotTo(HaveOccurred()) + + byType := condMapByType(conds) + g.Expect(byType[hyperfleetv1alpha1.ConditionAvailable].Status).To(Equal(metav1.ConditionTrue)) + g.Expect(byType[hyperfleetv1alpha1.ConditionAvailable].Reason).To(Equal(hyperfleetv1alpha1.ReasonDeploymentAvailable)) + g.Expect(byType[hyperfleetv1alpha1.ConditionProgressing].Status).To(Equal(metav1.ConditionFalse)) + g.Expect(byType[hyperfleetv1alpha1.ConditionProgressing].Reason).To(Equal(hyperfleetv1alpha1.ReasonRolloutComplete)) +} + +func TestConditionsMidRollout(t *testing.T) { + g := NewWithT(t) + + // Fully available at the old generation, but a new generation has not yet + // been observed by the Deployment controller: still Progressing. + dep := deploymentWithStatus(1, 1, 1, 2) + dep.Status.ObservedGeneration = 1 + conds, err := New("img", testNamespace, Options{}).Conditions(context.Background(), testCR(), []client.Object{dep}) + g.Expect(err).NotTo(HaveOccurred()) + + byType := condMapByType(conds) + g.Expect(byType[hyperfleetv1alpha1.ConditionProgressing].Status).To(Equal(metav1.ConditionTrue)) + g.Expect(byType[hyperfleetv1alpha1.ConditionProgressing].Reason).To(Equal(hyperfleetv1alpha1.ReasonRolloutInProgress)) +} + +func TestConditionsFindsDeploymentInMixedKindSlice(t *testing.T) { + g := NewWithT(t) + + // applied carries every rendered operand kind, in Render's actual order — + // proves findDeployment picks the Deployment out by kind+name rather than + // assuming it is the only or first element. + dep := deploymentWithStatus(1, 1, 1, 1) + applied := []client.Object{ + &corev1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{Name: ResourceName}}, + &rbacv1.Role{ObjectMeta: metav1.ObjectMeta{Name: ResourceName}}, + &rbacv1.RoleBinding{ObjectMeta: metav1.ObjectMeta{Name: ResourceName}}, + &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: ConfigMapName}}, + &corev1.Service{ObjectMeta: metav1.ObjectMeta{Name: ResourceName}}, + dep, + } + + conds, err := New("img", testNamespace, Options{}).Conditions(context.Background(), testCR(), applied) + g.Expect(err).NotTo(HaveOccurred()) + + byType := condMapByType(conds) + g.Expect(byType[hyperfleetv1alpha1.ConditionAvailable].Status).To(Equal(metav1.ConditionTrue)) + g.Expect(byType[hyperfleetv1alpha1.ConditionAvailable].Reason).To(Equal(hyperfleetv1alpha1.ReasonDeploymentAvailable)) +} + +func TestConditionsNilSpecReplicasDefaultsToOne(t *testing.T) { + g := NewWithT(t) + + // Spec.Replicas == nil (never set) must fall back to the same default of 1 + // that render.go's deployment() currently hardcodes, not a zero-value 0 + // (which would make every replica count vacuously "desired"). + dep := deploymentWithStatus(1, 1, 1, 1) + dep.Spec.Replicas = nil + + conds, err := New("img", testNamespace, Options{}).Conditions(context.Background(), testCR(), []client.Object{dep}) + g.Expect(err).NotTo(HaveOccurred()) + + byType := condMapByType(conds) + g.Expect(byType[hyperfleetv1alpha1.ConditionAvailable].Status).To(Equal(metav1.ConditionTrue)) + g.Expect(byType[hyperfleetv1alpha1.ConditionProgressing].Status).To(Equal(metav1.ConditionFalse)) +} + func TestRenderService(t *testing.T) { g := NewWithT(t) diff --git a/internal/controller/hyperfleetconfig_controller.go b/internal/controller/hyperfleetconfig_controller.go index ece5d87..ddd6a50 100644 --- a/internal/controller/hyperfleetconfig_controller.go +++ b/internal/controller/hyperfleetconfig_controller.go @@ -18,6 +18,7 @@ package controller import ( "context" + "errors" "fmt" "net/http" "sync" @@ -25,6 +26,7 @@ import ( appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" @@ -106,16 +108,41 @@ type HyperFleetConfigReconciler struct { // // For more details, check Reconcile and its Result here: // - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.21.0/pkg/reconcile -func (r *HyperFleetConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { +func (r *HyperFleetConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) (result ctrl.Result, reterr error) { log := logf.FromContext(ctx) cr := &hyperfleetv1alpha1.HyperFleetConfig{} if err := r.Get(ctx, req.NamespacedName, cr); err != nil { // The CR is gone: its operands carry controller owner references, so the - // built-in garbage collector removes them. No finalizer is required. + // built-in garbage collector removes them. No finalizer is required. There + // is also nothing to write status to, so this path bypasses the deferred + // status patch below entirely. return ctrl.Result{}, client.IgnoreNotFound(err) } + // componentConditions, componentsCollected and missingSecrets feed + // aggregateStatus in the deferred status patch below. They are populated as + // reconciliation proceeds so that a mid-loop failure (reterr set, named + // return) still status-patches with whatever was learned before the failure + // — e.g. a missing Secret discovered before a later component's apply fails + // still surfaces as Degraded. componentsCollected is set true only once the + // per-component loop below completes without error; aggregateStatus uses it + // to avoid publishing a fabricated Available/Progressing value when the + // real state was never actually checked this reconcile. + var componentConditions []metav1.Condition + var componentsCollected bool + var missingSecrets []string + defer func() { + if reterr != nil { + log.Error(reterr, "reconcile failed") + } + if aggregateStatus(cr, componentConditions, componentsCollected, missingSecrets, reterr) { + if err := r.Status().Update(ctx, cr); err != nil { + reterr = errors.Join(reterr, fmt.Errorf("update status: %w", err)) + } + } + }() + // Resolve the JWKS URL. When auth is on and the CR pins neither a JWKS URL nor // a JWKS Secret, this performs OIDC discovery — a network read, so it lives // here rather than in the pure renderer. Empty otherwise. @@ -127,13 +154,18 @@ func (r *HyperFleetConfigReconciler) Reconcile(ctx context.Context, req ctrl.Req // Read each referenced Secret's resourceVersion for the rollout hash. A // rotation bumps resourceVersion without changing the rendered config.yaml or // the pod spec, so without hashing it the pods would keep running with stale - // credentials/certs. Missing Secrets are not fatal here (validation and the - // Degraded condition are HYPERFLEET-1512); they are hashed as absent so the - // pods roll once the Secret appears. + // credentials/certs. A missing Secret is not fatal here — it is hashed as + // absent so the pods roll once the Secret appears — but it does drive the + // Degraded condition via missingSecrets below. secretData, err := r.referencedSecretData(ctx, cr) if err != nil { return ctrl.Result{}, fmt.Errorf("read referenced secrets: %w", err) } + for _, e := range secretData { + if !e.present { + missingSecrets = append(missingSecrets, e.id) + } + } components, err := bundle.Resolve(cr.Spec.Bundle, bundle.Config{ APIImage: r.APIImage, @@ -144,6 +176,10 @@ func (r *HyperFleetConfigReconciler) Reconcile(ctx context.Context, req ctrl.Req return ctrl.Result{}, fmt.Errorf("resolve components: %w", err) } + // Preallocated to the largest plausible size: each component contributes + // exactly one Available and one Progressing condition today. + componentConditions = make([]metav1.Condition, 0, len(components)*2) + for _, component := range components { objs, err := component.Render(ctx, cr) if err != nil { @@ -156,13 +192,21 @@ func (r *HyperFleetConfigReconciler) Reconcile(ctx context.Context, req ctrl.Req if err := apply.Objects(ctx, r.Client, cr, r.Scheme, objs); err != nil { return ctrl.Result{}, fmt.Errorf("apply component %q: %w", component.Name(), err) } + // apply.Objects patches each object in place via server-side apply, so objs + // now carries whatever status the API server currently has for it (e.g. the + // Deployment controller's last-written replica counts) — Conditions reads + // that back rather than performing its own Get. + conds, err := component.Conditions(ctx, cr, objs) + if err != nil { + return ctrl.Result{}, fmt.Errorf("get conditions for component %q: %w", component.Name(), err) + } + componentConditions = append(componentConditions, conds...) } + componentsCollected = true - // TODO(HYPERFLEET-1409): roll each component's Conditions up into - // status.conditions and set status.observedGeneration. // TODO(HYPERFLEET-1512): enforce that referenced Secrets exist in - // r.OperatorNamespace and surface a Degraded condition when one is missing - // (today a missing Secret is tolerated and merely hashed as absent). + // r.OperatorNamespace beyond driving the Degraded condition above — e.g. + // rejecting or fast-failing before Render/Apply run at all. log.Info("reconciled HyperFleetConfig", "bundle", cr.Spec.Bundle, "components", len(components), "namespace", r.OperatorNamespace) diff --git a/internal/controller/hyperfleetconfig_controller_test.go b/internal/controller/hyperfleetconfig_controller_test.go index da294a9..5ce8fab 100644 --- a/internal/controller/hyperfleetconfig_controller_test.go +++ b/internal/controller/hyperfleetconfig_controller_test.go @@ -18,6 +18,8 @@ package controller import ( "context" + "net/http" + "net/http/httptest" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -252,6 +254,205 @@ var _ = Describe("HyperFleetConfig Controller", func() { Expect(err).NotTo(HaveOccurred()) }) + // Status-condition transition matrix (HYPERFLEET-1409). envtest is apiserver + + // etcd only — there is no Deployment controller, so nothing ever auto-populates + // a Deployment's .status. Each scenario below patches the Deployment's status + // subresource directly to simulate what the real Deployment controller would + // have written, then reconciles again and asserts the CR's rolled-up status. + + // conditionByType is defined package-level in hyperfleetconfig_status_test.go. + + // getCR re-fetches the singleton's current state. + getCR := func() *hyperfleetv1alpha1.HyperFleetConfig { + GinkgoHelper() + cr := &hyperfleetv1alpha1.HyperFleetConfig{} + Expect(k8sClient.Get(ctx, typeNamespacedName, cr)).To(Succeed()) + return cr + } + + // patchDeploymentStatus fetches the API Deployment and overwrites its status + // subresource, simulating what the built-in Deployment controller would write. + patchDeploymentStatus := func(mutate func(*appsv1.Deployment)) { + GinkgoHelper() + dep := &appsv1.Deployment{} + Expect(k8sClient.Get(ctx, operandKey(apicomponent.ResourceName), dep)).To(Succeed()) + mutate(dep) + Expect(k8sClient.Status().Update(ctx, dep)).To(Succeed()) + } + + // createReferencedSecrets creates every Secret the fixture CR (validHyperFleetConfig) + // references — the database Secret and the pinned JWKS Secret — so scenarios + // about Deployment/rollout health are not incidentally also Degraded on a + // missing Secret (that is its own dedicated scenario below). + createReferencedSecrets := func() { + GinkgoHelper() + dbSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: testDBSecretName, Namespace: operatorNamespace}, + Data: map[string][]byte{ + apicomponent.SecretKeyDBHost: []byte("db.example.com"), + apicomponent.SecretKeyDBPort: []byte("5432"), + apicomponent.SecretKeyDBName: []byte("hyperfleet"), + apicomponent.SecretKeyDBUser: []byte("hyperfleet"), + apicomponent.SecretKeyDBPassword: []byte("password"), + }, + } + jwksSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: testJWKSSecretName, Namespace: operatorNamespace}, + Data: map[string][]byte{apicomponent.SecretKeyJWKS: []byte(`{"keys":[]}`)}, + } + for _, s := range []*corev1.Secret{dbSecret, jwksSecret} { + Expect(k8sClient.Create(ctx, s)).To(Succeed()) + DeferCleanup(func(ctx context.Context, obj client.Object) { + Expect(client.IgnoreNotFound(k8sClient.Delete(ctx, obj))).To(Succeed()) + }, ctx, s) + } + } + + It("reports Available=False and Progressing=True on install, before the Deployment is ready", func() { + By("creating the referenced secrets so Degraded is not also triggered") + createReferencedSecrets() + + By("reconciling the freshly-created singleton") + doReconcile() + + cr := getCR() + Expect(conditionByType(cr, hyperfleetv1alpha1.ConditionAvailable).Status).To(Equal(metav1.ConditionFalse)) + Expect(conditionByType(cr, hyperfleetv1alpha1.ConditionAvailable).Reason).To(Equal(hyperfleetv1alpha1.ReasonDeploymentUnavailable)) + Expect(conditionByType(cr, hyperfleetv1alpha1.ConditionProgressing).Status).To(Equal(metav1.ConditionTrue)) + Expect(conditionByType(cr, hyperfleetv1alpha1.ConditionDegraded).Status).To(Equal(metav1.ConditionFalse)) + Expect(conditionByType(cr, hyperfleetv1alpha1.ConditionDegraded).Reason).To(Equal(hyperfleetv1alpha1.ReasonAsExpected)) + Expect(cr.Status.ObservedGeneration).To(Equal(cr.Generation)) + }) + + It("reports Available=True and Progressing=False once the Deployment is fully ready", func() { + By("reconciling to create the operands") + doReconcile() + + By("marking the Deployment fully ready, as the real Deployment controller would") + patchDeploymentStatus(func(dep *appsv1.Deployment) { + dep.Status.Replicas = 1 + dep.Status.AvailableReplicas = 1 + dep.Status.ReadyReplicas = 1 + dep.Status.UpdatedReplicas = 1 + dep.Status.ObservedGeneration = dep.Generation + }) + + By("reconciling again and reading the rolled-up status") + doReconcile() + + cr := getCR() + Expect(conditionByType(cr, hyperfleetv1alpha1.ConditionAvailable).Status).To(Equal(metav1.ConditionTrue)) + Expect(conditionByType(cr, hyperfleetv1alpha1.ConditionAvailable).Reason).To(Equal(hyperfleetv1alpha1.ReasonDeploymentAvailable)) + Expect(conditionByType(cr, hyperfleetv1alpha1.ConditionProgressing).Status).To(Equal(metav1.ConditionFalse)) + Expect(conditionByType(cr, hyperfleetv1alpha1.ConditionProgressing).Reason).To(Equal(hyperfleetv1alpha1.ReasonRolloutComplete)) + }) + + It("reports Progressing=True while a rollout to a new generation has not completed", func() { + doReconcile() + + By("marking the Deployment healthy at its current generation") + patchDeploymentStatus(func(dep *appsv1.Deployment) { + dep.Status.Replicas = 1 + dep.Status.AvailableReplicas = 1 + dep.Status.ReadyReplicas = 1 + dep.Status.UpdatedReplicas = 1 + dep.Status.ObservedGeneration = dep.Generation + }) + doReconcile() + Expect(conditionByType(getCR(), hyperfleetv1alpha1.ConditionProgressing).Status).To(Equal(metav1.ConditionFalse)) + + By("simulating a lagging rollout: the Deployment has not caught up to its latest generation") + patchDeploymentStatus(func(dep *appsv1.Deployment) { + dep.Status.ObservedGeneration = dep.Generation - 1 + }) + + doReconcile() + cr := getCR() + Expect(conditionByType(cr, hyperfleetv1alpha1.ConditionProgressing).Status).To(Equal(metav1.ConditionTrue)) + Expect(conditionByType(cr, hyperfleetv1alpha1.ConditionProgressing).Reason).To(Equal(hyperfleetv1alpha1.ReasonRolloutInProgress)) + }) + + It("reports Available=False when the operand Deployment goes down after having been healthy", func() { + doReconcile() + patchDeploymentStatus(func(dep *appsv1.Deployment) { + dep.Status.Replicas = 1 + dep.Status.AvailableReplicas = 1 + dep.Status.ReadyReplicas = 1 + dep.Status.UpdatedReplicas = 1 + dep.Status.ObservedGeneration = dep.Generation + }) + doReconcile() + Expect(conditionByType(getCR(), hyperfleetv1alpha1.ConditionAvailable).Status).To(Equal(metav1.ConditionTrue)) + + By("simulating the operand going down") + patchDeploymentStatus(func(dep *appsv1.Deployment) { + dep.Status.AvailableReplicas = 0 + dep.Status.ReadyReplicas = 0 + }) + + doReconcile() + cr := getCR() + Expect(conditionByType(cr, hyperfleetv1alpha1.ConditionAvailable).Status).To(Equal(metav1.ConditionFalse)) + Expect(conditionByType(cr, hyperfleetv1alpha1.ConditionAvailable).Reason).To(Equal(hyperfleetv1alpha1.ReasonDeploymentUnavailable)) + }) + + It("reports Degraded=True when a referenced Secret is missing", func() { + By("reconciling without creating the fixture's referenced database Secret") + // The fixture's HyperFleetConfig references a database Secret that is never + // created in this spec, so referencedSecretData reports it absent. + doReconcile() + + cr := getCR() + Expect(conditionByType(cr, hyperfleetv1alpha1.ConditionDegraded).Status).To(Equal(metav1.ConditionTrue)) + Expect(conditionByType(cr, hyperfleetv1alpha1.ConditionDegraded).Reason).To(Equal(hyperfleetv1alpha1.ReasonReferencedSecretMissing)) + Expect(conditionByType(cr, hyperfleetv1alpha1.ConditionDegraded).Message).To(ContainSubstring("database")) + }) + + It("does not bump lastTransitionTime on a no-op reconcile", func() { + By("reconciling once to establish a baseline status") + doReconcile() + firstTransition := conditionByType(getCR(), hyperfleetv1alpha1.ConditionAvailable).LastTransitionTime + + By("reconciling again with no state change") + doReconcile() + + Expect(conditionByType(getCR(), hyperfleetv1alpha1.ConditionAvailable).LastTransitionTime).To(Equal(firstTransition)) + }) + + It("reports Degraded=True but leaves Available/Progressing untouched when reconcile fails before any component reports", func() { + By("reconciling once so a real Available/Progressing value is already recorded") + createReferencedSecrets() + doReconcile() + cr := getCR() + Expect(conditionByType(cr, hyperfleetv1alpha1.ConditionAvailable).Status).To(Equal(metav1.ConditionFalse)) + Expect(conditionByType(cr, hyperfleetv1alpha1.ConditionAvailable).Reason).To(Equal(hyperfleetv1alpha1.ReasonDeploymentUnavailable)) + + By("pointing auth at an issuer whose discovery endpoint fails, so OIDC discovery errors before referencedSecretData or any component ever runs") + badDiscovery := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + })) + DeferCleanup(badDiscovery.Close) + reconciler.HTTPClient = badDiscovery.Client() + + cr.Spec.API.Auth.JWKCertSecretRef = nil + cr.Spec.API.Auth.Issuer = badDiscovery.URL + Expect(k8sClient.Update(ctx, cr)).To(Succeed()) + + _, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: typeNamespacedName}) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("resolve JWKS URL")) + + By("verifying Degraded reflects the failure while Available is left at its last-recorded value, not fabricated healthy") + cr = getCR() + Expect(conditionByType(cr, hyperfleetv1alpha1.ConditionDegraded).Status).To(Equal(metav1.ConditionTrue)) + Expect(conditionByType(cr, hyperfleetv1alpha1.ConditionDegraded).Reason).To(Equal(hyperfleetv1alpha1.ReasonReconcileError)) + Expect(conditionByType(cr, hyperfleetv1alpha1.ConditionDegraded).Message).NotTo(ContainSubstring("500"), + "the Degraded message must not echo the raw wrapped error") + Expect(conditionByType(cr, hyperfleetv1alpha1.ConditionAvailable).Status).To(Equal(metav1.ConditionFalse), + "Available must remain at its last-recorded value, not be fabricated True") + Expect(conditionByType(cr, hyperfleetv1alpha1.ConditionAvailable).Reason).To(Equal(hyperfleetv1alpha1.ReasonDeploymentUnavailable)) + }) + It("returns a wrapped error when the operand namespace is absent", func() { // The reconciler wraps apply failures as "apply component %q: %w". Point it // at a namespace that does not exist so the first apply (the ServiceAccount) diff --git a/internal/controller/hyperfleetconfig_status.go b/internal/controller/hyperfleetconfig_status.go new file mode 100644 index 0000000..905e947 --- /dev/null +++ b/internal/controller/hyperfleetconfig_status.go @@ -0,0 +1,155 @@ +/* +Copyright 2026. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controller + +import ( + "strings" + + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + hyperfleetv1alpha1 "github.com/openshift-hyperfleet/hyperfleet-operator/api/v1alpha1" +) + +// aggregateStatus rolls up each component's reported Conditions plus the +// controller-level Degraded signal (a missing referenced Secret, or a +// reconcile error) into the CR's top-level status.conditions, and sets +// status.observedGeneration. It reports whether anything actually changed, so +// Reconcile can skip a no-op Status().Update() call. +// +// Available is the AND of every component's Available condition. +// Progressing is the OR of every component's Progressing condition. Degraded is driven only +// by controller-level signals, not by component health, keeping the "component" boundary +// limited to what it can observe about its own operands (see internal/bundle.Component). +// +// componentsCollected must be true only when every component's Conditions() +// was actually gathered this reconcile (the render/apply/Conditions loop ran +// to completion without error). When false — Reconcile aborted early, e.g. a +// failed OIDC discovery or a failed apply before any component reported in — +// Available/Progressing are deliberately left untouched rather than defaulted +// to a fabricated healthy value: with zero fresh data, publishing "True" would +// contradict the Degraded=True this same reconcile is about to report, and +// would misrepresent an operand whose real state was never checked. Degraded +// is always evaluated regardless, since a reconcile error is itself the signal. +func aggregateStatus(cr *hyperfleetv1alpha1.HyperFleetConfig, componentConditions []metav1.Condition, componentsCollected bool, missingSecrets []string, reconcileErr error) bool { + changed := false + + conditions := []metav1.Condition{degradedCondition(missingSecrets, reconcileErr)} + if componentsCollected { + conditions = append(conditions, + rollupCondition(hyperfleetv1alpha1.ConditionAvailable, componentConditions, + hyperfleetv1alpha1.ReasonDeploymentAvailable, "all components are available"), + rollupProgressing(componentConditions), + ) + } + + for _, cond := range conditions { + cond.ObservedGeneration = cr.Generation + if meta.SetStatusCondition(&cr.Status.Conditions, cond) { + changed = true + } + } + + if cr.Status.ObservedGeneration != cr.Generation { + cr.Status.ObservedGeneration = cr.Generation + changed = true + } + + return changed +} + +// rollupCondition ANDs every component condition of the given type: True only +// when every reported condition of that type is True. The first non-True +// condition's reason/message is surfaced. Absent any component conditions of +// this type, it reports True with the supplied default reason/message +// (vacuously true — there is nothing to be unavailable). +func rollupCondition(condType string, componentConditions []metav1.Condition, trueReason, trueMessage string) metav1.Condition { + for _, c := range componentConditions { + if c.Type == condType && c.Status != metav1.ConditionTrue { + return metav1.Condition{ + Type: condType, + Status: c.Status, + Reason: c.Reason, + Message: c.Message, + } + } + } + return metav1.Condition{ + Type: condType, + Status: metav1.ConditionTrue, + Reason: trueReason, + Message: trueMessage, + } +} + +// rollupProgressing ORs every component's Progressing condition: True if any +// component reports Progressing=True. +func rollupProgressing(componentConditions []metav1.Condition) metav1.Condition { + for _, c := range componentConditions { + if c.Type == hyperfleetv1alpha1.ConditionProgressing && c.Status == metav1.ConditionTrue { + return metav1.Condition{ + Type: hyperfleetv1alpha1.ConditionProgressing, + Status: metav1.ConditionTrue, + Reason: c.Reason, + Message: c.Message, + } + } + } + return metav1.Condition{ + Type: hyperfleetv1alpha1.ConditionProgressing, + Status: metav1.ConditionFalse, + Reason: hyperfleetv1alpha1.ReasonRolloutComplete, + Message: "all components have completed rollout", + } +} + +// degradedCondition reports Degraded=True when one or more referenced Secrets +// are missing or the most recent reconcile failed; a missing Secret takes +// precedence in its message when both are true, since it is usually the root +// cause of a downstream reconcile error (e.g. a mount failure). The +// reconcile-error branch deliberately does NOT echo reconcileErr.Error() into +// the Message: status.conditions is readable by any principal with get/list on +// this cluster-scoped CRD, and wrapped errors on the JWKS-discovery path can +// contain internal network details (see blockDiscoveryDial in +// hyperfleetconfig_rollout.go, which names the disallowed resolved IP in its +// error text). The caller logs the real error; only a safe, static summary is +// published here. +func degradedCondition(missingSecrets []string, reconcileErr error) metav1.Condition { + switch { + case len(missingSecrets) > 0: + return metav1.Condition{ + Type: hyperfleetv1alpha1.ConditionDegraded, + Status: metav1.ConditionTrue, + Reason: hyperfleetv1alpha1.ReasonReferencedSecretMissing, + Message: "referenced secret(s) missing in the operator's namespace: " + strings.Join(missingSecrets, ", "), + } + case reconcileErr != nil: + return metav1.Condition{ + Type: hyperfleetv1alpha1.ConditionDegraded, + Status: metav1.ConditionTrue, + Reason: hyperfleetv1alpha1.ReasonReconcileError, + Message: "the last reconcile failed; see the operator's logs for details", + } + default: + return metav1.Condition{ + Type: hyperfleetv1alpha1.ConditionDegraded, + Status: metav1.ConditionFalse, + Reason: hyperfleetv1alpha1.ReasonAsExpected, + Message: "no failure detected", + } + } +} diff --git a/internal/controller/hyperfleetconfig_status_test.go b/internal/controller/hyperfleetconfig_status_test.go new file mode 100644 index 0000000..e6ce0d2 --- /dev/null +++ b/internal/controller/hyperfleetconfig_status_test.go @@ -0,0 +1,207 @@ +/* +Copyright 2026. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controller + +import ( + "errors" + "testing" + + . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + hyperfleetv1alpha1 "github.com/openshift-hyperfleet/hyperfleet-operator/api/v1alpha1" +) + +func condition(condType string, status metav1.ConditionStatus, reason string) metav1.Condition { + return metav1.Condition{Type: condType, Status: status, Reason: reason, Message: "test"} +} + +func conditionByType(cr *hyperfleetv1alpha1.HyperFleetConfig, condType string) metav1.Condition { + for _, c := range cr.Status.Conditions { + if c.Type == condType { + return c + } + } + return metav1.Condition{} +} + +func TestAggregateStatusNoBumpOnUnrelatedReconcile(t *testing.T) { + g := NewWithT(t) + + cr := &hyperfleetv1alpha1.HyperFleetConfig{} + cr.Generation = 1 + componentConditions := []metav1.Condition{ + condition(hyperfleetv1alpha1.ConditionAvailable, metav1.ConditionTrue, hyperfleetv1alpha1.ReasonDeploymentAvailable), + condition(hyperfleetv1alpha1.ConditionProgressing, metav1.ConditionFalse, hyperfleetv1alpha1.ReasonRolloutComplete), + } + + changed := aggregateStatus(cr, componentConditions, true, nil, nil) + g.Expect(changed).To(BeTrue(), "first write must report a change") + firstTransition := conditionByType(cr, hyperfleetv1alpha1.ConditionAvailable).LastTransitionTime + + // Second reconcile: Generation moved (an unrelated spec/annotation edit) but + // component health did not — observedGeneration must still advance + // (changed=true) while LastTransitionTime must stay byte-identical (AC#5). + cr.Generation = 2 + changed = aggregateStatus(cr, componentConditions, true, nil, nil) + g.Expect(changed).To(BeTrue(), "observedGeneration must still advance even though health is unchanged") + g.Expect(conditionByType(cr, hyperfleetv1alpha1.ConditionAvailable).LastTransitionTime).To(Equal(firstTransition)) + g.Expect(cr.Status.ObservedGeneration).To(Equal(int64(2))) + + // A third reconcile with nothing changed at all (same generation, same + // health) must report no change whatsoever. + changed = aggregateStatus(cr, componentConditions, true, nil, nil) + g.Expect(changed).To(BeFalse(), "a fully unrelated no-op reconcile must not report a change") +} + +func TestAggregateStatusAvailableIsAndOverComponents(t *testing.T) { + g := NewWithT(t) + + cr := &hyperfleetv1alpha1.HyperFleetConfig{} + cr.Generation = 1 + componentConditions := []metav1.Condition{ + condition(hyperfleetv1alpha1.ConditionAvailable, metav1.ConditionTrue, hyperfleetv1alpha1.ReasonDeploymentAvailable), + condition(hyperfleetv1alpha1.ConditionAvailable, metav1.ConditionTrue, hyperfleetv1alpha1.ReasonDeploymentAvailable), + condition(hyperfleetv1alpha1.ConditionAvailable, metav1.ConditionFalse, hyperfleetv1alpha1.ReasonDeploymentNotReady), + } + + aggregateStatus(cr, componentConditions, true, nil, nil) + + g.Expect(conditionByType(cr, hyperfleetv1alpha1.ConditionAvailable).Status).To(Equal(metav1.ConditionFalse)) + g.Expect(conditionByType(cr, hyperfleetv1alpha1.ConditionAvailable).Reason).To(Equal(hyperfleetv1alpha1.ReasonDeploymentNotReady)) +} + +func TestAggregateStatusProgressingIsOrOverComponents(t *testing.T) { + g := NewWithT(t) + + cr := &hyperfleetv1alpha1.HyperFleetConfig{} + cr.Generation = 1 + componentConditions := []metav1.Condition{ + condition(hyperfleetv1alpha1.ConditionProgressing, metav1.ConditionFalse, hyperfleetv1alpha1.ReasonRolloutComplete), + condition(hyperfleetv1alpha1.ConditionProgressing, metav1.ConditionFalse, hyperfleetv1alpha1.ReasonRolloutComplete), + condition(hyperfleetv1alpha1.ConditionProgressing, metav1.ConditionTrue, hyperfleetv1alpha1.ReasonRolloutInProgress), + } + + aggregateStatus(cr, componentConditions, true, nil, nil) + + g.Expect(conditionByType(cr, hyperfleetv1alpha1.ConditionProgressing).Status).To(Equal(metav1.ConditionTrue)) + g.Expect(conditionByType(cr, hyperfleetv1alpha1.ConditionProgressing).Reason).To(Equal(hyperfleetv1alpha1.ReasonRolloutInProgress)) +} + +func TestAggregateStatusDegradedPrecedence(t *testing.T) { + g := NewWithT(t) + + cr := &hyperfleetv1alpha1.HyperFleetConfig{} + cr.Generation = 1 + + // missingSecrets and a reconcile error both set: missingSecrets takes + // precedence in the surfaced reason (it is usually the root cause). + aggregateStatus(cr, nil, false, []string{"database"}, errors.New("apply component \"api\": boom")) + g.Expect(conditionByType(cr, hyperfleetv1alpha1.ConditionDegraded).Status).To(Equal(metav1.ConditionTrue)) + g.Expect(conditionByType(cr, hyperfleetv1alpha1.ConditionDegraded).Reason).To(Equal(hyperfleetv1alpha1.ReasonReferencedSecretMissing)) + g.Expect(conditionByType(cr, hyperfleetv1alpha1.ConditionDegraded).Message).To(ContainSubstring("database")) + + // Only a reconcile error: ReconcileError, with a safe static message (never + // the raw wrapped error text — see degradedCondition's doc comment). + cr2 := &hyperfleetv1alpha1.HyperFleetConfig{} + cr2.Generation = 1 + aggregateStatus(cr2, nil, false, nil, errors.New("apply component \"api\": boom")) + g.Expect(conditionByType(cr2, hyperfleetv1alpha1.ConditionDegraded).Status).To(Equal(metav1.ConditionTrue)) + g.Expect(conditionByType(cr2, hyperfleetv1alpha1.ConditionDegraded).Reason).To(Equal(hyperfleetv1alpha1.ReasonReconcileError)) + g.Expect(conditionByType(cr2, hyperfleetv1alpha1.ConditionDegraded).Message).NotTo(ContainSubstring("boom")) + + // Neither: AsExpected/False. + cr3 := &hyperfleetv1alpha1.HyperFleetConfig{} + cr3.Generation = 1 + aggregateStatus(cr3, nil, false, nil, nil) + g.Expect(conditionByType(cr3, hyperfleetv1alpha1.ConditionDegraded).Status).To(Equal(metav1.ConditionFalse)) + g.Expect(conditionByType(cr3, hyperfleetv1alpha1.ConditionDegraded).Reason).To(Equal(hyperfleetv1alpha1.ReasonAsExpected)) +} + +func TestAggregateStatusSetsObservedGeneration(t *testing.T) { + g := NewWithT(t) + + cr := &hyperfleetv1alpha1.HyperFleetConfig{} + cr.Generation = 3 + + // componentsCollected=true with zero component conditions models a bundle + // that genuinely has no components — the vacuous-true default is correct + // here, unlike the componentsCollected=false case tested below. + changed := aggregateStatus(cr, nil, true, nil, nil) + g.Expect(changed).To(BeTrue()) + g.Expect(cr.Status.ObservedGeneration).To(Equal(int64(3))) + g.Expect(conditionByType(cr, hyperfleetv1alpha1.ConditionAvailable).Status).To(Equal(metav1.ConditionTrue)) + g.Expect(conditionByType(cr, hyperfleetv1alpha1.ConditionAvailable).Reason).To(Equal(hyperfleetv1alpha1.ReasonDeploymentAvailable)) + g.Expect(conditionByType(cr, hyperfleetv1alpha1.ConditionProgressing).Status).To(Equal(metav1.ConditionFalse)) + g.Expect(conditionByType(cr, hyperfleetv1alpha1.ConditionProgressing).Reason).To(Equal(hyperfleetv1alpha1.ReasonRolloutComplete)) + for _, c := range cr.Status.Conditions { + g.Expect(c.ObservedGeneration).To(Equal(int64(3))) + } + + // A later reconcile at the same generation, same health: no change. + changed = aggregateStatus(cr, nil, true, nil, nil) + g.Expect(changed).To(BeFalse()) +} + +// TestAggregateStatusComponentsNotCollectedLeavesAvailableProgressingUnset +// covers the bug this story shipped and fixed: when Reconcile aborts before +// any component reports health (componentsCollected=false), Available and +// Progressing must NOT be fabricated as healthy — only Degraded is written. +func TestAggregateStatusComponentsNotCollectedLeavesAvailableProgressingUnset(t *testing.T) { + g := NewWithT(t) + + cr := &hyperfleetv1alpha1.HyperFleetConfig{} + cr.Generation = 1 + + changed := aggregateStatus(cr, nil, false, nil, errors.New("resolve JWKS URL: boom")) + g.Expect(changed).To(BeTrue(), "Degraded must still be written") + + g.Expect(conditionByType(cr, hyperfleetv1alpha1.ConditionDegraded).Status).To(Equal(metav1.ConditionTrue)) + g.Expect(conditionByType(cr, hyperfleetv1alpha1.ConditionDegraded).Reason).To(Equal(hyperfleetv1alpha1.ReasonReconcileError)) + g.Expect(conditionByType(cr, hyperfleetv1alpha1.ConditionAvailable)).To(Equal(metav1.Condition{}), + "Available must not be fabricated when components were never collected") + g.Expect(conditionByType(cr, hyperfleetv1alpha1.ConditionProgressing)).To(Equal(metav1.Condition{}), + "Progressing must not be fabricated when components were never collected") +} + +// TestAggregateStatusComponentsNotCollectedPreservesPriorAvailable covers the +// same fix from the other direction: a transient failure on a LATER reconcile +// (after a prior successful one already recorded real health) must not +// overwrite that last-known-good Available/Progressing with a guess. +func TestAggregateStatusComponentsNotCollectedPreservesPriorAvailable(t *testing.T) { + g := NewWithT(t) + + cr := &hyperfleetv1alpha1.HyperFleetConfig{} + cr.Generation = 1 + + // A prior, fully successful reconcile recorded real health. + componentConditions := []metav1.Condition{ + condition(hyperfleetv1alpha1.ConditionAvailable, metav1.ConditionTrue, hyperfleetv1alpha1.ReasonDeploymentAvailable), + condition(hyperfleetv1alpha1.ConditionProgressing, metav1.ConditionFalse, hyperfleetv1alpha1.ReasonRolloutComplete), + } + aggregateStatus(cr, componentConditions, true, nil, nil) + g.Expect(conditionByType(cr, hyperfleetv1alpha1.ConditionAvailable).Status).To(Equal(metav1.ConditionTrue)) + + // A later reconcile fails before reaching any component (e.g. a transient + // OIDC discovery blip): Available must remain exactly as last recorded. + cr.Generation = 2 + aggregateStatus(cr, nil, false, nil, errors.New("resolve JWKS URL: boom")) + + g.Expect(conditionByType(cr, hyperfleetv1alpha1.ConditionAvailable).Status).To(Equal(metav1.ConditionTrue), + "a failed reconcile that never reached the components must not change the last-known Available value") + g.Expect(conditionByType(cr, hyperfleetv1alpha1.ConditionDegraded).Status).To(Equal(metav1.ConditionTrue)) +}