Move The 'wait' Argument Handling Logic To Its Exec Function #1630

ihcsim · 2022-09-09T19:59:33Z

Change Overview

This PR moves the handling of the wait function's arguments to its Exec() function, to give it more control over the rendering of the conditions arguments, which are made up of a mixed of Go templates with nested jsonpath variables. This change isolates the rendering of these conditions to the wait function code, allowing the removal of the jsonpath check from the general param.render() function.

The previous implementation of rendering Go templates with nested jsonpath variables doesn't work with templates contains control structure like conditionals and loops. These constructs may refer to root-scoped variables using the $ reference, leading to template validation errors, when these root-scoped variables are being misinterpreted as jsonpath variables by the existing regex. Since Go regex doesn't support negative lookbehind, there isn't an easy way to update the regex to match "only one { and } but not before or after".

The handling of all other arguments remain unchanged.

Pull request type

Please check the type of change your PR introduces:

Issues

fixes Template does not evaluate as expected in the pods field of KubeExecAll #1470

Test Plan

Deploy this Nginx statefulset workload to your cluster:

cat <<EOF apiVersion: apps/v1 | kubectl apply -f -
kind: StatefulSet
metadata:
  name: nginx
  labels:
    app: nginx
spec:
  serviceName: nginx
  replicas: 3
  selector:
    matchLabels:
      app: nginx
  template:
    metadata:
      labels:
        app: nginx
    spec:
      containers:
      - name: nginx
        image: nginx
  volumeClaimTemplates:
  - metadata:
      name: www
    spec:
      accessModes: ["ReadWriteOnce"]
      storageClassName: local-path      # update this to match your setup
      resources:
        requests:
          storage: 1Gi
EOF

Deploy this blueprint to test the KubeExecAll and Wait functions:

cat<<EOF | kubectl apply -f -
apiVersion: cr.kanister.io/v1alpha1
kind: Blueprint
metadata:
  name: exec-all
  namespace: kanister
actions:
  execAll:
    phases:
    - func: KubeExecAll
      name: execAll
      args:
        namespace: "{{ .Object.metadata.namespace }}"
        pods: "{{range until (int .Object.spec.replicas)}} {{ $.Object.metadata.name }}-{{.}}{{end}}"   # should work with range loop with root-scoped variable
        containers: nginx
        command:
        - sh
        - -c
        - |
          echo "hello world"
  execAllErr:
    phases:
    - func: KubeExecAll
      name: execAllErr
      args:
        namespace: "{{ .Object.metadata.namespace }}"
        pods: "{{range until (int .Object.spec.replicas)}} {{ $.Foo.metadata.name }}-{{.}}{{end}}"   # should catch invalid Foo field
        containers: nginx
        command:
        - sh
        - -c
        - |
          echo "hello world"
  checkReady:
    phases:
    - func: Wait
      name: checkReady
      args:
        timeout: 60s
        conditions:
          allOf:
          - condition: '{{ if (eq "{ $.status.readyReplicas }" "3")}}true{{ else }}false{{ end }}'  # should still work with jsonpath variables in wait conditions
            objectReference:
              apiVersion: v1
              group: apps
              resource: statefulsets
              name: "{{ .Object.metadata.name }}"
              namespace: "{{ .Object.metadata.namespace }}"
EOF

Deploy this actionset to trigger the execAll action and expect the actionset to complete successfully:

cat <<EOF | kubectl create -f -
apiVersion: cr.kanister.io/v1alpha1
kind: ActionSet
metadata:
  generateName: exec-all-
  namespace: kanister
spec:
  actions:
  - name: execAll
    blueprint: exec-all
    object:
      kind: StatefulSet
      name: nginx
      namespace: default
EOF

Deploy this actionset to trigger the execAllErr action and expect the actionset to fail and report the unidenfitied Foo field:

cat <<EOF | kubectl create -f -
apiVersion: cr.kanister.io/v1alpha1
kind: ActionSet
metadata:
  generateName: exec-all-err-
  namespace: kanister
spec:
  actions:
  - name: execAll
    blueprint: exec-all
    object:
      kind: StatefulSet
      name: nginx
      namespace: default
EOF

Deploy this actionset to trigger the wait action and expect the actionset to complete successfully:

cat<<EOF | kubectl apply -f -
apiVersion: cr.kanister.io/v1alpha1
kind: ActionSet
metadata:
  generateName: wait-
  namespace: kanister
spec:
  actions:
  - name: checkReady
    blueprint: exec-all
    object:
      kind: StatefulSet
      name: nginx
      namespace: default
EOF

💪 Manual
⚡ Unit test
💚 E2E

This ensures that 'wait' has more control over the rendering of the conditions arguments, which are made up of a mixed of Go template actions and jsonpath variables. The handling of all other arguments remain unchanged. Signed-off-by: Ivan Sim <ivan.sim@kasten.io>

github-actions · 2022-09-09T19:59:47Z

Thanks for submitting this pull request 🎉. The team will review it soon and get back to you.

If you haven't already, please take a moment to review our project contributing guideline and Code of Conduct document.

Signed-off-by: Ivan Sim <ivan.sim@kasten.io>

PrasadG193 · 2022-09-12T05:26:24Z

pkg/phase.go

+	args map[string]interface{},
+	tp param.TemplateParams) (map[string]interface{}, error) {
+	// let wait handle its own go template + jsonpath mixed arguments
+	if strings.ToLower(funcName) == "wait" {


Can we use the declared name from function pkg - function.WaitFuncName

No, it will lead to an import loop.

PrasadG193 · 2022-09-12T05:31:18Z

pkg/function/wait.go

-	var conditions WaitConditions
-	var err error
-	if err = Arg(args, WaitTimeoutArg, &timeout); err != nil {
+	if err := Arg(rendered, WaitTimeoutArg, &timeout); err != nil {


Why do we need rendered arg to parse timeout? As per my understanding, it's always going to be const value.

I prefer to retain the original behaviour where we used the rendered argument in Arg(), in this PR. I am curious - do we support Go template everywhere in the blueprint, or only specific fields?

PrasadG193 · 2022-09-12T05:34:56Z

pkg/function/wait.go

+	objRefRaw := map[string]crv1alpha1.ObjectReference{
+		"objRef": cond.ObjectReference,
+	}
+	rendered, err := param.RenderObjectRefs(objRefRaw, tp)
+	if err != nil {
+		return false, err
+	}
+	objRef := rendered["objRef"]
+


Should we move this to a separate helper function?

michaelcourcy · 2022-09-12T09:09:48Z

@ihcsim I've been testing on the couchbase cluster example this blueprint

apiVersion: cr.kanister.io/v1alpha1
kind: Blueprint
metadata:
  # test PR 1630 https://github.com/kanisterio/kanister/pull/1630
  name: couchbase-blueprint-test-1630
  namespace: kasten-io
actions:
  backup:
    kind: CustomResource
    phases:
    - func: KubeExecAll
      name: testExecAll      
      args:
        namespace: "{{ .Object.metadata.namespace }}"
        containers: couchbase-server
        pods: "{{range until (int (index .Object.spec.servers 0).size )}} {{ $.Object.metadata.name }}-000{{.}}{{end}}"
        command:
          - bash
          - -o
          - pipefail
          - -o
          - errexit
          - -c
          - |
            touch /tmp/execAll-1630
    - func: Wait
      name: waitCouchbaseClusterReadyAndBalanced
      args:
        timeout: 60s
        conditions:
          allOf:
            - condition: '{{$available := false}}{{ range $condition := $.status.conditions }}{{ if and (eq .type "Available") (eq .status "True")  }}{{$available=true}}{{ end }}{{ end }}{{ $available }}'
              objectReference:
                apiVersion: couchbase.com/v2
                resource: couchbaseclusters
                name: "{{ .Object.Name }}"
                namespace: "{{ .Object.namespace}}"
            - condition: '{{$balanced := false}}{{ range $condition := $.status.conditions }}{{ if and (eq .type "Balanced") (eq .status "True")  }}{{$balanced=true}}{{ end }}{{ end }}{{ $balanced }}'
              objectReference:
                apiVersion: couchbase.com/v2
                resource: couchbaseclusters
                name: "{{ .Object.Name }}"
                namespace: "{{ .Object.namespace}}"

The first func kubeExecAll work as expected but the second has this error

error:
                      message: "template: config:1: bad character U+003D '='"

In your example I can see that you use { and } inside a go template expression

condition: '{{ if (eq "{ $.status.readyReplicas }" "3")}}true{{ else }}false{{ end }}'  # should still work with jsonpath variables in wait conditions

I'm not sure I understand why you are using that, is it to evaluate a jsonpath expression or the contrary avoid the evaluation as a jsonpath ?

My understanding was that now we could use plain go-template in conditions but I may have misunderstood.

ihcsim · 2022-09-12T14:38:07Z

@michaelcourcy thanks for trying it out. I thought the proper jsonpath syntax requires the { and } delimiter, i.e. ${.status.conditions}. See original code here.

Signed-off-by: Ivan Sim <ivan.sim@kasten.io>

PrasadG193 · 2022-09-13T06:49:06Z

I am working on switching to go-template syntax for fetching k8s object value in wait condition. Can we hold off on the PR till then?

michaelcourcy · 2022-09-13T07:19:30Z

I am working on switching to go-template syntax for fetching k8s object value in wait condition. Can we hold off on the PR till then?

I really would prefer that personally, it's a bit ubiquitous to switch from jsonpath to go-template in the same expression.

PrasadG193 · 2022-09-13T11:58:25Z

I have raised - #1635 on top of the current PR

* Switch to go template syntax for Wait conditions Signed-off-by: Prasad Ghangal <prasad.ghangal@gmail.com> * Update K8ssandra BP as per the new Wait func syntax Signed-off-by: Prasad Ghangal <prasad.ghangal@gmail.com> Signed-off-by: Prasad Ghangal <prasad.ghangal@gmail.com>

github-actions · 2022-12-31T00:08:59Z

This PR is marked as stale due to inactivity. Add a new comment to reactivate it.

PrasadG193 · 2023-01-02T06:48:26Z

Since we decided to implement a new function which supports only go-template syntax. This PR needs to be refactored to remove the changes in wait function implementation to fix only #1470

) This reverts commit 2be38c8.

pavannd1 · 2023-01-06T19:46:49Z

@PrasadG193 I have approved this. Do you want to merge this and refactor the required things in your PR?

ihcsim requested review from pavannd1 and PrasadG193 September 9, 2022 19:59

infraq added this to In Progress in Kanister Sep 9, 2022

Fix unit test

f0b309e

Signed-off-by: Ivan Sim <ivan.sim@kasten.io>

ihcsim force-pushed the fix-go-template-jsonpath branch from 2d005f9 to f0b309e Compare September 9, 2022 23:18

PrasadG193 reviewed Sep 12, 2022

View reviewed changes

ihcsim added 2 commits September 12, 2022 14:00

Fix unit test

0b541db

Signed-off-by: Ivan Sim <ivan.sim@kasten.io>

Fix another unit test issue

bb7c5b3

Signed-off-by: Ivan Sim <ivan.sim@kasten.io>

ihcsim mentioned this pull request Oct 3, 2022

Switch to go template syntax for Wait conditions #1635

Merged

11 tasks

PrasadG193 added the hold off merging label Oct 31, 2022

github-actions bot added the stale label Dec 31, 2022

PrasadG193 added 2 commits January 2, 2023 12:27

Revert "Switch to go template syntax for Wait conditions (#1635)" (#1833

563d84f

) This reverts commit 2be38c8.

Merge branch 'master' into fix-go-template-jsonpath

9ab55c4

github-actions bot removed the stale label Jan 3, 2023

Merge branch 'master' into fix-go-template-jsonpath

25e9dd3

PrasadG193 removed the hold off merging label Jan 4, 2023

pavannd1 approved these changes Jan 6, 2023

View reviewed changes

Kanister automation moved this from In Progress to Reviewer approved Jan 6, 2023

PrasadG193 added the kueue label Jan 9, 2023

Merge branch 'master' into fix-go-template-jsonpath

807f105

mergify bot merged commit 15157fb into master Jan 9, 2023

Kanister automation moved this from Reviewer approved to Done Jan 9, 2023

mergify bot deleted the fix-go-template-jsonpath branch January 9, 2023 19:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move The 'wait' Argument Handling Logic To Its Exec Function #1630

Move The 'wait' Argument Handling Logic To Its Exec Function #1630

ihcsim commented Sep 9, 2022

github-actions bot commented Sep 9, 2022

PrasadG193 Sep 12, 2022

ihcsim Sep 12, 2022

PrasadG193 Sep 12, 2022

ihcsim Sep 12, 2022

PrasadG193 Sep 12, 2022

michaelcourcy commented Sep 12, 2022 •

edited

Loading

ihcsim commented Sep 12, 2022

PrasadG193 commented Sep 13, 2022

michaelcourcy commented Sep 13, 2022

PrasadG193 commented Sep 13, 2022

github-actions bot commented Dec 31, 2022

PrasadG193 commented Jan 2, 2023

pavannd1 commented Jan 6, 2023

Move The 'wait' Argument Handling Logic To Its Exec Function #1630

Move The 'wait' Argument Handling Logic To Its Exec Function #1630

Conversation

ihcsim commented Sep 9, 2022

Change Overview

Pull request type

Issues

Test Plan

github-actions bot commented Sep 9, 2022

PrasadG193 Sep 12, 2022

Choose a reason for hiding this comment

ihcsim Sep 12, 2022

Choose a reason for hiding this comment

PrasadG193 Sep 12, 2022

Choose a reason for hiding this comment

ihcsim Sep 12, 2022

Choose a reason for hiding this comment

PrasadG193 Sep 12, 2022

Choose a reason for hiding this comment

michaelcourcy commented Sep 12, 2022 • edited Loading

ihcsim commented Sep 12, 2022

PrasadG193 commented Sep 13, 2022

michaelcourcy commented Sep 13, 2022

PrasadG193 commented Sep 13, 2022

github-actions bot commented Dec 31, 2022

PrasadG193 commented Jan 2, 2023

pavannd1 commented Jan 6, 2023

michaelcourcy commented Sep 12, 2022 •

edited

Loading