Skip to content

fix(site-agent): use mTLS for Flow gRPC and protect temporal-certs secret #2942

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
shayan1995 merged 2 commits into from
Jun 29, 2026
+19 −3
Merged

fix(site-agent): use mTLS for Flow gRPC and protect temporal-certs secret #2942

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
125842b
fix(site-agent): use mTLS for Flow gRPC and protect temporal-certs se…
shayan1995 Jun 26, 2026
309fe8c
fix(site-agent): enable Flow gRPC by default (FLOW_GRPC_ENABLED=true)
shayan1995 Jun 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion helm/rest/nico-rest-site-agent/templates/temporal-certs-secret.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,18 @@
# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
# SPDX-License-Identifier: Apache-2.0

# Temporal client TLS certs for site-agent — placeholder values populated by bootstrap
# Temporal client TLS certs for site-agent — placeholder values populated by bootstrap.
# resource-policy: keep prevents helm upgrade from overwriting certs written by the
# bootstrap process; deleting and re-installing the release creates a fresh placeholder.
Comment thread
coderabbitai[bot] marked this conversation as resolved.
apiVersion: v1
kind: Secret
metadata:
name: {{ .Values.secrets.temporalClientCerts }}
namespace: {{ include "nico-rest-site-agent.namespace" . }}
labels:
{{- include "nico-rest-site-agent.labels" . | nindent 4 }}
annotations:
helm.sh/resource-policy: keep
Comment on lines +5 to +15

@coderabbitai coderabbitai Bot Jun 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect whether this chart has any preserve-existing-data logic around the kept Secret.
rg -n --hidden --glob 'helm/rest/nico-rest-site-agent/**' \
  'temporalClientCerts|temporal-client-site-agent-certs|helm.sh/resource-policy|lookup|stringData' \
  helm/rest/nico-rest-site-agent

Repository: NVIDIA/infra-controller

Length of output: 797

🏁 Script executed:

#!/bin/bash
set -euo pipefail

cat -n helm/rest/nico-rest-site-agent/templates/temporal-certs-secret.yaml

Repository: NVIDIA/infra-controller

Length of output: 1062

🌐 Web query:

Helm "helm.sh/resource-policy: keep" upgrade patch behavior Secret

💡 Result:

Helm annotation helm.sh/resource-policy: keep on a Secret tells Helm to skip deleting that Secret during helm upgrade when the upgrade would otherwise result in its deletion. However, once kept, the Secret becomes “orphaned” and Helm “will no longer manage it in any way”, which means Helm will not reliably patch/update its content on subsequent upgrades. [1][2] So for “upgrade patch behavior” on Secrets: - If the Secret is still rendered by the chart, Helm may patch it normally as part of the upgrade; the main guarantee of keep is about avoiding deletion when Helm would remove it. - If the Secret would be removed because it no longer appears in the rendered manifests, keep prevents deletion, but then Helm stops managing it; patching/updating it from the chart is not expected. [1][2] Notes from Helm issue history: there were versions where the annotation was not respected during helm upgrade (leading to deletion/data loss), with reports that it worked again in later versions. [3] Reference: Helm docs for helm.sh/resource-policy: keep explicitly state it’s used to skip deleting resources during operations like helm upgrade and that the resource becomes orphaned / unmanaged afterward. [1][2]

Citations:

Prevent Helm upgrades from blanking the Temporal cert Secret
helm.sh/resource-policy: keep only avoids deletion; it does not make this Secret install-only. Because the template still renders empty stringData, a routine helm upgrade can reapply those blanks and overwrite the bootstrap-populated certs. Use lookup/conditional rendering so upgrades preserve the existing data.

🧰 Tools
🪛 YAMLlint (1.37.1)

[error] 13-13: syntax error: expected the node content, but found '-'

(syntax)

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@helm/rest/nico-rest-site-agent/templates/temporal-certs-secret.yaml` around
lines 5 - 15, The Temporal cert Secret template still renders blank data on
every upgrade, so the bootstrap-populated values can be overwritten even with
helm.sh/resource-policy: keep. Update temporal-certs-secret.yaml to use lookup
or conditional rendering in the Secret manifest so the existing Secret data is
preserved on helm upgrade, and only emit the placeholder empty data when the
Secret does not already exist.

Source: Path instructions

type: Opaque
stringData:
otp: ""
Expand Down
2 changes: 2 additions & 0 deletions helm/rest/nico-rest-site-agent/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,8 @@ envConfig:
ENABLE_TLS: "true"
NICO_ADDRESS: ""
NICO_SEC_OPT: "0"
FLOW_GRPC_ENABLED: "true"
FLOW_GRPC_SEC_OPT: "2"
CLUSTER_ID: ""
TEMPORAL_HOST: "temporal-frontend.temporal"
TEMPORAL_PORT: "7233"
Expand Down
14 changes: 12 additions & 2 deletions rest-api/site-workflow/pkg/grpc/client/flow_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,9 +158,19 @@ func NewFlowGrpcClient(config *FlowGrpcClientConfig) (client *FlowGrpcClient, er
if !capool.AppendCertsFromPEM(cabytes) {
return nil, fmt.Errorf("FlowGrpcClient: Failed to append CA cert to CA pool")
}
// Use GetClientCertificate (not Certificates) to unconditionally present
// the client cert. With Certificates, Go's TLS stack only selects a cert
// whose issuer matches the acceptable CA list from the server's
// CertificateRequest; when no match is found it silently sends no cert,
// causing the server to reject with "tls: certificate required".
// GetClientCertificate bypasses that matching and always returns the cert,
// leaving verification to the server — the same approach used in
// rest-api/flow/pkg/certs/certs.go TLSConfig().
mutualTLSConfig := &tls.Config{
Certificates: []tls.Certificate{clientCert},
RootCAs: capool,
GetClientCertificate: func(*tls.CertificateRequestInfo) (*tls.Certificate, error) {
return &clientCert, nil
},
RootCAs: capool,
}
creds := credentials.NewTLS(mutualTLSConfig)

Expand Down
Loading