From 69b4e2c470892153a8a39eafedd047cfa8f3b9fc Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Fri, 8 Aug 2025 13:45:35 +0100 Subject: [PATCH 1/2] Update KAL to enable new required fields policy --- .golangci.yaml | 14 + tools/go.mod | 2 +- tools/go.sum | 2 + tools/vendor/modules.txt | 3 +- .../pkg/analysis/conflictingmarkers/doc.go | 30 +-- .../pkg/analysis/optionalfields/analyzer.go | 187 ++------------ .../pkg/analysis/requiredfields/analyzer.go | 115 ++++----- .../pkg/analysis/requiredfields/config.go | 105 ++++++-- .../analysis/requiredfields/initializer.go | 45 +++- .../analysis/utils/serialization/config.go | 110 ++++++++ .../serialization/serialization_check.go | 240 ++++++++++++++++++ .../serialization}/util.go | 79 ++---- .../pkg/analysis/utils/utils.go | 30 +++ 13 files changed, 641 insertions(+), 321 deletions(-) create mode 100644 tools/vendor/sigs.k8s.io/kube-api-linter/pkg/analysis/utils/serialization/config.go create mode 100644 tools/vendor/sigs.k8s.io/kube-api-linter/pkg/analysis/utils/serialization/serialization_check.go rename tools/vendor/sigs.k8s.io/kube-api-linter/pkg/analysis/{optionalfields => utils/serialization}/util.go (65%) diff --git a/.golangci.yaml b/.golangci.yaml index c5278d9c8d2..a2a8be77cf2 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -29,6 +29,20 @@ linters: # Discoverability is for configuration APIs, generally singletons. # Refer to the API conventions for when to use discoverability (not our default stance). policy: Ignore + requiredFields: + pointers: + # This will force pointers when the field is required, but only when the zero + # value is a valid user choice, and has a semantic difference to being omitted (e.g. replicas allows 0). + policy: SuggestFix + omitempty: + # This will force omitempty on required fields. + # We do this so that the behaviour of not setting a value for the field is the same between + # both structured and unstructured clients. + policy: SuggestFix + omitzero: + # This will force omitzero on required struct fields. + # This means they can be omitted correctly and prevents the need for pointers to structs. + policy: SuggestFix uniqueMarkers: customMarkers: - identifier: "openshift:validation:FeatureGateAwareEnum" diff --git a/tools/go.mod b/tools/go.mod index 99a7420c8b1..0c330635687 100644 --- a/tools/go.mod +++ b/tools/go.mod @@ -30,7 +30,7 @@ require ( k8s.io/kube-openapi v0.0.0-20250318190949-c8a335a9a2ff k8s.io/utils v0.0.0-20250604170112-4c0f3b243397 sigs.k8s.io/controller-tools v0.15.0 - sigs.k8s.io/kube-api-linter v0.0.0-20250729132427-47bfeef6cd38 + sigs.k8s.io/kube-api-linter v0.0.0-20250808120943-48643eb2563d sigs.k8s.io/yaml v1.4.0 ) diff --git a/tools/go.sum b/tools/go.sum index 11a2784c634..d269c6125ce 100644 --- a/tools/go.sum +++ b/tools/go.sum @@ -994,6 +994,8 @@ sigs.k8s.io/kube-api-linter v0.0.0-20250723124831-1b29e82a0f55 h1:kD9x5uu1/A7wvh sigs.k8s.io/kube-api-linter v0.0.0-20250723124831-1b29e82a0f55/go.mod h1:Jxl3NU9lRf9WJ8dgwgF4U6tLF229jR/KEvtxSwRAKnE= sigs.k8s.io/kube-api-linter v0.0.0-20250729132427-47bfeef6cd38 h1:5WuFSvNbquqwM82aBQ36AfsFGsf2Jc0OJM4SCC2rw4w= sigs.k8s.io/kube-api-linter v0.0.0-20250729132427-47bfeef6cd38/go.mod h1:Jxl3NU9lRf9WJ8dgwgF4U6tLF229jR/KEvtxSwRAKnE= +sigs.k8s.io/kube-api-linter v0.0.0-20250808120943-48643eb2563d h1:BcgCRoMLmIxRTLokQ1K1LAle+21fKPqgA6OvzN04xEg= +sigs.k8s.io/kube-api-linter v0.0.0-20250808120943-48643eb2563d/go.mod h1:Jxl3NU9lRf9WJ8dgwgF4U6tLF229jR/KEvtxSwRAKnE= sigs.k8s.io/randfill v0.0.0-20250304075658-069ef1bbf016/go.mod h1:XeLlZ/jmk4i1HRopwe7/aU3H5n1zNUcX6TM94b3QxOY= sigs.k8s.io/randfill v1.0.0 h1:JfjMILfT8A6RbawdsK2JXGBR5AQVfd+9TbzrlneTyrU= sigs.k8s.io/randfill v1.0.0/go.mod h1:XeLlZ/jmk4i1HRopwe7/aU3H5n1zNUcX6TM94b3QxOY= diff --git a/tools/vendor/modules.txt b/tools/vendor/modules.txt index 0b49fa4ca16..6a4d8d9af2e 100644 --- a/tools/vendor/modules.txt +++ b/tools/vendor/modules.txt @@ -2378,7 +2378,7 @@ sigs.k8s.io/controller-tools/pkg/webhook ## explicit; go 1.21 sigs.k8s.io/json sigs.k8s.io/json/internal/golang/encoding/json -# sigs.k8s.io/kube-api-linter v0.0.0-20250729132427-47bfeef6cd38 +# sigs.k8s.io/kube-api-linter v0.0.0-20250808120943-48643eb2563d ## explicit; go 1.24.0 sigs.k8s.io/kube-api-linter/pkg/analysis/commentstart sigs.k8s.io/kube-api-linter/pkg/analysis/conditions @@ -2406,6 +2406,7 @@ 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/analysis/uniquemarkers 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/config sigs.k8s.io/kube-api-linter/pkg/markers sigs.k8s.io/kube-api-linter/pkg/plugin diff --git a/tools/vendor/sigs.k8s.io/kube-api-linter/pkg/analysis/conflictingmarkers/doc.go b/tools/vendor/sigs.k8s.io/kube-api-linter/pkg/analysis/conflictingmarkers/doc.go index 765f60cf6c5..28eb0f3fbe1 100644 --- a/tools/vendor/sigs.k8s.io/kube-api-linter/pkg/analysis/conflictingmarkers/doc.go +++ b/tools/vendor/sigs.k8s.io/kube-api-linter/pkg/analysis/conflictingmarkers/doc.go @@ -32,21 +32,21 @@ Each conflict set must specify: Example configuration: ```yaml -lintersConfig: - - conflictingmarkers: - conflicts: - - name: "optional_vs_required" - sets: - - ["optional", "+kubebuilder:validation:Optional", "+k8s:validation:optional"] - - ["required", "+kubebuilder:validation:Required", "+k8s:validation:required"] - description: "A field cannot be both optional and required" - - name: "my_custom_conflict" - sets: - - ["custom:marker1", "custom:marker2"] - - ["custom:marker3", "custom:marker4"] - - ["custom:marker5", "custom:marker6"] - description: "These markers define different storage backends that cannot be used simultaneously" + + lintersConfig: + conflictingmarkers: + conflicts: + - name: "default_vs_required" + sets: + - ["default", "kubebuilder:default"] + - ["required", "kubebuilder:validation:Required", "k8s:required"] + description: "A field with a default value cannot be required" + - name: "three_way_conflict" + sets: + - ["custom:marker1", "custom:marker2"] + - ["custom:marker3", "custom:marker4"] + - ["custom:marker5", "custom:marker6"] + description: "Three-way conflict between marker sets" ``` diff --git a/tools/vendor/sigs.k8s.io/kube-api-linter/pkg/analysis/optionalfields/analyzer.go b/tools/vendor/sigs.k8s.io/kube-api-linter/pkg/analysis/optionalfields/analyzer.go index eb241ebf59e..80c0d840343 100644 --- a/tools/vendor/sigs.k8s.io/kube-api-linter/pkg/analysis/optionalfields/analyzer.go +++ b/tools/vendor/sigs.k8s.io/kube-api-linter/pkg/analysis/optionalfields/analyzer.go @@ -23,7 +23,7 @@ import ( "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" ) @@ -46,10 +46,7 @@ func init() { } type analyzer struct { - pointerPolicy OptionalFieldsPointerPolicy - pointerPreference OptionalFieldsPointerPreference - omitEmptyPolicy OptionalFieldsOmitEmptyPolicy - omitZeroPolicy OptionalFieldsOmitZeroPolicy + serializationCheck serialization.SerializationCheck } // newAnalyzer creates a new analyzer. @@ -60,11 +57,21 @@ func newAnalyzer(cfg *OptionalFieldsConfig) *analysis.Analyzer { defaultConfig(cfg) + serializationCheck := serialization.New(&serialization.Config{ + Pointers: serialization.PointersConfig{ + Policy: serialization.PointersPolicy(cfg.Pointers.Policy), + Preference: serialization.PointersPreference(cfg.Pointers.Preference), + }, + OmitEmpty: serialization.OmitEmptyConfig{ + Policy: serialization.OmitEmptyPolicy(cfg.OmitEmpty.Policy), + }, + OmitZero: serialization.OmitZeroConfig{ + Policy: serialization.OmitZeroPolicy(cfg.OmitZero.Policy), + }, + }) + a := &analyzer{ - pointerPolicy: cfg.Pointers.Policy, - pointerPreference: cfg.Pointers.Preference, - omitEmptyPolicy: cfg.OmitEmpty.Policy, - omitZeroPolicy: cfg.OmitZero.Policy, + serializationCheck: serializationCheck, } return &analysis.Analyzer{ @@ -99,9 +106,6 @@ func (a *analyzer) checkField(pass *analysis.Pass, field *ast.Field, markersAcce } fieldMarkers := markersAccess.FieldMarkers(field) - - fieldName := field.Names[0].Name - if !isFieldOptional(fieldMarkers) { // The field is not marked optional, so we don't need to check it. return @@ -112,7 +116,7 @@ func (a *analyzer) checkField(pass *analysis.Pass, field *ast.Field, markersAcce return } - a.checkFieldProperties(pass, field, fieldName, markersAccess, jsonTags) + a.serializationCheck.Check(pass, field, markersAccess, jsonTags) } func defaultConfig(cfg *OptionalFieldsConfig) { @@ -133,158 +137,7 @@ func defaultConfig(cfg *OptionalFieldsConfig) { } } -func (a *analyzer) checkFieldProperties(pass *analysis.Pass, field *ast.Field, fieldName string, markersAccess markershelper.Markers, jsonTags extractjsontags.FieldTagInfo) { - hasValidZeroValue, completeValidation := utils.IsZeroValueValid(pass, field, field.Type, markersAccess, a.omitZeroPolicy != OptionalFieldsOmitZeroPolicyForbid) - hasOmitEmpty := jsonTags.OmitEmpty - hasOmitZero := jsonTags.OmitZero - isPointer, underlying := isStarExpr(field.Type) - isStruct := utils.IsStructType(pass, field.Type) - - if a.pointerPreference == OptionalFieldsPointerPreferenceAlways { - // The field must always be a pointer, pointers require omitempty, so enforce that too. - a.handleFieldShouldBePointer(pass, field, fieldName, isPointer, underlying) - a.handleFieldShouldHaveOmitEmpty(pass, field, fieldName, hasOmitEmpty, jsonTags) - - return - } - - // The pointer preference is now when required. - // Validate for omitzero policy. - if a.omitZeroPolicy != OptionalFieldsOmitZeroPolicyForbid { - // If we require omitzero, we can check the field properties based on it being an omitzero field. - a.checkFieldPropertiesWithOmitZeroRequired(pass, field, fieldName, jsonTags, hasOmitZero, isPointer, isStruct, hasValidZeroValue) - } else { - // when the omitzero policy is set to forbid, we need to report removing omitzero if set on the struct fields. - a.checkFieldPropertiesWithOmitZeroForbidPolicy(pass, field, fieldName, isStruct, hasOmitZero, jsonTags) - } - - // The pointer preference is now when required. - // Validate for omitempty policy. - if a.omitEmptyPolicy != OptionalFieldsOmitEmptyPolicyIgnore || hasOmitEmpty { - // If we require omitempty, or the field has omitempty, we can check the field properties based on it being an omitempty field. - a.checkFieldPropertiesWithOmitEmptyRequired(pass, field, fieldName, jsonTags, underlying, hasOmitEmpty, hasValidZeroValue, completeValidation, isPointer, isStruct) - } else { - // The field does not have omitempty, and does not require it. - a.checkFieldPropertiesWithoutOmitEmpty(pass, field, fieldName, jsonTags, underlying, hasValidZeroValue, completeValidation, isPointer, isStruct) - } -} - -func (a *analyzer) checkFieldPropertiesWithOmitEmptyRequired(pass *analysis.Pass, field *ast.Field, fieldName string, jsonTags extractjsontags.FieldTagInfo, underlying ast.Expr, hasOmitEmpty, hasValidZeroValue, completeValidation, isPointer, isStruct bool) { - // In this case, we should always add the omitempty if it isn't present. - a.handleFieldShouldHaveOmitEmpty(pass, field, fieldName, hasOmitEmpty, jsonTags) - - switch { - case isStruct && !hasValidZeroValue && a.omitZeroPolicy != OptionalFieldsOmitZeroPolicyForbid: - // The struct field need not be pointer if it does not have a valid zero value. - return - case hasValidZeroValue && !completeValidation: - a.handleIncompleteFieldValidation(pass, field, fieldName, isPointer, underlying) - fallthrough // Since it's a valid zero value, we should still enforce the pointer. - case hasValidZeroValue, isStruct: - // The field validation infers that the zero value is valid, the field needs to be a pointer. - // Optional structs with omitempty should always be pointers, else they won't actually be omitted. - a.handleFieldShouldBePointer(pass, field, fieldName, isPointer, underlying) - case !hasValidZeroValue && completeValidation && !isStruct: - // The validation is fully complete, and the zero value is not valid, so we don't need a pointer. - a.handleFieldShouldNotBePointer(pass, field, fieldName, isPointer, "field %s is optional and does not allow the zero value. The field does not need to be a pointer.") - } -} - -func (a *analyzer) checkFieldPropertiesWithoutOmitEmpty(pass *analysis.Pass, field *ast.Field, fieldName string, jsonTags extractjsontags.FieldTagInfo, underlying ast.Expr, hasValidZeroValue, completeValidation, isPointer, isStruct bool) { - switch { - case hasValidZeroValue: - // The field is not omitempty, and the zero value is valid, the field does not need to be a pointer. - a.handleFieldShouldNotBePointer(pass, field, fieldName, isPointer, "field %s is optional, without omitempty and allows the zero value. The field does not need to be a pointer.") - case !hasValidZeroValue: - // The zero value would not be accepted, so the field needs to have omitempty. - // Force the omitempty policy to suggest a fix. We can only get to this function when the policy is configured to Ignore. - // Since we absolutely have to add the omitempty tag, we can report it as a suggestion. - reportShouldAddOmitEmpty(pass, field, OptionalFieldsOmitEmptyPolicySuggestFix, fieldName, "field %s is optional and does not allow the zero value. It must have the omitempty tag.", jsonTags) - // Once it has the omitempty tag, it will also need to be a pointer in some cases. - // Now handle it as if it had the omitempty already. - // We already handle the omitempty tag above, so force the `hasOmitEmpty` to true. - a.checkFieldPropertiesWithOmitEmptyRequired(pass, field, fieldName, jsonTags, underlying, false, hasValidZeroValue, completeValidation, isPointer, isStruct) - } -} - -func (a *analyzer) checkFieldPropertiesWithOmitZeroRequired(pass *analysis.Pass, field *ast.Field, fieldName string, jsonTags extractjsontags.FieldTagInfo, hasOmitZero, isPointer, isStruct, hasValidZeroValue bool) { - if !isStruct || hasValidZeroValue { - return - } - - a.handleFieldShouldHaveOmitZero(pass, field, fieldName, hasOmitZero, jsonTags) - a.handleFieldShouldNotBePointer(pass, field, fieldName, isPointer, "field %s is optional and does not have a valid zero value. The field does not need to be a pointer.") -} - -func (a *analyzer) checkFieldPropertiesWithOmitZeroForbidPolicy(pass *analysis.Pass, field *ast.Field, fieldName string, isStruct, hasOmitZero bool, jsonTags extractjsontags.FieldTagInfo) { - if !isStruct || !hasOmitZero { - // Handle omitzero only for struct field having omitZero tag. - return - } - - reportShouldRemoveOmitZero(pass, field, fieldName, jsonTags) -} - -func (a *analyzer) handleFieldShouldBePointer(pass *analysis.Pass, field *ast.Field, fieldName string, isPointer bool, underlying ast.Expr) { - if isPointerType(pass, underlying) { - if isPointer { - switch a.pointerPolicy { - case OptionalFieldsPointerPolicySuggestFix: - reportShouldRemovePointer(pass, field, OptionalFieldsPointerPolicySuggestFix, fieldName, "field %s is optional but the underlying type does not need to be a pointer. The pointer should be removed.") - case OptionalFieldsPointerPolicyWarn: - pass.Reportf(field.Pos(), "field %s is optional but the underlying type does not need to be a pointer. The pointer should be removed.", fieldName) - } - } - - return - } - - if isPointer { - return - } - - switch a.pointerPolicy { - case OptionalFieldsPointerPolicySuggestFix: - reportShouldAddPointer(pass, field, OptionalFieldsPointerPolicySuggestFix, fieldName, "field %s is optional and should be a pointer") - case OptionalFieldsPointerPolicyWarn: - pass.Reportf(field.Pos(), "field %s is optional and should be a pointer", fieldName) - } -} - -func (a *analyzer) handleFieldShouldNotBePointer(pass *analysis.Pass, field *ast.Field, fieldName string, isPointer bool, message string) { - if !isPointer { - return - } - - reportShouldRemovePointer(pass, field, a.pointerPolicy, fieldName, message) -} - -func (a *analyzer) handleFieldShouldHaveOmitEmpty(pass *analysis.Pass, field *ast.Field, fieldName string, hasOmitEmpty bool, jsonTags extractjsontags.FieldTagInfo) { - if hasOmitEmpty { - return - } - - reportShouldAddOmitEmpty(pass, field, a.omitEmptyPolicy, fieldName, "field %s is optional and should have the omitempty tag", jsonTags) -} - -func (a *analyzer) handleFieldShouldHaveOmitZero(pass *analysis.Pass, field *ast.Field, fieldName string, hasOmitZero bool, jsonTags extractjsontags.FieldTagInfo) { - if hasOmitZero { - return - } - // Currently, add omitzero tags to only struct fields. - reportShouldAddOmitZero(pass, field, a.omitZeroPolicy, fieldName, "field %s is optional and does not allow the zero value. It must have the omitzero tag.", jsonTags) -} - -func (a *analyzer) handleIncompleteFieldValidation(pass *analysis.Pass, field *ast.Field, fieldName string, isPointer bool, underlying ast.Expr) { - if isPointer || isPointerType(pass, underlying) { - // Don't warn them if the field is already a pointer. - // If they change the validation then they'll fall into the correct logic for categorizing the field. - // When the field is a pointer type (e.g. map, array), we don't need to warn them either as they should not make these fields pointers. - return - } - - zeroValue := utils.GetTypedZeroValue(pass, underlying) - validationHint := utils.GetTypedValidationHint(pass, underlying) - - pass.Reportf(field.Pos(), "field %s is optional and has a valid zero value (%s), but the validation is not complete (e.g. %s). The field should be a pointer to allow the zero value to be set. If the zero value is not a valid use case, complete the validation and remove the pointer.", fieldName, zeroValue, validationHint) +// isFieldOptional checks if a field has an optional marker. +func isFieldOptional(fieldMarkers markershelper.MarkerSet) bool { + return fieldMarkers.Has(markers.OptionalMarker) || fieldMarkers.Has(markers.KubebuilderOptionalMarker) } diff --git a/tools/vendor/sigs.k8s.io/kube-api-linter/pkg/analysis/requiredfields/analyzer.go b/tools/vendor/sigs.k8s.io/kube-api-linter/pkg/analysis/requiredfields/analyzer.go index e47bd92e003..d3ffc6491b6 100644 --- a/tools/vendor/sigs.k8s.io/kube-api-linter/pkg/analysis/requiredfields/analyzer.go +++ b/tools/vendor/sigs.k8s.io/kube-api-linter/pkg/analysis/requiredfields/analyzer.go @@ -16,16 +16,14 @@ limitations under the License. package requiredfields import ( - "fmt" "go/ast" - "strings" "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" ) @@ -34,11 +32,19 @@ const ( ) func init() { - markershelper.DefaultRegistry().Register(markers.RequiredMarker, markers.KubebuilderRequiredMarker) + markershelper.DefaultRegistry().Register( + markers.RequiredMarker, + markers.KubebuilderRequiredMarker, + markers.KubebuilderMinItemsMarker, + markers.KubebuilderMinLengthMarker, + markers.KubebuilderMinPropertiesMarker, + markers.KubebuilderMinimumMarker, + markers.KubebuilderEnumMarker, + ) } type analyzer struct { - pointerPolicy RequiredFieldPointerPolicy + serializationCheck serialization.SerializationCheck } // newAnalyzer creates a new analyzer. @@ -49,15 +55,34 @@ func newAnalyzer(cfg *RequiredFieldsConfig) *analysis.Analyzer { defaultConfig(cfg) + serializationCheck := serialization.New(&serialization.Config{ + Pointers: serialization.PointersConfig{ + Policy: serialization.PointersPolicy(cfg.Pointers.Policy), + // We only allow the WhenRequired preference for required fields. + // This works for both built-in types and custom resources, and + // avoids pointers unless absolutely necessary. + Preference: serialization.PointersPreferenceWhenRequired, + }, + OmitEmpty: serialization.OmitEmptyConfig{ + Policy: serialization.OmitEmptyPolicy(cfg.OmitEmpty.Policy), + }, + OmitZero: serialization.OmitZeroConfig{ + Policy: serialization.OmitZeroPolicy(cfg.OmitZero.Policy), + }, + }) + a := &analyzer{ - pointerPolicy: cfg.PointerPolicy, + serializationCheck: serializationCheck, } return &analysis.Analyzer{ - Name: name, - Doc: "Checks that all required fields are not pointers, and do not have the omitempty tag.", + Name: name, + Doc: `Checks that all required fields are serialized correctly. + Where the zero value is not valid, this means the field should not be a pointer, and should have the omitempty tag. + Where the zero value is valid, this means the field should be a pointer and should not have the omitempty tag. + `, Run: a.run, - Requires: []*analysis.Analyzer{inspector.Analyzer}, + Requires: []*analysis.Analyzer{inspector.Analyzer, extractjsontags.Analyzer}, } } @@ -68,76 +93,46 @@ func (a *analyzer) run(pass *analysis.Pass) (any, error) { } inspect.InspectFields(func(field *ast.Field, stack []ast.Node, jsonTagInfo extractjsontags.FieldTagInfo, markersAccess markershelper.Markers) { - a.checkField(pass, field, markersAccess.FieldMarkers(field), jsonTagInfo) + a.checkField(pass, field, markersAccess, jsonTagInfo) }) return nil, nil //nolint:nilnil } -func (a *analyzer) checkField(pass *analysis.Pass, field *ast.Field, fieldMarkers markershelper.MarkerSet, fieldTagInfo extractjsontags.FieldTagInfo) { - fieldName := utils.FieldName(field) - if fieldName == "" { +func (a *analyzer) checkField(pass *analysis.Pass, field *ast.Field, markersAccess markershelper.Markers, jsonTags extractjsontags.FieldTagInfo) { + if field == nil || len(field.Names) == 0 { return } - if !fieldMarkers.Has(markers.RequiredMarker) && !fieldMarkers.Has(markers.KubebuilderRequiredMarker) { + fieldMarkers := markersAccess.FieldMarkers(field) + if !isFieldRequired(fieldMarkers) { // The field is not marked required, so we don't need to check it. return } - if fieldTagInfo.OmitEmpty { - pass.Report(analysis.Diagnostic{ - Pos: field.Pos(), - Message: fmt.Sprintf("field %s is marked as required, but has the omitempty tag", fieldName), - SuggestedFixes: []analysis.SuggestedFix{ - { - Message: "should remove the omitempty tag", - TextEdits: []analysis.TextEdit{ - { - Pos: fieldTagInfo.Pos, - End: fieldTagInfo.End, - NewText: []byte(strings.Replace(fieldTagInfo.RawValue, ",omitempty", "", 1)), - }, - }, - }, - }, - }) - } - if field.Type == nil { // The field has no type? We can't check if it's a pointer. return } - if starExpr, ok := field.Type.(*ast.StarExpr); ok { - var suggestedFixes []analysis.SuggestedFix - - switch a.pointerPolicy { - case RequiredFieldPointerWarn: - // Do not suggest a fix. - case RequiredFieldPointerSuggestFix: - suggestedFixes = append(suggestedFixes, analysis.SuggestedFix{ - Message: "should remove the pointer", - TextEdits: []analysis.TextEdit{ - { - Pos: starExpr.Pos(), - End: starExpr.X.Pos(), - NewText: nil, - }, - }, - }) - } - - pass.Report(analysis.Diagnostic{ - Pos: field.Pos(), - Message: fmt.Sprintf("field %s is marked as required, should not be a pointer", fieldName), - SuggestedFixes: suggestedFixes, - }) - } + a.serializationCheck.Check(pass, field, markersAccess, jsonTags) } func defaultConfig(cfg *RequiredFieldsConfig) { - if cfg.PointerPolicy == "" { - cfg.PointerPolicy = RequiredFieldPointerSuggestFix + if cfg.Pointers.Policy == "" { + cfg.Pointers.Policy = RequiredFieldsPointerPolicySuggestFix } + + if cfg.OmitEmpty.Policy == "" { + cfg.OmitEmpty.Policy = RequiredFieldsOmitEmptyPolicySuggestFix + } + + if cfg.OmitZero.Policy == "" { + cfg.OmitZero.Policy = RequiredFieldsOmitZeroPolicySuggestFix + } +} + +// isFieldRequired checks if a field has an required marker. +func isFieldRequired(fieldMarkers markershelper.MarkerSet) bool { + return fieldMarkers.Has(markers.RequiredMarker) || fieldMarkers.Has(markers.KubebuilderRequiredMarker) } diff --git a/tools/vendor/sigs.k8s.io/kube-api-linter/pkg/analysis/requiredfields/config.go b/tools/vendor/sigs.k8s.io/kube-api-linter/pkg/analysis/requiredfields/config.go index 19952e87aa2..a5872c57465 100644 --- a/tools/vendor/sigs.k8s.io/kube-api-linter/pkg/analysis/requiredfields/config.go +++ b/tools/vendor/sigs.k8s.io/kube-api-linter/pkg/analysis/requiredfields/config.go @@ -15,23 +15,98 @@ limitations under the License. */ package requiredfields -// RequiredFieldPointerPolicy is the policy for pointers in required fields. -type RequiredFieldPointerPolicy string +// RequiredFieldsConfig contains configuration for the requiredfields linter. +type RequiredFieldsConfig struct { + // pointers is the policy for pointers in required fields. + // This will allow configuration of whether or not to suggest fixes for pointers in required fields, + // or just warn about their absence. + // Pointers on required fields are only recommended when the zero value is a valid user choice. + Pointers RequiredFieldsPointers `json:"pointers"` -const ( - // RequiredFieldPointerWarn indicates that the linter will emit a warning if a required field is a pointer. - RequiredFieldPointerWarn RequiredFieldPointerPolicy = "Warn" + // omitempty is the policy for the `omitempty` tag on required fields. + // This will allow configuration of whether or not to suggest fixes for the `omitempty` tag on required fields, + // or just warn about their absence. + // Required fields should have the `omitempty` tag to allow structured clients to make an explicit choice + // to include the field. + OmitEmpty RequiredFieldsOmitEmpty `json:"omitempty"` - // RequiredFieldPointerSuggestFix indicates that the linter will emit a warning if a required field is a pointer and suggest a fix. - RequiredFieldPointerSuggestFix RequiredFieldPointerPolicy = "SuggestFix" -) + // omitzero is the policy for the `omitzero` tag within the json tag for fields. + // This defines how the linter should handle optional fields, and whether they should have the omitzero tag or not. + // By default, all the struct fields will be expected to have the `omitzero` tag when their zero value is not an acceptable user choice. + // 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. + OmitZero RequiredFieldsOmitZero `json:"omitzero"` +} -// RequiredFieldsConfig contains configuration for the requiredfields linter. -type RequiredFieldsConfig struct { - // pointerPolicy is the policy for pointers in required fields. - // Valid values are "Warn" and "SuggestFix". - // When set to "Warn", the linter will emit a warning if a required field is a pointer. - // When set to "SuggestFix", the linter will emit a warning if a required field is a pointer and suggest a fix. +// RequiredFieldsPointers is the configuration for pointers in required fields. +type RequiredFieldsPointers struct { + // policy is the policy for the pointer preferences for required fields. + // Valid values are "SuggestFix" and "Warn". + // When set to "SuggestFix", the linter will emit a warning and suggest a fix if the field is a pointer and doesn't need to be, or, where it needs to be a pointer, but isn't. + // When set to "Warn", the linter will emit a warning per the above, but without suggesting a fix. // When otherwise not specified, the default value is "SuggestFix". - PointerPolicy RequiredFieldPointerPolicy `json:"pointerPolicy"` + Policy RequiredFieldsPointerPolicy `json:"policy"` } + +// RequiredFieldsOmitEmpty is the configuration for the `omitempty` tag on required fields. +type RequiredFieldsOmitEmpty struct { + // policy is the policy for the `omitempty` tag on required fields. + // Valid values are "SuggestFix", "Warn" and "Ignore". + // When set to "SuggestFix", the linter will suggest adding the `omitempty` tag when an required field 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 required field missing the `omitempty` tag will be ignored. + // Note, when set to "Ignore", and a field does not have the `omitempty` tag, this may affect whether the field should be a pointer or not. + Policy RequiredFieldsOmitEmptyPolicy `json:"policy"` +} + +// RequiredFieldsOmitZero is the configuration for the `omitzero` tag on required fields. +type RequiredFieldsOmitZero struct { + // policy is the policy for the `omitzero` tag on required fields. + // Valid values are "SuggestFix", "Warn" and "Forbid". + // When set to "SuggestFix", the linter will suggest adding the `omitzero` tag when an required field 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 suggest to remove the `omitzero` tag. + // 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. + Policy RequiredFieldsOmitZeroPolicy `json:"policy"` +} + +// RequiredFieldsPointerPolicy is the policy for pointers in required fields. +type RequiredFieldsPointerPolicy string + +const ( + // RequiredFieldsPointerPolicyWarn indicates that the linter will emit a warning if a required field is a pointer. + RequiredFieldsPointerPolicyWarn RequiredFieldsPointerPolicy = "Warn" + + // RequiredFieldsPointerPolicySuggestFix indicates that the linter will emit a warning if a required field is a pointer and suggest a fix. + RequiredFieldsPointerPolicySuggestFix RequiredFieldsPointerPolicy = "SuggestFix" +) + +// RequiredFieldsOmitEmptyPolicy is the policy for the `omitempty` tag on required fields. +type RequiredFieldsOmitEmptyPolicy string + +const ( + // RequiredFieldsOmitEmptyPolicySuggestFix indicates that the linter will emit a warning if the field does not have omitempty, and suggest a fix. + RequiredFieldsOmitEmptyPolicySuggestFix RequiredFieldsOmitEmptyPolicy = "SuggestFix" + + // RequiredFieldsOmitEmptyPolicyWarn indicates that the linter will emit a warning if the field does not have omitempty. + RequiredFieldsOmitEmptyPolicyWarn RequiredFieldsOmitEmptyPolicy = "Warn" + + // RequiredFieldsOmitEmptyPolicyIgnore indicates that a required field missing the `omitempty` tag will be ignored. + RequiredFieldsOmitEmptyPolicyIgnore RequiredFieldsOmitEmptyPolicy = "Ignore" +) + +// RequiredFieldsOmitZeroPolicy is the policy for the `omitzero` tag on required fields. +type RequiredFieldsOmitZeroPolicy string + +const ( + // RequiredFieldsOmitZeroPolicySuggestFix indicates that the linter will suggest adding the `omitzero` tag when an required field does not have it. + RequiredFieldsOmitZeroPolicySuggestFix RequiredFieldsOmitZeroPolicy = "SuggestFix" + + // RequiredFieldsOmitZeroPolicyWarn indicates that the linter will emit a warning if the field does not have omitzero. + RequiredFieldsOmitZeroPolicyWarn RequiredFieldsOmitZeroPolicy = "Warn" + + // RequiredFieldsOmitZeroPolicyForbid indicates that the linter will not consider `omitzero` tags, and will suggest to remove the `omitzero` tag if a field has it. + RequiredFieldsOmitZeroPolicyForbid RequiredFieldsOmitZeroPolicy = "Forbid" +) diff --git a/tools/vendor/sigs.k8s.io/kube-api-linter/pkg/analysis/requiredfields/initializer.go b/tools/vendor/sigs.k8s.io/kube-api-linter/pkg/analysis/requiredfields/initializer.go index b6295b5cbed..01dcb679763 100644 --- a/tools/vendor/sigs.k8s.io/kube-api-linter/pkg/analysis/requiredfields/initializer.go +++ b/tools/vendor/sigs.k8s.io/kube-api-linter/pkg/analysis/requiredfields/initializer.go @@ -43,7 +43,7 @@ func initAnalyzer(rfc *RequiredFieldsConfig) (*analysis.Analyzer, error) { return newAnalyzer(rfc), nil } -// validateConfig is used to validate the configuration in the config.RequiredFieldsConfig struct. +// validateConfig validates the configuration in the config.RequiredFieldsConfig struct. func validateConfig(rfc *RequiredFieldsConfig, fldPath *field.Path) field.ErrorList { if rfc == nil { return field.ErrorList{} @@ -51,10 +51,47 @@ func validateConfig(rfc *RequiredFieldsConfig, fldPath *field.Path) field.ErrorL fieldErrors := field.ErrorList{} - switch rfc.PointerPolicy { - case "", RequiredFieldPointerWarn, RequiredFieldPointerSuggestFix: + fieldErrors = append(fieldErrors, validateRequiredFieldsPointers(rfc.Pointers, fldPath.Child("pointers"))...) + fieldErrors = append(fieldErrors, validateRequiredFieldsOmitEmpty(rfc.OmitEmpty, fldPath.Child("omitempty"))...) + fieldErrors = append(fieldErrors, validateRequiredFieldsOmitZero(rfc.OmitZero, fldPath.Child("omitzero"))...) + + return fieldErrors +} + +// validateRequiredFieldsPointers is used to validate the configuration in the config.RequiredFieldsPointers struct. +func validateRequiredFieldsPointers(rfc RequiredFieldsPointers, fldPath *field.Path) field.ErrorList { + fieldErrors := field.ErrorList{} + + switch rfc.Policy { + case "", RequiredFieldsPointerPolicySuggestFix, RequiredFieldsPointerPolicyWarn: + default: + fieldErrors = append(fieldErrors, field.Invalid(fldPath.Child("policy"), rfc.Policy, fmt.Sprintf("invalid value, must be one of %q, %q or omitted", RequiredFieldsPointerPolicySuggestFix, RequiredFieldsPointerPolicyWarn))) + } + + return fieldErrors +} + +// validateOptionFieldsOmitEmpty is used to validate the configuration in the config.OptionalFieldsOmitEmpty struct. +func validateRequiredFieldsOmitEmpty(rfc RequiredFieldsOmitEmpty, fldPath *field.Path) field.ErrorList { + fieldErrors := field.ErrorList{} + + switch rfc.Policy { + case "", RequiredFieldsOmitEmptyPolicyIgnore, RequiredFieldsOmitEmptyPolicyWarn, RequiredFieldsOmitEmptyPolicySuggestFix: + default: + fieldErrors = append(fieldErrors, field.Invalid(fldPath.Child("policy"), rfc.Policy, fmt.Sprintf("invalid value, must be one of %q, %q, %q or omitted", RequiredFieldsOmitEmptyPolicyIgnore, RequiredFieldsOmitEmptyPolicyWarn, RequiredFieldsOmitEmptyPolicySuggestFix))) + } + + return fieldErrors +} + +// validateRequiredFieldsOmitZero is used to validate the configuration in the config.RequiredFieldsOmitZero struct. +func validateRequiredFieldsOmitZero(rfc RequiredFieldsOmitZero, fldPath *field.Path) field.ErrorList { + fieldErrors := field.ErrorList{} + + switch rfc.Policy { + case "", RequiredFieldsOmitZeroPolicySuggestFix, RequiredFieldsOmitZeroPolicyWarn, RequiredFieldsOmitZeroPolicyForbid: default: - fieldErrors = append(fieldErrors, field.Invalid(fldPath.Child("pointerPolicy"), rfc.PointerPolicy, fmt.Sprintf("invalid value, must be one of %q, %q or omitted", RequiredFieldPointerWarn, RequiredFieldPointerSuggestFix))) + fieldErrors = append(fieldErrors, field.Invalid(fldPath.Child("policy"), rfc.Policy, fmt.Sprintf("invalid value, must be one of %q, %q, %q or omitted", RequiredFieldsOmitZeroPolicySuggestFix, RequiredFieldsOmitZeroPolicyWarn, RequiredFieldsOmitZeroPolicyForbid))) } return fieldErrors diff --git a/tools/vendor/sigs.k8s.io/kube-api-linter/pkg/analysis/utils/serialization/config.go b/tools/vendor/sigs.k8s.io/kube-api-linter/pkg/analysis/utils/serialization/config.go new file mode 100644 index 00000000000..82ce747050f --- /dev/null +++ b/tools/vendor/sigs.k8s.io/kube-api-linter/pkg/analysis/utils/serialization/config.go @@ -0,0 +1,110 @@ +/* +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 serialization + +// PointersPolicy is the policy for pointers. +// SuggestFix will suggest a fix for the field. +// Warn will warn about the field. +// Ignore will ignore the field. +type PointersPolicy string + +const ( + // PointersPolicySuggestFix will suggest a fix for the field. + PointersPolicySuggestFix PointersPolicy = "SuggestFix" + + // PointersPolicyWarn will warn about the field. + PointersPolicyWarn PointersPolicy = "Warn" +) + +// PointersPreference is the preference for pointers. +// Always will always suggest a fix for the field. +// WhenRequired will only suggest a fix for the field when it is required. +type PointersPreference string + +const ( + // PointersPreferenceAlways will always suggest a pointer. + PointersPreferenceAlways PointersPreference = "Always" + + // PointersPreferenceWhenRequired will only suggest a pointer for the field when it is required. + PointersPreferenceWhenRequired PointersPreference = "WhenRequired" +) + +// OmitEmptyPolicy is the policy for omitempty. +// SuggestFix will suggest a fix for the field to add omitempty. +// Warn will warn about the field to add omitempty. +// Ignore will ignore the the absence of omitempty. +type OmitEmptyPolicy string + +const ( + // OmitEmptyPolicySuggestFix will suggest a fix for the field. + OmitEmptyPolicySuggestFix OmitEmptyPolicy = "SuggestFix" + + // OmitEmptyPolicyWarn will warn about the field. + OmitEmptyPolicyWarn OmitEmptyPolicy = "Warn" + + // OmitEmptyPolicyIgnore will ignore the field. + OmitEmptyPolicyIgnore OmitEmptyPolicy = "Ignore" +) + +// OmitZeroPolicy is the policy for omitzero. +// SuggestFix will suggest a fix for the field to add omitzero. +// Warn will warn about the field to add omitzero. +// Forbid will forbid the field to have omitzero. +type OmitZeroPolicy string + +const ( + // OmitZeroPolicySuggestFix will suggest a fix for the field. + OmitZeroPolicySuggestFix OmitZeroPolicy = "SuggestFix" + + // OmitZeroPolicyWarn will warn about the field. + OmitZeroPolicyWarn OmitZeroPolicy = "Warn" + + // OmitZeroPolicyForbid will forbid the field. + OmitZeroPolicyForbid OmitZeroPolicy = "Forbid" +) + +// Config is the configuration for the serialization check. +type Config struct { + // Pointers is the configuration for pointers. + Pointers PointersConfig + + // OmitEmpty is the configuration for omitempty. + OmitEmpty OmitEmptyConfig + + // OmitZero is the configuration for omitzero. + OmitZero OmitZeroConfig +} + +// PointersConfig is the configuration for pointers. +type PointersConfig struct { + // Policy is the policy for pointers. + Policy PointersPolicy + + // Preference is the preference for pointers. + Preference PointersPreference +} + +// OmitEmptyConfig is the configuration for omitempty. +type OmitEmptyConfig struct { + // Policy is the policy for omitempty. + Policy OmitEmptyPolicy +} + +// OmitZeroConfig is the configuration for omitzero. +type OmitZeroConfig struct { + // Policy is the policy for omitzero. + Policy OmitZeroPolicy +} diff --git a/tools/vendor/sigs.k8s.io/kube-api-linter/pkg/analysis/utils/serialization/serialization_check.go b/tools/vendor/sigs.k8s.io/kube-api-linter/pkg/analysis/utils/serialization/serialization_check.go new file mode 100644 index 00000000000..d22880ad8ee --- /dev/null +++ b/tools/vendor/sigs.k8s.io/kube-api-linter/pkg/analysis/utils/serialization/serialization_check.go @@ -0,0 +1,240 @@ +/* +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 serialization + +import ( + "fmt" + "go/ast" + + "golang.org/x/tools/go/analysis" + "sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/extractjsontags" + markershelper "sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/markers" + "sigs.k8s.io/kube-api-linter/pkg/analysis/utils" +) + +// SerializationCheck is an interface for checking serialization of fields. +type SerializationCheck interface { + Check(pass *analysis.Pass, field *ast.Field, markersAccess markershelper.Markers, jsonTags extractjsontags.FieldTagInfo) +} + +// New creates a new SerializationCheck with the given configuration. +func New(cfg *Config) SerializationCheck { + validateConfig(cfg) + + return &serializationCheck{ + pointerPolicy: cfg.Pointers.Policy, + pointerPreference: cfg.Pointers.Preference, + omitEmptyPolicy: cfg.OmitEmpty.Policy, + omitZeroPolicy: cfg.OmitZero.Policy, + } +} + +// validateConfig validates the configuration. +// We panic if the configuration is invalid as this checker is intended to be +// used as an implementation detail of the kube-api-linter. +// Linters implementing this checker should validate the configuration themselves. +func validateConfig(cfg *Config) { + if cfg == nil { + panic("configuration must be provided") + } + + switch cfg.Pointers.Policy { + case PointersPolicySuggestFix, PointersPolicyWarn: + default: + panic(fmt.Sprintf("pointers.policy is required and must be one of %q or %q", PointersPolicySuggestFix, PointersPolicyWarn)) + } + + switch cfg.Pointers.Preference { + case PointersPreferenceAlways, PointersPreferenceWhenRequired: + default: + panic(fmt.Sprintf("pointers.preference is required and must be one of %q or %q", PointersPreferenceAlways, PointersPreferenceWhenRequired)) + } + + switch cfg.OmitEmpty.Policy { + case OmitEmptyPolicySuggestFix, OmitEmptyPolicyWarn, OmitEmptyPolicyIgnore: + default: + panic(fmt.Sprintf("omitempty.policy is required and must be one of %q, %q or %q", OmitEmptyPolicySuggestFix, OmitEmptyPolicyWarn, OmitEmptyPolicyIgnore)) + } + + switch cfg.OmitZero.Policy { + case OmitZeroPolicySuggestFix, OmitZeroPolicyWarn, OmitZeroPolicyForbid: + default: + panic(fmt.Sprintf("omitzero.policy is required and must be one of %q, %q or %q", OmitZeroPolicySuggestFix, OmitZeroPolicyWarn, OmitZeroPolicyForbid)) + } +} + +// serializationCheck is the implementation of the SerializationCheck interface. +type serializationCheck struct { + pointerPolicy PointersPolicy + pointerPreference PointersPreference + omitEmptyPolicy OmitEmptyPolicy + omitZeroPolicy OmitZeroPolicy +} + +// Check checks the serialization of the field. +// It will check if the zero value of the field is valid, and whether the field should be a pointer or not. +func (s *serializationCheck) Check(pass *analysis.Pass, field *ast.Field, markersAccess markershelper.Markers, jsonTags extractjsontags.FieldTagInfo) { + fieldName := utils.FieldName(field) + + hasValidZeroValue, completeValidation := utils.IsZeroValueValid(pass, field, field.Type, markersAccess, s.omitZeroPolicy != OmitZeroPolicyForbid) + hasOmitEmpty := jsonTags.OmitEmpty + hasOmitZero := jsonTags.OmitZero + isPointer, underlying := utils.IsStarExpr(field.Type) + isStruct := utils.IsStructType(pass, field.Type) + + switch s.pointerPreference { + case PointersPreferenceAlways: + // The field must always be a pointer, pointers require omitempty, so enforce that too. + s.handleFieldShouldBePointer(pass, field, fieldName, isPointer, underlying, "should be a pointer.") + s.handleFieldShouldHaveOmitEmpty(pass, field, fieldName, hasOmitEmpty, jsonTags) + case PointersPreferenceWhenRequired: + s.handleFieldOmitZero(pass, field, fieldName, jsonTags, hasOmitZero, hasValidZeroValue, isPointer, isStruct) + + if s.omitEmptyPolicy != OmitEmptyPolicyIgnore || hasOmitEmpty { + // If we require omitempty, or the field has omitempty, we can check the field properties based on it being an omitempty field. + s.checkFieldPropertiesWithOmitEmptyRequired(pass, field, fieldName, jsonTags, underlying, hasOmitEmpty, hasValidZeroValue, completeValidation, isPointer, isStruct) + } else { + // The field does not have omitempty, and does not require it. + s.checkFieldPropertiesWithoutOmitEmpty(pass, field, fieldName, jsonTags, underlying, hasValidZeroValue, completeValidation, isPointer, isStruct) + } + default: + panic(fmt.Sprintf("unknown pointer preference: %s", s.pointerPreference)) + } +} + +func (s *serializationCheck) handleFieldOmitZero(pass *analysis.Pass, field *ast.Field, fieldName string, jsonTags extractjsontags.FieldTagInfo, hasOmitZero, hasValidZeroValue, isPointer, isStruct bool) { + switch s.omitZeroPolicy { + case OmitZeroPolicyForbid: + // when the omitzero policy is set to forbid, we need to report removing omitzero if set on the struct fields. + s.checkFieldPropertiesWithOmitZeroForbidPolicy(pass, field, fieldName, isStruct, hasOmitZero, jsonTags) + case OmitZeroPolicyWarn, OmitZeroPolicySuggestFix: + // If we require omitzero, or the field has omitzero, we can check the field properties based on it being an omitzero field. + s.checkFieldPropertiesWithOmitZeroRequired(pass, field, fieldName, jsonTags, hasOmitZero, isPointer, isStruct, hasValidZeroValue) + default: + panic(fmt.Sprintf("unknown omit zero policy: %s", s.omitZeroPolicy)) + } +} + +func (s *serializationCheck) handleFieldShouldHaveOmitEmpty(pass *analysis.Pass, field *ast.Field, fieldName string, hasOmitEmpty bool, jsonTags extractjsontags.FieldTagInfo) { + if hasOmitEmpty { + return + } + + reportShouldAddOmitEmpty(pass, field, s.omitEmptyPolicy, fieldName, "field %s should have the omitempty tag.", jsonTags) +} + +func (s *serializationCheck) checkFieldPropertiesWithOmitEmptyRequired(pass *analysis.Pass, field *ast.Field, fieldName string, jsonTags extractjsontags.FieldTagInfo, underlying ast.Expr, hasOmitEmpty, hasValidZeroValue, completeValidation, isPointer, isStruct bool) { + switch { + case isStruct && !hasValidZeroValue && s.omitZeroPolicy != OmitZeroPolicyForbid: + // The struct field need not be pointer if it does not have a valid zero value. + return + case hasValidZeroValue && !completeValidation: + zeroValue := utils.GetTypedZeroValue(pass, underlying) + validationHint := utils.GetTypedValidationHint(pass, underlying) + + s.handleFieldShouldBePointer(pass, field, fieldName, isPointer, underlying, fmt.Sprintf("has a valid zero value (%s), but the validation is not complete (e.g. %s). The field should be a pointer to allow the zero value to be set. If the zero value is not a valid use case, complete the validation and remove the pointer.", zeroValue, validationHint)) + case hasValidZeroValue, isStruct: + // The field validation infers that the zero value is valid, the field needs to be a pointer. + // Structs with omitempty should always be pointers, else they won't actually be omitted. + zeroValue := utils.GetTypedZeroValue(pass, underlying) + + s.handleFieldShouldBePointer(pass, field, fieldName, isPointer, underlying, fmt.Sprintf("has a valid zero value (%s) and should be a pointer.", zeroValue)) + case !hasValidZeroValue && completeValidation && !isStruct: + // The validation is fully complete, and the zero value is not valid, so we don't need a pointer. + s.handleFieldShouldNotBePointer(pass, field, fieldName, isPointer, "field %s does not allow the zero value. The field does not need to be a pointer.") + } + + // In this case, we should always add the omitempty if it isn't present. + s.handleFieldShouldHaveOmitEmpty(pass, field, fieldName, hasOmitEmpty, jsonTags) +} + +func (s *serializationCheck) checkFieldPropertiesWithoutOmitEmpty(pass *analysis.Pass, field *ast.Field, fieldName string, jsonTags extractjsontags.FieldTagInfo, underlying ast.Expr, hasValidZeroValue, completeValidation, isPointer, isStruct bool) { + switch { + case hasValidZeroValue: + // The field is not omitempty, and the zero value is valid, the field does not need to be a pointer. + s.handleFieldShouldNotBePointer(pass, field, fieldName, isPointer, "field %s does not have omitempty and allows the zero value. The field does not need to be a pointer.") + case !hasValidZeroValue: + // The zero value would not be accepted, so the field needs to have omitempty. + // Force the omitempty policy to suggest a fix. We can only get to this function when the policy is configured to Ignore. + // Since we absolutely have to add the omitempty tag, we can report it as a suggestion. + reportShouldAddOmitEmpty(pass, field, OmitEmptyPolicySuggestFix, fieldName, "field %s does not allow the zero value. It must have the omitempty tag.", jsonTags) + // Once it has the omitempty tag, it will also need to be a pointer in some cases. + // Now handle it as if it had the omitempty already. + // We already handle the omitempty tag above, so force the `hasOmitEmpty` to true. + s.checkFieldPropertiesWithOmitEmptyRequired(pass, field, fieldName, jsonTags, underlying, true, hasValidZeroValue, completeValidation, isPointer, isStruct) + } +} + +func (s *serializationCheck) checkFieldPropertiesWithOmitZeroRequired(pass *analysis.Pass, field *ast.Field, fieldName string, jsonTags extractjsontags.FieldTagInfo, hasOmitZero, isPointer, isStruct, hasValidZeroValue bool) { + if !isStruct || hasValidZeroValue { + return + } + + s.handleFieldShouldHaveOmitZero(pass, field, fieldName, hasOmitZero, jsonTags) + s.handleFieldShouldNotBePointer(pass, field, fieldName, isPointer, "field %s does not allow the zero value. The field does not need to be a pointer.") +} + +func (s *serializationCheck) checkFieldPropertiesWithOmitZeroForbidPolicy(pass *analysis.Pass, field *ast.Field, fieldName string, isStruct, hasOmitZero bool, jsonTags extractjsontags.FieldTagInfo) { + if !isStruct || !hasOmitZero { + // Handle omitzero only for struct field having omitZero tag. + return + } + + reportShouldRemoveOmitZero(pass, field, fieldName, jsonTags) +} + +func (s *serializationCheck) handleFieldShouldHaveOmitZero(pass *analysis.Pass, field *ast.Field, fieldName string, hasOmitZero bool, jsonTags extractjsontags.FieldTagInfo) { + if hasOmitZero { + return + } + + // Currently, add omitzero tags to only struct fields. + reportShouldAddOmitZero(pass, field, s.omitZeroPolicy, fieldName, "field %s does not allow the zero value. It must have the omitzero tag.", jsonTags) +} + +func (s *serializationCheck) handleFieldShouldBePointer(pass *analysis.Pass, field *ast.Field, fieldName string, isPointer bool, underlying ast.Expr, reason string) { + if utils.IsPointerType(pass, underlying) { + if isPointer { + switch s.pointerPolicy { + case PointersPolicySuggestFix: + reportShouldRemovePointer(pass, field, PointersPolicySuggestFix, fieldName, "field %s underlying type does not need to be a pointer. The pointer should be removed.", fieldName) + case PointersPolicyWarn: + pass.Reportf(field.Pos(), "field %s underlying type does not need to be a pointer. The pointer should be removed.", fieldName) + } + } + + return + } + + if isPointer { + return + } + + switch s.pointerPolicy { + case PointersPolicySuggestFix: + reportShouldAddPointer(pass, field, PointersPolicySuggestFix, fieldName, "field %s %s", fieldName, reason) + case PointersPolicyWarn: + pass.Reportf(field.Pos(), "field %s %s", fieldName, reason) + } +} + +func (s *serializationCheck) handleFieldShouldNotBePointer(pass *analysis.Pass, field *ast.Field, fieldName string, isPointer bool, message string) { + if !isPointer { + return + } + + reportShouldRemovePointer(pass, field, s.pointerPolicy, fieldName, message, fieldName) +} diff --git a/tools/vendor/sigs.k8s.io/kube-api-linter/pkg/analysis/optionalfields/util.go b/tools/vendor/sigs.k8s.io/kube-api-linter/pkg/analysis/utils/serialization/util.go similarity index 65% rename from tools/vendor/sigs.k8s.io/kube-api-linter/pkg/analysis/optionalfields/util.go rename to tools/vendor/sigs.k8s.io/kube-api-linter/pkg/analysis/utils/serialization/util.go index 2ccc9f372e4..3695f6ce925 100644 --- a/tools/vendor/sigs.k8s.io/kube-api-linter/pkg/analysis/optionalfields/util.go +++ b/tools/vendor/sigs.k8s.io/kube-api-linter/pkg/analysis/utils/serialization/util.go @@ -13,7 +13,7 @@ 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 optionalfields +package serialization import ( "fmt" @@ -23,54 +23,16 @@ import ( "golang.org/x/tools/go/analysis" "sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/extractjsontags" - 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" ) -// isStarExpr checks if the expression is a pointer type. -// If it is, it returns the expression inside the pointer. -func isStarExpr(expr ast.Expr) (bool, ast.Expr) { - if ptrType, ok := expr.(*ast.StarExpr); ok { - return true, ptrType.X - } - - return false, expr -} - -// isPointerType checks if the expression is a pointer type. -// This is for types that are always implemented as pointers and therefore should -// not be the underlying type of a star expr. -func isPointerType(pass *analysis.Pass, expr ast.Expr) bool { - switch t := expr.(type) { - case *ast.StarExpr, *ast.MapType, *ast.ArrayType: - return true - case *ast.Ident: - // If the ident is a type alias, keep checking until we find the underlying type. - typeSpec, ok := utils.LookupTypeSpec(pass, t) - if !ok { - return false - } - - return isPointerType(pass, typeSpec.Type) - default: - return false - } -} - -// isFieldOptional checks if a field has an optional marker. -func isFieldOptional(fieldMarkers markershelper.MarkerSet) bool { - return fieldMarkers.Has(markers.OptionalMarker) || fieldMarkers.Has(markers.KubebuilderOptionalMarker) -} - // reportShouldAddPointer adds an analysis diagnostic that explains that a pointer should be added. // Where the pointer policy is suggest fix, it also adds a suggested fix to add the pointer. -func reportShouldAddPointer(pass *analysis.Pass, field *ast.Field, pointerPolicy OptionalFieldsPointerPolicy, fieldName, messageFmt string) { +func reportShouldAddPointer(pass *analysis.Pass, field *ast.Field, pointerPolicy PointersPolicy, fieldName, messageFmt string, args ...any) { switch pointerPolicy { - case OptionalFieldsPointerPolicySuggestFix: + case PointersPolicySuggestFix: pass.Report(analysis.Diagnostic{ Pos: field.Pos(), - Message: fmt.Sprintf(messageFmt, fieldName), + Message: fmt.Sprintf(messageFmt, args...), SuggestedFixes: []analysis.SuggestedFix{ { Message: "should make the field a pointer", @@ -83,8 +45,8 @@ func reportShouldAddPointer(pass *analysis.Pass, field *ast.Field, pointerPolicy }, }, }) - case OptionalFieldsPointerPolicyWarn: - pass.Reportf(field.Pos(), messageFmt, fieldName) + case PointersPolicyWarn: + pass.Reportf(field.Pos(), messageFmt, args...) default: panic(fmt.Sprintf("unknown pointer policy: %s", pointerPolicy)) } @@ -92,12 +54,12 @@ func reportShouldAddPointer(pass *analysis.Pass, field *ast.Field, pointerPolicy // reportShouldRemovePointer adds an analysis diagnostic that explains that a pointer should be removed. // Where the pointer policy is suggest fix, it also adds a suggested fix to remove the pointer. -func reportShouldRemovePointer(pass *analysis.Pass, field *ast.Field, pointerPolicy OptionalFieldsPointerPolicy, fieldName, messageFmt string) { +func reportShouldRemovePointer(pass *analysis.Pass, field *ast.Field, pointerPolicy PointersPolicy, fieldName, messageFmt string, args ...any) { switch pointerPolicy { - case OptionalFieldsPointerPolicySuggestFix: + case PointersPolicySuggestFix: pass.Report(analysis.Diagnostic{ Pos: field.Pos(), - Message: fmt.Sprintf(messageFmt, fieldName), + Message: fmt.Sprintf(messageFmt, args...), SuggestedFixes: []analysis.SuggestedFix{ { Message: "should remove the pointer", @@ -110,17 +72,17 @@ func reportShouldRemovePointer(pass *analysis.Pass, field *ast.Field, pointerPol }, }, }) - case OptionalFieldsPointerPolicyWarn: - pass.Reportf(field.Pos(), messageFmt, fieldName) + case PointersPolicyWarn: + pass.Reportf(field.Pos(), messageFmt, args...) default: panic(fmt.Sprintf("unknown pointer policy: %s", pointerPolicy)) } } // reportShouldAddOmitEmpty adds an analysis diagnostic that explains that an omitempty tag should be added. -func reportShouldAddOmitEmpty(pass *analysis.Pass, field *ast.Field, omitEmptyPolicy OptionalFieldsOmitEmptyPolicy, fieldName, messageFmt string, fieldTagInfo extractjsontags.FieldTagInfo) { +func reportShouldAddOmitEmpty(pass *analysis.Pass, field *ast.Field, omitEmptyPolicy OmitEmptyPolicy, fieldName, messageFmt string, fieldTagInfo extractjsontags.FieldTagInfo) { switch omitEmptyPolicy { - case OptionalFieldsOmitEmptyPolicySuggestFix: + case OmitEmptyPolicySuggestFix: pass.Report(analysis.Diagnostic{ Pos: field.Pos(), Message: fmt.Sprintf(messageFmt, fieldName), @@ -136,9 +98,9 @@ func reportShouldAddOmitEmpty(pass *analysis.Pass, field *ast.Field, omitEmptyPo }, }, }) - case OptionalFieldsOmitEmptyPolicyWarn: + case OmitEmptyPolicyWarn: pass.Reportf(field.Pos(), messageFmt, fieldName) - case OptionalFieldsOmitEmptyPolicyIgnore: + case OmitEmptyPolicyIgnore: // Do nothing, as the policy is to ignore the missing omitempty tag. default: panic(fmt.Sprintf("unknown omit empty policy: %s", omitEmptyPolicy)) @@ -146,9 +108,9 @@ func reportShouldAddOmitEmpty(pass *analysis.Pass, field *ast.Field, omitEmptyPo } // reportShouldAddOmitZero adds an analysis diagnostic that explains that an omitzero tag should be added. -func reportShouldAddOmitZero(pass *analysis.Pass, field *ast.Field, omitZeroPolicy OptionalFieldsOmitZeroPolicy, fieldName, messageFmt string, fieldTagInfo extractjsontags.FieldTagInfo) { +func reportShouldAddOmitZero(pass *analysis.Pass, field *ast.Field, omitZeroPolicy OmitZeroPolicy, fieldName, messageFmt string, fieldTagInfo extractjsontags.FieldTagInfo) { switch omitZeroPolicy { - case OptionalFieldsOmitZeroPolicySuggestFix: + case OmitZeroPolicySuggestFix: pass.Report(analysis.Diagnostic{ Pos: field.Pos(), Message: fmt.Sprintf(messageFmt, fieldName), @@ -164,10 +126,10 @@ func reportShouldAddOmitZero(pass *analysis.Pass, field *ast.Field, omitZeroPoli }, }, }) - case OptionalFieldsOmitZeroPolicyWarn: + case OmitZeroPolicyWarn: pass.Reportf(field.Pos(), messageFmt, fieldName) - case OptionalFieldsOmitZeroPolicyForbid: - // Do nothing, as the policy is to ignore the missing omitzero tag. + case OmitZeroPolicyForbid: + // Do nothing, as the policy is to forbid the missing omitzero tag. default: panic(fmt.Sprintf("unknown omit zero policy: %s", omitZeroPolicy)) } @@ -176,6 +138,7 @@ func reportShouldAddOmitZero(pass *analysis.Pass, field *ast.Field, omitZeroPoli // reportShouldRemoveOmitZero adds an analysis diagnostic that explains that an omitzero tag should be removed. func reportShouldRemoveOmitZero(pass *analysis.Pass, field *ast.Field, fieldName string, jsonTags extractjsontags.FieldTagInfo) { omitZeroPos := jsonTags.Pos + token.Pos(strings.Index(jsonTags.RawValue, ",omitzero")) + pass.Report(analysis.Diagnostic{ Pos: field.Pos(), Message: fmt.Sprintf("field %s has the omitzero tag, but by policy is not allowed. The omitzero tag should be removed.", fieldName), diff --git a/tools/vendor/sigs.k8s.io/kube-api-linter/pkg/analysis/utils/utils.go b/tools/vendor/sigs.k8s.io/kube-api-linter/pkg/analysis/utils/utils.go index 15b989103b7..6a7592ca73a 100644 --- a/tools/vendor/sigs.k8s.io/kube-api-linter/pkg/analysis/utils/utils.go +++ b/tools/vendor/sigs.k8s.io/kube-api-linter/pkg/analysis/utils/utils.go @@ -52,6 +52,36 @@ func IsStructType(pass *analysis.Pass, expr ast.Expr) bool { return false } +// IsStarExpr checks if the expression is a pointer type. +// If it is, it returns the expression inside the pointer. +func IsStarExpr(expr ast.Expr) (bool, ast.Expr) { + if ptrType, ok := expr.(*ast.StarExpr); ok { + return true, ptrType.X + } + + return false, expr +} + +// IsPointerType checks if the expression is a pointer type. +// This is for types that are always implemented as pointers and therefore should +// not be the underlying type of a star expr. +func IsPointerType(pass *analysis.Pass, expr ast.Expr) bool { + switch t := expr.(type) { + case *ast.StarExpr, *ast.MapType, *ast.ArrayType: + return true + case *ast.Ident: + // If the ident is a type alias, keep checking until we find the underlying type. + typeSpec, ok := LookupTypeSpec(pass, t) + if !ok { + return false + } + + return IsPointerType(pass, typeSpec.Type) + default: + return false + } +} + // LookupTypeSpec is used to search for the type spec of a given identifier. // It will first check to see if the ident has an Obj, and if so, it will return the type spec // from the Obj. If the Obj is nil, it will search through the files in the package to find the From 96c90e07240142dec213782c43905a15986031c8 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Fri, 8 Aug 2025 13:47:33 +0100 Subject: [PATCH 2/2] Switch optional fields to require omitempty --- .golangci.yaml | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/.golangci.yaml b/.golangci.yaml index a2a8be77cf2..6a22f61576a 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -25,10 +25,14 @@ linters: preference: WhenRequired policy: SuggestFix omitEmpty: - # Ignore missing omitempty so that we can omit the omitempty for discoverability. - # Discoverability is for configuration APIs, generally singletons. - # Refer to the API conventions for when to use discoverability (not our default stance). - policy: Ignore + # This will force omitempty on optional fields. + # This is in line with upstream guidance where optional fields should be omitted + # from the serialized output unless they are non-zero. + policy: SuggestFix + omitzero: + # This will force omitzero on optional struct fields. + # This means they can be omitted correctly and prevents the need for pointers to structs. + policy: SuggestFix requiredFields: pointers: # This will force pointers when the field is required, but only when the zero