feat(manifests): make the package the single source of operator versions - #32
Merged
Conversation
Versions were declared in two places that could disagree, and did. The
manifests package pinned Knative v1.15.0 while the regenerate workflow
installed v1.20.0 — so the client was generated against one version and the
package shipped manifests for another, with nothing to notice.
Versions now live only in packages/manifests/scripts/pull-manifests.ts. The
workflow installs whatever that package vendored, which makes the direction
explicit: manifests decide what the cluster runs, the cluster decides what its
OpenAPI says, and the OpenAPI decides what the client claims exists.
Operators refreshed and aligned:
knative-serving v1.15.0 -> v1.22.1
cert-manager v1.17.0 -> v1.21.1
cloudnative-pg pinned to the v1.25.2 tag rather than the release-1.25
branch, which could change under a fixed filename
cilium 1.19.5 new — CiliumNetworkPolicy types were ungenerable
traefik 34.4.1 new — previously fetched by the workflow only
tekton-pipelines v1.15.0 new — same
ingress-nginx removed, unused
net-kourier now comes from knative-extensions rather than knative. The old path
redirects, so both worked and neither looked wrong — which is how two consumers
came to name different repos for the same file.
The workflow's per-operator booleans are gone. Regeneration is all-or-nothing —
the generator emits whatever the cluster advertises — so a disabled checkbox
did not skip an operator's types, it dropped them. Six switches that each
silently deleted a section of the client. The remaining input controls whether
manifests are re-pulled from upstream first.
The PR it opens now carries the manifests and generated objects alongside the
client, since shipping one without the other reintroduces the drift.
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01V6watrJsLurfsr3uhnqibB
Made the pull unconditional rather than an input. Versions are pinned in pull-manifests.ts, so this re-downloads the same content rather than picking up something newer -- it is idempotent, and making it optional only created a way for the vendored files to lag the config that names them. Leaves the workflow with no inputs at all, which is the point: there is one source of truth and no switch that can make a run mean something different.
CI keeps failing while installing kind:
Installing kind...
curl: (56) Connection died, tried 5 times before giving up
helm/kind-action fetches the binary from GitHub releases unauthenticated. Those
are rate-limited per IP, and CI runners share egress addresses -- so the
allowance is spent by other people's traffic, and the limit arrives as a killed
connection rather than a clean error. The action's five internal retries cannot
help: retrying against a spent allowance spends more of nothing.
`gh` authenticates with the token every run already has, which moves the
download onto a per-token allowance instead of a shared per-IP one.
Done as a composite action rather than inline, because there are four call
sites across three workflows and the fix should not be pasted four times. It
installs the same kind version helm/kind-action@v1.12.0 did (0.26.0), so
nothing changes but the way the binary is fetched.
Fourth occurrence of this failure today across two repos.
Removing ingress-nginx from the manifests package left three consumers behind: an e2e matrix entry, a standalone e2e apply test, and two fixtures that named its namespace and chart version. The unit test failed because getOperatorResources returned nothing for an operator that no longer ships. Also extends the namespace-coverage assertion to traefik. That test checks a manifest set carries a Namespace, which is what makes it applicable standalone -- worth asserting for the operators added here rather than only the ones that predate them. Unit tests: 7 passed.
…er assertions ci.yml installed its own tooling and so was missed by the earlier sweep. Two problems beyond the flaky download: kubectl came from dl.k8s.io resolved through stable.txt, so the version moved whenever upstream cut a release -- an unpinned dependency in a job whose purpose is reproducibility. kind came from kind.sigs.k8s.io/dl, which redirects to GitHub releases, and pinned 0.20.0 while every other workflow used 0.26.0. Both now go through .github/actions/setup-kind, which leaves no unauthenticated binary fetch anywhere in the workflows. Also fixes the cert-manager integration test. Removing ingress-nginx left it asserting two apply calls when one operator remains, and my earlier regex deleted the arguments from a getOperatorResources call rather than the whole assertion -- so the suite failed to compile rather than failing an assertion, which is why it showed as a suite error with its tests still passing. Unit tests: 7 passed.
cert-manager was bumped to v1.21.1 on the reasoning that no consumer installs it. That was wrong -- constructive-cloud deploys v1.17.0 (k8s/operators/cert-manager.yaml carries helm.sh/chart: cert-manager-v1.17.0), so a client generated from v1.21.1 CRDs would describe an API that is not running. Held at v1.17.0 with a note saying why, since 'newer' is the obvious thing for someone to do next. The knative e2e failure was not the predicate: isAdmissionWebhookTransient already matches 'failed calling webhook' plus 'connection refused', and both the create and replace paths already retry. The budget was the problem -- 6 attempts capped at 10s is roughly 34 seconds, which is sized for a rolling webhook being briefly unavailable. That is not this case. The webhook is created by the same apply: Knative v1.22.1 ships Certificates in serving-core.yaml that its own webhook must admit, so the wait is a Deployment scheduling, pulling an image and passing a readiness probe. On a cold cluster that is minutes. Now 12 attempts capped at 15s, about 150s. Also drops the superseded versioned copies. The puller writes a per-version file and never removed old ones, so OPERATOR_VERSIONS listed both v1.17.0 and v1.21.1 for cert-manager -- which reintroduced the ambiguity this package exists to remove, and is why the e2e fixture silently picked the older one.
Collaborator
Author
Status: 11 green, 1 red — and the red is a genuine version-compatibility question, not a flake
What it is notNot the retry budget, and not the transient-error predicate. Both were already correct: Not a race either. There is already a The webhook Deployment never becomes Ready at all. Waiting longer does not help something that is not coming. What I think it isTwo version decisions in this PR that interact:
Knative v1.22 ships cert-manager Worth noting the shape: neither version choice is wrong on its own. Each matches a real consumer. They are only wrong together, and nothing in the previous arrangement would have surfaced that, because the versions lived in separate repos and never met. The decision, which is not mine
I would do (3) before choosing between (1) and (2). Everything else is green11 passing, including all other operators, both builds, lint, integration and e2e-postgres. Also fixed along the way: the e2e fixture hardcoded operator versions (a third place they were declared — now derived from the package), and the puller kept superseded versioned copies so |
…l idempotent Three problems, found by checking that two consecutive pulls produce identical output. Knative was concatenated into one document set. serving-crds must be applied and established before serving-core, which creates custom resources of those very kinds -- merged, a single-pass apply races them. The combineUrls option existed for exactly this and was declared but never read, so every URL source was concatenated whether or not that was safe. It is now implemented, Knative sets it false, and the parts are written numbered under the version directory: the order they must be applied in is the order they sort in. Cilium is removed. Its chart cannot be vendored safely or usefully: cilium-ca-secret.yaml emits a freshly generated CA certificate and private key on every -- unconditionally, tls.auto.enabled=false does not suppress it -- so vendoring the output would have committed a private key to a public repository and published it to npm, and would never reproduce. Filtering to CRDs yields nothing either, because Cilium's operator registers its CRDs at runtime rather than shipping them in the chart. It is installed by the workflow instead, where the CA is generated in the cluster and stays there. Two consecutive pulls now produce byte-identical output. They did not before, and the difference was that private key. The codegen understands both shapes -- <version>.yaml and <version>/ of ordered parts -- so a split operator keeps its version rather than reporting its parts as versions. The workflow applies the parts in order, waits for CRDs to establish between them, and installs Cilium at runtime so its CRDs exist for generation.
The split broke the build. codegen discovers an operator's kinds from its
top-level <name>.yaml, and writing only the numbered parts removed that file --
so knative-serving.ts was generated but never imported by the aggregator, and
OPERATOR_MAP referenced an identifier that did not exist:
src/generated/index.ts: error TS2304: Cannot find name 'KnativeServing'
Both shapes are now written, because they serve different consumers:
knative-serving.yaml read by codegen; never applied, so the
ordering problem does not arise
knative-serving/<version>/NN-* applied in order by the workflow
The ordering hazard is on the apply path only. Conflating the two is what made
this look like a choice between them.
Verified locally: build clean, 7 unit tests pass, two consecutive pulls
byte-identical, no key material vendored.
applyAll phases CRDs first, then applies everything else -- but creating a CRD returns as soon as the object is accepted, not when the API server is serving that kind. Establishment is asynchronous, so a custom resource in a later phase can arrive before its own kind exists and fail with 'no matches for kind'. Knative is where this shows: serving-core creates Certificates and Images of kinds serving-crds defines moments earlier. Installing the same manifests by hand with an explicit between them works first time and the webhook rolls out in about fourteen seconds, which is what ruled out both of my earlier theories -- the retry budget and a cert-manager incompatibility. Neither was involved. The manifests are fine; the apply raced itself. A CRD that never establishes is reported rather than fatal, so the failure surfaces as the resource that needed it instead of an opaque wait.
The knative e2e failed because the webhook service wait expired, not because the webhook was broken. E2E_TESTS set the timeout to 30 seconds while every other path gets 240. Thirty seconds is shorter than a cold image pull. Installing the same manifests by hand, Knative's webhook rolls out in about fourteen seconds on a warm machine -- and a CI runner fetching that image for the first time takes longer. So the wait expired, the Certificate that needs the webhook was applied regardless, and the resulting 500 read as a webhook fault rather than as a timeout that had already given up. 180s: long enough for a cold pull, still short enough that a genuinely stuck webhook does not hold a suite for four minutes.
Two corrections, both of my own making. The knative e2e was failing, and my response was to widen timeouts twice: the apply retries from ~34s to ~150s and the E2E webhook wait from 30s to 180s. Multiplied by an outer 3x retry that already existed, that exceeded the test's own 15-minute jest budget -- so the job went from failing in 2.3 minutes with a legible webhook error to hanging for 19 and reporting nothing. A slower failure is bad; a failure that no longer reaches a verdict is worse, and it meant my last two changes were never actually validated. Retries are back to 6/10s. The E2E webhook wait is 60s: longer than the original 30s, which was genuinely too short for a cold image pull, and short enough that the suite reports a verdict well inside its budget. Knative is held at v1.15.0. v1.22.1 is what downstream installs and is where this should land, but the bump fails the e2e in a way that does not reproduce: the same manifests, applied in the same order after cert-manager, install cleanly on a local kind cluster with the webhook up in about fourteen seconds and zero apply errors. That is a client-side apply problem worth chasing on its own, and it should not hold up the work that is actually valuable here -- pinning versions that were previously unpinned or fetched from mutable branches. Kept: the CRD establishment wait, which is a real bug fix. Creating a CRD returns before the API server serves that kind, so a custom resource in a later phase could race its own definition.
pyramation
force-pushed
the
feat/manifests-single-source
branch
from
August 13, 2026 00:58
bff5bdb to
6084c86
Compare
Collaborator
Author
|
Knative held at The bump to Split out with a full record of what has been eliminated — cert-manager compatibility, the retry predicate, the retry budget, CRD establishment, and manifest concatenation were all investigated and are all not the cause. Two of those turned up real bugs that are fixed and kept in this PR. One warning carried over: do not widen timeouts to make it pass. Doing so pushes the suite past its own 15-minute jest budget, and it stops reporting a verdict at all — the failure went from a legible 2.3-minute webhook error to a 19-minute hang. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Operator versions were declared in two places that could disagree — and did.
So the client was generated against one version while the package shipped manifests for another, and nothing could notice.
The direction is now explicit
The workflow no longer names a single version. It installs whatever the package vendored, so the manifests and the client cannot describe different clusters. Changing a version means editing
pull-manifests.tsand running the workflow.Operators
v1.25.2tag rather than therelease-1.25branch, which could change under a fixed filenameCiliumNetworkPolicytypes were ungenerable because the cluster never had the CRDsnet-kouriernow comes fromknative-extensionsrather thanknative. The old path redirects, so both worked and neither looked wrong — which is how two consumers ended up naming different repos for the same file.The per-operator checkboxes are gone
Regeneration is all-or-nothing — the generator emits whatever the cluster advertises. So a disabled checkbox did not skip an operator's types, it dropped them. Six switches that each silently deleted a section of the published client.
The workflow now takes no inputs at all. That is the point: one source of truth, and no switch that can make a run mean something different from the last one.
The pull is unconditional
Versions are pinned in the config, so re-pulling fetches the same content — it is idempotent. Making it optional only created a way for the vendored files to lag the config that names them.
What the PR it opens now contains
The manifests and generated objects alongside the client. Shipping one without the other is how the drift started.
Review note
+162k lines, almost entirely vendored upstream YAML and the typed objects generated from it (Traefik and Tekton alone are ~13k lines of CRD each). The reviewable surface is
scripts/pull-manifests.ts,.github/workflows/regenerate-ops.yml, and the catalog entry removed fromsrc/index.ts.Not run yet
regenerate-opsisworkflow_dispatchonly. The manifests here were produced by runningpull:allandcodegenlocally, so those two steps are exercised — the cluster install and client regeneration are not. Worth one dispatch before publishing.Follow-up: this supersedes fetching manifests from URLs in downstream repos, which can consume the package once published.