Automatically add the docker network to no_proxy #628
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/M
|
/lgtm Thanks. |
k8s-ci-robot
added
the
lgtm
| @@ -33,6 +33,14 @@ import ( | |||
| "sigs.k8s.io/kind/pkg/exec" | |||
| ) | |||
|
|
|||
| const ( | |||
| // Docker default bridge network is named "bridge" (https://docs.docker.com/network/bridge/#use-the-default-bridge-network) | |||
| defaultNetwork = "bridge" | |||
There was a problem hiding this comment.
should it be a constant in the docker pkg?
There was a problem hiding this comment.
probably yes, but this is also mostly a stub to make #484 viable if we go that route so I'm ok to deal with it later.
the docker <-> node coupling needs a lot more work soon, I'm sketching that out in a branch ... 😅
pkg/cluster/nodes/node.go
Outdated
|
|
||
| // Specifically add the docker network subnets to NO_PROXY | ||
| subnets, err := getSubnets(defaultNetwork) | ||
| if err == nil { |
There was a problem hiding this comment.
is ok to ignore the error? just curiosity
There was a problem hiding this comment.
I previously had it log the error, but decided to get rid of it since there's not really anything to do if we fail to get the subnets.
There was a problem hiding this comment.
not really? everything else would work just fine for users that aren't using a proxy.
There was a problem hiding this comment.
if we didn't detect any proxy info we can elide even querying this instead
pkg/cluster/nodes/node.go
Outdated
| subnets, err := getSubnets(defaultNetwork) | ||
| if err == nil { | ||
| details.Envs[noProxy] = strings.Join(append(subnets, details.Envs[noProxy]), ",") | ||
| details.Envs[strings.ToLower(noProxy)] = strings.Join(append(subnets, details.Envs[strings.ToLower(noProxy)]), ",") |
There was a problem hiding this comment.
we are only passing HTTP_PROXY and HTTPS_PROXY, maybe we should pass the lower caps of those too like you are doing here with no_proxy
There was a problem hiding this comment.
this is a good point, but would be reasonable to tackle in a different PR.
seperately I'd like to decouple this tooling more later, and I think HTTPS_PROXY will require a bit more finesse :-)
There was a problem hiding this comment.
On second though, we seem to be prioritizing the all-caps envs over the lower cased one on line 278 and also masking them to be all caps finally, was that intentional?
There was a problem hiding this comment.
on the host the user can control this if necessary and we set caps to be higher priority, but on the nodes we should maybe(?) supply the values in both keys since this isn't fully standardized
on the host side, the user can always explicitly set the caps ones when calling kind.
There was a problem hiding this comment.
seems curl only works with lowercase , there are few of stack overflows posts about this. I dont have it clear though
| ) | ||
|
|
||
| // NetworkInspect displays detailed information on one or more networks | ||
| func NetworkInspect(networkNames []string, format string) ([]string, error) { |
There was a problem hiding this comment.
IMHO their should be a higher level abstraction that dealing with docker format strings outside this package, but also fine to iterate on this.
k8s-ci-robot
removed
the
lgtm
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: amwat, BenTheElder, neolit123 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
CI (prow) is flaky today and we have a release to get going... |
|
/override pull-kind-conformance-parallel |
|
@BenTheElder: Overrode contexts on behalf of BenTheElder: pull-kind-conformance-parallel In response to this:
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. |
fixes #525
Compute the host docker network subnets and plumb through the NO_PROXY env.