Skip to content

fix: change default value for receptor_replace_tls to true#76

Open
kurokobo wants to merge 1 commit intoansible:mainfrom
kurokobo:replace
Open

fix: change default value for receptor_replace_tls to true#76
kurokobo wants to merge 1 commit intoansible:mainfrom
kurokobo:replace

Conversation

@kurokobo
Copy link
Copy Markdown
Contributor

@kurokobo kurokobo commented Jan 24, 2024

related #71

It is unnatural (unintended) behavior that the cert and key on the target node are not updated even if the cert and key that passed by custom_tls_* or custom_ca_* are updated on the control node.

This PR changes default value for receptor_replace_tls to true.

Please let me know if there are situations where I should specify false for this option. I can't imagine any.

@TheRealHaoLiu
Copy link
Copy Markdown
Member

i recall the original reason why we defaulted to false because everytime we generate a bundle a new cert/key is generated and we "for some reason" didn't want to always replacing it...

@fosterseth did u recall more details?

@TheRealHaoLiu
Copy link
Copy Markdown
Member

o right i remember a bit now...
reloading cert is a hard restart on receptor
if you just want to add a peer when u regenerate and rerun the instance install bundle it will cause a hard restart of receptor instead of a reload of the config

@kurokobo
Copy link
Copy Markdown
Contributor Author

@TheRealHaoLiu
Thanks for clarifing the background, that makes sense.
However, isn't it just a matter of AWX implementation that the key and certificate are newly generated each time on generating install bundle, and wouldn't it be more natural for this option to true as the default behavior of the role? I think users expect that simply the certs will be replaced if the certs on the control node are updated. I don't think the implementation of this collection should depend on the AWX implementation.

How about:

  • Change the default value to true for receptor_replace_tls (this PR)
  • Specify receptor_replace_tls: false in group_vars/all.yaml in install bundle on the AWX side (no PR yet)

If there is receptor_replace_tls in group_vars/all.yaml, it will be easier for users to modify it at runtime. If no, users have to examine the documentation and implementation of this collection to determine why certificates are not being replaced.

@kurokobo
Copy link
Copy Markdown
Contributor Author

We could leave it as is, but I had trouble that certs were not replaced in the situation I described in this comment: #71 (comment)

@jbradberry jbradberry requested review from TheRealHaoLiu and removed request for jbradberry June 10, 2024 17:02
@joekohlsdorf
Copy link
Copy Markdown

I ran into the same issue and wasted a lot of time trying to debug it.

The Restart Receptor notify when TLS certificates are swapped should not be necessary. Certificates are referenced in the listeners and peers config sections which are reloadable. See: https://ansible.readthedocs.io/projects/receptor/en/latest/user_guide/interacting_with_nodes.html#reload

Therefore I propose to change the notify after certificate upload to Reload Receptor which resolves the concerns of @TheRealHaoLiu.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants