From 0c921b2f35e9186403962c6ae7d15b34ee570ea8 Mon Sep 17 00:00:00 2001 From: minherz Date: Sun, 21 May 2023 19:03:24 -0700 Subject: [PATCH 1/3] Support JSONPath condition without value Extend current JSONPath condition logic to return from wait on "any" value. Change parsing JSONPath input to support the syntax without value. Match any simple or complex (object or array) values. Kubernetes-commit: 9d3e55ec431f3f595a7739fcc592602f7cc1d69b --- pkg/cmd/wait/wait.go | 50 ++++++----- pkg/cmd/wait/wait_test.go | 172 ++++++++++++++++++++++++++------------ 2 files changed, 150 insertions(+), 72 deletions(-) diff --git a/pkg/cmd/wait/wait.go b/pkg/cmd/wait/wait.go index dbbbd66a..04955b2e 100644 --- a/pkg/cmd/wait/wait.go +++ b/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 the service "loadbalancer" to have ingress. + kubectl wait --for=jsonpath='{.status.loadBalancer.ingress}' service/loadbalancer + # Wait for the pod "busybox1" to be deleted, with a timeout of 60s, after having issued the "delete" command kubectl delete pod/busybox1 kubectl wait --for=delete pod/busybox1 --timeout=60s`)) @@ -120,7 +123,7 @@ func NewCmdWait(restClientGetter genericclioptions.RESTClientGetter, streams gen flags := NewWaitFlags(restClientGetter, streams) cmd := &cobra.Command{ - Use: "wait ([-f FILENAME] | resource.group/resource.name | resource.group [(-l label | --all)]) [--for=delete|--for condition=available|--for=jsonpath='{}'=value]", + Use: "wait ([-f FILENAME] | resource.group/resource.name | resource.group [(-l label | --all)]) [--for=delete|--for condition=available|--for=jsonpath='{}'[=value]]", Short: i18n.T("Experimental: Wait for a specific condition on one or many resources"), Long: waitLong, Example: waitExample, @@ -145,7 +148,7 @@ func (flags *WaitFlags) AddFlags(cmd *cobra.Command) { flags.ResourceBuilderFlags.AddFlags(cmd.Flags()) cmd.Flags().DurationVar(&flags.Timeout, "timeout", flags.Timeout, "The length of time to wait before giving up. Zero means check once and don't wait, negative means wait for a week.") - cmd.Flags().StringVar(&flags.ForCondition, "for", flags.ForCondition, "The condition to wait on: [delete|condition=condition-name[=condition-value]|jsonpath='{JSONPath expression}'=JSONPath Condition]. The default condition-value is true. Condition values are compared after Unicode simple case folding, which is a more general form of case-insensitivity.") + cmd.Flags().StringVar(&flags.ForCondition, "for", flags.ForCondition, "The condition to wait on: [delete|condition=condition-name[=condition-value]|jsonpath='{JSONPath expression}'=[JSONPath value]]. The default condition-value is true. Condition values are compared after Unicode simple case folding, which is a more general form of case-insensitivity.") } // ToOptions converts from CLI inputs to runtime inputs @@ -207,10 +210,7 @@ func conditionFuncFor(condition string, errOut io.Writer) (ConditionFunc, error) } if strings.HasPrefix(condition, "jsonpath=") { splitStr := strings.Split(condition, "=") - if len(splitStr) != 3 { - return nil, fmt.Errorf("jsonpath wait format must be --for=jsonpath='{.status.readyReplicas}'=3") - } - jsonPathExp, jsonPathCond, err := processJSONPathInput(splitStr[1], splitStr[2]) + jsonPathExp, jsonPathValue, err := processJSONPathInput(splitStr[1:]) if err != nil { return nil, err } @@ -219,9 +219,10 @@ func conditionFuncFor(condition string, errOut io.Writer) (ConditionFunc, error) return nil, err } return JSONPathWait{ - jsonPathCondition: jsonPathCond, - jsonPathParser: j, - errOut: errOut, + matchAnyValue: jsonPathValue == "", + jsonPathValue: jsonPathValue, + jsonPathParser: j, + errOut: errOut, }.IsJSONPathConditionMet, nil } @@ -240,18 +241,23 @@ func newJSONPathParser(jsonPathExpression string) (*jsonpath.JSONPath, error) { return j, nil } -// processJSONPathInput will parses the user's JSONPath input and process the string -func processJSONPathInput(jsonPathExpression, jsonPathCond string) (string, string, error) { - relaxedJSONPathExp, err := cmdget.RelaxedJSONPathExpression(jsonPathExpression) +// 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) { + if numOfArgs := len(jsonPathInput); 1 < numOfArgs || numOfArgs > 2 { + return "", "", fmt.Errorf("jsonpath wait format must be --for=jsonpath='{.status.readyReplicas}'=3 or --for=jsonpath='{.status.readyReplicas}'") + } + relaxedJSONPathExp, err := cmdget.RelaxedJSONPathExpression(jsonPathInput[0]) if err != nil { return "", "", err } - if jsonPathCond == "" { - return "", "", errors.New("jsonpath wait condition cannot be empty") + if len(jsonPathInput) > 1 { + jsonPathValue := jsonPathInput[1] + if jsonPathValue == "" { + return "", "", errors.New("jsonpath wait value cannot be empty") + } + return relaxedJSONPathExp, strings.Trim(jsonPathValue, `'"`), nil } - jsonPathCond = strings.Trim(jsonPathCond, `'"`) - - return relaxedJSONPathExp, jsonPathCond, nil + return relaxedJSONPathExp, "", nil } // ResourceLocation holds the location of a resource @@ -590,8 +596,9 @@ func getObservedGeneration(obj *unstructured.Unstructured, condition map[string] // JSONPathWait holds a JSONPath Parser which has the ability // to check for the JSONPath condition and compare with the API server provided JSON output. type JSONPathWait struct { - jsonPathCondition string - jsonPathParser *jsonpath.JSONPath + matchAnyValue bool + jsonPathValue string + jsonPathParser *jsonpath.JSONPath // errOut is written to if an error occurs errOut io.Writer } @@ -635,7 +642,10 @@ func (j JSONPathWait) checkCondition(obj *unstructured.Unstructured) (bool, erro if err := verifyParsedJSONPath(parseResults); err != nil { return false, err } - isConditionMet, err := compareResults(parseResults[0][0], j.jsonPathCondition) + if j.matchAnyValue { + return true, nil + } + isConditionMet, err := compareResults(parseResults[0][0], j.jsonPathValue) if err != nil { return false, err } diff --git a/pkg/cmd/wait/wait_test.go b/pkg/cmd/wait/wait_test.go index d1ff60fa..50d2b0a0 100644 --- a/pkg/cmd/wait/wait_test.go +++ b/pkg/cmd/wait/wait_test.go @@ -1027,10 +1027,11 @@ func TestWaitForDifferentJSONPathExpression(t *testing.T) { } tests := []struct { - name string - fakeClient func() *dynamicfakeclient.FakeDynamicClient - jsonPathExp string - jsonPathCond string + name string + fakeClient func() *dynamicfakeclient.FakeDynamicClient + jsonPathExp string + jsonPathValue string + matchAnyValue bool expectedErr string }{ @@ -1041,8 +1042,9 @@ func TestWaitForDifferentJSONPathExpression(t *testing.T) { fakeClient.PrependReactor("list", "theresource", listReactionfunc) return fakeClient }, - jsonPathExp: "{.foo.bar}", - jsonPathCond: "baz", + jsonPathExp: "{.foo.bar}", + jsonPathValue: "baz", + matchAnyValue: false, expectedErr: "timed out waiting for the condition on theresource/foo-b6699dcfb-rnv7t", }, @@ -1053,8 +1055,9 @@ func TestWaitForDifferentJSONPathExpression(t *testing.T) { fakeClient.PrependReactor("list", "theresource", listReactionfunc) return fakeClient }, - jsonPathExp: "{.status.containerStatuses[0].ready}", - jsonPathCond: "true", + jsonPathExp: "{.status.containerStatuses[0].ready}", + jsonPathValue: "true", + matchAnyValue: false, expectedErr: None, }, @@ -1065,8 +1068,9 @@ func TestWaitForDifferentJSONPathExpression(t *testing.T) { fakeClient.PrependReactor("list", "theresource", listReactionfunc) return fakeClient }, - jsonPathExp: "{.status.containerStatuses[0].ready}", - jsonPathCond: "false", + jsonPathExp: "{.status.containerStatuses[0].ready}", + jsonPathValue: "false", + matchAnyValue: false, expectedErr: "timed out waiting for the condition on theresource/foo-b6699dcfb-rnv7t", }, @@ -1077,8 +1081,9 @@ func TestWaitForDifferentJSONPathExpression(t *testing.T) { fakeClient.PrependReactor("list", "theresource", listReactionfunc) return fakeClient }, - jsonPathExp: "{.spec.containers[0].ports[0].containerPort}", - jsonPathCond: "80", + jsonPathExp: "{.spec.containers[0].ports[0].containerPort}", + jsonPathValue: "80", + matchAnyValue: false, expectedErr: None, }, @@ -1089,8 +1094,9 @@ func TestWaitForDifferentJSONPathExpression(t *testing.T) { fakeClient.PrependReactor("list", "theresource", listReactionfunc) return fakeClient }, - jsonPathExp: "{.spec.containers[0].ports[0].containerPort}", - jsonPathCond: "81", + jsonPathExp: "{.spec.containers[0].ports[0].containerPort}", + jsonPathValue: "81", + matchAnyValue: false, expectedErr: "timed out waiting for the condition on theresource/foo-b6699dcfb-rnv7t", }, @@ -1101,11 +1107,41 @@ func TestWaitForDifferentJSONPathExpression(t *testing.T) { fakeClient.PrependReactor("list", "theresource", listReactionfunc) return fakeClient }, - jsonPathExp: "{.spec.nodeName}", - jsonPathCond: "knode0", + jsonPathExp: "{.spec.nodeName}", + jsonPathValue: "knode0", + matchAnyValue: false, expectedErr: None, }, + { + name: "matches literal value of JSONPath entry without value condition", + fakeClient: func() *dynamicfakeclient.FakeDynamicClient { + fakeClient := dynamicfakeclient.NewSimpleDynamicClientWithCustomListKinds(scheme, listMapping) + fakeClient.PrependReactor("list", "theresource", listReactionfunc) + return fakeClient + }, + jsonPathExp: "{.spec.nodeName}", + jsonPathValue: "", + matchAnyValue: true, + + expectedErr: None, + }, + { + name: "matches complex types map[string]interface{} without value condition", + fakeClient: func() *dynamicfakeclient.FakeDynamicClient { + fakeClient := dynamicfakeclient.NewSimpleDynamicClientWithCustomListKinds(scheme, listMapping) + fakeClient.PrependReactor("list", "theresource", func(action clienttesting.Action) (handled bool, ret runtime.Object, err error) { + return true, newUnstructuredList(createUnstructured(t, podYAML)), nil + }) + return fakeClient + }, + jsonPathExp: "{.spec}", + jsonPathValue: "", + matchAnyValue: true, + + expectedErr: None, + }, + { name: "compare string JSONPath entry wrong value", fakeClient: func() *dynamicfakeclient.FakeDynamicClient { @@ -1113,8 +1149,9 @@ func TestWaitForDifferentJSONPathExpression(t *testing.T) { fakeClient.PrependReactor("list", "theresource", listReactionfunc) return fakeClient }, - jsonPathExp: "{.spec.nodeName}", - jsonPathCond: "kmaster", + jsonPathExp: "{.spec.nodeName}", + jsonPathValue: "kmaster", + matchAnyValue: false, expectedErr: "timed out waiting for the condition on theresource/foo-b6699dcfb-rnv7t", }, @@ -1125,8 +1162,22 @@ func TestWaitForDifferentJSONPathExpression(t *testing.T) { fakeClient.PrependReactor("list", "theresource", listReactionfunc) return fakeClient }, - jsonPathExp: "{.status.conditions[*]}", - jsonPathCond: "foo", + jsonPathExp: "{.status.conditions[*]}", + jsonPathValue: "foo", + matchAnyValue: false, + + expectedErr: "given jsonpath expression matches more than one value", + }, + { + name: "matches more than one value without value condition", + fakeClient: func() *dynamicfakeclient.FakeDynamicClient { + fakeClient := dynamicfakeclient.NewSimpleDynamicClientWithCustomListKinds(scheme, listMapping) + fakeClient.PrependReactor("list", "theresource", listReactionfunc) + return fakeClient + }, + jsonPathExp: "{.status.conditions[*]}", + jsonPathValue: "", + matchAnyValue: true, expectedErr: "given jsonpath expression matches more than one value", }, @@ -1137,8 +1188,22 @@ func TestWaitForDifferentJSONPathExpression(t *testing.T) { fakeClient.PrependReactor("list", "theresource", listReactionfunc) return fakeClient }, - jsonPathExp: "{range .status.conditions[*]}[{.status}] {end}", - jsonPathCond: "foo", + jsonPathExp: "{range .status.conditions[*]}[{.status}] {end}", + jsonPathValue: "foo", + matchAnyValue: false, + + expectedErr: "given jsonpath expression matches more than one list", + }, + { + name: "matches more than one list without value condition", + fakeClient: func() *dynamicfakeclient.FakeDynamicClient { + fakeClient := dynamicfakeclient.NewSimpleDynamicClientWithCustomListKinds(scheme, listMapping) + fakeClient.PrependReactor("list", "theresource", listReactionfunc) + return fakeClient + }, + jsonPathExp: "{range .status.conditions[*]}[{.status}] {end}", + jsonPathValue: "", + matchAnyValue: true, expectedErr: "given jsonpath expression matches more than one list", }, @@ -1149,8 +1214,9 @@ func TestWaitForDifferentJSONPathExpression(t *testing.T) { fakeClient.PrependReactor("list", "theresource", listReactionfunc) return fakeClient }, - jsonPathExp: "{.status.conditions}", - jsonPathCond: "True", + jsonPathExp: "{.status.conditions}", + jsonPathValue: "True", + matchAnyValue: false, expectedErr: "jsonpath leads to a nested object or list which is not supported", }, @@ -1163,8 +1229,9 @@ func TestWaitForDifferentJSONPathExpression(t *testing.T) { }) return fakeClient }, - jsonPathExp: "{.spec}", - jsonPathCond: "foo", + jsonPathExp: "{.spec}", + jsonPathValue: "foo", + matchAnyValue: false, expectedErr: "jsonpath leads to a nested object or list which is not supported", }, @@ -1180,9 +1247,10 @@ func TestWaitForDifferentJSONPathExpression(t *testing.T) { Printer: printers.NewDiscardingPrinter(), ConditionFn: JSONPathWait{ - jsonPathCondition: test.jsonPathCond, - jsonPathParser: j, - errOut: io.Discard}.IsJSONPathConditionMet, + matchAnyValue: test.matchAnyValue, + jsonPathValue: test.jsonPathValue, + jsonPathParser: j, + errOut: io.Discard}.IsJSONPathConditionMet, IOStreams: genericiooptions.NewTestIOStreamsDiscard(), } @@ -1212,12 +1280,12 @@ func TestWaitForJSONPathCondition(t *testing.T) { } tests := []struct { - name string - infos []*resource.Info - fakeClient func() *dynamicfakeclient.FakeDynamicClient - timeout time.Duration - jsonPathExp string - jsonPathCond string + name string + infos []*resource.Info + fakeClient func() *dynamicfakeclient.FakeDynamicClient + timeout time.Duration + jsonPathExp string + jsonPathValue string expectedErr string }{ @@ -1240,9 +1308,9 @@ func TestWaitForJSONPathCondition(t *testing.T) { }) return fakeClient }, - timeout: 3 * time.Second, - jsonPathExp: "{.metadata.name}", - jsonPathCond: "foo-b6699dcfb-rnv7t", + timeout: 3 * time.Second, + jsonPathExp: "{.metadata.name}", + jsonPathValue: "foo-b6699dcfb-rnv7t", expectedErr: None, }, @@ -1330,9 +1398,9 @@ func TestWaitForJSONPathCondition(t *testing.T) { }) return fakeClient }, - timeout: 3 * time.Second, - jsonPathExp: "{.metadata.name}", - jsonPathCond: "foo", // use incorrect name so it'll keep waiting + timeout: 3 * time.Second, + jsonPathExp: "{.metadata.name}", + jsonPathValue: "foo", // use incorrect name so it'll keep waiting expectedErr: "timed out waiting for the condition on theresource/foo-b6699dcfb-rnv7t", }, @@ -1360,9 +1428,9 @@ func TestWaitForJSONPathCondition(t *testing.T) { }) return fakeClient }, - timeout: 10 * time.Second, - jsonPathExp: "{.metadata.name}", - jsonPathCond: "foo-b6699dcfb-rnv7t", + timeout: 10 * time.Second, + jsonPathExp: "{.metadata.name}", + jsonPathValue: "foo-b6699dcfb-rnv7t", expectedErr: None, }, @@ -1385,9 +1453,9 @@ func TestWaitForJSONPathCondition(t *testing.T) { }) return fakeClient }, - timeout: 1 * time.Second, - jsonPathExp: "{.spec.containers[0].image}", - jsonPathCond: "nginx", + timeout: 1 * time.Second, + jsonPathExp: "{.spec.containers[0].image}", + jsonPathValue: "nginx", expectedErr: None, }, @@ -1426,9 +1494,9 @@ func TestWaitForJSONPathCondition(t *testing.T) { }) return fakeClient }, - timeout: 10 * time.Second, - jsonPathExp: "{.metadata.name}", - jsonPathCond: "foo-b6699dcfb-rnv7t", + timeout: 10 * time.Second, + jsonPathExp: "{.metadata.name}", + jsonPathValue: "foo-b6699dcfb-rnv7t", expectedErr: None, }, @@ -1444,8 +1512,8 @@ func TestWaitForJSONPathCondition(t *testing.T) { Printer: printers.NewDiscardingPrinter(), ConditionFn: JSONPathWait{ - jsonPathCondition: test.jsonPathCond, - jsonPathParser: j, errOut: io.Discard}.IsJSONPathConditionMet, + jsonPathValue: test.jsonPathValue, + jsonPathParser: j, errOut: io.Discard}.IsJSONPathConditionMet, IOStreams: genericiooptions.NewTestIOStreamsDiscard(), } From baf5b1c9997d10c45a4ba9db0fa3d64e9048b019 Mon Sep 17 00:00:00 2001 From: minherz Date: Sat, 3 Jun 2023 21:40:16 -0700 Subject: [PATCH 2/3] chore(fix): test jsonpath condition parsing errors Test parsing logic for invalid JSONPath condition formats, excluding JSON path expression parsing. Fix error in parsing logic Kubernetes-commit: a5c4fbe979188335f4f414a1aef303f1c0f353f6 --- pkg/cmd/wait/wait.go | 2 +- pkg/cmd/wait/wait_test.go | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/wait/wait.go b/pkg/cmd/wait/wait.go index 04955b2e..5f41566a 100644 --- a/pkg/cmd/wait/wait.go +++ b/pkg/cmd/wait/wait.go @@ -243,7 +243,7 @@ func newJSONPathParser(jsonPathExpression string) (*jsonpath.JSONPath, error) { // 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) { - if numOfArgs := len(jsonPathInput); 1 < numOfArgs || numOfArgs > 2 { + 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}'") } relaxedJSONPathExp, err := cmdget.RelaxedJSONPathExpression(jsonPathInput[0]) diff --git a/pkg/cmd/wait/wait_test.go b/pkg/cmd/wait/wait_test.go index 50d2b0a0..2923b7bc 100644 --- a/pkg/cmd/wait/wait_test.go +++ b/pkg/cmd/wait/wait_test.go @@ -1533,3 +1533,41 @@ 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) { + tests := []struct { + name string + condition string + expectedResult JSONPathWait + expectedErr string + }{ + { + name: "missing JSONPath expression", + condition: "jsonpath=", + expectedErr: "jsonpath expression cannot be empty", + }, + { + name: "value in JSONPath expression has equal sign", + 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", + condition: "jsonpath={.metadata.name}=", + expectedErr: "jsonpath wait value cannot be empty", + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + _, err := conditionFuncFor(test.condition, io.Discard) + if err == nil { + 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()) + } + }) + } +} From bd48fd4c165349ab529fcaf27df872705ec1d105 Mon Sep 17 00:00:00 2001 From: minherz Date: Thu, 29 Jun 2023 00:36:07 -0700 Subject: [PATCH 3/3] chore: address review feedback add integration test to wait for json without value refactor JSON condition value parsing and validating adjusting test to reflect the error message refactoring Kubernetes-commit: dbdd861ea366af50fb74983426587dad7222cb89 --- pkg/cmd/wait/wait.go | 14 +++++++------- pkg/cmd/wait/wait_test.go | 4 ++-- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/pkg/cmd/wait/wait.go b/pkg/cmd/wait/wait.go index 5f41566a..4dfedf01 100644 --- a/pkg/cmd/wait/wait.go +++ b/pkg/cmd/wait/wait.go @@ -250,14 +250,14 @@ func processJSONPathInput(jsonPathInput []string) (string, string, error) { if err != nil { return "", "", err } - if len(jsonPathInput) > 1 { - jsonPathValue := jsonPathInput[1] - if jsonPathValue == "" { - return "", "", errors.New("jsonpath wait value cannot be empty") - } - return relaxedJSONPathExp, strings.Trim(jsonPathValue, `'"`), nil + if len(jsonPathInput) == 1 { + return relaxedJSONPathExp, "", nil } - return relaxedJSONPathExp, "", nil + jsonPathValue := strings.Trim(jsonPathInput[1], `'"`) + if jsonPathValue == "" { + return "", "", errors.New("jsonpath wait has to have a value after equal sign, like --for=jsonpath='{.status.readyReplicas}'=3") + } + return relaxedJSONPathExp, jsonPathValue, nil } // ResourceLocation holds the location of a resource diff --git a/pkg/cmd/wait/wait_test.go b/pkg/cmd/wait/wait_test.go index 2923b7bc..2d01cce5 100644 --- a/pkg/cmd/wait/wait_test.go +++ b/pkg/cmd/wait/wait_test.go @@ -1556,13 +1556,13 @@ func TestWaitForJSONPathBadConditionParsing(t *testing.T) { { name: "undefined value", condition: "jsonpath={.metadata.name}=", - expectedErr: "jsonpath wait value cannot be empty", + expectedErr: "jsonpath wait has to have a value after equal sign, like --for=jsonpath='{.status.readyReplicas}'=3", }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { _, err := conditionFuncFor(test.condition, io.Discard) - if err == nil { + if err == nil && test.expectedErr != "" { t.Fatalf("expected %q, got empty", test.expectedErr) } if !strings.Contains(err.Error(), test.expectedErr) {