HYPERFLEET-538 - feat: CEL-based condition mapping engine - #315
HYPERFLEET-538 - feat: CEL-based condition mapping engine#315ldornele wants to merge 17 commits into
Conversation
|
[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 |
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds CEL-based condition mappings with registry-time validation, reserved condition checks, and compile-time cost limits. Adds CEL helpers for bounded JSON conversion, nested lookup, sensitive-data masking, and adapter condition naming. Adds Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Adapter
participant ResourceService
participant ConditionMapper
participant CEL
Adapter->>ResourceService: Submit adapter conditions and data
ResourceService->>ConditionMapper: Apply resource state and prior conditions
ConditionMapper->>CEL: Evaluate compiled mapping expressions
CEL-->>ConditionMapper: Return mapped condition fields
ConditionMapper-->>ResourceService: Return mapped conditions or error
ResourceService-->>Adapter: Persist result or roll back
Suggested reviewers: 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Risk Score: 3 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 5345 lines (>500) | +2 |
| Sensitive paths | none | +0 |
| Test coverage | Missing tests for: pkg/config | +1 |
Computed by hyperfleet-risk-scorer
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (9)
pkg/services/condition_mapper_test.go (1)
1261-1392: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThree subtests are parented under the wrong test function, and one assertion is vacuous.
"invalid status string from CEL expression", "non-boolean when expression return type", and "concurrent Apply() access from multiple goroutines" live inside
TestTruncateUTF8. They exerciseApply(), not truncation — the test name no longer describes the scenario, and a-run TestTruncateUTF8filter now drags in a 10-goroutine race test.Separately, Line 1387:
result.erris never assigned by the producer goroutine, soExpect(res.err).NotTo(HaveOccurred())can never fail. Drop the field or makeApplyfailures observable.♻️ Split into dedicated test functions
+} + +func TestConditionMapper_InvalidOutputs(t *testing.T) { t.Run("invalid status string from CEL expression", func(t *testing.T) {t.Run("concurrent Apply() access from multiple goroutines", func(t *testing.T) {Move to its own
TestConditionMapper_ConcurrentApply, and reduce the channel payload to[]api.ResourceCondition:- type result struct { - conditions []api.ResourceCondition - err error - } - results := make(chan result, numGoroutines) + results := make(chan []api.ResourceCondition, numGoroutines)As per coding guidelines: "Test names describe the scenario, not the implementation."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/services/condition_mapper_test.go` around lines 1261 - 1392, Move the three Apply-focused subtests out of TestTruncateUTF8 into appropriately named dedicated test functions, including TestConditionMapper_ConcurrentApply for the concurrency case, so truncation filters remain scoped correctly. In the concurrent test, remove the unused result.err field and its assertion, and send/receive only []api.ResourceCondition through the channel while preserving the existing result assertions.Source: Path instructions
pkg/util/cel.go (1)
55-70: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueSize guard fires after the allocation it claims to prevent (CWE-770 adjacent).
json.Marshalfully materializes the payload before Line 65 rejects it, so the comment on Line 62 ("prevent unbounded intermediate allocations") does not hold. Attacker-influenced adapterdatablobs land in this path via the CEL activation. The CEL cost limit bounds expression complexity, not marshaled output size.Not exploitable at current JSONB sizes, but either use a size-capped encoder or drop the misleading rationale.
♻️ Bound the write instead of measuring after the fact
func toJSONFunc(val ref.Val) ref.Val { v := val.Value() - data, err := json.Marshal(v) - if err != nil { - return types.NewErr("toJson: %v", err) - } - - // Size guard: prevent unbounded intermediate allocations from large payloads - // Limit matches Kubernetes ConfigMap max size (1MB) as a reasonable upper bound - const maxJSONSize = 1 * 1024 * 1024 // 1MB - if len(data) > maxJSONSize { - return types.NewErr("toJson: output exceeds 1MB limit (%d bytes)", len(data)) - } - - return types.String(string(data)) + // Limit matches Kubernetes ConfigMap max size (1MB) as a reasonable upper bound + const maxJSONSize = 1 * 1024 * 1024 // 1MB + + var buf bytes.Buffer + buf.Grow(1024) + if err := json.NewEncoder(&limitedWriter{w: &buf, n: maxJSONSize}).Encode(v); err != nil { + return types.NewErr("toJson: %v", err) + } + return types.String(strings.TrimRight(buf.String(), "\n")) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/util/cel.go` around lines 55 - 70, Update toJSONFunc so serialization enforces the 1MB limit while writing, rather than calling json.Marshal and checking the fully allocated result afterward. Use a size-capped JSON encoder or equivalent bounded writer, preserve the existing toJson error behavior, and retain the maxJSONSize limit without claiming it prevents allocations it cannot prevent.pkg/services/resource_test.go (1)
3160-3232: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOnly the happy path is covered for the new mapper integration.
Two gaps worth closing here, both cheap given the helper you just added:
whenevaluating false → noCustomReadycondition emitted (verifies rules don't leak conditions unconditionally).- A rule keyed with a type that collides with a built-in (
Reconciled) or an adapter-derived type (ValidationSuccessful) — this is the scenario behind the collision issue flagged onpkg/services/resource.goLine 690-699, and a test would pin whatever behavior you settle on.As per coding guidelines: "Error paths SHOULD be tested, not just happy paths."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/services/resource_test.go` around lines 3160 - 3232, Extend TestResourceService_ConditionMapper_IntegrationPath with a false-evaluating when expression and assert no CustomReady condition is emitted. Add collision coverage using mapped types Reconciled and ValidationSuccessful, asserting the intended behavior from the condition-mapping logic without unintended overwrites or duplicates. Reuse the existing test helpers and verify both outcomes through rcDao.conditions.Source: Path instructions
pkg/services/condition_mapper.go (1)
205-209: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the duplicate condition-type length check here.
pkg/config/conditions.goalready rejects overlong condition types duringConditionsConfig.Validate(), so this branch is redundant in the normal startup path. IfNewConditionMappercan still be called without validated config, return the error instead of swallowing it and dropping the condition. (CWE-391)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/services/condition_mapper.go` around lines 205 - 209, Update the error handling around validateFieldLengths in NewConditionMapper’s condition-mapping flow: remove the redundant condition-type length validation there, relying on ConditionsConfig.Validate() for validated startup configuration. If validation can still fail for unvalidated configurations, propagate or return the error instead of silently returning nil, nil and dropping the condition.pkg/util/naming_test.go (1)
10-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd degenerate-input cases:
"","-adapter","multi--word".Empty or dash-edged adapter names yield
"Successful"/"AdapterSuccessful", which feedsbuildReservedConditionTypes(pkg/config/conditions.go:115-140) and can silently reserve an unintended type.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/util/naming_test.go` around lines 10 - 22, Extend the testCases table in the naming tests with degenerate adapter inputs "", "-adapter", and "multi--word", asserting the intended naming behavior for each. Use the existing naming function under test and ensure these cases verify that empty or dash-edged names do not produce unintended reserved condition types consumed by buildReservedConditionTypes.pkg/config/conditions.go (1)
84-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeduplicate the cluster/nodepool validation loops.
Two identical sort-and-iterate blocks. Extract a helper so adding a third entity kind is a one-line change instead of a copy-paste.
♻️ Proposed refactor
- // Validate cluster mappings (sort keys for deterministic error messages) - clusterKeys := make([]string, 0, len(c.Clusters)) - for condType := range c.Clusters { - clusterKeys = append(clusterKeys, condType) - } - sort.Strings(clusterKeys) - for _, condType := range clusterKeys { - if err := validateConditionMapping("clusters", condType, c.Clusters[condType], reserved, env); err != nil { - return err - } - } - - // Validate nodepool mappings (sort keys for deterministic error messages) - nodepoolKeys := make([]string, 0, len(c.NodePools)) - for condType := range c.NodePools { - nodepoolKeys = append(nodepoolKeys, condType) - } - sort.Strings(nodepoolKeys) - for _, condType := range nodepoolKeys { - if err := validateConditionMapping("nodepools", condType, c.NodePools[condType], reserved, env); err != nil { - return err - } - } - - return nil + if err := validateRuleSet("clusters", c.Clusters, reserved, env); err != nil { + return err + } + return validateRuleSet("nodepools", c.NodePools, reserved, env) +} + +// validateRuleSet validates a rule map with deterministic (sorted) error ordering. +func validateRuleSet(resourceType string, rules map[string]ConditionMappingRule, reserved map[string]bool, env *cel.Env) error { + keys := make([]string, 0, len(rules)) + for condType := range rules { + keys = append(keys, condType) + } + sort.Strings(keys) + for _, condType := range keys { + if err := validateConditionMapping(resourceType, condType, rules[condType], reserved, env); err != nil { + return err + } + } + return nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/config/conditions.go` around lines 84 - 106, Deduplicate the repeated validation logic in the cluster and nodepool sections of the configuration validation flow. Extract a helper that accepts the entity name and condition-mapping collection, sorts its keys, and calls validateConditionMapping for each entry while propagating errors; invoke it for both c.Clusters and c.NodePools.pkg/config/conditions_test.go (1)
188-208: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd invalid-expression cases for
output.reasonandoutput.message.Only
whenandoutput.statusare exercised; the two remainingvalidateCELExpressioncalls (pkg/config/conditions.go:167-172) have no coverage, so a typo in either field-name string would go unnoticed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/config/conditions_test.go` around lines 188 - 208, Extend the condition validation tests alongside the existing invalid output status case to cover malformed CEL expressions in output.reason and output.message. Add separate configurations using incomplete expressions for each field, and assert wantError is true with the existing “invalid CEL expression” error expectation, exercising the validateCELExpression calls for both fields.pkg/util/cel_test.go (2)
88-115: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the parse/check/program/eval pipeline into a
t.Helper()helper.The same four-step block is copied in three tests. A single helper keeps the tests aligned with
compileExpressionif the pipeline changes.Also applies to: 175-198
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/util/cel_test.go` around lines 88 - 115, Extract the repeated CEL parse, check, program creation, and evaluation sequence from the affected tests into a shared test helper marked with t.Helper(). Have the helper accept the expression and resource data, return the evaluated output and error, and update all three tests—including the block around the additional referenced lines—to use it while preserving their existing assertions and CEL cost limit.
199-227: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
toJson1MB size guard is untested.
toJSONFunc(pkg/util/cel.go:55-70) rejects payloads >1MB — that's the DoS containment boundary for CEL-driven serialization of adapter data (CWE-400). No test asserts it. Add a case feeding a large map and asserting theexceeds 1MB limiterror surfaces as an eval error. Same gap applies to theCELCostLimitrejection path.As per path instructions: "Error paths SHOULD be tested, not just happy paths".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/util/cel_test.go` around lines 199 - 227, Extend TestToJsonFunc with error-path cases for the toJSONFunc 1MB payload guard and CELCostLimit rejection. Feed toJson a sufficiently large map and assert evaluation returns an error containing “exceeds 1MB limit”; separately exercise a program that exceeds CELCostLimit and assert the evaluation surfaces the cost-limit error.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@configs/config.yaml.example`:
- Around line 166-168: Update the TODO near the CEL env context configuration to
reference the specific HYPERFLEET ticket ID for tracking env variable support,
replacing the instruction to create a ticket; preserve the existing description
of the planned env.REGION and env.ENVIRONMENT access.
- Around line 177-180: Update the “type” field-length constraint in the
configuration example to document the implemented behavior: values exceeding 128
bytes cause validation to return an error and prevent startup, rather than being
skipped. Keep the existing truncation descriptions for “reason” and “message”
unchanged.
In `@go.mod`:
- Line 15: Update the direct github.com/google/cel-go dependency in go.mod from
v0.26.1 to v0.29.0 or newer, and refresh the module checksums or related
dependency metadata as required while preserving build compatibility.
In `@pkg/services/condition_mapper_test.go`:
- Around line 369-372: Replace the pullSecret value in the test data map near
data2 with a non-decodable placeholder that cannot represent auth JSON, and
update the corresponding value at the referenced later test location
consistently. Keep the test’s pullSecret coverage intact while removing the
base64-like secret pattern.
In `@pkg/services/resource.go`:
- Around line 612-617: Reduce unnecessary condition recomputation in
pkg/services/resource.go lines 612-617 by avoiding unconditional
hasMapper-triggered aggregation; run it only for triggerAggregation or when the
incoming status conditions/data differ from existingStatus. In
pkg/services/condition_mapper.go lines 376-393, change Apply so the resource
JSON marshal and MaskSensitiveFields traversal are performed lazily or once per
Apply call and reused across rule evaluations, rather than rebuilt for each
rule. Use the existing ProcessAdapterStatus and Apply flows as the anchors for
these changes.
In `@test/integration/condition_mapping_test.go`:
- Around line 255-290: The condition-mapping test currently uses PolicyValid for
both the adapter input and mapped output, making the assertion ineffective. In
the test setup and corresponding expected mapping, rename the source adapter
condition to a distinct type such as PolicyCheckPassed while keeping the
resource output and assertion as PolicyValid, so the mapping is verified.
- Around line 19-20: Gate TestConditionMapping_BEFORE with the inverse of
HYPERFLEET_TEST_CONDITION_MAPPING, matching the existing conditional gating in
TestConditionMapping_AFTER. Keep its QuotaValid-absent assertions unchanged so
the mutually exclusive tests run only against their corresponding
configurations.
---
Nitpick comments:
In `@pkg/config/conditions_test.go`:
- Around line 188-208: Extend the condition validation tests alongside the
existing invalid output status case to cover malformed CEL expressions in
output.reason and output.message. Add separate configurations using incomplete
expressions for each field, and assert wantError is true with the existing
“invalid CEL expression” error expectation, exercising the validateCELExpression
calls for both fields.
In `@pkg/config/conditions.go`:
- Around line 84-106: Deduplicate the repeated validation logic in the cluster
and nodepool sections of the configuration validation flow. Extract a helper
that accepts the entity name and condition-mapping collection, sorts its keys,
and calls validateConditionMapping for each entry while propagating errors;
invoke it for both c.Clusters and c.NodePools.
In `@pkg/services/condition_mapper_test.go`:
- Around line 1261-1392: Move the three Apply-focused subtests out of
TestTruncateUTF8 into appropriately named dedicated test functions, including
TestConditionMapper_ConcurrentApply for the concurrency case, so truncation
filters remain scoped correctly. In the concurrent test, remove the unused
result.err field and its assertion, and send/receive only
[]api.ResourceCondition through the channel while preserving the existing result
assertions.
In `@pkg/services/condition_mapper.go`:
- Around line 205-209: Update the error handling around validateFieldLengths in
NewConditionMapper’s condition-mapping flow: remove the redundant condition-type
length validation there, relying on ConditionsConfig.Validate() for validated
startup configuration. If validation can still fail for unvalidated
configurations, propagate or return the error instead of silently returning nil,
nil and dropping the condition.
In `@pkg/services/resource_test.go`:
- Around line 3160-3232: Extend
TestResourceService_ConditionMapper_IntegrationPath with a false-evaluating when
expression and assert no CustomReady condition is emitted. Add collision
coverage using mapped types Reconciled and ValidationSuccessful, asserting the
intended behavior from the condition-mapping logic without unintended overwrites
or duplicates. Reuse the existing test helpers and verify both outcomes through
rcDao.conditions.
In `@pkg/util/cel_test.go`:
- Around line 88-115: Extract the repeated CEL parse, check, program creation,
and evaluation sequence from the affected tests into a shared test helper marked
with t.Helper(). Have the helper accept the expression and resource data, return
the evaluated output and error, and update all three tests—including the block
around the additional referenced lines—to use it while preserving their existing
assertions and CEL cost limit.
- Around line 199-227: Extend TestToJsonFunc with error-path cases for the
toJSONFunc 1MB payload guard and CELCostLimit rejection. Feed toJson a
sufficiently large map and assert evaluation returns an error containing
“exceeds 1MB limit”; separately exercise a program that exceeds CELCostLimit and
assert the evaluation surfaces the cost-limit error.
In `@pkg/util/cel.go`:
- Around line 55-70: Update toJSONFunc so serialization enforces the 1MB limit
while writing, rather than calling json.Marshal and checking the fully allocated
result afterward. Use a size-capped JSON encoder or equivalent bounded writer,
preserve the existing toJson error behavior, and retain the maxJSONSize limit
without claiming it prevents allocations it cannot prevent.
In `@pkg/util/naming_test.go`:
- Around line 10-22: Extend the testCases table in the naming tests with
degenerate adapter inputs "", "-adapter", and "multi--word", asserting the
intended naming behavior for each. Use the existing naming function under test
and ensure these cases verify that empty or dash-edged names do not produce
unintended reserved condition types consumed by buildReservedConditionTypes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: ba0e7a9e-f99d-4e9b-b7fd-cd75c3691fa3
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum,!**/go.sum
📒 Files selected for processing (20)
configs/config.yaml.examplego.modpkg/config/conditions.gopkg/config/conditions_test.gopkg/config/config.gopkg/config/loader.gopkg/services/aggregation.gopkg/services/aggregation_test.gopkg/services/condition_mapper.gopkg/services/condition_mapper_test.gopkg/services/resource.gopkg/services/resource_test.gopkg/util/cel.gopkg/util/cel_test.gopkg/util/mask_sensitive.gopkg/util/mask_sensitive_test.gopkg/util/naming.gopkg/util/naming_test.goplugins/resources/plugin.gotest/integration/condition_mapping_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
💤 Files with no reviewable changes (1)
- pkg/services/aggregation_test.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/services/condition_mapper_test.go (1)
1364-1366: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemove the tautological error assertion.
result.erris never populated, andmapper.Applyreturns only conditions. ThereforeExpect(res.err).NotTo(HaveOccurred())always passes and provides no error-path coverage. Remove the field/assertion or capture a real error from an error-returning API.As per path instructions, error paths SHOULD be tested, not just represented by a never-populated error field.
Also applies to: 1387-1389, 1393-1396
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/services/condition_mapper_test.go` around lines 1364 - 1366, Remove the unused result.err field and all tautological Expect(res.err).NotTo(HaveOccurred()) assertions in the mapper.Apply tests, including the referenced cases. Keep assertions focused on the conditions returned by Apply; only test errors if an actual error-returning API is introduced.Source: Path instructions
🧹 Nitpick comments (1)
pkg/services/condition_mapper_test.go (1)
1403-1413: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMark
testBuildActivationas a test helper. Pass*testing.Tthrough, callt.Helper()immediately, and update the two call sites atpkg/services/condition_mapper_test.go:580and:889so failures point at the calling test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/services/condition_mapper_test.go` around lines 1403 - 1413, Update testBuildActivation to accept *testing.T, call t.Helper() as its first operation, and adjust both call sites around the tests at lines 580 and 889 to pass their testing instance through. Preserve the existing activation-building behavior while ensuring failures are attributed to the calling tests.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pkg/services/condition_mapper_test.go`:
- Around line 1364-1366: Remove the unused result.err field and all tautological
Expect(res.err).NotTo(HaveOccurred()) assertions in the mapper.Apply tests,
including the referenced cases. Keep assertions focused on the conditions
returned by Apply; only test errors if an actual error-returning API is
introduced.
---
Nitpick comments:
In `@pkg/services/condition_mapper_test.go`:
- Around line 1403-1413: Update testBuildActivation to accept *testing.T, call
t.Helper() as its first operation, and adjust both call sites around the tests
at lines 580 and 889 to pass their testing instance through. Preserve the
existing activation-building behavior while ensuring failures are attributed to
the calling tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: d45a63ca-d0b1-4251-8063-d27d97047dba
📒 Files selected for processing (2)
pkg/services/condition_mapper.gopkg/services/condition_mapper_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/services/condition_mapper.go
| conditions: | ||
| clusters: | ||
| # Example: Expose Landing Zone namespace readiness | ||
| # LandingZoneReady: |
There was a problem hiding this comment.
Viper lowercases map keys, so LandingZoneReady in YAML becomes landingzoneready internally
This means that the mapping names become lowercase.
Here is an output from one of my tests (see the last condition)
{
...
"kind": "Cluster",
"name": "my-cluster4",
...
"status": {
"conditions": [
{
...
"status": "False",
"type": "Reconciled"
},
{
...
"status": "False",
"type": "LastKnownReconciled"
},
{
"created_time": "2026-07-28T10:22:02.861305Z",
"last_transition_time": "2026-07-28T10:22:02.861305Z",
"last_updated_time": "2026-07-28T10:22:02.861305Z",
"message": "Landing zone: YYYYYYYYY YYYYYY",
"observed_generation": 2,
"reason": "XXXXXXXXX",
"status": "True",
"type": "landingzoneready"
}
]
},
There was a problem hiding this comment.
Good catch! Updated to follow the same pattern as adapter conditions in hyperfleet-infra — array with explicit type: field preserves PascalCase.
| # - reason: 256 bytes (truncated if exceeded, preserving UTF-8 boundaries) | ||
| # - message: 2048 bytes (truncated if exceeded, preserving UTF-8 boundaries) | ||
| conditions: | ||
| clusters: |
There was a problem hiding this comment.
If these are configured per entity type... what about having the conditions as part of the entity configuration?
entities:
- kind: Cluster
plural: clusters
spec_schema_name: ClusterSpec
required_adapters: [validation, dns, pullsecret, hypershift]
conditions:
--> HERE <--
There was a problem hiding this comment.
Migrated to entities[].conditions
| // Log error but don't fail the entire aggregation | ||
| logger.With(ctx, "resource_kind", m.resourceKind, "condition_type", rule.conditionType). | ||
| WithError(err). | ||
| Warn("Failed to evaluate condition mapping rule, skipping") |
There was a problem hiding this comment.
Warning
Blocking
Category: Architecture
The design doc (condition-mapping-design.md § Error Handling) says:
If a CEL expression fails, the entire mapping operation fails and the database transaction is rolled back. [...] Accepting partial mapping results would prevent timely retry.
But Apply() logs a warning and skips the failing rule — partial results are committed. If this is intentional (e.g., you decided skip-and-continue is better for availability), the design doc should be updated to match. Otherwise, Apply should return an error that propagates up to recomputeAndSaveResourceConditions and triggers rollback.
Address code review findings from PR review: - Move condition validation from pkg/config to pkg/registry (better cohesion) - Add blank lines between adjacent top-level function declarations - Update config.yaml.example documentation for Unknown filtering behavior - Format code with gofmt Changes: - pkg/registry/conditions.go: Moved from pkg/config (validation logic belongs with registry) - pkg/registry/conditions_test.go: Moved from pkg/config - configs/config.yaml.example: Clarify Unknown filtering (entire adapter status dropped) - pkg/services/condition_mapper_test.go: Add blank lines between functions - pkg/util/cel_test.go: Add blank lines between functions - pkg/util/mask_sensitive_test.go: Add blank line before TestIsSensitiveKey All tests passing. No functional changes. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Design doc (condition-mapping-design.md § Error Handling) mandates: "If a CEL expression fails, the entire mapping operation fails and the database transaction is rolled back." Changes: - Apply() signature: ([]api.ResourceCondition, error) instead of []api.ResourceCondition - CEL evaluation errors return error instead of skip-and-continue - resource.go propagates error as GeneralError → triggers rollback - Test: TestProcessAdapterStatus_ConditionMapperError_TriggersRollback Impact: CEL failures trigger 10s retry instead of 30min delay. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
/retest |
Without MarkForRollback, transaction commits despite error. Aligns with other error paths (lines 234, 265, 339, 932).
Expect(err) inside goroutine calls t.Fatal from non-test goroutine, which panics. Move assertion to main test goroutine instead. Fixes: send err through channel, assert on res.err after receive.
…ent skip validateFieldLengths error returned (nil, nil) - indistinguishable from when=false skip. Propagate error for consistency with CEL rollback-on-failure design (lines 170-177). Note: conditionType length validated at startup, so this is defense-in-depth (runtime path unreachable in practice).
When hasUnknown=true, adapterStatusToMapWithUnknownCheck allocated a full 4-key map that buildStatusesList immediately discarded. Return nil instead - caller guards with if !hasUnknown so never reads the map. Also adds test coverage for Unknown filtering path (missing coverage for hasUnknown=true code path).
| # Generic resource types registered at startup. Each entry auto-generates | ||
| # REST endpoints, spec validation, and delete policies. | ||
| # | ||
| # Condition Mapping (HYPERFLEET-538): |
There was a problem hiding this comment.
I suggest to remove jira task references across this file - "HYPERFLEET-538", to not tie config with Jira
| # | ||
| # Condition Mapping (HYPERFLEET-538): | ||
| # Each entity can define CEL-based condition mapping rules that expose | ||
| # provider-specific adapter conditions in the public status.conditions array. | ||
| # | ||
| # Rules are compiled at startup (fail-fast). Invalid CEL expressions prevent API startup. | ||
| # Evaluation happens during status aggregation. Unknown adapter conditions are filtered. | ||
| # | ||
| # Reserved condition types (cannot be overridden by mapping): | ||
| # - Reconciled | ||
| # - LastKnownReconciled | ||
| # - Per-adapter synthesized types (auto-generated from required_adapters): | ||
| # Example: "validation" adapter → "ValidationSuccessful" condition type | ||
| # | ||
| # CEL Context Variables: | ||
| # - statuses: array of adapter statuses (adapter entries with any Unknown condition are excluded entirely) | ||
| # Each status: adapter (string), observed_generation (number), conditions (array), data (map) | ||
| # - resource: full cluster/nodepool object as map (sensitive fields masked) | ||
| # - env: environment variables as map (currently always empty, future enhancement) | ||
| # | ||
| # Custom CEL Functions: | ||
| # - toJson(value): marshal to JSON string | ||
| # - dig(target, "dot.path"): safe nested navigation | ||
| # | ||
| # Security: Adapter data fields matching sensitive patterns (password, secret, token, | ||
| # auth, private, connection, cert, credential, etc.) are automatically masked with | ||
| # "***REDACTED***" before CEL evaluation. This prevents credential leakage in public | ||
| # condition messages/reasons. See pkg/util/mask_sensitive.go for the full pattern list. | ||
| # | ||
| # Field Length Constraints: | ||
| # - type: 128 bytes (validation error if exceeded, prevents startup) | ||
| # - reason: 256 bytes (truncated if exceeded) | ||
| # - message: 2048 bytes (truncated if exceeded) | ||
| # | ||
| entities: |
There was a problem hiding this comment.
I would decouple this block somewhere into /docs and instead, eg. docs/config.md, and keep only a link here or keep just the configuration example to keep it minimal
| if valErr := config.Metrics.Validate(); valErr != nil { | ||
| return fmt.Errorf("metrics config validation failed: %w", valErr) | ||
| } | ||
| // Conditions validation now happens in registry.Validate() after entity descriptors are loaded |
There was a problem hiding this comment.
Do we need this comment? (can be easily forgotten if anything changes which could make this statement not true)
| // Create condition mappers from entity descriptors | ||
| // Iterate all registered entities and build mappers from inline conditions | ||
| conditionMappers := make(map[string]*ConditionMapper) | ||
| for _, descriptor := range registry.All() { | ||
| if len(descriptor.Conditions) > 0 { | ||
| mapper, err := NewConditionMapper(descriptor.Kind, descriptor.Conditions) | ||
| if err != nil { | ||
| // This should not happen since config was validated at startup - indicates a code bug | ||
| // (e.g., validation logic vs. mapper logic mismatch). Use Error level to alert ops team, | ||
| // but continue in degraded mode (no CEL mapping) to keep service available per HYG-02. | ||
| logger.With(context.Background(), "resource_kind", descriptor.Kind). | ||
| WithError(err). | ||
| Error("Failed to create condition mapper, continuing without CEL mapping") | ||
| } else { | ||
| conditionMappers[descriptor.Kind] = mapper | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Small nit — this loop pulls straight from registry.All(), which is a bit inconsistent with the rest of the constructor since everything else (resourceDao, resourceLabelDao, etc.) is passed in explicitly. It also means the tests have to registry.Register(...) + t.Cleanup(registry.Reset) just to get a mapper in there, which is more than it needs to be.
Wonder if it'd be cleaner to pull this out into a buildConditionMappers(entities []registry.EntityDescriptor) helper next to NewConditionMapper in condition_mapper.go, and just call buildConditionMappers(registry.All()) here. Would keep the mapper-building logic where the rest of the mapper code lives, and make it easier to unit test without touching the global registry.
| "unicode" | ||
| ) | ||
|
|
||
| // adapterConditionSuffix is the suffix appended to adapter names when generating condition types (QUAL-01) |
There was a problem hiding this comment.
What does this stand for "QUAL-01"?
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
pkg/services/resource_test.go (1)
3280-3301: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe comment claims "standard conditions + mapped condition"; only the mapped one is asserted.
recomputeAndSaveResourceConditionsappends mapped conditions toreconciled,lastKnownReconciled, and the per-adapter conditions. A regression that replaces instead of appends would still pass this test. Assert that the reserved types survive.findConditionalready exists at Line 2357.💚 Proposed addition
Expect(customReady).ToNot(BeNil(), "CustomReady condition should be created by mapper") + Expect(findCondition(conditions, "Reconciled")).ToNot(BeNil(), + "mapped conditions must be appended to, not replace, aggregated conditions") + Expect(findCondition(conditions, "LastKnownReconciled")).ToNot(BeNil(), + "LastKnownReconciled must survive condition mapping") Expect(customReady.Status).To(Equal(api.ConditionTrue))As per path instructions: "New exported functions and critical logic paths SHOULD have tests".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/services/resource_test.go` around lines 3280 - 3301, Strengthen the test around the condition collection in the mapped-condition verification by using the existing findCondition helper to assert that the reserved standard condition types remain present alongside CustomReady. Keep the existing mapped-condition assertions, and verify the expected standard conditions are not replaced when mapped conditions are appended.Source: Path instructions
pkg/services/condition_mapper.go (1)
254-262: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe log message says "skipping"; the code rolls back the transaction.
validateFieldLengthsreturns an error here.evaluateRulepropagates it,Applypropagates it, andrecomputeAndSaveResourceConditionscallsdb.MarkForRollback. Nothing is skipped. An operator readingCondition type exceeds max length, skippingwill not connect it to a failed adapter status report.This branch is also unreachable in normal operation:
registry.ValidateEntityConditionsrejects a type longer thanMaxConditionTypeLengthat startup, so reaching it means the registry and the mapper disagree. Log at Error level and state the real effect, or drop the log and keep only the wrapped error.♻️ Proposed fix
if len(rule.conditionType) > registry.MaxConditionTypeLength { logger.With( ctx, "resource_kind", m.resourceKind, "condition_type", rule.conditionType, "length", len(rule.conditionType), - ).Warn("Condition type exceeds max length, skipping") - return "", "", fmt.Errorf("condition type exceeds max length") + ).Error("Condition type exceeds max length; failing mapping and rolling back") + return "", "", fmt.Errorf( + "condition type exceeds max length %d (got %d)", + registry.MaxConditionTypeLength, len(rule.conditionType), + ) }As per path instructions: "Log levels must match severity".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/services/condition_mapper.go` around lines 254 - 262, Update the length-validation branch in validateFieldLengths to use Error-level logging and describe that the condition type causes processing to fail and the transaction to roll back, rather than saying it is skipped. Keep the existing error return so evaluateRule, Apply, and recomputeAndSaveResourceConditions continue propagating the failure.Source: Path instructions
pkg/services/resource.go (1)
46-60: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winFail fast when
NewConditionMapperviolates validated registry invariants.
registry.Validate()runs beforeEnvironment().Initialize(), and both paths use the same CEL compilation pipeline andutil.CELCostLimit. Any error indicates an initialization defect. Panic, or return an initialization error after changing the constructor signature, instead of omitting mapped conditions fromstatus.conditions. Add a metric only if degraded mode remains intentional. (CWE-754)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/services/resource.go` around lines 46 - 60, Update the initialization flow around NewConditionMapper to fail fast when mapper creation returns an error, rather than logging and continuing without storing the mapper. Propagate an initialization error or panic consistently with the surrounding Environment initialization contract, ensuring invalid registry invariants cannot silently omit status conditions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@configs/config.yaml.example`:
- Around line 113-114: Update the configuration comment near the status
aggregation rules to state that adapters with unknown conditions are discarded
from statuses, matching parseConditionsWithUnknownCheck and buildStatusesList;
remove the claim that only individual unknown conditions are filtered.
In `@pkg/util/cel.go`:
- Around line 91-99: Update the encoder setup in toJSONFunc by removing the
SetEscapeHTML(false) override so encoding/json retains its default HTML escaping
for <, >, and &. Preserve the existing size-limit and error-handling behavior
around enc.Encode.
---
Nitpick comments:
In `@pkg/services/condition_mapper.go`:
- Around line 254-262: Update the length-validation branch in
validateFieldLengths to use Error-level logging and describe that the condition
type causes processing to fail and the transaction to roll back, rather than
saying it is skipped. Keep the existing error return so evaluateRule, Apply, and
recomputeAndSaveResourceConditions continue propagating the failure.
In `@pkg/services/resource_test.go`:
- Around line 3280-3301: Strengthen the test around the condition collection in
the mapped-condition verification by using the existing findCondition helper to
assert that the reserved standard condition types remain present alongside
CustomReady. Keep the existing mapped-condition assertions, and verify the
expected standard conditions are not replaced when mapped conditions are
appended.
In `@pkg/services/resource.go`:
- Around line 46-60: Update the initialization flow around NewConditionMapper to
fail fast when mapper creation returns an error, rather than logging and
continuing without storing the mapper. Propagate an initialization error or
panic consistently with the surrounding Environment initialization contract,
ensuring invalid registry invariants cannot silently omit status conditions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: cdb97833-c783-4716-96d1-d53a05fff313
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum,!**/go.sum
📒 Files selected for processing (20)
configs/config.yaml.examplego.modpkg/config/loader.gopkg/registry/conditions.gopkg/registry/conditions_test.gopkg/registry/descriptor.gopkg/registry/registry.gopkg/services/aggregation.gopkg/services/aggregation_test.gopkg/services/condition_mapper.gopkg/services/condition_mapper_test.gopkg/services/resource.gopkg/services/resource_test.gopkg/util/cel.gopkg/util/cel_test.gopkg/util/mask_sensitive.gopkg/util/mask_sensitive_test.gopkg/util/naming.gopkg/util/naming_test.gotest/integration/condition_mapping_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
💤 Files with no reviewable changes (1)
- pkg/services/aggregation_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- pkg/util/naming_test.go
- pkg/util/naming.go
- pkg/services/aggregation.go
- pkg/util/mask_sensitive.go
- test/integration/condition_mapping_test.go
- pkg/util/mask_sensitive_test.go
- go.mod
| # Rules are compiled at startup (fail-fast). Invalid CEL expressions prevent API startup. | ||
| # Evaluation happens during status aggregation. Unknown adapter conditions are filtered. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Line 114 contradicts line 123.
Line 114 states that unknown adapter conditions are filtered. The implementation in pkg/services/condition_mapper.go (parseConditionsWithUnknownCheck breaks and buildStatusesList discards the map) removes the whole adapter entry from statuses, not the single condition. Line 123 documents that correctly. An operator who reads only line 114 will write a when expression that never matches.
📝 Proposed fix
# Rules are compiled at startup (fail-fast). Invalid CEL expressions prevent API startup.
-# Evaluation happens during status aggregation. Unknown adapter conditions are filtered.
+# Evaluation happens during status aggregation. An adapter reporting any Unknown
+# condition is excluded from `statuses` entirely (see CEL Context Variables below).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Rules are compiled at startup (fail-fast). Invalid CEL expressions prevent API startup. | |
| # Evaluation happens during status aggregation. Unknown adapter conditions are filtered. | |
| # Rules are compiled at startup (fail-fast). Invalid CEL expressions prevent API startup. | |
| # Evaluation happens during status aggregation. An adapter reporting any Unknown | |
| # condition is excluded from `statuses` entirely (see CEL Context Variables below). |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@configs/config.yaml.example` around lines 113 - 114, Update the configuration
comment near the status aggregation rules to state that adapters with unknown
conditions are discarded from statuses, matching parseConditionsWithUnknownCheck
and buildStatusesList; remove the claim that only individual unknown conditions
are filtered.
| enc := json.NewEncoder(&buf) | ||
| enc.SetEscapeHTML(false) // Match json.Marshal behavior | ||
|
|
||
| if err := enc.Encode(v); err != nil { | ||
| if buf.exceeded { | ||
| return types.NewErr("toJson: output exceeds 1MB limit") | ||
| } | ||
| return types.NewErr("toJson: %v", err) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Go encoding/json does json.Marshal escape HTML by default and what does Encoder SetEscapeHTML(false) change
💡 Result:
Yes, Go's json.Marshal escapes specific HTML characters by default [1][2]. It automatically converts the characters "<", ">", "&", U+2028, and U+2029 into their Unicode-escaped forms (e.g., "<" becomes "\u003c") to ensure the generated JSON is safe to embed directly into HTML <script> tags without causing security issues [1][3][4]. The Encoder.SetEscapeHTML(false) method changes this behavior by disabling this automatic escaping [1][5]. When this is set to false, characters like "<", ">", and "&" remain as their literal selves in the output rather than being converted to Unicode escape sequences [2]. This is useful for scenarios where the JSON output is not intended for HTML embedding and readability is preferred [1][4]. Because json.Marshal does not expose an option to disable this behavior, you must use an Encoder if you wish to turn off HTML escaping [6][2]. Example usage: var buf bytes.Buffer enc:= json.NewEncoder(&buf) enc.SetEscapeHTML(false) enc.Encode(v) // Output will contain literal <, >, & characters [2]
Citations:
- 1: https://pkg.go.dev/encoding/json
- 2: https://wildandfreetools.com/blog/json-escape-in-go-golang/
- 3: https://go.dev/src/encoding/json/encode.go?h=return+err
- 4: https://golang.google.cn/pkg/encoding/json/
- 5: https://pkg.go.dev/encoding/json@go1.26.4
- 6: https://stackoverflow.com/questions/28595664/how-to-stop-json-marshal-from-escaping-and
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- pkg/util/cel.go ---'
cat -n pkg/util/cel.go | sed -n '1,180p'
printf '%s\n' '--- toJson usages ---'
rg -n -C 3 'toJson|ToJson|toJSON' --glob '*.go' . || true
printf '%s\n' '--- condition message/reason construction ---'
rg -n -C 3 'conditions|Condition|message|reason|Message|Reason' --glob '*.go' pkg | head -n 400 || trueRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 31609
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- relevant standards ---'
find . -path '*hyperfleet/standards/code-review*' -type f -maxdepth 8 -print 2>/dev/null | sort | head -n 100
rg -n -C 3 'SEC-0[1-3]|output encoding|CWE-116|HTML|condition' hyperfleet/standards/code-review . 2>/dev/null | head -n 300 || trueRepository: openshift-hyperfleet/architecture
Length of output: 29697
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- CEL tests ---'
cat -n pkg/util/cel_test.go | sed -n '190,275p'
printf '%s\n' '--- condition mapping and status aggregation symbols ---'
rg -n -C 4 'ConditionMapping|condition mapping|toJson\\(|message|reason|AdapterCondition|ClusterCondition|ResourceCondition' --glob '*.go' . | head -n 600
printf '%s\n' '--- API schemas and status documentation ---'
rg -n -C 3 'message|reason|conditions|Conditions' --glob '*.yaml' --glob '*.yml' --glob '*.json' --glob '*.md' . | head -n 500Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 39361
🏁 Script executed:
#!/bin/bash
set -eu
# Standalone standard-library probe. It does not import or execute repository code.
cat >/tmp/json_escape_probe.go <<'EOF'
package main
import (
"bytes"
"encoding/json"
"fmt"
)
func main() {
value := map[string]string{"value": "<tag>&"}
marshalResult, marshalErr := json.Marshal(value)
if marshalErr != nil {
panic(marshalErr)
}
var buf bytes.Buffer
encoder := json.NewEncoder(&buf)
encoder.SetEscapeHTML(false)
if err := encoder.Encode(value); err != nil {
panic(err)
}
fmt.Printf("json.Marshal: %s\n", marshalResult)
fmt.Printf("Encoder.SetEscapeHTML(false): %s", bytes.TrimSuffix(buf.Bytes(), []byte{'\n'}))
}
EOF
go run /tmp/json_escape_probe.go
rm -f /tmp/json_escape_probe.goRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 269
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- condition mapper files ---'
fd -i 'condition' . --type f | sort
printf '%s\n' '--- condition mapping references ---'
rg -n -C 5 'ConditionMapping|condition mapping|compileExpression|CELVarResource|CELVarStatuses|toJson' --glob '*.go' . | head -n 800
printf '%s\n' '--- assignments to condition reason/message ---'
rg -n -C 6 'Reason:|Message:|\.Reason|\.Message|reason.*condition|message.*condition' --glob '*.go' pkg | head -n 800
printf '%s\n' '--- configuration examples containing toJson ---'
rg -n -C 8 'toJson|message:|reason:' --glob '*.yaml' --glob '*.yml' --glob '*.json' --glob '*.md' config deploy charts docs . 2>/dev/null | head -n 800 || trueRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 50392
Restore HTML escaping in toJSONFunc. json.Marshal escapes <, >, and &; SetEscapeHTML(false) emits them literally. CEL mappings can place this output in public condition messages and reasons. Keep escaping enabled to prevent raw adapter-controlled markup from reaching consumers (CWE-116).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/util/cel.go` around lines 91 - 99, Update the encoder setup in toJSONFunc
by removing the SetEscapeHTML(false) override so encoding/json retains its
default HTML escaping for <, >, and &. Preserve the existing size-limit and
error-handling behavior around enc.Encode.
| References []ReferenceDescriptor `mapstructure:"references" json:"references,omitempty"` | ||
| // CEL-based condition mapping rules for this entity type | ||
| // Each rule maps adapter conditions to public API conditions | ||
| // See HYPERFLEET-538 for condition mapping design |
| // EntityDescriptor defines everything specific to a HyperFleet entity type. | ||
| // Descriptors are loaded from the application config YAML at startup via LoadDescriptors. | ||
| // | ||
| //nolint:govet // fieldalignment: gofmt field ordering conflicts with memory alignment optimization |
| for _, name := range m.sortedNames { | ||
| rule := m.rules[name] | ||
|
|
||
| // Lookup previous condition for this type (O(1) instead of O(N)) |
There was a problem hiding this comment.
I suggest running an agent to trim some comments like this, just noise
// Build lookup map for previous conditions to avoid O(N×M) linear scans
// Lookup previous condition for this type (O(1) instead of O(N))
| "resource_kind", m.resourceKind, | ||
| "condition_type", rule.conditionType, | ||
| "length", len(rule.conditionType), | ||
| ).Warn("Condition type exceeds max length, skipping") |
There was a problem hiding this comment.
the message says skipping, but tracing this path, it doesn't look like "skip" - it returns an error on top
|
|
||
| // Field length constraints | ||
| const ( | ||
| MaxConditionTypeLength = 128 |
| // Expected: adapter custom conditions do NOT appear in public status.conditions | ||
| // NOTE: This test will PASS when the API is configured WITHOUT CEL mapping. | ||
| func TestConditionMapping_BEFORE(t *testing.T) { | ||
| if os.Getenv("HYPERFLEET_TEST_CONDITION_MAPPING") != "" { |
There was a problem hiding this comment.
this variable appears nowhere else in the repo, not in makefile or configs 🤔
| // Core Masking Tests | ||
| // ============================================================================ | ||
|
|
||
| func TestMaskSensitiveFields(t *testing.T) { |
There was a problem hiding this comment.
I got a lot of errors while running these with the race flag. Could you verify, please?
| google.golang.org/genproto/googleapis/rpc v0.0.0-20260706201446-f0a921348800 // indirect | ||
| google.golang.org/grpc v1.82.0 // indirect | ||
| google.golang.org/protobuf v1.36.11 // indirect | ||
| gopkg.in/yaml.v3 v3.0.1 // indirect |
There was a problem hiding this comment.
go mod tidy bringing it back to indirect
| return types.NullValue | ||
| } | ||
| current = next | ||
| case []interface{}: |
There was a problem hiding this comment.
[]interface{} != []map[string]interface{} in Go, slices are invariant (thanks Go)
dig() returns null on real data because of this
4 participants
Summary
Implements a CEL-based condition mapping engine that allows declarative exposure of adapter-specific conditions in the public API
status.conditionsarray without code changes. Operators can now configureYAML-based mapping rules that compile at startup (fail-fast) and evaluate at runtime to transform rich adapter status into public conditions.
Motivation
Currently, the HyperFleet API only exposes aggregated conditions (
Reconciled,LastKnownReconciled) and per-adapter success flags (AdapterXSuccessful) in the public API. Rich provider-specific conditionsfrom adapters (e.g., Landing Zone namespace readiness, Validation quota status) exist in the internal
/statusesendpoint but are not accessible to external consumers (CLI, UI, customer integrations).This creates a gap: operators cannot surface meaningful adapter state (quota limits, policy validation, namespace readiness) to end users without API code changes for each new condition type.
This PR closes that gap.
What Changed
🎯 Core Features
1. Config-driven Mapping Rules (
pkg/config/conditions.go)conditions.clusters.<ConditionType>andconditions.nodepools.<ConditionType>when: CEL expression (boolean) - when to generate the conditionoutput.status/reason/message: CEL expressions for field valuesReconciled,LastKnownReconciled)2. CEL Evaluation Engine (
pkg/services/condition_mapper.go)statuses: array of adapter statuses (Unknown conditions filtered)resource: full cluster/nodepool object (sensitive fields masked)env: environment variables (reserved for future use)pkg/util/cel.go):toJson(value): marshal to JSON stringdig(target, "dot.path"): safe nested navigation (map keys + array indices)3. Integration (
pkg/services/resource.go,pkg/services/aggregation.go)status.conditionsarray (after Reconciled/LastKnownReconciled)nilmapper → no CEL mapping (existing behavior)4. Security (
pkg/util/mask_sensitive.go)MaskSensitiveFields()applied to bothresourceanddatacontext variables5. Performance Optimizations
Unknowncondition found (PERF-03) - skips JSON parsing + maskingbuildActivation()- skips nil adapter statuses📦 Files Changed
New files (11):
pkg/config/conditions.go- Config schema & validationpkg/config/conditions_test.go- 7 validation testspkg/services/condition_mapper.go- CEL mapper implementationpkg/services/condition_mapper_test.go- 9 mapper testspkg/util/cel.go- CEL environment & custom functionspkg/util/cel_test.go- 5 CEL function testspkg/util/mask_sensitive.go- Sensitive data maskingpkg/util/mask_sensitive_test.go- Masking testspkg/util/naming.go- Adapter name → condition type helperpkg/util/naming_test.go- Naming teststest/integration/condition_mapping_test.go- 3 integration testsModified files (10):
pkg/config/config.go- AddConditionsfield toApplicationConfigpkg/config/loader.go- CallConditions.Validate()at startuppkg/services/resource.go- Create mapper, trigger recomputationpkg/services/aggregation.go- Append mapped conditionspkg/services/aggregation_test.go- Updated test expectationspkg/services/resource_test.go- Added mapper integration testplugins/resources/plugin.go- Pass config to service constructorconfigs/config.yaml.example- CEL mapping documentation + examplego.mod/go.sum- Addgithub.com/google/cel-go v0.26.1Example Usage
config.yaml:
Testing
✅ Unit Tests (24 tests)
Config validation (pkg/config/conditions_test.go - 7 tests):
Mapper logic (pkg/services/condition_mapper_test.go - 9 tests):
CEL functions (pkg/util/cel_test.go - 5 tests):
Masking (pkg/util/mask_sensitive_test.go - tests):
✅ Integration Tests (3 tests)
End-to-end (test/integration/condition_mapping_test.go):
✅ Test Results
DONE 1476 tests in 21.998s
✅ 100% passing
Backward Compatibility
✅ No breaking changes
Rollback Plan
If issues discovered post-merge:
Security Considerations
✅ Defense-in-depth implemented:
✅ CEL security:
Checklist
Notes for Reviewers
🔍 Key Files to Review
Core logic:
Security:
4. pkg/util/mask_sensitive.go - Sensitive data protection
Testing:
5. pkg/services/condition_mapper_test.go - Comprehensive test coverage