tighten validation of inlining and metadata #504

pohly · 2024-08-21T12:50:16Z

This catches the following mistake:

type ResourceSliceList struct {
   metav1.ListMeta `json:"listMeta" protobuf:"bytes,1,opt,name=listMeta"`
   ...
}

The "listMeta" name matches the Go field name, but the convention is to call that JSON field "metadata".

The same applies to ObjectMeta.

While at it, usage of the empty JSON name also gets checked more thoroughly. This is a mistake which was not detected earlier:

type SomeStruct struct {
    SomeInt int `json:",inline"`
}

Jefftree

Minor comments.

cc @liggitt for changes affect api linting

Jefftree · 2024-08-21T15:51:13Z

pkg/generators/rules/naming_convention.go

@@ -0,0 +1,70 @@
+/*
+Copyright 2018 The Kubernetes Authors.


Jefftree · 2024-08-21T15:54:51Z

pkg/generators/api_linter.go

@@ -136,6 +136,7 @@ type apiLinter struct {
 func newAPILinter() *apiLinter {
 	return &apiLinter{
 		rules: []APIRule{
+			&rules.NamingConvention{},


Should we be more specific that this is only referring to ListMeta naming convention?

I asked on Slack whether there should be other conventions that this should enforce:
https://kubernetes.slack.com/archives/C0409NGC1TK/p1724256359105049?thread_ts=1723854248.858719&cid=C0409NGC1TK

If there aren't then renaming makes sense.

It's also for ObjectMeta. That wasn't in my original commit, but I've included it in the revised one.

Jefftree · 2024-08-21T15:55:27Z

pkg/generators/rules/naming_convention.go

+// Validate evaluates API rule on type t and returns a list of field names in
+// the type that violate the rule. Empty field name [""] implies the entire
+// type violates the rule.
+func (n *NamingConvention) Validate(t *types.Type) ([]string, error) {


can you add a test for this?

Absolutely. It's WIP because I wanted to be sure first that this is going in the right direction.

pohly · 2024-08-23T12:54:55Z

pkg/generators/rules/naming_convention.go

+			return false
+		}
+		jsonName := strings.Split(jsonTag, ",")[0]
+		if jsonName != "metadata" {


"metadata" is excluded from name checking in

kube-openapi/pkg/generators/rules/names_match.go

Lines 39 to 40 in 76de80e

// Special case for object and list meta

"metadata",

The effect is that this mistake is not caught:

Items []ResourceSlice `json:"metadata" protobuf:"bytes,2,rep,name=metadata"`

An alternative to adding this NamingConvention rule would be to extend the NamesMatch rule:

Check for ListMeta and ObjectMeta there.

Those fields must be called "metadata".

Other fields that are called "metadata" must have matching Go and JSON names.

Something along those lines sounds like a good direction to me

Done, PR updated accordingly.

I couldn't resist also addressing this false positive:

SomeInt int `json:""`

This wasn't reported because "" was also in the jsonNameBlacklist.

Not checking empty JSON names and "metadata" has the effect that incorrect usage of those does not get detected. These new unit tests demonstrate that. Fixing the behavior and updating the tests accordingly will follow.

liggitt · 2024-08-26T14:20:49Z

/lgtm
/approve

pohly · 2024-08-26T14:45:56Z

Using this in Kubernetes might be tricky, updating the dependency wasn't straight-forward - see kubernetes/kubernetes#126917 (not including this PR yet).

liggitt · 2024-08-26T14:57:50Z

~~try rebasing, some other fixups went in last week (#507, #508)~~

I'd recommend waiting until kubernetes/kubernetes#126787 gets in and handles the regen cleanup

pohly · 2024-08-26T16:21:47Z

pkg/generators/rules/names_match.go

+func nameIsOkay(member types.Member, jsonName string) bool {
+	if jsonName == "" {
+		return member.Type.Kind == types.Struct ||
+			member.Type.Kind == types.Pointer && member.Type.Underlying.Kind == types.Struct


Ooops. This should be member.Type.Elem.

/cancel lgtm

https://github.com/kubernetes/kubernetes/blob/5ac315faf428567ec023aa7d8c2b89b1d9c9211a/vendor/k8s.io/gengo/v2/types/types.go#L327-L335

Noticed just in time when looking at the gengo API changes, but missed it earlier when writing the code... sorry!

liggitt · 2024-08-26T16:41:35Z

/lgtm

Because "" and "metadata" were excluded from checking, some incorrect usage of those were not detected. Also, naming ListMeta or ObjectMeta "listMeta" or "objectMeta" was allowed because the names match, even though by convention the name should have been "metadata".

Jefftree · 2024-09-03T16:35:55Z

/lgtm
/approve

k8s-ci-robot · 2024-09-03T16:36:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Jefftree, liggitt, pohly

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/generators/rules/OWNERS~~ [Jefftree]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 21, 2024

k8s-ci-robot requested review from apelisse and roycaihw August 21, 2024 12:50

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 21, 2024

Jefftree reviewed Aug 21, 2024

View reviewed changes

pohly force-pushed the naming-convention branch from ae7eba5 to 250daf8 Compare August 21, 2024 16:08

pohly commented Aug 23, 2024

View reviewed changes

pohly force-pushed the naming-convention branch from 250daf8 to 3d12da0 Compare August 26, 2024 07:38

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 26, 2024

pohly changed the title ~~WIP: rules: enforce that ListMeta is called "metadata"~~ tighten validation of inlining and metadata Aug 26, 2024

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 26, 2024

names match: add unit tests demonstrating false positives

4fca853

Not checking empty JSON names and "metadata" has the effect that incorrect usage of those does not get detected. These new unit tests demonstrate that. Fixing the behavior and updating the tests accordingly will follow.

k8s-ci-robot assigned liggitt Aug 26, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 26, 2024

pohly commented Aug 26, 2024

View reviewed changes

pohly force-pushed the naming-convention branch from 3d12da0 to 5b13d40 Compare August 26, 2024 16:25

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 26, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 26, 2024

k8s-ci-robot assigned Jefftree Sep 3, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 3, 2024

k8s-ci-robot merged commit 9e1beec into kubernetes:master Sep 3, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tighten validation of inlining and metadata #504

tighten validation of inlining and metadata #504

pohly commented Aug 21, 2024 •

edited

Loading

Jefftree left a comment

Jefftree Aug 21, 2024

Jefftree Aug 21, 2024

pohly Aug 21, 2024

pohly Aug 21, 2024

Jefftree Aug 21, 2024

pohly Aug 21, 2024

pohly Aug 23, 2024

liggitt Aug 23, 2024

pohly Aug 26, 2024

liggitt commented Aug 26, 2024

pohly commented Aug 26, 2024

liggitt commented Aug 26, 2024 •

edited

Loading

pohly Aug 26, 2024

pohly Aug 26, 2024

pohly Aug 26, 2024

liggitt commented Aug 26, 2024

Jefftree commented Sep 3, 2024

k8s-ci-robot commented Sep 3, 2024

tighten validation of inlining and metadata #504

tighten validation of inlining and metadata #504

Conversation

pohly commented Aug 21, 2024 • edited Loading

Jefftree left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Aug 26, 2024

pohly commented Aug 26, 2024

liggitt commented Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Aug 26, 2024

Jefftree commented Sep 3, 2024

k8s-ci-robot commented Sep 3, 2024

pohly commented Aug 21, 2024 •

edited

Loading

liggitt commented Aug 26, 2024 •

edited

Loading