From bcf9e5d9e34490247e1fb4909afff9be81ff790f Mon Sep 17 00:00:00 2001 From: yasun Date: Sat, 28 Feb 2026 16:28:52 +0800 Subject: [PATCH] HYPERFLEET-657 - fix: enforce mandatory conditions in adapter status Reject adapter status updates missing mandatory conditions (Available, Applied, Health) or containing Unknown status. Returns HTTP 204 and preserves existing status to prevent loss of critical cluster state. Changes: - Add condition type constants in pkg/api/status_types.go - Validate mandatory conditions in cluster/nodepool services - Replace hardcoded condition strings with constants - Add 4 new tests, update 8 existing tests for validation - Update integration tests to use constants All tests passing: 416 unit, 51 integration, 0 lint issues Co-Authored-By: Claude Sonnet 4.5 update update update update fix lint update fix unkonwn bypass mandatory issue reject duplicate condition update update update update --- pkg/api/status_types.go | 8 + pkg/services/cluster.go | 45 +- pkg/services/cluster_test.go | 527 ++++++++++++++++++------ pkg/services/node_pool.go | 47 ++- pkg/services/node_pool_test.go | 123 +++--- pkg/services/status_aggregation.go | 49 ++- pkg/services/status_aggregation_test.go | 153 ++++++- test/integration/adapter_status_test.go | 523 +++++++++++++++++++++-- 8 files changed, 1214 insertions(+), 261 deletions(-) diff --git a/pkg/api/status_types.go b/pkg/api/status_types.go index c0d8b67..b1c87e5 100644 --- a/pkg/api/status_types.go +++ b/pkg/api/status_types.go @@ -21,6 +21,14 @@ const ( AdapterConditionUnknown AdapterConditionStatus = "Unknown" ) +// Condition type constants +const ( + ConditionTypeAvailable = "Available" + ConditionTypeApplied = "Applied" + ConditionTypeHealth = "Health" + ConditionTypeReady = "Ready" +) + // ResourceCondition represents a condition of a resource // Domain equivalent of openapi.ResourceCondition // JSON tags match database JSONB structure diff --git a/pkg/services/cluster.go b/pkg/services/cluster.go index 9a5fd5c..730a08c 100644 --- a/pkg/services/cluster.go +++ b/pkg/services/cluster.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" stderrors "errors" + "fmt" "strings" "time" @@ -30,7 +31,8 @@ type ClusterService interface { UpdateClusterStatusFromAdapters(ctx context.Context, clusterID string) (*api.Cluster, *errors.ServiceError) // ProcessAdapterStatus handles the business logic for adapter status: - // - If Available condition is "Unknown": returns (nil, nil) indicating no-op + // - First report: accepts Unknown Available condition, skips aggregation + // - Subsequent reports: rejects Unknown Available condition // - Otherwise: upserts the status and triggers aggregation ProcessAdapterStatus( ctx context.Context, clusterID string, adapterStatus *api.AdapterStatus, @@ -237,9 +239,12 @@ func (s *sqlClusterService) UpdateClusterStatusFromAdapters( } // ProcessAdapterStatus handles the business logic for adapter status: -// - If Available is "Unknown" and a status already exists: returns (nil, nil) as no-op -// - If Available is "Unknown" and no status exists (first report): upserts the status -// - Otherwise: upserts the status and triggers aggregation +// - Validates that all mandatory conditions (Available, Applied, Health) are present +// - Rejects duplicate condition types +// - For first status report: accepts Unknown Available condition to avoid data loss +// - For subsequent reports: rejects Unknown Available condition to preserve existing valid state +// - Uses complete replacement semantics: each update replaces all conditions for this adapter +// - Returns (nil, nil) for discarded updates func (s *sqlClusterService) ProcessAdapterStatus( ctx context.Context, clusterID string, adapterStatus *api.AdapterStatus, ) (*api.AdapterStatus, *errors.ServiceError) { @@ -264,35 +269,45 @@ func (s *sqlClusterService) ProcessAdapterStatus( } } - // Find the "Available" condition - hasAvailableCondition := false + // Validate mandatory conditions and check for duplicates + if errorType, conditionName := ValidateMandatoryConditions(conditions); errorType != "" { + ctx = logger.WithClusterID(ctx, clusterID) + logger.Info(ctx, fmt.Sprintf("Discarding adapter status update from %s: %s condition %s", + adapterStatus.Adapter, errorType, conditionName)) + return nil, nil + } + + // Check Available condition for Unknown status + triggerAggregation := false for _, cond := range conditions { - if cond.Type != "Available" { + if cond.Type != api.ConditionTypeAvailable { continue } - hasAvailableCondition = true + triggerAggregation = true if cond.Status == api.AdapterConditionUnknown { if existingStatus != nil { - // Available condition is "Unknown" and a status already exists, return nil to indicate no-op + // Non-first report && Available=Unknown → reject + ctx = logger.WithClusterID(ctx, clusterID) + logger.Info(ctx, fmt.Sprintf("Discarding adapter status update from %s: subsequent Unknown Available", + adapterStatus.Adapter)) return nil, nil } // First report from this adapter: allow storing even with Available=Unknown // but skip aggregation since Unknown should not affect cluster-level conditions - hasAvailableCondition = false + triggerAggregation = false } + break } - // Upsert the adapter status + // Upsert the adapter status (complete replacement) upsertedStatus, err := s.adapterStatusDao.Upsert(ctx, adapterStatus) if err != nil { return nil, handleCreateError("AdapterStatus", err) } - // Only trigger aggregation when the adapter reported an Available condition. - // If the adapter status doesn't include Available (e.g. it only reports Ready/Progressing), - // saving it should not overwrite the cluster's synthetic Available/Ready conditions. - if hasAvailableCondition { + // Only trigger aggregation when triggerAggregation is true + if triggerAggregation { if _, aggregateErr := s.UpdateClusterStatusFromAdapters( ctx, clusterID, ); aggregateErr != nil { diff --git a/pkg/services/cluster_test.go b/pkg/services/cluster_test.go index a66cfb4..e79d6fb 100644 --- a/pkg/services/cluster_test.go +++ b/pkg/services/cluster_test.go @@ -169,34 +169,38 @@ func TestProcessAdapterStatus_FirstUnknownCondition(t *testing.T) { clusterDao := newMockClusterDao() adapterStatusDao := newMockAdapterStatusDao() - config := testAdapterConfig() service := NewClusterService(clusterDao, adapterStatusDao, config) ctx := context.Background() clusterID := testClusterID - // Create adapter status with Available=Unknown + // Create cluster first + cluster := &api.Cluster{Generation: 1} + cluster.ID = clusterID + _, svcErr := service.Create(ctx, cluster) + Expect(svcErr).To(BeNil()) + + // Send first status with Available=Unknown conditions := []api.AdapterCondition{ - { - Type: conditionTypeAvailable, - Status: api.AdapterConditionUnknown, - LastTransitionTime: time.Now(), - }, + {Type: api.ConditionTypeAvailable, Status: api.AdapterConditionUnknown, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeApplied, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeHealth, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, } conditionsJSON, _ := json.Marshal(conditions) - now := time.Now() adapterStatus := &api.AdapterStatus{ - ResourceType: "Cluster", - ResourceID: clusterID, - Adapter: "test-adapter", - Conditions: conditionsJSON, - CreatedTime: &now, + ResourceType: "Cluster", + ResourceID: clusterID, + Adapter: "test-adapter", + Conditions: conditionsJSON, + ObservedGeneration: 1, + CreatedTime: &now, } result, err := service.ProcessAdapterStatus(ctx, clusterID, adapterStatus) + // First report with Unknown should be accepted Expect(err).To(BeNil()) Expect(result).ToNot(BeNil(), "First report with Available=Unknown should be stored") Expect(result.Adapter).To(Equal("test-adapter")) @@ -221,11 +225,9 @@ func TestProcessAdapterStatus_SubsequentUnknownCondition(t *testing.T) { // Pre-populate an existing adapter status to simulate a previously stored report conditions := []api.AdapterCondition{ - { - Type: conditionTypeAvailable, - Status: api.AdapterConditionUnknown, - LastTransitionTime: time.Now(), - }, + {Type: api.ConditionTypeAvailable, Status: api.AdapterConditionUnknown, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeApplied, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeHealth, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, } conditionsJSON, _ := json.Marshal(conditions) @@ -275,10 +277,20 @@ func TestProcessAdapterStatus_TrueCondition(t *testing.T) { _, svcErr := service.Create(ctx, cluster) Expect(svcErr).To(BeNil()) - // Create adapter status with Available=True + // Create adapter status with all mandatory conditions conditions := []api.AdapterCondition{ { - Type: conditionTypeAvailable, + Type: api.ConditionTypeAvailable, + Status: api.AdapterConditionTrue, + LastTransitionTime: time.Now(), + }, + { + Type: api.ConditionTypeApplied, + Status: api.AdapterConditionTrue, + LastTransitionTime: time.Now(), + }, + { + Type: api.ConditionTypeHealth, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now(), }, @@ -326,10 +338,20 @@ func TestProcessAdapterStatus_FalseCondition(t *testing.T) { _, svcErr := service.Create(ctx, cluster) Expect(svcErr).To(BeNil()) - // Create adapter status with Available=False + // Create adapter status with all mandatory conditions conditions := []api.AdapterCondition{ { - Type: conditionTypeAvailable, + Type: api.ConditionTypeAvailable, + Status: api.AdapterConditionFalse, + LastTransitionTime: time.Now(), + }, + { + Type: api.ConditionTypeApplied, + Status: api.AdapterConditionTrue, + LastTransitionTime: time.Now(), + }, + { + Type: api.ConditionTypeHealth, Status: api.AdapterConditionFalse, LastTransitionTime: time.Now(), }, @@ -355,8 +377,9 @@ func TestProcessAdapterStatus_FalseCondition(t *testing.T) { Expect(len(storedStatuses)).To(Equal(1), "Status should be stored for False condition") } -// TestProcessAdapterStatus_NoAvailableCondition tests when there's no Available condition -func TestProcessAdapterStatus_NoAvailableCondition(t *testing.T) { +// TestProcessAdapterStatus_FirstMultipleConditions_AvailableUnknown tests that first reports with +// Available=Unknown are accepted even when other conditions are present +func TestProcessAdapterStatus_FirstMultipleConditions_AvailableUnknown(t *testing.T) { RegisterTestingT(t) clusterDao := newMockClusterDao() @@ -368,100 +391,31 @@ func TestProcessAdapterStatus_NoAvailableCondition(t *testing.T) { ctx := context.Background() clusterID := testClusterID - // Create the cluster first - fixedNow := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) - initialConditions := []api.ResourceCondition{ - { - Type: conditionTypeAvailable, - Status: api.ConditionFalse, - ObservedGeneration: 1, - LastTransitionTime: fixedNow, - CreatedTime: fixedNow, - LastUpdatedTime: fixedNow, - }, - { - Type: "Ready", - Status: api.ConditionFalse, - ObservedGeneration: 7, - LastTransitionTime: fixedNow, - CreatedTime: fixedNow, - LastUpdatedTime: fixedNow, - }, - } - initialConditionsJSON, _ := json.Marshal(initialConditions) - - cluster := &api.Cluster{ - Generation: 7, - StatusConditions: initialConditionsJSON, - } + // Create cluster first + cluster := &api.Cluster{Generation: 1} cluster.ID = clusterID _, svcErr := service.Create(ctx, cluster) Expect(svcErr).To(BeNil()) - initialClusterStatusConditions := api.Cluster{}.StatusConditions - initialClusterStatusConditions = append(initialClusterStatusConditions, cluster.StatusConditions...) - // Create adapter status with Health condition (no Available) + // Create first adapter status with all mandatory conditions but Available=Unknown conditions := []api.AdapterCondition{ { - Type: "Health", - Status: api.AdapterConditionTrue, + Type: api.ConditionTypeAvailable, + Status: api.AdapterConditionUnknown, LastTransitionTime: time.Now(), }, - } - conditionsJSON, _ := json.Marshal(conditions) - - now := time.Now() - adapterStatus := &api.AdapterStatus{ - ResourceType: "Cluster", - ResourceID: clusterID, - Adapter: "test-adapter", - Conditions: conditionsJSON, - CreatedTime: &now, - } - - result, err := service.ProcessAdapterStatus(ctx, clusterID, adapterStatus) - - Expect(err).To(BeNil()) - Expect(result).ToNot(BeNil(), "ProcessAdapterStatus should proceed when no Available condition") - - // Verify the status was stored - storedStatuses, _ := adapterStatusDao.FindByResource(ctx, "Cluster", clusterID) - Expect(len(storedStatuses)).To(Equal(1), "Status should be stored when no Available condition") - - // Verify that saving a non-Available condition did not overwrite cluster Available/Ready - storedCluster, _ := clusterDao.Get(ctx, clusterID) - Expect(storedCluster.StatusConditions).To(Equal(initialClusterStatusConditions), - "Cluster status conditions should not be overwritten when adapter status lacks Available") -} - -// TestProcessAdapterStatus_FirstMultipleConditions_AvailableUnknown tests that the first report with -// multiple conditions including Available=Unknown is stored -func TestProcessAdapterStatus_FirstMultipleConditions_AvailableUnknown(t *testing.T) { - RegisterTestingT(t) - - clusterDao := newMockClusterDao() - adapterStatusDao := newMockAdapterStatusDao() - - config := testAdapterConfig() - service := NewClusterService(clusterDao, adapterStatusDao, config) - - ctx := context.Background() - clusterID := testClusterID - - // Create adapter status with multiple conditions including Available=Unknown - conditions := []api.AdapterCondition{ { - Type: "Ready", + Type: api.ConditionTypeApplied, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now(), }, { - Type: conditionTypeAvailable, - Status: api.AdapterConditionUnknown, + Type: api.ConditionTypeHealth, + Status: api.AdapterConditionTrue, LastTransitionTime: time.Now(), }, { - Type: "Progressing", + Type: api.ConditionTypeReady, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now(), }, @@ -480,11 +434,11 @@ func TestProcessAdapterStatus_FirstMultipleConditions_AvailableUnknown(t *testin result, err := service.ProcessAdapterStatus(ctx, clusterID, adapterStatus) Expect(err).To(BeNil()) - Expect(result).ToNot(BeNil(), "First report with Available=Unknown should be stored") + Expect(result).ToNot(BeNil(), "First report with Available=Unknown should be accepted") // Verify the status was stored storedStatuses, _ := adapterStatusDao.FindByResource(ctx, "Cluster", clusterID) - Expect(len(storedStatuses)).To(Equal(1), "First Unknown status should be stored") + Expect(len(storedStatuses)).To(Equal(1), "First status with Available=Unknown should be stored") } // TestProcessAdapterStatus_SubsequentMultipleConditions_AvailableUnknown tests that subsequent reports @@ -503,11 +457,9 @@ func TestProcessAdapterStatus_SubsequentMultipleConditions_AvailableUnknown(t *t // Pre-populate an existing adapter status existingConditions := []api.AdapterCondition{ - { - Type: conditionTypeAvailable, - Status: api.AdapterConditionUnknown, - LastTransitionTime: time.Now(), - }, + {Type: api.ConditionTypeAvailable, Status: api.AdapterConditionUnknown, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeApplied, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeHealth, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, } existingConditionsJSON, _ := json.Marshal(existingConditions) @@ -523,21 +475,11 @@ func TestProcessAdapterStatus_SubsequentMultipleConditions_AvailableUnknown(t *t // Now send another report with multiple conditions including Available=Unknown conditions := []api.AdapterCondition{ - { - Type: "Ready", - Status: api.AdapterConditionTrue, - LastTransitionTime: time.Now(), - }, - { - Type: conditionTypeAvailable, - Status: api.AdapterConditionUnknown, - LastTransitionTime: time.Now(), - }, - { - Type: "Progressing", - Status: api.AdapterConditionTrue, - LastTransitionTime: time.Now(), - }, + {Type: api.ConditionTypeAvailable, Status: api.AdapterConditionUnknown, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeApplied, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeHealth, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeReady, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: "Progressing", Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, } conditionsJSON, _ := json.Marshal(conditions) @@ -585,9 +527,9 @@ func TestClusterAvailableReadyTransitions(t *testing.T) { var available, ready *api.ResourceCondition for i := range conds { switch conds[i].Type { - case conditionTypeAvailable: + case api.ConditionTypeAvailable: available = &conds[i] - case conditionTypeReady: + case api.ConditionTypeReady: ready = &conds[i] } } @@ -598,7 +540,9 @@ func TestClusterAvailableReadyTransitions(t *testing.T) { upsert := func(adapter string, available api.AdapterConditionStatus, observedGen int32) { conditions := []api.AdapterCondition{ - {Type: conditionTypeAvailable, Status: available, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeAvailable, Status: available, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeApplied, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeHealth, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, } conditionsJSON, _ := json.Marshal(conditions) now := time.Now() @@ -689,7 +633,9 @@ func TestClusterAvailableReadyTransitions(t *testing.T) { prevStatus := api.Cluster{}.StatusConditions prevStatus = append(prevStatus, clusterDao.clusters[clusterID].StatusConditions...) unknownConds := []api.AdapterCondition{ - {Type: conditionTypeAvailable, Status: api.AdapterConditionUnknown, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeAvailable, Status: api.AdapterConditionUnknown, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeApplied, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeHealth, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, } unknownJSON, _ := json.Marshal(unknownConds) unknownStatus := &api.AdapterStatus{ @@ -730,7 +676,7 @@ func TestClusterStaleAdapterStatusUpdatePolicy(t *testing.T) { var conds []api.ResourceCondition Expect(json.Unmarshal(stored.StatusConditions, &conds)).To(Succeed()) for i := range conds { - if conds[i].Type == conditionTypeAvailable { + if conds[i].Type == api.ConditionTypeAvailable { return conds[i] } } @@ -740,7 +686,9 @@ func TestClusterStaleAdapterStatusUpdatePolicy(t *testing.T) { upsert := func(adapter string, available api.AdapterConditionStatus, observedGen int32) { conditions := []api.AdapterCondition{ - {Type: conditionTypeAvailable, Status: available, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeAvailable, Status: available, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeApplied, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeHealth, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, } conditionsJSON, _ := json.Marshal(conditions) now := time.Now() @@ -796,7 +744,7 @@ func TestClusterSyntheticTimestampsStableWithoutAdapterStatus(t *testing.T) { fixedNow := time.Date(2026, 1, 1, 12, 0, 0, 0, time.UTC) initialConditions := []api.ResourceCondition{ { - Type: conditionTypeAvailable, + Type: api.ConditionTypeAvailable, Status: api.ConditionFalse, ObservedGeneration: 1, LastTransitionTime: fixedNow, @@ -804,7 +752,7 @@ func TestClusterSyntheticTimestampsStableWithoutAdapterStatus(t *testing.T) { LastUpdatedTime: fixedNow, }, { - Type: "Ready", + Type: api.ConditionTypeReady, Status: api.ConditionFalse, ObservedGeneration: 1, LastTransitionTime: fixedNow, @@ -829,9 +777,9 @@ func TestClusterSyntheticTimestampsStableWithoutAdapterStatus(t *testing.T) { var createdAvailable, createdReady *api.ResourceCondition for i := range createdConds { switch createdConds[i].Type { - case conditionTypeAvailable: + case api.ConditionTypeAvailable: createdAvailable = &createdConds[i] - case conditionTypeReady: + case api.ConditionTypeReady: createdReady = &createdConds[i] } } @@ -854,9 +802,9 @@ func TestClusterSyntheticTimestampsStableWithoutAdapterStatus(t *testing.T) { var updatedAvailable, updatedReady *api.ResourceCondition for i := range updatedConds { switch updatedConds[i].Type { - case conditionTypeAvailable: + case api.ConditionTypeAvailable: updatedAvailable = &updatedConds[i] - case conditionTypeReady: + case api.ConditionTypeReady: updatedReady = &updatedConds[i] } } @@ -869,3 +817,310 @@ func TestClusterSyntheticTimestampsStableWithoutAdapterStatus(t *testing.T) { Expect(updatedReady.LastTransitionTime).To(Equal(fixedNow)) Expect(updatedReady.LastUpdatedTime).To(Equal(fixedNow)) } + +// TestProcessAdapterStatus_MissingMandatoryCondition_Available tests that updates missing Available are rejected +func TestProcessAdapterStatus_MissingMandatoryCondition_Available(t *testing.T) { + RegisterTestingT(t) + + clusterDao := newMockClusterDao() + adapterStatusDao := newMockAdapterStatusDao() + config := testAdapterConfig() + service := NewClusterService(clusterDao, adapterStatusDao, config) + + ctx := context.Background() + clusterID := testClusterID + + // Create cluster first + cluster := &api.Cluster{Generation: 1} + cluster.ID = clusterID + _, svcErr := service.Create(ctx, cluster) + Expect(svcErr).To(BeNil()) + + // First, send a valid status + validConditions := []api.AdapterCondition{ + {Type: api.ConditionTypeAvailable, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeApplied, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeHealth, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + } + validConditionsJSON, _ := json.Marshal(validConditions) + now := time.Now() + validStatus := &api.AdapterStatus{ + ResourceType: "Cluster", + ResourceID: clusterID, + Adapter: "test-adapter", + Conditions: validConditionsJSON, + ObservedGeneration: 1, + CreatedTime: &now, + } + result, err := service.ProcessAdapterStatus(ctx, clusterID, validStatus) + Expect(err).To(BeNil()) + Expect(result).ToNot(BeNil()) + + // Now send an update without Available condition + invalidConditions := []api.AdapterCondition{ + {Type: api.ConditionTypeApplied, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeHealth, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: "CustomCondition", Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + } + invalidConditionsJSON, _ := json.Marshal(invalidConditions) + invalidStatus := &api.AdapterStatus{ + ResourceType: "Cluster", + ResourceID: clusterID, + Adapter: "test-adapter", + Conditions: invalidConditionsJSON, + ObservedGeneration: 2, + CreatedTime: &now, + } + + result, err = service.ProcessAdapterStatus(ctx, clusterID, invalidStatus) + + // Should be rejected (nil, nil) + Expect(err).To(BeNil()) + Expect(result).To(BeNil(), "Update missing Available condition should be rejected") + + // Verify old status is preserved + storedStatus, _ := adapterStatusDao.FindByResourceAndAdapter(ctx, "Cluster", clusterID, "test-adapter") + Expect(storedStatus).ToNot(BeNil()) + Expect(storedStatus.ObservedGeneration).To(Equal(int32(1)), "Old status should be preserved") + + var storedConditions []api.AdapterCondition + unmarshalErr := json.Unmarshal(storedStatus.Conditions, &storedConditions) + Expect(unmarshalErr).To(BeNil()) + Expect(len(storedConditions)).To(Equal(3)) + // Verify Available is still there + hasAvailable := false + for _, cond := range storedConditions { + if cond.Type == api.ConditionTypeAvailable { + hasAvailable = true + break + } + } + Expect(hasAvailable).To(BeTrue(), "Available condition should be preserved") +} + +// TestProcessAdapterStatus_AllMandatoryConditions_WithCustom tests that valid +// updates with all mandatory conditions are accepted +func TestProcessAdapterStatus_AllMandatoryConditions_WithCustom(t *testing.T) { + RegisterTestingT(t) + + clusterDao := newMockClusterDao() + adapterStatusDao := newMockAdapterStatusDao() + config := testAdapterConfig() + service := NewClusterService(clusterDao, adapterStatusDao, config) + + ctx := context.Background() + clusterID := testClusterID + + // Create cluster first + cluster := &api.Cluster{Generation: 1} + cluster.ID = clusterID + _, svcErr := service.Create(ctx, cluster) + Expect(svcErr).To(BeNil()) + + // Send status with all mandatory conditions + custom condition + conditions := []api.AdapterCondition{ + {Type: api.ConditionTypeAvailable, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeApplied, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeHealth, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: "CustomCondition", Status: api.AdapterConditionFalse, LastTransitionTime: time.Now()}, + } + conditionsJSON, _ := json.Marshal(conditions) + now := time.Now() + adapterStatus := &api.AdapterStatus{ + ResourceType: "Cluster", + ResourceID: clusterID, + Adapter: "test-adapter", + Conditions: conditionsJSON, + ObservedGeneration: 1, + CreatedTime: &now, + } + + result, err := service.ProcessAdapterStatus(ctx, clusterID, adapterStatus) + + // Should be accepted + Expect(err).To(BeNil()) + Expect(result).ToNot(BeNil(), "Update with all mandatory conditions should be accepted") + + // Verify status was stored with all 4 conditions + var storedConditions []api.AdapterCondition + unmarshalErr := json.Unmarshal(result.Conditions, &storedConditions) + Expect(unmarshalErr).To(BeNil()) + Expect(len(storedConditions)).To(Equal(4), "All 4 conditions should be stored") + + // Verify all conditions are present + conditionTypes := make(map[string]bool) + for _, cond := range storedConditions { + conditionTypes[cond.Type] = true + } + Expect(conditionTypes[api.ConditionTypeAvailable]).To(BeTrue()) + Expect(conditionTypes[api.ConditionTypeApplied]).To(BeTrue()) + Expect(conditionTypes[api.ConditionTypeHealth]).To(BeTrue()) + Expect(conditionTypes["CustomCondition"]).To(BeTrue()) +} + +// TestProcessAdapterStatus_CustomConditionRemoval tests that custom conditions can be removed +func TestProcessAdapterStatus_CustomConditionRemoval(t *testing.T) { + RegisterTestingT(t) + + clusterDao := newMockClusterDao() + adapterStatusDao := newMockAdapterStatusDao() + config := testAdapterConfig() + service := NewClusterService(clusterDao, adapterStatusDao, config) + + ctx := context.Background() + clusterID := testClusterID + + // Create cluster first + cluster := &api.Cluster{Generation: 1} + cluster.ID = clusterID + _, svcErr := service.Create(ctx, cluster) + Expect(svcErr).To(BeNil()) + + // First update: send all mandatory + custom condition + conditions1 := []api.AdapterCondition{ + {Type: api.ConditionTypeAvailable, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeApplied, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeHealth, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: "CustomCondition", Status: api.AdapterConditionFalse, LastTransitionTime: time.Now()}, + } + conditionsJSON1, _ := json.Marshal(conditions1) + now := time.Now() + adapterStatus1 := &api.AdapterStatus{ + ResourceType: "Cluster", + ResourceID: clusterID, + Adapter: "test-adapter", + Conditions: conditionsJSON1, + ObservedGeneration: 1, + CreatedTime: &now, + } + result1, err := service.ProcessAdapterStatus(ctx, clusterID, adapterStatus1) + Expect(err).To(BeNil()) + Expect(result1).ToNot(BeNil()) + + var storedConditions1 []api.AdapterCondition + unmarshalErr := json.Unmarshal(result1.Conditions, &storedConditions1) + Expect(unmarshalErr).To(BeNil()) + Expect(len(storedConditions1)).To(Equal(4)) + + // Second update: remove custom condition (only send mandatory conditions) + conditions2 := []api.AdapterCondition{ + {Type: api.ConditionTypeAvailable, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeApplied, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeHealth, Status: api.AdapterConditionFalse, LastTransitionTime: time.Now()}, + } + conditionsJSON2, _ := json.Marshal(conditions2) + adapterStatus2 := &api.AdapterStatus{ + ResourceType: "Cluster", + ResourceID: clusterID, + Adapter: "test-adapter", + Conditions: conditionsJSON2, + ObservedGeneration: 2, + CreatedTime: &now, + } + result2, err := service.ProcessAdapterStatus(ctx, clusterID, adapterStatus2) + Expect(err).To(BeNil()) + Expect(result2).ToNot(BeNil()) + + // Verify CustomCondition was removed + var storedConditions2 []api.AdapterCondition + unmarshalErr = json.Unmarshal(result2.Conditions, &storedConditions2) + Expect(unmarshalErr).To(BeNil()) + Expect(len(storedConditions2)).To(Equal(3), "CustomCondition should be removed") + + conditionTypes := make(map[string]bool) + for _, cond := range storedConditions2 { + conditionTypes[cond.Type] = true + } + Expect(conditionTypes[api.ConditionTypeAvailable]).To(BeTrue()) + Expect(conditionTypes[api.ConditionTypeApplied]).To(BeTrue()) + Expect(conditionTypes[api.ConditionTypeHealth]).To(BeTrue()) + Expect(conditionTypes["CustomCondition"]).To(BeFalse(), "CustomCondition should not be present") +} + +// TestProcessAdapterStatus_DuplicateCondition tests that updates with duplicate +// condition types are rejected +func TestProcessAdapterStatus_DuplicateCondition(t *testing.T) { + RegisterTestingT(t) + + clusterDao := newMockClusterDao() + adapterStatusDao := newMockAdapterStatusDao() + config := testAdapterConfig() + service := NewClusterService(clusterDao, adapterStatusDao, config) + + ctx := context.Background() + clusterID := testClusterID + + // Create cluster first + cluster := &api.Cluster{Generation: 1} + cluster.ID = clusterID + _, svcErr := service.Create(ctx, cluster) + Expect(svcErr).To(BeNil()) + + // Send status with duplicate Available condition + duplicateConditions := []api.AdapterCondition{ + {Type: api.ConditionTypeAvailable, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeApplied, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeHealth, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeAvailable, Status: api.AdapterConditionFalse, LastTransitionTime: time.Now()}, // Duplicate! + } + duplicateConditionsJSON, _ := json.Marshal(duplicateConditions) + now := time.Now() + duplicateStatus := &api.AdapterStatus{ + ResourceType: "Cluster", + ResourceID: clusterID, + Adapter: "test-adapter", + Conditions: duplicateConditionsJSON, + ObservedGeneration: 1, + CreatedTime: &now, + } + + result, err := service.ProcessAdapterStatus(ctx, clusterID, duplicateStatus) + + // Should be rejected (nil, nil) + Expect(err).To(BeNil()) + Expect(result).To(BeNil(), "Status with duplicate condition types should be rejected") +} + +// TestProcessAdapterStatus_EmptyConditionType tests that updates with empty +// condition types are rejected +func TestProcessAdapterStatus_EmptyConditionType(t *testing.T) { + RegisterTestingT(t) + + clusterDao := newMockClusterDao() + adapterStatusDao := newMockAdapterStatusDao() + config := testAdapterConfig() + service := NewClusterService(clusterDao, adapterStatusDao, config) + + ctx := context.Background() + clusterID := testClusterID + + // Create cluster first + cluster := &api.Cluster{Generation: 1} + cluster.ID = clusterID + _, svcErr := service.Create(ctx, cluster) + Expect(svcErr).To(BeNil()) + + // Send status with empty condition type + emptyTypeConditions := []api.AdapterCondition{ + {Type: api.ConditionTypeAvailable, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeApplied, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: "", Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, // Empty type! + {Type: api.ConditionTypeHealth, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + } + emptyTypeConditionsJSON, _ := json.Marshal(emptyTypeConditions) + now := time.Now() + emptyTypeStatus := &api.AdapterStatus{ + ResourceType: "Cluster", + ResourceID: clusterID, + Adapter: "test-adapter", + Conditions: emptyTypeConditionsJSON, + ObservedGeneration: 1, + CreatedTime: &now, + } + + result, err := service.ProcessAdapterStatus(ctx, clusterID, emptyTypeStatus) + + // Should be rejected (nil, nil) + Expect(err).To(BeNil()) + Expect(result).To(BeNil(), "Status with empty condition type should be rejected") +} diff --git a/pkg/services/node_pool.go b/pkg/services/node_pool.go index c341de4..123b13a 100644 --- a/pkg/services/node_pool.go +++ b/pkg/services/node_pool.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" stderrors "errors" + "fmt" "strings" "time" @@ -30,7 +31,8 @@ type NodePoolService interface { UpdateNodePoolStatusFromAdapters(ctx context.Context, nodePoolID string) (*api.NodePool, *errors.ServiceError) // ProcessAdapterStatus handles the business logic for adapter status: - // - If Available condition is "Unknown": returns (nil, nil) indicating no-op + // - First report: accepts Unknown Available condition, skips aggregation + // - Subsequent reports: rejects Unknown Available condition // - Otherwise: upserts the status and triggers aggregation ProcessAdapterStatus( ctx context.Context, nodePoolID string, adapterStatus *api.AdapterStatus, @@ -173,7 +175,7 @@ func (s *sqlNodePoolService) UpdateNodePoolStatusFromAdapters( // Find the "Available" condition var availableCondition *api.AdapterCondition for i := range conditions { - if conditions[i].Type == conditionTypeAvailable { + if conditions[i].Type == api.ConditionTypeAvailable { availableCondition = &conditions[i] break } @@ -238,9 +240,12 @@ func (s *sqlNodePoolService) UpdateNodePoolStatusFromAdapters( } // ProcessAdapterStatus handles the business logic for adapter status: -// - If Available is "Unknown" and a status already exists: returns (nil, nil) as no-op -// - If Available is "Unknown" and no status exists (first report): upserts the status -// - Otherwise: upserts the status and triggers aggregation +// - Validates that all mandatory conditions (Available, Applied, Health) are present +// - Rejects duplicate condition types +// - For first status report: accepts Unknown Available condition to avoid data loss +// - For subsequent reports: rejects Unknown Available condition to preserve existing valid state +// - Uses complete replacement semantics: each update replaces all conditions for this adapter +// - Returns (nil, nil) for discarded updates func (s *sqlNodePoolService) ProcessAdapterStatus( ctx context.Context, nodePoolID string, adapterStatus *api.AdapterStatus, ) (*api.AdapterStatus, *errors.ServiceError) { @@ -265,35 +270,45 @@ func (s *sqlNodePoolService) ProcessAdapterStatus( } } - // Find the "Available" condition - hasAvailableCondition := false + // Validate mandatory conditions and check for duplicates + if errorType, conditionName := ValidateMandatoryConditions(conditions); errorType != "" { + logger.With(ctx, logger.FieldNodePoolID, nodePoolID). + Info(fmt.Sprintf("Discarding adapter status update from %s: %s condition %s", + adapterStatus.Adapter, errorType, conditionName)) + return nil, nil + } + + // Check Available condition for Unknown status + triggerAggregation := false for _, cond := range conditions { - if cond.Type != conditionTypeAvailable { + if cond.Type != api.ConditionTypeAvailable { continue } - hasAvailableCondition = true + triggerAggregation = true if cond.Status == api.AdapterConditionUnknown { if existingStatus != nil { - // Available condition is "Unknown" and a status already exists, return nil to indicate no-op + // Non-first report && Available=Unknown → reject + logger.With(ctx, logger.FieldNodePoolID, nodePoolID). + Info(fmt.Sprintf("Discarding adapter status update from %s: subsequent Unknown Available", + adapterStatus.Adapter)) return nil, nil } // First report from this adapter: allow storing even with Available=Unknown // but skip aggregation since Unknown should not affect nodepool-level conditions - hasAvailableCondition = false + triggerAggregation = false } + break } - // Upsert the adapter status + // Upsert the adapter status (complete replacement) upsertedStatus, err := s.adapterStatusDao.Upsert(ctx, adapterStatus) if err != nil { return nil, handleCreateError("AdapterStatus", err) } - // Only trigger aggregation when the adapter reported an Available condition. - // If the adapter status doesn't include Available, saving it should not overwrite - // the nodepool's synthetic Available/Ready conditions. - if hasAvailableCondition { + // Only trigger aggregation when triggerAggregation is true + if triggerAggregation { if _, aggregateErr := s.UpdateNodePoolStatusFromAdapters( ctx, nodePoolID, ); aggregateErr != nil { diff --git a/pkg/services/node_pool_test.go b/pkg/services/node_pool_test.go index 9c5f39e..a61d5cf 100644 --- a/pkg/services/node_pool_test.go +++ b/pkg/services/node_pool_test.go @@ -93,13 +93,23 @@ func TestNodePoolProcessAdapterStatus_FirstUnknownCondition(t *testing.T) { ctx := context.Background() nodePoolID := testNodePoolID - // Create adapter status with Available=Unknown + // Create first adapter status with all mandatory conditions but Available=Unknown conditions := []api.AdapterCondition{ { - Type: conditionTypeAvailable, + Type: api.ConditionTypeAvailable, Status: api.AdapterConditionUnknown, LastTransitionTime: time.Now(), }, + { + Type: api.ConditionTypeApplied, + Status: api.AdapterConditionTrue, + LastTransitionTime: time.Now(), + }, + { + Type: api.ConditionTypeHealth, + Status: api.AdapterConditionTrue, + LastTransitionTime: time.Now(), + }, } conditionsJSON, _ := json.Marshal(conditions) @@ -115,7 +125,7 @@ func TestNodePoolProcessAdapterStatus_FirstUnknownCondition(t *testing.T) { result, err := service.ProcessAdapterStatus(ctx, nodePoolID, adapterStatus) Expect(err).To(BeNil()) - Expect(result).ToNot(BeNil(), "First report with Available=Unknown should be stored") + Expect(result).ToNot(BeNil(), "First report with Available=Unknown should be accepted") Expect(result.Adapter).To(Equal("test-adapter")) // Verify the status was stored @@ -138,11 +148,9 @@ func TestNodePoolProcessAdapterStatus_SubsequentUnknownCondition(t *testing.T) { // Pre-populate an existing adapter status conditions := []api.AdapterCondition{ - { - Type: conditionTypeAvailable, - Status: api.AdapterConditionUnknown, - LastTransitionTime: time.Now(), - }, + {Type: api.ConditionTypeAvailable, Status: api.AdapterConditionUnknown, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeApplied, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeHealth, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, } conditionsJSON, _ := json.Marshal(conditions) @@ -192,10 +200,20 @@ func TestNodePoolProcessAdapterStatus_TrueCondition(t *testing.T) { _, svcErr := service.Create(ctx, nodePool) Expect(svcErr).To(BeNil()) - // Create adapter status with Available=True + // Create adapter status with all mandatory conditions conditions := []api.AdapterCondition{ { - Type: conditionTypeAvailable, + Type: api.ConditionTypeAvailable, + Status: api.AdapterConditionTrue, + LastTransitionTime: time.Now(), + }, + { + Type: api.ConditionTypeApplied, + Status: api.AdapterConditionTrue, + LastTransitionTime: time.Now(), + }, + { + Type: api.ConditionTypeHealth, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now(), }, @@ -222,8 +240,8 @@ func TestNodePoolProcessAdapterStatus_TrueCondition(t *testing.T) { Expect(len(storedStatuses)).To(Equal(1), "Status should be stored for True condition") } -// TestNodePoolProcessAdapterStatus_FirstMultipleConditions_AvailableUnknown tests that the first report -// with multiple conditions including Available=Unknown is stored +// TestNodePoolProcessAdapterStatus_FirstMultipleConditions_AvailableUnknown tests that first reports +// with Available=Unknown are accepted even when other conditions are present func TestNodePoolProcessAdapterStatus_FirstMultipleConditions_AvailableUnknown(t *testing.T) { RegisterTestingT(t) @@ -236,16 +254,26 @@ func TestNodePoolProcessAdapterStatus_FirstMultipleConditions_AvailableUnknown(t ctx := context.Background() nodePoolID := testNodePoolID - // Create adapter status with multiple conditions including Available=Unknown + // Create first adapter status with all mandatory conditions but Available=Unknown conditions := []api.AdapterCondition{ { - Type: conditionTypeReady, + Type: api.ConditionTypeAvailable, + Status: api.AdapterConditionUnknown, + LastTransitionTime: time.Now(), + }, + { + Type: api.ConditionTypeApplied, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now(), }, { - Type: conditionTypeAvailable, - Status: api.AdapterConditionUnknown, + Type: api.ConditionTypeHealth, + Status: api.AdapterConditionTrue, + LastTransitionTime: time.Now(), + }, + { + Type: api.ConditionTypeReady, + Status: api.AdapterConditionTrue, LastTransitionTime: time.Now(), }, } @@ -263,11 +291,11 @@ func TestNodePoolProcessAdapterStatus_FirstMultipleConditions_AvailableUnknown(t result, err := service.ProcessAdapterStatus(ctx, nodePoolID, adapterStatus) Expect(err).To(BeNil()) - Expect(result).ToNot(BeNil(), "First report with Available=Unknown should be stored") + Expect(result).ToNot(BeNil(), "First report with Available=Unknown should be accepted") // Verify the status was stored storedStatuses, _ := adapterStatusDao.FindByResource(ctx, "NodePool", nodePoolID) - Expect(len(storedStatuses)).To(Equal(1), "First Unknown status should be stored") + Expect(len(storedStatuses)).To(Equal(1), "First status with Available=Unknown should be stored") } // TestNodePoolProcessAdapterStatus_SubsequentMultipleConditions_AvailableUnknown tests that subsequent @@ -286,11 +314,9 @@ func TestNodePoolProcessAdapterStatus_SubsequentMultipleConditions_AvailableUnkn // Pre-populate an existing adapter status existingConditions := []api.AdapterCondition{ - { - Type: conditionTypeAvailable, - Status: api.AdapterConditionUnknown, - LastTransitionTime: time.Now(), - }, + {Type: api.ConditionTypeAvailable, Status: api.AdapterConditionUnknown, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeApplied, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeHealth, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, } existingConditionsJSON, _ := json.Marshal(existingConditions) @@ -306,16 +332,11 @@ func TestNodePoolProcessAdapterStatus_SubsequentMultipleConditions_AvailableUnkn // Now send another report with multiple conditions including Available=Unknown conditions := []api.AdapterCondition{ - { - Type: conditionTypeReady, - Status: api.AdapterConditionTrue, - LastTransitionTime: time.Now(), - }, - { - Type: conditionTypeAvailable, - Status: api.AdapterConditionUnknown, - LastTransitionTime: time.Now(), - }, + {Type: api.ConditionTypeAvailable, Status: api.AdapterConditionUnknown, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeApplied, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeHealth, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeReady, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: "Progressing", Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, } conditionsJSON, _ := json.Marshal(conditions) @@ -362,9 +383,9 @@ func TestNodePoolAvailableReadyTransitions(t *testing.T) { var available, ready *api.ResourceCondition for i := range conds { switch conds[i].Type { - case conditionTypeAvailable: + case api.ConditionTypeAvailable: available = &conds[i] - case conditionTypeReady: + case api.ConditionTypeReady: ready = &conds[i] } } @@ -375,7 +396,9 @@ func TestNodePoolAvailableReadyTransitions(t *testing.T) { upsert := func(adapter string, available api.AdapterConditionStatus, observedGen int32) { conditions := []api.AdapterCondition{ - {Type: conditionTypeAvailable, Status: available, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeAvailable, Status: available, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeApplied, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeHealth, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, } conditionsJSON, _ := json.Marshal(conditions) now := time.Now() @@ -447,11 +470,11 @@ func TestNodePoolAvailableReadyTransitions(t *testing.T) { Expect(avail.ObservedGeneration).To(Equal(int32(0))) Expect(ready.Status).To(Equal(api.ConditionFalse)) - // Adapter status with no Available condition should not overwrite synthetic conditions. + // Adapter status missing mandatory conditions should be rejected and not overwrite synthetic conditions. prevStatus := api.NodePool{}.StatusConditions prevStatus = append(prevStatus, nodePoolDao.nodePools[nodePoolID].StatusConditions...) nonAvailableConds := []api.AdapterCondition{ - {Type: "Health", Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeHealth, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, } nonAvailableJSON, _ := json.Marshal(nonAvailableConds) nonAvailableStatus := &api.AdapterStatus{ @@ -463,14 +486,16 @@ func TestNodePoolAvailableReadyTransitions(t *testing.T) { } result, svcErr := service.ProcessAdapterStatus(ctx, nodePoolID, nonAvailableStatus) Expect(svcErr).To(BeNil()) - Expect(result).ToNot(BeNil()) + Expect(result).To(BeNil(), "Update missing mandatory conditions should be rejected") Expect(nodePoolDao.nodePools[nodePoolID].StatusConditions).To(Equal(prevStatus)) // Available=Unknown is a no-op (does not store, does not overwrite nodepool conditions). prevStatus = api.NodePool{}.StatusConditions prevStatus = append(prevStatus, nodePoolDao.nodePools[nodePoolID].StatusConditions...) unknownConds := []api.AdapterCondition{ - {Type: conditionTypeAvailable, Status: api.AdapterConditionUnknown, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeAvailable, Status: api.AdapterConditionUnknown, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeApplied, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeHealth, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, } unknownJSON, _ := json.Marshal(unknownConds) unknownStatus := &api.AdapterStatus{ @@ -511,7 +536,7 @@ func TestNodePoolStaleAdapterStatusUpdatePolicy(t *testing.T) { var conds []api.ResourceCondition Expect(json.Unmarshal(stored.StatusConditions, &conds)).To(Succeed()) for i := range conds { - if conds[i].Type == conditionTypeAvailable { + if conds[i].Type == api.ConditionTypeAvailable { return conds[i] } } @@ -521,7 +546,9 @@ func TestNodePoolStaleAdapterStatusUpdatePolicy(t *testing.T) { upsert := func(adapter string, available api.AdapterConditionStatus, observedGen int32) { conditions := []api.AdapterCondition{ - {Type: conditionTypeAvailable, Status: available, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeAvailable, Status: available, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeApplied, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeHealth, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, } conditionsJSON, _ := json.Marshal(conditions) now := time.Now() @@ -577,7 +604,7 @@ func TestNodePoolSyntheticTimestampsStableWithoutAdapterStatus(t *testing.T) { fixedNow := time.Date(2026, 1, 1, 12, 0, 0, 0, time.UTC) initialConditions := []api.ResourceCondition{ { - Type: conditionTypeAvailable, + Type: api.ConditionTypeAvailable, Status: api.ConditionFalse, ObservedGeneration: 1, LastTransitionTime: fixedNow, @@ -585,7 +612,7 @@ func TestNodePoolSyntheticTimestampsStableWithoutAdapterStatus(t *testing.T) { LastUpdatedTime: fixedNow, }, { - Type: conditionTypeReady, + Type: api.ConditionTypeReady, Status: api.ConditionFalse, ObservedGeneration: 1, LastTransitionTime: fixedNow, @@ -610,9 +637,9 @@ func TestNodePoolSyntheticTimestampsStableWithoutAdapterStatus(t *testing.T) { var createdAvailable, createdReady *api.ResourceCondition for i := range createdConds { switch createdConds[i].Type { - case conditionTypeAvailable: + case api.ConditionTypeAvailable: createdAvailable = &createdConds[i] - case conditionTypeReady: + case api.ConditionTypeReady: createdReady = &createdConds[i] } } @@ -635,9 +662,9 @@ func TestNodePoolSyntheticTimestampsStableWithoutAdapterStatus(t *testing.T) { var updatedAvailable, updatedReady *api.ResourceCondition for i := range updatedConds { switch updatedConds[i].Type { - case conditionTypeAvailable: + case api.ConditionTypeAvailable: updatedAvailable = &updatedConds[i] - case conditionTypeReady: + case api.ConditionTypeReady: updatedReady = &updatedConds[i] } } diff --git a/pkg/services/status_aggregation.go b/pkg/services/status_aggregation.go index b1a155b..9afa275 100644 --- a/pkg/services/status_aggregation.go +++ b/pkg/services/status_aggregation.go @@ -10,9 +10,13 @@ import ( "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" ) +// Mandatory condition types that must be present in all adapter status updates +var mandatoryConditionTypes = []string{api.ConditionTypeAvailable, api.ConditionTypeApplied, api.ConditionTypeHealth} + +// Condition validation error types const ( - conditionTypeAvailable = "Available" - conditionTypeReady = "Ready" + ConditionValidationErrorDuplicate = "duplicate" + ConditionValidationErrorMissing = "missing" ) // Required adapter lists configured via pkg/config/adapter.go (see AdapterRequirementsConfig) @@ -26,6 +30,35 @@ var adapterConditionSuffixMap = map[string]string{ // Add custom mappings here when needed } +// ValidateMandatoryConditions checks if all mandatory conditions are present and rejects duplicate condition types. +// Returns (errorType, conditionName) where: +// - If duplicate found: (ConditionValidationErrorDuplicate, duplicateConditionType) +// - If missing condition: (ConditionValidationErrorMissing, missingConditionType) +// - If all valid: ("", "") +func ValidateMandatoryConditions(conditions []api.AdapterCondition) (errorType, conditionName string) { + // Check for duplicate condition types + seen := make(map[string]bool) + for _, cond := range conditions { + // Reject empty condition types + if cond.Type == "" { + return ConditionValidationErrorMissing, "" + } + if seen[cond.Type] { + return ConditionValidationErrorDuplicate, cond.Type + } + seen[cond.Type] = true + } + + // Check that all mandatory conditions are present + for _, mandatoryType := range mandatoryConditionTypes { + if !seen[mandatoryType] { + return ConditionValidationErrorMissing, mandatoryType + } + } + + return "", "" +} + // MapAdapterToConditionType converts an adapter name to a semantic condition type // by converting the adapter name to PascalCase and appending a suffix. // @@ -87,7 +120,7 @@ func ComputeAvailableCondition(adapterStatuses api.AdapterStatusList, requiredAd if len(adapterStatus.Conditions) > 0 { if err := json.Unmarshal(adapterStatus.Conditions, &conditions); err == nil { for _, cond := range conditions { - if cond.Type == conditionTypeAvailable { + if cond.Type == api.ConditionTypeAvailable { adapterMap[adapterStatus.Adapter] = struct { available string observedGeneration int32 @@ -158,7 +191,7 @@ func ComputeReadyCondition( if len(adapterStatus.Conditions) > 0 { if err := json.Unmarshal(adapterStatus.Conditions, &conditions); err == nil { for _, cond := range conditions { - if cond.Type == conditionTypeAvailable { + if cond.Type == api.ConditionTypeAvailable { adapterMap[adapterStatus.Adapter] = struct { available string observedGeneration int32 @@ -216,9 +249,9 @@ func BuildSyntheticConditions( if err := json.Unmarshal(existingConditionsJSON, &existingConditions); err == nil { for i := range existingConditions { switch existingConditions[i].Type { - case conditionTypeAvailable: + case api.ConditionTypeAvailable: existingAvailable = &existingConditions[i] - case conditionTypeReady: + case api.ConditionTypeReady: existingReady = &existingConditions[i] } } @@ -231,7 +264,7 @@ func BuildSyntheticConditions( availableStatus = api.ConditionTrue } availableCondition := api.ResourceCondition{ - Type: conditionTypeAvailable, + Type: api.ConditionTypeAvailable, Status: availableStatus, ObservedGeneration: minObservedGeneration, LastTransitionTime: now, @@ -246,7 +279,7 @@ func BuildSyntheticConditions( readyStatus = api.ConditionTrue } readyCondition := api.ResourceCondition{ - Type: conditionTypeReady, + Type: api.ConditionTypeReady, Status: readyStatus, ObservedGeneration: resourceGeneration, LastTransitionTime: now, diff --git a/pkg/services/status_aggregation_test.go b/pkg/services/status_aggregation_test.go index 56eedc6..81f8e1b 100644 --- a/pkg/services/status_aggregation_test.go +++ b/pkg/services/status_aggregation_test.go @@ -1,6 +1,11 @@ package services -import "testing" +import ( + "testing" + "time" + + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" +) func TestMapAdapterToConditionType(t *testing.T) { tests := []struct { @@ -51,3 +56,149 @@ func TestMapAdapterToConditionType_DefaultAfterCustom(t *testing.T) { "dns", result, expected) } } + +func TestValidateMandatoryConditions_AllPresent(t *testing.T) { + conditions := []api.AdapterCondition{ + {Type: api.ConditionTypeAvailable, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeApplied, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeHealth, Status: api.AdapterConditionFalse, LastTransitionTime: time.Now()}, + } + + errorType, conditionName := ValidateMandatoryConditions(conditions) + if errorType != "" { + t.Errorf("Expected no errors, got errorType: %s, conditionName: %s", errorType, conditionName) + } +} + +func TestValidateMandatoryConditions_MissingAvailable(t *testing.T) { + conditions := []api.AdapterCondition{ + {Type: api.ConditionTypeApplied, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeHealth, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + } + + errorType, conditionName := ValidateMandatoryConditions(conditions) + if errorType != ConditionValidationErrorMissing { + t.Errorf("Expected errorType ConditionValidationErrorMissing, got: %s", errorType) + } + if conditionName != api.ConditionTypeAvailable { + t.Errorf("Expected missing condition %s, got: %s", api.ConditionTypeAvailable, conditionName) + } +} + +func TestValidateMandatoryConditions_MandatoryConditionUnknown(t *testing.T) { + // Unknown status in Applied/Health is allowed; only Available=Unknown has special handling elsewhere + conditions := []api.AdapterCondition{ + {Type: api.ConditionTypeAvailable, Status: api.AdapterConditionUnknown, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeApplied, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeHealth, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + } + + errorType, conditionName := ValidateMandatoryConditions(conditions) + if errorType != "" { + t.Errorf("Expected no errors (Unknown is allowed), got errorType: %s, conditionName: %s", errorType, conditionName) + } +} + +func TestValidateMandatoryConditions_WithCustomConditions(t *testing.T) { + conditions := []api.AdapterCondition{ + {Type: api.ConditionTypeAvailable, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeApplied, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeHealth, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: "CustomCondition", Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeReady, Status: api.AdapterConditionFalse, LastTransitionTime: time.Now()}, + } + + errorType, conditionName := ValidateMandatoryConditions(conditions) + if errorType != "" { + t.Errorf("Expected no errors, got errorType: %s, conditionName: %s", errorType, conditionName) + } +} + +func TestValidateMandatoryConditions_EmptyConditions(t *testing.T) { + conditions := []api.AdapterCondition{} + + errorType, conditionName := ValidateMandatoryConditions(conditions) + if errorType != ConditionValidationErrorMissing { + t.Errorf("Expected errorType ConditionValidationErrorMissing, got: %s", errorType) + } + if conditionName != api.ConditionTypeAvailable { + t.Errorf("Expected missing condition %s, got: %s", api.ConditionTypeAvailable, conditionName) + } +} + +func TestValidateMandatoryConditions_DuplicateCondition(t *testing.T) { + // Test: If Available appears twice, should reject as duplicate + conditions := []api.AdapterCondition{ + {Type: api.ConditionTypeAvailable, Status: api.AdapterConditionUnknown, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeApplied, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeHealth, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeAvailable, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, // Duplicate! + } + + errorType, conditionName := ValidateMandatoryConditions(conditions) + + if errorType != ConditionValidationErrorDuplicate { + t.Errorf("Expected errorType ConditionValidationErrorDuplicate, got: %s", errorType) + } + if conditionName != api.ConditionTypeAvailable { + t.Errorf("Expected duplicate condition %s, got: %s", api.ConditionTypeAvailable, conditionName) + } +} + +func TestValidateMandatoryConditions_DuplicateCustomCondition(t *testing.T) { + // Test: Duplicate custom condition should also be rejected + conditions := []api.AdapterCondition{ + {Type: api.ConditionTypeAvailable, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeApplied, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeHealth, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: "CustomCondition", Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: "CustomCondition", Status: api.AdapterConditionFalse, LastTransitionTime: time.Now()}, // Duplicate! + } + + errorType, conditionName := ValidateMandatoryConditions(conditions) + + if errorType != ConditionValidationErrorDuplicate { + t.Errorf("Expected errorType ConditionValidationErrorDuplicate, got: %s", errorType) + } + if conditionName != "CustomCondition" { + t.Errorf("Expected duplicate condition CustomCondition, got: %s", conditionName) + } +} + +// TestValidateMandatoryConditions_MissingMultiple tests that when multiple conditions are missing, +// the function returns the first missing one +func TestValidateMandatoryConditions_MissingMultiple(t *testing.T) { + // Test: Only Available present, missing Applied and Health + conditions := []api.AdapterCondition{ + {Type: api.ConditionTypeAvailable, Status: api.AdapterConditionUnknown, LastTransitionTime: time.Now()}, + } + + errorType, conditionName := ValidateMandatoryConditions(conditions) + + // Should return missing condition + if errorType != ConditionValidationErrorMissing { + t.Errorf("Expected errorType ConditionValidationErrorMissing, got: %s", errorType) + } + if conditionName != api.ConditionTypeApplied && conditionName != api.ConditionTypeHealth { + t.Errorf("Expected missing condition to be Applied or Health, got: %s", conditionName) + } +} + +func TestValidateMandatoryConditions_EmptyConditionType(t *testing.T) { + // Test: Condition with empty type should be rejected + conditions := []api.AdapterCondition{ + {Type: api.ConditionTypeAvailable, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeApplied, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: "", Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, // Empty type + {Type: api.ConditionTypeHealth, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + } + + errorType, conditionName := ValidateMandatoryConditions(conditions) + + if errorType != ConditionValidationErrorMissing { + t.Errorf("Expected errorType ConditionValidationErrorMissing, got: %s", errorType) + } + if conditionName != "" { + t.Errorf("Expected conditionName '', got: %s", conditionName) + } +} diff --git a/test/integration/adapter_status_test.go b/test/integration/adapter_status_test.go index 7e55206..14d5a5a 100644 --- a/test/integration/adapter_status_test.go +++ b/test/integration/adapter_status_test.go @@ -8,6 +8,7 @@ import ( . "github.com/onsi/gomega" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/util" "github.com/openshift-hyperfleet/hyperfleet-api/test" @@ -46,7 +47,22 @@ func TestClusterStatusPost(t *testing.T) { cluster.Generation, []openapi.ConditionRequest{ { - Type: "Ready", + Type: api.ConditionTypeAvailable, + Status: openapi.AdapterConditionStatusTrue, + Reason: util.PtrString("ClusterAvailable"), + }, + { + Type: api.ConditionTypeApplied, + Status: openapi.AdapterConditionStatusTrue, + Reason: util.PtrString("ConfigurationApplied"), + }, + { + Type: api.ConditionTypeHealth, + Status: openapi.AdapterConditionStatusTrue, + Reason: util.PtrString("HealthyCluster"), + }, + { + Type: api.ConditionTypeReady, Status: openapi.AdapterConditionStatusTrue, Reason: util.PtrString("AdapterReady"), }, @@ -84,7 +100,19 @@ func TestClusterStatusGet(t *testing.T) { cluster.Generation, []openapi.ConditionRequest{ { - Type: "Ready", + Type: api.ConditionTypeAvailable, + Status: openapi.AdapterConditionStatusTrue, + }, + { + Type: api.ConditionTypeApplied, + Status: openapi.AdapterConditionStatusTrue, + }, + { + Type: api.ConditionTypeHealth, + Status: openapi.AdapterConditionStatusTrue, + }, + { + Type: api.ConditionTypeReady, Status: openapi.AdapterConditionStatusTrue, }, }, @@ -144,7 +172,22 @@ func TestNodePoolStatusPost(t *testing.T) { 1, []openapi.ConditionRequest{ { - Type: "Ready", + Type: api.ConditionTypeAvailable, + Status: openapi.AdapterConditionStatusFalse, + Reason: util.PtrString("Initializing"), + }, + { + Type: api.ConditionTypeApplied, + Status: openapi.AdapterConditionStatusTrue, + Reason: util.PtrString("ConfigurationApplied"), + }, + { + Type: api.ConditionTypeHealth, + Status: openapi.AdapterConditionStatusFalse, + Reason: util.PtrString("Initializing"), + }, + { + Type: api.ConditionTypeReady, Status: openapi.AdapterConditionStatusFalse, Reason: util.PtrString("Initializing"), }, @@ -182,7 +225,19 @@ func TestNodePoolStatusGet(t *testing.T) { 1, []openapi.ConditionRequest{ { - Type: "Ready", + Type: api.ConditionTypeAvailable, + Status: openapi.AdapterConditionStatusTrue, + }, + { + Type: api.ConditionTypeApplied, + Status: openapi.AdapterConditionStatusTrue, + }, + { + Type: api.ConditionTypeHealth, + Status: openapi.AdapterConditionStatusTrue, + }, + { + Type: api.ConditionTypeReady, Status: openapi.AdapterConditionStatusTrue, }, }, @@ -222,7 +277,19 @@ func TestAdapterStatusPaging(t *testing.T) { cluster.Generation, []openapi.ConditionRequest{ { - Type: "Ready", + Type: api.ConditionTypeAvailable, + Status: openapi.AdapterConditionStatusTrue, + }, + { + Type: api.ConditionTypeApplied, + Status: openapi.AdapterConditionStatusTrue, + }, + { + Type: api.ConditionTypeHealth, + Status: openapi.AdapterConditionStatusTrue, + }, + { + Type: api.ConditionTypeReady, Status: openapi.AdapterConditionStatusTrue, }, }, @@ -269,7 +336,22 @@ func TestAdapterStatusIdempotency(t *testing.T) { cluster.Generation, []openapi.ConditionRequest{ { - Type: "Ready", + Type: api.ConditionTypeAvailable, + Status: openapi.AdapterConditionStatusFalse, + Reason: util.PtrString("Initializing"), + }, + { + Type: api.ConditionTypeApplied, + Status: openapi.AdapterConditionStatusTrue, + Reason: util.PtrString("ConfigurationApplied"), + }, + { + Type: api.ConditionTypeHealth, + Status: openapi.AdapterConditionStatusFalse, + Reason: util.PtrString("Initializing"), + }, + { + Type: api.ConditionTypeReady, Status: openapi.AdapterConditionStatusFalse, Reason: util.PtrString("Initializing"), }, @@ -296,7 +378,22 @@ func TestAdapterStatusIdempotency(t *testing.T) { cluster.Generation, []openapi.ConditionRequest{ { - Type: "Ready", + Type: api.ConditionTypeAvailable, + Status: openapi.AdapterConditionStatusTrue, + Reason: util.PtrString("ClusterAvailable"), + }, + { + Type: api.ConditionTypeApplied, + Status: openapi.AdapterConditionStatusTrue, + Reason: util.PtrString("ConfigurationApplied"), + }, + { + Type: api.ConditionTypeHealth, + Status: openapi.AdapterConditionStatusTrue, + Reason: util.PtrString("HealthyCluster"), + }, + { + Type: api.ConditionTypeReady, Status: openapi.AdapterConditionStatusTrue, Reason: util.PtrString("AdapterReady"), }, @@ -335,9 +432,9 @@ func TestAdapterStatusIdempotency(t *testing.T) { To(Equal(openapi.AdapterConditionStatusTrue), "Conditions should be updated to latest") } -// TestClusterStatusPost_UnknownReturns201ThenSubsequent204 tests that the first Unknown Available -// status report is stored (201), while subsequent Unknown reports return 204 No Content. -func TestClusterStatusPost_UnknownReturns201ThenSubsequent204(t *testing.T) { +// TestClusterStatusPost_FirstUnknownAccepted tests that first status reports with Unknown +// Available condition are accepted, subsequent ones are rejected (HYPERFLEET-657) +func TestClusterStatusPost_FirstUnknownAccepted(t *testing.T) { h, client := test.RegisterIntegration(t) account := h.NewRandAccount() @@ -347,30 +444,40 @@ func TestClusterStatusPost_UnknownReturns201ThenSubsequent204(t *testing.T) { cluster, err := h.Factories.NewClusters(h.NewID()) Expect(err).NotTo(HaveOccurred()) - // Create an adapter status with Available=Unknown + // Create an adapter status with all mandatory conditions but Available=Unknown statusInput := newAdapterStatusRequest( "test-adapter-unknown", cluster.Generation, []openapi.ConditionRequest{ { - Type: "Available", + Type: api.ConditionTypeAvailable, Status: openapi.AdapterConditionStatusUnknown, Reason: util.PtrString("StartupPending"), }, + { + Type: api.ConditionTypeApplied, + Status: openapi.AdapterConditionStatusTrue, + Reason: util.PtrString("ConfigurationApplied"), + }, + { + Type: api.ConditionTypeHealth, + Status: openapi.AdapterConditionStatusTrue, + Reason: util.PtrString("HealthyCluster"), + }, }, nil, ) - // First report: should be stored (201 Created) + // First report with Unknown Available condition: should be accepted resp, err := client.PostClusterStatusesWithResponse( ctx, cluster.ID, openapi.PostClusterStatusesJSONRequestBody(statusInput), test.WithAuthToken(ctx), ) Expect(err).NotTo(HaveOccurred(), "Error posting cluster status: %v", err) Expect(resp.StatusCode()). - To(Equal(http.StatusCreated), "Expected 201 Created for first Unknown status report") + To(Equal(http.StatusCreated), "Expected 201 Created for first status with Unknown Available condition") - // Verify the status was stored + // Verify status was stored listResp, err := client.GetClusterStatusesWithResponse(ctx, cluster.ID, nil, test.WithAuthToken(ctx)) Expect(err).NotTo(HaveOccurred()) Expect(listResp.JSON200).NotTo(BeNil()) @@ -382,9 +489,9 @@ func TestClusterStatusPost_UnknownReturns201ThenSubsequent204(t *testing.T) { break } } - Expect(found).To(BeTrue(), "First Unknown status report should be stored") + Expect(found).To(BeTrue(), "First status with Unknown Available condition should be stored") - // Subsequent report with same adapter: should be no-op (204 No Content) + // Subsequent report with same adapter: should be rejected (204 No Content) resp2, err := client.PostClusterStatusesWithResponse( ctx, cluster.ID, openapi.PostClusterStatusesJSONRequestBody(statusInput), test.WithAuthToken(ctx), @@ -394,9 +501,9 @@ func TestClusterStatusPost_UnknownReturns201ThenSubsequent204(t *testing.T) { To(Equal(http.StatusNoContent), "Expected 204 No Content for subsequent Unknown status report") } -// TestNodePoolStatusPost_UnknownReturns201ThenSubsequent204 tests that the first Unknown Available -// status report is stored (201), while subsequent Unknown reports return 204 No Content. -func TestNodePoolStatusPost_UnknownReturns201ThenSubsequent204(t *testing.T) { +// TestNodePoolStatusPost_FirstUnknownAccepted tests that first status reports with Unknown +// Available condition are accepted, subsequent ones are rejected (HYPERFLEET-657) +func TestNodePoolStatusPost_FirstUnknownAccepted(t *testing.T) { h, client := test.RegisterIntegration(t) account := h.NewRandAccount() @@ -406,30 +513,40 @@ func TestNodePoolStatusPost_UnknownReturns201ThenSubsequent204(t *testing.T) { nodePool, err := h.Factories.NewNodePools(h.NewID()) Expect(err).NotTo(HaveOccurred()) - // Create an adapter status with Available=Unknown + // Create an adapter status with all mandatory conditions but Available=Unknown statusInput := newAdapterStatusRequest( "test-nodepool-adapter-unknown", 1, []openapi.ConditionRequest{ { - Type: "Available", + Type: api.ConditionTypeAvailable, Status: openapi.AdapterConditionStatusUnknown, Reason: util.PtrString("StartupPending"), }, + { + Type: api.ConditionTypeApplied, + Status: openapi.AdapterConditionStatusTrue, + Reason: util.PtrString("ConfigurationApplied"), + }, + { + Type: api.ConditionTypeHealth, + Status: openapi.AdapterConditionStatusTrue, + Reason: util.PtrString("HealthyCluster"), + }, }, nil, ) - // First report: should be stored (201 Created) + // First report with Unknown Available condition: should be accepted resp, err := client.PostNodePoolStatusesWithResponse( ctx, nodePool.OwnerID, nodePool.ID, openapi.PostNodePoolStatusesJSONRequestBody(statusInput), test.WithAuthToken(ctx), ) Expect(err).NotTo(HaveOccurred(), "Error posting nodepool status: %v", err) Expect(resp.StatusCode()). - To(Equal(http.StatusCreated), "Expected 201 Created for first Unknown status report") + To(Equal(http.StatusCreated), "Expected 201 Created for first status with Unknown Available condition") - // Verify the status was stored + // Verify status was stored listResp, err := client.GetNodePoolsStatusesWithResponse( ctx, nodePool.OwnerID, nodePool.ID, nil, test.WithAuthToken(ctx), ) @@ -443,9 +560,9 @@ func TestNodePoolStatusPost_UnknownReturns201ThenSubsequent204(t *testing.T) { break } } - Expect(found).To(BeTrue(), "First Unknown status report should be stored") + Expect(found).To(BeTrue(), "First status with Unknown Available condition should be stored") - // Subsequent report with same adapter: should be no-op (204 No Content) + // Subsequent report with same adapter: should be rejected (204 No Content) resp2, err := client.PostNodePoolStatusesWithResponse( ctx, nodePool.OwnerID, nodePool.ID, openapi.PostNodePoolStatusesJSONRequestBody(statusInput), test.WithAuthToken(ctx), @@ -456,7 +573,7 @@ func TestNodePoolStatusPost_UnknownReturns201ThenSubsequent204(t *testing.T) { } // TestClusterStatusPost_MultipleConditionsWithUnknownAvailable tests that -// Unknown Available is detected among multiple conditions +// first report with Unknown Available is accepted, subsequent ones rejected (HYPERFLEET-657) func TestClusterStatusPost_MultipleConditionsWithUnknownAvailable(t *testing.T) { h, client := test.RegisterIntegration(t) @@ -467,19 +584,29 @@ func TestClusterStatusPost_MultipleConditionsWithUnknownAvailable(t *testing.T) cluster, err := h.Factories.NewClusters(h.NewID()) Expect(err).NotTo(HaveOccurred()) - // Create an adapter status with multiple conditions including Available=Unknown + // Create an adapter status with all mandatory conditions but Available=Unknown statusInput := newAdapterStatusRequest( "test-adapter-multi-unknown", cluster.Generation, []openapi.ConditionRequest{ { - Type: "Ready", + Type: api.ConditionTypeAvailable, + Status: openapi.AdapterConditionStatusUnknown, + Reason: util.PtrString("StartupPending"), + }, + { + Type: api.ConditionTypeApplied, Status: openapi.AdapterConditionStatusTrue, + Reason: util.PtrString("ConfigurationApplied"), }, { - Type: "Available", - Status: openapi.AdapterConditionStatusUnknown, - Reason: util.PtrString("StartupPending"), + Type: api.ConditionTypeHealth, + Status: openapi.AdapterConditionStatusTrue, + Reason: util.PtrString("HealthyCluster"), + }, + { + Type: api.ConditionTypeReady, + Status: openapi.AdapterConditionStatusTrue, }, { Type: "Progressing", @@ -489,7 +616,7 @@ func TestClusterStatusPost_MultipleConditionsWithUnknownAvailable(t *testing.T) nil, ) - // First report: should be stored (201 Created) even with Available=Unknown + // First report with Unknown Available condition: should be accepted resp, err := client.PostClusterStatusesWithResponse( ctx, cluster.ID, openapi.PostClusterStatusesJSONRequestBody(statusInput), test.WithAuthToken(ctx), @@ -498,7 +625,7 @@ func TestClusterStatusPost_MultipleConditionsWithUnknownAvailable(t *testing.T) Expect(resp.StatusCode()).To(Equal(http.StatusCreated), "Expected 201 Created for first report with Available=Unknown among multiple conditions") - // Subsequent report: should be no-op (204 No Content) + // Subsequent report: should be rejected (204 No Content) resp2, err := client.PostClusterStatusesWithResponse( ctx, cluster.ID, openapi.PostClusterStatusesJSONRequestBody(statusInput), test.WithAuthToken(ctx), @@ -526,7 +653,19 @@ func TestAdapterStatusPagingEdgeCases(t *testing.T) { cluster.Generation, []openapi.ConditionRequest{ { - Type: "Ready", + Type: api.ConditionTypeAvailable, + Status: openapi.AdapterConditionStatusTrue, + }, + { + Type: api.ConditionTypeApplied, + Status: openapi.AdapterConditionStatusTrue, + }, + { + Type: api.ConditionTypeHealth, + Status: openapi.AdapterConditionStatusTrue, + }, + { + Type: api.ConditionTypeReady, Status: openapi.AdapterConditionStatusTrue, }, }, @@ -571,7 +710,19 @@ func TestAdapterStatusPagingEdgeCases(t *testing.T) { singleCluster.Generation, []openapi.ConditionRequest{ { - Type: "Ready", + Type: api.ConditionTypeAvailable, + Status: openapi.AdapterConditionStatusTrue, + }, + { + Type: api.ConditionTypeApplied, + Status: openapi.AdapterConditionStatusTrue, + }, + { + Type: api.ConditionTypeHealth, + Status: openapi.AdapterConditionStatusTrue, + }, + { + Type: api.ConditionTypeReady, Status: openapi.AdapterConditionStatusTrue, }, }, @@ -623,3 +774,301 @@ func TestAdapterStatusPagingEdgeCases(t *testing.T) { // Verify we got all 10 unique adapters Expect(len(allItems)).To(Equal(10), "Should retrieve all items exactly once across pages") } + +// TestClusterStatusPost_MissingMandatoryConditionsRejected tests that adapter status updates +// without mandatory conditions are rejected and existing conditions are preserved +func TestClusterStatusPost_MissingMandatoryConditionsRejected(t *testing.T) { + h, client := test.RegisterIntegration(t) + + account := h.NewRandAccount() + ctx := h.NewAuthenticatedContext(account) + + // Create a cluster first + cluster, err := h.Factories.NewClusters(h.NewID()) + Expect(err).NotTo(HaveOccurred()) + + // Send initial valid status with all mandatory conditions + initialStatus := newAdapterStatusRequest( + "adapter1", + cluster.Generation, + []openapi.ConditionRequest{ + { + Type: api.ConditionTypeAvailable, + Status: openapi.AdapterConditionStatusFalse, + Reason: util.PtrString("ConfigMap data not yet available"), + Message: util.PtrString("ConfigMap data not yet available"), + }, + { + Type: api.ConditionTypeApplied, + Status: openapi.AdapterConditionStatusTrue, + Reason: util.PtrString("ConfigMapApplied"), + Message: util.PtrString("ConfigMap has been applied correctly"), + }, + { + Type: api.ConditionTypeHealth, + Status: openapi.AdapterConditionStatusFalse, + Reason: util.PtrString("ConfigMap data not yet available"), + Message: util.PtrString("ConfigMap data not yet available"), + }, + }, + nil, + ) + + resp1, err := client.PostClusterStatusesWithResponse( + ctx, cluster.ID, + openapi.PostClusterStatusesJSONRequestBody(initialStatus), test.WithAuthToken(ctx), + ) + Expect(err).NotTo(HaveOccurred()) + Expect(resp1.StatusCode()).To(Equal(http.StatusCreated)) + Expect(resp1.JSON201).ToNot(BeNil()) + Expect(len(resp1.JSON201.Conditions)).To(Equal(3)) + + // Verify Available, Applied, Health are present + conditionTypes := make(map[string]bool) + for _, cond := range resp1.JSON201.Conditions { + conditionTypes[cond.Type] = true + } + Expect(conditionTypes[api.ConditionTypeAvailable]).To(BeTrue()) + Expect(conditionTypes[api.ConditionTypeApplied]).To(BeTrue()) + Expect(conditionTypes[api.ConditionTypeHealth]).To(BeTrue()) + + // Now send an incomplete update (missing mandatory conditions) - this is the bug scenario + incompleteStatus := newAdapterStatusRequest( + "adapter1", + cluster.Generation, + []openapi.ConditionRequest{ + { + Type: "Available2", + Status: openapi.AdapterConditionStatusUnknown, + Reason: util.PtrString("ClusterProvisioned"), + Message: util.PtrString("Cluster successfully provisioned"), + }, + { + Type: "Health2", + Status: openapi.AdapterConditionStatusUnknown, + Reason: util.PtrString("ClusterProvisioned"), + Message: util.PtrString("Cluster successfully provisioned"), + }, + }, + &map[string]interface{}{ + "duration": "10m", + "job_name": "provision-job-123", + }, + ) + + resp2, err := client.PostClusterStatusesWithResponse( + ctx, cluster.ID, + openapi.PostClusterStatusesJSONRequestBody(incompleteStatus), test.WithAuthToken(ctx), + ) + Expect(err).NotTo(HaveOccurred()) + // Should return 204 No Content (update was discarded) + Expect(resp2.StatusCode()).To(Equal(http.StatusNoContent)) + + // Verify that the original conditions are preserved + respGet, err := client.GetClusterStatusesWithResponse(ctx, cluster.ID, nil, test.WithAuthToken(ctx)) + Expect(err).NotTo(HaveOccurred()) + Expect(respGet.StatusCode()).To(Equal(http.StatusOK)) + Expect(respGet.JSON200).ToNot(BeNil()) + Expect(len(respGet.JSON200.Items)).To(Equal(1)) + + // Verify Available, Applied, Health are still present (not replaced by Available2/Health2) + storedConditionTypes := make(map[string]bool) + for _, cond := range respGet.JSON200.Items[0].Conditions { + storedConditionTypes[cond.Type] = true + } + Expect(storedConditionTypes[api.ConditionTypeAvailable]).To(BeTrue(), "Available condition should be preserved") + Expect(storedConditionTypes[api.ConditionTypeApplied]).To(BeTrue(), "Applied condition should be preserved") + Expect(storedConditionTypes[api.ConditionTypeHealth]).To(BeTrue(), "Health condition should be preserved") + Expect(storedConditionTypes["Available2"]).To(BeFalse(), "Available2 should not be present") + Expect(storedConditionTypes["Health2"]).To(BeFalse(), "Health2 should not be present") +} + +// TestClusterStatusPost_FirstUnknownAcceptedSubsequentRejected tests that first status with +// Unknown Available is accepted, subsequent ones are rejected +func TestClusterStatusPost_FirstUnknownAcceptedSubsequentRejected(t *testing.T) { + h, client := test.RegisterIntegration(t) + + account := h.NewRandAccount() + ctx := h.NewAuthenticatedContext(account) + + // Create a cluster first + cluster, err := h.Factories.NewClusters(h.NewID()) + Expect(err).NotTo(HaveOccurred()) + + // Send first status with all mandatory conditions but Available=Unknown + statusWithUnknown := newAdapterStatusRequest( + "adapter1", + cluster.Generation, + []openapi.ConditionRequest{ + { + Type: api.ConditionTypeAvailable, + Status: openapi.AdapterConditionStatusUnknown, + Reason: util.PtrString("Checking"), + Message: util.PtrString("Checking availability"), + }, + { + Type: api.ConditionTypeApplied, + Status: openapi.AdapterConditionStatusTrue, + Reason: util.PtrString("Applied"), + Message: util.PtrString("Configuration applied"), + }, + { + Type: api.ConditionTypeHealth, + Status: openapi.AdapterConditionStatusTrue, + Reason: util.PtrString("Healthy"), + Message: util.PtrString("Cluster is healthy"), + }, + }, + nil, + ) + + // First report: should be accepted + resp, err := client.PostClusterStatusesWithResponse( + ctx, cluster.ID, + openapi.PostClusterStatusesJSONRequestBody(statusWithUnknown), test.WithAuthToken(ctx), + ) + Expect(err).NotTo(HaveOccurred()) + Expect(resp.StatusCode()).To(Equal(http.StatusCreated), "First status with Unknown Available should be accepted") + + // Verify status was stored + respGet, err := client.GetClusterStatusesWithResponse(ctx, cluster.ID, nil, test.WithAuthToken(ctx)) + Expect(err).NotTo(HaveOccurred()) + Expect(respGet.StatusCode()).To(Equal(http.StatusOK)) + Expect(respGet.JSON200).ToNot(BeNil()) + Expect(len(respGet.JSON200.Items)).To(Equal(1), "First status with Unknown Available should be stored") + + // Subsequent report: should be rejected (204 No Content) + resp2, err := client.PostClusterStatusesWithResponse( + ctx, cluster.ID, + openapi.PostClusterStatusesJSONRequestBody(statusWithUnknown), test.WithAuthToken(ctx), + ) + Expect(err).NotTo(HaveOccurred()) + Expect(resp2.StatusCode()).To(Equal(http.StatusNoContent), + "Subsequent status with Unknown Available should be rejected") +} + +// TestClusterStatusPost_DuplicateConditionsRejected tests that adapter status updates +// with duplicate condition types are rejected and return HTTP 204 +func TestClusterStatusPost_DuplicateConditionsRejected(t *testing.T) { + h, client := test.RegisterIntegration(t) + + account := h.NewRandAccount() + ctx := h.NewAuthenticatedContext(account) + + // Create a cluster first + cluster, err := h.Factories.NewClusters(h.NewID()) + Expect(err).NotTo(HaveOccurred()) + + // Send status with duplicate Available condition + duplicateStatus := newAdapterStatusRequest( + "adapter1", + cluster.Generation, + []openapi.ConditionRequest{ + { + Type: api.ConditionTypeAvailable, + Status: openapi.AdapterConditionStatusTrue, + Reason: util.PtrString("FirstAvailable"), + Message: util.PtrString("First Available condition"), + }, + { + Type: api.ConditionTypeApplied, + Status: openapi.AdapterConditionStatusTrue, + Reason: util.PtrString("Applied"), + Message: util.PtrString("Configuration applied"), + }, + { + Type: api.ConditionTypeHealth, + Status: openapi.AdapterConditionStatusTrue, + Reason: util.PtrString("Healthy"), + Message: util.PtrString("Cluster is healthy"), + }, + { + Type: api.ConditionTypeAvailable, // Duplicate! + Status: openapi.AdapterConditionStatusFalse, + Reason: util.PtrString("SecondAvailable"), + Message: util.PtrString("Second Available condition"), + }, + }, + nil, + ) + + resp, err := client.PostClusterStatusesWithResponse( + ctx, cluster.ID, + openapi.PostClusterStatusesJSONRequestBody(duplicateStatus), test.WithAuthToken(ctx), + ) + Expect(err).NotTo(HaveOccurred()) + // Should return 204 No Content (update was discarded) + Expect(resp.StatusCode()).To(Equal(http.StatusNoContent), + "Status with duplicate condition types should be rejected") + + // Verify that no status was stored + respGet, err := client.GetClusterStatusesWithResponse(ctx, cluster.ID, nil, test.WithAuthToken(ctx)) + Expect(err).NotTo(HaveOccurred()) + Expect(respGet.StatusCode()).To(Equal(http.StatusOK)) + Expect(respGet.JSON200).ToNot(BeNil()) + Expect(len(respGet.JSON200.Items)).To(Equal(0), + "No status should be stored when duplicate conditions are rejected") +} + +// TestClusterStatusPost_EmptyConditionTypeRejected tests that adapter status updates +// with empty condition types are rejected and return HTTP 204 +func TestClusterStatusPost_EmptyConditionTypeRejected(t *testing.T) { + h, client := test.RegisterIntegration(t) + + account := h.NewRandAccount() + ctx := h.NewAuthenticatedContext(account) + + // Create a cluster first + cluster, err := h.Factories.NewClusters(h.NewID()) + Expect(err).NotTo(HaveOccurred()) + + // Send status with empty condition type + emptyTypeStatus := newAdapterStatusRequest( + "adapter1", + cluster.Generation, + []openapi.ConditionRequest{ + { + Type: api.ConditionTypeAvailable, + Status: openapi.AdapterConditionStatusTrue, + Reason: util.PtrString("Available"), + Message: util.PtrString("Cluster is available"), + }, + { + Type: api.ConditionTypeApplied, + Status: openapi.AdapterConditionStatusTrue, + Reason: util.PtrString("Applied"), + Message: util.PtrString("Configuration applied"), + }, + { + Type: "", // Empty type! + Status: openapi.AdapterConditionStatusTrue, + Reason: util.PtrString("EmptyType"), + Message: util.PtrString("Condition with empty type"), + }, + { + Type: api.ConditionTypeHealth, + Status: openapi.AdapterConditionStatusTrue, + Reason: util.PtrString("Healthy"), + Message: util.PtrString("Cluster is healthy"), + }, + }, + nil, + ) + + resp, err := client.PostClusterStatusesWithResponse( + ctx, cluster.ID, + openapi.PostClusterStatusesJSONRequestBody(emptyTypeStatus), test.WithAuthToken(ctx), + ) + Expect(err).NotTo(HaveOccurred()) + // Should return 204 No Content (update was discarded) + Expect(resp.StatusCode()).To(Equal(http.StatusNoContent), + "Status with empty condition type should be rejected") + + // Verify that no status was stored + respGet, err := client.GetClusterStatusesWithResponse(ctx, cluster.ID, nil, test.WithAuthToken(ctx)) + Expect(err).NotTo(HaveOccurred()) + Expect(respGet.StatusCode()).To(Equal(http.StatusOK)) + Expect(respGet.JSON200).ToNot(BeNil()) + Expect(len(respGet.JSON200.Items)).To(Equal(0), + "No status should be stored when empty condition type is rejected") +}