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.

Sign up for GitHub

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

loose http_proxy and https_proxy check #8149

Merged
merged 1 commit into from
Aug 7, 2018
Merged

loose http_proxy and https_proxy check #8149

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
26 changes: 16 additions & 10 deletions cmd/vic-machine/common/proxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,13 @@ func (p *Proxies) ProxyFlags() []cli.Flag {
cli.GenericFlag{
Name: "https-proxy",
Value: flags.NewOptionalString(&p.HTTPSProxy),
Usage: "An HTTPS proxy for use when fetching images, in the form https://fqdn_or_ip:port",
Usage: "An HTTPS proxy for use when fetching images, in the form http(s)://fqdn_or_ip:port",
Hidden: true,
},
cli.GenericFlag{
Name: "http-proxy",
Value: flags.NewOptionalString(&p.HTTPProxy),
Usage: "An HTTP proxy for use when fetching images, in the form http://fqdn_or_ip:port",
Usage: "An HTTP proxy for use when fetching images, in the form http(s)://fqdn_or_ip:port",
Hidden: true,
},
}
Expand All @@ -52,19 +52,25 @@ func (p *Proxies) ProcessProxies() (hproxy, sproxy *url.URL, err error) {
p.IsSet = true
}
if p.HTTPProxy != nil && *p.HTTPProxy != "" {
hproxy, err = url.Parse(*p.HTTPProxy)
if err != nil || hproxy.Host == "" || hproxy.Scheme != "http" {
err = cli.NewExitError(fmt.Sprintf("Could not parse HTTP proxy - expected format http://fqnd_or_ip:port: %s", *p.HTTPProxy), 1)
hproxy, err = p.validate(*p.HTTPProxy)
if err != nil {
return
}
}

if p.HTTPSProxy != nil && *p.HTTPSProxy != "" {
sproxy, err = url.Parse(*p.HTTPSProxy)
if err != nil || sproxy.Host == "" || sproxy.Scheme != "https" {
err = cli.NewExitError(fmt.Sprintf("Could not parse HTTPS proxy - expected format https://fqnd_or_ip:port: %s", *p.HTTPSProxy), 1)
return
}
sproxy, err = p.validate(*p.HTTPSProxy)
}
return
}

func (p *Proxies) validate(ref string) (proxy *url.URL, err error) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we have a unit test that ensures the validation is successful for the various cases?
We do not have a test for omitting the scheme currently and I recall there was a url.Parse problem if scheme was omitted - the host portion could end up as the scheme.

I suggest testing at a minimum:

https://fully.qualified.domain.name
https://fully.qualified.domain.name:443
http://fully.qualified.domain.name
http://fully.qualified.domain.name:80
fully.qualified.domain.name
fully.qualified.domain.name:80
raw-host
raw-host:80

Copy link
Contributor Author

@wjun wjun Aug 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Good point. I will add one. Please note our current format is http(s)://hostname, so we will get OK for the first 4 urls above, and failure for the last 4 urls.

proxy, err = url.Parse(ref)
if err != nil {
return
}
if proxy.Host == "" || (proxy.Scheme != "http" && proxy.Scheme != "https") {
err = cli.NewExitError(fmt.Sprintf("Could not parse HTTP(S) proxy - expected format http(s)://fqnd_or_ip:port: %s", ref), 1)
}
return
}
78 changes: 78 additions & 0 deletions cmd/vic-machine/common/proxy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
// Copyright 2018 VMware, Inc. All Rights Reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package common

import (
"testing"

"github.com/stretchr/testify/assert"
)

func TestProcessProxies(t *testing.T) {
gurls := [...]string{
"https://fully.qualified.example.com",
"https://fully.qualified.example.com:443",
"http://fully.qualified.example.com",
"http://fully.qualified.example.com:80",
"http://203.0.113.123",
"http://[2001:DB8:0123::]",
}

burls := [...]string{
"example.com",
"example.com:80",
"localhost",
"localhost:80",
"ftp://example.com",
"httpd://example.com",
}

for _, ghttp := range gurls {
for _, ghttps := range gurls {
gproxy := Proxies{HTTPProxy: &ghttp, HTTPSProxy: &ghttps}

_, _, err := gproxy.ProcessProxies()
assert.NoError(t, err, "Expected %s and %s to be accepted", ghttp, ghttps)
assert.True(t, gproxy.IsSet, "Expected proxy to be marked as set")
}
}

for _, ghttp := range gurls {
for _, bhttps := range burls {
bproxy := Proxies{HTTPProxy: &ghttp, HTTPSProxy: &bhttps}

_, _, err := bproxy.ProcessProxies()
assert.Error(t, err, "Expected %s to be rejected", bhttps)
}
}

for _, bhttp := range burls {
for _, ghttps := range gurls {
bproxy := Proxies{HTTPProxy: &bhttp, HTTPSProxy: &ghttps}

_, _, err := bproxy.ProcessProxies()
assert.Error(t, err, "Expected %s to be rejected", bhttp)
}
}

for _, bhttp := range burls {
for _, bhttps := range burls {
bproxy := Proxies{HTTPProxy: &bhttp, HTTPSProxy: &bhttps}

_, _, err := bproxy.ProcessProxies()
assert.Error(t, err, "Expected %s and %s to be rejected", bhttp, bhttps)
}
}
}
18 changes: 0 additions & 18 deletions tests/test-cases/Group23-VIC-Machine-Service/23-03-VCH-Create.robot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -418,24 +418,6 @@ Fail to create a VCH specifying an ID
# Delete Path Under Target vch/${id}


Fail to create VCH where http != https (on http key/pair) in image_fetch_proxy - registry settings
Copy link
Member

Choose a reason for hiding this comment

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

It seems like we should update these to be positive tests that you can configure "mismatched" proxies instead of removing them entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I add them into another test case where we test http(s) proxy settings.

Create VCH '{"name":"%{VCH-NAME}-invalid_registry","compute":{"resource":{"name":"%{TEST_RESOURCE}"}},"storage":{"image_stores":["ds://%{TEST_DATASTORE}"]},"network":{"bridge":{"ip_range":"172.16.0.0/12","port_group":{"name":"%{BRIDGE_NETWORK}"}},"public":{"port_group":{"name":"${PUBLIC_NETWORK}"}}},"registry":{"image_fetch_proxy":{"http":"https://example.com","https":"https://example.com"},"insecure":["https://insecure.example.com"],"whitelist":["10.0.0.0/8"]},"auth":{"server":{"generate":{"cname":"vch.example.com","organization":["VMware, Inc."],"size":{"value":2048,"units":"bits"}}},"client":{"no_tls_verify": true}}}'

Verify Return Code
Verify Status Bad Request

Output Should Contain error processing proxies: Could not parse HTTP proxy


Fail to create VCH where https != http (on https key/pair) in image_fetch_proxy - registry settings
Create VCH '{"name":"%{VCH-NAME}-invalid_registry","compute":{"resource":{"name":"%{TEST_RESOURCE}"}},"storage":{"image_stores":["ds://%{TEST_DATASTORE}"]},"network":{"bridge":{"ip_range":"172.16.0.0/12","port_group":{"name":"%{BRIDGE_NETWORK}"}},"public":{"port_group":{"name":"${PUBLIC_NETWORK}"}}},"registry":{"image_fetch_proxy":{"http":"http://example.com","https":"http://example.com"},"insecure":["https://insecure.example.com"],"whitelist":["10.0.0.0/8"]},"auth":{"server":{"generate":{"cname":"vch.example.com","organization":["VMware, Inc."],"size":{"value":2048,"units":"bits"}}},"client":{"no_tls_verify": true}}}'

Verify Return Code
Verify Status Bad Request

Output Should Contain error processing proxies: Could not parse HTTPS proxy


Fail to create VCH where whitelist contains an int and not string - registry settings
Create VCH '{"name":"%{VCH-NAME}-invalid_registry","compute":{"resource":{"name":"%{TEST_RESOURCE}"}},"storage":{"image_stores":["ds://%{TEST_DATASTORE}"]},"network":{"bridge":{"ip_range":"172.16.0.0/12","port_group":{"name":"%{BRIDGE_NETWORK}"}},"public":{"port_group":{"name":"${PUBLIC_NETWORK}"}}},"registry":{"image_fetch_proxy":{"http":"http://example.com","https":"https://example.com"},"insecure":["https://insecure.example.com"],"whitelist":[100008]},"auth":{"server":{"generate":{"cname":"vch.example.com","organization":["VMware, Inc."],"size":{"value":2048,"units":"bits"}}},"client":{"no_tls_verify": true}}}'

Expand Down
24 changes: 24 additions & 0 deletions tests/test-cases/Group6-VIC-Machine/6-16-Config.robot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,18 @@ Configure VCH https-proxy
Should Contain ${output} --http-proxy=http://proxy.vmware.com:3128
Should Not Contain ${output} --https-proxy

${output}= Run bin/vic-machine-linux configure --name=%{VCH-NAME} --target=%{TEST_URL}%{TEST_DATACENTER} --thumbprint=%{TEST_THUMBPRINT} --user=%{TEST_USERNAME} --password=%{TEST_PASSWORD} --timeout %{TEST_TIMEOUT} --http-proxy https://proxy.vmware.com:3128
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to have the proxy pulled from the environment as this test will fail if not on the vmware internal network.
I know the test is already flawed in that regard but lets fix it up while we're making improvements.

I'm interested in moving to a situation where the test logic is independent of the infrastructure it runs on. While the bulk of that is around abstracting the topology of vSphere environments it should also include the direct manipulation of vSphere elements in tests and the infrastructure specific URIs so as this proxy server.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I understand, these cases only configure proxy settings but not use them, so any correct proxy urls will be OK.

Copy link
Member

Choose a reason for hiding this comment

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

You're correct. Which suggests we should have an additional test that makes sure that we actually use the configured proxy value. That's going to be a little more involved as it would require deployment of a proxy (could be a container in a different VCH if we want to deploy as part of the test) and checking proxy logs to ensure that traffic was routed through it.

Please open a follow up issue for the functional test (as opposed to this configuration test) if that's too involved for this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to #8144, proxy settings are used as environment variables, so that is the very common usage of http proxies.

Copy link
Member

Choose a reason for hiding this comment

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

Your're correct. If/when we add configuration for per destination proxy configurations it will become much more involved, but for now the port layer does not use the proxy configuration so we can safely assume normal Linux proxy environment behaviour.

Should Contain ${output} Completed successfully
${rc} ${output}= Run And Return Rc And Output govc vm.info -e %{VCH-NAME} | grep HTTP_PROXY
Should Be Equal As Integers ${rc} 0
Should Contain ${output} https://proxy.vmware.com:3128
${rc} ${output}= Run And Return Rc And Output govc vm.info -e %{VCH-NAME} | grep HTTPS_PROXY
Should Be Equal As Integers ${rc} 1
Should Not Contain ${output} proxy.vmware.com:3128
${output}= Run bin/vic-machine-linux inspect config --name=%{VCH-NAME} --target="%{TEST_USERNAME}:%{TEST_PASSWORD}@%{TEST_URL}" --thumbprint=%{TEST_THUMBPRINT}
Should Contain ${output} --http-proxy=https://proxy.vmware.com:3128
Should Not Contain ${output} --https-proxy

${output}= Run bin/vic-machine-linux configure --name=%{VCH-NAME} --target=%{TEST_URL}%{TEST_DATACENTER} --thumbprint=%{TEST_THUMBPRINT} --user=%{TEST_USERNAME} --password=%{TEST_PASSWORD} --timeout %{TEST_TIMEOUT} --https-proxy https://proxy.vmware.com:3128
Should Contain ${output} Completed successfully
${rc} ${output}= Run And Return Rc And Output govc vm.info -e %{VCH-NAME} | grep HTTPS_PROXY
Expand All @@ -165,6 +177,18 @@ Configure VCH https-proxy
Should Contain ${output} --https-proxy=https://proxy.vmware.com:3128
Should Not Contain ${output} --http-proxy

${output}= Run bin/vic-machine-linux configure --name=%{VCH-NAME} --target=%{TEST_URL}%{TEST_DATACENTER} --thumbprint=%{TEST_THUMBPRINT} --user=%{TEST_USERNAME} --password=%{TEST_PASSWORD} --timeout %{TEST_TIMEOUT} --https-proxy http://proxy.vmware.com:3128
Should Contain ${output} Completed successfully
${rc} ${output}= Run And Return Rc And Output govc vm.info -e %{VCH-NAME} | grep HTTPS_PROXY
Should Be Equal As Integers ${rc} 0
Should Contain ${output} http://proxy.vmware.com:3128
${rc} ${output}= Run And Return Rc And Output govc vm.info -e %{VCH-NAME} | grep HTTP_PROXY
Should Be Equal As Integers ${rc} 1
Should Not Contain ${output} proxy.vmware.com:3128
${output}= Run bin/vic-machine-linux inspect config --name=%{VCH-NAME} --target="%{TEST_USERNAME}:%{TEST_PASSWORD}@%{TEST_URL}" --thumbprint=%{TEST_THUMBPRINT}
Should Contain ${output} --https-proxy=http://proxy.vmware.com:3128
Should Not Contain ${output} --http-proxy

Configure VCH ops user credentials and thumbprint
${output}= Run bin/vic-machine-linux configure --name=%{VCH-NAME} --target=%{TEST_URL} --thumbprint=%{TEST_THUMBPRINT} --user=%{TEST_USERNAME} --password=%{TEST_PASSWORD} --timeout %{TEST_TIMEOUT} --ops-user=%{TEST_USERNAME} --ops-password=%{TEST_PASSWORD}
Should Contain ${output} Completed successfully
Expand Down