MGMT-24487: Allow https scheme in HTTPS proxy URL by yoavsc0302 · Pull Request #10589 · openshift/assisted-service

yoavsc0302 · 2026-07-05T12:57:09Z

Allow https:// scheme in the https_proxy proxy URL field. Previously, the https_proxy field only accepted http:// URLs. This change enables users to configure HTTPS-based proxy servers for their clusters.

Commit 1: Update swagger https_proxy field description to document both http:// and https:// schemes
Commit 2: skipper make generate — regenerate models and embedded spec
Commit 3: Add ValidateHTTPSProxyFormat for httpsProxy validation

List all the issues related to this PR

What environments does this code impact?

Automation (CI, tools, etc)
Cloud
Operator Managed Deployments
None

How was this code tested?

assisted-test-infra environment
dev-scripts environment
Reviewer's test appreciated
Waiting for CI to do a full test run
Manual (Elaborate on how it was tested)
No tests needed

Checklist

Title and description added to both, commit and PR.
Relevant issues have been associated (see CONTRIBUTING guide)
This change does not require a documentation update (docstring, docs, README, etc)
Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

Are the title and description (in both PR and commit) meaningful and clear?
Is there a bug required (and linked) for this change?
Should this PR be backported?

Summary by CodeRabbit

Bug Fixes
- HTTPS proxy values are now validated using HTTPS-aware rules, allowing both http:// and https:// proxy URL formats where applicable.
Documentation
- Updated API schema and embedded API documentation to clearly state that https_proxy can be provided as either http://... or https://....
Tests
- Expanded test coverage for HTTPS proxy validation and added scenarios verifying HTTPS proxy handling across cluster update flows.

Update the https_proxy field description in all 4 swagger definitions (cluster-create-params, v2-cluster-update-params, cluster, proxy) to document that both http:// and https:// schemes are valid, aligning with OCP docs since 4.9.

…on update Auto-generated by skipper make generate.

openshift-ci-robot · 2026-07-05T12:57:12Z

coderabbitai · 2026-07-05T12:57:32Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 392868b3-2c96-4dc8-b4aa-b97b72336324

📥 Commits

Reviewing files that changed from the base of the PR and between 6212cfd and 0cea09d.

📒 Files selected for processing (4)

internal/bminventory/inventory.go
internal/bminventory/inventory_test.go
pkg/validations/validations.go
pkg/validations/validations_test.go

🚧 Files skipped from review as they are similar to previous changes (3)

internal/bminventory/inventory.go
pkg/validations/validations_test.go
pkg/validations/validations.go

Walkthrough

This PR adds HTTPS proxy URL validation, updates inventory validation to use it, adds coverage for valid HTTPS proxy settings, and documents HTTP/HTTPS formats in model comments and API specifications.

Changes

HTTPS proxy validation

Layer / File(s)	Summary
Shared proxy validation contract `pkg/validations/validations.go`, `pkg/validations/validations_test.go`	Adds shared URL validation, preserves HTTP-only validation, introduces HTTPS validation for HTTP and HTTPS schemes, and tests valid and invalid inputs.
Inventory HTTPS proxy integration `internal/bminventory/inventory.go`, `internal/bminventory/inventory_test.go`	Uses the HTTPS validator for HTTPS proxy settings and tests valid HTTPS proxies through cluster and infrastructure environment updates.
Proxy format documentation `models/*.go`, `swagger.yaml`, `restapi/embedded_spec.go`	Documents HTTP and HTTPS proxy URL formats across model fields and API schema descriptions.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 15

✅ Passed checks (15 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly states the main change: allowing HTTPS scheme support for the HTTPS proxy URL.
Description check	✅ Passed	The description includes a clear summary, linked issue, impact, and testing details, with the template mostly filled out.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names	✅ Passed	Added Ginkgo titles are static descriptive strings; no interpolated or run-dependent values appear in the changed tests.
Test Structure And Quality	✅ Passed	Added It blocks are single-purpose and follow existing BeforeEach/DB test patterns; no waits/timeouts or cleanup gaps were introduced.
Microshift Test Compatibility	✅ Passed	New Ginkgo cases only validate proxy-string handling in unit tests; they don't use MicroShift-unsupported OpenShift APIs, namespaces, or need skips.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	Added Ginkgo tests only validate proxy URL parsing/storage and don’t assume node count or HA topology; no SNO guard is needed.
Topology-Aware Scheduling Compatibility	✅ Passed	Only proxy URL validation/tests changed; no manifests, controllers, replicas, affinities, nodeSelectors, or topology logic were touched.
Ote Binary Stdout Contract	✅ Passed	Changed files only touch validation/docs/tests; no main/init/TestMain/BeforeSuite stdout writes or fmt/log calls to stdout were added.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	New Ginkgo tests only exercise proxy URL strings (proxy.proxy, 10.9.8.7, IPv6 literals) and do not make network calls or IPv4-only assumptions.
No-Weak-Crypto	✅ Passed	Patch only updates proxy validation/docs; added lines contain no MD5/SHA1/DES/RC4/3DES/Blowfish/ECB or secret-comparison logic.
Container-Privileges	✅ Passed	PR diff is proxy validation/docs only; no changed manifests or any privileged settings (privileged, hostPID/Network/IPC, SYS_ADMIN, allowPrivilegeEscalation).
No-Sensitive-Data-In-Logs	✅ Passed	The PR updates proxy validation and docs only; it adds no new logs exposing proxy values or other sensitive data.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ast-grep (0.44.1)

internal/bminventory/inventory_test.go

ast-grep timed out on this file

_{Comment @coderabbitai help to get the list of available commands.}

openshift-ci · 2026-07-05T12:57:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: yoavsc0302

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [yoavsc0302]

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

yoavsc0302 · 2026-07-05T21:29:33Z

/retest

danmanor · 2026-07-06T09:15:31Z

/retest

yoavsc0302 · 2026-07-06T11:54:33Z

/retest

codecov · 2026-07-06T13:53:36Z

Codecov Report

❌ Patch coverage is 52.63158% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 44.48%. Comparing base (f0f5309) to head (0cea09d).
⚠️ Report is 18 commits behind head on master.

Files with missing lines	Patch %	Lines
restapi/embedded_spec.go	0.00%	8 Missing ⚠️
internal/bminventory/inventory.go	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #10589   +/-   ##
=======================================
  Coverage   44.48%   44.48%           
=======================================
  Files         423      423           
  Lines       73327    73331    +4     
=======================================
+ Hits        32617    32622    +5     
+ Misses      37801    37800    -1     
  Partials     2909     2909

Files with missing lines	Coverage Δ
pkg/validations/validations.go	`95.91% <100.00%> (+0.17%)`	⬆️
internal/bminventory/inventory.go	`71.82% <0.00%> (+0.06%)`	⬆️
restapi/embedded_spec.go	`0.00% <0.00%> (ø)`

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

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

yoavsc0302 · 2026-07-09T07:18:52Z

/retest

eranco74

Overall this looks like a solid fix.

eranco74 · 2026-07-12T08:48:20Z

 	}
 	if httpsProxy != nil && *httpsProxy != "" {
-		if err := pkgvalidations.ValidateHTTPProxyFormat(*httpsProxy); err != nil {
+		if err := pkgvalidations.ValidateHTTPSProxyFormat(*httpsProxy); err != nil {


Now that this calls ValidateHTTPSProxyFormat, it would be good to add an integration-level test that sends an https:// URL as httpsProxy through one of the API handlers (e.g., V2UpdateCluster or RegisterInfraEnv) and asserts success.

Currently all the tests that exercise validateProxySettings pass http:// or empty string for httpsProxy. The existing https://proxy.proxy fixtures at lines 3722 and 5412 in inventory_test.go write directly to the DB, bypassing validation entirely. Without a test going through the API path, a future revert of this line would go undetected.

eranco74 · 2026-07-12T08:48:31Z

@@ -113,6 +113,21 @@ func ValidateHTTPProxyFormat(proxyURL string) error {
 	return nil
 }


nit: The comment on ValidateHTTPProxyFormat above (line 98) still says "validates the HTTP Proxy and HTTPS Proxy format" — now that HTTPS proxy validation has its own function, this comment is stale. Should be updated to just "validates the HTTP Proxy URL format" or similar.

eranco74 · 2026-07-12T08:48:41Z

+	if u.Scheme != "http" && u.Scheme != "https" {
+		return errors.Errorf("The URL scheme must be http or https and specified in the URL: '%s'", proxyURL)
+	}
+	return nil


nit: This is almost identical to ValidateHTTPProxyFormat — both share the govalidator.IsURL check, url.Parse, and error formatting. The only difference is the scheme check. Could consider extracting a small helper like:

func validateProxyURLFormat(proxyURL string, allowedSchemes ...string) error {

and having both functions call it. Not a blocker, just something to keep in mind if more proxy variants appear.

I agree. I addressed your comments and pushed the changes. Thanks!

Add ValidateHTTPSProxyFormat that accepts both http:// and https:// schemes for the https_proxy field. Extract shared validation logic into validateProxyURLFormat helper with variadic allowedSchemes parameter. Add integration tests for https:// proxy through V2UpdateCluster and UpdateInfraEnv API handlers.

yoavsc0302 · 2026-07-12T18:51:57Z

/retest

gamli75 · 2026-07-16T09:21:51Z

/override ci/prow/edge-e2e-ai-operator-disconnected-capi

gamli75 · 2026-07-16T09:21:57Z

/retest

openshift-ci · 2026-07-16T09:22:34Z

@gamli75: Overrode contexts on behalf of gamli75: ci/prow/edge-e2e-ai-operator-disconnected-capi

Details

In response to this:

/override ci/prow/edge-e2e-ai-operator-disconnected-capi

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.

openshift-ci · 2026-07-16T10:52:41Z

@yoavsc0302: 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/edge-subsystem-aws	`0cea09d`	link	true	`/test edge-subsystem-aws`
ci/prow/edge-subsystem-kubeapi-aws	`0cea09d`	link	true	`/test edge-subsystem-kubeapi-aws`
ci/prow/edge-e2e-ai-operator-ztp-capi	`0cea09d`	link	true	`/test edge-e2e-ai-operator-ztp-capi`
ci/prow/edge-e2e-ai-operator-ztp	`0cea09d`	link	true	`/test edge-e2e-ai-operator-ztp`

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.

yoavsc0302 added 2 commits July 5, 2026 15:49

MGMT-24487: skipper make generate after swagger https_proxy descripti…

8316f31

…on update Auto-generated by skipper make generate.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jul 5, 2026

openshift-ci Bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 5, 2026

openshift-ci Bot requested review from eranco74 and gamli75 July 5, 2026 12:57

openshift-ci Bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. api-review Categorizes an issue or PR as actively needing an API review. labels Jul 5, 2026

pastequo reviewed Jul 9, 2026

View reviewed changes

Comment thread pkg/validations/validations.go Outdated

pastequo reviewed Jul 9, 2026

View reviewed changes

Comment thread pkg/validations/validations.go

eranco74 reviewed Jul 12, 2026

View reviewed changes

yoavsc0302 force-pushed the MGMT-24487/allow-https-proxy-scheme branch from 6212cfd to 0cea09d Compare July 12, 2026 10:43

LiorSoffer mentioned this pull request Jul 16, 2026

MGMT-24493: Allow HTTPS proxy for Discovery ISO openshift-assisted/assisted-installer-ui#3777

Open

		@@ -113,6 +113,21 @@ func ValidateHTTPProxyFormat(proxyURL string) error {
		return nil
		}

Uh oh!

Conversation

yoavsc0302 commented Jul 5, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

Summary by CodeRabbit

Uh oh!

openshift-ci-robot commented Jul 5, 2026 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

Uh oh!

coderabbitai Bot commented Jul 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

openshift-ci Bot commented Jul 5, 2026

Uh oh!

yoavsc0302 commented Jul 5, 2026

Uh oh!

danmanor commented Jul 6, 2026

Uh oh!

yoavsc0302 commented Jul 6, 2026

Uh oh!

codecov Bot commented Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

yoavsc0302 commented Jul 9, 2026

Uh oh!

Uh oh!

Uh oh!

eranco74 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eranco74 Jul 12, 2026

Choose a reason for hiding this comment

Uh oh!

eranco74 Jul 12, 2026

Choose a reason for hiding this comment

Uh oh!

eranco74 Jul 12, 2026

Choose a reason for hiding this comment

Uh oh!

yoavsc0302 Jul 12, 2026

Choose a reason for hiding this comment

Uh oh!

yoavsc0302 commented Jul 12, 2026

Uh oh!

gamli75 commented Jul 16, 2026

Uh oh!

gamli75 commented Jul 16, 2026

Uh oh!

openshift-ci Bot commented Jul 16, 2026

Uh oh!

openshift-ci Bot commented Jul 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

yoavsc0302 commented Jul 5, 2026 •

edited by coderabbitai Bot

Loading

openshift-ci-robot commented Jul 5, 2026 •

edited by openshift-ci Bot

Loading

coderabbitai Bot commented Jul 5, 2026 •

edited

Loading

codecov Bot commented Jul 6, 2026 •

edited

Loading

eranco74 left a comment •

edited

Loading