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

Set CN to container DNS name #14951

Merged
merged 1 commit into from
Jul 19, 2023
Merged

Set CN to container DNS name #14951

merged 1 commit into from
Jul 19, 2023

Conversation

lukebakken
Copy link
Contributor

@lukebakken lukebakken mentioned this pull request Jun 28, 2023
Merged
tianon
tianon reviewed Jun 28, 2023
View reviewed changes
Copy link
Member

@tianon tianon left a comment

Choose a reason for hiding this comment

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

Oh nice, that's way easier than I was afraid this might end up being!

Do you think we should also update our rabbitmq-env.conf file to opt into this new default explicitly too so it's more clear what's happening?

Additionally, could you please remove the @tianon from your commit message? It ends up sending an email from your fork notifying me of the commit, will likely send me another email when this gets merged, and then will send me emails again if anyone ever mirrors this repository elsewhere without a fork or rebases the history such that it's a different commit (which has happened quite a few times in our project's history 😭).

As for testing, it's a little bit janky, but I think I can point my PR over there at this PR (or maybe @yosifkit still has his local build he can pull this down and test against 👀).

test/tests/rabbitmq-tls/run.sh Outdated Show resolved Hide resolved
@lukebakken
Copy link
Contributor Author

OK, round two. I'm giving an optfile a try next.

@yosifkit
Copy link
Member

3.13-rc still fails with the inet-dist-tls.config. (I did double check that 3.12 is successful to try to rule out anything environmental)

@lukebakken
Copy link
Contributor Author

Thanks. OK my next step will be to try out what run.sh does in my local environment to see if I can repro.

@lukebakken
Copy link
Contributor Author

I was able to reproduce the issue locally and it appears that commit 7335ae3 fixes the issue using 3.13.0-beta.1 and OTP 26. I'm not exactly sure why so I'm going to test some more but let's see how it works with CI here.

@lukebakken
Copy link
Contributor Author

This is interesting, I can start a cluster using 3.13.0-beta.1 and OTP 26 and my code here https://github.com/lukebakken/docker-rabbitmq-cluster/tree/tls

...so my next step is to generate certs for run.sh using tls-gen rather than the method currently used. I won't be able to return to this until July 17 or so.

@lukebakken
Copy link
Contributor Author

Turns out that 3.13 has a bug in check_port_connectivity when used with TLS (cc @michaelklishin). I'll create an issue in rabbitmq/rabbitmq-server and will link this one to it.

@lukebakken
Copy link
Contributor Author

Looks like the bug is actually in Erlang/OTP - erlang/otp#7497

@lukebakken lukebakken marked this pull request as ready for review July 14, 2023 16:02
@lukebakken
Copy link
Contributor Author

After resolving the discussion in erlang/otp#7497, this PR is ready for review!

@lukebakken @tianon
Set CN to container DNS name
e3a1857 
Related to docker-library/rabbitmq#652

Give a TLS dist optfile a try

Remove `fail_if_no_peer_cert` option for client. It does not seem to be supported by OTP 26 🤔
@tianon tianon force-pushed the docker-library_rabbitmq-652 branch from dee1b88 to e3a1857 Compare July 19, 2023 19:38
@tianon tianon merged commit 8fce1b5 into docker-library:master Jul 19, 2023
6 checks passed
@lukebakken lukebakken deleted the docker-library_rabbitmq-652 branch July 19, 2023 20:25
@lukebakken
Copy link
Contributor Author

Thanks everyone.

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

@tianon tianon tianon left review comments

@yosifkit yosifkit yosifkit left review comments

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

Successfully merging this pull request may close these issues.
3 participants
@lukebakken @yosifkit @tianon