Skip to content

refactor(headless-client): remove FIREZONE_TOKEN CLI arg#7770

Merged
thomaseizinger merged 7 commits into
mainfrom
fix/no-positional-token-arg
Jan 21, 2025
Merged

refactor(headless-client): remove FIREZONE_TOKEN CLI arg#7770
thomaseizinger merged 7 commits into
mainfrom
fix/no-positional-token-arg

Conversation

@thomaseizinger
Copy link
Copy Markdown
Member

@thomaseizinger thomaseizinger commented Jan 16, 2025

The current CLI of the headless-client allows passing the token as a positional parameter in addition to an env variable. This can be very confusing if you make a spelling error in the command that you are trying to pass to the CLI, i.e. standalone. A misspelled command will be interpreted as the token to use to connect to the portal without any warning that it is similar to a command. The env variable FIREZONE_TOKEN is completely ignored in that case.

To fix this, we remove the ability to pass the token via stdin. The token should instead be set via en env variable or read from a file at FIREZONE_TOKEN_PATH.

@vercel
Copy link
Copy Markdown

vercel Bot commented Jan 16, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
firezone ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 21, 2025 1:53pm

@jamilbk
Copy link
Copy Markdown
Member

jamilbk commented Jan 16, 2025

This is a breaking API change. Is there perhaps another way to alleviate the confusion without the CLI change?

@thomaseizinger
Copy link
Copy Markdown
Member Author

This is a breaking API change. Is there perhaps another way to alleviate the confusion without the CLI change?

Yeah I know that it is breaking the CLI. What is our policy on such changes? There is an easy migration to env params / secret files and the UX is much better if we don't have positional parameters. I wasted almost half an hour trying to debug why my headless client wouldn't connect any more, creating new actors in the portal, rotating the token etc and was only able to figure out the problem by making a source code change to print out the token. Users are unlikely to build from source so their debugging experience is much worse.

@jamilbk
Copy link
Copy Markdown
Member

jamilbk commented Jan 16, 2025

This is a breaking API change. Is there perhaps another way to alleviate the confusion without the CLI change?

Yeah I know that it is breaking the CLI. What is our policy on such changes? There is an easy migration to env params / secret files and the UX is much better if we don't have positional parameters. I wasted almost half an hour trying to debug why my headless client wouldn't connect any more, creating new actors in the portal, rotating the token etc and was only able to figure out the problem by making a source code change to print out the token. Users are unlikely to build from source so their debugging experience is much worse.

Hm could we perhaps enforce the user to specify a --token param with it? I know we have customers using the headless client in CI jobs but I'm not sure which method they're using to supply it, so depending on how they've got it set up we might be costing them 30 minutes too, or more.

@thomaseizinger
Copy link
Copy Markdown
Member Author

Hm could we perhaps enforce the user to specify a --token param with it?

That is what this PR does, it converts it from a positional arg to a named on. I should probably clarify that in the changelog.

Comment thread website/src/components/Changelog/Headless.tsx Outdated
Comment thread website/src/components/Changelog/Headless.tsx Outdated
Signed-off-by: Thomas Eizinger <thomas@eizinger.io>
@jamilbk jamilbk changed the title fix(headless-client): disallow token as positional parameter refactor(headless-client): force --token arg for positional parameter Jan 17, 2025
@jamilbk jamilbk changed the title refactor(headless-client): force --token arg for positional parameter refactor(headless-client): force --token for CLI arg Jan 17, 2025
Copy link
Copy Markdown
Member

@jamilbk jamilbk left a comment

Choose a reason for hiding this comment

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

Looks like just needs a test updated.

@thomaseizinger thomaseizinger changed the title refactor(headless-client): force --token for CLI arg refactor(headless-client): remove FIREZONE_TOKEN CLI arg Jan 21, 2025
@thomaseizinger
Copy link
Copy Markdown
Member Author

@jamilbk Looking at the test made me find #4682. Given what is documented there, I opted to entirely remove the firezone-token from the clap CLI args. It must be passed as an env variable now in order to not leak it.

Comment thread website/src/components/Changelog/Headless.tsx Outdated
Co-authored-by: Jamil <jamilbk@users.noreply.github.com>
Signed-off-by: Thomas Eizinger <thomas@eizinger.io>
@thomaseizinger thomaseizinger added this pull request to the merge queue Jan 21, 2025
Merged via the queue into main with commit e50b719 Jan 21, 2025
@thomaseizinger thomaseizinger deleted the fix/no-positional-token-arg branch January 21, 2025 14:38
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.

2 participants