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

Warn user on blank secrets #1218

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

andrewjanssen
Copy link

@andrewjanssen andrewjanssen commented Nov 15, 2024

Motivation

It's easy to misconfigure Kamal secrets, secret helpers, and environment variables. This is especially true for i) users who are new to Kamal, ii) users who may not have noticed the changes to secrets between Kamal 1 and 2, and iii) users like myself who are using Kamal inside Docker and/or in remote environments.

To see how often people have challenges related to secrets, consider the number of issues opened about them. For example, #970, #1001, #1108, and discussion 977. Secrets are simple yet easy to get wrong!

We should make it easier for users to get back on track when secrets are misconfigured. This PR makes it easier.

Pain point

Currently, if secret values are blank, there is no warning.

A common special case is when KAMAL_REGISTRY_PASSWORD is not set, as easily happens if the user is trying to get Kamal to work in, say, a GitHub Action. This causes kamal setup and kamal deploy to emit the following output. (This example is from #970.)

$ kamal setup
  INFO [4a37da09] Running /usr/bin/env mkdir -p .kamal on 1.2.3.4
  INFO [4a37da09] Finished in 5.948 seconds with exit status 0 (successful).
Acquiring the deploy lock...
Ensure Docker is installed...
  INFO [312bfcba] Running docker -v on 1.2.3.4
  INFO [312bfcba] Finished in 1.201 seconds with exit status 0 (successful).
  INFO [05559197] Running docker login registry.gitlab.com -u [REDACTED] -p [REDACTED] on 1.2.3.4
Releasing the deploy lock...
  Finished all in 9.5 seconds
  ERROR (SSHKit::Command::Failed): Exception while executing on host 1.2.3.4: docker exit status: 125
docker stdout: Nothing written
docker stderr: flag needs an argument: 'p' in -p
See 'docker login --help'.

It's hard to understand the root cause there.

Proposed change

  • Kamal::Secrets should show a warning whenever it loads an empty value.
  • If there is no registry password, Kamal::Configuration::Registry should raise a helpful exception instead of showing that Docker argument error.

New behaviour introduced by this PR

  1. Any time Kamal::Secrets::[] fetches a blank value, it displays a warning and tips, as shown in the next cases where a blank value is fetched by Kamal::Configuration::Registry.

  2. If a blank registry password comes from an unset environment variable, the user sees the warning and tips, and a Kamal::ConfigurationError is raised. (Similar if the env var is set to the empty string.)

bundle exec ../kamal/bin/kamal deploy
Log into image registry...
Warning: Kamal secret key 'KAMAL_REGISTRY_PASSWORD' is blank.
Tip: see .kamal/secrets:8: KAMAL_REGISTRY_PASSWORD=$KAMAL_REGISTRY_PASSWORD
Tip: the environment variable KAMAL_REGISTRY_PASSWORD is not set. Did you forget to set it?
  Finished all in 0.1 seconds
  ERROR (Kamal::ConfigurationError): The required secret 'registry.password' does not have a value.
  1. If a blank value comes from a secret helper, these are the tips:
Tip: see .kamal/secrets:8: KAMAL_REGISTRY_PASSWORD=$(echo '')
Tip: the shell command `echo ''` returned an empty value.

Other comments

It's true that Kamal::Configuration::Registry validates the presence of the password key, but it does not detect when that key is replaced with an empty value.

I'm happy to make any change to this PR, just let me know. Thanks for contributing to Kamal!

@andrewjanssen andrewjanssen force-pushed the detect-blank-secrets branch 2 times, most recently from b3a6a0e to b33ffa6 Compare November 16, 2024 00:40
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.

1 participant