Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug 2048451: Fix proxy dial to pick all proxies #5743

Merged
merged 2 commits into from
Apr 21, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@ require (
github.com/vmware/govmomi v0.24.0
golang.org/x/crypto v0.0.0-20211202192323-5770296d904e
golang.org/x/lint v0.0.0-20210508222113-6edffad5e616
golang.org/x/net v0.0.0-20220127200216-cd36cc0744dd
golang.org/x/oauth2 v0.0.0-20210819190943-2bc19b11175f
golang.org/x/sys v0.0.0-20211216021012-1d35b9e2eb4e
google.golang.org/api v0.44.0
Expand Down Expand Up @@ -189,6 +188,7 @@ require (
github.com/prometheus/client_model v0.2.0 // indirect
github.com/prometheus/procfs v0.6.0 // indirect
github.com/satori/go.uuid v1.2.0 // indirect
golang.org/x/net v0.0.0-20220127200216-cd36cc0744dd // indirect
)

// OpenShift Forks
Expand Down
16 changes: 4 additions & 12 deletions pkg/asset/installconfig/aws/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"fmt"
"net"
"net/http"
"net/url"
"sort"
"strings"
Expand All @@ -14,7 +15,6 @@ import (
"github.com/aws/aws-sdk-go/service/ec2"
"github.com/aws/aws-sdk-go/service/route53"
"github.com/pkg/errors"
"golang.org/x/net/proxy"
utilerrors "k8s.io/apimachinery/pkg/util/errors"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/apimachinery/pkg/util/validation/field"
Expand Down Expand Up @@ -321,20 +321,12 @@ func validateEndpointAccessibility(endpointURL string) error {
if endpointURL == "e2e.local" {
return nil
}
URL, err := url.Parse(endpointURL)
_, err := url.Parse(endpointURL)
if err != nil {
return err
}
port := URL.Port()
if port == "" {
port = "https"
}
conn, err := proxy.Dial(context.Background(), "tcp", net.JoinHostPort(URL.Hostname(), port))
if err != nil {
return err
}
conn.Close()
return nil
_, err = http.Head(endpointURL)
return err
}

var requiredServices = []string{
Expand Down
57 changes: 57 additions & 0 deletions pkg/asset/installconfig/aws/validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package aws

import (
"context"
"os"
"testing"

"github.com/aws/aws-sdk-go/service/route53"
Expand Down Expand Up @@ -122,6 +123,16 @@ func validServiceEndpoints() []aws.ServiceEndpoint {
}}
}

func invalidServiceEndpoint() []aws.ServiceEndpoint {
return []aws.ServiceEndpoint{{
Name: "testing",
URL: "testing",
}, {
Name: "test",
URL: "http://testing.non",
}}
}

func validInstanceTypes() map[string]InstanceType {
return map[string]InstanceType{
"t2.small": {
Expand All @@ -146,6 +157,7 @@ func TestValidate(t *testing.T) {
privateSubnets map[string]Subnet
publicSubnets map[string]Subnet
instanceTypes map[string]InstanceType
proxy string
expectErr string
}{{
name: "valid no byo",
Expand Down Expand Up @@ -570,6 +582,46 @@ func TestValidate(t *testing.T) {
privateSubnets: validPrivateSubnets(),
publicSubnets: validPublicSubnets(),
expectErr: `^platform\.aws\.amiID: Required value: AMI must be provided$`,
}, {
name: "invalid endpoint URL",
installConfig: func() *types.InstallConfig {
c := validInstallConfig()
c.Platform.AWS.Region = "us-east-1"
c.Platform.AWS.ServiceEndpoints = invalidServiceEndpoint()
c.Platform.AWS.AMIID = "custom-ami"
return c
}(),
availZones: validAvailZones(),
privateSubnets: validPrivateSubnets(),
publicSubnets: validPublicSubnets(),
expectErr: `^\Q[platform.aws.serviceEndpoints[0].url: Invalid value: "testing": Head "testing": unsupported protocol scheme "", platform.aws.serviceEndpoints[1].url: Invalid value: "http://testing.non": Head "http://testing.non": dial tcp: lookup testing.non\E.*: no such host\]$`,
}, {
name: "invalid proxy URL but valid URL",
installConfig: func() *types.InstallConfig {
c := validInstallConfig()
c.Platform.AWS.Region = "us-east-1"
c.Platform.AWS.AMIID = "custom-ami"
c.Platform.AWS.ServiceEndpoints = []aws.ServiceEndpoint{{Name: "test", URL: "http://testing.com"}}
return c
}(),
availZones: validAvailZones(),
privateSubnets: validPrivateSubnets(),
publicSubnets: validPublicSubnets(),
proxy: "proxy",
}, {
name: "invalid proxy URL and invalid URL",
installConfig: func() *types.InstallConfig {
c := validInstallConfig()
c.Platform.AWS.Region = "us-east-1"
c.Platform.AWS.AMIID = "custom-ami"
c.Platform.AWS.ServiceEndpoints = []aws.ServiceEndpoint{{Name: "test", URL: "http://test"}}
return c
}(),
availZones: validAvailZones(),
privateSubnets: validPrivateSubnets(),
publicSubnets: validPublicSubnets(),
proxy: "http://proxy.com",
expectErr: `^\Qplatform.aws.serviceEndpoints[0].url: Invalid value: "http://test": Head "http://test": dial tcp: lookup test\E.*: no such host$`,
}}

for _, test := range tests {
Expand All @@ -580,6 +632,11 @@ func TestValidate(t *testing.T) {
publicSubnets: test.publicSubnets,
instanceTypes: test.instanceTypes,
}
if test.proxy != "" {
os.Setenv("HTTP_PROXY", test.proxy)
} else {
os.Unsetenv("HTTP_PROXY")
}
Comment on lines +635 to +639
Copy link
Contributor

Choose a reason for hiding this comment

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

As it stands, I don't think test.proxy is worth including in these tests. We're not testing whether the proxy is being used or not. So I don't think including the proxy in the test is doing much.

If we were going to write a test for the proxy, I would imagine it would use something like [httpTest](https://pkg.go.dev/net/http/httptest#example-Server) and create a proxy server and make sure the proxy is being called when trying to connect to the endpoint.

While that could be cool, it seems like a fair amount of effort to test code that is mostly in the stdlib--not code we're writing. So I think it might be best to just remove these parts from the test.

err := Validate(context.TODO(), meta, test.installConfig)
if test.expectErr == "" {
assert.NoError(t, err)
Expand Down
168 changes: 0 additions & 168 deletions vendor/golang.org/x/net/internal/socks/client.go

This file was deleted.