Fix: Fold bundle service and token broker to operator image

[davidhadas]

Summary

Done:

1. ✅ bundle-service folded into the operator image
2. ✅ token-broker folded — module merged into github.com/rossoctl/operator (23 files, history preserved via git mv)
3. ✅ Explicit command: in both Deployments (ENTRYPOINT stays /manager)
4. ✅ Both templated into the chart behind bundleService.enabled / tokenBroker.enabled, default false

Remaining:

• Item 5 (platform chart toggles) — cross-repo, rossoctl/rossoctl
• Item 6 (E2E smoke test) — deferred; verified manually on kind, no CI coverage
• Item 7 (revise rossoctl#2139) — cross-repo, now obsolete instructions

Limitation until item 5 lands: the components are installable from the operator chart but not reachable through the rossoctl platform installer. Testers must install the operator chart directly with the two --set flags.

Two notes on the work as landed:

• The chart reads .Values.controllerManager.container.image.* rather than carrying its own tag, because release.yml:142 rewrites only the manager's tag — a second PLACEHOLDER would ship unsubstituted. One tag to pin, and both binaries always come from the same commit.
• The bundle-service NetworkPolicy is gated on bundleService.enabled alone, not networkPolicy.enable (which defaults false). The server wires identity.NoopVerifier on plain HTTP and will serve any bundle named in ?spiffe=, so that policy's ingress restriction is the only access control. It still needs a CNI that enforces NetworkPolicy — kind's default does not.

Also removed as cleanup: operator/config/bundleservice/ (orphaned kustomize manifests), operator/cmd/bundle-service/Dockerfile and hack/bundle-service-kind.sh (a standalone image path that no longer ships). hack/kind-reload-all.sh is now the single dev script and renders everything from the chart, so the dev path exercises the same manifests and RBAC as a real install.

Related issue(s)

Fixes #536

[mrsabath]

Summary

Good consolidation, with unusually careful documentation — the "why" comments on image inheritance, the replicas: 1 constraint, and the bundle-service NetworkPolicy posture all explain reasoning a reader would otherwise have to reconstruct from scratch.

Verified while reviewing:

• The authlib requirement is now a pinned pseudo-version (v0.0.0-20260915135208-2d373605612d), which is the actual fix for the #537 class of problem.
• The CRD ships in charts/operator/crds/, which Helm installs ahead of templates/, so the templated default-policy CR orders correctly on a fresh install.
• .env is gitignored at repo root (.env, .env.*, .env.local), so the dev script's .env convention does not risk committing credentials.
• Gating the new ServiceAccounts on rbac.enable matches the existing manager rbac/service_account.yaml exactly — consistent with the chart, not a new behaviour.
• The repeated --show-only flags in the dev script are fine: Helm types that flag as stringArray, so the values accumulate rather than the last one winning.

The security note in the PR description is doing the right thing by stating plainly that the NetworkPolicy is the sole access control and that kind's default CNI does not enforce it. That is the kind of limitation that causes real incidents when it is left implicit.

No must-fix issues. Four non-blocking comments inline; the CI-coverage gap (comment 1) is the one I would most want addressed before the next release cut, given that #537 already shipped once.

Body-only note: operator/hack/kind-reload-all.sh:119

The export $(grep -v '^#' "${ROOT_DIR}/.env" | grep -v '^\s*$' | xargs) line is unchanged context in this PR, so GitHub will not accept an inline comment anchored to it — the word-splitting detail in comment 2 is really about that line. Worth noting that it survived this refactor for the same reason it is not commentable: nothing touched it.

---

Areas reviewed: Helm/K8s templates, Dockerfile, CI workflow, Shell, Go module/deps, docs
Commits: 1 commit, signed off
CI status: passing (17/17, including E2E)

[mrsabath]

Removing test-token-broker is correct — the module is gone, and the job's working-directory: token-broker would fail outright. But it is worth checking what replaced its coverage, because the deleted job's own comment states it existed so that #537 "cannot recur silently."

After this PR:

• make test and vet run go test ./... / go vet ./..., so the merged internal/tokenbroker/... and pkg/oauth packages are compiled and tested. An unresolvable dependency — exactly token-broker module fails to build: authlib pinned to pre-rename commit with old module path #537 — is still caught. Good.
• make build, however, is go build -o bin/manager cmd/main.go (Makefile:146). It does not build ./cmd/bundle-service/ or ./cmd/token-broker/.

So the two new main packages are compiled in CI only as a side effect of the Docker image build. A build break confined to one of those two main.go files would pass Lint, Unit Tests and Build, and surface in the image build — recoverable, but a noisier signal than a failing build job.

Cheapest fix is to extend the build target to match the Dockerfile:

build: manifests generate fmt vet ## Build manager, bundle-service and token-broker binaries.
	go build -o bin/manager cmd/main.go
	go build -o bin/bundle-service ./cmd/bundle-service/
	go build -o bin/token-broker ./cmd/token-broker/

That also keeps make build honest about what the image now ships, which is the same single-source-of-truth argument the chart rendering in kind-reload-all.sh makes.

[davidhadas]

Confirmed and fixed. Makefile:146 was go build -o bin/manager cmd/main.go, and neither new main package appeared anywhere in the Makefile — so a break confined to either would pass Lint, Unit Tests and Build and only surface in the image build. That's the silent-failure mode the deleted test-token-broker job existed to prevent.

[davidhadas]

build now compiles all three, matching the Dockerfile. Verified: produces bin/manager, bin/bundle-service, bin/token-broker.

[mrsabath]

Turning the missing-credentials case from exit 1 into else is the right call now that this script deploys all three components — a dev without OAuth creds should still get the manager and bundle-service.

One robustness note on the .env loading this now guards (line 119, unchanged so not inline-commentable):

export $(grep -v '^#' "${ROOT_DIR}/.env" | grep -v '^\s*$' | xargs)

xargs word-splits on whitespace, so a value containing a space — plausible enough for a client secret, and near-certain if anyone adds a quoted value later — exports a truncated variable and passes the remaining words to export as separate names. With set -u active the failure is silent in the direction that matters: GITHUB_OAUTH_CLIENT_SECRET ends up set but wrong, the guard on this line passes, and the Secret is created with a partial credential. The broker then fails at OAuth time with an error that points at the provider rather than at the loader.

More robust, and shorter:

if [ -f "${ROOT_DIR}/.env" ]; then
  set -a
  # shellcheck disable=SC1091
  . "${ROOT_DIR}/.env"
  set +a
fi

That handles quoting and spaces the way a developer writing a .env would expect. Non-blocking, but this is now the single dev entry point, so its failure modes are worth more than they used to be.

[davidhadas]

Good catch.

[mrsabath]

The comment here is accurate and appropriately blunt — "REQUIRED for production, otherwise incoming JWTs are not verified" — but the default is still the unverified one, and it is reachable with nothing more than --set tokenBroker.enabled=true.

That combination (empty default + a comment explaining the danger) puts the whole weight of the invariant on whoever reads values.yaml before installing. Given the broker holds OAuth tokens for other principals, the blast radius of getting this wrong is larger than the usual "dev default" tradeoff.

Two options, either of which keeps the dev path working:

1. A NOTES.txt stanza that prints a loud warning when tokenBroker.enabled is true and jwt.jwksUrl is empty — zero friction, but visible at install time rather than only at values-authoring time.
2. A required-style guard keyed off an explicit opt-out, e.g. tokenBroker.jwt.insecureSkipVerify: true must be set to install with empty jwksUrl. Makes the unverified mode a deliberate statement in values rather than an omission.

I lean toward (2) for something brokering tokens, with (1) as the minimum. Flagging rather than blocking since the component defaults to enabled: false, so nothing is exposed by this PR as merged.

[davidhadas]

Agreed on the substance: an empty jwt.jwksUrl default plus a comment puts the whole invariant on whoever reads values.yaml, and for a component holding other principals' OAuth tokens that's thinner than it should be.

Not folding it into this PR. Choosing between a NOTES.txt warning and an explicit insecureSkipVerify opt-out is an install-contract decision — option 2 would break anyone already running with an empty jwksUrl — so it deserves its own diff and discussion.

I suggest to leave it out of this release.

[mrsabath]

nit: the comment is exactly the right thing to write here —

Sessions and the token cache are held in memory, so this must stay at 1. Scaling out requires shared state first; it is deliberately not a value.

Since the constraint is real correctness rather than preference, consider making it enforced instead of documented, so a future --set tokenBroker.replicas=3 fails loudly rather than silently splitting sessions across pods:

{{- if .Values.tokenBroker.replicas }}
{{- fail "tokenBroker.replicas is not supported: sessions and the token cache are in-memory, so the broker must run a single replica. Scaling out requires shared session state first." }}
{{- end }}
replicas: 1

That way the reasoning in the comment is also the thing that stops the mistake. Purely optional — the current form is already clearer than most.

[davidhadas]

Taken. The constraint is correctness rather than preference, so the fail guard is a better home for the reasoning than a comment.

The guard keys off the value being set at all rather than its value, since 1 is the only legal setting — which
works because tokenBroker.replicas is deliberately absent from values.yaml. Verified: renders replicas: 1
normally, and --set tokenBroker.replicas=3 fails with the message rather than silently splitting sessions.