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

Jump to bottom

Adding fix for and tests for no proxy support for deployer #729

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Adding fix for and tests for no proxy support for deployer #729

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
31 changes: 29 additions & 2 deletions artifactory/utils/buildinfoproperties.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@ package utils

import (
"errors"
"github.com/jfrog/jfrog-client-go/utils/log"
"io/ioutil"
"net"
"net/http"
"net/url"
"os"
"path/filepath"
Expand All @@ -20,6 +22,7 @@ import (

const (
HttpProxy = "HTTP_PROXY"
NoProxy = "NO_PROXY"
)

type BuildConfigMapping map[ProjectType][]*map[string]string
Expand Down Expand Up @@ -235,6 +238,9 @@ func CreateBuildInfoPropertiesFile(buildName, buildNumber string, config *viper.
func setProxyIfDefined(config *viper.Viper) error {
// Add HTTP_PROXY if exists
proxy := os.Getenv(HttpProxy)
noProxy := os.Getenv(NoProxy)
log.Debug("Proxy:" + proxy)
log.Debug("No Proxy:" + noProxy)
if proxy != "" {
url, err := url.Parse(proxy)
if err != nil {
Expand All @@ -244,8 +250,29 @@ func setProxyIfDefined(config *viper.Viper) error {
if err != nil {
return errorutils.CheckError(err)
}
config.Set(PROXY+HOST, host)
config.Set(PROXY+PORT, port)
if noProxy != "" {
//TODO: This is for the Deployer as proxy properties are set only once (clientConfig). Proxy for dependency resolution can be set via the gradle client
deployRequest, err := http.NewRequest("HEAD", config.GetString(DEPLOYER_PREFIX+URL), nil)
if err != nil {
return errorutils.CheckError(err)
}
deployProxyURL, err := http.ProxyFromEnvironment(deployRequest)
log.Debug("Deploy URL:" + config.GetString(DEPLOYER_PREFIX+URL))
if err != nil {
return errorutils.CheckError(err)
}
if deployProxyURL != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a chance I'm missing something here, but it looks like deployProxyURL is used only for the debug logging. My guess is that your intention was to extract the host and the post values from deployProxyURL before adding them to the properties file.
If this is the case, I suggest simplifying the implementation of this function and doing it once, instead of twice. In other words, we can use the http.ProxyFromEnvironment API regardless of whether noProxy is defined or not.

Notice that the tests are also wrong, because when they set no_proxy, the no_proxy value is identical to the http_proxy value, and that's why no host and port are added to the properties file.
The tests should use no_proxy values with wildcards, and check that the no proxy filtering works properly.

Copy link
Author

@Enquier Enquier Jun 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I agree that this could be simplified.

DeployURLProxy is used as a logic operation to determine if the proxy should be set or not depending on the environment’s No_Proxy rules. The key element here is the introduction of the http.ProxyFromEnvironment API which has a robust “NoProxy” logic in that if it returns nil, nil then you should not set the proxy flag and if it returns nil, <proxy_url> then you should set the proxy flag to that URL.

As you suggest I could effectively pull most of this code out (I tried to keep the flow of the original code) and replace it with http.ProxyFromEnvironment. I think it also was this way due to the fact that I originally thought that it mattered if we set it for Deploy vs Resolve but after digging in I realized that Gradle handles that part itself.

Copy link
Author

Choose a reason for hiding this comment

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

As for the tests there are two sets of tests. One set where the No Proxy is identical and one where they differ.

Copy link
Author

@Enquier Enquier Jun 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

We could add tests that test the full gambit of no_proxy rules, I figured that was best left to the http.ProxyFromEnvironment team. Instead, I simply wanted a test that proved if the proxy was set or not depending on the variables we passed

log.Debug("Resolved Deploy Proxy:" + deployProxyURL.String())
config.Set(PROXY+HOST, host)
config.Set(PROXY+PORT, port)
} else {
log.Debug("Deploy URL is within no_proxy")
}
} else {
config.Set(PROXY+HOST, host)
config.Set(PROXY+PORT, port)
}

}
return nil
}
Expand Down
62 changes: 57 additions & 5 deletions artifactory/utils/buildinfoproperties_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,19 +9,24 @@ import (
)

const (
host = "localhost"
port = "8888"
proxy = "http://" + host + ":" + port
host = "localhost"
port = "8888"
proxy = "http://" + host + ":" + port
noproxy = "some.other.url.com"
othernoproxy = "some.different.url.com"
)

func TestCreateDefaultPropertiesFile(t *testing.T) {
proxyOrg := getOriginalProxyValue()
noproxyOrg := getOriginalNoProxyValue()
setProxy("", t)
setNoProxy("", t)

for index := range ProjectTypes {
testCreateDefaultPropertiesFile(ProjectType(index), t)
}
setProxy(proxyOrg, t)
setNoProxy(noproxyOrg, t)
}

func testCreateDefaultPropertiesFile(projectType ProjectType, t *testing.T) {
Expand Down Expand Up @@ -51,9 +56,11 @@ func testCreateDefaultPropertiesFile(projectType ProjectType, t *testing.T) {
compareViperConfigs(t, actualConfig, expectedConfig, projectType)
}

func TestCreateSimplePropertiesFileWithProxy(t *testing.T) {
func TestCreateSimplePropertiesFileWithProxyAndWithoutNoProxy(t *testing.T) {
proxyOrg := getOriginalProxyValue()
noproxyOrg := getOriginalNoProxyValue()
setProxy(proxy, t)
setNoProxy("", t)
var propertiesFileConfig = map[string]string{
"artifactory.resolve.contextUrl": "http://some.url.com",
"artifactory.publish.contextUrl": "http://some.other.url.com",
Expand All @@ -63,21 +70,55 @@ func TestCreateSimplePropertiesFileWithProxy(t *testing.T) {
}
createSimplePropertiesFile(t, propertiesFileConfig)
setProxy(proxyOrg, t)
setNoProxy(noproxyOrg, t)
}

func TestCreateSimplePropertiesFileWithoutProxy(t *testing.T) {
func TestCreateSimplePropertiesFileWithoutProxyAndNoProxy(t *testing.T) {
proxyOrg := getOriginalProxyValue()
noproxyOrg := getOriginalNoProxyValue()
setProxy("", t)
setNoProxy("", t)
var propertiesFileConfig = map[string]string{
"artifactory.resolve.contextUrl": "http://some.url.com",
"artifactory.publish.contextUrl": "http://some.other.url.com",
"artifactory.deploy.build.name": "buildName",
}
createSimplePropertiesFile(t, propertiesFileConfig)
setProxy(proxyOrg, t)
setNoProxy(noproxyOrg, t)

}

func TestCreateSimplePropertiesFileWithProxyAndNoProxy(t *testing.T) {
proxyOrg := getOriginalProxyValue()
noproxyOrg := getOriginalNoProxyValue()
setProxy(proxy, t)
setNoProxy(noproxy, t)
var propertiesFileConfig = map[string]string{
"artifactory.resolve.contextUrl": "http://some.url.com",
"artifactory.publish.contextUrl": "http://some.other.url.com",
"artifactory.deploy.build.name": "buildName",
}
createSimplePropertiesFile(t, propertiesFileConfig)
setProxy(proxyOrg, t)
setNoProxy(noproxyOrg, t)
}
Copy link
Author

Choose a reason for hiding this comment

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

Here is a test where the proxy matches the noproxy


func TestCreateSimplePropertiesFileWithProxyAndDifferentNoProxy(t *testing.T) {
proxyOrg := getOriginalProxyValue()
noproxyOrg := getOriginalNoProxyValue()
setProxy(proxy, t)
setNoProxy(othernoproxy, t)
var propertiesFileConfig = map[string]string{
"artifactory.resolve.contextUrl": "http://some.url.com",
"artifactory.publish.contextUrl": "http://some.other.url.com",
"artifactory.deploy.build.name": "buildName",
}
createSimplePropertiesFile(t, propertiesFileConfig)
setProxy(proxyOrg, t)
setNoProxy(noproxyOrg, t)
}

Copy link
Author

Choose a reason for hiding this comment

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

This test has the proxy and the no proxy not matching and should return proxy/port

func createSimplePropertiesFile(t *testing.T, propertiesFileConfig map[string]string) {
var yamlConfig = map[string]string{
RESOLVER_PREFIX + URL: "http://some.url.com",
Expand Down Expand Up @@ -191,9 +232,20 @@ func getOriginalProxyValue() string {
return os.Getenv(HttpProxy)
}

func getOriginalNoProxyValue() string {
return os.Getenv(NoProxy)
}

func setProxy(proxy string, t *testing.T) {
err := os.Setenv(HttpProxy, proxy)
if err != nil {
t.Error(err)
}
}

func setNoProxy(noproxy string, t *testing.T) {
err := os.Setenv(NoProxy, noproxy)
if err != nil {
t.Error(err)
}
}