docs: rewrite Kubernetes page #3928
Conversation
- re-structure the page (mainly introducing `===`) to enable users to switch easily between individual parts of this docs page - rewrite the PROXY protocol parts - add Traefik PROXY protocol example with deciated proxy-ports
georglauterbach
added
area/documentation
orchestrator/kubernetes
kind/update
There was a problem hiding this comment.
This looks pretty great! I really like the change to content tabs 😎
I understand the frustration with improving docs and how time consuming that can be, so this is greatly appreciated ❤️
Not all changes can be applied as-is (some require additional changes that aren't easy to suggest via the web UI, so their context needs to be taken into account), or are optional.
This review took me hours 😂 (I don't know why I'm so slow at it 😭 ), I'm going to take a break but can come back and apply some of the suggestions myself, while leaving some others open to discussion.
I'd also like to adjust the presentation of the final section a little bit, as the content lacks scope boundaries which makes some tabs a bit unclear where they stop, or that they're nested.
It should be doable via empty admonition wrapping with quote type which I think we used in the DNS docs page. That'll need to be handled via a separate commit though as it'll be noisy and hard to track meaningful diffs when applying feedback 😅
|
I will tackle this during the weekend :) |
It should be better structured now and container a more uniform structure.
|
I manually adjusted the docs to your feedback and left the comments open that, I felt, should be left open for you to resolv @polarathene. This way, you can see what my answer is (it's difficult when GitHub collapses them, especially when there are numerous comments). |
The scaling-issue is incorrect here, because there is no actual relation to the `Service`. Whether not the Origin IP is preserved is covered later too - no need to make the reader confused at this point.
This is not required because `readOnlyRootFilesystem: false`.
| --- | ||
| apiVersion: v1 | ||
| kind: PersistentVolumeClaim | ||
| With the inline `postfix-accounts.cf` file configured above, the content is fixed: you cannot change the configuration or persists modifications, i.e. adding or removing accounts is not possible. You need to use a `PersistentVolumeClaim` in case `postfix-accounts.cf` cannot be static. |
There was a problem hiding this comment.
In Docker Compose the equivalent feature supports writes IIRC, but same caveats with no persistence. Is the claim here about the file being read-only accurate, or likewise only lacking the ability to persist changes?
| With the inline `postfix-accounts.cf` file configured above, the content is fixed: you cannot change the configuration or persists modifications, i.e. adding or removing accounts is not possible. You need to use a `PersistentVolumeClaim` in case `postfix-accounts.cf` cannot be static. | |
| With the inline `postfix-accounts.cf` config above, the file content is static (_modifications like adding or removing account lines will not persist_). When your configs should be dynamic you should instead use a `PersistentVolumeClaim`. |
Reworded the final sentence to be more generic about inline/static config vs persisting advice for dynamic config needs.
There was a problem hiding this comment.
I think my version is accurate: the file is read-only, and even if you appear to have write permissions, you will not be able to actually write to the file. Mounting a ConfigMap or Secret is always read-only, no modifications whatsoever.
| For Dovecot, you can configure [`dovecot.cf`][docs-dovecot] to look like this: | ||
|
|
||
| ```cf | ||
| haproxy_trusted_networks = <YOUR POD CIDR> | ||
|
|
||
| service imap-login { | ||
| inet_listener imaps-proxied { | ||
| haproxy = yes | ||
| port = 10993 | ||
| ssl = yes | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| Last but not least, the `ports` section in the `Deployment` needs to be extended: | ||
|
|
||
| ```yaml | ||
| - name: smtp-proxy | ||
| containerPort: 10025 | ||
| protocol: TCP | ||
| - name: subs-proxy | ||
| containerPort: 10465 | ||
| protocol: TCP | ||
| - name: sub-proxy | ||
| containerPort: 10587 | ||
| protocol: TCP | ||
| - name: imaps-proxy | ||
| containerPort: 10993 | ||
| protocol: TCP | ||
| ``` |
There was a problem hiding this comment.
Extending the Deployment config makes sense in that this snippet would append/merge to that earlier example.
- But that already has the regular ports configured, whereas I think the intention here is to replace them?
- Or were you referring to extending the
Deploymentconfig of the sibling tab? It's a little vague which one is referred to here.
Or is this section of config relevant to other containers within the cluster, not only for the ingress?
These two tabs have effectively the same pro/cons and `info` admonition content. The primary difference is their YAML snippet paired with a bit of context. Removes the redundant content, minor revisions to content.
The external IP for DMS `Service` is a simpler configuration than the Load Balancer one. Shift it to be presented first.
This wraps the two tabs in an `example` admonition, then indents content correctly. No other changes to minimize diff noise.
NGINX tab relocated closer to Traefik, as the nested config tabs for DMS Postfix + Dovecot config have been extracted out to be visible after the reverse proxy config tab group instead of buried in the Traefik tab. The warning admonition is shifted into the relevant config tab where it's visible upfront for the reader to be aware of and change to the duplicate ports tab early.
Host + PROXY protocol tabs still needed this step.
Stylistic choice, could be reverted. This was done to better visually scope the boundary of content tabs, as it's less obvious where they actually end when they're not within an admonition block. This commit only adds the `!!! quote ""` lines with associated md comment above them, everything else is just indentation.
|
I've addressed some of the larger change suggestions, notably with the layout shifts and presentation of the pro/con lists. Let me know if it seems a bit overkill 😅 |
I have addressed this concern now :)
Cilium is extremely capable, and I actually place a lot of trust in my cluster in Cilium. IMO, it makes sense to have the CNI configure firewalls, etc. They are the core developers of eBPF in the Linux kernel IIRC, which already says everything 😆
👍🏼
Already reverted this exact part; I like bullet points, but not when literally chaining sentences together; that is what a "." is for :D |
I like it quite a lot! Looks excellent to me 🚀 I will push one more commit concerning the Dovecot proxy ports, and then we're good to go I think :D |
❤️
I only see a merge to master, either you didn't get around to it or didn't push? (EDIT: Oh nevermind, I see the commit!) I think you're referring to a task I've assigned myself below, so I'll take care of that.
Seems like it! I've gone over the feedback and this should summarize the current state:
Reference note:
|
Done 👍🏼 (added a comment)
Done 👍🏼 (added a comment)
GitHub won't let me answer directly to the comment, for whatever reason, hence I'll answer here:
Indeed; I thought I updated this already, but now I see that I did not do it for all occurrences. I will push another commit.
After thinking about it for a second, the first tab (where we do not duplicate ports) need "replacement", the second where we do duplicate ports, we need "extend". I will make sure the next commit takes care of that.
No, the deployment is relevant to the Service, and the service is then relevant to the ingress.
👍🏼 |
Make sure that we are consistent and sound with "replace" and "add" when it comes to ports.
- Collapsed by default `question` admonition. - `warning` admonition relocated to start of Dovecot + Postfix config tabs section. - Wrapped duplicate ports intro paragraph into `info` admonition. - Minor formatting revisions to existing content.
Minor revisions mostly, reworded the static configuration admonition. `attention` is also no longer a valid admonition type, thus it's been falling back to `note` prior doc versions since. --- "Certificates" and "Sensitive Data" tabs were merged into "Certificate" (_without `s` to match convention of tab name by manifest `kind`_). Dropped an admonition that wasn't contributing much value.
This commit is primarily wrapping content with some `example` admonitions. The `Certificate` tab does shuffle the content a little bit with minor revisions, but otherwise non-layout revisions in this commit are minimal. PROXY protocol tabs split off to a separate `example` admonition. Some longer example admonitions may instead be open by default, but are collapsible (`???+`) for improved UX.
Mostly renaming the link refs, and linking the deployment manifest tab when referencing it. `mkdocs.yml` updated to slugify the tab names for linking as a friendlier and more descriptive name. Only top-level tabs can be properly linked to, anything nested doesn't really work. PROXY protocol section, revised the `user-patches.sh` to drop mentioning running commands manually. Also added a note at the end about other Dovecot ports being properly secured for PROXY protocol if public (_most likely for port 4190, potentially POP3S?_)
|
I'm probably a little too admonition trigger happy with this PR 😂 I've done a final pass, hopefully all good now 👍 |
|
Documentation preview for this PR is ready! 🎉 Built with commit: 9131bcb |
I think it's fine for now; but the K8s site is probably at its maximum when it comes to admonition density 😂😂😂 But we should dial it down for future rewrites :) |
Description
===) to enable users to switch easily between individual parts of this docs pageThe main configuration of the manifests remained the same, they have just been indented. Hence, the diff is a bit bigger. Please apply suggestions directly, no need to have me looking over it again; this took more time than I have already.
Fixes #3865
Type of change
Checklist
docs/)CHANGELOG.md