Added support for env *_PROXY variables for agent loadbalancer

[pierre-az]

Types of Changes

To me, this is a bugfix as the doc specifies we can use these env variables for k3s: https://docs.k3s.io/advanced#configuring-an-http-proxy.
There is also this documentation that may be misleading as it suggests that in addition to proxies, you should open your network to various URLs, which is incompatible: https://docs.k3s.io/installation/requirements#networking.

Verification

You can test the new agent on a airgapped environment, where internet access is only possible through an external proxy.

This was tested in production on such environment on my end.

Testing

There are no linked unit tests as this is the first time I'm working on Go.

Linked Issues

Linked issue is: #8976

User-Facing Change

HTTP_PROXY, HTTPS_PROXY and NO_PROXY environment variables are now taken into account by the agent loadbalancer if K3S_AGENT_HTTP_PROXY_ALLOWED env variable is set to true.
This however doesn't affect local requests as the function used prevents that: https://pkg.go.dev/net/http#ProxyFromEnvironment. 

Further Comments

As this is a known issue for net.Dialer, this fix is based on another library which can be found on: https://github.com/mwitkow/go-http-dialer

[brandond]

This needs to be opt-in. As I mentioned in the issue, we DO NOT want the agent to use a proxy by default, even if one is configured. Suddenly doing so after an upgrade could be a breaking change for many users. If you want to enable this behavior, it needs to be gated on a CLI flag or environment variable.

[pierre-az]

This needs to be opt-in. As I mentioned in the issue, we DO NOT want the agent to use a proxy by default, even if one is configured. Suddenly doing so after an upgrade could be a breaking change for many users. If you want to enable this behavior, it needs to be gated on a CLI flag or environment variable.

That is not an issue to do so, however I fail to imagine a case where anyone is using a proxy, but not using a proxy at the same time, as the proxy variables are taken into account everywhere else in the code.
Could you please clarify this behaviour so that we can properly document it ?

[brandond]

I fail to imagine a case where anyone is using a proxy, but not using a proxy at the same time.

Consider someone who has configured a HTTP proxy, but has not excluded their server addresses or hostnames via NO_PROXY. Currently their agents are connecting directly to the server due to the lack of proxy support. With this change, the connection would now be made via the proxy, despite the fact that the proxy may not allow that access, or even have connectivity to the server.

the proxy variables are taken into account everywhere else in the code

I don't believe this statement is correct. There are other places in the code where the agent does not include proxy support, for example the client access helpers that facilitate retrieving agent configuration do not use the default HTTP client and should not be using the proxy:

k3s/pkg/clientaccess/token.go

Lines 31 to 41 in 3f23723

	defaultClient = &http.Client{
	Timeout: defaultClientTimeout,
	}
	insecureClient = &http.Client{
	Timeout: defaultClientTimeout,
	Transport: &http.Transport{
	TLSClientConfig: &tls.Config{
	InsecureSkipVerify: true,
	},
	},
	}

[pierre-az]

Consider someone who has configured a HTTP proxy, but has not excluded their server addresses or hostnames via NO_PROXY. Currently their agents are connecting directly to the server due to the lack of proxy support. With this change, the connection would now be made via the proxy, despite the fact that the proxy may not allow that access, or even have connectivity to the server.

That I get, it is probably better to be opt-in if people already made arrangements to make it work another way. I will go through an env variable as it seems the easiest way for a newbie in Go.

I don't believe this statement is correct. There are other places in the code where the agent does not include proxy support, for example the client access helpers that facilitate retrieving agent configuration do not use the default HTTP client and should not be using the proxy:

k3s/pkg/clientaccess/token.go

Lines 31 to 41 in 3f23723

	defaultClient = &http.Client{
	Timeout: defaultClientTimeout,
	}
	insecureClient = &http.Client{
	Timeout: defaultClientTimeout,
	Transport: &http.Transport{
	TLSClientConfig: &tls.Config{
	InsecureSkipVerify: true,
	},
	},
	}

&http.Client is using environment variables such as *_PROXY, as opposite to the Dialer that is created in the loadbalancer code.
Source is a bit old but I think it follows any library standards: https://stackoverflow.com/questions/14661511/setting-up-proxy-for-http-client.

Overall you're right about the opt-in part, but I also believe we should consider this option to be core as it would make more sense, if it is ever doable.

Will come back with updated changes, thanks for your time !

[ChristianCiach]

That is not an issue to do so, however I fail to imagine a case where anyone is using a proxy, but not using a proxy at the same time, as the proxy variables are taken into account everywhere else in the code.

We (as heavy users of K3s) only want to use the proxy for pulling images. Yes, there is CONTAINERD_HTTP(S)_PROXY now, but this was not always the case. So in the past we had to set HTTPS_PROXY and we didn't specify any node-IPs as NO_PROXY, because it just wasn't necessary and we didn't give it any second thought.

I am pretty sure that we have quite a few clusters running (hard to reach, because air-gapped) where we never came around to migrate to the CONTAINERD_ prefixed environment variables.

[pierre-az]

Hello,

I've introduced the USE_PROXY variable, that can either be set to true or nothing to indicate if you're willing to use a proxy.
This has been tested three times: an env without proxy without the variable set, an env with proxy without the USE_PROXY set to true (which failed as expected) and the same env with the USE_PROXY variable set to true, which worked.

Please let me know if you need anything, this may not be the cleanest PR yet.

[ChristianCiach]

I have no horse in this race, so please don't mind my comments too much. But I wonder if introducing yet another variable is really the best way to approach this.

Idea: Instead of adding a boolean variable USE_PROXY (which name I think is too generic; it could clash with the system configuration of the host), maybe we should configure the proxy address to be used by the k3s agent directly. Maybe something like K3S_LB_HTTPS_PROXY. Please don't focus on the name too much. My point is to avoid the extra boolean variable in favor of a single variable that configures the proxy for this use-case directly.

This would also have the added benefit that users could configure another proxy address here that may differ from the system- or containerd proxy configuration.

[brandond]

&http.Client is using environment variables such as *_PROXY, as opposite to the Dialer that is created in the loadbalancer code.

http.Client doesn't manage proxies, that is part of http.Transport. The Transports for the insecure client, and the one created to validate the cluster CA, do not set Proxy: ProxyFromEnvironment and will not use proxies.

• k3s/pkg/clientaccess/token.go

  Lines 34 to 41 in 3f23723

  	insecureClient = &http.Client{
  	Timeout: defaultClientTimeout,
  	Transport: &http.Transport{
  	TLSClientConfig: &tls.Config{
  	InsecureSkipVerify: true,
  	},
  	},
  	}

• k3s/pkg/clientaccess/token.go

  Lines 254 to 260 in 3f23723

  	return &http.Client{
  	Timeout: defaultClientTimeout,
  	Transport: &http.Transport{
  	DisableKeepAlives: true,
  	TLSClientConfig: tlsConfig,
  	},
  	}

[pierre-az]

I have no horse in this race, so please don't mind my comments too much. But I wonder if introducing yet another variable is really the best way to approach this.

Idea: Instead of adding a boolean variable USE_PROXY (which name I think is too generic; it could clash with the system configuration of the host), maybe we should configure the proxy address to be used by the k3s agent directly. Maybe something like K3S_LB_HTTPS_PROXY. Please don't focus on the name too much. My point is to avoid the extra boolean variable in favor of a single variable that configures the proxy for this use-case directly.

This would also have the added benefit that users could configure another proxy address here that may differ from the system- or containerd proxy configuration.

Your comments are valuable since you are using a setup I didn't know was in place!

I prefer the boolean approach (renaming the env variable to be more explicit), since it looks to be to good practice to setup the proxy variables in one file.
I'm not firmly against adding the option, but having a default behavior of fetching the environment variables when K3S_USE_LB_PROXY (still USE_PROXY) is set to true would make more sense, then having the option to specify the proxy in another variable would not be a big change.

[pierre-az]

&http.Client is using environment variables such as *_PROXY, as opposite to the Dialer that is created in the loadbalancer code.

http.Client doesn't manage proxies, that is part of http.Transport. The Transports for the insecure client, and the one created to validate the cluster CA, do not set Proxy: ProxyFromEnvironment and will not use proxies.

• k3s/pkg/clientaccess/token.go

  Lines 34 to 41 in 3f23723

  	insecureClient = &http.Client{
  	Timeout: defaultClientTimeout,
  	Transport: &http.Transport{
  	TLSClientConfig: &tls.Config{
  	InsecureSkipVerify: true,
  	},
  	},
  	}

• k3s/pkg/clientaccess/token.go

  Lines 254 to 260 in 3f23723

  	return &http.Client{
  	Timeout: defaultClientTimeout,
  	Transport: &http.Transport{
  	DisableKeepAlives: true,
  	TLSClientConfig: tlsConfig,
  	},
  	}

As you may be right, even if you were to setup http.Transport method it would ignore proxies as I believe these requests are going to the LB, hence localhost, at what point proxies are ignored by the method.

Anyway, tests were conclusive in production with the newly built agent, would you have any remarks on the actualized code on that PR ?

[brandond]

nits: idiomatic go

[brandond]

Would you mind doing a rebase, squashing all the commits, and making sure that the resulting commit is properly signed-off? It looks like DCO is failing.

You also need to go mod tidy and check in the resulting changes. go-http-dialer is no longer an indirect dep, since it's used directly by k3s - so the current go.mod is incorrect.

[pierre-az]

I absolutely exploded the repository while trying to squash all commits into one, hence the new PR: #9070. Really sorry about that. @brandond