Updated NGINX Service Mesh references in Helm templates

[jbyers19]

Proposed changes

This change updates the deployments/helm-chart/templates/controller-daemonset.yaml and deployments/helm-chart/templates/controller-deployment.yaml templates. In the latest NSM release, the nsm.nginx.com/enable-ingress and nsm.nginx.com/enable-egress annotations were moved to labels. This change updates the templates accordingly. It also removes most if .Values.nginxServiceMesh.enable/enableEgress references as these values are now handled by a mutating webhook run by NSM.

Update: reverted this change. One thing to note is that I removed the if for the POD_SERVICEACCOUNT env variable so it is now added regardless of whether NSM is enabled or not. Since this is an environment variable used by the KIC controller, I thought it would be better to always include it in the pod spec.

Testing:

• Verified helm lint passed on the updated templates:
  • helm lint . -f values-nsm.yaml --set controller.kind=daemonset
  • helm lint . -f values-nsm.yaml --set controller.kind=deployment
• Manually verified rendered manifest matched what was expected:
  • helm install kic -f values-nsm.yaml --set controller.kind=daemonset . --dry-run
  • helm install kic -f values-nsm.yaml --set controller.kind=deployment . --dry-run
• Verified linting and unit tests passed.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

• I have read the CONTRIBUTING doc
• I have added tests that prove my fix is effective or that my feature works
• I have checked that all unit tests pass after adding my changes
• I have updated necessary documentation
• I have rebased my branch onto main
• I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

[codecov-commenter]

Codecov Report

Merging #3602 (487f7ee) into main (9ab3c9b) will decrease coverage by 0.11%.
The diff coverage is n/a.

❗ Current head 487f7ee differs from pull request most recent head 67e1713. Consider uploading reports for the commit 67e1713 to get more accurate results

@@            Coverage Diff             @@
##             main    #3602      +/-   ##
==========================================
- Coverage   52.35%   52.24%   -0.11%     
==========================================
  Files          59       59              
  Lines       16880    16873       -7     
==========================================
- Hits         8838     8816      -22     
- Misses       7747     7762      +15     
  Partials      295      295              

see 7 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[lucacome]

@jbyers19 does this mean that we can remove -spire-agent-address and -enable-internal-routes entirely from the codebase?

[jbyers19]

@lucacome, no. The internal functionality is still the same. The webhook just adds all these values to the pod spec when it is deployed. So if a user is deploying the ingress from a plain manifest instead of helm they do not have to manually add all these fields. They just need to add the nsm.nginx.com/enable-ingress/egress labels and the webhook will deal with the rest.

[lucacome]

so we're still giving users the option to use those flags as well?

[jbyers19]

Yes, a user can manually add those if they want. However, the webhook will detect the incoming ingress deployment/daemonset and automatically add those to the spec if the nsm.nginx.com/enable-ingress|egress=true labels are present and set to true.

Here's a snippet from that webhook (NSM code) that adds those specific flags you are asking about:

	args = append(args, "-spire-agent-address=/run/spire/sockets/agent.sock")

	if pod.HasEgressLabelEnabled(objForMutation) {
		args = append(args, "-enable-internal-routes")
	}

The webhook will add all those fields I removed from the template if the NSM label is present and set to true. That's why it is no longer necessary to have them as part of these templates.

[lucacome]

I was hoping that we could remove them for good and have fewer moving pieces 😅

[lucacome]

@jbyers19 I was actually thinking that IMO it would make more sense for KIC to enable the internal routes, etc. based on the labels, rather than having NSM add CLI arguments to KIC based on KIC's labels. What do you think?

[jbyers19]

@lucacome, maybe. We already did the work and added the webhook to do this in the last NSM release though. I think it would only work for args though. We would still need the labels, volumes, etc. in the pod spec.

[vepatel]

@jbyers19 looks like pr has fair bit of merge commits, can you please rebase with main and only keep the relevant bits