support docker info proxy details #1176

BenTheElder · 2019-12-18T01:46:43Z

fixes #1175

I also wound up needing to fix some things along the way to test this properly:

subpar output from our test helpers (couldn't tell nil map vs empty map)
bugs versus our interface in cmd helpers
needed a simple exec fake (will write a nicer one later when I write some more test with it...)

k8s-ci-robot · 2019-12-18T01:46:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [BenTheElder]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

BenTheElder · 2019-12-18T01:50:10Z

pkg/cluster/internal/providers/provider/common/proxy.go

+	// we prefer the proxy env to allow more explicit per-invocation override
+	// of proxy settings
+	if len(envs) < 1 {
+		e, err := getProxyEnvFromDocker(cmder)


TODO: probably re-work this so some providers don't do this? need to think about this more, but that shouldn't be hard and it's academic at the moment.

can it be an interface getProxyEnvFromProvider?

pkg/cluster/internal/providers/provider/common/proxy.go

BenTheElder · 2019-12-18T02:47:47Z

/hold
sigh. why is this so complicated with docker.
I suppose we may have to identify the config location and parse that too but I'd rather avoid that :|

tao12345666333 · 2019-12-18T02:54:46Z

We don't need to cover everything, IMO.

For me personally, due to my network situation, I have to configure a proxy for the docker daemon, but usually this proxy runs on my local localhost:8081, and I don't want to proxy this to the cluster created by kind. Because that doesn't make it works well.

BenTheElder · 2019-12-18T02:57:09Z

Probably we should pick up the config.json options instead of the daemon options then..?

…

On Tue, Dec 17, 2019, 18:54 Jintao Zhang ***@***.***> wrote: We don't need to cover everything, IMO. For me personally, due to my network situation, I have to configure a proxy for the docker daemon, but usually this proxy runs on my local localhost:8081, and I don't want to proxy this to the cluster created by kind. Because that doesn't make it works well. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1176?email_source=notifications&email_token=AAHADKYVPG2OSHZMQZMGN3DQZGGHPA5CNFSM4J4EA5GKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHEVWXA#issuecomment-566844252>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHADK7FCADYQRKMLSJBN7DQZGGHPANCNFSM4J4EA5GA> .

BenTheElder · 2019-12-18T02:57:57Z

Those seem to be purpose built as a way to set the env on containers, but we want to append to it.

…

On Tue, Dec 17, 2019, 18:56 Benjamin Elder ***@***.***> wrote: Probably we should pick up the config.json options instead of the daemon options then..? On Tue, Dec 17, 2019, 18:54 Jintao Zhang ***@***.***> wrote: > We don't need to cover everything, IMO. > > For me personally, due to my network situation, I have to configure a > proxy for the docker daemon, but usually this proxy runs on my local > localhost:8081, and I don't want to proxy this to the cluster created by > kind. Because that doesn't make it works well. > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#1176?email_source=notifications&email_token=AAHADKYVPG2OSHZMQZMGN3DQZGGHPA5CNFSM4J4EA5GKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHEVWXA#issuecomment-566844252>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAHADK7FCADYQRKMLSJBN7DQZGGHPANCNFSM4J4EA5GA> > . >

tao12345666333 · 2019-12-18T03:26:42Z

Sorry for delay.

As I said just now, because of my network environment (basically the same for most users in China), I need to run a proxy locally.
I will set the docker daemon use it, this is for the convenience of pulling the image from a registry like gcr.io.

But usually when the container is running, no proxy is needed. Even if I want to use a proxy, it will be configured in ~ /.docker/config, or passed in through env.

Probably we should pick up the config.json options instead of the daemon options then..?

For me, yes.

aojea · 2019-12-18T05:50:48Z

pkg/cluster/internal/providers/provider/common/proxy.go

+			"docker", "info",
+			"--format",
+			// one per line, upper(key)=value
+			"HTTP_PROXY={{.HTTPProxy}}\nHTTPS_PROXY={{.HTTPSProxy}}\nNO_PROXY={{.NoProxy}}",


is this format stable?
This is what I have in my local machine

$ docker info --format "HTTP_PROXY={{.HTTPProxy}}\nHTTPS_PROXY={{.HTTPSProxy}}\nNO_PROXY={{.NoProxy}}" HTTP_PROXY=\nHTTPS_PROXY=\nNO_PROXY= $

aojea · 2019-12-18T05:56:35Z

pkg/cluster/internal/providers/provider/common/proxy.go

+	}
+	// parse out environment settings from the format we specified
+	envs := make(map[string]string)
+	for _, line := range lines {


we should check that the number of lines is 3 and the names of the variables []string{HTTPProxy, HTTPSProxy, NOProxy}
to be more robust.
If docker outputs whatever message instead of the variables we are going to return this wrong info

we control the format

error output goes to stderr, not stdout

aojea · 2019-12-18T06:00:31Z

pkg/exec/helpers.go

@@ -62,8 +62,8 @@ func RunErrorForError(err error) *RunError {
 // stderr + stdout, it scans these for lines and returns a slice of output lines
 func CombinedOutputLines(cmd Cmd) (lines []string, err error) {
 	var buff bytes.Buffer
-	cmd.SetStdout(&buff)
-	cmd.SetStderr(&buff)
+	cmd = cmd.SetStdout(&buff)


just curious, we need the assignment to make the code testable with the new fakeCmder?

At one point we did. This is more correct regardless.

aojea · 2019-12-18T06:03:44Z

aghh, I've just read now @tao12345666333 comments,

is better to document that you MUST use environment variables if you want to use a proxy instead of open this new can of worms ;) ?

tao12345666333 · 2019-12-18T06:47:15Z

is better to document that you MUST use environment variables if you want to use a proxy instead of open this new can of worms ;) ?

yes, I think so. providing available solutions (use environment variables) is simpler and more effective than covering all possibilities

aojea · 2019-12-18T08:47:12Z

/test pull-kind-conformance-parallel-ipv6

aojea · 2019-12-18T08:47:38Z

ahhg is parallel, the issue is only happening on serial 🤦‍♂️

k8s-ci-robot · 2019-12-18T09:17:54Z

@BenTheElder: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kind-conformance-parallel-ipv6	`77fcadc`	link	`/test pull-kind-conformance-parallel-ipv6`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

BenTheElder · 2020-01-06T23:37:07Z

see the issue, this needs a different solution.

improve assert output

b140d98

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 18, 2019

k8s-ci-robot requested review from amwat and aojea December 18, 2019 01:46

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 18, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 18, 2019

BenTheElder added 2 commits December 17, 2019 17:47

implement trivial exec fake

eeecdcd

correct some cmd helpers when methods return new values

9a176f5

BenTheElder force-pushed the port-maps branch from 29fcc0c to 1978aeb Compare December 18, 2019 01:47

BenTheElder commented Dec 18, 2019

View reviewed changes

BenTheElder mentioned this pull request Dec 18, 2019

proxy info from docker config is not respected #1175

Open

tao12345666333 reviewed Dec 18, 2019

View reviewed changes

pkg/cluster/internal/providers/provider/common/proxy.go Outdated Show resolved Hide resolved

support extracting proxy info from docker config

77fcadc

BenTheElder force-pushed the port-maps branch from 1978aeb to 77fcadc Compare December 18, 2019 02:44

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 18, 2019

aojea reviewed Dec 18, 2019

View reviewed changes

BenTheElder closed this Jan 6, 2020

BenTheElder deleted the port-maps branch January 6, 2020 23:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support docker info proxy details #1176

support docker info proxy details #1176

BenTheElder commented Dec 18, 2019

k8s-ci-robot commented Dec 18, 2019

BenTheElder Dec 18, 2019

aojea Dec 18, 2019

BenTheElder commented Dec 18, 2019

tao12345666333 commented Dec 18, 2019

BenTheElder commented Dec 18, 2019 via email

BenTheElder commented Dec 18, 2019 via email

tao12345666333 commented Dec 18, 2019

aojea Dec 18, 2019

aojea Dec 18, 2019

BenTheElder Dec 18, 2019 •

edited

Loading

aojea Dec 18, 2019

BenTheElder Dec 18, 2019

aojea commented Dec 18, 2019

tao12345666333 commented Dec 18, 2019

aojea commented Dec 18, 2019

aojea commented Dec 18, 2019

k8s-ci-robot commented Dec 18, 2019

BenTheElder commented Jan 6, 2020

support docker info proxy details #1176

support docker info proxy details #1176

Conversation

BenTheElder commented Dec 18, 2019

k8s-ci-robot commented Dec 18, 2019

BenTheElder Dec 18, 2019

Choose a reason for hiding this comment

aojea Dec 18, 2019

Choose a reason for hiding this comment

BenTheElder commented Dec 18, 2019

tao12345666333 commented Dec 18, 2019

BenTheElder commented Dec 18, 2019 via email

BenTheElder commented Dec 18, 2019 via email

tao12345666333 commented Dec 18, 2019

aojea Dec 18, 2019

Choose a reason for hiding this comment

aojea Dec 18, 2019

Choose a reason for hiding this comment

BenTheElder Dec 18, 2019 • edited Loading

Choose a reason for hiding this comment

aojea Dec 18, 2019

Choose a reason for hiding this comment

BenTheElder Dec 18, 2019

Choose a reason for hiding this comment

aojea commented Dec 18, 2019

tao12345666333 commented Dec 18, 2019

aojea commented Dec 18, 2019

aojea commented Dec 18, 2019

k8s-ci-robot commented Dec 18, 2019

BenTheElder commented Jan 6, 2020

BenTheElder Dec 18, 2019 •

edited

Loading