eventchecker: support generic map types#4763
Conversation
a46d958 to
40f6d5e
Compare
|
This is note for myself: I was able to generate the last commit in this series by executing: There is a comment in the Makefile for the protogen target: The code originally did this when this comment was introduced, and has since changed to not do this. |
| checkerVar := fmt.Sprintf("%s.%s", checkerVarName, field.GoName) | ||
| eventVar := fmt.Sprintf("%s.%s", eventVarName, field.GoName) | ||
| checkerVar := checkerVarName + "." + field.GoName | ||
| eventVar := eventVarName + "." + field.GoName |
There was a problem hiding this comment.
There are a lot of changes in this commit like this, where we change Sprintf() calls to string concatenation ("+"). It doesn't seem like the motivation to do this is related to what is described in the commit message. Should these changes be moved out into a separate commit?
There was a problem hiding this comment.
Yes moved this to a separate commit,thanks.
| // For Pod_Labels specifically | ||
| if field.GoIdent.GoName == "Pod_Labels" { | ||
| smatcher := common.StringMatcherIdent(g, "Full") | ||
| return `{ | ||
| checkerVar := make(` + mident + `) | ||
| splitN := strings.SplitN | ||
| for _, s := range event.PodLabels { | ||
| kv := splitN(s, "=", 2) | ||
| if len(kv) == 2 { | ||
| checkerVar[kv[0]] = *` + smatcher + `(kv[1]) | ||
| } | ||
| } | ||
| checker.PodLabels = checkerVar | ||
| }`, nil | ||
| } |
There was a problem hiding this comment.
I don't see this in the generated output. Why is this code not currently used?
There was a problem hiding this comment.
Great catch! It's dead code left over from earlier iterations,the struct field is actually named PodLabels (so "Pod_Labels" never matches) and new generic map logic handles the native map perfectly, so I'll just remove it.
| @@ -0,0 +1,109 @@ | |||
| // SPDX-License-Identifier: Apache-2.0 | |||
There was a problem hiding this comment.
It would probably be better if this was in a different commit; one commit for your changes and another for the result of "make vendor".
I do it this way because I find it easier to rebase / cherry-pick. This way, I don't have to handle conflicts for the generated files. Instead, I just run "make vendor" to regenerate the commit that corresponds to the original "make vendor" commit.
| // Matcher is a generic interface for matching values of type T. | ||
| type Matcher[T any] interface { | ||
| Match(T) error | ||
| } |
There was a problem hiding this comment.
I see that this type is never used. I think you created this for clarity and/or documentation purposes, which makes sense to me. I just want to confirm that this is intentional, and not something left over from an earlier approach.
There was a problem hiding this comment.
You're spot on! Matcher[T] is indeed currently unused as a formal type constraint,it's mostly there for docs to show the expected shape of the matchers that go into MapMatcher.
The main reason I used any for M in MapMatcher[K, V, M] instead of the Matcher interface is to give us max flexibility. Specifically, it lets this work seamlessly with existing matchers (like StringMatcher) that have pointer receivers, without us having to tweak their source files at all. Since the Match method dynamically checks both the value and its pointer during execution, having the formal interface constraint wasn't strictly required to get the job done.
| return nil | ||
| } | ||
|
|
||
| func TestMapMatcher(t *testing.T) { |
There was a problem hiding this comment.
Should this test also test UnmarshalJSON ( viayaml.Unmarshal), like how is done in TestPrimitiveMatcher?
There was a problem hiding this comment.
I didn't add a specific unmarshal test for MapMatcher because its UnmarshalJSON method is just standard boilerplate mapping (using the common Alias type trick to prevent infinite recursion).
It has no custom branching or fallback logic to verify. PrimitiveMatcher, on the other hand, absolutely needed a specific test because it has handwritten, custom fallback logic to handle two completely different YAML formats (parsing both shorthand bare values and full structs).
|
I see the benefit of making more generic code that can be re-used. I also like how the From method is implemented for maps, in case we want to use that in the future. That being said, I'm wondering if there any other motivation that I'm not aware of. Do you plan to use this for another change in the future? If so, please add that information to the PR, so that reviewers can better understand where you are headed. |
There was a problem hiding this comment.
I left some comments, but overall LGTM.
That being said, all this code and concepts were new to me as I had never looked at the protoc-gen-go-tetragon part of the codebase before reviewing this. So, it would be great if we could get another review from someone with more background/expertise in this this area before merging.
40f6d5e to
3dc6558
Compare
✅ Deploy Preview for tetragon ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
This commit adds a new MapMatcher that works with different generic map types. This removes the hardcoded limitation of only checking string-to-string maps. A PrimitiveMatcher is also included to compare simple values like numbers or booleans inside maps. Signed-off-by: Aritra Dey <adey01027@gmail.com>
3dc6558 to
4c7857b
Compare
Update the api/vendor directory to include the newly added generic MapMatcher and PrimitiveMatcher. This is required for the eventchecker code generation to correctly reference these matchers in the API. Signed-off-by: Aritra Dey <adey01027@gmail.com>
4c7857b to
4c70e12
Compare
84bbee1 to
83e3350
Compare
Replace fmt.Sprintf with standard string concatenation when generating field and event names. This addresses perfsprint linter warnings about simple string formatting. Signed-off-by: Aritra Dey <adey01027@gmail.com>
This commit updates the codegen tool to handle map types beyond strings. The tool now automatically creates matchers for maps with different key and value types. It also adds the required logic to extract these map values from events. Signed-off-by: Aritra Dey <adey01027@gmail.com>
Regenerate the eventchecker code using the updated codegen tool. This includes the new generic map matchers in the API. Signed-off-by: Aritra Dey <adey01027@gmail.com>
83e3350 to
c1953c3
Compare
Hey, the main fix was |
will-isovalent
left a comment
There was a problem hiding this comment.
Hey, thanks so much for the contribution. The changes make sense to me.
If it's not too much effort, I think we probably also need to think about how you could express integer ranges and inequalities here in addition to raw equality checks. This could also be done as a follow-up though.
@will-isovalent do you think this should be in the context of this PR, or can it be a follow-up? |
Description
This PR extends the
eventcheckerto support generic map fields. Previously,PR #260 hardcoded support only formap[string]string.This PR introduces a generic
MapMatcherto automatically handle different key-value types (like int and bool).Note:
MapMatcheris made generic so future proto map fields with non-string value types (e.g.map<string, uint32>) are automatically supported by codegen without additional matcher changes.Changelog