docs: rewrite Kubernetes page by georglauterbach · Pull Request #3928 · docker-mailserver/docker-mailserver · GitHub

Description

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 dedicated proxy-ports
removed old, outdated information

The 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

Improvement (non-breaking change that does improve existing functionality)
This change is a documentation update

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation (README.md or the documentation under docs/)
I have added information about changes made in this PR to CHANGELOG.md

- 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

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`.

Another big round of feedback 😅

I can apply some of these changes directly, perhaps I'll take care of the formatting/layout ones too that I couldn't suggest via web UI. If you disagree with those changes just revert them 👍

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?

Suggested change

      
                    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.

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.

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 Deployment config 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 😅

#3928 (comment)

You're not configuring imap-proxy below, why is this here?

Can you elaborate? I am not sure if I understand.

Your dovecot.cf config example also did IMAP / 143, but didn't have a matching proxy port definition in YAML. Didn't make sense to keep it there, especially since you don't in other snippets.

I have addressed this concern now :)

I'm still not a fan of an entire subnet as is encouraged here with the pod CIDR, but I understand that's a k8s concern. You just have to trust that there is no networking surprises that permit traffic to slip through and inject their own PROXY protocol header.

It's actually required because the ingress pod IP can change at any point of the pod is destroyed (for whatever reason). Using the Pod-CIDR here is absolutely correct.

I understand, I'm just not fond of the fact that there's a lack of control there forcing the trusted network to be wider than it should otherwise be.

Properly firewalling the connections is the Container Network Interface's job! I use Cilium, and they have a nice editor for network policies, have a look :D

That tool does look pretty cool!

I just don't have the greatest of trust with networking and containers from all the surprises I've experienced with Docker (published ports bypass any firewall rules, the default IPv6 host route to IPv4 private subnet gateway IP masquerading, and more niche gotchas). Maybe it's not so bad in k8s land, I just lack the time to investigate 😅

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 😆

That said, Postfix doesn't have the same concept of trusted networks for PROXY protocol, which kinda makes it a moot point security wise 🤷‍♂️

👍🏼

#3928 (comment)

I also cannot understand why we suddenly use bullet points for normal sentences - why do it here now?

Probably a bad habit of mine 🙈

My main gripe is usually with our docs typography and larger paragraphs not being as pleasant to read. Whereas that's a non-issue for me with Github comments. Thus I tend to favor splitting paragraphs sometimes and much shorter lengths, potentially as bullet points 😓

Other than the typography concern, probably just different styles of communication we have. If you disagree with the usage feel free to revert when I do introduce it 👍

Already reverted this exact part; I like bullet points, but not when literally chaining sentences together; that is what a "." is for :D

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 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 like it quite a lot! Looks excellent to me 🚀

❤️

I will push one more commit concerning the Dovecot proxy ports

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.

and then we're good to go I think :D

Seems like it! I've gone over the feedback and this should summarize the current state:

I need to:
- Add this line: docs: rewrite Kubernetes page #3928 (comment)
- Adjust the dovecot.cf example + raise awareness of related service ports demonstrated on the main PROXY protocol docs page: docs: rewrite Kubernetes page #3928 (comment)
I'd appreciate:
- Clarity on the static config being additionally read-only, or if it's only not persisting changes (expected).
- Confirmation on if all approaches preserve the client IP correctly, or if that is a burden for you to verify (acceptable).
- Clarity on which Deployment config is referenced to extend, I only got a ":+1:"?
Delayed to future PRs:
- AppArmor annotation migrate to SecurityContext.
- Improved context on minimal cap requirements (probably applicable to compose.yaml equivalent config too? Default caps here)
- Update apiVersion: traefik.io/v1alpha1 for Traefik 3 once it's officially released as support moves out of alpha.

Reference note:

Service.spec.ports.targetPort value refers to Deployment.spec.containers.ports.name, likewise Service.spec.selector.app value is matched by Deployment.spec.selector.matchLabels.app value.

I'd appreciate:

Clarity on the static config being additionally read-only, or if it's only not persisting changes (expected).

Done 👍🏼 (added a comment)

Confirmation on if all approaches preserve the client IP correctly, or if that is a burden for you to verify (acceptable).

Done 👍🏼 (added a comment)

Clarity on which Deployment config is referenced to extend, I only got a ":+1:"?

GitHub won't let me answer directly to the comment, for whatever reason, hence I'll answer here:

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?

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.

Or were you referring to extending the Deployment config of the sibling tab? It's a little vague which one is referred to here.

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.

Or is this section of config relevant to other containers within the cluster, not only for the ingress?

No, the deployment is relevant to the Service, and the service is then relevant to the ingress.

Delayed to future PRs:

AppArmor annotation migrate to SecurityContext.

Improved context on minimal cap requirements (probably applicable to compose.yaml equivalent config too? Default caps here)

Update apiVersion: traefik.io/v1alpha1 for Traefik 3 once it's officially released as support moves out of alpha.

👍🏼

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

Merge if you're happy with the current revision 👍

I'm probably a little too admonition trigger happy with this PR 😂

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 :)

rewrite Kubernetes page

d2a57be

- 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 Update an existing feature, configuration file or the documentation labels Mar 7, 2024

georglauterbach added this to the v14.0.0 milestone Mar 7, 2024

georglauterbach requested a review from polarathene March 7, 2024 10:12

georglauterbach self-assigned this Mar 7, 2024

polarathene requested changes Mar 8, 2024

View reviewed changes

polarathene added 2 commits March 9, 2024 00:41

Apply suggestions from code review

93298b9

Update kubernetes.md

65abc56

polarathene reviewed Mar 8, 2024

View reviewed changes

Comment thread docs/content/config/advanced/kubernetes.md Outdated

polarathene and others added 4 commits March 9, 2024 00:50

Update docs/content/config/advanced/kubernetes.md

b8eb1d9

add note on the service type for PROXY protocol

faff9c3

move Deployment section

aa6894e

rephrasing for exposing DMS section

48fddf3

georglauterbach added 5 commits March 9, 2024 16:51

rewrite "Exposing your Mail ..." section

4d68958

It should be better structured now and container a more uniform structure.

correct apiVersion for Deployment

e53095c

make port names consistent

9c7a53e

remove superflous CIDR

78ad4dc

praise be linters

bcf933c

georglauterbach added 7 commits March 9, 2024 17:10

wording

8c39bdc

more wording

cb704d5

formatting

a6d8715

drop content that is not relevant or incorrect

4dfe11e

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.

remove superflous tmp-files

b561b17

This is not required because `readOnlyRootFilesystem: false`.

more details for PROXY-protocol NGINX

8edf55e

add one more requirement for using PROXY protocol

edf0a62

polarathene requested changes Mar 10, 2024

View reviewed changes

polarathene added 6 commits March 10, 2024 21:51

docs(chore): Manual IP config tabs merge (part 1)

239dc3e

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.

docs(chore): Manual IP config tabs merge (part 2)

c1a9a7f

The external IP for DMS `Service` is a simpler configuration than the Load Balancer one. Shift it to be presented first.

docs(chore): Manual IP config tabs merge (part 3)

fc0ef0f

This wraps the two tabs in an `example` admonition, then indents content correctly. No other changes to minimize diff noise.

docs(chore): Wrap exposure tabs in example admonitions

e270525

Host + PROXY protocol tabs still needed this step.

add leftover Dovecot ports for PROXY protocol

b4799e8

georglauterbach requested a review from polarathene March 10, 2024 17:48

Merge branch 'master' into docs/kubernetes

57fa506

georglauterbach and others added 6 commits March 11, 2024 11:56

improve PROXY protocol ports further

db20aaa

Make sure that we are consistent and sound with "replace" and "add" when it comes to ports.

add warning for firewalling traffic in K8s

8eb966d

docs: Adjust k8s PROXY protocol tab

336452c

- 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.

polarathene previously approved these changes Mar 12, 2024

View reviewed changes

lint: Praise be

5005dc4

polarathene dismissed their stale review via 5005dc4 March 12, 2024 02:42

Merge branch 'master' into docs/kubernetes

9131bcb

polarathene approved these changes Mar 12, 2024

View reviewed changes

georglauterbach merged commit 2133b51 into master Mar 12, 2024

georglauterbach deleted the docs/kubernetes branch March 12, 2024 08:31

+                          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
+                          ```

-              ---
-              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.

	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`.

docs: rewrite Kubernetes page by georglauterbach · Pull Request #3928 · docker-mailserver/docker-mailserver · GitHub

Uh oh!

Uh oh!

docs: rewrite Kubernetes page - #3928

docs: rewrite Kubernetes page#3928georglauterbach merged 39 commits intomasterdocker-mailserver/docker-mailserver:masterfrom docs/kubernetesdocker-mailserver/docker-mailserver:docs/kubernetesCopy head branch name to clipboard

Conversation

georglauterbach commented Mar 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

Checklist

Uh oh!

polarathene left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

georglauterbach commented Mar 8, 2024

Uh oh!

georglauterbach commented Mar 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

polarathene left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

polarathene Mar 9, 2024

Choose a reason for hiding this comment

Uh oh!

georglauterbach Mar 11, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

polarathene Mar 10, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

polarathene commented Mar 10, 2024

Uh oh!

georglauterbach commented Mar 10, 2024

Uh oh!

georglauterbach commented Mar 10, 2024

Uh oh!

polarathene commented Mar 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

georglauterbach commented Mar 11, 2024 • edited by polarathene Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

polarathene commented Mar 12, 2024

Uh oh!

github-actions Bot commented Mar 12, 2024

Uh oh!

polarathene left a comment

Choose a reason for hiding this comment

Uh oh!

georglauterbach commented Mar 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

docs: rewrite Kubernetes page#3928
georglauterbach merged 39 commits into
masterfrom
docs/kubernetes

georglauterbach commented Mar 7, 2024 •
edited
Loading

georglauterbach commented Mar 9, 2024 •
edited
Loading

polarathene commented Mar 11, 2024 •
edited
Loading

georglauterbach commented Mar 11, 2024 •
edited by polarathene
Loading