OCM-18900 | fix: Improve proxy validation error messages for special characters in passwords #3052

olucasfreitas · 2025-10-21T13:18:42Z

Details

This PR fixes a usability issue where rosa create cluster --http-proxy was displaying unhelpful error messages when proxy passwords contained special characters like forward slashes, making it difficult for users to understand what was wrong with their input.

Fixed Behavior

Run the command with a proxy password containing a forward slash:

./rosa create cluster --http-proxy "http://proxyuser:QvoZjyy/trkCiY5@10.0.0.161:8080"

Now shows clear error message:

E: Invalid http-proxy value 'http://proxyuser:QvoZjyy/trkCiY5@10.0.0.161:8080': 
   password contains special characters that need URL encoding

Additional Validations Added

Spaces in URL - Detects and rejects URLs with spaces:

./rosa create cluster --http-proxy "http://proxy.example.com :8080"

Output:

E: Invalid http-proxy value 'http://proxy.example.com :8080': URL cannot contain spaces

Invalid Port Range - Validates port numbers are between 1 and 65535:

./rosa create cluster --http-proxy "http://proxy.example.com:99999"

Output:

E: Invalid http-proxy value 'http://proxy.example.com:99999': port must be between 1 and 65535

Multiple @ Symbols - Detects unencoded @ in passwords:

./rosa create cluster --http-proxy "http://user:pass@word@proxy.example.com:8080"

Output:

E: Invalid http-proxy value 'http://user:pass@word@proxy.example.com:8080': 
   password contains special characters that need URL encoding

The --https-proxy Flag Behavior

Using HTTPS scheme with --http-proxy continues to show the existing error:

./rosa create cluster --http-proxy "https://proxy.example.com:8080"

Output:

E: Expected http-proxy to have an http:// scheme

Ticket

Closes OCM-18900

pkg/ocm/helpers_test.go

codecov · 2025-10-21T13:54:05Z

Codecov Report

❌ Patch coverage is 44.92754% with 38 lines in your changes missing coverage. Please review.
✅ Project coverage is 23.94%. Comparing base (c854a37) to head (770854f).
⚠️ Report is 10 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/helper/url/validation.go	0.00%	31 Missing ⚠️
pkg/ocm/helpers.go	88.57%	4 Missing ⚠️
cmd/create/cluster/cmd.go	0.00%	2 Missing ⚠️
cmd/edit/cluster/cmd.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3052      +/-   ##
==========================================
+ Coverage   23.91%   23.94%   +0.03%     
==========================================
  Files         317      318       +1     
  Lines       35202    35238      +36     
==========================================
+ Hits         8418     8439      +21     
- Misses      26143    26159      +16     
+ Partials      641      640       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

cmd/list/dnsdomains/cmd.go

olucasfreitas · 2025-11-03T11:37:00Z

/retest-required

cmd/list/dnsdomains/cmd.go

tests/utils/exec/rosacli/upgrade_service.go

pkg/ocm/helpers.go

olucasfreitas · 2025-11-03T15:15:52Z

/retest-required

olucasfreitas · 2025-11-03T15:22:59Z

/retest-required

hunterkepley

The test case in the bug ticket does not seem to pass:

E: parse "http://proxyuser:QvoZjyy/trkCiY5@10.0.0.161:8080": invalid port ":QvoZjyy" after host

I think there may be a bug in the parse code

olucasfreitas · 2025-11-03T15:58:28Z

@hunterkepley I just did some tests here, and it seems to be working after the fix, did you test it with the branch locally ?

olucasfreitas · 2025-11-04T11:08:34Z

/retest-required

olucasfreitas · 2025-11-04T11:16:05Z

/retest-required

tests/e2e/test_rosacli_cluster.go

pkg/ocm/helpers_test.go

pkg/ocm/helpers.go

hunterkepley · 2025-11-13T20:51:13Z

/test all

hunterkepley · 2025-11-13T20:52:08Z

/test e2e-presubmits-pr-rosa-sts-advanced

hunterkepley · 2025-11-13T20:53:09Z

/test e2e-presubmits-pr-rosa-hcp-advanced

hunterkepley · 2025-11-13T20:53:59Z

Some reason the e2e tests didn't run, I manually ran them so we can verify this MR didn't break any

pkg/helper/helpers.go

pkg/interactive/validation.go

pkg/ocm/helpers.go

pkg/ocm/helpers_test.go

pkg/ocm/registry_config_test.go

olucasfreitas · 2025-11-18T20:38:19Z

/retest

olucasfreitas · 2025-11-18T20:47:09Z

/retest

olucasfreitas · 2025-11-19T11:42:09Z

/retest

pkg/interactive/validation.go

pkg/ocm/helpers.go

pkg/interactive/validation.go

pkg/ocm/helpers.go

hunterkepley · 2025-11-19T14:35:39Z

/test e2e-presubmits-pr-rosa-hcp-advanced

hunterkepley · 2025-11-19T14:35:45Z

/test e2e-presubmits-pr-rosa-sts-advanced

openshift-ci · 2025-11-19T20:05:45Z

@olucasfreitas: 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
ci/prow/coverage	`2637639`	link	true	`/test coverage`

Full PR test history. Your PR dashboard.

Details

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-sigs/prow repository. I understand the commands that are listed here.

olucasfreitas · 2025-11-19T20:22:04Z

/retest

pkg/helper/helpers.go

pkg/ocm/helpers.go

hunterkepley · 2025-12-08T16:28:27Z

pkg/ocm/helpers.go

+	}
+
+	// Validate host format
+	_, _, err = net.SplitHostPort(parsedURL.Host)


Could you let me know why this is here?

hunterkepley · 2025-12-08T16:29:01Z

pkg/ocm/helpers.go

+	httpProxy := val.(string)
+	if httpProxy == "" {
 		return nil
 	}


won't this panic if val is not a string?

or a type which can be directly casted into one?

I think there was some confusion on what I suggested in my comment here: #3052 (comment) because the guarding for it panicking was removed after

hunterkepley · 2025-12-08T16:29:21Z

pkg/ocm/helpers.go

+}
+
+func ValidateHTTPSProxy(val interface{}) error {
+	httpsProxy := val.(string)


won't this panic?

hunterkepley · 2025-12-08T16:39:03Z

pkg/helper/url/validation.go

+	schemeIdx := strings.Index(urlString, "://")
+	if schemeIdx == -1 {
+		return &URLCredentialValidation{}
+	}


Im not sure I understand, we return an empty validation error when there is no ://?

#3052 (comment)

It looks like that previously, this returned an error but didn't have enough information in it. Now it has no information, could you let me know what this block of code should be doing? I might be just confused on it

Also when looking at the block of code above this one, I am even more confused on what this should be accomplishing

hunterkepley · 2025-12-08T16:42:07Z

pkg/helper/url/validation.go

+	atIdx := strings.LastIndex(rest, "@")
+	if atIdx == -1 {
+		return &URLCredentialValidation{}
+	}


When an @ is not found, we return an empty validation error

What does this result in when the final error is printed out?

…al characters

openshift-ci bot requested review from ciaranRoche and hunterkepley October 21, 2025 13:19

gdbranco reviewed Oct 21, 2025

View reviewed changes

pkg/ocm/helpers_test.go Show resolved Hide resolved

olucasfreitas force-pushed the OCM-18900 branch from fc09c54 to aa528ca Compare October 21, 2025 15:11

olucasfreitas requested a review from gdbranco October 21, 2025 17:03

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 23, 2025

gdbranco reviewed Oct 28, 2025

View reviewed changes

cmd/list/dnsdomains/cmd.go Outdated Show resolved Hide resolved

olucasfreitas force-pushed the OCM-18900 branch 2 times, most recently from 00638f4 to 120a803 Compare November 3, 2025 11:36

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 3, 2025

olucasfreitas requested a review from gdbranco November 3, 2025 11:36

hunterkepley reviewed Nov 3, 2025

View reviewed changes

cmd/list/dnsdomains/cmd.go Outdated Show resolved Hide resolved

hunterkepley reviewed Nov 3, 2025

View reviewed changes

tests/utils/exec/rosacli/upgrade_service.go Show resolved Hide resolved

hunterkepley reviewed Nov 3, 2025

View reviewed changes

pkg/ocm/helpers.go Outdated Show resolved Hide resolved

olucasfreitas force-pushed the OCM-18900 branch 2 times, most recently from 2260456 to c6b94b4 Compare November 3, 2025 14:46

olucasfreitas requested a review from hunterkepley November 3, 2025 15:16

hunterkepley reviewed Nov 3, 2025

View reviewed changes

olucasfreitas force-pushed the OCM-18900 branch 2 times, most recently from 96029c9 to a50904c Compare November 3, 2025 19:49

hunterkepley reviewed Nov 4, 2025

View reviewed changes

olucasfreitas force-pushed the OCM-18900 branch 2 times, most recently from 9502aa4 to 2637639 Compare November 5, 2025 15:38

olucasfreitas force-pushed the OCM-18900 branch 4 times, most recently from 560e392 to 3df4265 Compare November 13, 2025 19:27

olucasfreitas requested a review from hunterkepley November 17, 2025 20:49

hunterkepley reviewed Nov 17, 2025

View reviewed changes

olucasfreitas force-pushed the OCM-18900 branch from 3df4265 to 055b821 Compare November 18, 2025 20:24

hunterkepley reviewed Nov 19, 2025

View reviewed changes

olucasfreitas force-pushed the OCM-18900 branch 2 times, most recently from 34c6edf to 875a547 Compare November 19, 2025 19:47

olucasfreitas force-pushed the OCM-18900 branch from 875a547 to 4fe55d7 Compare November 19, 2025 20:37

olucasfreitas requested a review from hunterkepley November 19, 2025 21:11

olucasfreitas force-pushed the OCM-18900 branch from 4fe55d7 to a51e1b8 Compare December 1, 2025 17:17

hunterkepley reviewed Dec 2, 2025

View reviewed changes

olucasfreitas force-pushed the OCM-18900 branch 2 times, most recently from f7318dc to 3d09b7b Compare December 8, 2025 15:43

hunterkepley reviewed Dec 8, 2025

View reviewed changes

OCM-18900 | fix: Improve error message for proxy passwords with speci…

770854f

…al characters

olucasfreitas force-pushed the OCM-18900 branch from 3d09b7b to 770854f Compare December 9, 2025 20:03

OCM-18900 | fix: Improve proxy validation error messages for special characters in passwords #3052

Are you sure you want to change the base?

OCM-18900 | fix: Improve proxy validation error messages for special characters in passwords #3052

Uh oh!

Conversation

olucasfreitas commented Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Behavior

Additional Validations Added

The --https-proxy Flag Behavior

Ticket

Uh oh!

Uh oh!

codecov bot commented Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

olucasfreitas commented Nov 3, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

olucasfreitas commented Nov 3, 2025

Uh oh!

olucasfreitas commented Nov 3, 2025

Uh oh!

hunterkepley left a comment

Choose a reason for hiding this comment

Uh oh!

olucasfreitas commented Nov 3, 2025

Uh oh!

olucasfreitas commented Nov 4, 2025

Uh oh!

olucasfreitas commented Nov 4, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hunterkepley commented Nov 13, 2025

Uh oh!

hunterkepley commented Nov 13, 2025

Uh oh!

hunterkepley commented Nov 13, 2025

Uh oh!

hunterkepley commented Nov 13, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

olucasfreitas commented Nov 18, 2025

Uh oh!

olucasfreitas commented Nov 18, 2025

Uh oh!

olucasfreitas commented Nov 19, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hunterkepley commented Nov 19, 2025

Uh oh!

hunterkepley commented Nov 19, 2025

Uh oh!

openshift-ci bot commented Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

olucasfreitas commented Oct 21, 2025 •

edited

Loading

codecov bot commented Oct 21, 2025 •

edited

Loading

openshift-ci bot commented Nov 19, 2025 •

edited

Loading

hunterkepley Dec 8, 2025 •

edited

Loading