Skip to content

fix(operations): two-pass reconfigure validation to prevent partial CP pollution - #10438

Closed
weicao wants to merge 3 commits into
mainfrom
bugfix/reconfigure-prevalidate-two-pass
Closed

fix(operations): two-pass reconfigure validation to prevent partial CP pollution#10438
weicao wants to merge 3 commits into
mainfrom
bugfix/reconfigure-prevalidate-two-pass

Conversation

@weicao

@weicao weicao commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Separate Action() into validate-all then apply-all (two-pass) so that an invalid parameter in a later Reconfigure entry cannot leave dirty ComponentParameter.spec.desired from an earlier entry.
  • For sharding, resolve each shard's actual ComponentDefinition via ListShardingComponents for schema validation, instead of using only the sharding template default.
  • Add three new test cases covering: single invalid parameter rejection, multi-entry first-valid/second-invalid no-dirty-CP invariant, and sharding with distinct template vs actual CompDef regression guard.

Problem

The original PR #10415 had two P1 issues identified by @leon-ape:

  1. Validation and patching were interleaved in a single loop — if entry[0] patched CP desired and entry[1] failed validation, entry[0]'s dirty write remained.
  2. Sharding validation only checked the template default ComponentDefinition, missing heterogeneous shard CompDefs.

Solution

Pass 1 iterates all Reconfigure entries and calls validateReconfigureParameters() which resolves the actual ComponentDefinition(s), loads their ParametersDefinition JSON schema, and validates parameter assignments. Any failure returns a fatal error immediately — no CP is touched.

Pass 2 only runs after all validations pass, applying patches via applyReconfigureToParameters().

For sharding, resolveCompDefNames() calls sharding.ListShardingComponents() to get the real Component objects and their Spec.CompDef, collecting unique CompDef names for validation.

Test plan

  • rejects unknown parameter before patching CP desired — single invalid param rejected, CP desired clean
  • multi-entry: invalid second entry blocks valid first entry from patching CP — two components, first valid + second invalid, neither CP patched
  • sharding: validates against actual shard ComponentDefinition, not template default — template CompDef has no schema, actual shard CompDef has strict schema; only the ListShardingComponents path rejects unknown_param
  • Existing Test Reconfigure OpsRequest and propagates ComponentParameter merge failure still pass
  • go build, go vet, git diff --check clean
  • Jade peer review: no design-contract blocker

Supersedes #10415 (addresses P1 review comments).

weicao and others added 3 commits June 23, 2026 16:36
…P pollution

Separate Action() into validate-all then apply-all so that an invalid
parameter in a later Reconfigure entry cannot leave dirty CP desired
from an earlier entry. Also resolve each shard actual CompDef for
schema validation instead of using only the sharding template default.

Fixes: PR #10415 P1 review comments

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ion tests

Cover both P1 review scenarios:
- Multi-entry: valid first entry + invalid second entry verifies neither
  CP desired gets patched (two-pass invariant)
- Sharding: validates against actual shard ComponentDefinition, not just
  template default

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…actual CompDef

Use templateCompDef (no schema) for the sharding template and
actualShardCompDef (strict schema) for the real shard Component.
If the code regressed to checking only the template default, validation
would pass; only the ListShardingComponents path rejects unknown_param.
Also add CP desired no-dirty assertion.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@apecloud-bot

Copy link
Copy Markdown
Collaborator

Auto Cherry-pick Instructions

Usage:
  - /nopick: Not auto cherry-pick when PR merged.
  - /pick: release-x.x [release-x.x]: Auto cherry-pick to the specified branch when PR merged.

Example:
  - /nopick
  - /pick release-1.1

CLA Recheck Instructions

Usage:
  - /recheck-cla: Trigger a re-check of CLA status for this pull request.
Example:
  - /recheck-cla

@github-actions github-actions Bot added the size/L Denotes a PR that changes 100-499 lines. label Jun 23, 2026
@codecov

codecov Bot commented Jun 23, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 62.50000% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 62.02%. Comparing base (4933980) to head (afc0684).

Files with missing lines Patch % Lines
pkg/operations/reconfigure.go 62.50% 12 Missing and 6 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10438      +/-   ##
==========================================
- Coverage   62.04%   62.02%   -0.02%     
==========================================
  Files         533      533              
  Lines       63625    63673      +48     
==========================================
+ Hits        39473    39493      +20     
- Misses      20550    20579      +29     
+ Partials     3602     3601       -1     
Flag Coverage Δ
unittests 62.02% <62.50%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

if len(reconfigure.Parameters) == 0 {
return intctrlutil.NewErrorf(intctrlutil.ErrorTypeFatal, "invalid reconfigure request for component %s: no parameters", reconfigure.ComponentName)
}
if err := r.validateReconfigureParameters(reqCtx, cli, resource.Cluster, reconfigure); err != nil {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This two-pass prevalidation needs stronger justification. Parameter schema validation belongs to the ComponentParameter/parameters controller path; Ops should trigger the desired update, not duplicate the schema validation responsibility. If the partial-write concern is about multiple entries targeting the same ComponentParameter, the safer fix is to coalesce those changes and patch that CP once. If the entries target different components, there has not been cross-component transaction semantics, so making one component invalid parameter block unrelated components adds a new all-or-nothing behavior that the API does not currently promise. Please keep schema validation in the parameters controller path, or explain the strong API-level reason for introducing this cross-component preflight semantics.

@weicao

weicao commented Jun 23, 2026

Copy link
Copy Markdown
Contributor Author

Closing per reviewer direction: parameter schema validation belongs to the ComponentParameter/parameters controller path, not the Ops reconfigure action. The Ops action should only trigger desired updates and let the parameters controller handle validation. This applies to both this PR and the original #10415.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/L Denotes a PR that changes 100-499 lines.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants