Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Resolve HOST to ADDRESS only if ADDRESS is not already set #1114

Merged
merged 1 commit into from Sep 17, 2019
Merged

Resolve HOST to ADDRESS only if ADDRESS is not already set #1114

merged 1 commit into from Sep 17, 2019

Conversation

micw
Copy link
Contributor

@micw micw commented Aug 16, 2019 •
edited

What type of PR?

bug-fix

What does this PR do?

Makes the rsolving from hosts to ips at startup configurable

I rewrote the pull request after #940 was merged. Now it resolves HOSTs to ADDRESSes only of ADDRESSes are not already set. So on kubernetes we can jsut set the address and have working service discovery.

Related issue(s)

Prerequistes

Minor change, backward compatible
Changelog will be added

@mergify

This comment has been minimized.

Sign in to view
bors bot added a commit that referenced this pull request Aug 16, 2019
@bors
Try #1114:
948e3f6
@bors

This comment has been minimized.

Sign in to view
@mergify

This comment has been minimized.

Sign in to view
bors bot added a commit that referenced this pull request Aug 16, 2019
@bors
Try #1114:
288b787
@bors

This comment has been minimized.

Sign in to view
@kaiyou
Copy link
Member

kaiyou commented Aug 19, 2019

This sounds good to me, but I am not in favor of backporting immediately to 1.7. I would first give it a chance in master, then if we validate that the behavior is unchanged in a default setup maybe backport it for larger scale testing.

@kaiyou
Copy link
Member

kaiyou commented Aug 19, 2019

Another review is more than welcome :)

@muhlemmer
Copy link
Member

Conflicts with most of the things done in #940.

@micw micw mentioned this pull request Aug 21, 2019
Closed
2 tasks
@micw
Copy link
Contributor Author

micw commented Aug 21, 2019

Conflicts with most of the things done in #940.

No worries, I'll rebase on master when #940 is merged.

@mergify

This comment has been minimized.

Sign in to view
bors bot added a commit that referenced this pull request Aug 22, 2019
@bors
Try #1114:
72f260a
@bors

This comment has been minimized.

Sign in to view
@mergify

This comment has been minimized.

Sign in to view
bors bot added a commit that referenced this pull request Aug 22, 2019
@bors
Try #1114:
55a869b
@bors

This comment has been minimized.

Sign in to view
muhlemmer
muhlemmer previously requested changes Aug 22, 2019
View reviewed changes
core/admin/mailu/configuration.py Outdated Show resolved Hide resolved
core/dovecot/start.py Show resolved Hide resolved
core/nginx/config.py Show resolved Hide resolved
core/postfix/start.py Show resolved Hide resolved
services/rspamd/start.py Show resolved Hide resolved
@muhlemmer
Copy link
Member

Thanks for this PR, I imagine functionality like this is most welcome. I've left some comments that reflect my opinions. But you know what they say about opinions ;). Anyway, it's open to discussion.

@micw
Copy link
Contributor Author

micw commented Aug 22, 2019

@muhlemmer I wrote that PR before #940 was merged. I will rethink and rewrite it when I understand fully why the hosts are resolved at all. IMO hosts should not be resolved at startup at all because it breaks service discovery on both, docker and kubernetes.

@mergify

This comment has been minimized.

Sign in to view
@mergify mergify bot dismissed muhlemmer’s stale review August 23, 2019 06:44

Pull request has been modified.

bors bot added a commit that referenced this pull request Aug 23, 2019
@bors
Try #1114:
7f3c58f
@micw micw changed the title Make host to ip resolving configurable Resolve HOST to ADDRESS only if ADDRESS is not already set Aug 23, 2019
@bors

This comment has been minimized.

Sign in to view
@mergify
Copy link
Contributor

mergify bot commented Aug 31, 2019

Thanks for submitting this pull request.
Bors-ng will now build test images. When it succeeds, we will continue to review and test your PR.

bors try

Note: if this build fails, read this.

bors bot added a commit that referenced this pull request Aug 31, 2019
@bors
Try #1114:
2409d72
@micw
Copy link
Contributor Author

micw commented Aug 31, 2019

@muhlemmer Looks like I broke my test system with a combination of too many different services from different pull requests. I try to fix it to give you feedback. I'm quite busy this weekend, so it might take a few days.

@bors
Copy link
Contributor

bors bot commented Aug 31, 2019

try

Build succeeded

@micw
Copy link
Contributor Author

micw commented Aug 31, 2019 •
edited

@muhlemmer I got the system running again, now replacing the services one by one with my builds for #1114.

  1. Replaced Postfix. Env for postfix is:
bash-5.0# env|grep -e _ADDRESS -e HOST_
POD_ADDRESS_RANGE=10.42.0.0/16
LMTP_ADDRESS=mailu-dovecot:2525
ANTISPAM_ADDRESS=mailu-rspamd:11332
FRONT_ADDRESS=mailu-front
ADMIN_ADDRESS=mailu-admin

works (inbound+outbound mail)

  1. Replaced Dovecot. Env for dovecot is:
bash-5.0# env|grep -e _ADDRESS -e HOST_
REDIS_ADDRESS=mailu-redis
ANTISPAM_ADDRESS=mailu-rspamd
FRONT_ADDRESS=mailu-front
ADMIN_ADDRESS=mailu-admin

works (inbound+outbound mail)

  1. Replaced Nginx. Env for nginx is:
bash-5.0# env|grep -e _ADDRESS -e HOST_
WEBMAIL_ADDRESS=localhost
ANTISPAM_ADDRESS=mailu-rspamd:11334
ADMIN_ADDRESS=mailu-admin
WEBDAV_ADDRESS=localhost

(i have no webmail set up, so i just set the variable it to localhost)

Fails. Reason is that nginx resolver requires full qualified domain names.

Updated the environment for nginx:

bash-5.0# env|grep -e _ADDRESS -e HOST_
WEBMAIL_ADDRESS=localhost
ANTISPAM_ADDRESS=mailu-rspamd.mailu.svc.cluster.local:11334
ADMIN_ADDRESS=mailu-admin.mailu.svc.cluster.local
WEBDAV_ADDRESS=localhost

works (inbound+outbound mail, webui)

  1. Updated admin. Env for admin:
bash-5.0# env|grep -e _ADDRESS -e HOST_
POD_ADDRESS_RANGE=10.42.0.0/16
REDIS_ADDRESS=mailu-redis.mailu
WEBMAIL_ADDRESS=localhost
SMTP_ADDRESS=mailu-postfix.mailu
IMAP_ADDRESS=mailu-dovecot.mailu
POP3_ADDRESS=mailu-dovecot.mailu
AUTHSMTP_ADDRESS=mailu-postfix.mailu
FRONT_ADDRESS=mailu-front.mailu

fails because nginx expects an ip address as response from the auth server, not a hostname.

Modified the admin code: if it's a host, it is resolved on demand for each request.

works (inbound+outbound mail, webui)

  1. Updated rspamd + clamav

works (inbound+outbound mail)

@mergify
Copy link
Contributor

mergify bot commented Aug 31, 2019

Thanks for submitting this pull request.
Bors-ng will now build test images. When it succeeds, we will continue to review and test your PR.

bors try

Note: if this build fails, read this.

bors bot added a commit that referenced this pull request Aug 31, 2019
@bors
Try #1114:
71e82d0
@bors
Copy link
Contributor

bors bot commented Aug 31, 2019

try

Build succeeded

@mergify
Copy link
Contributor

mergify bot commented Aug 31, 2019

Thanks for submitting this pull request.
Bors-ng will now build test images. When it succeeds, we will continue to review and test your PR.

bors try

Note: if this build fails, read this.

bors bot added a commit that referenced this pull request Aug 31, 2019
@bors
Try #1114:
bcd83bd
@bors
Copy link
Contributor

bors bot commented Aug 31, 2019

try

Build succeeded

@mergify
Copy link
Contributor

mergify bot commented Sep 2, 2019

Thanks for submitting this pull request.
Bors-ng will now build test images. When it succeeds, we will continue to review and test your PR.

bors try

Note: if this build fails, read this.

bors bot added a commit that referenced this pull request Sep 2, 2019
@bors
Try #1114:
1ad7fd3
@bors
Copy link
Contributor

bors bot commented Sep 2, 2019

try

Build failed

@muhlemmer
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Sep 14, 2019
@bors
Try #1114:
a439652
@bors
Copy link
Contributor

bors bot commented Sep 14, 2019

try

Build succeeded

kaiyou
kaiyou reviewed Sep 17, 2019
View reviewed changes
core/admin/mailu/internal/nginx.py
return hostname, port

@tenacity.retry(stop=tenacity.stop_after_attempt(100),
Copy link
Member

Choose a reason for hiding this comment

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

I do not like this here, I would have prevered something more generic on the path to getting back to resolving things dynamically. But I guess it is a necessary evil toward it :)

@mergify
Copy link
Contributor

mergify bot commented Sep 17, 2019

bors r+

bors bot added a commit that referenced this pull request Sep 17, 2019
@bors @micw
Merge #1114
e46153c 
1114: Resolve HOST to ADDRESS only if ADDRESS is not already set r=mergify[bot] a=micw

## What type of PR?

bug-fix

## What does this PR do?

~Makes the rsolving from hosts to ips at startup configurable~

I rewrote the pull request after #940 was merged. Now it resolves HOSTs to ADDRESSes only of ADDRESSes are not already set. So on kubernetes we can jsut set the address and have working service discovery.

### Related issue(s)
- closes #1113

## Prerequistes

~Minor change, backward compatible~
Changelog will be added

Co-authored-by: Michael Wyraz <michael@wyraz.de>
@bors
Copy link
Contributor

bors bot commented Sep 17, 2019

Build succeeded

@bors bors bot merged commit de2f166 into Mailu:master Sep 17, 2019
@muhlemmer muhlemmer mentioned this pull request Oct 29, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ionutfilip ionutfilip ionutfilip left review comments

@kaiyou kaiyou kaiyou approved these changes

@muhlemmer muhlemmer muhlemmer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make dns name to ip resolving at startup configurable
4 participants
@micw @kaiyou @muhlemmer @ionutfilip