From 41889984304c44b879d8bcae92c9ed7aa3fd8c67 Mon Sep 17 00:00:00 2001 From: Andreas Karis Date: Tue, 20 Jun 2023 03:34:50 +0200 Subject: [PATCH] Improve conditionFuncFor expression parsing for wait --for jsonpath Make it possible to parse jsonpath filter expressions: Split jsonpath expressions on single '=' only and leave '==' as part of the string. Reported-at: https://github.com/kubernetes/kubernetes/issues/119206 Signed-off-by: Andreas Karis --- .../src/k8s.io/kubectl/pkg/cmd/wait/wait.go | 34 +++++- .../k8s.io/kubectl/pkg/cmd/wait/wait_test.go | 105 +++++++++++++++--- test/cmd/wait.sh | 9 +- 3 files changed, 126 insertions(+), 22 deletions(-) diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/wait/wait.go b/staging/src/k8s.io/kubectl/pkg/cmd/wait/wait.go index 4dfedf010e5..e211ccec6cd 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/wait/wait.go +++ b/staging/src/k8s.io/kubectl/pkg/cmd/wait/wait.go @@ -74,6 +74,9 @@ var ( # Wait for the pod "busybox1" to contain the status phase to be "Running" kubectl wait --for=jsonpath='{.status.phase}'=Running pod/busybox1 + # Wait for pod "busybox1" to be Ready + kubectl wait --for='jsonpath={.status.conditions[?(@.type=="Ready")].status}=True' pod/busybox1 + # Wait for the service "loadbalancer" to have ingress. kubectl wait --for=jsonpath='{.status.loadBalancer.ingress}' service/loadbalancer @@ -209,8 +212,8 @@ func conditionFuncFor(condition string, errOut io.Writer) (ConditionFunc, error) }.IsConditionMet, nil } if strings.HasPrefix(condition, "jsonpath=") { - splitStr := strings.Split(condition, "=") - jsonPathExp, jsonPathValue, err := processJSONPathInput(splitStr[1:]) + jsonPathInput := strings.TrimPrefix(condition, "jsonpath=") + jsonPathExp, jsonPathValue, err := processJSONPathInput(jsonPathInput) if err != nil { return nil, err } @@ -241,8 +244,10 @@ func newJSONPathParser(jsonPathExpression string) (*jsonpath.JSONPath, error) { return j, nil } -// processJSONPathInput will parses the user's JSONPath input containing JSON expression and, optionally, JSON value for matching condition and process it -func processJSONPathInput(jsonPathInput []string) (string, string, error) { +// processJSONPathInput will parse and process the provided JSONPath input containing a JSON expression and optionally +// a value for the matching condition. +func processJSONPathInput(input string) (string, string, error) { + jsonPathInput := splitJSONPathInput(input) if numOfArgs := len(jsonPathInput); numOfArgs < 1 || numOfArgs > 2 { return "", "", fmt.Errorf("jsonpath wait format must be --for=jsonpath='{.status.readyReplicas}'=3 or --for=jsonpath='{.status.readyReplicas}'") } @@ -260,6 +265,27 @@ func processJSONPathInput(jsonPathInput []string) (string, string, error) { return relaxedJSONPathExp, jsonPathValue, nil } +// splitJSONPathInput splits the provided input string on single '='. Double '==' will not cause the string to be +// split. E.g., "a.b.c====d.e.f===g.h.i===" will split to ["a.b.c====d.e.f==","g.h.i==",""]. +func splitJSONPathInput(input string) []string { + var output []string + var element strings.Builder + for i := 0; i < len(input); i++ { + if input[i] == '=' { + if i < len(input)-1 && input[i+1] == '=' { + element.WriteString("==") + i++ + continue + } + output = append(output, element.String()) + element.Reset() + continue + } + element.WriteByte(input[i]) + } + return append(output, element.String()) +} + // ResourceLocation holds the location of a resource type ResourceLocation struct { GroupResource schema.GroupResource diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/wait/wait_test.go b/staging/src/k8s.io/kubectl/pkg/cmd/wait/wait_test.go index 2d01cce5b76..bb03160f490 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/wait/wait_test.go +++ b/staging/src/k8s.io/kubectl/pkg/cmd/wait/wait_test.go @@ -1534,39 +1534,112 @@ func TestWaitForJSONPathCondition(t *testing.T) { } } -// TestWaitForJSONPathBadConditionParsing will test errors in parsing JSONPath bad condition expressions -// except for parsing JSONPath expression itself (i.e. call to cmdget.RelaxedJSONPathExpression()) -func TestWaitForJSONPathBadConditionParsing(t *testing.T) { +// TestConditionFuncFor tests that the condition string can be properly parsed into a ConditionFunc. +func TestConditionFuncFor(t *testing.T) { tests := []struct { - name string - condition string - expectedResult JSONPathWait - expectedErr string + name string + condition string + expectedErr string }{ { - name: "missing JSONPath expression", + name: "jsonpath missing JSONPath expression", condition: "jsonpath=", expectedErr: "jsonpath expression cannot be empty", }, { - name: "value in JSONPath expression has equal sign", + name: "jsonpath check for condition without value", + condition: "jsonpath={.metadata.name}", + expectedErr: None, + }, + { + name: "jsonpath check for condition without value relaxed parsing", + condition: "jsonpath=abc", + expectedErr: None, + }, + { + name: "jsonpath check for expression and value", + condition: "jsonpath={.metadata.name}=foo-b6699dcfb-rnv7t", + expectedErr: None, + }, + { + name: "jsonpath check for expression and value relaxed parsing", + condition: "jsonpath=.metadata.name=foo-b6699dcfb-rnv7t", + expectedErr: None, + }, + { + name: "jsonpath selecting based on condition", + condition: `jsonpath={.status.containerStatuses[?(@.name=="foo")].ready}=True`, + expectedErr: None, + }, + { + name: "jsonpath selecting based on condition relaxed parsing", + condition: "jsonpath=status.conditions[?(@.type==\"Available\")].status=True", + expectedErr: None, + }, + { + name: "jsonpath selecting based on condition without value", + condition: `jsonpath={.status.containerStatuses[?(@.name=="foo")].ready}`, + expectedErr: None, + }, + { + name: "jsonpath selecting based on condition without value relaxed parsing", + condition: `jsonpath=.status.containerStatuses[?(@.name=="foo")].ready`, + expectedErr: None, + }, + { + name: "jsonpath invalid expression with repeated '='", condition: "jsonpath={.metadata.name}='test=wrong'", expectedErr: "jsonpath wait format must be --for=jsonpath='{.status.readyReplicas}'=3 or --for=jsonpath='{.status.readyReplicas}'", }, { - name: "undefined value", + name: "jsonpath undefined value after '='", condition: "jsonpath={.metadata.name}=", - expectedErr: "jsonpath wait has to have a value after equal sign, like --for=jsonpath='{.status.readyReplicas}'=3", + expectedErr: "jsonpath wait has to have a value after equal sign", + }, + { + name: "jsonpath complex expressions not supported", + condition: "jsonpath={.status.conditions[?(@.type==\"Failed\"||@.type==\"Complete\")].status}=True", + expectedErr: "unrecognized character in action: U+007C '|'", + }, + { + name: "jsonpath invalid expression", + condition: "jsonpath={=True", + expectedErr: "unexpected path string, expected a 'name1.name2' or '.name1.name2' or '{name1.name2}' or " + + "'{.name1.name2}'", + }, + { + name: "condition delete", + condition: "delete", + expectedErr: None, + }, + { + name: "condition true", + condition: "condition=hello", + expectedErr: None, + }, + { + name: "condition with value", + condition: "condition=hello=world", + expectedErr: None, + }, + { + name: "unrecognized condition", + condition: "cond=invalid", + expectedErr: "unrecognized condition: \"cond=invalid\"", }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { _, err := conditionFuncFor(test.condition, io.Discard) - if err == nil && test.expectedErr != "" { - t.Fatalf("expected %q, got empty", test.expectedErr) - } - if !strings.Contains(err.Error(), test.expectedErr) { - t.Fatalf("expected %q, got %q", test.expectedErr, err.Error()) + switch { + case err == nil && test.expectedErr != None: + t.Fatalf("expected error %q, got nil", test.expectedErr) + case err != nil && test.expectedErr == None: + t.Fatalf("expected no error, got %q", err) + case err != nil && test.expectedErr != None: + if !strings.Contains(err.Error(), test.expectedErr) { + t.Fatalf("expected error %q, got %q", test.expectedErr, err.Error()) + } } }) } diff --git a/test/cmd/wait.sh b/test/cmd/wait.sh index d33c44d3e61..d8bd7b17226 100644 --- a/test/cmd/wait.sh +++ b/test/cmd/wait.sh @@ -100,11 +100,16 @@ EOF # wait with jsonpath without value to succeed set +o errexit # Command: Wait with jsonpath without value - output_message=$(kubectl wait --for=jsonpath='{.status.replicas}' deploy/test-3 2>&1) + output_message_0=$(kubectl wait --for=jsonpath='{.status.replicas}' deploy/test-3 2>&1) + # Command: Wait with relaxed jsonpath and filter expression + output_message_1=$(kubectl wait \ + --for='jsonpath=spec.template.spec.containers[?(@.name=="busybox")].image=busybox' \ + deploy/test-3) set -o errexit # Post-Condition: Wait succeed - kube::test::if_has_string "${output_message}" 'deployment.apps/test-3 condition met' + kube::test::if_has_string "${output_message_0}" 'deployment.apps/test-3 condition met' + kube::test::if_has_string "${output_message_1}" 'deployment.apps/test-3 condition met' # Clean deployment kubectl delete deployment test-3