Skip to content

✨ use SecretManager to handle access to cluster secrets #441

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.

Sign up for GitHub

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

Jump to bottom
Open
tuminoid wants to merge 1 commit into
base: main
Choose a base branch
from
Open

✨ use SecretManager to handle access to cluster secrets #441

tuminoid wants to merge 1 commit into from

Conversation

@tuminoid
Copy link
Member

@tuminoid tuminoid commented Nov 28, 2025 •
edited
Loading

What this PR does / why we need it:

Use the SecretManager from BMO to also handle secrets in IRSO. Instead of holding all cluster secrets in cluster client cache, we only keep the ones that match our labels. This lowers memory consumption but also prevents a risk that the cluster secrets might get dumped. This has been adapted from BMO's SecretManager.

Fixes #425

Checklist:

  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • E2E tests have been added, if necessary.

@metal3-io-bot metal3-io-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 28, 2025
@metal3-io-bot
Copy link
Contributor

metal3-io-bot commented Nov 28, 2025

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@metal3-io-bot
Copy link
Contributor

metal3-io-bot commented Nov 28, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign lentzi90 for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@metal3-io-bot metal3-io-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 28, 2025
@tuminoid tuminoid requested a review from Copilot November 28, 2025 07:11
@tuminoid tuminoid force-pushed the tuomo/use-secret-manager branch from 267077b to a76d5df Compare November 28, 2025 07:38
@metal3-io-bot metal3-io-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 28, 2025
@tuminoid tuminoid mentioned this pull request Nov 28, 2025
Open
4 tasks
@tuminoid tuminoid marked this pull request as ready for review November 28, 2025 08:04
@metal3-io-bot metal3-io-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 28, 2025
Copilot AI reviewed Nov 28, 2025
View reviewed changes
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

dtantsur
dtantsur reviewed Nov 28, 2025
View reviewed changes
dtantsur
dtantsur reviewed Nov 28, 2025
View reviewed changes
@tuminoid tuminoid force-pushed the tuomo/use-secret-manager branch from a76d5df to 598393a Compare December 4, 2025 08:48
@tuminoid
Copy link
Member Author

tuminoid commented Dec 4, 2025

Changed label name and rebased, should be good now.

@tuminoid tuminoid requested a review from dtantsur December 4, 2025 08:50
@tuminoid tuminoid mentioned this pull request Dec 4, 2025
Closed
@tuminoid tuminoid force-pushed the tuomo/use-secret-manager branch from 598393a to 1661d0a Compare December 4, 2025 09:03
@tuminoid
Copy link
Member Author

tuminoid commented Dec 4, 2025

Changed label name and rebased, should be good now.

Make lint locally and GH workflow did not agree on linting here :( Created #456 to track that inconsistency.

matthewei
matthewei reviewed Dec 5, 2025
View reviewed changes
Copy link

@matthewei matthewei left a comment

Choose a reason for hiding this comment

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

Do we still need to monitor secret?

dtantsur
dtantsur reviewed Dec 6, 2025
View reviewed changes
dtantsur
dtantsur reviewed Dec 6, 2025
View reviewed changes
dtantsur
dtantsur reviewed Dec 6, 2025
View reviewed changes
@dtantsur
Copy link
Member

dtantsur commented Dec 6, 2025

/lgtm

Thanks!

Do we still need to monitor secret?

Not sure I fully understand the question, but owning the secret ensures that the controller gets notified when it changes, while this change ensures the relevant (and only relevant) secrets are cached.

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 6, 2025
@tuminoid
use SecretManager to handle access to cluster secrets
5418793 
Use the SecretManager from BMO to also handle secrets in IRSO.
Instead of holding all cluster secrets in cluster client cache, we
only keep the ones that match our labels. This lowers memory
consumption but also prevents a risk that the cluster secrets might
get dumped.

Signed-off-by: Tuomo Tanskanen <[email protected]>
@tuminoid tuminoid force-pushed the tuomo/use-secret-manager branch from 1661d0a to 5418793 Compare December 8, 2025 10:23
@metal3-io-bot metal3-io-bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 8, 2025
@tuminoid
Copy link
Member Author

tuminoid commented Dec 8, 2025

Pushed fixes to nits and rebased.
/cc @dtantsur @lentzi90

@dtantsur
Copy link
Member

dtantsur commented Dec 8, 2025

/lgtm

Thanks!

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@elfosardo elfosardo Awaiting requested review from elfosardo

@lentzi90 lentzi90 Awaiting requested review from lentzi90

@Rozzii Rozzii Awaiting requested review from Rozzii

@dtantsur dtantsur Awaiting requested review from dtantsur

+1 more reviewer

@matthewei matthewei matthewei left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

IrSO watches all secrets in the cluster

4 participants

@tuminoid @metal3-io-bot @dtantsur @matthewei