Extending the no_proxy env variable to automatically include all the nodes of the cluster

[wherka-ama]

See: #2884

[k8s-ci-robot]

Hi @wherka-ama. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[wherka-ama]

/retest

[k8s-ci-robot]

@wherka-ama: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

[wherka-ama]

/cc @BenTheElder @aojea
I'm pretty sure the test failure is unrelated to the change. It's worth rerunning it.

[aojea]

/ok-to-test
/retest

[BenTheElder]

we should probably just restructure to pass these in or add them to the list at the call site?

[aojea]

I was thinking in adding them here

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

Lines 41 to 63 in b6bc112

	func getProxyEnvs(cfg *config.Cluster, getEnv func(string) string) map[string]string {
	envs := make(map[string]string)
	for _, name := range []string{HTTPProxy, HTTPSProxy, NOProxy} {
	val := getEnv(name)
	if val == "" {
	val = getEnv(strings.ToLower(name))
	}
	if val != "" {
	envs[name] = val
	envs[strings.ToLower(name)] = val
	}
	}
	// Specifically add the cluster subnets to NO_PROXY if we are using a proxy
	if len(envs) > 0 {
	noProxy := envs[NOProxy]
	if noProxy != "" {
	noProxy += ","
	}
	noProxy += cfg.Networking.ServiceSubnet + "," + cfg.Networking.PodSubnet
	envs[NOProxy] = noProxy
	envs[strings.ToLower(NOProxy)] = noProxy
	}
	return envs

that will cascade for all the providers

[BenTheElder]

The docker implementation already has this AFAICT, we just need to modify podman to match and drop the docker changes, I think #2885 (comment)

[BenTheElder]

I don't think it's worth moving to common, it's fairly noise to plumb any deeper as we just need to append the names

[wherka-ama]

If we were to match the podman flow with the docker one in that regard I guess the best option to start the restructuring from the planCreation and trickle it down. I'd be happy to do that.

My entrypoint would be:

kind/pkg/cluster/internal/providers/podman/provision.go

Line 41 in b2ed53e

genericArgs, err := commonArgs(cfg, networkName)

And the reference:

kind/pkg/cluster/internal/providers/docker/provision.go

Lines 42 to 53 in b2ed53e

	names := make([]string, len(cfg.Nodes))
	for i, node := range cfg.Nodes {
	name := nodeNamer(string(node.Role)) // name the node
	names[i] = name
	}
	haveLoadbalancer := config.ClusterHasImplicitLoadBalancer(cfg)
	if haveLoadbalancer {
	names = append(names, nodeNamer(constants.ExternalLoadBalancerNodeRoleValue))
	}
	// these apply to all container creation
	genericArgs, err := commonArgs(cfg.Name, cfg, networkName, names)

@BenTheElder @aojea : are you fine with that direction?

[wherka-ama]

I've just pushed a fresh commit with the refactoring. In short - it's an alignment with what we have for docker in that regards. I must admit I like this version a lot more than the previous one. I hope you don't disagree ;-)

/cc @BenTheElder @aojea

[BenTheElder]

aren't we already adding these here?

[wherka-ama]

Good point! Indeed it looks like we can revert for docker and try to pass the list the same way as it's done here indeed.. I'll do it right away. Thanks!

[aojea]

/lgtm
/approve
/label tide/merge-method-squash

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aojea, wherka-ama

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:

• pkg/cluster/internal/providers/podman/OWNERS [aojea]

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