feat(armotypes): add ProfileDataRequired contract for profile-data projection#642
feat(armotypes): add ProfileDataRequired contract for profile-data projection#642
Conversation
Signed-off-by: Ben <ben@armosec.io>
Signed-off-by: Ben <ben@armosec.io>
Signed-off-by: Ben <ben@armosec.io>
…sEmpty Signed-off-by: Ben <ben@armosec.io>
Adds the profileDataRequired field (omitempty) alongside profileDependency in RuntimeRule, and two integration-level tests that verify YAML round-trip and omission when nil. Signed-off-by: Ben <ben@armosec.io>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe changes add a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes The implementation introduces a tagged-union serialization pattern across multiple formats with multi-layer validation logic, requiring careful verification of serialization correctness and edge case handling. The test suite comprehensiveness ensures validation coverage but demands cross-format verification. Poem
🚥 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)
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: 2
🧹 Nitpick comments (1)
armotypes/runtimerule.go (1)
134-139: Guard marshal paths against invalid union state to avoid silent data loss.If
All == trueandPatternsis non-empty, marshal currently emits"all"and drops patterns. If both are unset, it emits an empty list. Consider enforcing invariants directly in marshal methods.Suggested refactor
+func (f ProfileDataField) validateState() error { + if f.All && len(f.Patterns) > 0 { + return fmt.Errorf("profileDataField: cannot have both 'all' and pattern list") + } + if !f.All && len(f.Patterns) == 0 { + return fmt.Errorf("profileDataField: must declare 'all' or at least one pattern") + } + return nil +} + func (f ProfileDataField) MarshalYAML() (any, error) { + if err := f.validateState(); err != nil { + return nil, err + } if f.All { return profileDataFieldAllSentinel, nil } return f.Patterns, nil } @@ func (f ProfileDataField) MarshalJSON() ([]byte, error) { + if err := f.validateState(); err != nil { + return nil, err + } if f.All { return json.Marshal(profileDataFieldAllSentinel) } return json.Marshal(f.Patterns) } @@ func (f ProfileDataField) MarshalBSONValue() (bsontype.Type, []byte, error) { + if err := f.validateState(); err != nil { + return 0, nil, err + } if f.All { return bson.MarshalValue(profileDataFieldAllSentinel) } return bson.MarshalValue(f.Patterns) }Also applies to: 167-172, 193-198
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@armotypes/runtimerule.go` around lines 134 - 139, ProfileDataField.MarshalYAML currently drops patterns when All is true or silently emits an empty list when both fields are unset; change it to validate the union invariants: in ProfileDataField.MarshalYAML, if f.All && len(f.Patterns) > 0 return a non-nil error describing the conflicting state instead of returning profileDataFieldAllSentinel, and if !f.All && len(f.Patterns) == 0 return a non-nil error indicating the unset/empty union rather than silently emitting an empty value; apply the same validation pattern to the other marshal methods that use the same All vs Patterns union semantics so conflicts and empty-unset states fail fast.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@armotypes/runtimerule_profile_data_test.go`:
- Around line 92-103: Add a negative JSON test that ensures unmarshaling "null"
into a ProfileDataField is rejected: create a test (e.g., add a case in
TestProfileDataField_JSONRoundTrip or a new TestProfileDataField_JSONNullReject)
that does json.Unmarshal([]byte("null"), &got) where got is a ProfileDataField
and assert that an error is returned (use require.Error/require.NotNil on the
error) to lock the schema contract for ProfileDataField.
In `@armotypes/runtimerule.go`:
- Around line 174-190: ProfileDataField.UnmarshalJSON currently allows JSON null
because json.Unmarshal into []ProfileDataPattern returns nil instead of error;
modify UnmarshalJSON (ProfileDataField.UnmarshalJSON) to explicitly reject JSON
null before decoding the slice (e.g., check if data equals "null" or unmarshal
into a raw json.RawMessage and detect null) and return an error instead of
treating nil patterns as a valid list, preserving existing handling for the
"all" sentinel and non-null slices by setting f.All=false and f.Patterns
accordingly.
---
Nitpick comments:
In `@armotypes/runtimerule.go`:
- Around line 134-139: ProfileDataField.MarshalYAML currently drops patterns
when All is true or silently emits an empty list when both fields are unset;
change it to validate the union invariants: in ProfileDataField.MarshalYAML, if
f.All && len(f.Patterns) > 0 return a non-nil error describing the conflicting
state instead of returning profileDataFieldAllSentinel, and if !f.All &&
len(f.Patterns) == 0 return a non-nil error indicating the unset/empty union
rather than silently emitting an empty value; apply the same validation pattern
to the other marshal methods that use the same All vs Patterns union semantics
so conflicts and empty-unset states fail fast.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6c089bbd-0e06-43b0-8d9e-daeded393f54
📒 Files selected for processing (2)
armotypes/runtimerule.goarmotypes/runtimerule_profile_data_test.go
| func TestProfileDataField_JSONRoundTrip(t *testing.T) { | ||
| for _, f := range []ProfileDataField{ | ||
| {All: true}, | ||
| {Patterns: []ProfileDataPattern{{Prefix: "/x"}, {Suffix: "/y"}}}, | ||
| } { | ||
| b, err := json.Marshal(f) | ||
| require.NoError(t, err) | ||
| var got ProfileDataField | ||
| require.NoError(t, json.Unmarshal(b, &got)) | ||
| assert.Equal(t, f, got) | ||
| } | ||
| } |
There was a problem hiding this comment.
Add a JSON negative test for null input on ProfileDataField.
Current JSON coverage checks valid forms only. Please add a rejection test for null to lock the schema contract.
Suggested test addition
+func TestProfileDataField_UnmarshalJSON_RejectsNull(t *testing.T) {
+ var f ProfileDataField
+ err := json.Unmarshal([]byte(`null`), &f)
+ assert.Error(t, err)
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestProfileDataField_JSONRoundTrip(t *testing.T) { | |
| for _, f := range []ProfileDataField{ | |
| {All: true}, | |
| {Patterns: []ProfileDataPattern{{Prefix: "/x"}, {Suffix: "/y"}}}, | |
| } { | |
| b, err := json.Marshal(f) | |
| require.NoError(t, err) | |
| var got ProfileDataField | |
| require.NoError(t, json.Unmarshal(b, &got)) | |
| assert.Equal(t, f, got) | |
| } | |
| } | |
| func TestProfileDataField_JSONRoundTrip(t *testing.T) { | |
| for _, f := range []ProfileDataField{ | |
| {All: true}, | |
| {Patterns: []ProfileDataPattern{{Prefix: "/x"}, {Suffix: "/y"}}}, | |
| } { | |
| b, err := json.Marshal(f) | |
| require.NoError(t, err) | |
| var got ProfileDataField | |
| require.NoError(t, json.Unmarshal(b, &got)) | |
| assert.Equal(t, f, got) | |
| } | |
| } | |
| func TestProfileDataField_UnmarshalJSON_RejectsNull(t *testing.T) { | |
| var f ProfileDataField | |
| err := json.Unmarshal([]byte(`null`), &f) | |
| assert.Error(t, err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@armotypes/runtimerule_profile_data_test.go` around lines 92 - 103, Add a
negative JSON test that ensures unmarshaling "null" into a ProfileDataField is
rejected: create a test (e.g., add a case in TestProfileDataField_JSONRoundTrip
or a new TestProfileDataField_JSONNullReject) that does
json.Unmarshal([]byte("null"), &got) where got is a ProfileDataField and assert
that an error is returned (use require.Error/require.NotNil on the error) to
lock the schema contract for ProfileDataField.
| func (f *ProfileDataField) UnmarshalJSON(data []byte) error { | ||
| var s string | ||
| if err := json.Unmarshal(data, &s); err == nil { | ||
| if s != profileDataFieldAllSentinel { | ||
| return fmt.Errorf("profileDataField: scalar must be %q, got %q", profileDataFieldAllSentinel, s) | ||
| } | ||
| f.All = true | ||
| f.Patterns = nil | ||
| return nil | ||
| } | ||
| var patterns []ProfileDataPattern | ||
| if err := json.Unmarshal(data, &patterns); err != nil { | ||
| return fmt.Errorf("profileDataField: must be string %q or list: %w", profileDataFieldAllSentinel, err) | ||
| } | ||
| f.All = false | ||
| f.Patterns = patterns | ||
| return nil |
There was a problem hiding this comment.
Reject JSON null in ProfileDataField.UnmarshalJSON.
On Line 185, json.Unmarshal into []ProfileDataPattern accepts null, so profileDataField can decode from null even though the contract allows only "all" or a list.
Suggested fix
import (
+ "bytes"
"encoding/json"
"fmt"
@@
func (f *ProfileDataField) UnmarshalJSON(data []byte) error {
+ if bytes.Equal(bytes.TrimSpace(data), []byte("null")) {
+ return fmt.Errorf("profileDataField: must be string %q or list, got null", profileDataFieldAllSentinel)
+ }
+
var s string
if err := json.Unmarshal(data, &s); err == nil {
if s != profileDataFieldAllSentinel {
return fmt.Errorf("profileDataField: scalar must be %q, got %q", profileDataFieldAllSentinel, s)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (f *ProfileDataField) UnmarshalJSON(data []byte) error { | |
| var s string | |
| if err := json.Unmarshal(data, &s); err == nil { | |
| if s != profileDataFieldAllSentinel { | |
| return fmt.Errorf("profileDataField: scalar must be %q, got %q", profileDataFieldAllSentinel, s) | |
| } | |
| f.All = true | |
| f.Patterns = nil | |
| return nil | |
| } | |
| var patterns []ProfileDataPattern | |
| if err := json.Unmarshal(data, &patterns); err != nil { | |
| return fmt.Errorf("profileDataField: must be string %q or list: %w", profileDataFieldAllSentinel, err) | |
| } | |
| f.All = false | |
| f.Patterns = patterns | |
| return nil | |
| import ( | |
| "bytes" | |
| "encoding/json" | |
| "fmt" | |
| ) | |
| func (f *ProfileDataField) UnmarshalJSON(data []byte) error { | |
| if bytes.Equal(bytes.TrimSpace(data), []byte("null")) { | |
| return fmt.Errorf("profileDataField: must be string %q or list, got null", profileDataFieldAllSentinel) | |
| } | |
| var s string | |
| if err := json.Unmarshal(data, &s); err == nil { | |
| if s != profileDataFieldAllSentinel { | |
| return fmt.Errorf("profileDataField: scalar must be %q, got %q", profileDataFieldAllSentinel, s) | |
| } | |
| f.All = true | |
| f.Patterns = nil | |
| return nil | |
| } | |
| var patterns []ProfileDataPattern | |
| if err := json.Unmarshal(data, &patterns); err != nil { | |
| return fmt.Errorf("profileDataField: must be string %q or list: %w", profileDataFieldAllSentinel, err) | |
| } | |
| f.All = false | |
| f.Patterns = patterns | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@armotypes/runtimerule.go` around lines 174 - 190,
ProfileDataField.UnmarshalJSON currently allows JSON null because json.Unmarshal
into []ProfileDataPattern returns nil instead of error; modify UnmarshalJSON
(ProfileDataField.UnmarshalJSON) to explicitly reject JSON null before decoding
the slice (e.g., check if data equals "null" or unmarshal into a raw
json.RawMessage and detect null) and return an error instead of treating nil
patterns as a valid list, preserving existing handling for the "all" sentinel
and non-null slices by setting f.All=false and f.Patterns accordingly.
There was a problem hiding this comment.
Pull request overview
Adds a new schema contract in armotypes to describe which subsets of profile data a runtime rule requires (for rule-aware profile-data projection), including custom YAML/JSON/BSON encoding and validation, plus comprehensive round-trip tests.
Changes:
- Introduces
ProfileDataPattern,ProfileDataField(tagged-union), andProfileDataRequiredwith validation helpers. - Adds
ProfileDataRequired *ProfileDataRequired(omitempty) toRuntimeRule. - Adds YAML/JSON/BSON codec + round-trip/validation tests for the new contract.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| armotypes/runtimerule.go | Adds ProfileDataRequired to RuntimeRule and implements the new profile-data requirement types with custom codecs + validation. |
| armotypes/runtimerule_profile_data_test.go | Adds round-trip tests for YAML/JSON/BSON and validation/emptiness behavior for the new types. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for name, field := range map[string]*ProfileDataField{ | ||
| "opens": p.Opens, | ||
| "execs": p.Execs, | ||
| "capabilities": p.Capabilities, | ||
| "syscalls": p.Syscalls, | ||
| "endpoints": p.Endpoints, | ||
| "egressDomains": p.EgressDomains, | ||
| "egressAddresses": p.EgressAddresses, | ||
| "ingressDomains": p.IngressDomains, | ||
| "ingressAddresses": p.IngressAddresses, | ||
| } { | ||
| if field == nil { | ||
| continue | ||
| } | ||
| if err := validateProfileDataField(name, field); err != nil { |
| var patterns []ProfileDataPattern | ||
| if err := json.Unmarshal(data, &patterns); err != nil { | ||
| return fmt.Errorf("profileDataField: must be string %q or list: %w", profileDataFieldAllSentinel, err) | ||
| } |
| func (f ProfileDataField) MarshalJSON() ([]byte, error) { | ||
| if f.All { | ||
| return json.Marshal(profileDataFieldAllSentinel) | ||
| } | ||
| return json.Marshal(f.Patterns) | ||
| } |
| assert.Equal(t, f, got) | ||
| } | ||
| } | ||
|
|
Coverage Report for CI Build 25055508324Warning No base build found for commit Coverage: 43.169%Details
Uncovered Changes
Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
JSON null was silently accepted via the []ProfileDataPattern path, producing an invalid union state (All=false, Patterns=nil). Now returns an error, and a test locks the contract. Signed-off-by: Ben <ben@armosec.io>
Summary
ProfileDataPattern,ProfileDataField(tagged-union), andProfileDataRequiredtypes toarmotypes/runtimerule.go— the schema contract for NAUT-1295 rule-aware profile projection.ProfileDataRequired *ProfileDataRequired(omitempty) intoRuntimeRulealongsideProfileDependency.ProfileDataFieldsupports custom YAML/JSON/BSON codecs: marshals as the string"all"or as a list of pattern objects.ProfileDataRequired.Validate()andIsEmpty()are the single source of truth used by the rulelibrary lint (Phase 2) and node-agent load-time check.Test plan
TestProfileDataPattern_YAMLRoundTrip— all four match typesTestProfileDataPattern_JSONRoundTripTestProfileDataField_UnmarshalYAML_All/Patterns/RejectsUnknownTestProfileDataField_MarshalYAML_All/PatternsTestProfileDataField_JSONRoundTripTestProfileDataField_BSONRoundTripTestProfileDataRequired_Validate_Valid/FieldEmpty/PatternMultiKey/PatternEmpty/AllAndPatternsTestProfileDataRequired_IsEmpty(including nil-receiver)TestRuntimeRule_ProfileDataRequired_YAML— round-trip through RuntimeRuleTestRuntimeRule_ProfileDataRequired_OmittedWhenNilRelated: NAUT-1300 / NAUT-1295
Summary by CodeRabbit