From 834658cb26c1cfbb2b41ac2f04711563ee534682 Mon Sep 17 00:00:00 2001 From: Ahmad Zolfaghari Date: Tue, 21 May 2024 15:10:32 +0330 Subject: [PATCH] Fix kubectl explain bug when additionalProperties in schema defines as boolean. (#124506) * Fix kubectl explain bug when additionalProperties in schema defines as: `additionalProperties: true` to ignore iterating. * trigger error on kubectl explain with integration test on crd with non bool additionalfields * add changes to fix the problem * replace sleep with loop and retry for kubectl explain integration test * replaced testdata file with inline create --- .../pkg/explain/v2/templates/plaintext.tmpl | 10 +-- .../explain/v2/templates/plaintext_test.go | 14 ++++ test/cmd/discovery.sh | 70 +++++++++++++++++++ test/cmd/legacy-script.sh | 8 +++ 4 files changed, 98 insertions(+), 4 deletions(-) diff --git a/staging/src/k8s.io/kubectl/pkg/explain/v2/templates/plaintext.tmpl b/staging/src/k8s.io/kubectl/pkg/explain/v2/templates/plaintext.tmpl index 971dfff1f22..618a96128aa 100644 --- a/staging/src/k8s.io/kubectl/pkg/explain/v2/templates/plaintext.tmpl +++ b/staging/src/k8s.io/kubectl/pkg/explain/v2/templates/plaintext.tmpl @@ -118,7 +118,7 @@ Takes dictionary as argument with keys: {{- else if $resolved.items -}} {{- /* If the schema is an array then traverse the array item fields */ -}} {{- template "output" (set $nextContext "schema" $resolved.items) -}} - {{- else if $resolved.additionalProperties -}} + {{- else if and $resolved.additionalProperties (not (eq "bool" (printf "%T" $resolved.additionalProperties))) -}} {{- /* If the schema is a map then traverse the map item fields */ -}} {{- template "output" (set $nextContext "schema" $resolved.additionalProperties) -}} {{- else -}} @@ -182,7 +182,9 @@ Takes dictionary as argument with keys: {{- template "fieldList" (set $nextContext "schema" .)}} {{- end -}} {{- if $resolved.items}}{{- template "fieldList" (set $nextContext "schema" $resolved.items)}}{{end}} - {{- if $resolved.additionalProperties}}{{- template "fieldList" (set $nextContext "schema" $resolved.additionalProperties)}}{{end}} + {{- if and $resolved.additionalProperties (not (eq "bool" (printf "%T" $resolved.additionalProperties))) -}} + {{- template "fieldList" (set $nextContext "schema" $resolved.additionalProperties)}} + {{- end -}} {{- end -}} {{- end -}} @@ -235,7 +237,7 @@ Takes dictionary as argument with keys: {{- if .items -}} {{- template "description" (set $ "schema" .items) -}} {{- end -}} - {{- if .additionalProperties -}} + {{- if and .additionalProperties (not (eq "bool" (printf "%T" .additionalProperties))) -}} {{- template "description" (set $ "schema" .additionalProperties) -}} {{- end -}} {{- end -}} @@ -256,7 +258,7 @@ Takes dictionary as argument with keys: {{- with $.schema -}} {{- if .items -}} []{{template "typeGuess" (set $ "schema" .items)}} - {{- else if .additionalProperties -}} + {{- else if and .additionalProperties (not (eq "bool" (printf "%T" .additionalProperties))) -}} map[string]{{template "typeGuess" (set $ "schema" .additionalProperties)}} {{- else if and .allOf (not .properties) (eq (len .allOf) 1) -}} {{- /* If allOf has a single element and there are no direct diff --git a/staging/src/k8s.io/kubectl/pkg/explain/v2/templates/plaintext_test.go b/staging/src/k8s.io/kubectl/pkg/explain/v2/templates/plaintext_test.go index f209d488ce0..02596d62801 100644 --- a/staging/src/k8s.io/kubectl/pkg/explain/v2/templates/plaintext_test.go +++ b/staging/src/k8s.io/kubectl/pkg/explain/v2/templates/plaintext_test.go @@ -392,6 +392,20 @@ func TestPlaintext(t *testing.T) { checkEquals("string"), }, }, + { + // Shows that the typeguess template works with boolean additionalProperties + Name: "True additionalProperties", + Subtemplate: "typeGuess", + Context: map[string]any{ + "schema": map[string]any{ + "type": "string", + "additionalProperties": true, + }, + }, + Checks: []check{ + checkEquals("string"), + }, + }, { // Show that a ref to a primitive type uses the referred type's type Name: "PrimitiveRef", diff --git a/test/cmd/discovery.sh b/test/cmd/discovery.sh index b8f473e139f..1ea6fd195ef 100755 --- a/test/cmd/discovery.sh +++ b/test/cmd/discovery.sh @@ -407,3 +407,73 @@ __EOF__ set +o nounset set +o errexit } + +run_explain_crd_with_additional_properties_tests() { + set -o nounset + set -o errexit + + create_and_use_new_namespace + kube::log::status "Testing explain with custom CRD that uses additionalProperties as non boolean field" + + kubectl create -f - << __EOF__ +apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + name: mock-resources.test.com +spec: + group: test.com + scope: Namespaced + names: + plural: mock-resources + singular: mock-resource + kind: MockResource + listKind: MockResources + versions: + - name: v1 + storage: true + served: true + schema: + openAPIV3Schema: + type: object + properties: + spec: + type: object + properties: + test: + type: object + additionalProperties: + type: array + items: + type: string + additionalProperties: true +__EOF__ + + kube::test::wait_object_assert customresourcedefinitions "{{range.items}}{{if eq ${id_field:?} \"mock-resources.test.com\"}}{{$id_field}}:{{end}}{{end}}" 'mock-resources.test.com:' + + retries=0 + max_retries=5 + + # First try would get non zero exit code so we disable immediate exit here + set +o errexit + + # Loop until `kubectl explain` returns a zero exit code or maximum retries reached + until output_message=$(kubectl explain mock-resource) > /dev/null || [ $retries -eq $max_retries ]; do + kube::log::status "Retrying kubectl explain..." + ((retries++)) + sleep 1 + done + + set -o errexit + + output_message=$(kubectl explain mock-resource) + kube::test::if_has_string "${output_message}" 'FIELDS:' + + output_message=$(kubectl explain mock-resource --recursive) + kube::test::if_has_string "${output_message}" 'FIELDS:' + + # Cleanup + kubectl delete crd mock-resources.test.com + + set +o nounset + set +o errexit +} diff --git a/test/cmd/legacy-script.sh b/test/cmd/legacy-script.sh index 11d685b2808..10ab751f14b 100755 --- a/test/cmd/legacy-script.sh +++ b/test/cmd/legacy-script.sh @@ -526,6 +526,14 @@ runTests() { record_command run_ambiguous_shortname_tests fi + ################ + # Explain crd # + ################ + + if kube::test::if_supports_resource "${customresourcedefinitions}" ; then + record_command run_explain_crd_with_additional_properties_tests + fi + ######################### # Assert categories # #########################