chore: bump newrelic-auth-rs#2399
Conversation
| /// Valid data to create a SystemIdentity, represent [SystemIdentityArgs] after validation. | ||
| #[derive(Debug)] | ||
| pub struct SystemIdentitySpec { | ||
| pub system_identity_data: SystemIdentityData, |
There was a problem hiding this comment.
I find it quite confusing, the meaning of private_key_path is different depending on system_identity_data, no?
in one case is where to read the key, in the other is where to store it I guess
There was a problem hiding this comment.
Yes, exactly! In both cases it is the path were the private-key is expected to be (it could be because it was already there or because it needs to be created there)
| SystemIdentityData::Existing { auth_client_id } => auth_client_id.to_string(), | ||
| SystemIdentityData::Provision(provisioning_method) => { | ||
| let pub_key = public_key_from_key_pair(identity_spec.private_key_path.clone())?; | ||
| provide_identity_fn( |
There was a problem hiding this comment.
provide_identity_fn as a name is still a bit confusing, no? this creates the identity
There was a problem hiding this comment.
I agree, create_identity_fn would better, right?
There was a problem hiding this comment.
I've just renamed it. LTMKYT
| let client_id = match &identity_spec.system_identity_data { | ||
| SystemIdentityData::Existing { auth_client_id } => auth_client_id.to_string(), | ||
| SystemIdentityData::Provision(provisioning_method) => { | ||
| let pub_key = public_key_from_key_pair(identity_spec.private_key_path.clone())?; |
There was a problem hiding this comment.
also this is quite confusing, it seems that "from_key_pair" is retrieving a pub_key from the keypair, but it is actually generating it in that path!
There was a problem hiding this comment.
Do you have any better name suggestion for the function name?
There was a problem hiding this comment.
Maybe a two-step call can clarify? Like create_key_pair(identity_spec.private_key_path.clone())?.get_pub_key().
There was a problem hiding this comment.
Also this is probably too much but perhaps the identity structure or data can reside in the FleetControl type, which might allow to reuse some of the data.
There was a problem hiding this comment.
I can also completely get rid of the helper, we don't have a create_key_pair function because the key generator implementation (From nr-auth) returns the public-key only
There was a problem hiding this comment.
I finally ditched the helper, it was causing confusion so it wasn't helping that much 6a02f74
| let SystemIdentityData::Provision(provisioning_method) = &spec.identity.system_identity_data | ||
| else { | ||
| return Err(K8sCliError::Generic( | ||
| "the k8s cli requires provisioning a new System Identity; use --auth-parent-token or --auth-parent-client-secret instead of --auth-client-id" |
There was a problem hiding this comment.
IMO the message is a bit confusing, I think that the reason is that we are allowing an argument that is not actually allowed 😅 Shouldn't we avoid to have that enum variant for k8s?
There was a problem hiding this comment.
We already had this issue before but now it is more evident. I wanted to use the same arguments we already had in the on-host cli, but maybe it doesn't worth it.
There was a problem hiding this comment.
I can try to break the types a little bit. WDYT?
There was a problem hiding this comment.
yes I think it would be better unless there is a ton of code duplication
There was a problem hiding this comment.
I think it is better now 5c44470, let me know your thoughts
| })] | ||
| #[case::parent_secret(|| SystemIdentityArgs { | ||
| auth_private_key_path: Some(PathBuf::from("/some/path")), | ||
| #[case::parent_secret(|| ProvisionIdentityArgs { |
There was a problem hiding this comment.
Why this closure and not the value directly?
There was a problem hiding this comment.
It is a leftover! Thanks!
| let client_id = match &identity_spec.system_identity_data { | ||
| SystemIdentityData::Existing { auth_client_id } => auth_client_id.to_string(), | ||
| SystemIdentityData::Provision(provisioning_method) => { | ||
| let pub_key = public_key_from_key_pair(identity_spec.private_key_path.clone())?; |
There was a problem hiding this comment.
Maybe a two-step call can clarify? Like create_key_pair(identity_spec.private_key_path.clone())?.get_pub_key().
| let client_id = match &identity_spec.system_identity_data { | ||
| SystemIdentityData::Existing { auth_client_id } => auth_client_id.to_string(), | ||
| SystemIdentityData::Provision(provisioning_method) => { | ||
| let pub_key = public_key_from_key_pair(identity_spec.private_key_path.clone())?; |
There was a problem hiding this comment.
Also this is probably too much but perhaps the identity structure or data can reside in the FleetControl type, which might allow to reuse some of the data.
sigilioso
force-pushed
the
feat/k8s-cli-system-identity
branch
from
c51a3c8 to
8e82dca
Compare
sigilioso
force-pushed
the
chore/bump-nr-auth-040
branch
from
d6a634d to
cee90f1
Compare
| let result = system_identity_generator | ||
| .generate(&auth_credential) | ||
| let result = iam_client | ||
| .create_l2_system_identity(&auth_credentials, &pub_key) |
gsanchezgavier
left a comment
gsanchezgavier
left a comment
There was a problem hiding this comment.
Looks good thanks
| identity_input: &SystemIdentitySpec, | ||
| /// Creates a key-based identity considering the supplied args. It returns the corresponding **client_id** as a String. | ||
| pub fn create_identity( | ||
| method: &ProvisioningMethod, |
There was a problem hiding this comment.
nit: parent_auth: ParentAuth ?
There was a problem hiding this comment.
I've changed here: 98148db
parent_auth_method: ParentAuthMethod
sigilioso
merged commit f109f9f
into
feat/k8s-cli-system-identity
5 participants
Summary
PR on top of #2378
This PR bumps
newrelic-auth-rsto0.4.0and adapts the System Identity CLI code to the corresponding breaking change.Details
Creatortrait indirection for the k8s cli.