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") +}