Merge pull request #114948 from haoruan/feature-api-doc-field-linter

Add linter to check if api docs match field tag names

api/openapi-spec/swagger.json

@@ -14122,12 +14122,12 @@
       "description": "CustomResourceConversion describes how to convert different versions of a CR.",
       "properties": {
         "strategy": {
-          "description": "strategy specifies how custom resources are converted between versions. Allowed values are: - `None`: The converter only change the apiVersion and would not touch any other field in the custom resource. - `Webhook`: API Server will call to an external webhook to do the conversion. Additional information\n  is needed for this option. This requires spec.preserveUnknownFields to be false, and spec.conversion.webhook to be set.",
+          "description": "strategy specifies how custom resources are converted between versions. Allowed values are: - `\"None\"`: The converter only change the apiVersion and would not touch any other field in the custom resource. - `\"Webhook\"`: API Server will call to an external webhook to do the conversion. Additional information\n  is needed for this option. This requires spec.preserveUnknownFields to be false, and spec.conversion.webhook to be set.",
           "type": "string"
         },
         "webhook": {
           "$ref": "#/definitions/io.k8s.apiextensions-apiserver.pkg.apis.apiextensions.v1.WebhookConversion",
-          "description": "webhook describes how to call the conversion webhook. Required when `strategy` is set to `Webhook`."
+          "description": "webhook describes how to call the conversion webhook. Required when `strategy` is set to `\"Webhook\"`."
         }
       },
       "required": [

api/openapi-spec/v3/apis__apiextensions.k8s.io__v1_openapi.json

@@ -45,7 +45,7 @@
         "properties": {
           "strategy": {
             "default": "",
-            "description": "strategy specifies how custom resources are converted between versions. Allowed values are: - `None`: The converter only change the apiVersion and would not touch any other field in the custom resource. - `Webhook`: API Server will call to an external webhook to do the conversion. Additional information\n  is needed for this option. This requires spec.preserveUnknownFields to be false, and spec.conversion.webhook to be set.",
+            "description": "strategy specifies how custom resources are converted between versions. Allowed values are: - `\"None\"`: The converter only change the apiVersion and would not touch any other field in the custom resource. - `\"Webhook\"`: API Server will call to an external webhook to do the conversion. Additional information\n  is needed for this option. This requires spec.preserveUnknownFields to be false, and spec.conversion.webhook to be set.",
             "type": "string"
           },
           "webhook": {
@@ -54,7 +54,7 @@
                 "$ref": "#/components/schemas/io.k8s.apiextensions-apiserver.pkg.apis.apiextensions.v1.WebhookConversion"
               }
             ],
-            "description": "webhook describes how to call the conversion webhook. Required when `strategy` is set to `Webhook`."
+            "description": "webhook describes how to call the conversion webhook. Required when `strategy` is set to `\"Webhook\"`."
           }
         },
         "required": [

cmd/fieldnamedocscheck/field_name_docs_check.go

@@ -0,0 +1,105 @@
+/*
+Copyright 2023 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 main
+
+import (
+	"fmt"
+	"os"
+	"regexp"
+	"strings"
+
+	flag "github.com/spf13/pflag"
+	kruntime "k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/util/sets"
+	"k8s.io/klog/v2"
+)
+
+var (
+	typeSrc = flag.StringP("type-src", "s", "", "From where we are going to read the types")
+	re      = regexp.MustCompile("`(\\b\\w+\\b)`")
+)
+
+// kubeTypesMap is a map from field name to its tag name and doc.
+type kubeTypesMap map[string]kruntime.Pair
+
+func main() {
+	flag.Parse()
+
+	if *typeSrc == "" {
+		klog.Fatalf("Please define -s flag as it is the api type file")
+	}
+
+	docsForTypes := kruntime.ParseDocumentationFrom(*typeSrc)
+	rc := false
+
+	for _, ks := range docsForTypes {
+		typesMap := make(kubeTypesMap)
+
+		for _, p := range ks[1:] {
+			// skip the field with no tag name
+			if p.Name != "" {
+				typesMap[strings.ToLower(p.Name)] = p
+			}
+		}
+
+		structName := ks[0].Name
+
+		rc = checkFieldNameAndDoc(structName, "", ks[0].Doc, typesMap)
+		for _, p := range ks[1:] {
+			rc = checkFieldNameAndDoc(structName, p.Name, p.Doc, typesMap)
+		}
+	}
+
+	if rc {
+		os.Exit(1)
+	}
+}
+
+func checkFieldNameAndDoc(structName, fieldName, doc string, typesMap kubeTypesMap) bool {
+	rc := false
+	visited := sets.Set[string]{}
+
+	// The rule is:
+	// 1. Get all back-tick quoted names in the doc
+	// 2. Skip the name which is already found mismatched.
+	// 3. Skip the name whose lowercase is different from the lowercase of tag names,
+	//    because some docs use back-tick to quote field value or nil
+	// 4. Check if the name is different from its tag name
+
+	// TODO: a manual pass adding back-ticks to the doc strings, then update the linter to
+	// check the existence of back-ticks
+	nameGroups := re.FindAllStringSubmatch(doc, -1)
+	for _, nameGroup := range nameGroups {
+		name := nameGroup[1]
+		if visited.Has(name) {
+			continue
+		}
+		if p, ok := typesMap[strings.ToLower(name)]; ok && p.Name != name {
+			rc = true
+			visited.Insert(name)
+
+			fmt.Fprintf(os.Stderr, "Error: doc for %s", structName)
+			if fieldName != "" {
+				fmt.Fprintf(os.Stderr, ".%s", fieldName)
+			}
+
+			fmt.Fprintf(os.Stderr, " contains: %s, which should be: %s\n", name, p.Name)
+		}
+	}
+
+	return rc
+}

hack/verify-fieldname-docs.sh

@@ -0,0 +1,65 @@
+#!/usr/bin/env bash
+
+# Copyright 2023 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.
+
+# This script checks API-related files for mismatch in docs and field names,
+# and outputs a list of fields that their docs and field names are mismatched.
+# Usage: `hack/verify-fieldname-docs.sh`.
+
+set -o errexit
+set -o nounset
+set -o pipefail
+
+KUBE_ROOT=$(dirname "${BASH_SOURCE[0]}")/..
+source "${KUBE_ROOT}/hack/lib/init.sh"
+source "${KUBE_ROOT}/hack/lib/util.sh"
+
+kube::golang::setup_env
+
+make -C "${KUBE_ROOT}" WHAT=cmd/fieldnamedocscheck
+
+# Find binary
+fieldnamedocscheck=$(kube::util::find-binary "fieldnamedocscheck")
+
+result=0
+
+find_files() {
+  find . -not \( \
+      \( \
+        -wholename './output' \
+        -o -wholename './_output' \
+        -o -wholename './_gopath' \
+        -o -wholename './release' \
+        -o -wholename './target' \
+        -o -wholename '*/third_party/*' \
+        -o -wholename '*/vendor/*' \
+        -o -wholename './pkg/*' \
+      \) -prune \
+    \) \
+    \( -wholename './staging/src/k8s.io/api/*/v*/types.go' \
+       -o -wholename './staging/src/k8s.io/kube-aggregator/pkg/apis/*/v*/types.go' \
+       -o -wholename './staging/src/k8s.io/apiextensions-apiserver/pkg/apis/*/v*/types.go' \
+    \)
+}
+
+versioned_api_files=$(find_files) || true
+
+for file in ${versioned_api_files}; do
+  package="${file%"/types.go"}"
+  echo "Checking ${package}"
+  ${fieldnamedocscheck} -s "${file}" || result=$?
+done
+
+exit ${result}

pkg/generated/openapi/zz_generated.openapi.go

@@ -44441,7 +44441,7 @@ func schema_pkg_apis_apiextensions_v1_CustomResourceConversion(ref common.Refere
 				Properties: map[string]spec.Schema{
 					"strategy": {
 						SchemaProps: spec.SchemaProps{
-							Description: "strategy specifies how custom resources are converted between versions. Allowed values are: - `None`: The converter only change the apiVersion and would not touch any other field in the custom resource. - `Webhook`: API Server will call to an external webhook to do the conversion. Additional information\n  is needed for this option. This requires spec.preserveUnknownFields to be false, and spec.conversion.webhook to be set.",
+							Description: "strategy specifies how custom resources are converted between versions. Allowed values are: - `\"None\"`: The converter only change the apiVersion and would not touch any other field in the custom resource. - `\"Webhook\"`: API Server will call to an external webhook to do the conversion. Additional information\n  is needed for this option. This requires spec.preserveUnknownFields to be false, and spec.conversion.webhook to be set.",
 							Default:     "",
 							Type:        []string{"string"},
 							Format:      "",
@@ -44449,7 +44449,7 @@ func schema_pkg_apis_apiextensions_v1_CustomResourceConversion(ref common.Refere
 					},
 					"webhook": {
 						SchemaProps: spec.SchemaProps{
-							Description: "webhook describes how to call the conversion webhook. Required when `strategy` is set to `Webhook`.",
+							Description: "webhook describes how to call the conversion webhook. Required when `strategy` is set to `\"Webhook\"`.",
 							Ref:         ref("k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1.WebhookConversion"),
 						},
 					},

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/generated.proto

@@ -107,12 +107,12 @@ message CustomResourceColumnDefinition {
 // CustomResourceConversion describes how to convert different versions of a CR.
 message CustomResourceConversion {
   // strategy specifies how custom resources are converted between versions. Allowed values are:
-  // - `None`: The converter only change the apiVersion and would not touch any other field in the custom resource.
+  // - `"None"`: The converter only change the apiVersion and would not touch any other field in the custom resource.
-  // - `Webhook`: API Server will call to an external webhook to do the conversion. Additional information
+  // - `"Webhook"`: API Server will call to an external webhook to do the conversion. Additional information
   //   is needed for this option. This requires spec.preserveUnknownFields to be false, and spec.conversion.webhook to be set.
   optional string strategy = 1;
 
-  // webhook describes how to call the conversion webhook. Required when `strategy` is set to `Webhook`.
+  // webhook describes how to call the conversion webhook. Required when `strategy` is set to `"Webhook"`.
   // +optional
   optional WebhookConversion webhook = 2;
 }

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/types.go

@@ -74,12 +74,12 @@ type CustomResourceDefinitionSpec struct {
 // CustomResourceConversion describes how to convert different versions of a CR.
 type CustomResourceConversion struct {
 	// strategy specifies how custom resources are converted between versions. Allowed values are:
-	// - `None`: The converter only change the apiVersion and would not touch any other field in the custom resource.
+	// - `"None"`: The converter only change the apiVersion and would not touch any other field in the custom resource.
-	// - `Webhook`: API Server will call to an external webhook to do the conversion. Additional information
+	// - `"Webhook"`: API Server will call to an external webhook to do the conversion. Additional information
 	//   is needed for this option. This requires spec.preserveUnknownFields to be false, and spec.conversion.webhook to be set.
 	Strategy ConversionStrategyType `json:"strategy" protobuf:"bytes,1,name=strategy"`
 
-	// webhook describes how to call the conversion webhook. Required when `strategy` is set to `Webhook`.
+	// webhook describes how to call the conversion webhook. Required when `strategy` is set to `"Webhook"`.
 	// +optional
 	Webhook *WebhookConversion `json:"webhook,omitempty" protobuf:"bytes,2,opt,name=webhook"`
 }