proxy: Update trimTrailingEOL function to handle carriage return (\r)

[praveenkumar]

It will fix the proxy parse issue in case user update the cert on windows and add blank line at the end of certificate. Unit test is also added for same.

fixes: #3785

[cfergeau]

Oh good catch!
I don't think this necessarily is caused by users changing the cert file, it could also be tools which would add /r/n to the end of certificate files on Windows.
I'd give more details about what happens exactly. When the cert file ends with \r\n, trimTrailingEOL would only remove \n and the file will end with \r. Then this causes issues in

crc/pkg/crc/cluster/cluster.go

Lines 388 to 391 in 97705a0

	`
	// Replace the carriage return ("\n" or "\r\n") with literal `\n` string
	re := regexp.MustCompile(`\r?\n`)
	p := fmt.Sprintf(proxyCABundleTemplate, re.ReplaceAllString(proxy.ProxyCACert, `\n`), trustedCAName)

which tries to replace \r\n and \n with \\n but will not do anything with a single \r.
This in turn causes bug #3785

[praveenkumar]

When the cert file ends with \r\n, trimTrailingEOL would only remove \n and the file will end with \r. Then this causes issues in

@cfergeau you mean to say

crc/pkg/crc/cluster/cluster.go

Lines 388 to 391 in 97705a0

	`
	// Replace the carriage return ("\n" or "\r\n") with literal `\n` string
	re := regexp.MustCompile(`\r?\n`)
	p := fmt.Sprintf(proxyCABundleTemplate, re.ReplaceAllString(proxy.ProxyCACert, `\n`), trustedCAName)

should handle \r case also and replace it \n because that is again going to fail. what we want there should be no new line after -----END CERTIFICATE----- otherwise user-ca-bundle.json is not applied correctly on the cluster so I think this should be handled by trimTrailingEOL itself?

[cfergeau]

should handle \r case also and replace it \n because that is again going to fail.

I was only trying to describe the situation before your changes, imo it would be useful to have in the commit log.
I agree with the way you fixed it, fix trimEOL to fully remove EOL (ie both \r\n and \n). With trimEOL fixed, the regexp will also behave as expected. Without the fix, the string does not match the regexp expectations, and this causes the reported bug.

[cfergeau]

Not sure what happened, but the code was initially correct in #1443, and when moving it to a helper function, \r was lost :-/
After re-reading #1443 (comment) I'm still unsure if removing the EOL at the end of the file is required, or if it's done because it's nicer?

[cfergeau]

There's a second occurrence of the same code:

pkg/crc/cluster/cluster.go:     sshPublicKey := strings.TrimRight(string(sshPublicKeyByte), "\r\n")

and the test code has

test/extended/util/checks.go:   actual = strings.TrimRight(actual, "\n")

Let's make use of the recently added pkg/strings package and add a shared helper. It's trivial, but we got it wrong once, better to implement it once and for all.

[praveenkumar]

Let's make use of the recently added pkg/strings package and add a shared helper. It's trivial, but we got it wrong once, better to implement it once and for all.

Added another commit to do that.

[openshift-ci]

@praveenkumar: 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
ci/prow/e2e-crc	e0619ed	link	true	/test e2e-crc
ci/prow/integration-crc	799787a	link	true	/test integration-crc

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[cfergeau]

/lgtm
/approve

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cfergeau

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [cfergeau]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment