Skip to content

feat(k8s): register system identities through cli command#2378

Merged
sigilioso merged 8 commits into
mainfrom
feat/k8s-cli-system-identity
Apr 14, 2026
Merged

feat(k8s): register system identities through cli command#2378
sigilioso merged 8 commits into
mainfrom
feat/k8s-cli-system-identity

Conversation

@sigilioso
Copy link
Copy Markdown
Contributor

@sigilioso sigilioso commented Mar 27, 2026

Summary

Adds a register-system-identity subcommand to the Kubernetes CLI binary. This command provisions a New Relic system identity (client ID + private key) and stores it as a Kubernetes Secret, making it available for Agent Control to authenticate with Fleet Control. The operation is idempotent — if the secret already exists, it exits early without modifying anything.

This aligns the approach between k8s and on-host for System Identity provisioning, leveraging the CLI to do so in both cases, it also allows simplifying the corresponding job in the helm chart.

Notes for reviewers

  • Identity Creation is not covered in e2e (we leverage an already existing System Identity). The approach has been manually using an updated helm-chart. Check out the corresponding draf-PR for details: [agent-control-deployment] use cli to register system identity [NR-537956] helm-charts#2189
  • The provide_system_identity_secret function takes the identity provider as a parameter specifically to enable unit testing without hitting real auth endpoints (the k8s client is also taken as a parameter for the same purpose).
  • A new comment has been added to the SystemIdentityArgs struct to clarify the broader tradeoff between arguments and sub-commands.
  • The changes in the Tiltfile prepare everything for the new version but are supposed to be compatible with the current chart.

🚧 newrelic-auth-rs update

@sigilioso sigilioso requested a review from a team as a code owner March 27, 2026 10:30
@sigilioso sigilioso added the k8s-extended-e2e Trigger extended k8s e2e on a PR label Mar 27, 2026
Copy link
Copy Markdown
Contributor

@gsanchezgavier gsanchezgavier left a comment

Choose a reason for hiding this comment

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

Thanks! i left some doubts

Comment thread agent-control/src/bin/main_agent_control_k8s_cli.rs
Comment thread agent-control/src/cli/common/system_identity.rs
Comment thread agent-control/src/cli/k8s/system_identity.rs Outdated
})?;

let private_key = LocalFile
.read(identity.private_key_path.as_path())
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

are we storing the pk in the fs and the reading it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We are! Thanks for pointing out, I'm looking for alternatives to avoid it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I finally changing by using a custom key::Creator for the k8s use-case (it actually doesn't create anything but it allow us referencing to the key pair without external components). LMKWYT.

@sigilioso sigilioso force-pushed the feat/k8s-cli-system-identity branch from aa0e099 to 2f6bdf6 Compare March 31, 2026 15:11
Comment thread agent-control/src/cli/k8s/system_identity.rs Outdated
Comment thread agent-control/src/cli/k8s/system_identity.rs
Comment thread agent-control/src/cli/k8s/system_identity.rs Outdated
@sigilioso sigilioso force-pushed the feat/k8s-cli-system-identity branch from 2f6bdf6 to 6d98da8 Compare April 1, 2026 13:54
Comment thread agent-control/src/cli/on_host/config_gen.rs
Comment thread agent-control/Cargo.toml
@sigilioso sigilioso requested review from a team, danielorihuela and gsanchezgavier April 1, 2026 14:21
@sigilioso sigilioso marked this pull request as draft April 1, 2026 14:21
}

impl Creator for PublicKeyHolder {
type Error = Infallible;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

til

Comment thread agent-control/src/cli/k8s/system_identity.rs Outdated
gsanchezgavier
gsanchezgavier previously approved these changes Apr 2, 2026
Copy link
Copy Markdown
Contributor

@gsanchezgavier gsanchezgavier left a comment

Choose a reason for hiding this comment

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

thanks for addressing the comments!

@sigilioso sigilioso force-pushed the feat/k8s-cli-system-identity branch from 77a5477 to 0c7d509 Compare April 2, 2026 15:07
@sigilioso sigilioso marked this pull request as ready for review April 2, 2026 15:07
@sigilioso sigilioso requested review from a team and gsanchezgavier April 2, 2026 15:07
Comment thread agent-control/src/cli/common/system_identity.rs Outdated
Comment thread agent-control/src/cli/k8s/system_identity.rs Outdated
public_key: PublicKeyPem,
}

impl Creator for PublicKeyHolder {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why can't it generate and return the key without storing it?
Would make sense to have a inMemoryCreator instead of the hack?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I started with a in-memory creator but it was adding complexity: the create() method would mutate the internal state and it would need to provide something to access to the private key.

let KeyPair {
private_key,
public_key,
} = rsa(&KeyType::Rsa4096).map_err(|err| {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am not sure this should have been public after all 😅
I think it would have been cleaner to have a inMemory creator

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree but then I'd polish the Creator trait first. I'll leave it as it is for now and re-consider a future refactor for newrelic-auth-rs, if that's OK

Copy link
Copy Markdown
Contributor Author

@sigilioso sigilioso Apr 8, 2026

Choose a reason for hiding this comment

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

We'll improve this in a PR on top of this after newrelic/newrelic-auth-rs#201 is merged

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed here: #2399 (there is no Creator anymore!)

Copy link
Copy Markdown
Member

@paologallinaharbur paologallinaharbur left a comment

Choose a reason for hiding this comment

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

I have doubts regarding the keyHolder, but it is something internal, therefore I do not want to block it

@sigilioso
Copy link
Copy Markdown
Contributor Author

Now that newrelic/newrelic-auth-rs#201 is ready, I'm working on a PR on top of this to simplify things. It should address: #2378 (comment)

@sigilioso sigilioso force-pushed the feat/k8s-cli-system-identity branch from c51a3c8 to 8e82dca Compare April 13, 2026 09:18
|| String::from("--secret-name s --region us --auth-private-key-path /some/path --auth-parent-token TOKEN --auth-parent-client-id id --organization-id org-id")
)]
#[case::client_secret_based_identity(
|| String::from("--secret-name s --region us --auth-private-key-path /some/path --auth-parent-client-secret SECRET --auth-parent-client-id id --organization-id org-id")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are the closures required?

|| String::from("--secret-name s --region us --auth-client-id some-id")
)]
#[case::no_identity_method(
|| format!("--secret-name s --region us --auth-private-key-path {}", current_dir().unwrap().display())
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Even with this case I'm not sure the closures are required.

DavSanchez
DavSanchez previously approved these changes Apr 13, 2026
Copy link
Copy Markdown
Contributor

@DavSanchez DavSanchez left a comment

Choose a reason for hiding this comment

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

Needs rebase 🚀

@sigilioso
Copy link
Copy Markdown
Contributor Author

Needs rebase 🚀

Rebased! Thanks!

@sigilioso sigilioso requested a review from DavSanchez April 13, 2026 15:43
@sigilioso sigilioso merged commit e702f6c into main Apr 14, 2026
41 checks passed
@sigilioso sigilioso deleted the feat/k8s-cli-system-identity branch April 14, 2026 06:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

k8s-extended-e2e Trigger extended k8s e2e on a PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants