From 22262843f9cf3e20d334f6b6999e1a8dfc129248 Mon Sep 17 00:00:00 2001 From: Collin Olander Date: Tue, 18 Aug 2026 10:21:43 -0500 Subject: [PATCH 1/7] feat: Watchtower charts --- .github/workflows/internal-chart-publish.yaml | 9 +- .github/workflows/release.yaml | 16 +- Dockerfile | 11 + Makefile | 10 +- README.md | 1 + deploy/watchtower/Chart.yaml | 13 + deploy/watchtower/templates/NOTES.txt | 26 ++ deploy/watchtower/templates/_helpers.tpl | 94 +++++++ deploy/watchtower/templates/deployment.yaml | 134 +++++++++ deploy/watchtower/templates/role.yaml | 31 +++ deploy/watchtower/templates/secret.yaml | 28 ++ deploy/watchtower/templates/service.yaml | 26 ++ .../watchtower/templates/serviceaccount.yaml | 16 ++ deploy/watchtower/values.yaml | 145 ++++++++++ docs/watchtower-deployment.md | 169 ++++++++++++ docs/watchtower.md | 260 ++++++++++++++++++ 16 files changed, 984 insertions(+), 5 deletions(-) create mode 100644 deploy/watchtower/Chart.yaml create mode 100644 deploy/watchtower/templates/NOTES.txt create mode 100644 deploy/watchtower/templates/_helpers.tpl create mode 100644 deploy/watchtower/templates/deployment.yaml create mode 100644 deploy/watchtower/templates/role.yaml create mode 100644 deploy/watchtower/templates/secret.yaml create mode 100644 deploy/watchtower/templates/service.yaml create mode 100644 deploy/watchtower/templates/serviceaccount.yaml create mode 100644 deploy/watchtower/values.yaml create mode 100644 docs/watchtower-deployment.md create mode 100644 docs/watchtower.md diff --git a/.github/workflows/internal-chart-publish.yaml b/.github/workflows/internal-chart-publish.yaml index 0962199d..160e9990 100644 --- a/.github/workflows/internal-chart-publish.yaml +++ b/.github/workflows/internal-chart-publish.yaml @@ -29,6 +29,11 @@ jobs: echo "Internal chart publishing requires 2.x.y-dev.; got ${version}" >&2 exit 1 fi + wt_version="$(awk '$1 == "version:" { print $2; exit }' deploy/watchtower/Chart.yaml | tr -d '\"')" + if [[ "${wt_version}" != "${version}" ]]; then + echo "deploy/watchtower must be versioned with the operator; got ${wt_version} vs ${version}" >&2 + exit 1 + fi echo "version=${version}" >> "${GITHUB_OUTPUT}" - name: Install Helm @@ -81,7 +86,7 @@ jobs: exit 1 fi - - name: Package and publish development chart + - name: Package and publish development charts env: VERSION: ${{ steps.chart.outputs.version }} run: | @@ -90,3 +95,5 @@ jobs: mkdir -p dist helm package deploy/operator --destination dist helm push "dist/operator-${VERSION}.tgz" "oci://${CHART_REPOSITORY}" + helm package deploy/watchtower --destination dist + helm push "dist/watchtower-${VERSION}.tgz" "oci://${CHART_REPOSITORY}" diff --git a/.github/workflows/release.yaml b/.github/workflows/release.yaml index f8178ae2..b0d7b906 100644 --- a/.github/workflows/release.yaml +++ b/.github/workflows/release.yaml @@ -74,11 +74,17 @@ jobs: in_operator && $0 == " image:" { in_image = 1; next } in_image && $1 == "tag:" { print $2; exit } ' deploy/operator/values.yaml | tr -d '\"')" + # The watchtower chart ships separately but deploys the operator image, + # so its version doubles as that image's tag and must track the release. + wt_chart_version="$(awk '$1 == "version:" { print $2; exit }' deploy/watchtower/Chart.yaml | tr -d '\"')" + wt_app_version="$(awk '$1 == "appVersion:" { print $2; exit }' deploy/watchtower/Chart.yaml | tr -d '\"')" - for value in "${chart_version}" "${app_version}" "${image_tag}"; do + for value in "${chart_version}" "${app_version}" "${image_tag}" \ + "${wt_chart_version}" "${wt_app_version}"; do if [[ "${value}" != "${version}" ]]; then - echo "Chart version, appVersion, and operator image tag must all equal ${version}" >&2 + echo "Operator and watchtower chart versions, appVersions, and the operator image tag must all equal ${version}" >&2 echo "Found chart=${chart_version}, appVersion=${app_version}, image=${image_tag}" >&2 + echo " watchtower chart=${wt_chart_version}, appVersion=${wt_app_version}" >&2 exit 1 fi done @@ -153,7 +159,7 @@ jobs: - name: Lint charts run: ct lint --all --config deploy/ct.yaml - - name: Package and publish chart + - name: Package and publish charts env: VERSION: ${{ steps.release.outputs.version }} run: | @@ -162,6 +168,10 @@ jobs: mkdir -p dist helm package deploy/operator --destination dist helm push "dist/operator-${VERSION}.tgz" "oci://${CHART_REPOSITORY}" + # Published as its own chart, not an operator dependency: Watchtower is a + # separate release with a separate lifecycle. + helm package deploy/watchtower --destination dist + helm push "dist/watchtower-${VERSION}.tgz" "oci://${CHART_REPOSITORY}" - name: Create GitHub release env: diff --git a/Dockerfile b/Dockerfile index 554e717b..75cfc407 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,3 +1,10 @@ +# Watchtower ships in this image as a second entrypoint: the deploy/watchtower +# chart runs the same image with `command: ["/watchtower"]`. Its binary embeds a +# Next.js static export, so it cannot be rebuilt from Go source here — lift the +# binary out of the published Watchtower image instead. +ARG WATCHTOWER_IMAGE=us-docker.pkg.dev/wandb-production/public/wandb/watchtower +ARG WATCHTOWER_VERSION=0.11.0 + # Build the manager binary FROM golang:1.26 AS manager-builder @@ -26,11 +33,15 @@ COPY internal/ internal/ RUN CGO_ENABLED=0 GOOS=${TARGETOS:-linux} GOARCH=${TARGETARCH} go build -a -o manager ./cmd/manager RUN CGO_ENABLED=0 GOOS=${TARGETOS:-linux} GOARCH=${TARGETARCH} go build -a -o crd-installer ./cmd/crd-installer +FROM ${WATCHTOWER_IMAGE}:${WATCHTOWER_VERSION} AS watchtower + FROM registry.access.redhat.com/ubi9/ubi-minimal WORKDIR / COPY --from=manager-builder /workspace/manager . COPY --from=manager-builder /workspace/crd-installer . +# Built CGO-free on golang:alpine, so it runs unmodified on this glibc base. +COPY --from=watchtower /watchtower . RUN mkdir -p /helm/.cache/helm /helm/.config/helm /helm/.local/share/helm && \ chown -R 65532:65532 /helm diff --git a/Makefile b/Makefile index b66628d1..72759a4f 100644 --- a/Makefile +++ b/Makefile @@ -1,6 +1,11 @@ # Image URL to use all building/pushing image targets IMG ?= controller:latest +# Watchtower release whose binary is copied into the operator image as its second +# entrypoint. Must be a tag that exists in WATCHTOWER_IMAGE — the build pulls it. +WATCHTOWER_IMAGE ?= us-docker.pkg.dev/wandb-production/public/wandb/watchtower +WATCHTOWER_VERSION ?= 0.11.0 + # Get the currently used golang install path (in GOPATH/bin, unless GOBIN is set) ifeq (,$(shell go env GOBIN)) GOBIN=$(shell go env GOPATH)/bin @@ -210,7 +215,10 @@ run: manifests generate fmt vet ## Run the manager from your host. # More info: https://docs.docker.com/develop/develop-images/build_enhancements/ .PHONY: docker-build docker-build: ## Build controller docker image. - $(CONTAINER_TOOL) build --platform linux/amd64 -t ${IMG} -f Dockerfile . + $(CONTAINER_TOOL) build --platform linux/amd64 \ + --build-arg WATCHTOWER_IMAGE=$(WATCHTOWER_IMAGE) \ + --build-arg WATCHTOWER_VERSION=$(WATCHTOWER_VERSION) \ + -t ${IMG} -f Dockerfile . .PHONY: docker-push docker-push: diff --git a/README.md b/README.md index 65192ba3..1b8771b5 100644 --- a/README.md +++ b/README.md @@ -129,6 +129,7 @@ trust a CA on the W&B **application** workloads instead, use - [Migrating from Operator v1 to v2](docs/migrating-v1-to-v2.md) - [Monitoring and Telemetry Guide](docs/monitoring.md) - [Deploying on OpenShift](docs/openshift.md) +- [Deploying Watchtower](docs/watchtower-deployment.md) ## Development diff --git a/deploy/watchtower/Chart.yaml b/deploy/watchtower/Chart.yaml new file mode 100644 index 00000000..e85a2f5b --- /dev/null +++ b/deploy/watchtower/Chart.yaml @@ -0,0 +1,13 @@ +apiVersion: v2 +name: watchtower +description: A Helm chart for the W&B Watchtower cluster administration UI +type: application +# Versioned in lockstep with the operator: Watchtower's binary ships inside the +# operator image, so the chart version doubles as the image tag it deploys. The +# release pipeline enforces chart version == appVersion == operator release. +version: 2.0.0-beta.3 +appVersion: "2.0.0-beta.3" +maintainers: + - name: wandb + email: support@wandb.com + url: https://wandb.com diff --git a/deploy/watchtower/templates/NOTES.txt b/deploy/watchtower/templates/NOTES.txt new file mode 100644 index 00000000..ad046e36 --- /dev/null +++ b/deploy/watchtower/templates/NOTES.txt @@ -0,0 +1,26 @@ +Watchtower is installed as {{ include "watchtower.fullname" . }} in namespace {{ .Release.Namespace }}. + +It is published on a node port rather than through the W&B Ingress, so it is +reachable at: + +{{- if eq .Values.service.type "NodePort" }} + + http://:$(kubectl get svc -n {{ .Release.Namespace }} {{ include "watchtower.fullname" . }} -o jsonpath='{.spec.ports[0].nodePort}'){{ include "watchtower.basePath" . }}/ + +Every node publishes that port. Reaching it from the public internet needs the +port open in the node firewall / security group; nothing in this chart opens it. +{{- else }} + + a {{ .Values.service.type }} Service on port {{ .Values.service.port }}, path {{ include "watchtower.basePath" . }}/ +{{- end }} + +{{ if eq .Values.mode "cluster" }} +Log in with the admin password: + + kubectl get secret -n {{ .Release.Namespace }} {{ include "watchtower.authSecretName" . }} \ + -o jsonpath='{.data.{{ .Values.auth.secretKey }}}' | base64 -d + +{{- else }} +WARNING: mode is {{ .Values.mode | quote }}, not "cluster" — the password gate is +OFF and anyone who can reach the port has full cluster administration. +{{- end }} diff --git a/deploy/watchtower/templates/_helpers.tpl b/deploy/watchtower/templates/_helpers.tpl new file mode 100644 index 00000000..bf4bf7eb --- /dev/null +++ b/deploy/watchtower/templates/_helpers.tpl @@ -0,0 +1,94 @@ +{{- define "watchtower.name" -}} +{{- default .Chart.Name .Values.nameOverride | trunc 63 | trimSuffix "-" -}} +{{- end -}} + +{{- define "watchtower.fullname" -}} +{{- if .Values.fullnameOverride -}} +{{- .Values.fullnameOverride | trunc 63 | trimSuffix "-" -}} +{{- else -}} +{{- $name := default .Chart.Name .Values.nameOverride -}} +{{- if contains $name .Release.Name -}} +{{- .Release.Name | trunc 63 | trimSuffix "-" -}} +{{- else -}} +{{- printf "%s-%s" .Release.Name $name | trunc 63 | trimSuffix "-" -}} +{{- end -}} +{{- end -}} +{{- end -}} + +{{- define "watchtower.labels" -}} +helm.sh/chart: {{ printf "%s-%s" .Chart.Name .Chart.Version | replace "+" "_" | trunc 63 | trimSuffix "-" }} +{{ include "watchtower.selectorLabels" . }} +app.kubernetes.io/version: {{ .Chart.AppVersion | quote }} +app.kubernetes.io/managed-by: {{ .Release.Service }} +{{- end -}} + +{{- define "watchtower.selectorLabels" -}} +app.kubernetes.io/name: {{ include "watchtower.name" . }} +app.kubernetes.io/instance: {{ .Release.Name }} +{{- end -}} + +{{- define "watchtower.serviceAccountName" -}} +{{- if .Values.serviceAccount.create -}} +{{- default (include "watchtower.fullname" .) .Values.serviceAccount.name -}} +{{- else -}} +{{- default "default" .Values.serviceAccount.name -}} +{{- end -}} +{{- end -}} + +{{/* +The image is the operator's, not Watchtower's. Falling back to .Chart.AppVersion +is safe because this chart is versioned in lockstep with the operator release, so +the two are the same string by construction. +*/}} +{{- define "watchtower.image" -}} +{{- if .Values.image.digest -}} +{{- printf "%s@%s" .Values.image.repository .Values.image.digest -}} +{{- else -}} +{{- printf "%s:%s" .Values.image.repository (default .Chart.AppVersion .Values.image.tag) -}} +{{- end -}} +{{- end -}} + +{{/* +Normalizes basePath the same way the Watchtower binary does: "" or a "/"-prefixed +path with no trailing slash. Probe paths and the published URL are built from it, +so a values file writing "watchtower/" must not produce "//healthz". +*/}} +{{- define "watchtower.basePath" -}} +{{- $path := default "" .Values.basePath -}} +{{- if $path -}} +{{- if not (hasPrefix "/" $path) -}}{{- $path = printf "/%s" $path -}}{{- end -}} +{{- trimSuffix "/" $path -}} +{{- end -}} +{{- end -}} + +{{- define "watchtower.wandbName" -}} +{{- default .Release.Name .Values.wandbName -}} +{{- end -}} + +{{/* +ClusterRole/ClusterRoleBinding names are cluster-global, so two Watchtower +releases in different namespaces would otherwise fight over one object — the +second install silently adopting the first's rules and subject list. Qualify the +name with the namespace; namespaced Roles keep the plain fullname. +*/}} +{{- define "watchtower.roleName" -}} +{{- if eq .Values.role.type "Role" -}} +{{- include "watchtower.fullname" . -}} +{{- else -}} +{{- printf "%s-%s" .Release.Namespace (include "watchtower.fullname" .) | trunc 63 | trimSuffix "-" -}} +{{- end -}} +{{- end -}} + +{{/* +A user-managed Secret wins over the generated one; refusing to guess when neither +is available beats rendering a Deployment that CrashLoopBackOffs on a missing key. +*/}} +{{- define "watchtower.authSecretName" -}} +{{- if .Values.auth.existingSecret -}} +{{- .Values.auth.existingSecret -}} +{{- else if .Values.auth.create -}} +{{- printf "%s-auth" (include "watchtower.fullname" .) -}} +{{- else -}} +{{- fail "watchtower: set auth.create=true to generate an admin password, or auth.existingSecret to supply one" -}} +{{- end -}} +{{- end -}} diff --git a/deploy/watchtower/templates/deployment.yaml b/deploy/watchtower/templates/deployment.yaml new file mode 100644 index 00000000..52affdfd --- /dev/null +++ b/deploy/watchtower/templates/deployment.yaml @@ -0,0 +1,134 @@ +{{- $basePath := include "watchtower.basePath" . }} +apiVersion: apps/v1 +kind: Deployment +metadata: + name: {{ include "watchtower.fullname" . }} + namespace: {{ .Release.Namespace }} + labels: + {{- include "watchtower.labels" . | nindent 4 }} +spec: + # Deliberately not configurable: in-flight deploy jobs and their SSE streams + # live in the serving pod's memory, so a reconnect landing on a second pod + # would see no history. + replicas: 1 + selector: + matchLabels: + {{- include "watchtower.selectorLabels" . | nindent 6 }} + template: + metadata: + labels: + {{- include "watchtower.selectorLabels" . | nindent 8 }} + {{- with .Values.podLabels }} + {{- toYaml . | nindent 8 }} + {{- end }} + {{- with .Values.podAnnotations }} + annotations: + {{- toYaml . | nindent 8 }} + {{- end }} + spec: + serviceAccountName: {{ include "watchtower.serviceAccountName" . }} + {{- with .Values.imagePullSecrets }} + imagePullSecrets: + {{- toYaml . | nindent 8 }} + {{- end }} + {{- with .Values.podSecurityContext }} + securityContext: + {{- toYaml . | nindent 8 }} + {{- end }} + containers: + - name: watchtower + image: {{ include "watchtower.image" . }} + imagePullPolicy: {{ .Values.image.pullPolicy }} + # The operator image entrypoint is /manager; this selects the Watchtower + # binary that ships alongside it. + command: + {{- toYaml .Values.command | nindent 12 }} + args: + - --port + - {{ .Values.containerPort | quote }} + env: + - name: WATCHTOWER_MODE + value: {{ .Values.mode | quote }} + - name: WATCHTOWER_BASE_PATH + value: {{ $basePath | quote }} + {{- if eq .Values.mode "cluster" }} + # Never inlined: an env value here would be readable from the pod spec + # by anyone who can `kubectl get deployment`. + - name: WATCHTOWER_PASSWORD + valueFrom: + secretKeyRef: + name: {{ include "watchtower.authSecretName" . }} + key: {{ .Values.auth.secretKey }} + {{- end }} + - name: WATCHTOWER_WANDB_NAME + value: {{ include "watchtower.wandbName" . | quote }} + - name: WATCHTOWER_NAMESPACE + valueFrom: + fieldRef: + fieldPath: metadata.namespace + {{- range $name, $value := .Values.env }} + - name: {{ $name }} + value: {{ $value | quote }} + {{- end }} + ports: + - name: http + containerPort: {{ .Values.containerPort }} + protocol: TCP + # Health routes sit outside the auth gate but inside the base path, so + # the probes have to carry the prefix too. + livenessProbe: + httpGet: + path: {{ $basePath }}/healthz + port: http + initialDelaySeconds: 15 + periodSeconds: 20 + readinessProbe: + httpGet: + path: {{ $basePath }}/ready + port: http + initialDelaySeconds: 5 + periodSeconds: 10 + {{- with .Values.securityContext }} + securityContext: + {{- toYaml . | nindent 12 }} + {{- end }} + {{- with .Values.resources }} + resources: + {{- toYaml . | nindent 12 }} + {{- end }} + # Watchtower drives Helm and writes the air-gapped dependency bundle + # relative to its working directory, neither of which the read-only + # root filesystem allows. + workingDir: /home/watchtower + volumeMounts: + - name: home + mountPath: /home/watchtower + - name: helm + mountPath: /helm + - name: tmp + mountPath: /tmp + {{- with .Values.extraVolumeMounts }} + {{- toYaml . | nindent 12 }} + {{- end }} + volumes: + - name: home + emptyDir: {} + - name: helm + emptyDir: {} + - name: tmp + emptyDir: {} + {{- with .Values.extraVolumes }} + {{- toYaml . | nindent 8 }} + {{- end }} + {{- with .Values.nodeSelector }} + nodeSelector: + {{- toYaml . | nindent 8 }} + {{- end }} + {{- with .Values.tolerations }} + tolerations: + {{- toYaml . | nindent 8 }} + {{- end }} + {{- with .Values.affinity }} + affinity: + {{- toYaml . | nindent 8 }} + {{- end }} diff --git a/deploy/watchtower/templates/role.yaml b/deploy/watchtower/templates/role.yaml new file mode 100644 index 00000000..85a951a2 --- /dev/null +++ b/deploy/watchtower/templates/role.yaml @@ -0,0 +1,31 @@ +{{- if .Values.role.create }} +apiVersion: rbac.authorization.k8s.io/v1 +kind: {{ .Values.role.type }} +metadata: + name: {{ include "watchtower.roleName" . }} + {{- if eq .Values.role.type "Role" }} + namespace: {{ .Release.Namespace }} + {{- end }} + labels: + {{- include "watchtower.labels" . | nindent 4 }} +rules: + {{- toYaml .Values.role.rules | nindent 2 }} +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: {{ ternary "RoleBinding" "ClusterRoleBinding" (eq .Values.role.type "Role") }} +metadata: + name: {{ include "watchtower.roleName" . }} + {{- if eq .Values.role.type "Role" }} + namespace: {{ .Release.Namespace }} + {{- end }} + labels: + {{- include "watchtower.labels" . | nindent 4 }} +subjects: + - kind: ServiceAccount + name: {{ include "watchtower.serviceAccountName" . }} + namespace: {{ .Release.Namespace }} +roleRef: + kind: {{ .Values.role.type }} + name: {{ include "watchtower.roleName" . }} + apiGroup: rbac.authorization.k8s.io +{{- end }} diff --git a/deploy/watchtower/templates/secret.yaml b/deploy/watchtower/templates/secret.yaml new file mode 100644 index 00000000..c6b3b969 --- /dev/null +++ b/deploy/watchtower/templates/secret.yaml @@ -0,0 +1,28 @@ +{{- if .Values.auth.create }} +{{- $name := printf "%s-auth" (include "watchtower.fullname" .) }} +{{/* +Reuse the password already in the cluster. Without this lookup a plain +randAlphaNum re-rolls on every `helm upgrade`, silently locking the admin out of +an install that was working a moment earlier. lookup returns nothing under +`helm template` and `--dry-run`, so rendered output there will differ from a real +install — that is expected, not a bug. +*/}} +{{- $existing := lookup "v1" "Secret" .Release.Namespace $name }} +{{- $password := .Values.auth.password }} +{{- if and (not $password) $existing }} +{{- $password = index $existing.data "password" | b64dec }} +{{- end }} +{{- if not $password }} +{{- $password = randAlphaNum 32 }} +{{- end }} +apiVersion: v1 +kind: Secret +metadata: + name: {{ $name }} + namespace: {{ .Release.Namespace }} + labels: + {{- include "watchtower.labels" . | nindent 4 }} +type: Opaque +stringData: + password: {{ $password | quote }} +{{- end }} diff --git a/deploy/watchtower/templates/service.yaml b/deploy/watchtower/templates/service.yaml new file mode 100644 index 00000000..c01fca18 --- /dev/null +++ b/deploy/watchtower/templates/service.yaml @@ -0,0 +1,26 @@ +apiVersion: v1 +kind: Service +metadata: + name: {{ include "watchtower.fullname" . }} + namespace: {{ .Release.Namespace }} + labels: + {{- include "watchtower.labels" . | nindent 4 }} + {{- with .Values.service.labels }} + {{- toYaml . | nindent 4 }} + {{- end }} + {{- with .Values.service.annotations }} + annotations: + {{- toYaml . | nindent 4 }} + {{- end }} +spec: + type: {{ .Values.service.type }} + ports: + - name: http + port: {{ .Values.service.port }} + targetPort: http + protocol: TCP + {{- if and .Values.service.nodePort (eq .Values.service.type "NodePort") }} + nodePort: {{ .Values.service.nodePort }} + {{- end }} + selector: + {{- include "watchtower.selectorLabels" . | nindent 4 }} diff --git a/deploy/watchtower/templates/serviceaccount.yaml b/deploy/watchtower/templates/serviceaccount.yaml new file mode 100644 index 00000000..098feaa6 --- /dev/null +++ b/deploy/watchtower/templates/serviceaccount.yaml @@ -0,0 +1,16 @@ +{{- if .Values.serviceAccount.create }} +apiVersion: v1 +kind: ServiceAccount +metadata: + name: {{ include "watchtower.serviceAccountName" . }} + namespace: {{ .Release.Namespace }} + labels: + {{- include "watchtower.labels" . | nindent 4 }} + {{- with .Values.serviceAccount.annotations }} + annotations: + {{- toYaml . | nindent 4 }} + {{- end }} +# Unlike the W&B application pods, Watchtower calls the Kubernetes API on the +# user's behalf, so it needs its token projected. +automountServiceAccountToken: {{ .Values.serviceAccount.automount }} +{{- end }} diff --git a/deploy/watchtower/values.yaml b/deploy/watchtower/values.yaml new file mode 100644 index 00000000..dcab09f0 --- /dev/null +++ b/deploy/watchtower/values.yaml @@ -0,0 +1,145 @@ +nameOverride: "" +fullnameOverride: "" + +# Watchtower runs from the operator image — its binary is copied in at build time +# as a second entrypoint (see the repository Dockerfile). Keeping the two in one +# image means one artifact to mirror for air-gapped installs. +# +# This is the chart's only tie to the operator: the two install as independent +# releases, and Watchtower needs no operator running to come up. The tag is the +# operator's version, not Watchtower's, and it tracks this chart's version — the +# release pipeline holds them equal. Digest wins when both are set. +image: + repository: us-docker.pkg.dev/wandb-production/public/wandb/operator + tag: "" + digest: "" + pullPolicy: IfNotPresent +imagePullSecrets: [] + +replicaCount: 1 + +# The command that selects Watchtower rather than the operator manager. +command: + - /watchtower + +# URL prefix Watchtower serves under. It must match the prefix the image was +# built for: Next.js bakes basePath into every asset URL and router href at build +# time, and the binary refuses to start when its runtime value disagrees with the +# compiled-in one. The published Watchtower image is built with /watchtower, so +# only change this alongside an image built with a matching NEXT_PUBLIC_BASE_PATH. +basePath: /watchtower + +containerPort: 8080 + +# Admin login. Watchtower does not implement OIDC and does not share the W&B +# app's session — published on its own origin, that cookie never reaches it. A +# single admin password gates the UI instead, and in cluster mode the binary +# refuses to start without one rather than serve cluster administration +# unauthenticated. +auth: + # Generate the password into a Secret named -auth on first install. + # Existing values are reused on upgrade, so the password is stable. + create: true + # Pin a specific password instead of generating one. Prefer existingSecret for + # anything real — a value here lands in the Helm release history. + password: "" + # Use a Secret you manage yourself. Takes precedence over create. + existingSecret: "" + secretKey: password + +# Locks the UI to the cluster it runs in — no context switching, no teardown — +# and turns on the password gate. Only set this to "web" against a sandbox you +# do not care about: it disables the gate entirely. +mode: cluster + +# Name of the WeightsAndBiases CR this Watchtower administers. Defaults to the +# release name when empty. +wandbName: "" + +serviceAccount: + create: true + automount: true + name: "" + annotations: {} + +role: + create: true + # "Role" scopes the grant to the release namespace; "ClusterRole" widens it to + # every namespace, which Watchtower only needs when it manages installs outside + # its own. + type: ClusterRole + rules: + - apiGroups: + - apps.wandb.com + resources: + - weightsandbiases + - weightsandbiases/status + - applications + - applications/status + - applications/ + verbs: + - get + - list + - watch + - create + - update + - patch + - delete + - apiGroups: + - "" + resources: + - secrets + verbs: + - get + - list + - watch + - create + - update + - patch + - delete + +# Watchtower's own Go HTTP server is the public entry point — there is no Ingress +# and no reverse proxy in front of it, so it is reached on a port published by +# every node. Leave nodePort empty to let Kubernetes allocate one from +# --service-node-port-range. +service: + type: NodePort + port: 8080 + nodePort: "" + annotations: {} + labels: {} + +resources: + requests: + cpu: 100m + memory: 256Mi + limits: + cpu: 500m + memory: 512Mi + +podAnnotations: {} +podLabels: {} + +podSecurityContext: + runAsNonRoot: true + runAsUser: 65532 + runAsGroup: 65532 + fsGroup: 65532 + fsGroupChangePolicy: OnRootMismatch + seccompProfile: + type: RuntimeDefault + +securityContext: + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true + capabilities: + drop: + - ALL + +env: {} +extraVolumes: [] +extraVolumeMounts: [] + +nodeSelector: {} +tolerations: [] +affinity: {} diff --git a/docs/watchtower-deployment.md b/docs/watchtower-deployment.md new file mode 100644 index 00000000..6f5b08c1 --- /dev/null +++ b/docs/watchtower-deployment.md @@ -0,0 +1,169 @@ +# Deploying Watchtower + +[Watchtower](https://github.com/wandb/watchtower) is the cluster administration UI +that replaces the deprecated W&B console. This document covers how it is packaged +and installed alongside the operator. + +## Packaging: one image, two entrypoints + +Watchtower is **not** a separate image to pull, mirror and version. Its binary is +copied into the operator image at build time and sits next to `/manager`: + +```dockerfile +ARG WATCHTOWER_IMAGE=us-docker.pkg.dev/wandb-production/public/wandb/watchtower +ARG WATCHTOWER_VERSION=0.11.0 +... +FROM ${WATCHTOWER_IMAGE}:${WATCHTOWER_VERSION} AS watchtower +... +COPY --from=watchtower /watchtower . +``` + +The binary is lifted from the published Watchtower image rather than rebuilt from +Go source, because it embeds a Next.js static export — building it here would mean +carrying the Watchtower frontend, a Node toolchain and a circular module +dependency (Watchtower already depends on `github.com/wandb/operator`). + +Pin a different release with: + +```bash +make docker-build WATCHTOWER_VERSION=0.12.0 +``` + +Selecting which of the two binaries runs is the container's `command`: the image +`ENTRYPOINT` stays `/manager`, and the Watchtower Deployment overrides it with +`/watchtower`. + +## Installation: a separate release + +`deploy/watchtower/` is its own chart and its own Helm release. It is **not** a +dependency of `deploy/operator` — installing Watchtower does not install the +operator, upgrading one does not touch the other, and deleting one leaves the +other running. The only tie between them is the image. + +```bash +helm install watchtower oci://us-docker.pkg.dev/wandb-production/charts/watchtower \ + --version 2.0.0-beta.3 -n wandb --create-namespace +``` + +Both charts are published from this repo by the same release workflow, and the +version check there holds the watchtower chart version, its appVersion, and the +operator image tag equal to the release. So `--version 2.0.0-beta.3` deploys the +Watchtower binary from operator image `2.0.0-beta.3` with no second version to +track — which is also why `image.tag` can be left empty and defaults to the +chart's appVersion. + +It creates: + +| Resource | Purpose | +|----------|---------| +| `Deployment` | The operator image run as `/watchtower --port 8080` | +| `Service` | `NodePort`, publishing the Go HTTP server directly | +| `ServiceAccount` | With its token projected — Watchtower calls the Kubernetes API | +| `Role` + `RoleBinding` | Write access to the W&B CRs and to secrets | +| `Secret` | The generated admin password (see Authentication below) | + +A minimal values file: + +```yaml +service: + nodePort: 32080 # omit to let Kubernetes allocate one +role: + type: ClusterRole # Role scopes Watchtower to its own namespace +``` + +`replicas` is deliberately fixed at 1 and not exposed: in-flight deploy jobs and +their SSE streams live in the serving pod's memory, so a reconnect landing on a +second pod would see no history. + +## Routing: a node port, not an Ingress + +Watchtower's own Go HTTP server is the public entry point. There is no Ingress +path on the W&B hostname and no reverse proxy in front of it — the `NodePort` +Service publishes the port on every node, and reaching it from the public +internet is a matter of opening that port in the node firewall or security group. +Nothing in this chart opens it. + +Set `service.type` to `ClusterIP` for an internal-only install, or to +`LoadBalancer` to get a dedicated address instead. + +### The base path is a build-time value + +`basePath` defaults to `/watchtower` and cannot be changed on its own. Next.js +bakes `basePath` into every asset URL and router href at build time, and the +Watchtower binary refuses to start when its runtime base path disagrees with the +compiled-in one — so serving at the root requires a Watchtower image built with +`BASE_PATH=` empty, and `watchtower.basePath: ""` to match. The published image +is built with `/watchtower`. + +The health probes carry the prefix for the same reason: `{basePath}/healthz` and +`{basePath}/ready`, which sit outside the auth gate (the kubelet sends no cookie) +but inside the base path. + +## RBAC + +The chart's `Role` grants exactly what Watchtower needs to manage an install: + +- `apps.wandb.com` — `weightsandbiases`, `applications` and their `/status` + subresources, full verbs +- core `secrets`, full verbs + +`role.type` defaults to `ClusterRole`, so Watchtower can manage installs in any +namespace. Set it to `Role` to confine it to its own release namespace. + +Cluster-scoped names are qualified with the namespace — +`--watchtower` — because `ClusterRole` and +`ClusterRoleBinding` names are cluster-global. Without that, a second Watchtower +release in another namespace would adopt the first one's object and silently +overwrite its rules and subject list. Namespaced `Role`s keep the plain name. + +Because the container runs with `readOnlyRootFilesystem: true`, the Deployment +mounts `emptyDir`s at `/home/watchtower` (its working directory, where the +air-gapped dependency bundle lands), `/helm` and `/tmp`. + +## Authentication: a chart-generated admin password + +Watchtower implements no OIDC and does not share the W&B app's session. That +earlier design only worked because Watchtower was served under the app's +hostname, so the browser sent the app's cookie along; published on its own origin +it never arrives. A single admin password gates the UI instead. + +The chart generates it on first install into a Secret named +`-watchtower-auth` and injects it as `WATCHTOWER_PASSWORD` via +`secretKeyRef` — never inlined in the pod spec, where anyone with +`kubectl get deployment` could read it. Retrieve it with: + +```bash +kubectl get secret -n wandb wandb-watchtower-auth \ + -o jsonpath='{.data.password}' | base64 -d +``` + +The template reads any existing Secret via `lookup` before generating, so +`helm upgrade` preserves the password rather than silently rotating it and +locking the admin out. Two overrides: `auth.existingSecret` to manage the Secret +yourself (preferred for GitOps — a generated password is invisible until someone +reads it), or `auth.password` to pin a value, which lands in the Helm release +history and is best avoided. + +On the wire: `POST /login` checks the password in constant time and +sets an `HttpOnly`, `SameSite=Lax` session cookie scoped to the base path, +holding an expiry signed with an HMAC keyed on the password itself. There is no +server-side session store — Watchtower is a single replica that restarts freely — +and because the key is derived from the password, rotating the Secret invalidates +every outstanding session for free. Sessions last 12 hours. `Secure` is set only +when the request arrived over TLS, since the Service publishes plain HTTP and an +unconditionally-Secure cookie would never be sent back. + +Unauthenticated `/api/v1/*` calls get a JSON 401 so the frontend can render +"session expired"; page loads redirect to the login form. `/healthz` and `/ready` +stay outside the gate — the kubelet holds no session. + +`mode` defaults to `cluster`, which is what turns the gate on. Setting it to +`web` disables authentication entirely; only do that against a sandbox you do not +care about. + +### Still worth doing + +The password is a shared secret with no rate limiting on the login endpoint. A +32-character generated password is not guessable, but a user-chosen +`auth.password` might be — consider a lockout or backoff before this is exposed +broadly, and keep the node port firewalled to known source ranges regardless. diff --git a/docs/watchtower.md b/docs/watchtower.md new file mode 100644 index 00000000..626d95df --- /dev/null +++ b/docs/watchtower.md @@ -0,0 +1,260 @@ +With wandb console being deprecated, users want a way to access a wandb deployed UI for managing their infrastructure. Today we have https://github.com/wandb/watchtower which can connect to any context and manage instance deploys there, but this access is too general and we want a wandb app for on-prem customers to manage their deploys the way they used to with console. + +# How Console Was Set Up + +In https://github.com/wandb/helm-charts/ + +- **In-cluster nginx** — `charts/operator-wandb/templates/nginx.yaml:33` + + ``` + location /console { proxy_pass http://{{ .Release.Name }}-console:8082;} + ``` + +- **Ingress path** — `charts/operator-wandb/templates/_ingress.tpl:190` + + ```yaml + -pathType:Prefixpath:/consolebackend:service:name:{{$.Release.Name}}-consoleport:number:808 + ``` + + +With the Deployment of console coming from https://github.com/wandb/deployments with the service named `wandb-console` (`wandb/console/values.yaml`) so that, with release name `wandb` the chart's `{{ .Release.Name }}-console` selector resolves to it. + +In the console frontend itself, `next.config.ts` sets `basePath: "/console"` when `NODE_ENV === "production"`. + +## Authentication + +Console is/was not performing its own authorization or oidc validation. The OIDC login happens in the main W&B app on the same host. Console only asks Gorilla to validate the session cookie that the browser already carries. I believe if we piggyback watchtower off of the wandb app (which is the ask), we can use this same apporach that console was using. + +The check is a plain HTTP sub-request (`~/console/src/server/api/routers/auth/util.ts:7`): + +```tsx +exportconst APP_AUTH_CHECK_URL=`http://${env.AUTH_SERVICE}/oidc/auth`; +``` + +and (`:170`): + +```tsx +exportconst isLoggedInWithApp= async(headers: Headers)=>{const res=await fetch(APP_AUTH_CHECK_URL,{ method:"GET", headers, signal});return res.ok;}; +``` + +The incoming request headers — including `Cookie` — are forwarded verbatim. It is gated on every tRPC call through `protectedProcedure` (`~/console/src/server/api/trpc.ts:86`). + +`AUTH_SERVICE` comes from the `wandb-console-configmap` rendered by the chart https://github.com/wandb/helm-charts/:  (`charts/operator-wandb/templates/console.yaml`): + +``` +AUTH_SERVICE: "{{ .Release.Name }}-api:8081" # when global.localService.bypassAUTH_SERVICE: "{{ .Release.Name }}-app:8080" # otherwise +``` + +Dedicated-cloud values set `global.localService.bypass: true` and `app.install: false` (`~/deployments/shared-tenant/charts/wandb/values/enabled-services.yaml`), so in practice it resolves to `wandb-api:8081`. + +The endpoint on the other side is set up in Core `services/gorilla/api/handler/oidc.go:99` and does not require any changes for watchtower, just call `/oidc/auth` without `?admin=false`. The response also hands back the identity, which is useful for audit logging and for showing the current user in the UI. + +Because of this design, watchtower will need to come from the same hostname as the wandb app but I’m pretty sure that’s what we want, so we’re good there. Please flag if that’s not the case. + +Console also had a second, independent auth path intended for on-prem: a root password stored in the `{release}-password` Secret, verified server-side, issuing an ES512 JWT in a `wandb-console-auth` cookie (`util.ts`, `getPassword`/`generateToken`/ `decodeToken`). `isLoggedIn` accepted **either** path. + +## Deployment + +Console was deployed through ArgoCD/Control Plane which is also no longer the case. + +// TODO - Look into deployment through Orca? + +Rollout policy for Console per `~/deployments/wandb/user-spec/README.md`: sandbox/QA deploy automatically on merge; production requires manual approval with a 2-hour gradual rollout. + +# For Watchtower + +Watchtower today is a single Go binary that embeds a Next.js static export (`backend/static/`, populated by `make frontend`) and drives clusters through the `wsm` library. Serving it from inside a cluster means four changes: a base path, an auth gate, a locked-down single-cluster mode, and a deployment pipeline. + +``` +browser ──► ingress / wandb-nginx ──► /watchtower ──► Service wandb-watchtower:8080 │ ├─ auth middleware ──► http://wandb-api:8081/oidc/auth └─ wsm/client-go ────► in-cluster Kubernetes API +``` + +--- + +## Base Path + +**Frontend** + +- `frontend/next.config.ts` — add `basePath` and `assetPrefix`, driven by an env var so dev and desktop builds stay at `/`. `trailingSlash: true` is already set. +- `frontend/lib/api.ts:46` — `const BASE = "/api/v1"` is the single entry point for every typed fetch wrapper (`request()`). +- `frontend/lib/sse.ts:16` — `SSE_ORIGIN` needs to become base-path aware. SSE URLs are handed to the frontend by the backend as absolute paths (`/api/v1/deploy/{jobId}/stream`), so either the backend emits them prefixed or `useSSEStream` prefixes them. Pick one and keep it consistent; the backend is the better place since it already knows its own mount point. + +**Backend** + +- `backend/server/server.go:39-58` — wrap both the API router and the `http.FileServer` in `http.StripPrefix(basePath, …)`. Add a `BasePath` field to `server.Options` and a `-base-path` flag in `backend/main.go` (the desktop entrypoint keeps the default empty value). +- Add `GET /healthz` and `GET /ready`. **These do not exist today** — `backend/api/router.go` has no health endpoints. Console's probes hit `/console/api/healthz` and `/console/api/ready`; we need the equivalents for liveness/readiness/startup probes in the chart. `/healthz` should be a static 200 (process is up); `/ready` should additionally confirm the Kubernetes client initialized. + +## Auth + +middleware in `backend/api/router.go` mirroring `isLoggedInWithApp`: + +1. Forward the incoming `Cookie` (and `Authorization`, for API-token callers) to `GET http://$AUTH_SERVICE/oidc/auth`, with a short timeout (2-3s) +2. `200` → allow, and stash `X-Wandb-User-Email` on the request context for logging. +3. `401/403` → for HTML navigations, redirect to the app login with `?redirect_to=/watchtower`; for `/api/v1/*` calls, return `401` so the frontend can surface a clean "session expired" state instead of rendering a broken page. + +## RBAC + +Use Console's `role:` block in `~/deployments/wandb/console/values.yaml` as the starting point. + +Watchtower needs, on top of it: `apps.wandb.com` `weightsandbiases` at v2 with `get/list/watch` , `update/patch` , `apiextensions.k8s.io` `customresourcedefinitions` `get/list` (used to detect whether v2 is served), and `pods/portforward` for telemetry port-forward. + +## **Packaging, Deployment and Routing** + +The existing `Dockerfile` will not build in CI. It copies `go.work` / `go.work.sum` and relies on the workspace's `replace` directives pointing at `../operator` and `../wsm`, which do not exist in the build context  + +Convert it to the vendored path the rest of the repo uses: + +```docker +ENV GOWORK=offRUN go build-mod=vendor-ldflags"-X .../backend/version.Version=${VERSION}"-o/watchtower./backend/... +``` + +It should also `EXPOSE` the port the chart expects and default `--base-path` from env. + +`release.yml` currently produces desktop artifacts only. Add a job that builds and pushes `wandb/watchtower:`  + +Add `/watchtower` to the `operator-wandb` chart. A `location /watchtower` in `templates/nginx.yaml` and a path in `_ingress.tpl`, both gated on a `watchtower.install` value. Consistent with how `/console` works, and correct regardless of whether traffic enters via ingress or the internal nginx. + +The chart route above applies to **v1 / `operator-wandb` only**. Under the v2 +operator there is no in-cluster nginx and no chart in the request path: the +operator publishes `/watchtower` itself. See below. + +# Operator-side implementation (v2) + +Watchtower is deployed by `wandb/operator` as an **operator-owned component**. It +is not published in the server manifest, so it cannot ride the manifest-driven +`reconcileApplications` path the W&B applications use; instead the operator +synthesises its `Application` directly, the same way managed Kafka/etcd do. + +## Config surface + +```yaml +apiVersion: apps.wandb.com/v2 +kind: WeightsAndBiases +spec: + watchtower: + install: true # opt-in; defaults to false + image: + repository: us-docker.pkg.dev/wandb-production/public/wandb/watchtower + tag: 0.11.0 # digest wins over tag when both are set + basePath: /watchtower # published route and container base path + authService: "" # empty = derive from the server manifest + resources: {} # defaults to 100m / 256Mi requests + serviceAccount: + create: true + serviceAccountName: wandb-watchtower +``` + +`install` defaults to **false**: Watchtower grants cluster-wide read access to +anyone holding a W&B session, so turning it on is an explicit decision. + +## What the operator creates + +| Resource | Name | Notes | +|----------|------|-------| +| `Application` | `wandb-watchtower` | `Kind: Deployment`, `replicas: 1`, labelled `weightsandbiases.apps.wandb.com/component=watchtower` so manifest-driven pruning skips it | +| `Service` | `wandb-watchtower` | ClusterIP `8080`, derived from the Application by the application controller | +| `ServiceAccount` | `wandb-watchtower` | Token automounted — unlike the W&B app pods, Watchtower calls the Kubernetes API | +| `Role` / `RoleBinding` | `wandb-watchtower` | Namespaced reads: secrets, configmaps, jobs, ingresses | +| `ClusterRole` / `ClusterRoleBinding` | `--watchtower` | Cluster-wide reads plus `weightsandbiases` `update/patch` | +| Ingress path | `/watchtower` on the consolidated Ingress | Added in `reconcileConsolidatedIngress` | +| `HTTPRoute` | via `Application.spec.httpRouteTemplate` | Gateway API mode only, same hostnames as the app | + +`replicas` is deliberately **not** configurable: in-flight deploy jobs and their +SSE streams live in the serving pod's memory, so a reconnect landing on a second +pod would see no history. + +Reconciliation runs **before** the infra-readiness gate in +`ReconcileWandbManifest`, so Watchtower comes up even when the install it is +meant to diagnose is stuck. + +Status lands in `status.watchtowerStatus` (`ready`, `url`, `image`, +`authService`). Watchtower never gates the CR's `Ready` condition. + +## Container contract + +The operator passes the deployment-specific bits as env vars, so nothing has to +be baked into the image: + +| Env var | Value | Purpose | +|---------|-------|---------| +| `WATCHTOWER_MODE` | `cluster` | Locks the UI to its own cluster: no context selection, no teardown | +| `WATCHTOWER_BASE_PATH` | `spec.watchtower.basePath` | Backend `StripPrefix` mount and frontend `basePath`/`assetPrefix` | +| `WATCHTOWER_AUTH_SERVICE` | e.g. `api:8080` | Host:port for the `GET /oidc/auth` sub-request | +| `WATCHTOWER_WANDB_NAME` | CR name | Which install this Watchtower manages | +| `WATCHTOWER_NAMESPACE` | fieldRef | Namespace of that install | + +`WATCHTOWER_MODE` already exists in the Watchtower repo +(`backend/api/status/handler.go:27`, which checks for `desktop`); the other four +are new and still need implementing there. + +Probes are `GET {basePath}/healthz` (liveness) and `GET {basePath}/ready` +(readiness) on port 8080 — they go through the base path because the server +mounts every route, health included, behind it. **Neither endpoint exists in the +Watchtower repo yet**, so until Phase 1 lands there the pods will never go ready. + +## Deriving `authService` + +`spec.watchtower.authService` is normally left empty. The operator finds the +manifest application that owns the `/oidc` ingress path — `api` in current +manifests — and uses `:`, since the +application controller names each Service after its Application. That keeps +working across manifest renames and port changes. + +If no application declares `/oidc`, reconciliation **fails** rather than deploying +an unauthenticated Watchtower. + +## Known RBAC gap + +The apiserver refuses to grant permissions the operator does not itself hold, so +two rules from the list above are **missing** from the ClusterRole the operator +creates: + +- `apiextensions.k8s.io/customresourcedefinitions` `get/list` — used to detect + whether v2 is served +- `pods/portforward` — telemetry port-forward + +Both need the operator's own ClusterRole widened first (kubebuilder markers in +`internal/controller/weightsandbiases_controller.go`, then `make manifests`). +Installing or upgrading the operator from inside the pod needs more again, and is +not granted today. + +# Open Questions/Notes + +- How to test a deploy for this? Watchtower is only operator v2 compatible, do we have dedicated instances on operator v2? +- How to deploy with Orca? +- Need to pin replicas: 1 so that reconnects to land in a pod that has never seen a run and has no history +- Probably want to remove some functionality from Watchtower for this deployment like context selecting and `teardown` +- The Watchtower service is going to need greater permissions to install/upgrade the operator from inside the pods + +### Notes from Claude + +## **Suggested phasing** + +| **Phase** | **Scope** | **Rough size** | +| --- | --- | --- | +| 1 | `--base-path` (frontend + backend), `/healthz` + `/ready`, vendored `Dockerfile`, dev-only CORS | 2–3 days | +| 2 | `/oidc/auth` middleware, `AUTH_SERVICE` wiring, login redirect, verdict cache, fail-closed | 2–3 days | +| 3 | `Mode: "cluster"`, router allowlist, frontend gating, in-cluster config test coverage | 4–5 days | +| 4 | Role/ClusterRole, `wandb-base` chart, ArgoCD app, Ctrlplane deployment, releaser workflow, ingress routing | 4–5 days | + +## **Reference index** + +| **Thing** | **Location** | +| --- | --- | +| Console auth sub-request | `~/console/src/server/api/routers/auth/util.ts:7`, `:170` | +| Console auth gate | `~/console/src/server/api/trpc.ts:86` | +| Console base path | `~/console/next.config.ts` | +| Console health routes | `~/console/src/app/api/healthz/route.ts`, `.../ready/route.ts` | +| Gorilla sub-auth handler | `~/core/services/gorilla/api/handler/oidc.go:99`, `:590` | +| nginx `/console` route | `~/helm-charts/charts/operator-wandb/templates/nginx.yaml:33` | +| ingress `/console` path | `~/helm-charts/charts/operator-wandb/templates/_ingress.tpl:190` | +| `AUTH_SERVICE` ConfigMap | `~/helm-charts/charts/operator-wandb/templates/console.yaml` | +| Console chart + Argo app | `~/deployments/wandb/console/` | +| Ctrlplane deployment def | `~/deployments/systems-terraform/modules/wandb/deployments.tf:20` | +| Console releaser workflow | `~/deployments/.github/workflows/wandb-console-releaser.yaml` | +| Spec-ownership warning | `~/deployments/wandb/user-spec/README.md` | +| Dedicated-cloud service toggles | `~/deployments/shared-tenant/charts/wandb/values/enabled-services.yaml` | +| wsm in-cluster fallback | `vendor/github.com/wandb/wsm/pkg/kubectl/kubectl.go:100` | +| Watchtower router mounts | `backend/api/router.go` | +| Watchtower mode flag | `backend/types/api.go:117`, `backend/api/status/handler.go:27` | +| Watchtower manifest stub | `deploy/k8s/` | \ No newline at end of file From 7e89c8a30c70bf784fa204c378b16e8f70dfa123 Mon Sep 17 00:00:00 2001 From: Collin Olander Date: Wed, 19 Aug 2026 15:07:26 -0500 Subject: [PATCH 2/7] feat: Watchtower from app after networking --- .github/workflows/internal-chart-publish.yaml | 9 +- .github/workflows/release.yaml | 16 +- Dockerfile | 9 +- api/v2/weightsandbiases_types.go | 72 ++- api/v2/zz_generated.deepcopy.go | 59 ++ .../apps.wandb.com_weightsandbiases.yaml | 79 +++ deploy/operator/templates/_helpers.tpl | 5 + deploy/operator/values.yaml | 1 + deploy/watchtower/Chart.yaml | 13 - deploy/watchtower/templates/NOTES.txt | 26 - deploy/watchtower/templates/_helpers.tpl | 94 --- deploy/watchtower/templates/deployment.yaml | 134 ---- deploy/watchtower/templates/role.yaml | 31 - deploy/watchtower/templates/secret.yaml | 28 - deploy/watchtower/templates/service.yaml | 26 - .../watchtower/templates/serviceaccount.yaml | 16 - deploy/watchtower/values.yaml | 145 ----- docs/watchtower-deployment.md | 288 +++++---- .../controller/reconciler/infra_routes.go | 8 + internal/controller/reconciler/ingress.go | 20 +- .../controller/reconciler/reconcile_v2.go | 91 +-- internal/controller/reconciler/watchtower.go | 572 ++++++++++++++++++ .../controller/reconciler/watchtower_test.go | 540 +++++++++++++++++ .../apps.wandb.com_weightsandbiases.yaml | 79 +++ .../v2/weightsandbiases_watchtower_test.go | 92 +++ .../webhook/v2/weightsandbiases_webhook.go | 39 +- 26 files changed, 1802 insertions(+), 690 deletions(-) delete mode 100644 deploy/watchtower/Chart.yaml delete mode 100644 deploy/watchtower/templates/NOTES.txt delete mode 100644 deploy/watchtower/templates/_helpers.tpl delete mode 100644 deploy/watchtower/templates/deployment.yaml delete mode 100644 deploy/watchtower/templates/role.yaml delete mode 100644 deploy/watchtower/templates/secret.yaml delete mode 100644 deploy/watchtower/templates/service.yaml delete mode 100644 deploy/watchtower/templates/serviceaccount.yaml delete mode 100644 deploy/watchtower/values.yaml create mode 100644 internal/controller/reconciler/watchtower.go create mode 100644 internal/controller/reconciler/watchtower_test.go create mode 100644 internal/webhook/v2/weightsandbiases_watchtower_test.go diff --git a/.github/workflows/internal-chart-publish.yaml b/.github/workflows/internal-chart-publish.yaml index 160e9990..0962199d 100644 --- a/.github/workflows/internal-chart-publish.yaml +++ b/.github/workflows/internal-chart-publish.yaml @@ -29,11 +29,6 @@ jobs: echo "Internal chart publishing requires 2.x.y-dev.; got ${version}" >&2 exit 1 fi - wt_version="$(awk '$1 == "version:" { print $2; exit }' deploy/watchtower/Chart.yaml | tr -d '\"')" - if [[ "${wt_version}" != "${version}" ]]; then - echo "deploy/watchtower must be versioned with the operator; got ${wt_version} vs ${version}" >&2 - exit 1 - fi echo "version=${version}" >> "${GITHUB_OUTPUT}" - name: Install Helm @@ -86,7 +81,7 @@ jobs: exit 1 fi - - name: Package and publish development charts + - name: Package and publish development chart env: VERSION: ${{ steps.chart.outputs.version }} run: | @@ -95,5 +90,3 @@ jobs: mkdir -p dist helm package deploy/operator --destination dist helm push "dist/operator-${VERSION}.tgz" "oci://${CHART_REPOSITORY}" - helm package deploy/watchtower --destination dist - helm push "dist/watchtower-${VERSION}.tgz" "oci://${CHART_REPOSITORY}" diff --git a/.github/workflows/release.yaml b/.github/workflows/release.yaml index b0d7b906..1e845777 100644 --- a/.github/workflows/release.yaml +++ b/.github/workflows/release.yaml @@ -74,20 +74,10 @@ jobs: in_operator && $0 == " image:" { in_image = 1; next } in_image && $1 == "tag:" { print $2; exit } ' deploy/operator/values.yaml | tr -d '\"')" - # The watchtower chart ships separately but deploys the operator image, - # so its version doubles as that image's tag and must track the release. - wt_chart_version="$(awk '$1 == "version:" { print $2; exit }' deploy/watchtower/Chart.yaml | tr -d '\"')" - wt_app_version="$(awk '$1 == "appVersion:" { print $2; exit }' deploy/watchtower/Chart.yaml | tr -d '\"')" - - for value in "${chart_version}" "${app_version}" "${image_tag}" \ - "${wt_chart_version}" "${wt_app_version}"; do if [[ "${value}" != "${version}" ]]; then - echo "Operator and watchtower chart versions, appVersions, and the operator image tag must all equal ${version}" >&2 echo "Found chart=${chart_version}, appVersion=${app_version}, image=${image_tag}" >&2 - echo " watchtower chart=${wt_chart_version}, appVersion=${wt_app_version}" >&2 exit 1 fi - done echo "tag=${tag}" >> "${GITHUB_OUTPUT}" echo "tagged_commit=${tagged_commit}" >> "${GITHUB_OUTPUT}" @@ -159,7 +149,7 @@ jobs: - name: Lint charts run: ct lint --all --config deploy/ct.yaml - - name: Package and publish charts + - name: Package and publish chart env: VERSION: ${{ steps.release.outputs.version }} run: | @@ -168,10 +158,6 @@ jobs: mkdir -p dist helm package deploy/operator --destination dist helm push "dist/operator-${VERSION}.tgz" "oci://${CHART_REPOSITORY}" - # Published as its own chart, not an operator dependency: Watchtower is a - # separate release with a separate lifecycle. - helm package deploy/watchtower --destination dist - helm push "dist/watchtower-${VERSION}.tgz" "oci://${CHART_REPOSITORY}" - name: Create GitHub release env: diff --git a/Dockerfile b/Dockerfile index 75cfc407..89ad1285 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,7 +1,8 @@ -# Watchtower ships in this image as a second entrypoint: the deploy/watchtower -# chart runs the same image with `command: ["/watchtower"]`. Its binary embeds a -# Next.js static export, so it cannot be rebuilt from Go source here — lift the -# binary out of the published Watchtower image instead. +# Watchtower ships in this image as a second entrypoint: the operator synthesizes +# an Application that runs the same image with `command: ["/watchtower"]`, and +# finds this image by name through OPERATOR_IMAGE. Its binary embeds a Next.js +# static export, so it cannot be rebuilt from Go source here — lift the binary out +# of the published Watchtower image instead. ARG WATCHTOWER_IMAGE=us-docker.pkg.dev/wandb-production/public/wandb/watchtower ARG WATCHTOWER_VERSION=0.11.0 diff --git a/api/v2/weightsandbiases_types.go b/api/v2/weightsandbiases_types.go index 1655793f..1c148cdf 100644 --- a/api/v2/weightsandbiases_types.go +++ b/api/v2/weightsandbiases_types.go @@ -17,6 +17,8 @@ limitations under the License. package v2 import ( + "strings" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -138,8 +140,63 @@ type WeightsAndBiasesSpec struct { // Networking configures how the W&B application is exposed externally. // +optional Networking NetworkingSpec `json:"networking,omitempty"` + + Watchtower WatchtowerSpec `json:"watchtower,omitempty"` +} + +type WatchtowerSpec struct { + Install *bool `json:"install,omitempty"` + Image WatchtowerImageSpec `json:"image,omitempty"` + BasePath string `json:"basePath,omitempty"` + AuthService string `json:"authService,omitempty"` + Resources corev1.ResourceRequirements `json:"resources,omitempty"` + ServiceAccount ManagedServiceAccountSpec `json:"serviceAccount,omitempty"` +} + +type WatchtowerImageSpec struct { + // +optional + Repository string `json:"repository,omitempty"` + // +optional + Tag string `json:"tag,omitempty"` + // +optional + Digest string `json:"digest,omitempty"` +} + +func (s WatchtowerSpec) ResolvedBasePath() string { + basePath := s.BasePath + if basePath == "" { + basePath = DefaultWatchtowerBasePath + } + if !strings.HasPrefix(basePath, "/") { + basePath = "/" + basePath + } + return strings.TrimSuffix(basePath, "/") } +// GetImage returns an explicitly configured Watchtower image, or "" when none is +// set. Empty is the normal case. Binary shisp inside the operator's own image +func (s WatchtowerSpec) GetImage(globalImageRegistry string) string { + if s.Image.Repository == "" { + return "" + } + repository := s.Image.Repository + if globalImageRegistry != "" { + repository = globalImageRegistry + "/" + repository + } + if s.Image.Digest != "" { + return repository + "@" + s.Image.Digest + } + if s.Image.Tag != "" { + return repository + ":" + s.Image.Tag + } + return repository +} + +const ( + DefaultWatchtowerBasePath = "/watchtower" + DefaultWatchtowerServiceAccountName = "wandb-watchtower" +) + // GlobalSpec holds settings shared across every managed component. type GlobalSpec struct { // ImageRegistry, when set, retargets the container images to this registry. @@ -169,6 +226,10 @@ type GlobalSpec struct { Proxy *ProxySpec `json:"proxy,omitempty"` } +func (w *WeightsAndBiases) WatchtowerEnabled() bool { + return w.Spec.Watchtower.Install != nil && *w.Spec.Watchtower.Install +} + // ProxySpec is the forward-proxy configuration under spec.global.proxy. type ProxySpec struct { // HTTPProxy is the proxy URL for plain HTTP egress (HTTP_PROXY/http_proxy). @@ -755,7 +816,15 @@ type WeightsAndBiasesStatus struct { // +optional GatewayStatus *GatewayStatusSummary `json:"gatewayStatus,omitempty"` // +optional - IngressStatus *IngressStatusSummary `json:"ingressStatus,omitempty"` + IngressStatus *IngressStatusSummary `json:"ingressStatus,omitempty"` + WatchtowerStatus *WatchtowerStatusSummary `json:"watchtowerStatus,omitempty"` +} + +type WatchtowerStatusSummary struct { + Ready bool `json:"ready"` + URL string `json:"url,omitempty"` + Image string `json:"image,omitempty"` + AuthService string `json:"authService,omitempty"` } type GatewayStatusSummary struct { @@ -768,6 +837,7 @@ type GatewayStatusSummary struct { type IngressStatusSummary struct { Name string `json:"name,omitempty"` LoadBalancerIngress []corev1.LoadBalancerIngress `json:"loadBalancerIngress,omitempty"` + Ready bool `json:"ready"` } type WandbStatus struct { diff --git a/api/v2/zz_generated.deepcopy.go b/api/v2/zz_generated.deepcopy.go index 76ab543b..8195f562 100644 --- a/api/v2/zz_generated.deepcopy.go +++ b/api/v2/zz_generated.deepcopy.go @@ -1620,6 +1620,59 @@ func (in *WandbStatus) DeepCopy() *WandbStatus { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *WatchtowerImageSpec) DeepCopyInto(out *WatchtowerImageSpec) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new WatchtowerImageSpec. +func (in *WatchtowerImageSpec) DeepCopy() *WatchtowerImageSpec { + if in == nil { + return nil + } + out := new(WatchtowerImageSpec) + in.DeepCopyInto(out) + return out +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *WatchtowerSpec) DeepCopyInto(out *WatchtowerSpec) { + *out = *in + if in.Install != nil { + in, out := &in.Install, &out.Install + *out = new(bool) + **out = **in + } + out.Image = in.Image + in.Resources.DeepCopyInto(&out.Resources) + in.ServiceAccount.DeepCopyInto(&out.ServiceAccount) +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new WatchtowerSpec. +func (in *WatchtowerSpec) DeepCopy() *WatchtowerSpec { + if in == nil { + return nil + } + out := new(WatchtowerSpec) + in.DeepCopyInto(out) + return out +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *WatchtowerStatusSummary) DeepCopyInto(out *WatchtowerStatusSummary) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new WatchtowerStatusSummary. +func (in *WatchtowerStatusSummary) DeepCopy() *WatchtowerStatusSummary { + if in == nil { + return nil + } + out := new(WatchtowerStatusSummary) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *WeightsAndBiases) DeepCopyInto(out *WeightsAndBiases) { *out = *in @@ -1731,6 +1784,7 @@ func (in *WeightsAndBiasesSpec) DeepCopyInto(out *WeightsAndBiasesSpec) { } } in.Networking.DeepCopyInto(&out.Networking) + in.Watchtower.DeepCopyInto(&out.Watchtower) } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new WeightsAndBiasesSpec. @@ -1801,6 +1855,11 @@ func (in *WeightsAndBiasesStatus) DeepCopyInto(out *WeightsAndBiasesStatus) { *out = new(IngressStatusSummary) (*in).DeepCopyInto(*out) } + if in.WatchtowerStatus != nil { + in, out := &in.WatchtowerStatus, &out.WatchtowerStatus + *out = new(WatchtowerStatusSummary) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new WeightsAndBiasesStatus. diff --git a/config/crd/bases/apps.wandb.com_weightsandbiases.yaml b/config/crd/bases/apps.wandb.com_weightsandbiases.yaml index 4ee11815..2b344bda 100644 --- a/config/crd/bases/apps.wandb.com_weightsandbiases.yaml +++ b/config/crd/bases/apps.wandb.com_weightsandbiases.yaml @@ -4405,6 +4405,68 @@ spec: - hostname - version type: object + watchtower: + properties: + authService: + type: string + basePath: + type: string + image: + properties: + digest: + type: string + repository: + type: string + tag: + type: string + type: object + install: + type: boolean + resources: + properties: + claims: + items: + properties: + name: + type: string + request: + type: string + required: + - name + type: object + type: array + x-kubernetes-list-map-keys: + - name + x-kubernetes-list-type: map + limits: + additionalProperties: + anyOf: + - type: integer + - type: string + pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ + x-kubernetes-int-or-string: true + type: object + requests: + additionalProperties: + anyOf: + - type: integer + - type: string + pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ + x-kubernetes-int-or-string: true + type: object + type: object + serviceAccount: + properties: + annotations: + additionalProperties: + type: string + type: object + create: + type: boolean + serviceAccountName: + type: string + type: object + type: object required: - retentionPolicy type: object @@ -4659,6 +4721,10 @@ spec: type: array name: type: string + ready: + type: boolean + required: + - ready type: object kafkaStatus: properties: @@ -6356,6 +6422,19 @@ spec: required: - hostname type: object + watchtowerStatus: + properties: + authService: + type: string + image: + type: string + ready: + type: boolean + url: + type: string + required: + - ready + type: object required: - observedGeneration - ready diff --git a/deploy/operator/templates/_helpers.tpl b/deploy/operator/templates/_helpers.tpl index b39d2ee8..06d67e9c 100644 --- a/deploy/operator/templates/_helpers.tpl +++ b/deploy/operator/templates/_helpers.tpl @@ -79,3 +79,8 @@ wandb-operator values. Each is inert unless a CA source is configured. value: "{{ $ca.mountPath | default "/etc/wandb/ca-certs" }}:/etc/ssl/certs:/etc/pki/tls/certs" {{- end -}} {{- end -}} + +{{- define "wandb-operator.operatorImageEnv" -}} +- name: OPERATOR_IMAGE + value: "{{ .Values.image.repository }}:{{ .Values.image.tag }}" +{{- end -}} diff --git a/deploy/operator/values.yaml b/deploy/operator/values.yaml index 3da4589e..4b31741b 100644 --- a/deploy/operator/values.yaml +++ b/deploy/operator/values.yaml @@ -86,6 +86,7 @@ wandb-operator: - '{{ include "wandb-operator.caCertsVolume" . }}' envTpls: - '{{ include "wandb-operator.caCertsEnv" . }}' + - '{{ include "wandb-operator.operatorImageEnv" . }}' service: enabled: true diff --git a/deploy/watchtower/Chart.yaml b/deploy/watchtower/Chart.yaml deleted file mode 100644 index e85a2f5b..00000000 --- a/deploy/watchtower/Chart.yaml +++ /dev/null @@ -1,13 +0,0 @@ -apiVersion: v2 -name: watchtower -description: A Helm chart for the W&B Watchtower cluster administration UI -type: application -# Versioned in lockstep with the operator: Watchtower's binary ships inside the -# operator image, so the chart version doubles as the image tag it deploys. The -# release pipeline enforces chart version == appVersion == operator release. -version: 2.0.0-beta.3 -appVersion: "2.0.0-beta.3" -maintainers: - - name: wandb - email: support@wandb.com - url: https://wandb.com diff --git a/deploy/watchtower/templates/NOTES.txt b/deploy/watchtower/templates/NOTES.txt deleted file mode 100644 index ad046e36..00000000 --- a/deploy/watchtower/templates/NOTES.txt +++ /dev/null @@ -1,26 +0,0 @@ -Watchtower is installed as {{ include "watchtower.fullname" . }} in namespace {{ .Release.Namespace }}. - -It is published on a node port rather than through the W&B Ingress, so it is -reachable at: - -{{- if eq .Values.service.type "NodePort" }} - - http://:$(kubectl get svc -n {{ .Release.Namespace }} {{ include "watchtower.fullname" . }} -o jsonpath='{.spec.ports[0].nodePort}'){{ include "watchtower.basePath" . }}/ - -Every node publishes that port. Reaching it from the public internet needs the -port open in the node firewall / security group; nothing in this chart opens it. -{{- else }} - - a {{ .Values.service.type }} Service on port {{ .Values.service.port }}, path {{ include "watchtower.basePath" . }}/ -{{- end }} - -{{ if eq .Values.mode "cluster" }} -Log in with the admin password: - - kubectl get secret -n {{ .Release.Namespace }} {{ include "watchtower.authSecretName" . }} \ - -o jsonpath='{.data.{{ .Values.auth.secretKey }}}' | base64 -d - -{{- else }} -WARNING: mode is {{ .Values.mode | quote }}, not "cluster" — the password gate is -OFF and anyone who can reach the port has full cluster administration. -{{- end }} diff --git a/deploy/watchtower/templates/_helpers.tpl b/deploy/watchtower/templates/_helpers.tpl deleted file mode 100644 index bf4bf7eb..00000000 --- a/deploy/watchtower/templates/_helpers.tpl +++ /dev/null @@ -1,94 +0,0 @@ -{{- define "watchtower.name" -}} -{{- default .Chart.Name .Values.nameOverride | trunc 63 | trimSuffix "-" -}} -{{- end -}} - -{{- define "watchtower.fullname" -}} -{{- if .Values.fullnameOverride -}} -{{- .Values.fullnameOverride | trunc 63 | trimSuffix "-" -}} -{{- else -}} -{{- $name := default .Chart.Name .Values.nameOverride -}} -{{- if contains $name .Release.Name -}} -{{- .Release.Name | trunc 63 | trimSuffix "-" -}} -{{- else -}} -{{- printf "%s-%s" .Release.Name $name | trunc 63 | trimSuffix "-" -}} -{{- end -}} -{{- end -}} -{{- end -}} - -{{- define "watchtower.labels" -}} -helm.sh/chart: {{ printf "%s-%s" .Chart.Name .Chart.Version | replace "+" "_" | trunc 63 | trimSuffix "-" }} -{{ include "watchtower.selectorLabels" . }} -app.kubernetes.io/version: {{ .Chart.AppVersion | quote }} -app.kubernetes.io/managed-by: {{ .Release.Service }} -{{- end -}} - -{{- define "watchtower.selectorLabels" -}} -app.kubernetes.io/name: {{ include "watchtower.name" . }} -app.kubernetes.io/instance: {{ .Release.Name }} -{{- end -}} - -{{- define "watchtower.serviceAccountName" -}} -{{- if .Values.serviceAccount.create -}} -{{- default (include "watchtower.fullname" .) .Values.serviceAccount.name -}} -{{- else -}} -{{- default "default" .Values.serviceAccount.name -}} -{{- end -}} -{{- end -}} - -{{/* -The image is the operator's, not Watchtower's. Falling back to .Chart.AppVersion -is safe because this chart is versioned in lockstep with the operator release, so -the two are the same string by construction. -*/}} -{{- define "watchtower.image" -}} -{{- if .Values.image.digest -}} -{{- printf "%s@%s" .Values.image.repository .Values.image.digest -}} -{{- else -}} -{{- printf "%s:%s" .Values.image.repository (default .Chart.AppVersion .Values.image.tag) -}} -{{- end -}} -{{- end -}} - -{{/* -Normalizes basePath the same way the Watchtower binary does: "" or a "/"-prefixed -path with no trailing slash. Probe paths and the published URL are built from it, -so a values file writing "watchtower/" must not produce "//healthz". -*/}} -{{- define "watchtower.basePath" -}} -{{- $path := default "" .Values.basePath -}} -{{- if $path -}} -{{- if not (hasPrefix "/" $path) -}}{{- $path = printf "/%s" $path -}}{{- end -}} -{{- trimSuffix "/" $path -}} -{{- end -}} -{{- end -}} - -{{- define "watchtower.wandbName" -}} -{{- default .Release.Name .Values.wandbName -}} -{{- end -}} - -{{/* -ClusterRole/ClusterRoleBinding names are cluster-global, so two Watchtower -releases in different namespaces would otherwise fight over one object — the -second install silently adopting the first's rules and subject list. Qualify the -name with the namespace; namespaced Roles keep the plain fullname. -*/}} -{{- define "watchtower.roleName" -}} -{{- if eq .Values.role.type "Role" -}} -{{- include "watchtower.fullname" . -}} -{{- else -}} -{{- printf "%s-%s" .Release.Namespace (include "watchtower.fullname" .) | trunc 63 | trimSuffix "-" -}} -{{- end -}} -{{- end -}} - -{{/* -A user-managed Secret wins over the generated one; refusing to guess when neither -is available beats rendering a Deployment that CrashLoopBackOffs on a missing key. -*/}} -{{- define "watchtower.authSecretName" -}} -{{- if .Values.auth.existingSecret -}} -{{- .Values.auth.existingSecret -}} -{{- else if .Values.auth.create -}} -{{- printf "%s-auth" (include "watchtower.fullname" .) -}} -{{- else -}} -{{- fail "watchtower: set auth.create=true to generate an admin password, or auth.existingSecret to supply one" -}} -{{- end -}} -{{- end -}} diff --git a/deploy/watchtower/templates/deployment.yaml b/deploy/watchtower/templates/deployment.yaml deleted file mode 100644 index 52affdfd..00000000 --- a/deploy/watchtower/templates/deployment.yaml +++ /dev/null @@ -1,134 +0,0 @@ -{{- $basePath := include "watchtower.basePath" . }} -apiVersion: apps/v1 -kind: Deployment -metadata: - name: {{ include "watchtower.fullname" . }} - namespace: {{ .Release.Namespace }} - labels: - {{- include "watchtower.labels" . | nindent 4 }} -spec: - # Deliberately not configurable: in-flight deploy jobs and their SSE streams - # live in the serving pod's memory, so a reconnect landing on a second pod - # would see no history. - replicas: 1 - selector: - matchLabels: - {{- include "watchtower.selectorLabels" . | nindent 6 }} - template: - metadata: - labels: - {{- include "watchtower.selectorLabels" . | nindent 8 }} - {{- with .Values.podLabels }} - {{- toYaml . | nindent 8 }} - {{- end }} - {{- with .Values.podAnnotations }} - annotations: - {{- toYaml . | nindent 8 }} - {{- end }} - spec: - serviceAccountName: {{ include "watchtower.serviceAccountName" . }} - {{- with .Values.imagePullSecrets }} - imagePullSecrets: - {{- toYaml . | nindent 8 }} - {{- end }} - {{- with .Values.podSecurityContext }} - securityContext: - {{- toYaml . | nindent 8 }} - {{- end }} - containers: - - name: watchtower - image: {{ include "watchtower.image" . }} - imagePullPolicy: {{ .Values.image.pullPolicy }} - # The operator image entrypoint is /manager; this selects the Watchtower - # binary that ships alongside it. - command: - {{- toYaml .Values.command | nindent 12 }} - args: - - --port - - {{ .Values.containerPort | quote }} - env: - - name: WATCHTOWER_MODE - value: {{ .Values.mode | quote }} - - name: WATCHTOWER_BASE_PATH - value: {{ $basePath | quote }} - {{- if eq .Values.mode "cluster" }} - # Never inlined: an env value here would be readable from the pod spec - # by anyone who can `kubectl get deployment`. - - name: WATCHTOWER_PASSWORD - valueFrom: - secretKeyRef: - name: {{ include "watchtower.authSecretName" . }} - key: {{ .Values.auth.secretKey }} - {{- end }} - - name: WATCHTOWER_WANDB_NAME - value: {{ include "watchtower.wandbName" . | quote }} - - name: WATCHTOWER_NAMESPACE - valueFrom: - fieldRef: - fieldPath: metadata.namespace - {{- range $name, $value := .Values.env }} - - name: {{ $name }} - value: {{ $value | quote }} - {{- end }} - ports: - - name: http - containerPort: {{ .Values.containerPort }} - protocol: TCP - # Health routes sit outside the auth gate but inside the base path, so - # the probes have to carry the prefix too. - livenessProbe: - httpGet: - path: {{ $basePath }}/healthz - port: http - initialDelaySeconds: 15 - periodSeconds: 20 - readinessProbe: - httpGet: - path: {{ $basePath }}/ready - port: http - initialDelaySeconds: 5 - periodSeconds: 10 - {{- with .Values.securityContext }} - securityContext: - {{- toYaml . | nindent 12 }} - {{- end }} - {{- with .Values.resources }} - resources: - {{- toYaml . | nindent 12 }} - {{- end }} - # Watchtower drives Helm and writes the air-gapped dependency bundle - # relative to its working directory, neither of which the read-only - # root filesystem allows. - workingDir: /home/watchtower - volumeMounts: - - name: home - mountPath: /home/watchtower - - name: helm - mountPath: /helm - - name: tmp - mountPath: /tmp - {{- with .Values.extraVolumeMounts }} - {{- toYaml . | nindent 12 }} - {{- end }} - volumes: - - name: home - emptyDir: {} - - name: helm - emptyDir: {} - - name: tmp - emptyDir: {} - {{- with .Values.extraVolumes }} - {{- toYaml . | nindent 8 }} - {{- end }} - {{- with .Values.nodeSelector }} - nodeSelector: - {{- toYaml . | nindent 8 }} - {{- end }} - {{- with .Values.tolerations }} - tolerations: - {{- toYaml . | nindent 8 }} - {{- end }} - {{- with .Values.affinity }} - affinity: - {{- toYaml . | nindent 8 }} - {{- end }} diff --git a/deploy/watchtower/templates/role.yaml b/deploy/watchtower/templates/role.yaml deleted file mode 100644 index 85a951a2..00000000 --- a/deploy/watchtower/templates/role.yaml +++ /dev/null @@ -1,31 +0,0 @@ -{{- if .Values.role.create }} -apiVersion: rbac.authorization.k8s.io/v1 -kind: {{ .Values.role.type }} -metadata: - name: {{ include "watchtower.roleName" . }} - {{- if eq .Values.role.type "Role" }} - namespace: {{ .Release.Namespace }} - {{- end }} - labels: - {{- include "watchtower.labels" . | nindent 4 }} -rules: - {{- toYaml .Values.role.rules | nindent 2 }} ---- -apiVersion: rbac.authorization.k8s.io/v1 -kind: {{ ternary "RoleBinding" "ClusterRoleBinding" (eq .Values.role.type "Role") }} -metadata: - name: {{ include "watchtower.roleName" . }} - {{- if eq .Values.role.type "Role" }} - namespace: {{ .Release.Namespace }} - {{- end }} - labels: - {{- include "watchtower.labels" . | nindent 4 }} -subjects: - - kind: ServiceAccount - name: {{ include "watchtower.serviceAccountName" . }} - namespace: {{ .Release.Namespace }} -roleRef: - kind: {{ .Values.role.type }} - name: {{ include "watchtower.roleName" . }} - apiGroup: rbac.authorization.k8s.io -{{- end }} diff --git a/deploy/watchtower/templates/secret.yaml b/deploy/watchtower/templates/secret.yaml deleted file mode 100644 index c6b3b969..00000000 --- a/deploy/watchtower/templates/secret.yaml +++ /dev/null @@ -1,28 +0,0 @@ -{{- if .Values.auth.create }} -{{- $name := printf "%s-auth" (include "watchtower.fullname" .) }} -{{/* -Reuse the password already in the cluster. Without this lookup a plain -randAlphaNum re-rolls on every `helm upgrade`, silently locking the admin out of -an install that was working a moment earlier. lookup returns nothing under -`helm template` and `--dry-run`, so rendered output there will differ from a real -install — that is expected, not a bug. -*/}} -{{- $existing := lookup "v1" "Secret" .Release.Namespace $name }} -{{- $password := .Values.auth.password }} -{{- if and (not $password) $existing }} -{{- $password = index $existing.data "password" | b64dec }} -{{- end }} -{{- if not $password }} -{{- $password = randAlphaNum 32 }} -{{- end }} -apiVersion: v1 -kind: Secret -metadata: - name: {{ $name }} - namespace: {{ .Release.Namespace }} - labels: - {{- include "watchtower.labels" . | nindent 4 }} -type: Opaque -stringData: - password: {{ $password | quote }} -{{- end }} diff --git a/deploy/watchtower/templates/service.yaml b/deploy/watchtower/templates/service.yaml deleted file mode 100644 index c01fca18..00000000 --- a/deploy/watchtower/templates/service.yaml +++ /dev/null @@ -1,26 +0,0 @@ -apiVersion: v1 -kind: Service -metadata: - name: {{ include "watchtower.fullname" . }} - namespace: {{ .Release.Namespace }} - labels: - {{- include "watchtower.labels" . | nindent 4 }} - {{- with .Values.service.labels }} - {{- toYaml . | nindent 4 }} - {{- end }} - {{- with .Values.service.annotations }} - annotations: - {{- toYaml . | nindent 4 }} - {{- end }} -spec: - type: {{ .Values.service.type }} - ports: - - name: http - port: {{ .Values.service.port }} - targetPort: http - protocol: TCP - {{- if and .Values.service.nodePort (eq .Values.service.type "NodePort") }} - nodePort: {{ .Values.service.nodePort }} - {{- end }} - selector: - {{- include "watchtower.selectorLabels" . | nindent 4 }} diff --git a/deploy/watchtower/templates/serviceaccount.yaml b/deploy/watchtower/templates/serviceaccount.yaml deleted file mode 100644 index 098feaa6..00000000 --- a/deploy/watchtower/templates/serviceaccount.yaml +++ /dev/null @@ -1,16 +0,0 @@ -{{- if .Values.serviceAccount.create }} -apiVersion: v1 -kind: ServiceAccount -metadata: - name: {{ include "watchtower.serviceAccountName" . }} - namespace: {{ .Release.Namespace }} - labels: - {{- include "watchtower.labels" . | nindent 4 }} - {{- with .Values.serviceAccount.annotations }} - annotations: - {{- toYaml . | nindent 4 }} - {{- end }} -# Unlike the W&B application pods, Watchtower calls the Kubernetes API on the -# user's behalf, so it needs its token projected. -automountServiceAccountToken: {{ .Values.serviceAccount.automount }} -{{- end }} diff --git a/deploy/watchtower/values.yaml b/deploy/watchtower/values.yaml deleted file mode 100644 index dcab09f0..00000000 --- a/deploy/watchtower/values.yaml +++ /dev/null @@ -1,145 +0,0 @@ -nameOverride: "" -fullnameOverride: "" - -# Watchtower runs from the operator image — its binary is copied in at build time -# as a second entrypoint (see the repository Dockerfile). Keeping the two in one -# image means one artifact to mirror for air-gapped installs. -# -# This is the chart's only tie to the operator: the two install as independent -# releases, and Watchtower needs no operator running to come up. The tag is the -# operator's version, not Watchtower's, and it tracks this chart's version — the -# release pipeline holds them equal. Digest wins when both are set. -image: - repository: us-docker.pkg.dev/wandb-production/public/wandb/operator - tag: "" - digest: "" - pullPolicy: IfNotPresent -imagePullSecrets: [] - -replicaCount: 1 - -# The command that selects Watchtower rather than the operator manager. -command: - - /watchtower - -# URL prefix Watchtower serves under. It must match the prefix the image was -# built for: Next.js bakes basePath into every asset URL and router href at build -# time, and the binary refuses to start when its runtime value disagrees with the -# compiled-in one. The published Watchtower image is built with /watchtower, so -# only change this alongside an image built with a matching NEXT_PUBLIC_BASE_PATH. -basePath: /watchtower - -containerPort: 8080 - -# Admin login. Watchtower does not implement OIDC and does not share the W&B -# app's session — published on its own origin, that cookie never reaches it. A -# single admin password gates the UI instead, and in cluster mode the binary -# refuses to start without one rather than serve cluster administration -# unauthenticated. -auth: - # Generate the password into a Secret named -auth on first install. - # Existing values are reused on upgrade, so the password is stable. - create: true - # Pin a specific password instead of generating one. Prefer existingSecret for - # anything real — a value here lands in the Helm release history. - password: "" - # Use a Secret you manage yourself. Takes precedence over create. - existingSecret: "" - secretKey: password - -# Locks the UI to the cluster it runs in — no context switching, no teardown — -# and turns on the password gate. Only set this to "web" against a sandbox you -# do not care about: it disables the gate entirely. -mode: cluster - -# Name of the WeightsAndBiases CR this Watchtower administers. Defaults to the -# release name when empty. -wandbName: "" - -serviceAccount: - create: true - automount: true - name: "" - annotations: {} - -role: - create: true - # "Role" scopes the grant to the release namespace; "ClusterRole" widens it to - # every namespace, which Watchtower only needs when it manages installs outside - # its own. - type: ClusterRole - rules: - - apiGroups: - - apps.wandb.com - resources: - - weightsandbiases - - weightsandbiases/status - - applications - - applications/status - - applications/ - verbs: - - get - - list - - watch - - create - - update - - patch - - delete - - apiGroups: - - "" - resources: - - secrets - verbs: - - get - - list - - watch - - create - - update - - patch - - delete - -# Watchtower's own Go HTTP server is the public entry point — there is no Ingress -# and no reverse proxy in front of it, so it is reached on a port published by -# every node. Leave nodePort empty to let Kubernetes allocate one from -# --service-node-port-range. -service: - type: NodePort - port: 8080 - nodePort: "" - annotations: {} - labels: {} - -resources: - requests: - cpu: 100m - memory: 256Mi - limits: - cpu: 500m - memory: 512Mi - -podAnnotations: {} -podLabels: {} - -podSecurityContext: - runAsNonRoot: true - runAsUser: 65532 - runAsGroup: 65532 - fsGroup: 65532 - fsGroupChangePolicy: OnRootMismatch - seccompProfile: - type: RuntimeDefault - -securityContext: - allowPrivilegeEscalation: false - readOnlyRootFilesystem: true - capabilities: - drop: - - ALL - -env: {} -extraVolumes: [] -extraVolumeMounts: [] - -nodeSelector: {} -tolerations: [] -affinity: {} diff --git a/docs/watchtower-deployment.md b/docs/watchtower-deployment.md index 6f5b08c1..64a80b12 100644 --- a/docs/watchtower-deployment.md +++ b/docs/watchtower-deployment.md @@ -1,8 +1,9 @@ # Deploying Watchtower [Watchtower](https://github.com/wandb/watchtower) is the cluster administration UI -that replaces the deprecated W&B console. This document covers how it is packaged -and installed alongside the operator. +that replaces the deprecated W&B console. The operator deploys it as an +operator-owned component — there is no Watchtower chart, and nothing to install +separately. ## Packaging: one image, two entrypoints @@ -29,141 +30,212 @@ Pin a different release with: make docker-build WATCHTOWER_VERSION=0.12.0 ``` -Selecting which of the two binaries runs is the container's `command`: the image -`ENTRYPOINT` stays `/manager`, and the Watchtower Deployment overrides it with -`/watchtower`. +The Application the operator synthesizes selects the second entrypoint with +`command: ["/watchtower"]` and `args: ["--port", "8080"]` — the binary's own +default port is 9090, which would not match the Service or the probes. -## Installation: a separate release +### How the operator knows its own image -`deploy/watchtower/` is its own chart and its own Helm release. It is **not** a -dependency of `deploy/operator` — installing Watchtower does not install the -operator, upgrading one does not touch the other, and deleting one leaves the -other running. The only tie between them is the image. +Because the binary lives inside the operator image, the reconciler has to name +that image when it builds the Application. The operator chart passes it in: -```bash -helm install watchtower oci://us-docker.pkg.dev/wandb-production/charts/watchtower \ - --version 2.0.0-beta.3 -n wandb --create-namespace +```gotemplate +{{- define "wandb-operator.operatorImageEnv" -}} +- name: OPERATOR_IMAGE + value: "{{ .Values.image.repository }}:{{ .Values.image.tag }}" +{{- end -}} ``` -Both charts are published from this repo by the same release workflow, and the -version check there holds the watchtower chart version, its appVersion, and the -operator image tag equal to the release. So `--version 2.0.0-beta.3` deploys the -Watchtower binary from operator image `2.0.0-beta.3` with no second version to -track — which is also why `image.tag` can be left empty and defaults to the -chart's appVersion. - -It creates: +wired through `wandb-operator.envTpls` in `deploy/operator/values.yaml`, and read +by `watchtowerImage()`. Resolving it at runtime is deliberate: a default tag +compiled into the operator would drift on every release, so a 2.1.0 operator +would quietly deploy a 2.0.0 Watchtower. `spec.watchtower.image` overrides it when +set; leaving it empty is the normal case. -| Resource | Purpose | -|----------|---------| -| `Deployment` | The operator image run as `/watchtower --port 8080` | -| `Service` | `NodePort`, publishing the Go HTTP server directly | -| `ServiceAccount` | With its token projected — Watchtower calls the Kubernetes API | -| `Role` + `RoleBinding` | Write access to the W&B CRs and to secrets | -| `Secret` | The generated admin password (see Authentication below) | +If `OPERATOR_IMAGE` is unset the reconciler fails loudly rather than guessing. -A minimal values file: +## Configuration ```yaml -service: - nodePort: 32080 # omit to let Kubernetes allocate one -role: - type: ClusterRole # Role scopes Watchtower to its own namespace +apiVersion: apps.wandb.com/v2 +kind: WeightsAndBiases +spec: + watchtower: + install: true # opt-in; defaults to false + basePath: /watchtower # published route and container base path + authService: "" # empty = derive from the server manifest + resources: {} # defaults to 100m / 256Mi requests + image: {} # empty = the operator's own image + serviceAccount: + create: true +``` + +`install` defaults to **false**. Watchtower grants cluster-wide access to anyone +holding an admin credential, so turning it on is an explicit decision. + +## What the operator creates + +| Resource | Name | Notes | +|----------|------|-------| +| `Application` | `-watchtower` | `Kind: Deployment`, `replicas: 1`, labelled `weightsandbiases.apps.wandb.com/component=watchtower` so manifest-driven pruning skips it | +| `Service` | `-watchtower` | ClusterIP `8080`, derived from the Application by the application controller | +| `ServiceAccount` | `-watchtower` | Token automounted — unlike the W&B app pods, Watchtower calls the Kubernetes API | +| `Secret` | `-watchtower-auth` | The fallback admin password (see Authentication) | +| `Role` / `RoleBinding` | `-watchtower` | Namespaced reads | +| `ClusterRole` / `ClusterRoleBinding` | `--watchtower` | Cluster-wide reads plus `weightsandbiases` `update`/`patch` | +| Ingress path | `` on the consolidated Ingress | Ingress mode | +| `HTTPRoute` | via `Application.spec.httpRouteTemplate` | Gateway API mode, same hostnames as the app | + +`replicas` is deliberately **not** configurable: in-flight deploy jobs and their +SSE streams live in the serving pod's memory, so a reconnect landing on a second +pod would see no history. + +Status lands in `status.watchtowerStatus` (`ready`, `url`, `image`, +`authService`). Watchtower never contributes to the CR's `Ready` condition. + +### Naming and multiple installs + +Every namespaced object is named from the CR, and cluster-scoped RBAC is +additionally qualified with the namespace, so two `WeightsAndBiases` CRs can +coexist in one namespace *and* in one cluster without sharing an Application, +RBAC binding or Secret. + +Names go through `common.FitDefaultInfraName`, which hashes the CR name when the +derived name would exceed the 63-character DNS-1123 label budget. The budget is a +label rather than a subdomain because the application controller derives a Service +from the Application name. Plain truncation would be wrong here: two CR names +differing only past the cutoff would collapse onto one object. + +## Reconcile ordering + +Watchtower is reconciled inside `ReconcileWandbManifest`, in this order: + ``` +cleanupNetworkingModeResources + resetInactiveNetworkingStatus +gateway block (NetworkingModeGatewayAPI) +ingress block (NetworkingModeIngress) +reconcileWatchtower +─── infrastructure readiness gate ────────────── +migrations, applications, … +``` + +Two properties this buys, both deliberate: + +**Networking and Watchtower sit above the infrastructure gate.** Watchtower exists +to diagnose a broken install, so it has to come up when MySQL, Redis, Kafka, the +object store or ClickHouse are *not* ready — which is when the reconcile returns +early. Its route has to be published for the same reason, so the networking +reconcile moved up with it. + +Moving the Ingress reconcile above the gate means infra Services may not exist +yet. `resolveInfraRoutes` treats a missing Service as "skip this route" rather +than an error — a route to a Service that is not there is not publishable, and it +gets picked up on a later pass. -`replicas` is deliberately fixed at 1 and not exposed: in-flight deploy jobs and -their SSE streams live in the serving pod's memory, so a reconnect landing on a -second pod would see no history. +**A Watchtower failure never blocks the W&B install.** `reconcileWatchtower`'s +error is logged and stepped over, not returned. A bad image or an RBAC mistake +must not stop infra, migrations and applications from reconciling. Nothing +downstream reads a Watchtower readiness signal. -## Routing: a node port, not an Ingress +One cosmetic consequence: on the first pass after enabling Watchtower, the Ingress +publishes `` before the Service exists, so the route 503s for one +reconcile cycle. -Watchtower's own Go HTTP server is the public entry point. There is no Ingress -path on the W&B hostname and no reverse proxy in front of it — the `NodePort` -Service publishes the port on every node, and reaching it from the public -internet is a matter of opening that port in the node firewall or security group. -Nothing in this chart opens it. +## Routing -Set `service.type` to `ClusterIP` for an internal-only install, or to -`LoadBalancer` to get a dedicated address instead. +Watchtower is served from the W&B app's own hostname at ``, not on a +separate port or hostname. That is what makes the browser send the app's session +cookie to it, which is what the OIDC auth path depends on. + +`basePath` defaults to `/watchtower` and is validated to be non-root — `/` is the +W&B frontend's own path, and mounting Watchtower there would shadow the app it +manages. ### The base path is a build-time value -`basePath` defaults to `/watchtower` and cannot be changed on its own. Next.js -bakes `basePath` into every asset URL and router href at build time, and the -Watchtower binary refuses to start when its runtime base path disagrees with the -compiled-in one — so serving at the root requires a Watchtower image built with -`BASE_PATH=` empty, and `watchtower.basePath: ""` to match. The published image -is built with `/watchtower`. +`basePath` cannot be changed on its own. Next.js bakes `basePath` into every asset +URL and router href at build time, and the Watchtower binary refuses to start when +its runtime base path disagrees with the compiled-in one. Changing it requires a +Watchtower image built with a matching `NEXT_PUBLIC_BASE_PATH`; the published +image is built with `/watchtower`. The health probes carry the prefix for the same reason: `{basePath}/healthz` and -`{basePath}/ready`, which sit outside the auth gate (the kubelet sends no cookie) -but inside the base path. +`{basePath}/ready`, which sit outside the auth gate — the kubelet holds no +credential — but inside the base path. -## RBAC +## Authentication -The chart's `Role` grants exactly what Watchtower needs to manage an install: +Two independent credentials, either of which grants access. This mirrors what +console did on-prem: an app session *or* a root password. -- `apps.wandb.com` — `weightsandbiases`, `applications` and their `/status` - subresources, full verbs -- core `secrets`, full verbs +**The W&B app session.** Watchtower forwards the caller's `Cookie` and +`Authorization` headers to `GET http://$WATCHTOWER_AUTH_SERVICE/oidc/auth` and +allows the request when gorilla confirms an admin. It implements no OIDC of its +own. -`role.type` defaults to `ClusterRole`, so Watchtower can manage installs in any -namespace. Set it to `Role` to confine it to its own release namespace. +`spec.watchtower.authService` is normally left empty. The operator finds the +manifest application that owns the `/oidc` ingress path — `api` in current +manifests — and uses `:`, since the +application controller names each Service after its Application. That keeps +working across manifest renames and port changes. If no application declares +`/oidc`, reconciliation **fails** rather than deploying an unauthenticated +Watchtower. -Cluster-scoped names are qualified with the namespace — -`--watchtower` — because `ClusterRole` and -`ClusterRoleBinding` names are cluster-global. Without that, a second Watchtower -release in another namespace would adopt the first one's object and silently -overwrite its rules and subject list. Namespaced `Role`s keep the plain name. +**The generated admin password.** The operator creates +`-watchtower-auth` on first reconcile and injects it as +`WATCHTOWER_PASSWORD` via `secretKeyRef` — never inlined in the pod spec, where +anyone with `kubectl get deployment` could read it. Retrieve it with: -Because the container runs with `readOnlyRootFilesystem: true`, the Deployment -mounts `emptyDir`s at `/home/watchtower` (its working directory, where the -air-gapped dependency bundle lands), `/helm` and `/tmp`. +```bash +kubectl get secret -n wandb -watchtower-auth \ + -o jsonpath='{.data.password}' | base64 -d +``` -## Authentication: a chart-generated admin password +The password is generated once and never rewritten: regenerating on upgrade would +lock the operator's user out of a working install. This is the path that keeps +Watchtower usable when the W&B app itself is down, which is exactly when it is +needed — so the two credentials are not redundant. -Watchtower implements no OIDC and does not share the W&B app's session. That -earlier design only worked because Watchtower was served under the app's -hostname, so the browser sent the app's cookie along; published on its own origin -it never arrives. A single admin password gates the UI instead. +`secretKeyRef` env vars are resolved at pod creation and never refreshed, so +rotation is two steps: -The chart generates it on first install into a Secret named -`-watchtower-auth` and injects it as `WATCHTOWER_PASSWORD` via -`secretKeyRef` — never inlined in the pod spec, where anyone with -`kubectl get deployment` could read it. Retrieve it with: +```bash +kubectl delete secret -n wandb -watchtower-auth # reconcile regenerates it +kubectl rollout restart deployment/-watchtower -n wandb +``` + +## RBAC + +The apiserver refuses to grant permissions the operator does not itself hold, so +two rules are **missing** from the ClusterRole the operator creates: + +- `apiextensions.k8s.io/customresourcedefinitions` `get`/`list` — used to detect + whether v2 is served +- `pods/portforward` — telemetry port-forward + +Both need the operator's own ClusterRole widened first (kubebuilder markers in +`internal/controller/weightsandbiases_controller.go`, then `make manifests`). +Installing or upgrading the operator from inside the pod needs more again, and is +not granted today. + +This constraint is specific to operator-created RBAC. It did not apply when a Helm +chart created these objects, because Helm acts as the installing user. + +## Verifying a deployment + +The pod reaching `1/1 Ready` already confirms a lot: the readiness probe is +`{basePath}/ready`, which only answers 200 once the Kubernetes client has +initialized from the ServiceAccount token. Base path, RBAC and in-cluster +credentials are all proven before you load a page. + +To confirm the running binary is the one you built, compare digests rather than +tags: ```bash -kubectl get secret -n wandb wandb-watchtower-auth \ - -o jsonpath='{.data.password}' | base64 -d +kubectl exec -n wandb deploy/-watchtower -- sha256sum /watchtower +docker run --rm --entrypoint sha256sum /watchtower ``` -The template reads any existing Secret via `lookup` before generating, so -`helm upgrade` preserves the password rather than silently rotating it and -locking the admin out. Two overrides: `auth.existingSecret` to manage the Secret -yourself (preferred for GitOps — a generated password is invisible until someone -reads it), or `auth.password` to pin a value, which lands in the Helm release -history and is best avoided. - -On the wire: `POST /login` checks the password in constant time and -sets an `HttpOnly`, `SameSite=Lax` session cookie scoped to the base path, -holding an expiry signed with an HMAC keyed on the password itself. There is no -server-side session store — Watchtower is a single replica that restarts freely — -and because the key is derived from the password, rotating the Secret invalidates -every outstanding session for free. Sessions last 12 hours. `Secure` is set only -when the request arrived over TLS, since the Service publishes plain HTTP and an -unconditionally-Secure cookie would never be sent back. - -Unauthenticated `/api/v1/*` calls get a JSON 401 so the frontend can render -"session expired"; page loads redirect to the login form. `/healthz` and `/ready` -stay outside the gate — the kubelet holds no session. - -`mode` defaults to `cluster`, which is what turns the gate on. Setting it to -`web` disables authentication entirely; only do that against a sandbox you do not -care about. - -### Still worth doing - -The password is a shared secret with no rate limiting on the login endpoint. A -32-character generated password is not guessable, but a user-chosen -`auth.password` might be — consider a lockout or backoff before this is exposed -broadly, and keep the node port firewalled to known source ranges regardless. +Nodes default to `imagePullPolicy: IfNotPresent` for any tag but `latest`, so a +rebuilt floating tag can leave a node serving the old layer — which looks exactly +like a change that did not land. diff --git a/internal/controller/reconciler/infra_routes.go b/internal/controller/reconciler/infra_routes.go index 13c4faad..d10eb884 100644 --- a/internal/controller/reconciler/infra_routes.go +++ b/internal/controller/reconciler/infra_routes.go @@ -56,9 +56,13 @@ func resolveInfraRoutes(ctx context.Context, c ctrlClient.Client, wandb *apiv2.W } svcName := fmt.Sprintf("%s-s3", objectStoreSpec.Name) port, err := resolveInfraServicePort(ctx, c, types.NamespacedName{Name: svcName, Namespace: objectStoreSpec.Namespace}, cfg.Ingress, 8333) + if apiErrors.IsNotFound(err) { + continue + } if err != nil { return nil, fmt.Errorf("bucket instance %q: %w", instanceName, err) } + entries = append(entries, infraRouteEntry{ name: fmt.Sprintf("%s-bucket-%s", wandb.Name, infraRouteInstanceName(crKey, instanceName)), namespace: objectStoreSpec.Namespace, @@ -90,9 +94,13 @@ func resolveInfraRoutes(ctx context.Context, c ctrlClient.Client, wandb *apiv2.W cfg.Ingress, 8123, ) + if apiErrors.IsNotFound(err) { + continue + } if err != nil { return nil, fmt.Errorf("clickhouse instance %q: %w", instanceName, err) } + entries = append(entries, infraRouteEntry{ name: fmt.Sprintf("%s-clickhouse-%s", wandb.Name, infraRouteInstanceName(crKey, instanceName)), namespace: chSpec.Namespace, diff --git a/internal/controller/reconciler/ingress.go b/internal/controller/reconciler/ingress.go index 75410b57..c67b6f36 100644 --- a/internal/controller/reconciler/ingress.go +++ b/internal/controller/reconciler/ingress.go @@ -91,7 +91,9 @@ func reconcileConsolidatedIngress(ctx context.Context, c ctrlClient.Client, wand }, }) } - + if watchtowerPath := watchtowerIngressPath(wandb); watchtowerPath != nil { + paths = append(paths, *watchtowerPath) + } if len(paths) == 0 { return nil } @@ -170,6 +172,7 @@ func reconcileConsolidatedIngress(ctx context.Context, c ctrlClient.Client, wand return err } wandb.Status.IngressStatus = summarizeIngressStatus(desired) + wandb.Status.IngressStatus.Ready = isIngressReady(current) return nil } return err @@ -180,9 +183,24 @@ func reconcileConsolidatedIngress(ctx context.Context, c ctrlClient.Client, wand return err } wandb.Status.IngressStatus = summarizeIngressStatus(current) + wandb.Status.IngressStatus.Ready = isIngressReady(current) return nil } +// An Ingress has no Ready condition, so readiness is "an address was assigned" +// This status is needed prior to creating watchtower application. +func isIngressReady(ingress *networkingv1.Ingress) bool { + if ingress == nil { + return false + } + for _, lb := range ingress.Status.LoadBalancer.Ingress { + if lb.IP != "" || lb.Hostname != "" { + return true + } + } + return false +} + func deleteConsolidatedIngress(ctx context.Context, c ctrlClient.Client, wandb *apiv2.WeightsAndBiases) error { ingressName := consolidatedIngressName(wandb) ingress := &networkingv1.Ingress{} diff --git a/internal/controller/reconciler/reconcile_v2.go b/internal/controller/reconciler/reconcile_v2.go index 1a02d1ad..fdf6437f 100644 --- a/internal/controller/reconciler/reconcile_v2.go +++ b/internal/controller/reconciler/reconcile_v2.go @@ -138,6 +138,9 @@ func Reconcile( if err = deleteInfraHTTPRoutes(ctx, client, wandb); err != nil { return ctrl.Result{}, err } + if err = deleteWatchtower(ctx, client, wandb); err != nil { + return ctrl.Result{}, err + } if wandb.Spec.Networking.Mode == apiv2.NetworkingModeIngress { if err = deleteConsolidatedIngress(ctx, client, wandb); err != nil { return ctrl.Result{}, err @@ -297,6 +300,37 @@ func ReconcileWandbManifest( statusBefore := wandb.DeepCopy().Status + // Networking (gateway or ingress) need to be reconciled before infra gate. + // Watchtower relies on wandb networking for oidc auth + if err := cleanupNetworkingModeResources(ctx, client, wandb); err != nil { + logger.Error(err, "Failed to clean up stale networking resources") + return ctrl.Result{}, err + } + resetInactiveNetworkingStatus(wandb) + + // Reconcile networking + switch wandb.Spec.Networking.Mode { + case apiv2.NetworkingModeGatewayAPI: + wandb.Status.GatewayStatus = nil + if err := reconcileGateway(ctx, client, wandb); err != nil { + logger.Error(err, "Failed to reconcile Gateway") + return ctrl.Result{}, err + } + if err := reconcileInfraHTTPRoutes(ctx, client, wandb, manifest); err != nil { + logger.Error(err, "Failed to reconcile infra HTTPRoutes") + return ctrl.Result{}, err + } + case apiv2.NetworkingModeIngress: + wandb.Status.IngressStatus = nil + if err := reconcileConsolidatedIngress(ctx, client, wandb, manifest); err != nil { + logger.Error(err, "Failed to reconcile consolidated Ingress") + return ctrl.Result{}, err + } + } + // Do not block on Watchtower failure to reconcile + if err := reconcileWatchtower(ctx, client, wandb, manifest); err != nil { + logger.Error(err, "Failed to reconcile Watchtower") + } redisReady := redisAllReady(wandb) mysqlReady := mysqlAllReady(wandb) kafkaReady := wandb.Status.KafkaStatus.Ready @@ -368,25 +402,11 @@ func ReconcileWandbManifest( return ctrl.Result{}, err } - if err := cleanupNetworkingModeResources(ctx, client, wandb); err != nil { - logger.Error(err, "Failed to clean up stale networking resources") - return ctrl.Result{}, err - } - resetInactiveNetworkingStatus(wandb) - if err := reconcileCustomCACerts(ctx, client, wandb); err != nil { logger.Error(err, "Failed to reconcile custom CA certificates") return ctrl.Result{}, err } - if wandb.Spec.Networking.Mode == apiv2.NetworkingModeGatewayAPI { - wandb.Status.GatewayStatus = nil - if err := reconcileGateway(ctx, client, wandb); err != nil { - logger.Error(err, "Failed to reconcile Gateway") - return ctrl.Result{}, err - } - } - result, err = runMigrations(ctx, client, wandb, manifest) if err != nil { return result, err @@ -420,13 +440,6 @@ func ReconcileWandbManifest( "notReady", notReady) } - if wandb.Spec.Networking.Mode == apiv2.NetworkingModeGatewayAPI { - if err := reconcileInfraHTTPRoutes(ctx, client, wandb, manifest); err != nil { - logger.Error(err, "Failed to reconcile infra HTTPRoutes") - return ctrl.Result{}, err - } - } - if applicationsHealthy { setReadyStatus( wandb, @@ -633,14 +646,6 @@ func reconcileApplications( } } - if wandb.Spec.Networking.Mode == apiv2.NetworkingModeIngress { - wandb.Status.IngressStatus = nil - if err := reconcileConsolidatedIngress(ctx, client, wandb, manifest); err != nil { - logger.Error("Failed to reconcile consolidated Ingress", "err", err) - return ctrl.Result{}, err - } - } - hostname, err := url.Parse(wandb.Spec.Wandb.Hostname) if err != nil { logger.Error("Failed to parse provided hostname", "hostname", wandb.Spec.Wandb.Hostname, "err", err) @@ -686,6 +691,21 @@ func applicationManagedFieldsEqual(before, after *apiv2.Application) bool { } func buildHTTPRouteTemplate(wandb *apiv2.WeightsAndBiases, app serverManifest.Application) *apiv2.HTTPRouteTemplateSpec { + var paths []string + var pathType string + if app.Ingress != nil { + paths = app.Ingress.Paths + pathType = app.Ingress.PathType + } + return buildHTTPRouteTemplateForPaths(wandb, paths, pathType, resolveHTTPRouteServicePort(app)) +} + +func buildHTTPRouteTemplateForPaths( + wandb *apiv2.WeightsAndBiases, + paths []string, + pathType string, + servicePort *gatewayv1.PortNumber, +) *apiv2.HTTPRouteTemplateSpec { gwConfig := wandb.Spec.Networking.GatewayAPI ref := wandb.Status.GatewayStatus.GatewayRef @@ -696,7 +716,7 @@ func buildHTTPRouteTemplate(wandb *apiv2.WeightsAndBiases, app serverManifest.Ap ns := gatewayv1.Namespace(ref.Namespace) parentRef.Namespace = &ns } - if gwConfig.ListenerName != nil { + if gwConfig != nil && gwConfig.ListenerName != nil { sectionName := gatewayv1.SectionName(*gwConfig.ListenerName) parentRef.SectionName = §ionName } @@ -706,23 +726,16 @@ func buildHTTPRouteTemplate(wandb *apiv2.WeightsAndBiases, app serverManifest.Ap for _, h := range wandb.Spec.Wandb.AdditionalHostnames { hostnames = append(hostnames, gatewayv1.Hostname(h)) } - - var paths []string - var pathType string - if app.Ingress != nil { - paths = app.Ingress.Paths - pathType = app.Ingress.PathType - } - return &apiv2.HTTPRouteTemplateSpec{ ParentRefs: []gatewayv1.ParentReference{parentRef}, Hostnames: hostnames, Paths: paths, PathType: pathType, - ServicePort: resolveHTTPRouteServicePort(app), + ServicePort: servicePort, } } + func resolveHTTPRouteServicePort(app serverManifest.Application) *gatewayv1.PortNumber { if app.Ingress != nil && app.Ingress.ServicePort != "" { port := intstr.Parse(app.Ingress.ServicePort) diff --git a/internal/controller/reconciler/watchtower.go b/internal/controller/reconciler/watchtower.go new file mode 100644 index 00000000..a12519d4 --- /dev/null +++ b/internal/controller/reconciler/watchtower.go @@ -0,0 +1,572 @@ +package reconciler + +import ( + "context" + "fmt" + "os" + "slices" + "strings" + + apiv2 "github.com/wandb/operator/api/v2" + "github.com/wandb/operator/internal/controller/common" + "github.com/wandb/operator/internal/logx" + "github.com/wandb/operator/pkg/utils" + serverManifest "github.com/wandb/operator/pkg/wandb/manifest" + corev1 "k8s.io/api/core/v1" + networkingv1 "k8s.io/api/networking/v1" + rbacv1 "k8s.io/api/rbac/v1" + apiErrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/resource" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/apimachinery/pkg/util/validation" + "k8s.io/utils/ptr" + controllerruntime "sigs.k8s.io/controller-runtime" + ctrlClient "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" +) + +const ( + // watchtowerComponent labels every Watchtower resource so the manifest-driven + // Application pruning skips it and stale resources stay findable. + watchtowerComponent = "watchtower" + + watchtowerContainerPort = int32(8080) + watchtowerPortName = "http" + watchtowerPasswordKey = "password" + + // watchtowerOIDCIngressPath is the ingress path owned by the application that + // serves gorilla's /oidc/auth sub-request, used to derive AUTH_SERVICE. + watchtowerOIDCIngressPath = "/oidc" + operatorImageEnvVar = "OPERATOR_IMAGE" +) + +// reconcileWatchtower brings the operator-managed Watchtower deployment in line +// with spec.watchtower. Watchtower is not published in the server manifest, so +// the operator owns its Application, Service account and RBAC outright. +func reconcileWatchtower( + ctx context.Context, + c ctrlClient.Client, + wandb *apiv2.WeightsAndBiases, + manifest serverManifest.Manifest, +) error { + logger := logx.GetSlog(ctx) + + if !wandb.WatchtowerEnabled() { + return deleteWatchtower(ctx, c, wandb) + } + + authService, err := watchtowerAuthService(wandb, manifest) + if err != nil { + return err + } + + if err := reconcileWatchtowerServiceAccount(ctx, c, wandb); err != nil { + return err + } + if err := reconcileWatchtowerRBAC(ctx, c, wandb); err != nil { + return err + } + if err := reconcileWatchtowerSecret(ctx, c, wandb); err != nil { + return err + } + image, err := watchtowerImage(wandb) + if err != nil { + return err + } + desired := buildWatchtowerApplication(wandb, authService, image) + + application := &apiv2.Application{ + ObjectMeta: metav1.ObjectMeta{ + Name: watchtowerName(wandb), + Namespace: wandb.Namespace, + }, + } + op, err := controllerruntime.CreateOrUpdate(ctx, c, application, func() error { + application.Labels = utils.MergeMapsStringString(application.Labels, desired.Labels) + application.Spec = desired.Spec + return controllerutil.SetOwnerReference(wandb, application, c.Scheme()) + }) + if err != nil { + return fmt.Errorf("failed to reconcile Watchtower Application: %w", err) + } + logger.Info(fmt.Sprintf("Successfully %s Watchtower Application", op), + "application", watchtowerName(wandb), "authService", authService) + + wandb.Status.WatchtowerStatus = &apiv2.WatchtowerStatusSummary{ + Ready: application.Status.Ready, + URL: watchtowerURL(wandb), + Image: image, + AuthService: authService, + } + + return nil +} + +// watchtowerURL is where a browser reaches Watchtower: the W&B hostname plus the +// base path, since it is deliberately served from the app's own origin. +func watchtowerURL(wandb *apiv2.WeightsAndBiases) string { + hostname := strings.TrimSuffix(wandb.Spec.Wandb.Hostname, "/") + if hostname == "" { + return "" + } + if !strings.Contains(hostname, "://") { + hostname = "https://" + hostname + } + return hostname + wandb.Spec.Watchtower.ResolvedBasePath() +} + +// deleteWatchtower removes every Watchtower resource. The cluster-scoped +// ClusterRole and ClusterRoleBinding cannot carry an owner reference to a +// namespaced CR, so they are deleted here explicitly rather than by GC. +func deleteWatchtower(ctx context.Context, c ctrlClient.Client, wandb *apiv2.WeightsAndBiases) error { + objects := []ctrlClient.Object{ + &apiv2.Application{ObjectMeta: metav1.ObjectMeta{Name: watchtowerName(wandb), Namespace: wandb.Namespace}}, + &rbacv1.RoleBinding{ObjectMeta: metav1.ObjectMeta{Name: watchtowerName(wandb), Namespace: wandb.Namespace}}, + &rbacv1.Role{ObjectMeta: metav1.ObjectMeta{Name: watchtowerName(wandb), Namespace: wandb.Namespace}}, + &rbacv1.ClusterRoleBinding{ObjectMeta: metav1.ObjectMeta{Name: watchtowerClusterScopedName(wandb)}}, + &rbacv1.ClusterRole{ObjectMeta: metav1.ObjectMeta{Name: watchtowerClusterScopedName(wandb)}}, + &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: watchtowerSecretName(wandb), Namespace: wandb.Namespace}}, + } + + for _, obj := range objects { + if err := c.Delete(ctx, obj); err != nil && !apiErrors.IsNotFound(err) { + return fmt.Errorf("failed to delete Watchtower %T %s: %w", obj, obj.GetName(), err) + } + } + + // The ServiceAccount is owner-referenced and only deleted when the operator + // created it; a user-supplied account is left alone. + if ptr.Deref(wandb.Spec.Watchtower.ServiceAccount.Create, true) { + sa := &corev1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{ + Name: watchtowerServiceAccountName(wandb), + Namespace: wandb.Namespace, + }} + if err := c.Delete(ctx, sa); err != nil && !apiErrors.IsNotFound(err) { + return fmt.Errorf("failed to delete Watchtower ServiceAccount: %w", err) + } + } + + wandb.Status.WatchtowerStatus = nil + return nil +} + +func buildWatchtowerApplication(wandb *apiv2.WeightsAndBiases, authService string, image string) *apiv2.Application { + watchtower := wandb.Spec.Watchtower + labels := watchtowerLabels(wandb) + basePath := watchtower.ResolvedBasePath() + + app := &apiv2.Application{ + ObjectMeta: metav1.ObjectMeta{ + Name: watchtowerName(wandb), + Namespace: wandb.Namespace, + Labels: labels, + }, + Spec: apiv2.ApplicationSpec{ + Kind: "Deployment", + // Pinned to one replica: in-flight deploy jobs and their SSE streams + // live in the serving pod's memory, so a reconnect that lands on a + // second pod would see no history. + Replicas: ptr.To(int32(1)), + MetaTemplate: metav1.ObjectMeta{ + Labels: labels, + }, + PodTemplate: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + ServiceAccountName: watchtowerServiceAccountName(wandb), + SecurityContext: resolvePodSecurityContext(), + Affinity: wandb.Spec.Affinity, + Tolerations: watchtowerTolerations(wandb), + Containers: []corev1.Container{ + { + Name: watchtowerComponent, + Image: image, + Command: []string{"/watchtower"}, + Args: []string{"--port", fmt.Sprintf("%d", watchtowerContainerPort)}, + SecurityContext: resolveContainerSecurityContext(), + Env: watchtowerEnv(wandb, authService, basePath), + Resources: watchtowerResources(watchtower), + Ports: []corev1.ContainerPort{{ + Name: watchtowerPortName, + ContainerPort: watchtowerContainerPort, + Protocol: corev1.ProtocolTCP, + }}, + // Probes go through the base path because the server + // mounts every route, health included, behind it. + LivenessProbe: watchtowerProbe(basePath + "/healthz"), + ReadinessProbe: watchtowerProbe(basePath + "/ready"), + }, + }, + }, + }, + ServiceTemplate: &corev1.ServiceSpec{ + Type: corev1.ServiceTypeClusterIP, + Ports: []corev1.ServicePort{{ + Name: watchtowerPortName, + Port: watchtowerContainerPort, + TargetPort: intstr.FromInt32(watchtowerContainerPort), + Protocol: corev1.ProtocolTCP, + }}, + }, + }, + } + + if wandb.Spec.Networking.Mode == apiv2.NetworkingModeGatewayAPI && + wandb.Status.GatewayStatus != nil && wandb.Status.GatewayStatus.GatewayRef != nil { + app.Spec.HTTPRouteTemplate = buildHTTPRouteTemplateForPaths( + wandb, + []string{basePath}, + string(networkingv1.PathTypePrefix), + ptr.To(gatewayv1.PortNumber(watchtowerContainerPort)), + ) + } + + return app +} + +// watchtowerEnv is the operator's side of the contract with the Watchtower +// container: it is told where it is mounted and which service validates the +// caller's session, so neither has to be baked into the image. +func watchtowerEnv(wandb *apiv2.WeightsAndBiases, authService, basePath string) []corev1.EnvVar { + return []corev1.EnvVar{ + // Locks the UI to the cluster it runs in: no context switching, no teardown. + {Name: "WATCHTOWER_MODE", Value: "cluster"}, + {Name: "WATCHTOWER_BASE_PATH", Value: basePath}, + {Name: "WATCHTOWER_AUTH_SERVICE", Value: authService}, + {Name: "WATCHTOWER_WANDB_NAME", Value: wandb.Name}, + {Name: "WATCHTOWER_NAMESPACE", ValueFrom: &corev1.EnvVarSource{ + FieldRef: &corev1.ObjectFieldSelector{FieldPath: "metadata.namespace"}, + }}, + {Name: "WATCHTOWER_PASSWORD", ValueFrom: &corev1.EnvVarSource{ + SecretKeyRef: &corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{Name: watchtowerSecretName(wandb)}, + Key: watchtowerPasswordKey, + }, + }}, + } +} + +// watchtowerAuthService resolves the in-cluster host:port Watchtower calls to +// validate the browser's W&B session cookie. The manifest application that owns +// the /oidc ingress path is the one serving gorilla's /oidc/auth, and the +// application controller names its Service after the application, so this stays +// correct across manifest renames. +// +// Failing here is deliberate: without an auth service Watchtower would serve +// cluster administration unauthenticated. spec.watchtower.authService is the +// escape hatch for deployments whose manifest does not declare the path. +func watchtowerAuthService(wandb *apiv2.WeightsAndBiases, manifest serverManifest.Manifest) (string, error) { + if wandb.Spec.Watchtower.AuthService != "" { + return wandb.Spec.Watchtower.AuthService, nil + } + + for _, app := range sortedManifestApplications(manifest) { + if app.Ingress == nil || app.Service == nil { + continue + } + if len(app.Features) > 0 && !manifest.FeaturesEnabled(app.Features) { + continue + } + if !slices.Contains(app.Ingress.Paths, watchtowerOIDCIngressPath) { + continue + } + return fmt.Sprintf("%s:%d", app.Name, watchtowerAuthServicePort(app)), nil + } + + return "", fmt.Errorf( + "cannot derive spec.watchtower.authService: no manifest application serves the %q ingress path; set it explicitly", + watchtowerOIDCIngressPath, + ) +} + +// watchtowerAuthServicePort resolves the app's ingress service port to a number, +// following the named-port indirection the manifest allows. +func watchtowerAuthServicePort(app serverManifest.Application) int32 { + if app.Ingress != nil && app.Ingress.ServicePort != "" { + parsed := intstr.Parse(app.Ingress.ServicePort) + if parsed.Type == intstr.Int { + return parsed.IntVal + } + for _, port := range app.Service.Ports { + if port.Name == parsed.StrVal { + return port.Port + } + } + } + if len(app.Service.Ports) > 0 { + return app.Service.Ports[0].Port + } + return watchtowerContainerPort +} + +// watchtowerIngressPath returns the consolidated-Ingress path for Watchtower, or +// nil when it is not installed. +func watchtowerIngressPath(wandb *apiv2.WeightsAndBiases) *networkingv1.HTTPIngressPath { + if !wandb.WatchtowerEnabled() { + return nil + } + pathType := networkingv1.PathTypePrefix + return &networkingv1.HTTPIngressPath{ + Path: wandb.Spec.Watchtower.ResolvedBasePath(), + PathType: &pathType, + Backend: networkingv1.IngressBackend{ + Service: &networkingv1.IngressServiceBackend{ + Name: watchtowerName(wandb), + Port: networkingv1.ServiceBackendPort{Number: watchtowerContainerPort}, + }, + }, + } +} + +func watchtowerProbe(path string) *corev1.Probe { + return &corev1.Probe{ + ProbeHandler: corev1.ProbeHandler{ + HTTPGet: &corev1.HTTPGetAction{ + Path: path, + Port: intstr.FromInt32(watchtowerContainerPort), + }, + }, + TimeoutSeconds: 3, + PeriodSeconds: 10, + FailureThreshold: 3, + } +} + +// watchtowerResources keeps the UI modest by default; it is an admin console +// whose heavy work happens in the cluster, not in this pod. +func watchtowerResources(watchtower apiv2.WatchtowerSpec) corev1.ResourceRequirements { + if len(watchtower.Resources.Requests) > 0 || len(watchtower.Resources.Limits) > 0 { + return watchtower.Resources + } + return corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("100m"), + corev1.ResourceMemory: resource.MustParse("256Mi"), + }, + } +} + +func watchtowerImage(wandb *apiv2.WeightsAndBiases) (string, error) { + if override := wandb.Spec.Watchtower.GetImage(wandb.Spec.Global.ImageRegistry); override != "" { + return override, nil + } + image := os.Getenv(operatorImageEnvVar) + if image == "" { + return "", fmt.Errorf( + "%s is unset: cannot determine which image carries the Watchtower binary", + operatorImageEnvVar, + ) + } + return image, nil +} + +func watchtowerTolerations(wandb *apiv2.WeightsAndBiases) []corev1.Toleration { + if wandb.Spec.Tolerations == nil { + return nil + } + return *wandb.Spec.Tolerations +} + +func watchtowerServiceAccountName(wandb *apiv2.WeightsAndBiases) string { + if name := wandb.Spec.Watchtower.ServiceAccount.ServiceAccountName; name != "" { + return name + } + return watchtowerName(wandb) +} + +func watchtowerName(wandb *apiv2.WeightsAndBiases) string { + return common.FitDefaultInfraName(wandb.Name, "-watchtower", validation.DNS1123LabelMaxLength) +} + +func watchtowerSecretName(wandb *apiv2.WeightsAndBiases) string { + return common.FitDefaultInfraName(wandb.Name, "-watchtower-auth", validation.DNS1123LabelMaxLength) +} + +// watchtowerClusterScopedName qualifies cluster-scoped RBAC with the CR's +// namespace so two W&B installs in one cluster do not fight over one object. +func watchtowerClusterScopedName(wandb *apiv2.WeightsAndBiases) string { + return fmt.Sprintf("%s-%s-watchtower", wandb.Namespace, wandb.Name) +} + +func watchtowerLabels(wandb *apiv2.WeightsAndBiases) map[string]string { + labels := common.BuildWandbLabels(wandb, watchtowerComponent) + labels["app.kubernetes.io/managed-by"] = "wandb-operator" + labels["app.kubernetes.io/instance"] = wandb.Name + labels["app.kubernetes.io/part-of"] = "wandb" + return labels +} + +func reconcileWatchtowerSecret(ctx context.Context, c ctrlClient.Client, wandb *apiv2.WeightsAndBiases) error { + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: watchtowerSecretName(wandb), + Namespace: wandb.Namespace, + }, + } + _, err := controllerruntime.CreateOrUpdate(ctx, c, secret, func() error { + secret.Labels = utils.MergeMapsStringString(secret.Labels, watchtowerLabels(wandb)) + secret.Type = corev1.SecretTypeOpaque + if len(secret.Data[watchtowerPasswordKey]) == 0 { + password, err := utils.GenerateRandomPassword(32) + if err != nil { + return err + } + if secret.StringData == nil { + secret.StringData = map[string]string{} + } + secret.StringData[watchtowerPasswordKey] = password + } + return controllerutil.SetControllerReference(wandb, secret, c.Scheme()) + }) + if err != nil { + return fmt.Errorf("failed to reconcile Watchtower Secret: %w", err) + } + return nil +} + +func reconcileWatchtowerServiceAccount(ctx context.Context, c ctrlClient.Client, wandb *apiv2.WeightsAndBiases) error { + if !ptr.Deref(wandb.Spec.Watchtower.ServiceAccount.Create, true) { + return nil + } + + serviceAccount := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: watchtowerServiceAccountName(wandb), + Namespace: wandb.Namespace, + }, + } + _, err := controllerruntime.CreateOrUpdate(ctx, c, serviceAccount, func() error { + serviceAccount.Labels = utils.MergeMapsStringString(serviceAccount.Labels, watchtowerLabels(wandb)) + serviceAccount.Annotations = utils.MergeMapsStringString( + serviceAccount.Annotations, + wandb.Spec.Watchtower.ServiceAccount.Annotations, + ) + // Watchtower talks to the Kubernetes API with this token, so unlike the + // W&B application pods it must have one mounted. + serviceAccount.AutomountServiceAccountToken = ptr.To(true) + return controllerutil.SetControllerReference(wandb, serviceAccount, c.Scheme()) + }) + if err != nil { + return fmt.Errorf("failed to reconcile Watchtower ServiceAccount: %w", err) + } + return nil +} + +// reconcileWatchtowerRBAC grants Watchtower what the operator itself holds and +// can therefore delegate: the apiserver rejects a binding that would escalate +// beyond the operator's own permissions. +// +// Deliberately absent, because the operator does not hold them today: +// apiextensions.k8s.io/customresourcedefinitions (v2 served-version detection) +// and pods/portforward (telemetry port-forward). Both need the operator's own +// ClusterRole widened first. +func reconcileWatchtowerRBAC(ctx context.Context, c ctrlClient.Client, wandb *apiv2.WeightsAndBiases) error { + labels := watchtowerLabels(wandb) + serviceAccountName := watchtowerServiceAccountName(wandb) + clusterScopedName := watchtowerClusterScopedName(wandb) + + role := &rbacv1.Role{ObjectMeta: metav1.ObjectMeta{Name: watchtowerName(wandb), Namespace: wandb.Namespace}} + if _, err := controllerruntime.CreateOrUpdate(ctx, c, role, func() error { + role.Labels = utils.MergeMapsStringString(role.Labels, labels) + // Secrets and ConfigMaps stay namespace-scoped: Watchtower reads the + // install's license and connection material, not the whole cluster's. + role.Rules = []rbacv1.PolicyRule{ + { + APIGroups: []string{""}, + Resources: []string{"secrets", "configmaps"}, + Verbs: []string{"get", "list", "watch"}, + }, + { + APIGroups: []string{"batch"}, + Resources: []string{"jobs", "cronjobs"}, + Verbs: []string{"get", "list", "watch"}, + }, + { + APIGroups: []string{"networking.k8s.io"}, + Resources: []string{"ingresses"}, + Verbs: []string{"get", "list", "watch"}, + }, + } + return controllerutil.SetOwnerReference(wandb, role, c.Scheme()) + }); err != nil { + return fmt.Errorf("failed to reconcile Watchtower Role: %w", err) + } + + roleBinding := &rbacv1.RoleBinding{ObjectMeta: metav1.ObjectMeta{Name: watchtowerName(wandb), Namespace: wandb.Namespace}} + if _, err := controllerruntime.CreateOrUpdate(ctx, c, roleBinding, func() error { + roleBinding.Labels = utils.MergeMapsStringString(roleBinding.Labels, labels) + roleBinding.RoleRef = rbacv1.RoleRef{ + APIGroup: rbacv1.GroupName, + Kind: "Role", + Name: watchtowerName(wandb), + } + roleBinding.Subjects = []rbacv1.Subject{{ + Kind: "ServiceAccount", + Name: serviceAccountName, + Namespace: wandb.Namespace, + }} + return controllerutil.SetOwnerReference(wandb, roleBinding, c.Scheme()) + }); err != nil { + return fmt.Errorf("failed to reconcile Watchtower RoleBinding: %w", err) + } + + clusterRole := &rbacv1.ClusterRole{ObjectMeta: metav1.ObjectMeta{Name: clusterScopedName}} + if _, err := controllerruntime.CreateOrUpdate(ctx, c, clusterRole, func() error { + clusterRole.Labels = utils.MergeMapsStringString(clusterRole.Labels, labels) + clusterRole.Rules = []rbacv1.PolicyRule{ + { + APIGroups: []string{"apps.wandb.com"}, + Resources: []string{"weightsandbiases"}, + Verbs: []string{"get", "list", "watch", "update", "patch"}, + }, + { + APIGroups: []string{"apps.wandb.com"}, + Resources: []string{"applications"}, + Verbs: []string{"get", "list", "watch"}, + }, + { + // Only "get": the operator itself holds get/update/patch on these + // subresources, and it cannot grant verbs it does not have. + APIGroups: []string{"apps.wandb.com"}, + Resources: []string{"weightsandbiases/status", "applications/status"}, + Verbs: []string{"get"}, + }, + { + APIGroups: []string{""}, + Resources: []string{"namespaces", "pods", "pods/log", "services", "events"}, + Verbs: []string{"get", "list", "watch"}, + }, + { + APIGroups: []string{"apps"}, + Resources: []string{"deployments", "statefulsets", "replicasets", "daemonsets"}, + Verbs: []string{"get", "list", "watch"}, + }, + } + // Cluster-scoped objects cannot own-reference a namespaced CR; cleanup + // runs through deleteWatchtower instead. + return nil + }); err != nil { + return fmt.Errorf("failed to reconcile Watchtower ClusterRole: %w", err) + } + + clusterRoleBinding := &rbacv1.ClusterRoleBinding{ObjectMeta: metav1.ObjectMeta{Name: clusterScopedName}} + if _, err := controllerruntime.CreateOrUpdate(ctx, c, clusterRoleBinding, func() error { + clusterRoleBinding.Labels = utils.MergeMapsStringString(clusterRoleBinding.Labels, labels) + clusterRoleBinding.RoleRef = rbacv1.RoleRef{ + APIGroup: rbacv1.GroupName, + Kind: "ClusterRole", + Name: clusterScopedName, + } + clusterRoleBinding.Subjects = []rbacv1.Subject{{ + Kind: "ServiceAccount", + Name: serviceAccountName, + Namespace: wandb.Namespace, + }} + return nil + }); err != nil { + return fmt.Errorf("failed to reconcile Watchtower ClusterRoleBinding: %w", err) + } + + return nil +} diff --git a/internal/controller/reconciler/watchtower_test.go b/internal/controller/reconciler/watchtower_test.go new file mode 100644 index 00000000..f53ba5c8 --- /dev/null +++ b/internal/controller/reconciler/watchtower_test.go @@ -0,0 +1,540 @@ +/* +Copyright 2025. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 +*/ + +package reconciler + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" + apiv2 "github.com/wandb/operator/api/v2" + serverManifest "github.com/wandb/operator/pkg/wandb/manifest" + corev1 "k8s.io/api/core/v1" + networkingv1 "k8s.io/api/networking/v1" + rbacv1 "k8s.io/api/rbac/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/validation" + "k8s.io/utils/ptr" + ctrlClient "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +func watchtowerTestClient(t *testing.T, objects ...ctrlClient.Object) ctrlClient.Client { + t.Helper() + scheme := runtime.NewScheme() + require.NoError(t, corev1.AddToScheme(scheme)) + require.NoError(t, rbacv1.AddToScheme(scheme)) + require.NoError(t, apiv2.AddToScheme(scheme)) + + builder := fake.NewClientBuilder().WithScheme(scheme) + if len(objects) > 0 { + builder = builder.WithObjects(objects...) + } + return builder.Build() +} + +func watchtowerTestCR(name, namespace string) *apiv2.WeightsAndBiases { + return &apiv2.WeightsAndBiases{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace}, + Spec: apiv2.WeightsAndBiasesSpec{ + Watchtower: apiv2.WatchtowerSpec{Install: ptr.To(true)}, + Wandb: apiv2.WandbAppSpec{Hostname: "wandb.example.com"}, + }, + } +} + +// manifestWithOIDC returns a manifest whose "api" application owns /oidc, which +// is what watchtowerAuthService derives AUTH_SERVICE from. +func manifestWithOIDC() serverManifest.Manifest { + return serverManifest.Manifest{ + Applications: map[string]serverManifest.Application{ + "api": { + Name: "api", + Ingress: &serverManifest.AppIngressSpec{ + Paths: []string{"/oidc", "/graphql"}, + ServicePort: "http", + }, + Service: &serverManifest.ServiceSpec{ + Ports: []corev1.ServicePort{{Name: "http", Port: 8081}}, + }, + }, + }, + } +} + +// --- naming: same-namespace multi-install ----------------------------------- + +// Two CRs in one namespace must not share an Application, RBAC or Secret. Every +// namespaced Watchtower name is derived from the CR name for this reason. +func TestWatchtowerNamesAreDistinctPerCRInOneNamespace(t *testing.T) { + first := watchtowerTestCR("alpha", "wandb") + second := watchtowerTestCR("beta", "wandb") + + require.NotEqual(t, watchtowerName(first), watchtowerName(second)) + require.NotEqual(t, watchtowerSecretName(first), watchtowerSecretName(second)) + require.NotEqual(t, watchtowerServiceAccountName(first), watchtowerServiceAccountName(second)) + require.Equal(t, "alpha-watchtower", watchtowerName(first)) + require.Equal(t, "alpha-watchtower-auth", watchtowerSecretName(first)) +} + +// The application controller derives a Service from the Application name, and +// Service names are DNS-1123 labels, so long CR names must be shortened — but +// shortening must not collapse two distinct CRs onto one name. +func TestWatchtowerNamesStayWithinLabelBudgetAndDistinct(t *testing.T) { + longA := watchtowerTestCR("production-cluster-that-is-really-quite-long-east-region-one", "wandb") + longB := watchtowerTestCR("production-cluster-that-is-really-quite-long-west-region-two", "wandb") + + for _, name := range []string{ + watchtowerName(longA), watchtowerName(longB), + watchtowerSecretName(longA), watchtowerSecretName(longB), + } { + require.LessOrEqual(t, len(name), validation.DNS1123LabelMaxLength) + require.Empty(t, validation.IsDNS1123Label(name), "name %q is not a valid label", name) + } + + require.NotEqual(t, watchtowerName(longA), watchtowerName(longB)) + require.NotEqual(t, watchtowerSecretName(longA), watchtowerSecretName(longB)) +} + +// The Secret name is derived from the CR name with its own suffix rather than by +// appending to watchtowerName: composing on an already-hashed name would push +// the suffix past the budget and truncate the hash back off. +func TestWatchtowerSecretNameIsNotAPrefixCollisionOfAppName(t *testing.T) { + wandb := watchtowerTestCR("wandb", "wandb") + require.NotEqual(t, watchtowerName(wandb), watchtowerSecretName(wandb)) +} + +// Cluster-scoped RBAC is global, so two installs in *different* namespaces must +// not collide either. +func TestWatchtowerClusterScopedNameIncludesNamespace(t *testing.T) { + prod := watchtowerTestCR("wandb", "wandb") + staging := watchtowerTestCR("wandb", "wandb-staging") + + require.NotEqual(t, watchtowerClusterScopedName(prod), watchtowerClusterScopedName(staging)) + require.Contains(t, watchtowerClusterScopedName(prod), "wandb") +} + +// --- the generated admin password ------------------------------------------- + +func TestReconcileWatchtowerSecretGeneratesAPassword(t *testing.T) { + wandb := watchtowerTestCR("wandb", "wandb") + c := watchtowerTestClient(t, wandb) + + require.NoError(t, reconcileWatchtowerSecret(context.Background(), c, wandb)) + + secret := &corev1.Secret{} + require.NoError(t, c.Get(context.Background(), types.NamespacedName{ + Name: watchtowerSecretName(wandb), Namespace: wandb.Namespace, + }, secret)) + + password := secretPassword(secret) + require.NotEmpty(t, password, "expected a generated password") + require.Len(t, password, 32) + require.Equal(t, corev1.SecretTypeOpaque, secret.Type) +} + +// The whole point of create-if-not-found: an upgrade must not rotate the password +// out from under whoever is holding it. +func TestReconcileWatchtowerSecretPreservesAnExistingPassword(t *testing.T) { + wandb := watchtowerTestCR("wandb", "wandb") + existing := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: watchtowerSecretName(wandb), + Namespace: wandb.Namespace, + }, + Type: corev1.SecretTypeOpaque, + // Seeded via Data, which is how the apiserver returns a Secret written + // with StringData. + Data: map[string][]byte{watchtowerPasswordKey: []byte("do-not-rotate-me")}, + } + c := watchtowerTestClient(t, wandb, existing) + + require.NoError(t, reconcileWatchtowerSecret(context.Background(), c, wandb)) + + secret := &corev1.Secret{} + require.NoError(t, c.Get(context.Background(), types.NamespacedName{ + Name: watchtowerSecretName(wandb), Namespace: wandb.Namespace, + }, secret)) + require.Equal(t, "do-not-rotate-me", secretPassword(secret)) +} + +func TestReconcileWatchtowerSecretIsOwnedByTheCR(t *testing.T) { + wandb := watchtowerTestCR("wandb", "wandb") + c := watchtowerTestClient(t, wandb) + + require.NoError(t, reconcileWatchtowerSecret(context.Background(), c, wandb)) + + secret := &corev1.Secret{} + require.NoError(t, c.Get(context.Background(), types.NamespacedName{ + Name: watchtowerSecretName(wandb), Namespace: wandb.Namespace, + }, secret)) + require.NotEmpty(t, secret.OwnerReferences) + require.Equal(t, wandb.Name, secret.OwnerReferences[0].Name) +} + +// secretPassword reads the password from whichever field it landed in. The fake +// client does not perform the apiserver's StringData -> Data conversion, so a +// freshly written Secret carries StringData while a seeded one carries Data. +func secretPassword(secret *corev1.Secret) string { + if value, ok := secret.Data[watchtowerPasswordKey]; ok && len(value) > 0 { + return string(value) + } + return secret.StringData[watchtowerPasswordKey] +} + +// --- container environment --------------------------------------------------- + +func TestWatchtowerEnvReferencesThePasswordSecret(t *testing.T) { + wandb := watchtowerTestCR("wandb", "wandb") + + env := watchtowerEnv(wandb, "api:8081", "/watchtower") + + byName := map[string]corev1.EnvVar{} + for _, e := range env { + byName[e.Name] = e + } + + password, ok := byName["WATCHTOWER_PASSWORD"] + require.True(t, ok, "expected WATCHTOWER_PASSWORD to be set") + require.Nil(t, password.ValueFrom.SecretKeyRef.Optional) + require.Empty(t, password.Value, "the password must never be inlined in the pod spec") + require.Equal(t, watchtowerSecretName(wandb), password.ValueFrom.SecretKeyRef.Name) + require.Equal(t, watchtowerPasswordKey, password.ValueFrom.SecretKeyRef.Key) + + require.Equal(t, "cluster", byName["WATCHTOWER_MODE"].Value) + require.Equal(t, "/watchtower", byName["WATCHTOWER_BASE_PATH"].Value) + require.Equal(t, "api:8081", byName["WATCHTOWER_AUTH_SERVICE"].Value) + require.Equal(t, "wandb", byName["WATCHTOWER_WANDB_NAME"].Value) + require.Equal(t, + "metadata.namespace", + byName["WATCHTOWER_NAMESPACE"].ValueFrom.FieldRef.FieldPath, + ) +} + +// --- auth service derivation ------------------------------------------------- + +func TestWatchtowerAuthServiceDerivesFromTheOIDCApplication(t *testing.T) { + wandb := watchtowerTestCR("wandb", "wandb") + + authService, err := watchtowerAuthService(wandb, manifestWithOIDC()) + + require.NoError(t, err) + require.Equal(t, "api:8081", authService) +} + +func TestWatchtowerAuthServiceHonorsAnExplicitOverride(t *testing.T) { + wandb := watchtowerTestCR("wandb", "wandb") + wandb.Spec.Watchtower.AuthService = "custom-api:9999" + + authService, err := watchtowerAuthService(wandb, manifestWithOIDC()) + + require.NoError(t, err) + require.Equal(t, "custom-api:9999", authService) +} + +// Failing closed: deploying Watchtower with no way to validate a session would +// leave the app-hostname route unauthenticated. +func TestWatchtowerAuthServiceFailsWhenNoApplicationServesOIDC(t *testing.T) { + wandb := watchtowerTestCR("wandb", "wandb") + manifest := serverManifest.Manifest{ + Applications: map[string]serverManifest.Application{ + "frontend": { + Name: "frontend", + Ingress: &serverManifest.AppIngressSpec{Paths: []string{"/"}}, + Service: &serverManifest.ServiceSpec{ + Ports: []corev1.ServicePort{{Name: "http", Port: 80}}, + }, + }, + }, + } + + _, err := watchtowerAuthService(wandb, manifest) + + require.Error(t, err) + require.Contains(t, err.Error(), "/oidc") +} + +// --- routing ----------------------------------------------------------------- + +// The Ingress backend has to name the Service the Application produces. If these +// drift the route silently points at nothing, or at another install's Watchtower. +func TestWatchtowerIngressPathTargetsTheApplicationService(t *testing.T) { + wandb := watchtowerTestCR("wandb", "wandb") + + path := watchtowerIngressPath(wandb) + + require.NotNil(t, path) + require.Equal(t, "/watchtower", path.Path) + require.Equal(t, networkingv1.PathTypePrefix, *path.PathType) + require.Equal(t, watchtowerName(wandb), path.Backend.Service.Name) + require.Equal(t, watchtowerContainerPort, path.Backend.Service.Port.Number) +} + +func TestWatchtowerIngressPathIsNilWhenDisabled(t *testing.T) { + wandb := watchtowerTestCR("wandb", "wandb") + wandb.Spec.Watchtower.Install = ptr.To(false) + + require.Nil(t, watchtowerIngressPath(wandb)) +} + +func TestWatchtowerURL(t *testing.T) { + for _, tc := range []struct { + name string + hostname string + basePath string + want string + }{ + {"adds a scheme", "wandb.example.com", "", "https://wandb.example.com/watchtower"}, + {"keeps an explicit scheme", "http://wandb.example.com", "", "http://wandb.example.com/watchtower"}, + {"strips a trailing slash", "https://wandb.example.com/", "", "https://wandb.example.com/watchtower"}, + {"honors a custom base path", "wandb.example.com", "/admin", "https://wandb.example.com/admin"}, + {"empty hostname yields no URL", "", "", ""}, + } { + t.Run(tc.name, func(t *testing.T) { + wandb := watchtowerTestCR("wandb", "wandb") + wandb.Spec.Wandb.Hostname = tc.hostname + wandb.Spec.Watchtower.BasePath = tc.basePath + + require.Equal(t, tc.want, watchtowerURL(wandb)) + }) + } +} + +// --- image resolution ------------------------------------------------------- + +const testOperatorImage = "us-docker.pkg.dev/wandb-production/public/wandb/operator:2.0.0-beta.3" + +// The Watchtower binary ships inside the operator image, so an unconfigured CR +// resolves to whatever image this operator is itself running. +func TestWatchtowerImageFallsBackToTheOperatorImage(t *testing.T) { + t.Setenv(operatorImageEnvVar, testOperatorImage) + + image, err := watchtowerImage(watchtowerTestCR("wandb", "wandb")) + + require.NoError(t, err) + require.Equal(t, testOperatorImage, image) +} + +func TestWatchtowerImagePrefersTheSpecOverride(t *testing.T) { + t.Setenv(operatorImageEnvVar, testOperatorImage) + wandb := watchtowerTestCR("wandb", "wandb") + wandb.Spec.Watchtower.Image = apiv2.WatchtowerImageSpec{ + Repository: "custom/watchtower", + Tag: "1.2.3", + } + + image, err := watchtowerImage(wandb) + + require.NoError(t, err) + require.Equal(t, "custom/watchtower:1.2.3", image) +} + +// Air-gapped installs retarget every image at a mirror. +func TestWatchtowerImageOverrideHonorsTheGlobalRegistry(t *testing.T) { + t.Setenv(operatorImageEnvVar, testOperatorImage) + wandb := watchtowerTestCR("wandb", "wandb") + wandb.Spec.Global.ImageRegistry = "registry.internal" + wandb.Spec.Watchtower.Image = apiv2.WatchtowerImageSpec{ + Repository: "custom/watchtower", + Digest: "sha256:abc123", + } + + image, err := watchtowerImage(wandb) + + require.NoError(t, err) + require.Equal(t, "registry.internal/custom/watchtower@sha256:abc123", image) +} + +// Failing beats guessing: an empty image would be rejected by the apiserver with +// a message that never mentions the missing environment variable. +func TestWatchtowerImageFailsWhenOperatorImageIsUnset(t *testing.T) { + t.Setenv(operatorImageEnvVar, "") + + _, err := watchtowerImage(watchtowerTestCR("wandb", "wandb")) + + require.ErrorContains(t, err, operatorImageEnvVar) +} + +// --- the Application -------------------------------------------------------- + +// Replicas is deliberately not configurable: in-flight deploy jobs and their SSE +// streams live in the serving pod's memory. +func TestBuildWatchtowerApplicationPinsOneReplica(t *testing.T) { + wandb := watchtowerTestCR("wandb", "wandb") + + app := buildWatchtowerApplication(wandb, "api:8081", testOperatorImage) + + require.Equal(t, int32(1), *app.Spec.Replicas) + require.Equal(t, "Deployment", app.Spec.Kind) + require.Equal(t, watchtowerName(wandb), app.Name) +} + +// The component label is what makes manifest-driven Application pruning skip +// this Application instead of deleting it every reconcile. +func TestBuildWatchtowerApplicationCarriesTheComponentLabel(t *testing.T) { + wandb := watchtowerTestCR("wandb", "wandb") + + app := buildWatchtowerApplication(wandb, "api:8081", testOperatorImage) + + require.Equal(t, watchtowerComponent, app.Labels["weightsandbiases.apps.wandb.com/component"]) +} + +// The operator image's entrypoint is /manager, so without an explicit command the +// pod would come up healthy running a second operator instead of Watchtower. +func TestBuildWatchtowerApplicationSelectsTheWatchtowerEntrypoint(t *testing.T) { + wandb := watchtowerTestCR("wandb", "wandb") + + container := buildWatchtowerApplication(wandb, "api:8081", testOperatorImage). + Spec.PodTemplate.Spec.Containers[0] + + require.Equal(t, testOperatorImage, container.Image) + require.Equal(t, []string{"/watchtower"}, container.Command) + // The binary defaults to 9090; the Service, container port and probes are 8080. + require.Equal(t, []string{"--port", "8080"}, container.Args) + require.Equal(t, watchtowerContainerPort, container.Ports[0].ContainerPort) +} + +func TestBuildWatchtowerApplicationProbesGoThroughTheBasePath(t *testing.T) { + wandb := watchtowerTestCR("wandb", "wandb") + + container := buildWatchtowerApplication(wandb, "api:8081", testOperatorImage). + Spec.PodTemplate.Spec.Containers[0] + + require.Equal(t, "/watchtower/healthz", container.LivenessProbe.HTTPGet.Path) + require.Equal(t, "/watchtower/ready", container.ReadinessProbe.HTTPGet.Path) +} + +// --- teardown --------------------------------------------------------------- + +// deleteWatchtower runs when Watchtower is *disabled*, not just when the CR is +// deleted, so owner-reference GC does not cover it — every object has to be +// listed explicitly. +func TestDeleteWatchtowerRemovesEveryOwnedObject(t *testing.T) { + wandb := watchtowerTestCR("wandb", "wandb") + name := watchtowerName(wandb) + clusterName := watchtowerClusterScopedName(wandb) + + c := watchtowerTestClient(t, wandb, + &apiv2.Application{ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: wandb.Namespace}}, + &rbacv1.Role{ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: wandb.Namespace}}, + &rbacv1.RoleBinding{ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: wandb.Namespace}}, + &rbacv1.ClusterRole{ObjectMeta: metav1.ObjectMeta{Name: clusterName}}, + &rbacv1.ClusterRoleBinding{ObjectMeta: metav1.ObjectMeta{Name: clusterName}}, + &corev1.Secret{ObjectMeta: metav1.ObjectMeta{ + Name: watchtowerSecretName(wandb), Namespace: wandb.Namespace, + }}, + &corev1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{ + Name: watchtowerServiceAccountName(wandb), Namespace: wandb.Namespace, + }}, + ) + + require.NoError(t, deleteWatchtower(context.Background(), c, wandb)) + + ctx := context.Background() + for _, tc := range []struct { + what string + obj ctrlClient.Object + key types.NamespacedName + }{ + {"Application", &apiv2.Application{}, types.NamespacedName{Name: name, Namespace: wandb.Namespace}}, + {"Role", &rbacv1.Role{}, types.NamespacedName{Name: name, Namespace: wandb.Namespace}}, + {"RoleBinding", &rbacv1.RoleBinding{}, types.NamespacedName{Name: name, Namespace: wandb.Namespace}}, + {"ClusterRole", &rbacv1.ClusterRole{}, types.NamespacedName{Name: clusterName}}, + {"ClusterRoleBinding", &rbacv1.ClusterRoleBinding{}, types.NamespacedName{Name: clusterName}}, + {"Secret", &corev1.Secret{}, types.NamespacedName{Name: watchtowerSecretName(wandb), Namespace: wandb.Namespace}}, + {"ServiceAccount", &corev1.ServiceAccount{}, types.NamespacedName{Name: watchtowerServiceAccountName(wandb), Namespace: wandb.Namespace}}, + } { + err := c.Get(ctx, tc.key, tc.obj) + require.Error(t, err, "%s should have been deleted", tc.what) + } + + require.Nil(t, wandb.Status.WatchtowerStatus) +} + +func TestDeleteWatchtowerLeavesAUserSuppliedServiceAccount(t *testing.T) { + wandb := watchtowerTestCR("wandb", "wandb") + wandb.Spec.Watchtower.ServiceAccount = apiv2.ManagedServiceAccountSpec{ + Create: ptr.To(false), + ServiceAccountName: "byo-account", + } + + sa := &corev1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{ + Name: "byo-account", Namespace: wandb.Namespace, + }} + c := watchtowerTestClient(t, wandb, sa) + + require.NoError(t, deleteWatchtower(context.Background(), c, wandb)) + + require.NoError(t, c.Get(context.Background(), types.NamespacedName{ + Name: "byo-account", Namespace: wandb.Namespace, + }, &corev1.ServiceAccount{}), "a user-supplied ServiceAccount must be left alone") +} + +// --- ingress readiness ------------------------------------------------------ + +func TestIsIngressReady(t *testing.T) { + for _, tc := range []struct { + name string + ingress *networkingv1.Ingress + want bool + }{ + {"nil ingress", nil, false}, + { + "no load balancer entries", + &networkingv1.Ingress{}, + false, + }, + { + // Some controllers append an entry before filling in the address, so a + // non-empty slice is not on its own proof of readiness. + "entry present but no address", + &networkingv1.Ingress{Status: networkingv1.IngressStatus{ + LoadBalancer: networkingv1.IngressLoadBalancerStatus{ + Ingress: []networkingv1.IngressLoadBalancerIngress{{}}, + }, + }}, + false, + }, + { + "IP assigned", + &networkingv1.Ingress{Status: networkingv1.IngressStatus{ + LoadBalancer: networkingv1.IngressLoadBalancerStatus{ + Ingress: []networkingv1.IngressLoadBalancerIngress{{IP: "203.0.113.10"}}, + }, + }}, + true, + }, + { + "hostname assigned", + &networkingv1.Ingress{Status: networkingv1.IngressStatus{ + LoadBalancer: networkingv1.IngressLoadBalancerStatus{ + Ingress: []networkingv1.IngressLoadBalancerIngress{{Hostname: "lb.example.com"}}, + }, + }}, + true, + }, + { + "second entry carries the address", + &networkingv1.Ingress{Status: networkingv1.IngressStatus{ + LoadBalancer: networkingv1.IngressLoadBalancerStatus{ + Ingress: []networkingv1.IngressLoadBalancerIngress{{}, {IP: "203.0.113.11"}}, + }, + }}, + true, + }, + } { + t.Run(tc.name, func(t *testing.T) { + require.Equal(t, tc.want, isIngressReady(tc.ingress)) + }) + } +} diff --git a/internal/crdinstaller/crds/operator/apps.wandb.com_weightsandbiases.yaml b/internal/crdinstaller/crds/operator/apps.wandb.com_weightsandbiases.yaml index 4ee11815..2b344bda 100644 --- a/internal/crdinstaller/crds/operator/apps.wandb.com_weightsandbiases.yaml +++ b/internal/crdinstaller/crds/operator/apps.wandb.com_weightsandbiases.yaml @@ -4405,6 +4405,68 @@ spec: - hostname - version type: object + watchtower: + properties: + authService: + type: string + basePath: + type: string + image: + properties: + digest: + type: string + repository: + type: string + tag: + type: string + type: object + install: + type: boolean + resources: + properties: + claims: + items: + properties: + name: + type: string + request: + type: string + required: + - name + type: object + type: array + x-kubernetes-list-map-keys: + - name + x-kubernetes-list-type: map + limits: + additionalProperties: + anyOf: + - type: integer + - type: string + pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ + x-kubernetes-int-or-string: true + type: object + requests: + additionalProperties: + anyOf: + - type: integer + - type: string + pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ + x-kubernetes-int-or-string: true + type: object + type: object + serviceAccount: + properties: + annotations: + additionalProperties: + type: string + type: object + create: + type: boolean + serviceAccountName: + type: string + type: object + type: object required: - retentionPolicy type: object @@ -4659,6 +4721,10 @@ spec: type: array name: type: string + ready: + type: boolean + required: + - ready type: object kafkaStatus: properties: @@ -6356,6 +6422,19 @@ spec: required: - hostname type: object + watchtowerStatus: + properties: + authService: + type: string + image: + type: string + ready: + type: boolean + url: + type: string + required: + - ready + type: object required: - observedGeneration - ready diff --git a/internal/webhook/v2/weightsandbiases_watchtower_test.go b/internal/webhook/v2/weightsandbiases_watchtower_test.go new file mode 100644 index 00000000..c93da0fd --- /dev/null +++ b/internal/webhook/v2/weightsandbiases_watchtower_test.go @@ -0,0 +1,92 @@ +package v2 + +import ( + "strings" + "testing" + + appsv2 "github.com/wandb/operator/api/v2" + "k8s.io/utils/ptr" +) + +func wandbWithWatchtower(watchtower appsv2.WatchtowerSpec) *appsv2.WeightsAndBiases { + wandb := &appsv2.WeightsAndBiases{} + wandb.Spec.Watchtower = watchtower + return wandb +} + +func TestValidateWatchtowerSpec(t *testing.T) { + cases := []struct { + name string + watchtower appsv2.WatchtowerSpec + wantErr string // substring; "" = accept + }{ + { + // Watchtower is opt-in, so a CR that never mentions it must validate. + name: "disabled by default", + watchtower: appsv2.WatchtowerSpec{}, + }, + { + name: "explicitly disabled ignores other fields", + watchtower: appsv2.WatchtowerSpec{Install: ptr.To(false), BasePath: "no-leading-slash"}, + }, + { + // The common case: enable it and take the defaults. BasePath is optional + // and resolves to /watchtower, so an unset value must be accepted. + name: "enabled with defaults", + watchtower: appsv2.WatchtowerSpec{Install: ptr.To(true)}, + }, + { + name: "explicit base path", + watchtower: appsv2.WatchtowerSpec{Install: ptr.To(true), BasePath: "/watchtower"}, + }, + { + name: "custom base path", + watchtower: appsv2.WatchtowerSpec{Install: ptr.To(true), BasePath: "/admin"}, + }, + { + name: "base path without a leading slash", + watchtower: appsv2.WatchtowerSpec{Install: ptr.To(true), BasePath: "watchtower"}, + wantErr: "must start with '/'", + }, + { + // "/" is the W&B frontend's own path; mounting Watchtower there would + // shadow the app it exists to manage. + name: "base path of root", + watchtower: appsv2.WatchtowerSpec{Install: ptr.To(true), BasePath: "/"}, + wantErr: "must not be '/'", + }, + { + name: "auth service host port", + watchtower: appsv2.WatchtowerSpec{Install: ptr.To(true), AuthService: "api:8081"}, + }, + { + name: "auth service with a scheme", + watchtower: appsv2.WatchtowerSpec{Install: ptr.To(true), AuthService: "http://api:8081"}, + wantErr: "bare host:port", + }, + { + name: "auth service with a path", + watchtower: appsv2.WatchtowerSpec{Install: ptr.To(true), AuthService: "api:8081/oidc"}, + wantErr: "bare host:port", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + errs := validateWatchtowerSpec(wandbWithWatchtower(tc.watchtower)) + + if tc.wantErr == "" { + if len(errs) != 0 { + t.Fatalf("expected the spec to validate, got %v", errs) + } + return + } + if len(errs) == 0 { + t.Fatalf("expected an error containing %q, got none", tc.wantErr) + } + if !strings.Contains(errs.ToAggregate().Error(), tc.wantErr) { + t.Fatalf("expected an error containing %q, got %v", tc.wantErr, errs.ToAggregate()) + } + }) + } +} diff --git a/internal/webhook/v2/weightsandbiases_webhook.go b/internal/webhook/v2/weightsandbiases_webhook.go index e58bb84b..08ec7550 100644 --- a/internal/webhook/v2/weightsandbiases_webhook.go +++ b/internal/webhook/v2/weightsandbiases_webhook.go @@ -110,7 +110,7 @@ func (d *WeightsAndBiasesCustomDefaulter) Default(ctx context.Context, obj runti wandb.Spec.Wandb.InternalServiceAuth.Enabled = ptr.To(true) } - if wandb.Spec.Wandb.InternalServiceAuth.OIDCIssuer == "" && wandb.Spec.Wandb.InternalServiceAuth.Enabled != nil && *wandb.Spec.Wandb.InternalServiceAuth.Enabled{ + if wandb.Spec.Wandb.InternalServiceAuth.OIDCIssuer == "" && wandb.Spec.Wandb.InternalServiceAuth.Enabled != nil && *wandb.Spec.Wandb.InternalServiceAuth.Enabled { wandb.Spec.Wandb.InternalServiceAuth.OIDCIssuer = "https://kubernetes.default.svc.cluster.local" } @@ -335,6 +335,7 @@ func applyClickHouseDefaults(wandb *appsv2.WeightsAndBiases) { } } + func applyManagedServiceAccountDefaults(serviceAccount *appsv2.ManagedServiceAccountSpec, defaultName string) { if serviceAccount.Create == nil { serviceAccount.Create = ptr.To(true) @@ -360,6 +361,7 @@ func validateSpec(_ context.Context, newWandb, oldWandb *appsv2.WeightsAndBiases allErrors = append(allErrors, networkingErrors...) warnings = append(warnings, networkingWarnings...) allErrors = append(allErrors, validateProxySpec(newWandb)...) + allErrors = append(allErrors, validateWatchtowerSpec(newWandb)...) if len(allErrors) == 0 { return warnings, nil @@ -372,6 +374,41 @@ func validateSpec(_ context.Context, newWandb, oldWandb *appsv2.WeightsAndBiases ) } +func validateWatchtowerSpec(wandb *appsv2.WeightsAndBiases) field.ErrorList { + var errors field.ErrorList + + if !wandb.WatchtowerEnabled() { + return errors + } + + watchtower := wandb.Spec.Watchtower + watchtowerPath := field.NewPath("spec").Child("watchtower") + + if basePath := watchtower.BasePath; basePath != "" { + switch { + case !strings.HasPrefix(basePath, "/"): + errors = append(errors, field.Invalid( + watchtowerPath.Child("basePath"), basePath, "must start with '/'", + )) + case strings.Trim(basePath, "/") == "": + errors = append(errors, field.Invalid( + watchtowerPath.Child("basePath"), basePath, "must not be '/', which is served by the W&B frontend", + )) + } + } + + if authService := watchtower.AuthService; authService != "" { + if strings.Contains(authService, "://") || strings.Contains(authService, "/") { + errors = append(errors, field.Invalid( + watchtowerPath.Child("authService"), authService, + "must be a bare host:port, without a scheme or path", + )) + } + } + + return errors +} + func validateChanges(_ context.Context, newWandb *appsv2.WeightsAndBiases, oldWandb *appsv2.WeightsAndBiases) (admission.Warnings, error) { var allErrors field.ErrorList var warnings admission.Warnings From 422b9c8dd9b3566221b5e1319fb089fd30525a14 Mon Sep 17 00:00:00 2001 From: Collin Olander Date: Thu, 20 Aug 2026 12:41:33 -0500 Subject: [PATCH 3/7] fix: secret created after reconcile --- .../controller/reconciler/reconcile_v2.go | 63 +++++++++---------- internal/controller/reconciler/watchtower.go | 4 +- .../controller/reconciler/watchtower_test.go | 34 ++++++++++ 3 files changed, 67 insertions(+), 34 deletions(-) diff --git a/internal/controller/reconciler/reconcile_v2.go b/internal/controller/reconciler/reconcile_v2.go index ba4ae928..debbe418 100644 --- a/internal/controller/reconciler/reconcile_v2.go +++ b/internal/controller/reconciler/reconcile_v2.go @@ -260,6 +260,37 @@ func Reconcile( return ctrl.Result{}, err } + // Networking (gateway or ingress) need to be reconciled before infra gate. + // Watchtower relies on wandb networking for oidc auth + if err := cleanupNetworkingModeResources(ctx, client, wandb); err != nil { + log.Error("Failed to clean up stale networking resources", logx.ErrAttr(err)) + return ctrl.Result{}, err + } + resetInactiveNetworkingStatus(wandb) + + // Reconcile networking + switch wandb.Spec.Networking.Mode { + case apiv2.NetworkingModeGatewayAPI: + wandb.Status.GatewayStatus = nil + if err := reconcileGateway(ctx, client, wandb); err != nil { + log.Error("Failed to reconcile Gateway", logx.ErrAttr(err)) + return ctrl.Result{}, err + } + if err := reconcileInfraHTTPRoutes(ctx, client, wandb, manifest); err != nil { + log.Error("Failed to reconcile infra HTTPRoutes", logx.ErrAttr(err)) + return ctrl.Result{}, err + } + case apiv2.NetworkingModeIngress: + wandb.Status.IngressStatus = nil + if err := reconcileConsolidatedIngress(ctx, client, wandb, manifest); err != nil { + log.Error("Failed to reconcile consolidated Ingress", logx.ErrAttr(err)) + return ctrl.Result{}, err + } + } + // Do not block on Watchtower failure to reconcile + if err := reconcileWatchtower(ctx, client, wandb, manifest); err != nil { + log.Error("Failed to reconcile Watchtower", logx.ErrAttr(err)) + } redisReady := redisAllReady(wandb) mysqlReady := mysqlAllReady(wandb) kafkaReady := wandb.Status.KafkaStatus.Ready @@ -313,37 +344,6 @@ func ReconcileWandbManifest( statusBefore := wandb.DeepCopy().Status - // Networking (gateway or ingress) need to be reconciled before infra gate. - // Watchtower relies on wandb networking for oidc auth - if err := cleanupNetworkingModeResources(ctx, client, wandb); err != nil { - logger.Error(err, "Failed to clean up stale networking resources") - return ctrl.Result{}, err - } - resetInactiveNetworkingStatus(wandb) - - // Reconcile networking - switch wandb.Spec.Networking.Mode { - case apiv2.NetworkingModeGatewayAPI: - wandb.Status.GatewayStatus = nil - if err := reconcileGateway(ctx, client, wandb); err != nil { - logger.Error(err, "Failed to reconcile Gateway") - return ctrl.Result{}, err - } - if err := reconcileInfraHTTPRoutes(ctx, client, wandb, manifest); err != nil { - logger.Error(err, "Failed to reconcile infra HTTPRoutes") - return ctrl.Result{}, err - } - case apiv2.NetworkingModeIngress: - wandb.Status.IngressStatus = nil - if err := reconcileConsolidatedIngress(ctx, client, wandb, manifest); err != nil { - logger.Error(err, "Failed to reconcile consolidated Ingress") - return ctrl.Result{}, err - } - } - // Do not block on Watchtower failure to reconcile - if err := reconcileWatchtower(ctx, client, wandb, manifest); err != nil { - logger.Error(err, "Failed to reconcile Watchtower") - } redisReady := redisAllReady(wandb) mysqlReady := mysqlAllReady(wandb) kafkaReady := wandb.Status.KafkaStatus.Ready @@ -753,7 +753,6 @@ func buildHTTPRouteTemplateForPaths( } } - func resolveHTTPRouteServicePort(app serverManifest.Application) *gatewayv1.PortNumber { if app.Ingress != nil && app.Ingress.ServicePort != "" { port := intstr.Parse(app.Ingress.ServicePort) diff --git a/internal/controller/reconciler/watchtower.go b/internal/controller/reconciler/watchtower.go index a12519d4..374aad07 100644 --- a/internal/controller/reconciler/watchtower.go +++ b/internal/controller/reconciler/watchtower.go @@ -180,8 +180,8 @@ func buildWatchtowerApplication(wandb *apiv2.WeightsAndBiases, authService strin Tolerations: watchtowerTolerations(wandb), Containers: []corev1.Container{ { - Name: watchtowerComponent, - Image: image, + Name: watchtowerComponent, + Image: image, Command: []string{"/watchtower"}, Args: []string{"--port", fmt.Sprintf("%d", watchtowerContainerPort)}, SecurityContext: resolveContainerSecurityContext(), diff --git a/internal/controller/reconciler/watchtower_test.go b/internal/controller/reconciler/watchtower_test.go index f53ba5c8..5b9fa71a 100644 --- a/internal/controller/reconciler/watchtower_test.go +++ b/internal/controller/reconciler/watchtower_test.go @@ -538,3 +538,37 @@ func TestIsIngressReady(t *testing.T) { }) } } + +// --- independence from infrastructure readiness ------------------------------- + +// Watchtower exists to diagnose a broken install, so nothing in its reconcile +// path may depend on the infrastructure being healthy. This is the property that +// the two infra gates in Reconcile/ReconcileWandbManifest kept swallowing: the +// code was correct, but unreachable. +func TestReconcileWatchtowerIgnoresInfraReadiness(t *testing.T) { + t.Setenv(operatorImageEnvVar, testOperatorImage) + + wandb := watchtowerTestCR("wandb", "wandb") + // Declare an object store instance with no ready status behind it. Without a + // declared instance allInstancesReady is vacuously true, and the precondition + // below would pass for the wrong reason. + wandb.Spec.ObjectStore = map[string]apiv2.ObjectStoreSpec{"default": {}} + + require.False(t, wandb.Status.KafkaStatus.Ready, "precondition: kafka unready") + require.False(t, objectStoreAllReady(wandb), "precondition: object store unready") + + c := watchtowerTestClient(t, wandb) + require.NoError(t, reconcileWatchtower(context.Background(), c, wandb, manifestWithOIDC())) + + ctx := context.Background() + ns := wandb.Namespace + + require.NoError(t, c.Get(ctx, types.NamespacedName{Name: watchtowerSecretName(wandb), Namespace: ns}, + &corev1.Secret{}), "the admin password must exist even with infra down") + require.NoError(t, c.Get(ctx, types.NamespacedName{Name: watchtowerName(wandb), Namespace: ns}, + &apiv2.Application{}), "the Application must exist even with infra down") + require.NoError(t, c.Get(ctx, types.NamespacedName{Name: watchtowerServiceAccountName(wandb), Namespace: ns}, + &corev1.ServiceAccount{}), "the ServiceAccount must exist even with infra down") + require.NoError(t, c.Get(ctx, types.NamespacedName{Name: watchtowerName(wandb), Namespace: ns}, + &rbacv1.Role{}), "the Role must exist even with infra down") +} From 4e7e3fe964fd9aaf1bcbace69f1f9451c6c9ad1f Mon Sep 17 00:00:00 2001 From: Collin Olander Date: Thu, 20 Aug 2026 14:45:36 -0500 Subject: [PATCH 4/7] fix: tests, console wiring instead of /watchtower --- api/v2/weightsandbiases_types.go | 2 +- docs/watchtower-deployment.md | 2 +- .../controller/reconciler/reconcile_v2.go | 88 +++++++++++++------ .../controller/reconciler/watchtower_test.go | 17 ++-- ...htsandbiases_controller_networking_test.go | 4 + .../apps.wandb.com_weightsandbiases.yaml | 20 +++++ .../v2/weightsandbiases_watchtower_test.go | 2 +- .../webhook/v2/weightsandbiases_webhook.go | 3 + 8 files changed, 99 insertions(+), 39 deletions(-) diff --git a/api/v2/weightsandbiases_types.go b/api/v2/weightsandbiases_types.go index 7b14fde7..c3f46b20 100644 --- a/api/v2/weightsandbiases_types.go +++ b/api/v2/weightsandbiases_types.go @@ -193,7 +193,7 @@ func (s WatchtowerSpec) GetImage(globalImageRegistry string) string { } const ( - DefaultWatchtowerBasePath = "/watchtower" + DefaultWatchtowerBasePath = "/console" DefaultWatchtowerServiceAccountName = "wandb-watchtower" ) diff --git a/docs/watchtower-deployment.md b/docs/watchtower-deployment.md index 64a80b12..622c4cc8 100644 --- a/docs/watchtower-deployment.md +++ b/docs/watchtower-deployment.md @@ -31,7 +31,7 @@ make docker-build WATCHTOWER_VERSION=0.12.0 ``` The Application the operator synthesizes selects the second entrypoint with -`command: ["/watchtower"]` and `args: ["--port", "8080"]` — the binary's own +`command: ["/console"]` and `args: ["--port", "8080"]` — the binary's own default port is 9090, which would not match the Service or the probes. ### How the operator knows its own image diff --git a/internal/controller/reconciler/reconcile_v2.go b/internal/controller/reconciler/reconcile_v2.go index debbe418..0f78c21f 100644 --- a/internal/controller/reconciler/reconcile_v2.go +++ b/internal/controller/reconciler/reconcile_v2.go @@ -260,37 +260,10 @@ func Reconcile( return ctrl.Result{}, err } - // Networking (gateway or ingress) need to be reconciled before infra gate. - // Watchtower relies on wandb networking for oidc auth - if err := cleanupNetworkingModeResources(ctx, client, wandb); err != nil { - log.Error("Failed to clean up stale networking resources", logx.ErrAttr(err)) + if err := ReconcileNetworkingAndWatchtower(ctx, client, wandb, manifest); err != nil { return ctrl.Result{}, err } - resetInactiveNetworkingStatus(wandb) - // Reconcile networking - switch wandb.Spec.Networking.Mode { - case apiv2.NetworkingModeGatewayAPI: - wandb.Status.GatewayStatus = nil - if err := reconcileGateway(ctx, client, wandb); err != nil { - log.Error("Failed to reconcile Gateway", logx.ErrAttr(err)) - return ctrl.Result{}, err - } - if err := reconcileInfraHTTPRoutes(ctx, client, wandb, manifest); err != nil { - log.Error("Failed to reconcile infra HTTPRoutes", logx.ErrAttr(err)) - return ctrl.Result{}, err - } - case apiv2.NetworkingModeIngress: - wandb.Status.IngressStatus = nil - if err := reconcileConsolidatedIngress(ctx, client, wandb, manifest); err != nil { - log.Error("Failed to reconcile consolidated Ingress", logx.ErrAttr(err)) - return ctrl.Result{}, err - } - } - // Do not block on Watchtower failure to reconcile - if err := reconcileWatchtower(ctx, client, wandb, manifest); err != nil { - log.Error("Failed to reconcile Watchtower", logx.ErrAttr(err)) - } redisReady := redisAllReady(wandb) mysqlReady := mysqlAllReady(wandb) kafkaReady := wandb.Status.KafkaStatus.Ready @@ -329,6 +302,65 @@ func consolidateResults(results []ctrl.Result) ctrl.Result { } } +// ReconcileNetworkingAndWatchtower publishes the W&B app's route — a Gateway plus +// infra HTTPRoutes, or the consolidated Ingress — and then brings up Watchtower. +// +// Reconcile calls this *before* its infrastructure-readiness gate, and that +// placement is the point: Watchtower exists to diagnose a broken install, so it +// has to come up when MySQL, Redis, Kafka, the object store or ClickHouse are not +// ready, which is exactly when Reconcile returns early. Its route has to be +// published for the same reason, so networking moves up with it. Keep this above +// that gate. +// +// A Watchtower failure is logged and stepped over rather than returned: a bad +// image or an RBAC mistake must never stop the install it manages from +// reconciling. +func ReconcileNetworkingAndWatchtower( + ctx context.Context, + client ctrlClient.Client, + wandb *apiv2.WeightsAndBiases, + manifest serverManifest.Manifest, +) error { + ctx, log := logx.WithSlog(ctx, logx.ReconcileInfraV2) + + // Status is flushed here rather than left to ReconcileWandbManifest: that + // function is behind the infrastructure gate, so while infra is unready the + // gateway, ingress and Watchtower summaries would never reach the API — and an + // operator looking for Watchtower's URL during an outage would find nothing. + statusBefore := wandb.DeepCopy().Status + + if err := cleanupNetworkingModeResources(ctx, client, wandb); err != nil { + log.Error("Failed to clean up stale networking resources", logx.ErrAttr(err)) + return err + } + resetInactiveNetworkingStatus(wandb) + + switch wandb.Spec.Networking.Mode { + case apiv2.NetworkingModeGatewayAPI: + wandb.Status.GatewayStatus = nil + if err := reconcileGateway(ctx, client, wandb); err != nil { + log.Error("Failed to reconcile Gateway", logx.ErrAttr(err)) + return err + } + if err := reconcileInfraHTTPRoutes(ctx, client, wandb, manifest); err != nil { + log.Error("Failed to reconcile infra HTTPRoutes", logx.ErrAttr(err)) + return err + } + case apiv2.NetworkingModeIngress: + wandb.Status.IngressStatus = nil + if err := reconcileConsolidatedIngress(ctx, client, wandb, manifest); err != nil { + log.Error("Failed to reconcile consolidated Ingress", logx.ErrAttr(err)) + return err + } + } + + if err := reconcileWatchtower(ctx, client, wandb, manifest); err != nil { + log.Error("Failed to reconcile Watchtower", logx.ErrAttr(err)) + } + + return updateWandbStatusIfChanged(ctx, client, wandb, statusBefore) +} + func ReconcileWandbManifest( ctx context.Context, client ctrlClient.Client, diff --git a/internal/controller/reconciler/watchtower_test.go b/internal/controller/reconciler/watchtower_test.go index 5b9fa71a..15fa9d40 100644 --- a/internal/controller/reconciler/watchtower_test.go +++ b/internal/controller/reconciler/watchtower_test.go @@ -197,7 +197,7 @@ func secretPassword(secret *corev1.Secret) string { func TestWatchtowerEnvReferencesThePasswordSecret(t *testing.T) { wandb := watchtowerTestCR("wandb", "wandb") - env := watchtowerEnv(wandb, "api:8081", "/watchtower") + env := watchtowerEnv(wandb, "api:8081", "/console") byName := map[string]corev1.EnvVar{} for _, e := range env { @@ -212,7 +212,7 @@ func TestWatchtowerEnvReferencesThePasswordSecret(t *testing.T) { require.Equal(t, watchtowerPasswordKey, password.ValueFrom.SecretKeyRef.Key) require.Equal(t, "cluster", byName["WATCHTOWER_MODE"].Value) - require.Equal(t, "/watchtower", byName["WATCHTOWER_BASE_PATH"].Value) + require.Equal(t, "/console", byName["WATCHTOWER_BASE_PATH"].Value) require.Equal(t, "api:8081", byName["WATCHTOWER_AUTH_SERVICE"].Value) require.Equal(t, "wandb", byName["WATCHTOWER_WANDB_NAME"].Value) require.Equal(t, @@ -274,7 +274,7 @@ func TestWatchtowerIngressPathTargetsTheApplicationService(t *testing.T) { path := watchtowerIngressPath(wandb) require.NotNil(t, path) - require.Equal(t, "/watchtower", path.Path) + require.Equal(t, "/console", path.Path) require.Equal(t, networkingv1.PathTypePrefix, *path.PathType) require.Equal(t, watchtowerName(wandb), path.Backend.Service.Name) require.Equal(t, watchtowerContainerPort, path.Backend.Service.Port.Number) @@ -294,9 +294,9 @@ func TestWatchtowerURL(t *testing.T) { basePath string want string }{ - {"adds a scheme", "wandb.example.com", "", "https://wandb.example.com/watchtower"}, - {"keeps an explicit scheme", "http://wandb.example.com", "", "http://wandb.example.com/watchtower"}, - {"strips a trailing slash", "https://wandb.example.com/", "", "https://wandb.example.com/watchtower"}, + {"adds a scheme", "wandb.example.com", "", "https://wandb.example.com/console"}, + {"keeps an explicit scheme", "http://wandb.example.com", "", "http://wandb.example.com/console"}, + {"strips a trailing slash", "https://wandb.example.com/", "", "https://wandb.example.com/console"}, {"honors a custom base path", "wandb.example.com", "/admin", "https://wandb.example.com/admin"}, {"empty hostname yields no URL", "", "", ""}, } { @@ -398,6 +398,7 @@ func TestBuildWatchtowerApplicationSelectsTheWatchtowerEntrypoint(t *testing.T) Spec.PodTemplate.Spec.Containers[0] require.Equal(t, testOperatorImage, container.Image) + // The binary inside the image, not the URL prefix — those are independent. require.Equal(t, []string{"/watchtower"}, container.Command) // The binary defaults to 9090; the Service, container port and probes are 8080. require.Equal(t, []string{"--port", "8080"}, container.Args) @@ -410,8 +411,8 @@ func TestBuildWatchtowerApplicationProbesGoThroughTheBasePath(t *testing.T) { container := buildWatchtowerApplication(wandb, "api:8081", testOperatorImage). Spec.PodTemplate.Spec.Containers[0] - require.Equal(t, "/watchtower/healthz", container.LivenessProbe.HTTPGet.Path) - require.Equal(t, "/watchtower/ready", container.ReadinessProbe.HTTPGet.Path) + require.Equal(t, "/console/healthz", container.LivenessProbe.HTTPGet.Path) + require.Equal(t, "/console/ready", container.ReadinessProbe.HTTPGet.Path) } // --- teardown --------------------------------------------------------------- diff --git a/internal/controller/weightsandbiases_controller_networking_test.go b/internal/controller/weightsandbiases_controller_networking_test.go index ab18c5ed..0857d89c 100644 --- a/internal/controller/weightsandbiases_controller_networking_test.go +++ b/internal/controller/weightsandbiases_controller_networking_test.go @@ -341,6 +341,10 @@ func reconcileNetworkingManifest(ctx context.Context, wandb *apiv2.WeightsAndBia wandbManifest, err := manifest.GetServerManifest(ctx, wandb.Spec.Wandb.ManifestRepository, wandb.Spec.Wandb.Version, nil) Expect(err).NotTo(HaveOccurred()) + // Networking lives above Reconcile's infrastructure gate, in its own function, + // so it has to be driven separately from the manifest reconcile. + Expect(v2.ReconcileNetworkingAndWatchtower(ctx, k8sClient, wandb, wandbManifest)).To(Succeed()) + _, err = v2.ReconcileWandbManifest(ctx, k8sClient, wandb, wandbManifest, telemetry.DefaultTelemetryRuntimeConfig()) Expect(err).NotTo(HaveOccurred()) } diff --git a/internal/crdinstaller/crds/operator/apps.wandb.com_weightsandbiases.yaml b/internal/crdinstaller/crds/operator/apps.wandb.com_weightsandbiases.yaml index 7278f51c..1b0679cb 100644 --- a/internal/crdinstaller/crds/operator/apps.wandb.com_weightsandbiases.yaml +++ b/internal/crdinstaller/crds/operator/apps.wandb.com_weightsandbiases.yaml @@ -3951,6 +3951,26 @@ spec: items: type: string type: array + applications: + additionalProperties: + properties: + autoscaling: + properties: + maxReplicas: + format: int32 + minimum: 1 + type: integer + minReplicas: + format: int32 + minimum: 1 + type: integer + type: object + x-kubernetes-validations: + - message: minReplicas must be <= maxReplicas + rule: '!has(self.minReplicas) || !has(self.maxReplicas) + || self.minReplicas <= self.maxReplicas' + type: object + type: object bucketProxy: type: boolean features: diff --git a/internal/webhook/v2/weightsandbiases_watchtower_test.go b/internal/webhook/v2/weightsandbiases_watchtower_test.go index c93da0fd..9d1430b9 100644 --- a/internal/webhook/v2/weightsandbiases_watchtower_test.go +++ b/internal/webhook/v2/weightsandbiases_watchtower_test.go @@ -37,7 +37,7 @@ func TestValidateWatchtowerSpec(t *testing.T) { }, { name: "explicit base path", - watchtower: appsv2.WatchtowerSpec{Install: ptr.To(true), BasePath: "/watchtower"}, + watchtower: appsv2.WatchtowerSpec{Install: ptr.To(true), BasePath: "/console"}, }, { name: "custom base path", diff --git a/internal/webhook/v2/weightsandbiases_webhook.go b/internal/webhook/v2/weightsandbiases_webhook.go index aff2d241..f6e88078 100644 --- a/internal/webhook/v2/weightsandbiases_webhook.go +++ b/internal/webhook/v2/weightsandbiases_webhook.go @@ -409,6 +409,9 @@ func validateWatchtowerSpec(wandb *appsv2.WeightsAndBiases) field.ErrorList { } } + return errors +} + func validateNotificationSpec(wandb *appsv2.WeightsAndBiases) field.ErrorList { var errors field.ErrorList notifications := wandb.Spec.Wandb.Notifications From d33e2acafcb9727df44acdeb817cbc4910a72fc4 Mon Sep 17 00:00:00 2001 From: Collin Olander Date: Thu, 20 Aug 2026 15:19:54 -0500 Subject: [PATCH 5/7] fix: no watchtower in the CR, just AdminEnabled --- api/v2/weightsandbiases_types.go | 54 +---------- api/v2/zz_generated.deepcopy.go | 44 +-------- .../apps.wandb.com_weightsandbiases.yaml | 64 +------------ internal/controller/reconciler/watchtower.go | 48 +++------- .../controller/reconciler/watchtower_test.go | 73 +-------------- .../apps.wandb.com_weightsandbiases.yaml | 64 +------------ .../v2/weightsandbiases_watchtower_test.go | 92 ------------------- .../webhook/v2/weightsandbiases_webhook.go | 37 -------- 8 files changed, 25 insertions(+), 451 deletions(-) delete mode 100644 internal/webhook/v2/weightsandbiases_watchtower_test.go diff --git a/api/v2/weightsandbiases_types.go b/api/v2/weightsandbiases_types.go index c3f46b20..0f121f6f 100644 --- a/api/v2/weightsandbiases_types.go +++ b/api/v2/weightsandbiases_types.go @@ -17,8 +17,6 @@ limitations under the License. package v2 import ( - "strings" - corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -141,55 +139,7 @@ type WeightsAndBiasesSpec struct { // +optional Networking NetworkingSpec `json:"networking,omitempty"` - Watchtower WatchtowerSpec `json:"watchtower,omitempty"` -} - -type WatchtowerSpec struct { - Install *bool `json:"install,omitempty"` - Image WatchtowerImageSpec `json:"image,omitempty"` - BasePath string `json:"basePath,omitempty"` - AuthService string `json:"authService,omitempty"` - Resources corev1.ResourceRequirements `json:"resources,omitempty"` - ServiceAccount ManagedServiceAccountSpec `json:"serviceAccount,omitempty"` -} - -type WatchtowerImageSpec struct { - // +optional - Repository string `json:"repository,omitempty"` - // +optional - Tag string `json:"tag,omitempty"` - // +optional - Digest string `json:"digest,omitempty"` -} - -func (s WatchtowerSpec) ResolvedBasePath() string { - basePath := s.BasePath - if basePath == "" { - basePath = DefaultWatchtowerBasePath - } - if !strings.HasPrefix(basePath, "/") { - basePath = "/" + basePath - } - return strings.TrimSuffix(basePath, "/") -} - -// GetImage returns an explicitly configured Watchtower image, or "" when none is -// set. Empty is the normal case. Binary shisp inside the operator's own image -func (s WatchtowerSpec) GetImage(globalImageRegistry string) string { - if s.Image.Repository == "" { - return "" - } - repository := s.Image.Repository - if globalImageRegistry != "" { - repository = globalImageRegistry + "/" + repository - } - if s.Image.Digest != "" { - return repository + "@" + s.Image.Digest - } - if s.Image.Tag != "" { - return repository + ":" + s.Image.Tag - } - return repository + AdminConsoleEnabled *bool `json:"adminConsoleEnabled,omitempty"` } const ( @@ -234,7 +184,7 @@ type GlobalSpec struct { } func (w *WeightsAndBiases) WatchtowerEnabled() bool { - return w.Spec.Watchtower.Install != nil && *w.Spec.Watchtower.Install + return w.Spec.AdminConsoleEnabled != nil && *w.Spec.AdminConsoleEnabled } // ProxySpec is the forward-proxy configuration under spec.global.proxy. diff --git a/api/v2/zz_generated.deepcopy.go b/api/v2/zz_generated.deepcopy.go index 225f7837..90716234 100644 --- a/api/v2/zz_generated.deepcopy.go +++ b/api/v2/zz_generated.deepcopy.go @@ -1784,44 +1784,6 @@ func (in *WandbStatus) DeepCopy() *WandbStatus { return out } -// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *WatchtowerImageSpec) DeepCopyInto(out *WatchtowerImageSpec) { - *out = *in -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new WatchtowerImageSpec. -func (in *WatchtowerImageSpec) DeepCopy() *WatchtowerImageSpec { - if in == nil { - return nil - } - out := new(WatchtowerImageSpec) - in.DeepCopyInto(out) - return out -} - -// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *WatchtowerSpec) DeepCopyInto(out *WatchtowerSpec) { - *out = *in - if in.Install != nil { - in, out := &in.Install, &out.Install - *out = new(bool) - **out = **in - } - out.Image = in.Image - in.Resources.DeepCopyInto(&out.Resources) - in.ServiceAccount.DeepCopyInto(&out.ServiceAccount) -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new WatchtowerSpec. -func (in *WatchtowerSpec) DeepCopy() *WatchtowerSpec { - if in == nil { - return nil - } - out := new(WatchtowerSpec) - in.DeepCopyInto(out) - return out -} - // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *WatchtowerStatusSummary) DeepCopyInto(out *WatchtowerStatusSummary) { *out = *in @@ -1948,7 +1910,11 @@ func (in *WeightsAndBiasesSpec) DeepCopyInto(out *WeightsAndBiasesSpec) { } } in.Networking.DeepCopyInto(&out.Networking) - in.Watchtower.DeepCopyInto(&out.Watchtower) + if in.AdminConsoleEnabled != nil { + in, out := &in.AdminConsoleEnabled, &out.AdminConsoleEnabled + *out = new(bool) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new WeightsAndBiasesSpec. diff --git a/config/crd/bases/apps.wandb.com_weightsandbiases.yaml b/config/crd/bases/apps.wandb.com_weightsandbiases.yaml index 1b0679cb..59264247 100644 --- a/config/crd/bases/apps.wandb.com_weightsandbiases.yaml +++ b/config/crd/bases/apps.wandb.com_weightsandbiases.yaml @@ -81,6 +81,8 @@ spec: type: object spec: properties: + adminConsoleEnabled: + type: boolean affinity: properties: nodeAffinity: @@ -4566,68 +4568,6 @@ spec: - hostname - version type: object - watchtower: - properties: - authService: - type: string - basePath: - type: string - image: - properties: - digest: - type: string - repository: - type: string - tag: - type: string - type: object - install: - type: boolean - resources: - properties: - claims: - items: - properties: - name: - type: string - request: - type: string - required: - - name - type: object - type: array - x-kubernetes-list-map-keys: - - name - x-kubernetes-list-type: map - limits: - additionalProperties: - anyOf: - - type: integer - - type: string - pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ - x-kubernetes-int-or-string: true - type: object - requests: - additionalProperties: - anyOf: - - type: integer - - type: string - pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ - x-kubernetes-int-or-string: true - type: object - type: object - serviceAccount: - properties: - annotations: - additionalProperties: - type: string - type: object - create: - type: boolean - serviceAccountName: - type: string - type: object - type: object required: - retentionPolicy type: object diff --git a/internal/controller/reconciler/watchtower.go b/internal/controller/reconciler/watchtower.go index 374aad07..1ce49ab4 100644 --- a/internal/controller/reconciler/watchtower.go +++ b/internal/controller/reconciler/watchtower.go @@ -114,7 +114,7 @@ func watchtowerURL(wandb *apiv2.WeightsAndBiases) string { if !strings.Contains(hostname, "://") { hostname = "https://" + hostname } - return hostname + wandb.Spec.Watchtower.ResolvedBasePath() + return hostname + apiv2.DefaultWatchtowerBasePath } // deleteWatchtower removes every Watchtower resource. The cluster-scoped @@ -136,16 +136,12 @@ func deleteWatchtower(ctx context.Context, c ctrlClient.Client, wandb *apiv2.Wei } } - // The ServiceAccount is owner-referenced and only deleted when the operator - // created it; a user-supplied account is left alone. - if ptr.Deref(wandb.Spec.Watchtower.ServiceAccount.Create, true) { - sa := &corev1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{ - Name: watchtowerServiceAccountName(wandb), - Namespace: wandb.Namespace, - }} - if err := c.Delete(ctx, sa); err != nil && !apiErrors.IsNotFound(err) { - return fmt.Errorf("failed to delete Watchtower ServiceAccount: %w", err) - } + sa := &corev1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{ + Name: watchtowerServiceAccountName(wandb), + Namespace: wandb.Namespace, + }} + if err := c.Delete(ctx, sa); err != nil && !apiErrors.IsNotFound(err) { + return fmt.Errorf("failed to delete Watchtower ServiceAccount: %w", err) } wandb.Status.WatchtowerStatus = nil @@ -153,9 +149,8 @@ func deleteWatchtower(ctx context.Context, c ctrlClient.Client, wandb *apiv2.Wei } func buildWatchtowerApplication(wandb *apiv2.WeightsAndBiases, authService string, image string) *apiv2.Application { - watchtower := wandb.Spec.Watchtower labels := watchtowerLabels(wandb) - basePath := watchtower.ResolvedBasePath() + basePath := apiv2.DefaultWatchtowerBasePath app := &apiv2.Application{ ObjectMeta: metav1.ObjectMeta{ @@ -186,7 +181,7 @@ func buildWatchtowerApplication(wandb *apiv2.WeightsAndBiases, authService strin Args: []string{"--port", fmt.Sprintf("%d", watchtowerContainerPort)}, SecurityContext: resolveContainerSecurityContext(), Env: watchtowerEnv(wandb, authService, basePath), - Resources: watchtowerResources(watchtower), + Resources: watchtowerResources(), Ports: []corev1.ContainerPort{{ Name: watchtowerPortName, ContainerPort: watchtowerContainerPort, @@ -257,10 +252,6 @@ func watchtowerEnv(wandb *apiv2.WeightsAndBiases, authService, basePath string) // cluster administration unauthenticated. spec.watchtower.authService is the // escape hatch for deployments whose manifest does not declare the path. func watchtowerAuthService(wandb *apiv2.WeightsAndBiases, manifest serverManifest.Manifest) (string, error) { - if wandb.Spec.Watchtower.AuthService != "" { - return wandb.Spec.Watchtower.AuthService, nil - } - for _, app := range sortedManifestApplications(manifest) { if app.Ingress == nil || app.Service == nil { continue @@ -308,7 +299,7 @@ func watchtowerIngressPath(wandb *apiv2.WeightsAndBiases) *networkingv1.HTTPIngr } pathType := networkingv1.PathTypePrefix return &networkingv1.HTTPIngressPath{ - Path: wandb.Spec.Watchtower.ResolvedBasePath(), + Path: apiv2.DefaultWatchtowerBasePath, PathType: &pathType, Backend: networkingv1.IngressBackend{ Service: &networkingv1.IngressServiceBackend{ @@ -335,10 +326,7 @@ func watchtowerProbe(path string) *corev1.Probe { // watchtowerResources keeps the UI modest by default; it is an admin console // whose heavy work happens in the cluster, not in this pod. -func watchtowerResources(watchtower apiv2.WatchtowerSpec) corev1.ResourceRequirements { - if len(watchtower.Resources.Requests) > 0 || len(watchtower.Resources.Limits) > 0 { - return watchtower.Resources - } +func watchtowerResources() corev1.ResourceRequirements { return corev1.ResourceRequirements{ Requests: corev1.ResourceList{ corev1.ResourceCPU: resource.MustParse("100m"), @@ -348,9 +336,6 @@ func watchtowerResources(watchtower apiv2.WatchtowerSpec) corev1.ResourceRequire } func watchtowerImage(wandb *apiv2.WeightsAndBiases) (string, error) { - if override := wandb.Spec.Watchtower.GetImage(wandb.Spec.Global.ImageRegistry); override != "" { - return override, nil - } image := os.Getenv(operatorImageEnvVar) if image == "" { return "", fmt.Errorf( @@ -369,9 +354,6 @@ func watchtowerTolerations(wandb *apiv2.WeightsAndBiases) []corev1.Toleration { } func watchtowerServiceAccountName(wandb *apiv2.WeightsAndBiases) string { - if name := wandb.Spec.Watchtower.ServiceAccount.ServiceAccountName; name != "" { - return name - } return watchtowerName(wandb) } @@ -426,10 +408,6 @@ func reconcileWatchtowerSecret(ctx context.Context, c ctrlClient.Client, wandb * } func reconcileWatchtowerServiceAccount(ctx context.Context, c ctrlClient.Client, wandb *apiv2.WeightsAndBiases) error { - if !ptr.Deref(wandb.Spec.Watchtower.ServiceAccount.Create, true) { - return nil - } - serviceAccount := &corev1.ServiceAccount{ ObjectMeta: metav1.ObjectMeta{ Name: watchtowerServiceAccountName(wandb), @@ -438,10 +416,6 @@ func reconcileWatchtowerServiceAccount(ctx context.Context, c ctrlClient.Client, } _, err := controllerruntime.CreateOrUpdate(ctx, c, serviceAccount, func() error { serviceAccount.Labels = utils.MergeMapsStringString(serviceAccount.Labels, watchtowerLabels(wandb)) - serviceAccount.Annotations = utils.MergeMapsStringString( - serviceAccount.Annotations, - wandb.Spec.Watchtower.ServiceAccount.Annotations, - ) // Watchtower talks to the Kubernetes API with this token, so unlike the // W&B application pods it must have one mounted. serviceAccount.AutomountServiceAccountToken = ptr.To(true) diff --git a/internal/controller/reconciler/watchtower_test.go b/internal/controller/reconciler/watchtower_test.go index 15fa9d40..dabf2b6d 100644 --- a/internal/controller/reconciler/watchtower_test.go +++ b/internal/controller/reconciler/watchtower_test.go @@ -47,8 +47,8 @@ func watchtowerTestCR(name, namespace string) *apiv2.WeightsAndBiases { return &apiv2.WeightsAndBiases{ ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace}, Spec: apiv2.WeightsAndBiasesSpec{ - Watchtower: apiv2.WatchtowerSpec{Install: ptr.To(true)}, - Wandb: apiv2.WandbAppSpec{Hostname: "wandb.example.com"}, + AdminConsoleEnabled: ptr.To(true), + Wandb: apiv2.WandbAppSpec{Hostname: "wandb.example.com"}, }, } } @@ -231,19 +231,6 @@ func TestWatchtowerAuthServiceDerivesFromTheOIDCApplication(t *testing.T) { require.NoError(t, err) require.Equal(t, "api:8081", authService) } - -func TestWatchtowerAuthServiceHonorsAnExplicitOverride(t *testing.T) { - wandb := watchtowerTestCR("wandb", "wandb") - wandb.Spec.Watchtower.AuthService = "custom-api:9999" - - authService, err := watchtowerAuthService(wandb, manifestWithOIDC()) - - require.NoError(t, err) - require.Equal(t, "custom-api:9999", authService) -} - -// Failing closed: deploying Watchtower with no way to validate a session would -// leave the app-hostname route unauthenticated. func TestWatchtowerAuthServiceFailsWhenNoApplicationServesOIDC(t *testing.T) { wandb := watchtowerTestCR("wandb", "wandb") manifest := serverManifest.Manifest{ @@ -282,7 +269,7 @@ func TestWatchtowerIngressPathTargetsTheApplicationService(t *testing.T) { func TestWatchtowerIngressPathIsNilWhenDisabled(t *testing.T) { wandb := watchtowerTestCR("wandb", "wandb") - wandb.Spec.Watchtower.Install = ptr.To(false) + wandb.Spec.AdminConsoleEnabled = ptr.To(false) require.Nil(t, watchtowerIngressPath(wandb)) } @@ -297,13 +284,11 @@ func TestWatchtowerURL(t *testing.T) { {"adds a scheme", "wandb.example.com", "", "https://wandb.example.com/console"}, {"keeps an explicit scheme", "http://wandb.example.com", "", "http://wandb.example.com/console"}, {"strips a trailing slash", "https://wandb.example.com/", "", "https://wandb.example.com/console"}, - {"honors a custom base path", "wandb.example.com", "/admin", "https://wandb.example.com/admin"}, {"empty hostname yields no URL", "", "", ""}, } { t.Run(tc.name, func(t *testing.T) { wandb := watchtowerTestCR("wandb", "wandb") wandb.Spec.Wandb.Hostname = tc.hostname - wandb.Spec.Watchtower.BasePath = tc.basePath require.Equal(t, tc.want, watchtowerURL(wandb)) }) @@ -325,36 +310,6 @@ func TestWatchtowerImageFallsBackToTheOperatorImage(t *testing.T) { require.Equal(t, testOperatorImage, image) } -func TestWatchtowerImagePrefersTheSpecOverride(t *testing.T) { - t.Setenv(operatorImageEnvVar, testOperatorImage) - wandb := watchtowerTestCR("wandb", "wandb") - wandb.Spec.Watchtower.Image = apiv2.WatchtowerImageSpec{ - Repository: "custom/watchtower", - Tag: "1.2.3", - } - - image, err := watchtowerImage(wandb) - - require.NoError(t, err) - require.Equal(t, "custom/watchtower:1.2.3", image) -} - -// Air-gapped installs retarget every image at a mirror. -func TestWatchtowerImageOverrideHonorsTheGlobalRegistry(t *testing.T) { - t.Setenv(operatorImageEnvVar, testOperatorImage) - wandb := watchtowerTestCR("wandb", "wandb") - wandb.Spec.Global.ImageRegistry = "registry.internal" - wandb.Spec.Watchtower.Image = apiv2.WatchtowerImageSpec{ - Repository: "custom/watchtower", - Digest: "sha256:abc123", - } - - image, err := watchtowerImage(wandb) - - require.NoError(t, err) - require.Equal(t, "registry.internal/custom/watchtower@sha256:abc123", image) -} - // Failing beats guessing: an empty image would be rejected by the apiserver with // a message that never mentions the missing environment variable. func TestWatchtowerImageFailsWhenOperatorImageIsUnset(t *testing.T) { @@ -461,28 +416,6 @@ func TestDeleteWatchtowerRemovesEveryOwnedObject(t *testing.T) { require.Nil(t, wandb.Status.WatchtowerStatus) } - -func TestDeleteWatchtowerLeavesAUserSuppliedServiceAccount(t *testing.T) { - wandb := watchtowerTestCR("wandb", "wandb") - wandb.Spec.Watchtower.ServiceAccount = apiv2.ManagedServiceAccountSpec{ - Create: ptr.To(false), - ServiceAccountName: "byo-account", - } - - sa := &corev1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{ - Name: "byo-account", Namespace: wandb.Namespace, - }} - c := watchtowerTestClient(t, wandb, sa) - - require.NoError(t, deleteWatchtower(context.Background(), c, wandb)) - - require.NoError(t, c.Get(context.Background(), types.NamespacedName{ - Name: "byo-account", Namespace: wandb.Namespace, - }, &corev1.ServiceAccount{}), "a user-supplied ServiceAccount must be left alone") -} - -// --- ingress readiness ------------------------------------------------------ - func TestIsIngressReady(t *testing.T) { for _, tc := range []struct { name string diff --git a/internal/crdinstaller/crds/operator/apps.wandb.com_weightsandbiases.yaml b/internal/crdinstaller/crds/operator/apps.wandb.com_weightsandbiases.yaml index 1b0679cb..59264247 100644 --- a/internal/crdinstaller/crds/operator/apps.wandb.com_weightsandbiases.yaml +++ b/internal/crdinstaller/crds/operator/apps.wandb.com_weightsandbiases.yaml @@ -81,6 +81,8 @@ spec: type: object spec: properties: + adminConsoleEnabled: + type: boolean affinity: properties: nodeAffinity: @@ -4566,68 +4568,6 @@ spec: - hostname - version type: object - watchtower: - properties: - authService: - type: string - basePath: - type: string - image: - properties: - digest: - type: string - repository: - type: string - tag: - type: string - type: object - install: - type: boolean - resources: - properties: - claims: - items: - properties: - name: - type: string - request: - type: string - required: - - name - type: object - type: array - x-kubernetes-list-map-keys: - - name - x-kubernetes-list-type: map - limits: - additionalProperties: - anyOf: - - type: integer - - type: string - pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ - x-kubernetes-int-or-string: true - type: object - requests: - additionalProperties: - anyOf: - - type: integer - - type: string - pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ - x-kubernetes-int-or-string: true - type: object - type: object - serviceAccount: - properties: - annotations: - additionalProperties: - type: string - type: object - create: - type: boolean - serviceAccountName: - type: string - type: object - type: object required: - retentionPolicy type: object diff --git a/internal/webhook/v2/weightsandbiases_watchtower_test.go b/internal/webhook/v2/weightsandbiases_watchtower_test.go deleted file mode 100644 index 9d1430b9..00000000 --- a/internal/webhook/v2/weightsandbiases_watchtower_test.go +++ /dev/null @@ -1,92 +0,0 @@ -package v2 - -import ( - "strings" - "testing" - - appsv2 "github.com/wandb/operator/api/v2" - "k8s.io/utils/ptr" -) - -func wandbWithWatchtower(watchtower appsv2.WatchtowerSpec) *appsv2.WeightsAndBiases { - wandb := &appsv2.WeightsAndBiases{} - wandb.Spec.Watchtower = watchtower - return wandb -} - -func TestValidateWatchtowerSpec(t *testing.T) { - cases := []struct { - name string - watchtower appsv2.WatchtowerSpec - wantErr string // substring; "" = accept - }{ - { - // Watchtower is opt-in, so a CR that never mentions it must validate. - name: "disabled by default", - watchtower: appsv2.WatchtowerSpec{}, - }, - { - name: "explicitly disabled ignores other fields", - watchtower: appsv2.WatchtowerSpec{Install: ptr.To(false), BasePath: "no-leading-slash"}, - }, - { - // The common case: enable it and take the defaults. BasePath is optional - // and resolves to /watchtower, so an unset value must be accepted. - name: "enabled with defaults", - watchtower: appsv2.WatchtowerSpec{Install: ptr.To(true)}, - }, - { - name: "explicit base path", - watchtower: appsv2.WatchtowerSpec{Install: ptr.To(true), BasePath: "/console"}, - }, - { - name: "custom base path", - watchtower: appsv2.WatchtowerSpec{Install: ptr.To(true), BasePath: "/admin"}, - }, - { - name: "base path without a leading slash", - watchtower: appsv2.WatchtowerSpec{Install: ptr.To(true), BasePath: "watchtower"}, - wantErr: "must start with '/'", - }, - { - // "/" is the W&B frontend's own path; mounting Watchtower there would - // shadow the app it exists to manage. - name: "base path of root", - watchtower: appsv2.WatchtowerSpec{Install: ptr.To(true), BasePath: "/"}, - wantErr: "must not be '/'", - }, - { - name: "auth service host port", - watchtower: appsv2.WatchtowerSpec{Install: ptr.To(true), AuthService: "api:8081"}, - }, - { - name: "auth service with a scheme", - watchtower: appsv2.WatchtowerSpec{Install: ptr.To(true), AuthService: "http://api:8081"}, - wantErr: "bare host:port", - }, - { - name: "auth service with a path", - watchtower: appsv2.WatchtowerSpec{Install: ptr.To(true), AuthService: "api:8081/oidc"}, - wantErr: "bare host:port", - }, - } - - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - errs := validateWatchtowerSpec(wandbWithWatchtower(tc.watchtower)) - - if tc.wantErr == "" { - if len(errs) != 0 { - t.Fatalf("expected the spec to validate, got %v", errs) - } - return - } - if len(errs) == 0 { - t.Fatalf("expected an error containing %q, got none", tc.wantErr) - } - if !strings.Contains(errs.ToAggregate().Error(), tc.wantErr) { - t.Fatalf("expected an error containing %q, got %v", tc.wantErr, errs.ToAggregate()) - } - }) - } -} diff --git a/internal/webhook/v2/weightsandbiases_webhook.go b/internal/webhook/v2/weightsandbiases_webhook.go index f6e88078..4a8ad8d1 100644 --- a/internal/webhook/v2/weightsandbiases_webhook.go +++ b/internal/webhook/v2/weightsandbiases_webhook.go @@ -337,7 +337,6 @@ func applyClickHouseDefaults(wandb *appsv2.WeightsAndBiases) { } } - func applyManagedServiceAccountDefaults(serviceAccount *appsv2.ManagedServiceAccountSpec, defaultName string) { if serviceAccount.Create == nil { serviceAccount.Create = ptr.To(true) @@ -364,7 +363,6 @@ func validateSpec(_ context.Context, newWandb, oldWandb *appsv2.WeightsAndBiases allErrors = append(allErrors, networkingErrors...) warnings = append(warnings, networkingWarnings...) allErrors = append(allErrors, validateProxySpec(newWandb)...) - allErrors = append(allErrors, validateWatchtowerSpec(newWandb)...) if len(allErrors) == 0 { return warnings, nil @@ -377,41 +375,6 @@ func validateSpec(_ context.Context, newWandb, oldWandb *appsv2.WeightsAndBiases ) } -func validateWatchtowerSpec(wandb *appsv2.WeightsAndBiases) field.ErrorList { - var errors field.ErrorList - - if !wandb.WatchtowerEnabled() { - return errors - } - - watchtower := wandb.Spec.Watchtower - watchtowerPath := field.NewPath("spec").Child("watchtower") - - if basePath := watchtower.BasePath; basePath != "" { - switch { - case !strings.HasPrefix(basePath, "/"): - errors = append(errors, field.Invalid( - watchtowerPath.Child("basePath"), basePath, "must start with '/'", - )) - case strings.Trim(basePath, "/") == "": - errors = append(errors, field.Invalid( - watchtowerPath.Child("basePath"), basePath, "must not be '/', which is served by the W&B frontend", - )) - } - } - - if authService := watchtower.AuthService; authService != "" { - if strings.Contains(authService, "://") || strings.Contains(authService, "/") { - errors = append(errors, field.Invalid( - watchtowerPath.Child("authService"), authService, - "must be a bare host:port, without a scheme or path", - )) - } - } - - return errors -} - func validateNotificationSpec(wandb *appsv2.WeightsAndBiases) field.ErrorList { var errors field.ErrorList notifications := wandb.Spec.Wandb.Notifications From 0fdfd4ec240588da43a8dbd9ac47fb90022d7017 Mon Sep 17 00:00:00 2001 From: Collin Olander Date: Fri, 21 Aug 2026 12:56:17 -0500 Subject: [PATCH 6/7] Update docs/watchtower-deployment.md Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --- docs/watchtower-deployment.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/watchtower-deployment.md b/docs/watchtower-deployment.md index 622c4cc8..1df08a94 100644 --- a/docs/watchtower-deployment.md +++ b/docs/watchtower-deployment.md @@ -199,10 +199,10 @@ needed — so the two credentials are not redundant. `secretKeyRef` env vars are resolved at pod creation and never refreshed, so rotation is two steps: -```bash kubectl delete secret -n wandb -watchtower-auth # reconcile regenerates it +until kubectl get secret -n wandb -watchtower-auth \ + -o jsonpath='{.data.password}' | grep -q .; do sleep 1; done kubectl rollout restart deployment/-watchtower -n wandb -``` ## RBAC From 85f14e495d7fd390372ea11a7a973e7dce10fb0e Mon Sep 17 00:00:00 2001 From: Collin Olander Date: Fri, 21 Aug 2026 13:15:23 -0500 Subject: [PATCH 7/7] fix: code rabbit comment updates --- .../controller/reconciler/reconcile_v2.go | 28 +++++++----- internal/controller/reconciler/watchtower.go | 13 +++++- .../controller/reconciler/watchtower_test.go | 44 +++++++++++++++++- ...htsandbiases_controller_networking_test.go | 45 ++++++++++++++++++- 4 files changed, 116 insertions(+), 14 deletions(-) diff --git a/internal/controller/reconciler/reconcile_v2.go b/internal/controller/reconciler/reconcile_v2.go index bfa8675d..b9e6ba90 100644 --- a/internal/controller/reconciler/reconcile_v2.go +++ b/internal/controller/reconciler/reconcile_v2.go @@ -267,9 +267,11 @@ func Reconcile( return ctrl.Result{}, err } - if err := ReconcileNetworkingAndWatchtower(ctx, client, wandb, manifest); err != nil { + res, err = ReconcileNetworkingAndWatchtower(ctx, client, wandb, manifest) + if err != nil { return ctrl.Result{}, err } + ctrlResults = append(ctrlResults, res) redisReady := redisAllReady(wandb) mysqlReady := mysqlAllReady(wandb) @@ -319,15 +321,17 @@ func consolidateResults(results []ctrl.Result) ctrl.Result { // published for the same reason, so networking moves up with it. Keep this above // that gate. // -// A Watchtower failure is logged and stepped over rather than returned: a bad -// image or an RBAC mistake must never stop the install it manages from -// reconciling. +// A Watchtower failure is never returned as an error: a bad image or an RBAC +// mistake must not stop the install it manages from reconciling. It does come +// back as a requeue, though — dropping it outright left a transient failure with +// nothing to retry it, so Watchtower stayed down until an unrelated event +// happened to trigger another pass. func ReconcileNetworkingAndWatchtower( ctx context.Context, client ctrlClient.Client, wandb *apiv2.WeightsAndBiases, manifest serverManifest.Manifest, -) error { +) (ctrl.Result, error) { ctx, log := logx.WithSlog(ctx, logx.ReconcileInfraV2) // Status is flushed here rather than left to ReconcileWandbManifest: that @@ -338,7 +342,7 @@ func ReconcileNetworkingAndWatchtower( if err := cleanupNetworkingModeResources(ctx, client, wandb); err != nil { log.Error("Failed to clean up stale networking resources", logx.ErrAttr(err)) - return err + return ctrl.Result{}, err } resetInactiveNetworkingStatus(wandb) @@ -347,25 +351,29 @@ func ReconcileNetworkingAndWatchtower( wandb.Status.GatewayStatus = nil if err := reconcileGateway(ctx, client, wandb); err != nil { log.Error("Failed to reconcile Gateway", logx.ErrAttr(err)) - return err + return ctrl.Result{}, err } if err := reconcileInfraHTTPRoutes(ctx, client, wandb, manifest); err != nil { log.Error("Failed to reconcile infra HTTPRoutes", logx.ErrAttr(err)) - return err + return ctrl.Result{}, err } case apiv2.NetworkingModeIngress: wandb.Status.IngressStatus = nil if err := reconcileConsolidatedIngress(ctx, client, wandb, manifest); err != nil { log.Error("Failed to reconcile consolidated Ingress", logx.ErrAttr(err)) - return err + return ctrl.Result{}, err } } + // Requeue rather than propagate: the rest of the reconcile must still run, but + // the failure has to be retried by something. + var result ctrl.Result if err := reconcileWatchtower(ctx, client, wandb, manifest); err != nil { log.Error("Failed to reconcile Watchtower", logx.ErrAttr(err)) + result.RequeueAfter = defaultRequeueDuration } - return updateWandbStatusIfChanged(ctx, client, wandb, statusBefore) + return result, updateWandbStatusIfChanged(ctx, client, wandb, statusBefore) } func ReconcileWandbManifest( diff --git a/internal/controller/reconciler/watchtower.go b/internal/controller/reconciler/watchtower.go index 1ce49ab4..b67daae7 100644 --- a/internal/controller/reconciler/watchtower.go +++ b/internal/controller/reconciler/watchtower.go @@ -367,8 +367,19 @@ func watchtowerSecretName(wandb *apiv2.WeightsAndBiases) string { // watchtowerClusterScopedName qualifies cluster-scoped RBAC with the CR's // namespace so two W&B installs in one cluster do not fight over one object. +// +// The separator is "." because a namespace is a DNS-1123 label and so cannot +// contain one, which makes the first "." the unambiguous end of the namespace. +// Joining with "-" would not: namespace "a-b" with CR "c" and namespace "a" with +// CR "b-c" both render "a-b-c", reintroducing the collision this exists to stop. +// FitDefaultInfraName then bounds the result to the 253 characters the apiserver +// allows, hashing the joined key rather than truncating it. func watchtowerClusterScopedName(wandb *apiv2.WeightsAndBiases) string { - return fmt.Sprintf("%s-%s-watchtower", wandb.Namespace, wandb.Name) + return common.FitDefaultInfraName( + wandb.Namespace+"."+wandb.Name, + "-watchtower", + validation.DNS1123SubdomainMaxLength, + ) } func watchtowerLabels(wandb *apiv2.WeightsAndBiases) map[string]string { diff --git a/internal/controller/reconciler/watchtower_test.go b/internal/controller/reconciler/watchtower_test.go index dabf2b6d..a3ff196c 100644 --- a/internal/controller/reconciler/watchtower_test.go +++ b/internal/controller/reconciler/watchtower_test.go @@ -12,6 +12,7 @@ package reconciler import ( "context" + "strings" "testing" "github.com/stretchr/testify/require" @@ -120,8 +121,47 @@ func TestWatchtowerClusterScopedNameIncludesNamespace(t *testing.T) { prod := watchtowerTestCR("wandb", "wandb") staging := watchtowerTestCR("wandb", "wandb-staging") - require.NotEqual(t, watchtowerClusterScopedName(prod), watchtowerClusterScopedName(staging)) - require.Contains(t, watchtowerClusterScopedName(prod), "wandb") + require.Equal(t, "wandb-wandb-b8716-watchtower", watchtowerClusterScopedName(prod)) + require.Equal(t, "wandb-staging-wandb-2b09f-watchtower", watchtowerClusterScopedName(staging)) +} + +// The namespace and CR name are joined with "." rather than "-" because a +// namespace cannot contain one. Joined with "-" these two pairs would both +// render "a-b-c-watchtower" and share a single ClusterRole. +func TestWatchtowerClusterScopedNameSeparatorIsUnambiguous(t *testing.T) { + nsHasHyphen := watchtowerTestCR("c", "a-b") + nameHasHyphen := watchtowerTestCR("b-c", "a") + + require.NotEqual(t, + watchtowerClusterScopedName(nsHasHyphen), + watchtowerClusterScopedName(nameHasHyphen), + ) + require.Equal(t, "a-b-c-fb187-watchtower", watchtowerClusterScopedName(nsHasHyphen)) + require.Equal(t, "a-b-c-ab8bb-watchtower", watchtowerClusterScopedName(nameHasHyphen)) +} + +// ClusterRole names are DNS-1123 subdomains, so a long CR name must be hashed +// down rather than pushed past what the apiserver accepts. +func TestWatchtowerClusterScopedNameStaysWithinSubdomainBudget(t *testing.T) { + longNS := strings.Repeat("n", validation.DNS1123LabelMaxLength) + longName := strings.Repeat("c", validation.DNS1123SubdomainMaxLength) + + first := watchtowerTestCR(longName, longNS) + second := watchtowerTestCR(longName+"x", longNS) + + for _, name := range []string{ + watchtowerClusterScopedName(first), + watchtowerClusterScopedName(second), + } { + require.LessOrEqual(t, len(name), validation.DNS1123SubdomainMaxLength) + require.Empty(t, validation.IsDNS1123Subdomain(name), "name %q is not a valid subdomain", name) + } + + // Truncation alone would collapse these onto one name; the hash keeps them apart. + require.NotEqual(t, + watchtowerClusterScopedName(first), + watchtowerClusterScopedName(second), + ) } // --- the generated admin password ------------------------------------------- diff --git a/internal/controller/weightsandbiases_controller_networking_test.go b/internal/controller/weightsandbiases_controller_networking_test.go index 0857d89c..fcc38d41 100644 --- a/internal/controller/weightsandbiases_controller_networking_test.go +++ b/internal/controller/weightsandbiases_controller_networking_test.go @@ -15,6 +15,7 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" ) @@ -219,6 +220,47 @@ var _ = Describe("WeightsAndBiases Networking", func() { Expect(wandb.Status.IngressStatus.LoadBalancerIngress).To(HaveLen(1)) Expect(wandb.Status.IngressStatus.LoadBalancerIngress[0].IP).To(Equal("34.118.10.1")) }) + + // Watchtower must not be able to take down the install it manages, but a + // failure still has to be retried by something — dropping it outright left a + // transient error with nothing to pick it back up. + It("requeues without failing the reconcile when Watchtower cannot be reconciled", func() { + ctx := context.Background() + wandbName := "network-watchtower-failure" + ingressClassName := "nginx" + + wandb, service := newNetworkingWandb(wandbName, "") + wandb.Spec.AdminConsoleEnabled = ptr.To(true) + wandb.Spec.Networking = apiv2.NetworkingSpec{ + Mode: apiv2.NetworkingModeIngress, + Ingress: &apiv2.IngressConfig{IngressClassName: &ingressClassName}, + } + Expect(k8sClient.Create(ctx, wandb)).To(Succeed()) + Expect(k8sClient.Create(ctx, service)).To(Succeed()) + DeferCleanup(deleteIfPresent, ctx, wandb) + + // OPERATOR_IMAGE is what tells the reconciler which image carries the + // Watchtower binary; unset, watchtowerImage() fails. + GinkgoT().Setenv("OPERATOR_IMAGE", "") + + wandb = markWandbReadyForNetworking(ctx, wandbName, wandbNamespace) + wandbManifest, err := manifest.GetServerManifest(ctx, wandb.Spec.Wandb.ManifestRepository, wandb.Spec.Wandb.Version, nil) + Expect(err).NotTo(HaveOccurred()) + + result, err := v2.ReconcileNetworkingAndWatchtower(ctx, k8sClient, wandb, wandbManifest) + + // The error is swallowed so the rest of the reconcile still runs... + Expect(err).NotTo(HaveOccurred()) + // ...but it comes back as a requeue so the failure is retried. + Expect(result.RequeueAfter).To(BeNumerically(">", 0)) + + // And networking was still published — the failure did not abort early. + ingress := &networkingv1.Ingress{} + Expect(k8sClient.Get(ctx, types.NamespacedName{ + Name: wandbName, + Namespace: wandbNamespace, + }, ingress)).To(Succeed()) + }) }) func newNetworkingWandb(name string, infraNamespace string) (*apiv2.WeightsAndBiases, *corev1.Service) { @@ -343,7 +385,8 @@ func reconcileNetworkingManifest(ctx context.Context, wandb *apiv2.WeightsAndBia // Networking lives above Reconcile's infrastructure gate, in its own function, // so it has to be driven separately from the manifest reconcile. - Expect(v2.ReconcileNetworkingAndWatchtower(ctx, k8sClient, wandb, wandbManifest)).To(Succeed()) + _, err = v2.ReconcileNetworkingAndWatchtower(ctx, k8sClient, wandb, wandbManifest) + Expect(err).NotTo(HaveOccurred()) _, err = v2.ReconcileWandbManifest(ctx, k8sClient, wandb, wandbManifest, telemetry.DefaultTelemetryRuntimeConfig()) Expect(err).NotTo(HaveOccurred())