Add VenafiConnection support for NGTS#811
Conversation
2074c48 to
c860a1e
Compare
f306786 to
db798ec
Compare
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>
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
wallrj-cyberark
left a comment
There was a problem hiding this comment.
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.venafiConnectionvalues to the discovery-agent chart, with mutual exclusivity guards ({{- fail ...}}) for legacy keypair fields - Renames
VenafiCloudVenafiConnection→VenafiConnectionthroughout Go code, reflecting that VenafiConnection mode is no longer VCP-only - Updates
client_venconn.goto use theconnection_detailspackage fromvenafi-connection-lib(replacing the olddetails.VCP/details.TPPstruct pattern), supporting bothVCPAccessTokenandNGTSAccessToken - Moves
--venafi-cloudinside 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-libto a pre-release with NGTS support
Findings:
- Two stray periods in the
venafi-kubernetes-agentREADME description (carried over from the original, but now compounded) - The
config.secretNamefield 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)
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>
wallrj-cyberark
left a comment
There was a problem hiding this comment.
LGTM — review comments addressed.
Review conducted with assistance from Claude (Opus 4.6)
Adds support for authenticating using a VenafiConnection resource for NGTS.
Also unlocks OIDC authentication for NGTS.