[VC-42222] Proxy settings always ignored by hawksight · Pull Request #669 · jetstack/jetstack-secure

hawksight · 2025-06-18T08:26:36Z

fix: Only provide one env key in deployment.yaml
chore: Add two tests for env key and fix failing test for helm chart

This fixes an issue that meant the proxy settings were always ignored. The spec.template.spec.containers[0].env key was specified twice in the template:

Because the second invocation was never gated, it always took effect. This meant that even when specified, the proxy settings would be overridden.

Here is an example of what happened when set:

> helm template venafi-kubernetes-agent oci://registry.venafi.cloud/charts/venafi-kubernetes-agent --version v1.5.0 \
   -n venafi --set http_proxy="http://testing.me" \
   --set no_proxy="i_am_not_a_proxy.url" \
   --set https_proxy="https://testing.ssl" \
   | yq '. | select(.kind == "Deployment") | .spec.template.spec.containers[0].env'

Pulled: registry.venafi.cloud/charts/venafi-kubernetes-agent:v1.5.0
Digest: sha256:3a062cbe791dad88a5b858ca621bbccde956bfbf3e80ecd1a95646db1d26db2f
- name: POD_NAMESPACE
  valueFrom:
    fieldRef:
      fieldPath: metadata.namespace
- name: POD_NAME
  valueFrom:
    fieldRef:
      fieldPath: metadata.name
- name: POD_UID
  valueFrom:
    fieldRef:
      fieldPath: metadata.uid
- name: POD_NODE
  valueFrom:
    fieldRef:
      fieldPath: spec.nodeName

What happens now:

> helm template venafi-kubernetes-agent deploy/charts/venafi-kubernetes-agent --version v1.5.0 -n venafi \
   --set http_proxy="http://testing.me" \
   --set no_proxy="i_am_not_a_proxy_url" \
   --set https_proxy="https://testing.ssl" \
   | yq '. | select(.kind == "Deployment") | .spec.template.spec.containers[0].env'

- name: POD_NAMESPACE
  valueFrom:
    fieldRef:
      fieldPath: metadata.namespace
- name: POD_NAME
  valueFrom:
    fieldRef:
      fieldPath: metadata.name
- name: POD_UID
  valueFrom:
    fieldRef:
      fieldPath: metadata.uid
- name: POD_NODE
  valueFrom:
    fieldRef:
      fieldPath: spec.nodeName
- name: HTTP_PROXY
  value: http://testing.me
- name: HTTPS_PROXY
  value: https://testing.ssl
- name: NO_PROXY
  value: i_am_not_a_proxy_url

Signed-off-by: Peter Fiddes <peter.fiddes@jetstack.io>

hawksight · 2025-06-18T08:28:27Z

I have left in draft to find out if we are not using the helm unittests?
One test was failing, so I commented out the incorrect values being set. Probably a copy paste from the previous chart that never got changed.

Test do not run though when run manually:

> helm unittest deploy/charts/venafi-kubernetes-agent/ --strict

### Chart [ venafi-kubernetes-agent ] deploy/charts/venafi-kubernetes-agent/

 PASS  test deployment	deploy/charts/venafi-kubernetes-agent/tests/deployment_test.yaml

Charts:      1 passed, 1 total
Test Suites: 1 passed, 1 total
Tests:       8 passed, 8 total
Snapshot:    0 passed, 0 total
Time:        79.913708ms

wallrj · 2025-06-18T08:43:05Z

deploy/charts/venafi-kubernetes-agent/templates/deployment.yaml

-          - name: POD_NODE
-            valueFrom:
-              fieldRef:
-                fieldPath: spec.nodeName


Apologies. I introduced this bug, in 5679dc5

…o existing test Signed-off-by: Peter Fiddes <peter.fiddes@jetstack.io>

hawksight · 2025-06-18T10:03:20Z

If testing manually, then you have to escape the , values in the helm command:

helm template venafi-kubernetes-agent deploy/charts/venafi-kubernetes-agent \
  --version v1.5.0 -n venafi \
  --set http_proxy='http://<proxy server>:<port>' \
  --set no_proxy='127.0.0.1\,kubernetes.default.svc\,kubernetes.default.svc.cluster.local' \
  --set https_proxy='https://<proxy server>:<port>'

Don't believe this affects YAML values files as this is handled for you there.

wallrj · 2025-06-18T14:31:08Z

I was curious to know why this bug wasn't picked up by the kubeconform check, and the answer is that the duplicate env stanza only appears if we supply the the proxy values.
I verified this on the master branch by temporarily enabling the no_proxy value by default and re-running kubeconform, and this time it identified the problem:

--- a/deploy/charts/venafi-kubernetes-agent/values.yaml
+++ b/deploy/charts/venafi-kubernetes-agent/values.yaml
@@ -106,7 +106,7 @@ podSecurityContext: {}
 # Configures the NO_PROXY environment variable where a HTTP proxy is required,
 # but certain domains should be excluded.
 # +docs:property
-# no_proxy: 127.0.0.1,localhost
+no_proxy: 127.0.0.1,localhost

 # Add Container specific SecurityContext settings to the container. Takes
 # precedence over `podSecurityContext` when set. See

Branch: master:

$ make verify-helm-kubeconform
...
stdin - Deployment venafi-kubernetes-agent-release-name failed validation: error unmarshalling resource: error converting YAML to JSON: yaml: unmarshal errors:
  line 68: key "env" already set in map
make: *** [make/_shared/helm///helm.mk:186: verify-helm-kubeconform] Error 1

Branch: pf/env:

$ make verify-helm-kubeconform
...
$ echo $?
0

An idea for the future is that we could (somehow) extract all the example values (commented out) and the kube-conform check with those values enabled.

Signed-off-by: Richard Wall <richard.wall@cyberark.com>

wallrj · 2025-06-18T16:18:56Z

I pushed 37aad2f which enables the helm unittest tool to run in GitHub actions. The changes are based on @maelvls 's work in Firefly MR 543 (internal).

Here's the output:

https://github.com/jetstack/jetstack-secure/actions/runs/15738101973/job/44356148309?pr=669#step:7:87

wallrj

Thanks for fixing this @hawksight

I gave up trying to get the agent running in a cluster with a proxy available, but the changes to the Helm templates make perfect sense to me and the tests demonstrate that the correct proxy environment variables are added to the right part of the Deployment.

wallrj · 2025-06-18T16:44:28Z

deploy/charts/venafi-kubernetes-agent/tests/deployment_test.yaml

-      config.organisation: test_org
-      config.cluster: test_cluster
+      # config.organisation: test_org -> Should be removed?
+      # config.cluster: test_cluster -> Should be config.clusterName?


@maelvls Will know more about this. It was introduced in #543 but the tests were originally written for the jetstack-secure chart, so perhaps there's been some mixup when running the tests for each chart, which went unnoticed because the helm unittests weren't being run in CI.

jetstack-secure/deploy/charts/jetstack-agent/values.yaml

Lines 90 to 99 in 076f734

# -- Configuration section for the Jetstack Agent itself

config:

# -- Overrides the server if using a proxy between agent and Jetstack Secure

server: "https://platform.jetstack.io"

# -- REQUIRED - Your Jetstack Secure Organisation Name

organisation: ""

# -- REQUIRED - Your Jetstack Secure Cluster Name

cluster: ""

# -- Send data back to the platform every minute unless changed

period: "0h1m0s"

Better resolve this before merging this PR.

Looking at the test, its validating mounting custom volumes / volumeMount for a CA cert, which still works. I think the config option as you say is for the previous revision of the agent / chart.

Given it works without those set, let's remove the values and leave the test as it. Sorry I probably indeed added this based on some customer options.

Signed-off-by: Peter Fiddes <peter.fiddes@jetstack.io>

…/envs

deploy/charts/venafi-kubernetes-agent/tests/deployment_test.yaml

Signed-off-by: Richard Wall <richard.wall@cyberark.com>

wallrj · 2025-06-19T09:35:41Z

deploy/charts/venafi-kubernetes-agent/templates/deployment.yaml

+            valueFrom:
+              fieldRef:
+                fieldPath: spec.nodeName
+          {{- if or .Values.http_proxy .Values.https_proxy .Values.no_proxy }}


I think this if statement can be removed, because its original purpose was to omit the env stanza if no proxy env vars were needed. Now the env stanza is always included. I'll remove it.

wallrj · 2025-06-19T09:36:40Z

make/02_mod.mk

+## Run `helm unittest`.
+## @category Testing
+test-helm: | $(NEEDS_HELM-UNITTEST)
+	$(HELM-UNITTEST) ./deploy/charts/venafi-kubernetes-agent/


Not including the jetstack-secure chart here because it is soon to be removed: #672

Signed-off-by: Richard Wall <richard.wall@cyberark.com>

wallrj · 2025-06-19T10:04:43Z

I've done some further testing.
I ran make test-e2e-gke and observed the test pass. .

I then modified the values for the test to include the proxy settings.

diff --git a/hack/e2e/values.venafi-kubernetes-agent.yaml b/hack/e2e/values.venafi-kubernetes-agent.yaml
index 630d76e..99d1757 100644
--- a/hack/e2e/values.venafi-kubernetes-agent.yaml
+++ b/hack/e2e/values.venafi-kubernetes-agent.yaml
@@ -11,3 +11,7 @@ authentication:
 extraArgs:
 - --logging-format=json
 - --log-level=6
+
+https_proxy: "https://proxy:8080"
+
+no_proxy: 127.0.0.1,localhost,kubernetes.default.svc,kubernetes.default.svc.cluster.local

The test fail, because I hadn't deployed a proxy.
But I could see the new expected proxy environment variables in addition to the standard env vars

$ kubectl describe deploy -n venafi venafi-kubernetes-agent
...
    Environment:
      POD_NAMESPACE:   (v1:metadata.namespace)
      POD_NAME:        (v1:metadata.name)
      POD_UID:         (v1:metadata.uid)
      POD_NODE:        (v1:spec.nodeName)
      HTTPS_PROXY:    https://proxy:8080
      NO_PROXY:       127.0.0.1,localhost,kubernetes.default.svc,kubernetes.default.svc.cluster.local

wallrj

/approve
/lgtm

wallrj · 2025-06-26T11:44:13Z

@maelvls, @hawksight I stumbled on this comment in the Kubernetes code today:

// http.ProxyFromEnvironment doesn't respect CIDRs and that makes it impossible to exclude things like pod and service IPs from proxy settings
// ProxierWithNoProxyCIDR allows CIDR rules in NO_PROXY
--https://github.com/kubernetes/apimachinery/blob/d6651abdfec8bb9c001b60653bbda1168bb74a0b/pkg/util/net/http.go#L110-L114

https://github.com/kubernetes/kubernetes/blob/dcefe0ef41113324a4e4e43606f868a0858c55a6/staging/src/k8s.io/apimachinery/pkg/util/net/http.go#L396-L432

Perhaps we should use that wrapper too, to support CIDR in NO_PROXY?

hawksight added 2 commits June 18, 2025 09:13

fix: Only provide one env key in deployment.yaml

32c4fd4

Signed-off-by: Peter Fiddes <peter.fiddes@jetstack.io>

chore: Add two tests for env key and fix failing test for helm chart

4b6be34

Signed-off-by: Peter Fiddes <peter.fiddes@jetstack.io>

hawksight requested review from maelvls and wallrj June 18, 2025 08:36

wallrj reviewed Jun 18, 2025

View reviewed changes

chore: Add actual values of proxy settings from our documentation int…

c5c4033

…o existing test Signed-off-by: Peter Fiddes <peter.fiddes@jetstack.io>

Run the Helm tests automatically

37aad2f

Signed-off-by: Richard Wall <richard.wall@cyberark.com>

wallrj marked this pull request as ready for review June 18, 2025 16:19

wallrj approved these changes Jun 18, 2025

View reviewed changes

wallrj changed the title ~~fix: Proxy settings always ignored~~ [VC-42222] Proxy settings always ignored Jun 18, 2025

wallrj requested changes Jun 18, 2025

View reviewed changes

hawksight added 2 commits June 19, 2025 08:58

chore: Remove commented out values as not needed

a443ec8

Signed-off-by: Peter Fiddes <peter.fiddes@jetstack.io>

Merge branch 'pf/envs' of github.com:jetstack/jetstack-secure into pf…

de853ed

…/envs

wallrj reviewed Jun 19, 2025

View reviewed changes

deploy/charts/venafi-kubernetes-agent/tests/deployment_test.yaml Outdated Show resolved Hide resolved

Update deploy/charts/venafi-kubernetes-agent/tests/deployment_test.yaml

fb1813c

Signed-off-by: Richard Wall <richard.wall@cyberark.com>

wallrj force-pushed the pf/envs branch from 92a9c44 to fb1813c Compare June 19, 2025 09:32

wallrj reviewed Jun 19, 2025

View reviewed changes

Remove unnecessary if block around the Deployment env section

d40149b

Signed-off-by: Richard Wall <richard.wall@cyberark.com>

wallrj approved these changes Jun 19, 2025

View reviewed changes

wallrj merged commit ee0d9af into master Jun 19, 2025
2 checks passed

wallrj deleted the pf/envs branch June 19, 2025 10:07

	# -- Configuration section for the Jetstack Agent itself
	config:
	# -- Overrides the server if using a proxy between agent and Jetstack Secure
	server: "https://platform.jetstack.io"
	# -- REQUIRED - Your Jetstack Secure Organisation Name
	organisation: ""
	# -- REQUIRED - Your Jetstack Secure Cluster Name
	cluster: ""
	# -- Send data back to the platform every minute unless changed
	period: "0h1m0s"

Conversation

hawksight commented Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hawksight commented Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wallrj Jun 18, 2025

Choose a reason for hiding this comment

Uh oh!

hawksight commented Jun 18, 2025

Uh oh!

wallrj commented Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wallrj commented Jun 18, 2025

Uh oh!

wallrj left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wallrj Jun 18, 2025

Choose a reason for hiding this comment

Uh oh!

hawksight Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wallrj Jun 19, 2025

Choose a reason for hiding this comment

Uh oh!

wallrj Jun 19, 2025

Choose a reason for hiding this comment

Uh oh!

wallrj commented Jun 19, 2025

Uh oh!

wallrj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wallrj commented Jun 26, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hawksight commented Jun 18, 2025 •

edited

Loading

hawksight commented Jun 18, 2025 •

edited

Loading

wallrj commented Jun 18, 2025 •

edited

Loading

wallrj left a comment •

edited

Loading

hawksight Jun 19, 2025 •

edited

Loading