Adds support for the Confluent CLI against Confluent Cloud #184
Conversation
There was a problem hiding this comment.
Thank you for this contribution! It's awesome that you thought about adding all the credential types from the get-go. We hope to support multiple credential types in the future, you've really made it easy to ensure that having support for both Confluent Cloud and Platform will be a breeze, at that point 😄
Only a few comments and good to go!
plugins/confluent/confluent.go
Outdated
| Runs: []string{"confluent"}, | ||
| DocsURL: sdk.URL("https://docs.confluent.io/confluent-cli/current/overview.html"), | ||
| NeedsAuth: needsauth.IfAll( | ||
| needsauth.ForCommand("login"), |
There was a problem hiding this comment.
Does the login command write credentials to disk to pass them to the other commands? One of the goals of Shell Plugins is to avoid credentials ever having to touch the disk in plaintext. Instead they're stored encrypted in 1Password and are only passed to the tools that need the credential in memory and only to authorized terminal sessions.
In case Confluent uses a file to pass the credentials between the login and other commands, we can use the file-provisioner to pass the crednetials directly to the commands that require them (without needing to run login beforehand). It uses a temporary in-memory file to pass the credentials to the CLI. As a reference example, a.o. the MySQL plugin use a temporary file to pass the credential.
Let us know if we can help out. In addition to here on GitHub we're also on Slack for quick back-and-forth and you can book a pairing session with us.
There was a problem hiding this comment.
The login command performs the exchange of credentials for a JWT Token that has a TTL (Default 1hr if I recall). So without implementing the login API calls within the OP Plugin itself and writing out the token to a temp file. This was the only way I could see fit to connect the 2. As credentials aren't able to be provide in any other way at this time.
It also worth adding that the APIs themselves are rate limited. So even attempting to login, get the token, write it to disk, then performance the user intended action. Would be problematic for the end power user.
There was a problem hiding this comment.
It also worth adding that the APIs themselves are rate limited. So even attempting to login, get the token, write it to disk, then performance the user intended action. Would be problematic for the end power user.
For this the plugin ecosystem offers an encrypted cache to persist the token between command executions. An example of this in use are temporary AWS credentials.
So without implementing the login API calls within the OP Plugin itself and writing out the token to a temp file.
Is there a Confluent SDK we could use? Or could we possibly capture the output of the login command before it hits the disk?
There was a problem hiding this comment.
As it stands, there currently is no way to provide the token to the CLI. So while there is a GoLang SDK, there wouldn't be a way at this time to then translate that token to the CLI. I will open a separate ticket/issue with the Confluent CLI team and see about addressing this at a later date. But for now this seems to be the only route forward.
There was a problem hiding this comment.
@nerdynick Is there a reason we cannot use this exact logic that Confluent CLI is using? The project is open source so we could even re-use those exact functions.
There was a problem hiding this comment.
A possible reason would be that this code currently lives in the internal dir (see https://github.com/golang-standards/project-layout#internal). 🤔
@nerdynick what would be the way to go about asking for some changes in the Confluent CLI's codebase, if we were to go down this route?
There was a problem hiding this comment.
I'd be slightly hesitant about attempting to manipulate the internal state cache that the CLI manages. It's constantly changing without backward compatibility, since it's deemed ephemeral and internal to the CLI itself and would require keeping up with new releases, which happen regularly.
Outside of going the route that I did with this PR so far. Our next option would be to file an issue against the project or do the work ourselves and get a PR accepted to extend support for a Token to be provided via ENV Vars like the User/Pass can be. For which I started exploring down that route when this thread was brought up. It'd take a bit of time to complete and then go through that PR process.
The follow on question would then be, if we go down the route of asking the Confluent CLI team to make the needed change, would this project, 1Pass CLI Plugins, be OK with importing an external project (SDK) to acquire that token or would we need to duplicate that logic in a minimal fashion within this plugin to achieve that?
There was a problem hiding this comment.
The follow on question would then be, if we go down the route of asking the Confluent CLI team to make the needed change, would this project, 1Pass CLI Plugins, be OK with importing an external project (SDK) to acquire that token or would we need to duplicate that logic in a minimal fashion within this plugin to achieve that?
The best way forward here seems to start work on the Confluent CLI to be able to take in the token via the env var, as you said. After this is done, we are more than open to import any external SDK that would retrieve that token.
There was a problem hiding this comment.
Alright, I was able to modify the plugin and make use of what is essentially an undocumented feature. In that the Confluent CLI is able to take the credentials on every command. So the plugin now provides credentials on all calls but the Help and Version ones. This should mitigate the security concern that have been brought up I believe. This should also be a future proof route as the API documents the use of Credentials on all calls rather then the use of a Token.
There was a problem hiding this comment.
Hi @nerdynick !
I was just trying to use this shell plugin but can't get the username + password to work with commands without first running confluent login and writing an auth_token to disk. Is there a trick to get this to work without that step?
~ confluent kafka topic list --url https://<my-domain>.us-east-2.aws.confluent.cloud:443
No session token found, please enter user credentials. To avoid being prompted, run `confluent login`.
Username:
AndyTitu
added
in-progress
There was a problem hiding this comment.
lgtm, excited to get this out! Thank you for the contribution and making it happen with the confluent env vars! @nerdynick
AndyTitu
added
waiting-on-sec-review
and removed
in-progress
|
Hi @nerdynick ! We're about ready to merge this in, but you have some unsigned commits preventing that from happening. Are you able to take a minute to sign them? Thank you! |
|
Actually that doesn't look like it did the trick - I think you might need a rebase: https://stackoverflow.com/questions/41882919/is-there-a-way-to-gpg-sign-all-previous-commits |
nerdynick
force-pushed
the
Confluent-CLI
branch
2 times, most recently
from
5a97c5f
to
62caf8e
Compare
Signed-off-by: Nikoleta Verbeck <nerdynick@gmail.com> Revert to using Username field instead of Email to improve Import from Login Credential Types. Improve outputs for helpfulness Signed-off-by: Nikoleta Verbeck <nerdynick@gmail.com> Change MDS URL to URL and update Description. Remove Custom Names in favor of just Credentials Comment out all things related to Platform with TODOs Remove TODO in Plugin
Add aditional CMDs not neeting AuthN details Changes per latest Review
nerdynick
force-pushed
the
Confluent-CLI
branch
2 times, most recently
from
8781739
to
2ffbdbf
Compare
|
Alright, it took some creativity but I believe it's all Rebased, squashed, and signed now. |
This adds support for Confluent's Official CLI. However due to current limits of the Plugin framework, only support for Confluent Cloud has been enabled. However the code is made ready for the eventual support for Confluent Platform installations.