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.