Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions pkg/recipe/metadata.go
Original file line number Diff line number Diff line change
Expand Up @@ -456,22 +456,27 @@ func (s *RecipeMetadataSpec) Merge(other *RecipeMetadataSpec) {
return s.ComponentRefs[i].Name < s.ComponentRefs[j].Name
})

// Merge validation config - overlay phases take precedence
// Merge validation config - overlay phases take precedence. Phase
// pointers are cloned (not aliased) so successive merges cannot mutate
// the source's cached ValidationConfig — a previous version aliased
// other.Validation when s.Validation was nil, then later writes through
// s.Validation.Deployment etc. corrupted whichever overlay's cached
// metadata the alias pointed at.
if other.Validation != nil {
if s.Validation == nil {
s.Validation = other.Validation
s.Validation = cloneValidationConfig(other.Validation)
} else {
if other.Validation.Readiness != nil {
s.Validation.Readiness = other.Validation.Readiness
s.Validation.Readiness = cloneValidationPhase(other.Validation.Readiness)
}
if other.Validation.Deployment != nil {
s.Validation.Deployment = other.Validation.Deployment
s.Validation.Deployment = cloneValidationPhase(other.Validation.Deployment)
}
if other.Validation.Performance != nil {
s.Validation.Performance = other.Validation.Performance
s.Validation.Performance = cloneValidationPhase(other.Validation.Performance)
}
if other.Validation.Conformance != nil {
s.Validation.Conformance = other.Validation.Conformance
s.Validation.Conformance = cloneValidationPhase(other.Validation.Conformance)
}
}
}
Expand Down
4 changes: 3 additions & 1 deletion pkg/recipe/metadata_store.go
Original file line number Diff line number Diff line change
Expand Up @@ -636,11 +636,13 @@ func (s *MetadataStore) buildMixinConstraintCandidateIndex(candidateOverlays []s
}

// initBaseMergedSpec creates a copy of the base spec for overlay merging.
// Validation is deep-cloned so downstream Merge calls cannot reach back
// into the cached base ValidationConfig and mutate it.
func (s *MetadataStore) initBaseMergedSpec() (RecipeMetadataSpec, []string) {
mergedSpec := RecipeMetadataSpec{
Constraints: make([]Constraint, len(s.Base.Spec.Constraints)),
ComponentRefs: make([]ComponentRef, len(s.Base.Spec.ComponentRefs)),
Validation: s.Base.Spec.Validation,
Validation: cloneValidationConfig(s.Base.Spec.Validation),
}
copy(mergedSpec.Constraints, s.Base.Spec.Constraints)
copy(mergedSpec.ComponentRefs, s.Base.Spec.ComponentRefs)
Expand Down
33 changes: 33 additions & 0 deletions pkg/recipe/metadata_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1027,6 +1027,39 @@ func TestMergeValidationConfig(t *testing.T) {
t.Fatal("validation should be preserved from base when overlay has nil")
}
})

// Regression: a prior version of Merge aliased other.Validation when the
// destination's was nil. Subsequent merges then wrote through that alias
// into whichever overlay's cached ValidationConfig the alias pointed at,
// polluting it across calls. The fix deep-copies via cloneValidationConfig.
t.Run("merge does not alias source validation", func(t *testing.T) {
source := &ValidationConfig{
Conformance: &ValidationPhase{Checks: []string{"check-from-source"}},
}
first := RecipeMetadataSpec{
Validation: source,
}
second := RecipeMetadataSpec{
Validation: &ValidationConfig{
Deployment: &ValidationPhase{Checks: []string{"deployment-from-second"}},
},
}

// dest starts with nil Validation, so without the fix it would alias
// source. Merging second then plants Deployment via the alias into
// the source — corrupting subsequent reads of the source.
var dest RecipeMetadataSpec
dest.Merge(&first)
dest.Merge(&second)

if source.Deployment != nil {
t.Errorf("source.Deployment leaked to %v — Merge aliased the source ValidationConfig",
source.Deployment.Checks)
}
if dest.Validation.Conformance == source.Conformance {
t.Error("dest.Validation.Conformance aliases source.Conformance — phase pointers must be cloned")
}
})
}

func TestFinalizeRecipeResultIncludesValidation(t *testing.T) {
Expand Down
49 changes: 49 additions & 0 deletions pkg/recipe/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@

package recipe

import "maps"

// ValidationConfig defines validation phases and settings.
type ValidationConfig struct {
// Readiness defines readiness validation phase settings.
Expand Down Expand Up @@ -59,3 +61,50 @@ type NodeSelection struct {
// ExcludeNodes lists node names to exclude from validation.
ExcludeNodes []string `json:"excludeNodes,omitempty" yaml:"excludeNodes,omitempty"`
}

// cloneValidationConfig returns a deep copy of v. RecipeMetadataSpec.Merge
// uses this to avoid aliasing the source's nested phase pointers — without
// it, successive merges mutate whichever cached overlay's ValidationConfig
// the destination aliased.
func cloneValidationConfig(v *ValidationConfig) *ValidationConfig {
if v == nil {
return nil
}
return &ValidationConfig{
Readiness: cloneValidationPhase(v.Readiness),
Deployment: cloneValidationPhase(v.Deployment),
Performance: cloneValidationPhase(v.Performance),
Conformance: cloneValidationPhase(v.Conformance),
}
}

// cloneValidationPhase returns a deep copy of p with independent backing
// slices and a freshly allocated NodeSelection, so callers writing through
// the clone cannot reach the source's cached metadata.
func cloneValidationPhase(p *ValidationPhase) *ValidationPhase {
if p == nil {
return nil
}
out := *p
if p.Constraints != nil {
out.Constraints = make([]Constraint, len(p.Constraints))
copy(out.Constraints, p.Constraints)
}
if p.Checks != nil {
out.Checks = make([]string, len(p.Checks))
copy(out.Checks, p.Checks)
}
if p.NodeSelection != nil {
ns := *p.NodeSelection
if p.NodeSelection.Selector != nil {
ns.Selector = make(map[string]string, len(p.NodeSelection.Selector))
maps.Copy(ns.Selector, p.NodeSelection.Selector)
}
if p.NodeSelection.ExcludeNodes != nil {
ns.ExcludeNodes = make([]string, len(p.NodeSelection.ExcludeNodes))
copy(ns.ExcludeNodes, p.NodeSelection.ExcludeNodes)
}
out.NodeSelection = &ns
}
return &out
}
Loading
Loading