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.

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

Auto-generate a stateful erlangCookie #89

Merged
merged 3 commits into from
Oct 24, 2022
Merged

Conversation

colearendt
Copy link
Contributor

@colearendt colearendt commented Jun 20, 2022

What this PR does / why we need it:

Discussed in #78

Which issue this PR fixes

(optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged)

Special notes for your reviewer:

  • Need to make sure that NOTES.txt and the new "stateful" secret generation does not error on various types of inputs (I intend to do more testing here)
  • Some design choices could be evaluated. For instance:
    • Simplify the input by passing the global context (so Namespace can be determined) and hard-coding the secret name
    • Whether or not to use NOTES.txt the way we have, or whether a generic message would be more helpful (i.e. is it too verbose? Should it be in a different location?)
    • Whether NEWS.md seems like a helpful convention
    • Whether we should be using index to protect against missing keys like I did in allow uuid being created and managed by kubernetes #73
    • Whether this is a desirable approach or erlangCookie should be refactored more heavily

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.

  • Chart Version bumped
  • e2e tests pass
  • Variables are documented in the README.md
  • Chart tgz added to /docs and index updated

@colearendt
Copy link
Contributor Author

colearendt commented Jun 20, 2022

Interesting CI error... Kernel pid terminated (application_controller) ({application_start_failure,kernel,{{shutdown,{failed_to_start_child,net_sup,{shutdown,{failed_to_start_child,auth,{"Bad characters in cookie",[{auth,ini

I think I may have messed up quoting...

EDIT: fixed!

@colearendt
Copy link
Contributor Author

For the record, I haven’t had a chance to test this locally, but I am pretty sure it will resolve #88 because the “helm upgrade” cannot proceed with a different erlang cookie

related to apache#78, apache#88. We auto-generate the secret if it is not provided, and then continue to use that value on upgrades rather than auto-generating fresh each time.
@colearendt
Copy link
Contributor Author

colearendt commented Sep 19, 2022

Alright! Finally got around to testing this! This does indeed fix the upgrading issue that is breaking CI (#78). It also works for this upgrade because it detects the previously autogenerated value and persists that! (I had an issue with base64 operations that was breaking the PR CI).

FYI @willholley

@colearendt colearendt mentioned this pull request Sep 19, 2022
@colearendt
Copy link
Contributor Author

@willholley is this good to merge? (I don't have access to do so 😄 )

@willholley willholley merged commit c93f124 into apache:main Oct 24, 2022
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.

Fix broken CI on main Changed auto-generated erlang cookie causes cluster restart issues
2 participants