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

feat(ctl): Add support for stacklet credentials subcommand #108

Merged
merged 13 commits into from
Sep 26, 2023

Conversation

Techassi
Copy link
Member

@Techassi Techassi commented Sep 5, 2023

Tracked by #98, closes #107

This adds support for credential output when using. They are only displayed when the user explicitly want to retrieve them via a dedicated subcommand. Using a subcommand makes this an opt-in, as the output contains sensitive data. Test it with:

cargo run -p stackablectl -- stacklets credentials superset

The implementation is not final and will change with the introduction of Discovery 2.0.


Currently blocked by #106

Currently blocked by #113

@Techassi Techassi self-assigned this Sep 5, 2023
@Techassi Techassi marked this pull request as ready for review September 7, 2023 09:06
@Techassi Techassi requested review from nightkr, maltesander and a team September 7, 2023 09:07
@Techassi
Copy link
Member Author

Techassi commented Sep 7, 2023

To conditionally display the CREDENTIALS column, Nukesor/comfy-table#126 is required. As a workaround, we could also duplicate the table header code, once with and once without the credentials column.

Copy link
Member

@nightkr nightkr left a comment

Choose a reason for hiding this comment

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

On second thought, I'm not sure list is the right place to display this.. I think it might be more appropriate to have a "show creds for this specific stacklet" command?

rust/stackable-cockpit/src/platform/credentials.rs Outdated Show resolved Hide resolved
rust/stackable-cockpit/src/platform/credentials.rs Outdated Show resolved Hide resolved
rust/stackable-cockpit/src/platform/credentials.rs Outdated Show resolved Hide resolved
rust/stackable-cockpit/src/platform/credentials.rs Outdated Show resolved Hide resolved
}

#[derive(Debug, Snafu)]
pub enum StackletError {
#[snafu(display("kubernetes error"))]
#[snafu(display("kubernetes error"), context(false))]
Copy link
Member

Choose a reason for hiding this comment

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

Really don't love this, if anything I'd like to get rid of these "oh this library failed" catch-alls entirely.

Copy link
Member Author

@Techassi Techassi Sep 25, 2023

Choose a reason for hiding this comment

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

Generally agreed, but let's maybe tackle this in #55

Ok(credentials) => credentials,
Err(err) => match err {
CredentialsError::KubeError { source } => return Err(source.into()),
CredentialsError::NoSecret => None,
Copy link
Member

Choose a reason for hiding this comment

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

Is there much value to treating this as a separate condition in get_credentials if we just end up flattening it back into None afterwards?

Copy link
Member Author

Choose a reason for hiding this comment

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

I feel like the get_credentials function should report this error. The implementer (stackablectl) chooses to map it to None, which indicates that no credentials output should be printed.

rust/stackable-cockpit/src/platform/stacklet/mod.rs Outdated Show resolved Hide resolved
rust/stackable-cockpit/src/utils/k8s/client.rs Outdated Show resolved Hide resolved
rust/stackable-cockpit/src/utils/k8s/client.rs Outdated Show resolved Hide resolved
rust/stackable-cockpit/src/utils/k8s/client.rs Outdated Show resolved Hide resolved
The command currently fails with an 404 error. It seems like that the k8s
API is unable to find the requested object.
@Techassi Techassi changed the title feat(ctl): Add support for --show-credentials in stacklets list command feat(ctl): Add support for stacklet credentials subcommand Sep 15, 2023
@sbernauer
Copy link
Member

sbernauer commented Sep 21, 2023

For the record: a52b933 fixes the 404 error, happy to explain it offline

Copy link
Member

@sbernauer sbernauer left a comment

Choose a reason for hiding this comment

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

Command worked for me 👍

➜  stackable-cockpit git:(feat/credential-output) cargo r -p stackablectl -- stacklet credentials --product-namespace sbernauer superset simple-superset
Credentials for superset (simple-superset) in namespace 'sbernauer':

 USERNAME  admin 
 PASSWORD  admin

In case i changed stacklet.data["spec"]["clusterConfig"]["credentialsSecret"] to stacklet.data["spec"]["clusterConfig"]["credentialsSecret123"] it showed No credentials and did not give an error.
This is good, as we will remove that field in the future.

@Techassi Techassi added this pull request to the merge queue Sep 26, 2023
Merged via the queue into main with commit 233540e Sep 26, 2023
@Techassi Techassi deleted the feat/credential-output branch September 26, 2023 12:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Add credential output to stackablectl
3 participants