HYPERFLEET-657 - fix: enforce mandatory conditions in adapter status#60
Open
yasun1 wants to merge 1 commit intoopenshift-hyperfleet:mainfrom
Open
HYPERFLEET-657 - fix: enforce mandatory conditions in adapter status#60yasun1 wants to merge 1 commit intoopenshift-hyperfleet:mainfrom
yasun1 wants to merge 1 commit intoopenshift-hyperfleet:mainfrom
Conversation
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 <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughThis pull request introduces four new public condition type constants to the API (ConditionTypeAvailable, ConditionTypeApplied, ConditionTypeHealth, ConditionTypeReady) and implements strict validation of mandatory adapter conditions across cluster and node pool services. When processing adapter status updates, the services now validate that all mandatory conditions (Available, Applied, Health) are present and not in Unknown status; updates failing validation are rejected without persisting status. Valid updates trigger status aggregation. All internal constant references are replaced with the public API constants, and tests are updated to verify rejection behavior and mandatory condition preservation. Sequence DiagramsequenceDiagram
participant Adapter
participant Service as Cluster/NodePool Service
participant Storage as Status Storage
participant Aggregation as Status Aggregation
Adapter->>Service: POST adapter status<br/>(conditions with Available, Applied, Health)
rect rgba(200, 100, 100, 0.5)
Service->>Service: Build condition map
Service->>Service: Validate mandatory conditions<br/>(Available, Applied, Health)
alt Invalid (missing or Unknown)
Service->>Service: Log rejection reason
Service-->>Adapter: 204 No Content
Note over Service: Update discarded
end
end
rect rgba(100, 200, 100, 0.5)
alt Valid (all present, not Unknown)
Service->>Storage: Upsert adapter status<br/>(complete replacement)
Service->>Aggregation: Trigger status aggregation
Aggregation->>Aggregation: Compute Available/Ready<br/>conditions
Aggregation-->>Service: Aggregation complete
Service-->>Adapter: 200/201 Success
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
pkg/services/cluster_test.go (2)
943-946: Inconsistent use of string literals for condition types.Lines 944, 1024, 1071, 1130, and 1156 use the string literal
"Available"instead ofapi.ConditionTypeAvailable. For consistency with the rest of the file, consider using the API constants throughout.♻️ Suggested fix for line 944 (apply similar fixes to other occurrences)
validConditions := []api.AdapterCondition{ - {Type: "Available", Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {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()}, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/services/cluster_test.go` around lines 943 - 946, Replace the hard-coded string "Available" used for AdapterCondition Type with the API constant to maintain consistency: update the entries in validConditions (and the other occurrences at the noted locations) to use api.ConditionTypeAvailable instead of the string literal; ensure the type field remains of the same type (api.AdapterCondition.Type) and run tests to verify no type mismatches.
857-864: Inconsistent use of string literal instead of API constant.Line 858 uses the string literal
"Ready"while the rest of the file usesapi.ConditionTypeReady. For consistency, consider using the API constant.♻️ Suggested fix
{ - Type: "Ready", + Type: api.ConditionTypeReady, Status: api.ConditionFalse, ObservedGeneration: 1,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/services/cluster_test.go` around lines 857 - 864, Replace the hard-coded string "Ready" in the Condition struct literal with the API constant to maintain consistency: change the Type field value from the string literal to use api.ConditionTypeReady (the same constant used elsewhere in this file/tests), ensuring imports/reference to api remain correct.pkg/services/node_pool.go (1)
269-292: Validation logic is correct; consider structured logging improvement.The mandatory condition validation correctly mirrors the cluster.go implementation. However, similar to the cluster service, consider using structured logging fields instead of
fmt.Sprintf.♻️ Suggested refactor for structured logging
for _, mandatoryType := range mandatoryConditionTypes { status, exists := conditionMap[mandatoryType] if !exists { // 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, mandatoryType)) + logger.With(ctx, logger.FieldNodePoolID, nodePoolID, "adapter", adapterStatus.Adapter, "missing_condition", mandatoryType). + Info("Discarding adapter status update: missing mandatory condition") return nil, nil } if status == api.AdapterConditionUnknown { // 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, mandatoryType)) + logger.With(ctx, logger.FieldNodePoolID, nodePoolID, "adapter", adapterStatus.Adapter, "condition", mandatoryType). + Info("Discarding adapter status update: mandatory condition has Unknown status") return nil, nil } }Based on learnings: "In the openshift-hyperfleet/hyperfleet-api repository, NewOCMLogger(ctx) is deprecated. Switch to the slog-based logger and use logger.With(ctx, ...) to produce structured, context-aware logs."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/services/node_pool.go` around lines 269 - 292, The logging uses fmt.Sprintf for messages in the mandatory condition checks; replace these with structured, context-aware logs using logger.With(ctx, logger.FieldNodePoolID, nodePoolID, "adapter", adapterStatus.Adapter, "mandatoryCondition", mandatoryType, "status", status) and then call Info with a short message (e.g., "Discarding adapter status update") so fields carry the details; update both the missing-condition and Unknown-status branches and keep the validation logic around conditionMap, mandatoryConditionTypes, and adapterStatus.Adapter unchanged.pkg/services/cluster.go (1)
272-295: Consider using structured logging fields instead of fmt.Sprintf.The current approach uses
fmt.Sprintfto format the log message. Per repository conventions, structured logging with fields is preferred for better log parsing and filtering.♻️ Suggested refactor for structured logging
for _, mandatoryType := range mandatoryConditionTypes { status, exists := conditionMap[mandatoryType] if !exists { // 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, mandatoryType)) + logger.With(ctx, "adapter", adapterStatus.Adapter, "missing_condition", mandatoryType). + Info("Discarding adapter status update: missing mandatory condition") return nil, nil } if status == api.AdapterConditionUnknown { // 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, mandatoryType)) + logger.With(ctx, "adapter", adapterStatus.Adapter, "condition", mandatoryType). + Info("Discarding adapter status update: mandatory condition has Unknown status") return nil, nil } }Based on learnings: "In the openshift-hyperfleet/hyperfleet-api repository, NewOCMLogger(ctx) is deprecated. Switch to the slog-based logger and use logger.With(ctx, ...) to produce structured, context-aware logs."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/services/cluster.go` around lines 272 - 295, Replace the fmt.Sprintf-based logs with structured slog-style logging using logger.With(...) and pass clusterID, adapter and mandatory condition as fields; for the missing-condition case call something like ctx = logger.With(ctx, "cluster_id", clusterID, "adapter", adapterStatus.Adapter, "mandatory_condition", mandatoryType) and then logger.Info(ctx, "Discarding adapter status update: missing mandatory condition"); do the same for the Unknown-status branch (use the same fields and a message like "Discarding adapter status update: mandatory condition has Unknown status"), removing fmt.Sprintf and keeping the existing early returns; references: conditionMap, mandatoryConditionTypes, logger.WithClusterID / logger.With, logger.Info, adapterStatus.Adapter, mandatoryType.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/services/cluster_test.go`:
- Around line 943-946: Replace the hard-coded string "Available" used for
AdapterCondition Type with the API constant to maintain consistency: update the
entries in validConditions (and the other occurrences at the noted locations) to
use api.ConditionTypeAvailable instead of the string literal; ensure the type
field remains of the same type (api.AdapterCondition.Type) and run tests to
verify no type mismatches.
- Around line 857-864: Replace the hard-coded string "Ready" in the Condition
struct literal with the API constant to maintain consistency: change the Type
field value from the string literal to use api.ConditionTypeReady (the same
constant used elsewhere in this file/tests), ensuring imports/reference to api
remain correct.
In `@pkg/services/cluster.go`:
- Around line 272-295: Replace the fmt.Sprintf-based logs with structured
slog-style logging using logger.With(...) and pass clusterID, adapter and
mandatory condition as fields; for the missing-condition case call something
like ctx = logger.With(ctx, "cluster_id", clusterID, "adapter",
adapterStatus.Adapter, "mandatory_condition", mandatoryType) and then
logger.Info(ctx, "Discarding adapter status update: missing mandatory
condition"); do the same for the Unknown-status branch (use the same fields and
a message like "Discarding adapter status update: mandatory condition has
Unknown status"), removing fmt.Sprintf and keeping the existing early returns;
references: conditionMap, mandatoryConditionTypes, logger.WithClusterID /
logger.With, logger.Info, adapterStatus.Adapter, mandatoryType.
In `@pkg/services/node_pool.go`:
- Around line 269-292: The logging uses fmt.Sprintf for messages in the
mandatory condition checks; replace these with structured, context-aware logs
using logger.With(ctx, logger.FieldNodePoolID, nodePoolID, "adapter",
adapterStatus.Adapter, "mandatoryCondition", mandatoryType, "status", status)
and then call Info with a short message (e.g., "Discarding adapter status
update") so fields carry the details; update both the missing-condition and
Unknown-status branches and keep the validation logic around conditionMap,
mandatoryConditionTypes, and adapterStatus.Adapter unchanged.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
pkg/api/status_types.gopkg/services/cluster.gopkg/services/cluster_test.gopkg/services/node_pool.gopkg/services/node_pool_test.gopkg/services/status_aggregation.gotest/integration/adapter_status_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
All tests passing: 416 unit, 51 integration, 0 lint issues
Summary by CodeRabbit
New Features
Bug Fixes
Tests