From d5df0d0de9e9baf67c806856152ef80e03349060 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 --- pkg/api/status_types.go | 8 + pkg/services/cluster.go | 57 ++- pkg/services/cluster_test.go | 448 +++++++++++++++++++----- pkg/services/node_pool.go | 52 ++- pkg/services/node_pool_test.go | 105 ++++-- pkg/services/status_aggregation.go | 57 ++- pkg/services/status_aggregation_test.go | 130 ++++++- test/integration/adapter_status_test.go | 391 ++++++++++++++++++--- 8 files changed, 1005 insertions(+), 243 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..40e2820 100644 --- a/pkg/services/cluster.go +++ b/pkg/services/cluster.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" stderrors "errors" + "fmt" "strings" "time" @@ -237,9 +238,10 @@ 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 updates where any mandatory condition is missing or has status "Unknown" +// - Uses complete replacement semantics: each update replaces all conditions for this adapter +// - Returns (nil, nil) for discarded updates, which results in HTTP 204 No Content func (s *sqlClusterService) ProcessAdapterStatus( ctx context.Context, clusterID string, adapterStatus *api.AdapterStatus, ) (*api.AdapterStatus, *errors.ServiceError) { @@ -264,42 +266,33 @@ func (s *sqlClusterService) ProcessAdapterStatus( } } - // Find the "Available" condition - hasAvailableCondition := false - for _, cond := range conditions { - if cond.Type != "Available" { - continue - } - - hasAvailableCondition = true - if cond.Status == api.AdapterConditionUnknown { - if existingStatus != nil { - // Available condition is "Unknown" and a status already exists, return nil to indicate no-op - 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 - } + // Validate that all mandatory conditions are present and not Unknown + if missingCondition, unknownCondition := ValidateMandatoryConditions(conditions); missingCondition != "" { + // Missing mandatory condition - discard the update + ctx = logger.WithClusterID(ctx, clusterID) + logger.Info(ctx, fmt.Sprintf("Discarding adapter status update from %s: missing mandatory condition %s", + adapterStatus.Adapter, missingCondition)) + return nil, nil + } else if unknownCondition != "" { + // Mandatory condition has Unknown status - discard the update + ctx = logger.WithClusterID(ctx, clusterID) + logger.Info(ctx, fmt.Sprintf("Discarding adapter status update from %s: mandatory condition %s has Unknown status", + adapterStatus.Adapter, unknownCondition)) + return nil, nil } - // Upsert the adapter status + // All validations passed - 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 { - if _, aggregateErr := s.UpdateClusterStatusFromAdapters( - ctx, clusterID, - ); aggregateErr != nil { - // Log error but don't fail the request - the status will be computed on next update - ctx = logger.WithClusterID(ctx, clusterID) - logger.WithError(ctx, aggregateErr).Warn("Failed to aggregate cluster status") - } + // Trigger aggregation to update cluster-level conditions + if _, aggregateErr := s.UpdateClusterStatusFromAdapters( + ctx, clusterID, + ); aggregateErr != nil { + ctx = logger.WithClusterID(ctx, clusterID) + logger.WithError(ctx, aggregateErr).Warn("Failed to aggregate cluster status") } return upsertedStatus, nil diff --git a/pkg/services/cluster_test.go b/pkg/services/cluster_test.go index a66cfb4..59b01df 100644 --- a/pkg/services/cluster_test.go +++ b/pkg/services/cluster_test.go @@ -163,47 +163,50 @@ func (d *mockAdapterStatusDao) All(ctx context.Context) (api.AdapterStatusList, var _ dao.AdapterStatusDao = &mockAdapterStatusDao{} -// TestProcessAdapterStatus_FirstUnknownCondition tests that the first Unknown Available condition is stored -func TestProcessAdapterStatus_FirstUnknownCondition(t *testing.T) { +// TestProcessAdapterStatus_FirstUnknownCondition tests that updates with Unknown mandatory conditions are rejected +func TestProcessAdapterStatus_MandatoryConditionUnknown(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 Available=Unknown + // Create cluster first + cluster := &api.Cluster{Generation: 1} + cluster.ID = clusterID + _, svcErr := service.Create(ctx, cluster) + Expect(svcErr).To(BeNil()) + + // Send 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) + // Should be rejected Expect(err).To(BeNil()) - Expect(result).ToNot(BeNil(), "First report with Available=Unknown should be stored") - Expect(result.Adapter).To(Equal("test-adapter")) + Expect(result).To(BeNil(), "Update with Available=Unknown should be rejected") - // Verify the status was stored + // Verify no 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(0), "No status should be stored") } // TestProcessAdapterStatus_SubsequentUnknownCondition tests that subsequent Unknown Available conditions are discarded @@ -222,7 +225,7 @@ func TestProcessAdapterStatus_SubsequentUnknownCondition(t *testing.T) { // Pre-populate an existing adapter status to simulate a previously stored report conditions := []api.AdapterCondition{ { - Type: conditionTypeAvailable, + Type: api.ConditionTypeAvailable, Status: api.AdapterConditionUnknown, LastTransitionTime: time.Now(), }, @@ -275,10 +278,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 +339,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,7 +378,7 @@ 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 +// TestProcessAdapterStatus_NoAvailableCondition tests that updates missing mandatory conditions are rejected func TestProcessAdapterStatus_NoAvailableCondition(t *testing.T) { RegisterTestingT(t) @@ -369,73 +392,74 @@ func TestProcessAdapterStatus_NoAvailableCondition(t *testing.T) { clusterID := testClusterID // Create the cluster first - fixedNow := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) - initialConditions := []api.ResourceCondition{ + cluster := &api.Cluster{Generation: 1} + cluster.ID = clusterID + _, svcErr := service.Create(ctx, cluster) + Expect(svcErr).To(BeNil()) + + // First, send a valid complete status + validConditions := []api.AdapterCondition{ { - Type: conditionTypeAvailable, - Status: api.ConditionFalse, - ObservedGeneration: 1, - LastTransitionTime: fixedNow, - CreatedTime: fixedNow, - LastUpdatedTime: fixedNow, + Type: api.ConditionTypeAvailable, + Status: api.AdapterConditionTrue, + LastTransitionTime: time.Now(), }, { - Type: "Ready", - Status: api.ConditionFalse, - ObservedGeneration: 7, - LastTransitionTime: fixedNow, - CreatedTime: fixedNow, - LastUpdatedTime: fixedNow, + Type: api.ConditionTypeApplied, + Status: api.AdapterConditionTrue, + LastTransitionTime: time.Now(), + }, + { + Type: api.ConditionTypeHealth, + Status: api.AdapterConditionTrue, + LastTransitionTime: time.Now(), }, } - initialConditionsJSON, _ := json.Marshal(initialConditions) - - cluster := &api.Cluster{ - Generation: 7, - StatusConditions: initialConditionsJSON, + validConditionsJSON, _ := json.Marshal(validConditions) + now := time.Now() + validStatus := &api.AdapterStatus{ + ResourceType: "Cluster", + ResourceID: clusterID, + Adapter: "test-adapter", + Conditions: validConditionsJSON, + ObservedGeneration: 1, + CreatedTime: &now, } - cluster.ID = clusterID - _, svcErr := service.Create(ctx, cluster) - Expect(svcErr).To(BeNil()) - initialClusterStatusConditions := api.Cluster{}.StatusConditions - initialClusterStatusConditions = append(initialClusterStatusConditions, cluster.StatusConditions...) + result, err := service.ProcessAdapterStatus(ctx, clusterID, validStatus) + Expect(err).To(BeNil()) + Expect(result).ToNot(BeNil()) - // Create adapter status with Health condition (no Available) - conditions := []api.AdapterCondition{ + // Now try to send an update with only Health condition (missing Available and Applied) + incompleteConditions := []api.AdapterCondition{ { - Type: "Health", - Status: api.AdapterConditionTrue, + Type: api.ConditionTypeHealth, + 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, - CreatedTime: &now, + incompleteConditionsJSON, _ := json.Marshal(incompleteConditions) + incompleteStatus := &api.AdapterStatus{ + ResourceType: "Cluster", + ResourceID: clusterID, + Adapter: "test-adapter", + Conditions: incompleteConditionsJSON, + ObservedGeneration: 2, + CreatedTime: &now, } - result, err := service.ProcessAdapterStatus(ctx, clusterID, adapterStatus) + result, err = service.ProcessAdapterStatus(ctx, clusterID, incompleteStatus) Expect(err).To(BeNil()) - Expect(result).ToNot(BeNil(), "ProcessAdapterStatus should proceed when no Available condition") + Expect(result).To(BeNil(), "Update missing Available condition should be rejected") - // 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") + // Verify the original valid status is preserved + storedStatus, _ := adapterStatusDao.FindByResourceAndAdapter(ctx, "Cluster", clusterID, "test-adapter") + Expect(storedStatus).ToNot(BeNil()) + Expect(storedStatus.ObservedGeneration).To(Equal(int32(1)), "Original status should be preserved") } -// TestProcessAdapterStatus_FirstMultipleConditions_AvailableUnknown tests that the first report with -// multiple conditions including Available=Unknown is stored +// TestProcessAdapterStatus_FirstMultipleConditions_AvailableUnknown tests that reports with +// Available=Unknown are rejected even when other conditions are present func TestProcessAdapterStatus_FirstMultipleConditions_AvailableUnknown(t *testing.T) { RegisterTestingT(t) @@ -448,20 +472,31 @@ func TestProcessAdapterStatus_FirstMultipleConditions_AvailableUnknown(t *testin ctx := context.Background() clusterID := testClusterID - // Create adapter status with multiple conditions including Available=Unknown + // Create cluster first + cluster := &api.Cluster{Generation: 1} + cluster.ID = clusterID + _, svcErr := service.Create(ctx, cluster) + Expect(svcErr).To(BeNil()) + + // Create adapter status with all mandatory conditions but Available=Unknown conditions := []api.AdapterCondition{ { - Type: "Ready", + 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: "Progressing", + Type: "Ready", Status: api.AdapterConditionTrue, LastTransitionTime: time.Now(), }, @@ -480,11 +515,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).To(BeNil(), "Report with Available=Unknown should be rejected") - // Verify the status was stored + // Verify no 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(0), "No status should be stored when Available=Unknown") } // TestProcessAdapterStatus_SubsequentMultipleConditions_AvailableUnknown tests that subsequent reports @@ -504,7 +539,7 @@ func TestProcessAdapterStatus_SubsequentMultipleConditions_AvailableUnknown(t *t // Pre-populate an existing adapter status existingConditions := []api.AdapterCondition{ { - Type: conditionTypeAvailable, + Type: api.ConditionTypeAvailable, Status: api.AdapterConditionUnknown, LastTransitionTime: time.Now(), }, @@ -529,7 +564,7 @@ func TestProcessAdapterStatus_SubsequentMultipleConditions_AvailableUnknown(t *t LastTransitionTime: time.Now(), }, { - Type: conditionTypeAvailable, + Type: api.ConditionTypeAvailable, Status: api.AdapterConditionUnknown, LastTransitionTime: time.Now(), }, @@ -585,9 +620,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 +633,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 +726,7 @@ 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()}, } unknownJSON, _ := json.Marshal(unknownConds) unknownStatus := &api.AdapterStatus{ @@ -730,7 +767,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 +777,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 +835,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, @@ -829,9 +868,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 +893,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 +908,222 @@ 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: "Available", 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 == "Available" { + 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: "Available", 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: "Available", 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: "Available", 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") +} diff --git a/pkg/services/node_pool.go b/pkg/services/node_pool.go index c341de4..43025d1 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" @@ -173,7 +174,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 } @@ -265,42 +266,33 @@ func (s *sqlNodePoolService) ProcessAdapterStatus( } } - // Find the "Available" condition - hasAvailableCondition := false - for _, cond := range conditions { - if cond.Type != conditionTypeAvailable { - continue - } - - hasAvailableCondition = true - if cond.Status == api.AdapterConditionUnknown { - if existingStatus != nil { - // Available condition is "Unknown" and a status already exists, return nil to indicate no-op - 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 - } + // Validate that all mandatory conditions are present and not Unknown + if missingCondition, unknownCondition := ValidateMandatoryConditions(conditions); missingCondition != "" { + // Missing mandatory condition - discard the update + logger.With(ctx, logger.FieldNodePoolID, nodePoolID). + Info(fmt.Sprintf("Discarding adapter status update from %s: missing mandatory condition %s", + adapterStatus.Adapter, missingCondition)) + return nil, nil + } else if unknownCondition != "" { + // Mandatory condition has Unknown status - discard the update + logger.With(ctx, logger.FieldNodePoolID, nodePoolID). + Info(fmt.Sprintf("Discarding adapter status update from %s: mandatory condition %s has Unknown status", + adapterStatus.Adapter, unknownCondition)) + return nil, nil } - // Upsert the adapter status + // All validations passed - 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 { - if _, aggregateErr := s.UpdateNodePoolStatusFromAdapters( - ctx, nodePoolID, - ); aggregateErr != nil { - // Log error but don't fail the request - the status will be computed on next update - logger.With(ctx, logger.FieldNodePoolID, nodePoolID). - WithError(aggregateErr).Warn("Failed to aggregate nodepool status") - } + // Trigger aggregation to update nodepool-level conditions + if _, aggregateErr := s.UpdateNodePoolStatusFromAdapters( + ctx, nodePoolID, + ); aggregateErr != nil { + logger.With(ctx, logger.FieldNodePoolID, nodePoolID). + WithError(aggregateErr).Warn("Failed to aggregate nodepool status") } return upsertedStatus, nil diff --git a/pkg/services/node_pool_test.go b/pkg/services/node_pool_test.go index 9c5f39e..84c76ea 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 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,12 +125,11 @@ 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.Adapter).To(Equal("test-adapter")) + Expect(result).To(BeNil(), "Update with Available=Unknown should be rejected") - // Verify the status was stored + // Verify no 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(0), "No status should be stored when mandatory condition is Unknown") } // TestNodePoolProcessAdapterStatus_SubsequentUnknownCondition tests that subsequent Unknown conditions are discarded @@ -139,7 +148,7 @@ func TestNodePoolProcessAdapterStatus_SubsequentUnknownCondition(t *testing.T) { // Pre-populate an existing adapter status conditions := []api.AdapterCondition{ { - Type: conditionTypeAvailable, + Type: api.ConditionTypeAvailable, Status: api.AdapterConditionUnknown, LastTransitionTime: time.Now(), }, @@ -192,10 +201,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 +241,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 reports +// with Available=Unknown are rejected even when other conditions are present func TestNodePoolProcessAdapterStatus_FirstMultipleConditions_AvailableUnknown(t *testing.T) { RegisterTestingT(t) @@ -236,16 +255,26 @@ func TestNodePoolProcessAdapterStatus_FirstMultipleConditions_AvailableUnknown(t ctx := context.Background() nodePoolID := testNodePoolID - // Create adapter status with multiple conditions including Available=Unknown + // Create 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 +292,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).To(BeNil(), "Report with Available=Unknown should be rejected") - // Verify the status was stored + // Verify no 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(0), "No status should be stored when Available=Unknown") } // TestNodePoolProcessAdapterStatus_SubsequentMultipleConditions_AvailableUnknown tests that subsequent @@ -287,7 +316,7 @@ func TestNodePoolProcessAdapterStatus_SubsequentMultipleConditions_AvailableUnkn // Pre-populate an existing adapter status existingConditions := []api.AdapterCondition{ { - Type: conditionTypeAvailable, + Type: api.ConditionTypeAvailable, Status: api.AdapterConditionUnknown, LastTransitionTime: time.Now(), }, @@ -307,12 +336,12 @@ func TestNodePoolProcessAdapterStatus_SubsequentMultipleConditions_AvailableUnkn // Now send another report with multiple conditions including Available=Unknown conditions := []api.AdapterCondition{ { - Type: conditionTypeReady, + Type: api.ConditionTypeReady, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now(), }, { - Type: conditionTypeAvailable, + Type: api.ConditionTypeAvailable, Status: api.AdapterConditionUnknown, LastTransitionTime: time.Now(), }, @@ -362,9 +391,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 +404,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 +478,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 +494,14 @@ 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()}, } unknownJSON, _ := json.Marshal(unknownConds) unknownStatus := &api.AdapterStatus{ @@ -511,7 +542,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 +552,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 +610,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 +618,7 @@ func TestNodePoolSyntheticTimestampsStableWithoutAdapterStatus(t *testing.T) { LastUpdatedTime: fixedNow, }, { - Type: conditionTypeReady, + Type: api.ConditionTypeReady, Status: api.ConditionFalse, ObservedGeneration: 1, LastTransitionTime: fixedNow, @@ -610,9 +643,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 +668,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..b352f2a 100644 --- a/pkg/services/status_aggregation.go +++ b/pkg/services/status_aggregation.go @@ -10,10 +10,8 @@ import ( "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" ) -const ( - conditionTypeAvailable = "Available" - conditionTypeReady = "Ready" -) +// Mandatory condition types that must be present in all adapter status updates +var mandatoryConditionTypes = []string{api.ConditionTypeAvailable, api.ConditionTypeApplied, api.ConditionTypeHealth} // Required adapter lists configured via pkg/config/adapter.go (see AdapterRequirementsConfig) @@ -26,6 +24,45 @@ var adapterConditionSuffixMap = map[string]string{ // Add custom mappings here when needed } +// ValidateMandatoryConditions checks if all mandatory conditions are present and not Unknown. +// Returns (missingCondition, unknownCondition) where: +// - missingCondition: the name of the first missing mandatory condition, or empty string if all present +// - unknownCondition: the name of the first mandatory condition with Unknown status, or empty string if none +// +// Usage: +// +// if missing, unknown := ValidateMandatoryConditions(conditions); missing != "" { +// // Handle missing condition +// } else if unknown != "" { +// // Handle unknown condition +// } +func ValidateMandatoryConditions(conditions []api.AdapterCondition) (missingCondition, unknownCondition string) { + // Build a map of condition types for validation + // If duplicate condition types exist, preserve Unknown status (highest severity) + conditionMap := make(map[string]api.AdapterConditionStatus) + for _, cond := range conditions { + existing, exists := conditionMap[cond.Type] + // If already Unknown, keep it (don't let True/False overwrite Unknown) + if exists && existing == api.AdapterConditionUnknown { + continue + } + conditionMap[cond.Type] = cond.Status + } + + // Check that all mandatory conditions are present and not Unknown + for _, mandatoryType := range mandatoryConditionTypes { + status, exists := conditionMap[mandatoryType] + if !exists { + return mandatoryType, "" + } + if status == api.AdapterConditionUnknown { + return "", 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 +124,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 +195,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 +253,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 +268,7 @@ func BuildSyntheticConditions( availableStatus = api.ConditionTrue } availableCondition := api.ResourceCondition{ - Type: conditionTypeAvailable, + Type: api.ConditionTypeAvailable, Status: availableStatus, ObservedGeneration: minObservedGeneration, LastTransitionTime: now, @@ -246,7 +283,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..5b942a7 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,126 @@ 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()}, + } + + missing, unknown := ValidateMandatoryConditions(conditions) + if missing != "" { + t.Errorf("Expected no missing conditions, got missing: %s", missing) + } + if unknown != "" { + t.Errorf("Expected no unknown conditions, got unknown: %s", unknown) + } +} + +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()}, + } + + missing, unknown := ValidateMandatoryConditions(conditions) + if missing != api.ConditionTypeAvailable { + t.Errorf("Expected missing condition %s, got: %s", api.ConditionTypeAvailable, missing) + } + if unknown != "" { + t.Errorf("Expected no unknown conditions, got: %s", unknown) + } +} + +func TestValidateMandatoryConditions_MandatoryConditionUnknown(t *testing.T) { + 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()}, + } + + missing, unknown := ValidateMandatoryConditions(conditions) + if missing != "" { + t.Errorf("Expected no missing conditions, got: %s", missing) + } + if unknown != api.ConditionTypeAvailable { + t.Errorf("Expected unknown condition %s, got: %s", api.ConditionTypeAvailable, unknown) + } +} + +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()}, + } + + missing, unknown := ValidateMandatoryConditions(conditions) + if missing != "" { + t.Errorf("Expected no missing conditions, got: %s", missing) + } + if unknown != "" { + t.Errorf("Expected no unknown conditions, got: %s", unknown) + } +} + +func TestValidateMandatoryConditions_EmptyConditions(t *testing.T) { + conditions := []api.AdapterCondition{} + + missing, unknown := ValidateMandatoryConditions(conditions) + if missing != api.ConditionTypeAvailable { + t.Errorf("Expected missing condition %s, got: %s", api.ConditionTypeAvailable, missing) + } + if unknown != "" { + t.Errorf("Expected no unknown conditions, got: %s", unknown) + } +} + +func TestValidateMandatoryConditions_DuplicateCondition_UnknownThenTrue(t *testing.T) { + // Test: If Available appears twice (Unknown first, True second), + // should detect Unknown (Unknown has highest priority) + 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! + } + + missing, unknown := ValidateMandatoryConditions(conditions) + + // Should detect Unknown even though True comes later + if missing != "" { + t.Errorf("Expected no missing conditions, got: %s", missing) + } + if unknown != api.ConditionTypeAvailable { + t.Errorf("Expected unknown condition %s (Unknown should be preserved), got: %s", + api.ConditionTypeAvailable, unknown) + } +} + +func TestValidateMandatoryConditions_DuplicateCondition_TrueThenUnknown(t *testing.T) { + // Test: If Available appears twice (True first, Unknown second), + // should detect Unknown (Unknown has highest priority) + conditions := []api.AdapterCondition{ + // First: True + {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()}, + // Duplicate: Unknown! + {Type: api.ConditionTypeAvailable, Status: api.AdapterConditionUnknown, LastTransitionTime: time.Now()}, + } + + missing, unknown := ValidateMandatoryConditions(conditions) + + // Should detect Unknown even though True comes first + if missing != "" { + t.Errorf("Expected no missing conditions, got: %s", missing) + } + if unknown != api.ConditionTypeAvailable { + t.Errorf("Expected unknown condition %s (Unknown should overwrite True), got: %s", + api.ConditionTypeAvailable, unknown) + } +} diff --git a/test/integration/adapter_status_test.go b/test/integration/adapter_status_test.go index 7e55206..fce48eb 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_UnknownReturns204 tests that status reports with Unknown +// mandatory conditions are always rejected (HYPERFLEET-657) +func TestClusterStatusPost_UnknownReturns204(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 mandatory condition: should be rejected (204 No Content) 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.StatusNoContent), "Expected 204 No Content for status with Unknown mandatory condition") - // Verify the status was stored + // Verify no 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(BeFalse(), "Status with Unknown mandatory condition should not be stored") - // Subsequent report with same adapter: should be no-op (204 No Content) + // Subsequent report with same adapter: should also 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_UnknownReturns204 tests that status reports with Unknown +// mandatory conditions are always rejected (HYPERFLEET-657) +func TestNodePoolStatusPost_UnknownReturns204(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 mandatory condition: should be rejected (204 No Content) 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.StatusNoContent), "Expected 204 No Content for status with Unknown mandatory condition") - // Verify the status was stored + // Verify no 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(BeFalse(), "Status with Unknown mandatory condition should not be stored") - // Subsequent report with same adapter: should be no-op (204 No Content) + // Subsequent report with same adapter: should also 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 +// Unknown Available is always rejected even among multiple conditions (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,16 +616,16 @@ func TestClusterStatusPost_MultipleConditionsWithUnknownAvailable(t *testing.T) nil, ) - // First report: should be stored (201 Created) even with Available=Unknown + // First report with Unknown mandatory condition: should be rejected (204 No Content) 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 report with Available=Unknown among multiple conditions") + Expect(resp.StatusCode()).To(Equal(http.StatusNoContent), + "Expected 204 No Content for first report with Available=Unknown among multiple conditions") - // Subsequent report: should be no-op (204 No Content) + // Subsequent report: should also 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,165 @@ 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") } +// TestHYPERFLEET657_MandatoryConditionsPreserved tests that adapter status updates +// without mandatory conditions are rejected and existing conditions are preserved +func TestHYPERFLEET657_MandatoryConditionsPreserved(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") +} + +// TestHYPERFLEET657_UnknownMandatoryConditionsRejected tests that updates with +// Unknown status for mandatory conditions are rejected +func TestHYPERFLEET657_UnknownMandatoryConditionsRejected(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()) + + // Try to send 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, + ) + + resp, err := client.PostClusterStatusesWithResponse( + ctx, cluster.ID, + openapi.PostClusterStatusesJSONRequestBody(statusWithUnknown), test.WithAuthToken(ctx), + ) + Expect(err).NotTo(HaveOccurred()) + // Should return 204 No Content (update was discarded because Available=Unknown) + Expect(resp.StatusCode()).To(Equal(http.StatusNoContent)) + + // Verify 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 mandatory condition is Unknown") +}