🐛 normalize Helm values before schema validation - #389
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
Walkthrough
ChangesHelm values normalization
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
629fddc to
b3f875e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/addonfactory/helm_agentaddon.go (1)
184-214: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winNormalize user values before merging to prevent deep-merge data loss.
Normalizing
overrideValuesat the end meansMergeValuesis executed with un-normalizeduserValues(which may contain concrete Go maps likemap[string]string). Standard deep-merge implementations often fail type assertions (e.g.,v.(map[string]interface{})) on concrete maps, resulting in silent overwrites of entire blocks instead of expected deep merges.Since
defaultValuesandbuiltinValuesare already normalized internally before merging, applyingJsonStructToValuestouserValuesbefore the merge ensures thatMergeValuesalways operates on compatiblemap[string]interface{}types. This protects the integrity of the merged values and allows the redundant final normalization to be safely removed.🐛 Proposed fix
if a.getValuesFuncs[i] != nil { userValues, err := a.getValuesFuncs[i](cluster, addon) if err != nil { return overrideValues, err } + normalizedUserValues, err := JsonStructToValues(userValues) + if err != nil { + return overrideValues, fmt.Errorf("failed to normalize Helm values: %w", err) + } + klog.V(4).Infof("index=%d, user values: %v", i, userValues) - overrideValues = MergeValues(overrideValues, userValues) + overrideValues = MergeValues(overrideValues, normalizedUserValues) klog.V(4).Infof("index=%d, override values: %v", i, overrideValues) } } builtinValues, err := a.getBuiltinValues(cluster, addon) if err != nil { klog.Errorf("failed to get builtinValue. err:%v", err) return nil, err } overrideValues = MergeValues(overrideValues, builtinValues) releaseOptions, err := a.releaseOptions(addon) if err != nil { return nil, err } cap := a.capabilities(cluster, addon) - normalizedValues, err := JsonStructToValues(overrideValues) - if err != nil { - return overrideValues, fmt.Errorf("failed to normalize Helm values: %w", err) - } - values, err := chartutil.ToRenderValues(a.chart, normalizedValues, + values, err := chartutil.ToRenderValues(a.chart, overrideValues, releaseOptions, cap) if err != nil { klog.Errorf("failed to render helm chart with values %v. err:%v", overrideValues, err) return values, err }🤖 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/addonfactory/helm_agentaddon.go` around lines 184 - 214, Normalize each user value returned by getValuesFuncs before passing it to MergeValues, ensuring concrete map types are converted to compatible map[string]interface{} structures. Handle normalization errors at that point and return them, then remove the redundant final JsonStructToValues call and use the already-normalized overrideValues for chartutil.ToRenderValues.
🤖 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/addonfactory/helm_agentaddon.go`:
- Around line 184-214: Normalize each user value returned by getValuesFuncs
before passing it to MergeValues, ensuring concrete map types are converted to
compatible map[string]interface{} structures. Handle normalization errors at
that point and return them, then remove the redundant final JsonStructToValues
call and use the already-normalized overrideValues for chartutil.ToRenderValues.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eae15ce5-a6c5-4c41-9adc-b0364f378d43
📒 Files selected for processing (4)
docs/helmAgentAddon.mdpkg/addonfactory/helm_agentaddon.gopkg/addonfactory/helm_agentaddon_test.gopkg/addonfactory/testmanifests/chart/values.schema.json
b3f875e to
893c955
Compare
|
@mikeshng Hi ✋ |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/addonfactory/helm_agentaddon_test.go (1)
307-309: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
assert.Equaloverreflect.DeepEqualfor clearer test failure diffs.Since
github.com/stretchr/testify/assertis now imported, you can useassert.Equalhere. When slices or structs don't match,assert.Equalprovides a much clearer line-by-line diff thanreflect.DeepEqualcombined with the%vformatter, making test failures easier to debug.♻️ Proposed refactor
- if c.expectedTolerations != nil && !reflect.DeepEqual(object.Spec.Template.Spec.Tolerations, c.expectedTolerations) { - t.Errorf("expected tolerations are %v, but got %v", c.expectedTolerations, object.Spec.Template.Spec.Tolerations) - } + if c.expectedTolerations != nil { + assert.Equal(t, c.expectedTolerations, object.Spec.Template.Spec.Tolerations, "expected tolerations to match") + }🤖 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/addonfactory/helm_agentaddon_test.go` around lines 307 - 309, Update the toleration comparison in the test case around expectedTolerations to use testify's assert.Equal instead of reflect.DeepEqual and the manual t.Errorf, preserving the existing conditional check and failure context.
🤖 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.
Nitpick comments:
In `@pkg/addonfactory/helm_agentaddon_test.go`:
- Around line 307-309: Update the toleration comparison in the test case around
expectedTolerations to use testify's assert.Equal instead of reflect.DeepEqual
and the manual t.Errorf, preserving the existing conditional check and failure
context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 648679ef-41a4-45ea-8c9f-0743a9bf87e6
📒 Files selected for processing (4)
docs/helmAgentAddon.mdpkg/addonfactory/helm_agentaddon.gopkg/addonfactory/helm_agentaddon_test.gopkg/addonfactory/testmanifests/chart/values.schema.json
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/helmAgentAddon.md
- pkg/addonfactory/helm_agentaddon.go
Signed-off-by: kahirokunn <okinakahiro@gmail.com>
893c955 to
a09bf57
Compare
|
Thank you @coderabbitai . Fixed it. |
|
Tip For best results, initiate chat on the files or code changes.
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
| @@ -0,0 +1,10 @@ | |||
| { | |||
| "$schema": "https://json-schema.org/draft-07/schema#", | |||
| "$comment": "exercises helm schema validation; typed Go values that are not normalized to JSON-compatible types fail this validation", | |||
There was a problem hiding this comment.
I welcome this enhancement.
My biggest concern will be any backward compatibility issues for existing addons? Any migration step require? I am worry that if we upgrade to this addon-framework version with this change and it might break existing addons unless they do something.
If it's possible to break existing addons, could we enhance the PR so that we get this feature and also not break existing users. That will be ideal.
/assign @qiujian16
There was a problem hiding this comment.
Thanks for raising this.
Existing add-ons that return JSON-compatible values will keep working without any migration. This includes annotation values and values already converted with JsonStructToValues. values.schema.json also remains optional; this change does not enable schema validation for existing charts.
I also checked addon-contrib. There is currently no tracked use of BuildHelmAgentAddon; its addonfactory consumers all use BuildTemplateAgentAddon, so they are not affected.
For schema-enabled Helm add-ons, this removes the need for local normalization. For example, cluster-proxy currently has its own merge-and-normalize workaround, although that change is still part of cluster-proxy PR #329.
Normalization is applied to Helm charts without a schema as well. In theory, values that cannot be marshaled to JSON will now fail, but those are outside the documented and recommended usage of Helm values. Concrete nested maps will also be deep-merged as intended by MergeValues.
Therefore, I do not expect this change to affect existing add-ons, and no migration should be required.
There was a problem hiding this comment.
/lgtm
Thank you for taking a look at my feedback.
I am not a maintainer for this repo so @qiujian16 will have to approve. Thanks.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kahirokunn, qiujian16 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 |
8c9c7b7
into
open-cluster-management-io:main
|
Thanks for your review 🙏 |
3 participants
Summary
Helm schema validation rejects concrete Go collections returned by
GetValuesFuncswithinvalid jsonType, forcing each add-on to merge and normalize values itself; normalize the merged values in addon-framework instead.Related issue(s)
N/A
Summary by CodeRabbit