fixup! finalize implementation

Signed-off-by: Bryce Palmer <[email protected]>

Author: everettraven

docs/linters.md

@@ -3,12 +3,14 @@
 - [Conditions](#conditions) - Checks that `Conditions` fields are correctly formatted
 - [CommentStart](#commentstart) - Ensures comments start with the serialized form of the type
 - [DuplicateMarkers](#duplicatemarkers) - Checks for exact duplicates of markers
+- [ForbiddenMarkers](#forbiddenmarkers) - Checks that no forbidden markers are present on types/fields.
 - [Integers](#integers) - Validates usage of supported integer types
 - [JSONTags](#jsontags) - Ensures proper JSON tag formatting
 - [MaxLength](#maxlength) - Checks for maximum length constraints on strings and arrays
 - [NoBools](#nobools) - Prevents usage of boolean types
 - [NoFloats](#nofloats) - Prevents usage of floating-point types
 - [Nomaps](#nomaps) - Restricts usage of map types
+- [NoNullable](#nonullable) - Prevents usage of the nullable marker
 - [Nophase](#nophase) - Prevents usage of 'Phase' fields
 - [Notimestamp](#notimestamp) - Prevents usage of 'TimeStamp' fields
 - [OptionalFields](#optionalfields) - Validates optional field conventions
@@ -98,6 +100,62 @@ will not.
 The `duplicatemarkers` linter can automatically fix all markers that are exact match to another markers.
 If there are duplicates across fields and their underlying type, the marker on the type will be preferred and the marker on the field will be removed.
 
+## ForbiddenMarkers
+
+The `forbiddenmarkers` linter ensures that types and fields do not contain any markers that are forbidden.
+
+By default, `forbiddenmarkers` is not enabled.
+
+### Configuation
+
+It can be configured with a list of marker identifiers and optionally their attributes and values that are forbidden
+
+```yaml
+lintersConfig:
+  forbiddenMarkers:
+    markers:
+      - identifier: forbidden:marker
+      - identifier: forbidden:withAttribute
+        attributes:
+          - name: fruit
+      - identifier: forbidden:withMultipleAttributes
+        attributes:
+          - name: fruit
+          - name: color
+      - identifier: forbidden:withAttributeValues
+        attributes:
+          - name: fruit
+            values:
+              - apple
+              - banana
+              - orange
+      - identifier: forbidden:withMultipleAttributesValues
+        attributes:
+          - name: fruit
+            values:
+              - apple
+              - banana
+              - orange
+          - name: color
+            values:
+              - red
+              - green
+              - blue
+```
+
+Using the config above, the following examples would be forbidden:
+
+- `+forbidden:marker` (all instances, including if they have attributes and values)
+- `+forbidden:withAttribute:fruit=*,*` (all instances of this marker containing the attribute 'fruit')
+- `+forbidden:withMultipleAttributes:fruit=*,color=*,*` (all instances of this marker containing both the 'fruit' AND 'color' attributes)
+- `+forbidden:withAttributeValues:fruit={apple || banana || orange},*` (all instances of this marker containing the 'fruit' attribute where the value is set to one of 'apple', 'banana', or 'orange')
+
+- `+forbidden:withMultipleAttributesValues:fruit={apple || banana || orange},color={red || green || blue},*` (all instances of this marker containing the 'fruit' attribute where the value is set to one of 'apple', 'banana', or 'orange' AND the 'color' attribute where the value is set to one of 'red', 'green', or 'blue')
+
+### Fixes
+
+Fixes are suggested to remove all markers that are forbidden.
+
 ## Integers
 
 The `integers` linter checks for usage of unsupported integer types.
@@ -161,6 +219,14 @@ lintersConfig:
     policy: Enforce | AllowStringToStringMaps | Ignore # Determines how the linter should handle maps of simple types. Defaults to AllowStringToStringMaps.
 ```
 
+## NoNullable
+
+The `nonullable` linter ensures that types and fields do not have the `nullable` marker.
+
+### Fixes
+
+Fixes are suggested to remove the `nullable` marker.
+
 ## Notimestamp
 
 The `notimestamp` linter checks that the fields in the API are not named with the word 'Timestamp'.
@@ -334,10 +400,12 @@ linter will suggest adding the comment `// +kubebuilder:subresource:status` abov
 
 The `uniquemarkers` linter ensures that types and fields do not contain more than a single definition of a marker that should only be present once.
 
-Because this linter has no way of determining which marker definition was intended it does not suggest any fixes 
+Because this linter has no way of determining which marker definition was intended it does not suggest any fixes
 
 ### Configuration
+
 It can configured to include a set of custom markers in the analysis by setting:
+
 ```yaml
 lintersConfig:
   uniquemarkers:

pkg/analysis/forbiddenmarkers/analyzer.go

@@ -25,36 +25,17 @@ import (
 	"sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/inspector"
 	"sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/markers"
 	"sigs.k8s.io/kube-api-linter/pkg/analysis/utils"
-	"sigs.k8s.io/kube-api-linter/pkg/config"
 )
 
 const name = "forbiddenmarkers"
 
 type analyzer struct {
-	forbiddenMarkers []config.ForbiddenMarker
-}
-
-// ForbiddenMarkersOptions is a function that configures the
-// forbiddenmarkers analysis.Analyzer
-type ForbiddenMarkersOption func(a *analysis.Analyzer)
-
-// WithName sets the name of the forbiddenmarkers analysis.Analyzer
-func WithName(name string) ForbiddenMarkersOption {
-	return func(a *analysis.Analyzer) {
-		a.Name = name
-	}
-}
-
-// WithDoc sets the doc string of the forbiddenmarkers analysis.Analyzer
-func WithDoc(doc string) ForbiddenMarkersOption {
-	return func(a *analysis.Analyzer) {
-		a.Doc = doc
-	}
+	forbiddenMarkers []Marker
 }
 
 // NewAnalyzer creates a new analysis.Analyzer for the forbiddenmarkers
 // linter based on the provided config.ForbiddenMarkersConfig.
-func NewAnalyzer(cfg config.ForbiddenMarkersConfig, opts ...ForbiddenMarkersOption) *analysis.Analyzer {
+func newAnalyzer(cfg *Config) *analysis.Analyzer {
 	a := &analyzer{
 		forbiddenMarkers: cfg.Markers,
 	}
@@ -66,8 +47,8 @@ func NewAnalyzer(cfg config.ForbiddenMarkersConfig, opts ...ForbiddenMarkersOpti
 		Requires: []*analysis.Analyzer{inspector.Analyzer},
 	}
 
-	for _, opt := range opts {
-		opt(analyzer)
+	for _, marker := range a.forbiddenMarkers {
+		markers.DefaultRegistry().Register(marker.Identifier)
 	}
 
 	return analyzer
@@ -90,7 +71,7 @@ func (a *analyzer) run(pass *analysis.Pass) (any, error) {
 	return nil, nil //nolint:nilnil
 }
 
-func checkField(pass *analysis.Pass, field *ast.Field, markersAccess markers.Markers, forbiddenMarkers []config.ForbiddenMarker) {
+func checkField(pass *analysis.Pass, field *ast.Field, markersAccess markers.Markers, forbiddenMarkers []Marker) {
 	if field == nil || len(field.Names) == 0 {
 		return
 	}
@@ -99,7 +80,7 @@ func checkField(pass *analysis.Pass, field *ast.Field, markersAccess markers.Mar
 	check(markers, forbiddenMarkers, reportField(pass, field))
 }
 
-func checkType(pass *analysis.Pass, typeSpec *ast.TypeSpec, markersAccess markers.Markers, forbiddenMarkers []config.ForbiddenMarker) {
+func checkType(pass *analysis.Pass, typeSpec *ast.TypeSpec, markersAccess markers.Markers, forbiddenMarkers []Marker) {
 	if typeSpec == nil {
 		return
 	}
@@ -108,7 +89,7 @@ func checkType(pass *analysis.Pass, typeSpec *ast.TypeSpec, markersAccess marker
 	check(markers, forbiddenMarkers, reportType(pass, typeSpec))
 }
 
-func check(markerSet markers.MarkerSet, forbiddenMarkers []config.ForbiddenMarker, reportFunc func(marker markers.Marker)) {
+func check(markerSet markers.MarkerSet, forbiddenMarkers []Marker, reportFunc func(marker markers.Marker)) {
 	for _, marker := range forbiddenMarkers {
 		marks := markerSet.Get(marker.Identifier)
 		for _, mark := range marks {
@@ -119,12 +100,10 @@ func check(markerSet markers.MarkerSet, forbiddenMarkers []config.ForbiddenMarke
 	}
 }
 
-// TODO: this should probably return some representation of the marker that is failing the
-// attribute rules so that it can be returned to users helpfully.
-func markerMatchesAttributeRules(marker markers.Marker, attrRules ...config.ForbiddenMarkerAttribute) bool {
+func markerMatchesAttributeRules(marker markers.Marker, attrRules ...MarkerAttribute) bool {
 	matchesAll := true
 	for _, attrRule := range attrRules {
-		if val, ok := marker.Expressions[attrRule.Attribute]; ok {
+		if val, ok := marker.Expressions[attrRule.Name]; ok {
 			// if no values are specified, that means the existence match is enough
 			// and we can continue to the next rule
 			if len(attrRule.Values) == 0 {
@@ -144,6 +123,7 @@ func markerMatchesAttributeRules(marker markers.Marker, attrRules ...config.Forb
 				matchesAll = false
 				break
 			}
+			continue
 		}
 		// if the marker doesn't contain the attribute for a specified rule it fails the AND
 		// operation.
@@ -158,14 +138,14 @@ func reportField(pass *analysis.Pass, field *ast.Field) func(marker markers.Mark
 	return func(marker markers.Marker) {
 		pass.Report(analysis.Diagnostic{
 			Pos:     field.Pos(),
-			Message: fmt.Sprintf("field %s has forbidden marker %q", field.Names[0].Name, marker.Identifier),
+			Message: fmt.Sprintf("field %s has forbidden marker %q", field.Names[0].Name, marker.String()),
 			SuggestedFixes: []analysis.SuggestedFix{
 				{
-					Message: fmt.Sprintf("remove forbidden marker %q", marker.Identifier),
+					Message: fmt.Sprintf("remove forbidden marker %q", marker.String()),
 					TextEdits: []analysis.TextEdit{
 						{
 							Pos: marker.Pos,
-							End: marker.End + 1,
+							End: marker.End,
 						},
 					},
 				},
@@ -178,7 +158,18 @@ func reportType(pass *analysis.Pass, typeSpec *ast.TypeSpec) func(marker markers
 	return func(marker markers.Marker) {
 		pass.Report(analysis.Diagnostic{
 			Pos:     typeSpec.Pos(),
-			Message: fmt.Sprintf("type %s has forbidden marker %q", typeSpec.Name, marker.Identifier),
+			Message: fmt.Sprintf("type %s has forbidden marker %q", typeSpec.Name, marker.String()),
+			SuggestedFixes: []analysis.SuggestedFix{
+				{
+					Message: fmt.Sprintf("remove forbidden marker %q", marker.String()),
+					TextEdits: []analysis.TextEdit{
+						{
+							Pos: marker.Pos,
+							End: marker.End,
+						},
+					},
+				},
+			},
 		})
 	}
 }

pkg/analysis/forbiddenmarkers/analyzer_test.go

@@ -13,36 +13,34 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 See the License for the specific language governing permissions and
 limitations under the License.
 */
-package forbiddenmarkers_test
+package forbiddenmarkers
 
 import (
 	"testing"
 
 	"golang.org/x/tools/go/analysis/analysistest"
-	"sigs.k8s.io/kube-api-linter/pkg/analysis/forbiddenmarkers"
-	"sigs.k8s.io/kube-api-linter/pkg/config"
 )
 
 func TestWithConfiguration(t *testing.T) {
 	testdata := analysistest.TestData()
-	analysistest.RunWithSuggestedFixes(t, testdata, forbiddenmarkers.NewAnalyzer(config.ForbiddenMarkersConfig{
-		Markers: []config.ForbiddenMarker{
+	analysistest.RunWithSuggestedFixes(t, testdata, newAnalyzer(&Config{
+		Markers: []Marker{
 			{
 				Identifier: "custom:forbidden",
 			},
 			{
 				Identifier: "custom:AttrNoValues",
-				Attributes: []config.ForbiddenMarkerAttribute{
+				Attributes: []MarkerAttribute{
 					{
-						Attribute: "fruit",
+						Name: "fruit",
 					},
 				},
 			},
 			{
 				Identifier: "custom:AttrValues",
-				Attributes: []config.ForbiddenMarkerAttribute{
+				Attributes: []MarkerAttribute{
 					{
-						Attribute: "fruit",
+						Name: "fruit",
 						Values: []string{
 							"apple",
 							"orange",
@@ -53,28 +51,28 @@ func TestWithConfiguration(t *testing.T) {
 			},
 			{
 				Identifier: "custom:AttrsNoValues",
-				Attributes: []config.ForbiddenMarkerAttribute{
+				Attributes: []MarkerAttribute{
 					{
-						Attribute: "fruit",
+						Name: "fruit",
 					},
 					{
-						Attribute: "color",
+						Name: "color",
 					},
 				},
 			},
 			{
 				Identifier: "custom:AttrsValues",
-				Attributes: []config.ForbiddenMarkerAttribute{
+				Attributes: []MarkerAttribute{
 					{
-						Attribute: "fruit",
+						Name: "fruit",
 						Values: []string{
 							"apple",
 							"orange",
 							"banana",
 						},
 					},
 					{
-						Attribute: "color",
+						Name: "color",
 						Values: []string{
 							"red",
 							"blue",

pkg/analysis/forbiddenmarkers/config.go

@@ -0,0 +1,43 @@
+package forbiddenmarkers
+
+// Config is the configuration type
+// for the forbiddenmarkers linter.
+type Config struct {
+	// markers is the unique set of markers
+	// that are forbidden on types/fields.
+	// Uniqueness is keyed on the `identifier`
+	// field of entries.
+	// Must have at least one entry.
+	Markers []Marker `json:"markers"`
+}
+
+// Marker is a representation of a
+// type/field marker that should be forbidden.
+type Marker struct {
+	// identifier is the identifier for the forbidden marker.
+	Identifier string `json:"identifier"`
+	// attributes is an optional unique set of
+	// attributes that is forbidden for this marker.
+	// Uniqueness is keyed on the `name` field of entries.
+	// When specified, only instances of this marker
+	// that contains all the attributes will be considered
+	// forbidden.
+	// When not specified, any instance of the marker will be
+	// considered forbidden.
+	Attributes []MarkerAttribute `json:"attributes,omitempty"`
+}
+
+// MarkerAttribute is a representation of an
+// attribute for a marker.
+type MarkerAttribute struct {
+	// name is the name of the forbidden attribute
+	Name string `json:"name"`
+	// values is an optional unique set of
+	// values that are forbidden for this marker.
+	// When specified, only the instances of this
+	// attribute that set one of these forbidden values
+	// will be considered forbidden.
+	// When not specified, any use of this attribute
+	// will be considered forbidden.
+	Values []string `json:"values,omitempty"`
+}