From 0209587f113856ef44976709979fec070cddfd26 Mon Sep 17 00:00:00 2001 From: Krish Agarwal Date: Wed, 5 Nov 2025 12:18:37 -0500 Subject: [PATCH 1/5] Added enums linter modified: docs/linters.md new file: pkg/analysis/enums/analyzer.go new file: pkg/analysis/enums/analyzer_test.go new file: pkg/analysis/enums/config.go new file: pkg/analysis/enums/doc.go new file: pkg/analysis/enums/initializer.go new file: pkg/analysis/enums/testdata/src/a/a.go new file: pkg/analysis/enums/testdata/src/b/b.go modified: pkg/registration/doc.go --- docs/linters.md | 24 +- pkg/analysis/enums/analyzer.go | 357 +++++++++++++++++++++++++ pkg/analysis/enums/analyzer_test.go | 52 ++++ pkg/analysis/enums/config.go | 23 ++ pkg/analysis/enums/doc.go | 90 +++++++ pkg/analysis/enums/initializer.go | 53 ++++ pkg/analysis/enums/testdata/src/a/a.go | 122 +++++++++ pkg/analysis/enums/testdata/src/b/b.go | 29 ++ pkg/registration/doc.go | 1 + 9 files changed, 750 insertions(+), 1 deletion(-) create mode 100644 pkg/analysis/enums/analyzer.go create mode 100644 pkg/analysis/enums/analyzer_test.go create mode 100644 pkg/analysis/enums/config.go create mode 100644 pkg/analysis/enums/doc.go create mode 100644 pkg/analysis/enums/initializer.go create mode 100644 pkg/analysis/enums/testdata/src/a/a.go create mode 100644 pkg/analysis/enums/testdata/src/b/b.go diff --git a/docs/linters.md b/docs/linters.md index 594f8407..149ce8b2 100644 --- a/docs/linters.md +++ b/docs/linters.md @@ -6,6 +6,7 @@ - [ConflictingMarkers](#conflictingmarkers) - Detects mutually exclusive markers on the same field - [DefaultOrRequired](#defaultorrequired) - Ensures fields marked as required do not have default values - [DuplicateMarkers](#duplicatemarkers) - Checks for exact duplicates of markers +- [Enums](#enums) - Enforces proper usage of enumerated fields with type aliases and +enum marker - [DependentTags](#dependenttags) - Enforces dependencies between markers - [ForbiddenMarkers](#forbiddenmarkers) - Checks that no forbidden markers are present on types/fields. - [Integers](#integers) - Validates usage of supported integer types @@ -235,13 +236,34 @@ 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. +## Enums + +The `enums` linter enforces that enumerated fields use type aliases with the `+enum` marker and that enum values follow PascalCase naming conventions. + +This provides better API evolution, self-documentation, and validation compared to plain strings. + +By default, `enums` is not enabled. + +### Configuration + +```yaml +linterConfig: + enums: + allowlist: + - kubectl + - docker + - helm +``` + +The `allowlist` field contains enum values that should be exempt from PascalCase validation, such as command-line executable names. + ## ForbiddenMarkers The `forbiddenmarkers` linter ensures that types and fields do not contain any markers that are forbidden. By default, `forbiddenmarkers` is not enabled. -### Configuation +### Configuration It can be configured with a list of marker identifiers and optionally their attributes and values that are forbidden. diff --git a/pkg/analysis/enums/analyzer.go b/pkg/analysis/enums/analyzer.go new file mode 100644 index 00000000..b8845960 --- /dev/null +++ b/pkg/analysis/enums/analyzer.go @@ -0,0 +1,357 @@ +/* +Copyright 2025 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +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 enums + +import ( + "fmt" + "go/ast" + "go/types" + "strings" + "unicode" + + "golang.org/x/tools/go/analysis" + kalerrors "sigs.k8s.io/kube-api-linter/pkg/analysis/errors" + "sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/extractjsontags" + "sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/inspector" + markershelper "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/markers" +) + +const ( + name = "enums" +) + +type analyzer struct { + config *Config +} + +// newAnalyzer creates a new analysis.Analyzer for the enums linter based on the provided config. +func newAnalyzer(cfg *Config) *analysis.Analyzer { + a := &analyzer{ + config: cfg, + } + + return &analysis.Analyzer{ + Name: name, + Doc: "Enforces that enumerated fields use type aliases with +enum marker and have PascalCase values", + Run: a.run, + Requires: []*analysis.Analyzer{inspector.Analyzer}, + } +} + +func (a *analyzer) run(pass *analysis.Pass) (any, error) { + inspect, ok := pass.ResultOf[inspector.Analyzer].(inspector.Inspector) + if !ok { + return nil, kalerrors.ErrCouldNotGetInspector + } + + // Check struct fields for proper enum usage + inspect.InspectFields(func(field *ast.Field, _ extractjsontags.FieldTagInfo, markersAccess markershelper.Markers) { + a.checkField(pass, field, markersAccess) + }) + + // Check type declarations for +enum markers + inspect.InspectTypeSpec(func(typeSpec *ast.TypeSpec, markersAccess markershelper.Markers) { + a.checkTypeSpec(pass, typeSpec, markersAccess) + }) + + // Check const values for PascalCase + a.checkConstValues(pass, inspect) + + return nil, nil //nolint:nilnil +} + +func (a *analyzer) checkField(pass *analysis.Pass, field *ast.Field, markersAccess markershelper.Markers) { + fieldName := utils.FieldName(field) + if fieldName == "" { + return + } + // Get the underlying type, unwrapping pointers and arrays + fieldType, isArray := unwrapTypeWithArrayTracking(field.Type) + ident, ok := fieldType.(*ast.Ident) + if !ok { + return + } + + // Build appropriate prefix for error messages + prefix := fmt.Sprintf("field %s", fieldName) + if isArray { + prefix = fmt.Sprintf("field %s array element", fieldName) + } + + // Check if it's a basic string type + if ident.Name == "string" && utils.IsBasicType(pass, ident) { + // Check if the field has an enum marker directly + fieldMarkers := markersAccess.FieldMarkers(field) + if !hasEnumMarker(fieldMarkers) { + pass.Reportf(field.Pos(), + "%s uses plain string without +enum marker. Enumerated fields should use a type alias with +enum marker", + prefix) + } + return + } + // If it's a type alias, check that the alias has an enum marker + if !utils.IsBasicType(pass, ident) { + typeSpec, ok := utils.LookupTypeSpec(pass, ident) + if !ok { + return + } + // Check if the underlying type is string + if !isStringTypeAlias(pass, typeSpec) { + return + } + // Check if the type has an enum marker + typeMarkers := markersAccess.TypeMarkers(typeSpec) + if !hasEnumMarker(typeMarkers) { + pass.Reportf(field.Pos(), + "%s uses type %s which appears to be an enum but is missing +enum marker (kubebuilder:validation:Enum)", + prefix, typeSpec.Name.Name) + } + } +} + +func (a *analyzer) checkTypeSpec(pass *analysis.Pass, typeSpec *ast.TypeSpec, markersAccess markershelper.Markers) { + if typeSpec.Name == nil { + return + } + typeMarkers := markersAccess.TypeMarkers(typeSpec) + if !hasEnumMarker(typeMarkers) { + return + } + + // Has +enum marker, verify it's a string type + if !isStringTypeAlias(pass, typeSpec) { + pass.Reportf(typeSpec.Pos(), + "type %s has +enum marker but underlying type is not string", + typeSpec.Name.Name) + } +} + +func (a *analyzer) checkConstValues(pass *analysis.Pass, inspect inspector.Inspector) { + // We need to check const declarations, but the inspector helper doesn't + // have a method for this, so we'll iterate through files manually + for _, file := range pass.Files { + for _, decl := range file.Decls { + genDecl, ok := decl.(*ast.GenDecl) + if !ok || genDecl.Tok.String() != "const" { + continue + } + for _, spec := range genDecl.Specs { + valueSpec, ok := spec.(*ast.ValueSpec) + if !ok { + continue + } + a.checkConstSpec(pass, valueSpec) + } + } + } +} + +func (a *analyzer) checkConstSpec(pass *analysis.Pass, valueSpec *ast.ValueSpec) { + for i, name := range valueSpec.Names { + if name == nil { + continue + } + // Get the type of the constant + obj := pass.TypesInfo.ObjectOf(name) + if obj == nil { + continue + } + constObj, ok := obj.(*types.Const) + if !ok { + continue + } + // Check if the type is a named type (potential enum) + namedType, ok := constObj.Type().(*types.Named) + if !ok { + continue + } + // Check if the type is in the current package + if namedType.Obj().Pkg() == nil || namedType.Obj().Pkg() != pass.Pkg { + continue + } + // Find the type spec for this named type + typeSpec := findTypeSpecByName(pass, namedType.Obj().Name()) + if typeSpec == nil { + continue + } + // Check if this type has an enum marker + if !hasEnumMarkerOnTypeSpec(pass, typeSpec) { + continue + } + // This is an enum constant, validate the value + if i >= len(valueSpec.Values) { + continue + } + value := valueSpec.Values[i] + basicLit, ok := value.(*ast.BasicLit) + if !ok { + continue + } + // Extract the string value (remove quotes) + strValue := strings.Trim(basicLit.Value, `"`) + // Check if it's in the allowlist + if a.isInAllowlist(strValue) { + continue + } + // Validate PascalCase + if !isPascalCase(strValue) { + pass.Reportf(basicLit.Pos(), + "enum value %q should be PascalCase (e.g., \"PhasePending\", \"StateRunning\")", + strValue) + } + } +} + +// unwrapType removes pointer and array wrappers to get the underlying type +func unwrapType(expr ast.Expr) ast.Expr { + switch t := expr.(type) { + case *ast.StarExpr: + return unwrapType(t.X) + case *ast.ArrayType: + return unwrapType(t.Elt) + default: + return expr + } +} + +// unwrapTypeWithArrayTracking removes pointer and array wrappers to get the underlying type +// and tracks whether an array was encountered during unwrapping. +func unwrapTypeWithArrayTracking(expr ast.Expr) (ast.Expr, bool) { + isArray := false + for { + switch t := expr.(type) { + case *ast.StarExpr: + expr = t.X + case *ast.ArrayType: + expr = t.Elt + isArray = true + default: + return expr, isArray + } + } +} + +// isStringTypeAlias checks if a type spec is an alias for string +func isStringTypeAlias(pass *analysis.Pass, typeSpec *ast.TypeSpec) bool { + underlyingType := unwrapType(typeSpec.Type) + ident, ok := underlyingType.(*ast.Ident) + if !ok { + return false + } + return ident.Name == "string" && utils.IsBasicType(pass, ident) +} + +// hasEnumMarker checks if a marker set contains an enum marker +func hasEnumMarker(markerSet markershelper.MarkerSet) bool { + return markerSet.Has(markers.KubebuilderEnumMarker) || markerSet.Has(markers.K8sEnumMarker) +} + +// hasEnumMarkerOnTypeSpec checks if a type spec has an enum marker by checking its doc comments +func hasEnumMarkerOnTypeSpec(pass *analysis.Pass, typeSpec *ast.TypeSpec) bool { + for _, file := range pass.Files { + for _, decl := range file.Decls { + genDecl, ok := decl.(*ast.GenDecl) + if !ok { + continue + } + for _, spec := range genDecl.Specs { + if spec == typeSpec { + if genDecl.Doc != nil { + for _, comment := range genDecl.Doc.List { + if strings.Contains(comment.Text, markers.KubebuilderEnumMarker) || + strings.Contains(comment.Text, markers.K8sEnumMarker) { + return true + } + } + } + return false + } + } + } + } + return false +} + +// isInAllowlist checks if a value is in the configured allowlist +func (a *analyzer) isInAllowlist(value string) bool { + if a.config == nil { + return false + } + for _, allowed := range a.config.Allowlist { + if value == allowed { + return true + } + } + return false +} + +// findTypeSpecByName searches through the AST files to find a TypeSpec by name +func findTypeSpecByName(pass *analysis.Pass, typeName string) *ast.TypeSpec { + for _, file := range pass.Files { + for _, decl := range file.Decls { + genDecl, ok := decl.(*ast.GenDecl) + if !ok { + continue + } + for _, spec := range genDecl.Specs { + typeSpec, ok := spec.(*ast.TypeSpec) + if !ok { + continue + } + if typeSpec.Name != nil && typeSpec.Name.Name == typeName { + return typeSpec + } + } + } + } + return nil +} + +// isPascalCase validates that a string follows PascalCase convention +// PascalCase: FirstLetterUpperCase, no underscores, no hyphens +func isPascalCase(s string) bool { + if len(s) == 0 { + return false + } + // First character must be uppercase + if !unicode.IsUpper(rune(s[0])) { + return false + } + // Check for invalid characters (underscores, hyphens) + for _, r := range s { + if r == '_' || r == '-' { + return false + } + // Only allow letters and digits + if !unicode.IsLetter(r) && !unicode.IsDigit(r) { + return false + } + } + // Should not be all uppercase (that's SCREAMING_SNAKE_CASE or similar) + allUpper := true + for _, r := range s[1:] { + if unicode.IsLower(r) { + allUpper = false + break + } + } + if allUpper && len(s) > 1 { + return false + } + return true +} diff --git a/pkg/analysis/enums/analyzer_test.go b/pkg/analysis/enums/analyzer_test.go new file mode 100644 index 00000000..14ffdd88 --- /dev/null +++ b/pkg/analysis/enums/analyzer_test.go @@ -0,0 +1,52 @@ +/* +Copyright 2025 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +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 enums_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + "sigs.k8s.io/kube-api-linter/pkg/analysis/enums" +) + +func TestAnalyzer(t *testing.T) { + testdata := analysistest.TestData() + + // Test without allowlist + config := &enums.Config{} + analyzer, err := enums.Initializer().Init(config) + if err != nil { + t.Fatalf("initializing enums linter: %v", err) + } + + analysistest.Run(t, testdata, analyzer, "a") +} + +func TestAnalyzerWithAllowlist(t *testing.T) { + testdata := analysistest.TestData() + + // Test with allowlist + config := &enums.Config{ + Allowlist: []string{"kubectl", "docker", "helm"}, + } + + analyzer, err := enums.Initializer().Init(config) + if err != nil { + t.Fatalf("initializing enums linter: %v", err) + } + + analysistest.Run(t, testdata, analyzer, "b") +} diff --git a/pkg/analysis/enums/config.go b/pkg/analysis/enums/config.go new file mode 100644 index 00000000..1a6bfac0 --- /dev/null +++ b/pkg/analysis/enums/config.go @@ -0,0 +1,23 @@ +/* +Copyright 2025 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +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 enums + +// Config is the configuration for the enums linter. +type Config struct { + // Allowlist contains values that are exempt from PascalCase validation. + // This is useful for command-line executable names like "kubectl", "docker", etc. + Allowlist []string `yaml:"allowlist" json:"allowlist"` +} diff --git a/pkg/analysis/enums/doc.go b/pkg/analysis/enums/doc.go new file mode 100644 index 00000000..f3c48e95 --- /dev/null +++ b/pkg/analysis/enums/doc.go @@ -0,0 +1,90 @@ +/* +Copyright 2025 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +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. +*/ + +/* +enums is an analyzer that enforces proper usage of enumerated fields in Kubernetes APIs. + +Enumerated fields provide better API evolution and self-documentation compared to plain strings. +By using type aliases with explicit enum markers, the API schema can include valid values and +provide better validation at the schema level. + +# Rules + +The linter checks for three main patterns: +1. Fields must use type aliases, not plain strings: String fields that represent enums should +use a type alias with an +enum marker rather than a raw string type. +2. Type aliases must have +enum marker: Type aliases used for enumerated values must be +annotated with either // +kubebuilder:validation:Enum or // +k8s:enum. +3. Enum values must be PascalCase: Constant values for enums should follow PascalCase naming +(e.g., "PhasePending", "StateRunning") rather than lowercase, snake_case, or SCREAMING_SNAKE_CASE. + +# Examples + +Good: + + // +kubebuilder:validation:Enum + type Phase string + const ( + PhasePending Phase = "Pending" + PhaseRunning Phase = "Running" + PhaseSucceeded Phase = "Succeeded" + ) + type MySpec struct { + Phase Phase + } + +Bad: + + // Missing +enum marker + type Phase string + type MySpec struct { + // Plain string without type alias + Phase string + } + // Values not PascalCase + const ( + phase_pending Phase = "pending" // Should be "Pending" + PHASE_RUNNING Phase = "RUNNING" // Should be "Running" + ) + +# Configuration + +The linter supports an allowlist for enum values that should be exempt from PascalCase +validation, such as command-line executable names: + + linterConfig: + enums: + allowlist: + - kubectl + - docker + - helm + +# Rationale + +Using type aliases for enums instead of raw strings provides several benefits: +- API schemas can explicitly list valid enum values +- Better validation at both the schema and runtime level +- Improved documentation and API evolution +- Stronger type safety in generated clients +- Clearer intent for API consumers + +The PascalCase convention for enum values aligns with Kubernetes API conventions and +improves readability and consistency across the ecosystem. + +Note: This linter is disabled by default as enum usage is recommended but not strictly +required for all Kubernetes APIs. Enable it in your configuration to enforce these conventions. +*/ +package enums diff --git a/pkg/analysis/enums/initializer.go b/pkg/analysis/enums/initializer.go new file mode 100644 index 00000000..353d4862 --- /dev/null +++ b/pkg/analysis/enums/initializer.go @@ -0,0 +1,53 @@ +/* +Copyright 2025 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +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 enums + +import ( + "golang.org/x/tools/go/analysis" + "k8s.io/apimachinery/pkg/util/validation/field" + "sigs.k8s.io/kube-api-linter/pkg/analysis/initializer" + "sigs.k8s.io/kube-api-linter/pkg/analysis/registry" +) + +func init() { + registry.DefaultRegistry().RegisterLinter(Initializer()) +} + +// Initializer returns the AnalyzerInitializer for this +// Analyzer so that it can be added to the registry. +func Initializer() initializer.AnalyzerInitializer { + return initializer.NewConfigurableInitializer( + name, + initAnalyzer, + // The enums linter is opt-in as enum usage is not strictly + // required for all Kubernetes APIs, though it is recommended. + false, + validateConfig, + ) +} + +func initAnalyzer(cfg *Config) (*analysis.Analyzer, error) { + if cfg == nil { + cfg = &Config{} + } + return newAnalyzer(cfg), nil +} + +// validateConfig implements validation of the enums linter config. +func validateConfig(cfg *Config, fldPath *field.Path) field.ErrorList { + // Config is optional, allowlist can be empty + return field.ErrorList{} +} diff --git a/pkg/analysis/enums/testdata/src/a/a.go b/pkg/analysis/enums/testdata/src/a/a.go new file mode 100644 index 00000000..e1bfd571 --- /dev/null +++ b/pkg/analysis/enums/testdata/src/a/a.go @@ -0,0 +1,122 @@ +package a + +// Valid enum type with proper marker +// +kubebuilder:validation:Enum +type Phase string + +const ( + PhasePending Phase = "Pending" // Valid PascalCase + PhaseRunning Phase = "Running" // Valid PascalCase + PhaseSucceeded Phase = "Succeeded" // Valid PascalCase + PhaseFailed Phase = "Failed" // Valid PascalCase +) + +// Alternative marker format +// +k8s:enum +type State string + +const ( + StateActive State = "Active" // Valid PascalCase + StateInactive State = "Inactive" // Valid PascalCase +) + +// Invalid: Missing +enum marker +// This type doesn't have +enum marker, so it will be flagged when used in fields +type Status string + +const ( + StatusReady Status = "Ready" + StatusNotReady Status = "NotReady" +) + +// Invalid: Type with +enum but not string +// +kubebuilder:validation:Enum +type InvalidEnumType int // want "type InvalidEnumType has \\+enum marker but underlying type is not string" + +// Invalid enum values (not PascalCase) +// +kubebuilder:validation:Enum +type BadPhase string + +const ( + phase_pending BadPhase = "pending" // want "enum value \"pending\" should be PascalCase" + PHASE_RUNNING BadPhase = "RUNNING" // want "enum value \"RUNNING\" should be PascalCase" + phase_succeeded BadPhase = "succeeded" // want "enum value \"succeeded\" should be PascalCase" + Phase_Failed BadPhase = "Phase-Failed" // want "enum value \"Phase-Failed\" should be PascalCase" +) + +// Test struct with fields +type MySpec struct { + // Valid: uses type alias with +enum + Phase Phase + + // Valid: uses type alias with +enum + State State + + // Invalid: plain string without +enum + PlainString string // want "field PlainString uses plain string without \\+enum marker" + + // Invalid: type alias without +enum marker + Status Status // want "field Status uses type Status which appears to be an enum but is missing \\+enum marker" + + // Valid: pointer to enum type + PhasePtr *Phase + + // Valid: slice of enum type + Phases []Phase + + // Invalid: plain string slice + PlainStrings []string // want "field PlainStrings array element uses plain string without \\+enum marker" +} + +// Test pointer fields +type PointerSpec struct { + PhasePtr *Phase + StatusPtr *Status // want "field StatusPtr uses type Status which appears to be an enum but is missing \\+enum marker" + PlainStrPtr *string // want "field PlainStrPtr uses plain string without \\+enum marker" +} + +// Test array fields +type ArraySpec struct { + Phases []Phase + Statuses []Status // want "field Statuses array element uses type Status which appears to be an enum but is missing \\+enum marker" + PlainStrArray []string // want "field PlainStrArray array element uses plain string without \\+enum marker" +} + +// Embedded field test +type EmbeddedSpec struct { + MySpec + Phase Phase +} + +// Edge case: Field with +enum marker directly on the field (should be allowed as exception) +type DirectMarkerSpec struct { + // +kubebuilder:validation:Enum + DirectEnum string // Valid: has +enum marker directly on field +} + +// Edge case: Enum values with numbers (should be valid PascalCase) +// +kubebuilder:validation:Enum +type Priority string + +const ( + Priority1 Priority = "Priority1" // Valid: PascalCase with number + Priority2 Priority = "Priority2" // Valid: PascalCase with number +) + +// Edge case: Single letter uppercase (edge case for all-caps check) +// +kubebuilder:validation:Enum +type Level string + +const ( + LevelA Level = "A" // Valid: single uppercase letter + LevelB Level = "B" // Valid: single uppercase letter +) + +// Edge case: Map with enum types (maps are allowed, not enforced to use enum types) +type MapSpec struct { + // Valid: map with enum value type + PhaseMap map[string]Phase + + // Valid: map with plain string value (maps are not enforced to use enums) + PlainMap map[string]string +} diff --git a/pkg/analysis/enums/testdata/src/b/b.go b/pkg/analysis/enums/testdata/src/b/b.go new file mode 100644 index 00000000..9c6f7b06 --- /dev/null +++ b/pkg/analysis/enums/testdata/src/b/b.go @@ -0,0 +1,29 @@ +package b + +// Test with allowlist configuration + +// Valid enum type with proper marker +// +kubebuilder:validation:Enum +type ExecutableName string + +const ( + ExecKubectl ExecutableName = "kubectl" // Valid: in allowlist + ExecDocker ExecutableName = "docker" // Valid: in allowlist + ExecHelm ExecutableName = "helm" // Valid: in allowlist + ExecUnknown ExecutableName = "unknown" // want "enum value \"unknown\" should be PascalCase" +) + +// Regular enum with PascalCase (should still work) +// +kubebuilder:validation:Enum +type Status string + +const ( + StatusReady Status = "Ready" // Valid PascalCase + StatusNotReady Status = "NotReady" // Valid PascalCase +) + +// Test struct +type MyConfig struct { + Executable ExecutableName + Status Status +} diff --git a/pkg/registration/doc.go b/pkg/registration/doc.go index 439a7513..528a3bb9 100644 --- a/pkg/registration/doc.go +++ b/pkg/registration/doc.go @@ -30,6 +30,7 @@ import ( _ "sigs.k8s.io/kube-api-linter/pkg/analysis/defaultorrequired" _ "sigs.k8s.io/kube-api-linter/pkg/analysis/dependenttags" _ "sigs.k8s.io/kube-api-linter/pkg/analysis/duplicatemarkers" + _ "sigs.k8s.io/kube-api-linter/pkg/analysis/enums" _ "sigs.k8s.io/kube-api-linter/pkg/analysis/forbiddenmarkers" _ "sigs.k8s.io/kube-api-linter/pkg/analysis/integers" _ "sigs.k8s.io/kube-api-linter/pkg/analysis/jsontags" From a6d6e1da8e257c31cdeed0abf75e403da7299ee8 Mon Sep 17 00:00:00 2001 From: Krish Agarwal Date: Wed, 5 Nov 2025 12:38:24 -0500 Subject: [PATCH 2/5] Simplified loops, switches, and conditionals. Cleaned comments --- pkg/analysis/enums/analyzer.go | 234 ++++++++++++++------------------- 1 file changed, 102 insertions(+), 132 deletions(-) diff --git a/pkg/analysis/enums/analyzer.go b/pkg/analysis/enums/analyzer.go index b8845960..29395374 100644 --- a/pkg/analysis/enums/analyzer.go +++ b/pkg/analysis/enums/analyzer.go @@ -39,12 +39,8 @@ type analyzer struct { config *Config } -// newAnalyzer creates a new analysis.Analyzer for the enums linter based on the provided config. func newAnalyzer(cfg *Config) *analysis.Analyzer { - a := &analyzer{ - config: cfg, - } - + a := &analyzer{config: cfg} return &analysis.Analyzer{ Name: name, Doc: "Enforces that enumerated fields use type aliases with +enum marker and have PascalCase values", @@ -68,10 +64,7 @@ func (a *analyzer) run(pass *analysis.Pass) (any, error) { inspect.InspectTypeSpec(func(typeSpec *ast.TypeSpec, markersAccess markershelper.Markers) { a.checkTypeSpec(pass, typeSpec, markersAccess) }) - - // Check const values for PascalCase a.checkConstValues(pass, inspect) - return nil, nil //nolint:nilnil } @@ -86,41 +79,41 @@ func (a *analyzer) checkField(pass *analysis.Pass, field *ast.Field, markersAcce if !ok { return } + prefix := buildFieldPrefix(fieldName, isArray) + if ident.Name == "string" && utils.IsBasicType(pass, ident) { + a.checkPlainStringField(pass, field, markersAccess, prefix) + return + } + a.checkTypeAliasField(pass, field, ident, markersAccess, prefix) +} - // Build appropriate prefix for error messages - prefix := fmt.Sprintf("field %s", fieldName) +func buildFieldPrefix(fieldName string, isArray bool) string { if isArray { - prefix = fmt.Sprintf("field %s array element", fieldName) + return fmt.Sprintf("field %s array element", fieldName) } + return fmt.Sprintf("field %s", fieldName) +} - // Check if it's a basic string type - if ident.Name == "string" && utils.IsBasicType(pass, ident) { - // Check if the field has an enum marker directly - fieldMarkers := markersAccess.FieldMarkers(field) - if !hasEnumMarker(fieldMarkers) { - pass.Reportf(field.Pos(), - "%s uses plain string without +enum marker. Enumerated fields should use a type alias with +enum marker", - prefix) - } +func (a *analyzer) checkPlainStringField(pass *analysis.Pass, field *ast.Field, markersAccess markershelper.Markers, prefix string) { + if !hasEnumMarker(markersAccess.FieldMarkers(field)) { + pass.Reportf(field.Pos(), + "%s uses plain string without +enum marker. Enumerated fields should use a type alias with +enum marker", + prefix) + } +} + +func (a *analyzer) checkTypeAliasField(pass *analysis.Pass, field *ast.Field, ident *ast.Ident, markersAccess markershelper.Markers, prefix string) { + if utils.IsBasicType(pass, ident) { return } - // If it's a type alias, check that the alias has an enum marker - if !utils.IsBasicType(pass, ident) { - typeSpec, ok := utils.LookupTypeSpec(pass, ident) - if !ok { - return - } - // Check if the underlying type is string - if !isStringTypeAlias(pass, typeSpec) { - return - } - // Check if the type has an enum marker - typeMarkers := markersAccess.TypeMarkers(typeSpec) - if !hasEnumMarker(typeMarkers) { - pass.Reportf(field.Pos(), - "%s uses type %s which appears to be an enum but is missing +enum marker (kubebuilder:validation:Enum)", - prefix, typeSpec.Name.Name) - } + typeSpec, ok := utils.LookupTypeSpec(pass, ident) + if !ok || !isStringTypeAlias(pass, typeSpec) { + return + } + if !hasEnumMarker(markersAccess.TypeMarkers(typeSpec)) { + pass.Reportf(field.Pos(), + "%s uses type %s which appears to be an enum but is missing +enum marker (kubebuilder:validation:Enum)", + prefix, typeSpec.Name.Name) } } @@ -132,8 +125,6 @@ func (a *analyzer) checkTypeSpec(pass *analysis.Pass, typeSpec *ast.TypeSpec, ma if !hasEnumMarker(typeMarkers) { return } - - // Has +enum marker, verify it's a string type if !isStringTypeAlias(pass, typeSpec) { pass.Reportf(typeSpec.Pos(), "type %s has +enum marker but underlying type is not string", @@ -142,8 +133,6 @@ func (a *analyzer) checkTypeSpec(pass *analysis.Pass, typeSpec *ast.TypeSpec, ma } func (a *analyzer) checkConstValues(pass *analysis.Pass, inspect inspector.Inspector) { - // We need to check const declarations, but the inspector helper doesn't - // have a method for this, so we'll iterate through files manually for _, file := range pass.Files { for _, decl := range file.Decls { genDecl, ok := decl.(*ast.GenDecl) @@ -151,11 +140,9 @@ func (a *analyzer) checkConstValues(pass *analysis.Pass, inspect inspector.Inspe continue } for _, spec := range genDecl.Specs { - valueSpec, ok := spec.(*ast.ValueSpec) - if !ok { - continue + if valueSpec, ok := spec.(*ast.ValueSpec); ok { + a.checkConstSpec(pass, valueSpec) } - a.checkConstSpec(pass, valueSpec) } } } @@ -163,60 +150,47 @@ func (a *analyzer) checkConstValues(pass *analysis.Pass, inspect inspector.Inspe func (a *analyzer) checkConstSpec(pass *analysis.Pass, valueSpec *ast.ValueSpec) { for i, name := range valueSpec.Names { - if name == nil { - continue - } - // Get the type of the constant - obj := pass.TypesInfo.ObjectOf(name) - if obj == nil { - continue - } - constObj, ok := obj.(*types.Const) - if !ok { - continue - } - // Check if the type is a named type (potential enum) - namedType, ok := constObj.Type().(*types.Named) - if !ok { - continue - } - // Check if the type is in the current package - if namedType.Obj().Pkg() == nil || namedType.Obj().Pkg() != pass.Pkg { - continue - } - // Find the type spec for this named type - typeSpec := findTypeSpecByName(pass, namedType.Obj().Name()) - if typeSpec == nil { - continue - } - // Check if this type has an enum marker - if !hasEnumMarkerOnTypeSpec(pass, typeSpec) { - continue - } - // This is an enum constant, validate the value - if i >= len(valueSpec.Values) { - continue - } - value := valueSpec.Values[i] - basicLit, ok := value.(*ast.BasicLit) - if !ok { - continue - } - // Extract the string value (remove quotes) - strValue := strings.Trim(basicLit.Value, `"`) - // Check if it's in the allowlist - if a.isInAllowlist(strValue) { - continue - } - // Validate PascalCase - if !isPascalCase(strValue) { - pass.Reportf(basicLit.Pos(), - "enum value %q should be PascalCase (e.g., \"PhasePending\", \"StateRunning\")", - strValue) - } + a.validateEnumConstant(pass, name, valueSpec, i) + } +} + +func (a *analyzer) validateEnumConstant(pass *analysis.Pass, name *ast.Ident, valueSpec *ast.ValueSpec, index int) { + if name == nil || index >= len(valueSpec.Values) { + return + } + typeSpec := a.getEnumTypeSpec(pass, name) + if typeSpec == nil { + return + } + // Extract and validate the enum value + basicLit, ok := valueSpec.Values[index].(*ast.BasicLit) + if !ok { + return + } + strValue := strings.Trim(basicLit.Value, `"`) + if !a.isInAllowlist(strValue) && !isPascalCase(strValue) { + pass.Reportf(basicLit.Pos(), + "enum value %q should be PascalCase (e.g., \"PhasePending\", \"StateRunning\")", + strValue) } } +func (a *analyzer) getEnumTypeSpec(pass *analysis.Pass, name *ast.Ident) *ast.TypeSpec { + constObj, ok := pass.TypesInfo.ObjectOf(name).(*types.Const) + if !ok { + return nil + } + namedType, ok := constObj.Type().(*types.Named) + if !ok || namedType.Obj().Pkg() == nil || namedType.Obj().Pkg() != pass.Pkg { + return nil + } + typeSpec := findTypeSpecByName(pass, namedType.Obj().Name()) + if typeSpec == nil || !hasEnumMarkerOnTypeSpec(pass, typeSpec) { + return nil + } + return typeSpec +} + // unwrapType removes pointer and array wrappers to get the underlying type func unwrapType(expr ast.Expr) ast.Expr { switch t := expr.(type) { @@ -246,7 +220,6 @@ func unwrapTypeWithArrayTracking(expr ast.Expr) (ast.Expr, bool) { } } -// isStringTypeAlias checks if a type spec is an alias for string func isStringTypeAlias(pass *analysis.Pass, typeSpec *ast.TypeSpec) bool { underlyingType := unwrapType(typeSpec.Type) ident, ok := underlyingType.(*ast.Ident) @@ -256,34 +229,44 @@ func isStringTypeAlias(pass *analysis.Pass, typeSpec *ast.TypeSpec) bool { return ident.Name == "string" && utils.IsBasicType(pass, ident) } -// hasEnumMarker checks if a marker set contains an enum marker func hasEnumMarker(markerSet markershelper.MarkerSet) bool { return markerSet.Has(markers.KubebuilderEnumMarker) || markerSet.Has(markers.K8sEnumMarker) } -// hasEnumMarkerOnTypeSpec checks if a type spec has an enum marker by checking its doc comments func hasEnumMarkerOnTypeSpec(pass *analysis.Pass, typeSpec *ast.TypeSpec) bool { for _, file := range pass.Files { - for _, decl := range file.Decls { - genDecl, ok := decl.(*ast.GenDecl) - if !ok { - continue - } - for _, spec := range genDecl.Specs { - if spec == typeSpec { - if genDecl.Doc != nil { - for _, comment := range genDecl.Doc.List { - if strings.Contains(comment.Text, markers.KubebuilderEnumMarker) || - strings.Contains(comment.Text, markers.K8sEnumMarker) { - return true - } - } - } - return false - } + if genDecl := findGenDeclForSpec(file, typeSpec); genDecl != nil { + return hasEnumMarkerInDoc(genDecl.Doc) + } + } + return false +} + +func findGenDeclForSpec(file *ast.File, typeSpec *ast.TypeSpec) *ast.GenDecl { + for _, decl := range file.Decls { + genDecl, ok := decl.(*ast.GenDecl) + if !ok { + continue + } + for _, spec := range genDecl.Specs { + if spec == typeSpec { + return genDecl } } } + return nil +} + +func hasEnumMarkerInDoc(doc *ast.CommentGroup) bool { + if doc == nil { + return false + } + for _, comment := range doc.List { + text := comment.Text + if strings.Contains(text, markers.KubebuilderEnumMarker) || strings.Contains(text, markers.K8sEnumMarker) { + return true + } + } return false } @@ -300,7 +283,6 @@ func (a *analyzer) isInAllowlist(value string) bool { return false } -// findTypeSpecByName searches through the AST files to find a TypeSpec by name func findTypeSpecByName(pass *analysis.Pass, typeName string) *ast.TypeSpec { for _, file := range pass.Files { for _, decl := range file.Decls { @@ -322,36 +304,24 @@ func findTypeSpecByName(pass *analysis.Pass, typeName string) *ast.TypeSpec { return nil } -// isPascalCase validates that a string follows PascalCase convention -// PascalCase: FirstLetterUpperCase, no underscores, no hyphens func isPascalCase(s string) bool { if len(s) == 0 { return false } - // First character must be uppercase if !unicode.IsUpper(rune(s[0])) { return false } - // Check for invalid characters (underscores, hyphens) + hasLower := false for _, r := range s { if r == '_' || r == '-' { return false } - // Only allow letters and digits if !unicode.IsLetter(r) && !unicode.IsDigit(r) { return false } - } - // Should not be all uppercase (that's SCREAMING_SNAKE_CASE or similar) - allUpper := true - for _, r := range s[1:] { if unicode.IsLower(r) { - allUpper = false - break + hasLower = true } } - if allUpper && len(s) > 1 { - return false - } - return true + return len(s) == 1 || hasLower } From 6f6da92206169d79ab15f85e4936de9bf7c7d24e Mon Sep 17 00:00:00 2001 From: Krish Agarwal Date: Thu, 6 Nov 2025 16:29:54 -0500 Subject: [PATCH 3/5] Updated lint failures --- pkg/analysis/enums/analyzer.go | 61 +++++++++++++++++++++++------ pkg/analysis/enums/analyzer_test.go | 1 + pkg/analysis/enums/initializer.go | 1 + 3 files changed, 52 insertions(+), 11 deletions(-) diff --git a/pkg/analysis/enums/analyzer.go b/pkg/analysis/enums/analyzer.go index 29395374..4bf9c530 100644 --- a/pkg/analysis/enums/analyzer.go +++ b/pkg/analysis/enums/analyzer.go @@ -19,6 +19,7 @@ import ( "fmt" "go/ast" "go/types" + "slices" "strings" "unicode" @@ -41,6 +42,7 @@ type analyzer struct { func newAnalyzer(cfg *Config) *analysis.Analyzer { a := &analyzer{config: cfg} + return &analysis.Analyzer{ Name: name, Doc: "Enforces that enumerated fields use type aliases with +enum marker and have PascalCase values", @@ -56,7 +58,7 @@ func (a *analyzer) run(pass *analysis.Pass) (any, error) { } // Check struct fields for proper enum usage - inspect.InspectFields(func(field *ast.Field, _ extractjsontags.FieldTagInfo, markersAccess markershelper.Markers) { + inspect.InspectFields(func(field *ast.Field, _ extractjsontags.FieldTagInfo, markersAccess markershelper.Markers, _ string) { a.checkField(pass, field, markersAccess) }) @@ -64,7 +66,8 @@ func (a *analyzer) run(pass *analysis.Pass) (any, error) { inspect.InspectTypeSpec(func(typeSpec *ast.TypeSpec, markersAccess markershelper.Markers) { a.checkTypeSpec(pass, typeSpec, markersAccess) }) - a.checkConstValues(pass, inspect) + a.checkConstValues(pass) + return nil, nil //nolint:nilnil } @@ -75,15 +78,21 @@ func (a *analyzer) checkField(pass *analysis.Pass, field *ast.Field, markersAcce } // Get the underlying type, unwrapping pointers and arrays fieldType, isArray := unwrapTypeWithArrayTracking(field.Type) + ident, ok := fieldType.(*ast.Ident) + if !ok { return } + prefix := buildFieldPrefix(fieldName, isArray) + if ident.Name == "string" && utils.IsBasicType(pass, ident) { a.checkPlainStringField(pass, field, markersAccess, prefix) + return } + a.checkTypeAliasField(pass, field, ident, markersAccess, prefix) } @@ -91,6 +100,7 @@ func buildFieldPrefix(fieldName string, isArray bool) string { if isArray { return fmt.Sprintf("field %s array element", fieldName) } + return fmt.Sprintf("field %s", fieldName) } @@ -106,10 +116,13 @@ func (a *analyzer) checkTypeAliasField(pass *analysis.Pass, field *ast.Field, id if utils.IsBasicType(pass, ident) { return } + typeSpec, ok := utils.LookupTypeSpec(pass, ident) + if !ok || !isStringTypeAlias(pass, typeSpec) { return } + if !hasEnumMarker(markersAccess.TypeMarkers(typeSpec)) { pass.Reportf(field.Pos(), "%s uses type %s which appears to be an enum but is missing +enum marker (kubebuilder:validation:Enum)", @@ -121,10 +134,13 @@ func (a *analyzer) checkTypeSpec(pass *analysis.Pass, typeSpec *ast.TypeSpec, ma if typeSpec.Name == nil { return } + typeMarkers := markersAccess.TypeMarkers(typeSpec) + if !hasEnumMarker(typeMarkers) { return } + if !isStringTypeAlias(pass, typeSpec) { pass.Reportf(typeSpec.Pos(), "type %s has +enum marker but underlying type is not string", @@ -132,13 +148,14 @@ func (a *analyzer) checkTypeSpec(pass *analysis.Pass, typeSpec *ast.TypeSpec, ma } } -func (a *analyzer) checkConstValues(pass *analysis.Pass, inspect inspector.Inspector) { +func (a *analyzer) checkConstValues(pass *analysis.Pass) { for _, file := range pass.Files { for _, decl := range file.Decls { genDecl, ok := decl.(*ast.GenDecl) if !ok || genDecl.Tok.String() != "const" { continue } + for _, spec := range genDecl.Specs { if valueSpec, ok := spec.(*ast.ValueSpec); ok { a.checkConstSpec(pass, valueSpec) @@ -158,16 +175,20 @@ func (a *analyzer) validateEnumConstant(pass *analysis.Pass, name *ast.Ident, va if name == nil || index >= len(valueSpec.Values) { return } + typeSpec := a.getEnumTypeSpec(pass, name) if typeSpec == nil { return } + // Extract and validate the enum value basicLit, ok := valueSpec.Values[index].(*ast.BasicLit) if !ok { return } + strValue := strings.Trim(basicLit.Value, `"`) + if !a.isInAllowlist(strValue) && !isPascalCase(strValue) { pass.Reportf(basicLit.Pos(), "enum value %q should be PascalCase (e.g., \"PhasePending\", \"StateRunning\")", @@ -180,18 +201,22 @@ func (a *analyzer) getEnumTypeSpec(pass *analysis.Pass, name *ast.Ident) *ast.Ty if !ok { return nil } + namedType, ok := constObj.Type().(*types.Named) if !ok || namedType.Obj().Pkg() == nil || namedType.Obj().Pkg() != pass.Pkg { return nil } + typeSpec := findTypeSpecByName(pass, namedType.Obj().Name()) + if typeSpec == nil || !hasEnumMarkerOnTypeSpec(pass, typeSpec) { return nil } + return typeSpec } -// unwrapType removes pointer and array wrappers to get the underlying type +// unwrapType removes pointer and array wrappers to get the underlying type. func unwrapType(expr ast.Expr) ast.Expr { switch t := expr.(type) { case *ast.StarExpr: @@ -207,6 +232,7 @@ func unwrapType(expr ast.Expr) ast.Expr { // and tracks whether an array was encountered during unwrapping. func unwrapTypeWithArrayTracking(expr ast.Expr) (ast.Expr, bool) { isArray := false + for { switch t := expr.(type) { case *ast.StarExpr: @@ -222,10 +248,13 @@ func unwrapTypeWithArrayTracking(expr ast.Expr) (ast.Expr, bool) { func isStringTypeAlias(pass *analysis.Pass, typeSpec *ast.TypeSpec) bool { underlyingType := unwrapType(typeSpec.Type) + ident, ok := underlyingType.(*ast.Ident) + if !ok { return false } + return ident.Name == "string" && utils.IsBasicType(pass, ident) } @@ -239,6 +268,7 @@ func hasEnumMarkerOnTypeSpec(pass *analysis.Pass, typeSpec *ast.TypeSpec) bool { return hasEnumMarkerInDoc(genDecl.Doc) } } + return false } @@ -248,12 +278,14 @@ func findGenDeclForSpec(file *ast.File, typeSpec *ast.TypeSpec) *ast.GenDecl { if !ok { continue } + for _, spec := range genDecl.Specs { if spec == typeSpec { return genDecl } } } + return nil } @@ -261,26 +293,24 @@ func hasEnumMarkerInDoc(doc *ast.CommentGroup) bool { if doc == nil { return false } + for _, comment := range doc.List { text := comment.Text if strings.Contains(text, markers.KubebuilderEnumMarker) || strings.Contains(text, markers.K8sEnumMarker) { return true } } + return false } -// isInAllowlist checks if a value is in the configured allowlist +// isInAllowlist checks if a value is in the configured allowlist. func (a *analyzer) isInAllowlist(value string) bool { if a.config == nil { return false } - for _, allowed := range a.config.Allowlist { - if value == allowed { - return true - } - } - return false + + return slices.Contains(a.config.Allowlist, value) } func findTypeSpecByName(pass *analysis.Pass, typeName string) *ast.TypeSpec { @@ -290,17 +320,20 @@ func findTypeSpecByName(pass *analysis.Pass, typeName string) *ast.TypeSpec { if !ok { continue } + for _, spec := range genDecl.Specs { typeSpec, ok := spec.(*ast.TypeSpec) if !ok { continue } + if typeSpec.Name != nil && typeSpec.Name.Name == typeName { return typeSpec } } } } + return nil } @@ -308,20 +341,26 @@ func isPascalCase(s string) bool { if len(s) == 0 { return false } + if !unicode.IsUpper(rune(s[0])) { return false } + hasLower := false + for _, r := range s { if r == '_' || r == '-' { return false } + if !unicode.IsLetter(r) && !unicode.IsDigit(r) { return false } + if unicode.IsLower(r) { hasLower = true } } + return len(s) == 1 || hasLower } diff --git a/pkg/analysis/enums/analyzer_test.go b/pkg/analysis/enums/analyzer_test.go index 14ffdd88..d1c03cf3 100644 --- a/pkg/analysis/enums/analyzer_test.go +++ b/pkg/analysis/enums/analyzer_test.go @@ -28,6 +28,7 @@ func TestAnalyzer(t *testing.T) { // Test without allowlist config := &enums.Config{} analyzer, err := enums.Initializer().Init(config) + if err != nil { t.Fatalf("initializing enums linter: %v", err) } diff --git a/pkg/analysis/enums/initializer.go b/pkg/analysis/enums/initializer.go index 353d4862..2102531c 100644 --- a/pkg/analysis/enums/initializer.go +++ b/pkg/analysis/enums/initializer.go @@ -43,6 +43,7 @@ func initAnalyzer(cfg *Config) (*analysis.Analyzer, error) { if cfg == nil { cfg = &Config{} } + return newAnalyzer(cfg), nil } From 4822af78eea3288bd1e3dc930e962727e3f97f3c Mon Sep 17 00:00:00 2001 From: Krish Agarwal Date: Thu, 6 Nov 2025 22:53:53 -0500 Subject: [PATCH 4/5] Added/removed extra comments, moved helper functions at the bottom of the file --- docs/linters.md | 13 --------- pkg/analysis/enums/analyzer.go | 50 ++++++++++++++++++++-------------- 2 files changed, 30 insertions(+), 33 deletions(-) diff --git a/docs/linters.md b/docs/linters.md index 149ce8b2..3237ca0a 100644 --- a/docs/linters.md +++ b/docs/linters.md @@ -244,19 +244,6 @@ This provides better API evolution, self-documentation, and validation compared By default, `enums` is not enabled. -### Configuration - -```yaml -linterConfig: - enums: - allowlist: - - kubectl - - docker - - helm -``` - -The `allowlist` field contains enum values that should be exempt from PascalCase validation, such as command-line executable names. - ## ForbiddenMarkers The `forbiddenmarkers` linter ensures that types and fields do not contain any markers that are forbidden. diff --git a/pkg/analysis/enums/analyzer.go b/pkg/analysis/enums/analyzer.go index 4bf9c530..f69f94f4 100644 --- a/pkg/analysis/enums/analyzer.go +++ b/pkg/analysis/enums/analyzer.go @@ -32,9 +32,7 @@ import ( "sigs.k8s.io/kube-api-linter/pkg/markers" ) -const ( - name = "enums" -) +const name = "enums" type analyzer struct { config *Config @@ -76,6 +74,7 @@ func (a *analyzer) checkField(pass *analysis.Pass, field *ast.Field, markersAcce if fieldName == "" { return } + // Get the underlying type, unwrapping pointers and arrays fieldType, isArray := unwrapTypeWithArrayTracking(field.Type) @@ -87,23 +86,17 @@ func (a *analyzer) checkField(pass *analysis.Pass, field *ast.Field, markersAcce prefix := buildFieldPrefix(fieldName, isArray) + // Check if it's a plain string (basic type) vs. a type alias if ident.Name == "string" && utils.IsBasicType(pass, ident) { a.checkPlainStringField(pass, field, markersAccess, prefix) return } + // Check if it's a type alias that might be an enum a.checkTypeAliasField(pass, field, ident, markersAccess, prefix) } -func buildFieldPrefix(fieldName string, isArray bool) string { - if isArray { - return fmt.Sprintf("field %s array element", fieldName) - } - - return fmt.Sprintf("field %s", fieldName) -} - func (a *analyzer) checkPlainStringField(pass *analysis.Pass, field *ast.Field, markersAccess markershelper.Markers, prefix string) { if !hasEnumMarker(markersAccess.FieldMarkers(field)) { pass.Reportf(field.Pos(), @@ -216,6 +209,25 @@ func (a *analyzer) getEnumTypeSpec(pass *analysis.Pass, name *ast.Ident) *ast.Ty return typeSpec } +func (a *analyzer) isInAllowlist(value string) bool { + if a.config == nil { + return false + } + + return slices.Contains(a.config.Allowlist, value) +} + +// Helper functions below this line. + +// buildFieldPrefix constructs a human-readable prefix for error messages. +func buildFieldPrefix(fieldName string, isArray bool) string { + if isArray { + return fmt.Sprintf("field %s array element", fieldName) + } + + return fmt.Sprintf("field %s", fieldName) +} + // unwrapType removes pointer and array wrappers to get the underlying type. func unwrapType(expr ast.Expr) ast.Expr { switch t := expr.(type) { @@ -246,6 +258,7 @@ func unwrapTypeWithArrayTracking(expr ast.Expr) (ast.Expr, bool) { } } +// isStringTypeAlias checks if a type spec is a string type alias. func isStringTypeAlias(pass *analysis.Pass, typeSpec *ast.TypeSpec) bool { underlyingType := unwrapType(typeSpec.Type) @@ -258,10 +271,12 @@ func isStringTypeAlias(pass *analysis.Pass, typeSpec *ast.TypeSpec) bool { return ident.Name == "string" && utils.IsBasicType(pass, ident) } +// hasEnumMarker checks if a marker set contains enum markers. func hasEnumMarker(markerSet markershelper.MarkerSet) bool { return markerSet.Has(markers.KubebuilderEnumMarker) || markerSet.Has(markers.K8sEnumMarker) } +// hasEnumMarkerOnTypeSpec checks if a type spec has enum markers in its documentation. func hasEnumMarkerOnTypeSpec(pass *analysis.Pass, typeSpec *ast.TypeSpec) bool { for _, file := range pass.Files { if genDecl := findGenDeclForSpec(file, typeSpec); genDecl != nil { @@ -272,6 +287,7 @@ func hasEnumMarkerOnTypeSpec(pass *analysis.Pass, typeSpec *ast.TypeSpec) bool { return false } +// findGenDeclForSpec finds the GenDecl that contains a given TypeSpec. func findGenDeclForSpec(file *ast.File, typeSpec *ast.TypeSpec) *ast.GenDecl { for _, decl := range file.Decls { genDecl, ok := decl.(*ast.GenDecl) @@ -289,6 +305,7 @@ func findGenDeclForSpec(file *ast.File, typeSpec *ast.TypeSpec) *ast.GenDecl { return nil } +// hasEnumMarkerInDoc checks if a comment group contains enum markers. func hasEnumMarkerInDoc(doc *ast.CommentGroup) bool { if doc == nil { return false @@ -304,15 +321,7 @@ func hasEnumMarkerInDoc(doc *ast.CommentGroup) bool { return false } -// isInAllowlist checks if a value is in the configured allowlist. -func (a *analyzer) isInAllowlist(value string) bool { - if a.config == nil { - return false - } - - return slices.Contains(a.config.Allowlist, value) -} - +// findTypeSpecByName finds a type spec by its name in the pass's files. func findTypeSpecByName(pass *analysis.Pass, typeName string) *ast.TypeSpec { for _, file := range pass.Files { for _, decl := range file.Decls { @@ -337,6 +346,7 @@ func findTypeSpecByName(pass *analysis.Pass, typeName string) *ast.TypeSpec { return nil } +// isPascalCase checks if a string follows PascalCase naming convention. func isPascalCase(s string) bool { if len(s) == 0 { return false From f3cc2835c00f12b91c4d015f08c9bc09f7fb638b Mon Sep 17 00:00:00 2001 From: Krish Agarwal Date: Tue, 11 Nov 2025 14:52:00 -0500 Subject: [PATCH 5/5] Declarative and CRD validation markers, default=true, updated docs, and other reviews --- docs/linters.md | 30 +++- pkg/analysis/enums/analyzer.go | 164 ++++++++++++------ pkg/analysis/enums/analyzer_test.go | 6 +- pkg/analysis/enums/config.go | 5 + pkg/analysis/enums/doc.go | 88 +++++++--- pkg/analysis/enums/initializer.go | 5 +- pkg/analysis/enums/testdata/src/a/a.go | 85 +++++---- .../default_configurations/affinity.go | 12 +- .../default_configurations/container.go | 28 +-- .../testdata/default_configurations/pod.go | 2 + .../security_context.go | 5 +- .../testdata/default_configurations/volume.go | 14 +- 12 files changed, 299 insertions(+), 145 deletions(-) diff --git a/docs/linters.md b/docs/linters.md index 3237ca0a..58ed0775 100644 --- a/docs/linters.md +++ b/docs/linters.md @@ -238,11 +238,35 @@ If there are duplicates across fields and their underlying type, the marker on t ## Enums -The `enums` linter enforces that enumerated fields use type aliases with the `+enum` marker and that enum values follow PascalCase naming conventions. +The `enums` linter enforces that string type aliases used for enumerated values have proper enum markers. -This provides better API evolution, self-documentation, and validation compared to plain strings. +**Enum Marker Types:** +- **CRD Validation Marker** (`+kubebuilder:validation:Enum`): Used for CRD validation in projects with CustomResourceDefinitions. Processed by controller-gen to generate OpenAPI schema validation. +- **Declarative Validation Marker** (`+k8s:enum`): Used in Kubernetes core API types for declarative validation. -By default, `enums` is not enabled. +**Enum Marker Modes:** +- **Auto-discovery** (`+k8s:enum` or `+kubebuilder:validation:Enum`): Validates that constants follow PascalCase +- **Explicit values** (`+kubebuilder:validation:Enum=Value1;Value2`): Skips constant validation + +**PascalCase allows:** "Pending", "PhaseRunning", "HTTP", "IPv4" (acronyms and digits). +**PascalCase rejects:** "pending", "phase_pending", "Phase-Failed". + +**Note:** The linter only flags string type aliases that have associated constants (indicating enum usage), avoiding false positives for generic string types. + +By default, `enums` is enabled. + +### Configuration + +```yaml +linterConfig: + enums: + # Values exempt from PascalCase validation + allowlist: + - kubectl + - docker + # When true, flags plain string fields (default: false) + requireTypeAliasForEnums: false +``` ## ForbiddenMarkers diff --git a/pkg/analysis/enums/analyzer.go b/pkg/analysis/enums/analyzer.go index f69f94f4..c9d9d58e 100644 --- a/pkg/analysis/enums/analyzer.go +++ b/pkg/analysis/enums/analyzer.go @@ -34,6 +34,14 @@ import ( const name = "enums" +func init() { + // Register enum markers so they can be parsed + markershelper.DefaultRegistry().Register( + markers.KubebuilderEnumMarker, + markers.K8sEnumMarker, + ) +} + type analyzer struct { config *Config } @@ -43,7 +51,7 @@ func newAnalyzer(cfg *Config) *analysis.Analyzer { return &analysis.Analyzer{ Name: name, - Doc: "Enforces that enumerated fields use type aliases with +enum marker and have PascalCase values", + Doc: "Enforces that string type aliases with constants have enum markers (+kubebuilder:validation:Enum for CRDs, +k8s:enum for core APIs) and that enum values follow PascalCase conventions", Run: a.run, Requires: []*analysis.Analyzer{inspector.Analyzer}, } @@ -56,51 +64,49 @@ func (a *analyzer) run(pass *analysis.Pass) (any, error) { } // Check struct fields for proper enum usage - inspect.InspectFields(func(field *ast.Field, _ extractjsontags.FieldTagInfo, markersAccess markershelper.Markers, _ string) { - a.checkField(pass, field, markersAccess) + inspect.InspectFields(func(field *ast.Field, _ extractjsontags.FieldTagInfo, markersAccess markershelper.Markers, qualifiedFieldName string) { + a.checkField(pass, field, markersAccess, qualifiedFieldName) }) - // Check type declarations for +enum markers + // Check type declarations for enum markers inspect.InspectTypeSpec(func(typeSpec *ast.TypeSpec, markersAccess markershelper.Markers) { a.checkTypeSpec(pass, typeSpec, markersAccess) }) + a.checkConstValues(pass) return nil, nil //nolint:nilnil } -func (a *analyzer) checkField(pass *analysis.Pass, field *ast.Field, markersAccess markershelper.Markers) { - fieldName := utils.FieldName(field) - if fieldName == "" { +func (a *analyzer) checkField(pass *analysis.Pass, field *ast.Field, markersAccess markershelper.Markers, qualifiedFieldName string) { + if qualifiedFieldName == "" { return } - // Get the underlying type, unwrapping pointers and arrays fieldType, isArray := unwrapTypeWithArrayTracking(field.Type) - ident, ok := fieldType.(*ast.Ident) if !ok { return } - prefix := buildFieldPrefix(fieldName, isArray) + prefix := buildFieldPrefix(qualifiedFieldName, isArray) - // Check if it's a plain string (basic type) vs. a type alias if ident.Name == "string" && utils.IsBasicType(pass, ident) { - a.checkPlainStringField(pass, field, markersAccess, prefix) + if a.config != nil && a.config.RequireTypeAliasForEnums { + a.checkPlainStringField(pass, field, markersAccess, prefix) + } return } - // Check if it's a type alias that might be an enum a.checkTypeAliasField(pass, field, ident, markersAccess, prefix) } func (a *analyzer) checkPlainStringField(pass *analysis.Pass, field *ast.Field, markersAccess markershelper.Markers, prefix string) { if !hasEnumMarker(markersAccess.FieldMarkers(field)) { pass.Reportf(field.Pos(), - "%s uses plain string without +enum marker. Enumerated fields should use a type alias with +enum marker", + "%s uses plain string type. Consider using a type alias with an enum marker (+kubebuilder:validation:Enum or +k8s:enum)", prefix) } } @@ -111,14 +117,17 @@ func (a *analyzer) checkTypeAliasField(pass *analysis.Pass, field *ast.Field, id } typeSpec, ok := utils.LookupTypeSpec(pass, ident) - if !ok || !isStringTypeAlias(pass, typeSpec) { return } - + // Only check if this type has constants (indicating enum usage) + if !typeHasConstants(pass, typeSpec.Name.Name) { + return + } + // Check for enum markers (CRD validation or declarative validation) if !hasEnumMarker(markersAccess.TypeMarkers(typeSpec)) { pass.Reportf(field.Pos(), - "%s uses type %s which appears to be an enum but is missing +enum marker (kubebuilder:validation:Enum)", + "%s uses type %s which appears to be an enum but is missing an enum marker (+kubebuilder:validation:Enum or +k8s:enum)", prefix, typeSpec.Name.Name) } } @@ -136,7 +145,7 @@ func (a *analyzer) checkTypeSpec(pass *analysis.Pass, typeSpec *ast.TypeSpec, ma if !isStringTypeAlias(pass, typeSpec) { pass.Reportf(typeSpec.Pos(), - "type %s has +enum marker but underlying type is not string", + "type %s has enum marker but underlying type is not string", typeSpec.Name.Name) } } @@ -196,13 +205,13 @@ func (a *analyzer) getEnumTypeSpec(pass *analysis.Pass, name *ast.Ident) *ast.Ty } namedType, ok := constObj.Type().(*types.Named) + if !ok || namedType.Obj().Pkg() == nil || namedType.Obj().Pkg() != pass.Pkg { return nil } typeSpec := findTypeSpecByName(pass, namedType.Obj().Name()) - - if typeSpec == nil || !hasEnumMarkerOnTypeSpec(pass, typeSpec) { + if typeSpec == nil || !usesAutoDiscovery(pass, typeSpec) { return nil } @@ -261,33 +270,70 @@ func unwrapTypeWithArrayTracking(expr ast.Expr) (ast.Expr, bool) { // isStringTypeAlias checks if a type spec is a string type alias. func isStringTypeAlias(pass *analysis.Pass, typeSpec *ast.TypeSpec) bool { underlyingType := unwrapType(typeSpec.Type) - ident, ok := underlyingType.(*ast.Ident) if !ok { return false } + // Both checks are needed: name check is fast, IsBasicType handles edge cases + // where 'string' might be redefined (rare but possible) return ident.Name == "string" && utils.IsBasicType(pass, ident) } -// hasEnumMarker checks if a marker set contains enum markers. +// hasEnumMarker checks if a marker set contains enum markers +// (either CRD validation marker or declarative validation marker). func hasEnumMarker(markerSet markershelper.MarkerSet) bool { - return markerSet.Has(markers.KubebuilderEnumMarker) || markerSet.Has(markers.K8sEnumMarker) + return markerSet.Has(markers.KubebuilderEnumMarker) || + markerSet.Has(markers.K8sEnumMarker) } -// hasEnumMarkerOnTypeSpec checks if a type spec has enum markers in its documentation. -func hasEnumMarkerOnTypeSpec(pass *analysis.Pass, typeSpec *ast.TypeSpec) bool { +// usesAutoDiscovery checks if a type uses auto-discovery mode for enum values. +// Auto-discovery mode validates that constants follow PascalCase conventions. +// +// Returns true for: +// - +k8s:enum (declarative validation marker, always auto-discovers) +// - +kubebuilder:validation:Enum without explicit values (CRD validation marker in auto-discovery mode) +// +// Returns false for: +// - +kubebuilder:validation:Enum=value1;value2 (CRD validation marker with explicit values) +func usesAutoDiscovery(pass *analysis.Pass, typeSpec *ast.TypeSpec) bool { for _, file := range pass.Files { - if genDecl := findGenDeclForSpec(file, typeSpec); genDecl != nil { - return hasEnumMarkerInDoc(genDecl.Doc) + genDecl := findGenDeclForSpec(file, typeSpec) + if genDecl == nil || genDecl.Doc == nil { + continue + } + + for _, comment := range genDecl.Doc.List { + text := comment.Text + // Must be an actual marker (starts with "// +") + if !strings.HasPrefix(text, "// +") { + continue + } + + markerContent := strings.TrimPrefix(text, "// +") + + // +k8s:enum always uses auto-discovery + if strings.HasPrefix(markerContent, markers.K8sEnumMarker) { + return true + } + // +kubebuilder:validation:Enum without explicit values uses auto-discovery + if strings.HasPrefix(markerContent, markers.KubebuilderEnumMarker) { + afterMarker := markerContent[len(markers.KubebuilderEnumMarker):] + // If there's an "=" or ":=" immediately after, it has explicit values + trimmed := strings.TrimSpace(afterMarker) + if strings.HasPrefix(trimmed, "=") || strings.HasPrefix(trimmed, ":=") { + return false // Explicit values mode + } + + return true // Auto-discovery mode + } } } return false } -// findGenDeclForSpec finds the GenDecl that contains a given TypeSpec. func findGenDeclForSpec(file *ast.File, typeSpec *ast.TypeSpec) *ast.GenDecl { for _, decl := range file.Decls { genDecl, ok := decl.(*ast.GenDecl) @@ -305,22 +351,6 @@ func findGenDeclForSpec(file *ast.File, typeSpec *ast.TypeSpec) *ast.GenDecl { return nil } -// hasEnumMarkerInDoc checks if a comment group contains enum markers. -func hasEnumMarkerInDoc(doc *ast.CommentGroup) bool { - if doc == nil { - return false - } - - for _, comment := range doc.List { - text := comment.Text - if strings.Contains(text, markers.KubebuilderEnumMarker) || strings.Contains(text, markers.K8sEnumMarker) { - return true - } - } - - return false -} - // findTypeSpecByName finds a type spec by its name in the pass's files. func findTypeSpecByName(pass *analysis.Pass, typeName string) *ast.TypeSpec { for _, file := range pass.Files { @@ -346,31 +376,55 @@ func findTypeSpecByName(pass *analysis.Pass, typeName string) *ast.TypeSpec { return nil } +// typeHasConstants checks if any constants are defined for the given type name. +func typeHasConstants(pass *analysis.Pass, typeName string) bool { + for _, file := range pass.Files { + for _, decl := range file.Decls { + genDecl, ok := decl.(*ast.GenDecl) + + if !ok || genDecl.Tok.String() != "const" { + continue + } + + for _, spec := range genDecl.Specs { + valueSpec, ok := spec.(*ast.ValueSpec) + if !ok || valueSpec.Type == nil { + continue + } + // Check if the const has this type + if ident, ok := valueSpec.Type.(*ast.Ident); ok && ident.Name == typeName { + return true + } + } + } + } + + return false +} + // isPascalCase checks if a string follows PascalCase naming convention. +// Allows: PascalCase (Running), all-uppercase acronyms (HTTP, API), single letters (A). +// Rejects: lowercase start (running), snake_case (phase_pending), kebab-case (phase-pending). func isPascalCase(s string) bool { if len(s) == 0 { return false } - + // Must start with uppercase if !unicode.IsUpper(rune(s[0])) { return false } - hasLower := false - + // No underscores or hyphens allowed (these indicate snake_case/kebab-case) for _, r := range s { if r == '_' || r == '-' { return false } - - if !unicode.IsLetter(r) && !unicode.IsDigit(r) { + // Allow letters, digits, and "+" (for signal names like SIGRTMIN+1) + if !unicode.IsLetter(r) && !unicode.IsDigit(r) && r != '+' { return false } - - if unicode.IsLower(r) { - hasLower = true - } } - - return len(s) == 1 || hasLower + // Valid: starts with uppercase, no underscores/hyphens + // Accepts: PascalCase, HTTP, HTTPS, SIGRTMIN+1, etc. + return true } diff --git a/pkg/analysis/enums/analyzer_test.go b/pkg/analysis/enums/analyzer_test.go index d1c03cf3..6150cfbc 100644 --- a/pkg/analysis/enums/analyzer_test.go +++ b/pkg/analysis/enums/analyzer_test.go @@ -25,8 +25,10 @@ import ( func TestAnalyzer(t *testing.T) { testdata := analysistest.TestData() - // Test without allowlist - config := &enums.Config{} + // Test with default config (plain strings allowed) + config := &enums.Config{ + RequireTypeAliasForEnums: false, + } analyzer, err := enums.Initializer().Init(config) if err != nil { diff --git a/pkg/analysis/enums/config.go b/pkg/analysis/enums/config.go index 1a6bfac0..cb38407e 100644 --- a/pkg/analysis/enums/config.go +++ b/pkg/analysis/enums/config.go @@ -20,4 +20,9 @@ type Config struct { // Allowlist contains values that are exempt from PascalCase validation. // This is useful for command-line executable names like "kubectl", "docker", etc. Allowlist []string `yaml:"allowlist" json:"allowlist"` + + // RequireTypeAliasForEnums when true, enforces that string fields representing enums + // must use type aliases instead of plain string types. + // Default: false (plain strings are allowed) + RequireTypeAliasForEnums bool `yaml:"requireTypeAliasForEnums" json:"requireTypeAliasForEnums"` } diff --git a/pkg/analysis/enums/doc.go b/pkg/analysis/enums/doc.go index f3c48e95..72bc8770 100644 --- a/pkg/analysis/enums/doc.go +++ b/pkg/analysis/enums/doc.go @@ -23,13 +23,52 @@ provide better validation at the schema level. # Rules -The linter checks for three main patterns: -1. Fields must use type aliases, not plain strings: String fields that represent enums should -use a type alias with an +enum marker rather than a raw string type. -2. Type aliases must have +enum marker: Type aliases used for enumerated values must be -annotated with either // +kubebuilder:validation:Enum or // +k8s:enum. -3. Enum values must be PascalCase: Constant values for enums should follow PascalCase naming -(e.g., "PhasePending", "StateRunning") rather than lowercase, snake_case, or SCREAMING_SNAKE_CASE. +1. String type aliases that have associated constants must be annotated with an enum marker: + - +kubebuilder:validation:Enum for CRD validation (used in projects with CustomResourceDefinitions) + - +k8s:enum for declarative validation (used in Kubernetes core API types) + +2. Enum constant values must follow PascalCase when using auto-discovery mode. +Valid: "Pending", "Running", "HTTP", "HTTPS" (acronyms allowed). +Invalid: "pending", "phase_pending", "Phase-Failed" (snake_case/kebab-case). + +3. (Optional) When RequireTypeAliasForEnums is true: String fields without enum markers +should use type aliases instead of plain string types. + +The linter only flags type aliases that have constants defined, avoiding false positives +for generic string wrapper types. + +# Enum Marker Types + +CRD Validation Marker (+kubebuilder:validation:Enum): +- Used in projects with CustomResourceDefinitions +- Processed by controller-gen to generate OpenAPI schema validation +- Supports two modes: + - Auto-discovery: +kubebuilder:validation:Enum (validates constants as PascalCase) + - Explicit values: +kubebuilder:validation:Enum=Pending;Running;Failed (skips constant validation) + +Declarative Validation Marker (+k8s:enum): +- Used in Kubernetes core API types (in-tree APIs) +- Part of the k8s declarative validation system +- Always uses auto-discovery mode (validates constants as PascalCase) + +Examples: + +Auto-discovery (validates constants): + + // +k8s:enum ← Always auto-discovers constants + // +kubebuilder:validation:Enum ← Auto-discovers when no explicit values + type Phase string + const ( + PhasePending Phase = "Pending" ← These must be PascalCase + ) + +Explicit values (constants not validated): + + // +kubebuilder:validation:Enum=Pending;Running;Failed ← Uses these specific values + type Phase string + const ( + helper Phase = "helper" ← Not validated, marker values are used + ) # Examples @@ -38,9 +77,8 @@ Good: // +kubebuilder:validation:Enum type Phase string const ( - PhasePending Phase = "Pending" - PhaseRunning Phase = "Running" - PhaseSucceeded Phase = "Succeeded" + PhasePending Phase = "Pending" + PhaseRunning Phase = "Running" ) type MySpec struct { Phase Phase @@ -48,33 +86,33 @@ Good: Bad: - // Missing +enum marker + // String type alias with constants but missing enum marker type Phase string - type MySpec struct { - // Plain string without type alias - Phase string - } - // Values not PascalCase const ( phase_pending Phase = "pending" // Should be "Pending" - PHASE_RUNNING Phase = "RUNNING" // Should be "Running" + phase_running Phase = "phase_running" // Should be "PhaseRunning" + Phase_Failed Phase = "Phase-Failed" // Should be "PhaseFailed" ) +Note: Acronyms (HTTP, HTTPS, API) are allowed. The linter only flags type aliases with +constants, not all string types. + # Configuration -The linter supports an allowlist for enum values that should be exempt from PascalCase -validation, such as command-line executable names: +Configuration options: linterConfig: enums: + # Values exempt from PascalCase validation allowlist: - kubectl - docker - - helm + # Require type aliases for enum fields (default: false) + requireTypeAliasForEnums: false # Rationale -Using type aliases for enums instead of raw strings provides several benefits: +Using type aliases with enum markers instead of raw strings provides several benefits: - API schemas can explicitly list valid enum values - Better validation at both the schema and runtime level - Improved documentation and API evolution @@ -84,7 +122,11 @@ Using type aliases for enums instead of raw strings provides several benefits: The PascalCase convention for enum values aligns with Kubernetes API conventions and improves readability and consistency across the ecosystem. -Note: This linter is disabled by default as enum usage is recommended but not strictly -required for all Kubernetes APIs. Enable it in your configuration to enforce these conventions. +The distinction between CRD validation markers and declarative validation markers allows +the linter to work correctly in both CRD-based projects (using controller-gen) and +Kubernetes core API development (using declarative validation). + +Note: This linter is enabled by default to ensure string types with constants follow enum +conventions. It only flags types that have associated constants, minimizing false positives. */ package enums diff --git a/pkg/analysis/enums/initializer.go b/pkg/analysis/enums/initializer.go index 2102531c..2bd0e428 100644 --- a/pkg/analysis/enums/initializer.go +++ b/pkg/analysis/enums/initializer.go @@ -32,9 +32,8 @@ func Initializer() initializer.AnalyzerInitializer { return initializer.NewConfigurableInitializer( name, initAnalyzer, - // The enums linter is opt-in as enum usage is not strictly - // required for all Kubernetes APIs, though it is recommended. - false, + // Enabled by default: validates string type aliases with constants have enum markers + true, validateConfig, ) } diff --git a/pkg/analysis/enums/testdata/src/a/a.go b/pkg/analysis/enums/testdata/src/a/a.go index e1bfd571..cfc92584 100644 --- a/pkg/analysis/enums/testdata/src/a/a.go +++ b/pkg/analysis/enums/testdata/src/a/a.go @@ -1,7 +1,7 @@ package a -// Valid enum type with proper marker -// +kubebuilder:validation:Enum +// Valid: +kubebuilder:validation:Enum without explicit values (auto-discovers constants) +// +kubebuilder:validation:Enum=Pending;Running;Succeeded;Failed type Phase string const ( @@ -11,7 +11,7 @@ const ( PhaseFailed Phase = "Failed" // Valid PascalCase ) -// Alternative marker format +// Valid: +k8s:enum always auto-discovers constants // +k8s:enum type State string @@ -20,8 +20,17 @@ const ( StateInactive State = "Inactive" // Valid PascalCase ) -// Invalid: Missing +enum marker -// This type doesn't have +enum marker, so it will be flagged when used in fields +// Valid: +kubebuilder:validation:Enum with explicit values (doesn't check constants) +// +kubebuilder:validation:Enum=Pending;helper +type ExplicitPhase string + +// These constants are helpers - not validated because marker has explicit values +const ( + ExplicitPending ExplicitPhase = "Pending" + explicit_helper ExplicitPhase = "helper" // Not flagged - explicit values mode +) + +// Invalid: Type alias without enum marker (will be flagged when used in fields) type Status string const ( @@ -29,34 +38,42 @@ const ( StatusNotReady Status = "NotReady" ) -// Invalid: Type with +enum but not string +// Invalid: Type with kubebuilder:validation:Enum marker but underlying type is not string // +kubebuilder:validation:Enum -type InvalidEnumType int // want "type InvalidEnumType has \\+enum marker but underlying type is not string" +type InvalidEnumType int // want "type InvalidEnumType has enum marker but underlying type is not string" -// Invalid enum values (not PascalCase) +// Invalid enum values (not PascalCase) - using auto-discovery // +kubebuilder:validation:Enum type BadPhase string const ( - phase_pending BadPhase = "pending" // want "enum value \"pending\" should be PascalCase" - PHASE_RUNNING BadPhase = "RUNNING" // want "enum value \"RUNNING\" should be PascalCase" - phase_succeeded BadPhase = "succeeded" // want "enum value \"succeeded\" should be PascalCase" - Phase_Failed BadPhase = "Phase-Failed" // want "enum value \"Phase-Failed\" should be PascalCase" + phase_pending BadPhase = "pending" // want "enum value \"pending\" should be PascalCase" + phase_failed BadPhase = "Phase-Failed" // want "enum value \"Phase-Failed\" should be PascalCase" +) + +// Valid: Acronyms and all-caps are allowed +// +kubebuilder:validation:Enum +type Protocol string + +const ( + ProtocolHTTP Protocol = "HTTP" // Valid: acronym + ProtocolHTTPS Protocol = "HTTPS" // Valid: acronym + ProtocolTCP Protocol = "TCP" // Valid: acronym ) // Test struct with fields type MySpec struct { - // Valid: uses type alias with +enum + // Valid: uses type alias with enum marker Phase Phase - // Valid: uses type alias with +enum + // Valid: uses type alias with enum marker State State - // Invalid: plain string without +enum - PlainString string // want "field PlainString uses plain string without \\+enum marker" + // Valid: plain string (not required to be enum unless RequireTypeAliasForEnums is enabled) + PlainString string - // Invalid: type alias without +enum marker - Status Status // want "field Status uses type Status which appears to be an enum but is missing \\+enum marker" + // Invalid: type alias without enum marker + Status Status // want "field MySpec.Status uses type Status which appears to be an enum but is missing an enum marker \\(\\+kubebuilder:validation:Enum or \\+k8s:enum\\)" // Valid: pointer to enum type PhasePtr *Phase @@ -64,22 +81,23 @@ type MySpec struct { // Valid: slice of enum type Phases []Phase - // Invalid: plain string slice - PlainStrings []string // want "field PlainStrings array element uses plain string without \\+enum marker" + // Valid: plain string slice (not required to be enum) + PlainStrings []string + + // Valid: explicit enum type + Explicit ExplicitPhase } // Test pointer fields type PointerSpec struct { - PhasePtr *Phase - StatusPtr *Status // want "field StatusPtr uses type Status which appears to be an enum but is missing \\+enum marker" - PlainStrPtr *string // want "field PlainStrPtr uses plain string without \\+enum marker" + PhasePtr *Phase + StatusPtr *Status // want "field PointerSpec.StatusPtr uses type Status which appears to be an enum but is missing an enum marker \\(\\+kubebuilder:validation:Enum or \\+k8s:enum\\)" } // Test array fields type ArraySpec struct { - Phases []Phase - Statuses []Status // want "field Statuses array element uses type Status which appears to be an enum but is missing \\+enum marker" - PlainStrArray []string // want "field PlainStrArray array element uses plain string without \\+enum marker" + Phases []Phase + Statuses []Status // want "field ArraySpec.Statuses array element uses type Status which appears to be an enum but is missing an enum marker \\(\\+kubebuilder:validation:Enum or \\+k8s:enum\\)" } // Embedded field test @@ -88,14 +106,14 @@ type EmbeddedSpec struct { Phase Phase } -// Edge case: Field with +enum marker directly on the field (should be allowed as exception) +// Edge case: Field with enum marker directly on the field (allowed as exception) type DirectMarkerSpec struct { // +kubebuilder:validation:Enum - DirectEnum string // Valid: has +enum marker directly on field + DirectEnum string // Valid: has enum marker directly on field } // Edge case: Enum values with numbers (should be valid PascalCase) -// +kubebuilder:validation:Enum +// +kubebuilder:validation:Enum=Priority1;Priority2 type Priority string const ( @@ -103,13 +121,14 @@ const ( Priority2 Priority = "Priority2" // Valid: PascalCase with number ) -// Edge case: Single letter uppercase (edge case for all-caps check) -// +kubebuilder:validation:Enum +// Valid: Single letter and all-caps values +// +kubebuilder:validation:Enum=A;B;API type Level string const ( - LevelA Level = "A" // Valid: single uppercase letter - LevelB Level = "B" // Valid: single uppercase letter + LevelA Level = "A" // Valid: single letter + LevelB Level = "B" // Valid: single letter + LevelAPI Level = "API" // Valid: acronym ) // Edge case: Map with enum types (maps are allowed, not enforced to use enum types) diff --git a/tests/integration/testdata/default_configurations/affinity.go b/tests/integration/testdata/default_configurations/affinity.go index 64eb5417..a0820d4c 100644 --- a/tests/integration/testdata/default_configurations/affinity.go +++ b/tests/integration/testdata/default_configurations/affinity.go @@ -215,7 +215,7 @@ type Taint struct { } -// +enum +// +kubebuilder:validation:Enum type TaintEffect string const ( @@ -269,7 +269,7 @@ type Toleration struct { } // A toleration operator is the set of operators that can be used in a toleration. -// +enum +// +kubebuilder:validation:Enum type TolerationOperator string const ( @@ -322,7 +322,7 @@ type NodeSelectorRequirement struct { // A node selector operator is the set of operators that can be used in // a node selector requirement. -// +enum +// +kubebuilder:validation:Enum type NodeSelectorOperator string const ( @@ -454,7 +454,7 @@ type TopologySpreadConstraint struct { MatchLabelKeys []string `json:"matchLabelKeys,omitempty" protobuf:"bytes,8,opt,name=matchLabelKeys"` } -// +enum +// +kubebuilder:validation:Enum type UnsatisfiableConstraintAction string const ( @@ -467,7 +467,7 @@ const ( ) // NodeInclusionPolicy defines the type of node inclusion policy -// +enum +// +kubebuilder:validation:Enum type NodeInclusionPolicy string const ( @@ -478,7 +478,7 @@ const ( ) // PreemptionPolicy describes a policy for if/when to preempt a pod. -// +enum +// +kubebuilder:validation:Enum type PreemptionPolicy string const ( diff --git a/tests/integration/testdata/default_configurations/container.go b/tests/integration/testdata/default_configurations/container.go index 6d48cd7f..b0c47ace 100644 --- a/tests/integration/testdata/default_configurations/container.go +++ b/tests/integration/testdata/default_configurations/container.go @@ -209,7 +209,7 @@ type Container struct { } // Protocol defines network protocols supported for things like container ports. -// +enum +// +kubebuilder:validation:Enum type Protocol string const ( @@ -453,6 +453,7 @@ type ResourceClaim struct { } // ResourceResizeRestartPolicy specifies how to handle container resource resize. +// +kubebuilder:validation:Enum=NotRequired;RestartContainer type ResourceResizeRestartPolicy string // These are the valid resource resize restart policy values: @@ -484,6 +485,7 @@ type ContainerResizePolicy struct { // ContainerRestartPolicy is the restart policy for a single container. // The only allowed values are "Always", "Never", and "OnFailure". +// +kubebuilder:validation:Enum=Always;Never;OnFailure type ContainerRestartPolicy string const ( @@ -498,7 +500,7 @@ type ContainerRestartRule struct { // are satisfied. The only possible value is "Restart" to restart the // container. // +required - Action ContainerRestartRuleAction `json:"action,omitempty" proto:"bytes,1,opt,name=action" protobuf:"bytes,1,opt,name=action,casttype=ContainerRestartRuleAction"` // want "requiredfields: field Action has a valid zero value \\(\\\"\\\"\\), but the validation is not complete \\(e.g. minimum length\\). The field should be a pointer to allow the zero value to be set. If the zero value is not a valid use case, complete the validation and remove the pointer." + Action ContainerRestartRuleAction `json:"action,omitempty" proto:"bytes,1,opt,name=action" protobuf:"bytes,1,opt,name=action,casttype=ContainerRestartRuleAction"` // Represents the exit codes to check on container exits. // want "commentstart: godoc for field ContainerRestartRule.ExitCodes should start with 'exitCodes ...'" // +optional @@ -508,6 +510,7 @@ type ContainerRestartRule struct { // ContainerRestartRuleAction describes the action to take when the // container exits. +// +kubebuilder:validation:Enum=Restart type ContainerRestartRuleAction string // The only valid action is Restart. @@ -525,7 +528,7 @@ type ContainerRestartRuleOnExitCodes struct { // - NotIn: the requirement is satisfied if the container exit code is // not in the set of specified values. // +required - Operator ContainerRestartRuleOnExitCodesOperator `json:"operator,omitempty" proto:"bytes,1,opt,name=operator" protobuf:"bytes,1,opt,name=operator,casttype=ContainerRestartRuleOnExitCodesOperator"` // want "requiredfields: field Operator has a valid zero value \\(\\\"\\\"\\), but the validation is not complete \\(e.g. minimum length\\). The field should be a pointer to allow the zero value to be set. If the zero value is not a valid use case, complete the validation and remove the pointer." + Operator ContainerRestartRuleOnExitCodesOperator `json:"operator,omitempty" proto:"bytes,1,opt,name=operator" protobuf:"bytes,1,opt,name=operator,casttype=ContainerRestartRuleOnExitCodesOperator"` // Specifies the set of values to check for container exit codes. // want "commentstart: godoc for field ContainerRestartRuleOnExitCodes.Values should start with 'values ...'" // At most 255 elements are allowed. @@ -536,6 +539,7 @@ type ContainerRestartRuleOnExitCodes struct { // ContainerRestartRuleOnExitCodesOperator describes the operator // to take for the exit codes. +// +kubebuilder:validation:Enum type ContainerRestartRuleOnExitCodesOperator string const ( @@ -634,7 +638,7 @@ type HTTPHeader struct { } // URIScheme identifies the scheme used for connection to a host for Get actions -// +enum +// +kubebuilder:validation:Enum type URIScheme string const ( @@ -689,7 +693,7 @@ type SleepAction struct { } // Signal defines the stop signal of containers -// +enum +// +kubebuilder:validation:Enum=SIGABRT;SIGALRM;SIGBUS;SIGCHLD;SIGCLD;SIGCONT;SIGFPE;SIGHUP;SIGILL;SIGINT;SIGIO;SIGIOT;SIGKILL;SIGPIPE;SIGPOLL;SIGPROF;SIGPWR;SIGQUIT;SIGSEGV;SIGSTKFLT;SIGSTOP;SIGSYS;SIGTERM;SIGTRAP;SIGTSTP;SIGTTIN;SIGTTOU;SIGURG;SIGUSR1;SIGUSR2;SIGVTALRM;SIGWINCH;SIGXCPU;SIGXFSZ;SIGRTMIN;SIGRTMIN+1;SIGRTMIN+2;SIGRTMIN+3;SIGRTMIN+4;SIGRTMIN+5;SIGRTMIN+6;SIGRTMIN+7;SIGRTMIN+8;SIGRTMIN+9;SIGRTMIN+10;SIGRTMIN+11;SIGRTMIN+12;SIGRTMIN+13;SIGRTMIN+14;SIGRTMIN+15;SIGRTMAX-14;SIGRTMAX-13;SIGRTMAX-12;SIGRTMAX-11;SIGRTMAX-10;SIGRTMAX-9;SIGRTMAX-8;SIGRTMAX-7;SIGRTMAX-6;SIGRTMAX-5;SIGRTMAX-4;SIGRTMAX-3;SIGRTMAX-2;SIGRTMAX-1;SIGRTMAX type Signal string const ( @@ -808,7 +812,7 @@ type LifecycleHandler struct { } // TerminationMessagePolicy describes how termination messages are retrieved from a container. -// +enum +// +kubebuilder:validation:Enum type TerminationMessagePolicy string const ( @@ -822,7 +826,7 @@ const ( ) // PullPolicy describes a policy for if/when to pull a container image -// +enum +// +kubebuilder:validation:Enum type PullPolicy string const ( @@ -918,7 +922,7 @@ type SecurityContext struct { AppArmorProfile *AppArmorProfile `json:"appArmorProfile,omitempty" protobuf:"bytes,12,opt,name=appArmorProfile"` } -// +enum +// +kubebuilder:validation:Enum type ProcMountType string const ( @@ -1012,7 +1016,7 @@ type SeccompProfile struct { } // SeccompProfileType defines the supported seccomp profile types. -// +enum +// +kubebuilder:validation:Enum type SeccompProfileType string const ( @@ -1044,7 +1048,7 @@ type AppArmorProfile struct { LocalhostProfile *string `json:"localhostProfile,omitempty" protobuf:"bytes,2,opt,name=localhostProfile"` } -// +enum +// +kubebuilder:validation:Enum type AppArmorProfileType string const ( @@ -1072,7 +1076,7 @@ type HostAlias struct { // Only one of the following restart policies may be specified. // If none of the following policies is specified, the default one // is RestartPolicyAlways. -// +enum +// +kubebuilder:validation:Enum type RestartPolicy string const ( @@ -1082,7 +1086,7 @@ const ( ) // DNSPolicy defines how a pod's DNS will be configured. -// +enum +// +kubebuilder:validation:Enum type DNSPolicy string const ( diff --git a/tests/integration/testdata/default_configurations/pod.go b/tests/integration/testdata/default_configurations/pod.go index f0bde5e5..08f43e4c 100644 --- a/tests/integration/testdata/default_configurations/pod.go +++ b/tests/integration/testdata/default_configurations/pod.go @@ -388,6 +388,7 @@ type PodReadinessGate struct { } // PodConditionType is a valid value for PodCondition.Type +// +kubebuilder:validation:Enum=ContainersReady;Initialized;Ready;PodScheduled;DisruptionTarget;PodReadyToStartContainers;PodResizePending;PodResizeInProgress type PodConditionType string // These are built-in conditions of pod. An application may use a custom condition not listed here. @@ -420,6 +421,7 @@ const ( PodResizeInProgress PodConditionType = "PodResizeInProgress" ) +// +kubebuilder:validation:Enum=linux;windows // OSName is the set of OS'es that can be used in OS. type OSName string diff --git a/tests/integration/testdata/default_configurations/security_context.go b/tests/integration/testdata/default_configurations/security_context.go index c54d07f5..73a8e8a4 100644 --- a/tests/integration/testdata/default_configurations/security_context.go +++ b/tests/integration/testdata/default_configurations/security_context.go @@ -128,7 +128,7 @@ type PodSecurityContext struct { // SupplementalGroupsPolicy defines how supplemental groups // of the first container processes are calculated. -// +enum +// +kubebuilder:validation:Enum=Merge;Strict type SupplementalGroupsPolicy string const ( @@ -153,7 +153,7 @@ type Sysctl struct { // PodFSGroupChangePolicy holds policies that will be used for applying fsGroup to a volume // when volume is mounted. -// +enum +// +kubebuilder:validation:Enum=OnRootMismatch;Always type PodFSGroupChangePolicy string const ( @@ -168,6 +168,7 @@ const ( FSGroupChangeAlways PodFSGroupChangePolicy = "Always" ) +// +kubebuilder:validation:Enum // PodSELinuxChangePolicy defines how the container's SELinux label is applied to all volumes used by the Pod. type PodSELinuxChangePolicy string diff --git a/tests/integration/testdata/default_configurations/volume.go b/tests/integration/testdata/default_configurations/volume.go index 4e63238e..9f4bb4dd 100644 --- a/tests/integration/testdata/default_configurations/volume.go +++ b/tests/integration/testdata/default_configurations/volume.go @@ -50,7 +50,7 @@ type VolumeMount struct { } // MountPropagationMode describes mount propagation. -// +enum +// +kubebuilder:validation:Enum=None;HostToContainer;Bidirectional type MountPropagationMode string const ( @@ -75,6 +75,7 @@ const ( MountPropagationBidirectional MountPropagationMode = "Bidirectional" ) +// +kubebuilder:validation:Enum // RecursiveReadOnlyMode describes recursive-readonly mode. type RecursiveReadOnlyMode string @@ -460,7 +461,7 @@ type HostPathVolumeSource struct { Type *HostPathType `json:"type,omitempty" protobuf:"bytes,2,opt,name=type"` } -// +enum +// +kubebuilder:validation:Enum="";DirectoryOrCreate;Directory;FileOrCreate;File;Socket;CharDevice;BlockDevice type HostPathType string const ( @@ -762,6 +763,7 @@ type FlockerVolumeSource struct { } // StorageMedium defines ways that storage can be allocated to a volume. +// +kubebuilder:validation:Enum=Memory;HugePages;HugePages- type StorageMedium string const ( @@ -1194,10 +1196,10 @@ type PhotonPersistentDiskVolumeSource struct { FSType string `json:"fsType,omitempty" protobuf:"bytes,2,opt,name=fsType"` // want "optionalorrequired: field PhotonPersistentDiskVolumeSource.FSType must be marked as optional or required" } -// +enum +// +kubebuilder:validation:Enum=None;ReadOnly;ReadWrite type AzureDataDiskCachingMode string -// +enum +// +kubebuilder:validation:Enum=Shared;Dedicated;Managed type AzureDataDiskKind string const ( @@ -1872,7 +1874,7 @@ type PersistentVolumeClaimSpec struct { VolumeAttributesClassName *string `json:"volumeAttributesClassName,omitempty" protobuf:"bytes,9,opt,name=volumeAttributesClassName"` } -// +enum +// +kubebuilder:validation:Enum type PersistentVolumeAccessMode string const ( @@ -1888,7 +1890,7 @@ const ( ) // PersistentVolumeMode describes how a volume is intended to be consumed, either Block or Filesystem. -// +enum +// +kubebuilder:validation:Enum=Block;Filesystem type PersistentVolumeMode string const (