Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion internal/bminventory/inventory.go
Original file line number Diff line number Diff line change
Expand Up @@ -4890,7 +4890,7 @@ func validateProxySettings(httpProxy, httpsProxy, noProxy, ocpVersion *string) e
}
}
if httpsProxy != nil && *httpsProxy != "" {
if err := pkgvalidations.ValidateHTTPProxyFormat(*httpsProxy); err != nil {
if err := pkgvalidations.ValidateHTTPSProxyFormat(*httpsProxy); err != nil {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return errors.Errorf("Failed to validate HTTPS Proxy: %s", err)
}
}
Expand Down
11 changes: 11 additions & 0 deletions internal/bminventory/inventory_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3755,6 +3755,13 @@ var _ = Describe("cluster", func() {
_ = updateCluster("http://proxy.proxy", "", "proxy.proxy")
})

It("set a valid https proxy", func() {
mockEvents.EXPECT().SendClusterEvent(gomock.Any(), eventstest.NewEventMatcher(
eventstest.WithNameMatcher(eventgen.ProxySettingsChangedEventName),
eventstest.WithClusterIdMatcher(clusterID.String())))
_ = updateCluster("", "https://proxy.proxy", "")
})

It("set a valid noProxy wildcard", func() {
mockEvents.EXPECT().SendClusterEvent(gomock.Any(), eventstest.NewEventMatcher(
eventstest.WithNameMatcher(eventgen.ProxySettingsChangedEventName),
Expand Down Expand Up @@ -10418,6 +10425,10 @@ var _ = Describe("infraEnvs", func() {
_ = updateInfraEnv("http://proxy.proxy", "", "proxy.proxy")
})

It("set a valid https proxy", func() {
_ = updateInfraEnv("", "https://proxy.proxy", "")
})

It("set a valid noProxy wildcard", func() {
_ = updateInfraEnv("", "", "*")
})
Expand Down
2 changes: 1 addition & 1 deletion models/cluster.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion models/cluster_create_params.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion models/proxy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion models/v2_cluster_update_params.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

25 changes: 17 additions & 8 deletions pkg/validations/validations.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,22 +95,31 @@ func ValidateHTTPFormat(theurl string) error {
return nil
}

// ValidateHTTPProxyFormat validates the HTTP Proxy and HTTPS Proxy format
func ValidateHTTPProxyFormat(proxyURL string) error {
func validateProxyURLFormat(proxyURL string, allowedSchemes ...string) error {
if !govalidator.IsURL(proxyURL) {
return errors.Errorf("Proxy URL format is not valid: '%s'", proxyURL)
}
u, err := url.Parse(proxyURL)
if err != nil {
return errors.Errorf("Proxy URL format is not valid: '%s'", proxyURL)
}
if u.Scheme == "https" {
return errors.Errorf("The URL scheme must be http; https is currently not supported: '%s'", proxyURL)
}
if u.Scheme != "http" {
return errors.Errorf("The URL scheme must be http and specified in the URL: '%s'", proxyURL)
for _, scheme := range allowedSchemes {
if u.Scheme == scheme {
return nil
}
}
return nil
return errors.Errorf("The URL scheme must be %s and specified in the URL: '%s'",
strings.Join(allowedSchemes, " or "), proxyURL)
}

// ValidateHTTPProxyFormat validates the HTTP Proxy format (http scheme only)
func ValidateHTTPProxyFormat(proxyURL string) error {
return validateProxyURLFormat(proxyURL, "http")
}

// ValidateHTTPSProxyFormat validates the HTTPS Proxy URL format (http or https schemes are valid)
func ValidateHTTPSProxyFormat(proxyURL string) error {
Comment thread
yoavsc0302 marked this conversation as resolved.
return validateProxyURLFormat(proxyURL, "http", "https")
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


func validateNoProxyEntry(entry string) error {
Expand Down
46 changes: 45 additions & 1 deletion pkg/validations/validations_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ var _ = Describe("URL validations", func() {
},
{
"https://proxy.com:3128",
"The URL scheme must be http; https is currently not supported: 'https://proxy.com:3128'",
"The URL scheme must be http and specified in the URL: 'https://proxy.com:3128'",
},
{
"ftp://proxy.com:3128",
Expand Down Expand Up @@ -82,6 +82,50 @@ var _ = Describe("URL validations", func() {
})
})

Context("test HTTPS proxy URL", func() {
var parameters = []struct {
input, err string
}{
{"http://proxy.com:3128", ""},
{"http://username:pswd@proxy.com", ""},
{"http://10.9.8.7:123", ""},
{"http://username:pswd@10.9.8.7:123", ""},
{"https://proxy.com:3128", ""},
{"https://username:pswd@proxy.com", ""},
{"https://10.9.8.7:123", ""},
{"https://username:pswd@10.9.8.7:443", ""},
{"https://[1080:0:0:0:8:800:200C:417A]:8888", ""},
{
"ftp://proxy.com:3128",
"The URL scheme must be http or https and specified in the URL: 'ftp://proxy.com:3128'",
},
{
"proxy.com:3128",
"The URL scheme must be http or https and specified in the URL: 'proxy.com:3128'",
},
{
"xyz",
"Proxy URL format is not valid: 'xyz'",
},
{
"",
"Proxy URL format is not valid: ''",
},
}

It("validates HTTPS proxy URL input", func() {
for _, param := range parameters {
err := ValidateHTTPSProxyFormat(param.input)
if param.err == "" {
Expect(err).Should(BeNil())
} else {
Expect(err).ShouldNot(BeNil())
Expect(err.Error()).To(Equal(param.err))
}
}
})
})

Context("test URL", func() {
var parameters = []struct {
input, err string
Expand Down
16 changes: 8 additions & 8 deletions restapi/embedded_spec.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 4 additions & 4 deletions swagger.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -5252,7 +5252,7 @@ definitions:
type: string
description: |
A proxy URL to use for creating HTTPS connections outside the cluster.
http://\<username\>:\<pswd\>@\<ip\>:\<port\>
http://\<username\>:\<pswd\>@\<ip\>:\<port\> or https://\<username\>:\<pswd\>@\<ip\>:\<port\>
x-nullable: true
no_proxy:
type: string
Expand Down Expand Up @@ -5488,7 +5488,7 @@ definitions:
type: string
description: |
A proxy URL to use for creating HTTPS connections outside the cluster.
http://\<username\>:\<pswd\>@\<ip\>:\<port\>
http://\<username\>:\<pswd\>@\<ip\>:\<port\> or https://\<username\>:\<pswd\>@\<ip\>:\<port\>
x-nullable: true
no_proxy:
type: string
Expand Down Expand Up @@ -5718,7 +5718,7 @@ definitions:
type: string
description: |
A proxy URL to use for creating HTTPS connections outside the cluster.
http://\<username\>:\<pswd\>@\<ip\>:\<port\>
http://\<username\>:\<pswd\>@\<ip\>:\<port\> or https://\<username\>:\<pswd\>@\<ip\>:\<port\>
x-go-custom-tag: gorm:"column:https_proxy"
no_proxy:
type: string
Expand Down Expand Up @@ -7979,7 +7979,7 @@ definitions:
type: string
description: |
A proxy URL to use for creating HTTPS connections outside the cluster.
http://\<username\>:\<pswd\>@\<ip\>:\<port\>
http://\<username\>:\<pswd\>@\<ip\>:\<port\> or https://\<username\>:\<pswd\>@\<ip\>:\<port\>
x-nullable: true
x-go-custom-tag: gorm:"column:https_proxy"
no_proxy:
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.