add systemProxy env vars

[jessegoodier]

What does this PR change?

Add systemProxy env variables to amp/gmp/federator containers

Does this PR rely on any other PRs?

no

How does this PR impact users? (This is the kind of thing that goes in release notes!)

Added systemProxy env variables to amp/gmp/federator containers

What risks are associated with merging this PR? What is required to fully test this PR?

Worst case would be spacing, but I verified each block was aligned, though I didn't deploy each.

How was this PR tested?

changing gmp and amp, federator on:

global:
  gmp:
    enabled: true
    prometheusServerEndpoint: https://localhost:8085/xxxxx/
    remoteWriteService: https://test.com
    sigv4:
      region: us-east-2
systemProxy:
  enabled: true
  httpProxyUrl: http://test
  httpsProxyUrl: http://test1
  noProxy: "0,localhost"

Have you made an update to documentation? If so, please provide the corresponding PR.

NA

[chipzoller]

Why are the env vars being duplicated except with different cases?

[jessegoodier]

Why are the env vars being duplicated except with different cases?

Best I could find is because linux doesn't have a governing body.

[thomasvn]

Interesting that .Values.systemProxy already exists, but isn't currently referenced anywhere in the helm chart.

cost-analyzer-helm-chart/cost-analyzer/values.yaml

Lines 300 to 306 in 374399c

	# Adds an httpProxy as an environment variable. systemProxy.enabled must be `true`to have any effect.
	# Ref: https://www.oreilly.com/library/view/security-with-go/9781788627917/5ea6a02b-3d96-44b1-ad3c-6ab60fcbbe4f.xhtml
	systemProxy:
	enabled: false
	httpProxyUrl: ""
	httpsProxyUrl: ""
	noProxy: ""

A few requests for this PR:

• Could we further document in the values.yaml specific situations when a user needs to set this value?
• We are only setting this env var for containers created in the cost-analyzer-deployment-template.yaml and federator-deployment-template.yaml. Do we need to set these env vars for other containers? (e.g. Prometheus, KSM, metrics emitter, etc)

[jessegoodier]

Interesting that .Values.systemProxy already exists, but isn't currently referenced anywhere in the helm chart.

cost-analyzer-helm-chart/cost-analyzer/values.yaml

Lines 300 to 306 in 374399c

	# Adds an httpProxy as an environment variable. systemProxy.enabled must be `true`to have any effect.
	# Ref: https://www.oreilly.com/library/view/security-with-go/9781788627917/5ea6a02b-3d96-44b1-ad3c-6ab60fcbbe4f.xhtml
	systemProxy:
	enabled: false
	httpProxyUrl: ""
	httpsProxyUrl: ""
	noProxy: ""

A few requests for this PR:

• Could we further document in the values.yaml specific situations when a user needs to set this value?
• We are only setting this env var for containers created in the cost-analyzer-deployment-template.yaml and federator-deployment-template.yaml. Do we need to set these env vars for other containers? (e.g. Prometheus, KSM, metrics emitter, etc)

Thank you for the review!
we do use this, currently here: https://github.com/kubecost/cost-analyzer-helm-chart/blob/v1.106/cost-analyzer/templates/cost-analyzer-deployment-template.yaml#L850

It is only needed for things that would exit the cluster, so what I have here. Perhaps thanos stuff, but that's outside of scope for what we need here.

I'll add a comment. Suggesting that this problem is a good use-case for a service mesh. :)

[chipzoller]

Why are the env vars being duplicated except with different cases?

Best I could find is because linux doesn't have a governing body.

Most env vars standardize on upper case, these proxy vars as well, so I suggest dropping the lower case duplicates.

[jessegoodier]

Why are the env vars being duplicated except with different cases?

Best I could find is because linux doesn't have a governing body.

Most env vars standardize on upper case, these proxy vars as well, so I suggest dropping the lower case duplicates.

I agree that it looks dumb and someone should have figured this out 20 years ago.

These systemProxy variables with the duplicate case are not new to our helm chart, or linux.

If you want to test the possible permutations, I'd be happy to review that change.

Otherwise, I'm simply looking to fix the problem for the small handful of users that depend on this setting.

[thomasvn]

@jessegoodier Tested this PR with the following values. Validated that it's adding the system proxy env vars to the cost-model in cost-analyzer and federator.

systemProxy:
  enabled: true
  httpProxyUrl: "http://TEST"
  httpsProxyUrl: "https://TEST"
  noProxy: "TEST"
federatedETL:
  federator:
    enabled: true

helm template ./cost-analyzer -f values.yaml > test1.yaml