From 60896da2d308e801f9c708e09564d9a9d75dd12f Mon Sep 17 00:00:00 2001 From: yongruilin Date: Tue, 29 Apr 2025 18:10:30 +0000 Subject: [PATCH] Add statusoptional linter This commit introduces a new linter, `statusoptional`, which checks that all first-level children fields within a status struct are marked as optional. It adds functionality to automatically suggest fixes for fields that are missing the appropriate markers. --- README.md | 14 + go.sum | 2 - pkg/analysis/registry.go | 2 + pkg/analysis/registry_test.go | 2 + pkg/analysis/statusoptional/analyzer.go | 260 ++++++++++++++++++ pkg/analysis/statusoptional/analyzer_test.go | 38 +++ pkg/analysis/statusoptional/doc.go | 32 +++ pkg/analysis/statusoptional/initializer.go | 46 ++++ .../statusoptional/testdata/src/a/a.go | 79 ++++++ .../statusoptional/testdata/src/a/a.go.golden | 83 ++++++ .../statusoptional/testdata/src/b/b.go | 79 ++++++ .../statusoptional/testdata/src/b/b.go.golden | 83 ++++++ .../statusoptional/testdata/src/c/c.go | 79 ++++++ .../statusoptional/testdata/src/c/c.go.golden | 83 ++++++ pkg/config/linters_config.go | 11 + pkg/validation/linters_config.go | 14 + pkg/validation/linters_config_test.go | 33 +++ 17 files changed, 938 insertions(+), 2 deletions(-) create mode 100644 pkg/analysis/statusoptional/analyzer.go create mode 100644 pkg/analysis/statusoptional/analyzer_test.go create mode 100644 pkg/analysis/statusoptional/doc.go create mode 100644 pkg/analysis/statusoptional/initializer.go create mode 100644 pkg/analysis/statusoptional/testdata/src/a/a.go create mode 100644 pkg/analysis/statusoptional/testdata/src/a/a.go.golden create mode 100644 pkg/analysis/statusoptional/testdata/src/b/b.go create mode 100644 pkg/analysis/statusoptional/testdata/src/b/b.go.golden create mode 100644 pkg/analysis/statusoptional/testdata/src/c/c.go create mode 100644 pkg/analysis/statusoptional/testdata/src/c/c.go.golden diff --git a/README.md b/README.md index 924f711b..5c7f65c6 100644 --- a/README.md +++ b/README.md @@ -310,6 +310,20 @@ It will suggest to remove the pointer from the field, and update the `json` tag If you prefer not to suggest fixes for pointers in required fields, you can change the `pointerPolicy` to `Warn`. The linter will then only suggest to remove the `omitempty` value from the `json` tag. +## StatusOptional + +The `statusoptional` linter checks that all first-level children fields within a status struct are marked as optional. + +This is important because status fields should be optional to allow for partial updates and backward compatibility. +The linter ensures that all direct child fields of any status struct have either the `// +optional` or +`// +kubebuilder:validation:Optional` marker. + +### Fixes + +The `statusoptional` linter can automatically fix fields in status structs that are not marked as optional. + +It will suggest adding the `// +optional` marker to any status field that is missing it. + ## StatusSubresource The `statussubresource` linter checks that the status subresource is configured correctly for diff --git a/go.sum b/go.sum index 2e08b9c6..826e30ec 100644 --- a/go.sum +++ b/go.sum @@ -5,8 +5,6 @@ github.com/go-logr/logr v1.4.2 h1:6pFjapn8bFcIbiKo3XT4j/BhANplGihG6tvd+8rYgrY= github.com/go-logr/logr v1.4.2/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY= github.com/go-task/slim-sprig/v3 v3.0.0 h1:sUs3vkvUymDpBKi3qH1YSqBQk9+9D/8M2mN1vB6EwHI= github.com/go-task/slim-sprig/v3 v3.0.0/go.mod h1:W848ghGpv3Qj3dhTPRyJypKRiqCdHZiAzKg9hl15HA8= -github.com/golangci/golangci-lint/v2 v2.0.0 h1:RQWk8VCuMQv9bBDy3x23yds2yf9aRZ86C9MWGIdNRuU= -github.com/golangci/golangci-lint/v2 v2.0.0/go.mod h1:ptNNMeGBQrbves0Qq38xvfdJg18PzxmT+7KRCOpm6i8= github.com/golangci/plugin-module-register v0.1.1 h1:TCmesur25LnyJkpsVrupv1Cdzo+2f7zX0H6Jkw1Ol6c= github.com/golangci/plugin-module-register v0.1.1/go.mod h1:TTpqoB6KkwOJMV8u7+NyXMrkwwESJLOkfl9TxR1DGFc= github.com/google/go-cmp v0.7.0 h1:wk8382ETsv4JYUZwIsn6YpYiWiBsYLSJiTsyBybVuN8= diff --git a/pkg/analysis/registry.go b/pkg/analysis/registry.go index db51b18e..1abb0ca6 100644 --- a/pkg/analysis/registry.go +++ b/pkg/analysis/registry.go @@ -30,6 +30,7 @@ import ( "sigs.k8s.io/kube-api-linter/pkg/analysis/nophase" "sigs.k8s.io/kube-api-linter/pkg/analysis/optionalorrequired" "sigs.k8s.io/kube-api-linter/pkg/analysis/requiredfields" + "sigs.k8s.io/kube-api-linter/pkg/analysis/statusoptional" "sigs.k8s.io/kube-api-linter/pkg/analysis/statussubresource" "sigs.k8s.io/kube-api-linter/pkg/config" @@ -83,6 +84,7 @@ func NewRegistry() Registry { nophase.Initializer(), optionalorrequired.Initializer(), requiredfields.Initializer(), + statusoptional.Initializer(), statussubresource.Initializer(), }, } diff --git a/pkg/analysis/registry_test.go b/pkg/analysis/registry_test.go index 446f7cfd..43497a0c 100644 --- a/pkg/analysis/registry_test.go +++ b/pkg/analysis/registry_test.go @@ -40,6 +40,7 @@ var _ = Describe("Registry", func() { "nophase", "optionalorrequired", "requiredfields", + "statusoptional", )) }) }) @@ -59,6 +60,7 @@ var _ = Describe("Registry", func() { "nophase", "optionalorrequired", "requiredfields", + "statusoptional", "statussubresource", )) }) diff --git a/pkg/analysis/statusoptional/analyzer.go b/pkg/analysis/statusoptional/analyzer.go new file mode 100644 index 00000000..0f381f2c --- /dev/null +++ b/pkg/analysis/statusoptional/analyzer.go @@ -0,0 +1,260 @@ +/* +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 statusoptional + +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/markers" +) + +const ( + name = "statusoptional" + + statusJSONTag = "status" +) + +func init() { + markershelper.DefaultRegistry().Register( + markers.OptionalMarker, + markers.KubebuilderOptionalMarker, + markers.K8sOptionalMarker, + markers.RequiredMarker, + markers.KubebuilderRequiredMarker, + markers.K8sRequiredMarker, + ) +} + +type analyzer struct { + preferredOptionalMarker string +} + +// newAnalyzer creates a new analyzer. +func newAnalyzer(preferredOptionalMarker string) *analysis.Analyzer { + if preferredOptionalMarker == "" { + preferredOptionalMarker = markers.OptionalMarker + } + + a := &analyzer{ + preferredOptionalMarker: preferredOptionalMarker, + } + + return &analysis.Analyzer{ + Name: name, + Doc: "Checks that all first-level children fields within status struct are marked as optional", + Run: a.run, + Requires: []*analysis.Analyzer{inspector.Analyzer, extractjsontags.Analyzer}, + } +} + +func (a *analyzer) run(pass *analysis.Pass) (any, error) { + inspect, ok := pass.ResultOf[inspector.Analyzer].(inspector.Inspector) + if !ok { + return nil, kalerrors.ErrCouldNotGetInspector + } + + jsonTags, ok := pass.ResultOf[extractjsontags.Analyzer].(extractjsontags.StructFieldTags) + if !ok { + return nil, kalerrors.ErrCouldNotGetJSONTags + } + + inspect.InspectFields(func(field *ast.Field, stack []ast.Node, jsonTagInfo extractjsontags.FieldTagInfo, markersAccess markershelper.Markers) { + if jsonTagInfo.Name != statusJSONTag { + return + } + + statusStructType := getStructFromField(field) + if statusStructType == nil { + return + } + + a.checkStatusStruct(pass, statusStructType, markersAccess, jsonTags) + }) + + return nil, nil //nolint:nilnil +} + +func (a *analyzer) checkStatusStruct(pass *analysis.Pass, statusType *ast.StructType, markersAccess markershelper.Markers, jsonTags extractjsontags.StructFieldTags) { + if statusType.Fields == nil || statusType.Fields.List == nil { + return + } + + // Check each child field of the status struct + for _, childField := range statusType.Fields.List { + fieldName := utils.FieldName(childField) + jsonTagInfo := jsonTags.FieldTags(childField) + + switch { + case fieldName == "", jsonTagInfo.Ignored: + // Skip fields that are ignored or have no name + case jsonTagInfo.Inline: + if len(childField.Names) > 0 { + // Inline fields should not have names + continue + } + // Check embedded structs recursively + a.checkStatusStruct(pass, getStructFromField(childField), markersAccess, jsonTags) + default: + // Check if the field has the required optional markers + a.checkFieldOptionalMarker(pass, childField, fieldName, markersAccess) + } + } +} + +// checkFieldOptionalMarker checks if a field has the required optional markers. +// If the field has a required marker, it will be replaced with the preferred optional marker. +// If the field does not have an optional marker, it will be added. +func (a *analyzer) checkFieldOptionalMarker(pass *analysis.Pass, field *ast.Field, fieldName string, markersAccess markershelper.Markers) { + fieldMarkers := markersAccess.FieldMarkers(field) + + // Check if the field has either the optional or kubebuilder:validation:Optional marker + if hasOptionalMarker(fieldMarkers) { + return + } + + // Check if the field has required markers that need to be replaced + if hasRequiredMarker(fieldMarkers) { + a.reportAndReplaceRequiredMarkers(pass, field, fieldName, fieldMarkers) + } else { + // Report the error and suggest a fix to add the optional marker + a.reportAndAddOptionalMarker(pass, field, fieldName) + } +} + +// hasOptionalMarker checks if a field has any optional marker. +func hasOptionalMarker(fieldMarkers markershelper.MarkerSet) bool { + return fieldMarkers.Has(markers.OptionalMarker) || + fieldMarkers.Has(markers.KubebuilderOptionalMarker) || + fieldMarkers.Has(markers.K8sOptionalMarker) +} + +// hasRequiredMarker checks if a field has any required marker. +func hasRequiredMarker(fieldMarkers markershelper.MarkerSet) bool { + return fieldMarkers.Has(markers.RequiredMarker) || + fieldMarkers.Has(markers.KubebuilderRequiredMarker) || + fieldMarkers.Has(markers.K8sRequiredMarker) +} + +// reportAndReplaceRequiredMarkers reports an error and suggests replacing required markers with optional ones. +func (a *analyzer) reportAndReplaceRequiredMarkers(pass *analysis.Pass, field *ast.Field, fieldName string, fieldMarkers markershelper.MarkerSet) { + textEdits := createMarkerRemovalEdits(fieldMarkers) + + // Add the preferred optional marker at the beginning of the field + textEdits = append(textEdits, analysis.TextEdit{ + Pos: field.Pos(), + NewText: fmt.Appendf(nil, "// +%s\n", a.preferredOptionalMarker), + }) + + pass.Report(analysis.Diagnostic{ + Pos: field.Pos(), + Message: fmt.Sprintf("status field %q must be marked as optional, not required", fieldName), + SuggestedFixes: []analysis.SuggestedFix{{ + Message: fmt.Sprintf("replace required marker(s) with %s", a.preferredOptionalMarker), + TextEdits: textEdits, + }}, + }) +} + +// reportAndAddOptionalMarker reports an error and suggests adding an optional marker. +// TODO: consolidate the logic for removing markers with other linters. +func (a *analyzer) reportAndAddOptionalMarker(pass *analysis.Pass, field *ast.Field, fieldName string) { + pass.Report(analysis.Diagnostic{ + Pos: field.Pos(), + Message: fmt.Sprintf("status field %q must be marked as optional", fieldName), + SuggestedFixes: []analysis.SuggestedFix{ + { + Message: "add the optional marker", + TextEdits: []analysis.TextEdit{ + { + // Position at the beginning of the line of the field + Pos: field.Pos(), + // Insert the marker before the field + NewText: fmt.Appendf(nil, "// +%s\n", a.preferredOptionalMarker), + }, + }, + }, + }, + }) +} + +// createMarkerRemovalEdits creates text edits to remove required markers. +// TODO: consolidate the logic for removing markers with other linters. +func createMarkerRemovalEdits(fieldMarkers markershelper.MarkerSet) []analysis.TextEdit { + var textEdits []analysis.TextEdit + + // Handle standard required markers + if fieldMarkers.Has(markers.RequiredMarker) { + for _, marker := range fieldMarkers[markers.RequiredMarker] { + textEdits = append(textEdits, analysis.TextEdit{ + Pos: marker.Pos, + End: marker.End + 1, + NewText: []byte(""), + }) + } + } + + // Handle kubebuilder required markers + if fieldMarkers.Has(markers.KubebuilderRequiredMarker) { + for _, marker := range fieldMarkers[markers.KubebuilderRequiredMarker] { + textEdits = append(textEdits, analysis.TextEdit{ + Pos: marker.Pos, + End: marker.End + 1, + NewText: []byte(""), + }) + } + } + + // Handle k8s required markers + if fieldMarkers.Has(markers.K8sRequiredMarker) { + for _, marker := range fieldMarkers[markers.K8sRequiredMarker] { + textEdits = append(textEdits, analysis.TextEdit{ + Pos: marker.Pos, + End: marker.End + 1, + NewText: []byte(""), + }) + } + } + + return textEdits +} + +// getStructFromField extracts the struct type from an AST Field. +func getStructFromField(field *ast.Field) *ast.StructType { + ident, ok := field.Type.(*ast.Ident) + if !ok { + return nil + } + + typeSpec, ok := ident.Obj.Decl.(*ast.TypeSpec) + if !ok { + return nil + } + + structType, ok := typeSpec.Type.(*ast.StructType) + if !ok { + return nil + } + + return structType +} diff --git a/pkg/analysis/statusoptional/analyzer_test.go b/pkg/analysis/statusoptional/analyzer_test.go new file mode 100644 index 00000000..8abb94a7 --- /dev/null +++ b/pkg/analysis/statusoptional/analyzer_test.go @@ -0,0 +1,38 @@ +/* +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 statusoptional + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + "sigs.k8s.io/kube-api-linter/pkg/markers" +) + +func Test(t *testing.T) { + testdata := analysistest.TestData() + analysistest.RunWithSuggestedFixes(t, testdata, newAnalyzer(markers.OptionalMarker), "a") +} + +func TestWithKubebuilderOptionalMarker(t *testing.T) { + testdata := analysistest.TestData() + analysistest.RunWithSuggestedFixes(t, testdata, newAnalyzer(markers.KubebuilderOptionalMarker), "b") +} + +func TestWithK8sOptionalMarker(t *testing.T) { + testdata := analysistest.TestData() + analysistest.RunWithSuggestedFixes(t, testdata, newAnalyzer(markers.K8sOptionalMarker), "c") +} diff --git a/pkg/analysis/statusoptional/doc.go b/pkg/analysis/statusoptional/doc.go new file mode 100644 index 00000000..4aca9213 --- /dev/null +++ b/pkg/analysis/statusoptional/doc.go @@ -0,0 +1,32 @@ +/* +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. +*/ + +/* +The statusoptional linter ensures that all first-level children fields within a status struct +are marked as optional. + +This is important because status fields should be optional to allow for partial updates +and backward compatibility. + +This linter checks: +1. For structs with a JSON tag of "status" +2. All direct child fields of the status struct +3. Ensures each child field has an optional marker + +The linter will report an issue if any field in the status struct is not marked as optional +and will suggest a fix to add the appropriate optional marker. +*/ +package statusoptional diff --git a/pkg/analysis/statusoptional/initializer.go b/pkg/analysis/statusoptional/initializer.go new file mode 100644 index 00000000..cb0be745 --- /dev/null +++ b/pkg/analysis/statusoptional/initializer.go @@ -0,0 +1,46 @@ +/* +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 statusoptional + +import ( + "golang.org/x/tools/go/analysis" + + "sigs.k8s.io/kube-api-linter/pkg/config" +) + +// Initializer returns the AnalyzerInitializer for this +// Analyzer so that it can be added to the registry. +func Initializer() initializer { + return initializer{} +} + +// initializer implements the AnalyzerInitializer interface. +type initializer struct{} + +// Name returns the name of the Analyzer. +func (initializer) Name() string { + return name +} + +// Init returns the initialized Analyzer. +func (initializer) Init(cfg config.LintersConfig) (*analysis.Analyzer, error) { + return newAnalyzer(cfg.StatusOptional.PreferredOptionalMarker), nil +} + +// Default determines whether this Analyzer is on by default, or not. +func (initializer) Default() bool { + return true +} diff --git a/pkg/analysis/statusoptional/testdata/src/a/a.go b/pkg/analysis/statusoptional/testdata/src/a/a.go new file mode 100644 index 00000000..71f14845 --- /dev/null +++ b/pkg/analysis/statusoptional/testdata/src/a/a.go @@ -0,0 +1,79 @@ +package a + +// Different embedding scenarios +type ResourceWithEmbeddings struct { + Status StatusWithEmbeddings `json:"status"` +} + +type StatusWithEmbeddings struct { + // Regular inlined embed + InlineEmbed `json:",inline"` + + // Non-inlined embed + NonInlineEmbed `json:"nonInlineEmbed"` // want "status field \"NonInlineEmbed\" must be marked as optional" + + // Non-inlined embed with omitempty + NonInlineOmitEmptyEmbed `json:"nonInlineOmitEmpty,omitempty"` // want "status field \"NonInlineOmitEmptyEmbed\" must be marked as optional" + + // Pointer to non-inlined embed + *PointerEmbed `json:"pointerEmbed"` // want "status field \"PointerEmbed\" must be marked as optional" + + // Pointer to non-inlined embed with omitempty + *PointerOmitEmptyEmbed `json:"pointerOmitEmpty,omitempty"` // want "status field \"PointerOmitEmptyEmbed\" must be marked as optional" +} + +type InlineEmbed struct { + InlineField string `json:"inlineField"` // want "status field \"InlineField\" must be marked as optional" +} + +type NonInlineEmbed struct { + NonInlineField string `json:"nonInlineField"` +} + +type NonInlineOmitEmptyEmbed struct { + NonInlineOmitEmptyField string `json:"nonInlineOmitEmptyField"` +} + +type PointerEmbed struct { + PointerField string `json:"pointerField"` +} + +type PointerOmitEmptyEmbed struct { + PointerOmitEmptyField string `json:"pointerOmitEmptyField"` +} + +type NonInlineOmitZeroEmbed struct { + NonInlineOmitZeroField string `json:"nonInlineOmitZeroField"` +} + +type PointerOmitZeroEmbed struct { + PointerOmitZeroField string `json:"pointerOmitZeroField"` +} + +type ResourceWithNestedStatus struct { + Status NestedStatusStatus `json:"status"` +} + +type NestedStatusStatus struct { + // +optional + NestedStatus SecondLevelStatus `json:"nestedStatus"` +} + +type SecondLevelStatus struct { + // The required here is ignored because it is not the top-level status field. + // +required + NestedField string `json:"nestedField"` +} + +type ResourceWithStatusMarkedRequired struct { + Status StatusMarkedRequired `json:"status"` +} + +type StatusMarkedRequired struct { + // +required + OneRequiredField string `json:"oneRequiredField"` // want "status field \"OneRequiredField\" must be marked as optional" + + // +required + // +kubebuilder:validation:Required + BothRequiredField string `json:"bothRequiredField"` // want "status field \"BothRequiredField\" must be marked as optional" +} diff --git a/pkg/analysis/statusoptional/testdata/src/a/a.go.golden b/pkg/analysis/statusoptional/testdata/src/a/a.go.golden new file mode 100644 index 00000000..63ff76e2 --- /dev/null +++ b/pkg/analysis/statusoptional/testdata/src/a/a.go.golden @@ -0,0 +1,83 @@ +package a + +// Different embedding scenarios +type ResourceWithEmbeddings struct { + Status StatusWithEmbeddings `json:"status"` +} + +type StatusWithEmbeddings struct { + // Regular inlined embed + InlineEmbed `json:",inline"` + + // Non-inlined embed + // +optional + NonInlineEmbed `json:"nonInlineEmbed"` // want "status field \"NonInlineEmbed\" must be marked as optional" + + // Non-inlined embed with omitempty + // +optional + NonInlineOmitEmptyEmbed `json:"nonInlineOmitEmpty,omitempty"` // want "status field \"NonInlineOmitEmptyEmbed\" must be marked as optional" + + // Pointer to non-inlined embed + // +optional + *PointerEmbed `json:"pointerEmbed"` // want "status field \"PointerEmbed\" must be marked as optional" + + // Pointer to non-inlined embed with omitempty + // +optional + *PointerOmitEmptyEmbed `json:"pointerOmitEmpty,omitempty"` // want "status field \"PointerOmitEmptyEmbed\" must be marked as optional" +} + +type InlineEmbed struct { + // +optional + InlineField string `json:"inlineField"` // want "status field \"InlineField\" must be marked as optional" +} + +type NonInlineEmbed struct { + NonInlineField string `json:"nonInlineField"` +} + +type NonInlineOmitEmptyEmbed struct { + NonInlineOmitEmptyField string `json:"nonInlineOmitEmptyField"` +} + +type PointerEmbed struct { + PointerField string `json:"pointerField"` +} + +type PointerOmitEmptyEmbed struct { + PointerOmitEmptyField string `json:"pointerOmitEmptyField"` +} + +type NonInlineOmitZeroEmbed struct { + NonInlineOmitZeroField string `json:"nonInlineOmitZeroField"` +} + +type PointerOmitZeroEmbed struct { + PointerOmitZeroField string `json:"pointerOmitZeroField"` +} + +type ResourceWithNestedStatus struct { + Status NestedStatusStatus `json:"status"` +} + +type NestedStatusStatus struct { + // +optional + NestedStatus SecondLevelStatus `json:"nestedStatus"` +} + +type SecondLevelStatus struct { + // The required here is ignored because it is not the top-level status field. + // +required + NestedField string `json:"nestedField"` +} + +type ResourceWithStatusMarkedRequired struct { + Status StatusMarkedRequired `json:"status"` +} + +type StatusMarkedRequired struct { + // +optional + OneRequiredField string `json:"oneRequiredField"` // want "status field \"OneRequiredField\" must be marked as optional" + + // +optional + BothRequiredField string `json:"bothRequiredField"` // want "status field \"BothRequiredField\" must be marked as optional" +} diff --git a/pkg/analysis/statusoptional/testdata/src/b/b.go b/pkg/analysis/statusoptional/testdata/src/b/b.go new file mode 100644 index 00000000..8387e753 --- /dev/null +++ b/pkg/analysis/statusoptional/testdata/src/b/b.go @@ -0,0 +1,79 @@ +package b + +// Different embedding scenarios +type ResourceWithEmbeddings struct { + Status StatusWithEmbeddings `json:"status"` +} + +type StatusWithEmbeddings struct { + // Regular inlined embed + InlineEmbed `json:",inline"` + + // Non-inlined embed + NonInlineEmbed `json:"nonInlineEmbed"` // want "status field \"NonInlineEmbed\" must be marked as optional" + + // Non-inlined embed with omitempty + NonInlineOmitEmptyEmbed `json:"nonInlineOmitEmpty,omitempty"` // want "status field \"NonInlineOmitEmptyEmbed\" must be marked as optional" + + // Pointer to non-inlined embed + *PointerEmbed `json:"pointerEmbed"` // want "status field \"PointerEmbed\" must be marked as optional" + + // Pointer to non-inlined embed with omitempty + *PointerOmitEmptyEmbed `json:"pointerOmitEmpty,omitempty"` // want "status field \"PointerOmitEmptyEmbed\" must be marked as optional" +} + +type InlineEmbed struct { + InlineField string `json:"inlineField"` // want "status field \"InlineField\" must be marked as optional" +} + +type NonInlineEmbed struct { + NonInlineField string `json:"nonInlineField"` +} + +type NonInlineOmitEmptyEmbed struct { + NonInlineOmitEmptyField string `json:"nonInlineOmitEmptyField"` +} + +type PointerEmbed struct { + PointerField string `json:"pointerField"` +} + +type PointerOmitEmptyEmbed struct { + PointerOmitEmptyField string `json:"pointerOmitEmptyField"` +} + +type NonInlineOmitZeroEmbed struct { + NonInlineOmitZeroField string `json:"nonInlineOmitZeroField"` +} + +type PointerOmitZeroEmbed struct { + PointerOmitZeroField string `json:"pointerOmitZeroField"` +} + +type ResourceWithNestedStatus struct { + Status NestedStatusStatus `json:"status"` +} + +type NestedStatusStatus struct { + // +kubebuilder:validation:Optional + NestedStatus SecondLevelStatus `json:"nestedStatus"` +} + +type SecondLevelStatus struct { + // The required here is ignored because it is not the top-level status field. + // +required + NestedField string `json:"nestedField"` +} + +type ResourceWithStatusMarkedRequired struct { + Status StatusMarkedRequired `json:"status"` +} + +type StatusMarkedRequired struct { + // +required + OneRequiredField string `json:"oneRequiredField"` // want "status field \"OneRequiredField\" must be marked as optional" + + // +required + // +kubebuilder:validation:Required + BothRequiredField string `json:"bothRequiredField"` // want "status field \"BothRequiredField\" must be marked as optional" +} diff --git a/pkg/analysis/statusoptional/testdata/src/b/b.go.golden b/pkg/analysis/statusoptional/testdata/src/b/b.go.golden new file mode 100644 index 00000000..a746227f --- /dev/null +++ b/pkg/analysis/statusoptional/testdata/src/b/b.go.golden @@ -0,0 +1,83 @@ +package b + +// Different embedding scenarios +type ResourceWithEmbeddings struct { + Status StatusWithEmbeddings `json:"status"` +} + +type StatusWithEmbeddings struct { + // Regular inlined embed + InlineEmbed `json:",inline"` + + // Non-inlined embed + // +kubebuilder:validation:Optional + NonInlineEmbed `json:"nonInlineEmbed"` // want "status field \"NonInlineEmbed\" must be marked as optional" + + // Non-inlined embed with omitempty + // +kubebuilder:validation:Optional + NonInlineOmitEmptyEmbed `json:"nonInlineOmitEmpty,omitempty"` // want "status field \"NonInlineOmitEmptyEmbed\" must be marked as optional" + + // Pointer to non-inlined embed + // +kubebuilder:validation:Optional + *PointerEmbed `json:"pointerEmbed"` // want "status field \"PointerEmbed\" must be marked as optional" + + // Pointer to non-inlined embed with omitempty + // +kubebuilder:validation:Optional + *PointerOmitEmptyEmbed `json:"pointerOmitEmpty,omitempty"` // want "status field \"PointerOmitEmptyEmbed\" must be marked as optional" +} + +type InlineEmbed struct { + // +kubebuilder:validation:Optional + InlineField string `json:"inlineField"` // want "status field \"InlineField\" must be marked as optional" +} + +type NonInlineEmbed struct { + NonInlineField string `json:"nonInlineField"` +} + +type NonInlineOmitEmptyEmbed struct { + NonInlineOmitEmptyField string `json:"nonInlineOmitEmptyField"` +} + +type PointerEmbed struct { + PointerField string `json:"pointerField"` +} + +type PointerOmitEmptyEmbed struct { + PointerOmitEmptyField string `json:"pointerOmitEmptyField"` +} + +type NonInlineOmitZeroEmbed struct { + NonInlineOmitZeroField string `json:"nonInlineOmitZeroField"` +} + +type PointerOmitZeroEmbed struct { + PointerOmitZeroField string `json:"pointerOmitZeroField"` +} + +type ResourceWithNestedStatus struct { + Status NestedStatusStatus `json:"status"` +} + +type NestedStatusStatus struct { + // +kubebuilder:validation:Optional + NestedStatus SecondLevelStatus `json:"nestedStatus"` +} + +type SecondLevelStatus struct { + // The required here is ignored because it is not the top-level status field. + // +required + NestedField string `json:"nestedField"` +} + +type ResourceWithStatusMarkedRequired struct { + Status StatusMarkedRequired `json:"status"` +} + +type StatusMarkedRequired struct { + // +kubebuilder:validation:Optional + OneRequiredField string `json:"oneRequiredField"` // want "status field \"OneRequiredField\" must be marked as optional" + + // +kubebuilder:validation:Optional + BothRequiredField string `json:"bothRequiredField"` // want "status field \"BothRequiredField\" must be marked as optional" +} diff --git a/pkg/analysis/statusoptional/testdata/src/c/c.go b/pkg/analysis/statusoptional/testdata/src/c/c.go new file mode 100644 index 00000000..c3cb426d --- /dev/null +++ b/pkg/analysis/statusoptional/testdata/src/c/c.go @@ -0,0 +1,79 @@ +package c + +// Different embedding scenarios +type ResourceWithEmbeddings struct { + Status StatusWithEmbeddings `json:"status"` +} + +type StatusWithEmbeddings struct { + // Regular inlined embed + InlineEmbed `json:",inline"` + + // Non-inlined embed + NonInlineEmbed `json:"nonInlineEmbed"` // want "status field \"NonInlineEmbed\" must be marked as optional" + + // Non-inlined embed with omitempty + NonInlineOmitEmptyEmbed `json:"nonInlineOmitEmpty,omitempty"` // want "status field \"NonInlineOmitEmptyEmbed\" must be marked as optional" + + // Pointer to non-inlined embed + *PointerEmbed `json:"pointerEmbed"` // want "status field \"PointerEmbed\" must be marked as optional" + + // Pointer to non-inlined embed with omitempty + *PointerOmitEmptyEmbed `json:"pointerOmitEmpty,omitempty"` // want "status field \"PointerOmitEmptyEmbed\" must be marked as optional" +} + +type InlineEmbed struct { + InlineField string `json:"inlineField"` // want "status field \"InlineField\" must be marked as optional" +} + +type NonInlineEmbed struct { + NonInlineField string `json:"nonInlineField"` +} + +type NonInlineOmitEmptyEmbed struct { + NonInlineOmitEmptyField string `json:"nonInlineOmitEmptyField"` +} + +type PointerEmbed struct { + PointerField string `json:"pointerField"` +} + +type PointerOmitEmptyEmbed struct { + PointerOmitEmptyField string `json:"pointerOmitEmptyField"` +} + +type NonInlineOmitZeroEmbed struct { + NonInlineOmitZeroField string `json:"nonInlineOmitZeroField"` +} + +type PointerOmitZeroEmbed struct { + PointerOmitZeroField string `json:"pointerOmitZeroField"` +} + +type ResourceWithNestedStatus struct { + Status NestedStatusStatus `json:"status"` +} + +type NestedStatusStatus struct { + // +k8s:optional + NestedStatus SecondLevelStatus `json:"nestedStatus"` +} + +type SecondLevelStatus struct { + // The required here is ignored because it is not the top-level status field. + // +required + NestedField string `json:"nestedField"` +} + +type ResourceWithStatusMarkedRequired struct { + Status StatusMarkedRequired `json:"status"` +} + +type StatusMarkedRequired struct { + // +required + OneRequiredField string `json:"oneRequiredField"` // want "status field \"OneRequiredField\" must be marked as optional" + + // +required + // +kubebuilder:validation:Required + BothRequiredField string `json:"bothRequiredField"` // want "status field \"BothRequiredField\" must be marked as optional" +} diff --git a/pkg/analysis/statusoptional/testdata/src/c/c.go.golden b/pkg/analysis/statusoptional/testdata/src/c/c.go.golden new file mode 100644 index 00000000..adb0c317 --- /dev/null +++ b/pkg/analysis/statusoptional/testdata/src/c/c.go.golden @@ -0,0 +1,83 @@ +package c + +// Different embedding scenarios +type ResourceWithEmbeddings struct { + Status StatusWithEmbeddings `json:"status"` +} + +type StatusWithEmbeddings struct { + // Regular inlined embed + InlineEmbed `json:",inline"` + + // Non-inlined embed + // +k8s:optional + NonInlineEmbed `json:"nonInlineEmbed"` // want "status field \"NonInlineEmbed\" must be marked as optional" + + // Non-inlined embed with omitempty + // +k8s:optional + NonInlineOmitEmptyEmbed `json:"nonInlineOmitEmpty,omitempty"` // want "status field \"NonInlineOmitEmptyEmbed\" must be marked as optional" + + // Pointer to non-inlined embed + // +k8s:optional + *PointerEmbed `json:"pointerEmbed"` // want "status field \"PointerEmbed\" must be marked as optional" + + // Pointer to non-inlined embed with omitempty + // +k8s:optional + *PointerOmitEmptyEmbed `json:"pointerOmitEmpty,omitempty"` // want "status field \"PointerOmitEmptyEmbed\" must be marked as optional" +} + +type InlineEmbed struct { + // +k8s:optional + InlineField string `json:"inlineField"` // want "status field \"InlineField\" must be marked as optional" +} + +type NonInlineEmbed struct { + NonInlineField string `json:"nonInlineField"` +} + +type NonInlineOmitEmptyEmbed struct { + NonInlineOmitEmptyField string `json:"nonInlineOmitEmptyField"` +} + +type PointerEmbed struct { + PointerField string `json:"pointerField"` +} + +type PointerOmitEmptyEmbed struct { + PointerOmitEmptyField string `json:"pointerOmitEmptyField"` +} + +type NonInlineOmitZeroEmbed struct { + NonInlineOmitZeroField string `json:"nonInlineOmitZeroField"` +} + +type PointerOmitZeroEmbed struct { + PointerOmitZeroField string `json:"pointerOmitZeroField"` +} + +type ResourceWithNestedStatus struct { + Status NestedStatusStatus `json:"status"` +} + +type NestedStatusStatus struct { + // +k8s:optional + NestedStatus SecondLevelStatus `json:"nestedStatus"` +} + +type SecondLevelStatus struct { + // The required here is ignored because it is not the top-level status field. + // +required + NestedField string `json:"nestedField"` +} + +type ResourceWithStatusMarkedRequired struct { + Status StatusMarkedRequired `json:"status"` +} + +type StatusMarkedRequired struct { + // +k8s:optional + OneRequiredField string `json:"oneRequiredField"` // want "status field \"OneRequiredField\" must be marked as optional" + + // +k8s:optional + BothRequiredField string `json:"bothRequiredField"` // want "status field \"BothRequiredField\" must be marked as optional" +} diff --git a/pkg/config/linters_config.go b/pkg/config/linters_config.go index a8d83184..c42964a4 100644 --- a/pkg/config/linters_config.go +++ b/pkg/config/linters_config.go @@ -31,6 +31,9 @@ type LintersConfig struct { // requiredFields contains configuration for the requiredfields linter. RequiredFields RequiredFieldsConfig `json:"requiredFields"` + + // statusOptional contains configuration for the statusoptional linter. + StatusOptional StatusOptionalConfig `json:"statusOptional"` } // ConditionsFirstField is the policy for the conditions linter. @@ -172,3 +175,11 @@ type RequiredFieldsConfig struct { // When otherwise not specified, the default value is "SuggestFix". PointerPolicy RequiredFieldPointerPolicy `json:"pointerPolicy"` } + +// StatusOptionalConfig contains configuration for the statusoptional linter. +type StatusOptionalConfig struct { + // preferredOptionalMarker is the preferred marker to use for optional fields. + // If this field is not set, the default value is "optional". + // Valid values are "optional", "kubebuilder:validation:Optional" and "k8s:optional". + PreferredOptionalMarker string `json:"preferredOptionalMarker"` +} diff --git a/pkg/validation/linters_config.go b/pkg/validation/linters_config.go index 7834367d..3ca735e2 100644 --- a/pkg/validation/linters_config.go +++ b/pkg/validation/linters_config.go @@ -34,6 +34,7 @@ func ValidateLintersConfig(lc config.LintersConfig, fldPath *field.Path) field.E fieldErrors = append(fieldErrors, validateNoMapsConfig(lc.NoMaps, fldPath.Child("nomaps"))...) fieldErrors = append(fieldErrors, validateOptionalOrRequiredConfig(lc.OptionalOrRequired, fldPath.Child("optionalOrRequired"))...) fieldErrors = append(fieldErrors, validateRequiredFieldsConfig(lc.RequiredFields, fldPath.Child("requiredFields"))...) + fieldErrors = append(fieldErrors, validateStatusOptionalConfig(lc.StatusOptional, fldPath.Child("statusOptional"))...) return fieldErrors } @@ -109,6 +110,19 @@ func validateOptionalOrRequiredConfig(oorc config.OptionalOrRequiredConfig, fldP return fieldErrors } +// validateStatusOptionalConfig is used to validate the configuration in the config.StatusOptionalConfig struct. +func validateStatusOptionalConfig(soc config.StatusOptionalConfig, fldPath *field.Path) field.ErrorList { + fieldErrors := field.ErrorList{} + + switch soc.PreferredOptionalMarker { + case "", markers.OptionalMarker, markers.KubebuilderOptionalMarker, markers.K8sOptionalMarker: + default: + fieldErrors = append(fieldErrors, field.Invalid(fldPath.Child("preferredOptionalMarker"), soc.PreferredOptionalMarker, fmt.Sprintf("invalid value, must be one of %q, %q, %q or omitted", markers.OptionalMarker, markers.KubebuilderOptionalMarker, markers.K8sOptionalMarker))) + } + + return fieldErrors +} + // validateRequiredFieldsConfig is used to validate the configuration in the config.RequiredFieldsConfig struct. func validateRequiredFieldsConfig(rfc config.RequiredFieldsConfig, fldPath *field.Path) field.ErrorList { fieldErrors := field.ErrorList{} diff --git a/pkg/validation/linters_config_test.go b/pkg/validation/linters_config_test.go index dc5aa005..550ae998 100644 --- a/pkg/validation/linters_config_test.go +++ b/pkg/validation/linters_config_test.go @@ -290,5 +290,38 @@ var _ = Describe("LintersConfig", func() { }, expectedErr: "lintersConfig.requiredFields.pointerPolicy: Invalid value: \"invalid\": invalid value, must be one of \"Warn\", \"SuggestFix\" or omitted", }), + + // StatusOptionalConfig validation + Entry("With a valid StatusOptionalConfig", validateLintersConfigTableInput{ + config: config.LintersConfig{ + StatusOptional: config.StatusOptionalConfig{ + PreferredOptionalMarker: markers.OptionalMarker, + }, + }, + expectedErr: "", + }), + Entry("With a valid StatusOptionalConfig: k8s optional marker", validateLintersConfigTableInput{ + config: config.LintersConfig{ + StatusOptional: config.StatusOptionalConfig{ + PreferredOptionalMarker: markers.K8sOptionalMarker, + }, + }, + expectedErr: "", + }), + Entry("With a valid StatusOptionalConfig: kubebuilder optional marker", validateLintersConfigTableInput{ + config: config.LintersConfig{ + StatusOptional: config.StatusOptionalConfig{ + PreferredOptionalMarker: markers.KubebuilderOptionalMarker, + }, + }, + }), + Entry("With an invalid StatusOptionalConfig", validateLintersConfigTableInput{ + config: config.LintersConfig{ + StatusOptional: config.StatusOptionalConfig{ + PreferredOptionalMarker: "invalid", + }, + }, + expectedErr: "lintersConfig.statusOptional.preferredOptionalMarker: Invalid value: \"invalid\": invalid value, must be one of \"optional\", \"kubebuilder:validation:Optional\", \"k8s:optional\" or omitted", + }), ) })