HYPERFLEET-538 - feat: CEL-based condition mapping engine - #315
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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds configurable CEL-based condition mapping for clusters and nodepools. Startup validation checks reserved names, field limits, CEL syntax, function usage, and evaluation cost. Runtime mapping masks sensitive data, filters unknown adapter conditions, evaluates deterministic rules, preserves timestamps, and appends mapped conditions during resource recomputation. Adapter condition naming is centralized in Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AdapterStatus
participant ResourceService
participant ConditionMapper
participant ResourceStore
AdapterStatus->>ResourceService: provide adapter statuses
ResourceService->>ConditionMapper: apply configured CEL mappings
ConditionMapper->>ConditionMapper: evaluate rules and build conditions
ConditionMapper-->>ResourceService: return mapped conditions
ResourceService->>ResourceStore: persist recomputed conditions
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 | 5000 lines (>500) | +2 |
| Sensitive paths | none | +0 |
| Test coverage | Missing tests for: plugins/resources | +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
| # - env: environment variables as map (NOT YET IMPLEMENTED - currently always empty) | ||
| # TODO: Create HYPERFLEET ticket for env variable support in CEL context | ||
| # Feature: Allow CEL expressions to access runtime config (e.g., env.REGION, env.ENVIRONMENT) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
TODO has no ticket ID.
The TODO asks the reader to create the ticket, which is the violation itself. File the HYPERFLEET issue and reference the ID here.
Want me to open the tracking issue for CEL env context support?
As per path instructions: "TODOs and FIXMEs must reference a ticket ID".
🤖 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 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.
Source: Path instructions
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"
}
]
},
| # - 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 <--
| // 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.
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
2 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