Repository: kubermatic/machine-controller
Pull request #1367 (2022-07-21 02:54:45)

Fix unmarshalling for `configvarstring` in cloudProviderSpec
Signed-off-by: Waleed Malik <ahmedwaleedmalik@gmail.com>

**What this PR does / why we need it**:
To keep strings safe to embed inside HTML `json.Marshal()` encodes them using [HTMLEscape](https://github.com/golang/go/blob/go1.18.4/src/encoding/json/encode.go#L161). It replaces invalid bytes with the Unicode replacement rune. For example "<", ">" will be converted to "\u003c","\u003e", respectively. `json.Unmarshal` takes care of replacing these escape characters with the actual value. 

In general, this works fine for us but in the case of `ConfigVarString` we override the unmarshalling and marshalling behavior. And Unmarshalling [doesn't replace the escape characters](https://github.com/kubermatic/machine-controller/blob/master/pkg/providerconfig/types/types.go#L164).

This PR reworks the `ConfigVarString` unmashalling to rectify this issue.

Special thanks to @xmudrii for coming up with the fix. ⚡ 

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>` format, will close the issue(s) when PR gets merged)*:
Fixes #

**Special notes for your reviewer**:

**Optional Release Note**:
<!--
Write your release note:
1. Enter your extended release note in the below block. If the PR requires additional action from users switching to the new release, include the string "action required".
2. If no release note is required, just write "NONE".
-->
```release-note
Fix unmarshalling for `configvarstring` in cloudProviderSpec.
```


kubermatic-bot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubermatic/machine-controller/pull/1367#" title="Author self-approved">ahmedwaleedmalik</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubermatic%2Fmachine-controller).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/kubermatic/machine-controller/blob/master/OWNERS)~~ [ahmedwaleedmalik]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
ahmedwaleedmalik: /test pull-machine-controller-e2e-vmware-cloud-director 
kubermatic-bot: @ahmedwaleedmalik: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
pull-machine-controller-e2e-aws-spot-instance | 445cf6a060d06d3c313ded86aef8cf6d59a457f6 | [link](https://public-prow.loodse.com/view/gs/prow-dev-public-data/pr-logs/pull/kubermatic_machine-controller/1367/pull-machine-controller-e2e-aws-spot-instance/1549950998892515328) | true | `/test pull-machine-controller-e2e-aws-spot-instance`

[Full PR test history](https://public-prow.loodse.com/pr-history?org=kubermatic&repo=machine-controller&pr=1367)

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
ahmedwaleedmalik: /retest
kubermatic-bot: @ahmedwaleedmalik: The following tests **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
pull-machine-controller-e2e-aws-spot-instance | 445cf6a060d06d3c313ded86aef8cf6d59a457f6 | [link](https://public-prow.loodse.com/view/gs/prow-dev-public-data/pr-logs/pull/kubermatic_machine-controller/1367/pull-machine-controller-e2e-aws-spot-instance/1549950998892515328) | true | `/test pull-machine-controller-e2e-aws-spot-instance`
pull-machine-controller-e2e-aws | 445cf6a060d06d3c313ded86aef8cf6d59a457f6 | [link](https://public-prow.loodse.com/view/gs/prow-dev-public-data/pr-logs/pull/kubermatic_machine-controller/1367/pull-machine-controller-e2e-aws/1549950998716354560) | true | `/test pull-machine-controller-e2e-aws`
pull-machine-controller-e2e-vsphere | 445cf6a060d06d3c313ded86aef8cf6d59a457f6 | [link](https://public-prow.loodse.com/view/gs/prow-dev-public-data/pr-logs/pull/kubermatic_machine-controller/1367/pull-machine-controller-e2e-vsphere/1549951006047997952) | true | `/test pull-machine-controller-e2e-vsphere`

[Full PR test history](https://public-prow.loodse.com/pr-history?org=kubermatic&repo=machine-controller&pr=1367)

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
kubermatic-bot: LGTM label has been added.  <details>Git tree hash: 9d2de1089fa01f31c251af64a9e090e8344a23d9</details>
kubermatic-bot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubermatic/machine-controller/pull/1367#" title="Author self-approved">ahmedwaleedmalik</a>*, *<a href="https://github.com/kubermatic/machine-controller/pull/1367#pullrequestreview-1045951180" title="Approved">xmudrii</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubermatic%2Fmachine-controller).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/kubermatic/machine-controller/blob/master/OWNERS)~~ [ahmedwaleedmalik,xmudrii]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
kubermatic-bot: LGTM label has been added.  <details>Git tree hash: ebc17c1209fd7d4ebc929417948c0b5c06b3f5be</details>
hdurand0710: /lgtm
kubermatic-bot: @ahmedwaleedmalik: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
pull-machine-controller-e2e-vsphere | 9fe84d5f84c51137bb7621f850637569733bf223 | [link](https://public-prow.loodse.com/view/gs/prow-dev-public-data/pr-logs/pull/kubermatic_machine-controller/1367/pull-machine-controller-e2e-vsphere/1550004396635459584) | true | `/test pull-machine-controller-e2e-vsphere`

[Full PR test history](https://public-prow.loodse.com/pr-history?org=kubermatic&repo=machine-controller&pr=1367)

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
ahmedwaleedmalik: /unhold

Patch:
diff --git a/pkg/providerconfig/types/types.go b/pkg/providerconfig/types/types.go
index b9ddbbca..b4e12ad7 100644
--- a/pkg/providerconfig/types/types.go
+++ b/pkg/providerconfig/types/types.go
@@ -21,6 +21,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"strconv"
 
 	clusterv1alpha1 "github.com/kubermatic/machine-controller/pkg/apis/cluster/v1alpha1"
 	"github.com/kubermatic/machine-controller/pkg/cloudprovider/util"
@@ -226,7 +227,16 @@ func (configVarString *ConfigVarString) UnmarshalJSON(b []byte) error {
 	if !bytes.HasPrefix(b, []byte("{")) {
 		b = bytes.TrimPrefix(b, []byte(`"`))
 		b = bytes.TrimSuffix(b, []byte(`"`))
-		configVarString.Value = string(b)
+
+		// `Unquote` expects the input string to be inside quotation marks.
+		//  Since we can have a string without any quotations, in which case `TrimPrefix` and
+		// `TrimSuffix` will be noop. We explicitly add quotation marks to the input string
+		// to make sure that `Unquote` never fails.
+		s, err := strconv.Unquote("\"" + string(b) + "\"")
+		if err != nil {
+			return err
+		}
+		configVarString.Value = s
 		return nil
 	}
 	// This type must have the same fields as ConfigVarString but not


Test patch:
diff --git a/pkg/providerconfig/types/types_test.go b/pkg/providerconfig/types/types_test.go
index 7b0d8601..d1d7252b 100644
--- a/pkg/providerconfig/types/types_test.go
+++ b/pkg/providerconfig/types/types_test.go
@@ -21,7 +21,7 @@ import (
 	"reflect"
 	"testing"
 
-	"k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"
 	"k8s.io/utils/pointer"
 )
 
@@ -160,6 +160,7 @@ func TestConfigVarBoolMarshalling(t *testing.T) {
 func TestConfigVarStringMarshallingAndUnmarshalling(t *testing.T) {
 	testCases := []ConfigVarString{
 		{Value: "val"},
+		{Value: "spe<ialv&lue"},
 		{SecretKeyRef: GlobalSecretKeySelector{ObjectReference: v1.ObjectReference{Namespace: "ns", Name: "name"}, Key: "key"}},
 		{Value: "val", SecretKeyRef: GlobalSecretKeySelector{ObjectReference: v1.ObjectReference{Namespace: "ns", Name: "name"}, Key: "key"}},
 		{ConfigMapKeyRef: GlobalConfigMapKeySelector{ObjectReference: v1.ObjectReference{Namespace: "ns", Name: "name"}, Key: "key"}},
@@ -172,6 +173,11 @@ func TestConfigVarStringMarshallingAndUnmarshalling(t *testing.T) {
 			ConfigMapKeyRef: GlobalConfigMapKeySelector{ObjectReference: v1.ObjectReference{Namespace: "ns", Name: "name"}, Key: "key"},
 			SecretKeyRef:    GlobalSecretKeySelector{ObjectReference: v1.ObjectReference{Namespace: "ns", Name: "name"}, Key: "key"},
 		},
+		{
+			Value:           "spe<ialv&lue",
+			ConfigMapKeyRef: GlobalConfigMapKeySelector{ObjectReference: v1.ObjectReference{Namespace: "ns", Name: "name"}, Key: "key"},
+			SecretKeyRef:    GlobalSecretKeySelector{ObjectReference: v1.ObjectReference{Namespace: "ns", Name: "name"}, Key: "key"},
+		},
 	}
 
 	for _, cvs := range testCases {