ROSAENG-61802: feat: add field validation middleware using codegen registry - #275
Closed
cdoan1 wants to merge 4 commits into
Closed
ROSAENG-61802: feat: add field validation middleware using codegen registry#275cdoan1 wants to merge 4 commits into
cdoan1 wants to merge 4 commits into
Conversation
…and codegen pipeline New standalone module at api/public/v2alpha1/ with generated passthrough types (HostedClusterSpecPassthrough, NodePoolSpecPassthrough), envelope types (Cluster, NodePool), configuration mirror types, and per-field markers for write-mode, visibility, and feature gates. Adds platform-api codegen packages: field metadata registry (120 fields), feature gate registry (6 gates), and conversion helpers. Adds Makefile codegen pipeline (make codegen) and updates verify/deps targets. v1alpha1 internal CRD types are unchanged. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The marker-scanner produced flat registry keys (e.g. "pausedUntil") with no root-type namespace, so fields with the same JSON name in different passthrough types silently overwrote each other with non-deterministic results. Prefix passthrough fields with their root type context (spec.hostedCluster.* / spec.nodePool.*) to match the paths that downstream consumers already construct. Add a verify-codegen Makefile target that re-runs the full codegen pipeline and fails on git diff, same pattern as verify-clientset, so CI catches stale generated code. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
openshift-ci-robot
added
the
jira/valid-reference
Collaborator
|
@cdoan1: This pull request references ROSAENG-61802 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cdoan1 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
Contributor
WalkthroughThe PR adds a public ChangesPublic API and validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ClusterHandler
participant FieldValidator
participant FieldRegistry
Client->>ClusterHandler: Submit create or update request
ClusterHandler->>FieldValidator: Validate specification
FieldValidator->>FieldRegistry: Read field metadata
FieldValidator-->>ClusterHandler: Return validation errors
alt Validation fails
ClusterHandler-->>Client: HTTP 422 validation response
else Validation succeeds
ClusterHandler-->>Client: Continue request processing
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 9 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
platform-api/internal/codegen/featuregate/types.go (1)
6-10: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the ordinal dependency between the stage constants and
Includes.
Includescompares stages by ordinal value. The result is correct only while theiotablock stays ordered from least to most restrictive. If someone reorders the constants later, gate availability changes silently and no test in this layer detects it. Add a comment that records the ordering contract.♻️ Proposed comment
const ( + // Order matters: FeatureSet.Includes compares stages by ordinal value, + // so stages must stay ordered from most stable to least stable. GA FeatureStage = iota TechPreview DevPreview )Also applies to: 51-53
🤖 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 `@platform-api/internal/codegen/featuregate/types.go` around lines 6 - 10, Add a comment immediately above the FeatureStage constants documenting that their iota ordinal values must remain ordered from least to most restrictive because Includes compares stages by ordinal. Keep the GA, TechPreview, and DevPreview declarations unchanged.platform-api/pkg/validation/field_validator.go (1)
82-101: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSort the returned errors for a stable 422 response.
validateiteratesfields, which is a Go map with randomized iteration order. The handlers serializeerrsdirectly into theerrorsarray of the 422 body. The order of reported fields changes between identical requests.This also affects the tests.
platform-api/pkg/validation/field_validator_test.golines 87 and 120 indexerrs[0]. Each of those tests registers one entry, so only one error is possible today. The assertions become flaky if a second entry is added.Sort
errsbyFieldbefore returning.♻️ Proposed refactor
if len(errs) > 0 { + sort.Slice(errs, func(i, j int) bool { return errs[i].Field < errs[j].Field }) return errs } return nilAdd
"sort"to the imports.🤖 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 `@platform-api/pkg/validation/field_validator.go` around lines 82 - 101, Update validate to sort the accumulated errs slice by each ValidationError’s Field before returning it, adding the sort import as needed. Preserve the existing validation and error-generation behavior while ensuring 422 responses have deterministic field ordering.platform-api/pkg/handlers/cluster.go (1)
312-322: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
writeValidationErrorsis duplicated across both handlers. The two functions are identical except for the error code string, and each one also repeats the body of the neighboringwriteError. The shared root cause is a missing common error-response helper, which lets the 422 envelope drift between handlers.
platform-api/pkg/handlers/cluster.go#L312-L322: replace the body with a call to a shared helper, passing"CLUSTERS-MGMT-VALIDATION-001"anderrs.platform-api/pkg/handlers/nodepool.go#L266-L276: replace the body with a call to the same shared helper, passing"NODEPOOLS-MGMT-VALIDATION-001"anderrs.🤖 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 `@platform-api/pkg/handlers/cluster.go` around lines 312 - 322, The duplicated writeValidationErrors implementations in platform-api/pkg/handlers/cluster.go lines 312-322 and platform-api/pkg/handlers/nodepool.go lines 266-276 should delegate to one shared error-response helper. Update the cluster handler to pass "CLUSTERS-MGMT-VALIDATION-001" and errs, and the nodepool handler to pass "NODEPOOLS-MGMT-VALIDATION-001" and errs; centralize the existing 422 JSON envelope in that helper.platform-api/internal/codegen/conversion/cluster.go (1)
10-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the doc comment with the placement behavior.
The comment states the function "strips client-supplied service-set fields and replaces them with platform-injected values".
cloudUrlandcreatorARNfollow that rule, because the code deletes them unconditionally before the conditional re-set.placementdoes not. The code preserves a non-empty client-suppliedplacementand only populates it when absent.If preserving a client value is intended, record that exception in the comment. If
placementis service-set, delete it first like the other two fields.♻️ Proposed comment update
// InjectClusterServiceSet strips client-supplied service-set fields and // replaces them with platform-injected values. +// Exception: placement is preserved when the client supplies a non-empty +// value, and is populated from ssf only when absent. func InjectClusterServiceSet(spec map[string]interface{}, ssf ClusterServiceSetFields) {Also applies to: 26-31
🤖 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 `@platform-api/internal/codegen/conversion/cluster.go` around lines 10 - 11, Update the InjectClusterServiceSet documentation to accurately describe placement behavior: state that cloudUrl and creatorARN are always replaced with platform-injected values, while a non-empty client-supplied placement is preserved and only populated when absent; alternatively, if placement should be platform-controlled, delete it unconditionally before resetting it like the other fields.platform-api/pkg/validation/field_validator_test.go (1)
148-185: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for the registry key space, unknown paths, and unchanged service-set values.
Every test registers keys that already carry the
spec.prefix, so none of them exercises the key mismatch described onplatform-api/internal/codegen/registry/field_metadata.go. Three cases are missing:
- An un-prefixed registry key such as
machineConfig.fips. The validator computesspec.machineConfig.fips, so the rule does not apply. A test pins this contract.- A field that is absent from the registry.
validateskips it at line 84 offield_validator.go. A test records that this fail-open behavior is intended.ValidateUpdatewith aServiceSetfield whose value is unchanged.TestValidateUpdate_RejectsServiceSetFieldschanges the value, so it does not cover the read-modify-write case.💚 Proposed tests
func TestValidateCreate_UnprefixedRegistryKeyIsNotMatched(t *testing.T) { v := newTestValidator(map[string]registry.FieldMeta{ // Un-prefixed key, as emitted today for non-passthrough root types. "machineConfig.fips": {FieldPath: "machineConfig.fips", WriteMode: registry.ServiceSet}, }) spec := map[string]any{"machineConfig": map[string]any{"fips": true}} if errs := v.ValidateCreate(spec, featuregate.Default); errs != nil { t.Fatalf("un-prefixed key unexpectedly matched: %v", errs) } t.Log("documents that un-prefixed registry keys are unreachable") } func TestValidateCreate_UnknownFieldIsSkipped(t *testing.T) { v := newTestValidator(map[string]registry.FieldMeta{}) spec := map[string]any{"somethingNew": "value"} if errs := v.ValidateCreate(spec, featuregate.Default); errs != nil { t.Errorf("expected unregistered field to be skipped, got %v", errs) } } func TestValidateUpdate_ServiceSetUnchangedValue(t *testing.T) { v := newTestValidator(map[string]registry.FieldMeta{ "spec.creatorARN": {FieldPath: "spec.creatorARN", WriteMode: registry.ServiceSet}, }) existing := map[string]any{"creatorARN": "same-arn"} updated := map[string]any{"creatorARN": "same-arn"} errs := v.ValidateUpdate(updated, existing, featuregate.Default) // Today this returns an error. Assert the behavior you intend. if errs != nil { t.Logf("read-modify-write of an unchanged service-set field is rejected: %v", errs) } }🤖 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 `@platform-api/pkg/validation/field_validator_test.go` around lines 148 - 185, Add tests covering the validator’s registry key-space behavior and unchanged service-set updates. Extend the tests near TestValidateUpdate_RejectsServiceSetFields and TestFlattenToFieldPaths with cases for an unprefixed registry key remaining unmatched, an unregistered field being skipped, and ValidateUpdate receiving an unchanged ServiceSet value; assert each case’s intended current behavior, including the unchanged-value outcome.
🤖 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 `@api/public/v2alpha1/hostedclusterspec.passthrough.go`:
- Around line 94-97: Update the Configuration field in the passthrough generator
override to use the local v2alpha1 ClusterConfiguration type defined in
configuration.go instead of hypershiftv1beta1.ClusterConfiguration, then
regenerate hostedclusterspec.passthrough.go. Add a regression test covering
nested configuration paths and confirming the local marker-bearing type is used.
In `@Makefile`:
- Around line 329-353: Update the codegen pipeline around codegen-passthrough
and verify-codegen so the generated zz_generated.passthrough.go.raw is applied
to or compared against the effective checked-in passthrough source before
generate-public-deepcopy and codegen-registry run. Ensure verify-codegen fails
when passthrough output differs, preventing stale field definitions and
validation metadata from reaching later stages.
In `@platform-api/pkg/handlers/cluster.go`:
- Around line 115-118: Replace featuregate.Default at all four validation sites
with the feature set resolved for the relevant caller or existing resource:
cluster.go lines 115-118 for ValidateCreate, cluster.go lines 226-229 for
ValidateUpdate, nodepool.go lines 102-105 using the referenced cluster for
ValidateCreate, and nodepool.go lines 191-194 using the existing node pool’s
cluster for ValidateUpdate. Ensure each resolved feature set is passed to
ValidateCreate or ValidateUpdate so gated validation and validateWriteMode can
operate correctly.
In `@platform-api/pkg/validation/field_validator.go`:
- Around line 131-136: Update the ServiceSet handling in ValidateUpdate to
compare each submitted value with its existing value, accepting unchanged values
and rejecting only modifications, matching the behavior of the Immutable branch.
Ensure this comparison applies to parent ServiceSet paths recorded by flattenMap
without rejecting unchanged child fields during full-spec read-modify-write
updates.
---
Nitpick comments:
In `@platform-api/internal/codegen/conversion/cluster.go`:
- Around line 10-11: Update the InjectClusterServiceSet documentation to
accurately describe placement behavior: state that cloudUrl and creatorARN are
always replaced with platform-injected values, while a non-empty client-supplied
placement is preserved and only populated when absent; alternatively, if
placement should be platform-controlled, delete it unconditionally before
resetting it like the other fields.
In `@platform-api/internal/codegen/featuregate/types.go`:
- Around line 6-10: Add a comment immediately above the FeatureStage constants
documenting that their iota ordinal values must remain ordered from least to
most restrictive because Includes compares stages by ordinal. Keep the GA,
TechPreview, and DevPreview declarations unchanged.
In `@platform-api/pkg/handlers/cluster.go`:
- Around line 312-322: The duplicated writeValidationErrors implementations in
platform-api/pkg/handlers/cluster.go lines 312-322 and
platform-api/pkg/handlers/nodepool.go lines 266-276 should delegate to one
shared error-response helper. Update the cluster handler to pass
"CLUSTERS-MGMT-VALIDATION-001" and errs, and the nodepool handler to pass
"NODEPOOLS-MGMT-VALIDATION-001" and errs; centralize the existing 422 JSON
envelope in that helper.
In `@platform-api/pkg/validation/field_validator_test.go`:
- Around line 148-185: Add tests covering the validator’s registry key-space
behavior and unchanged service-set updates. Extend the tests near
TestValidateUpdate_RejectsServiceSetFields and TestFlattenToFieldPaths with
cases for an unprefixed registry key remaining unmatched, an unregistered field
being skipped, and ValidateUpdate receiving an unchanged ServiceSet value;
assert each case’s intended current behavior, including the unchanged-value
outcome.
In `@platform-api/pkg/validation/field_validator.go`:
- Around line 82-101: Update validate to sort the accumulated errs slice by each
ValidationError’s Field before returning it, adding the sort import as needed.
Preserve the existing validation and error-generation behavior while ensuring
422 responses have deterministic field ordering.
🪄 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: Repository: openshift-online/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3ca36363-daec-45db-858a-0ad9d1d5d33f
⛔ Files ignored due to path filters (4)
api/public/v2alpha1/go.sumis excluded by!**/*.sumapi/public/v2alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*hack/api-codegen/go.sumis excluded by!**/*.sumplatform-api/go.sumis excluded by!**/*.sum
📒 Files selected for processing (22)
.gitignoreMakefileapi/public/v2alpha1/cluster_types.goapi/public/v2alpha1/configuration.goapi/public/v2alpha1/go.modapi/public/v2alpha1/groupversion_info.goapi/public/v2alpha1/hostedclusterspec.passthrough.goapi/public/v2alpha1/nodepool_types.gohack/api-codegen/go.modhack/api-codegen/pkg/markers/scanner.gohack/api-codegen/pkg/markers/scanner_test.goplatform-api/Containerfileplatform-api/go.modplatform-api/internal/codegen/conversion/cluster.goplatform-api/internal/codegen/featuregate/registry.goplatform-api/internal/codegen/featuregate/types.goplatform-api/internal/codegen/registry/field_metadata.goplatform-api/internal/codegen/registry/field_metadata.jsonplatform-api/pkg/handlers/cluster.goplatform-api/pkg/handlers/nodepool.goplatform-api/pkg/validation/field_validator.goplatform-api/pkg/validation/field_validator_test.go
17 tasks
Wire the generated FieldRegistry into cluster and nodepool handlers to enforce write-mode and feature-gate rules on create/update requests. - Service-set fields rejected if sent by customers (422) - Immutable fields rejected on update if changed (422) - Feature-gated fields rejected without the gate enabled (422) - Validation errors returned with per-field detail Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
cdoan1
force-pushed
the
ROSAENG-61802-field-validation-v2
branch
from
August 4, 2026 22:36
a62d5a7 to
c73b046
Compare
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@platform-api/internal/codegen/registry/field_metadata.json`:
- Around line 261-270: Update the registry metadata scanner and the entries for
registryBurst and registryPullQPS so generated field paths use the spec. prefix
matching flattenToFieldPaths output, including the kubelet hierarchy where
applicable. Ensure exact metadata lookup enforces HyperFleetKubeletAdvanced, and
add a regression test verifying these fields are rejected when the gate is
disabled.
🪄 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: Repository: openshift-online/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 388e92cf-5b04-4002-b533-56186c8aa60d
⛔ Files ignored due to path filters (4)
api/public/v2alpha1/go.sumis excluded by!**/*.sumapi/public/v2alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*hack/api-codegen/go.sumis excluded by!**/*.sumplatform-api/go.sumis excluded by!**/*.sum
📒 Files selected for processing (22)
.gitignoreMakefileapi/public/v2alpha1/cluster_types.goapi/public/v2alpha1/configuration.goapi/public/v2alpha1/go.modapi/public/v2alpha1/groupversion_info.goapi/public/v2alpha1/hostedclusterspec.passthrough.goapi/public/v2alpha1/nodepool_types.gohack/api-codegen/go.modhack/api-codegen/pkg/markers/scanner.gohack/api-codegen/pkg/markers/scanner_test.goplatform-api/Containerfileplatform-api/go.modplatform-api/internal/codegen/conversion/cluster.goplatform-api/internal/codegen/featuregate/registry.goplatform-api/internal/codegen/featuregate/types.goplatform-api/internal/codegen/registry/field_metadata.goplatform-api/internal/codegen/registry/field_metadata.jsonplatform-api/pkg/handlers/cluster.goplatform-api/pkg/handlers/nodepool.goplatform-api/pkg/validation/field_validator.goplatform-api/pkg/validation/field_validator_test.go
🚧 Files skipped from review as they are similar to previous changes (20)
- platform-api/Containerfile
- .gitignore
- hack/api-codegen/go.mod
- platform-api/go.mod
- platform-api/internal/codegen/registry/field_metadata.go
- platform-api/pkg/handlers/nodepool.go
- platform-api/internal/codegen/conversion/cluster.go
- api/public/v2alpha1/groupversion_info.go
- hack/api-codegen/pkg/markers/scanner.go
- api/public/v2alpha1/hostedclusterspec.passthrough.go
- platform-api/internal/codegen/featuregate/types.go
- hack/api-codegen/pkg/markers/scanner_test.go
- api/public/v2alpha1/configuration.go
- platform-api/pkg/handlers/cluster.go
- platform-api/pkg/validation/field_validator_test.go
- api/public/v2alpha1/go.mod
- platform-api/pkg/validation/field_validator.go
- api/public/v2alpha1/nodepool_types.go
- api/public/v2alpha1/cluster_types.go
- platform-api/internal/codegen/featuregate/registry.go
13 tasks
Go structs without omitempty serialize zero values to JSON, causing the field validator to reject fields the client never explicitly set. Skip ServiceSet enforcement when the field value is at its zero value. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
platform-api/pkg/validation/field_validator.go (1)
131-139: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAccept unchanged
ServiceSetvalues during updates.
ServiceSetdoes not inspectoporexistingFields. An update with an unchanged non-zero service-managed value returns 422. Compare the submitted value with the existing value before rejection.🤖 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 `@platform-api/pkg/validation/field_validator.go` around lines 131 - 139, Update the ServiceSet branch in the field validation switch to allow non-zero submitted values when they match the corresponding existingFields value during an update. Preserve acceptance of zero values, and continue returning the platform-managed ValidationError for changed or newly provided values.
🤖 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 `@platform-api/pkg/validation/field_validator.go`:
- Around line 131-135: Update field_validator.go at lines 131-135 and 177-203 to
use request-presence metadata from flattenToFieldPaths when deciding whether a
ServiceSet field was omitted, rather than relying on isZeroValue; allow only
truly absent fields and reject explicit false, 0, empty string, null, and
empty-object writes. Extend field_validator_test.go at lines 44-65 with create
and update cases covering these explicit zero values.
---
Duplicate comments:
In `@platform-api/pkg/validation/field_validator.go`:
- Around line 131-139: Update the ServiceSet branch in the field validation
switch to allow non-zero submitted values when they match the corresponding
existingFields value during an update. Preserve acceptance of zero values, and
continue returning the platform-managed ValidationError for changed or newly
provided values.
🪄 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: Repository: openshift-online/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7958ca5d-a0d4-4311-be69-7314d47ae242
📒 Files selected for processing (2)
platform-api/pkg/validation/field_validator.goplatform-api/pkg/validation/field_validator_test.go
Comment on lines
+131
to
+135
| switch effectiveMode { | ||
| case registry.ServiceSet: | ||
| if isZeroValue(fields[fieldPath]) { | ||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect how handlers decode requests and persist validated specifications.
ast-grep outline platform-api/pkg/handlers/cluster.go --items all
ast-grep outline platform-api/pkg/handlers/nodepool.go --items all
rg -n -C 6 'ValidateCreate|ValidateUpdate|req\.Spec|json\.NewDecoder|Decode\(|Create\(|Update\(' \
platform-api/pkg/handlers/cluster.go \
platform-api/pkg/handlers/nodepool.go
# Locate request-presence or raw-payload handling that can distinguish omitted
# fields from explicit JSON zero values.
rg -n -C 4 'RawMessage|Unstructured|map\[string\]any|json\.NewDecoder|Decode\(' \
platform-apiRepository: openshift-online/rosa-hyperfleet-api
Length of output: 50393
Authorization Bypass (CWE-863): Incorrect Authorization
Reachability: External · Exploitability: Moderate
Reachability path
● Entry
platform-api/pkg/validation/field_validator_test.go:25
ValidateCreate
│
▼
● Sink
platform-api/pkg/validation/field_validator.go
Preserve field presence before applying ServiceSet authorization.
flattenToFieldPaths preserves explicit JSON zero values, and isZeroValue currently treats them as omitted. This allows external callers to submit forbidden ServiceSet fields with values such as false, 0, "", null, or empty objects.
Use request-presence metadata for omission checks. Reject explicit zero-value ServiceSet writes, and update the tests to cover create and update requests.
📍 Affects 2 files
platform-api/pkg/validation/field_validator.go#L131-L135(this comment)platform-api/pkg/validation/field_validator.go#L177-L203platform-api/pkg/validation/field_validator_test.go#L44-L65
🤖 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 `@platform-api/pkg/validation/field_validator.go` around lines 131 - 135,
Update field_validator.go at lines 131-135 and 177-203 to use request-presence
metadata from flattenToFieldPaths when deciding whether a ServiceSet field was
omitted, rather than relying on isZeroValue; allow only truly absent fields and
reject explicit false, 0, empty string, null, and empty-object writes. Extend
field_validator_test.go at lines 44-65 with create and update cases covering
these explicit zero values.
|
@cdoan1: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Type of Change
Testing
Unit tests for all validation paths (service-set, immutable, mutable, feature-gated, nil spec, FeatureGateAwareWriteMode)
Verify create request with a service-set field returns 422 with field-level detail
Verify update request changing an immutable field returns 422
Verify feature-gated field accepted when gate is enabled, rejected when not
Unit tests pass (
make test)Integration tests pass (if applicable)
Manual verification completed
Checklist
Summary by CodeRabbit