From 0c23a8815ced00f642ec3a663a80b016404ff398 Mon Sep 17 00:00:00 2001 From: sivchari Date: Sat, 6 Dec 2025 12:30:34 +0900 Subject: [PATCH] new linter: defaults Signed-off-by: sivchari --- docs/linters.md | 40 ++ pkg/analysis/defaults/analyzer.go | 378 ++++++++++++++++++ pkg/analysis/defaults/analyzer_test.go | 109 +++++ pkg/analysis/defaults/config.go | 61 +++ pkg/analysis/defaults/defaults_suite_test.go | 29 ++ pkg/analysis/defaults/doc.go | 41 ++ pkg/analysis/defaults/initializer.go | 92 +++++ pkg/analysis/defaults/initializer_test.go | 132 ++++++ pkg/analysis/defaults/testdata/src/a/a.go | 119 ++++++ .../defaults/testdata/src/a/a.go.golden | 119 ++++++ pkg/analysis/defaults/testdata/src/b/b.go | 37 ++ .../defaults/testdata/src/b/b.go.golden | 37 ++ pkg/analysis/defaults/testdata/src/c/c.go | 29 ++ .../defaults/testdata/src/c/c.go.golden | 29 ++ pkg/analysis/defaults/testdata/src/d/d.go | 29 ++ .../defaults/testdata/src/d/d.go.golden | 29 ++ pkg/analysis/defaults/testdata/src/e/e.go | 34 ++ .../defaults/testdata/src/e/e.go.golden | 34 ++ pkg/analysis/defaults/testdata/src/f/f.go | 34 ++ .../defaults/testdata/src/f/f.go.golden | 34 ++ pkg/markers/markers.go | 3 + pkg/registration/doc.go | 1 + .../default_configurations/container.go | 2 +- .../object_references.go | 2 +- .../testdata/default_configurations/volume.go | 2 +- 25 files changed, 1453 insertions(+), 3 deletions(-) create mode 100644 pkg/analysis/defaults/analyzer.go create mode 100644 pkg/analysis/defaults/analyzer_test.go create mode 100644 pkg/analysis/defaults/config.go create mode 100644 pkg/analysis/defaults/defaults_suite_test.go create mode 100644 pkg/analysis/defaults/doc.go create mode 100644 pkg/analysis/defaults/initializer.go create mode 100644 pkg/analysis/defaults/initializer_test.go create mode 100644 pkg/analysis/defaults/testdata/src/a/a.go create mode 100644 pkg/analysis/defaults/testdata/src/a/a.go.golden create mode 100644 pkg/analysis/defaults/testdata/src/b/b.go create mode 100644 pkg/analysis/defaults/testdata/src/b/b.go.golden create mode 100644 pkg/analysis/defaults/testdata/src/c/c.go create mode 100644 pkg/analysis/defaults/testdata/src/c/c.go.golden create mode 100644 pkg/analysis/defaults/testdata/src/d/d.go create mode 100644 pkg/analysis/defaults/testdata/src/d/d.go.golden create mode 100644 pkg/analysis/defaults/testdata/src/e/e.go create mode 100644 pkg/analysis/defaults/testdata/src/e/e.go.golden create mode 100644 pkg/analysis/defaults/testdata/src/f/f.go create mode 100644 pkg/analysis/defaults/testdata/src/f/f.go.golden diff --git a/docs/linters.md b/docs/linters.md index cc1c336e..4b2e62a4 100644 --- a/docs/linters.md +++ b/docs/linters.md @@ -5,6 +5,7 @@ - [CommentStart](#commentstart) - Ensures comments start with the serialized form of the type - [ConflictingMarkers](#conflictingmarkers) - Detects mutually exclusive markers on the same field - [DefaultOrRequired](#defaultorrequired) - Ensures fields marked as required do not have default values +- [Defaults](#defaults) - Checks that fields with default markers are configured correctly - [DuplicateMarkers](#duplicatemarkers) - Checks for exact duplicates of markers - [DependentTags](#dependenttags) - Enforces dependencies between markers - [ForbiddenMarkers](#forbiddenmarkers) - Checks that no forbidden markers are present on types/fields. @@ -212,6 +213,45 @@ The linter also detects conflicts with: This linter is enabled by default and helps ensure that API designs are consistent and unambiguous about whether fields are truly required or have default values. +## Defaults + +The `defaults` linter checks that fields with default markers are configured correctly. + +Fields with default markers (`+default`, `+kubebuilder:default`, or `+k8s:default`) should also be marked as optional. +Additionally, fields with default markers should have `omitempty` or `omitzero` in their json tags to ensure that +the default values are applied correctly during serialization and deserialization. + +### Example + +A well-configured field with a default: + +```go +type MyStruct struct { + // +optional + // +default="default-value" + Field string `json:"field,omitempty"` +} +``` + +The following issues will be flagged by the linter: + +```go +type MyStruct struct { + // Missing optional marker + // +default="value" + Field1 string `json:"field1,omitempty"` // Error: has default but not marked as optional + + // Missing omitempty tag + // +optional + // +default="value" + Field2 string `json:"field2"` // Error: has default but no omitempty or omitzero +} +``` + +### Fixes + +The `defaults` linter can automatically add `omitempty,omitzero` to the json tag when missing. + ## DuplicateMarkers The duplicatemarkers linter checks for exact duplicates of markers for types and fields. diff --git a/pkg/analysis/defaults/analyzer.go b/pkg/analysis/defaults/analyzer.go new file mode 100644 index 00000000..c2996383 --- /dev/null +++ b/pkg/analysis/defaults/analyzer.go @@ -0,0 +1,378 @@ +/* +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 defaults + +import ( + "fmt" + "go/ast" + + "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/analysis/utils/serialization" + "sigs.k8s.io/kube-api-linter/pkg/markers" +) + +const ( + name = "defaults" +) + +func init() { + markershelper.DefaultRegistry().Register( + markers.DefaultMarker, + markers.KubebuilderDefaultMarker, + markers.K8sDefaultMarker, + markers.OptionalMarker, + markers.KubebuilderOptionalMarker, + markers.K8sOptionalMarker, + markers.RequiredMarker, + markers.KubebuilderRequiredMarker, + markers.K8sRequiredMarker, + ) +} + +type analyzer struct { + preferredDefaultMarker string + secondaryDefaultMarker string + omitEmptyPolicy serialization.OmitEmptyPolicy + omitZeroPolicy serialization.OmitZeroPolicy +} + +// newAnalyzer creates a new analyzer with the given configuration. +func newAnalyzer(cfg *DefaultsConfig) *analysis.Analyzer { + if cfg == nil { + cfg = &DefaultsConfig{} + } + + defaultConfig(cfg) + + a := &analyzer{ + omitEmptyPolicy: cfg.OmitEmpty.Policy, + omitZeroPolicy: cfg.OmitZero.Policy, + } + + switch cfg.PreferredDefaultMarker { + case markers.DefaultMarker: + a.preferredDefaultMarker = markers.DefaultMarker + a.secondaryDefaultMarker = markers.KubebuilderDefaultMarker + case markers.KubebuilderDefaultMarker: + a.preferredDefaultMarker = markers.KubebuilderDefaultMarker + a.secondaryDefaultMarker = markers.DefaultMarker + } + + return &analysis.Analyzer{ + Name: name, + Doc: `Checks that fields with default markers are configured correctly. +Fields with default markers (+default, +kubebuilder:default, or +k8s:default) should also be marked as optional and not required. +Additionally, fields with default markers should have "omitempty" or "omitzero" in their json tags to ensure that the default values are applied correctly during serialization and deserialization. +`, + Run: a.run, + Requires: []*analysis.Analyzer{inspector.Analyzer}, + } +} + +func defaultConfig(cfg *DefaultsConfig) { + if cfg.PreferredDefaultMarker == "" { + cfg.PreferredDefaultMarker = markers.DefaultMarker + } + + if cfg.OmitEmpty.Policy == "" { + cfg.OmitEmpty.Policy = serialization.OmitEmptyPolicySuggestFix + } + + if cfg.OmitZero.Policy == "" { + cfg.OmitZero.Policy = serialization.OmitZeroPolicySuggestFix + } +} + +func (a *analyzer) run(pass *analysis.Pass) (any, error) { + inspect, ok := pass.ResultOf[inspector.Analyzer].(inspector.Inspector) + if !ok { + return nil, kalerrors.ErrCouldNotGetInspector + } + + inspect.InspectFields(func(field *ast.Field, jsonTagInfo extractjsontags.FieldTagInfo, markersAccess markershelper.Markers, qualifiedFieldName string) { + a.checkField(pass, field, jsonTagInfo, markersAccess, qualifiedFieldName) + }) + + return nil, nil //nolint:nilnil +} + +func (a *analyzer) checkField(pass *analysis.Pass, field *ast.Field, jsonTagInfo extractjsontags.FieldTagInfo, markersAccess markershelper.Markers, qualifiedFieldName string) { + if field == nil || len(field.Names) == 0 { + return + } + + fieldMarkers := markersAccess.FieldMarkers(field) + + // Check for any default marker (+default, +kubebuilder:default, or +k8s:default) + hasPreferredDefault := fieldMarkers.Has(a.preferredDefaultMarker) + hasSecondaryDefault := fieldMarkers.Has(a.secondaryDefaultMarker) + hasK8sDefault := fieldMarkers.Has(markers.K8sDefaultMarker) + + hasAnyDefault := hasPreferredDefault || hasSecondaryDefault || hasK8sDefault + + if !hasAnyDefault { + return + } + + // Check +k8s:default marker (for declarative validation, separate from preferred/secondary) + // If +k8s:default is present but neither +default nor +kubebuilder:default is present, suggest adding the preferred one + a.checkK8sDefault(pass, field, fieldMarkers, qualifiedFieldName, hasPreferredDefault || hasSecondaryDefault) + + // Check secondary marker usage + // If both preferred and secondary exist, suggest removing secondary + // If only secondary exists, suggest replacing with preferred + if hasSecondaryDefault { + hasBothDefaults := hasPreferredDefault && hasSecondaryDefault + a.checkSecondaryDefault(pass, field, fieldMarkers, qualifiedFieldName, hasBothDefaults) + } + + a.checkDefaultNotRequired(pass, field, markersAccess, qualifiedFieldName) + + a.checkDefaultOptional(pass, field, markersAccess, qualifiedFieldName) + + a.checkDefaultOmitEmptyOrOmitZero(pass, field, jsonTagInfo, qualifiedFieldName) +} + +// checkK8sDefault checks for +k8s:default marker usage. +// +k8s:default is for declarative validation and is separate from preferred/secondary default markers. +// If the field has +k8s:default but doesn't have +default or +kubebuilder:default, we suggest adding the preferred one. +func (a *analyzer) checkK8sDefault(pass *analysis.Pass, field *ast.Field, fieldMarkers markershelper.MarkerSet, qualifiedFieldName string, hasOtherDefault bool) { + if !fieldMarkers.Has(markers.K8sDefaultMarker) { + return + } + + // If the field already has +default or +kubebuilder:default, +k8s:default is acceptable alongside them + // (e.g., in K/K where both are needed during transition period) + if hasOtherDefault { + return + } + + // If only +k8s:default is present, suggest adding the preferred default marker + k8sDefaultMarkers := fieldMarkers.Get(markers.K8sDefaultMarker) + for _, marker := range k8sDefaultMarkers { + payloadValue := marker.Payload.Value + pass.Report(analysis.Diagnostic{ + Pos: field.Pos(), + Message: fmt.Sprintf("field %s has +%s but should also have +%s marker", qualifiedFieldName, markers.K8sDefaultMarker, a.preferredDefaultMarker), + SuggestedFixes: []analysis.SuggestedFix{ + { + Message: fmt.Sprintf("add +%s=%s", a.preferredDefaultMarker, payloadValue), + TextEdits: []analysis.TextEdit{ + { + Pos: marker.Pos, + End: marker.Pos, + NewText: fmt.Appendf(nil, "// +%s=%s\n\t", a.preferredDefaultMarker, payloadValue), + }, + }, + }, + }, + }) + } +} + +func (a *analyzer) checkSecondaryDefault(pass *analysis.Pass, field *ast.Field, fieldMarkers markershelper.MarkerSet, qualifiedFieldName string, hasBothDefaults bool) { + secondaryDefaultMarkers := fieldMarkers.Get(a.secondaryDefaultMarker) + + if hasBothDefaults { + // Both preferred and secondary markers exist - suggest removing secondary + pass.Report(reportShouldRemoveSecondaryMarker(field, secondaryDefaultMarkers, a.preferredDefaultMarker, a.secondaryDefaultMarker, qualifiedFieldName)) + return + } + // Only secondary marker exists - suggest replacing with preferred + pass.Report(reportShouldReplaceSecondaryMarker(field, secondaryDefaultMarkers, a.preferredDefaultMarker, a.secondaryDefaultMarker, qualifiedFieldName)) +} + +func reportShouldReplaceSecondaryMarker(field *ast.Field, markers []markershelper.Marker, preferredMarker, secondaryMarker, qualifiedFieldName string) analysis.Diagnostic { + textEdits := make([]analysis.TextEdit, len(markers)) + + for i, marker := range markers { + if i == 0 { + textEdits[i] = analysis.TextEdit{ + Pos: marker.Pos, + End: marker.End, + NewText: fmt.Appendf(nil, "// +%s=%s", preferredMarker, marker.Payload.Value), + } + + continue + } + + textEdits[i] = analysis.TextEdit{ + Pos: marker.Pos, + End: marker.End + 1, // Add 1 to position to include the new line + NewText: nil, + } + } + + return analysis.Diagnostic{ + Pos: field.Pos(), + Message: fmt.Sprintf("field %s should use +%s marker instead of +%s", qualifiedFieldName, preferredMarker, secondaryMarker), + SuggestedFixes: []analysis.SuggestedFix{ + { + Message: fmt.Sprintf("replace +%s with +%s", secondaryMarker, preferredMarker), + TextEdits: textEdits, + }, + }, + } +} + +func reportShouldRemoveSecondaryMarker(field *ast.Field, markers []markershelper.Marker, preferredMarker, secondaryMarker, qualifiedFieldName string) analysis.Diagnostic { + textEdits := make([]analysis.TextEdit, len(markers)) + + for i, marker := range markers { + textEdits[i] = analysis.TextEdit{ + Pos: marker.Pos, + End: marker.End + 1, // Add 1 to position to include the new line + NewText: nil, + } + } + + return analysis.Diagnostic{ + Pos: field.Pos(), + Message: fmt.Sprintf("field %s should use only the marker +%s, +%s is not required", qualifiedFieldName, preferredMarker, secondaryMarker), + SuggestedFixes: []analysis.SuggestedFix{ + { + Message: fmt.Sprintf("remove +%s", secondaryMarker), + TextEdits: textEdits, + }, + }, + } +} + +func (a *analyzer) checkDefaultNotRequired(pass *analysis.Pass, field *ast.Field, markersAccess markershelper.Markers, qualifiedFieldName string) { + if utils.IsFieldRequired(field, markersAccess) { + pass.Report(analysis.Diagnostic{ + Pos: field.Pos(), + Message: fmt.Sprintf("field %s has a default value but is marked as required, which is contradictory", qualifiedFieldName), + }) + } +} + +func (a *analyzer) checkDefaultOptional(pass *analysis.Pass, field *ast.Field, markersAccess markershelper.Markers, qualifiedFieldName string) { + // If the field is required, we've already reported that issue in checkDefaultNotRequired + if utils.IsFieldRequired(field, markersAccess) { + return + } + + if !utils.IsFieldOptional(field, markersAccess) { + pass.Report(analysis.Diagnostic{ + Pos: field.Pos(), + Message: fmt.Sprintf("field %s has a default value but is not marked as optional", qualifiedFieldName), + }) + } +} + +func (a *analyzer) checkDefaultOmitEmptyOrOmitZero(pass *analysis.Pass, field *ast.Field, jsonTagInfo extractjsontags.FieldTagInfo, qualifiedFieldName string) { + if jsonTagInfo.Inline || jsonTagInfo.Ignored { + return + } + + hasOmitEmpty := jsonTagInfo.OmitEmpty + hasOmitZero := jsonTagInfo.OmitZero + + // Check if the field is a pointer type - pointers don't need omitzero because nil is their zero value + isPointer, _ := utils.IsStarExpr(field.Type) + isStruct := !isPointer && utils.IsStructType(pass, field.Type) + + // Check omitzero for struct types (but not pointers) + // When omitempty policy is Ignore, we don't add omitempty even if it's missing + shouldAddOmitEmpty := a.omitEmptyPolicy != serialization.OmitEmptyPolicyIgnore + + if isStruct && a.omitZeroPolicy != serialization.OmitZeroPolicyForbid { + if !hasOmitZero { + a.reportMissingOmitZero(pass, field, jsonTagInfo, qualifiedFieldName, hasOmitEmpty, shouldAddOmitEmpty) + } + } + + // Check omitempty (only if policy is not Ignore) + if a.omitEmptyPolicy != serialization.OmitEmptyPolicyIgnore && !hasOmitEmpty { + a.reportMissingOmitEmpty(pass, field, jsonTagInfo, qualifiedFieldName) + } +} + +func (a *analyzer) reportMissingOmitEmpty(pass *analysis.Pass, field *ast.Field, jsonTagInfo extractjsontags.FieldTagInfo, qualifiedFieldName string) { + suggestedTag := fmt.Sprintf("%s,omitempty", jsonTagInfo.RawValue) + message := fmt.Sprintf("add omitempty to the json tag of field %s", qualifiedFieldName) + + switch a.omitEmptyPolicy { + case serialization.OmitEmptyPolicySuggestFix: + pass.Report(analysis.Diagnostic{ + Pos: field.Pos(), + Message: fmt.Sprintf("field %s has a default value but does not have omitempty in its json tag", qualifiedFieldName), + SuggestedFixes: []analysis.SuggestedFix{ + { + Message: message, + TextEdits: []analysis.TextEdit{ + { + Pos: jsonTagInfo.Pos, + End: jsonTagInfo.End, + NewText: []byte(suggestedTag), + }, + }, + }, + }, + }) + case serialization.OmitEmptyPolicyWarn: + pass.Reportf(field.Pos(), "field %s has a default value but does not have omitempty in its json tag", qualifiedFieldName) + case serialization.OmitEmptyPolicyIgnore: + // Unreachable: this function is only called when the policy is not Ignore. + return + } +} + +func (a *analyzer) reportMissingOmitZero(pass *analysis.Pass, field *ast.Field, jsonTagInfo extractjsontags.FieldTagInfo, qualifiedFieldName string, hasOmitEmpty, shouldAddOmitEmpty bool) { + var suggestedTag string + + switch { + case hasOmitEmpty: + suggestedTag = fmt.Sprintf("%s,omitzero", jsonTagInfo.RawValue) + case shouldAddOmitEmpty: + suggestedTag = fmt.Sprintf("%s,omitempty,omitzero", jsonTagInfo.RawValue) + default: + suggestedTag = fmt.Sprintf("%s,omitzero", jsonTagInfo.RawValue) + } + + message := fmt.Sprintf("add omitzero to the json tag of field %s", qualifiedFieldName) + + switch a.omitZeroPolicy { + case serialization.OmitZeroPolicySuggestFix: + pass.Report(analysis.Diagnostic{ + Pos: field.Pos(), + Message: fmt.Sprintf("field %s has a default value but does not have omitzero in its json tag", qualifiedFieldName), + SuggestedFixes: []analysis.SuggestedFix{ + { + Message: message, + TextEdits: []analysis.TextEdit{ + { + Pos: jsonTagInfo.Pos, + End: jsonTagInfo.End, + NewText: []byte(suggestedTag), + }, + }, + }, + }, + }) + case serialization.OmitZeroPolicyWarn: + pass.Reportf(field.Pos(), "field %s has a default value but does not have omitzero in its json tag", qualifiedFieldName) + case serialization.OmitZeroPolicyForbid: + // Unreachable: this function is only called when the policy is not Forbid. + return + } +} diff --git a/pkg/analysis/defaults/analyzer_test.go b/pkg/analysis/defaults/analyzer_test.go new file mode 100644 index 00000000..d678a13d --- /dev/null +++ b/pkg/analysis/defaults/analyzer_test.go @@ -0,0 +1,109 @@ +/* +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 defaults_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + "sigs.k8s.io/kube-api-linter/pkg/analysis/defaults" + "sigs.k8s.io/kube-api-linter/pkg/analysis/utils/serialization" + "sigs.k8s.io/kube-api-linter/pkg/markers" +) + +func TestDefaultConfiguration(t *testing.T) { + testdata := analysistest.TestData() + + a, err := defaults.Initializer().Init(&defaults.DefaultsConfig{}) + if err != nil { + t.Fatal(err) + } + + analysistest.RunWithSuggestedFixes(t, testdata, a, "a") +} + +func TestKubebuilderDefaultMarkerPreferred(t *testing.T) { + testdata := analysistest.TestData() + + a, err := defaults.Initializer().Init(&defaults.DefaultsConfig{ + PreferredDefaultMarker: markers.KubebuilderDefaultMarker, + }) + if err != nil { + t.Fatal(err) + } + + analysistest.RunWithSuggestedFixes(t, testdata, a, "b") +} + +func TestOmitZeroForbidden(t *testing.T) { + testdata := analysistest.TestData() + + a, err := defaults.Initializer().Init(&defaults.DefaultsConfig{ + OmitZero: defaults.DefaultsOmitZero{ + Policy: serialization.OmitZeroPolicyForbid, + }, + }) + if err != nil { + t.Fatal(err) + } + + analysistest.RunWithSuggestedFixes(t, testdata, a, "c") +} + +func TestOmitEmptyWarn(t *testing.T) { + testdata := analysistest.TestData() + + a, err := defaults.Initializer().Init(&defaults.DefaultsConfig{ + OmitEmpty: defaults.DefaultsOmitEmpty{ + Policy: serialization.OmitEmptyPolicyWarn, + }, + }) + if err != nil { + t.Fatal(err) + } + + analysistest.RunWithSuggestedFixes(t, testdata, a, "d") +} + +func TestOmitEmptyIgnore(t *testing.T) { + testdata := analysistest.TestData() + + a, err := defaults.Initializer().Init(&defaults.DefaultsConfig{ + OmitEmpty: defaults.DefaultsOmitEmpty{ + Policy: serialization.OmitEmptyPolicyIgnore, + }, + }) + if err != nil { + t.Fatal(err) + } + + analysistest.RunWithSuggestedFixes(t, testdata, a, "e") +} + +func TestOmitZeroWarn(t *testing.T) { + testdata := analysistest.TestData() + + a, err := defaults.Initializer().Init(&defaults.DefaultsConfig{ + OmitZero: defaults.DefaultsOmitZero{ + Policy: serialization.OmitZeroPolicyWarn, + }, + }) + if err != nil { + t.Fatal(err) + } + + analysistest.RunWithSuggestedFixes(t, testdata, a, "f") +} diff --git a/pkg/analysis/defaults/config.go b/pkg/analysis/defaults/config.go new file mode 100644 index 00000000..71514a78 --- /dev/null +++ b/pkg/analysis/defaults/config.go @@ -0,0 +1,61 @@ +/* +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 defaults + +import "sigs.k8s.io/kube-api-linter/pkg/analysis/utils/serialization" + +// DefaultsConfig contains configuration for the defaults linter. +type DefaultsConfig struct { + // PreferredDefaultMarker is the preferred marker to use for default values. + // If this field is not set, the default value is "default". + // Valid values are "default" and "kubebuilder:default". + PreferredDefaultMarker string `json:"preferredDefaultMarker"` + + // OmitEmpty is the configuration for the `omitempty` tag within the json tag for fields with defaults. + // This defines how the linter should handle fields with defaults, and whether they should have the omitempty tag or not. + // By default, all fields with defaults will be expected to have the `omitempty` tag. + OmitEmpty DefaultsOmitEmpty `json:"omitempty"` + + // OmitZero is the configuration for the `omitzero` tag within the json tag for fields with defaults. + // This defines how the linter should handle fields with defaults, and whether they should have the omitzero tag or not. + // By default, struct fields with defaults will be expected to have the `omitzero` tag. + OmitZero DefaultsOmitZero `json:"omitzero"` +} + +// DefaultsOmitEmpty is the configuration for the `omitempty` tag on fields with defaults. +type DefaultsOmitEmpty struct { + // Policy determines whether the linter should require omitempty for fields with defaults. + // Valid values are "SuggestFix", "Warn" and "Ignore". + // When set to "SuggestFix", the linter will suggest adding the `omitempty` tag when a field with default does not have it. + // When set to "Warn", the linter will emit a warning if the field does not have the `omitempty` tag. + // When set to "Ignore", a field with default missing the `omitempty` tag will be ignored. + // When otherwise not specified, the default value is "SuggestFix". + Policy serialization.OmitEmptyPolicy `json:"policy"` +} + +// DefaultsOmitZero is the configuration for the `omitzero` tag on fields with defaults. +type DefaultsOmitZero struct { + // Policy determines whether the linter should require omitzero for struct fields with defaults. + // Valid values are "SuggestFix", "Warn" and "Forbid". + // When set to "SuggestFix", the linter will suggest adding the `omitzero` tag when a struct field with default does not have it. + // When set to "Warn", the linter will emit a warning if the field does not have the `omitzero` tag. + // When set to "Forbid", 'omitzero' tags wont be considered. + // Note, when set to "Forbid", and a field have the `omitzero` tag, the linter will not suggest adding it. + // Note, `omitzero` tag is supported in go version starting from go 1.24. + // Note, Configure omitzero policy to 'Forbid', if using with go version less than go 1.24. + // When otherwise not specified, the default value is "SuggestFix". + Policy serialization.OmitZeroPolicy `json:"policy"` +} diff --git a/pkg/analysis/defaults/defaults_suite_test.go b/pkg/analysis/defaults/defaults_suite_test.go new file mode 100644 index 00000000..d0cd6113 --- /dev/null +++ b/pkg/analysis/defaults/defaults_suite_test.go @@ -0,0 +1,29 @@ +/* +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 defaults_test + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestDefaults(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "defaults") +} diff --git a/pkg/analysis/defaults/doc.go b/pkg/analysis/defaults/doc.go new file mode 100644 index 00000000..5ccb0bea --- /dev/null +++ b/pkg/analysis/defaults/doc.go @@ -0,0 +1,41 @@ +/* +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. +*/ + +/* +defaults is a linter to check that fields with default markers are configured correctly. + +Fields with default markers (+default, +kubebuilder:default, or +k8s:default) should also be marked as optional. +Additionally, fields with default markers should have "omitempty" or "omitzero" in their json tags +to ensure that the default values are applied correctly during serialization and deserialization. + +Example of a well-configured field with a default: + + // +optional + // +default="default-value" + Field string `json:"field,omitempty"` + +Example of issues this linter will catch: + + // Missing optional marker + // +default="value" + Field string `json:"field,omitempty"` + + // Missing omitempty tag + // +optional + // +default="value" + Field string `json:"field"` +*/ +package defaults diff --git a/pkg/analysis/defaults/initializer.go b/pkg/analysis/defaults/initializer.go new file mode 100644 index 00000000..c1912646 --- /dev/null +++ b/pkg/analysis/defaults/initializer.go @@ -0,0 +1,92 @@ +/* +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 defaults + +import ( + "fmt" + + "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" + "sigs.k8s.io/kube-api-linter/pkg/analysis/utils/serialization" + "sigs.k8s.io/kube-api-linter/pkg/markers" +) + +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, + true, + validateConfig, + ) +} + +func initAnalyzer(cfg *DefaultsConfig) (*analysis.Analyzer, error) { + return newAnalyzer(cfg), nil +} + +// validateConfig is used to validate the configuration in the DefaultsConfig struct. +func validateConfig(cfg *DefaultsConfig, fldPath *field.Path) field.ErrorList { + if cfg == nil { + return field.ErrorList{} + } + + fieldErrors := field.ErrorList{} + + switch cfg.PreferredDefaultMarker { + case "", markers.DefaultMarker, markers.KubebuilderDefaultMarker: + default: + fieldErrors = append(fieldErrors, field.Invalid(fldPath.Child("preferredDefaultMarker"), cfg.PreferredDefaultMarker, fmt.Sprintf("invalid value, must be one of %q, %q or omitted", markers.DefaultMarker, markers.KubebuilderDefaultMarker))) + } + + fieldErrors = append(fieldErrors, validateOmitEmpty(cfg.OmitEmpty, fldPath.Child("omitempty"))...) + fieldErrors = append(fieldErrors, validateOmitZero(cfg.OmitZero, fldPath.Child("omitzero"))...) + + return fieldErrors +} + +// validateOmitEmpty is used to validate the configuration in the DefaultsOmitEmpty struct. +func validateOmitEmpty(oec DefaultsOmitEmpty, fldPath *field.Path) field.ErrorList { + fieldErrors := field.ErrorList{} + + switch oec.Policy { + case "", serialization.OmitEmptyPolicyIgnore, serialization.OmitEmptyPolicyWarn, serialization.OmitEmptyPolicySuggestFix: + default: + fieldErrors = append(fieldErrors, field.Invalid(fldPath.Child("policy"), oec.Policy, fmt.Sprintf("invalid value, must be one of %q, %q, %q or omitted", serialization.OmitEmptyPolicyIgnore, serialization.OmitEmptyPolicyWarn, serialization.OmitEmptyPolicySuggestFix))) + } + + return fieldErrors +} + +// validateOmitZero is used to validate the configuration in the DefaultsOmitZero struct. +func validateOmitZero(ozc DefaultsOmitZero, fldPath *field.Path) field.ErrorList { + fieldErrors := field.ErrorList{} + + switch ozc.Policy { + case "", serialization.OmitZeroPolicyForbid, serialization.OmitZeroPolicyWarn, serialization.OmitZeroPolicySuggestFix: + default: + fieldErrors = append(fieldErrors, field.Invalid(fldPath.Child("policy"), ozc.Policy, fmt.Sprintf("invalid value, must be one of %q, %q, %q or omitted", serialization.OmitZeroPolicyForbid, serialization.OmitZeroPolicyWarn, serialization.OmitZeroPolicySuggestFix))) + } + + return fieldErrors +} diff --git a/pkg/analysis/defaults/initializer_test.go b/pkg/analysis/defaults/initializer_test.go new file mode 100644 index 00000000..5ea82019 --- /dev/null +++ b/pkg/analysis/defaults/initializer_test.go @@ -0,0 +1,132 @@ +/* +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 defaults_test + +import ( + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "k8s.io/apimachinery/pkg/util/validation/field" + "sigs.k8s.io/kube-api-linter/pkg/analysis/defaults" + "sigs.k8s.io/kube-api-linter/pkg/analysis/initializer" + "sigs.k8s.io/kube-api-linter/pkg/analysis/utils/serialization" + "sigs.k8s.io/kube-api-linter/pkg/markers" +) + +var _ = Describe("defaults initializer", func() { + Context("config validation", func() { + type testCase struct { + config defaults.DefaultsConfig + expectedErr string + } + + DescribeTable("should validate the provided config", func(in testCase) { + ci, ok := defaults.Initializer().(initializer.ConfigurableAnalyzerInitializer) + Expect(ok).To(BeTrue()) + + errs := ci.ValidateConfig(&in.config, field.NewPath("defaults")) + if len(in.expectedErr) > 0 { + Expect(errs.ToAggregate()).To(MatchError(in.expectedErr)) + } else { + Expect(errs).To(HaveLen(0), "No errors were expected") + } + }, + Entry("With a valid DefaultsConfig using default marker", testCase{ + config: defaults.DefaultsConfig{ + PreferredDefaultMarker: markers.DefaultMarker, + }, + expectedErr: "", + }), + Entry("With kubebuilder:default preferred marker", testCase{ + config: defaults.DefaultsConfig{ + PreferredDefaultMarker: markers.KubebuilderDefaultMarker, + }, + expectedErr: "", + }), + Entry("With invalid preferred default marker", testCase{ + config: defaults.DefaultsConfig{ + PreferredDefaultMarker: "invalid", + }, + expectedErr: "defaults.preferredDefaultMarker: Invalid value: \"invalid\": invalid value, must be one of \"default\", \"kubebuilder:default\" or omitted", + }), + Entry("With SuggestFix omitempty policy", testCase{ + config: defaults.DefaultsConfig{ + OmitEmpty: defaults.DefaultsOmitEmpty{ + Policy: serialization.OmitEmptyPolicySuggestFix, + }, + }, + expectedErr: "", + }), + Entry("With Warn omitempty policy", testCase{ + config: defaults.DefaultsConfig{ + OmitEmpty: defaults.DefaultsOmitEmpty{ + Policy: serialization.OmitEmptyPolicyWarn, + }, + }, + expectedErr: "", + }), + Entry("With Ignore omitempty policy", testCase{ + config: defaults.DefaultsConfig{ + OmitEmpty: defaults.DefaultsOmitEmpty{ + Policy: serialization.OmitEmptyPolicyIgnore, + }, + }, + expectedErr: "", + }), + Entry("With invalid omitempty policy", testCase{ + config: defaults.DefaultsConfig{ + OmitEmpty: defaults.DefaultsOmitEmpty{ + Policy: "invalid", + }, + }, + expectedErr: "defaults.omitempty.policy: Invalid value: \"invalid\": invalid value, must be one of \"Ignore\", \"Warn\", \"SuggestFix\" or omitted", + }), + Entry("With SuggestFix omitzero policy", testCase{ + config: defaults.DefaultsConfig{ + OmitZero: defaults.DefaultsOmitZero{ + Policy: serialization.OmitZeroPolicySuggestFix, + }, + }, + expectedErr: "", + }), + Entry("With Warn omitzero policy", testCase{ + config: defaults.DefaultsConfig{ + OmitZero: defaults.DefaultsOmitZero{ + Policy: serialization.OmitZeroPolicyWarn, + }, + }, + expectedErr: "", + }), + Entry("With Forbid omitzero policy", testCase{ + config: defaults.DefaultsConfig{ + OmitZero: defaults.DefaultsOmitZero{ + Policy: serialization.OmitZeroPolicyForbid, + }, + }, + expectedErr: "", + }), + Entry("With invalid omitzero policy", testCase{ + config: defaults.DefaultsConfig{ + OmitZero: defaults.DefaultsOmitZero{ + Policy: "invalid", + }, + }, + expectedErr: "defaults.omitzero.policy: Invalid value: \"invalid\": invalid value, must be one of \"Forbid\", \"Warn\", \"SuggestFix\" or omitted", + }), + ) + }) +}) diff --git a/pkg/analysis/defaults/testdata/src/a/a.go b/pkg/analysis/defaults/testdata/src/a/a.go new file mode 100644 index 00000000..f5bb7883 --- /dev/null +++ b/pkg/analysis/defaults/testdata/src/a/a.go @@ -0,0 +1,119 @@ +package a + +type A struct { + // GoodDefaultField is correctly configured with +default, +optional, and omitempty. + // +optional + // +default="default-value" + GoodDefaultField string `json:"goodDefaultField,omitempty"` + + // K8sDefaultOnlyField uses only +k8s:default which requires adding the preferred marker. + // +optional + // +k8s:default="default-value" + K8sDefaultOnlyField string `json:"k8sDefaultOnlyField,omitempty"` // want "field A.K8sDefaultOnlyField has \\+k8s:default but should also have \\+default marker" + + // KubebuilderDefaultField uses +kubebuilder:default which should be replaced. + // +optional + // +kubebuilder:default="default-value" + KubebuilderDefaultField string `json:"kubebuilderDefaultField,omitempty"` // want "field A.KubebuilderDefaultField should use \\+default marker instead of \\+kubebuilder:default" + + // MissingOptionalField has a default but is not marked optional. + // +default="value" + MissingOptionalField string `json:"missingOptionalField,omitempty"` // want "field A.MissingOptionalField has a default value but is not marked as optional" + + // MissingOmitEmptyField has a default and optional but no omitempty. + // +optional + // +default="value" + MissingOmitEmptyField string `json:"missingOmitEmptyField"` // want "field A.MissingOmitEmptyField has a default value but does not have omitempty in its json tag" + + // BothIssuesField has both issues: not optional and no omitempty. + // +default="value" + BothIssuesField string `json:"bothIssuesField"` // want "field A.BothIssuesField has a default value but is not marked as optional" "field A.BothIssuesField has a default value but does not have omitempty in its json tag" + + // NoDefaultField is a regular field without default - should not be flagged. + // +optional + NoDefaultField string `json:"noDefaultField,omitempty"` + + // RequiredFieldWithDefault has both required and default, which is contradictory. + // +required + // +default="value" + RequiredFieldWithDefault string `json:"requiredFieldWithDefault,omitempty"` // want "field A.RequiredFieldWithDefault has a default value but is marked as required, which is contradictory" + + // RequiredFieldNoDefault is a required field without default - should not be flagged. + // +required + RequiredFieldNoDefault string `json:"requiredFieldNoDefault"` + + // RequiredFieldNoOmitEmpty is a required field without omitempty - should not be flagged. + // This tests that required fields without omitempty are not incorrectly flagged. + // +required + RequiredFieldNoOmitEmpty string `json:"requiredFieldNoOmitEmpty"` + + // InlineField with default and inline tag should not require omitempty. + // +optional + // +default={} + InlineField NestedStruct `json:",inline"` + + // IgnoredField with json ignore tag is skipped entirely by the inspector. + // No checks are performed for optional/omitempty on ignored fields. + // +default="ignored" + IgnoredField string `json:"-"` + + // BothDefaultAndK8sMarkers has both +default and +k8s:default which is acceptable (K/K transition). + // +optional + // +default="value" + // +k8s:default="value" + BothDefaultAndK8sMarkers string `json:"bothDefaultAndK8sMarkers,omitempty"` + + // BothDefaultAndKubebuilderMarkers has both +default and +kubebuilder:default which should suggest removing kubebuilder:default. + // +optional + // +default="value" + // +kubebuilder:default="value" + BothDefaultAndKubebuilderMarkers string `json:"bothDefaultAndKubebuilderMarkers,omitempty"` // want "field A.BothDefaultAndKubebuilderMarkers should use only the marker \\+default, \\+kubebuilder:default is not required" + + // StructFieldWithOmitEmpty has omitempty only (missing omitzero for struct types). + // +optional + // +default={} + StructFieldWithOmitEmpty NestedStruct `json:"structFieldWithOmitEmpty,omitempty"` // want "field A.StructFieldWithOmitEmpty has a default value but does not have omitzero in its json tag" + + // StructFieldWithOmitZero has omitzero only (missing omitempty). + // +optional + // +default={} + StructFieldWithOmitZero NestedStruct `json:"structFieldWithOmitZero,omitzero"` // want "field A.StructFieldWithOmitZero has a default value but does not have omitempty in its json tag" + + // StructFieldWithBothOmit has both omitempty and omitzero (valid for struct types). + // +optional + // +default={} + StructFieldWithBothOmit NestedStruct `json:"structFieldWithBothOmit,omitempty,omitzero"` + + // DefinedTypeField uses a defined type (alias to struct) - should require omitzero. + // +optional + // +default={} + DefinedTypeField DefinedStructType `json:"definedTypeField,omitempty"` // want "field A.DefinedTypeField has a default value but does not have omitzero in its json tag" + + // TypeAliasField uses a type alias to struct - should require omitzero. + // +optional + // +default={} + TypeAliasField StructTypeAlias `json:"typeAliasField,omitempty"` // want "field A.TypeAliasField has a default value but does not have omitzero in its json tag" + + // PointerToStructField is a pointer to struct - should NOT require omitzero (pointers have nil zero value). + // +optional + // +default={} + PointerToStructField *NestedStruct `json:"pointerToStructField,omitempty"` + + // PointerToStructMissingOmitEmpty is a pointer to struct missing omitempty. + // +optional + // +default={} + PointerToStructMissingOmitEmpty *NestedStruct `json:"pointerToStructMissingOmitEmpty"` // want "field A.PointerToStructMissingOmitEmpty has a default value but does not have omitempty in its json tag" +} + +// NestedStruct is a simple struct for testing. +type NestedStruct struct { + // Field is a nested field. + // +optional + Field string `json:"field,omitempty"` +} + +// DefinedStructType is a defined type based on NestedStruct. +type DefinedStructType NestedStruct + +// StructTypeAlias is a type alias to NestedStruct. +type StructTypeAlias = NestedStruct diff --git a/pkg/analysis/defaults/testdata/src/a/a.go.golden b/pkg/analysis/defaults/testdata/src/a/a.go.golden new file mode 100644 index 00000000..0323c73a --- /dev/null +++ b/pkg/analysis/defaults/testdata/src/a/a.go.golden @@ -0,0 +1,119 @@ +package a + +type A struct { + // GoodDefaultField is correctly configured with +default, +optional, and omitempty. + // +optional + // +default="default-value" + GoodDefaultField string `json:"goodDefaultField,omitempty"` + + // K8sDefaultOnlyField uses only +k8s:default which requires adding the preferred marker. + // +optional + // +default=default-value + // +k8s:default="default-value" + K8sDefaultOnlyField string `json:"k8sDefaultOnlyField,omitempty"` // want "field A.K8sDefaultOnlyField has \\+k8s:default but should also have \\+default marker" + + // KubebuilderDefaultField uses +kubebuilder:default which should be replaced. + // +optional + // +default="default-value" + KubebuilderDefaultField string `json:"kubebuilderDefaultField,omitempty"` // want "field A.KubebuilderDefaultField should use \\+default marker instead of \\+kubebuilder:default" + + // MissingOptionalField has a default but is not marked optional. + // +default="value" + MissingOptionalField string `json:"missingOptionalField,omitempty"` // want "field A.MissingOptionalField has a default value but is not marked as optional" + + // MissingOmitEmptyField has a default and optional but no omitempty. + // +optional + // +default="value" + MissingOmitEmptyField string `json:"missingOmitEmptyField,omitempty"` // want "field A.MissingOmitEmptyField has a default value but does not have omitempty in its json tag" + + // BothIssuesField has both issues: not optional and no omitempty. + // +default="value" + BothIssuesField string `json:"bothIssuesField,omitempty"` // want "field A.BothIssuesField has a default value but is not marked as optional" "field A.BothIssuesField has a default value but does not have omitempty in its json tag" + + // NoDefaultField is a regular field without default - should not be flagged. + // +optional + NoDefaultField string `json:"noDefaultField,omitempty"` + + // RequiredFieldWithDefault has both required and default, which is contradictory. + // +required + // +default="value" + RequiredFieldWithDefault string `json:"requiredFieldWithDefault,omitempty"` // want "field A.RequiredFieldWithDefault has a default value but is marked as required, which is contradictory" + + // RequiredFieldNoDefault is a required field without default - should not be flagged. + // +required + RequiredFieldNoDefault string `json:"requiredFieldNoDefault"` + + // RequiredFieldNoOmitEmpty is a required field without omitempty - should not be flagged. + // This tests that required fields without omitempty are not incorrectly flagged. + // +required + RequiredFieldNoOmitEmpty string `json:"requiredFieldNoOmitEmpty"` + + // InlineField with default and inline tag should not require omitempty. + // +optional + // +default={} + InlineField NestedStruct `json:",inline"` + + // IgnoredField with json ignore tag is skipped entirely by the inspector. + // No checks are performed for optional/omitempty on ignored fields. + // +default="ignored" + IgnoredField string `json:"-"` + + // BothDefaultAndK8sMarkers has both +default and +k8s:default which is acceptable (K/K transition). + // +optional + // +default="value" + // +k8s:default="value" + BothDefaultAndK8sMarkers string `json:"bothDefaultAndK8sMarkers,omitempty"` + + // BothDefaultAndKubebuilderMarkers has both +default and +kubebuilder:default which should suggest removing kubebuilder:default. + // +optional + // +default="value" + BothDefaultAndKubebuilderMarkers string `json:"bothDefaultAndKubebuilderMarkers,omitempty"` // want "field A.BothDefaultAndKubebuilderMarkers should use only the marker \\+default, \\+kubebuilder:default is not required" + + // StructFieldWithOmitEmpty has omitempty only (missing omitzero for struct types). + // +optional + // +default={} + StructFieldWithOmitEmpty NestedStruct `json:"structFieldWithOmitEmpty,omitempty,omitzero"` // want "field A.StructFieldWithOmitEmpty has a default value but does not have omitzero in its json tag" + + // StructFieldWithOmitZero has omitzero only (missing omitempty). + // +optional + // +default={} + StructFieldWithOmitZero NestedStruct `json:"structFieldWithOmitZero,omitzero,omitempty"` // want "field A.StructFieldWithOmitZero has a default value but does not have omitempty in its json tag" + + // StructFieldWithBothOmit has both omitempty and omitzero (valid for struct types). + // +optional + // +default={} + StructFieldWithBothOmit NestedStruct `json:"structFieldWithBothOmit,omitempty,omitzero"` + + // DefinedTypeField uses a defined type (alias to struct) - should require omitzero. + // +optional + // +default={} + DefinedTypeField DefinedStructType `json:"definedTypeField,omitempty,omitzero"` // want "field A.DefinedTypeField has a default value but does not have omitzero in its json tag" + + // TypeAliasField uses a type alias to struct - should require omitzero. + // +optional + // +default={} + TypeAliasField StructTypeAlias `json:"typeAliasField,omitempty,omitzero"` // want "field A.TypeAliasField has a default value but does not have omitzero in its json tag" + + // PointerToStructField is a pointer to struct - should NOT require omitzero (pointers have nil zero value). + // +optional + // +default={} + PointerToStructField *NestedStruct `json:"pointerToStructField,omitempty"` + + // PointerToStructMissingOmitEmpty is a pointer to struct missing omitempty. + // +optional + // +default={} + PointerToStructMissingOmitEmpty *NestedStruct `json:"pointerToStructMissingOmitEmpty,omitempty"` // want "field A.PointerToStructMissingOmitEmpty has a default value but does not have omitempty in its json tag" +} + +// NestedStruct is a simple struct for testing. +type NestedStruct struct { + // Field is a nested field. + // +optional + Field string `json:"field,omitempty"` +} + +// DefinedStructType is a defined type based on NestedStruct. +type DefinedStructType NestedStruct + +// StructTypeAlias is a type alias to NestedStruct. +type StructTypeAlias = NestedStruct diff --git a/pkg/analysis/defaults/testdata/src/b/b.go b/pkg/analysis/defaults/testdata/src/b/b.go new file mode 100644 index 00000000..ce15827b --- /dev/null +++ b/pkg/analysis/defaults/testdata/src/b/b.go @@ -0,0 +1,37 @@ +package b + +// This test file tests the behavior when kubebuilder:default marker is preferred. + +type B struct { + // GoodKubebuilderDefaultField is correctly configured with +kubebuilder:default, +optional, and omitempty. + // +optional + // +kubebuilder:default="default-value" + GoodKubebuilderDefaultField string `json:"goodKubebuilderDefaultField,omitempty"` + + // DefaultMarkerField uses +default which should be replaced with +kubebuilder:default when kubebuilder:default is preferred. + // +optional + // +default="default-value" + DefaultMarkerField string `json:"defaultMarkerField,omitempty"` // want "field B.DefaultMarkerField should use \\+kubebuilder:default marker instead of \\+default" + + // K8sDefaultOnlyField uses only +k8s:default which requires adding the preferred marker. + // +optional + // +k8s:default="default-value" + K8sDefaultOnlyField string `json:"k8sDefaultOnlyField,omitempty"` // want "field B.K8sDefaultOnlyField has \\+k8s:default but should also have \\+kubebuilder:default marker" + + // BothKubebuilderAndK8sMarkers has both +kubebuilder:default and +k8s:default which is acceptable. + // +optional + // +kubebuilder:default="value" + // +k8s:default="value" + BothKubebuilderAndK8sMarkers string `json:"bothKubebuilderAndK8sMarkers,omitempty"` + + // BothDefaultMarkers has both +kubebuilder:default and +default which should suggest removing +default. + // +optional + // +kubebuilder:default="value" + // +default="value" + BothDefaultMarkers string `json:"bothDefaultMarkers,omitempty"` // want "field B.BothDefaultMarkers should use only the marker \\+kubebuilder:default, \\+default is not required" + + // RequiredFieldWithDefault has both required and default, which is contradictory. + // +required + // +kubebuilder:default="value" + RequiredFieldWithDefault string `json:"requiredFieldWithDefault,omitempty"` // want "field B.RequiredFieldWithDefault has a default value but is marked as required, which is contradictory" +} diff --git a/pkg/analysis/defaults/testdata/src/b/b.go.golden b/pkg/analysis/defaults/testdata/src/b/b.go.golden new file mode 100644 index 00000000..8b43736a --- /dev/null +++ b/pkg/analysis/defaults/testdata/src/b/b.go.golden @@ -0,0 +1,37 @@ +package b + +// This test file tests the behavior when kubebuilder:default marker is preferred. + +type B struct { + // GoodKubebuilderDefaultField is correctly configured with +kubebuilder:default, +optional, and omitempty. + // +optional + // +kubebuilder:default="default-value" + GoodKubebuilderDefaultField string `json:"goodKubebuilderDefaultField,omitempty"` + + // DefaultMarkerField uses +default which should be replaced with +kubebuilder:default when kubebuilder:default is preferred. + // +optional + // +kubebuilder:default="default-value" + DefaultMarkerField string `json:"defaultMarkerField,omitempty"` // want "field B.DefaultMarkerField should use \\+kubebuilder:default marker instead of \\+default" + + // K8sDefaultOnlyField uses only +k8s:default which requires adding the preferred marker. + // +optional + // +kubebuilder:default=default-value + // +k8s:default="default-value" + K8sDefaultOnlyField string `json:"k8sDefaultOnlyField,omitempty"` // want "field B.K8sDefaultOnlyField has \\+k8s:default but should also have \\+kubebuilder:default marker" + + // BothKubebuilderAndK8sMarkers has both +kubebuilder:default and +k8s:default which is acceptable. + // +optional + // +kubebuilder:default="value" + // +k8s:default="value" + BothKubebuilderAndK8sMarkers string `json:"bothKubebuilderAndK8sMarkers,omitempty"` + + // BothDefaultMarkers has both +kubebuilder:default and +default which should suggest removing +default. + // +optional + // +kubebuilder:default="value" + BothDefaultMarkers string `json:"bothDefaultMarkers,omitempty"` // want "field B.BothDefaultMarkers should use only the marker \\+kubebuilder:default, \\+default is not required" + + // RequiredFieldWithDefault has both required and default, which is contradictory. + // +required + // +kubebuilder:default="value" + RequiredFieldWithDefault string `json:"requiredFieldWithDefault,omitempty"` // want "field B.RequiredFieldWithDefault has a default value but is marked as required, which is contradictory" +} diff --git a/pkg/analysis/defaults/testdata/src/c/c.go b/pkg/analysis/defaults/testdata/src/c/c.go new file mode 100644 index 00000000..a263a058 --- /dev/null +++ b/pkg/analysis/defaults/testdata/src/c/c.go @@ -0,0 +1,29 @@ +package c + +// This test file tests the behavior when omitzero policy is Forbid. +// When forbidden, the linter should only suggest omitempty, not omitzero, +// even for struct types. + +type C struct { + // GoodDefaultField is correctly configured with +default, +optional, and omitempty. + // +optional + // +default="default-value" + GoodDefaultField string `json:"goodDefaultField,omitempty"` + + // StructFieldMissingOmitEmpty has a default and optional but no omitempty. + // When omitzero is forbidden, only omitempty should be suggested. + // +optional + // +default={} + StructFieldMissingOmitEmpty NestedStruct `json:"structFieldMissingOmitEmpty"` // want "field C.StructFieldMissingOmitEmpty has a default value but does not have omitempty in its json tag" + + // StructFieldWithOmitEmpty is correctly configured with omitempty (omitzero not required when forbidden). + // +optional + // +default={} + StructFieldWithOmitEmpty NestedStruct `json:"structFieldWithOmitEmpty,omitempty"` +} + +type NestedStruct struct { + // Field is a nested field. + // +optional + Field string `json:"field,omitempty"` +} diff --git a/pkg/analysis/defaults/testdata/src/c/c.go.golden b/pkg/analysis/defaults/testdata/src/c/c.go.golden new file mode 100644 index 00000000..375637ea --- /dev/null +++ b/pkg/analysis/defaults/testdata/src/c/c.go.golden @@ -0,0 +1,29 @@ +package c + +// This test file tests the behavior when omitzero policy is Forbid. +// When forbidden, the linter should only suggest omitempty, not omitzero, +// even for struct types. + +type C struct { + // GoodDefaultField is correctly configured with +default, +optional, and omitempty. + // +optional + // +default="default-value" + GoodDefaultField string `json:"goodDefaultField,omitempty"` + + // StructFieldMissingOmitEmpty has a default and optional but no omitempty. + // When omitzero is forbidden, only omitempty should be suggested. + // +optional + // +default={} + StructFieldMissingOmitEmpty NestedStruct `json:"structFieldMissingOmitEmpty,omitempty"` // want "field C.StructFieldMissingOmitEmpty has a default value but does not have omitempty in its json tag" + + // StructFieldWithOmitEmpty is correctly configured with omitempty (omitzero not required when forbidden). + // +optional + // +default={} + StructFieldWithOmitEmpty NestedStruct `json:"structFieldWithOmitEmpty,omitempty"` +} + +type NestedStruct struct { + // Field is a nested field. + // +optional + Field string `json:"field,omitempty"` +} diff --git a/pkg/analysis/defaults/testdata/src/d/d.go b/pkg/analysis/defaults/testdata/src/d/d.go new file mode 100644 index 00000000..18513df0 --- /dev/null +++ b/pkg/analysis/defaults/testdata/src/d/d.go @@ -0,0 +1,29 @@ +package d + +// This test file tests the behavior when OmitEmpty.Policy is Warn. +// When set to Warn, the linter emits a warning without a suggested fix. + +type D struct { + // GoodField is correctly configured with +default, +optional, and omitempty. + // +optional + // +default="value" + GoodField string `json:"goodField,omitempty"` + + // MissingOmitEmpty has a default and optional but no omitempty. + // When policy is Warn, this should emit a warning without a fix. + // +optional + // +default="value" + MissingOmitEmpty string `json:"missingOmitEmpty"` // want "field D.MissingOmitEmpty has a default value but does not have omitempty in its json tag" + + // StructFieldMissingOmitZero is a struct field with default but missing omitzero. + // OmitZero policy is still SuggestFix by default, so this should suggest a fix. + // +optional + // +default={} + StructFieldMissingOmitZero NestedStruct `json:"structFieldMissingOmitZero,omitempty"` // want "field D.StructFieldMissingOmitZero has a default value but does not have omitzero in its json tag" +} + +type NestedStruct struct { + // Field is a nested field. + // +optional + Field string `json:"field,omitempty"` +} diff --git a/pkg/analysis/defaults/testdata/src/d/d.go.golden b/pkg/analysis/defaults/testdata/src/d/d.go.golden new file mode 100644 index 00000000..0b1d4e62 --- /dev/null +++ b/pkg/analysis/defaults/testdata/src/d/d.go.golden @@ -0,0 +1,29 @@ +package d + +// This test file tests the behavior when OmitEmpty.Policy is Warn. +// When set to Warn, the linter emits a warning without a suggested fix. + +type D struct { + // GoodField is correctly configured with +default, +optional, and omitempty. + // +optional + // +default="value" + GoodField string `json:"goodField,omitempty"` + + // MissingOmitEmpty has a default and optional but no omitempty. + // When policy is Warn, this should emit a warning without a fix. + // +optional + // +default="value" + MissingOmitEmpty string `json:"missingOmitEmpty"` // want "field D.MissingOmitEmpty has a default value but does not have omitempty in its json tag" + + // StructFieldMissingOmitZero is a struct field with default but missing omitzero. + // OmitZero policy is still SuggestFix by default, so this should suggest a fix. + // +optional + // +default={} + StructFieldMissingOmitZero NestedStruct `json:"structFieldMissingOmitZero,omitempty,omitzero"` // want "field D.StructFieldMissingOmitZero has a default value but does not have omitzero in its json tag" +} + +type NestedStruct struct { + // Field is a nested field. + // +optional + Field string `json:"field,omitempty"` +} diff --git a/pkg/analysis/defaults/testdata/src/e/e.go b/pkg/analysis/defaults/testdata/src/e/e.go new file mode 100644 index 00000000..6e8c273d --- /dev/null +++ b/pkg/analysis/defaults/testdata/src/e/e.go @@ -0,0 +1,34 @@ +package e + +// This test file tests the behavior when OmitEmpty.Policy is Ignore. +// When set to Ignore, the linter does not check for omitempty at all. + +type E struct { + // GoodField is correctly configured with +default, +optional, and omitempty. + // +optional + // +default="value" + GoodField string `json:"goodField,omitempty"` + + // MissingOmitEmpty has a default and optional but no omitempty. + // When policy is Ignore, this should NOT emit any warning. + // +optional + // +default="value" + MissingOmitEmpty string `json:"missingOmitEmpty"` + + // StructFieldMissingBoth is a struct field missing both omitempty and omitzero. + // When OmitEmpty policy is Ignore, only omitzero warning should be emitted. + // +optional + // +default={} + StructFieldMissingBoth NestedStruct `json:"structFieldMissingBoth"` // want "field E.StructFieldMissingBoth has a default value but does not have omitzero in its json tag" + + // StructFieldWithOmitEmpty has omitempty but missing omitzero. + // +optional + // +default={} + StructFieldWithOmitEmpty NestedStruct `json:"structFieldWithOmitEmpty,omitempty"` // want "field E.StructFieldWithOmitEmpty has a default value but does not have omitzero in its json tag" +} + +type NestedStruct struct { + // Field is a nested field. + // +optional + Field string `json:"field,omitempty"` +} diff --git a/pkg/analysis/defaults/testdata/src/e/e.go.golden b/pkg/analysis/defaults/testdata/src/e/e.go.golden new file mode 100644 index 00000000..5172979c --- /dev/null +++ b/pkg/analysis/defaults/testdata/src/e/e.go.golden @@ -0,0 +1,34 @@ +package e + +// This test file tests the behavior when OmitEmpty.Policy is Ignore. +// When set to Ignore, the linter does not check for omitempty at all. + +type E struct { + // GoodField is correctly configured with +default, +optional, and omitempty. + // +optional + // +default="value" + GoodField string `json:"goodField,omitempty"` + + // MissingOmitEmpty has a default and optional but no omitempty. + // When policy is Ignore, this should NOT emit any warning. + // +optional + // +default="value" + MissingOmitEmpty string `json:"missingOmitEmpty"` + + // StructFieldMissingBoth is a struct field missing both omitempty and omitzero. + // When OmitEmpty policy is Ignore, only omitzero warning should be emitted. + // +optional + // +default={} + StructFieldMissingBoth NestedStruct `json:"structFieldMissingBoth,omitzero"` // want "field E.StructFieldMissingBoth has a default value but does not have omitzero in its json tag" + + // StructFieldWithOmitEmpty has omitempty but missing omitzero. + // +optional + // +default={} + StructFieldWithOmitEmpty NestedStruct `json:"structFieldWithOmitEmpty,omitempty,omitzero"` // want "field E.StructFieldWithOmitEmpty has a default value but does not have omitzero in its json tag" +} + +type NestedStruct struct { + // Field is a nested field. + // +optional + Field string `json:"field,omitempty"` +} diff --git a/pkg/analysis/defaults/testdata/src/f/f.go b/pkg/analysis/defaults/testdata/src/f/f.go new file mode 100644 index 00000000..5b0dffa5 --- /dev/null +++ b/pkg/analysis/defaults/testdata/src/f/f.go @@ -0,0 +1,34 @@ +package f + +// This test file tests the behavior when OmitZero.Policy is Warn. +// When set to Warn, the linter emits a warning for struct fields without omitzero but does not suggest a fix. + +type F struct { + // GoodField is correctly configured with +default, +optional, and omitempty. + // +optional + // +default="value" + GoodField string `json:"goodField,omitempty"` + + // MissingOmitEmpty has a default and optional but no omitempty. + // OmitEmpty policy is still SuggestFix by default. + // +optional + // +default="value" + MissingOmitEmpty string `json:"missingOmitEmpty"` // want "field F.MissingOmitEmpty has a default value but does not have omitempty in its json tag" + + // StructFieldMissingOmitZero is a struct field with default but missing omitzero. + // When OmitZero policy is Warn, this should emit a warning without a fix. + // +optional + // +default={} + StructFieldMissingOmitZero NestedStruct `json:"structFieldMissingOmitZero,omitempty"` // want "field F.StructFieldMissingOmitZero has a default value but does not have omitzero in its json tag" + + // StructFieldWithBothOmit is correctly configured with omitempty and omitzero. + // +optional + // +default={} + StructFieldWithBothOmit NestedStruct `json:"structFieldWithBothOmit,omitempty,omitzero"` +} + +type NestedStruct struct { + // Field is a nested field. + // +optional + Field string `json:"field,omitempty"` +} diff --git a/pkg/analysis/defaults/testdata/src/f/f.go.golden b/pkg/analysis/defaults/testdata/src/f/f.go.golden new file mode 100644 index 00000000..6124d19c --- /dev/null +++ b/pkg/analysis/defaults/testdata/src/f/f.go.golden @@ -0,0 +1,34 @@ +package f + +// This test file tests the behavior when OmitZero.Policy is Warn. +// When set to Warn, the linter emits a warning for struct fields without omitzero but does not suggest a fix. + +type F struct { + // GoodField is correctly configured with +default, +optional, and omitempty. + // +optional + // +default="value" + GoodField string `json:"goodField,omitempty"` + + // MissingOmitEmpty has a default and optional but no omitempty. + // OmitEmpty policy is still SuggestFix by default. + // +optional + // +default="value" + MissingOmitEmpty string `json:"missingOmitEmpty,omitempty"` // want "field F.MissingOmitEmpty has a default value but does not have omitempty in its json tag" + + // StructFieldMissingOmitZero is a struct field with default but missing omitzero. + // When OmitZero policy is Warn, this should emit a warning without a fix. + // +optional + // +default={} + StructFieldMissingOmitZero NestedStruct `json:"structFieldMissingOmitZero,omitempty"` // want "field F.StructFieldMissingOmitZero has a default value but does not have omitzero in its json tag" + + // StructFieldWithBothOmit is correctly configured with omitempty and omitzero. + // +optional + // +default={} + StructFieldWithBothOmit NestedStruct `json:"structFieldWithBothOmit,omitempty,omitzero"` +} + +type NestedStruct struct { + // Field is a nested field. + // +optional + Field string `json:"field,omitempty"` +} diff --git a/pkg/markers/markers.go b/pkg/markers/markers.go index 300303c6..e275419b 100644 --- a/pkg/markers/markers.go +++ b/pkg/markers/markers.go @@ -205,4 +205,7 @@ const ( // K8sListMapKeyMarker is the marker that indicates that a field is a map in k8s declarative validation. K8sListMapKeyMarker = "k8s:listMapKey" + + // K8sDefaultMarker is the marker that indicates the default value for a field in k8s declarative validation. + K8sDefaultMarker = "k8s:default" ) diff --git a/pkg/registration/doc.go b/pkg/registration/doc.go index c9070c7c..aec94d2a 100644 --- a/pkg/registration/doc.go +++ b/pkg/registration/doc.go @@ -28,6 +28,7 @@ import ( _ "sigs.k8s.io/kube-api-linter/pkg/analysis/conditions" _ "sigs.k8s.io/kube-api-linter/pkg/analysis/conflictingmarkers" _ "sigs.k8s.io/kube-api-linter/pkg/analysis/defaultorrequired" + _ "sigs.k8s.io/kube-api-linter/pkg/analysis/defaults" _ "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/forbiddenmarkers" diff --git a/tests/integration/testdata/default_configurations/container.go b/tests/integration/testdata/default_configurations/container.go index f9594a8d..3628739a 100644 --- a/tests/integration/testdata/default_configurations/container.go +++ b/tests/integration/testdata/default_configurations/container.go @@ -667,7 +667,7 @@ type GRPCAction struct { // If this is not specified, the default behavior is defined by gRPC. // +optional // +default="" - Service *string `json:"service" protobuf:"bytes,2,opt,name=service"` // want "optionalfields: field GRPCAction.Service should have the omitempty tag." + Service *string `json:"service" protobuf:"bytes,2,opt,name=service"` // want "optionalfields: field GRPCAction.Service should have the omitempty tag." "defaults: field GRPCAction.Service has a default value but does not have omitempty in its json tag" } // ExecAction describes a "run in container" action. diff --git a/tests/integration/testdata/default_configurations/object_references.go b/tests/integration/testdata/default_configurations/object_references.go index 5b8ad4db..a6af8582 100644 --- a/tests/integration/testdata/default_configurations/object_references.go +++ b/tests/integration/testdata/default_configurations/object_references.go @@ -80,7 +80,7 @@ type LocalObjectReference struct { // +default="" // +kubebuilder:default="" // TODO: Drop `kubebuilder:default` when controller-gen doesn't need it https://github.com/kubernetes-sigs/kubebuilder/issues/3896. - Name string `json:"name,omitempty" protobuf:"bytes,1,opt,name=name"` // want "optionalfields: field LocalObjectReference.Name should be a pointer." + Name string `json:"name,omitempty" protobuf:"bytes,1,opt,name=name"` // want "optionalfields: field LocalObjectReference.Name should be a pointer." "defaults: field LocalObjectReference.Name should use only the marker \\+default, \\+kubebuilder:default is not required" } // TypedLocalObjectReference contains enough information to let you locate the diff --git a/tests/integration/testdata/default_configurations/volume.go b/tests/integration/testdata/default_configurations/volume.go index cd17a81f..fb8a26e9 100644 --- a/tests/integration/testdata/default_configurations/volume.go +++ b/tests/integration/testdata/default_configurations/volume.go @@ -1233,7 +1233,7 @@ type AzureDiskVolumeSource struct { ReadOnly *bool `json:"readOnly,omitempty" protobuf:"varint,5,opt,name=readOnly"` // kind expected values are Shared: multiple blob disks per storage account Dedicated: single blob disk per storage account Managed: azure managed data disk (only in managed availability set). defaults to shared // +default=ref(AzureSharedBlobDisk) - Kind *AzureDataDiskKind `json:"kind,omitempty" protobuf:"bytes,6,opt,name=kind,casttype=AzureDataDiskKind"` // want "optionalorrequired: field AzureDiskVolumeSource.Kind must be marked as optional or required" + Kind *AzureDataDiskKind `json:"kind,omitempty" protobuf:"bytes,6,opt,name=kind,casttype=AzureDataDiskKind"` // want "optionalorrequired: field AzureDiskVolumeSource.Kind must be marked as optional or required" "defaults: field AzureDiskVolumeSource.Kind has a default value but is not marked as optional" } // PortworxVolumeSource represents a Portworx volume resource.