feat: support headscale - #21
Open
kerthcet wants to merge 5 commits into
Open
Conversation
This reverts commit 2cae49c.
Signed-off-by: kerthcet <kerthcet@gmail.com>
There was a problem hiding this comment.
Pull request overview
Adds an optional SandD access channel backed by headscale/Tailscale so Nebula-managed AWS workloads can be accessed (exec/shell) over an outbound-only mesh, including an in-cluster key-broker sidecar to mint per-workload pre-auth keys.
Changes:
- Introduces a headscale + keybroker deployment overlay (
config/sandd) plus deploy-time substitution for the public headscale endpoint. - Adds AWS-side SandD bootstrap support (host fetch + in-container entrypoint shim) and a manager-side broker client + provider seam for per-daemon key minting.
- Adds the
cmd/keybrokerservice and supporting tests/docs/manifests for end-to-end wiring.
Reviewed changes
Copilot reviewed 26 out of 28 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| THIRD_PARTY_NOTICES.md | Adds third-party notice content for SandD channel dependencies. |
| README.md | Updates project messaging, adds architecture image + community section, links third-party notices. |
| pkg/sandd/keybroker.go | Manager-side HTTP client to request daemon keys from the in-cluster broker. |
| pkg/sandd/keybroker_test.go | Unit tests for broker client request/response behavior. |
| pkg/provider/provider.go | Adds provider-agnostic SanddConfig and DaemonKeyMinter seam. |
| pkg/provider/aws/translate.go | Extends EC2 user-data generation to optionally inject SandD via entrypoint shim + host-fetched binaries. |
| pkg/provider/aws/sandd_test.go | Adds provisioning tests ensuring per-workload minting and proper resolution onto instance spec. |
| pkg/provider/aws/client.go | Threads SandD config into AWS provider construction. |
| pkg/provider/aws/client_test.go | Adds/updates user-data tests for SandD enabled/disabled behavior and quoting. |
| pkg/provider/aws/aws.go | Resolves per-instance SandD config (mint key on Provision) and stamps onto instance spec. |
| Makefile | Adds keybroker image build/push targets and deploy-time token substitution for SANDD_TUNNEL_SERVER. |
| hack/deploy.sh | Plumbs SANDD_TUNNEL_SERVER from .env/env into make deploy and warns when unset. |
| Dockerfile.keybroker | Builds a keybroker image based on headscale/headscale to include the CLI. |
| config/sandd/README.md | Documents the SandD access channel architecture and deployment flow. |
| config/sandd/manager-config.yaml | Adds required manager ConfigMap for SandD endpoints and keybroker URL. |
| config/sandd/kustomization.yaml | New overlay for headscale + manager SandD config. |
| config/sandd/headscale.yaml | Headscale Deployment/ConfigMap/PVC plus keybroker sidecar + in-cluster Service. |
| config/samples/sandd-controller.yaml | Sample SandD controller Deployment that joins the mesh and drives daemons. |
| config/samples/headscale-service.yaml | Sample public LoadBalancer Service for headscale (applied before overlay). |
| config/samples/deployment.yaml | Updates sample GPU workload Deployment. |
| config/manager/manager.yaml | Projects SandD ConfigMap into manager env (required). |
| config/default/kustomization.yaml | Includes the SandD overlay and scopes patches to avoid impacting headscale Deployment. |
| cmd/main.go | Wires SandD env into AWS provider registration and enables broker minting when configured. |
| cmd/keybroker/main.go | Implements the keybroker HTTP service that shells out to headscale CLI to mint keys. |
| cmd/keybroker/main_test.go | Unit tests for keybroker policy selection, handler behavior, and retry logic. |
| .env.example | Documents SANDD_TUNNEL_SERVER as a per-cluster (non-secret) deploy-time value. |
Suppressed comments (3)
cmd/keybroker/main.go:105
- The controller key policy TTL is currently set to 128h, but the comments in this file and config/sandd/README.md describe a longer-lived controller key (e.g. 720h). Align the implementation with the documented intent so operators get the expected stability vs. exposure tradeoff.
return keyPolicy{reusable: true, ephemeral: true, expiration: "128h"}, true
pkg/provider/provider.go:345
- The DaemonKeyMinter doc says it mints a "single-use" key, but the broker mints reusable+ephemeral keys to allow re-auth after a mesh blip. Update the doc to match the implemented policy so callers/tests don’t encode the wrong expectation.
// DaemonKeyMinter mints a single-use, ephemeral headscale pre-auth key for one
pkg/provider/aws/translate.go:93
- The SandD shim path is selected whenever SanddConfig.Enabled() is true, but if the Pod relies on the image ENTRYPOINT (Command empty) the shim can’t faithfully relaunch the workload and can exit before starting it. Gate shim injection on an explicit Command to avoid breaking workloads when SandD is enabled cluster-wide.
if spec.Sandd.Enabled() {
runArgs = writeSanddEntrypoint(&b, spec)
} else {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Signed-off-by: kerthcet <kerthcet@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 27 changed files in this pull request and generated 1 comment.
Suppressed comments (13)
pkg/provider/aws/translate.go:165
- This bullet says the shim works on "distroless" images, but the implementation requires /bin/sh (and uses sh -c). Distroless images generally don’t ship a shell. Tighten the wording so users don’t assume this will work on true distroless images.
// so the user's image needs only /bin/sh (works on distroless/minimal images too).
config/default/kustomization.yaml:42
- This comment refers to a "nebula-sandd-config Secret", but the manifests mount a ConfigMap, and injection is controlled by whether SANDD_KEYBROKER_URL is set (not by the mere existence of the ConfigMap, which is required). Update this to reflect the actual configuration model and the fact that enabling SandD requires uncommenting the overlay line below.
# the in-cluster pieces; the manager reads SandD URLs from the nebula-sandd-config ConfigMap.
# To disable injection, blank SANDD_KEYBROKER_URL there; to skip deploying these components, comment this line out.
# See config/sandd/README.md.
config/default/kustomization.yaml:43
- Enabling ../sandd in the default install deploys the headscale/keybroker dev example (headscale.yaml is explicitly marked "DEVELOPMENT / EXAMPLE" and requests 4 CPU / 8Gi plus a PVC). That’s a substantial default footprint and can cause installs on smaller clusters to fail or consume unexpected resources. Consider leaving this overlay commented out by default and documenting SandD as an explicit opt-in.
- ../sandd
pkg/provider/provider.go:341
- SanddConfig.KeyMinter’s comment says each workload gets a "single-use" key, but the keybroker policy and sandd client code explicitly require REUSABLE keys so daemons/controllers can re-register after an ephemeral-node reap. Update this comment to avoid documenting the wrong key semantics.
// KeyMinter is what enables SandD: when non-nil it mints a FRESH per-instance
// mesh key at provision time (see DaemonKeyMinter), so each workload gets its own
// reusable, ephemeral credential (tenant isolation + auto-reaped nodes). It is
// a seam so the manager can inject an HTTP client to the key broker while this
// package stays transport-agnostic and unit-testable. Nil => SandD off.
pkg/provider/provider.go:349
- DaemonKeyMinter is documented as minting a "single-use" key, but the broker policies are REUSABLE (to allow re-registration after ephemeral-node reap during mesh blips). Adjust the interface comment so it matches the actual/required behavior.
// DaemonKeyMinter mints a single-use, ephemeral headscale pre-auth key for one
// workload daemon. The manager backs it with an HTTP call to the in-cluster key
// broker (which alone holds headscale admin authority); tests inject a fake. It is
// called on the Provision path, so it MUST honour ctx (the Provision deadline) and
// return promptly. The returned key is a secret and must never be logged.
cmd/keybroker/main.go:105
- policyFor(kindController) returns expiration "128h", but the surrounding comments/docs describe a 720h controller TTL. This mismatch will either shorten controller keys unexpectedly or mislead operators. Align the code and comments (and config/sandd/README.md) to the intended TTL.
case kindController:
// 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
// (128h) just bounds a key that outlives a brief reap gap.
return keyPolicy{reusable: true, ephemeral: true, expiration: "128h"}, true
pkg/provider/aws/aws.go:727
- This comment says the per-workload key is "single-use", but the keybroker policy is explicitly REUSABLE (to allow daemons to re-register after ephemeral-node reap during transient mesh blips). Update wording here to match the actual key policy.
// SandD daemon (opt-in): resolve THIS instance's mesh key by minting a fresh
// reusable, ephemeral key per workload (tenant isolation + auto-reaped nodes).
// This runs on the Provision path, so a mint failure aborts provisioning rather
// than launching a daemon that can never join the mesh.
cmd/main.go:360
- The AWS registration comment says SandD mints a "single-use" key per workload, but the keybroker policies are REUSABLE + ephemeral. This distinction is important for reliability (re-registering after ephemeral-node reap). Update the comment to match the actual behavior.
// its container in tunnel mode — commands and interactive shells run in the user's
// own environment over the mesh with no inbound access — and the adapter mints a
// FRESH reusable, ephemeral key per workload (tenant isolation + auto-reaped
// off by default. Minted keys are secrets and are NEVER logged.
config/sandd/manager-config.yaml:23
- The comment describes the broker-minted key as "single-use", but the keybroker’s daemon policy is REUSABLE + ephemeral (so a daemon can re-register after an ephemeral-node reap during transient mesh blips). Update this to avoid documenting the wrong key lifecycle.
# In-cluster key broker (the switch that turns injection ON): the manager mints a
# fresh reusable, ephemeral key per workload from here. Stable internal Service
# DNS — no per-cluster edit needed.
pkg/provider/aws/translate.go:131
- These comments claim the host-fetch approach "works on distroless" images, but the injection explicitly sets the container ENTRYPOINT to /bin/sh (see sanddShimScript and writeSanddEntrypoint). Distroless images typically do not include a shell, so this would fail. Update the comment to match the actual requirement (image must include /bin/sh, or use a distroless-with-shell variant).
This issue also appears on line 165 of the same file.
// 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.
config/default/kustomization.yaml:39
- This comment says the ../sandd overlay is "deployed by default" and includes the SandD controller, but the overlay only includes headscale + manager-config; the controller is a hand-applied sample (config/samples/sandd-controller.yaml). Also, headscale.yaml is explicitly marked a development/example config, so the wording should reflect that SandD is opt-in.
This issue also appears in the following locations of the same file:
- line 40
- line 43
# [SANDD] SandD access channel — deployed by default. Stands up headscale (with the keybroker sidecar)
# plus the manager SandD ConfigMap; the user-facing SandD controller is a hand-applied sample
# (config/samples/sandd-controller.yaml).
pkg/provider/provider.go:328
- The AuthKey field comment says it is "NOT operator-configured", but the AWS provider path will inject SandD whenever SanddConfig.Enabled() is true (AuthKey != "" || KeyMinter != nil). That means a non-empty AuthKey does enable a static-key configuration. Either enforce broker-only operation or soften this comment so it matches actual behavior.
// AuthKey is the Tailscale/headscale pre-auth key the daemon joins the mesh
// with (sandd --tunnel-authkey). It is NOT operator-configured: the adapter
// fills it in per instance from the key minted by KeyMinter (see
// resolveSanddConfig), so the minted key — not the seam — is what the launch
// bootstrap bakes in. It is a secret and is never logged.
config/sandd/README.md:17
- This states "There is no static-key path", but the provider seam supports static AuthKey injection (SanddConfig.AuthKey) if configured. If the intent is "this deployment uses broker-minted keys" (not "the codebase cannot use static keys"), clarify the wording to avoid overpromising.
**Minting keys** — the broker is the only component with headscale admin authority;
it reaches headscale over a local unix socket. Static keys are supported but discouraged; prefer broker-minted keys.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 27 changed files in this pull request and generated 3 comments.
Suppressed comments (5)
pkg/provider/provider.go:352
- The interface comment says “single-use” keys, but the broker/client logic and surrounding docs describe daemon keys as reusable + ephemeral (to allow re-registration after an ephemeral reap). Update this comment to match the actual policy to avoid misleading implementers and reviewers.
// DaemonKeyMinter mints a single-use, ephemeral headscale pre-auth key for one
// workload daemon. The manager backs it with an HTTP call to the in-cluster key
// broker (which alone holds headscale admin authority); tests inject a fake. It is
// called on the Provision path, so it MUST honour ctx (the Provision deadline) and
// return promptly. The returned key is a secret and must never be logged.
type DaemonKeyMinter interface {
MintDaemonKey(ctx context.Context) (string, error)
}
cmd/keybroker/main.go:105
- There’s inconsistency across the PR about the controller key TTL (e.g., other docs mention 720h). Please align the controller expiration value and its documentation across
policyForand the SandD docs, or explicitly document why it differs. Also consider making the TTL values constants to reduce drift.
case kindController:
// 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
// (128h) just bounds a key that outlives a brief reap gap.
return keyPolicy{reusable: true, ephemeral: true, expiration: "128h"}, true
cmd/keybroker/main_test.go:43
TestPolicyForasserts reusable/ephemeral and only checks thatexpirationis non-empty, so changes to TTLs (including accidental regressions) won’t be caught. Add an explicit expectedexpirationper case (e.g.,"24h","128h"/"720h"as intended) and assert it matches to keep policy + docs consistent.
func TestPolicyFor(t *testing.T) {
cases := []struct {
kind keyKind
ok bool
reusable bool
ephemeral bool
}{
// Daemon: reusable + ephemeral — a per-workload credential that auto-reaps on
// disconnect, but reusable so a reaped daemon can re-register after a blip.
{kindDaemon, true, true, true},
// Controller: reusable (survives restarts) + ephemeral (same as a daemon) so
// the old node is reaped on disconnect, freeing its MagicDNS name to reclaim.
{kindController, true, true, true},
{keyKind("bogus"), false, false, false},
{keyKind(""), false, false, false},
}
THIRD_PARTY_NOTICES.md:20
- The
## Headscaleheading and following lines are indented by one leading space, which will render as a code block in Markdown rather than a section heading/paragraph. Remove the leading spaces so the heading renders correctly.
## Headscale
The optional SandD access channel deploys [headscale](https://github.com/juanfont/headscale)
(container image `headscale/headscale`). headscale is licensed separately; see its
upstream license at https://github.com/juanfont/headscale/blob/main/LICENSE.
config/sandd/kustomization.yaml:1
- This comment says “disabled by default”, but
config/default/kustomization.yamlnow includes- ../sanddby default, which makes the overlay applied in the default deployment. Please reconcile this wording with the actual default behavior (either adjust the comment here, or change the default kustomization to keep the overlay truly opt-in).
# [SANDD] SandD access channel — OPT-IN, disabled by default.
Comment on lines
+59
to
+65
| // SandD (opt-in): fetch the daemon + Tailscale binaries on the HOST now, before | ||
| // the container starts. They are bind-mounted read-only into the container by | ||
| // writeSanddEntrypoint, so the user's image needs no fetcher. Fail-open: a failed | ||
| // download just leaves the mount empty and the shim skips the daemon. | ||
| if spec.Sandd.Enabled() && len(spec.Command) > 0 { | ||
| fmt.Fprintf(&b, sanddHostFetchScript, sanddHostDir, sanddBinaryURL, tailscaleTarballURL) | ||
| } |
Comment on lines
+91
to
+93
| if spec.Sandd.Enabled() { | ||
| runArgs = writeSanddEntrypoint(&b, spec) | ||
| } else { |
Comment on lines
+355
to
+368
| // SandD daemon (opt-in): SandD turns on when SANDD_KEYBROKER_URL points at the | ||
| // in-cluster key broker. When set, every AWS workload runs the SandD daemon inside | ||
| // its container in tunnel mode — commands and interactive shells run in the user's | ||
| // own environment over the mesh with no inbound access — and the adapter mints a | ||
| // FRESH reusable, ephemeral key per workload (tenant isolation + auto-reaped | ||
| // off by default. Minted keys are secrets and are NEVER logged. | ||
| sanddCfg := provider.SanddConfig{ | ||
| ControlServer: os.Getenv("SANDD_TUNNEL_SERVER"), | ||
| ServerURL: os.Getenv("SANDD_SERVER_URL"), | ||
| } | ||
| 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") | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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?