Escaping of " in values for restore modified patches doesn't work

[aschleifer]

I have the following modifier:

  - conditions:
      groupResource: configmaps
      resourceNameRegex: "^sample-configmap"
      namespaces:
        - default
    patches:
      - operation: replace
        path: "/data/TEST_ENV_VAR"
        value: "\"true\""

The original configmap has this:

data:
  TEST_ENV_VAR: "false"

When executing this modifier during a restore I get this error:

Errors:
  Velero:     <none>
  Cluster:    <none>
  Namespaces:
    exb:  error in decoding json patch invalid character 't' after object key:value pair

This is happening with these velero versioons:

Client:
        Version: v1.12.0
        Git commit: 7112c62e493b0f7570f0e7cd2088f8cad968db99
Server:
        Version: v1.12.0

[aschleifer]

I forgot to mention the configuration with the \" was taken from https://velero.io/docs/main/restore-resource-modifiers/#advanced-scenarios

[mateusoliveira43]

I think its not possible to make this change today, because false and true strings are converted to boolean, instead of strings. Reference:

velero/internal/resourcemodifiers/json_patch.go

Lines 36 to 39 in d42505d

	// if value is a boolean, then don't add quotes
	if _, err := strconv.ParseBool(value); err == nil {
	return false
	}

I beleive what your operation is doing is changing "false" to ""true"".

@anshulahuja98 what you think?

[blackpiglet]

I think the reason is the \ is not handled correctly in code.
The generated patch string is:

{\"op\": \"replace\", \"from\": \"\", \"path\": \"/data/test\", \"value\": \"\"true\"\"}"

It should be:

{\"op\": \"replace\", \"from\": \"\", \"path\": \"/data/test\", \"value\": \"\\"true\"\\"}"

Although the example has \ character, it is used in a user-organized string.
@anshulahuja98 Please check whether this is a valid use case.

[Segaja]

I think I also tried it with multiple \ but couldn't get it to work.

/edit: Sorry this is my private account, but I'm the author of the bug report.

[blackpiglet]

Make it work with this ConfigMap setting.

apiVersion: v1
data:
  modify_deploy.yaml: |
    version: v1
    resourceModifierRules:
    - conditions:
        groupResource: configmaps
        resourceNameRegex: "test"
        namespaces:
        - upgrade
      patches:
      - operation: replace
        path: "/data/test"
        value: "\\\"true\\\""
kind: ConfigMap
metadata:
  name: modify-deploy
  namespace: velero

apiVersion: v1
data:
  test: '"true"'
kind: ConfigMap
metadata:
  creationTimestamp: "2023-11-17T01:21:32Z"
  labels:
    velero.io/backup-name: test-01
    velero.io/restore-name: test-01-20231117092120
  name: test
  namespace: upgrade
  resourceVersion: "130749607"
  uid: 63ab5b05-041b-4bfc-912e-61315b758e98

[anshulahuja98]

So overall the ask is to support putting values where "true" / "false" is a string rather than a bool.
I think that is a fair ask, and we should have a sample in the docs on how to achieve this.

The second scenario is where / is part of an actual user value string. A probable solution can be to leverage
`
Add unit test case to evaluate and support ~1 based escaping #6801 (comment)

as called out in the issue for enhancements #7050

[anshulahuja98]

Let me try out few POCs and get back.
Should mostly be a documentation thing than a code fix.

[Segaja]

Make it work with this ConfigMap setting.

apiVersion: v1
data:
  modify_deploy.yaml: |
    version: v1
    resourceModifierRules:
    - conditions:
        groupResource: configmaps
        resourceNameRegex: "test"
        namespaces:
        - upgrade
      patches:
      - operation: replace
        path: "/data/test"
        value: "\\\"true\\\""
kind: ConfigMap
metadata:
  name: modify-deploy
  namespace: velero

apiVersion: v1
data:
  test: '"true"'
kind: ConfigMap
metadata:
  creationTimestamp: "2023-11-17T01:21:32Z"
  labels:
    velero.io/backup-name: test-01
    velero.io/restore-name: test-01-20231117092120
  name: test
  namespace: upgrade
  resourceVersion: "130749607"
  uid: 63ab5b05-041b-4bfc-912e-61315b758e98

I have tried that before and the service who is reading this data.test value was not happy about this. So this doesn't work either.

[anshulahuja98]

func ParseBool(str string) (bool, error) {
switch str {
case "1", "t", "T", "true", "TRUE", "True":
return true, nil
case "0", "f", "F", "false", "FALSE", "False":
return false, nil
}
return false, syntaxError("ParseBool", str)
}

Found some more issues
parse bool will actually catch these scenarios. Whereas we just built it for true /false

checkig further a proper fix for this.