diff --git a/docs/INDEX.md b/docs/INDEX.md index d9a8b8cd..df2b20bc 100644 --- a/docs/INDEX.md +++ b/docs/INDEX.md @@ -28,6 +28,11 @@ folder by **lifecycle**, not by topic. 5. [`design/support-boundary/support-contract.md`](design/support-boundary/support-contract.md) — **what the operator edits, what it refuses, and why.** +Provider-specific setup that needed writing down: +[`azure-devops-getting-started.md`](azure-devops-getting-started.md) — Azure DevOps end to end, why its +credential Secret is shaped differently from every other provider's, and the three test layers that +cover a provider CI cannot reach. + ## The contracts — [`spec/`](spec/) The code cites these. Breaking one without updating it is how the next person gets @@ -66,7 +71,7 @@ says what we support and refuse** — and then its kustomize field taxonomy, the write boundary, the orchestrator/expansion line, and how secrets are handled. -Sixteen other open items: +Seventeen other open items: | Doc | Open question | |---|---| @@ -85,6 +90,7 @@ Sixteen other open items: | [`e2e-finish-plan.md`](design/e2e-finish-plan.md) | remaining e2e harness work | | [`sensitive-resource-diagnostics-follow-up.md`](design/sensitive-resource-diagnostics-follow-up.md) | deferred diagnostics | | [`e2e-git-server-choice.md`](design/e2e-git-server-choice.md) | stay on Gitea or move to Forgejo — the `_csrf` pin is fixable in place on both, so the migration is now a preference call, not a fix; also why we adopt no SDK either way | +| [`azure-devops-multi-ack.md`](design/azure-devops-multi-ack.md) | **decided and built: go-git v6** — why Azure DevOps rejects our fetches, and what to do instead of PR [#292](https://github.com/ConfigButler/gitops-reverser/pull/292)'s bundled `git` binary. The capability filter fails in two independent halves: advertising `multi_ack` is a four-line change, but v5 then cannot parse the multi-ACK **response**, which only a fetch with `have` lines provokes. That is why **Flux ships ADO support on v5 with no git binary — it never fetches**, only `CloneContext`, so it never enters the path v5 cannot serve; our persistent-clone-plus-incremental-fetch design is the opposite, which makes the trim alone insufficient for us. **go-git v6 already implements `multi_ack`** (PR #1204, in every v6 tag; upstream then deleted their ADO example saying it "works out of the box"), and its churn in the packages we import runs 96 → 39 → **1** → **9** removals per alpha, so it is one settled breaking wave rather than a moving target; the migration is four known API removals over two rewritten files, `transport.AuthMethod` being the invasive one. Prices PR #292 as measured rather than argued: the image goes **217 MB → 940 MB**, of which 723 MB is a `cp -rL` that dereferences 165 hardlinks to one binary (a one-character fix), arm64 is unaffected and native, but **Trivy reports zero findings on both images** while the new one carries git 2.54.0, OpenSSH 10.3p1 and OpenSSL 3.5.7 as loose files no package database describes — so the CRITICAL gate is blind to a third of the runtime. Also catches an unflagged non-ADO regression (`Depth: 1` dropped, so every provider full-fetches) and 10% patch coverage on an untestable path. The unlock is that **canonical `git upload-pack` advertises `multi_ack`** (verified), so the Gitea already in the e2e lab plus a 400-injecting proxy is a faithful ADO simulator — no tenant needed, and the only way any option becomes CI-testable. Four options priced, and Option A (v6) is the one shipped. Carries a measured **capability matrix** over our three network calls with two diagrams, which narrows the blast radius to **one call, `repo.Fetch`**: `receive-pack` never advertises `multi_ack` (measured), so **the atomic push is out of scope for every option** — its safety rests on the same-session advertisement plus the server-side `Old`/`New` compare-and-swap in `packp.Command`, neither of which touches `upload-pack`, and we already push from a shallow store today. v6 keeps that pattern 1:1 (`Handshake` → `GetRemoteRefs`/`Push`, same `[]*packp.Command`), which is an argument *for* migrating. Records what the migration actually cost, including the four v6 behaviour changes it surfaced — two of them settings v6 reads from the environment and fails closed on, invisible to unit tests | | [`watchrule-source-namespace/`](design/watchrule-source-namespace/README.md) | letting a WatchRule address differently-named namespaces on its source cluster — a deny-by-default `allowedSourceNamespaces` on the **GitTarget** (so scope is per-tenant, not a provider-wide union), unlocked by a false-by-default delegation flag on the ClusterProvider. Five PRs: three landed prerequisite scope fixes (the namespace-blind resync sweep that would delete other namespaces' manifests, the cluster-wide/named stream collapse, and ClusterWatchRule's unchecked GitTarget attachment), then the breaking **scope-by-kind** change — `WatchRule.spec.rules[].sourceNamespace` (a name or `"*"` for the target's admitted set) and a cluster-scope-only ClusterWatchRule — and a GitTarget `prune.mode` that makes the resync sweep opt-in, released together with it | ## Deferred, but still wanted — [`future/`](future/) diff --git a/docs/README.md b/docs/README.md index 9d2c27f0..dee9baa0 100644 --- a/docs/README.md +++ b/docs/README.md @@ -15,6 +15,7 @@ If you only want the supported product docs, start with the files below. document: Flux `HelmRelease`, Argo CD `Application`, KRO, and core resources all mirror and edit alike - [`commit-signing.md`](commit-signing.md): how valid Git signatures map to platform verification - [`github-setup-guide.md`](github-setup-guide.md): GitHub repository and credential setup +- [`azure-devops-getting-started.md`](azure-devops-getting-started.md): Azure DevOps repository and credential setup - [`attribution-setup-guide.md`](attribution-setup-guide.md): naming real Kubernetes users as commit authors via kube-apiserver audit delivery - [`sops-age-guide.md`](sops-age-guide.md): Secret encryption with SOPS + age diff --git a/docs/azure-devops-getting-started.md b/docs/azure-devops-getting-started.md new file mode 100644 index 00000000..ef4a1ce2 --- /dev/null +++ b/docs/azure-devops-getting-started.md @@ -0,0 +1,124 @@ +# Getting started with Azure DevOps + +Mirror a cluster into an Azure DevOps repository. Only the credential differs from any other +provider, and it differs in a way that trips people up. + +Prerequisites: the operator installed (see the [root README](../README.md)), and an ADO repository you +can write to. An empty repository is fine — the operator creates the branch. + +## 1. A Personal Access Token + +**User settings → Personal access tokens → New Token**, in the organization that owns the repository. +Scope **Code (read & write)**; read alone is not enough, because the operator pushes. Set an expiry +you are willing to rotate, and copy the token when it is shown — ADO never displays it again. The +token inherits its user's permissions, so that user needs write access to the repository. + +## 2. Namespace and Secret + +ADO sends a PAT as HTTP basic auth with the token as the **password** and ignores the username, so the +Secret carries `password` and no `username`: + +```bash +kubectl create namespace my-namespace + +kubectl create secret generic ado-creds \ + --namespace my-namespace \ + --from-literal=password='' +``` + +A `username` is optional here and ADO ignores whatever you set, so leaving it out is simplest. What +does not work is a `username` with no `password`: the password is what selects basic auth. + +## 3. The resources + +Replace `` with your repository's default branch (`main` on a new ADO repository), the same +value in both places: + +```yaml +apiVersion: configbutler.ai/v1alpha3 +kind: GitProvider +metadata: + name: ado-provider + namespace: my-namespace +spec: + url: https://dev.azure.com///_git/ + secretRef: + name: ado-creds + allowedBranches: + - +--- +apiVersion: configbutler.ai/v1alpha3 +kind: GitTarget +metadata: + name: ado-target + namespace: my-namespace +spec: + providerRef: + name: ado-provider + branch: + path: clusters/my-cluster +--- +apiVersion: configbutler.ai/v1alpha3 +kind: WatchRule +metadata: + name: ado-rule + namespace: my-namespace +spec: + targetRef: + name: ado-target + rules: + - resources: ["configmaps"] +``` + +```bash +kubectl wait --for=condition=Ready gitprovider/ado-provider -n my-namespace --timeout=60s +``` + +## 4. Check it works + +```bash +kubectl create configmap ado-demo -n my-namespace --from-literal=greeting=hello +``` + +A commit appears on `` under +`clusters/my-cluster/my-namespace/configmaps/ado-demo.yaml`. Commits are authored by the configured +committer; audit delivery is only needed to attribute them to the Kubernetes user who made the change +(see [configuration.md](configuration.md)). + +## Other credentials + +SSH (`ssh://git@ssh.dev.azure.com/v3///`) and Entra ID bearer tokens use the same +Secret keys as any other provider. **Neither is tested against ADO**, unlike the PAT path above; both +are expected to work, as they do for GitHub. + +## If fetches fail with HTTP 400 + +```text +TF401041: Clients must support multi-ack. +``` + +ADO rejects fetches from clients that do not advertise `multi_ack`, which go-git only implements from +v6. Releases before [#297](https://github.com/ConfigButler/gitops-reverser/pull/297) cannot fetch from +ADO and no configuration changes that — upgrade. + +Two things make this confusing to debug: `GitProvider` can still reach `Ready`, because the +connectivity check is a different request that never needed the capability, and pushes still work, +because the push protocol has no `multi_ack` at all. Details and sources: +[`facts/azure-devops-multi-ack-requirement.md`](facts/azure-devops-multi-ack-requirement.md). + +## Testing against a real tenant + +CI cannot reach ADO, so two layers are opt-in and skip themselves without configuration: + +```bash +export E2E_ADO_REPO_URL='https://dev.azure.com///_git/' # written to +export E2E_ADO_PAT='' +export E2E_ADO_EMPTY_REPO_URL='.../_git/empty' # optional; must name a repository with no commits + +go test ./internal/git/ -run TestADOLive -v # library +task test-e2e-ado # operator, needs a prepared e2e cluster +``` + +`TestADOLive_StillRequiresMultiAck` is a canary: if it fails, Microsoft fixed their end. +[`internal/git/ado_multiack_test.go`](../internal/git/ado_multiack_test.go) covers the same rule in CI +with no credential. diff --git a/docs/configuration.md b/docs/configuration.md index dcdbf115..d7d7bc40 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -101,9 +101,17 @@ which match the built-in Secret types and the tooling around them (`kubectl crea | Auth | Keys | |---|---| | SSH | `ssh-privatekey` (+ optional `ssh-password` passphrase, `known_hosts`) | -| HTTP basic | `username` + `password` | +| HTTP basic | `password` (+ optional `username`) | | HTTP bearer token | `bearerToken` (GitHub fine-grained PAT, GitLab access token; no username) | +**`password` is what selects HTTP basic auth**, and `username` is optional: a Secret with only +`password` authenticates with an empty username, which is what Azure DevOps expects for a Personal +Access Token. ADO ignores the username entirely; measured, any value including none is accepted. +A `username` with no `password` is an error, because that one is a real mistake. + +Note that an empty value and an absent key are the same thing to the credential reader, so +`username: ""` behaves exactly like omitting it. + #### Reusing a Flux or Argo CD credentials Secret The credential reader's design is **inspired by both Flux and Argo CD**: it accepts their Secret key @@ -1198,4 +1206,5 @@ The chart value reference for the starter `quickstart` block lives in - [commit-signing.md](commit-signing.md) for signing behavior on Git hosting platforms - [github-setup-guide.md](github-setup-guide.md) for GitHub auth setup +- [azure-devops-getting-started.md](azure-devops-getting-started.md) for Azure DevOps auth and setup - [sops-age-guide.md](sops-age-guide.md) for `GitTarget.spec.encryption` diff --git a/docs/design/azure-devops-multi-ack.md b/docs/design/azure-devops-multi-ack.md new file mode 100644 index 00000000..93423905 --- /dev/null +++ b/docs/design/azure-devops-multi-ack.md @@ -0,0 +1,641 @@ +# Azure DevOps and `multi_ack`: why it fails, and what to do about it + +> **design** — **decided and built: Option A, go-git v6.** Index: [`../INDEX.md`](../INDEX.md) +> +> Written against PR [#292](https://github.com/ConfigButler/gitops-reverser/pull/292) +> (issue [#288](https://github.com/ConfigButler/gitops-reverser/issues/288)), which proposes shelling +> out to a bundled `git` binary for Azure DevOps remotes. This page records what was **measured** on +> that branch and in the go-git history, separates it from what was **inferred**, and prices four +> options. +> +> **Outcome:** the migration to `go-git/v6@v6.0.0-alpha.5` is implemented. The +> [red-first ADO test](#what-the-migration-actually-cost) reproduces the failure locally with no +> Azure DevOps tenant, failed on v5 with ADO's exact HTTP 400, and passes on v6. `task lint` and +> `task test` pass. The e2e suite found a **fourth, production-breaking v6 behaviour change that no +> unit test could see** — v6 reads on-disk `known_hosts` even when a host key callback is supplied, so +> every SSH remote would have failed in the distroless image. It is fixed and pinned by a regression +> test. What the migration cost, and all four findings, are in the last section. + +## The short version + +- **go-git v6 already fixes this.** `multi_ack` is implemented (PR + [#1204](https://github.com/go-git/go-git/pull/1204)) and present in every v6 tag. Upstream then + deleted their Azure DevOps workaround example, saying ADO "works out of the box, no longer + requiring code changes". +- **v6 is not the moving target it looks like.** In the fourteen packages we import, the + breaking churn per alpha runs 96 → 39 → **1** → **9**. One breaking wave, settled in May. +- **Flux does not solve this with a git binary. It solves it by never fetching.** Its go-git client + has zero `Fetch` calls — every sync is a fresh clone. That is the whole trick, and it explains why + a four-line change is enough for Flux and would not be enough for us unchanged. +- **PR #292 as written costs 723 MB and blinds the CVE gate.** Measured: the image goes from 217 MB + to 940 MB, and Trivy reports **zero** findings on both, because the bundled git/OpenSSH/OpenSSL + arrive as loose files with no package database. +- **We can test all of this without an Azure DevOps tenant.** Canonical `git upload-pack` advertises + `multi_ack`, so the Gitea already in the e2e lab is a genuine multi_ack server. Front it with a + proxy that rejects requests omitting the capability and you have a faithful ADO simulator. + +## The mechanism + +`multi_ack` and `multi_ack_detailed` are protocol-v0 `upload-pack` capabilities. They change how the +server acknowledges the client's `have` lines during negotiation: instead of one `ACK`/`NAK`, the +server may stream `ACK continue` lines. + +go-git v5 declares both capabilities unsupported. In `plumbing/transport/common.go`: + +```text +var UnsupportedCapabilities = []capability.Capability{ + capability.MultiACK, + capability.MultiACKDetailed, + capability.ThinPack, +} +``` + +`FilterUnsupportedCapabilities` **deletes** them from the server's advertisement as the client parses +it. `ulreq.go` then builds the client's `want` line from what survived, so the request goes out +without `multi_ack`. Azure DevOps rejects any such `upload-pack` request with HTTP 400 and +`TF401041: Clients must support multi-ack.` + +This is six years old ([go-git#64](https://github.com/go-git/go-git/issues/64), +[source-controller#104](https://github.com/fluxcd/source-controller/issues/104)) and it is ADO's +fault, not go-git's: the capability is optional in the protocol and ADO treats it as mandatory. + +### The half that is not obvious + +The capability filter cuts both ways, and the two halves fail independently: + +- **The request half.** Advertising `multi_ack` is what ADO demands. Trimming + `UnsupportedCapabilities` fixes this, and nothing else is required. +- **The response half.** Once the client advertises `multi_ack`, the server is entitled to reply with + a multi-ACK stream, and v5 cannot parse it. `plumbing/protocol/packp/srvresp.go` in v5.19.1 (the + version we pin) still carries `TODO: Implement support for multi_ack or multi_ack_detailed + responses` and wraps the resulting scanner error as `multi_ack and multi_ack_detailed are not + supported`. + +The response half only triggers when the client sends `have` lines. **A fresh clone sends none** — +there is nothing local to negotiate against — so the server answers `NAK` and v5 copes. An +incremental fetch into a populated object store does send them, and v5 breaks. + +Upstream states exactly this in the v5 example `_examples/azure_devops/main.go`: + +```text +The initial clone operations require a full download of the repository, and therefore those +unsupported capabilities are not as crucial, so by removing them from that list allows for the +first clone to work successfully. + +Additional fetches will yield issues, therefore work always from a clean clone until those +capabilities are fully supported. + +New commits and pushes against a remote worked without any issues. +``` + +Note the last line. `receive-pack` does not use `multi_ack` at all, so **push is unaffected**. + +> **Inferred, not measured.** The clone-works/fetch-breaks split above is read off upstream's code +> and comments plus Flux's usage, not from a request against a real ADO endpoint. Everything in the +> next section about Flux *is* measured from its source. See +> [Testing without a tenant](#testing-without-an-azure-devops-tenant) for how to close the gap. + +## What our git paths actually need + +This section exists because the answer to "does the fix break our push?" is **no, and it cannot** — +but the reason is only obvious once you see which wire endpoint each of our operations touches. + +### The capability matrix + +Three operations reach the network. Everything in the table below is measured: the advertisements +from a local git server with protocol v0 forced, the request capabilities from go-git v5.19.1's +`packp.NewUploadPackRequestFromCapabilities` and `remote.newUploadPackRequest`. + +| Our operation | Service | Wire step | Capabilities we end up asking for | ADO risk | +|---|---|---|---|---| +| [`CheckRepo`](../../internal/git/git.go) — `remote.List()` | `upload-pack` | advertisement only (`GET /info/refs`) | none; we only read the advertisement | **none** | +| [`listRemoteRefs`](../../internal/git/git_smart_fetch.go) — `remote.List()` | `upload-pack` | advertisement only (`GET /info/refs`) | none | **none** | +| [`SmartFetch`](../../internal/git/git_smart_fetch.go) — `repo.Fetch()` | `upload-pack` | **negotiation** (`POST /git-upload-pack`) | `side-band-64k`, `ofs-delta`, `agent`, `no-progress`, `shallow` (because `Depth: 1`) — and `multi_ack_detailed`/`multi_ack` **only if advertised and not filtered** | **this is the failure** | +| [`PushAtomic`](../../internal/git/git_atomic_push.go) | `receive-pack` | advertisement + `POST /git-receive-pack` | `report-status` (we set exactly this one) | **none** | + +The decisive measurement is the `receive-pack` advertisement from canonical git: + +```text + refs/heads/main\0report-status report-status-v2 delete-refs side-band-64k quiet atomic + ofs-delta object-format=sha1 agent=git/2.39.5 +``` + +**`multi_ack` is not there, and it never is.** It is an `upload-pack` capability that does not exist +in the `receive-pack` protocol at all. `FilterUnsupportedCapabilities` runs against the receive-pack +advertisement too, but deleting a capability the server never offered is a no-op. + +So the blast radius of this whole problem is **one call**: `repo.Fetch`. Not the connectivity check, +not the ref listing, and **not the push**. + +```mermaid +flowchart LR + subgraph ours["our code"] + CR["CheckRepo"] + LRR["listRemoteRefs"] + SF["repo.Fetch"] + PA["PushAtomic"] + end + subgraph wire["Azure DevOps"] + ADV["GET /info/refs
advertisement"] + UP["POST /git-upload-pack
want + have negotiation"] + RP["POST /git-receive-pack
commands + packfile"] + end + CR --> ADV + LRR --> ADV + SF --> ADV + SF --> UP + PA --> RP + ADV --> OK1["works"] + UP --> BAD["HTTP 400
TF401041 must support multi-ack"] + RP --> OK2["works: receive-pack
has no multi_ack"] +``` + +### How the multi_ack request is actually built + +The filter does not reject the request. It makes the request incomplete, by hiding the capability +from the code that decides what to ask for. In v5's `ulreq.go`: + +```go +func NewUploadRequestFromCapabilities(adv *capability.List) *UploadRequest { + r := NewUploadRequest() + if adv.Supports(capability.MultiACKDetailed) { + r.Capabilities.Set(capability.MultiACKDetailed) + } else if adv.Supports(capability.MultiACK) { + r.Capabilities.Set(capability.MultiACK) + } + // ... side-band, thin-pack, ofs-delta, agent +} +``` + +`adv` has already been through `FilterUnsupportedCapabilities`, so both `Supports` checks are false +even though ADO advertised the capability. The `want` line goes out without it, and ADO 400s. Trimming +`UnsupportedCapabilities` restores the advertisement, both checks pass, and the capability is +requested. That is the entire request-half fix. + +### The special push, and why it is safe + +Your recollection is right. There is one `ReceivePackSession`, and the remote state is read **on that +same session** before pushing to it: + +```mermaid +sequenceDiagram + participant W as branch worker + participant S as ReceivePackSession + participant R as remote + W->>S: NewReceivePackSession, one connection + S->>R: GET /info/refs?service=git-receive-pack + R-->>S: advertisement: refs + report-status, atomic, ofs-delta + Note over W,S: validatePushState reads the SAME session + S-->>W: AdvertisedReferences: branch to remoteHash, root to currentRootHash + W->>W: guard 1 - rootHash must equal currentRootHash
else "remote received unknown updates" + W->>W: revlist.Objects from localHash stopping at rootHash + W->>W: packfile.NewEncoder over exactly that delta + W->>S: ReceivePack: Command Old=oldHash New=localHash + packfile + S->>R: POST /git-receive-pack + R-->>W: report-status - guard 2, server rejects if ref is not at Old +``` + +Two things carry the safety, and it is worth separating them because only one is about the session: + +- **Guard 1, client-side** — `rootHash != currentRootHash` is a freshness check against the + advertisement. Reading it on the same session is what makes the value trustworthy: there is no + second connection during which the remote could move. +- **Guard 2, server-side** — `packp.Command{Old: oldHash, New: localHash}` is a + **compare-and-swap**. The server refuses the update unless the ref is exactly at `Old`. This is + what makes the push genuinely atomic rather than merely well-timed, and it is enforced by the + remote, not by us. + +Neither guard involves `upload-pack`, so **neither is touched by `multi_ack`, by trimming +`UnsupportedCapabilities`, or by changing how we read.** The push path is out of scope for every +option on this page. + +### The one real interaction: shallow object stores + +The push does depend on the shape of the local object store, via +`revlist.Objects(repo.Storer, []{localHash}, []{rootHash})` — walk from our new commit, stop at the +commit we based it on, pack exactly that delta. + +This is where a clone-versus-fetch change could in principle bite, and the answer is that **it already +does not**, because we already push from a shallow repository today: `SmartFetch` on `main` fetches +with `Depth: 1`, so the local store never holds more than the tip plus what we wrote. `rootHash` is +that tip, it is present, and its tree is complete, so the walk terminates and the delta is correct. + +Any option that keeps `rootHash` present as the tip and leaves `HEAD` on the working branch leaves the +push byte-identical. That is a constraint on the *plumbing around* the read, not on the push. + +> Worth noting in passing: `newUploadPackRequest` sets `no-progress` only when `o.Progress == nil`. +> PR #292's `Progress: io.Discard` therefore stops us asking for `no-progress`, so the server starts +> streaming sideband progress data that we then throw away. A second small cost of that +> out-of-scope change, on top of losing `Depth: 1`. + +## How Flux resolves this + +This is the most useful precedent available, because Flux supports Azure DevOps in production on +**go-git v5** with no git binary in the image. + +**Step one: it trims the capability list.** In `pkg/git/gogit/client.go` (upstream `fluxcd/pkg`, and +the local read-only checkout under `external-sources/flux/`), in a package `init()`: + +```go +func init() { + // Git servers that exclusively use the v2 wire protocol, such as Azure + // Devops and AWS CodeCommit require the capabilities multi_ack + // and multi_ack_detailed, which are not fully implemented by go-git. + // Hence, by default they are included in transport.UnsupportedCapabilities. + transport.UnsupportedCapabilities = []capability.Capability{ + capability.ThinPack, + } +} +``` + +Four lines, applied globally to every provider. We do not do this anywhere. + +**Step two — and this is the part that actually makes it work: Flux never fetches.** Its go-git +client contains **zero** `Fetch` calls. Every operation that touches a remote is +`extgogit.CloneContext`, in four variants (by branch, by tag, by commit, by semver range), all in +`pkg/git/gogit/clone.go`. Clones are shallow (`Depth: 1`) when `ShallowClone` is set. Pushes go +through the high-level `PushContext` with refspecs, not a hand-rolled receive-pack session. + +So Flux only ever exercises the case upstream says works. It never enters the negotiation path v5 +cannot parse. Its comment even repeats the constraint verbatim — *"work always from a clean clone"*. + +**What this means for us.** Our design is the opposite: [`git.go`](../../internal/git/git.go) does +`PlainInit(repoPath, false)` into a persistent non-bare working clone, and +[`git_smart_fetch.go`](../../internal/git/git_smart_fetch.go) does incremental +`repo.Fetch` with computed refspecs against it. That is deliberate and it is the efficient design. +It is also precisely the case v5 cannot serve against ADO. The four-line trim alone will not save +us — but it is not therefore useless, see Option B. + +**One caveat on the precedent.** Flux's ADO coverage is an integration test against a real tenant +(`TF_VAR_azuredevops_org` / `TF_VAR_azuredevops_pat`, `pkg/tests/integration/azure_test.go`), run +manually and explicitly excluded from their normal test targets. Nobody has this in CI. + +## Is go-git v6 stable enough? + +### `multi_ack` is done there + +- Implementation `14eabbda`, merged as `858d421c` (PR #1204). Present in **every** v6 tag, + alpha.1 through alpha.5. +- Upstream commit `2ef805c2` then **removed** their ADO example: + *"Since the multi_ack implementation (#1204), Azure DevOps works out of the box, no longer + requiring code changes."* +- v6 negotiation reads and sets both capabilities (`plumbing/transport/negotiate.go`, + `upload_pack.go`), and its transport test suite asserts the advertisement. + +### The churn, measured + +Alpha cadence: alpha.1 2026-04-01, alpha.2 04-16, alpha.3 05-06, alpha.4 05-18, alpha.5 2026-07-29. + +Exported declarations removed-or-changed versus added, counted **only in the fourteen packages this +repository imports**: + +| Transition | Removed / changed | Added | +|---|---|---| +| alpha.1 → alpha.2 | 96 | 64 | +| alpha.2 → alpha.3 | 39 | 32 | +| alpha.3 → alpha.4 | **1** | 27 | +| alpha.4 → alpha.5 | **9** | 69 | + +That is not a project that breaks its API often. It is one large breaking wave — the transport +rewrite, landed before alpha.1 and settled by alpha.2 — followed by three months of essentially +additive change. The concern in PR #292 that adapting to v6 means "keeping up with the changes until +a stable version is released" is a fair prior, but the data does not support it. + +`v5` is also still maintained in lockstep: `v5.19.2` was tagged the **same day** as alpha.5. There is +no rollback cliff. + +### What the migration actually costs us + +Measured by compiling this repository against the v6 checkout. Four removals matter: + +| Gone in v6 | Replacement | Where it hurts | +|---|---|---| +| `plumbing/transport/client` (whole package) | transport loader / `client.Option` | [`git_atomic_push.go`](../../internal/git/git_atomic_push.go) | +| `transport.AuthMethod` | functional options: `client.WithSSHAuth`, `client.WithHTTPAuth`, passed via `ClientOptions` | 21 references across 8 files | +| `transport.NewEndpoint`, `NewReceivePackSession`, `transport.ReceivePackSession` | `transport.ParseURL`, `transport.PushRequest`, `transport.ReceivePack(...)` | our hand-rolled atomic push | +| `plumbing/protocol/packp/capability` | moved to `plumbing/protocol/capability`; `NewList` removed | import sweep | + +`transport.AuthMethod` is the painful one: it is our central credential abstraction, returned by +`AuthFromSecretData` and threaded from the GitProvider controller through every git operation. v6 +replaces the `Options.Auth` field with `ClientOptions []client.Option`. + +Realistically: two files genuinely rewritten (`git_atomic_push.go`, and the auth plumbing in +[`internal/ssh/auth.go`](../../internal/ssh/auth.go) plus +[`credentials.go`](../../internal/git/credentials.go)), then a mechanical import and typing sweep. +Not a small PR. Bounded, and the existing suite is what makes it affordable. + +## What merging PR #292 as written would cost + +All figures below are from building both images locally for `linux/amd64` and scanning them. + +### Image size: 217 MB → 940 MB + +```text +rev:main 217MB +rev:pr292 940MB +``` + +**+723 MB, 4.3×.** This is a Dockerfile bug, not the inherent cost of bundling git. +`cp -rL /usr/libexec/git-core` dereferences 165 entries that are all links to the same git binary: + +```text +/usr/libexec/git-core in the alpine stage 3.2M +the same directory after cp -rL 437.5M +``` + +`cp -a` instead of `cp -rL`, or copying only `git-remote-http` and `git-remote-https`, brings a +correct bundle to roughly 15–20 MB. Worth saying on the PR: it is a one-character fix and it removes +most of the size argument. + +### arm64: still works + +- `alpine:3.24@sha256:28bd…` is an OCI image index that includes `linux/arm64`. +- The `git-bundle` stage carries no `--platform`, so it resolves to `TARGETPLATFORM` and an arm64 + build gets arm64 binaries. +- The `ld-musl*.so*` glob is architecture-agnostic. +- `build-release-arm64` runs on a native `ubuntu-24.04-arm` runner + ([`ci.yml`](../../.github/workflows/ci.yml)), so no QEMU emulation is involved. + +Functionally fine. The 30-minute job timeout now has to push 940 MB per architecture. + +TLS from the bundled git also works: `git ls-remote https://github.com/go-git/go-git.git` succeeds +inside the built image, so distroless' CA bundle is found. + +### Security: worse in a way the gate cannot see + +Trivy against both images, `CRITICAL,HIGH,MEDIUM`: + +```text +rev:main (debian 13.6) 0 vulnerabilities +rev:pr292 (debian 13.6) 0 vulnerabilities +``` + +The PR image contains **git 2.54.0, OpenSSH 10.3p1, OpenSSL 3.5.7**, libcurl, libssl, libcrypto, +zlib, pcre2, nghttp2, brotli, c-ares and libidn2. Trivy detects **none** of it. The image identifies +as distroless/Debian while those libraries arrive as loose Alpine files with no apk database for the +scanner to read. + +The CI gate — `severity: CRITICAL`, `ignore-unfixed`, `exit-code: 1` in +[`ci.yml`](../../.github/workflows/ci.yml) — is therefore structurally blind to every future CVE in +git, OpenSSH and OpenSSL. So is Dependabot. **That is the real security cost: not more +vulnerabilities, but unscannable and unschedulable ones.** + +Not a new problem: a shell was already present at `/busybox/sh` in the current image, so `/bin/sh` is +a convenience path rather than added attack surface. But git plus ssh plus a shell is a materially +better post-exploit toolkit than one static Go binary. + +### An out-of-scope regression on every provider + +[`git_smart_fetch.go`](../../internal/git/git_smart_fetch.go) drops `Depth: 1` and adds +`Progress: io.Discard` on the **non-ADO** path. Every provider changes from a shallow fetch to a +full-history one. This is not mentioned in the PR description, and it undoes deliberate efficiency +work. + +### Smaller items + +- **Coverage is 10%.** 223 of 243 new lines in `ado_system_git.go` are untested, and there is no ADO + e2e (there cannot be — no ADO in the lab). The fallback ships essentially unexercised; only URL + parsing and environment construction are covered. +- **Environment is stripped.** `cmd.Env` is set to only `GIT_TERMINAL_PROMPT`, + `GIT_CONFIG_NOSYSTEM`, `PATH` and the auth variables. Pod-level `HTTPS_PROXY`, `NO_PROXY` and + `SSL_CERT_FILE` are dropped. No regression today — `GitProvider` has no proxy or CA-bundle field — + but the two code paths now behave differently, and Flux's clone options carry `CABundle` and + `ProxyOptions` precisely because users need them. +- **New unencrypted key material on disk.** The SSH path decrypts the private key and writes a + plaintext PEM to a `0600` temp file. It is cleaned up, but go-git never persisted key material. + This also changes `internal/ssh` for **all** providers, not just ADO: `GetAuthMethod` now always + re-serialises, and a key type `MarshalPrivateKey` cannot handle silently yields a nil + `PrivateKeyPEM`. +- **The Go code itself is careful.** Host-parsed detection rather than substring matching, a scheme + allowlist that rejects `ext::`, `extraHeader` scoped per origin, no credentials in argv, stderr + redaction. The craft is good; the objection is to the packaging and the premise, not the + implementation. + +## Testing without an Azure DevOps tenant + +Not having a tenant is the thing blocking every option here, including deciding whether the +diagnosis is even complete. Two ways out, and the second is the interesting one. + +### A free tenant + +Azure DevOps has a free tier — unlimited private Git repositories for small teams — that needs only a +Microsoft account, with no Azure subscription. Worth ten minutes, and it is what Flux's own +integration test requires. This is the only way to validate against the real server. + +### A faithful local simulator + +**Canonical `git upload-pack` advertises `multi_ack` and `multi_ack_detailed`.** Verified on a local +bare repository with protocol v0 forced: + +```text +packet: ls-remote< refs/heads/main\0multi_ack thin-pack side-band side-band-64k ofs-delta + shallow deepen-since deepen-not deepen-relative no-progress include-tag multi_ack_detailed + object-format=sha1 agent=git/2.39.5 +``` + +That is the load-bearing fact. It means a plain git server is a **genuine** multi_ack server, so both +failure halves reproduce locally: + +- **The request half** — add a small reverse proxy in front of the git HTTP endpoint that returns + HTTP 400 with a `TF401041` body when a `POST` to `*/git-upload-pack` does not contain `multi_ack`. + That is ADO's rejection, exactly. +- **The response half** — comes for free. Once the client advertises the capability, real + `upload-pack` will actually emit multi-ACK streams on a fetch with `have` lines, which is the thing + v5 cannot parse. + +The e2e lab already runs Gitea (see [`e2e-git-server-choice.md`](e2e-git-server-choice.md)), whose +backend is canonical git, so no new server is needed — only the proxy and a `GitProvider` pointed at +it. This is cheap, it is deterministic, it runs in CI, and it would let us: + +1. Confirm the diagnosis in #288 end to end. +2. Prove or kill Option B below without a tenant. +3. Regression-test whichever fix we take, forever — which is impossible today for any option, + including the one PR #292 proposes. + +## The options + +### Option A — migrate to go-git v6 + +The real fix. Upstream says ADO works out of the box; the churn data says the API has been stable +since May; v5 stays maintained so backing out is possible. Cost: two files rewritten plus an import +sweep, validated by the existing suite. No Dockerfile change, no image growth, no scanner blindness, +and it deletes the problem instead of routing around it. + +Wins if the v6 spike comes back green. + +### Option B — trim `UnsupportedCapabilities`, and clone instead of fetch on ADO + +The option neither participant in #292 raised, and it is Flux's actual architecture. Four lines to +advertise the capability, plus an ADO-specific path that re-clones rather than fetching into the +persistent clone. Stays on v5, stays pure Go, no git binary, no image growth. + +Cost: a full or shallow clone per sync for ADO users, which is the efficiency the current design +exists to avoid — but it is scoped to ADO, and it is provably enough for Flux's entire user base. + +**It does not touch the atomic push.** Per +[the capability matrix](#the-capability-matrix), `PushAtomic` speaks `receive-pack`, which has no +`multi_ack`, and the two guards that make it atomic live in the advertisement of that session and in +the server-side compare-and-swap. A clone-fresh read path changes what fills the object store, not +how we write. The only obligations it inherits are the ones +[the shallow-store section](#the-one-real-interaction-shallow-object-stores) names: leave `rootHash` +present as the tip, and leave `HEAD` on the working branch — which is what the post-fetch branch +setup does today anyway, since a fresh clone lands `HEAD` on the remote default branch. + +Wins if the v6 spike is worse than expected and we want a pure-Go stopgap. + +### Option C — PR #292, repaired + +Take the system-git fallback, but not as written. Minimum changes: + +- `cp -a` rather than `cp -rL` (940 MB → roughly 20 MB). +- Revert the `Depth: 1` and `Progress` change on the non-ADO path. +- Restore CVE visibility for the bundled git, OpenSSH and OpenSSL — otherwise the image-scan gate is + decorative for a third of the runtime. +- Pass through proxy and TLS environment. + +Wins only if both A and B fail. Note that the PR's own documentation says the fallback is temporary +and should be deleted when v6 lands, so this option is by construction the one that has to be paid +for twice. + +### Option D — do nothing yet + +Defensible while the diagnosis is unverified by us. Costs an ADO user, who has already turned up. + +## What has to be true, and what to do next + +Ordered by what unblocks the most: + +1. **Build the local simulator.** It is independent of which option wins, it is the only piece that + makes any of them testable in CI, and it does not need a tenant. Everything else is a guess until + this exists. +2. **Spike v6 on a branch** and run `task test`. Four known removals, two files. This is the + cheapest way to find out whether Option A is a week or an afternoon. +3. **Ask the contributor to try the four-line trim** against their real tenant. They have what we do + not. The result tells us whether the diagnosis is complete: if `CheckRepo` and push start working + and only fetch fails, Option B is confirmed and the fix is small. +4. **Decide between A and B on the spike result**, and treat #292 as the fallback rather than the + proposal. + +## What the migration actually cost + +Built on `feat/go-git-v6` against `go-git/v6@v6.0.0-alpha.5` (the latest tag, and identical to +upstream `main` at the time). `task lint` and `task test` pass. + +### The red-first test + +[`internal/git/ado_multiack_test.go`](../../internal/git/ado_multiack_test.go) implements the +simulator this page proposed: canonical git's `git-http-backend` behind a proxy that returns HTTP 400 +with `TF401041` when an `upload-pack` POST omits `multi_ack`, and strips the `Git-Protocol` header so +a v2-capable client cannot sidestep the capability under test. Four tests, and the v5 baseline +confirmed the capability matrix exactly: + +| Test | on v5 | on v6 | What it pins | +|---|---|---|---| +| `TestADOSimulator_IsFaithful` | pass | pass | the harness rejects like ADO, and real git clones through it | +| `TestADO_CheckRepo_NeedsNoNegotiation` | pass | pass | advertisement-only, asserts **zero** `upload-pack` POSTs | +| `TestADO_PushAtomic_NeedsNoMultiAck` | pass | pass | `receive-pack` is unaffected; the push is out of scope | +| `TestADO_SmartFetch_RequiresMultiAck` | **fail, HTTP 400** | **pass** | the fix | + +The middle two passing on v5 is the point: they prove PR #292 routes `CheckRepo` through system git +for no reason, and that the atomic push never needed touching. + +### The API changes, as built + +All four predicted removals were real, and the predicted shape held: + +- `transport.AuthMethod` → `[]gitclient.Option`. A nil slice means anonymous, which is cleaner than + v5's nil interface. +- `transport.NewEndpoint` + `client.NewClient` + `NewReceivePackSession` → + `transport.ParseURL` + `gitclient.New(opts...).Handshake`. +- `session.AdvertisedReferences()` → `session.GetRemoteRefs(ctx, nil)`, returning a + `[]*plumbing.Reference` rather than a map, so `advertisedHashes` builds the lookup. +- `session.ReceivePack(ctx, req)` → `session.Push(ctx, storer, *transport.PushRequest)`. + +**The compare-and-swap survived verbatim**, as predicted: `PushRequest.Commands` takes the same +`[]*packp.Command`, so `Old`/`New` is unchanged. Two things got better — v6 negotiates +`report-status` itself and returns a rejected command as the error from `Push` (so our separate +status-struct inspection collapsed into one error check), and `Atomic` is now a first-class field. + +One thing the analysis got wrong in emphasis: because v6's options are **opaque closures**, callers +can no longer inspect what kind of credential they hold, which broke a genuinely valuable test matrix +(which Secret key maps to which auth field). The fix is a `git.Credential` struct that carries the +concrete value, with `Options()` rendering it — a better abstraction than the bare slice, and the +place to hang v6's new `WithCABundle` / `WithProxyEnvironment` when we want them. + +### Four findings the analysis did not predict + +**0. The one that would have shipped a broken release: v6 reads on-disk `known_hosts` even when you +supply a host key callback.** Its SSH transport derives `HostKeyAlgorithms` by loading +`~/.ssh/known_hosts` and `/etc/ssh/ssh_known_hosts` whenever `ClientConfig` returns with that field +empty — *including when a `HostKeyCallback` was already set* (`plumbing/transport/ssh/ssh.go`, the +`else if len(config.HostKeyAlgorithms) == 0` branch) — and fails the connection with +`unable to find any valid known_hosts file, set SSH_KNOWN_HOSTS env variable` when neither file +exists. The controller image is distroless with no home directory and no system `known_hosts`, so +**every SSH remote would have failed in production**, whether or not the credential pinned a host +key. v5 derived no algorithms and so never looked. + +Fixed by `ssh.KeyAuth`, which wraps go-git's `PublicKeys` and guarantees `HostKeyAlgorithms` is +populated: from the pinned `known_hosts` when there is one (matching git's own behaviour — offering an +algorithm the pin does not cover just makes the server present a key the callback then rejects), and +from a modern default set when host key verification is disabled. + +Worth dwelling on *how* this was found, because it is the whole argument for the e2e gate. Unit tests +could not catch it: the fallback lives in the transport's `connect`, not in `ClientConfig`, so a test +that builds a credential and inspects it passes cleanly — mine did. It took a real SSH server, and it +presented as one failing spec out of 71 whose error text pointed at host keys rather than at the +migration. The regression test that now pins it asserts the property that actually matters — the +algorithm list is never empty — rather than re-asserting the credential shape. + +And the other three. + +**1. v6 honours `commit.gpgSign`, and fails closed.** `worktree_commit.go` consults the setting +merged across system, global and local scope whenever `CommitOptions.Signer` is nil, and refuses with +`cannot auto-sign commit` when it is true with no signer registered. v5 ignored it entirely. Any +environment whose gitconfig sets it — a developer machine, a mounted config, a future base image — +would break every commit we make. Handled by `PinExplicitSigningPolicy`, which writes +`commit.gpgSign = false` into the repository's own config at init: our signing policy comes from the +GitProvider, not from ambient config. + +**2. `file://` is no longer a faithful server, and that silently weakened a test.** v5's file +transport spawned the real `git-receive-pack` binary; v6 runs go-git's own in-process +`transport.ReceivePack`, whose `updateReferences` checks only that a reference *exists* and then +calls `SetReference(cmd.New)` — **it never compares `cmd.Old`**. So over `file://` every push wins. +`TestBranchWorker_ConcurrentOperations` caught it: three racing pushes all "succeeded", last write +won, and the repository ended with 2 commits instead of 4. This is not a v6 regression in the server +(v5's built-in server had identical code) — it is that `file://` stopped using real git. The test now +runs against `startRealGitServer`, where real git rejects with +`cannot lock ref 'refs/heads/main': is at X but expected Y` and our retry serialises the writers. +**Any future test of the compare-and-swap must use a real server, not `file://`.** + +**3. `Progress` controls `no-progress`.** `newUploadPackRequest` sets `no-progress` only when +`Progress == nil`, which is a second cost of PR #292's out-of-scope change to the non-ADO path. + +Also mechanical: `PlainClone` lost its `isBare` positional argument (it moved into `CloneOptions`), +`Worktree.Filesystem` became a method, `Signer.Sign` takes a context, `Commit.PGPSignature` became +`Commit.Signature`, and `plumbing/transport/client` moved to `plumbing/client`. + +### Upstream opportunity + +go-git's built-in `receive-pack` accepting an update whose `Old` does not match the current +reference makes it non-conformant as a server and, worse, makes it a test double that silently passes +things a real server rejects. That looks like a reportable upstream bug with a small fix. + +## Open questions + +- Does the four-line trim actually let `CheckRepo` and `PushAtomic` succeed against ADO? Inferred + yes from upstream's *"new commits and pushes worked without any issues"*, unverified here. +- Does `remote.List()` — our connectivity check — even hit the 400? Code-read says no: `remote.list` + calls only `AdvertisedReferencesContext`, never a `POST`, and ADO's `TF401041` is documented on the + `upload-pack` `POST`. So `CheckRepo` and `listRemoteRefs` should already work today and PR #292 + routes them through system git unnecessarily. The simulator confirms or refutes this. +- **Answered: v6 preserves the single-session push.** Its `Session` interface is + `Handshake` → `Capabilities` / `GetRemoteRefs` / `Fetch` / `Push` / `Close`, which maps 1:1 onto our + `NewReceivePackSession` → `AdvertisedReferences` → `ReceivePack`, and `PushRequest.Commands` is the + **same `[]*packp.Command`** type we build today, so the `Old`/`New` compare-and-swap survives + verbatim. `Atomic` becomes a first-class field, and `GetRemoteRefsOptions.RefPrefixes` maps to + protocol-v2 `ls-refs`, so v6 would let us stop pulling a full advertisement. This is an argument + **for** Option A, not a risk against it. +- Should the ADO clone-fresh strategy in Option B be shallow? Probably yes, matching both Flux and + our current `Depth: 1`. It does not disturb the push path's `revlist` walk — see + [shallow object stores](#the-one-real-interaction-shallow-object-stores) — but it does need + `capability.Shallow`, which ADO advertises. diff --git a/docs/facts/azure-devops-multi-ack-requirement.md b/docs/facts/azure-devops-multi-ack-requirement.md new file mode 100644 index 00000000..c5bd82aa --- /dev/null +++ b/docs/facts/azure-devops-multi-ack-requirement.md @@ -0,0 +1,133 @@ +# Azure DevOps and `multi_ack`: what was actually measured + +> **facts** — durable reference. Index: [`../INDEX.md`](../INDEX.md) +> +> Measured 2026-07-30 against `dev.azure.com` with a Personal Access Token. The folklore is nearly +> right, in a way that makes it easy to reproduce the wrong thing; the +> [wrong turn](#a-wrong-turn-worth-recording) is recorded so nobody repeats it. + +## The rule + +**Azure DevOps answers HTTP 400 to an `upload-pack` request whose capability list omits both +`multi_ack` and `multi_ack_detailed`:** + +```text +TF401041: The Git protocol sent is not as expected (Clients must support multi-ack...) +``` + +The trigger is a capability list that omits it, not the absence of negotiation. Measured, one request +shape per row, all against the same repository and tip: + +| `want` line | HTTP | +|---|---| +| `want ` — no capability list at all | **200** | +| `want side-band-64k ofs-delta agent=git/2.39.5` | **400 `TF401041`** | +| `want side-band-64k` | **400 `TF401041`** | +| `want agent=git/2.39.5` | **400 `TF401041`** | +| `want multi_ack side-band-64k ofs-delta agent=git/2.39.5` | **200** | +| `want multi_ack_detailed side-band-64k ofs-delta` | **200** | + +Either capability satisfies it. The first row is the trap: a bare `want` with no capability list is +accepted, and no real client sends that, so probing with one measures nothing useful. + +Reproduce a row with: + +```bash +SHA=$(git ls-remote HEAD | awk '{print $1}') +CAPS=" side-band-64k ofs-delta" # add multi_ack here to flip the result +python3 -c " +import sys +want='want $SHA'+sys.argv[1]+'\n' +sys.stdout.write('%04x%s' % (len(want)+4, want) + '0000' + '%04xdone\n' % 9)" "$CAPS" | +curl -s -o /dev/stdout -w '\nHTTP %{http_code}\n' \ + -H "Authorization: Basic $(printf ':%s' "$PAT" | base64 -w0)" \ + -H 'Content-Type: application/x-git-upload-pack-request' --data-binary @- \ + "/git-upload-pack" +``` + +## What it does to go-git v5 + +go-git v5 keeps `MultiACK` and `MultiACKDetailed` in `transport.UnsupportedCapabilities` and deletes +them from the server's advertisement while parsing it, so +`packp.NewUploadPackRequestFromCapabilities` never sees them and never asks for them. Every fetch it +sends therefore carries a capability list without `multi_ack`, which is precisely the rejected shape. + +Run directly against Azure DevOps with `go-git/v5@v5.19.1`, the version this project pinned before +[#297](https://github.com/ConfigButler/gitops-reverser/pull/297): + +```text +== step 1: clone (want/done, no have lines) == +CLONE FAILED: unexpected client error: unexpected requesting +"https://dev.azure.com///_git//git-upload-pack" status code: 400 +``` + +**Not even the clone works.** Upstream's note that the initial clone succeeds holds only *after* +trimming `UnsupportedCapabilities`, which makes the client advertise the capability again. That trim +is Flux's four-line workaround; it fixes the request, but v5 still cannot decode the multi-ACK +*responses* the server may then send (`srvresp.go` carries a `TODO` for exactly that), which is why +Flux only ever clones. v6 implements the capability properly. + +## Why canonical git never notices + +`git upload-pack` advertises `multi_ack` and `multi_ack_detailed`, so canonical git always negotiates +one of them and never constructs the rejected shape. Verified against a local repository with +protocol v0 forced: + +```text + refs/heads/main\0multi_ack thin-pack side-band side-band-64k ofs-delta shallow deepen-since + deepen-not deepen-relative no-progress include-tag multi_ack_detailed object-format=sha1 + agent=git/2.39.5 +``` + +This is also what makes a local `git-http-backend` a usable stand-in for ADO in tests: it is a real +multi_ack-speaking server, so putting a proxy in front that enforces ADO's rule reproduces both halves +of the problem without needing a tenant. + +`receive-pack` is a different protocol and has no `multi_ack` at all, measured: + +```text + refs/heads/main\0report-status report-status-v2 delete-refs side-band-64k quiet atomic + ofs-delta object-format=sha1 agent=git/2.39.5 +``` + +So **pushing to Azure DevOps was never affected**, on any version. + +## A wrong turn worth recording + +A bare `want ` with no capability list returns 200, which briefly looked like evidence that the +six-year-old bug reports were wrong. It is the one row above ADO accepts, and no client sends it. +Running go-git v5 against the same repository produced the reported 400 immediately. + +Probe with the shape the real client sends: a hand-rolled request tests the request you built, not +the behaviour you are attributing to it. + +## Sources + +- [go-git#64](https://github.com/go-git/go-git/issues/64) — the original Azure DevOps report, open + since 2019. +- [fluxcd/source-controller#104](https://github.com/fluxcd/source-controller/issues/104) — Flux + hitting the same wall, and the comment thread that leads to their workaround. +- [go-git#1204](https://github.com/go-git/go-git/pull/1204) — the `multi_ack` implementation, merged + for v6. Upstream subsequently deleted their `_examples/azure_devops` with the message *"Since the + multi_ack implementation (#1204), Azure DevOps works out of the box, no longer requiring code + changes."* +- go-git v5 `_examples/azure_devops/main.go` — the `UnsupportedCapabilities` trim, and the warning + that additional fetches will yield issues. +- go-git v5 `plumbing/protocol/packp/srvresp.go` — the unimplemented multi-ACK response decoding. +- `fluxcd/pkg` `pkg/git/gogit/client.go` — the trim applied in an `init()`, alongside a client that + only ever clones. +- [Git protocol capabilities](https://git-scm.com/docs/protocol-capabilities) — `multi_ack` and + `multi_ack_detailed` are `upload-pack` capabilities; `receive-pack` has neither. + +## Where this is exercised + +| Test | Needs a tenant? | +|---|---| +| [`internal/git/ado_multiack_test.go`](../../internal/git/ado_multiack_test.go) | no — a local `git-http-backend` behind a proxy enforcing the rule, runs in CI | +| [`internal/git/ado_live_test.go`](../../internal/git/ado_live_test.go) | yes — including `TestADOLive_StillRequiresMultiAck`, the canary that fails if Microsoft ever fixes this | +| [`test/e2e/ado_e2e_test.go`](../../test/e2e/ado_e2e_test.go) | yes — the operator mirroring into a real ADO repository | + +The simulator is deliberately stricter than ADO: it rejects any `upload-pack` POST carrying neither +`multi_ack` nor `multi_ack_detailed` — its check matches the `multi_ack` prefix, so either satisfies +it, exactly as ADO does — including the bare-`want` shape ADO accepts. That difference does not matter for what it +gates, and the strictness is what keeps it simple. diff --git a/docs/security-model.md b/docs/security-model.md index 5212b1a6..5b591486 100644 --- a/docs/security-model.md +++ b/docs/security-model.md @@ -134,8 +134,8 @@ names so an existing GitOps Secret works unchanged (see | Key | Required | Notes | |---|---|---| -| `username` | yes | Git username. | -| `password` | yes | Token or password. | +| `username` | no | Git username. Omit it for Azure DevOps, which ignores the username and reads the PAT from `password`. | +| `password` | yes | Token or password. It is what selects basic auth: a `username` with no `password` is an error. | ### HTTPS (bearer token) diff --git a/go.mod b/go.mod index 81e7c3bf..eed29246 100644 --- a/go.mod +++ b/go.mod @@ -7,8 +7,8 @@ require ( github.com/alicebob/miniredis/v2 v2.38.0 github.com/cespare/xxhash/v2 v2.3.0 github.com/fluxcd/pkg/apis/meta v1.31.0 - github.com/go-git/go-billy/v5 v5.9.1 - github.com/go-git/go-git/v5 v5.19.1 + github.com/go-git/go-billy/v6 v6.0.0-alpha.2 + github.com/go-git/go-git/v6 v6.0.0-alpha.5 github.com/go-logr/logr v1.4.4 github.com/onsi/ginkgo/v2 v2.32.0 github.com/onsi/gomega v1.42.1 @@ -38,7 +38,6 @@ require ( require ( cel.dev/expr v0.25.2 // indirect - dario.cat/mergo v1.0.2 // indirect filippo.io/hpke v0.4.0 // indirect github.com/Masterminds/semver/v3 v3.5.0 // indirect github.com/Microsoft/go-winio v0.6.2 // indirect @@ -48,7 +47,6 @@ require ( github.com/blang/semver/v4 v4.0.0 // indirect github.com/cenkalti/backoff/v5 v5.0.3 // indirect github.com/cloudflare/circl v1.6.4 // indirect - github.com/cyphar/filepath-securejoin v0.7.0 // indirect github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect github.com/emicklei/go-restful/v3 v3.13.0 // indirect github.com/emirpasic/gods v1.18.1 // indirect @@ -57,7 +55,7 @@ require ( github.com/fsnotify/fsnotify v1.10.1 // indirect github.com/fxamacker/cbor/v2 v2.9.2 // indirect github.com/go-errors/errors v1.5.1 // indirect - github.com/go-git/gcfg v1.5.1-0.20230307220236-3a3c6141e376 // indirect + github.com/go-git/gcfg/v2 v2.0.2 // indirect github.com/go-logr/stdr v1.2.2 // indirect github.com/go-logr/zapr v1.3.0 // indirect github.com/go-openapi/jsonpointer v1.0.0 // indirect @@ -75,7 +73,6 @@ require ( github.com/go-openapi/swag/typeutils v0.27.3 // indirect github.com/go-openapi/swag/yamlutils v0.27.3 // indirect github.com/go-task/slim-sprig/v3 v3.0.0 // indirect - github.com/golang/groupcache v0.0.0-20241129210726-2c02b8208cf8 // indirect github.com/google/cel-go v0.30.0 // indirect github.com/google/gnostic-models v0.7.1 // indirect github.com/google/go-cmp v0.7.0 // indirect @@ -83,7 +80,6 @@ require ( github.com/google/uuid v1.6.0 // indirect github.com/grpc-ecosystem/grpc-gateway/v2 v2.29.0 // indirect github.com/inconshreveable/mousetrap v1.1.0 // indirect - github.com/jbenet/go-context v0.0.0-20150711004518-d14ea06fba99 // indirect github.com/json-iterator/go v1.1.12 // indirect github.com/kevinburke/ssh_config v1.6.0 // indirect github.com/klauspost/cpuid/v2 v2.4.0 // indirect @@ -96,12 +92,10 @@ require ( github.com/prometheus/otlptranslator v1.0.0 // indirect github.com/prometheus/procfs v0.21.1 // indirect github.com/sergi/go-diff v1.4.0 // indirect - github.com/skeema/knownhosts v1.3.2 // indirect github.com/spf13/cobra v1.10.2 // indirect github.com/spf13/pflag v1.0.10 // indirect github.com/stretchr/objx v0.5.3 // indirect github.com/x448/float16 v0.8.4 // indirect - github.com/xanzy/ssh-agent v0.3.3 // indirect github.com/xlab/treeprint v1.2.0 // indirect github.com/yuin/gopher-lua v1.1.2 // indirect go.opentelemetry.io/auto/sdk v1.2.1 // indirect @@ -132,7 +126,6 @@ require ( google.golang.org/protobuf v1.36.12-0.20260120151049-f2248ac996af // indirect gopkg.in/evanphx/json-patch.v4 v4.13.0 // indirect gopkg.in/inf.v0 v0.9.1 // indirect - gopkg.in/warnings.v0 v0.1.2 // indirect k8s.io/apiextensions-apiserver v0.36.3 // indirect k8s.io/component-base v0.36.3 // indirect k8s.io/klog/v2 v2.140.0 // indirect diff --git a/go.sum b/go.sum index 8e1daaba..4a713afb 100644 --- a/go.sum +++ b/go.sum @@ -2,15 +2,12 @@ c2sp.org/CCTV/age v0.0.0-20251208015420-e9274a7bdbfd h1:ZLsPO6WdZ5zatV4UfVpr7oAw c2sp.org/CCTV/age v0.0.0-20251208015420-e9274a7bdbfd/go.mod h1:SrHC2C7r5GkDk8R+NFVzYy/sdj0Ypg9htaPXQq5Cqeo= cel.dev/expr v0.25.2 h1:K6j46C81hXtZQfuX60cVWQFBJahKSE2gfRbNuvr5bFs= cel.dev/expr v0.25.2/go.mod h1:hrXvqGP6G6gyx8UAHSHJ5RGk//1Oj5nXQ2NI02Nrsg4= -dario.cat/mergo v1.0.2 h1:85+piFYR1tMbRrLcDwR18y4UKJ3aH1Tbzi24VRW1TK8= -dario.cat/mergo v1.0.2/go.mod h1:E/hbnu0NxMFBjpMIE34DRGLWqDy0g5FuKDhCb31ngxA= filippo.io/age v1.3.1 h1:hbzdQOJkuaMEpRCLSN1/C5DX74RPcNCk6oqhKMXmZi0= filippo.io/age v1.3.1/go.mod h1:EZorDTYUxt836i3zdori5IJX/v2Lj6kWFU0cfh6C0D4= filippo.io/hpke v0.4.0 h1:p575VVQ6ted4pL+it6M00V/f2qTZITO0zgmdKCkd5+A= filippo.io/hpke v0.4.0/go.mod h1:EmAN849/P3qdeK+PCMkDpDm83vRHM5cDipBJ8xbQLVY= github.com/Masterminds/semver/v3 v3.5.0 h1:kQceYJfbupGfZOKZQg0kou0DgAKhzDg2NZPAwZ/2OOE= github.com/Masterminds/semver/v3 v3.5.0/go.mod h1:4V+yj/TJE1HU9XfppCwVMZq3I84lprf4nC11bSS5beM= -github.com/Microsoft/go-winio v0.5.2/go.mod h1:WpS1mjBmmwHBEWmogvA2mj8546UReBk4v8QkMxJ6pZY= github.com/Microsoft/go-winio v0.6.2 h1:F2VQgta7ecxGYO8k3ZZz3RS8fVIXVxONVUPlNERoyfY= github.com/Microsoft/go-winio v0.6.2/go.mod h1:yd8OoFMLzJbo9gZq8j5qaps8bJ9aShtEA8Ipt1oGCvU= github.com/ProtonMail/go-crypto v1.4.1 h1:9RfcZHqEQUvP8RzecWEUafnZVtEvrBVL9BiF67IQOfM= @@ -38,14 +35,10 @@ github.com/cespare/xxhash/v2 v2.3.0/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XL github.com/cloudflare/circl v1.6.4 h1:pOXuDTCEYyzydgUpQ0CQz3LsinKjiSk6nNP5Lt5K64U= github.com/cloudflare/circl v1.6.4/go.mod h1:YxarevkLlbaHuWsxG6vmYNWBEsSp4pnp7j+4VljMavY= github.com/cpuguy83/go-md2man/v2 v2.0.6/go.mod h1:oOW0eioCTA6cOiMLiUPZOpcVxMig6NIQQ7OS05n1F4g= -github.com/cyphar/filepath-securejoin v0.7.0 h1:s0Y3ITPy6sQn5xt54DuYvTF8hu134ooYLUb58DX/HjE= -github.com/cyphar/filepath-securejoin v0.7.0/go.mod h1:ymLGms/u3BYaviIiuKFnUx8EkQEZeK6cInNoAPJA3o4= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc h1:U9qPSI2PIWSS1VwoXQT9A3Wy9MM3WgvqSxFWenqJduM= github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= -github.com/elazarl/goproxy v1.7.2 h1:Y2o6urb7Eule09PjlhQRGNsqRfPmYI3KKQLFpCAV3+o= -github.com/elazarl/goproxy v1.7.2/go.mod h1:82vkLNir0ALaW14Rc399OTTjyNREgmdL2cVoIbS6XaE= github.com/emicklei/go-restful/v3 v3.13.0 h1:C4Bl2xDndpU6nJ4bc1jXd+uTmYPVUwkD6bFY/oTyCes= github.com/emicklei/go-restful/v3 v3.13.0/go.mod h1:6n3XBCmQQb25CM2LCACGz8ukIrRry+4bhvbpWn3mrbc= github.com/emirpasic/gods v1.18.1 h1:FXtiHYKDGKCW2KzwZKx0iC0PQmdlorYgdFG9jPXJ1Bc= @@ -72,14 +65,14 @@ github.com/gliderlabs/ssh v0.3.8 h1:a4YXD1V7xMF9g5nTkdfnja3Sxy1PVDCj1Zg4Wb8vY6c= github.com/gliderlabs/ssh v0.3.8/go.mod h1:xYoytBv1sV0aL3CavoDuJIQNURXkkfPA/wxQ1pL1fAU= github.com/go-errors/errors v1.5.1 h1:ZwEMSLRCapFLflTpT7NKaAc7ukJ8ZPEjzlxt8rPN8bk= github.com/go-errors/errors v1.5.1/go.mod h1:sIVyrIiJhuEF+Pj9Ebtd6P/rEYROXFi3BopGUQ5a5Og= -github.com/go-git/gcfg v1.5.1-0.20230307220236-3a3c6141e376 h1:+zs/tPmkDkHx3U66DAb0lQFJrpS6731Oaa12ikc+DiI= -github.com/go-git/gcfg v1.5.1-0.20230307220236-3a3c6141e376/go.mod h1:an3vInlBmSxCcxctByoQdvwPiA7DTK7jaaFDBTtu0ic= -github.com/go-git/go-billy/v5 v5.9.1 h1:8U73XiOTfINdItHVa6z4Gv7ToObcZ6grkqQbLryLCdA= -github.com/go-git/go-billy/v5 v5.9.1/go.mod h1:ExsU+jcGwXTBOnyilvAnEM1wug1IxHr4yP2ZXsNRtV0= -github.com/go-git/go-git-fixtures/v4 v4.3.2-0.20231010084843-55a94097c399 h1:eMje31YglSBqCdIqdhKBW8lokaMrL3uTkpGYlE2OOT4= -github.com/go-git/go-git-fixtures/v4 v4.3.2-0.20231010084843-55a94097c399/go.mod h1:1OCfN199q1Jm3HZlxleg+Dw/mwps2Wbk9frAWm+4FII= -github.com/go-git/go-git/v5 v5.19.1 h1:nX27AnaU43/K5bKktKwgBmR9lawoYVe1Ckg0rgzzN00= -github.com/go-git/go-git/v5 v5.19.1/go.mod h1:Pb1v0c7/g8aGQJwx9Us09W85yGoyvSwuhEGMH7zjDKQ= +github.com/go-git/gcfg/v2 v2.0.2 h1:MY5SIIfTGGEMhdA7d7JePuVVxtKL7Hp+ApGDJAJ7dpo= +github.com/go-git/gcfg/v2 v2.0.2/go.mod h1:/lv2NsxvhepuMrldsFilrgct6pxzpGdSRC13ydTLSLs= +github.com/go-git/go-billy/v6 v6.0.0-alpha.2 h1:1Sv5WemXL8CxKrAx1gioJ+uHNb2bZJhiQLfwSZ4Et8c= +github.com/go-git/go-billy/v6 v6.0.0-alpha.2/go.mod h1:r/bsv9i/iDyyEU8/Z6mjC+YraOVwie1ddfUqBCElKXQ= +github.com/go-git/go-git-fixtures/v6 v6.0.0-alpha.1 h1:gmqi2jvsreu0s8JMLylYDFq4sbjHwwlhktMw0DUg3mA= +github.com/go-git/go-git-fixtures/v6 v6.0.0-alpha.1/go.mod h1:ECf1MqJlBdYpKggBrOXjo/0EnvRZx6D++I86UYjPgAQ= +github.com/go-git/go-git/v6 v6.0.0-alpha.5 h1:sE+OlkHgYWNMVmN1s9sR7uyFgsWLtxcNWse/vBYKxRE= +github.com/go-git/go-git/v6 v6.0.0-alpha.5/go.mod h1:3IjhiZnM+uBmUrOGSeqrJpsmi4Vd0H2NZO/uK2a7d0s= github.com/go-logr/logr v1.2.2/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A= github.com/go-logr/logr v1.4.4 h1:tG4xh9yMsRCAiodLVTxyrkzSZ9+o0L1Kg/+cPVcbP/8= github.com/go-logr/logr v1.4.4/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY= @@ -127,8 +120,6 @@ github.com/goccy/go-yaml v1.18.0 h1:8W7wMFS12Pcas7KU+VVkaiCng+kG8QiFeFwzFb+rwuw= github.com/goccy/go-yaml v1.18.0/go.mod h1:XBurs7gK8ATbW4ZPGKgcbrY1Br56PdM69F7LkFRi1kA= github.com/golang-jwt/jwt/v5 v5.3.1 h1:kYf81DTWFe7t+1VvL7eS+jKFVWaUnK9cB1qbwn63YCY= github.com/golang-jwt/jwt/v5 v5.3.1/go.mod h1:fxCRLWMO43lRc8nhHWY6LGqRcf+1gQWArsqaEUEa5bE= -github.com/golang/groupcache v0.0.0-20241129210726-2c02b8208cf8 h1:f+oWsMOmNPc8JmEHVZIycC7hBoQxHH9pNKQORJNozsQ= -github.com/golang/groupcache v0.0.0-20241129210726-2c02b8208cf8/go.mod h1:wcDNUvekVysuuOpQKo3191zZyTpiI6se1N1ULghS0sw= github.com/golang/protobuf v1.5.4 h1:i7eJL8qZTpSEXOPTxNKhASYpMn+8e5Q6AdndVa1dWek= github.com/golang/protobuf v1.5.4/go.mod h1:lnTiLA8Wa4RWRcIUkrtSVa5nRhsEGBg48fD6rSs7xps= github.com/google/cel-go v0.30.0 h1:ll54AkzKunWkBn9wSoiUXbFZXYZTkdJGNXTBXUoolGo= @@ -148,8 +139,6 @@ github.com/grpc-ecosystem/grpc-gateway/v2 v2.29.0 h1:5VipnvEpbqr2gA2VbM+nYVbkIF2 github.com/grpc-ecosystem/grpc-gateway/v2 v2.29.0/go.mod h1:Hyl3n6Twe1hvtd9XUXDec4pTvgMSEixRuQKPTMH2bNs= github.com/inconshreveable/mousetrap v1.1.0 h1:wN+x4NVGpMsO7ErUn/mUI3vEoE6Jt13X2s0bqwp9tc8= github.com/inconshreveable/mousetrap v1.1.0/go.mod h1:vpF70FUmC8bwa3OWnCshd2FqLfsEA9PFc4w1p2J65bw= -github.com/jbenet/go-context v0.0.0-20150711004518-d14ea06fba99 h1:BQSFePA1RWJOlocH6Fxy8MmwDt+yVQYULKfN0RoTN8A= -github.com/jbenet/go-context v0.0.0-20150711004518-d14ea06fba99/go.mod h1:1lJo3i6rXxKeerYnT8Nvf0QmHCRC1n8sfWVwXF2Frvo= github.com/joshdk/go-junit v1.0.0 h1:S86cUKIdwBHWwA6xCmFlf3RTLfVXYQfvanM5Uh+K6GE= github.com/joshdk/go-junit v1.0.0/go.mod h1:TiiV0PqkaNfFXjEiyjWM3XXrhVyCa1K4Zfga6W52ung= github.com/jpillora/backoff v1.0.0 h1:uvFg412JmmHBHw7iwprIxkPMI+sGQ4kzOWsMeHnm2EA= @@ -215,9 +204,6 @@ github.com/rogpeppe/go-internal v1.14.1/go.mod h1:MaRKkUm5W0goXpeCfT7UZI6fk/L7L7 github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM= github.com/sergi/go-diff v1.4.0 h1:n/SP9D5ad1fORl+llWyN+D6qoUETXNZARKjyY2/KVCw= github.com/sergi/go-diff v1.4.0/go.mod h1:A0bzQcvG0E7Rwjx0REVgAGH58e96+X0MeOfepqsbeW4= -github.com/sirupsen/logrus v1.7.0/go.mod h1:yWOB1SBYBC5VeMP7gHvWumXLIWorT60ONWic61uBYv0= -github.com/skeema/knownhosts v1.3.2 h1:EDL9mgf4NzwMXCTfaxSD/o/a5fxDw/xL9nkU28JjdBg= -github.com/skeema/knownhosts v1.3.2/go.mod h1:bEg3iQAuw+jyiw+484wwFJoKSLwcfd7fqRy+N0QTiow= github.com/spf13/cobra v1.10.2 h1:DMTTonx5m65Ic0GOoRY2c16WCbHxOOw6xxezuLaBpcU= github.com/spf13/cobra v1.10.2/go.mod h1:7C1pvHqHw5A4vrJfjNwvOdzYu0Gml16OCs2GRiTUUS4= github.com/spf13/pflag v1.0.9/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg= @@ -226,7 +212,6 @@ github.com/spf13/pflag v1.0.10/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3A github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/objx v0.5.3 h1:jmXUvGomnU1o3W/V5h2VEradbpJDwGrzugQQvL0POH4= github.com/stretchr/objx v0.5.3/go.mod h1:rDQraq+vQZU7Fde9LOZLr8Tax6zZvy4kuNKF+QYS+U0= -github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs= github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI= github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4= github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= @@ -242,8 +227,6 @@ github.com/tidwall/sjson v1.2.5 h1:kLy8mja+1c9jlljvWTlSazM7cKDRfJuR/bOJhcY5NcY= github.com/tidwall/sjson v1.2.5/go.mod h1:Fvgq9kS/6ociJEDnK0Fk1cpYF4FIW6ZF7LAe+6jwd28= github.com/x448/float16 v0.8.4 h1:qLwI1I70+NjRFUR3zs1JPUCgaCXSh3SW62uAKT1mSBM= github.com/x448/float16 v0.8.4/go.mod h1:14CWIYCyZA/cWjXOioeEpHeN/83MdbZDRQHoFcYsOfg= -github.com/xanzy/ssh-agent v0.3.3 h1:+/15pJfg/RsTxqYcX6fHqOXZwwMP+2VyYWJeWM2qQFM= -github.com/xanzy/ssh-agent v0.3.3/go.mod h1:6dzNDKs0J9rVPHPhaGCukekBHKqfl+L3KghI1Bc68Uw= github.com/xlab/treeprint v1.2.0 h1:HzHnuAF1plUN2zGlAFHbSQP2qJ0ZAD3XF5XD7OesXRQ= github.com/xlab/treeprint v1.2.0/go.mod h1:gj5Gd3gPdKtR1ikdDK6fnFLdmIS0X30kTTuNd/WEJu0= github.com/yuin/gopher-lua v1.1.2 h1:yF/FjE3hD65tBbt0VXLE13HWS9h34fdzJmrWRXwobGA= @@ -287,37 +270,26 @@ go.yaml.in/yaml/v2 v2.4.4/go.mod h1:gMZqIpDtDqOfM0uNfy0SkpRhvUryYH0Z6wdMYcacYXQ= go.yaml.in/yaml/v3 v3.0.4/go.mod h1:DhzuOOF2ATzADvBadXxruRBLzYTpT36CKvDb3+aBEFg= go.yaml.in/yaml/v3 v3.0.5 h1:N6y/pJk8buWs9NY5ERU2HSMfm+IuD/OtfdAnq6kESPw= go.yaml.in/yaml/v3 v3.0.5/go.mod h1:HVTZu1O7/Vkt2N+BFy8Zza+lnLsABggaTM2ZpNIGuKg= -golang.org/x/crypto v0.0.0-20220622213112-05595931fe9d/go.mod h1:IxCIyHEi3zRg3s0A5j5BB6A9Jmi73HwBIUl50j+osU4= golang.org/x/crypto v0.54.0 h1:YLIA59K4fiNzHzjnZt2tUJQjQtUWfWbeHBqKtk3eScw= golang.org/x/crypto v0.54.0/go.mod h1:KWL8ny2AZdGR2cWmzeHrp2azQPGogOv+HeQaVEXC2dk= golang.org/x/exp v0.0.0-20260718201538-764159d718ef h1:LkZ48HFgy/TvhTI0bcWkjgFkgLyKUwcTbDjS0DUjw+A= golang.org/x/exp v0.0.0-20260718201538-764159d718ef/go.mod h1:EdfpwwqSu+0Li0mzskwHU6FWDV3t9Q+RZDo3QMUtL3Q= golang.org/x/mod v0.38.0 h1:MECBjubtXD7yj4HrhIUcywNaGeNVUdfVnxmPajOk4yk= golang.org/x/mod v0.38.0/go.mod h1:V6Xz0pq8TQ3dGqVQ1FVHuelZpAL0uNhSkk9ogYP3c40= -golang.org/x/net v0.0.0-20211112202133-69e39bad7dc2/go.mod h1:9nx3DQGgdP8bBQD5qxJ1jj9UTztislL4KSBs9R2vV5Y= golang.org/x/net v0.57.0 h1:K5+3DljvIuDG9/Jv9rvyMywYNFCQ9RSUY6OOTTkT+tE= golang.org/x/net v0.57.0/go.mod h1:KpXc8iv+r3XplLAG/f7Jsf9RPszJzdR0f58q9vGOuEU= golang.org/x/oauth2 v0.36.0 h1:peZ/1z27fi9hUOFCAZaHyrpWG5lwe0RJEEEeH0ThlIs= golang.org/x/oauth2 v0.36.0/go.mod h1:YDBUJMTkDnJS+A4BP4eZBjCqtokkg1hODuPjwiGPO7Q= golang.org/x/sync v0.22.0 h1:SZjpbeLmrCk4xhRSZFNZW5gFUeCeFgjekvI/+gfScek= golang.org/x/sync v0.22.0/go.mod h1:9xrNwdLfx4jkKbNva9FpL6vEN7evnE43NNNJQ2LF3+0= -golang.org/x/sys v0.0.0-20191026070338-33540a1f6037/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= -golang.org/x/sys v0.0.0-20201119102817-f84b799fce68/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= -golang.org/x/sys v0.0.0-20210124154548-22da62e12c0c/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= -golang.org/x/sys v0.0.0-20210423082822-04245dca01da/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= -golang.org/x/sys v0.0.0-20210615035016-665e8c7367d1/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.0.0-20220715151400-c0bba94af5f8/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.47.0 h1:o7XGOvZQCADBQQ4Y7VNq2dRWQR7JmOUW8Kxx4ZsNgWs= golang.org/x/sys v0.47.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= -golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo= golang.org/x/term v0.45.0 h1:NwWyBmoJCbfTHpxrWoZ9C6/VxOf7ic219I8xZZFdrf0= golang.org/x/term v0.45.0/go.mod h1:9aqxs0blBcrm/n0L9QW0aRVD+ktan8ssZromtqJC43w= -golang.org/x/text v0.3.6/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= golang.org/x/text v0.40.0 h1:Ub2Z6/xjgF1WrYQz2nuITOEegKFtiIy+rieRJ5lHZKs= golang.org/x/text v0.40.0/go.mod h1:hpnzDAfGV753zIKo+wk3u1bVKCGPbrnF7+7LBF/UHVY= golang.org/x/time v0.15.0 h1:bbrp8t3bGUeFOx08pvsMYRTCVSMk89u4tKbNOZbp88U= golang.org/x/time v0.15.0/go.mod h1:Y4YMaQmXwGQZoFaVFk4YpCt4FLQMYKZe9oeV/f4MSno= -golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= golang.org/x/tools v0.48.0 h1:3+hClM1aLL5mjMKm5ovokw9epgRXPuu2tILgismM6RE= golang.org/x/tools v0.48.0/go.mod h1:08xX0orndb/F7jJxGDicx061tyd5pcMto75YMAXr6lk= gomodules.xyz/jsonpatch/v2 v2.5.0 h1:JELs8RLM12qJGXU4u/TO3V25KW8GreMKl9pdkk14RM0= @@ -340,8 +312,6 @@ gopkg.in/evanphx/json-patch.v4 v4.13.0 h1:czT3CmqEaQ1aanPc5SdlgQrrEIb8w/wwCvWWnf gopkg.in/evanphx/json-patch.v4 v4.13.0/go.mod h1:p8EYWUEYMpynmqDbY58zCKCFZw8pRWMG4EsWvDvM72M= gopkg.in/inf.v0 v0.9.1 h1:73M5CoZyi3ZLMOyDlQh031Cx6N9NDJ2Vvfl76EDAgDc= gopkg.in/inf.v0 v0.9.1/go.mod h1:cWUDdTG/fYaXco+Dcufb5Vnc6Gp2YChqWtbxRZE0mXw= -gopkg.in/warnings.v0 v0.1.2 h1:wFXVbFY8DY5/xOe1ECiWdKCzZlxgshcYVNkBHstARME= -gopkg.in/warnings.v0 v0.1.2/go.mod h1:jksf8JmL6Qr/oQM2OXTHunEvvTAsrWBLb6OOjuVWRNI= gopkg.in/yaml.v2 v2.2.2/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= gopkg.in/yaml.v2 v2.4.0/go.mod h1:RDklbk79AGWmwhnvt/jBztapEOGDOx6ZbXqjP6csGnQ= gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/internal/controller/gitprovider_controller.go b/internal/controller/gitprovider_controller.go index 792cb83d..99efae3d 100644 --- a/internal/controller/gitprovider_controller.go +++ b/internal/controller/gitprovider_controller.go @@ -9,6 +9,7 @@ import ( "strings" "sync" + gitclient "github.com/go-git/go-git/v6/plumbing/client" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" @@ -19,7 +20,6 @@ import ( logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/predicate" - "github.com/go-git/go-git/v5/plumbing/transport" "github.com/go-logr/logr" configbutleraiv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" @@ -243,13 +243,28 @@ func (r *GitProviderReconciler) extractCredentials( ctx context.Context, gitProvider *configbutleraiv1alpha3.GitProvider, secret *corev1.Secret, -) (transport.AuthMethod, error) { - return gitpkg.AuthFromSecretData(ctx, r.Client, gitProvider, secret, r.SSHHostKeys) +) ([]gitclient.Option, error) { + cred, err := r.extractCredential(ctx, gitProvider, secret) + if err != nil { + return nil, err + } + return cred.Options(), nil +} + +// extractCredential is extractCredentials before the options wrapper. go-git v6 credentials are +// opaque closures, so this returns the concrete credential for callers — and tests — that need to +// see which kind was produced and how it was configured. +func (r *GitProviderReconciler) extractCredential( + ctx context.Context, + gitProvider *configbutleraiv1alpha3.GitProvider, + secret *corev1.Secret, +) (gitpkg.Credential, error) { + return gitpkg.CredentialFromSecretData(ctx, r.Client, gitProvider, secret, r.SSHHostKeys) } // checkRemoteConnectivity performs a lightweight check of repository connectivity and returns branch count. func (r *GitProviderReconciler) checkRemoteConnectivity( - ctx context.Context, repoURL string, auth transport.AuthMethod, + ctx context.Context, repoURL string, auth []gitclient.Option, ) (int, error) { log := logf.FromContext(ctx).WithName("checkRemoteConnectivity") diff --git a/internal/controller/gitprovider_controller_test.go b/internal/controller/gitprovider_controller_test.go index f1e6f041..64fec0f3 100644 --- a/internal/controller/gitprovider_controller_test.go +++ b/internal/controller/gitprovider_controller_test.go @@ -17,9 +17,6 @@ import ( "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/reconcile" - "github.com/go-git/go-git/v5/plumbing/transport/http" - "github.com/go-git/go-git/v5/plumbing/transport/ssh" - configbutleraiv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" gitpkg "github.com/ConfigButler/gitops-reverser/internal/git" ) @@ -49,16 +46,14 @@ var _ = Describe("GitProvider Controller", func() { }, } - auth, err := reconciler.extractCredentials( + auth, err := reconciler.extractCredential( context.Background(), &configbutleraiv1alpha3.GitProvider{}, secret, ) Expect(err).NotTo(HaveOccurred()) - Expect(auth).To(BeAssignableToTypeOf(&ssh.PublicKeys{})) + Expect(auth.SSH).NotTo(BeNil()) - sshAuth := auth.(*ssh.PublicKeys) - Expect(sshAuth.User).To(Equal("git")) }) It("should extract SSH credentials with passphrase", func() { @@ -73,13 +68,13 @@ var _ = Describe("GitProvider Controller", func() { }, } - auth, err := reconciler.extractCredentials( + auth, err := reconciler.extractCredential( context.Background(), &configbutleraiv1alpha3.GitProvider{}, secret, ) Expect(err).NotTo(HaveOccurred()) - Expect(auth).To(BeAssignableToTypeOf(&ssh.PublicKeys{})) + Expect(auth.SSH).NotTo(BeNil()) }) It("should fail with invalid SSH key", func() { @@ -89,7 +84,7 @@ var _ = Describe("GitProvider Controller", func() { }, } - _, err := reconciler.extractCredentials( + _, err := reconciler.extractCredential( context.Background(), &configbutleraiv1alpha3.GitProvider{}, secret, @@ -109,13 +104,13 @@ var _ = Describe("GitProvider Controller", func() { }, } - auth, err := reconciler.extractCredentials( + auth, err := reconciler.extractCredential( context.Background(), &configbutleraiv1alpha3.GitProvider{}, secret, ) Expect(err).NotTo(HaveOccurred()) - Expect(auth).To(BeAssignableToTypeOf(&ssh.PublicKeys{})) + Expect(auth.SSH).NotTo(BeNil()) }) }) @@ -128,17 +123,14 @@ var _ = Describe("GitProvider Controller", func() { }, } - auth, err := reconciler.extractCredentials( + auth, err := reconciler.extractCredential( context.Background(), &configbutleraiv1alpha3.GitProvider{}, secret, ) Expect(err).NotTo(HaveOccurred()) - Expect(auth).To(BeAssignableToTypeOf(&http.BasicAuth{})) + Expect(auth.Basic).NotTo(BeNil()) - httpAuth := auth.(*http.BasicAuth) - Expect(httpAuth.Username).To(Equal("testuser")) - Expect(httpAuth.Password).To(Equal("testpass")) }) It("should fail with username but no password", func() { @@ -148,7 +140,7 @@ var _ = Describe("GitProvider Controller", func() { }, } - _, err := reconciler.extractCredentials( + _, err := reconciler.extractCredential( context.Background(), &configbutleraiv1alpha3.GitProvider{}, secret, @@ -164,7 +156,7 @@ var _ = Describe("GitProvider Controller", func() { Data: map[string][]byte{}, } - _, err := reconciler.extractCredentials( + _, err := reconciler.extractCredential( context.Background(), &configbutleraiv1alpha3.GitProvider{}, secret, @@ -182,7 +174,7 @@ var _ = Describe("GitProvider Controller", func() { }, } - _, err := reconciler.extractCredentials( + _, err := reconciler.extractCredential( context.Background(), &configbutleraiv1alpha3.GitProvider{}, secret, diff --git a/internal/controller/ssh_test.go b/internal/controller/ssh_test.go index 301611ce..ba1c1270 100644 --- a/internal/controller/ssh_test.go +++ b/internal/controller/ssh_test.go @@ -11,7 +11,9 @@ import ( "fmt" "testing" - "github.com/go-git/go-git/v5/plumbing/transport/ssh" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + gossh "golang.org/x/crypto/ssh" "golang.org/x/crypto/ssh/knownhosts" corev1 "k8s.io/api/core/v1" @@ -81,20 +83,18 @@ var _ = Describe("SSH Authentication", func() { Describe("extractCredentials", func() { Context("with valid SSH secret", func() { It("should successfully create SSH authentication", func() { - auth, err := reconciler.extractCredentials( + auth, err := reconciler.extractCredential( context.Background(), &configbutleraiv1alpha3.GitProvider{}, validSSHSecret, ) Expect(err).NotTo(HaveOccurred()) - Expect(auth).NotTo(BeNil()) - Expect(auth).To(BeAssignableToTypeOf(&ssh.PublicKeys{})) + Expect(auth.SSH).NotTo(BeNil()) - sshAuth := auth.(*ssh.PublicKeys) - Expect(sshAuth.User).To(Equal("git")) - Expect(sshAuth.Signer).NotTo(BeNil()) - Expect(sshAuth.HostKeyCallback).NotTo(BeNil()) + Expect(auth.SSH.User).To(Equal("git")) + Expect(auth.SSH.Signer).NotTo(BeNil()) + Expect(auth.SSH.HostKeyCallback).NotTo(BeNil()) }) }) @@ -110,7 +110,7 @@ var _ = Describe("SSH Authentication", func() { }, } - auth, err := reconciler.extractCredentials( + auth, err := reconciler.extractCredential( context.Background(), &configbutleraiv1alpha3.GitProvider{}, secretWithoutKnownHosts, @@ -118,7 +118,7 @@ var _ = Describe("SSH Authentication", func() { Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("known_hosts is required")) - Expect(auth).To(BeNil()) + Expect(auth).To(Equal(gitpkg.Credential{}), "a failed credential resolution must yield no credential") }) }) @@ -135,18 +135,16 @@ var _ = Describe("SSH Authentication", func() { } reconciler.SSHHostKeys.AllowMissingKnownHosts = true - auth, err := reconciler.extractCredentials( + auth, err := reconciler.extractCredential( context.Background(), &configbutleraiv1alpha3.GitProvider{}, secret, ) Expect(err).NotTo(HaveOccurred()) - Expect(auth).NotTo(BeNil()) - Expect(auth).To(BeAssignableToTypeOf(&ssh.PublicKeys{})) + Expect(auth.SSH).NotTo(BeNil()) - sshAuth := auth.(*ssh.PublicKeys) - Expect(sshAuth.HostKeyCallback).NotTo(BeNil()) + Expect(auth.SSH.HostKeyCallback).NotTo(BeNil()) }) }) @@ -160,7 +158,7 @@ var _ = Describe("SSH Authentication", func() { Context("with invalid SSH secret", func() { It("should return error for malformed private key", func() { - auth, err := reconciler.extractCredentials( + auth, err := reconciler.extractCredential( context.Background(), &configbutleraiv1alpha3.GitProvider{}, invalidSSHSecret, @@ -168,7 +166,7 @@ var _ = Describe("SSH Authentication", func() { Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("failed to create SSH public keys")) - Expect(auth).To(BeNil()) + Expect(auth).To(Equal(gitpkg.Credential{}), "a failed credential resolution must yield no credential") }) }) @@ -185,27 +183,29 @@ var _ = Describe("SSH Authentication", func() { }, } - auth, err := reconciler.extractCredentials( + auth, err := reconciler.extractCredential( context.Background(), &configbutleraiv1alpha3.GitProvider{}, httpSecret, ) Expect(err).NotTo(HaveOccurred()) - Expect(auth).NotTo(BeNil()) + Expect(auth.Basic).NotTo(BeNil()) + Expect(auth.Basic.Username).To(Equal("testuser")) + Expect(auth.Basic.Password).To(Equal("testpass")) }) }) Context("with empty secret", func() { It("should return nil auth for anonymous access", func() { - auth, err := reconciler.extractCredentials( + auth, err := reconciler.extractCredential( context.Background(), &configbutleraiv1alpha3.GitProvider{}, nil, ) Expect(err).NotTo(HaveOccurred()) - Expect(auth).To(BeNil()) + Expect(auth).To(Equal(gitpkg.Credential{}), "a failed credential resolution must yield no credential") }) }) @@ -222,7 +222,7 @@ var _ = Describe("SSH Authentication", func() { }, } - auth, err := reconciler.extractCredentials( + auth, err := reconciler.extractCredential( context.Background(), &configbutleraiv1alpha3.GitProvider{}, incompleteSecret, @@ -230,7 +230,7 @@ var _ = Describe("SSH Authentication", func() { Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("contains username but no password")) - Expect(auth).To(BeNil()) + Expect(auth).To(Equal(gitpkg.Credential{}), "a failed credential resolution must yield no credential") }) }) @@ -244,7 +244,7 @@ var _ = Describe("SSH Authentication", func() { Data: map[string][]byte{}, } - auth, err := reconciler.extractCredentials( + auth, err := reconciler.extractCredential( context.Background(), &configbutleraiv1alpha3.GitProvider{}, emptySecret, @@ -254,7 +254,7 @@ var _ = Describe("SSH Authentication", func() { Expect( err.Error(), ).To(ContainSubstring("does not contain valid authentication data")) - Expect(auth).To(BeNil()) + Expect(auth).To(Equal(gitpkg.Credential{}), "a failed credential resolution must yield no credential") }) }) }) @@ -318,15 +318,13 @@ func TestSSHCredentials(t *testing.T) { }, } - auth, err := reconciler.extractCredentials(context.Background(), &configbutleraiv1alpha3.GitProvider{}, secret) + auth, err := reconciler.extractCredential(context.Background(), &configbutleraiv1alpha3.GitProvider{}, secret) if err != nil { t.Errorf("Expected no error, got: %v", err) } - if auth == nil { - t.Error("Expected auth object, got nil") - } - if _, ok := auth.(*ssh.PublicKeys); !ok { - t.Errorf("Expected *ssh.PublicKeys, got %T", auth) + + if auth.SSH == nil { + t.Error("Expected an SSH credential") } }) @@ -338,16 +336,21 @@ func TestSSHCredentials(t *testing.T) { }, } - auth, err := reconciler.extractCredentials(context.Background(), &configbutleraiv1alpha3.GitProvider{}, secret) + auth, err := reconciler.extractCredential(context.Background(), &configbutleraiv1alpha3.GitProvider{}, secret) if err == nil { t.Error("Expected error for invalid SSH key") } - if auth != nil { - t.Error("Expected nil auth for invalid key") + if auth.SSH != nil { + t.Error("Expected no SSH credential for an invalid key") } }) +} + +// TestCredentials_HTTPAndAnonymous covers the non-SSH credential shapes. Split out of +// TestSSHCredentials to keep that function under the cognitive-complexity gate. +func TestCredentials_HTTPAndAnonymous(t *testing.T) { + reconciler := &GitProviderReconciler{} - // Test with HTTP credentials t.Run("HTTP Credentials", func(t *testing.T) { secret := &corev1.Secret{ Data: map[string][]byte{ @@ -356,24 +359,19 @@ func TestSSHCredentials(t *testing.T) { }, } - auth, err := reconciler.extractCredentials(context.Background(), &configbutleraiv1alpha3.GitProvider{}, secret) - if err != nil { - t.Errorf("Expected no error, got: %v", err) - } - if auth == nil { - t.Error("Expected auth object, got nil") - } + auth, err := reconciler.extractCredential(context.Background(), &configbutleraiv1alpha3.GitProvider{}, secret) + require.NoError(t, err) + require.NotNil(t, auth.Basic, "expected HTTP basic credentials") + assert.Equal(t, "testuser", auth.Basic.Username) + assert.Equal(t, "testpass", auth.Basic.Password) + assert.Len(t, auth.Options(), 1, "a basic credential renders as one transport option") }) - // Test with nil secret (anonymous access) t.Run("Anonymous Access", func(t *testing.T) { - auth, err := reconciler.extractCredentials(context.Background(), &configbutleraiv1alpha3.GitProvider{}, nil) - if err != nil { - t.Errorf("Expected no error, got: %v", err) - } - if auth != nil { - t.Error("Expected nil auth for anonymous access") - } + auth, err := reconciler.extractCredential(context.Background(), &configbutleraiv1alpha3.GitProvider{}, nil) + require.NoError(t, err) + assert.Equal(t, gitpkg.Credential{}, auth, "no secret means anonymous") + assert.Nil(t, auth.Options(), "anonymous renders no transport options") }) } diff --git a/internal/git/acceptance_gate_test.go b/internal/git/acceptance_gate_test.go index 3ed6911e..8a266262 100644 --- a/internal/git/acceptance_gate_test.go +++ b/internal/git/acceptance_gate_test.go @@ -8,7 +8,7 @@ import ( "path/filepath" "testing" - gogit "github.com/go-git/go-git/v5" + gogit "github.com/go-git/go-git/v6" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -37,7 +37,7 @@ const cmYAML = "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: seeded\n na func TestPlanFlush_RefusesUnsupportedKustomizeFolder(t *testing.T) { writer := newContentWriter(types.SensitiveResourcePolicy{}) worktree := newWorktreeForTest(t) - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() seedPlacedManifest(t, worktree, "kustomization.yaml", hardKustomizeYAML) @@ -91,7 +91,7 @@ func TestPlanFlush_DoesNotRefuseOwnSopsConfig(t *testing.T) { func TestResyncRefusal_DoesNotStageSOPSBootstrap(t *testing.T) { writer := newContentWriter(types.SensitiveResourcePolicy{}) worktree := newWorktreeForTest(t) - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() repo, err := gogit.PlainOpen(root) require.NoError(t, err) diff --git a/internal/git/ado_live_test.go b/internal/git/ado_live_test.go new file mode 100644 index 00000000..9a902bb7 --- /dev/null +++ b/internal/git/ado_live_test.go @@ -0,0 +1,412 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +// An opt-in test against a real Azure DevOps repository. +// +// The simulator in ado_multiack_test.go reproduces ADO's multi_ack rule faithfully enough to gate CI, +// but it is still a local stand-in: it asserts what we believe ADO does. This one asserts what ADO +// actually does, and it is the only place that belief gets checked. It is skipped unless the +// environment supplies a repository, so it never runs in CI. +// +// export E2E_ADO_REPO_URL='https://dev.azure.com///_git/' +// export E2E_ADO_PAT='' # scope: Code (read & write) +// go test ./internal/git/ -run TestADOLive -v +// +// E2E_ADO_EMPTY_REPO_URL optionally names a second, commit-less repository, which is the only way to +// cover the empty-repository contract repeatedly: the test below seeds whatever repository it is +// given, so it only ever sees "empty" once. +// +// E2E_ADO_USERNAME is optional; ADO ignores the username when a PAT is the password, and the default +// (empty) is the form ADO documents. +// +// It writes freely to the repository, including the default branch, and does not clean up. Point it at +// a scratch repository. +// +// Beyond "does ADO work", the substance here is the branch-resolution order SmartFetch promises: +// prefer the target branch, always fetch the default branch as a safety net, and report nothing for an +// empty repository. That ordering is easy to get subtly wrong and cheap to assert, so +// TestADOLive_BranchResolutionOrder walks it against the real remote. + +import ( + "context" + "encoding/base64" + "fmt" + "io" + "net/http" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/go-git/go-git/v6" + "github.com/go-git/go-git/v6/config" + "github.com/go-git/go-git/v6/plumbing" + gitclient "github.com/go-git/go-git/v6/plumbing/client" + "github.com/go-git/go-git/v6/plumbing/object" + gogithttp "github.com/go-git/go-git/v6/plumbing/transport/http" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// adoLiveTarget is the repository and credential under test. +type adoLiveTarget struct { + URL string + Auth []gitclient.Option +} + +func adoLiveFromEnv(tb testing.TB) adoLiveTarget { + tb.Helper() + + url := os.Getenv("E2E_ADO_REPO_URL") + pat := os.Getenv("E2E_ADO_PAT") + if url == "" || pat == "" { + tb.Skip("set E2E_ADO_REPO_URL and E2E_ADO_PAT to run the live Azure DevOps test") + } + + // Empty username with the PAT as password is the form ADO documents. + cred := Credential{Basic: &gogithttp.BasicAuth{ + Username: os.Getenv("E2E_ADO_USERNAME"), + Password: pat, + }} + + return adoLiveTarget{URL: url, Auth: cred.Options()} +} + +// TestADOLive_CheckRepo is the cheapest real-ADO assertion: connectivity and metadata over the ref +// advertisement, which needs no multi_ack. If this passes while the fetch test fails, the capability +// matrix is right and only negotiation is affected. +// +// An empty repository is a pass. Reaching the advertisement is what is being checked, and a freshly +// created ADO repository has no commits. +func TestADOLive_CheckRepo(t *testing.T) { + target := adoLiveFromEnv(t) + + info, err := CheckRepo(context.Background(), target.URL, target.Auth) + require.NoError(t, err, "CheckRepo reads only the advertisement and must not need multi_ack") + + if info.DefaultBranch == nil { + t.Log("repository is empty: the advertisement was served, which is what this test checks") + assert.Zero(t, info.RemoteBranchCount) + return + } + + t.Logf("default branch %q sha=%s unborn=%t, %d remote branches", + info.DefaultBranch.ShortName, info.DefaultBranch.Sha, + info.DefaultBranch.Unborn, info.RemoteBranchCount) + assert.Positive(t, info.RemoteBranchCount) +} + +// TestADOLive_BranchResolutionOrder walks the branch-resolution contract against the real remote, in +// the order the code implements it, and is the test to read when that order changes. +// +// The fetch in phase 4 is also the one that matters for ADO: by then the local store has history, so +// the request carries have lines and a real negotiation happens. That is the exact request go-git v5 +// could not make — it fails with HTTP 400 "TF401041: Clients must support multi-ack" — and the whole +// reason for the v6 migration. +func TestADOLive_BranchResolutionOrder(t *testing.T) { + target := adoLiveFromEnv(t) + ctx := context.Background() + + // --- Phase 0: an empty repository reports no branch at all ----------------------------------- + info, err := CheckRepo(ctx, target.URL, target.Auth) + require.NoError(t, err) + + if info.DefaultBranch == nil { + t.Log("phase 0: repository is empty — CheckRepo reports no default branch") + assert.Zero(t, info.RemoteBranchCount) + + empty := filepath.Join(t.TempDir(), "empty-probe") + probe := adoLiveRepo(t, empty, target.URL) + resolved, ferr := SmartFetch(ctx, probe, plumbing.NewBranchReferenceName("main"), target.Auth) + require.NoError(t, ferr, "fetching an empty remote is a valid state, not an error") + assert.Empty(t, resolved, "an empty remote resolves to no branch") + + adoLiveSeed(t, target, "main") + } + + // --- Phase 1: the default branch is reported, with a hash, and is not unborn ------------------ + info, err = CheckRepo(ctx, target.URL, target.Auth) + require.NoError(t, err) + require.NotNil(t, info.DefaultBranch, "a seeded repository must report a default branch") + + defaultBranch := info.DefaultBranch.ShortName + t.Logf("phase 1: default branch is %q at %s (unborn=%t)", + defaultBranch, info.DefaultBranch.Sha, info.DefaultBranch.Unborn) + assert.False(t, info.DefaultBranch.Unborn, "a branch with commits must not be reported unborn") + assert.NotEmpty(t, info.DefaultBranch.Sha) + + // --- Phase 2: a target that does not exist falls back to the default ------------------------- + work := filepath.Join(t.TempDir(), "work") + repo, err := PrepareBranchLive(ctx, t, target, work, defaultBranch) + require.NoError(t, err) + + absent := plumbing.NewBranchReferenceName(liveBranchName("absent")) + resolved, err := SmartFetch(ctx, repo, absent, target.Auth) + require.NoError(t, err) + assert.Equal(t, plumbing.NewBranchReferenceName(defaultBranch), resolved, + "a target missing on the remote must fall back to the default branch") + t.Logf("phase 2: target %q is absent, resolved to %q", absent.Short(), resolved.Short()) + + requireRemoteTracking(t, repo, defaultBranch, "the default branch is always fetched as a safety net") + + // --- Phase 3: a target that does exist wins, and the default is still fetched ---------------- + feature := liveBranchName("feature") + rootHash := adoLivePushBranch(ctx, t, target, repo, defaultBranch, feature, "phase 3: create the target branch") + + resolved, err = SmartFetch(ctx, repo, plumbing.NewBranchReferenceName(feature), target.Auth) + require.NoError(t, err) + assert.Equal(t, plumbing.NewBranchReferenceName(feature), resolved, + "a target present on the remote must win over the default") + t.Logf("phase 3: target %q exists, resolved to %q", feature, resolved.Short()) + + requireRemoteTracking(t, repo, feature, "the target branch must be fetched") + requireRemoteTracking(t, repo, defaultBranch, "the default branch is fetched even when the target wins") + + // --- Phase 4: the negotiating fetch ----------------------------------------------------------- + // Move the remote on from a second clone, so the fetch below has something to retrieve and the + // local store already has history to advertise as haves. + second := filepath.Join(t.TempDir(), "second") + secondRepo, err := PrepareBranchLive(ctx, t, target, second, defaultBranch) + require.NoError(t, err) + advanced := adoLiveCommitAndPush(ctx, t, target, secondRepo, defaultBranch, rootHash, + "phase 4: advance the default branch") + + resolved, err = SmartFetch(ctx, repo, plumbing.NewBranchReferenceName(defaultBranch), target.Auth) + require.NoError(t, err, + "an incremental fetch against ADO must succeed; go-git v5 fails here with HTTP 400 TF401041") + assert.Equal(t, plumbing.NewBranchReferenceName(defaultBranch), resolved) + + ref, err := repo.Reference(plumbing.NewRemoteReferenceName("origin", defaultBranch), true) + require.NoError(t, err) + assert.Equal(t, advanced, ref.Hash(), + "the negotiating fetch must have retrieved the commit pushed from elsewhere") + t.Logf("phase 4: negotiated fetch advanced origin/%s to %s", defaultBranch, ref.Hash()) +} + +// adoCanaryTimeout bounds the canary's round trip against a remote nobody here controls. +const adoCanaryTimeout = 30 * time.Second + +// liveBranchName returns a branch name unlikely to collide with anything in the repository. +func liveBranchName(kind string) string { + return fmt.Sprintf("reverser-live-%s-%d", kind, os.Getpid()) +} + +// adoLiveRepo initialises an empty local repository wired to the remote. +func adoLiveRepo(tb testing.TB, path, url string) *git.Repository { + tb.Helper() + + repo, err := git.PlainInit(path, false) + require.NoError(tb, err) + require.NoError(tb, PinExplicitSigningPolicy(repo)) + _, err = repo.CreateRemote(&config.RemoteConfig{Name: "origin", URLs: []string{url}}) + require.NoError(tb, err) + + return repo +} + +// PrepareBranchLive clones through the production PrepareBranch path and reopens the result. +func PrepareBranchLive( + ctx context.Context, tb testing.TB, target adoLiveTarget, path, branch string, +) (*git.Repository, error) { + tb.Helper() + + if _, err := PrepareBranch(ctx, target.URL, path, branch, target.Auth); err != nil { + return nil, err + } + return git.PlainOpen(path) +} + +// adoLiveSeed gives an empty repository its first commit on branch. +func adoLiveSeed(tb testing.TB, target adoLiveTarget, branch string) { + tb.Helper() + ctx := context.Background() + + dir := filepath.Join(tb.TempDir(), "seed") + repo := adoLiveRepo(tb, dir, target.URL) + require.NoError(tb, setHead(repo, branch)) + + adoLiveWriteAndCommit(tb, repo, dir, "README.md", + "# scratch repository for the gitops-reverser live Azure DevOps test\n", + "test: seed the live Azure DevOps scratch repository") + + require.NoError(tb, + PushAtomic(ctx, repo, plumbing.ZeroHash, plumbing.NewBranchReferenceName(branch), target.Auth), + "creating the first branch on an empty ADO repository must succeed") + tb.Logf("seeded %q with a first commit", branch) +} + +// adoLivePushBranch branches off the default branch and pushes the new branch, returning the hash the +// branch was based on -- which is the root hash the push compared against. +func adoLivePushBranch( + ctx context.Context, tb testing.TB, target adoLiveTarget, + repo *git.Repository, defaultBranch, newBranch, message string, +) plumbing.Hash { + tb.Helper() + + rootHash, err := repo.Reference(plumbing.NewRemoteReferenceName("origin", defaultBranch), true) + require.NoError(tb, err) + + worktree, err := repo.Worktree() + require.NoError(tb, err) + require.NoError(tb, worktree.Checkout(&git.CheckoutOptions{ + Hash: rootHash.Hash(), + Branch: plumbing.NewBranchReferenceName(newBranch), + Create: true, + Force: true, + })) + + root, err := repo.Worktree() + require.NoError(tb, err) + adoLiveWriteAndCommit(tb, repo, root.Filesystem().Root(), + "reverser-live-test.yaml", fmt.Sprintf("# %s at %s\n", message, time.Now().UTC()), message) + + require.NoError(tb, + PushAtomic(ctx, repo, rootHash.Hash(), plumbing.NewBranchReferenceName(defaultBranch), target.Auth), + "receive-pack has no multi_ack, so the push must succeed") + tb.Logf("pushed branch %q", newBranch) + + return rootHash.Hash() +} + +// adoLiveCommitAndPush commits on the checked-out default branch and pushes it, returning the new hash. +func adoLiveCommitAndPush( + ctx context.Context, tb testing.TB, target adoLiveTarget, + repo *git.Repository, defaultBranch string, _ plumbing.Hash, message string, +) plumbing.Hash { + tb.Helper() + + rootRef, err := repo.Reference(plumbing.NewRemoteReferenceName("origin", defaultBranch), true) + require.NoError(tb, err) + + worktree, err := repo.Worktree() + require.NoError(tb, err) + require.NoError(tb, worktree.Checkout(&git.CheckoutOptions{ + Branch: plumbing.NewBranchReferenceName(defaultBranch), + Force: true, + })) + + newHash := adoLiveWriteAndCommit(tb, repo, worktree.Filesystem().Root(), + "reverser-live-test.yaml", fmt.Sprintf("# %s at %s\n", message, time.Now().UTC()), message) + + require.NoError(tb, + PushAtomic(ctx, repo, rootRef.Hash(), plumbing.NewBranchReferenceName(defaultBranch), target.Auth)) + tb.Logf("advanced %q to %s", defaultBranch, newHash) + + return newHash +} + +// adoLiveWriteAndCommit writes a file and commits it, returning the commit hash. +func adoLiveWriteAndCommit(tb testing.TB, repo *git.Repository, dir, name, content, message string) plumbing.Hash { + tb.Helper() + + worktree, err := repo.Worktree() + require.NoError(tb, err) + + require.NoError(tb, os.WriteFile(filepath.Join(dir, name), []byte(content), 0600)) + _, err = worktree.Add(name) + require.NoError(tb, err) + + hash, err := worktree.Commit(message, &git.CommitOptions{ + Author: &object.Signature{Name: "reverser-live-test", Email: "noreply@example.com", When: time.Now()}, + }) + require.NoError(tb, err) + + return hash +} + +// requireRemoteTracking asserts a remote-tracking ref exists locally after a fetch. +func requireRemoteTracking(tb testing.TB, repo *git.Repository, branch, why string) { + tb.Helper() + + _, err := repo.Reference(plumbing.NewRemoteReferenceName("origin", branch), true) + require.NoError(tb, err, why) +} + +// TestADOLive_EmptyRepository covers the empty-repository contract against a real ADO remote: no +// default branch, no branches, and a fetch that reports nothing rather than failing. +// +// It needs a repository that stays empty, which TestADOLive_BranchResolutionOrder's cannot be — that +// one seeds itself on first run. Point E2E_ADO_EMPTY_REPO_URL at a repository you never write to. +func TestADOLive_EmptyRepository(t *testing.T) { + target := adoLiveFromEnv(t) + + url := strings.TrimSpace(os.Getenv("E2E_ADO_EMPTY_REPO_URL")) + if url == "" { + t.Skip("set E2E_ADO_EMPTY_REPO_URL to a commit-less ADO repository to run this") + } + ctx := context.Background() + + info, err := CheckRepo(ctx, url, target.Auth) + require.NoError(t, err, "an empty repository is a valid state, not an error") + assert.Nil(t, info.DefaultBranch, "an empty repository has no default branch to report") + assert.Zero(t, info.RemoteBranchCount) + + repo := adoLiveRepo(t, filepath.Join(t.TempDir(), "empty"), url) + resolved, err := SmartFetch(ctx, repo, plumbing.NewBranchReferenceName("main"), target.Auth) + require.NoError(t, err) + assert.Empty(t, resolved, "an empty remote resolves to no branch") +} + +// TestADOLive_StillRequiresMultiAck is a canary rather than a regression test. +// +// It asserts that Azure DevOps STILL rejects an upload-pack request whose capability list omits +// multi_ack. That is the premise the whole go-git v6 migration rests on, it is a premise about someone +// else's server, and it can stop being true without anyone telling us. +// +// The request shape matters, and getting it wrong produces a confidently wrong answer: a want line +// carrying NO capability list is answered 200, while `want side-band-64k` is answered 400. Real +// clients always send a capability list, so that is the case to probe. The full measured matrix, and +// the sources, are in docs/facts/azure-devops-multi-ack-requirement.md. +// +// If this test fails, nothing is broken: Microsoft has fixed their end, and the constraint behind #288 +// no longer exists. The failure message says so, because a red test that means good news is otherwise +// deeply confusing. +func TestADOLive_StillRequiresMultiAck(t *testing.T) { + target := adoLiveFromEnv(t) + + // Both halves need a deadline: http.DefaultClient has none, so an unreachable or mid-response + // Azure DevOps would hang the test rather than skipping it the way the error path below intends. + ctx, cancel := context.WithTimeout(context.Background(), adoCanaryTimeout) + defer cancel() + + info, err := CheckRepo(ctx, target.URL, target.Auth) + require.NoError(t, err) + require.NotNil(t, info.DefaultBranch, "the canary needs a repository with at least one commit") + + want := fmt.Sprintf("want %s side-band-64k ofs-delta agent=gitops-reverser-canary\n", + info.DefaultBranch.Sha) + body := fmt.Sprintf("%04x%s0000%04x%s", len(want)+4, want, len("done\n")+4, "done\n") + + req, err := http.NewRequestWithContext(ctx, http.MethodPost, + target.URL+"/git-upload-pack", strings.NewReader(body)) + require.NoError(t, err) + req.Header.Set("Content-Type", "application/x-git-upload-pack-request") + req.Header.Set("Authorization", "Basic "+base64.StdEncoding.EncodeToString( + []byte(os.Getenv("E2E_ADO_USERNAME")+":"+os.Getenv("E2E_ADO_PAT")))) + + resp, err := http.DefaultClient.Do(req) + if err != nil { + t.Skipf("skipping canary, request failed: %v", err) + } + defer func() { _ = resp.Body.Close() }() + + payload, err := io.ReadAll(io.LimitReader(resp.Body, 4096)) + require.NoError(t, err) + + if resp.StatusCode == http.StatusBadRequest { + t.Logf("Azure DevOps still rejects a multi_ack-less fetch: HTTP 400, %q", + strings.TrimSpace(string(payload))) + assert.Contains(t, string(payload), "TF401041", + "the rejection should still carry ADO's documented error code") + return + } + + t.Fatalf( + "GOOD NEWS, NOT A BUG: Azure DevOps accepted an upload-pack request whose capability list "+ + "omits multi_ack (HTTP %d). The constraint behind #288 appears to be gone, so go-git v6 is "+ + "no longer forced by ADO and this canary has done its job. Re-check "+ + "docs/facts/azure-devops-multi-ack-requirement.md and delete this test. Response: %q", + resp.StatusCode, strings.TrimSpace(string(payload))) +} diff --git a/internal/git/ado_multiack_test.go b/internal/git/ado_multiack_test.go new file mode 100644 index 00000000..6a80b7c2 --- /dev/null +++ b/internal/git/ado_multiack_test.go @@ -0,0 +1,351 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +// Azure DevOps rejects any protocol-v0 `upload-pack` request whose capability list omits +// `multi_ack`, answering HTTP 400 with `TF401041: Clients must support multi-ack.` go-git v5 keeps +// MultiACK and MultiACKDetailed in transport.UnsupportedCapabilities and deletes them from the +// server's advertisement as it parses, so the capability is never requested and every fetch against +// ADO fails. +// +// Nobody on the team has an Azure DevOps tenant, so these tests reproduce the failure locally. The +// load-bearing fact is that canonical git's own `upload-pack` advertises `multi_ack` and +// `multi_ack_detailed`, which makes `git-http-backend` a genuine multi_ack server. Wrapping it in a +// proxy that enforces ADO's rule gives a faithful simulator for both halves of the problem: the +// proxy reproduces the rejected request, and the real backend reproduces the multi-ACK response that +// v5 additionally cannot parse. +// +// References: +// - https://github.com/go-git/go-git/issues/64 — the original ADO report, open since 2019 +// - https://github.com/fluxcd/source-controller/issues/104 — Flux hitting the same wall +// - https://github.com/go-git/go-git/pull/1204 — the multi_ack implementation, v6 only +// - https://git-scm.com/docs/protocol-capabilities — multi_ack is upload-pack only +// - docs/design/azure-devops-multi-ack.md — the capability matrix these tests encode + +import ( + "bytes" + "compress/gzip" + "context" + "io" + "net/http" + "net/http/cgi" + "net/http/httptest" + "os" + "os/exec" + "path/filepath" + "strings" + "sync/atomic" + "testing" + + "github.com/go-git/go-git/v6" + "github.com/go-git/go-git/v6/plumbing" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// adoRejectionBody is Azure DevOps' response to an upload-pack request without multi_ack. +const adoRejectionBody = "TF401041: Clients must support multi-ack." + +// adoSimulator is a git HTTP server that enforces Azure DevOps' multi_ack rule. +type adoSimulator struct { + // RepoURL is the clone URL of the served repository. + RepoURL string + + // uploadPackPosts counts POSTs to git-upload-pack; rejected counts those answered 400. + uploadPackPosts atomic.Int64 + rejected atomic.Int64 +} + +// gitHTTPBackend locates canonical git's CGI server, skipping the test when it is unavailable. +func gitHTTPBackend(tb testing.TB) string { + tb.Helper() + + out, err := exec.Command("git", "--exec-path").Output() + if err != nil { + tb.Skipf("git --exec-path failed, cannot run the ADO simulator: %v", err) + } + + path := filepath.Join(strings.TrimSpace(string(out)), "git-http-backend") + if _, err := os.Stat(path); err != nil { + tb.Skipf("git-http-backend not found at %s: %v", path, err) + } + + return path +} + +// startADOSimulator serves a bare repository over HTTP through canonical git's http-backend and +// reproduces Azure DevOps' rejection: an upload-pack POST whose body does not contain the multi_ack +// capability is answered with HTTP 400 instead of being forwarded. +// +// The v2 opt-in header is stripped from every request. ADO's failure is a protocol-v0 behaviour, and +// a client that can negotiate protocol v2 would otherwise sidestep multi_ack entirely and pass these +// tests without implementing the capability under test. +func startADOSimulator(tb testing.TB, projectRoot, repoDir string) *adoSimulator { + tb.Helper() + return startGitHTTPServer(tb, projectRoot, repoDir, true) +} + +// startRealGitServer serves a bare repository over HTTP through canonical git's http-backend with no +// added restrictions. +// +// This exists because go-git's `file://` transport is no longer a faithful server. In v5 it spawned +// the real git-upload-pack/git-receive-pack binaries; in v6 it runs go-git's own in-process +// transport.ReceivePack, whose updateReferences validates only that a reference exists and then +// calls SetReference(cmd.New) — it never compares cmd.Old against the current value. A push over +// `file://` therefore always wins, which silently makes any test of our compare-and-swap vacuous. +// Real git enforces it, so tests that depend on a rejected concurrent push must use this. +func startRealGitServer(tb testing.TB, projectRoot, repoDir string) *adoSimulator { + tb.Helper() + return startGitHTTPServer(tb, projectRoot, repoDir, false) +} + +// startGitHTTPServer serves repoDir over HTTP via canonical git's CGI backend. When +// enforceMultiAck is set it additionally rejects upload-pack requests that omit the capability, +// which is what makes it an Azure DevOps simulator. +func startGitHTTPServer(tb testing.TB, projectRoot, repoDir string, enforceMultiAck bool) *adoSimulator { + tb.Helper() + + backendPath := gitHTTPBackend(tb) + sim := &adoSimulator{} + + backend := &cgi.Handler{ + Path: backendPath, + Env: []string{ + "GIT_PROJECT_ROOT=" + projectRoot, + "GIT_HTTP_EXPORT_ALL=1", + }, + InheritEnv: []string{"PATH"}, + } + + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Force protocol v0: see the doc comment above. + r.Header.Del("Git-Protocol") + + if r.Method == http.MethodPost && strings.HasSuffix(r.URL.Path, "/git-upload-pack") { + sim.uploadPackPosts.Add(1) + + body, err := io.ReadAll(r.Body) + if err != nil { + http.Error(w, "read body: "+err.Error(), http.StatusInternalServerError) + return + } + + if enforceMultiAck && !requestOffersMultiAck(body, r.Header.Get("Content-Encoding")) { + sim.rejected.Add(1) + http.Error(w, adoRejectionBody, http.StatusBadRequest) + return + } + + // Rewind for the backend. + r.Body = io.NopCloser(bytes.NewReader(body)) + r.ContentLength = int64(len(body)) + } + + backend.ServeHTTP(w, r) + }) + + server := httptest.NewServer(handler) + tb.Cleanup(server.Close) + + sim.RepoURL = server.URL + "/" + filepath.Base(repoDir) + return sim +} + +// requestOffersMultiAck reports whether an upload-pack request body advertises multi_ack. The +// capability list travels on the first `want` pkt-line, so a substring test over the body is +// exactly the check ADO performs. +func requestOffersMultiAck(body []byte, contentEncoding string) bool { + if strings.Contains(strings.ToLower(contentEncoding), "gzip") { + zr, err := gzip.NewReader(bytes.NewReader(body)) + if err != nil { + return false + } + defer func() { _ = zr.Close() }() + if plain, err := io.ReadAll(zr); err == nil { + body = plain + } + } + return bytes.Contains(body, []byte("multi_ack")) +} + +// newADORepo creates a bare repository seeded with one commit on main, configured so http-backend +// will serve both fetch and push. +func newADORepo(tb testing.TB) (string, string) { + tb.Helper() + + projectRoot := tb.TempDir() + repoDir := filepath.Join(projectRoot, "repo.git") + createBareRepo(tb, repoDir) + + // http-backend refuses receive-pack unless the repository opts in. + cmd := exec.Command("git", "-C", repoDir, "config", "http.receivepack", "true") + out, err := cmd.CombinedOutput() + require.NoError(tb, err, "git config http.receivepack: %s", out) + + simulateClientCommitOnDisk(tb, repoDir, "main", "seed.yaml", "kind: Seed\n") + return projectRoot, repoDir +} + +// setRemoteURL repoints origin at a different URL, so a fixture can be built over a cheap file +// path and then exercised across the simulator. +func setRemoteURL(tb testing.TB, repo *git.Repository, url string) { + tb.Helper() + + cfg, err := repo.Config() + require.NoError(tb, err) + + remote, ok := cfg.Remotes["origin"] + require.True(tb, ok, "origin remote must exist") + remote.URLs = []string{url} + + require.NoError(tb, repo.SetConfig(cfg)) +} + +// revParse returns the hash a ref points at in an on-disk repository, read with canonical git so +// the assertion does not depend on the library under test. +func revParse(tb testing.TB, repoDir, ref string) string { + tb.Helper() + + out, err := exec.Command("git", "-C", repoDir, "rev-parse", ref).Output() + require.NoError(tb, err, "git rev-parse %s", ref) + + return strings.TrimSpace(string(out)) +} + +// TestADOSimulator_IsFaithful guards the harness itself. It asserts the two properties the rest of +// this file depends on: a request without multi_ack is rejected exactly as ADO rejects it, and +// canonical git — which does advertise multi_ack — is served normally. If this test fails, the other +// results in this file mean nothing. +func TestADOSimulator_IsFaithful(t *testing.T) { + projectRoot, repoDir := newADORepo(t) + sim := startADOSimulator(t, projectRoot, repoDir) + + t.Run("an upload-pack request without multi_ack is rejected with TF401041", func(t *testing.T) { + body := "0032want 0000000000000000000000000000000000000000\n00000009done\n" + resp, err := http.Post( + sim.RepoURL+"/git-upload-pack", + "application/x-git-upload-pack-request", + strings.NewReader(body), + ) + require.NoError(t, err) + defer func() { _ = resp.Body.Close() }() + + payload, err := io.ReadAll(resp.Body) + require.NoError(t, err) + + assert.Equal(t, http.StatusBadRequest, resp.StatusCode, + "the simulator must reject a request that omits multi_ack") + assert.Contains(t, string(payload), "TF401041", + "the rejection must carry Azure DevOps' error code") + }) + + t.Run("canonical git clones successfully because it advertises multi_ack", func(t *testing.T) { + dest := filepath.Join(t.TempDir(), "clone") + cmd := exec.Command("git", "-c", "protocol.version=0", "clone", sim.RepoURL, dest) + cmd.Env = append(os.Environ(), "GIT_TERMINAL_PROMPT=0") + out, err := cmd.CombinedOutput() + require.NoError(t, err, "canonical git must be able to clone: %s", out) + + assert.FileExists(t, filepath.Join(dest, "seed.yaml"), + "the clone must have retrieved the seeded commit") + }) +} + +// TestADO_CheckRepo_NeedsNoNegotiation encodes the first row of the capability matrix: CheckRepo +// reads only the ref advertisement (GET /info/refs) and never posts a want/have negotiation, so +// ADO's multi_ack rule cannot reach it. +// +// This passes on go-git v5, which is the point: PR #292 routes CheckRepo through system git +// unnecessarily. +func TestADO_CheckRepo_NeedsNoNegotiation(t *testing.T) { + projectRoot, repoDir := newADORepo(t) + sim := startADOSimulator(t, projectRoot, repoDir) + + info, err := CheckRepo(context.Background(), sim.RepoURL, nil) + require.NoError(t, err, "CheckRepo must not need multi_ack: it only reads the advertisement") + + require.NotNil(t, info.DefaultBranch) + assert.Equal(t, "main", info.DefaultBranch.ShortName) + assert.Equal(t, 1, info.RemoteBranchCount) + + assert.Zero(t, sim.uploadPackPosts.Load(), + "CheckRepo must not POST to git-upload-pack at all") +} + +// TestADO_PushAtomic_NeedsNoMultiAck encodes the last row of the capability matrix: our atomic push +// speaks receive-pack, and multi_ack does not exist in that protocol. Measured advertisement from +// canonical git's receive-pack: +// +// report-status report-status-v2 delete-refs side-band-64k quiet atomic ofs-delta object-format=sha1 +// +// So the push is unaffected by ADO's rule, and by every fix for it. This passes on go-git v5 and +// must keep passing on v6: it is the regression guard on the single-session advertise-then-push with +// the server-side Old/New compare-and-swap. +func TestADO_PushAtomic_NeedsNoMultiAck(t *testing.T) { + projectRoot, repoDir := newADORepo(t) + sim := startADOSimulator(t, projectRoot, repoDir) + + // Work from a local clone made over the file path, so getting the fixture in place does not + // depend on the fetch path that is broken on v5. + local := filepath.Join(t.TempDir(), "work") + repo, worktree := initLocalRepo(t, local, repoDir, "main") + + rootHash, err := repo.ResolveRevision(plumbing.Revision("refs/heads/main")) + require.NoError(t, err) + + // Point origin at the simulator, so the push crosses the enforcing proxy. + setRemoteURL(t, repo, sim.RepoURL) + + newHash := commitFileChange(t, worktree, local, "pushed.yaml", "kind: Pushed\n") + + err = PushAtomic(context.Background(), repo, *rootHash, plumbing.ReferenceName("refs/heads/main"), nil) + require.NoError(t, err, "receive-pack has no multi_ack, so the push must succeed") + + assert.Zero(t, sim.uploadPackPosts.Load(), "a push must not touch git-upload-pack") + + // Confirm the remote actually moved to our commit. + remoteHash := revParse(t, repoDir, "refs/heads/main") + assert.Equal(t, newHash.String(), remoteHash, "the remote must be at the pushed commit") +} + +// TestADO_SmartFetch_RequiresMultiAck is the red-first test for the migration. +// +// It asserts the behaviour we want: a fetch from an Azure DevOps-style remote succeeds. On go-git +// v5 it FAILS, because MultiACK and MultiACKDetailed sit in transport.UnsupportedCapabilities and +// are stripped from the advertisement before packp.NewUploadPackRequestFromCapabilities decides +// what to ask for, so the want line omits multi_ack and the simulator answers 400 exactly as ADO +// does. On go-git v6 it passes, because PR #1204 implements the capability. +// +// See https://github.com/go-git/go-git/pull/1204 and docs/design/azure-devops-multi-ack.md. +func TestADO_SmartFetch_RequiresMultiAck(t *testing.T) { + projectRoot, repoDir := newADORepo(t) + sim := startADOSimulator(t, projectRoot, repoDir) + + // A repository that already has objects, so the fetch sends have lines and a real negotiation + // happens. This is the case Flux avoids by only ever cloning. + local := filepath.Join(t.TempDir(), "work") + repo, _ := initLocalRepo(t, local, repoDir, "main") + setRemoteURL(t, repo, sim.RepoURL) + + // Move the remote on, so there is something to fetch. + simulateClientCommitOnDisk(t, repoDir, "main", "second.yaml", "kind: Second\n") + wantHash := revParse(t, repoDir, "refs/heads/main") + + branch, err := SmartFetch( + context.Background(), repo, plumbing.ReferenceName("refs/heads/main"), nil) + require.NoError(t, err, + "fetch from an ADO-style remote must succeed; on go-git v5 this fails with %q", + adoRejectionBody) + + assert.Equal(t, "refs/heads/main", branch.String()) + + // The fetch must have actually advanced the remote-tracking ref, not merely not errored. + ref, err := repo.Reference(plumbing.ReferenceName("refs/remotes/origin/main"), true) + require.NoError(t, err) + assert.Equal(t, wantHash, ref.Hash().String(), "the fetch must have retrieved the new commit") + + assert.Positive(t, sim.uploadPackPosts.Load(), + "the test is meaningless unless a real negotiation happened") + assert.Zero(t, sim.rejected.Load(), + "the request must have offered multi_ack rather than being rejected") +} diff --git a/internal/git/bootstrapped_repo_template.go b/internal/git/bootstrapped_repo_template.go index 4d37257a..039c949a 100644 --- a/internal/git/bootstrapped_repo_template.go +++ b/internal/git/bootstrapped_repo_template.go @@ -14,7 +14,7 @@ import ( "strings" "text/template" - gogit "github.com/go-git/go-git/v5" + gogit "github.com/go-git/go-git/v6" ) const ( @@ -86,7 +86,7 @@ func stageBootstrapTemplateInPath(worktree *gogit.Worktree, targetPath string, o } func bootstrapTargetDirectory(worktree *gogit.Worktree, targetPath string) (string, error) { - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() if targetPath == "" { return root, nil } diff --git a/internal/git/branch_worker.go b/internal/git/branch_worker.go index eb97f2de..62723cb7 100644 --- a/internal/git/branch_worker.go +++ b/internal/git/branch_worker.go @@ -14,9 +14,9 @@ import ( "sync/atomic" "time" - gogit "github.com/go-git/go-git/v5" - "github.com/go-git/go-git/v5/plumbing" - "github.com/go-git/go-git/v5/plumbing/transport" + gogit "github.com/go-git/go-git/v6" + "github.com/go-git/go-git/v6/plumbing" + gitclient "github.com/go-git/go-git/v6/plumbing/client" "github.com/go-logr/logr" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/metric" @@ -1347,7 +1347,7 @@ func fetchRemoteBranchHash( ctx context.Context, repo *gogit.Repository, branch plumbing.ReferenceName, - auth transport.AuthMethod, + auth []gitclient.Option, ) (plumbing.Hash, error) { if _, err := SmartFetch(ctx, repo, branch, auth); err != nil { return plumbing.ZeroHash, err diff --git a/internal/git/branch_worker_metrics_test.go b/internal/git/branch_worker_metrics_test.go index 6efbe034..c5b9b88c 100644 --- a/internal/git/branch_worker_metrics_test.go +++ b/internal/git/branch_worker_metrics_test.go @@ -7,7 +7,7 @@ import ( "testing" "time" - "github.com/go-git/go-git/v5/plumbing" + "github.com/go-git/go-git/v6/plumbing" "github.com/go-logr/logr" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" diff --git a/internal/git/branch_worker_split_test.go b/internal/git/branch_worker_split_test.go index 9078be99..4d6ca788 100644 --- a/internal/git/branch_worker_split_test.go +++ b/internal/git/branch_worker_split_test.go @@ -11,11 +11,11 @@ import ( "testing" "time" - "github.com/go-git/go-git/v5" - "github.com/go-git/go-git/v5/config" - "github.com/go-git/go-git/v5/plumbing" - "github.com/go-git/go-git/v5/plumbing/object" - "github.com/go-git/go-git/v5/plumbing/transport" + "github.com/go-git/go-git/v6" + "github.com/go-git/go-git/v6/config" + "github.com/go-git/go-git/v6/plumbing" + gitclient "github.com/go-git/go-git/v6/plumbing/client" + "github.com/go-git/go-git/v6/plumbing/object" "github.com/go-logr/logr" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -525,7 +525,7 @@ func TestBranchWorker_TransientPushFailure_RetriesSameLocalCommits(t *testing.T) _ *git.Repository, _ plumbing.Hash, _ plumbing.ReferenceName, - _ transport.AuthMethod, + _ []gitclient.Option, ) error { return pushErr } @@ -533,7 +533,7 @@ func TestBranchWorker_TransientPushFailure_RetriesSameLocalCommits(t *testing.T) _ context.Context, _ *git.Repository, _ plumbing.ReferenceName, - _ transport.AuthMethod, + _ []gitclient.Option, ) (plumbing.Hash, error) { return rootHashBefore, nil } @@ -541,7 +541,7 @@ func TestBranchWorker_TransientPushFailure_RetriesSameLocalCommits(t *testing.T) _ context.Context, _ *git.Repository, _ plumbing.ReferenceName, - _ transport.AuthMethod, + _ []gitclient.Option, ) (*PullReport, error) { syncCalled = true return &PullReport{}, nil @@ -604,7 +604,7 @@ func TestBranchWorker_PushFollowedByFetchFailure_TreatsAsTransient(t *testing.T) _ *git.Repository, _ plumbing.Hash, _ plumbing.ReferenceName, - _ transport.AuthMethod, + _ []gitclient.Option, ) error { return pushErr } @@ -612,7 +612,7 @@ func TestBranchWorker_PushFollowedByFetchFailure_TreatsAsTransient(t *testing.T) _ context.Context, _ *git.Repository, _ plumbing.ReferenceName, - _ transport.AuthMethod, + _ []gitclient.Option, ) (plumbing.Hash, error) { return plumbing.ZeroHash, fetchErr } @@ -620,7 +620,7 @@ func TestBranchWorker_PushFollowedByFetchFailure_TreatsAsTransient(t *testing.T) _ context.Context, _ *git.Repository, _ plumbing.ReferenceName, - _ transport.AuthMethod, + _ []gitclient.Option, ) (*PullReport, error) { syncCalled = true return &PullReport{}, nil diff --git a/internal/git/branch_worker_test.go b/internal/git/branch_worker_test.go index 6f096205..4c9c72af 100644 --- a/internal/git/branch_worker_test.go +++ b/internal/git/branch_worker_test.go @@ -10,10 +10,10 @@ import ( "time" "filippo.io/age" - "github.com/go-git/go-git/v5" - "github.com/go-git/go-git/v5/config" - "github.com/go-git/go-git/v5/plumbing" - "github.com/go-git/go-git/v5/plumbing/object" + "github.com/go-git/go-git/v6" + "github.com/go-git/go-git/v6/config" + "github.com/go-git/go-git/v6/plumbing" + "github.com/go-git/go-git/v6/plumbing/object" "github.com/go-logr/logr" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -975,7 +975,7 @@ func TestBranchWorker_CommitAndPushRequest_SignsCommitWhenConfigured(t *testing. commit, err := serverRepo.CommitObject(remoteHeadRef.Hash()) require.NoError(t, err) - assert.Contains(t, commit.PGPSignature, "-----BEGIN SSH SIGNATURE-----") + assert.Contains(t, commit.Signature, "-----BEGIN SSH SIGNATURE-----") signingPublicKey, err := SSHAuthorizedPublicKeyFromSecret(signingSecret) require.NoError(t, err) diff --git a/internal/git/commit.go b/internal/git/commit.go index a10fdc27..b9c98bf2 100644 --- a/internal/git/commit.go +++ b/internal/git/commit.go @@ -11,8 +11,8 @@ import ( "time" "unicode" - "github.com/go-git/go-git/v5" - "github.com/go-git/go-git/v5/plumbing/object" + "github.com/go-git/go-git/v6" + "github.com/go-git/go-git/v6/plumbing/object" "k8s.io/apimachinery/pkg/runtime/schema" "github.com/ConfigButler/gitops-reverser/internal/types" diff --git a/internal/git/commit_executor.go b/internal/git/commit_executor.go index ba8eb9a7..c3f39867 100644 --- a/internal/git/commit_executor.go +++ b/internal/git/commit_executor.go @@ -10,8 +10,8 @@ import ( "strings" "time" - gogit "github.com/go-git/go-git/v5" - "github.com/go-git/go-git/v5/plumbing" + gogit "github.com/go-git/go-git/v6" + "github.com/go-git/go-git/v6/plumbing" "sigs.k8s.io/controller-runtime/pkg/log" ) @@ -118,7 +118,7 @@ func (w *BranchWorker) executePendingWrite( } target := pendingWrite.Target() - encryptionPath := filepath.Join(worktree.Filesystem.Root(), sanitizePath(pendingWrite.path())) + encryptionPath := filepath.Join(worktree.Filesystem().Root(), sanitizePath(pendingWrite.path())) if err := configureSecretEncryptionWriter( w.contentWriter, encryptionPath, diff --git a/internal/git/commit_executor_test.go b/internal/git/commit_executor_test.go index 16b37972..97e85974 100644 --- a/internal/git/commit_executor_test.go +++ b/internal/git/commit_executor_test.go @@ -9,8 +9,8 @@ import ( "testing" "time" - "github.com/go-git/go-git/v5" - "github.com/go-git/go-git/v5/plumbing/object" + "github.com/go-git/go-git/v6" + "github.com/go-git/go-git/v6/plumbing/object" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -24,6 +24,7 @@ func newExecutorTestRepo(t *testing.T) (*BranchWorker, *git.Repository, *git.Wor repoPath := t.TempDir() repo, err := git.PlainInit(repoPath, false) require.NoError(t, err) + require.NoError(t, PinExplicitSigningPolicy(repo)) require.NoError(t, setHeadToMain(repo)) worktree, err := repo.Worktree() diff --git a/internal/git/commit_request_attach_test.go b/internal/git/commit_request_attach_test.go index f967fe27..ac4645a7 100644 --- a/internal/git/commit_request_attach_test.go +++ b/internal/git/commit_request_attach_test.go @@ -7,9 +7,9 @@ import ( "testing" "time" - gogit "github.com/go-git/go-git/v5" - "github.com/go-git/go-git/v5/config" - "github.com/go-git/go-git/v5/plumbing" + gogit "github.com/go-git/go-git/v6" + "github.com/go-git/go-git/v6/config" + "github.com/go-git/go-git/v6/plumbing" "github.com/go-logr/logr" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" diff --git a/internal/git/credentials.go b/internal/git/credentials.go index 9300ba12..2f4dbc81 100644 --- a/internal/git/credentials.go +++ b/internal/git/credentials.go @@ -4,16 +4,19 @@ package git import ( "context" + "errors" "fmt" - "github.com/go-git/go-git/v5/plumbing/transport" + gitclient "github.com/go-git/go-git/v6/plumbing/client" + gogithttp "github.com/go-git/go-git/v6/plumbing/transport/http" + corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" "github.com/ConfigButler/gitops-reverser/api/v1alpha3" - "github.com/ConfigButler/gitops-reverser/internal/ssh" + sshpkg "github.com/ConfigButler/gitops-reverser/internal/ssh" ) // SSHHostKeyConfig configures where SSH known_hosts (host-trust material) are sourced and the @@ -37,15 +40,30 @@ type SSHHostKeyConfig struct { } // getAuthFromSecret fetches the credentials Secret named by the GitProvider and resolves it into -// a go-git auth method. A GitProvider with no secretRef authenticates anonymously (public repos). +// go-git transport options. A GitProvider with no secretRef authenticates anonymously (public repos). func getAuthFromSecret( ctx context.Context, k8sClient client.Client, provider *v1alpha3.GitProvider, hostKeys SSHHostKeyConfig, -) (transport.AuthMethod, error) { +) ([]gitclient.Option, error) { + cred, err := credentialFromSecret(ctx, k8sClient, provider, hostKeys) + if err != nil { + return nil, err + } + return cred.Options(), nil +} + +// credentialFromSecret is getAuthFromSecret before the options wrapper: it returns the concrete +// credential so the Secret-to-auth mapping stays assertable. See Credential. +func credentialFromSecret( + ctx context.Context, + k8sClient client.Client, + provider *v1alpha3.GitProvider, + hostKeys SSHHostKeyConfig, +) (Credential, error) { if provider.Spec.SecretRef == nil || provider.Spec.SecretRef.Name == "" { - return nil, nil //nolint:nilnil // Returning nil auth for public repos is semantically correct + return Credential{}, nil // anonymous access for public repositories } secretName := types.NamespacedName{ @@ -55,53 +73,113 @@ func getAuthFromSecret( var secret corev1.Secret if err := k8sClient.Get(ctx, secretName, &secret); err != nil { - return nil, fmt.Errorf("failed to get secret %s: %w", secretName, err) + return Credential{}, fmt.Errorf("failed to get secret %s: %w", secretName, err) } - return AuthFromSecretData(ctx, k8sClient, provider, &secret, hostKeys) + return CredentialFromSecretData(ctx, k8sClient, provider, &secret, hostKeys) } -// AuthFromSecretData resolves a go-git auth method from an already-fetched Git credentials Secret, +// Credential is the concrete credential a Secret yields, before it is wrapped into go-git v6's +// opaque transport client options. At most one field is non-nil; all nil means anonymous access to a +// public repository. +// +// This type exists because v6 removed transport.AuthMethod: authentication is now supplied as +// functional options, which are closures and therefore cannot be inspected. Keeping the concrete +// value on the way past preserves the ability to assert which Secret key maps to which auth field, +// which is the contract CredentialFromSecretData is actually responsible for. +type Credential struct { + SSH *sshpkg.KeyAuth + Basic *gogithttp.BasicAuth + Bearer *gogithttp.TokenAuth +} + +// Options renders the credential as go-git v6 transport client options. A zero Credential yields +// nil, which go-git treats as anonymous. +func (c Credential) Options() []gitclient.Option { + switch { + case c.SSH != nil: + return []gitclient.Option{gitclient.WithSSHAuth(c.SSH)} + case c.Basic != nil: + return []gitclient.Option{gitclient.WithHTTPAuth(c.Basic)} + case c.Bearer != nil: + return []gitclient.Option{gitclient.WithHTTPAuth(c.Bearer)} + default: + return nil + } +} + +// AuthFromSecretData resolves go-git transport options from an already-fetched Git credentials +// Secret. It is the thin wrapper over CredentialFromSecretData; callers that need to know what kind +// of credential was produced should use that instead. +func AuthFromSecretData( + ctx context.Context, + k8sClient client.Client, + provider *v1alpha3.GitProvider, + secret *corev1.Secret, + hostKeys SSHHostKeyConfig, +) ([]gitclient.Option, error) { + cred, err := CredentialFromSecretData(ctx, k8sClient, provider, secret, hostKeys) + if err != nil { + return nil, err + } + return cred.Options(), nil +} + +// CredentialFromSecretData resolves a credential from an already-fetched Git credentials Secret, // accepting the Kubernetes-native, Flux, and Argo CD key dialects (the credentials Secret is the // one portable artifact across those ecosystems). provider supplies the namespace and the optional // knownHostsRef for SSH host trust; hostKeys supplies the install-level default and the dev escape // hatch. Auth precedence is: SSH key (if present) → HTTP basic (username+password) → bearer token. -func AuthFromSecretData( +func CredentialFromSecretData( ctx context.Context, k8sClient client.Client, provider *v1alpha3.GitProvider, secret *corev1.Secret, hostKeys SSHHostKeyConfig, -) (transport.AuthMethod, error) { +) (Credential, error) { if secret == nil { - return nil, nil //nolint:nilnil // no secret means anonymous (public repository) access + return Credential{}, nil // no secret means anonymous (public repository) access } // SSH private key: ssh-privatekey (Kubernetes-native) → identity (Flux) → sshPrivateKey (Argo). if privateKey, ok := firstSecretValue(secret, "ssh-privatekey", "identity", "sshPrivateKey"); ok { knownHosts, err := resolveKnownHosts(ctx, k8sClient, provider, secret, hostKeys) if err != nil { - return nil, err + return Credential{}, err + } + publicKeys, err := sshpkg.NewPublicKeyAuth( + privateKey, sshPassphrase(secret), knownHosts, hostKeys.AllowMissingKnownHosts) + if err != nil { + return Credential{}, err } - return ssh.GetAuthMethod(privateKey, sshPassphrase(secret), knownHosts, hostKeys.AllowMissingKnownHosts) + return Credential{SSH: publicKeys}, nil } // HTTP basic auth: username + password — already identical across all three ecosystems. - if username, ok := firstSecretValue(secret, "username"); ok { - password, hasPassword := firstSecretValue(secret, "password") - if !hasPassword { - return nil, fmt.Errorf( - "secret %s/%s contains username but no password for HTTP basic auth", secret.Namespace, secret.Name) - } - return GetHTTPAuthMethod(username, password) + // + // The password is what carries the credential, so it is what we branch on. Azure DevOps + // documents its Personal Access Tokens as an EMPTY username with the PAT as the password + // (the https://:PAT@dev.azure.com/... form), and firstSecretValue treats an empty value as + // an absent key — so keying off the username would refuse the very Secret ADO tells people + // to create. A username without a password stays an error: that one is a real mistake. + username, hasUsername := firstSecretValue(secret, "username") + if password, ok := firstSecretValue(secret, "password"); ok { + return Credential{Basic: &gogithttp.BasicAuth{Username: username, Password: password}}, nil + } + if hasUsername { + return Credential{}, fmt.Errorf( + "secret %s/%s contains username but no password for HTTP basic auth", secret.Namespace, secret.Name) } // HTTP bearer token: bearerToken — the common token path in both Flux and Argo. if token, ok := firstSecretValue(secret, "bearerToken"); ok { - return GetHTTPTokenAuthMethod(token) + if token == "" { + return Credential{}, errors.New("bearer token cannot be empty") + } + return Credential{Bearer: &gogithttp.TokenAuth{Token: token}}, nil } - return nil, fmt.Errorf( + return Credential{}, fmt.Errorf( "secret %s/%s does not contain valid authentication data "+ "(an SSH private key, username/password, or bearerToken)", secret.Namespace, secret.Name, diff --git a/internal/git/credentials_test.go b/internal/git/credentials_test.go index 912d996a..fb108c88 100644 --- a/internal/git/credentials_test.go +++ b/internal/git/credentials_test.go @@ -10,8 +10,6 @@ import ( "encoding/pem" "testing" - gogithttp "github.com/go-git/go-git/v5/plumbing/transport/http" - gogitssh "github.com/go-git/go-git/v5/plumbing/transport/ssh" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" gossh "golang.org/x/crypto/ssh" @@ -58,10 +56,10 @@ func TestAuthFromSecretData_SSHKeyDialects(t *testing.T) { keyName: privateKey, "known_hosts": []byte(knownHosts), }} - auth, err := AuthFromSecretData( + auth, err := CredentialFromSecretData( context.Background(), c, &configv1alpha3.GitProvider{}, secret, SSHHostKeyConfig{}) require.NoError(t, err) - assert.IsType(t, &gogitssh.PublicKeys{}, auth) + assert.NotNil(t, auth.SSH) }) } } @@ -76,9 +74,10 @@ func TestAuthFromSecretData_PasswordIsSSHPassphraseWhenKeyPresent(t *testing.T) "password": []byte(""), // unencrypted key: ignored, but must not divert to basic auth "known_hosts": []byte(knownHosts), }} - auth, err := AuthFromSecretData(context.Background(), c, &configv1alpha3.GitProvider{}, secret, SSHHostKeyConfig{}) + auth, err := CredentialFromSecretData( + context.Background(), c, &configv1alpha3.GitProvider{}, secret, SSHHostKeyConfig{}) require.NoError(t, err) - assert.IsType(t, &gogitssh.PublicKeys{}, auth) + assert.NotNil(t, auth.SSH) } func TestAuthFromSecretData_HTTPBasicAndBearer(t *testing.T) { @@ -89,10 +88,10 @@ func TestAuthFromSecretData_HTTPBasicAndBearer(t *testing.T) { "username": []byte("u"), "password": []byte("p"), }} - auth, err := AuthFromSecretData( + auth, err := CredentialFromSecretData( context.Background(), c, &configv1alpha3.GitProvider{}, secret, SSHHostKeyConfig{}) require.NoError(t, err) - basic, ok := auth.(*gogithttp.BasicAuth) + basic, ok := auth.Basic, auth.Basic != nil require.True(t, ok) assert.Equal(t, "u", basic.Username) assert.Equal(t, "p", basic.Password) @@ -100,17 +99,17 @@ func TestAuthFromSecretData_HTTPBasicAndBearer(t *testing.T) { t.Run("bearer token", func(t *testing.T) { secret := &corev1.Secret{Data: map[string][]byte{"bearerToken": []byte("gho_token")}} - auth, err := AuthFromSecretData( + auth, err := CredentialFromSecretData( context.Background(), c, &configv1alpha3.GitProvider{}, secret, SSHHostKeyConfig{}) require.NoError(t, err) - token, ok := auth.(*gogithttp.TokenAuth) + token, ok := auth.Bearer, auth.Bearer != nil require.True(t, ok) assert.Equal(t, "gho_token", token.Token) }) t.Run("username without password", func(t *testing.T) { secret := &corev1.Secret{Data: map[string][]byte{"username": []byte("u")}} - _, err := AuthFromSecretData( + _, err := CredentialFromSecretData( context.Background(), c, &configv1alpha3.GitProvider{}, secret, SSHHostKeyConfig{}) require.Error(t, err) assert.Contains(t, err.Error(), "no password") @@ -118,17 +117,18 @@ func TestAuthFromSecretData_HTTPBasicAndBearer(t *testing.T) { t.Run("no recognizable credentials", func(t *testing.T) { secret := &corev1.Secret{Data: map[string][]byte{"random": []byte("x")}} - _, err := AuthFromSecretData( + _, err := CredentialFromSecretData( context.Background(), c, &configv1alpha3.GitProvider{}, secret, SSHHostKeyConfig{}) require.Error(t, err) assert.Contains(t, err.Error(), "does not contain valid authentication data") }) t.Run("nil secret is anonymous", func(t *testing.T) { - auth, err := AuthFromSecretData( + auth, err := CredentialFromSecretData( context.Background(), c, &configv1alpha3.GitProvider{}, nil, SSHHostKeyConfig{}) require.NoError(t, err) - assert.Nil(t, auth) + assert.Equal(t, Credential{}, auth, "no secret means anonymous") + assert.Nil(t, auth.Options(), "anonymous renders no transport options") }) } @@ -153,9 +153,9 @@ func TestResolveKnownHosts_Priority(t *testing.T) { }, } secret := &corev1.Secret{Data: map[string][]byte{"ssh-privatekey": privateKey}} - auth, err := AuthFromSecretData(context.Background(), c, provider, secret, SSHHostKeyConfig{}) + auth, err := CredentialFromSecretData(context.Background(), c, provider, secret, SSHHostKeyConfig{}) require.NoError(t, err) - assert.IsType(t, &gogitssh.PublicKeys{}, auth) + assert.NotNil(t, auth.SSH) }) t.Run("knownHostsRef ConfigMap (Argo ssh_known_hosts key)", func(t *testing.T) { @@ -167,9 +167,9 @@ func TestResolveKnownHosts_Priority(t *testing.T) { }, } secret := &corev1.Secret{Data: map[string][]byte{"ssh-privatekey": privateKey}} - auth, err := AuthFromSecretData(context.Background(), c, provider, secret, SSHHostKeyConfig{}) + auth, err := CredentialFromSecretData(context.Background(), c, provider, secret, SSHHostKeyConfig{}) require.NoError(t, err) - assert.IsType(t, &gogitssh.PublicKeys{}, auth) + assert.NotNil(t, auth.SSH) }) t.Run("knownHostsRef Secret", func(t *testing.T) { @@ -185,9 +185,9 @@ func TestResolveKnownHosts_Priority(t *testing.T) { }, } secret := &corev1.Secret{Data: map[string][]byte{"ssh-privatekey": privateKey}} - auth, err := AuthFromSecretData(context.Background(), c, provider, secret, SSHHostKeyConfig{}) + auth, err := CredentialFromSecretData(context.Background(), c, provider, secret, SSHHostKeyConfig{}) require.NoError(t, err) - assert.IsType(t, &gogitssh.PublicKeys{}, auth) + assert.NotNil(t, auth.SSH) }) t.Run("knownHostsRef missing object is an error", func(t *testing.T) { @@ -199,7 +199,7 @@ func TestResolveKnownHosts_Priority(t *testing.T) { }, } secret := &corev1.Secret{Data: map[string][]byte{"ssh-privatekey": privateKey}} - _, err := AuthFromSecretData(context.Background(), c, provider, secret, SSHHostKeyConfig{}) + _, err := CredentialFromSecretData(context.Background(), c, provider, secret, SSHHostKeyConfig{}) require.Error(t, err) assert.Contains(t, err.Error(), "absent") }) @@ -208,16 +208,16 @@ func TestResolveKnownHosts_Priority(t *testing.T) { c := credTestClient(t, khConfigMap("cluster-hosts", "known_hosts")) secret := &corev1.Secret{Data: map[string][]byte{"ssh-privatekey": privateKey}} hostKeys := SSHHostKeyConfig{ControllerNamespace: "ns", DefaultKnownHostsConfigMap: "cluster-hosts"} - auth, err := AuthFromSecretData(context.Background(), c, &configv1alpha3.GitProvider{}, secret, hostKeys) + auth, err := CredentialFromSecretData(context.Background(), c, &configv1alpha3.GitProvider{}, secret, hostKeys) require.NoError(t, err) - assert.IsType(t, &gogitssh.PublicKeys{}, auth) + assert.NotNil(t, auth.SSH) }) t.Run("absent install-level default falls through to fail-closed", func(t *testing.T) { c := credTestClient(t) secret := &corev1.Secret{Data: map[string][]byte{"ssh-privatekey": privateKey}} hostKeys := SSHHostKeyConfig{ControllerNamespace: "ns", DefaultKnownHostsConfigMap: "missing"} - _, err := AuthFromSecretData(context.Background(), c, &configv1alpha3.GitProvider{}, secret, hostKeys) + _, err := CredentialFromSecretData(context.Background(), c, &configv1alpha3.GitProvider{}, secret, hostKeys) require.Error(t, err) assert.Contains(t, err.Error(), "known_hosts is required") }) @@ -225,7 +225,8 @@ func TestResolveKnownHosts_Priority(t *testing.T) { t.Run("no source and no opt-out fails closed", func(t *testing.T) { c := credTestClient(t) secret := &corev1.Secret{Data: map[string][]byte{"ssh-privatekey": privateKey}} - _, err := AuthFromSecretData(context.Background(), c, &configv1alpha3.GitProvider{}, secret, SSHHostKeyConfig{}) + _, err := CredentialFromSecretData( + context.Background(), c, &configv1alpha3.GitProvider{}, secret, SSHHostKeyConfig{}) require.Error(t, err) assert.Contains(t, err.Error(), "known_hosts is required") }) @@ -233,11 +234,11 @@ func TestResolveKnownHosts_Priority(t *testing.T) { t.Run("opt-out permits missing known_hosts", func(t *testing.T) { c := credTestClient(t) secret := &corev1.Secret{Data: map[string][]byte{"ssh-privatekey": privateKey}} - auth, err := AuthFromSecretData( + auth, err := CredentialFromSecretData( context.Background(), c, &configv1alpha3.GitProvider{}, secret, SSHHostKeyConfig{AllowMissingKnownHosts: true}) require.NoError(t, err) - assert.IsType(t, &gogitssh.PublicKeys{}, auth) + assert.NotNil(t, auth.SSH) }) } @@ -247,9 +248,10 @@ func TestGetAuthFromSecret_FetchPaths(t *testing.T) { t.Run("no secretRef is anonymous", func(t *testing.T) { c := credTestClient(t) provider := &configv1alpha3.GitProvider{ObjectMeta: metav1.ObjectMeta{Namespace: "ns"}} - auth, err := getAuthFromSecret(context.Background(), c, provider, SSHHostKeyConfig{}) + auth, err := credentialFromSecret(context.Background(), c, provider, SSHHostKeyConfig{}) require.NoError(t, err) - assert.Nil(t, auth) + assert.Equal(t, Credential{}, auth, "no secretRef means anonymous") + assert.Nil(t, auth.Options(), "anonymous renders no transport options") }) t.Run("present secret resolves", func(t *testing.T) { @@ -262,9 +264,9 @@ func TestGetAuthFromSecret_FetchPaths(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Namespace: "ns"}, Spec: configv1alpha3.GitProviderSpec{SecretRef: &configv1alpha3.LocalSecretReference{Name: "creds"}}, } - auth, err := getAuthFromSecret(context.Background(), c, provider, SSHHostKeyConfig{}) + auth, err := credentialFromSecret(context.Background(), c, provider, SSHHostKeyConfig{}) require.NoError(t, err) - assert.IsType(t, &gogithttp.BasicAuth{}, auth) + assert.NotNil(t, auth.Basic) }) t.Run("missing referenced secret errors", func(t *testing.T) { @@ -273,16 +275,85 @@ func TestGetAuthFromSecret_FetchPaths(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Namespace: "ns"}, Spec: configv1alpha3.GitProviderSpec{SecretRef: &configv1alpha3.LocalSecretReference{Name: "absent"}}, } - _, err := getAuthFromSecret(context.Background(), c, provider, SSHHostKeyConfig{}) + _, err := credentialFromSecret(context.Background(), c, provider, SSHHostKeyConfig{}) require.Error(t, err) }) } -func TestGetHTTPTokenAuthMethod(t *testing.T) { - auth, err := GetHTTPTokenAuthMethod("abc") +func TestCredentialFromSecretData_BearerToken(t *testing.T) { + c := credTestClient(t) + + secret := &corev1.Secret{Data: map[string][]byte{"bearerToken": []byte("abc")}} + auth, err := CredentialFromSecretData( + context.Background(), c, &configv1alpha3.GitProvider{}, secret, SSHHostKeyConfig{}) require.NoError(t, err) - assert.Equal(t, "abc", auth.(*gogithttp.TokenAuth).Token) + require.NotNil(t, auth.Bearer) + assert.Equal(t, "abc", auth.Bearer.Token) + assert.Len(t, auth.Options(), 1, "a bearer credential must render as one transport option") - _, err = GetHTTPTokenAuthMethod("") + empty := &corev1.Secret{Data: map[string][]byte{"bearerToken": []byte("")}} + _, err = CredentialFromSecretData( + context.Background(), c, &configv1alpha3.GitProvider{}, empty, SSHHostKeyConfig{}) require.Error(t, err) } + +// Azure DevOps documents its Personal Access Tokens as an empty username with the PAT as the +// password — the https://:PAT@dev.azure.com/... form. firstSecretValue treats an empty value as an +// absent key, so keying the basic-auth branch off the username refused exactly that Secret with +// "does not contain valid authentication data". The password is what carries the credential, so it +// is what we branch on. +func TestCredentialFromSecretData_AzureDevOpsPATForm(t *testing.T) { + c := credTestClient(t) + + for _, tc := range []struct { + name string + data map[string][]byte + }{ + {"empty username with a PAT", map[string][]byte{"username": []byte(""), "password": []byte("pat")}}, + {"no username key at all", map[string][]byte{"password": []byte("pat")}}, + } { + t.Run(tc.name, func(t *testing.T) { + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "ado", Namespace: "ns"}, + Data: tc.data, + } + + cred, err := CredentialFromSecretData( + context.Background(), c, &configv1alpha3.GitProvider{}, secret, SSHHostKeyConfig{}) + require.NoError(t, err) + require.NotNil(t, cred.Basic, "an ADO PAT must resolve to HTTP basic auth") + assert.Empty(t, cred.Basic.Username) + assert.Equal(t, "pat", cred.Basic.Password) + assert.Len(t, cred.Options(), 1) + }) + } + + // The rule is general, not an Azure DevOps carve-out: a username is simply optional, and is passed + // through untouched when supplied. Azure DevOps ignores it server-side (measured), but nothing in + // the credential reader knows or cares which provider it is talking to. + t.Run("a supplied username is still honoured", func(t *testing.T) { + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "basic", Namespace: "ns"}, + Data: map[string][]byte{"username": []byte("alice"), "password": []byte("pw")}, + } + + cred, err := CredentialFromSecretData( + context.Background(), c, &configv1alpha3.GitProvider{}, secret, SSHHostKeyConfig{}) + require.NoError(t, err) + require.NotNil(t, cred.Basic) + assert.Equal(t, "alice", cred.Basic.Username, "a username must not be dropped") + assert.Equal(t, "pw", cred.Basic.Password) + }) + + t.Run("a username with no password is still a mistake", func(t *testing.T) { + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "half", Namespace: "ns"}, + Data: map[string][]byte{"username": []byte("someone")}, + } + + _, err := CredentialFromSecretData( + context.Background(), c, &configv1alpha3.GitProvider{}, secret, SSHHostKeyConfig{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "contains username but no password") + }) +} diff --git a/internal/git/fieldpatch_flush_test.go b/internal/git/fieldpatch_flush_test.go index 7183b572..79c0b985 100644 --- a/internal/git/fieldpatch_flush_test.go +++ b/internal/git/fieldpatch_flush_test.go @@ -7,7 +7,7 @@ import ( "os" "testing" - gogit "github.com/go-git/go-git/v5" + gogit "github.com/go-git/go-git/v6" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/runtime/schema" diff --git a/internal/git/git.go b/internal/git/git.go index efceb02b..ae619860 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -13,13 +13,13 @@ import ( "path/filepath" "strings" - billyutil "github.com/go-git/go-billy/v5/util" - "github.com/go-git/go-git/v5" - "github.com/go-git/go-git/v5/config" - "github.com/go-git/go-git/v5/plumbing" - "github.com/go-git/go-git/v5/plumbing/format/index" - "github.com/go-git/go-git/v5/plumbing/transport" - "github.com/go-git/go-git/v5/plumbing/transport/http" + billyutil "github.com/go-git/go-billy/v6/util" + "github.com/go-git/go-git/v6" + "github.com/go-git/go-git/v6/config" + "github.com/go-git/go-git/v6/plumbing" + gitclient "github.com/go-git/go-git/v6/plumbing/client" + "github.com/go-git/go-git/v6/plumbing/format/index" + "github.com/go-git/go-git/v6/plumbing/transport" "github.com/go-logr/logr" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "sigs.k8s.io/controller-runtime/pkg/log" @@ -35,34 +35,8 @@ var ( ErrRemoteRefNotFoundEmptyRepo = errors.New("remote ref not found (empty repo)") ) -// GetHTTPAuthMethod returns an HTTP basic authentication method from username and password. -func GetHTTPAuthMethod(username, password string) (transport.AuthMethod, error) { - if username == "" { - return nil, errors.New("username cannot be empty") - } - if password == "" { - return nil, errors.New("password cannot be empty") - } - - return &http.BasicAuth{ - Username: username, - Password: password, - }, nil -} - -// GetHTTPTokenAuthMethod returns an HTTP bearer-token authentication method. Both Flux and Argo -// CD store token credentials (GitHub fine-grained PATs, GitLab project/group access tokens) under -// a "bearerToken" Secret key and authenticate without a username; go-git's TokenAuth sends the -// token as an Authorization: Bearer header. -func GetHTTPTokenAuthMethod(token string) (transport.AuthMethod, error) { - if token == "" { - return nil, errors.New("bearer token cannot be empty") - } - return &http.TokenAuth{Token: token}, nil -} - // CheckRepo performs lightweight connectivity checks and gathers repository metadata. -func CheckRepo(ctx context.Context, repoURL string, auth transport.AuthMethod) (*RepoInfo, error) { +func CheckRepo(ctx context.Context, repoURL string, auth []gitclient.Option) (*RepoInfo, error) { logger := log.FromContext(ctx) logger.V(1).Info("Checking repository connectivity and metadata", "url", repoURL) @@ -73,7 +47,7 @@ func CheckRepo(ctx context.Context, repoURL string, auth transport.AuthMethod) ( }) refs, err := remote.List(&git.ListOptions{ - Auth: auth, + ClientOptions: auth, }) if err != nil { // Check if this is an empty repository error @@ -121,7 +95,7 @@ func CheckRepo(ctx context.Context, repoURL string, auth transport.AuthMethod) ( func PrepareBranch( ctx context.Context, repoURL, repoPath, targetBranchName string, - auth transport.AuthMethod, + auth []gitclient.Option, ) (*PullReport, error) { logger := log.FromContext(ctx) logger.Info("Preparing branch for operations", "url", repoURL, "path", repoPath, "branch", targetBranchName) @@ -147,6 +121,13 @@ func PrepareBranch( } } + // Both paths, not just the fresh one: a repository from a persistent volume, or one created + // before this pin existed, needs the same policy or its next commit fails under an ambient + // commit.gpgSign. See PinExplicitSigningPolicy. + if err := PinExplicitSigningPolicy(repo); err != nil { + return nil, err + } + // Ensure the remote origin is set correctly if err := ensureRemoteOrigin(ctx, repo, repoURL); err != nil { return nil, fmt.Errorf("failed to ensure remote origin: %w", err) @@ -382,7 +363,7 @@ func syncToRemote( ctx context.Context, repo *git.Repository, branch plumbing.ReferenceName, - auth transport.AuthMethod, + auth []gitclient.Option, ) (*PullReport, error) { _, currentHash, err := GetCurrentBranch(repo) if err != nil { @@ -470,7 +451,7 @@ func cleanWorktree(r *git.Repository) error { return fmt.Errorf("failed to get worktree: %w", err) } - entries, err := w.Filesystem.ReadDir(".") + entries, err := w.Filesystem().ReadDir(".") if err != nil { return fmt.Errorf("failed to read worktree root: %w", err) } @@ -481,7 +462,7 @@ func cleanWorktree(r *git.Repository) error { continue } - if err := billyutil.RemoveAll(w.Filesystem, name); err != nil { + if err := billyutil.RemoveAll(w.Filesystem(), name); err != nil { return fmt.Errorf("failed to remove %q from worktree: %w", name, err) } } @@ -665,3 +646,33 @@ func initializeCleanRepository(repoPath string, logger logr.Logger) (*git.Reposi return repo, nil } + +// PinExplicitSigningPolicy records in the repository's own config that commits are not signed +// unless this operator signs them. +// +// go-git v6 consults commit.gpgSign — merged across system, global and local scope — whenever +// CommitOptions.Signer is nil, and refuses the commit outright when the setting is true and no +// signer is registered ("cannot auto-sign commit"). v5 ignored the setting entirely. +// +// Our signing policy comes from the GitProvider's signing Secret and is passed as +// CommitOptions.Signer, so an ambient commit.gpgSign — a developer's ~/.gitconfig, a mounted +// config, a future base image — must not be able to decide it for us. Writing the local value +// false makes the intent explicit and takes precedence over the wider scopes. Where we do sign, +// Signer is non-nil and this setting is never consulted. +func PinExplicitSigningPolicy(repo *git.Repository) error { + cfg, err := repo.Config() + if err != nil { + return fmt.Errorf("read repository config: %w", err) + } + + if cfg.Commit.GpgSign == config.OptBoolFalse { + return nil + } + + cfg.Commit.GpgSign = config.NewOptBool(false) + if err := repo.SetConfig(cfg); err != nil { + return fmt.Errorf("pin commit signing policy: %w", err) + } + + return nil +} diff --git a/internal/git/git_atomic_push.go b/internal/git/git_atomic_push.go index 8572c671..93635fef 100644 --- a/internal/git/git_atomic_push.go +++ b/internal/git/git_atomic_push.go @@ -9,44 +9,50 @@ import ( "fmt" "io" - "github.com/go-git/go-git/v5" - "github.com/go-git/go-git/v5/plumbing" - "github.com/go-git/go-git/v5/plumbing/format/packfile" - "github.com/go-git/go-git/v5/plumbing/protocol/packp" - "github.com/go-git/go-git/v5/plumbing/protocol/packp/capability" - "github.com/go-git/go-git/v5/plumbing/revlist" - "github.com/go-git/go-git/v5/plumbing/transport" - "github.com/go-git/go-git/v5/plumbing/transport/client" + "github.com/go-git/go-git/v6" + "github.com/go-git/go-git/v6/plumbing" + gitclient "github.com/go-git/go-git/v6/plumbing/client" + "github.com/go-git/go-git/v6/plumbing/format/packfile" + "github.com/go-git/go-git/v6/plumbing/protocol/packp" + "github.com/go-git/go-git/v6/plumbing/revlist" + "github.com/go-git/go-git/v6/plumbing/transport" "github.com/go-logr/logr" "sigs.k8s.io/controller-runtime/pkg/log" ) -// getPushSession creates and returns a receive-pack session for pushing. +// getPushSession opens a single receive-pack session for pushing. +// +// go-git v6 replaced v5's transport.NewEndpoint + client.NewClient + NewReceivePackSession with +// transport.ParseURL + client.New(opts...).Handshake. The property PushAtomic depends on is +// unchanged and now explicit in the interface: one Session serves both GetRemoteRefs (the +// advertisement) and Push, so the remote state we validate against is read on the same connection +// we then write to. func getPushSession( - _ context.Context, + ctx context.Context, repo *git.Repository, - auth transport.AuthMethod, -) (transport.ReceivePackSession, error) { - // Get remote configuration + auth []gitclient.Option, +) (transport.Session, error) { remote, err := repo.Remote("origin") if err != nil { return nil, fmt.Errorf("failed to get remote: %w", err) } - // Establish transport endpoint - endpoint, err := transport.NewEndpoint(remote.Config().URLs[0]) - if err != nil { - return nil, fmt.Errorf("failed to create endpoint: %w", err) + // go-git's own config validation rejects a remote with no URL, but a hand-edited or truncated + // .git/config can still present one, and indexing it would panic rather than fail. + urls := remote.Config().URLs + if len(urls) == 0 { + return nil, errors.New("remote origin has no URL configured") } - // Get the transport client - transportClient, err := client.NewClient(endpoint) + endpoint, err := transport.ParseURL(urls[0]) if err != nil { - return nil, fmt.Errorf("failed to create transport client: %w", err) + return nil, fmt.Errorf("failed to parse remote URL: %w", err) } - // Create receive-pack session (single session for verification and push) - session, err := transportClient.NewReceivePackSession(endpoint, auth) + session, err := gitclient.New(auth...).Handshake(ctx, &transport.Request{ + URL: endpoint, + Command: transport.ReceivePackService, + }) if err != nil { return nil, fmt.Errorf("failed to create receive-pack session: %w", err) } @@ -54,10 +60,23 @@ func getPushSession( return session, nil } +// advertisedHashes indexes a v6 advertisement by reference name. v5 handed back a +// map[string]plumbing.Hash directly; v6's transport.RemoteRefs carries a []*plumbing.Reference so +// that fields can be added without breaking the interface, so the lookup is built here. +func advertisedHashes(refs *transport.RemoteRefs) map[plumbing.ReferenceName]plumbing.Hash { + out := make(map[plumbing.ReferenceName]plumbing.Hash, len(refs.References)) + for _, ref := range refs.References { + if ref.Type() == plumbing.HashReference { + out[ref.Name()] = ref.Hash() + } + } + return out +} + // validatePushState checks if the push can proceed based on remote state. func validatePushState( ctx context.Context, - session transport.ReceivePackSession, + session transport.Session, repo *git.Repository, rootHash plumbing.Hash, rootBranch plumbing.ReferenceName, @@ -71,16 +90,17 @@ func validatePushState( branchName := branch.Short() - // Phase 1: Get advertised references (remote state) - refs, err := session.AdvertisedReferences() + // Phase 1: Get advertised references (remote state) on this same session. + remoteRefs, err := session.GetRemoteRefs(ctx, nil) if err != nil { return plumbing.ZeroHash, plumbing.ZeroHash, fmt.Errorf("failed to get advertised references: %w", err) } + refs := advertisedHashes(remoteRefs) // Determine the "old" hash for the push command and validate state var oldHash = plumbing.ZeroHash - remoteHash, found := refs.References[string(branch)] - currentRootHash, rootFound := refs.References[string(rootBranch)] + remoteHash, found := refs[branch] + currentRootHash, rootFound := refs[rootBranch] if !rootFound && !rootHash.IsZero() { return plumbing.ZeroHash, plumbing.ZeroHash, errors.New("remote went missing") } @@ -108,7 +128,7 @@ func validatePushState( // performPush executes the packfile creation and push operation. func performPush( ctx context.Context, - session transport.ReceivePackSession, + session transport.Session, repo *git.Repository, rootHash, localHash, oldHash plumbing.Hash, branch plumbing.ReferenceName, @@ -147,44 +167,32 @@ func performPush( return fmt.Errorf("failed to create packfile: %w", err) } - // Create reference update request - req := packp.NewReferenceUpdateRequest() - if err := req.Capabilities.Set(capability.ReportStatus); err != nil { - return fmt.Errorf("failed to set capability: %w", err) - } - req.Packfile = packfileData - - // Use oldHash (either remoteHash or ZeroHash) as the expected "old" value - // This tells Git what we expect the current state to be - cmd := &packp.Command{ - Name: branch, - Old: oldHash, - New: localHash, - } - req.Commands = []*packp.Command{cmd} - - // Send request via session - logger.Info("Sending packfile via ReceivePack", "objects", len(objectsToSend)) - rs, err := session.ReceivePack(ctx, req) - if err != nil { - logger.Error(err, "ReceivePack failed") - return fmt.Errorf("failed to receive pack: %w", err) - } - - // Check for errors in response - if err := rs.Error(); err != nil { - logger.Error(err, "Push rejected by server") - return fmt.Errorf("push rejected: %w", err) - } - - // Check command status - if len(rs.CommandStatuses) > 0 { - status := rs.CommandStatuses[0] - if err := status.Error(); err != nil { - logger.Error(err, "Command status indicates failure", "ref", status.ReferenceName) - return fmt.Errorf("push failed for ref %s: %w", status.ReferenceName, err) - } - logger.Info("Command status OK", "ref", status.ReferenceName) + // Build the push request. The compare-and-swap that makes this push atomic is unchanged from + // v5: packp.Command carries the Old hash we expect the ref to be at, and the server refuses the + // update if it has moved. v6 takes the same *packp.Command type, and negotiates report-status + // itself in buildUpdateRequests, so the capability no longer has to be set by hand. + // + // Atomic asks for the receive-pack `atomic` capability when the server offers it. We send a + // single command, so it changes nothing today; it is set because the guarantee this function + // promises is exactly what the capability names, and it becomes load-bearing the moment a + // second command is added. + req := &transport.PushRequest{ + Packfile: packfileData, + Commands: []*packp.Command{{ + Name: branch, + Old: oldHash, + New: localHash, + }}, + Atomic: true, + } + + // Push on the same session the advertisement was read from. + logger.Info("Sending packfile via receive-pack", "objects", len(objectsToSend)) + if err := session.Push(ctx, repo.Storer, req); err != nil { + // v6's SendPack decodes report-status and returns the per-command rejection as this error, + // so a refused compare-and-swap arrives here rather than in a separate status struct. + logger.Error(err, "push rejected or failed", "ref", branch) + return fmt.Errorf("push failed for ref %s: %w", branch, err) } logger.Info("Push successful via single session", "branch", branch.Short(), "from", oldHash, "to", localHash) @@ -199,7 +207,7 @@ func PushAtomic( repo *git.Repository, rootHash plumbing.Hash, rootBranch plumbing.ReferenceName, // only pushes if this branch is in exact same state, e.g. refs/heads/main (HEAD not allowed since a ReceivePackSession never returns it) - auth transport.AuthMethod, + auth []gitclient.Option, ) error { if !rootBranch.IsBranch() { return errors.New("rootBranch is not a branch") diff --git a/internal/git/git_atomic_push_test.go b/internal/git/git_atomic_push_test.go index bcdcfdfc..1f81f6c6 100644 --- a/internal/git/git_atomic_push_test.go +++ b/internal/git/git_atomic_push_test.go @@ -7,7 +7,7 @@ import ( "path/filepath" "testing" - "github.com/go-git/go-git/v5/plumbing" + "github.com/go-git/go-git/v6/plumbing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) diff --git a/internal/git/git_operations_test.go b/internal/git/git_operations_test.go index 5f529ec3..39820eb4 100644 --- a/internal/git/git_operations_test.go +++ b/internal/git/git_operations_test.go @@ -7,15 +7,16 @@ import ( "errors" "fmt" "os" + "os/exec" "path/filepath" "strings" "testing" "time" - "github.com/go-git/go-git/v5" - "github.com/go-git/go-git/v5/config" - "github.com/go-git/go-git/v5/plumbing" - "github.com/go-git/go-git/v5/plumbing/object" + "github.com/go-git/go-git/v6" + "github.com/go-git/go-git/v6/config" + "github.com/go-git/go-git/v6/plumbing" + "github.com/go-git/go-git/v6/plumbing/object" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -63,6 +64,7 @@ func TestCheckRepo_ConnectivityAndMetadata(t *testing.T) { repoPath := filepath.Join(tempDir, "test-repo") repo, err := git.PlainInit(repoPath, false) require.NoError(t, err) + require.NoError(t, PinExplicitSigningPolicy(repo)) worktree, err := repo.Worktree() require.NoError(t, err) @@ -213,6 +215,7 @@ func TestCheckRepo_OrphanBranches(t *testing.T) { // Initialize as bare repository repo, err := git.PlainInit(repoPath, true) // true = bare require.NoError(t, err) + require.NoError(t, PinExplicitSigningPolicy(repo)) // Create two orphan branches by directly creating branch references // This simulates branches that exist but have no commits @@ -359,6 +362,7 @@ func TestMakeHeadUnborn_CleansWorktreeIncludingTrackedFiles(t *testing.T) { repo, err := git.PlainInit(repoPath, false) require.NoError(t, err) + require.NoError(t, PinExplicitSigningPolicy(repo)) worktree, err := repo.Worktree() require.NoError(t, err) @@ -480,16 +484,26 @@ func TestBranchWorker_ConflictResolution(t *testing.T) { func TestBranchWorker_ConcurrentOperations(t *testing.T) { // Test concurrent worker writes to simulate multiple GitDestinations. + // + // This must run against a REAL git server, not `file://`. The whole point of the test is that + // racing pushes are rejected by the Old/New compare-and-swap so our retry serialises them, and + // go-git v6's in-process receive-pack (which now backs `file://`, where v5 spawned the real git + // binary) never compares cmd.Old — it just sets the reference. Over `file://` all three pushes + // would "succeed", last write wins, and this test would pass vacuously with 2 commits. + // See startRealGitServer. tempDir := t.TempDir() // Create shared bare remote repository remotePath := filepath.Join(tempDir, "remote.git") createBareRepo(t, remotePath) + require.NoError(t, exec.Command("git", "-C", remotePath, "config", "http.receivepack", "true").Run()) // Simulate client creating initial commit. README.md is a recognized operator artifact, // so it does not trip the operator-exclusive-subtree refusal. simulateClientCommitOnDisk(t, "file://"+remotePath, "main", "README.md", "init") + remoteURL := startRealGitServer(t, tempDir, remotePath).RepoURL + // Number of concurrent operations numWorkers := 3 results := make(chan error, numWorkers) @@ -499,7 +513,7 @@ func TestBranchWorker_ConcurrentOperations(t *testing.T) { go func(workerID int) { // Each worker gets its own clone worker, err := newTestBranchWorker( - "file://"+remotePath, + remoteURL, fmt.Sprintf("test-repo-%d", workerID), "main", ) @@ -1098,3 +1112,40 @@ func BenchmarkPrepareBranch_ShallowClone(b *testing.B) { } // Benchmark for writing the first commit to an empty repository. + +// PrepareBranch must pin the signing policy on repositories it REUSES, not only on ones it creates. +// The controller keeps worker clones on a volume across restarts, and repositories created before the +// pin existed are the common case on upgrade — either would otherwise hit go-git v6's +// "cannot auto-sign commit" the first time an ambient commit.gpgSign is true. +func TestPrepareBranch_PinsSigningPolicyOnReusedRepository(t *testing.T) { + tempDir := t.TempDir() + + remotePath := filepath.Join(tempDir, "remote.git") + createBareRepo(t, remotePath) + simulateClientCommitOnDisk(t, "file://"+remotePath, "main", "README.md", "init") + + repoPath := filepath.Join(tempDir, "worker") + + // First call creates the repository. + _, err := PrepareBranch(context.Background(), "file://"+remotePath, repoPath, "main", nil) + require.NoError(t, err) + + // Simulate a repository that predates the pin, or an ambient setting arriving later. + reopened, err := git.PlainOpen(repoPath) + require.NoError(t, err) + cfg, err := reopened.Config() + require.NoError(t, err) + cfg.Commit.GpgSign = config.NewOptBool(true) + require.NoError(t, reopened.SetConfig(cfg)) + + // Second call reuses it, and must re-pin. + _, err = PrepareBranch(context.Background(), "file://"+remotePath, repoPath, "main", nil) + require.NoError(t, err) + + reopened, err = git.PlainOpen(repoPath) + require.NoError(t, err) + cfg, err = reopened.Config() + require.NoError(t, err) + assert.Equal(t, config.OptBoolFalse, cfg.Commit.GpgSign, + "a reused repository must have the signing policy pinned too") +} diff --git a/internal/git/git_smart_fetch.go b/internal/git/git_smart_fetch.go index 02a90a79..5458afb1 100644 --- a/internal/git/git_smart_fetch.go +++ b/internal/git/git_smart_fetch.go @@ -7,10 +7,11 @@ import ( "errors" "fmt" - "github.com/go-git/go-git/v5" - "github.com/go-git/go-git/v5/config" - "github.com/go-git/go-git/v5/plumbing" - "github.com/go-git/go-git/v5/plumbing/transport" + "github.com/go-git/go-git/v6" + "github.com/go-git/go-git/v6/config" + "github.com/go-git/go-git/v6/plumbing" + gitclient "github.com/go-git/go-git/v6/plumbing/client" + "github.com/go-git/go-git/v6/plumbing/transport" "sigs.k8s.io/controller-runtime/pkg/log" ) @@ -25,7 +26,7 @@ func SmartFetch( ctx context.Context, repo *git.Repository, target plumbing.ReferenceName, // e.g. "refs/heads/feature" or "HEAD" - auth transport.AuthMethod, + auth []gitclient.Option, ) (plumbing.ReferenceName, error) { remoteName := "origin" remote, err := repo.Remote(remoteName) @@ -62,12 +63,12 @@ func SmartFetch( // 4. Execute: Fetch if len(refSpecs) > 0 { err = repo.Fetch(&git.FetchOptions{ - RemoteName: remoteName, - Auth: auth, - RefSpecs: refSpecs, - Depth: 1, - Force: true, - Prune: true, + RemoteName: remoteName, + ClientOptions: auth, + RefSpecs: refSpecs, + Depth: 1, + Force: true, + Prune: true, }) if err != nil && !errors.Is(err, git.NoErrAlreadyUpToDate) { return "", fmt.Errorf("smart fetch failed: %w", err) @@ -80,8 +81,8 @@ func SmartFetch( return result, nil } -func listRemoteRefs(remote *git.Remote, auth transport.AuthMethod) ([]*plumbing.Reference, error) { - refs, err := remote.List(&git.ListOptions{Auth: auth}) +func listRemoteRefs(remote *git.Remote, auth []gitclient.Option) ([]*plumbing.Reference, error) { + refs, err := remote.List(&git.ListOptions{ClientOptions: auth}) if errors.Is(err, transport.ErrEmptyRemoteRepository) { return nil, nil // Valid state, not an error } diff --git a/internal/git/helpers.go b/internal/git/helpers.go index 5be782bf..abbdae2e 100644 --- a/internal/git/helpers.go +++ b/internal/git/helpers.go @@ -8,7 +8,7 @@ import ( "fmt" "strings" - gogit "github.com/go-git/go-git/v5" + gogit "github.com/go-git/go-git/v6" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" diff --git a/internal/git/helpers_test.go b/internal/git/helpers_test.go index 5795801d..2e548ba2 100644 --- a/internal/git/helpers_test.go +++ b/internal/git/helpers_test.go @@ -12,11 +12,11 @@ import ( "testing" "time" - "github.com/go-git/go-git/v5" - "github.com/go-git/go-git/v5/config" - "github.com/go-git/go-git/v5/plumbing" - "github.com/go-git/go-git/v5/plumbing/object" - "github.com/go-git/go-git/v5/plumbing/transport" + "github.com/go-git/go-git/v6" + "github.com/go-git/go-git/v6/config" + "github.com/go-git/go-git/v6/plumbing" + "github.com/go-git/go-git/v6/plumbing/object" + "github.com/go-git/go-git/v6/plumbing/transport" "github.com/go-logr/logr" "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -43,7 +43,7 @@ func initLocalRepo( tb.Helper() // --- 1. Clone or Init --- - repo, err := git.PlainClone(localPath, false, &git.CloneOptions{ + repo, err := git.PlainClone(localPath, &git.CloneOptions{ URL: remoteURL, }) @@ -62,6 +62,11 @@ func initLocalRepo( require.NoError(tb, err) } + // go-git v6 refuses to commit when an ambient commit.gpgSign is true and no signer is + // registered, and a developer machine commonly sets it globally. Production pins the same + // setting when it initialises a repository; do it here so tests are hermetic. + require.NoError(tb, PinExplicitSigningPolicy(repo)) + worktree, err := repo.Worktree() require.NoError(tb, err) @@ -130,7 +135,6 @@ func commitFileChange(tb testing.TB, worktree *git.Worktree, repoFolder, file, c _, err = worktree.Add(file) require.NoError(tb, err) - // Commit createdHash, err := worktree.Commit("Client commit", &git.CommitOptions{ Author: &object.Signature{Name: "Client", Email: "client@example.com", When: time.Now()}, }) @@ -178,6 +182,7 @@ func createBareRepo(tb testing.TB, path string) *git.Repository { repo, err := git.PlainInit(path, true) // true = bare require.NoError(tb, err) + require.NoError(tb, PinExplicitSigningPolicy(repo)) setHeadToMain(repo) @@ -257,10 +262,11 @@ func simulateSimpleMerge(tb testing.TB, repoURL, srcBranchShort, dstBranchShort sourceFilesDir := filepath.Join(tempDir, "source-files") // Clone the repository - repo, err := git.PlainClone(localPath, false, &git.CloneOptions{ + repo, err := git.PlainClone(localPath, &git.CloneOptions{ URL: repoURL, }) require.NoError(tb, err) + require.NoError(tb, PinExplicitSigningPolicy(repo)) worktree, err := repo.Worktree() require.NoError(tb, err) diff --git a/internal/git/inplace_edit_test.go b/internal/git/inplace_edit_test.go index 3a693553..7e46ebf5 100644 --- a/internal/git/inplace_edit_test.go +++ b/internal/git/inplace_edit_test.go @@ -8,7 +8,7 @@ import ( "path/filepath" "testing" - gogit "github.com/go-git/go-git/v5" + gogit "github.com/go-git/go-git/v6" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -41,6 +41,7 @@ func newWorktreeForTest(t *testing.T) *gogit.Worktree { t.Helper() repo, err := gogit.PlainInit(t.TempDir(), false) require.NoError(t, err) + require.NoError(t, PinExplicitSigningPolicy(repo)) worktree, err := repo.Worktree() require.NoError(t, err) return worktree @@ -79,7 +80,7 @@ func applyEventsViaPlanFlushWithMapper( func TestPlanFlush_PreservesHandAuthoredFormatting(t *testing.T) { writer := newContentWriter(types.SensitiveResourcePolicy{}) worktree := newWorktreeForTest(t) - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() event := inplaceCMEvent("green") relPath := writer.filePathForIdentifier(event.Identifier) @@ -106,7 +107,7 @@ func TestPlanFlush_PreservesHandAuthoredFormatting(t *testing.T) { func TestPlanFlush_PreservesKustomizeNamespaceStyle(t *testing.T) { writer := newContentWriter(types.SensitiveResourcePolicy{}) worktree := newWorktreeForTest(t) - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() relPath := "apps/bundle.yaml" full := filepath.Join(root, relPath) @@ -148,7 +149,7 @@ func TestPlanFlush_PreservesKustomizeNamespaceStyle(t *testing.T) { func TestPlanFlush_AppliesChangeToCanonicalFile(t *testing.T) { writer := newContentWriter(types.SensitiveResourcePolicy{}) worktree := newWorktreeForTest(t) - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() event := inplaceCMEvent("green") relPath := writer.filePathForIdentifier(event.Identifier) @@ -176,7 +177,7 @@ func TestPlanFlush_AppliesChangeToCanonicalFile(t *testing.T) { func TestPlanFlush_IdenticalUpdateIsNoOp(t *testing.T) { writer := newContentWriter(types.SensitiveResourcePolicy{}) worktree := newWorktreeForTest(t) - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() event := inplaceCMEvent("blue") relPath := writer.filePathForIdentifier(event.Identifier) diff --git a/internal/git/inplace_overrides_test.go b/internal/git/inplace_overrides_test.go index 48789968..4a804bf3 100644 --- a/internal/git/inplace_overrides_test.go +++ b/internal/git/inplace_overrides_test.go @@ -120,7 +120,7 @@ func assertFileBytes(t *testing.T, path, want, msg string) { func TestPlanFlush_RoutesImageTagToKustomizationEntry(t *testing.T) { writer := newContentWriter(types.SensitiveResourcePolicy{}) worktree := newWorktreeForTest(t) - deployPath, kustPath := seedOverridesWorktree(t, worktree.Filesystem.Root()) + deployPath, kustPath := seedOverridesWorktree(t, worktree.Filesystem().Root()) changed := applyEventsViaPlanFlushWithMapper(t, writer, worktree, deploymentMapper(), overridesDeploymentEvent("ghcr.io/example/podinfo:6.5.0", 3)) @@ -141,7 +141,7 @@ func TestPlanFlush_RoutesImageTagToKustomizationEntry(t *testing.T) { func TestPlanFlush_LiveMatchingOverlayRenderIsNoOp(t *testing.T) { writer := newContentWriter(types.SensitiveResourcePolicy{}) worktree := newWorktreeForTest(t) - deployPath, kustPath := seedOverridesWorktree(t, worktree.Filesystem.Root()) + deployPath, kustPath := seedOverridesWorktree(t, worktree.Filesystem().Root()) changed := applyEventsViaPlanFlushWithMapper(t, writer, worktree, deploymentMapper(), overridesDeploymentEvent("ghcr.io/example/podinfo:6.4.0", 3)) @@ -157,7 +157,7 @@ func TestPlanFlush_LiveMatchingOverlayRenderIsNoOp(t *testing.T) { func TestPlanFlush_RoutesReplicaCountToKustomizationEntry(t *testing.T) { writer := newContentWriter(types.SensitiveResourcePolicy{}) worktree := newWorktreeForTest(t) - deployPath, kustPath := seedOverridesWorktree(t, worktree.Filesystem.Root()) + deployPath, kustPath := seedOverridesWorktree(t, worktree.Filesystem().Root()) changed := applyEventsViaPlanFlushWithMapper(t, writer, worktree, deploymentMapper(), overridesDeploymentEvent("ghcr.io/example/podinfo:6.4.0", 5)) @@ -177,7 +177,7 @@ func TestPlanFlush_RoutesReplicaCountToKustomizationEntry(t *testing.T) { func TestPlanFlush_RoutesScaleFieldPatchToKustomizationEntry(t *testing.T) { writer := newContentWriter(types.SensitiveResourcePolicy{}) worktree := newWorktreeForTest(t) - deployPath, kustPath := seedOverridesWorktree(t, worktree.Filesystem.Root()) + deployPath, kustPath := seedOverridesWorktree(t, worktree.Filesystem().Root()) scale := Event{ Identifier: types.ResourceIdentifier{ @@ -237,7 +237,7 @@ func TestApplyOverrideEdits_SkipLeavesBuffersUntouched(t *testing.T) { func TestResync_GovernedFolderInSyncIsNoOp(t *testing.T) { writer := newContentWriter(types.SensitiveResourcePolicy{}) worktree := newWorktreeForTest(t) - deployPath, kustPath := seedOverridesWorktree(t, worktree.Filesystem.Root()) + deployPath, kustPath := seedOverridesWorktree(t, worktree.Filesystem().Root()) stats, changed := applyResyncViaWorktree(t, writer, deploymentMapper(), worktree, desiredOverridesDeployment("ghcr.io/example/podinfo:6.4.0", 3)) @@ -252,7 +252,7 @@ func TestResync_GovernedFolderInSyncIsNoOp(t *testing.T) { func TestResync_GovernedDriftRoutesToKustomizationEntry(t *testing.T) { writer := newContentWriter(types.SensitiveResourcePolicy{}) worktree := newWorktreeForTest(t) - deployPath, kustPath := seedOverridesWorktree(t, worktree.Filesystem.Root()) + deployPath, kustPath := seedOverridesWorktree(t, worktree.Filesystem().Root()) stats, changed := applyResyncViaWorktree(t, writer, deploymentMapper(), worktree, desiredOverridesDeployment("ghcr.io/example/podinfo:6.5.0", 3)) @@ -277,7 +277,7 @@ func desiredOverridesDeployment(image string, replicas int64) manifestanalyzer.D func TestPlanFlush_UngovernedChangeStillPatchesSourceFile(t *testing.T) { writer := newContentWriter(types.SensitiveResourcePolicy{}) worktree := newWorktreeForTest(t) - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() deployPath, kustPath := seedOverridesWorktree(t, root) // Repoint the images entry at an image this Deployment does not use. ungoverned := "apiVersion: kustomize.config.k8s.io/v1beta1\n" + diff --git a/internal/git/known_placement_bugs_test.go b/internal/git/known_placement_bugs_test.go index 3c853f2c..60e95941 100644 --- a/internal/git/known_placement_bugs_test.go +++ b/internal/git/known_placement_bugs_test.go @@ -22,7 +22,7 @@ import ( "path/filepath" "testing" - gogit "github.com/go-git/go-git/v5" + gogit "github.com/go-git/go-git/v6" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -41,7 +41,7 @@ const placedManifestBlue = "apiVersion: v1\nkind: ConfigMap\n" + // absolute path so callers can assert on the file afterwards. func seedPlacedManifest(t *testing.T, worktree *gogit.Worktree, relPath, content string) string { t.Helper() - full := filepath.Join(worktree.Filesystem.Root(), relPath) + full := filepath.Join(worktree.Filesystem().Root(), relPath) require.NoError(t, os.MkdirAll(filepath.Dir(full), 0o750)) require.NoError(t, os.WriteFile(full, []byte(content), 0o600)) _, err := worktree.Add(relPath) @@ -66,7 +66,7 @@ func TestPlanFlush_UpdateFollowsExistingPlacement(t *testing.T) { assert.Contains(t, string(placedAfter), "color: green", "the update must land in the existing manifest at apps/foo.yaml") - canonicalFull := filepath.Join(worktree.Filesystem.Root(), writer.filePathForIdentifier(event.Identifier)) + canonicalFull := filepath.Join(worktree.Filesystem().Root(), writer.filePathForIdentifier(event.Identifier)) _, statErr := os.Stat(canonicalFull) assert.Truef(t, os.IsNotExist(statErr), "no duplicate copy must be created at the canonical path %s", canonicalFull) @@ -98,7 +98,7 @@ func TestPlanFlush_DeleteFollowsExistingPlacement(t *testing.T) { func TestPlanFlush_NoOpInMultiDocReportsNoChange(t *testing.T) { writer := newContentWriter(types.SensitiveResourcePolicy{}) worktree := newWorktreeForTest(t) - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() event := inplaceCMEvent("blue") full := filepath.Join(root, placedManifestPath) @@ -135,7 +135,7 @@ func TestPlanFlush_NoOpInMultiDocReportsNoChange(t *testing.T) { func TestPlanFlush_MultiDocCanonicalDoesNotDropSiblings(t *testing.T) { writer := newContentWriter(types.SensitiveResourcePolicy{}) worktree := newWorktreeForTest(t) - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() event := inplaceCMEvent("green") relPath := writer.filePathForIdentifier(event.Identifier) diff --git a/internal/git/kustomize_delete_test.go b/internal/git/kustomize_delete_test.go index c121ceae..4c6fdf3c 100644 --- a/internal/git/kustomize_delete_test.go +++ b/internal/git/kustomize_delete_test.go @@ -8,7 +8,7 @@ import ( "path/filepath" "testing" - gogit "github.com/go-git/go-git/v5" + gogit "github.com/go-git/go-git/v6" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/runtime/schema" @@ -100,7 +100,7 @@ func mergedMapper() typeset.Lookup { func TestPlanFlush_DeletingOneDocumentOfAFileKeepsTheFileAndItsResourcesEntry(t *testing.T) { writer := newContentWriter(types.SensitiveResourcePolicy{}) worktree := newWorktreeForTest(t) - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() seedDeleteWorktree(t, worktree, deleteKustomizationYAML) changed, err := flushEventsForTest(t, writer, worktree, configMapMapper(), @@ -125,7 +125,7 @@ func TestPlanFlush_DeletingOneDocumentOfAFileKeepsTheFileAndItsResourcesEntry(t func TestPlanFlush_DeletingTheLastDocumentAlsoRemovesTheResourcesEntry(t *testing.T) { writer := newContentWriter(types.SensitiveResourcePolicy{}) worktree := newWorktreeForTest(t) - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() seedDeleteWorktree(t, worktree, deleteKustomizationYAML) changed, err := flushEventsForTest(t, writer, worktree, configMapMapper(), @@ -157,7 +157,7 @@ func TestPlanFlush_DeletingTheLastDocumentAlsoRemovesTheResourcesEntry(t *testin func TestPlanFlush_DeleteInsideARenderRootIsVerified(t *testing.T) { writer := newContentWriter(types.SensitiveResourcePolicy{}) worktree := newWorktreeForTest(t) - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() seedDeleteWorktree(t, worktree, deleteKustomizationYAML) scan, err := scanWorktreeSubtree(root) @@ -175,7 +175,7 @@ func TestPlanFlush_DeleteInsideARenderRootIsVerified(t *testing.T) { func TestPlanFlush_DeleteAndGovernedWriteInOneFlush(t *testing.T) { writer := newContentWriter(types.SensitiveResourcePolicy{}) worktree := newWorktreeForTest(t) - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() seedDeleteWorktree(t, worktree, deleteKustomizationYAML+`images: - name: ghcr.io/example/shared newTag: "1.0.0" diff --git a/internal/git/kustomize_oracle_test.go b/internal/git/kustomize_oracle_test.go index 392cba5b..c8643c03 100644 --- a/internal/git/kustomize_oracle_test.go +++ b/internal/git/kustomize_oracle_test.go @@ -10,7 +10,7 @@ import ( "strings" "testing" - gogit "github.com/go-git/go-git/v5" + gogit "github.com/go-git/go-git/v6" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -129,7 +129,7 @@ func sharedImageEvent(name, image string) Event { //nolint:unparam // image vari func TestPlanFlush_RefusesAWriteThatDragsASiblingAlong(t *testing.T) { writer := newContentWriter(types.SensitiveResourcePolicy{}) worktree := newWorktreeForTest(t) - webPath, apiPath, kustPath := seedSharedEntryWorktree(t, worktree.Filesystem.Root()) + webPath, apiPath, kustPath := seedSharedEntryWorktree(t, worktree.Filesystem().Root()) _, err := flushEventsForTest(t, writer, worktree, deploymentMapper(), sharedImageEvent("web", "ghcr.io/example/shared:2.0.0")) @@ -164,7 +164,7 @@ func TestPlanFlush_RefusesAWriteThatDragsASiblingAlong(t *testing.T) { func TestPlanFlush_NewResourceInANamespaceInheritingDirIsNotCollateralDamage(t *testing.T) { writer := newContentWriter(types.SensitiveResourcePolicy{}) worktree := newWorktreeForTest(t) - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() // FLAT on purpose: the kustomization sits beside the manifests it lists, so a new // document lands in the same directory AND gets a resources: entry — which is what puts @@ -234,7 +234,7 @@ func findFileContaining(t *testing.T, root, needle string) string { func TestPlanFlush_RefusesANewResourceAnEntryWouldOverride(t *testing.T) { writer := newContentWriter(types.SensitiveResourcePolicy{}) worktree := newWorktreeForTest(t) - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() require.NoError(t, os.WriteFile(filepath.Join(root, "web.yaml"), []byte(sharedImageDeploymentYAML("web")), 0o600)) @@ -293,7 +293,7 @@ func newCacheDeploymentEvent(image string) Event { func TestPlanFlush_AllowsTheSharedEntryWriteWhenEverySiblingAgrees(t *testing.T) { writer := newContentWriter(types.SensitiveResourcePolicy{}) worktree := newWorktreeForTest(t) - webPath, apiPath, kustPath := seedSharedEntryWorktree(t, worktree.Filesystem.Root()) + webPath, apiPath, kustPath := seedSharedEntryWorktree(t, worktree.Filesystem().Root()) changed := applyEventsViaPlanFlushWithMapper(t, writer, worktree, deploymentMapper(), sharedImageEvent("web", "ghcr.io/example/shared:2.0.0"), diff --git a/internal/git/patches_test.go b/internal/git/patches_test.go index 95837cae..1cca2303 100644 --- a/internal/git/patches_test.go +++ b/internal/git/patches_test.go @@ -63,7 +63,7 @@ func seedPatchedWorktree(t *testing.T, root string) (string, string, string) { func TestPlanFlush_PatchedFolderStillRoutesAnImageBumpToTheEntry(t *testing.T) { writer := newContentWriter(types.SensitiveResourcePolicy{}) worktree := newWorktreeForTest(t) - deployPath, patchPath, kustPath := seedPatchedWorktree(t, worktree.Filesystem.Root()) + deployPath, patchPath, kustPath := seedPatchedWorktree(t, worktree.Filesystem().Root()) // The live object is what the folder renders: the patch's 5 replicas, the entry's 6.4.0 tag — // with the tag bumped to 6.5.0, which is the user's edit. @@ -88,7 +88,7 @@ func TestPlanFlush_PatchedFolderStillRoutesAnImageBumpToTheEntry(t *testing.T) { func TestPlanFlush_PatchedFolderInSyncIsANoOp(t *testing.T) { writer := newContentWriter(types.SensitiveResourcePolicy{}) worktree := newWorktreeForTest(t) - deployPath, patchPath, kustPath := seedPatchedWorktree(t, worktree.Filesystem.Root()) + deployPath, patchPath, kustPath := seedPatchedWorktree(t, worktree.Filesystem().Root()) changed, err := flushEventsForTest(t, writer, worktree, deploymentMapper(), overridesDeploymentEvent("ghcr.io/example/podinfo:6.4.0", 5)) @@ -112,7 +112,7 @@ func TestPlanFlush_PatchedFolderInSyncIsANoOp(t *testing.T) { func TestPlanFlush_RefusesAnEditToAFieldThePatchOwns(t *testing.T) { writer := newContentWriter(types.SensitiveResourcePolicy{}) worktree := newWorktreeForTest(t) - deployPath, patchPath, kustPath := seedPatchedWorktree(t, worktree.Filesystem.Root()) + deployPath, patchPath, kustPath := seedPatchedWorktree(t, worktree.Filesystem().Root()) _, err := flushEventsForTest(t, writer, worktree, deploymentMapper(), overridesDeploymentEvent("ghcr.io/example/podinfo:6.4.0", 9)) diff --git a/internal/git/placement_metrics_test.go b/internal/git/placement_metrics_test.go index fac9c38f..8b638b2e 100644 --- a/internal/git/placement_metrics_test.go +++ b/internal/git/placement_metrics_test.go @@ -7,7 +7,7 @@ import ( "os" "testing" - gogit "github.com/go-git/go-git/v5" + gogit "github.com/go-git/go-git/v6" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" diff --git a/internal/git/placement_test.go b/internal/git/placement_test.go index d716e0a7..4d7ff578 100644 --- a/internal/git/placement_test.go +++ b/internal/git/placement_test.go @@ -9,7 +9,7 @@ import ( "strings" "testing" - gogit "github.com/go-git/go-git/v5" + gogit "github.com/go-git/go-git/v6" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -49,7 +49,7 @@ func applyEventsWithPolicy( func TestPlacement_DeclaredPolicy_NewFile(t *testing.T) { worktree := newWorktreeForTest(t) - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() policy := &manifestanalyzer.PlacementPolicy{ ByType: map[string]string{"v1/configmaps": "{namespace}/configmaps.yaml"}, } @@ -70,7 +70,7 @@ func TestPlacement_DeclaredPolicy_NewFile(t *testing.T) { // it if the repository wants it in the overlay. func TestPlacement_ExistingSiblingFile_DoesNotAttractTheNewFile(t *testing.T) { worktree := newWorktreeForTest(t) - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() seedPlacedManifest(t, worktree, "overlays/test/configmap-existing.yaml", "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: existing\n namespace: podinfo-test\ndata:\n k: v\n") @@ -180,7 +180,7 @@ func TestPlacement_SensitiveCollision_SkipsWithoutCrashing(t *testing.T) { func TestPlacement_KustomizeEntryAppended_SameCommit(t *testing.T) { worktree := newWorktreeForTest(t) - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() kustYAML := "# overlay for test\n" + "namespace: podinfo-test\n" + "resources:\n" + @@ -206,7 +206,7 @@ func TestPlacement_KustomizeEntryAppended_SameCommit(t *testing.T) { func TestPlacement_KustomizeEntryIdempotent_OnRepeatedApply(t *testing.T) { worktree := newWorktreeForTest(t) - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() kustYAML := "namespace: podinfo-test\nresources:\n - deployment.yaml\n" seedPlacedManifest(t, worktree, "overlays/test/kustomization.yaml", kustYAML) seedPlacedManifest(t, worktree, "overlays/test/deployment.yaml", @@ -230,7 +230,7 @@ func TestPlacement_KustomizeEntryIdempotent_OnRepeatedApply(t *testing.T) { // was for a human to complete. func TestPlacement_KustomizeEntryAppendSkipped_NoResourcesSequence(t *testing.T) { worktree := newWorktreeForTest(t) - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() kustYAML := "namespace: app\n" seedPlacedManifest(t, worktree, "overlays/test/kustomization.yaml", kustYAML) @@ -275,7 +275,7 @@ func TestPlacement_UndecodableKustomization_RefusesTheFlush(t *testing.T) { // resources: entry" row of render-root-scoping.md §4. func TestPlacement_ExternalBaseOverlay_NewObject(t *testing.T) { worktree := newWorktreeForTest(t) - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() // The read-only base, outside the overlay's own subtree. seedPlacedManifest(t, worktree, "base/kustomization.yaml", "resources:\n - deployment.yaml\n") @@ -371,7 +371,7 @@ func flushOverlayDeployment(t *testing.T, worktree *gogit.Worktree, event Event) // verified by the re-render oracle. Before this the flush was refused for escaping the write jail. func TestOverlayAuthors_ImageEntry_ForBaseSuppliedImage(t *testing.T) { worktree := overlayBaseDeploymentWorktree(t, "nginx:1.0", "") - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() require.NoError(t, flushOverlayDeployment(t, worktree, liveDeployment("nginx:2.0", -1)), "an image bump must be authored as an overlay images: entry, not refused") @@ -391,7 +391,7 @@ func TestOverlayAuthors_ImageEntry_ForBaseSuppliedImage(t *testing.T) { // overlay: the overlay authors a replicas: entry over the base's count, base untouched. func TestOverlayAuthors_ReplicaEntry_ForBaseSuppliedCount(t *testing.T) { worktree := overlayBaseDeploymentWorktree(t, "nginx:1.0", "2") - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() require.NoError(t, flushOverlayDeployment(t, worktree, liveDeployment("nginx:1.0", 5)), "a scale must be authored as an overlay replicas: entry, not refused") @@ -411,7 +411,7 @@ func TestOverlayAuthors_ReplicaEntry_ForBaseSuppliedCount(t *testing.T) { // authored: the store now sees the entry, so the change routes to it (no duplicate entry). func TestOverlayAuthors_Idempotent_OnResync(t *testing.T) { worktree := overlayBaseDeploymentWorktree(t, "nginx:1.0", "") - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() require.NoError(t, flushOverlayDeployment(t, worktree, liveDeployment("nginx:2.0", -1))) // A second flush of the same live state must not append a second entry. @@ -428,7 +428,7 @@ func TestOverlayAuthors_Idempotent_OnResync(t *testing.T) { // re-render oracle proves the object leaves the render, and the read-only base is untouched. func TestOverlayAuthors_DeletePatch_ForInheritedObject(t *testing.T) { worktree := newWorktreeForTest(t) - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() seedPlacedManifest(t, worktree, "base/kustomization.yaml", "resources:\n - cm.yaml\n") seedPlacedManifest(t, worktree, "base/cm.yaml", "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: shared\n namespace: podinfo-test\ndata:\n k: v\n") @@ -473,7 +473,7 @@ func TestOverlayAuthors_DeletePatch_ForInheritedObject(t *testing.T) { // the delete and leaves the existing file byte-for-byte, rather than overwriting it. func TestOverlayAuthors_DeletePatch_SkipsOnPathCollision(t *testing.T) { worktree := newWorktreeForTest(t) - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() seedPlacedManifest(t, worktree, "base/kustomization.yaml", "resources:\n - cm.yaml\n") seedPlacedManifest(t, worktree, "base/cm.yaml", "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: shared\n namespace: podinfo-test\ndata:\n k: v\n") @@ -571,13 +571,13 @@ func TestPlacement_ColdBundleCollision_BothSurviveRegardlessOfOrder(t *testing.T forward := newWorktreeForTest(t) changed := applyEventsWithPolicy(t, forward, policy, first, second) require.True(t, changed) - forwardBody, err := os.ReadFile(filepath.Join(forward.Filesystem.Root(), "all.yaml")) + forwardBody, err := os.ReadFile(filepath.Join(forward.Filesystem().Root(), "all.yaml")) require.NoError(t, err) reversed := newWorktreeForTest(t) changed = applyEventsWithPolicy(t, reversed, policy, second, first) require.True(t, changed) - reversedBody, err := os.ReadFile(filepath.Join(reversed.Filesystem.Root(), "all.yaml")) + reversedBody, err := os.ReadFile(filepath.Join(reversed.Filesystem().Root(), "all.yaml")) require.NoError(t, err) assert.Contains(t, string(forwardBody), "name: alpha", "the first resource must survive") @@ -600,7 +600,7 @@ func TestPlacement_ColdBundleCollision_ThreeResourcesAllSurvive(t *testing.T) { ) require.True(t, changed) - got, err := os.ReadFile(filepath.Join(worktree.Filesystem.Root(), "all.yaml")) + got, err := os.ReadFile(filepath.Join(worktree.Filesystem().Root(), "all.yaml")) require.NoError(t, err) body := string(got) assert.Equal(t, 3, strings.Count(body, "kind: ConfigMap")) @@ -644,7 +644,7 @@ func TestPlacement_ColdBundleCollision_SensitiveNeverMerged(t *testing.T) { require.NoError(t, err) assert.True(t, changed, "the first secret must still be written") - got, readErr := os.ReadFile(filepath.Join(worktree.Filesystem.Root(), "secrets/app.sops.yaml")) + got, readErr := os.ReadFile(filepath.Join(worktree.Filesystem().Root(), "secrets/app.sops.yaml")) require.NoError(t, readErr) assert.Equal(t, 1, strings.Count(string(got), "kind: Secret"), "the second secret must be skipped, never merged into the first's file") @@ -685,7 +685,7 @@ func TestPlacement_ColdBundleCollision_SensitiveAndPlaintextNeverMix(t *testing. context.Background(), secretFirst, "", []Event{secretEvent, configMapEvent}, policy, v1alpha3.PruneOnEvent, ) require.NoError(t, err) - secretFirstBody, readErr := os.ReadFile(filepath.Join(secretFirst.Filesystem.Root(), "all.yaml")) + secretFirstBody, readErr := os.ReadFile(filepath.Join(secretFirst.Filesystem().Root(), "all.yaml")) require.NoError(t, readErr) assert.Contains(t, string(secretFirstBody), "kind: Secret") assert.NotContains(t, string(secretFirstBody), "kind: ConfigMap", @@ -698,7 +698,7 @@ func TestPlacement_ColdBundleCollision_SensitiveAndPlaintextNeverMix(t *testing. context.Background(), configMapFirst, "", []Event{configMapEvent, secretEvent}, policy, v1alpha3.PruneOnEvent, ) require.NoError(t, err) - configMapFirstBody, readErr := os.ReadFile(filepath.Join(configMapFirst.Filesystem.Root(), "all.yaml")) + configMapFirstBody, readErr := os.ReadFile(filepath.Join(configMapFirst.Filesystem().Root(), "all.yaml")) require.NoError(t, readErr) assert.Contains(t, string(configMapFirstBody), "kind: ConfigMap") assert.NotContains(t, string(configMapFirstBody), "kind: Secret", @@ -741,7 +741,7 @@ func TestPlacement_ColdBundleCollision_ViaResync(t *testing.T) { require.NoError(t, err) assert.True(t, changed) - got, readErr := os.ReadFile(filepath.Join(worktree.Filesystem.Root(), "all.yaml")) + got, readErr := os.ReadFile(filepath.Join(worktree.Filesystem().Root(), "all.yaml")) require.NoError(t, readErr) assert.Equal(t, 2, strings.Count(string(got), "kind: ConfigMap"), "both resync creates must survive") } diff --git a/internal/git/plan_flush.go b/internal/git/plan_flush.go index 95b9c929..bb68231f 100644 --- a/internal/git/plan_flush.go +++ b/internal/git/plan_flush.go @@ -14,7 +14,7 @@ import ( "sort" "strings" - gogit "github.com/go-git/go-git/v5" + gogit "github.com/go-git/go-git/v6" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "sigs.k8s.io/controller-runtime/pkg/log" sigsyaml "sigs.k8s.io/yaml" @@ -73,7 +73,7 @@ func (w *BranchWorker) flushEventsToWorktree( policy *manifestanalyzer.PlacementPolicy, pruneMode v1alpha3.PruneMode, ) (bool, error) { - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() scoped, err := scanRenderScope(root, base) if err != nil { return false, err diff --git a/internal/git/plan_flush_test.go b/internal/git/plan_flush_test.go index 1671d11d..d411edda 100644 --- a/internal/git/plan_flush_test.go +++ b/internal/git/plan_flush_test.go @@ -39,7 +39,7 @@ func cmEvent(op, name, color string) Event { func TestPlanFlush_CreatesNewResourceAtCanonicalPath(t *testing.T) { writer := newContentWriter(types.SensitiveResourcePolicy{}) worktree := newWorktreeForTest(t) - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() event := cmEvent("CREATE", "fresh", "green") changed := applyEventsViaPlanFlush(t, writer, worktree, event) @@ -59,7 +59,7 @@ func TestPlanFlush_CreatesNewResourceAtCanonicalPath(t *testing.T) { func TestPlanFlush_DeleteOneDocFromMultiDocKeepsSiblings(t *testing.T) { writer := newContentWriter(types.SensitiveResourcePolicy{}) worktree := newWorktreeForTest(t) - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() keep := "apiVersion: v1\nkind: ConfigMap\n" + "metadata:\n name: keep\n namespace: default\n" + @@ -146,7 +146,7 @@ func TestPlanFlush_SensitiveMovedResourceRewritesInPlaceNotCanonical(t *testing. writer := newContentWriter(types.SensitiveResourcePolicy{}) writer.setEncryptor(enc, "test-scope") worktree := newWorktreeForTest(t) - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() // A Secret moved off its canonical .sops path. It carries a cleartext identity and a // sops key, so the store indexes it as an encrypted managed document. The encrypted @@ -187,7 +187,7 @@ func TestPlanFlush_SensitiveMovedResourceRewritesInPlaceNotCanonical(t *testing. func TestPlanFlush_BatchDeleteThenUpdateSiblingTargetsCorrectDoc(t *testing.T) { writer := newContentWriter(types.SensitiveResourcePolicy{}) worktree := newWorktreeForTest(t) - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() rel := "apps/multi.yaml" first := "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: first\n namespace: default\ndata:\n k: v\n" diff --git a/internal/git/prune_mode_test.go b/internal/git/prune_mode_test.go index 26575f4f..75169dcf 100644 --- a/internal/git/prune_mode_test.go +++ b/internal/git/prune_mode_test.go @@ -10,7 +10,7 @@ import ( "strings" "testing" - gogit "github.com/go-git/go-git/v5" + gogit "github.com/go-git/go-git/v6" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -126,7 +126,7 @@ func TestPrune_NeverSuppressesBothPaths(t *testing.T) { // the same pass. Sweeping without upserting would be a different, much worse bug. func TestPrune_AlwaysReproducesMarkAndSweep(t *testing.T) { worktree := newWorktreeForTest(t) - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() orphan := seedPlacedManifest(t, worktree, "apps/orphan.yaml", cmManifest("orphan", "blue")) stats := resyncUnder(t, worktree, v1alpha3.PruneAlways, desiredCM("keep", "green")) diff --git a/internal/git/render_fidelity_test.go b/internal/git/render_fidelity_test.go index db012b85..82b4bd7b 100644 --- a/internal/git/render_fidelity_test.go +++ b/internal/git/render_fidelity_test.go @@ -8,7 +8,7 @@ import ( "path/filepath" "testing" - gogit "github.com/go-git/go-git/v5" + gogit "github.com/go-git/go-git/v6" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -80,7 +80,7 @@ func TestRenderFidelityRefusal_BlocksLiveAndResyncWrites(t *testing.T) { } { t.Run(test.name, func(t *testing.T) { worktree := newWorktreeForTest(t) - path := seedPostBuildTokenManifest(t, worktree.Filesystem.Root()) + path := seedPostBuildTokenManifest(t, worktree.Filesystem().Root()) worker := &BranchWorker{ contentWriter: newContentWriter(types.SensitiveResourcePolicy{}), mapper: configMapMapper(), diff --git a/internal/git/render_scope_test.go b/internal/git/render_scope_test.go index 843484db..68ca0515 100644 --- a/internal/git/render_scope_test.go +++ b/internal/git/render_scope_test.go @@ -13,7 +13,7 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" - gogit "github.com/go-git/go-git/v5" + gogit "github.com/go-git/go-git/v6" v1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" @@ -138,7 +138,7 @@ func flushAtBase( // under the common ancestor, and reports the overlay itself as the write jail. func TestScanRenderScope_ResolvesOverlayBase(t *testing.T) { worktree := newWorktreeForTest(t) - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() seedOverlayWorktree(t, root) scoped, err := scanRenderScope(root, overlayGitPath) @@ -165,7 +165,7 @@ func TestScanRenderScope_ResolvesOverlayBase(t *testing.T) { // spec.path and writeSubdir is empty, so nothing downstream changes. func TestScanRenderScope_SelfContainedIsIdentity(t *testing.T) { worktree := newWorktreeForTest(t) - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() seedOverridesWorktree(t, root) // kustomization + apps/deployment.yaml, all in-subtree scoped, err := scanRenderScope(root, "") @@ -180,7 +180,7 @@ func TestScanRenderScope_SelfContainedIsIdentity(t *testing.T) { func TestPlanFlush_Overlay_RoutesImageTagToOverlayEntry(t *testing.T) { writer := newContentWriter(types.SensitiveResourcePolicy{}) worktree := newWorktreeForTest(t) - baseDeployPath, overlayKustPath := seedOverlayWorktree(t, worktree.Filesystem.Root()) + baseDeployPath, overlayKustPath := seedOverlayWorktree(t, worktree.Filesystem().Root()) changed, err := flushAtBase(t, writer, worktree, deploymentMapper(), overlayGitPath, overlayDeploymentEvent("ghcr.io/example/podinfo:6.5.0")) @@ -200,7 +200,7 @@ func TestPlanFlush_Overlay_RoutesImageTagToOverlayEntry(t *testing.T) { func TestPlanFlush_Overlay_InSyncIsNoOp(t *testing.T) { writer := newContentWriter(types.SensitiveResourcePolicy{}) worktree := newWorktreeForTest(t) - baseDeployPath, overlayKustPath := seedOverlayWorktree(t, worktree.Filesystem.Root()) + baseDeployPath, overlayKustPath := seedOverlayWorktree(t, worktree.Filesystem().Root()) changed, err := flushAtBase(t, writer, worktree, deploymentMapper(), overlayGitPath, overlayDeploymentEvent("ghcr.io/example/podinfo:6.4.0")) @@ -218,7 +218,7 @@ func TestPlanFlush_Overlay_InSyncIsNoOp(t *testing.T) { func TestPlanFlush_Overlay_BaseFieldEditRefused(t *testing.T) { writer := newContentWriter(types.SensitiveResourcePolicy{}) worktree := newWorktreeForTest(t) - baseDeployPath, overlayKustPath := seedOverlayWorktree(t, worktree.Filesystem.Root()) + baseDeployPath, overlayKustPath := seedOverlayWorktree(t, worktree.Filesystem().Root()) event := overlayDeploymentEvent("ghcr.io/example/podinfo:6.4.0") // Add a base-owned field the images/replicas entries do not govern. @@ -240,7 +240,7 @@ func TestPlanFlush_Overlay_BaseFieldEditRefused(t *testing.T) { // outside the repository it manages. func TestScanRenderScope_RefusesBaseEscapingRepoRoot(t *testing.T) { worktree := newWorktreeForTest(t) - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() // spec.path is a single-level directory; ../../base climbs to ../base, above the root. kust := "apiVersion: kustomize.config.k8s.io/v1beta1\nkind: Kustomization\nresources:\n - ../../base\n" full := filepath.Join(root, "app", "kustomization.yaml") @@ -276,7 +276,7 @@ func deploymentAndConfigMapMapper() typeset.Lookup { func TestPlanFlush_Overlay_NewObjectLandsInOverlayAndRenders(t *testing.T) { writer := newContentWriter(types.SensitiveResourcePolicy{}) worktree := newWorktreeForTest(t) - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() _, overlayKustPath := seedOverlayWorktree(t, root) cm := &unstructured.Unstructured{Object: map[string]interface{}{ @@ -311,7 +311,7 @@ func TestPlanFlush_Overlay_NewObjectLandsInOverlayAndRenders(t *testing.T) { // it by reading only the first. func TestScanRenderScope_DualKustomizationFilesImportedBoth(t *testing.T) { worktree := newWorktreeForTest(t) - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() write := func(rel, content string) { full := filepath.Join(root, filepath.FromSlash(rel)) require.NoError(t, os.MkdirAll(filepath.Dir(full), 0o750)) @@ -339,7 +339,7 @@ func TestScanRenderScope_DualKustomizationFilesImportedBoth(t *testing.T) { // not read outside the worktree during scope resolution. func TestScanRenderScope_DoesNotFollowKustomizationSymlink(t *testing.T) { worktree := newWorktreeForTest(t) - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() write := func(rel, content string) { full := filepath.Join(root, filepath.FromSlash(rel)) require.NoError(t, os.MkdirAll(filepath.Dir(full), 0o750)) @@ -372,7 +372,7 @@ func TestScanRenderScope_DoesNotFollowKustomizationSymlink(t *testing.T) { // contributes only its manifests to the render). func TestScanRenderScope_TransitiveBaseAndForeignRekey(t *testing.T) { worktree := newWorktreeForTest(t) - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() write := func(rel, content string) { full := filepath.Join(root, filepath.FromSlash(rel)) require.NoError(t, os.MkdirAll(filepath.Dir(full), 0o750)) @@ -426,7 +426,7 @@ func TestScanRenderScope_TransitiveBaseAndForeignRekey(t *testing.T) { // could not load the file and the target was refused. func TestScanRenderScope_ExternalResourceFile(t *testing.T) { worktree := newWorktreeForTest(t) - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() write := func(rel, content string) { full := filepath.Join(root, filepath.FromSlash(rel)) require.NoError(t, os.MkdirAll(filepath.Dir(full), 0o750)) @@ -458,7 +458,7 @@ func TestScanRenderScope_ExternalResourceFile(t *testing.T) { // reference is left out, so it cannot wrongly refuse the target. func TestScanRenderScope_UnrelatedBaseContentNotPulled(t *testing.T) { worktree := newWorktreeForTest(t) - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() write := func(rel, content string) { full := filepath.Join(root, filepath.FromSlash(rel)) require.NoError(t, os.MkdirAll(filepath.Dir(full), 0o750)) @@ -490,7 +490,7 @@ func TestScanRenderScope_UnrelatedBaseContentNotPulled(t *testing.T) { // directory to scan — while a local base beside it is still followed. func TestScanRenderScope_SkipsRemoteBase(t *testing.T) { worktree := newWorktreeForTest(t) - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() write := func(rel, content string) { full := filepath.Join(root, filepath.FromSlash(rel)) require.NoError(t, os.MkdirAll(filepath.Dir(full), 0o750)) @@ -543,7 +543,7 @@ func TestRenderScopePathHelpers(t *testing.T) { func TestFanInPrecondition_RefusesSharedBaseWriteThroughWithoutOverrides(t *testing.T) { writer := newContentWriter(types.SensitiveResourcePolicy{}) worktree := newWorktreeForTest(t) - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() plainOverlay := "apiVersion: kustomize.config.k8s.io/v1beta1\nkind: Kustomization\n" + "namespace: shared\nresources:\n - ../base\n" diff --git a/internal/git/resync_flush.go b/internal/git/resync_flush.go index 065e83b1..b86ff4de 100644 --- a/internal/git/resync_flush.go +++ b/internal/git/resync_flush.go @@ -9,7 +9,7 @@ import ( "path/filepath" "time" - gogit "github.com/go-git/go-git/v5" + gogit "github.com/go-git/go-git/v6" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/metric" "sigs.k8s.io/controller-runtime/pkg/log" @@ -235,7 +235,7 @@ func (w *BranchWorker) executeResyncPendingWrite( return 0, err } - encryptionPath := filepath.Join(worktree.Filesystem.Root(), base) + encryptionPath := filepath.Join(worktree.Filesystem().Root(), base) if err := configureSecretEncryptionWriter(w.contentWriter, encryptionPath, target.EncryptionConfig); err != nil { return 0, fmt.Errorf("configure secret encryptor: %w", err) } @@ -287,7 +287,7 @@ func (w *BranchWorker) refuseUnsafeWorktree( worktree *gogit.Worktree, base, clusterID string, ) error { - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() scoped, err := scanRenderScope(root, base) if err != nil { return err @@ -336,7 +336,7 @@ func (w *BranchWorker) applyResyncToWorktree( // "Never" to all of them while meaning "OnEvent". Doing it at the single entry point is why no // individual reader has to remember. target.PruneMode = target.PruneMode.OrDefault() - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() scoped, err := scanRenderScope(root, base) if err != nil { return ResyncStats{}, false, err diff --git a/internal/git/resync_flush_test.go b/internal/git/resync_flush_test.go index 341d042d..2ae66d87 100644 --- a/internal/git/resync_flush_test.go +++ b/internal/git/resync_flush_test.go @@ -8,7 +8,7 @@ import ( "path/filepath" "testing" - gogit "github.com/go-git/go-git/v5" + gogit "github.com/go-git/go-git/v6" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -83,7 +83,7 @@ func applyResyncViaWorktree( func TestResync_CreatesMissingResource(t *testing.T) { writer := newContentWriter(types.SensitiveResourcePolicy{}) worktree := newWorktreeForTest(t) - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() stats, changed := applyResyncViaWorktree(t, writer, configMapMapper(), worktree, desiredCM("api", "green")) require.True(t, changed, "a missing resource must be created") @@ -268,7 +268,7 @@ func TestResync_StructureOnlyNeverDrops(t *testing.T) { func TestResync_FoldsCreateUpdateDropTogether(t *testing.T) { writer := newContentWriter(types.SensitiveResourcePolicy{}) worktree := newWorktreeForTest(t) - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() keepFull := seedPlacedManifest(t, worktree, "apps/keep.yaml", cmManifest("keep", "blue")) dropFull := seedPlacedManifest(t, worktree, "apps/drop.yaml", cmManifest("drop", "blue")) @@ -407,7 +407,7 @@ func TestResync_SensitiveUpdateCountsAsUpdatedNotSkipped(t *testing.T) { func TestResync_DropsOneDocFromMultiDocKeepsSiblings(t *testing.T) { writer := newContentWriter(types.SensitiveResourcePolicy{}) worktree := newWorktreeForTest(t) - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() rel := "apps/multi.yaml" full := filepath.Join(root, rel) seedPlacedManifest(t, worktree, rel, cmManifest("keep", "blue")+"---\n"+cmManifest("drop", "blue")) diff --git a/internal/git/resync_heal_test.go b/internal/git/resync_heal_test.go index 176a264d..8c40577a 100644 --- a/internal/git/resync_heal_test.go +++ b/internal/git/resync_heal_test.go @@ -6,7 +6,7 @@ import ( "testing" "time" - "github.com/go-git/go-git/v5/plumbing" + "github.com/go-git/go-git/v6/plumbing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/runtime/schema" diff --git a/internal/git/resync_push_test.go b/internal/git/resync_push_test.go index ad3460cd..bdfd9ee0 100644 --- a/internal/git/resync_push_test.go +++ b/internal/git/resync_push_test.go @@ -6,7 +6,7 @@ import ( "testing" "time" - "github.com/go-git/go-git/v5/plumbing" + "github.com/go-git/go-git/v6/plumbing" "github.com/go-logr/logr" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" diff --git a/internal/git/secret_write_test.go b/internal/git/secret_write_test.go index 8f0ae05c..b637c649 100644 --- a/internal/git/secret_write_test.go +++ b/internal/git/secret_write_test.go @@ -9,10 +9,10 @@ import ( "time" "filippo.io/age" - gogit "github.com/go-git/go-git/v5" - "github.com/go-git/go-git/v5/config" - "github.com/go-git/go-git/v5/plumbing" - "github.com/go-git/go-git/v5/plumbing/object" + gogit "github.com/go-git/go-git/v6" + "github.com/go-git/go-git/v6/config" + "github.com/go-git/go-git/v6/plumbing" + "github.com/go-git/go-git/v6/plumbing/object" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" diff --git a/internal/git/signing.go b/internal/git/signing.go index aa561f48..f71f4dfc 100644 --- a/internal/git/signing.go +++ b/internal/git/signing.go @@ -4,6 +4,7 @@ package git import ( "bytes" + "context" "crypto/ed25519" "crypto/rand" "encoding/pem" @@ -12,7 +13,7 @@ import ( "io" "strings" - gogit "github.com/go-git/go-git/v5" + gogit "github.com/go-git/go-git/v6" "golang.org/x/crypto/ssh" corev1 "k8s.io/api/core/v1" @@ -110,7 +111,9 @@ func loadSSHSigner(secret *corev1.Secret) (ssh.Signer, error) { return signer, nil } -func (s *sshCommitSigner) Sign(message io.Reader) ([]byte, error) { +// Sign implements go-git v6's Signer, which passes a context so a signer can reach an external +// agent or KMS. Our signing is in-process, so the context is unused. +func (s *sshCommitSigner) Sign(_ context.Context, message io.Reader) ([]byte, error) { payload, err := io.ReadAll(message) if err != nil { return nil, fmt.Errorf("read commit payload: %w", err) diff --git a/internal/git/signing_test.go b/internal/git/signing_test.go index 9ea3b311..6f921c3a 100644 --- a/internal/git/signing_test.go +++ b/internal/git/signing_test.go @@ -4,6 +4,7 @@ package git import ( "bytes" + "context" "crypto/sha512" "encoding/binary" "encoding/pem" @@ -60,7 +61,7 @@ func TestLoadSSHCommitSigner_ProducesVerifiableSSHSig(t *testing.T) { require.NoError(t, err) message := []byte("tree deadbeef\nauthor Test 1 +0000\n\nsigned commit\n") - signature, err := signer.Sign(bytes.NewReader(message)) + signature, err := signer.Sign(context.Background(), bytes.NewReader(message)) require.NoError(t, err) assert.Contains(t, string(signature), "-----BEGIN SSH SIGNATURE-----") @@ -102,7 +103,7 @@ func TestLoadSSHCommitSigner_SSHKeygenVerify(t *testing.T) { const identity = "test@example.com" payload := []byte("tree deadbeef\nauthor Test 1 +0000\n\nsigned commit\n") - sig, err := signer.Sign(bytes.NewReader(payload)) + sig, err := signer.Sign(context.Background(), bytes.NewReader(payload)) require.NoError(t, err) tmpDir := t.TempDir() @@ -181,7 +182,7 @@ func TestLoadSSHCommitSigner_PassphraseProtectedKey(t *testing.T) { signer, err := LoadSSHCommitSigner(secret) require.NoError(t, err) - signature, err := signer.Sign(bytes.NewReader([]byte("example commit payload"))) + signature, err := signer.Sign(context.Background(), bytes.NewReader([]byte("example commit payload"))) require.NoError(t, err) assert.Contains(t, string(signature), "BEGIN SSH SIGNATURE") diff --git a/internal/git/source_form_test.go b/internal/git/source_form_test.go index 0ca6ff04..7a01ccf0 100644 --- a/internal/git/source_form_test.go +++ b/internal/git/source_form_test.go @@ -112,7 +112,7 @@ func seedLabelledWorktree(t *testing.T, root string) (string, string) { func TestPlanFlush_InjectedMetadataIsNeverWrittenIntoTheSourceManifest(t *testing.T) { writer := newContentWriter(types.SensitiveResourcePolicy{}) worktree := newWorktreeForTest(t) - deployPath, kustPath := seedLabelledWorktree(t, worktree.Filesystem.Root()) + deployPath, kustPath := seedLabelledWorktree(t, worktree.Filesystem().Root()) changed, err := flushEventsForTest(t, writer, worktree, deploymentMapper(), labelledLiveDeployment("prod")) @@ -129,7 +129,7 @@ func TestPlanFlush_InjectedMetadataIsNeverWrittenIntoTheSourceManifest(t *testin func TestPlanFlush_UngovernedFieldStillLandsInTheSourceManifest(t *testing.T) { writer := newContentWriter(types.SensitiveResourcePolicy{}) worktree := newWorktreeForTest(t) - deployPath, _ := seedLabelledWorktree(t, worktree.Filesystem.Root()) + deployPath, _ := seedLabelledWorktree(t, worktree.Filesystem().Root()) event := labelledLiveDeployment("prod") containers, _, err := unstructured.NestedSlice(event.Object.Object, "spec", "template", "spec", "containers") @@ -163,7 +163,7 @@ func TestPlanFlush_UngovernedFieldStillLandsInTheSourceManifest(t *testing.T) { func TestPlanFlush_RefusesAChangeToABuildSuppliedField(t *testing.T) { writer := newContentWriter(types.SensitiveResourcePolicy{}) worktree := newWorktreeForTest(t) - deployPath, kustPath := seedLabelledWorktree(t, worktree.Filesystem.Root()) + deployPath, kustPath := seedLabelledWorktree(t, worktree.Filesystem().Root()) _, err := flushEventsForTest(t, writer, worktree, deploymentMapper(), labelledLiveDeployment("staging")) diff --git a/internal/git/types.go b/internal/git/types.go index 611a21ae..5ec8e2a9 100644 --- a/internal/git/types.go +++ b/internal/git/types.go @@ -6,8 +6,8 @@ import ( "fmt" "strings" - gogit "github.com/go-git/go-git/v5" - "github.com/go-git/go-git/v5/plumbing" + gogit "github.com/go-git/go-git/v6" + "github.com/go-git/go-git/v6/plumbing" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" diff --git a/internal/git/write_boundary_precondition_test.go b/internal/git/write_boundary_precondition_test.go index 6a3a69a7..76289d0f 100644 --- a/internal/git/write_boundary_precondition_test.go +++ b/internal/git/write_boundary_precondition_test.go @@ -9,10 +9,10 @@ import ( "testing" "time" - gogit "github.com/go-git/go-git/v5" - "github.com/go-git/go-git/v5/config" - "github.com/go-git/go-git/v5/plumbing" - "github.com/go-git/go-git/v5/plumbing/object" + gogit "github.com/go-git/go-git/v6" + "github.com/go-git/go-git/v6/config" + "github.com/go-git/go-git/v6/plumbing" + "github.com/go-git/go-git/v6/plumbing/object" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -142,7 +142,7 @@ func seedDiamond(t *testing.T, root string) { func TestFanInPrecondition_RefusesAmbiguousOverrideWriteThrough(t *testing.T) { writer := newContentWriter(types.SensitiveResourcePolicy{}) worktree := newWorktreeForTest(t) - root := worktree.Filesystem.Root() + root := worktree.Filesystem().Root() seedDiamond(t, root) w := &BranchWorker{contentWriter: writer, mapper: deploymentMapper()} diff --git a/internal/manifestanalyzer/gittargetignore.go b/internal/manifestanalyzer/gittargetignore.go index d139cc8b..fcc2c518 100644 --- a/internal/manifestanalyzer/gittargetignore.go +++ b/internal/manifestanalyzer/gittargetignore.go @@ -7,7 +7,7 @@ import ( "io/fs" "strings" - gitignore "github.com/go-git/go-git/v5/plumbing/format/gitignore" + gitignore "github.com/go-git/go-git/v6/plumbing/format/gitignore" "github.com/ConfigButler/gitops-reverser/internal/git/manifestedit" ) diff --git a/internal/ssh/auth.go b/internal/ssh/auth.go index 2a140d02..b17863d2 100644 --- a/internal/ssh/auth.go +++ b/internal/ssh/auth.go @@ -7,10 +7,13 @@ import ( "context" "errors" "fmt" + "net" "os" - "github.com/go-git/go-git/v5/plumbing/transport" - "github.com/go-git/go-git/v5/plumbing/transport/ssh" + gitclient "github.com/go-git/go-git/v6/plumbing/client" + "github.com/go-git/go-git/v6/plumbing/transport" + gogitssh "github.com/go-git/go-git/v6/plumbing/transport/ssh" + "github.com/go-git/go-git/v6/plumbing/transport/ssh/knownhosts" "github.com/go-logr/logr" gossh "golang.org/x/crypto/ssh" "sigs.k8s.io/controller-runtime/pkg/log" @@ -20,14 +23,53 @@ import ( // out of SSH host key verification when no host-key source produced any known_hosts at all. const InsecureAllowMissingKnownHostsFlag = "--insecure-allow-missing-known-hosts" -// GetAuthMethod returns an SSH public key authentication method from a private key. +// defaultHostKeyAlgorithms is offered when host key verification is disabled, so that go-git never +// has a reason to consult on-disk known_hosts files. See KeyAuth.ClientConfig. +func defaultHostKeyAlgorithms() []string { + return []string{ + gossh.KeyAlgoED25519, + gossh.CertAlgoED25519v01, + gossh.KeyAlgoECDSA256, gossh.KeyAlgoECDSA384, gossh.KeyAlgoECDSA521, + gossh.CertAlgoECDSA256v01, gossh.CertAlgoECDSA384v01, gossh.CertAlgoECDSA521v01, + gossh.KeyAlgoRSASHA256, gossh.KeyAlgoRSASHA512, gossh.KeyAlgoRSA, + gossh.CertAlgoRSASHA256v01, gossh.CertAlgoRSASHA512v01, gossh.CertAlgoRSAv01, + } +} + +// KeyAuth is SSH public key authentication that always states which host key algorithms it will +// accept. +// +// It exists to close a hole in go-git v6. Its SSH transport reads the process's default known_hosts +// files — `~/.ssh/known_hosts` and `/etc/ssh/ssh_known_hosts` — whenever ClientConfig comes back with +// an empty HostKeyAlgorithms, *even when a HostKeyCallback was supplied*, purely to derive the +// algorithm list (`plumbing/transport/ssh/ssh.go`, the `else if len(config.HostKeyAlgorithms) == 0` +// branch). If neither file exists it fails the connection with "unable to find any valid known_hosts +// file, set SSH_KNOWN_HOSTS env variable". The controller image is distroless with no home directory +// and no system known_hosts, so every SSH remote would fail there regardless of the credential — +// which is exactly what the e2e suite caught. v5 derived no algorithms and so never looked. +// +// Populating the field ourselves keeps that fallback unreachable. The algorithms are derived from the +// pinned known_hosts when we have one, matching git's own behaviour: offering an algorithm the pin +// does not cover would make the server present a key our callback then rejects. +type KeyAuth struct { + *gogitssh.PublicKeys + + // db is built from the supplied known_hosts, or nil when host key verification is disabled. + db *knownhosts.HostKeyDB +} + +// NewPublicKeyAuth builds go-git's SSH public key authentication from a private key, applying this +// project's host-key policy. // // Host key verification fails closed: a known_hosts source is required. A known_hosts value that // is present but cannot be parsed is always a hard error — if a host key is declared it must be -// valid. When no known_hosts is available at all, GetAuthMethod returns an error unless +// valid. When no known_hosts is available at all, NewPublicKeyAuth returns an error unless // allowMissingKnownHosts is set (the controller's --insecure-allow-missing-known-hosts flag), // which disables host key verification and is intended for throwaway/dev clusters only. -func GetAuthMethod(privateKey, password, knownHosts string, allowMissingKnownHosts bool) (transport.AuthMethod, error) { +func NewPublicKeyAuth( + privateKey, password, knownHosts string, + allowMissingKnownHosts bool, +) (*KeyAuth, error) { logger := log.FromContext(context.Background()) if privateKey == "" { @@ -35,7 +77,7 @@ func GetAuthMethod(privateKey, password, knownHosts string, allowMissingKnownHos } // Create the public key authentication - publicKeys, err := ssh.NewPublicKeys("git", []byte(privateKey), password) + publicKeys, err := gogitssh.NewPublicKeys("git", []byte(privateKey), password) if err != nil { return nil, fmt.Errorf("failed to create SSH public keys: %w", err) } @@ -43,12 +85,12 @@ func GetAuthMethod(privateKey, password, knownHosts string, allowMissingKnownHos if knownHosts != "" { // A declared host key must parse: this is a hard error regardless of the // allow-missing opt-out, which only ever covers the no-key-at-all case. - callback, err := setupKnownHostsCallback(logger, knownHosts) + db, err := knownHostsDB(logger, knownHosts) if err != nil { return nil, fmt.Errorf("failed to parse known_hosts for SSH host key verification: %w", err) } - publicKeys.HostKeyCallback = callback - return publicKeys, nil + publicKeys.HostKeyCallback = db.HostKeyCallback() + return &KeyAuth{PublicKeys: publicKeys, db: db}, nil } if !allowMissingKnownHosts { @@ -62,7 +104,60 @@ func GetAuthMethod(privateKey, password, knownHosts string, allowMissingKnownHos logInsecureHostKey(logger, "no known_hosts provided") //nolint:gosec // explicit development opt-out via --insecure-allow-missing-known-hosts publicKeys.HostKeyCallback = gossh.InsecureIgnoreHostKey() - return publicKeys, nil + return &KeyAuth{PublicKeys: publicKeys}, nil +} + +// ClientConfig implements gitclient.SSHAuth. It delegates to go-git for the credential and host key +// callback, then guarantees HostKeyAlgorithms is set. +func (a *KeyAuth) ClientConfig(ctx context.Context, req *transport.Request) (*gossh.ClientConfig, error) { + cfg, err := a.PublicKeys.ClientConfig(ctx, req) + if err != nil { + return nil, err + } + + if len(cfg.HostKeyAlgorithms) > 0 { + return cfg, nil + } + + if a.db != nil { + cfg.HostKeyAlgorithms = a.db.HostKeyAlgorithms(hostWithPort(req)) + } + if len(cfg.HostKeyAlgorithms) == 0 { + // No pin to narrow the list: offer the modern set. Verification is still the callback's job. + cfg.HostKeyAlgorithms = defaultHostKeyAlgorithms() + } + + return cfg, nil +} + +// hostWithPort renders the request's host in the "host:port" form the known_hosts lookup expects, +// defaulting to the SSH port. +func hostWithPort(req *transport.Request) string { + if req == nil || req.URL == nil { + return "" + } + port := req.URL.Port() + if port == "" { + port = "22" + } + return net.JoinHostPort(req.URL.Hostname(), port) +} + +// GetAuthMethod returns SSH public key authentication as transport client options. +// +// go-git v6 removed the single transport.AuthMethod interface: authentication is supplied as +// functional options on the transport client (gitclient.WithSSHAuth / gitclient.WithHTTPAuth), so a +// credential travels as a []gitclient.Option and a nil slice means anonymous. +// +// The options are opaque closures, so callers cannot inspect what kind of credential they hold. +// NewPublicKeyAuth is the introspectable half, kept exported so the host-key policy can be asserted +// directly rather than through the option wrapper. +func GetAuthMethod(privateKey, password, knownHosts string, allowMissingKnownHosts bool) ([]gitclient.Option, error) { + publicKeys, err := NewPublicKeyAuth(privateKey, password, knownHosts, allowMissingKnownHosts) + if err != nil { + return nil, err + } + return []gitclient.Option{gitclient.WithSSHAuth(publicKeys)}, nil } // logInsecureHostKey emits a loud warning whenever SSH host key verification is disabled. @@ -71,14 +166,19 @@ func logInsecureHostKey(logger logr.Logger, reason string) { "; do not use in production", "reason", reason) } -// setupKnownHostsCallback creates a host key callback from known_hosts content. -func setupKnownHostsCallback(logger logr.Logger, knownHosts string) (gossh.HostKeyCallback, error) { +// knownHostsDB parses known_hosts content into a host key database. +// +// The database carries both halves we need: the verification callback, and the per-host algorithm +// list that keeps go-git from reaching for the process's default known_hosts files (see KeyAuth). +// go-git only parses from a path, so the content is staged in a temp file; the parse is eager, so +// the file is removed before returning. +func knownHostsDB(logger logr.Logger, knownHosts string) (*knownhosts.HostKeyDB, error) { tmpFile, err := os.CreateTemp("", "known_hosts_*") if err != nil { logger.Info("Warning: failed to create temp known_hosts file", "error", err) return nil, err } - defer os.Remove(tmpFile.Name()) + defer func() { _ = os.Remove(tmpFile.Name()) }() if _, err := tmpFile.WriteString(knownHosts); err != nil { logger.Info("Warning: failed to write known_hosts", "error", err) @@ -90,12 +190,12 @@ func setupKnownHostsCallback(logger logr.Logger, knownHosts string) (gossh.HostK return nil, err } - callback, err := ssh.NewKnownHostsCallback(tmpFile.Name()) + db, err := knownhosts.NewDB(tmpFile.Name()) if err != nil { logger.Info("Warning: failed to parse known_hosts", "error", err) return nil, err } logger.V(1).Info("Using known_hosts for SSH host key verification") - return callback, nil + return db, nil } diff --git a/internal/ssh/auth_test.go b/internal/ssh/auth_test.go index ab9451ab..b3b857b9 100644 --- a/internal/ssh/auth_test.go +++ b/internal/ssh/auth_test.go @@ -3,13 +3,16 @@ package ssh import ( + "context" "crypto/rand" "crypto/rsa" "crypto/x509" "encoding/pem" + "net/url" "os" "testing" + "github.com/go-git/go-git/v6/plumbing/transport" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" gossh "golang.org/x/crypto/ssh" @@ -118,3 +121,50 @@ func TestGetAuthMethod_UnparseableKnownHostsIsHardErrorEvenWithOptOut(t *testing assert.Nil(t, auth) assert.Contains(t, err.Error(), "failed to parse known_hosts") } + +// TestKeyAuth_AlwaysSetsHostKeyAlgorithms pins the fix for a go-git v6 behaviour that broke every +// SSH remote in the controller image. +// +// v6's SSH transport reads the process's default known_hosts files whenever ClientConfig returns an +// empty HostKeyAlgorithms — even when a HostKeyCallback was supplied — purely to derive the algorithm +// list, and fails the connection with "unable to find any valid known_hosts file" when neither +// ~/.ssh/known_hosts nor /etc/ssh/ssh_known_hosts exists. The controller runs distroless with +// neither, so the e2e SSH spec failed against a real Gitea while every unit test passed: the +// fallback lives in the transport's connect, not in ClientConfig. +// +// Both host-key policies must therefore yield a non-empty algorithm list. +func TestKeyAuth_AlwaysSetsHostKeyAlgorithms(t *testing.T) { + privateKey, knownHostsLine := generateTestSSHKey(t) + u, err := url.Parse("ssh://git@example.com/org/repo.git") + require.NoError(t, err) + req := &transport.Request{URL: u} + + t.Run("with a pinned known_hosts the algorithms come from the pin", func(t *testing.T) { + auth, err := NewPublicKeyAuth(privateKey, "", knownHostsLine, false) + require.NoError(t, err) + + cfg, err := auth.ClientConfig(context.Background(), req) + require.NoError(t, err) + require.NotEmpty(t, cfg.HostKeyAlgorithms, + "an empty list sends go-git to the default known_hosts files, which do not exist in the image") + t.Logf("pinned algorithms: %v", cfg.HostKeyAlgorithms) + assert.Contains(t, cfg.HostKeyAlgorithms, gossh.KeyAlgoRSASHA256, + "the pinned RSA key's algorithms must be offered") + // Without this the subtest cannot tell a pin hit from the default fallback, because the + // default set also contains the RSA algorithms. + assert.NotEqual(t, defaultHostKeyAlgorithms(), cfg.HostKeyAlgorithms, + "a list identical to the default set means the pin was never consulted") + assert.NotContains(t, cfg.HostKeyAlgorithms, gossh.KeyAlgoED25519, + "the pin holds only an RSA key, so offering ed25519 would mean the default set was used") + }) + + t.Run("with host key verification disabled the modern set is offered", func(t *testing.T) { + auth, err := NewPublicKeyAuth(privateKey, "", "", true) + require.NoError(t, err) + + cfg, err := auth.ClientConfig(context.Background(), req) + require.NoError(t, err) + assert.Equal(t, defaultHostKeyAlgorithms(), cfg.HostKeyAlgorithms) + require.NotNil(t, cfg.HostKeyCallback) + }) +} diff --git a/test/e2e/Taskfile-e2e.yml b/test/e2e/Taskfile-e2e.yml index b0323c98..33f872d0 100644 --- a/test/e2e/Taskfile-e2e.yml +++ b/test/e2e/Taskfile-e2e.yml @@ -251,7 +251,7 @@ tasks: # E2E_REPORT_NAME keeps each shard's Ginkgo JSON report distinct. go run github.com/onsi/ginkgo/v2/ginkgo \ --procs={{.E2E_GINKGO_PROCS}} --timeout="{{.E2E_FULL_TIMEOUT}}" -v \ - --label-filter='{{.E2E_LABEL_FILTER | default "!image-refresh && !bi-directional && !source-cluster"}}' \ + --label-filter='{{.E2E_LABEL_FILTER | default "!image-refresh && !bi-directional && !source-cluster && !ado"}}' \ --output-dir="{{.CS}}/{{.NAMESPACE}}" \ --json-report=ginkgo-report-{{.E2E_REPORT_NAME | default "full"}}.json \ ./test/e2e/ @@ -465,6 +465,21 @@ tasks: go test -timeout "{{.E2E_GO_TEST_TIMEOUT}}" ./test/e2e/ -v -ginkgo.v -ginkgo.label-filter=image-refresh \ -ginkgo.json-report="{{.CS}}/{{.NAMESPACE}}/ginkgo-report-image-refresh.json" + test-e2e-ado: + desc: 'Mirror into a real Azure DevOps repository (needs E2E_ADO_REPO_URL and E2E_ADO_PAT)' + cmds: + - | + if [ -z "${E2E_ADO_REPO_URL:-}" ] || [ -z "${E2E_ADO_PAT:-}" ]; then + echo "E2E_ADO_REPO_URL and E2E_ADO_PAT must be set; see docs/azure-devops-getting-started.md" >&2 + exit 1 + fi + export CTX="{{.CTX}}" + export INSTALL_MODE="{{.INSTALL_MODE}}" + export NAMESPACE="{{.NAMESPACE}}" + export E2E_AGE_KEY_FILE="{{.CS}}/age-key.txt" + go test -timeout "{{.E2E_GO_TEST_TIMEOUT}}" ./test/e2e/ -v -ginkgo.v -ginkgo.label-filter=ado \ + -ginkgo.json-report="{{.CS}}/{{.NAMESPACE}}/ginkgo-report-ado.json" + test-e2e-quickstart-helm: desc: Run quickstart install validation with Helm install cmds: diff --git a/test/e2e/ado_e2e_test.go b/test/e2e/ado_e2e_test.go new file mode 100644 index 00000000..5a3a6786 --- /dev/null +++ b/test/e2e/ado_e2e_test.go @@ -0,0 +1,187 @@ +// SPDX-License-Identifier: Apache-2.0 + +package e2e + +import ( + "encoding/base64" + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +// The Azure DevOps corner: the one e2e category that talks to a real hosted Git provider instead of +// the in-cluster Gitea. +// +// It exists because ADO is the provider that broke. It rejects any Git fetch whose capability list +// omits multi_ack with HTTP 400 "TF401041: Clients must support multi-ack", which go-git only +// implements from v6. `internal/git/ado_multiack_test.go` reproduces that rule locally and gates CI; +// `internal/git/ado_live_test.go` checks the library against the real remote. This spec is the layer +// above both: it proves the *operator* mirrors a live cluster into an ADO repository, which is what a +// user actually cares about, and it is the recipe docs/azure-devops-getting-started.md is written from. +// +// Opt-in, because it needs a credential nobody's CI has: +// +// export E2E_ADO_REPO_URL='https://dev.azure.com///_git/' +// export E2E_ADO_PAT='' # scope: Code (read & write) +// task test-e2e-ado +// +// It writes to a path of its own inside the repository and does not clean the repository up. Point it +// at a scratch repo. +const ( + adoRepoURLEnv = "E2E_ADO_REPO_URL" + adoPATEnv = "E2E_ADO_PAT" + adoUsernameEnv = "E2E_ADO_USERNAME" +) + +// skipUnlessADOConfigured aborts the calling spec unless a real ADO repository is supplied. The Ginkgo +// label alone is not enough: no credential, no test. +func skipUnlessADOConfigured() string { + GinkgoHelper() + + url := strings.TrimSpace(os.Getenv(adoRepoURLEnv)) + if url == "" || strings.TrimSpace(os.Getenv(adoPATEnv)) == "" { + Skip(fmt.Sprintf( + "Azure DevOps corner is disabled; set %s and %s, then run `task test-e2e-ado`", + adoRepoURLEnv, adoPATEnv, + )) + } + + return url +} + +var _ = Describe("Azure DevOps", Label("ado"), Ordered, func() { + const ( + providerName = "ado-provider" + targetName = "ado-target" + ruleName = "ado-rule" + secretName = "ado-creds" + ) + + var ( + testNs string + repoURL string + repoPath string + ) + + BeforeAll(func() { + repoURL = skipUnlessADOConfigured() + + testNs = testNamespaceFor("ado") + _, _ = kubectlRun("create", "namespace", testNs) // idempotent + + // A path of our own, so a shared scratch repository can carry several runs. + repoPath = fmt.Sprintf("e2e/ado-%d", GinkgoRandomSeed()) + + By("creating the ADO credentials Secret") + // ADO sends a PAT as HTTP basic auth with the token as the password and ignores the + // username, so only `password` is set. This is the Secret shape the getting-started guide + // documents, and creating it any other way is the most common way to get ADO wrong. + _, err := kubectlRunInNamespace(testNs, "create", "secret", "generic", secretName, + "--from-literal=password="+os.Getenv(adoPATEnv)) + Expect(err).NotTo(HaveOccurred(), "failed to create the ADO credentials Secret") + + applySOPSAgeKeyToNamespace(testNs) + + By("creating a GitProvider pointing at the ADO repository") + createGitProviderWithURLInNamespace(providerName, testNs, secretName, repoURL) + + // Reaching Ready here already proves more than it looks: the connectivity check reads the + // ref advertisement, which is the one ADO operation that never needed multi_ack. + verifyResourceStatus( + "gitprovider", providerName, testNs, + "True", "Succeeded", "Repository connectivity validated", + ) + + By("creating a GitTarget for a path of this run's own") + createGitTarget(targetName, testNs, providerName, repoPath, "main") + verifyResourceCondition("gittarget", targetName, testNs, "Validated", "True", "Succeeded", "") + + By("watching ConfigMaps in the test namespace") + applyIsolationWatchRule(ruleName, testNs, targetName, `"configmaps"`) + verifyResourceStatus("watchrule", ruleName, testNs, "True", "Succeeded", "") + + By("waiting for the stream to be live before creating anything to mirror") + waitForStreamsRunning(targetName, testNs) + }) + + AfterAll(func() { + if testNs != "" { + cleanupNamespace(testNs) + } + }) + + It("mirrors a live ConfigMap into the Azure DevOps repository", func() { + const cmName = "ado-demo" + + By("creating a ConfigMap in the cluster") + _, err := kubectlRunInNamespace(testNs, "create", "configmap", cmName, + "--from-literal=greeting=hello-from-azure-devops") + Expect(err).NotTo(HaveOccurred()) + + By("waiting for the operator to commit it to ADO, then reading the repository back") + // Cloning with canonical git rather than our own library is deliberate: the assertion must + // not depend on the code under test. + wanted := filepath.Join(repoPath, testNs, "configmaps", cmName+".yaml") + Eventually(func(g Gomega) { + content := adoReadFile(g, repoURL, wanted) + g.Expect(content).To(ContainSubstring("hello-from-azure-devops"), + "the committed manifest must carry the ConfigMap's data") + g.Expect(content).To(ContainSubstring("kind: ConfigMap")) + }, 2*time.Minute, 5*time.Second).Should(Succeed()) + }) +}) + +// adoReadFile clones the ADO repository with canonical git and returns one file's contents. The clone +// is shallow and single-branch: this is a read-back assertion, not a history check. +func adoReadFile(g Gomega, repoURL, relPath string) string { + GinkgoHelper() + + dir, err := os.MkdirTemp("", "ado-readback-*") + g.Expect(err).NotTo(HaveOccurred()) + defer func() { _ = os.RemoveAll(dir) }() + + // The PAT travels in a config header rather than in argv or the URL, so it stays out of process + // listings and out of any error text that quotes the command. + cfg := filepath.Join(dir, "gitconfig") + g.Expect(os.WriteFile(cfg, []byte(adoExtraHeader(repoURL)), 0600)).To(Succeed()) + + checkout := filepath.Join(dir, "checkout") + cmd := exec.Command("git", "clone", "--depth=1", "--single-branch", repoURL, checkout) + cmd.Env = append(os.Environ(), + "GIT_CONFIG_GLOBAL="+cfg, + "GIT_TERMINAL_PROMPT=0", + ) + out, err := cmd.CombinedOutput() + g.Expect(err).NotTo(HaveOccurred(), "git clone of the ADO repository failed: %s", out) + + content, err := os.ReadFile(filepath.Join(checkout, relPath)) + g.Expect(err).NotTo(HaveOccurred(), "expected %s in the ADO repository", relPath) + + return string(content) +} + +// adoExtraHeader renders a gitconfig that authenticates to this repository's origin only, so the +// credential is never offered to another host on a redirect. +func adoExtraHeader(repoURL string) string { + origin := repoURL + if idx := strings.Index(repoURL, "/_git/"); idx > 0 { + origin = repoURL[:idx] + } + + basic := adoBasicCredential() + + return fmt.Sprintf("[http %q]\n\textraHeader = Authorization: Basic %s\n", origin, basic) +} + +// adoBasicCredential base64-encodes the PAT as HTTP basic auth: an empty username with the token as +// the password, which is the form ADO documents. +func adoBasicCredential() string { + return base64.StdEncoding.EncodeToString( + []byte(os.Getenv(adoUsernameEnv) + ":" + os.Getenv(adoPATEnv))) +}