Skip to content

Add VenafiConnection support for NGTS#811

Merged
inteon merged 6 commits into
masterfrom
add_ngts_vencon
Jun 3, 2026
Merged

Add VenafiConnection support for NGTS#811
inteon merged 6 commits into
masterfrom
add_ngts_vencon

Conversation

@inteon
Copy link
Copy Markdown
Contributor

@inteon inteon commented Jun 2, 2026

Adds support for authenticating using a VenafiConnection resource for NGTS.

Also unlocks OIDC authentication for NGTS.

@inteon inteon force-pushed the add_ngts_vencon branch 3 times, most recently from 2074c48 to c860a1e Compare June 2, 2026 14:31
@inteon inteon added test-e2e To signal e2e test job to be run test-ngts test-ark labels Jun 2, 2026
@inteon inteon closed this Jun 2, 2026
@inteon inteon reopened this Jun 2, 2026
@inteon inteon force-pushed the add_ngts_vencon branch 3 times, most recently from f306786 to db798ec Compare June 3, 2026 08:10
inteon added 2 commits June 3, 2026 10:39
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
…an be used for NGTS too

Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
@inteon inteon force-pushed the add_ngts_vencon branch from db798ec to 5856f4a Compare June 3, 2026 08:39
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
@inteon inteon force-pushed the add_ngts_vencon branch from 5856f4a to 58469e8 Compare June 3, 2026 08:48
Copy link
Copy Markdown
Contributor

@wallrj-cyberark wallrj-cyberark left a comment

Choose a reason for hiding this comment

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

Review Summary

This PR adds VenafiConnection support for NGTS to both the discovery-agent and venafi-kubernetes-agent Helm charts, and updates the Go code to support the new NGTSAccessToken backend type from venafi-connection-lib. The scope is clear and the implementation is well-structured.

What this PR does:

  • Adds config.venafiConnection values to the discovery-agent chart, with mutual exclusivity guards ({{- fail ...}}) for legacy keypair fields
  • Renames VenafiCloudVenafiConnectionVenafiConnection throughout Go code, reflecting that VenafiConnection mode is no longer VCP-only
  • Updates client_venconn.go to use the connection_details package from venafi-connection-lib (replacing the old details.VCP / details.TPP struct pattern), supporting both VCPAccessToken and NGTSAccessToken
  • Moves --venafi-cloud inside the {{- else }} block in the venafi-kubernetes-agent template so it is only rendered in keypair mode
  • Adds Helm unit tests for both charts covering VenafiConnection mode and mutual exclusivity rejections
  • Bumps venafi-connection-lib to a pre-release with NGTS support

Findings:

  • Two stray periods in the venafi-kubernetes-agent README description (carried over from the original, but now compounded)
  • The config.secretName field is not rejected by {{- fail }} in VenafiConnection mode — it is silently ignored. Worth confirming this is intentional
  • Minor cosmetic nit in the discovery-agent README

Jira issue: I searched Jira extensively but could not find an exact issue for this work. The closest candidates are:

  • VC-52460 — "NGTS cert-manager team component improvements (Fast Follows)" (broad epic, In Progress)
  • VC-35747 — "Make it possible to share a single VenafiConnection across the three components" (closed)
  • VC-31964 — "AGENT: Secretless authentication (OIDC) to VCP" (closed)

None of these are a direct match. You may want to create a new issue or ask Tim which one this is tracked under.

Prior art: PR #644 (Make venafiConnection a root-level Helm option) is related but tackles the venafi-kubernetes-agent chart structure only and is still open. This PR appears complementary rather than overlapping.

Overall: The changes are well-tested, the mutual exclusivity guards are thorough with clear error messages, and the backwards-compatibility test (--venafi-cloud tolerated alongside --venafi-connection) is a nice touch. A few documentation nits inline.

Review conducted with assistance from Claude (Opus 4.6)

Comment thread deploy/charts/venafi-kubernetes-agent/README.md Outdated
Comment thread deploy/charts/venafi-kubernetes-agent/values.schema.json Outdated
Comment thread deploy/charts/discovery-agent/README.md
Comment thread deploy/charts/discovery-agent/templates/deployment.yaml
Comment thread pkg/client/client_venconn.go
Comment thread pkg/client/client_venconn.go Outdated
Comment thread pkg/agent/config.go
Comment thread pkg/agent/config_test.go
inteon added 3 commits June 3, 2026 14:55
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
@inteon inteon force-pushed the add_ngts_vencon branch from 41e5afa to 45ee05c Compare June 3, 2026 12:56
Copy link
Copy Markdown
Contributor

@wallrj-cyberark wallrj-cyberark left a comment

Choose a reason for hiding this comment

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

LGTM — review comments addressed.

Review conducted with assistance from Claude (Opus 4.6)

@inteon inteon merged commit cbb1bfc into master Jun 3, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test-ark test-e2e To signal e2e test job to be run test-ngts

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants