diff --git a/go.mod b/go.mod index f34e3977..49cdc472 100644 --- a/go.mod +++ b/go.mod @@ -9,6 +9,7 @@ require ( github.com/onsi/gomega v1.38.0 golang.org/x/tools v0.37.0 k8s.io/apimachinery v0.32.3 + k8s.io/gengo/v2 v2.0.0-20250922181213-ec3ebc5fd46b k8s.io/utils v0.0.0-20241104100929-3ea5e8cea738 sigs.k8s.io/yaml v1.4.0 ) diff --git a/go.sum b/go.sum index ce0fbf4f..5d2231f7 100644 --- a/go.sum +++ b/go.sum @@ -993,6 +993,8 @@ honnef.co/go/tools v0.6.1 h1:R094WgE8K4JirYjBaOpz/AvTyUu/3wbmAoskKN/pxTI= honnef.co/go/tools v0.6.1/go.mod h1:3puzxxljPCe8RGJX7BIy1plGbxEOZni5mR2aXe3/uk4= k8s.io/apimachinery v0.32.3 h1:JmDuDarhDmA/Li7j3aPrwhpNBA94Nvk5zLeOge9HH1U= k8s.io/apimachinery v0.32.3/go.mod h1:GpHVgxoKlTxClKcteaeuF1Ul/lDVb74KpZcxcmLDElE= +k8s.io/gengo/v2 v2.0.0-20250922181213-ec3ebc5fd46b h1:gMplByicHV/TJBizHd9aVEsTYoJBnnUAT5MHlTkbjhQ= +k8s.io/gengo/v2 v2.0.0-20250922181213-ec3ebc5fd46b/go.mod h1:CgujABENc3KuTrcsdpGmrrASjtQsWCT7R99mEV4U/fM= k8s.io/utils v0.0.0-20241104100929-3ea5e8cea738 h1:M3sRQVHv7vB20Xc2ybTt7ODCeFj6JSWYFzOFnYeS6Ro= k8s.io/utils v0.0.0-20241104100929-3ea5e8cea738/go.mod h1:OLgZIPagt7ERELqWJFomSt595RzquPNLL48iOWgYOg0= mvdan.cc/gofumpt v0.9.1 h1:p5YT2NfFWsYyTieYgwcQ8aKV3xRvFH4uuN/zB2gBbMQ= diff --git a/pkg/analysis/forbiddenmarkers/analyzer.go b/pkg/analysis/forbiddenmarkers/analyzer.go index 1263b57b..80846a16 100644 --- a/pkg/analysis/forbiddenmarkers/analyzer.go +++ b/pkg/analysis/forbiddenmarkers/analyzer.go @@ -112,7 +112,7 @@ func markerMatchesAttributeRules(marker markers.Marker, attrRules ...MarkerAttri for _, attrRule := range attrRules { // if the marker doesn't contain the attribute for a specified rule it fails the AND // operation. - val, ok := marker.Expressions[attrRule.Name] + val, ok := marker.Arguments[attrRule.Name] if !ok { return false } diff --git a/pkg/analysis/helpers/markers/analyzer.go b/pkg/analysis/helpers/markers/analyzer.go index e93c36ae..ee70b8a3 100644 --- a/pkg/analysis/helpers/markers/analyzer.go +++ b/pkg/analysis/helpers/markers/analyzer.go @@ -26,19 +26,24 @@ import ( "golang.org/x/tools/go/analysis/passes/inspect" "golang.org/x/tools/go/ast/inspector" + "k8s.io/gengo/v2/codetags" + kalerrors "sigs.k8s.io/kube-api-linter/pkg/analysis/errors" ) -// UnnamedExpression is the expression key used +// UnnamedArgument is the argument key used // when parsing markers that don't have a specific -// named expression. +// named argument. +// +// This is specific to declarative validation markers only. +// Kubebuilder-style markers either have named arguments or a payload. // -// An example of a marker without a named expression -// is "kubebuilder:default:=foo". +// An example of a Declarative Validation marker with an unnamed argument +// is "k8s:ifEnabled(\"my-feature\")=...". // -// An example of a marker with named expressions -// is "kubebuilder:validation:XValidation:rule='...',message='...'". -const UnnamedExpression = "" +// An example of a Declarative Validation marker with named arguments +// is "k8s:item(one: "value", two: "value")=...". +const UnnamedArgument = "" // Markers allows access to markers extracted from the // go types. @@ -193,7 +198,8 @@ func extractFieldMarkers(field *ast.Field, results *markers) { fieldMarkers := NewMarkerSet() for _, comment := range field.Doc.List { - if marker := extractMarker(comment); marker.Identifier != "" { + marker := extractMarker(comment) + if marker.Identifier != "" { fieldMarkers.Insert(marker) } } @@ -221,34 +227,98 @@ func extractMarker(comment *ast.Comment) Marker { return Marker{} } - id, expressions := extractMarkerIDAndExpressions(DefaultRegistry(), markerContent) + if isDeclarativeValidationMarker(markerContent) { + marker := extractDeclarativeValidationMarker(markerContent, comment) + if marker == nil { + return Marker{} + } + + return *marker + } + + return extractKubebuilderMarker(markerContent, comment) +} + +func extractKubebuilderMarker(markerContent string, comment *ast.Comment) Marker { + id, arguments, payload := extractMarkerIDArgumentsAndPayload(DefaultRegistry(), markerContent) return Marker{ - Identifier: id, - Expressions: expressions, - RawComment: comment.Text, - Pos: comment.Pos(), - End: comment.End(), + Type: MarkerTypeKubebuilder, + Identifier: id, + Arguments: arguments, + Payload: payload, + RawComment: comment.Text, + Pos: comment.Pos(), + End: comment.End(), } } -func extractMarkerIDAndExpressions(knownMarkers Registry, marker string) (string, map[string]string) { +func extractMarkerIDArgumentsAndPayload(knownMarkers Registry, marker string) (string, map[string]string, Payload) { if id, ok := knownMarkers.Match(marker); ok { - return extractKnownMarkerIDAndExpressions(id, marker) + return extractKnownMarkerIDArgumentsAndPayload(id, marker) } - return extractUnknownMarkerIDAndExpressions(marker) + return extractUnknownMarkerIDArgumentsAndPayload(marker) } -func extractKnownMarkerIDAndExpressions(id string, marker string) (string, map[string]string) { - return id, extractExpressions(strings.TrimPrefix(marker, id)) +func isDeclarativeValidationMarker(marker string) bool { + return strings.HasPrefix(marker, "k8s:") +} + +func extractDeclarativeValidationMarker(marker string, comment *ast.Comment) *Marker { + tag, err := codetags.Parse(marker) + if err != nil { + return nil + } + + return markerForTag(tag, comment) +} + +func markerForTag(tag codetags.Tag, comment *ast.Comment) *Marker { + out := &Marker{ + Type: MarkerTypeDeclarativeValidation, + Identifier: tag.Name, + Arguments: make(map[string]string), + RawComment: comment.Text, + Pos: comment.Pos(), + End: comment.End(), + } + + for _, arg := range tag.Args { + out.Arguments[arg.Name] = arg.Value + } + + switch tag.ValueType { + case codetags.ValueTypeString, codetags.ValueTypeInt, codetags.ValueTypeBool, codetags.ValueTypeRaw: + // all resolvable to an exact string value + out.Payload = Payload{ + Value: tag.Value, + } + case codetags.ValueTypeNone: + // nothing + case codetags.ValueTypeTag: + out.Payload = Payload{ + Marker: markerForTag(*tag.ValueTag, comment), + } + default: + return nil + } + + return out +} + +func extractKnownMarkerIDArgumentsAndPayload(id string, marker string) (string, map[string]string, Payload) { + args, payload := extractArgumentsAndPayload(strings.TrimPrefix(marker, id)) + return id, args, payload } var expressionRegex = regexp.MustCompile("\\w*=(?:'[^']*'|\"(\\\\\"|[^\"])*\"|[\\w;\\-\"]+|`[^`]*`)") -func extractExpressions(expressionStr string) map[string]string { +func extractArgumentsAndPayload(expressionStr string) (map[string]string, Payload) { expressionsMap := map[string]string{} + var payload Payload + // Do some normalization work to ensure we can parse expressions in // a standard way. Trim any lingering colons (:) and replace all ':='s with '=' expressionStr = strings.TrimPrefix(expressionStr, ":") @@ -261,13 +331,18 @@ func extractExpressions(expressionStr string) map[string]string { continue } + if key == UnnamedArgument { + payload.Value = value + continue + } + expressionsMap[key] = value } - return expressionsMap + return expressionsMap, payload } -func extractUnknownMarkerIDAndExpressions(marker string) (string, map[string]string) { +func extractUnknownMarkerIDArgumentsAndPayload(marker string) (string, map[string]string, Payload) { // if there is only a single "=" split on the equal sign and trim any // dangling ":" characters. if strings.Count(marker, "=") == 1 { @@ -277,10 +352,8 @@ func extractUnknownMarkerIDAndExpressions(marker string) (string, map[string]str identifier := strings.TrimSuffix(splits[0], ":") // If there is a single "=" sign that means the left side of the - // marker is the identifier and there is no real expression identifier. - expressions := map[string]string{UnnamedExpression: splits[1]} - - return identifier, expressions + // marker is the identifier and there is no real argument identifier. + return identifier, make(map[string]string), Payload{Value: splits[1]} } // split on : @@ -315,18 +388,65 @@ func extractUnknownMarkerIDAndExpressions(marker string) (string, map[string]str expressionString = strings.Join([]string{expressionString, item}, ",") } - expressions := extractExpressions(expressionString) + expressions, payload := extractArgumentsAndPayload(expressionString) + + return identifier, expressions, payload +} + +// MarkerType is a representation of the style of marker. +// Currently can be one of Kubebuilder or DeclarativeValidation. +type MarkerType string + +const ( + // MarkerTypeKubebuilder represents a Kubebuilder-style marker. + MarkerTypeKubebuilder MarkerType = "Kubebuilder" + // MarkerTypeDeclarativeValidation represents a Declarative Validation marker. + MarkerTypeDeclarativeValidation MarkerType = "DeclarativeValidation" +) - return identifier, expressions +// Payload represents the payload of a marker. +type Payload struct { + // Value is the payload value of a marker represented as a string. + // Value is set when the payload value of a marker is not another marker. + Value string + + // Marker is the marker in the payload value of another marker. + // Marker is only set when the payload value of a marker is another marker. + Marker *Marker } // Marker represents a marker extracted from a comment on a declaration. type Marker struct { + // Type is the marker representation this marker was identified as. + // Currently, the two marker format types are DeclarativeValidation and Kubebuilder. + // Because the Kubebuilder style has been around the longest and is widely + // used in projects that have CustomResourceDefinitions we default to Kubebuilder + // style parsing unless we detect that the marker follows the declarative validation + // format (i.e begins with +k8s:). + Type MarkerType + // Identifier is the value of the marker once the leading comment, '+', and expressions are trimmed. Identifier string - // Expressions are the set of expressions that have been specified for the marker - Expressions map[string]string + // Arguments are the set of named and unnamed arguments that have been specified for the marker. + // + // For Markers with Type == Kubebuilder, there will only ever be named arguments. The following examples highlight how arguments are extracted: + // - `+kubebuilder:validation:Required` would result in *no* arguments. + // - `+required` would result in *no* arguments. + // - `+kubebuilder:validation:MinLength=10` would result in no arguments`. + // - `+kubebuilder:validation:XValidation:rule="has(self)",message="should have self"` would result in 2 named arguments, `rule` and `message` with their respective values in string representation. + // + // For Markers with Type == DeclarativeValidation, arguments are extracted from the marker parameters. Arguments may be named or unnamed. + // Some examples: + // - `+k8s:forbidden` would result in *no* arguments. + // - `+k8s:ifEnabled("my-feature")=...` would result in a single unnamed argument (represented by key `""`) with a value of `"my-feature"`. + // - `+k8s:item(one: "value", two: "value")=...` would result in 2 named arguments, `one` and `two` with their respective values in string representation. + Arguments map[string]string + + // Payload is the payload specified by the marker. + // In general, it is what is present after the first `=` symbol + // of a marker. + Payload Payload // RawComment is the raw comment line, unfiltered. RawComment string @@ -377,22 +497,33 @@ func (ms MarkerSet) Has(identifier string) bool { } // HasWithValue returns whether marker(s) with the given identifier and -// expression values (i.e "kubebuilder:object:root:=true") is present +// argument/payload values (i.e "kubebuilder:object:root:=true") is present // in the MarkerSet. func (ms MarkerSet) HasWithValue(marker string) bool { - return ms.HasWithExpressions(extractMarkerIDAndExpressions(DefaultRegistry(), marker)) + if isDeclarativeValidationMarker(marker) { + marker := extractDeclarativeValidationMarker(marker, &ast.Comment{}) + if marker == nil { + return false + } + + return ms.HasWithArgumentsAndPayload(marker.Identifier, marker.Arguments, marker.Payload) + } + + id, args, payload := extractMarkerIDArgumentsAndPayload(DefaultRegistry(), marker) + + return ms.HasWithArgumentsAndPayload(id, args, payload) } -// HasWithExpressions returns whether marker(s) with the identifier and -// expressions are present in the MarkerSet. -func (ms MarkerSet) HasWithExpressions(identifier string, expressions map[string]string) bool { +// HasWithArgumentsAndPayload returns whether marker(s) with the +// identifier, arguments, and payload are present in the MarkerSet. +func (ms MarkerSet) HasWithArgumentsAndPayload(identifier string, arguments map[string]string, payload Payload) bool { markers, ok := ms[identifier] if !ok { return false } for _, marker := range markers { - if reflect.DeepEqual(marker.Expressions, expressions) { + if reflect.DeepEqual(marker.Arguments, arguments) && reflect.DeepEqual(marker.Payload, payload) { return true } } diff --git a/pkg/analysis/helpers/markers/analyzer_test.go b/pkg/analysis/helpers/markers/analyzer_test.go index d02f514d..eef6bae3 100644 --- a/pkg/analysis/helpers/markers/analyzer_test.go +++ b/pkg/analysis/helpers/markers/analyzer_test.go @@ -19,215 +19,357 @@ import ( "go/ast" "testing" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" . "github.com/onsi/gomega" ) -func TestExtractMarkerIdAndExpressions(t *testing.T) { +func TestExtractMarker(t *testing.T) { type testcase struct { - name string - marker string - expectedID string - expectedExpressions map[string]string + name string + comment *ast.Comment + expected Marker } testcases := []testcase{ + // Kubebuilder-style markers { - name: "registered marker with single unnamed expression using '='", - marker: "kubebuilder:object:root=true", - expectedID: "kubebuilder:object:root", - expectedExpressions: map[string]string{ - "": "true", + name: "non-namespaced marker", + comment: &ast.Comment{Text: "// +required"}, + expected: Marker{ + Type: MarkerTypeKubebuilder, + Identifier: "required", + Arguments: make(map[string]string), }, }, { - name: "registered marker with single unnamed expression using ':='", - marker: "kubebuilder:object:root:=true", - expectedID: "kubebuilder:object:root", - expectedExpressions: map[string]string{ - "": "true", + name: "non-namespaced marker with payload value", + comment: &ast.Comment{Text: "// +listType=atomic"}, + expected: Marker{ + Type: MarkerTypeKubebuilder, + Identifier: "listType", + Arguments: make(map[string]string), + Payload: Payload{ + Value: "atomic", + }, }, }, { - name: "registered marker with no expressions", - marker: "required", - expectedID: "required", - expectedExpressions: map[string]string{}, + name: "kubebuilder marker with single unnamed expression using '='", + comment: &ast.Comment{Text: "// +kubebuilder:object:root=true"}, + expected: Marker{ + Type: MarkerTypeKubebuilder, + Identifier: "kubebuilder:object:root", + Arguments: make(map[string]string), + Payload: Payload{ + Value: "true", + }, + }, }, { - name: "registered marker with multiple named expressions", - marker: "kubebuilder:validation:XValidation:rule='has(self.field)',message='must have field!'", - expectedID: "kubebuilder:validation:XValidation", - expectedExpressions: map[string]string{ - "rule": "'has(self.field)'", - "message": "'must have field!'", + name: "kubebuilder marker with single unnamed expression using ':='", + comment: &ast.Comment{Text: "// +kubebuilder:object:root:=true"}, + expected: Marker{ + Type: MarkerTypeKubebuilder, + Identifier: "kubebuilder:object:root", + Arguments: make(map[string]string), + Payload: Payload{ + Value: "true", + }, }, }, { - name: " unregistered marker with expression wrapped in double quotes (\")", - marker: "foo:bar:rule=\"foo\"", - expectedID: "foo:bar:rule", - expectedExpressions: map[string]string{ - "": "\"foo\"", + name: "kubebuilder marker with no expressions", + comment: &ast.Comment{Text: "// +kubebuilder:validation:Required"}, + expected: Marker{ + Type: MarkerTypeKubebuilder, + Identifier: "kubebuilder:validation:Required", + Arguments: make(map[string]string), }, }, { - name: "registered marker with expression with a comma in its value", - marker: `kubebuilder:validation:XValidation:rule='self.map(a, a == "someValue")',message='must have field!'`, - expectedID: "kubebuilder:validation:XValidation", - expectedExpressions: map[string]string{ - "rule": `'self.map(a, a == "someValue")'`, - "message": "'must have field!'", + name: "kubebuilder marker with multiple named expressions", + comment: &ast.Comment{Text: "// +kubebuilder:validation:XValidation:rule='has(self.field)',message='must have field!'"}, + expected: Marker{ + Type: MarkerTypeKubebuilder, + Identifier: "kubebuilder:validation:XValidation", + Arguments: map[string]string{ + "rule": "'has(self.field)'", + "message": "'must have field!'", + }, }, }, { - name: "registered marker with expression with a comma in its value with double quotes", - marker: `kubebuilder:validation:XValidation:rule="self.map(a, a == \"someValue\")",message="must have field!"`, - expectedID: "kubebuilder:validation:XValidation", - expectedExpressions: map[string]string{ - "rule": `"self.map(a, a == \"someValue\")"`, - "message": `"must have field!"`, + name: "other namespaced marker in kubebuilder-style with expression wrapped in double quotes (\")", + comment: &ast.Comment{Text: "// +foo:bar:rule=\"foo\""}, + expected: Marker{ + Type: MarkerTypeKubebuilder, + Identifier: "foo:bar:rule", + Arguments: make(map[string]string), + Payload: Payload{ + Value: "\"foo\"", + }, }, }, { - name: "registered marker with expression ending in a valid double quote", - marker: `kubebuilder:validation:Enum:=foo;bar;baz;""`, - expectedID: "kubebuilder:validation:Enum", - expectedExpressions: map[string]string{ - "": `foo;bar;baz;""`, + name: "kubebuilder marker with expression with a comma in its value", + comment: &ast.Comment{Text: `// +kubebuilder:validation:XValidation:rule='self.map(a, a == "someValue")',message='must have field!'`}, + expected: Marker{ + Type: MarkerTypeKubebuilder, + Identifier: "kubebuilder:validation:XValidation", + Arguments: map[string]string{ + "rule": `'self.map(a, a == "someValue")'`, + "message": "'must have field!'", + }, }, }, { - name: "registered marker with chained expressions without quotes", - marker: `custom:marker:fruit=apple,color=blue,country=UK`, - expectedID: "custom:marker", - expectedExpressions: map[string]string{ - "fruit": "apple", - "color": "blue", - "country": "UK", + name: "kubebuilder marker with expression with a comma in its value with double quotes", + comment: &ast.Comment{Text: `// +kubebuilder:validation:XValidation:rule="self.map(a, a == \"someValue\")",message="must have field!"`}, + expected: Marker{ + Type: MarkerTypeKubebuilder, + Identifier: "kubebuilder:validation:XValidation", + Arguments: map[string]string{ + "rule": `"self.map(a, a == \"someValue\")"`, + "message": `"must have field!"`, + }, }, }, { - name: "registered marker with numeric value", - marker: `kubebuilder:validation:Minimum=10`, - expectedID: "kubebuilder:validation:Minimum", - expectedExpressions: map[string]string{ - "": "10", + name: "kubebuilder marker with expression ending in a valid double quote", + comment: &ast.Comment{Text: `// +kubebuilder:validation:Enum:=foo;bar;baz;""`}, + expected: Marker{ + Type: MarkerTypeKubebuilder, + Identifier: "kubebuilder:validation:Enum", + Arguments: make(map[string]string), + Payload: Payload{ + Value: "foo;bar;baz;\"\"", + }, }, }, { - name: "registered marker with negative numeric value", - marker: `kubebuilder:validation:Minimum=-10`, - expectedID: "kubebuilder:validation:Minimum", - expectedExpressions: map[string]string{ - "": "-10", + name: "other namespaced kubebuilder-style marker with chained expressions without quotes", + comment: &ast.Comment{Text: `// +custom:marker:fruit=apple,color=blue,country=UK`}, + expected: Marker{ + Type: MarkerTypeKubebuilder, + Identifier: "custom:marker", + Arguments: map[string]string{ + "fruit": "apple", + "color": "blue", + "country": "UK", + }, }, }, { - name: "registered marker with named expression using backtick ('`') for strings", - marker: "kubebuilder:validation:XValidation:rule=`has(self.field)`,message=`must have field!`", - expectedID: "kubebuilder:validation:XValidation", - expectedExpressions: map[string]string{ - "rule": "`has(self.field)`", - "message": "`must have field!`", + name: "kubebuilder marker with numeric value", + comment: &ast.Comment{Text: `// +kubebuilder:validation:Minimum=10`}, + expected: Marker{ + Type: MarkerTypeKubebuilder, + Identifier: "kubebuilder:validation:Minimum", + Arguments: make(map[string]string), + Payload: Payload{ + Value: "10", + }, + }, + }, + { + name: "kubebuilder marker with negative numeric value", + comment: &ast.Comment{Text: `// +kubebuilder:validation:Minimum=-10`}, + expected: Marker{ + Type: MarkerTypeKubebuilder, + Identifier: "kubebuilder:validation:Minimum", + Arguments: make(map[string]string), + Payload: Payload{ + Value: "-10", + }, + }, + }, + { + name: "kubebuilder marker with named expression using backtick ('`') for strings", + comment: &ast.Comment{Text: "// +kubebuilder:validation:XValidation:rule=`has(self.field)`,message=`must have field!`"}, + expected: Marker{ + Type: MarkerTypeKubebuilder, + Identifier: "kubebuilder:validation:XValidation", + Arguments: map[string]string{ + "rule": "`has(self.field)`", + "message": "`must have field!`", + }, }, }, - } - - for _, tc := range testcases { - t.Run(tc.name, func(t *testing.T) { - g := NewWithT(t) - - reg := NewRegistry() - reg.Register(tc.expectedID) - - id, expressions := extractMarkerIDAndExpressions(reg, tc.marker) - - g.Expect(id).To(Equal(tc.expectedID), "marker", tc.marker) - g.Expect(expressions).To(Equal(tc.expectedExpressions), "marker", tc.marker) - }) - } -} - -func TestExtractMarker(t *testing.T) { - type testcase struct { - name string - comment string - shouldBeMarker bool - expectedID string - } - testcases := []testcase{ + // Not actually markers { - name: "valid marker - required", - comment: "// +required", - shouldBeMarker: true, - expectedID: "required", + name: "invalid marker - markdown table border", + comment: &ast.Comment{Text: "// +-------+-------+-------+"}, + expected: Marker{}, }, { - name: "valid marker - kubebuilder:validation:Required", - comment: "// +kubebuilder:validation:Required", - shouldBeMarker: true, - expectedID: "kubebuilder:validation:Required", + name: "invalid marker - markdown table border without pipes", + comment: &ast.Comment{Text: "// +----------"}, + expected: Marker{}, }, { - name: "valid marker - with expressions", - comment: "// +kubebuilder:validation:XValidation:rule=\"something\",message=\"haha\"", - shouldBeMarker: true, - expectedID: "kubebuilder:validation:XValidation", + name: "invalid marker - starts with special characters", + comment: &ast.Comment{Text: "// +!*@(#&KSDJUF:A"}, + expected: Marker{}, }, { - name: "valid marker - with parentheses", - comment: "// +k8s:ifEnabled(\"foo\")=+k8s:required", - shouldBeMarker: true, - expectedID: "k8s:ifEnabled(\"foo\")", + name: "regular comment - no plus sign", + comment: &ast.Comment{Text: "// This is a regular comment"}, + expected: Marker{}, }, + + // Declarative Validation Tag Parsing { - name: "valid marker - with single quotes and parentheses", - comment: "// +k8s:ifEnabled('foo')=+k8s:required", - shouldBeMarker: true, - expectedID: "k8s:ifEnabled('foo')", + name: "simple declarative validation marker", + comment: &ast.Comment{ + Text: "// +k8s:required", + }, + expected: Marker{ + Type: MarkerTypeDeclarativeValidation, + Identifier: "k8s:required", + Arguments: make(map[string]string), + }, }, { - name: "valid marker - with backtickets and parentheses", - comment: "// +k8s:ifEnabled(`foo`)=+k8s:required", - shouldBeMarker: true, - expectedID: "k8s:ifEnabled(`foo`)", + name: "declarative validation marker with a value", + comment: &ast.Comment{ + Text: "// +k8s:maxLength=10", + }, + expected: Marker{ + Type: MarkerTypeDeclarativeValidation, + Identifier: "k8s:maxLength", + Arguments: make(map[string]string), + Payload: Payload{ + Value: "10", + }, + }, }, { - name: "invalid marker - markdown table border", - comment: "// +-------+-------+-------+", - shouldBeMarker: false, - expectedID: "", + name: "declarative validation marker with named argument", + comment: &ast.Comment{ + Text: "// +k8s:unionMember(union: \"union1\")", + }, + expected: Marker{ + Type: MarkerTypeDeclarativeValidation, + Identifier: "k8s:unionMember", + Arguments: map[string]string{ + "union": "union1", + }, + }, }, { - name: "invalid marker - markdown table border without pipes", - comment: "// +----------", - shouldBeMarker: false, - expectedID: "", + name: "declarative validation marker with multiple named arguments", + comment: &ast.Comment{Text: "// +k8s:someThing(one: \"a\", two: \"b\")=+k8s:required"}, + expected: Marker{ + Type: MarkerTypeDeclarativeValidation, + Identifier: "k8s:someThing", + Arguments: map[string]string{ + "one": "a", + "two": "b", + }, + Payload: Payload{ + Marker: &Marker{ + Type: MarkerTypeDeclarativeValidation, + Identifier: "k8s:required", + Arguments: make(map[string]string), + }, + }, + }, }, { - name: "invalid marker - starts with special characters", - comment: "// +!*@(#&KSDJUF:A", - shouldBeMarker: false, - expectedID: "", + name: "declarative validation marker with unnamed argument", + comment: &ast.Comment{ + Text: "// +k8s:doesWork(100)", // not a real DV marker, but AFAIK nothing stops something like this from coming up + }, + expected: Marker{ + Type: MarkerTypeDeclarativeValidation, + Identifier: "k8s:doesWork", + Arguments: map[string]string{ + "": "100", + }, + }, }, { - name: "regular comment - no plus sign", - comment: "// This is a regular comment", - shouldBeMarker: false, - expectedID: "", + name: "declarative validation marker with unnamed argument in backticks", + comment: &ast.Comment{Text: "// +k8s:doesWork(`foo`)"}, // not a real DV marker, but AFAIK nothing stops something like this from coming up + expected: Marker{ + Type: MarkerTypeDeclarativeValidation, + Identifier: "k8s:doesWork", + Arguments: map[string]string{ + "": "foo", + }, + }, }, { - name: "valid marker - complex nested expression", - comment: "// +k8s:someThing(one: \"a\", two: \"b\")=+k8s:required", - shouldBeMarker: true, - expectedID: "k8s:someThing(one: \"a\", two: \"b\")", + name: "declarative validation marker with unnamed argument and simple validation tag payload", + comment: &ast.Comment{ + Text: "// +k8s:ifEnabled(\"my-feature\")=+k8s:required", + }, + expected: Marker{ + Type: MarkerTypeDeclarativeValidation, + Identifier: "k8s:ifEnabled", + Arguments: map[string]string{ + "": "my-feature", + }, + Payload: Payload{ + Marker: &Marker{ + Type: MarkerTypeDeclarativeValidation, + Identifier: "k8s:required", + Arguments: make(map[string]string), + }, + }, + }, + }, + { + name: "declarative validation marker with deeper chained validation tags", + comment: &ast.Comment{ + Text: "// +k8s:ifEnabled(\"my-feature\")=+k8s:item(type: \"Approved\")=+k8s:zeroOrOneOfMember", + }, + expected: Marker{ + Type: MarkerTypeDeclarativeValidation, + Identifier: "k8s:ifEnabled", + Arguments: map[string]string{ + "": "my-feature", + }, + Payload: Payload{ + Marker: &Marker{ + Type: MarkerTypeDeclarativeValidation, + Identifier: "k8s:item", + Arguments: map[string]string{ + "type": "Approved", + }, + Payload: Payload{ + Marker: &Marker{ + Type: MarkerTypeDeclarativeValidation, + Identifier: "k8s:zeroOrOneOfMember", + Arguments: make(map[string]string), + }, + }, + }, + }, + }, }, { - name: "valid marker - complex nested expression with '", - comment: "// +k8s:someThing(one: 'a', two: 'b')=+k8s:required", - shouldBeMarker: true, - expectedID: "k8s:someThing(one: 'a', two: 'b')", + name: "declarative validation marker parsing error", + comment: &ast.Comment{ + Text: "// +k8s:ifEnabled(\"my-feature\")=a,b,c,d", // DV tags do not allow comma separated payloads + }, + expected: Marker{}, + }, + { + name: "declarative validation ':='", + comment: &ast.Comment{ + Text: "// +k8s:format:=password", // DV tags parse out ':=' weirdly. This is a typo, DV tags are supposed to use '='. + }, + expected: Marker{ + Type: MarkerTypeDeclarativeValidation, + Identifier: "k8s:format:", + Arguments: make(map[string]string), + Payload: Payload{ + Value: "password", + }, + }, }, } @@ -235,18 +377,10 @@ func TestExtractMarker(t *testing.T) { t.Run(tc.name, func(t *testing.T) { g := NewWithT(t) - // Create a mock comment - comment := &ast.Comment{ - Text: tc.comment, - } - - marker := extractMarker(comment) + marker := extractMarker(tc.comment) - if tc.shouldBeMarker { - g.Expect(marker.Identifier).To(Equal(tc.expectedID), "comment", tc.comment) - } else { - g.Expect(marker.Identifier).To(BeEmpty(), "comment", tc.comment) - } + diff := cmp.Diff(marker, tc.expected, cmpopts.IgnoreFields(Marker{}, "RawComment", "End", "Pos")) + g.Expect(diff).To(BeEmpty()) }) } } diff --git a/pkg/analysis/ssatags/analyzer.go b/pkg/analysis/ssatags/analyzer.go index 455b529b..565eab8d 100644 --- a/pkg/analysis/ssatags/analyzer.go +++ b/pkg/analysis/ssatags/analyzer.go @@ -121,7 +121,7 @@ func (a *analyzer) checkField(pass *analysis.Pass, field *ast.Field, markersAcce } for _, marker := range listTypeMarkers { - listType := marker.Expressions[""] + listType := marker.Payload.Value a.checkListTypeMarker(pass, listType, field) @@ -208,7 +208,7 @@ func (a *analyzer) validateListMapKeys(pass *analysis.Pass, field *ast.Field, li fieldName := utils.FieldName(field) for _, marker := range listMapKeyMarkers { - keyName := marker.Expressions[""] + keyName := marker.Payload.Value if keyName == "" { continue } diff --git a/pkg/analysis/uniquemarkers/analyzer.go b/pkg/analysis/uniquemarkers/analyzer.go index 85c0fece..45690028 100644 --- a/pkg/analysis/uniquemarkers/analyzer.go +++ b/pkg/analysis/uniquemarkers/analyzer.go @@ -121,17 +121,42 @@ func constructIdentifier(marker markers.Marker, attributes ...string) string { return marker.Identifier } - // If a marker doesn't specify the attribute, we should assume it is equivalent - // to the empty string ("") so that we can still key on uniqueness of other attributes - // effectively. - id := fmt.Sprintf("%s:", marker.Identifier) - for _, attr := range attributes { - id += fmt.Sprintf("%s=%s,", attr, marker.Expressions[attr]) - } + switch marker.Type { + case markers.MarkerTypeDeclarativeValidation: + // If a marker doesn't specify the attribute, we should assume it is equivalent + // to the empty string ("") so that we can still key on uniqueness of other attributes + // effectively. + id := fmt.Sprintf("%s(", marker.Identifier) + + for _, attr := range attributes { + if attr == "" { + id += marker.Arguments[attr] + continue + } + + id += fmt.Sprintf("%s: %s,", attr, marker.Arguments[attr]) + } + + id = strings.TrimSuffix(id, ",") + id += ")" + + return id + case markers.MarkerTypeKubebuilder: + // If a marker doesn't specify the attribute, we should assume it is equivalent + // to the empty string ("") so that we can still key on uniqueness of other attributes + // effectively. + id := fmt.Sprintf("%s:", marker.Identifier) + for _, attr := range attributes { + id += fmt.Sprintf("%s=%s,", attr, marker.Arguments[attr]) + } - id = strings.TrimSuffix(id, ",") + id = strings.TrimSuffix(id, ",") - return id + return id + default: + // programmer error + panic(fmt.Sprintf("unknown marker format %s", marker.Type)) + } } func reportField(pass *analysis.Pass, field *ast.Field) func(id string) { diff --git a/pkg/analysis/uniquemarkers/analyzer_test.go b/pkg/analysis/uniquemarkers/analyzer_test.go index 2d6a9706..d5e26ba5 100644 --- a/pkg/analysis/uniquemarkers/analyzer_test.go +++ b/pkg/analysis/uniquemarkers/analyzer_test.go @@ -55,6 +55,21 @@ func TestWithConfiguration(t *testing.T) { "country", }, }, + { + Identifier: "k8s:maxLength", + }, + { + Identifier: "k8s:uniqueMarkerArguments", + Attributes: []string{ + "fruit", + }, + }, + { + Identifier: "k8s:uniqueMarkerUnnamedArguments", + Attributes: []string{ + "", + }, + }, }, }) if err != nil { diff --git a/pkg/analysis/uniquemarkers/testdata/src/a/k8s.go b/pkg/analysis/uniquemarkers/testdata/src/a/k8s.go index df228786..29562099 100644 --- a/pkg/analysis/uniquemarkers/testdata/src/a/k8s.go +++ b/pkg/analysis/uniquemarkers/testdata/src/a/k8s.go @@ -1,45 +1,45 @@ package a type K8s struct { - // +k8s:format:=date-time + // +k8s:format=date-time UniqueFormat string - // +k8s:format:=date-time - // +k8s:format:=password + // +k8s:format=date-time + // +k8s:format=password NonUniqueFormat string // want "field NonUniqueFormat has multiple definitions of marker k8s:format when only a single definition should exist" - // +k8s:minLength:=10 + // +k8s:minLength=10 UniqueMinLength string - // +k8s:minLength:=10 - // +k8s:minLength:=20 + // +k8s:minLength=10 + // +k8s:minLength=20 NonUniqueMinLength string // want "field NonUniqueMinLength has multiple definitions of marker k8s:minLength when only a single definition should exist" - // +k8s:maxLength:=100 + // +k8s:maxLength=100 UniqueMaxLength string - // +k8s:maxLength:=100 - // +k8s:maxLength:=200 + // +k8s:maxLength=100 + // +k8s:maxLength=200 NonUniqueMaxLength string // want "field NonUniqueMaxLength has multiple definitions of marker k8s:maxLength when only a single definition should exist" - // +k8s:minItems:=10 + // +k8s:minItems=10 UniqueMinItems []string - // +k8s:minItems:=10 - // +k8s:minItems:=20 + // +k8s:minItems=10 + // +k8s:minItems=20 NonUniqueMinItems []string // want "field NonUniqueMinItems has multiple definitions of marker k8s:minItems when only a single definition should exist" - // +k8s:maxItems:=100 + // +k8s:maxItems=100 UniqueMaxItems []string - // +k8s:maxItems:=100 - // +k8s:maxItems:=200 + // +k8s:maxItems=100 + // +k8s:maxItems=200 NonUniqueMaxItems []string // want "field NonUniqueMaxItems has multiple definitions of marker k8s:maxItems when only a single definition should exist" - // +k8s:listType:=map + // +k8s:listType=map UniqueListType []string - // +k8s:listType:=map - // +k8s:listType:=atomic + // +k8s:listType=map + // +k8s:listType=atomic NonUniqueListType []string // want "field NonUniqueListType has multiple definitions of marker k8s:listType when only a single definition should exist" } diff --git a/pkg/analysis/uniquemarkers/testdata/src/b/b.go b/pkg/analysis/uniquemarkers/testdata/src/b/b.go index 7ba5b8e7..5b860a8e 100644 --- a/pkg/analysis/uniquemarkers/testdata/src/b/b.go +++ b/pkg/analysis/uniquemarkers/testdata/src/b/b.go @@ -54,6 +54,38 @@ type B struct { // +custom:MultiMarker:fruit=apple,color=blue // +custom:MultiMarker:fruit=apple,color=blue NonUniqueMultiAttributeMarkerFromMissingAttribute string // want "field NonUniqueMultiAttributeMarkerFromMissingAttribute has multiple definitions of marker custom:MultiMarker:fruit=apple,color=blue,country= when only a single definition should exist" + + // +k8s:uniqueMarkerArguments(fruit: "apple")=10 + // +k8s:uniqueMarkerArguments(fruit: "blueberry")=10 + UniqueDeclarativeValidationMarkerWithArgumentsSimplePayload string + + // +k8s:uniqueMarkerArguments(fruit: "apple")=10 + // +k8s:uniqueMarkerArguments(fruit: "apple")=20 + NonUniqueDeclarativeValidationMarkerWithArgumentsSimplePayload string // want "field NonUniqueDeclarativeValidationMarkerWithArgumentsSimplePayload has multiple definitions of marker k8s:uniqueMarkerArguments\\(fruit\\: apple\\) when only a single definition should exist" + + // +k8s:uniqueMarkerArguments(fruit: "apple")=+k8s:maxLength=10 + // +k8s:uniqueMarkerArguments(fruit: "blueberry")=+k8s:maxLength=10 + UniqueDeclarativeValidationMarkerWithArgumentsTagPayload string + + // +k8s:uniqueMarkerArguments(fruit: "apple")=+k8s:maxLength=10 + // +k8s:uniqueMarkerArguments(fruit: "apple")=+k8s:maxLength=20 + NonUniqueDeclarativeValidationMarkerWithArgumentsTagPayload string // want "field NonUniqueDeclarativeValidationMarkerWithArgumentsTagPayload has multiple definitions of marker k8s:uniqueMarkerArguments\\(fruit\\: apple\\) when only a single definition should exist" + + // +k8s:uniqueMarkerArguments(fruit: "apple")=+k8s:ifEnabled("thingy")=+k8s:maxLength=10 + // +k8s:uniqueMarkerArguments(fruit: "blueberry")=+k8s:ifEnabled("thingy")=+k8s:maxLength=10 + UniqueDeclarativeValidationMarkerWithArgumentsComplexPayload string + + // +k8s:uniqueMarkerArguments(fruit: "apple")=+k8s:ifEnabled("thingy")=+k8s:maxLength=10 + // +k8s:uniqueMarkerArguments(fruit: "apple")=+k8s:ifEnabled("otherthingy")=+k8s:maxLength=20 + NonUniqueDeclarativeValidationMarkerWithArgumentsComplexPayload string // want "field NonUniqueDeclarativeValidationMarkerWithArgumentsComplexPayload has multiple definitions of marker k8s:uniqueMarkerArguments\\(fruit\\: apple\\) when only a single definition should exist" + + // +k8s:uniqueMarkerUnnamedArguments("apple")=10 + // +k8s:uniqueMarkerUnnamedArguments("blueberry")=10 + UniqueDeclarativeValidationMarkerWithUnnamedArgument string + + // +k8s:uniqueMarkerUnnamedArguments("apple")=10 + // +k8s:uniqueMarkerUnnamedArguments("apple")=20 + NonUniqueDeclarativeValidationMarkerWithUnnamedArgument string // want "field NonUniqueDeclarativeValidationMarkerWithUnnamedArgument has multiple definitions of marker k8s:uniqueMarkerUnnamedArguments\\(apple\\) when only a single definition should exist" } // +custom:SomeCustomMarker:=diffvalue diff --git a/pkg/analysis/utils/zero_value.go b/pkg/analysis/utils/zero_value.go index 10b73882..f80f4af4 100644 --- a/pkg/analysis/utils/zero_value.go +++ b/pkg/analysis/utils/zero_value.go @@ -234,7 +234,7 @@ func enumFieldAllowsEmpty(fieldMarkers markershelper.MarkerSet) bool { enumMarker := fieldMarkers.Get(markers.KubebuilderEnumMarker) for _, marker := range enumMarker { - return slices.Contains(strings.Split(marker.Expressions[""], ";"), "\"\"") + return slices.Contains(strings.Split(marker.Payload.Value, ";"), "\"\"") } return false @@ -290,11 +290,12 @@ func getMarkerNumericValueByName[N number](marker markershelper.MarkerSet, marke // getMarkerNumericValue extracts a numeric value from the default value of a marker. // Works for markers like MaxLength, MinLength, etc. func getMarkerNumericValue[N number](marker markershelper.Marker) (N, error) { - rawValue, ok := marker.Expressions[""] - if !ok { + if marker.Payload.Value == "" { return N(0), errMarkerMissingValue } + rawValue := marker.Payload.Value + value, err := strconv.ParseFloat(rawValue, 64) if err != nil { return N(0), fmt.Errorf("error converting value to number: %w", err)