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

Fix legacy proxy setting handling #8949

Merged
merged 1 commit into from Jul 23, 2018
Merged

Fix legacy proxy setting handling #8949

merged 1 commit into from Jul 23, 2018

Conversation

dshcherb
Copy link

@dshcherb dshcherb commented Jul 18, 2018

Please provide the following details to expedite Pull Request review:


Description of change

Auto-population of no-proxy with controller ip addresses caused
proxyupdater to use legacy proxy settings instead of new juju-* proxy
settings.

This change modifies this logic to first check if new juju proxy
settings are present and auto-populate them with controller IPs,
otherwise legacy proxy settings are auto-populated.

Logging is also improved to be more explicit about the type of settings
applied.

QA steps


# 1) replace IPs and ranges for your environment
cat bootstrap-config.yaml 
juju-http-proxy: http://10.10.101.2:8000
juju-https-proxy: http://10.10.101.2:8000
juju-no-proxy: 'localhost,127.0.0.1,10.10.10.0/24,10.10.101.0/24'
logging-config: '<root>=ERROR;unit=TRACE;juju.worker.proxyupdater=TRACE'

# 2) bootstrap a controller with new proxy settings set
juju bootstrap --config bootstrap-config.yaml --bootstrap-constraints mem=2048 vmaas

# 3) validate machine agent log
juju ssh -m controller 0 sudo -i
# the apiserver machine agent log should show
grep proxyupdater /var/log/juju/machine-0.log
2018-07-17 23:43:50 DEBUG juju.worker.proxyupdater proxyupdater.go:165 applying in-process juju proxy settings proxy.Settings{Http:"http://10.10.101.2:8000", Https:"http://10.10.101.2:8000", Ftp:"", NoProxy:"10.10.10.0/24,10.10.101.0/24,10.10.101.46,127.0.0.1,localhost", AutoNoProxy:""}
2018-07-17 23:43:50 DEBUG juju.worker.proxyupdater proxyupdater.go:188 saving new legacy proxy settings proxy.Settings{Http:"", Https:"", Ftp:"", NoProxy:"127.0.0.1,::1,localhost", AutoNoProxy:""}
2018-07-17 23:43:50 TRACE juju.worker.proxyupdater proxyupdater.go:216 setting snap proxy values: proxy.Settings{Http:"", Https:"", Ftp:"", NoProxy:"", AutoNoProxy:""}, "", ""
2018-07-17 23:43:50 DEBUG juju.worker.proxyupdater proxyupdater.go:233 snap core settings [proxy.http= proxy.https= proxy.store=] updated, output: ""

# 4) verify that the proxy server log does not contain any entries from no-proxy hostnames, IPs or subnet ranges

Documentation changes

No changes to CLI or API - this is fix to make this functionality work as documented.

Bug reference

https://bugs.launchpad.net/juju/+bug/1782236

@jujubot
Copy link
Collaborator

jujubot commented Jul 18, 2018

Can one of the admins verify this patch?

1 similar comment
@jujubot
Copy link
Collaborator

jujubot commented Jul 18, 2018

Can one of the admins verify this patch?

@manadart
Copy link
Member

!!build!!

@dshcherb
Copy link
Author

There is something wrong with DNS resolution of the build environment.

# cd .; git clone https://github.com/alecthomas/gometalinter /workspace/src/github.com/alecthomas/gometalinter
Cloning into '/workspace/src/github.com/alecthomas/gometalinter'...
fatal: unable to access 'https://github.com/alecthomas/gometalinter/': Could not resolve host: github.com
package github.com/alecthomas/gometalinter: exit status 128
Error: gometalinter is not installed.
Makefile:64: recipe for target 'pre-check' failed
make: *** [pre-check] Error 1

@dshcherb
Copy link
Author

!!build!!

@dshcherb
Copy link
Author

@manadart

I don't have any access to the CI environment to debug the above and not sure my build requests will be authorized to see if this is an intermittent issue.

My local snap builds were successful though and I was able to bootstrap a controller and test this functionality.

@SimonRichardson
Copy link
Member

!!build!!

@dshcherb
Copy link
Author

Pull-request updated, HEAD is now 1347544

@SimonRichardson
Copy link
Member

!!build!!

Copy link
Member

@manadart manadart left a comment

Choose a reason for hiding this comment

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

Can you add a test for this?

Something like:

func (s *ProxyUpdaterSuite) TestProxyConfigJujuProxy(c *gc.C) {
	s.state.SetModelConfig(coretesting.Attrs{
		"juju-http-proxy":  "http proxy",
		"juju-https-proxy": "https proxy",
		"apt-http-proxy":   "apt http proxy",
		"apt-https-proxy":  "apt https proxy",
	})

	cfg := s.facade.ProxyConfig(s.oneEntity())

	s.state.Stub.CheckCallNames(c,
		"ModelConfig",
		"APIHostPortsForAgents",
	)

	noProxy := "0.1.2.3,0.1.2.4,0.1.2.5"

	r := params.ProxyConfigResult{
		JujuProxySettings: params.ProxyConfig{
			HTTP: "http proxy", HTTPS: "https proxy", FTP: "", NoProxy: noProxy},
		APTProxySettings: params.ProxyConfig{
			HTTP: "http://apt http proxy", HTTPS: "https://apt https proxy", FTP: "", NoProxy: ""},
	}
	c.Assert(cfg.Results[0], jc.DeepEquals, r)
}

Fails before, passes after.

@dshcherb
Copy link
Author

dshcherb commented Jul 19, 2018

@manadart
Sure, will do.

@dshcherb
Copy link
Author

Pull-request updated, HEAD is now 737de74

Auto-population of no-proxy with controller ip addresses caused
proxyupdater to use legacy proxy settings instead of new juju-* proxy
settings.

This change modifies this logic to first check if new juju proxy
settings are present and auto-populate them with controller IPs,
otherwise legacy proxy settings are auto-populated.

Logging is also improved to be more explicit about the type of settings
applied.
@dshcherb
Copy link
Author

Pull-request updated, HEAD is now 4f8fb48

@dshcherb
Copy link
Author

@manadart

Modified one test and added another one.

Am I right to assume that I can use your CI for test runs or is there any documented process to run tests in a private lab (at least on a high-level)? Is running !!build!! only available for core Juju devs?

@SimonRichardson
Copy link
Member

!!build!!

@manadart
Copy link
Member

$$merge$$

@jujubot jujubot merged commit 0336051 into juju:develop Jul 23, 2018
jujubot added a commit that referenced this pull request Jul 25, 2018
#8967

## Please provide the following details to expedite Pull Request review:

----

## Description of change

A cherry-pick of a commit to fix https://bugs.launchpad.net/juju/+bug/1782236

## QA steps

#8949

## Documentation changes

No changes

## Bug reference

https://bugs.launchpad.net/juju/+bug/1782236
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants