Skip to content

docs: miscellaneous improvements#3219

Merged
georglauterbach merged 31 commits intomasterfrom
docs/improvements
Apr 8, 2023
Merged

docs: miscellaneous improvements#3219
georglauterbach merged 31 commits intomasterfrom
docs/improvements

Conversation

@georglauterbach
Copy link
Copy Markdown
Member

@georglauterbach georglauterbach commented Apr 3, 2023

Description

@polarathene I know you're loaded, but due to recent issues & discussions, I felt the need to improve out docs a bit further. I hope you don't mind reviewing - I'd love to see this being merged before v12.0.0 goes live on the 9th. Just tell me whether you think you'll be able to review until then :) If you have some suggestions, please go ahead and apply them immediately ❤️ I hope the preview helps a bit in visualising the changes and the decluttering. If you don't have time, just tell me, and I will do a more in-depth self-review and then merge it with admin privilege - because I really want this in the v12 docs.

@RebelSoftware this PR includes some of the things we discussed and I hope the docs are now a bit more clear on the subject. Go ahead and have a look at the preview :)

Type of change

  • Improvement (non-breaking change that does improve existing functionality)

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas

The page has been extended to include preliminary setup as well. It is
better structured and more coherent now. I stole a bit from the mailcow
documentation (not literally, just ideas).

I tried to apply a lot of decluttering and some reordering. Please see
the documentation preview and compare :)
I would like users to directly run `setup` inside the container. With
previous commits, I already replaced `./setup.sh` with `docker exec ...`
and this change puts the page about `setup.sh` further down.
Some decluttering and restructuring. No major rewrites, just a bit of
styling here and there.
removing now superflous detailed installation
@georglauterbach georglauterbach added kind/improvement Improve an existing feature, configuration file or the documentation area/documentation labels Apr 3, 2023
@georglauterbach georglauterbach added this to the v12.0.0 milestone Apr 3, 2023
@georglauterbach georglauterbach self-assigned this Apr 3, 2023
We had redundant information on the introduction page, and this was
coherently moved to the "Understanding the Ports" article
@polarathene

This comment was marked as resolved.

Copy link
Copy Markdown
Member

@polarathene polarathene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an incomplete review, and I noticed you've applied changes since I started it 😅

I'll try pick it up again in another 12 hours and apply any changes after review is complete 👍

Comment thread README.md Outdated
Comment thread docs/content/config/security/understanding-the-ports.md Outdated
Comment thread docs/content/introduction.md
Comment thread docs/content/introduction.md Outdated
Comment thread docs/content/introduction.md Outdated
Comment thread docs/content/introduction.md Outdated
Comment thread docs/content/introduction.md Outdated
Comment thread docs/content/introduction.md Outdated
Comment thread docs/content/introduction.md Outdated
Comment thread docs/content/introduction.md Outdated
@georglauterbach
Copy link
Copy Markdown
Member Author

This is an incomplete review, and I noticed you've applied changes since I started it 😅

I'm sorry about that; mostly minor nitpicks and the debugging page I revised because I felt it would need to go into v12/latest as well :)

I'll try pick it up again in another 12 hours and apply any changes after review is complete 👍

Thank you very much! I promise to not change this PR anymore. When you're done reviewing, just go ahead and apply your feedback and merge this PR if you have no more questions. I'd be doing this anyway :) I looked through all your suggestions and they are excellent!

Copy link
Copy Markdown
Member

@polarathene polarathene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part 2 of review.

I've still got a couple sections left to go over tomorrow. I'll apply bulk of suggestions after completing that.

Comment thread docs/content/config/environment.md
Comment thread docs/content/config/debugging.md Outdated
Comment thread docs/content/faq.md Outdated
Comment on lines +236 to +250
### My Connection is Refused / I Do No Get a Response At All

You see errors like "Connection Refused" and "Connection closed by foreign host", or you cannot connect at all? You may not be able to connect with your mail client (MUA)? Make sure to check Fail2Ban did not ban you (for exceeding the number of tried logins for example)! You can run

```bash
docker exec <CONTAINER NAME> setup fail2ban
```

and check whether your IP address appears. Use

```bash
docker exec <CONTAINER NAME> setup fail2ban unban <YOUR IP>
```

to unban the IP address.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a review comment for better git blame tracking (at least via Github Web UI if viewing this PR).

No change necessary.


Migrated from debugging.md and revised version of:

## Testing Connection

I spent HOURS trying to debug "Connection Refused" and "Connection closed by foreign host" errors when trying to use telnet to troubleshoot my connection. I was also trying to connect from my email client (macOS mail) around the same time. Telnet had also worked earlier, so I was extremely confused as to why it suddenly stopped working. I stumbled upon `fail2ban.log` in my container. In short, when trying to get my macOS client working, I exceeded the number of failed login attempts and fail2ban put dovecot and postfix in jail! I got around it by whitelisting my ipaddresses (my ec2 instance and my local computer)

```sh
sudo su
docker exec -it mailserver bash
cd /var/log
cat fail2ban.log | grep dovecot
# Whitelist IP addresses:
fail2ban-client set dovecot addignoreip <server ip>  # Server
fail2ban-client set postfix addignoreip <server ip>
fail2ban-client set dovecot addignoreip <client ip>  # Client
fail2ban-client set postfix addignoreip <client ip>
# This will delete the jails entirely - nuclear option
fail2ban-client stop dovecot
fail2ban-client stop postfix
```

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could (in the future) think about adding the ignoreip automatically with our scripts if that somehow makes sense (but I see no obvious reason to do so at the moment).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you get more users needing it, sure. This was contributed by one this year I think and I noticed that we don't document it, so users will need to hunt it down like this contributor did.

Your revised version moved into the FAQ instead instructs how to unban, which is helpful, I don't use F2B and assume the ignore is mostly useful when troubleshooting or working around an issue where unban would be tedious..

Comment thread docs/content/config/debugging.md Outdated
Comment thread docs/content/config/debugging.md Outdated
Comment thread docs/content/usage.md Outdated
Comment thread docs/content/usage.md Outdated
Comment thread docs/content/usage.md Outdated
Comment thread docs/content/usage.md Outdated
Comment thread docs/content/usage.md Outdated
georglauterbach added a commit that referenced this pull request Apr 6, 2023
- simplified the introductory text
- added link to the (new) debugging docs (see #3219)
georglauterbach added a commit that referenced this pull request Apr 6, 2023
- simplified the introductory text
- added link to the (new) debugging docs (see #3219)
georglauterbach and others added 3 commits April 6, 2023 16:10
**except** for `setup.sh`-related contentent. This will receive a
dedicated PR.
Copy link
Copy Markdown
Member

@polarathene polarathene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't get around to tackling the last review items I intended to:

  • Ports new + relocated content.
  • Deletion of bottom half of install example.

I should have time for that tomorrow. Really great work though! 💯

Comment thread docs/content/config/environment.md
Comment thread docs/content/faq.md Outdated
Comment thread docs/content/faq.md Outdated
Comment on lines +236 to +250
### My Connection is Refused / I Do No Get a Response At All

You see errors like "Connection Refused" and "Connection closed by foreign host", or you cannot connect at all? You may not be able to connect with your mail client (MUA)? Make sure to check Fail2Ban did not ban you (for exceeding the number of tried logins for example)! You can run

```bash
docker exec <CONTAINER NAME> setup fail2ban
```

and check whether your IP address appears. Use

```bash
docker exec <CONTAINER NAME> setup fail2ban unban <YOUR IP>
```

to unban the IP address.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you get more users needing it, sure. This was contributed by one this year I think and I noticed that we don't document it, so users will need to hunt it down like this contributor did.

Your revised version moved into the FAQ instead instructs how to unban, which is helpful, I don't use F2B and assume the ignore is mostly useful when troubleshooting or working around an issue where unban would be tedious..

Comment thread docs/content/introduction.md Outdated
Comment thread docs/content/usage.md Outdated
Comment thread docs/content/usage.md Outdated
Copy link
Copy Markdown
Member

@polarathene polarathene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll make some edits that suggestions feature isn't able to do, and rewrite some content, then finally complete 🎉

Comment thread docs/content/examples/tutorials/basic-installation.md
Comment thread docs/content/introduction.md Outdated
Comment thread docs/content/config/security/understanding-the-ports.md Outdated
Comment thread docs/content/config/security/understanding-the-ports.md Outdated
Comment thread docs/content/config/security/understanding-the-ports.md Outdated
Comment thread docs/content/config/security/understanding-the-ports.md Outdated
Copy link
Copy Markdown
Member

@polarathene polarathene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review complete 💪

Comment thread docs/content/examples/tutorials/basic-installation.md
Comment thread docs/content/examples/tutorials/basic-installation.md
Comment thread docs/content/examples/tutorials/basic-installation.md
Comment thread docs/content/examples/tutorials/basic-installation.md

6. Get an SSL certificate, [we have a guide for you here][docs-ssl] (_Let's Encrypt_ is a popular service to get free SSL certificates).

7. Start `docker-mailserver` and check the terminal output for any errors: `docker-compose up`.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise, it looks like the usage.md docs jump straight to using setup only indirectly mentioning up / down vs start / stop gotcha and "On first start".

I suppose the single line here is a bit more direct of an instruction to the reader 😅 (the advice to check output for errors is probably good to communicate too, and could link to the debugging page)


The numbered steps is probably a bit more helpful in structure / flow, as that seems to get a bit messy when looking at the actual rendering of the docs pages like usage.md.

For example we have "Get Up and Running" with two paragraphs and some admonitions interleaved, and then a larger heading "Further Miscellaneous Steps", but the "That's it! It really is that easy." doesn't feel like the guide finishes a running setup here.

Comment on lines -129 to -136
# Using letsencrypt for SSL/TLS certificates
- SSL_TYPE=letsencrypt
# Allow sending emails from other docker containers
# Beware creating an Open Relay: https://docker-mailserver.github.io/docker-mailserver/latest/config/environment/#permit_docker
- PERMIT_DOCKER=network
# You may want to enable this: https://docker-mailserver.github.io/docker-mailserver/latest/config/environment/#spoof_protection
# See step 8 below, which demonstrates setup with enabled/disabled SPOOF_PROTECTION:
- SPOOF_PROTECTION=0
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main docker-compose.yaml example we advise to start with omits these, deferring to mailserver.env. That file is not here keeping config simple, you can just copy/paste the wget approach.

Focuses on the most common SSL_TYPE value, and I assume DMS is popular for having other containers send mail through it. The SPOOF_PROTECTION doesn't matter too much here, but for the guide we get a nice comment about it.

Comment thread docs/content/examples/tutorials/basic-installation.md
Comment thread docs/content/examples/tutorials/basic-installation.md
Comment thread docs/content/examples/tutorials/basic-installation.md
Comment thread docs/content/examples/tutorials/basic-installation.md
Community script was recently updated for the private file paths, but not likely providing much benefit anymore over the change detection service that can do this automatically for non-LDAP users.
Comment thread docs/content/config/debugging.md
Comment thread docs/content/config/debugging.md
Comment thread docs/content/usage.md Outdated
Comment thread docs/content/usage.md
The script expects `/etc/postfix/ssl/cert` and `/etc/postfix/ssl/key` files to be configured paths for both Postfix and Dovecot to use.

Since the `docker-mailserver` 10.2 release, certificate files have moved to `/etc/dms/tls/`, and the file name may differ depending on provisioning method.
This is a community contributed script, and in most cases you will have better support via our _Change Detection_ service (_automatic for `SSL_TYPE` of `manual` and `letsencrypt`_) - Unless you're using LDAP which disables the service.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section can be dropped in a future release. The change detection service can support LDAP for updating certs without needing a community script, just needs to conditionally avoid the other config change handling.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏼

@polarathene
Copy link
Copy Markdown
Member

polarathene commented Apr 8, 2023

Remaining tasks

Overview of remaining feedback to address. The majority of these are for tracking of future changes to PR, thus not blocking approval:

polarathene
polarathene previously approved these changes Apr 8, 2023
Copy link
Copy Markdown
Member

@polarathene polarathene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good for merge 👍

Feel free to go over my review feedback or the remaining tasks (that aren't necessary for this PR), but all good on my end 😀

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 8, 2023

Documentation preview for this PR is ready! 🎉

Built with commit: f4a1704

@georglauterbach
Copy link
Copy Markdown
Member Author

I noted many of the review comments, and will provide follow-up PRs. I have adjusted the "Inward => Inbound" / "Outward => Outbound" issue, and will merge this PR now. v12.1.0 will then probably contain all the follow-ups that we discussed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/documentation kind/improvement Improve an existing feature, configuration file or the documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants