Integration with SandD

[kerthcet]

What this PR does / why we need it

Which issue(s) this PR fixes

Fixes #

Special notes for your reviewer

Does this PR introduce a user-facing change?

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Suppressed comments (3)

pkg/provider/aws/translate.go:86

• When SandD is enabled, the shim ultimately runs exec "$@", but runArgs is built from spec.Command then spec.Args. If spec.Command is empty (a valid Kubernetes case where the image ENTRYPOINT should run), $@ starts with the first arg from spec.Args (or is empty), which will fail and break workloads. Consider only enabling the shim when an explicit Command is present; otherwise fall back to the normal Docker ENTRYPOINT/CMD mapping to preserve Kubernetes semantics.

	var runArgs []string
	if spec.Sandd.Enabled() {
		runArgs = writeSanddEntrypoint(&b, spec)
	} else {
		// No shim: map Command/Args straight onto Docker's --entrypoint/CMD as before.

pkg/provider/provider.go:340

• SanddConfig.Enabled() only checks AuthKey, but the struct doc states ControlServer and ServerURL are required when AuthKey is set. As written, a partially configured SandD setup will be treated as enabled and attempt injection with empty values. Either validate elsewhere or make Enabled() reflect the documented requirements.

// Enabled reports whether the SandD daemon should be injected. A missing AuthKey
// means the operator did not opt in, so an adapter emits its plain bootstrap.
func (s SanddConfig) Enabled() bool { return s.AuthKey != "" }

cmd/main.go:364

• SandD env configuration is read from process env, but there is no validation/logging if SANDD_TUNNEL_AUTHKEY is set while SANDD_TUNNEL_SERVER or SANDD_SERVER_URL are missing. With the current fail-open shim this can silently disable the access channel (or attempt to start it with empty values) while still launching workloads. Add a guard that logs the missing required vars (without logging the auth key) and disables SandD injection when incomplete.

	sanddCfg := provider.SanddConfig{
		AuthKey:       os.Getenv("SANDD_TUNNEL_AUTHKEY"),
		ControlServer: os.Getenv("SANDD_TUNNEL_SERVER"),
		ServerURL:     os.Getenv("SANDD_SERVER_URL"),
	}

[Copilot]

Pull request overview

Copilot reviewed 28 out of 29 changed files in this pull request and generated no new comments.

Suppressed comments (6)

pkg/provider/aws/translate.go:117

• sanddBinaryURL downloads from GitHub Releases "latest", which is non-deterministic and can change (or be compromised) independently of Nebula releases. That creates a supply-chain risk and can make provisioning behavior change without a code deploy. Prefer pinning to an explicit SandD version (and ideally verifying a checksum/signature) so the bootstrap is reproducible.

// sanddBinaryURL is the statically-linked (musl) SandD daemon release asset. Being
// static, this one binary runs in any container image regardless of its libc, so the
// shim can fetch it into an arbitrary user image at boot. It is pinned to the same
// asset name install.sh resolves; amd64 matches the x86_64 GPU instance types.
const sanddBinaryURL = "https://github.com/InftyAI/SandD/releases/latest/download/sandd-linux-amd64"

hack/deploy.sh:69

• The .env parser claims to strip a layer of matching surrounding quotes, but the current check only looks at the first character. If a value starts with a quote but doesn’t end with the same quote, this will still drop the first and last characters, corrupting the value (e.g. a missing closing quote silently truncates the last character).

    # strip one layer of matching surrounding quotes from the value
    if [[ "${local_val}" == \"*\" || "${local_val}" == \'*\' ]]; then
      local_val="${local_val:1:${#local_val}-2}"
    fi

config/default/kustomization.yaml:43

• This comment block is inconsistent with the manifests: (1) it refers to a "nebula-sandd-config Secret", but the wiring uses a ConfigMap (config/manager/manager.yaml mounts it via configMapRef), and (2) ../sandd does not deploy the SandD controller (it’s a hand-applied sample in config/samples/sandd-controller.yaml). This can mislead operators into disabling ../sandd and breaking the manager due to optional:false.

# (kubectl exec does NOT work against a Nebula virtual node). This only stands up
# the in-cluster pieces; the manager starts injecting the daemon once the
# nebula-sandd-config Secret exists (still opt-in per cluster). To skip deploying
# these components, comment this line out. See config/sandd/README.md.
- ../sandd

pkg/provider/aws/client.go:214

• This docstring says the SandD auth key is accepted here and stamped into user-data, but provider.SanddConfig documents AuthKey as not operator-configured and the AWS provider now mints per-instance keys via KeyMinter during Provision (resolveSanddConfig). The comment should reflect the per-instance minting/resolution to avoid implying a static operator-provided key is the intended flow.

// the bootstrap untouched, so passing provider.SanddConfig{} is a no-op. When set,
// every workload this provider launches runs the SandD daemon inside its container
// in tunnel mode (see buildUserData), the workload's command-execution/shell
// channel. Unlike credentials, the auth key IS accepted here — it is delivered to
// the controller as a secret and stamped into the (base64) user-data.

cmd/main.go:369

• SandD key minting is enabled whenever SANDD_KEYBROKER_URL is set, even if SANDD_TUNNEL_SERVER or SANDD_SERVER_URL are empty. That produces a configuration that can mint keys but cannot successfully start/attach the daemon, and still logs "enabled". Consider gating enablement on the required URLs being non-empty and leaving SandD disabled (nil KeyMinter) when they’re missing.

	if minter := sandd.NewBrokerClient(os.Getenv("SANDD_KEYBROKER_URL")); minter != nil {
		sanddCfg.KeyMinter = minter
		setupLog.Info("SandD per-daemon key minting enabled via key broker")
	}

pkg/sandd/keybroker.go:46

• NewBrokerClient’s comment says an empty broker URL means callers can "fall back to the static key", but the rest of the SandD wiring (and config/sandd/README.md) positions the broker as the source of truth for minting and SandD as opt-in via the broker URL. Suggest rewording this to avoid implying a static-key path is expected/endorsed.

// NewBrokerClient builds a client for the broker at baseURL. A zero/empty baseURL
// yields a nil client so callers can treat "no broker configured" as "no dynamic
// minting" (fall back to the static key) without a separate flag.

[Copilot]

Pull request overview

Copilot reviewed 28 out of 29 changed files in this pull request and generated 1 comment.

Suppressed comments (4)

cmd/main.go:369

• SandD is enabled as soon as SANDD_KEYBROKER_URL is set, even if SANDD_TUNNEL_SERVER or SANDD_SERVER_URL are missing. That can result in provisioning workloads with a minted key but empty endpoints (daemon can’t join the mesh) and makes misconfiguration harder to diagnose. Consider only enabling the minter when the required endpoint env vars are present (otherwise log and leave SandD disabled).

	if minter := sandd.NewBrokerClient(os.Getenv("SANDD_KEYBROKER_URL")); minter != nil {
		sanddCfg.KeyMinter = minter
		setupLog.Info("SandD per-daemon key minting enabled via key broker")
	}

hack/deploy.sh:69

• The .env parser attempts to strip a layer of surrounding quotes, but it only checks the first character and then blindly drops the last character too. If the value starts with a quote but doesn’t end with the same quote (or is a 1-character string), the parser will silently truncate the value.

    # strip one layer of matching surrounding quotes from the value
    if [[ "${local_val}" == \"*\" || "${local_val}" == \'*\' ]]; then
      local_val="${local_val:1:${#local_val}-2}"
    fi

config/default/kustomization.yaml:43

• The SandD block comment is inconsistent with the actual wiring: the manager consumes a required ConfigMap (nebula-sandd-config), not a Secret, and commenting out - ../sandd will prevent the manager from starting unless that ConfigMap is still applied elsewhere (envFrom optional=false in config/manager/manager.yaml). The comment should reflect this to avoid broken installs.

# [SANDD] SandD access channel — deployed by default. Stands up headscale + the
# SandD controller so operators/agents can exec/shell into a workload container
# (kubectl exec does NOT work against a Nebula virtual node). This only stands up
# the in-cluster pieces; the manager starts injecting the daemon once the
# nebula-sandd-config Secret exists (still opt-in per cluster). To skip deploying
# these components, comment this line out. See config/sandd/README.md.
- ../sandd

config/sandd/manager-config.yaml:24

• This ConfigMap sets SANDD_KEYBROKER_URL to a non-empty value by default, which turns SandD injection on immediately (per cmd/main.go) even if SANDD_TUNNEL_SERVER is still the SANDD_TUNNEL_SERVER placeholder. That leads to partially-enabled SandD (keys minted, but daemons can’t successfully join). Consider leaving SANDD_KEYBROKER_URL blank by default (true opt-in), or otherwise gating injection until the tunnel server placeholder has been replaced.

  # In-cluster key broker (the switch that turns injection ON): the manager mints a
  # fresh single-use, ephemeral key per workload from here. Stable internal Service
  # DNS — no per-cluster edit needed.
  SANDD_KEYBROKER_URL: "http://nebula-keybroker.nebula-system:8090"

[Copilot]

Pull request overview

Copilot reviewed 30 out of 31 changed files in this pull request and generated no new comments.

Suppressed comments (5)

hack/deploy.sh:69

• The .env parser strips the first and last character whenever the value starts with a quote, without verifying it also ends with the same quote. This can silently corrupt values (e.g. a value that begins with a quote but doesn't end with one) and makes debugging credential issues harder.

    # strip one layer of matching surrounding quotes from the value
    if [[ "${local_val}" == \"*\" || "${local_val}" == \'*\' ]]; then
      local_val="${local_val:1:${#local_val}-2}"
    fi

pkg/provider/aws/translate.go:131

• The host-fetch comment claims this works on "distroless" images, but SandD injection explicitly requires /bin/sh in the workload container (ENTRYPOINT is overridden to /bin/sh). Most distroless images do not include a shell, so this statement is misleading.

// sanddHostDir is where the host fetches the sandd + tailscale binaries and where
// they are bind-mounted (read-only) into the workload container. Fetching on the
// HOST — the AL2 GPU AMI, which has curl+tar — instead of inside the container means
// the user's image needs no fetcher or package manager (works on distroless too),
// and the download happens once per instance rather than per container start.

cmd/keybroker/main.go:101

• policyFor(kindController) documents a long (720h) expiration for controller keys, but the implementation currently returns expiration "1h". This mismatch is easy to miss and changes the key policy from what the surrounding comments describe.

		// Reusable so it can re-register across restarts; ephemeral so the old node
		// is reaped on disconnect, freeing the stable MagicDNS name for the fresh pod
		// to reclaim (the controller has no PVC, so nothing to preserve). Long TTL
		// (720h) just bounds a key that outlives a brief reap gap.
		return keyPolicy{reusable: true, ephemeral: true, expiration: "1h"}, true

config/default/kustomization.yaml:42

• This SandD note says ../sandd "stands up headscale + the SandD controller", but the overlay only includes headscale/keybroker + the manager ConfigMap; the controller is shipped as a hand-applied sample. Also, because the manager mounts nebula-sandd-config with optional:false, commenting out ../sandd will require keeping that ConfigMap applied (or making the envFrom optional) to avoid the manager failing to start.

# [SANDD] SandD access channel — deployed by default. Stands up headscale + the
# SandD controller so operators/agents can exec/shell into a workload container
# (kubectl exec does NOT work against a Nebula virtual node). This only stands up
# the in-cluster pieces; the manager starts injecting the daemon once the
# nebula-sandd-config Secret exists (still opt-in per cluster). To skip deploying
# these components, comment this line out. See config/sandd/README.md.

config/sandd/README.md:17

• This README states "There is no static-key path", but the code in this PR still supports a static auth key path (provider.SanddConfig.AuthKey and AWS NewSDKClient docs explicitly say AuthKey can be provided). If the intention is to forbid static keys for this integration, the code/docs should be aligned; otherwise this line should be softened to avoid misleading operators.

**Minting keys** — the broker is the only component with headscale admin authority;
it reaches headscale over a local unix socket. There is no static-key path.

[Copilot]

Pull request overview

Copilot reviewed 30 out of 31 changed files in this pull request and generated no new comments.

Suppressed comments (10)

pkg/provider/aws/translate.go:118

• sanddBinaryURL uses the GitHub releases/latest redirect. That makes node bootstrap non-reproducible (a new SandD release changes behavior without any Nebula code/config change) and increases supply-chain risk for production clusters. Prefer pinning to an explicit version (and ideally verifying a checksum/signature) so rollouts are controlled.

// sanddBinaryURL is the statically-linked (musl) SandD daemon release asset. Being
// static, this one binary runs in any container image regardless of its libc, so the
// shim can fetch it into an arbitrary user image at boot. It is pinned to the same
// asset name install.sh resolves; amd64 matches the x86_64 GPU instance types.
const sanddBinaryURL = "https://github.com/InftyAI/SandD/releases/latest/download/sandd-linux-amd64"

cmd/keybroker/main.go:101

• policyFor(kindController) comment says the controller key should have a long TTL (720h), but the code currently returns expiration "1h". That mismatch likely causes controller keys to expire much sooner than intended and makes the behavior diverge from the documented lifecycle.

		return keyPolicy{reusable: true, ephemeral: true, expiration: "1h"}, true

cmd/keybroker/main_test.go:120

• TestKeysHandler_Controller validates reusable/ephemeral but doesn’t assert the controller key expiration. Since the broker owns policy (and the code comment explicitly calls out a long TTL), the test should pin the expected expiration to prevent silent regressions.

	if !seen.reusable || !seen.ephemeral {
		t.Errorf("controller policy = %+v, want reusable + ephemeral", *seen)
	}

hack/deploy.sh:69

• The .env parser claims to strip matching surrounding quotes, but it currently strips the first and last character whenever the value starts with a quote—even if it doesn’t end with the same quote (or is too short). That can corrupt values like AWS_SECRET_ACCESS_KEY that begin with a quote character or have unbalanced quoting.

    # strip one layer of matching surrounding quotes from the value
    if [[ "${local_val}" == \"*\" || "${local_val}" == \'*\' ]]; then
      local_val="${local_val:1:${#local_val}-2}"
    fi

pkg/provider/aws/translate.go:209

• writeSanddEntrypoint’s comment says the auth key is visible to the workload because it’s delivered as container env, but sanddShimScript unsets SANDD_TUNNEL_AUTHKEY/SANDD_TUNNEL_SERVER before exec "$@" (so the workload won’t see them in its steady-state environment). The comment should reflect the actual (more secure) behavior.

// The auth key is delivered as container env, so it is visible to the workload
// itself (the tenant owns this container). That is inherent to running the daemon
// INSIDE the container — the container must hold the credential to dial home — and is
// acceptable only because the daemon is single-tenant (one per container).

Makefile:221

• The Makefile’s sed substitution injects SANDD_TUNNEL_SERVER into manifests without escaping sed replacement metacharacters. If the URL contains & or \, sed will treat them specially and the rendered manifests can be corrupted. Escaping these characters makes make deploy more robust.

		$(if $(SANDD_TUNNEL_SERVER),sed 's|__SANDD_TUNNEL_SERVER__|$(SANDD_TUNNEL_SERVER)|g',cat) | \

config/default/kustomization.yaml:41

• This block says the ../sandd overlay "stands up headscale + the SandD controller" and that injection begins once a "nebula-sandd-config Secret" exists. In this PR, the overlay deploys headscale + manager SandD ConfigMap (and the keybroker sidecar), while the controller is a hand-applied sample; and the manager wiring uses a ConfigMap, with injection controlled by whether SANDD_KEYBROKER_URL is set/blank.

# [SANDD] SandD access channel — deployed by default. Stands up headscale + the
# SandD controller so operators/agents can exec/shell into a workload container
# (kubectl exec does NOT work against a Nebula virtual node). This only stands up
# the in-cluster pieces; the manager starts injecting the daemon once the
# nebula-sandd-config Secret exists (still opt-in per cluster). To skip deploying

pkg/provider/aws/translate.go:192

• sanddShimScript unsets SERVER_URL and DAEMON_ID before exec’ing the workload. Those names are very generic and may be intentionally set by user containers; unsetting them can break workloads. More generally, writeSanddEntrypoint injects SERVER_URL/DAEMON_ID into the container, which risks collisions with user env even if you stop unsetting them. Consider injecting SandD-prefixed env vars (e.g. SANDD_SERVER_URL/SANDD_DAEMON_ID) and passing them to sandd via per-command env (SERVER_URL=... DAEMON_ID=... sandd ...) so the workload env is not overwritten/cleared.

unset SANDD_TUNNEL_AUTHKEY SANDD_TUNNEL_SERVER SERVER_URL DAEMON_ID
exec "$@"

config/sandd/kustomization.yaml:1

• This header says the SandD overlay is "OPT-IN, disabled by default", but config/default/kustomization.yaml includes - ../sandd, and manager-config.yaml sets SANDD_KEYBROKER_URL by default. As a result, SandD infrastructure (and likely injection) is enabled in a default deploy unless the resource is commented out or the ConfigMap is edited. The comment should match the actual default behavior.

# [SANDD] SandD access channel — OPT-IN, disabled by default.

config/sandd/kustomization.yaml:25

• This comment says the manager’s envFrom is "optional" for the SandD ConfigMap, but config/manager/manager.yaml mounts it with optional: false (manager won’t start without it). Please update the comment to avoid implying a deploy will work without the ConfigMap present.

# This overlay flips the manager ON: it ships nebula-sandd-config (manager-config.yaml),
# which the manager's optional envFrom reads to enable injection. So deploying this
# overlay + a controller is all it takes — but SANDD_TUNNEL_SERVER in that ConfigMap

[kerthcet]

/lgtm
/kind feature

[InftyAI-Agent]

Approved: PR has both lgtm and approved labels

[InftyAI-Agent]

Approved: PR has both lgtm and approved labels