Skip to content

feat(helm): add nodeport access type support to Helm chart#2401

Open
styx0r wants to merge 2 commits intoskupperproject:mainfrom
styx0r:feat/helm-nodeport-access-type
Open

feat(helm): add nodeport access type support to Helm chart#2401
styx0r wants to merge 2 commits intoskupperproject:mainfrom
styx0r:feat/helm-nodeport-access-type

Conversation

@styx0r
Copy link

@styx0r styx0r commented Mar 12, 2026

Summary

The Skupper controller already has full support for nodeport as a RouterAccess access type (including validation, SKUPPER_CLUSTER_HOST env var reading, etc.), but the Helm chart had no way to configure the required controller env vars. This PR closes that gap.

Changes:

  • scripts/skupper-helm-chart-generator.sh: adds three new values to the generated values.yaml (clusterHost, enabledAccessTypes, defaultAccessType) and injects Helm conditional env-var blocks into both the cluster and namespace deployment templates, immediately before the SKUPPER_KUBE_ADAPTOR_IMAGE env var (used as a stable anchor).
  • charts/skupper/README.md: documents the three new values in a new "Access Type Configuration" section, with a concrete "Using NodePort" example.
  • Bonus fix: replaces all sed -i 'pattern' calls (GNU-only) with the portable sed 'pattern' file > file.tmp && mv file.tmp file pattern, fixing a pre-existing breakage on macOS where BSD sed requires sed -i ''.

Behaviour

Users who do not set any of the new values see zero change — the controller's own defaults apply as before (local,loadbalancer,route).

To deploy with nodeport:

helm install skupper oci://quay.io/skupper/helm/skupper \
  --set clusterHost=<NODE_IP> \
  --set "enabledAccessTypes=local,loadbalancer,route,nodeport" \
  --set defaultAccessType=nodeport

Test plan

  • Run make generate-skupper-helm-chart and verify charts/skupper/values.yaml contains the three new values
  • Inspect generated templates for the Helm conditional blocks ({{- if .Values.clusterHost }}, etc.)
  • helm lint charts/skupper passes
  • Dry-run with --set clusterHost=... --set enabledAccessTypes=... --set defaultAccessType=nodeport renders env vars in the deployment
  • Dry-run without those flags renders no new env vars
  • Deploy on a real cluster, create a Site + RouterAccess with accessType: nodeport, verify RouterAccess status shows the node IP and high ports

Assisted by Claude Code

Expose three new controller configuration values in the Skupper Helm
chart so that nodeport (and other non-default access types) can be
configured at install time:

  clusterHost          - IP/hostname of any cluster node; required
                         when nodeport is in enabledAccessTypes
  enabledAccessTypes   - comma-separated list of access types to enable
                         (defaults to local,loadbalancer,route when empty)
  defaultAccessType    - default access type for sites that don't specify
                         one; controller auto-selects when empty

The chart generator script injects these values into values.yaml and
uses awk to add Helm conditional blocks immediately before the
SKUPPER_KUBE_ADAPTOR_IMAGE env var in both cluster and namespace
deployment templates, so users who do not set these values see no
change in behaviour.

Also fixes a pre-existing macOS BSD sed incompatibility: replace all
`sed -i 'pattern' file` calls (GNU-only) with the portable tmp-file
pattern `sed 'pattern' file > file.tmp && mv file.tmp file`.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@styx0r
Copy link
Author

styx0r commented Mar 13, 2026

Tested end-to-end on a real Kubernetes cluster:

  1. Deployed the controller with helm install using --set clusterHost=
    --set "enabledAccessTypes=local,loadbalancer,route,nodeport" --set defaultAccessType=nodeport
  2. Created a Site and RouterAccess with accessType: nodeport
  3. RouterAccess status resolved with the node IP and Nodeports
  4. Created an AccessGrant and redeemed it on a second cluster using an AccessToken
  5. kubectl get link -A on the second cluster shows Ready / OK
  6. Cross-cluster link is established and stable over nodeport.

Copy link
Member

@fgiorgetti fgiorgetti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of minor suggestions. But overall, it looks good to me.

Correct the default value for enabledAccessTypes in the Helm chart generator script to "local,loadbalancer,route". Update the README to reflect the change in the Helm install command and add a note clarifying that defaultAccessType is optional, with the controller auto-selecting the default when omitted.
@styx0r styx0r requested a review from fgiorgetti March 16, 2026 18:40
Copy link
Member

@fgiorgetti fgiorgetti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution!
It looks good to me.

@ajssmith @AryanP123 @JPadovano1483 @nluaces I would like to get your thoughts here as well,
Thanks.

@styx0r
Copy link
Author

styx0r commented Mar 17, 2026

Thank you very much for your efforts. I hope this small addition helps others as well.

@AryanP123
Copy link
Contributor

LGTM. One note: the README says enabledAccessTypes defaults to local,loadbalancer,route when empty, but the chart generator sets it to that string explicitly. Should the generator use "" instead so the controller’s default in config.go is the single source of truth?

Let me know what you think.

@fgiorgetti
Copy link
Member

LGTM. One note: the README says enabledAccessTypes defaults to local,loadbalancer,route when empty, but the chart generator sets it to that string explicitly. Should the generator use "" instead so the controller’s default in config.go is the single source of truth?

Let me know what you think.

Actually, if the SKUPPER_ENABLED_ACCESS_TYPES environment variable is set with an empty value, no access types will be enabled. So I believe it is safer to keep them declared explicitly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants