From 5212ccea72e4a11c5609f4862983f19435695cff Mon Sep 17 00:00:00 2001 From: Robert Eggl Date: Tue, 28 Jul 2026 22:17:01 +0200 Subject: [PATCH 1/2] feat: add safe log URL templates Signed-off-by: Codex --- README.md | 6 ++ charts/github-deployment-bridge/README.md | 1 + .../templates/deployment.yaml | 2 + .../values.schema.json | 1 + charts/github-deployment-bridge/values.yaml | 3 + docs/configuration/environment.md | 65 ++++++++++++++++++- docs/configuration/helm-values.md | 1 + docs/install/helm.md | 5 +- docs/operations/grafana.md | 9 +++ docs/operations/runbook.md | 13 ++++ internal/config/config.go | 30 +++++++-- internal/config/config_test.go | 34 ++++++++++ internal/deployment/reporter.go | 9 ++- internal/deployment/reporter_test.go | 27 ++++++++ pkg/metadata/resolve.go | 11 ++-- 15 files changed, 199 insertions(+), 18 deletions(-) diff --git a/README.md b/README.md index 46ad2e5..7adfc21 100644 --- a/README.md +++ b/README.md @@ -81,6 +81,12 @@ PATs are not supported. Details: [docs/install/github-app.md](docs/install/githu Env vars and Helm mapping: [docs/configuration/](docs/configuration/). +GitHub prominently exposes only `environment_url` and status `log_url`. Point +`config.logURLTemplate` at Grafana Loki Explore, a Grafana dashboard, or another +Flux/Kubernetes dashboard; enable `config.logURLTemplateEscape` for safe +placeholder substitution. Copy-paste presets and Kustomization, HelmRelease, +and monorepo examples are in [Log URL placeholders](docs/configuration/environment.md#log-url-placeholders). + PVC / SQLite at `/data/cache.db` deduplicates `(owner, repo, environment, commitSHA, deploymentName)` across restarts - see [docs/install/persistence.md](docs/install/persistence.md). diff --git a/charts/github-deployment-bridge/README.md b/charts/github-deployment-bridge/README.md index 078ddfa..bde8409 100644 --- a/charts/github-deployment-bridge/README.md +++ b/charts/github-deployment-bridge/README.md @@ -50,6 +50,7 @@ helm upgrade --install github-deployment-bridge \ | `config.environmentURL` | `""` | Optional URL attached to deployment statuses | | `config.description` | `""` | Optional default deployment description (empty → `Deployed by FluxCD`) | | `config.logURLTemplate` | `""` | Optional log URL template (`{sha}`, `{namespace}`, `{name}`, `{service}`, `{environment}`, `{cluster}`) | +| `config.logURLTemplateEscape` | `false` | Percent-encode substituted log URL values | | `github.existingSecret` | `""` | Secret with `app-id`, `installation-id`, `private-key` | | `persistence.enabled` | `true` | PVC for SQLite deduplication cache (keep on in production) | | `networkPolicy.enabled` | `false` | Opt-in NetworkPolicy | diff --git a/charts/github-deployment-bridge/templates/deployment.yaml b/charts/github-deployment-bridge/templates/deployment.yaml index 0452ce2..ed7e035 100644 --- a/charts/github-deployment-bridge/templates/deployment.yaml +++ b/charts/github-deployment-bridge/templates/deployment.yaml @@ -68,6 +68,8 @@ spec: value: {{ .Values.config.description | quote }} - name: LOG_URL_TEMPLATE value: {{ .Values.config.logURLTemplate | quote }} + - name: LOG_URL_TEMPLATE_ESCAPE + value: {{ .Values.config.logURLTemplateEscape | quote }} - name: LOG_LEVEL value: {{ .Values.config.logLevel | default "info" | quote }} - name: METRICS_ADDR diff --git a/charts/github-deployment-bridge/values.schema.json b/charts/github-deployment-bridge/values.schema.json index 09bc935..e24e5d2 100644 --- a/charts/github-deployment-bridge/values.schema.json +++ b/charts/github-deployment-bridge/values.schema.json @@ -184,6 +184,7 @@ "environmentURL": { "type": "string" }, "description": { "type": "string" }, "logURLTemplate": { "type": "string" }, + "logURLTemplateEscape": { "type": "boolean" }, "logLevel": { "type": "string", "enum": ["debug", "info", "warn", "error"] diff --git a/charts/github-deployment-bridge/values.yaml b/charts/github-deployment-bridge/values.yaml index 88657ee..164fbc6 100644 --- a/charts/github-deployment-bridge/values.yaml +++ b/charts/github-deployment-bridge/values.yaml @@ -146,6 +146,9 @@ config: # {environment} {cluster}. Per-workload annotation log-url overrides this # and supports the same placeholders. logURLTemplate: "" + # Percent-encode every substituted placeholder value. The default false + # preserves the historical literal-substitution behavior. + logURLTemplateEscape: false # slog level: debug | info | warn | error logLevel: info leaderElection: true diff --git a/docs/configuration/environment.md b/docs/configuration/environment.md index 2a2dab1..d0fff6a 100644 --- a/docs/configuration/environment.md +++ b/docs/configuration/environment.md @@ -15,6 +15,7 @@ SPDX-License-Identifier: Apache-2.0 | `ENVIRONMENT_URL` | no | | Default HTTPS URL on deployment statuses (overridable) | | `DESCRIPTION` | no | | Default GitHub Deployment description (overridable). Empty → `Deployed by FluxCD` | | `LOG_URL_TEMPLATE` | no | | Default log URL template (overridable). Placeholders: `{sha}`, `{namespace}`, `{name}`, `{service}`, `{environment}`, `{cluster}` | +| `LOG_URL_TEMPLATE_ESCAPE` | no | `false` | Percent-encode substituted values. Enable for templates containing path segments or query parameters; `false` preserves existing literal expansion. | | `LOG_LEVEL` | no | `info` | slog level: `debug`, `info`, `warn`, or `error` | | `METRICS_ADDR` | no | `:8080` | Prometheus metrics + convenience probes | | `PROBE_ADDR` | no | `:8081` | controller-runtime health probes | @@ -42,10 +43,72 @@ annotation both expand these tokens (annotation wins when set): | `{environment}` | Resolved GitHub Deployment environment | | `{cluster}` | Resolved cluster name | -Example (Grafana Loki Explore per service): +Only GitHub's `environment_url` and Deployment Status `log_url` are prominent +operational links. The bridge deliberately keeps the Deployment payload small; +use `log_url` to send operators to the system that already owns the rich data. + +Set `LOG_URL_TEMPLATE_ESCAPE=true` (Helm: `config.logURLTemplateEscape: true`) +for the presets below. Each substituted value is percent-encoded without +changing the template's URL syntax. The final result must be an absolute HTTPS +URL. An invalid result is omitted with a warning; deployment reporting continues. + +## Copy-paste presets + +Replace only the hostname, datasource/dashboard identifiers, and any fixed +dashboard path. + +### Grafana Loki Explore ```text https://grafana.example.com/a/grafana-lokiexplore-app/explore/service/{name}/logs?from=now-1h&to=now&var-ds=loki&var-filters=service_name%7C%3D%7C{name}&var-filters=namespace%7C%3D%7C{namespace} ``` +### Grafana dashboard + +```text +https://grafana.example.com/d/workload/logs?var-cluster={cluster}&var-namespace={namespace}&var-service={service}&var-environment={environment}&var-sha={sha} +``` + +### Generic Flux/Kubernetes dashboard + +```text +https://ops.example.com/clusters/{cluster}/namespaces/{namespace}/workloads/{name}?environment={environment}&service={service}&revision={sha} +``` + +## Workload overrides and monorepos + +The annotation remains a template and takes precedence over the global value. +This Kustomization-managed Deployment uses the global preset and gives one +monorepo workload its own deployment identity: + +```yaml +apiVersion: apps/v1 +kind: Deployment +metadata: + name: storefront-api + namespace: storefront + annotations: + github-deployment-bridge.io/auto-report: "true" + github-deployment-bridge.io/deployment-name: "storefront-api" + github-deployment-bridge.io/service: "api" +``` + +A HelmRelease-managed StatefulSet can override the link (annotations belong on +the discovered workload, usually via the chart's pod/workload annotation values): + +```yaml +apiVersion: apps/v1 +kind: StatefulSet +metadata: + name: checkout-worker + namespace: checkout + annotations: + github-deployment-bridge.io/auto-report: "true" + github-deployment-bridge.io/deployment-name: "checkout-worker" + github-deployment-bridge.io/log-url: "https://grafana.example.com/d/workload/logs?var-cluster={cluster}&var-namespace={namespace}&var-service={service}&var-sha={sha}" +``` + +`deployment-name` separates cache/supersession identity for repositories with +multiple workloads; it does not add a new visible GitHub payload field. + See also: [Helm values map](./helm-values.md) diff --git a/docs/configuration/helm-values.md b/docs/configuration/helm-values.md index 63db4af..4befa24 100644 --- a/docs/configuration/helm-values.md +++ b/docs/configuration/helm-values.md @@ -14,6 +14,7 @@ SPDX-License-Identifier: Apache-2.0 | `config.environmentURL` | `ENVIRONMENT_URL` | | `config.description` | `DESCRIPTION` | | `config.logURLTemplate` | `LOG_URL_TEMPLATE` | +| `config.logURLTemplateEscape` | `LOG_URL_TEMPLATE_ESCAPE` | | `config.logLevel` | `LOG_LEVEL` | | `config.leaderElection` | `LEADER_ELECTION` | | `config.githubBaseURL` | `GITHUB_BASE_URL` | diff --git a/docs/install/helm.md b/docs/install/helm.md index 6786ea6..cff8362 100644 --- a/docs/install/helm.md +++ b/docs/install/helm.md @@ -22,7 +22,8 @@ helm upgrade --install github-deployment-bridge \ --set config.clusterName=production-eu \ --set config.environment=production \ --set config.environmentURL=https://app.example.com \ - --set config.logURLTemplate='https://grafana.example.com/explore?commit={sha}' + --set config.logURLTemplate='https://grafana.example.com/explore?commit={sha}' \ + --set config.logURLTemplateEscape=true ``` Per-service Loki Explore links (common for Grafana) can use workload placeholders @@ -54,6 +55,7 @@ config: environmentURL: https://app.example.com # Placeholders: {sha} {namespace} {name} {service} {environment} {cluster} logURLTemplate: https://grafana.example.com/explore?commit={sha} + logURLTemplateEscape: true logLevel: info github: @@ -150,6 +152,7 @@ reference (env vars, Helm map, metrics, registries). | `config.environmentURL` | `ENVIRONMENT_URL` | Optional URL on deployment statuses | | `config.description` | `DESCRIPTION` | Optional default deployment description (default `Deployed by FluxCD`) | | `config.logURLTemplate` | `LOG_URL_TEMPLATE` | Optional log URL template (`{sha}`, `{namespace}`, `{name}`, `{service}`, `{environment}`, `{cluster}`) | +| `config.logURLTemplateEscape` | `LOG_URL_TEMPLATE_ESCAPE` | Percent-encode substituted values (recommended) | | `config.logLevel` | `LOG_LEVEL` | `debug` / `info` / `warn` / `error` | | `config.githubBaseURL` | `GITHUB_BASE_URL` | GitHub Enterprise base URL | diff --git a/docs/operations/grafana.md b/docs/operations/grafana.md index d28194a..a6142b6 100644 --- a/docs/operations/grafana.md +++ b/docs/operations/grafana.md @@ -33,6 +33,15 @@ prometheusRule: release: kube-prometheus-stack ``` +## Link GitHub deployments to Grafana + +Use the bridge's `log_url` template to make GitHub's **View deployment logs** +link open the relevant Grafana view. Ready-to-use Loki Explore and dashboard +templates are in [Log URL placeholders](../configuration/environment.md#copy-paste-presets). +Enable `config.logURLTemplateEscape: true` so workload-derived values cannot +alter the URL query or path. This links to Grafana; it does not add Grafana data +or provider-specific fields to the GitHub Deployment payload. + ## Import ### Grafana UI diff --git a/docs/operations/runbook.md b/docs/operations/runbook.md index 557ae4b..a9189c8 100644 --- a/docs/operations/runbook.md +++ b/docs/operations/runbook.md @@ -108,3 +108,16 @@ When alerts are quiet but GitHub shows no Deployment: bridges share a repo. 4. Dedup cache: same `(owner, repo, environment, commit, deploymentName)` skips duplicates - expected after retries. + +## Deployment log link is missing (no alert) + +1. Confirm `LOG_URL_TEMPLATE` / `config.logURLTemplate`, or the workload's + `github-deployment-bridge.io/log-url` override, is non-empty. +2. Search bridge logs for `log URL template expanded to an invalid HTTPS URL`. + Invalid links are intentionally omitted without failing reconciliation or + GitHub deployment/status reporting. +3. The expanded URL must be absolute HTTPS. Enable + `LOG_URL_TEMPLATE_ESCAPE=true` / `config.logURLTemplateEscape: true` when + workload values appear in paths or query parameters. +4. Copy a known-good [preset](../configuration/environment.md#copy-paste-presets) + and replace its host and dashboard/datasource identifiers. diff --git a/internal/config/config.go b/internal/config/config.go index 4abd4fb..c7c84d1 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -8,6 +8,7 @@ package config import ( "fmt" "log/slog" + "net/url" "strings" "time" @@ -23,6 +24,7 @@ type Config struct { EnvironmentURL string `envconfig:"ENVIRONMENT_URL"` Description string `envconfig:"DESCRIPTION"` LogURLTemplate string `envconfig:"LOG_URL_TEMPLATE"` + LogURLTemplateEscape bool `envconfig:"LOG_URL_TEMPLATE_ESCAPE" default:"false"` LogLevel string `envconfig:"LOG_LEVEL" default:"info"` MetricsAddr string `envconfig:"METRICS_ADDR" default:":8080"` ProbeAddr string `envconfig:"PROBE_ADDR" default:":8081"` @@ -101,7 +103,7 @@ type LogURLVars struct { // ExpandLogURL substitutes placeholders in LOG_URL_TEMPLATE. func (c Config) ExpandLogURL(v LogURLVars) string { - return ExpandLogURLTemplate(c.LogURLTemplate, v) + return ExpandLogURLTemplateEscaped(c.LogURLTemplate, v, c.LogURLTemplateEscape) } // ExpandLogURLTemplate substitutes log URL placeholders in tmpl. @@ -109,6 +111,12 @@ func (c Config) ExpandLogURL(v LogURLVars) string { // Supported: {sha}, {namespace}, {name}, {service}, {environment}, {cluster}. // {service} falls back to {name} when Service is empty. func ExpandLogURLTemplate(tmpl string, v LogURLVars) string { + return ExpandLogURLTemplateEscaped(tmpl, v, false) +} + +// ExpandLogURLTemplateEscaped substitutes placeholders, optionally percent-encoding +// each value so it is safe in both URL paths and query parameters. +func ExpandLogURLTemplateEscaped(tmpl string, v LogURLVars, escape bool) string { if strings.TrimSpace(tmpl) == "" { return "" } @@ -116,12 +124,20 @@ func ExpandLogURLTemplate(tmpl string, v LogURLVars) string { if service == "" { service = v.Name } + value := func(s string) string { + if escape { + // QueryEscape covers URL delimiters (including '/', '&', '?' and '#'). + // Use %20 rather than '+' so the result is also safe in path segments. + return strings.ReplaceAll(url.QueryEscape(s), "+", "%20") + } + return s + } return strings.NewReplacer( - "{sha}", v.SHA, - "{namespace}", v.Namespace, - "{name}", v.Name, - "{service}", service, - "{environment}", v.Environment, - "{cluster}", v.Cluster, + "{sha}", value(v.SHA), + "{namespace}", value(v.Namespace), + "{name}", value(v.Name), + "{service}", value(service), + "{environment}", value(v.Environment), + "{cluster}", value(v.Cluster), ).Replace(tmpl) } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 5dc2e34..3e4abe4 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -97,6 +97,18 @@ func TestExpandLogURLTemplate(t *testing.T) { vars: config.LogURLVars{Name: "backend"}, want: "https://logs.example.com/s/backend", }, + { + name: "grafana dashboard preset", + tmpl: "https://grafana.example.com/d/workload/logs?var-cluster={cluster}&var-namespace={namespace}&var-service={service}&var-environment={environment}&var-sha={sha}", + vars: config.LogURLVars{SHA: "deadbeef", Namespace: "apps", Name: "api", Cluster: "prod", Environment: "production"}, + want: "https://grafana.example.com/d/workload/logs?var-cluster=prod&var-namespace=apps&var-service=api&var-environment=production&var-sha=deadbeef", + }, + { + name: "generic flux kubernetes dashboard preset", + tmpl: "https://ops.example.com/clusters/{cluster}/namespaces/{namespace}/workloads/{name}?environment={environment}&service={service}&revision={sha}", + vars: config.LogURLVars{SHA: "abc", Namespace: "payments", Name: "worker", Service: "payments-worker", Cluster: "eu-1", Environment: "staging"}, + want: "https://ops.example.com/clusters/eu-1/namespaces/payments/workloads/worker?environment=staging&service=payments-worker&revision=abc", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -108,6 +120,28 @@ func TestExpandLogURLTemplate(t *testing.T) { } } +func TestExpandLogURLTemplateEscaped(t *testing.T) { + t.Parallel() + tests := []struct { + name string + tmpl string + vars config.LogURLVars + escape bool + want string + }{ + {"backwards compatible literal values", "https://logs.example/{namespace}?q={service}", config.LogURLVars{Namespace: "team one", Service: "api/core"}, false, "https://logs.example/team one?q=api/core"}, + {"escape path and query values", "https://logs.example/{namespace}?q={service}&env={environment}", config.LogURLVars{Namespace: "team one", Service: "api/core?x=1", Environment: "preview & qa"}, true, "https://logs.example/team%20one?q=api%2Fcore%3Fx%3D1&env=preview%20%26%20qa"}, + {"escape fallback service", "https://logs.example/?service={service}", config.LogURLVars{Name: "api/canary"}, true, "https://logs.example/?service=api%2Fcanary"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := config.ExpandLogURLTemplateEscaped(tt.tmpl, tt.vars, tt.escape); got != tt.want { + t.Fatalf("ExpandLogURLTemplateEscaped = %q, want %q", got, tt.want) + } + }) + } +} + func TestLogLevel(t *testing.T) { base := func(t *testing.T) { t.Helper() diff --git a/internal/deployment/reporter.go b/internal/deployment/reporter.go index c7debb1..f6320d4 100644 --- a/internal/deployment/reporter.go +++ b/internal/deployment/reporter.go @@ -434,15 +434,18 @@ func (r *Reporter) resolveImage(ctx context.Context, img WorkloadImage) (metadat if !fromAnnotation { tmpl = r.cfg.LogURLTemplate } - if logURL := config.ExpandLogURLTemplate(tmpl, vars); logURL != "" { + if logURL := config.ExpandLogURLTemplateEscaped(tmpl, vars, r.cfg.LogURLTemplateEscape); logURL != "" { if !metadata.ValidHTTPSURL(logURL) { src := "LOG_URL_TEMPLATE" if fromAnnotation { src = metadata.AnnotationLogURL } - return metadata.Resolved{}, "", retry.Permanent(fmt.Errorf("%s expanded to invalid HTTPS URL %q", src, logURL)) + r.log.Warn("log URL template expanded to an invalid HTTPS URL; omitting log_url", + "source", src, "url", logURL, "workload_kind", img.Kind, + "workload_namespace", img.Namespace, "workload_name", img.Name) + } else { + resolved.LogURL = logURL } - resolved.LogURL = logURL } return resolved, ociMeta.Digest, nil diff --git a/internal/deployment/reporter_test.go b/internal/deployment/reporter_test.go index 634dce5..28d1e6e 100644 --- a/internal/deployment/reporter_test.go +++ b/internal/deployment/reporter_test.go @@ -5,10 +5,12 @@ package deployment_test import ( + "bytes" "context" "fmt" "log/slog" "path/filepath" + "strings" "sync" "testing" "time" @@ -225,6 +227,31 @@ func TestReporterLogURLAnnotationTemplate(t *testing.T) { } } +func TestReporterInvalidExpandedLogURLIsOmitted(t *testing.T) { + t.Parallel() + store, err := cache.Open(filepath.Join(t.TempDir(), "cache.db")) + if err != nil { + t.Fatalf("open cache: %v", err) + } + t.Cleanup(func() { _ = store.Close() }) + + var logs bytes.Buffer + logger := slog.New(slog.NewTextHandler(&logs, nil)) + g := &fakeGitHub{} + r := deployment.NewReporter(config.Config{ClusterName: "prod", Environment: "production", LogURLTemplate: "://bad/{name}"}, store, + &fakeRegistry{meta: ocilabels.Metadata{Source: "https://github.com/example/backend", Revision: "deadbeef", Digest: "sha256:abc"}}, g, nil, logger, "test") + + if err := r.Report(context.Background(), sampleInput(deployment.PhaseSuccess)); err != nil { + t.Fatalf("invalid log URL must not fail reporting: %v", err) + } + if len(g.statuses) != 1 || g.statuses[0].LogURL != "" { + t.Fatalf("statuses = %#v, want one status without log URL", g.statuses) + } + if !strings.Contains(logs.String(), "invalid HTTPS URL; omitting log_url") || !strings.Contains(logs.String(), "source=LOG_URL_TEMPLATE") { + t.Fatalf("warning log = %q", logs.String()) + } +} + func TestReporterPayloadOptionalAnnotations(t *testing.T) { t.Parallel() r, g := newTestReporter(t, nil) diff --git a/pkg/metadata/resolve.go b/pkg/metadata/resolve.go index 681547e..7ca9981 100644 --- a/pkg/metadata/resolve.go +++ b/pkg/metadata/resolve.go @@ -100,12 +100,11 @@ func Resolve(annotations map[string]string, oci ocilabels.Metadata, defaults Def return Resolved{}, err } - // Annotation log-url may still contain placeholders; the reporter expands them. - // defaults.LogURL is unused (template applied in the reporter after resolve). - logURL, err := resolveHTTPSURL(ann[AnnotationLogURL], defaults.LogURL, AnnotationLogURL) - if err != nil { - return Resolved{}, err - } + // log-url is a template, not a URL yet. The reporter expands and validates it + // non-fatally so a broken operational link cannot block deployment reporting. + // defaults.LogURL is retained for API compatibility; runtime templates are + // selected by the reporter. + logURL := firstNonEmpty(ann[AnnotationLogURL], defaults.LogURL) description := firstNonEmpty(ann[AnnotationDescription], defaults.Description) if description == "" { From b4e33a9f959aec86a80fb4ae3416078944688fb8 Mon Sep 17 00:00:00 2001 From: Robert Eggl Date: Tue, 28 Jul 2026 22:25:36 +0200 Subject: [PATCH 2/2] chore: resolve conflicts with main Signed-off-by: Codex --- README.md | 2 +- .../templates/deployment.yaml | 3 + .../templates/secret.yaml | 4 +- .../templates/validate.yaml | 2 +- .../values.schema.json | 1 + charts/github-deployment-bridge/values.yaml | 5 +- cmd/bridge/main.go | 15 +- docs/configuration/environment.md | 3 +- docs/configuration/github-app.md | 6 + docs/configuration/helm-values.md | 3 +- docs/configuration/metrics.md | 1 + docs/configuration/secrets.md | 4 +- docs/install/github-app.md | 14 +- docs/install/secrets.md | 11 +- internal/cache/cache.go | 60 +++++ internal/cache/cache_test.go | 14 ++ internal/config/config.go | 48 ++-- internal/config/config_test.go | 21 ++ internal/github/client.go | 232 +++++++++++++++--- internal/github/installation_test.go | 115 +++++++++ internal/metrics/metrics.go | 25 +- 21 files changed, 505 insertions(+), 84 deletions(-) create mode 100644 internal/github/installation_test.go diff --git a/README.md b/README.md index 7adfc21..abb6c63 100644 --- a/README.md +++ b/README.md @@ -99,7 +99,7 @@ PVC / SQLite at `/data/cache.db` deduplicates | `/healthz` | Liveness | | `/readyz` | Readiness | -Metrics include `deployments_created_total`, `deployment_status_updates_total`, `deployment_failures_total`, `deployment_errors_total`, `deployment_duplicates_skipped_total`, `deployment_inactive_total`, `github_api_requests_total`, `github_api_failures_total`, `github_api_latency_seconds`, and `oci_requests_total`. +Metrics include `deployments_created_total`, `deployment_status_updates_total`, `deployment_failures_total`, `deployment_errors_total`, `deployment_duplicates_skipped_total`, `deployment_inactive_total`, `github_api_requests_total`, `github_api_failures_total`, `github_api_latency_seconds`, `github_installation_resolutions_total`, and `oci_requests_total`. Helm: optional `serviceMonitor.enabled` / `prometheusRule.enabled` for Prometheus Operator; restrict scrape peers with `networkPolicy` + `metricsFrom` diff --git a/charts/github-deployment-bridge/templates/deployment.yaml b/charts/github-deployment-bridge/templates/deployment.yaml index ed7e035..f961a6e 100644 --- a/charts/github-deployment-bridge/templates/deployment.yaml +++ b/charts/github-deployment-bridge/templates/deployment.yaml @@ -94,6 +94,9 @@ spec: secretKeyRef: name: {{ include "github-deployment-bridge.secretName" . }} key: installation-id + optional: true + - name: GITHUB_INSTALLATION_CACHE_TTL + value: {{ .Values.config.githubInstallationCacheTTL | quote }} - name: GITHUB_PRIVATE_KEY_PATH value: /github/private-key.pem {{- if .Values.config.githubBaseURL }} diff --git a/charts/github-deployment-bridge/templates/secret.yaml b/charts/github-deployment-bridge/templates/secret.yaml index 5735e6f..6e7b547 100644 --- a/charts/github-deployment-bridge/templates/secret.yaml +++ b/charts/github-deployment-bridge/templates/secret.yaml @@ -11,7 +11,9 @@ metadata: type: Opaque stringData: app-id: {{ required "github.appId or github.existingSecret is required" .Values.github.appId | quote }} - installation-id: {{ required "github.installationId or github.existingSecret is required" .Values.github.installationId | quote }} + {{- with .Values.github.installationId }} + installation-id: {{ . | quote }} + {{- end }} private-key: | {{ required "github.privateKey or github.existingSecret is required" .Values.github.privateKey | indent 4 }} {{- end }} diff --git a/charts/github-deployment-bridge/templates/validate.yaml b/charts/github-deployment-bridge/templates/validate.yaml index 65eaf23..8205dbc 100644 --- a/charts/github-deployment-bridge/templates/validate.yaml +++ b/charts/github-deployment-bridge/templates/validate.yaml @@ -7,5 +7,5 @@ SPDX-License-Identifier: Apache-2.0 {{- fail (printf "github-deployment-bridge: replicaCount=%d with persistence.enabled=true is unsupported. The SQLite deduplication cache at /data/cache.db allows only one writer (even with ReadWriteMany). Set replicaCount: 1 (production default) or disable persistence only for ephemeral dev clusters. Multi-replica HA would require a shared database backend; this chart intentionally stays single-writer." (int .Values.replicaCount)) }} {{- end }} {{- if and (not .Values.github.existingSecret) (not .Values.github.allowInsecureValues) }} -{{- fail "github-deployment-bridge: set github.existingSecret to an externally managed Secret (recommended). Inline github.appId / installationId / privateKey are stored in the Helm release Secret - only allowed when github.allowInsecureValues=true (local/dev)." }} +{{- fail "github-deployment-bridge: set github.existingSecret to an externally managed Secret (recommended). Inline github.appId / privateKey are stored in the Helm release Secret - only allowed when github.allowInsecureValues=true (local/dev)." }} {{- end }} diff --git a/charts/github-deployment-bridge/values.schema.json b/charts/github-deployment-bridge/values.schema.json index e24e5d2..1def261 100644 --- a/charts/github-deployment-bridge/values.schema.json +++ b/charts/github-deployment-bridge/values.schema.json @@ -191,6 +191,7 @@ }, "leaderElection": { "type": "boolean" }, "githubBaseURL": { "type": "string" }, + "githubInstallationCacheTTL": { "type": "string", "minLength": 1 }, "retry": { "type": "object", "additionalProperties": false, diff --git a/charts/github-deployment-bridge/values.yaml b/charts/github-deployment-bridge/values.yaml index 164fbc6..bc95dc4 100644 --- a/charts/github-deployment-bridge/values.yaml +++ b/charts/github-deployment-bridge/values.yaml @@ -153,6 +153,8 @@ config: logLevel: info leaderElection: true githubBaseURL: "" + # TTL for owner-to-installation mappings persisted in SQLite. + githubInstallationCacheTTL: 1h # GitHub API and OCI registry retry backoff (maps to RETRY_* env vars). retry: maxAttempts: 5 @@ -162,13 +164,14 @@ config: github: # Name of an existing Secret containing: - # app-id, installation-id, private-key + # app-id, private-key (installation-id is optional as an explicit override) existingSecret: "" # Or provide values directly. Requires allowInsecureValues=true because the # PEM is stored in the Helm release Secret (sh.helm.release.v1.*) as well as # the chart-managed Secret - not recommended for production. allowInsecureValues: false appId: "" + # Optional fixed installation override. Empty resolves by repository owner. installationId: "" # PEM contents of the GitHub App private key. privateKey: "" diff --git a/cmd/bridge/main.go b/cmd/bridge/main.go index e0a535c..afe92e7 100644 --- a/cmd/bridge/main.go +++ b/cmd/bridge/main.go @@ -96,12 +96,15 @@ func run() error { } gh, err := ghclient.NewAppClient(ghclient.Options{ - AppID: cfg.GitHubAppID, - InstallationID: cfg.GitHubInstallationID, - PrivateKeyPath: cfg.GitHubPrivateKeyPath, - BaseURL: cfg.GitHubBaseURL, - Metrics: m, - Retry: retryCfg, + AppID: cfg.GitHubAppID, + InstallationID: cfg.GitHubInstallationID, + PrivateKeyPath: cfg.GitHubPrivateKeyPath, + BaseURL: cfg.GitHubBaseURL, + Metrics: m, + Retry: retryCfg, + InstallationCache: store, + InstallationCacheTTL: cfg.GitHubInstallationCacheTTL, + Log: log, }) if err != nil { return fmt.Errorf("create github client: %w", err) diff --git a/docs/configuration/environment.md b/docs/configuration/environment.md index d0fff6a..a360464 100644 --- a/docs/configuration/environment.md +++ b/docs/configuration/environment.md @@ -22,7 +22,8 @@ SPDX-License-Identifier: Apache-2.0 | `LEADER_ELECTION` | no | `true` | Enable leader election | | `LEADER_ELECTION_ID` | no | `github-deployment-bridge` | Lease name | | `GITHUB_APP_ID` | yes | | GitHub App ID | -| `GITHUB_INSTALLATION_ID` | yes | | GitHub App installation ID | +| `GITHUB_INSTALLATION_ID` | no | _(automatic)_ | Explicit installation override; when unset, resolve by repository owner and cache in SQLite | +| `GITHUB_INSTALLATION_CACHE_TTL` | no | `1h` | TTL for automatically resolved owner-to-installation mappings | | `GITHUB_PRIVATE_KEY_PATH` | yes | | Path to App private key PEM | | `GITHUB_BASE_URL` | no | | GitHub Enterprise API base URL | | `RETRY_MAX_ATTEMPTS` | no | `5` | Retry attempts for GitHub/OCI calls | diff --git a/docs/configuration/github-app.md b/docs/configuration/github-app.md index ecc06aa..be3b0a8 100644 --- a/docs/configuration/github-app.md +++ b/docs/configuration/github-app.md @@ -14,4 +14,10 @@ SPDX-License-Identifier: Apache-2.0 Webhook: inactive. PATs are not supported. +The App must be installed on each target repository owner. By default the bridge +lists installations using App JWT authentication, matches the repository owner, +and caches the installation ID in SQLite for one hour. This lookup requires no +additional configured App permission. `GITHUB_INSTALLATION_ID` remains available +as a fixed backwards-compatible override. + Create and install the App: [Install → GitHub App setup](../install/github-app.md). diff --git a/docs/configuration/helm-values.md b/docs/configuration/helm-values.md index 4befa24..b007103 100644 --- a/docs/configuration/helm-values.md +++ b/docs/configuration/helm-values.md @@ -18,11 +18,12 @@ SPDX-License-Identifier: Apache-2.0 | `config.logLevel` | `LOG_LEVEL` | | `config.leaderElection` | `LEADER_ELECTION` | | `config.githubBaseURL` | `GITHUB_BASE_URL` | +| `config.githubInstallationCacheTTL` | `GITHUB_INSTALLATION_CACHE_TTL` | | `config.retry.maxAttempts` | `RETRY_MAX_ATTEMPTS` | | `config.retry.initialBackoff` | `RETRY_INITIAL_BACKOFF` | | `config.retry.maxBackoff` | `RETRY_MAX_BACKOFF` | | _(fixed by chart)_ | `DATABASE=/data/cache.db` | -| `github.existingSecret` / chart Secret | `GITHUB_APP_ID`, `GITHUB_INSTALLATION_ID`, key file | +| `github.existingSecret` / chart Secret | `GITHUB_APP_ID`, optional `GITHUB_INSTALLATION_ID`, key file | | `github.allowInsecureValues` | _(chart only)_ - allow inline `appId` / `installationId` / `privateKey` (Helm release Secret risk) | | `commonLabels` / `podLabels` | _(chart only)_ - extra labels on all resources / pod template only | | `rbac.create` | _(chart only)_ - emit RBAC | diff --git a/docs/configuration/metrics.md b/docs/configuration/metrics.md index c601bea..9c4640e 100644 --- a/docs/configuration/metrics.md +++ b/docs/configuration/metrics.md @@ -19,6 +19,7 @@ Exposed on `METRICS_ADDR` (default `:8080`) at `/metrics` over unauthenticated H | `github_api_requests_total` | GitHub API calls by `operation` and `result` | | `github_api_failures_total` | Failed GitHub API calls | | `github_api_latency_seconds` | GitHub API latency histogram | +| `github_installation_resolutions_total` | Installation resolution outcomes (`resolved`, `cache_hit`, `failure`, or fixed-ID `fallback`) | | `oci_requests_total` | Registry inspect results | ## Scraping diff --git a/docs/configuration/secrets.md b/docs/configuration/secrets.md index 84fb114..11a612e 100644 --- a/docs/configuration/secrets.md +++ b/docs/configuration/secrets.md @@ -11,10 +11,10 @@ Credentials come from a Kubernetes Secret (never a PAT): | Key | Used as | |---|---| | `app-id` | `GITHUB_APP_ID` | -| `installation-id` | `GITHUB_INSTALLATION_ID` | +| `installation-id` (optional) | `GITHUB_INSTALLATION_ID` fixed override | | `private-key` | PEM mounted at `/github/private-key.pem` | -Prefer `github.existingSecret`. Inline `github.appId` / `installationId` / +Prefer `github.existingSecret`. Inline `github.appId` / optional `installationId` / `privateKey` require `github.allowInsecureValues=true` and are also stored in Helm release history - fine for local/dev only. diff --git a/docs/install/github-app.md b/docs/install/github-app.md index 7902a57..c33cade 100644 --- a/docs/install/github-app.md +++ b/docs/install/github-app.md @@ -42,15 +42,23 @@ Store it securely; the chart mounts it into the pod as a Secret. ## 4. Install the App -Install the App on the target org or repositories, then note: +Install the App on every target organization or user account and grant it access +to the repositories the bridge may report. The bridge uses App authentication to +list the App's installations and selects the installation whose account login +matches the repository owner discovered from workload metadata. This App-level +installation listing does not require an additional repository or organization +permission. + +Then note: | Value | Where to find it | |---|---| | **App ID** | App settings → **About** → App ID | -| **Installation ID** | After install, the URL looks like `…/installations/` | +| **Installation ID** (optional) | URL `…/installations/`; set only to force one installation for backwards compatibility | | **Private key** | The downloaded `.pem` | For GitHub Enterprise Server, also set `config.githubBaseURL` (or -`GITHUB_BASE_URL`) to your instance base URL. +`GITHUB_BASE_URL`) to your instance base URL. Automatic resolution uses that +same server's App and installation APIs. Next: [Secrets](./secrets.md) diff --git a/docs/install/secrets.md b/docs/install/secrets.md index 5af7d12..a198d9e 100644 --- a/docs/install/secrets.md +++ b/docs/install/secrets.md @@ -6,13 +6,14 @@ SPDX-License-Identifier: Apache-2.0 # Secrets -The controller needs three credentials from the GitHub App. They are read from a +The controller needs two credentials from the GitHub App, plus an optional +installation override. They are read from a Kubernetes Secret with these keys: | Secret key | Env var | Description | |---|---|---| | `app-id` | `GITHUB_APP_ID` | Numeric GitHub App ID | -| `installation-id` | `GITHUB_INSTALLATION_ID` | Installation ID for the org/repos | +| `installation-id` (optional) | `GITHUB_INSTALLATION_ID` | Fixed installation override; omit to resolve by repository owner | | `private-key` | (mounted as file) | PEM private key; path set via `GITHUB_PRIVATE_KEY_PATH` | The Helm chart mounts `private-key` at `/github/private-key.pem` and sets @@ -23,7 +24,6 @@ The Helm chart mounts `private-key` at `/github/private-key.pem` and sets ```bash kubectl -n flux-system create secret generic github-deployment-bridge \ --from-literal=app-id=123456 \ - --from-literal=installation-id=987654 \ --from-file=private-key=./github-app.pem ``` @@ -53,10 +53,13 @@ helm upgrade --install github-deployment-bridge \ --namespace flux-system \ --set github.allowInsecureValues=true \ --set github.appId=123456 \ - --set github.installationId=987654 \ --set-file github.privateKey=./github-app.pem \ --set config.clusterName=production-eu \ --set config.environment=production ``` +Add `--from-literal=installation-id=987654` or +`--set github.installationId=987654` only when all repositories must use one +specific installation. Existing secrets containing that key remain supported. + Next: [PVC](./persistence.md) · [Install with Helm](./helm.md) diff --git a/internal/cache/cache.go b/internal/cache/cache.go index df6f2d0..4d282ae 100644 --- a/internal/cache/cache.go +++ b/internal/cache/cache.go @@ -58,6 +58,20 @@ type Store interface { Close() error } +// InstallationEntry maps a repository owner to a GitHub App installation. +type InstallationEntry struct { + Owner string + InstallationID int64 + ResolvedAt time.Time +} + +// InstallationStore persists automatically resolved GitHub App installations. +type InstallationStore interface { + GetInstallation(ctx context.Context, owner string) (*InstallationEntry, error) + PutInstallation(ctx context.Context, entry InstallationEntry) error + DeleteInstallation(ctx context.Context, owner string) error +} + // SQLiteStore is a SQLite-backed Store. type SQLiteStore struct { db *sql.DB @@ -93,6 +107,11 @@ CREATE TABLE IF NOT EXISTS deployments ( reported_at TEXT NOT NULL, PRIMARY KEY (owner, repo, environment, commit_sha, deployment_name) ); +CREATE TABLE IF NOT EXISTS github_installations ( + owner TEXT NOT NULL PRIMARY KEY COLLATE NOCASE, + installation_id INTEGER NOT NULL, + resolved_at TEXT NOT NULL +); ` if _, err := s.db.Exec(schema); err != nil { return fmt.Errorf("migrate sqlite: %w", err) @@ -113,6 +132,47 @@ CREATE TABLE IF NOT EXISTS deployments ( return nil } +// GetInstallation returns the cached installation for owner, or nil when absent. +func (s *SQLiteStore) GetInstallation(ctx context.Context, owner string) (*InstallationEntry, error) { + var entry InstallationEntry + var resolvedAt string + err := s.db.QueryRowContext(ctx, `SELECT owner, installation_id, resolved_at FROM github_installations WHERE owner = ?`, owner). + Scan(&entry.Owner, &entry.InstallationID, &resolvedAt) + if errors.Is(err, sql.ErrNoRows) { + return nil, nil + } + if err != nil { + return nil, fmt.Errorf("installation cache get: %w", err) + } + entry.ResolvedAt, err = time.Parse(time.RFC3339Nano, resolvedAt) + if err != nil { + return nil, fmt.Errorf("parse installation resolved_at: %w", err) + } + return &entry, nil +} + +// PutInstallation upserts the installation for owner. +func (s *SQLiteStore) PutInstallation(ctx context.Context, entry InstallationEntry) error { + if entry.ResolvedAt.IsZero() { + entry.ResolvedAt = time.Now().UTC() + } + _, err := s.db.ExecContext(ctx, `INSERT INTO github_installations (owner, installation_id, resolved_at) VALUES (?, ?, ?) +ON CONFLICT(owner) DO UPDATE SET installation_id = excluded.installation_id, resolved_at = excluded.resolved_at`, + entry.Owner, entry.InstallationID, entry.ResolvedAt.UTC().Format(time.RFC3339Nano)) + if err != nil { + return fmt.Errorf("installation cache put: %w", err) + } + return nil +} + +// DeleteInstallation invalidates the cached installation for owner. +func (s *SQLiteStore) DeleteInstallation(ctx context.Context, owner string) error { + if _, err := s.db.ExecContext(ctx, `DELETE FROM github_installations WHERE owner = ?`, owner); err != nil { + return fmt.Errorf("installation cache delete: %w", err) + } + return nil +} + func (s *SQLiteStore) ensureDeploymentNameColumn() error { rows, err := s.db.Query(`PRAGMA table_info(deployments)`) if err != nil { diff --git a/internal/cache/cache_test.go b/internal/cache/cache_test.go index 5ae3cb5..1c73a46 100644 --- a/internal/cache/cache_test.go +++ b/internal/cache/cache_test.go @@ -152,4 +152,18 @@ INSERT INTO deployments VALUES ('acme','api','production','abc123',7,'success',' if cache.LatestStatus(got) != cache.StatusSuccess || got.DeploymentID != 7 { t.Fatalf("migrated entry = %#v", got) } + if err := store.PutInstallation(context.Background(), cache.InstallationEntry{Owner: "Acme", InstallationID: 42}); err != nil { + t.Fatalf("put installation after migration: %v", err) + } + installation, err := store.GetInstallation(context.Background(), "acme") + if err != nil || installation == nil || installation.InstallationID != 42 { + t.Fatalf("installation = %#v, err = %v", installation, err) + } + if err := store.DeleteInstallation(context.Background(), "ACME"); err != nil { + t.Fatal(err) + } + installation, err = store.GetInstallation(context.Background(), "acme") + if err != nil || installation != nil { + t.Fatalf("deleted installation = %#v, err = %v", installation, err) + } } diff --git a/internal/config/config.go b/internal/config/config.go index c7c84d1..e4ee6b6 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -17,26 +17,27 @@ import ( // Config holds runtime configuration for the deployment bridge. type Config struct { - ClusterName string `envconfig:"CLUSTER_NAME" required:"true"` - Environment string `envconfig:"ENVIRONMENT" required:"true"` - WatchNamespace string `envconfig:"WATCH_NAMESPACE" default:""` - DatabasePath string `envconfig:"DATABASE" default:"/data/cache.db"` - EnvironmentURL string `envconfig:"ENVIRONMENT_URL"` - Description string `envconfig:"DESCRIPTION"` - LogURLTemplate string `envconfig:"LOG_URL_TEMPLATE"` - LogURLTemplateEscape bool `envconfig:"LOG_URL_TEMPLATE_ESCAPE" default:"false"` - LogLevel string `envconfig:"LOG_LEVEL" default:"info"` - MetricsAddr string `envconfig:"METRICS_ADDR" default:":8080"` - ProbeAddr string `envconfig:"PROBE_ADDR" default:":8081"` - LeaderElection bool `envconfig:"LEADER_ELECTION" default:"true"` - LeaderElectionID string `envconfig:"LEADER_ELECTION_ID" default:"github-deployment-bridge"` - GitHubAppID int64 `envconfig:"GITHUB_APP_ID" required:"true"` - GitHubInstallationID int64 `envconfig:"GITHUB_INSTALLATION_ID" required:"true"` - GitHubPrivateKeyPath string `envconfig:"GITHUB_PRIVATE_KEY_PATH" required:"true"` - GitHubBaseURL string `envconfig:"GITHUB_BASE_URL"` // optional, for GHES - RetryMaxAttempts int `envconfig:"RETRY_MAX_ATTEMPTS" default:"5"` - RetryInitialBackoff time.Duration `envconfig:"RETRY_INITIAL_BACKOFF" default:"500ms"` - RetryMaxBackoff time.Duration `envconfig:"RETRY_MAX_BACKOFF" default:"30s"` + ClusterName string `envconfig:"CLUSTER_NAME" required:"true"` + Environment string `envconfig:"ENVIRONMENT" required:"true"` + WatchNamespace string `envconfig:"WATCH_NAMESPACE" default:""` + DatabasePath string `envconfig:"DATABASE" default:"/data/cache.db"` + EnvironmentURL string `envconfig:"ENVIRONMENT_URL"` + Description string `envconfig:"DESCRIPTION"` + LogURLTemplate string `envconfig:"LOG_URL_TEMPLATE"` + LogURLTemplateEscape bool `envconfig:"LOG_URL_TEMPLATE_ESCAPE" default:"false"` + LogLevel string `envconfig:"LOG_LEVEL" default:"info"` + MetricsAddr string `envconfig:"METRICS_ADDR" default:":8080"` + ProbeAddr string `envconfig:"PROBE_ADDR" default:":8081"` + LeaderElection bool `envconfig:"LEADER_ELECTION" default:"true"` + LeaderElectionID string `envconfig:"LEADER_ELECTION_ID" default:"github-deployment-bridge"` + GitHubAppID int64 `envconfig:"GITHUB_APP_ID" required:"true"` + GitHubInstallationID int64 `envconfig:"GITHUB_INSTALLATION_ID"` + GitHubInstallationCacheTTL time.Duration `envconfig:"GITHUB_INSTALLATION_CACHE_TTL" default:"1h"` + GitHubPrivateKeyPath string `envconfig:"GITHUB_PRIVATE_KEY_PATH" required:"true"` + GitHubBaseURL string `envconfig:"GITHUB_BASE_URL"` // optional, for GHES + RetryMaxAttempts int `envconfig:"RETRY_MAX_ATTEMPTS" default:"5"` + RetryInitialBackoff time.Duration `envconfig:"RETRY_INITIAL_BACKOFF" default:"500ms"` + RetryMaxBackoff time.Duration `envconfig:"RETRY_MAX_BACKOFF" default:"30s"` } // Load reads configuration from environment variables. @@ -60,8 +61,11 @@ func Load() (Config, error) { if cfg.GitHubAppID <= 0 { return Config{}, fmt.Errorf("GITHUB_APP_ID must be a positive integer") } - if cfg.GitHubInstallationID <= 0 { - return Config{}, fmt.Errorf("GITHUB_INSTALLATION_ID must be a positive integer") + if cfg.GitHubInstallationID < 0 { + return Config{}, fmt.Errorf("GITHUB_INSTALLATION_ID must be a positive integer when set") + } + if cfg.GitHubInstallationCacheTTL <= 0 { + return Config{}, fmt.Errorf("GITHUB_INSTALLATION_CACHE_TTL must be positive") } if strings.TrimSpace(cfg.GitHubPrivateKeyPath) == "" { return Config{}, fmt.Errorf("GITHUB_PRIVATE_KEY_PATH must not be empty") diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 3e4abe4..43fbdcc 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -2,7 +2,9 @@ package config_test import ( "log/slog" + "os" "testing" + "time" "github.com/roberteggl/github-deployment-bridge/internal/config" ) @@ -41,6 +43,25 @@ func TestLoadAndExpand(t *testing.T) { } } +func TestLoadAllowsAutomaticInstallationResolution(t *testing.T) { + t.Setenv("CLUSTER_NAME", "production-eu") + t.Setenv("ENVIRONMENT", "production") + t.Setenv("GITHUB_APP_ID", "123") + t.Setenv("GITHUB_INSTALLATION_ID", "temporary") + if err := os.Unsetenv("GITHUB_INSTALLATION_ID"); err != nil { + t.Fatal(err) + } + t.Setenv("GITHUB_PRIVATE_KEY_PATH", "/tmp/key.pem") + + cfg, err := config.Load() + if err != nil { + t.Fatalf("Load: %v", err) + } + if cfg.GitHubInstallationID != 0 || cfg.GitHubInstallationCacheTTL != time.Hour { + t.Fatalf("installation config = %d, %s", cfg.GitHubInstallationID, cfg.GitHubInstallationCacheTTL) + } +} + func TestExpandLogURLTemplate(t *testing.T) { t.Parallel() diff --git a/internal/github/client.go b/internal/github/client.go index a6a8d44..2f53d98 100644 --- a/internal/github/client.go +++ b/internal/github/client.go @@ -10,14 +10,17 @@ import ( "encoding/json" "errors" "fmt" + "log/slog" "net/http" "strconv" "strings" + "sync" "time" "github.com/bradleyfalzon/ghinstallation/v2" "github.com/google/go-github/v89/github" + "github.com/roberteggl/github-deployment-bridge/internal/cache" "github.com/roberteggl/github-deployment-bridge/internal/metrics" "github.com/roberteggl/github-deployment-bridge/pkg/retry" ) @@ -76,20 +79,31 @@ type Client interface { // AppClient is a GitHub App installation client. type AppClient struct { - client *github.Client - metrics *metrics.Metrics - retry retry.Config + client *github.Client // non-nil when the explicit installation override is used + appsClient *github.Client + appsTransport *ghinstallation.AppsTransport + baseURL string + cache cache.InstallationStore + cacheTTL time.Duration + log *slog.Logger + clients map[int64]*github.Client + clientsMu sync.Mutex + metrics *metrics.Metrics + retry retry.Config } // Options configures a new AppClient. type Options struct { - AppID int64 - InstallationID int64 - PrivateKeyPath string - BaseURL string - Metrics *metrics.Metrics - Retry retry.Config - Transport http.RoundTripper + AppID int64 + InstallationID int64 + PrivateKeyPath string + BaseURL string + Metrics *metrics.Metrics + Retry retry.Config + Transport http.RoundTripper + InstallationCache cache.InstallationStore + InstallationCacheTTL time.Duration + Log *slog.Logger } // NewAppClient builds a GitHub App installation client. @@ -99,26 +113,25 @@ func NewAppClient(opts Options) (*AppClient, error) { base = http.DefaultTransport } - itr, err := ghinstallation.NewKeyFromFile(base, opts.AppID, opts.InstallationID, opts.PrivateKeyPath) + atr, err := ghinstallation.NewAppsTransportKeyFromFile(base, opts.AppID, opts.PrivateKeyPath) if err != nil { return nil, fmt.Errorf("create GitHub App transport: %w", err) } - - httpClient := &http.Client{Transport: itr, Timeout: 30 * time.Second} - clientOpts := []github.ClientOptionsFunc{ - github.WithHTTPClient(httpClient), - } - if opts.BaseURL != "" { - // Accept either https://ghe.example.com or https://ghe.example.com/api/v3. - baseURL := strings.TrimRight(opts.BaseURL, "/") - baseURL = strings.TrimSuffix(baseURL, "/api/v3") - itr.BaseURL = baseURL + "/api/v3" - clientOpts = append(clientOpts, github.WithEnterpriseURLs(baseURL+"/", baseURL+"/")) + if opts.InstallationID == 0 && opts.InstallationCache == nil { + return nil, fmt.Errorf("installation cache is required when GITHUB_INSTALLATION_ID is unset") } - - client, err := github.NewClient(clientOpts...) + baseURL := enterpriseBaseURL(opts.BaseURL) + atr.BaseURL = strings.TrimRight(baseURL, "/") + appsClient, err := newGitHubClient(atr, baseURL) if err != nil { - return nil, fmt.Errorf("create GitHub client: %w", err) + return nil, err + } + var client *github.Client + if opts.InstallationID > 0 { + client, err = newGitHubClient(ghinstallation.NewFromAppsTransport(atr, opts.InstallationID), baseURL) + if err != nil { + return nil, err + } } cfg := opts.Retry @@ -126,17 +139,138 @@ func NewAppClient(opts Options) (*AppClient, error) { cfg = retry.Default() } - return &AppClient{ - client: client, + ttl := opts.InstallationCacheTTL + if ttl == 0 { + ttl = time.Hour + } + log := opts.Log + if log == nil { + log = slog.Default() + } + c := &AppClient{ + client: client, + appsClient: appsClient, appsTransport: atr, baseURL: baseURL, + cache: opts.InstallationCache, cacheTTL: ttl, log: log, clients: make(map[int64]*github.Client), metrics: opts.Metrics, retry: cfg, - }, nil + } + if client != nil { + log.Info("using explicit GitHub App installation", "installation_id", opts.InstallationID) + } + return c, nil +} + +func enterpriseBaseURL(raw string) string { + if raw == "" { + return "https://api.github.com" + } + base := strings.TrimRight(raw, "/") + return strings.TrimSuffix(base, "/api/v3") + "/api/v3" +} + +func newGitHubClient(transport http.RoundTripper, baseURL string) (*github.Client, error) { + options := []github.ClientOptionsFunc{github.WithHTTPClient(&http.Client{Transport: transport, Timeout: 30 * time.Second})} + if baseURL != "https://api.github.com" { + root := strings.TrimSuffix(baseURL, "/api/v3") + options = append(options, github.WithEnterpriseURLs(root+"/", root+"/")) + } + client, err := github.NewClient(options...) + if err != nil { + return nil, fmt.Errorf("create GitHub client: %w", err) + } + return client, nil +} + +func (c *AppClient) clientForOwner(ctx context.Context, owner string, force bool) (*github.Client, error) { + if c.client != nil { + c.metricResolution("fallback") + return c.client, nil + } + if !force { + entry, err := c.cache.GetInstallation(ctx, owner) + if err != nil { + c.metricResolution("failure") + return nil, err + } + if entry != nil && time.Since(entry.ResolvedAt) < c.cacheTTL { + c.metricResolution("cache_hit") + c.log.Debug("GitHub App installation cache hit", "owner", owner, "installation_id", entry.InstallationID) + return c.installationClient(entry.InstallationID) + } + } + start := time.Now() + installations, resp, err := c.appsClient.Apps.ListInstallations(ctx, &github.ListOptions{PerPage: 100}) + c.observe("list_installations", start, err, resp) + if err != nil { + c.metricResolution("failure") + return nil, fmt.Errorf("list GitHub App installations: %w", err) + } + for { + for _, installation := range installations { + if strings.EqualFold(installation.GetAccount().GetLogin(), owner) { + entry := cache.InstallationEntry{Owner: owner, InstallationID: installation.GetID(), ResolvedAt: time.Now().UTC()} + if err := c.cache.PutInstallation(ctx, entry); err != nil { + c.metricResolution("failure") + return nil, err + } + c.metricResolution("resolved") + c.log.Info("resolved GitHub App installation", "owner", owner, "installation_id", entry.InstallationID) + return c.installationClient(entry.InstallationID) + } + } + if resp == nil || resp.NextPage == 0 { + break + } + start = time.Now() + installations, resp, err = c.appsClient.Apps.ListInstallations(ctx, &github.ListOptions{Page: resp.NextPage, PerPage: 100}) + c.observe("list_installations", start, err, resp) + if err != nil { + c.metricResolution("failure") + return nil, fmt.Errorf("list GitHub App installations: %w", err) + } + } + c.metricResolution("failure") + return nil, retry.Permanent(fmt.Errorf("no GitHub App installation found for repository owner %q", owner)) +} + +func (c *AppClient) installationClient(id int64) (*github.Client, error) { + c.clientsMu.Lock() + defer c.clientsMu.Unlock() + if client := c.clients[id]; client != nil { + return client, nil + } + client, err := newGitHubClient(ghinstallation.NewFromAppsTransport(c.appsTransport, id), c.baseURL) + if err == nil { + c.clients[id] = client + } + return client, err +} + +func (c *AppClient) invalidate(ctx context.Context, owner string) { + if c.client != nil { + return + } + if err := c.cache.DeleteInstallation(ctx, owner); err != nil { + c.metricResolution("failure") + c.log.Warn("failed to invalidate GitHub App installation cache", "owner", owner, "error", err) + } + c.log.Warn("invalidated GitHub App installation cache", "owner", owner) +} + +func (c *AppClient) metricResolution(result string) { + if c.metrics != nil { + c.metrics.GitHubInstallationResolutionsTotal.WithLabelValues(result).Inc() + } } // CreateDeployment creates a GitHub Deployment. func (c *AppClient) CreateDeployment(ctx context.Context, req DeploymentRequest) (*DeploymentResult, error) { var result *DeploymentResult err := retry.Do(ctx, c.retry, func(ctx context.Context) error { + client, err := c.clientForOwner(ctx, req.Owner, false) + if err != nil { + return err + } start := time.Now() desc := req.Description if desc == "" { @@ -157,7 +291,15 @@ func (c *AppClient) CreateDeployment(ctx context.Context, req DeploymentRequest) ghReq.Payload = req.Payload } - dep, resp, err := c.client.Repositories.CreateDeployment(ctx, req.Owner, req.Repo, ghReq) + dep, resp, err := client.Repositories.CreateDeployment(ctx, req.Owner, req.Repo, ghReq) + if err != nil && installationFailure(resp) && c.client == nil { + c.invalidate(ctx, req.Owner) + client, resolveErr := c.clientForOwner(ctx, req.Owner, true) + if resolveErr != nil { + return resolveErr + } + dep, resp, err = client.Repositories.CreateDeployment(ctx, req.Owner, req.Repo, ghReq) + } c.observe("create_deployment", start, err, resp) if err != nil { return classifyGitHubError(err, resp) @@ -179,13 +321,25 @@ func (c *AppClient) CreateDeployment(ctx context.Context, req DeploymentRequest) func (c *AppClient) FindDeployment(ctx context.Context, req FindDeploymentRequest) (*DeploymentResult, error) { var result *DeploymentResult err := retry.Do(ctx, c.retry, func(ctx context.Context) error { + client, err := c.clientForOwner(ctx, req.Owner, false) + if err != nil { + return err + } start := time.Now() opts := &github.DeploymentsListOptions{ Environment: req.Environment, Ref: req.Ref, ListOptions: github.ListOptions{PerPage: 100}, } - deployments, resp, err := c.client.Repositories.ListDeployments(ctx, req.Owner, req.Repo, opts) + deployments, resp, err := client.Repositories.ListDeployments(ctx, req.Owner, req.Repo, opts) + if err != nil && installationFailure(resp) && c.client == nil { + c.invalidate(ctx, req.Owner) + client, resolveErr := c.clientForOwner(ctx, req.Owner, true) + if resolveErr != nil { + return resolveErr + } + deployments, resp, err = client.Repositories.ListDeployments(ctx, req.Owner, req.Repo, opts) + } c.observe("list_deployments", start, err, resp) if err != nil { return classifyGitHubError(err, resp) @@ -211,6 +365,10 @@ func (c *AppClient) FindDeployment(ctx context.Context, req FindDeploymentReques // CreateDeploymentStatus creates a deployment status. func (c *AppClient) CreateDeploymentStatus(ctx context.Context, req DeploymentStatusRequest) error { return retry.Do(ctx, c.retry, func(ctx context.Context) error { + client, err := c.clientForOwner(ctx, req.Owner, false) + if err != nil { + return err + } start := time.Now() desc := req.Description if desc == "" { @@ -228,7 +386,15 @@ func (c *AppClient) CreateDeploymentStatus(ctx context.Context, req DeploymentSt ghReq.LogURL = github.Ptr(req.LogURL) } - _, resp, err := c.client.Repositories.CreateDeploymentStatus(ctx, req.Owner, req.Repo, req.DeploymentID, ghReq) + _, resp, err := client.Repositories.CreateDeploymentStatus(ctx, req.Owner, req.Repo, req.DeploymentID, ghReq) + if err != nil && installationFailure(resp) && c.client == nil { + c.invalidate(ctx, req.Owner) + client, resolveErr := c.clientForOwner(ctx, req.Owner, true) + if resolveErr != nil { + return resolveErr + } + _, resp, err = client.Repositories.CreateDeploymentStatus(ctx, req.Owner, req.Repo, req.DeploymentID, ghReq) + } c.observe("create_deployment_status", start, err, resp) if err != nil { return classifyGitHubError(err, resp) @@ -237,6 +403,10 @@ func (c *AppClient) CreateDeploymentStatus(ctx context.Context, req DeploymentSt }) } +func installationFailure(resp *github.Response) bool { + return resp != nil && (resp.StatusCode == http.StatusUnauthorized || resp.StatusCode == http.StatusNotFound) +} + func (c *AppClient) observe(operation string, start time.Time, err error, resp *github.Response) { if c.metrics == nil { return diff --git a/internal/github/installation_test.go b/internal/github/installation_test.go new file mode 100644 index 0000000..961e84a --- /dev/null +++ b/internal/github/installation_test.go @@ -0,0 +1,115 @@ +// SPDX-FileCopyrightText: 2026 Robert Eggl +// +// SPDX-License-Identifier: Apache-2.0 + +package github_test + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "path/filepath" + "sync/atomic" + "testing" + "time" + + "github.com/roberteggl/github-deployment-bridge/internal/cache" + ghclient "github.com/roberteggl/github-deployment-bridge/internal/github" + "github.com/roberteggl/github-deployment-bridge/pkg/retry" +) + +func TestAutomaticInstallationResolutionAndSQLiteCache(t *testing.T) { + t.Parallel() + keyPath := generateRSAKey(t) + store, err := cache.Open(filepath.Join(t.TempDir(), "cache.db")) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = store.Close() }) + + var lists atomic.Int32 + mux := http.NewServeMux() + mux.HandleFunc("/api/v3/app/installations", func(w http.ResponseWriter, _ *http.Request) { + lists.Add(1) + _ = json.NewEncoder(w).Encode([]map[string]any{{"id": 99, "account": map[string]any{"login": "Acme"}}}) + }) + mux.HandleFunc("/api/v3/app/installations/99/access_tokens", installationToken) + mux.HandleFunc("/api/v3/repos/acme/api/deployments", func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodGet { + _ = json.NewEncoder(w).Encode([]any{}) + return + } + w.WriteHeader(http.StatusCreated) + _ = json.NewEncoder(w).Encode(map[string]any{"id": 7}) + }) + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + + client, err := ghclient.NewAppClient(ghclient.Options{AppID: 1, PrivateKeyPath: keyPath, BaseURL: srv.URL, + InstallationCache: store, InstallationCacheTTL: time.Hour, Retry: retry.Config{MaxAttempts: 1}}) + if err != nil { + t.Fatal(err) + } + ctx := context.Background() + if _, err := client.CreateDeployment(ctx, ghclient.DeploymentRequest{Owner: "acme", Repo: "api", Ref: "abc", Environment: "prod"}); err != nil { + t.Fatal(err) + } + if _, err := client.FindDeployment(ctx, ghclient.FindDeploymentRequest{Owner: "acme", Repo: "api", Ref: "abc", Environment: "prod"}); err != nil { + t.Fatal(err) + } + if got := lists.Load(); got != 1 { + t.Fatalf("installation lists = %d, want 1", got) + } + entry, err := store.GetInstallation(ctx, "acme") + if err != nil || entry == nil || entry.InstallationID != 99 { + t.Fatalf("cached entry = %#v, err = %v", entry, err) + } +} + +func TestInstallationFailureInvalidatesAndResolvesOnce(t *testing.T) { + t.Parallel() + keyPath := generateRSAKey(t) + store, err := cache.Open(filepath.Join(t.TempDir(), "cache.db")) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = store.Close() }) + _ = store.PutInstallation(context.Background(), cache.InstallationEntry{Owner: "acme", InstallationID: 99, ResolvedAt: time.Now()}) + + var lists atomic.Int32 + mux := http.NewServeMux() + mux.HandleFunc("/api/v3/app/installations", func(w http.ResponseWriter, _ *http.Request) { + lists.Add(1) + _ = json.NewEncoder(w).Encode([]map[string]any{{"id": 100, "account": map[string]any{"login": "acme"}}}) + }) + mux.HandleFunc("/api/v3/app/installations/99/access_tokens", installationToken) + mux.HandleFunc("/api/v3/app/installations/100/access_tokens", installationToken) + var calls atomic.Int32 + mux.HandleFunc("/api/v3/repos/acme/api/deployments", func(w http.ResponseWriter, _ *http.Request) { + if calls.Add(1) == 1 { + http.Error(w, "stale installation", http.StatusNotFound) + return + } + w.WriteHeader(http.StatusCreated) + _ = json.NewEncoder(w).Encode(map[string]any{"id": 8}) + }) + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + client, err := ghclient.NewAppClient(ghclient.Options{AppID: 1, PrivateKeyPath: keyPath, BaseURL: srv.URL, + InstallationCache: store, InstallationCacheTTL: time.Hour, Retry: retry.Config{MaxAttempts: 1}}) + if err != nil { + t.Fatal(err) + } + result, err := client.CreateDeployment(context.Background(), ghclient.DeploymentRequest{Owner: "acme", Repo: "api", Ref: "abc", Environment: "prod"}) + if err != nil || result.ID != 8 { + t.Fatalf("result = %#v, err = %v", result, err) + } + if calls.Load() != 2 || lists.Load() != 1 { + t.Fatalf("API calls = %d, lists = %d", calls.Load(), lists.Load()) + } +} + +func installationToken(w http.ResponseWriter, _ *http.Request) { + _ = json.NewEncoder(w).Encode(map[string]any{"token": "token", "expires_at": "2099-01-01T00:00:00Z"}) +} diff --git a/internal/metrics/metrics.go b/internal/metrics/metrics.go index d9eb59e..dd75e78 100644 --- a/internal/metrics/metrics.go +++ b/internal/metrics/metrics.go @@ -12,16 +12,17 @@ import ( // Metrics holds Prometheus collectors. type Metrics struct { - DeploymentsCreatedTotal prometheus.Counter - DeploymentStatusUpdatesTotal prometheus.Counter - DeploymentFailuresTotal prometheus.Counter - DeploymentErrorsTotal prometheus.Counter - DeploymentDuplicatesSkippedTotal prometheus.Counter - DeploymentInactiveTotal prometheus.Counter - GitHubAPIRequestsTotal *prometheus.CounterVec - GitHubAPIFailuresTotal prometheus.Counter - GitHubAPILatencySeconds *prometheus.HistogramVec - OCIRequestsTotal *prometheus.CounterVec + DeploymentsCreatedTotal prometheus.Counter + DeploymentStatusUpdatesTotal prometheus.Counter + DeploymentFailuresTotal prometheus.Counter + DeploymentErrorsTotal prometheus.Counter + DeploymentDuplicatesSkippedTotal prometheus.Counter + DeploymentInactiveTotal prometheus.Counter + GitHubAPIRequestsTotal *prometheus.CounterVec + GitHubAPIFailuresTotal prometheus.Counter + GitHubAPILatencySeconds *prometheus.HistogramVec + GitHubInstallationResolutionsTotal *prometheus.CounterVec + OCIRequestsTotal *prometheus.CounterVec } // New registers metrics with the given registerer (or the default if nil). @@ -69,6 +70,10 @@ func New(reg prometheus.Registerer) *Metrics { Help: "Latency of GitHub API requests in seconds.", Buckets: prometheus.DefBuckets, }, []string{"operation"}), + GitHubInstallationResolutionsTotal: factory.NewCounterVec(prometheus.CounterOpts{ + Name: "github_installation_resolutions_total", + Help: "Total GitHub App installation resolution outcomes.", + }, []string{"result"}), OCIRequestsTotal: factory.NewCounterVec(prometheus.CounterOpts{ Name: "oci_requests_total", Help: "Total number of OCI registry requests by result.",