diff --git a/.claude/agents/operator-reviewer.md b/.claude/agents/operator-reviewer.md new file mode 100644 index 0000000..326097a --- /dev/null +++ b/.claude/agents/operator-reviewer.md @@ -0,0 +1,15 @@ +--- +name: operator-reviewer +description: Reviews Kubernetes operator PRs for controller-runtime correctness, reconcile semantics, and API design +tools: Read, Grep, Glob, Bash +--- +You are a senior reviewer specializing in Kubernetes operators. +Review with focus on: +- Reconcile idempotency and requeue behavior; no state assumptions between reconciles +- Informer cache reads vs direct API reads; stale-cache races +- Finalizer handling, deletion flow, orphaned resources +- CRD schema evolution, conversion webhooks, status subresource / conditions conventions +- RBAC minimality vs what the controller actually touches +- Leader election, watch predicates, event filtering for churn reduction +- Go: context propagation, error wrapping, client.Object handling +Output: findings ranked by severity, with file:line refs. No praise padding. \ No newline at end of file diff --git a/.claude/settings.json b/.claude/settings.json index 06e584b..fa14aa8 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -71,7 +71,19 @@ "Bash(kind load *)", "Bash(make deploy *)", "Bash(kubectl -n egress-proxies-operator-system rollout status deploy/egress-proxies-operator-controller-manager --timeout=120s)", - "Bash(kubectl -n egress-proxies-operator-system rollout restart deploy/egress-proxies-operator-controller-manager)" + "Bash(kubectl -n egress-proxies-operator-system rollout restart deploy/egress-proxies-operator-controller-manager)", + "Bash(git -C /Users/jan.novak/srv/go/egress-proxies-operator add docs/plans/2026-08-11-1742-gcp-provider-verbose-logging.md)", + "Bash(git -C /Users/jan.novak/srv/go/egress-proxies-operator commit -m 'Add plan: verbose V-level logging in the GCP provider *)", + "Bash(echo \"exit: $?\")", + "Bash(echo \"tests exit: $?\")", + "Bash(./bin/manager --version)", + "Bash(./bin/manager-stamped --version)", + "Bash(./bin/manager-pkg --version)", + "Bash(docker run *)", + "Bash(kubectl -n egress-proxies-operator-system get pods -o wide)", + "Bash(kubectl -n egress-proxies-operator-system get deploy egress-proxies-operator-controller-manager -o jsonpath='{.spec.template.spec.containers[0].args}')", + "Bash(kubectl -n egress-proxies-operator-system logs deploy/egress-proxies-operator-controller-manager)", + "Bash(python3 -c \"import json; d=json.load\\(open\\('docs/deploy/sa_key.json'\\)\\); print\\(d.get\\('type'\\), d.get\\('client_email'\\)\\)\")" ], "additionalDirectories": [ "/Users/jan.novak/srv/go/egress-proxies-operator/.claude", diff --git a/.gitignore b/.gitignore index 9f0f3a1..6a93a35 100644 --- a/.gitignore +++ b/.gitignore @@ -28,3 +28,6 @@ go.work # Kubeconfig might contain secrets *.kubeconfig + +# GCP service-account keys (created per docs/gcp-in-specific-project.md) +sa_key.json diff --git a/docs/gcp-in-specific-project.md b/docs/gcp-in-specific-project.md new file mode 100644 index 0000000..7544b6b --- /dev/null +++ b/docs/gcp-in-specific-project.md @@ -0,0 +1,255 @@ +## Project egress-proxy + +```bash +PROJECT_ID=egress-proxy + +# 1. Create the service account +gcloud iam service-accounts create proxy-operator \ + --project ${PROJECT_ID} \ + --display-name "egress-proxies-operator" + +# Output: +# Created service account [proxy-operator]. +# Service account email: proxy-operator@egress-proxy.iam.gserviceaccount.com + +# 2. Grant compute.instanceAdmin.v1 on the project +gcloud projects add-iam-policy-binding ${PROJECT_ID} \ + --member "serviceAccount:proxy-operator@${PROJECT_ID}.iam.gserviceaccount.com" \ + --role roles/compute.instanceAdmin.v1 + +# Output: +# --------- +# Updated IAM policy for project [egress-proxy]. +# bindings: +# - members: +# - serviceAccount:proxy-operator@egress-proxy.iam.gserviceaccount.com +# role: roles/compute.instanceAdmin.v1 +# - members: +# - serviceAccount:541231138892@cloudservices.gserviceaccount.com +# role: roles/compute.instanceGroupManagerServiceAgent +# - members: +# - serviceAccount:service-541231138892@compute-system.iam.gserviceaccount.com +# role: roles/compute.serviceAgent +# - members: +# - user:admin@fujultimate.cz +# role: roles/owner +# etag: BwZYuFXko24= +# version: 1 + +# 3. Create the JSON key (this is what goes into the Secret) +SA_KEY_PATH=sa_key.json +gcloud iam service-accounts keys create $SA_KEY_PATH \ + --iam-account proxy-operator@${PROJECT_ID}.iam.gserviceaccount.com + +# output: +# created key [fdff85174a8e80bbd684e76c4d9fe28e2f4b2ddf] of type [json] as [sa_key.json] for [proxy-operator@egress-proxy.iam.gserviceaccount.com] + +# 4. A **firewall rule**: created VMs get network tag `proxy-operator` (the +# default; configurable as `gcp.networkTag`), an ephemeral external IP, +# and Squid listening on 3128. + +gcloud compute firewall-rules create allow-proxy-operator \ + --project $PROJECT_ID \ + --network default \ + --allow tcp:3128 \ + --target-tags proxy-operator \ + --source-ranges 94.230.145.216/32 +``` + +## Phase 2 - resources in kube + +```bash +SA_KEY_PATH=sa_key.json +kubectl -n egress-proxies-operator-system create secret generic gcp-credentials \ + --from-file=key.json=$SA_KEY_PATH +``` + + +## Appendix - full manifests + +```bash +# crawl CR +kubectl apply -f - <<'EOF' +apiVersion: crawl.example.com/v1alpha1 +kind: Proxy +metadata: + name: proxy-gcp-sample +spec: + mode: Managed + provider: gcp-eu # must match a provider NAME in providers.yaml + placement: + zone: europe-west1-b + machineType: e2-micro + # debian-cloud images have no cloud-init, so spec.cloudInit (passed as + # user-data metadata) would be silently ignored there. Ubuntu images do. + image: projects/ubuntu-os-cloud/global/images/family/ubuntu-2404-lts-amd64 + port: 3128 + cloudInit: + inline: | + #cloud-config + package_update: true + packages: + - squid + write_files: + - path: /etc/squid/conf.d/proxy-operator.conf + content: | + http_access allow all + via off + forwarded_for off + runcmd: + - systemctl restart squid + attributes: + geo: eu + purpose: crawl +EOF + + + +# configmap +kubectl apply -f - <<'EOF' +apiVersion: v1 +data: + providers.yaml: | + providers: + - name: kubernetes + type: kubernetes + - name: gcp-eu # spec.provider on a Proxy refers to this NAME, not the type + type: gcp + gcp: + project: egress-proxy + # network: default # these three default as shown + # networkTag: proxy-operator + # diskSizeGb: 10 +kind: ConfigMap +metadata: + labels: + app.kubernetes.io/managed-by: kustomize + app.kubernetes.io/name: egress-proxies-operator + name: egress-proxies-operator-providers-config + namespace: egress-proxies-operator-system +EOF + +# operator deployment +kubectl apply -f - <<'EOF' +apiVersion: apps/v1 +kind: Deployment +metadata: + annotations: + deployment.kubernetes.io/revision: "2" + labels: + app.kubernetes.io/managed-by: kustomize + app.kubernetes.io/name: egress-proxies-operator + control-plane: controller-manager + name: egress-proxies-operator-controller-manager + namespace: egress-proxies-operator-system +spec: + progressDeadlineSeconds: 600 + replicas: 1 + revisionHistoryLimit: 10 + selector: + matchLabels: + app.kubernetes.io/name: egress-proxies-operator + control-plane: controller-manager + strategy: + rollingUpdate: + maxSurge: 25% + maxUnavailable: 25% + type: RollingUpdate + template: + metadata: + annotations: + kubectl.kubernetes.io/default-container: manager + labels: + app.kubernetes.io/name: egress-proxies-operator + control-plane: controller-manager + spec: + containers: + - args: + - --metrics-bind-address=:8443 + - --leader-elect + - --health-probe-bind-address=:8081 + - --providers-config=/etc/proxy-operator/providers.yaml + command: + - /manager + env: + - name: DISCOVERY_TOKEN + valueFrom: + secretKeyRef: + key: token + name: discovery-token + optional: true + - name: GOOGLE_APPLICATION_CREDENTIALS + value: /var/secrets/gcp/key.json + image: egress-proxies-operator:dev + imagePullPolicy: IfNotPresent + livenessProbe: + failureThreshold: 3 + httpGet: + path: /healthz + port: 8081 + scheme: HTTP + initialDelaySeconds: 15 + periodSeconds: 20 + successThreshold: 1 + timeoutSeconds: 1 + name: manager + ports: + - containerPort: 8081 + name: health + protocol: TCP + - containerPort: 8090 + name: discovery + protocol: TCP + readinessProbe: + failureThreshold: 3 + httpGet: + path: /readyz + port: 8081 + scheme: HTTP + initialDelaySeconds: 5 + periodSeconds: 10 + successThreshold: 1 + timeoutSeconds: 1 + resources: + limits: + cpu: 500m + memory: 128Mi + requests: + cpu: 10m + memory: 64Mi + securityContext: + allowPrivilegeEscalation: false + capabilities: + drop: + - ALL + readOnlyRootFilesystem: true + terminationMessagePath: /dev/termination-log + terminationMessagePolicy: File + volumeMounts: + - mountPath: /etc/proxy-operator + name: providers-config + readOnly: true + - mountPath: /var/secrets/gcp + name: gcp-credentials + readOnly: true + dnsPolicy: ClusterFirst + restartPolicy: Always + schedulerName: default-scheduler + securityContext: + runAsNonRoot: true + seccompProfile: + type: RuntimeDefault + serviceAccount: egress-proxies-operator-controller-manager + serviceAccountName: egress-proxies-operator-controller-manager + terminationGracePeriodSeconds: 10 + volumes: + - configMap: + defaultMode: 420 + name: egress-proxies-operator-providers-config + name: providers-config + - name: gcp-credentials + secret: + defaultMode: 420 + secretName: gcp-credentials +EOF +``` \ No newline at end of file diff --git a/docs/gcp-vm-validation.md b/docs/gcp-vm-validation.md new file mode 100644 index 0000000..eddb741 --- /dev/null +++ b/docs/gcp-vm-validation.md @@ -0,0 +1,158 @@ +# Validating real VM creation on GCP + +Recipe for wiring the GCP provider into a live cluster and watching a +`Proxy` CR create a real Compute Engine VM. Angle brackets mark values you +supply: ``, ``, ``, ``, +``. + +The one important fact up front: **the operator takes no GCP credentials +through its own config.** The client is built with Application Default +Credentials (`internal/provider/gcp/gcp.go`, `New()`); there is no +key-file field in the providers config. The only secret to prepare is a +service-account JSON key, injected via the standard +`GOOGLE_APPLICATION_CREDENTIALS` mechanism. On GKE you would use workload +identity instead and skip the key entirely. + +## 1. GCP-side prerequisites (prepared outside the cluster) + +1. A project — `` — with the **Compute Engine API enabled**. +2. A **service account** with `roles/compute.instanceAdmin.v1` on the + project. The operator only calls instances + `Insert`/`Get`/`Delete`/`AggregatedList` and does not attach a service + account to the VMs it creates, so no `iam.serviceAccountUser` is + needed. +3. A **JSON key** for that service account, saved at ``. +4. A **firewall rule**: created VMs get network tag `proxy-operator` (the + default; configurable as `gcp.networkTag`), an ephemeral external IP, + and Squid listening on 3128. + + ```sh + gcloud compute firewall-rules create allow-proxy-operator \ + --project \ + --network default \ + --allow tcp:3128 \ + --target-tags proxy-operator \ + --source-ranges /32 + ``` + + The source range must cover the cluster's egress IP — the operator's + CONNECT health probes originate there, and without the rule the Proxy + hangs at `Running`/unhealthy instead of reaching `Ready`. ⚠️ The + sample cloud-init configures `http_access allow all`, so on a public + IP this is an open proxy — keep the source ranges tight. + +## 2. Create the credentials Secret + +Namespace is `egress-proxies-operator-system` after kustomize prefixing: + +```sh +kubectl -n egress-proxies-operator-system create secret generic gcp-credentials \ + --from-file=key.json= +``` + +## 3. Add a GCP entry to the providers ConfigMap + +Edit `config/manager/providers_config.yaml` (mounted at +`/etc/proxy-operator/providers.yaml`): + +```yaml +providers: + - name: kubernetes + type: kubernetes + - name: gcp-eu # spec.provider on a Proxy refers to this NAME, not the type + type: gcp + gcp: + project: + # network: default # these three default as shown + # networkTag: proxy-operator + # diskSizeGb: 10 +``` + +The config is validated fail-fast at startup — a typo shows up +immediately in the manager log, not on first use. + +## 4. Mount the Secret and point ADC at it + +In `config/manager/manager.yaml`, add to the manager container: + +```yaml +env: + - name: GOOGLE_APPLICATION_CREDENTIALS + value: /var/secrets/gcp/key.json +volumeMounts: + - name: gcp-credentials + mountPath: /var/secrets/gcp + readOnly: true +volumes: + - name: gcp-credentials + secret: + secretName: gcp-credentials +``` + +(`volumeMounts` merges into the existing container list; `volumes` into +the existing pod-level list.) + +## 5. Deploy and create the Proxy + +```sh +make deploy IMG= +``` + +`config/samples/proxy_gcp.yaml` is usable as-is once `spec.provider` +matches the name from step 3. All three placement fields are mandatory +for GCP — a missing one sets the Proxy to `Failed` with a message naming +it: + +```yaml +spec: + mode: Managed + provider: gcp-eu + placement: + zone: # e.g. europe-west1-b + machineType: e2-micro + image: projects/debian-cloud/global/images/family/debian-12 +``` + +```sh +kubectl apply -f config/samples/proxy_gcp.yaml +``` + +## 6. What you should see + +```sh +kubectl get proxy -w +``` + +`Provisioning` → `Running` (VM's external IP published in status) → +`Ready` (CONNECT health probe succeeded through the public IP). Then: + +```sh +# the VM exists and carries the GC labels +gcloud compute instances list --project \ + --filter 'labels.proxy-operator-managed=yes' + +# the proxy actually tunnels — should print the VM's external IP +curl -x http://:3128 https://ifconfig.me +``` + +Cleanup — the finalizer deletes the VM: + +```sh +kubectl delete proxy proxy-gcp-sample +gcloud compute instances list --project # should be empty again +``` + +## Gotchas + +- **The orphan GC sweeps the whole project**: any VM labeled + `proxy-operator-managed=yes` whose UID does not match a live Proxy CR + in *this* cluster is deleted once past the age threshold. Do not point + two operator installs at the same project, and do not hand-create VMs + with that label. +- **VM creation is fire-and-forget** — the provider never waits on the + insert operation; progress is discovered by polling `Get`. A quota + error or bad image name surfaces on the Proxy's status/conditions a + reconcile later, not synchronously. `kubectl describe proxy` is the + place to look when something stalls. +- **e2-micro costs pennies but is not free everywhere** — remember to + delete the CR (or check `gcloud compute instances list`) when done. diff --git a/docs/reviews/2026-08-10-1134-proxy-operator-pr-review.md b/docs/reviews/2026-08-10-1134-proxy-operator-pr-review.md new file mode 100644 index 0000000..fd36eac --- /dev/null +++ b/docs/reviews/2026-08-10-1134-proxy-operator-pr-review.md @@ -0,0 +1,131 @@ +# PR review findings: feat/proxy-operator + +**Created:** 2026-08-10 11:34 +**Scope:** `origin/main...feat/proxy-operator` (merge-base 076bc66, 25 commits, ~80 files) +**Reviewers:** `go-operator-reviewer` + `operator-reviewer` agents; findings consolidated, most severe first. Check off items as they're processed. + +Both reviewers rated the core reconcile architecture sound: single status writer with one +deferred patch, finalizer added before any provider call, Get-before-RemoveFinalizer on +delete, CEL immutability rules correctly split to avoid the oldSelf-on-CREATE trap, +leader-election gating on destructive runnables, GC tombstone rules (MinAge, UID-less +instances never deleted). + +## Merge-blockers + +- [ ] **Discovery leases proxies with an empty IP** — found independently by both reviewers. + `internal/discovery/handlers.go:151`, `internal/controller/proxy_controller.go:226` + During instance replacement (and the Get→NotFound recovery path) the reconciler clears + `status.ip` but only the create branch removes the `Healthy` condition, and the health + engine prunes state for empty-host proxies so nothing refreshes it. For the whole + delete→recreate window (minutes on GCP), `isHealthy` still returns true and + `handleAcquireLease` grants `201 Created` with `"ip": ""`, burning a `MaxLeases` slot. + **Fix:** add `EffectiveHost() != ""` to `isHealthy` (covers list + acquire), and + remove/downgrade `Healthy` wherever `status.IP` is cleared. + +- [ ] **Orphan GC deletes other installations' fleets in a shared GCP project.** + `internal/provider/gcp/insert.go:57`, `internal/gc/gc.go:93` + Instances are tagged only `proxy-operator-managed=true` + CR UID; the sweeper deletes any + tagged instance whose UID isn't in *its own cluster's* Proxy list. Two clusters sharing a + GCP project delete each other's VMs every GC interval in a permanent loop. + **Fix:** add an installation-identity label (cluster/deployment ID) set by both providers + and filtered on in `ListByTag`. + +- [ ] **Permanent-error latch wedges proxies on failures that aren't spec-caused.** + `internal/controller/proxy_controller.go:126` + Latch keys on `observedGeneration == generation`, but two failure inputs live outside the + spec: an unconfigured provider (config fix + restart doesn't bump generation, and + `spec.provider` is CEL-immutable → stuck `Failed` short of deleting the CR) and resolved + Secret content (Secret fix enqueues a reconcile that short-circuits at the latch before + re-resolving cloud-init). + **Fix:** latch should also consider current spec-hash / provider availability. + +## Worth fixing + +- [ ] **Deletion-path failures invisible in status** — flagged by both reviewers. + `internal/controller/proxy_controller.go:319`, `:266` + `deletionFailure` swallows `ErrQuotaExceeded` (nil error, no status write); unconfigured + provider returns a bare error forever. A Proxy wedged in `Deleting` shows nothing in + `kubectl describe`. Stage `setProvisioned(p, False, ReasonDeleting, ...)` before returning. + Also: `Delete` is resubmitted on every `DeletionPoll` pass, churning GCP quota — a state + check on the `Get` result would avoid it. + +- [ ] **Lost providerID on `setSpecHash` conflict.** + `internal/controller/proxy_controller.go:161` + On Update conflict the function returns before `p.Status.ProviderID = id`, so the deferred + patch persists an empty providerID for a just-created instance. Self-heals via GC. + **Fix:** set `p.Status.ProviderID = id` before returning the error (one line). + +- [ ] **Terminating pods still report `StateRunning`.** + `internal/provider/kubernetes/kubernetes.go:151` + A pod with a deletionTimestamp keeps `phase=Running` + `PodIP` while terminating, so drift + reconcile republishes `Provisioned=True` and discovery keeps leasing a dying pod. + **Fix:** map non-zero `pod.DeletionTimestamp` to `StateTerminated` in `instanceFromPod`. + +- [ ] **Stale-cache spec-hash race deletes the freshly created replacement instance.** + `internal/controller/proxy_controller.go:176` + Instance name derives from CR UID, so old and new instances share a providerID. A reconcile + served a cached object from before a just-completed replacement re-enters `replaceInstance` + and deletes the *new* healthy instance. Converges, but destroys a good instance. + **Fix:** re-read uncached before the destructive branch, or compare `inst.CreatedAt` + against the annotation-update time. + +- [ ] **`observedGeneration` written before the generation is actually processed.** + `internal/controller/status.go:114` + Set unconditionally in `patchStatusIfChanged`, including on the finalizer-add pass and + `resolveCloudInit` failures — misleads kstatus-style tooling. Set it only once the state + machine has genuinely evaluated the spec. + +- [ ] **No event filtering on the Proxy watch.** + `internal/controller/proxy_controller.go:402` + Every self-inflicted status patch triggers a follow-up reconcile with an extra cloud `Get`, + roughly doubling provider read traffic. Caution: a plain `GenerationChangedPredicate` + breaks the finalizer flow (relies on its own Update event to re-enter) — needs a + status-only/resourceVersion-only filter or an explicit requeue in the finalizer pass. + +- [ ] **Unlabelled cloud-init Secrets produce a misleading NotFound with endless backoff.** + `internal/controller/proxy_controller.go:344`, `cmd/main.go:210` + The label-restricted cache turns "exists but missing `crawl.example.com/cloud-init=true`" + into `CloudInitError: not found`. Mention the label requirement in the condition message, + or read via uncached `APIReader` and validate the label explicitly. + +- [ ] **RBAC over-grant.** + `config/rbac/role.yaml:25` + `create;delete` on `proxies` is scaffold residue (controller never creates/deletes CRs); + cluster-wide `pods create/delete` and `secrets get/list/watch` apply even when only the GCP + provider is configured — pod rules belong in an optional kustomize component. + +## Simplifications + +- [ ] **Delete `internal/provider/registry`** — 14 lines of logic, one caller + (`cmd/main.go:148`); fold `Build`/`Constructor` into the composition root. Also fixes the + two-sources-of-truth problem: `internal/provider/config.go:84` hardcodes + `"kubernetes"`/`"gcp"` while `registry.Build` dispatches through a caller-supplied map — + validate against the constructor map instead. Net −1 package, −44 lines, −92 test lines. + +- [ ] **Collapse `LeaseStore` interface to `*lease.Store`.** + `internal/discovery/server.go:28` + Single implementation and not a test seam (tests wire the real `lease.NewStore`). + Keep `HealthSnapshotter` and `instancesAPI` — those are genuine seams. + +- [ ] **Replace metrics nil-guards with no-op defaults.** + `internal/health/engine.go:68`, `internal/discovery/server.go:38`, + `internal/provider/metrics.go:8` + Keep the interfaces (legit "no prometheus in domain packages" rationale) but default the + fields to a no-op impl — `provider.WithMetrics` already dereferences unconditionally, so + the guards are inconsistent anyway. + +## Nice-to-have + +- [ ] Add an `OwnerReference` to provider pods (`internal/provider/kubernetes/pod.go:24`) — + free cascading deletion if the finalizer is ever bypassed; `CreateRequest` already carries + Namespace/ProxyName/UID. +- [ ] `Close()` the GCP `*compute.InstancesClient` (`internal/provider/gcp/gcp.go:89`) — + harmless today, a leak the moment providers are rebuilt on config reload. +- [ ] Fix `.golangci` config: it references a missing `logcheck` plugin, so the linter only + runs with the project config disabled. +- [ ] External-mode endpoint edits don't reset health-engine counters + (`internal/health/engine.go:206` keys on name+UID): flipping `endpoint.host` keeps the old + host's `Healthy=True` for `failureThreshold × interval`. Arguably a replacement, not a flap. +- [ ] `init()` funcs at `api/v1alpha1/proxy_types.go:353` and `cmd/main.go:67` conflict with + the repo's "no `init()`" convention; kubebuilder-idiomatic, but scheme registration could + use the scaffold's `SchemeBuilder.Register` at package var scope.