Detect unknown keys in GitOps (phase 1) - #40963
Conversation
sgress454
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #40963 +/- ##
==========================================
+ Coverage 66.31% 66.35% +0.04%
==========================================
Files 2473 2475 +2
Lines 198069 198359 +290
Branches 8738 8888 +150
==========================================
+ Hits 131347 131627 +280
- Misses 54843 54844 +1
- Partials 11879 11888 +9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| // collectFields recursively extracts JSON field names from a struct type, | ||
| // handling embedded structs by inlining their fields. | ||
| func collectFields(t reflect.Type, keys map[string]fieldInfo) { |
There was a problem hiding this comment.
Note that this handles a single type (doesn't recurse into non-inline struct fields). The way nested structs are validated is:
validateUnkownKeysis called on the top-level value- That calls
validateMapKeyswhich callsknownJSONKeyson the top-level type, which callscollectFields(if not cached) validateMapKeysrecursively calls itself on child structs, so the fields of each of those child types are collected.
| // levenshtein computes the edit distance between two strings. | ||
| func levenshtein(a, b string) int { |
There was a problem hiding this comment.
Could move this into pkg/str if we ever use it elsewhere
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughAdds detection of unknown/extraneous keys in GitOps YAML parsing. Introduces 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/fleetctl/fleetctl/gitops.go`:
- Around line 228-232: The pre-parse call to extractControlsForNoTeam is
invoking spec.GitOpsFromFile without passing options, so the
--allow-unknown-keys flag (flAllowUnknownKeys) isn't honored; update the code to
construct and pass the same gitOpsOpts (or a shared spec.GitOpsOptions with
AllowUnknownKeys set from flAllowUnknownKeys) into the extractControlsForNoTeam
path and any other pre-parse calls that call spec.GitOpsFromFile so that
GitOpsFromFile receives the options consistently (refer to gitOpsOpts,
flAllowUnknownKeys, extractControlsForNoTeam, and spec.GitOpsFromFile).
In `@pkg/spec/gitops.go`:
- Around line 1571-1574: The unknown-key validation currently only runs against
the top-level softwareRaw; update the loading logic that unmarshals files
referenced by software.packages[].path to also call validateRawKeys on each
package's raw YAML before unmarshaling (use reflect.TypeFor[Package]() and
include the package file path in the filePath context), or alternatively iterate
softwareRaw.packages and validate each package raw map with validateRawKeys so
extraneous keys in package files are caught; ensure you append any returned
errors to multiError just like the top-level validation.
- Around line 931-939: processControlsPathIfNeeded currently reassigns the local
pointer variable controlsFilePath instead of updating the caller's string value,
so downstream code still uses the original path; change the assignment to update
the pointed-to value (e.g. ensure controlsFilePath is non-nil and set
*controlsFilePath = resolveApplyRelativePath(filepath.Dir(*controlsFilePath),
*controlsTop.Path) or assign the result to a local newPath and then set
*controlsFilePath = newPath) so subsequent reads
(os.ReadFile(*controlsFilePath)) and parseControls use the resolved external
controls file path; keep using the existing symbols processControlsPathIfNeeded,
controlsTop.Path, resolveApplyRelativePath and filepath.Dir.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 275a2711-c388-41e9-85bd-7ef2d3959c38
📒 Files selected for processing (6)
changes/40496-detect-unknown-fieldscmd/fleetctl/fleetctl/gitops.gopkg/spec/gitops.gopkg/spec/gitops_test.gopkg/spec/gitops_validate.gopkg/spec/gitops_validate_test.go
iansltx
left a comment
iansltx
left a comment
There was a problem hiding this comment.
Still need to review gitops_test, gitops_validate, and gitops_validate_test, but here's a review queue flush to get things started.
iansltx
left a comment
iansltx
left a comment
There was a problem hiding this comment.
Since the remaining files are sizable, planning on flushing the review queue after each file if I have any feedback. 3 files to go.
| } | ||
| errPrefix := fmt.Sprintf("failed to parse policy install_software %q: ", policy.Name) | ||
| wrapErr := func(err error) error { | ||
| return fmt.Errorf("%s: %w", errPrefix, err) |
There was a problem hiding this comment.
Looks like we have two :s here.
iansltx
left a comment
iansltx
left a comment
There was a problem hiding this comment.
gitops_test.go reviewed. Two files to go.
iansltx
left a comment
iansltx
left a comment
There was a problem hiding this comment.
Feedback from gitops_validate.go. Re-reviewing latest changes on gitops.go and gitops_test.go next.
|
|
||
| var errs []error | ||
| for i, elem := range data { | ||
| elemPath := append(append([]string(nil), path...), fmt.Sprintf("[%d]", i)) |
There was a problem hiding this comment.
Same slices.Clone advice as above.
| func validateYAMLKeys(yamlBytes []byte, targetType reflect.Type, filePath string, keysPath []string) []error { | ||
| var data any | ||
| if err := YamlUnmarshal(yamlBytes, &data); err != nil { | ||
| return nil // parse errors already caught by the struct unmarshal |
There was a problem hiding this comment.
Not following what's going on here? Is there a test to explain why this works?
There was a problem hiding this comment.
The idea was that anything that calls this has already unmarshalled into a struct, and if there were parser errors there we'd have already caught them, so catching them here would mean a double log. In practice we always bail on parser errors rather than continuing, so it's unlikely we'd reach this spot. See
Lines 1184 to 1190 in 19f9009
Putting it that way, I guess it's better to return the error here and get someone bugging us about the double log than it is to risk someone calling this before parsing to a struct and thinking that bad YML worked? I can fix.
There was a problem hiding this comment.
Do we have any automated test paths that would catch the double log?
Thinking that we can add the extra error'ing here and see how this feels during QA. Either we've covered all cases upstream (which means no double-logs?) or the double-logging is some level of annoying and we back it back out. Either way it'll be a quick tweak during QA.
(and with the above I'm implying we need to call something out in the test plan for this so we try to hit this particular edge case)
There was a problem hiding this comment.
Same page, I updated to return errors (and updated the tests that checked it didn't). I check all upstream cases and they all bail out early, so we should never see the error, and if we do then it's an indication of something we should probably fix upstream.
Co-authored-by: Ian Littman <iansltx@gmail.com>
iansltx
left a comment
iansltx
left a comment
There was a problem hiding this comment.
Good to go; apologies for the delay. Did some additional fiddling with tests locally to make sure things behaved the way I expected, and they did.
2 participants
Related issue: Resolves #40496
Details
This is the first phase of an effort to detect unknown keys in GitOps .yml files. In the regular
fleetctl gitopscase, it will fail when unknown keys are detected. This behavior can be changed with a new--allow-unknown-keysflag which will log the issues and continue.In this first phase we are detecting unknown keys in most GitOps sections, other than the top-level
org_settings:andsettings:sections which have more complicated typing. I will tackle those separately as they require a bit more thought. Also ultimately I'd like us to be doing this validation in a more top-down fashion in one place, rather than spreading it across the code by doing it in each individual section, but this is a good first step.As a bonus, I invited my pal Mr. Levenshtein to the party so that we can make suggestions when unknown keys are detected, like:
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing
--allow-unknown-keys; GitOps outputted helpful messages but continued.Summary by CodeRabbit
New Features
Tests