docs: correct and complete the documentation against the code - #591
Merged
Merged
Conversation
Three classes of error that make a copied example fail rather than mislead
slowly.
spec.code is a list of sources, not a single one. Ten examples showed it as
an object; the API server answers 'unknown field spec.code.image' and
refuses the manifest. Verified against the CRD with kubectl apply
--dry-run=server: the list form is accepted, the object form is not.
The image tags 8.12.1 and 8.13.0 appeared at ten places and cannot exist.
images/openvox-versions.yaml states that the image name encodes the OpenVox
major while the tag carries the operator release version; the published tags
are 0.12.0, develop and latest.
Three label selectors in the troubleshooting and CA import guides do not
exist in labels.go: app.kubernetes.io/instance is never set,
app.kubernetes.io/name is openvox rather than openvox-server, and the CA
label is certificateauthority without a hyphen. A selector that matches
nothing is worse than no command at all, because it reads like an answer.
Also corrects two resource names: the Config ConfigMap is {name}-config,
and the CA setup Job is {ca}-ca-setup. The remaining names in the reference
tables were checked against the code and are correct.
The Error phase is documented for Config, Server and Database as 'reconciliation failed'. It exists in the CRD enum but no code path assigns it: a failing reconcile leaves the phase where it was and reports the reason in a condition. Someone waiting for phase Error waits forever, which is the worst kind of wrong - it looks like a working diagnosis. Replaced with the condition to watch, and the jsonpath was checked against a live object. The Gateway API concept showed serverRef on a Pool. That field exists neither on the CRD nor in the chart; a Server names its pools through poolRefs and the Pool selects those pods. Removed, and the direction of the relationship spelled out, since getting it backwards is the natural guess. Added what the code does but nothing described: renewal reuses the existing private key deliberately, because the CA renews for the same public key. So renewal extends validity without rotating the key, and replacing a key needs a new Certificate. Checked and left alone: the Renewing phase description is already accurate, and no page claims renewal generates a new key. The review report asserted both; neither held up.
The operator writes autosign = /usr/local/bin/openvox-autosign as soon as a CertificateAuthority exists, regardless of whether any SigningPolicy does, and evaluatePolicies returns false for an empty list. An empty policy list is therefore deny-all rather than off, which is the state every fresh install starts in. Nothing surfaces it: the servers run, the Config reports Running, and every agent sits in --waitforcert until it gives up. Documented in the quickstart and in the agent section of the troubleshooting guide, with the manual puppetserver ca sign path as the immediate way out. Also separates two cases the troubleshooting guide conflated. A Certificate resource stuck in Pending is almost never a policy problem: for an internal CA the operator signs its own Certificates over the CA API with the operator signing certificate, so autosign is not involved. That entry now lists the causes that do apply - CA not ready, signing certificate not yet available, certname already claimed, external CA - each with a command that reads the condition rather than the phase. Policies govern agents, and that is where the note now lives.
Compared every documented field against the generated CRDs, in both directions. observedGeneration exists on all nine status types since #549 but appeared on two reference pages. Added to the remaining seven, with the sentence that makes it useful: a value below metadata.generation means the rest of the status has not caught up yet. Certificate gained signedSpecHash and effectiveDNSAltNames in the status table. Both drive behaviour a reader needs to predict - the first decides when a certificate is re-signed, the second says which alt names it is actually issued for once Pools contribute. Server gained readOnlyRootFilesystem, the per-Server override added in #575. The ImageSpec table still listed defaults that #549 and #576 removed: repository ghcr.io/slauger/openvox-server-8, tag latest, pullPolicy IfNotPresent. It contradicted the prose directly beneath it. Corrected, and the reason spelled out - a nested default is applied whether or not the parent object was given, so a defaulted field can never mean inherit - plus the pullSecrets rule that a Server list replaces rather than extends. Checked and found correct: every other documented default matches its CRD, and no reference page names a field that does not exist.
The user documentation never showed an agent connecting. puppet agent, --waitforcert, ca_server and puppetserver ca sign appeared nowhere, so the moment the product exists for was the one step a reader had to work out alone. The guide covers what an agent needs, where to point it, running one inside the cluster and reaching one from outside, signing by hand when no policy matches, revoking, and how to tell from the server side that it worked. Two things it states that are easy to get wrong. ca_server is only needed once the CA and the compilers are in separate pools - with the default poolRefs [ca, server] one address serves both, which is why the setting appears nowhere in this repository. And puppet agent --test returns 2 when it applied changes, so a Job that treats non-zero as failure reports a successful run as broken. It also names what the agent image is: openvox-agent-<major> is built by CI and not by the release workflow, so it carries only the develop tag - no latest, no version. Documented as the test artifact it is, rather than implying it is a supported way to run agents. Every claim was checked: the Service default is ClusterIP, the Deployment carries the Server name, crlRefreshInterval defaults to 5m, and the image tags are what ghcr actually holds.
Extracted all 42 manifests from the documentation and ran them against a real API server with kubectl apply --dry-run=server. One was rejected for a reason a reader could not guess: the NodeClassifier page abbreviated the image block as 'image: ...', and the resulting error talks about a type mismatch rather than the ellipsis. Replaced with the real block; all 42 are accepted now. The Database concept claimed the Config controller waits when the Database is not Running. It does not: renderPuppetDBConf returns a puppetdb.conf without server_urls and the servers start regardless. Together with soft_write_failure = true, which the operator sets in every path, a server in that state compiles catalogs normally while reports and exported resources go nowhere and nothing is raised. That resolves itself during bring-up and only bites when the Database never becomes ready, so the symptom is an empty PuppetDB rather than an error. Monitoring now states that CertificateAuthority and Certificate write the same expiry metric with the same labels and nothing distinguishing them, so the .*-ca matcher in the CA alert is a naming convention and not a guarantee. Added an absence rule for the CRL metric: a staleness alert cannot fire on a series that was never created, which is exactly the case when the operator never got far enough to refresh one.
README and the feature list promise 'Heap size calculated from memory limits (90%) - no manual -Xmx tuning needed'. The controller does contain that calculation, but it is unreachable: ServerSpec.JavaArgs carries a CRD default, so the field is never empty and the first branch of resolveJavaArgs always wins. Every Server runs with -Xms512m -Xmx1024m no matter how much memory it is given. Replaced the claim with what actually applies, and noted it on the field in the Server reference with a link to #592, where the fix is tracked. Once the default is removed the derivation works and the wording can go back. Third instance of the same pattern after #550 and #576: a CRD default takes away the empty state a fallback depends on.
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.
A documentation review found that the docs had never been checked against the code. This works through the findings, verifying each one before changing anything -- two of the review's own claims did not survive that check and were left alone.
Work in progress; more commits will follow.
Examples the API server rejects
spec.codeis a list of sources, not a single one. Ten examples showed it as an object. Verified withkubectl apply --dry-run=server:The image tags
8.12.1and8.13.0appeared at ten places and cannot exist --images/openvox-versions.yamlstates the tag carries the operator release version, and the published tags are0.12.0,develop,latest.Three label selectors do not exist in
labels.go:app.kubernetes.io/instanceis never set,app.kubernetes.io/nameisopenvoxnotopenvox-server, and the CA label has no hyphen. A selector matching nothing is worse than no command, because it reads like an answer.Behaviour the controller does not have
Erroris documented as a phase for Config, Server and Database. It exists in the enum but nothing assigns it: a failed reconcile keeps the previous phase and reports the reason in a condition. Someone waiting forErrorwaits forever. Replaced with the condition to watch; the jsonpath was tested against a live object.serverRefon a Pool exists neither in the CRD nor the chart -- a Server names its pools viapoolRefs. Removed, and the direction spelled out.The deny-all trap
The operator sets
autosignas soon as a CertificateAuthority exists, andevaluatePoliciesreturns false for an empty list. Every fresh install starts in deny-all, and nothing shows it: servers run, the Config reportsRunning, agents sit in--waitforcert.Also separated two cases the troubleshooting guide conflated: a Certificate stuck in
Pendingis almost never a policy problem, because for an internal CA the operator signs its own Certificates over the CA API. Policies govern agents.Reference pages against the CRDs
Compared every documented field in both directions.
observedGenerationexists on all nine status types and appeared on two pages; added to the other seven. Certificate gainedsignedSpecHashandeffectiveDNSAltNames, Server gainedreadOnlyRootFilesystem.The ImageSpec table still listed defaults that #549 and #576 removed and contradicted the prose beneath it.
The missing guide
Nothing showed an agent connecting. New page covering where to point it, running one in and outside the cluster, signing by hand, revoking, and verifying. Two things it gets right that are easy to miss:
ca_serveris only needed once CA and compilers are in separate pools, andpuppet agent --testreturns 2 on success-with-changes.It also states that
openvox-agent-<major>is built by CI and not by the release workflow, so it has only adeveloptag -- a test artifact, not a supported way to run agents.Two review claims that did not hold
The report said the docs promise a new key on renewal and that the
Renewingphase is described wrongly. Neither is true; both pages were already correct. Applying the list blindly would have broken working documentation. What was actually missing is that renewal reuses the key deliberately, so it extends validity without rotating -- now documented.