-
Notifications
You must be signed in to change notification settings - Fork 89
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
Support MP deployments with self-generated TLS certificate #3721
Conversation
Important Auto Review SkippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM other than the password/defaults.
@@ -17,9 +17,9 @@ bacalhau_accept_networked_jobs = true | |||
|
|||
bacalhau_otel_collector_endpoint = "http://analytics.bacalhau.tech:4318" | |||
|
|||
bacalhau_requester_api_token = "token_for_requester_api" | |||
bacalhau_compute_api_token = "token_for_compute_api" | |||
bacalhau_requester_api_token = "password" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did you mean to check in these as defaults? Terraform offers a feature to generate a random string (or this should be left blank and required)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If these values are blank a random token will automatically be generated and used. I commit this since typing password
was easier than the former when dev-ing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hate that we auto-generate tokens and ask the users to go through the logs or to a local file on desk to get whatever token was generated. We should either allow unsecure access by default, or fail if no token was provided. but we shouldn't auto generate tokens
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hate that we auto-generate tokens
As mentioned above, we only auto generate tokens if the users don't provide one as part of the deployment. Users are able to set the token to whatever value they'd like to use, and in the event they do so we will not auto-generate a token and use the one they've provided.
ask the users to go through the logs or to a local file on desk to get whatever token was generated
Tokens are returned via terraform outputs once the deployment completes. Users do not need to look in the logs for the tokens.
We should either allow unsecure access by default, or fail if no token was provided. but we shouldn't auto generate tokens
What's the motivation for this? I believe it's a some-what standard practice to generate a password (token) on the behalf of a user and instruct them to change it once they login, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think i'm ok as well if we autogenerate (only if not provided) and the token goes into a defined file somewhere. Not logs. This feels like best practice to me.
b464bbf
to
93b3427
Compare
@@ -0,0 +1,107 @@ | |||
# create the bacalhau user that will run the bacalhau process | |||
packages: | |||
- vim |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
${bacalhau_service_file} | ||
|
||
# bacalhau install-script to install a custom bacalhau version | ||
- path: /etc/install-bacalhau.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/etc probably not the best place for this. /usr/local/sbin's the best place for binaries that need su,or /usr/local/bin if it is intended for wider use. If it's only used by the installer, then probably less of an issue.
${bacalhau_service_file} | ||
|
||
# bacalhau install-script to install a custom bacalhau version | ||
- path: /etc/install-bacalhau.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you change the other one (not compulsory) then change this one too.
@@ -74,7 +74,7 @@ locals { | |||
bacalhau_env_vars = { | |||
LOG_LEVEL = "debug" | |||
BACALHAU_NODE_LOGGINGMODE = "default" | |||
BACALHAU_DIR = "/data" | |||
BACALHAU_DIR = "/bacalhau_repo" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is okay, but wonder if it might be better in /opt/bacalhau/* eventually ?
c.f. https://refspecs.linuxfoundation.org/FHS_3.0/fhs/index.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Couple suggestions for potential directory changes, but nothing urgent.
1033c70
to
4293be3
Compare
4293be3
to
ed997ea
Compare
Generates a TLS certificate for communication between clients and requester node.