Skip to content

✨ Accept per-host pull secrets for external OCI registries#2745

Open
mabulgu wants to merge 2 commits intometal3-io:mainfrom
mabulgu:feature/bmo-per-host-oci-auth
Open

✨ Accept per-host pull secrets for external OCI registries#2745
mabulgu wants to merge 2 commits intometal3-io:mainfrom
mabulgu:feature/bmo-per-host-oci-auth

Conversation

@mabulgu
Copy link
Copy Markdown
Contributor

@mabulgu mabulgu commented Oct 22, 2025

What this PR does / why we need it:

Adds per-host registry authentication for oci:// provisioning images.
New optional field on BareMetalHost:

spec:
  image:
    url: oci://<registry>/<repo>/<artifact>:<tag|digest>
    ociAuthSecretName: <k8s-secret-name>  # optional; same namespace as the BMH

When set, the controller validates the Kubernetes Docker-config secret (kubernetes.io/dockerconfigjson or kubernetes.io/dockercfg), selects the correct registry entry (supports exact host and host:port), and uses those credentials during provisioning so private OCI artefacts can be fetched on a per-host basis. Public images continue to work without credentials.
This removes a blocker for users who need different registries/accounts per machine.

Assisted-By: Claude-4.5-sonnet, Claude-opus-4.6

@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 Oct 22, 2025
@metal3-io-bot metal3-io-bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 22, 2025
@metal3-io-bot
Copy link
Copy Markdown
Contributor

Hi @mabulgu. Thanks for your PR.

I'm waiting for a metal3-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@metal3-io-bot metal3-io-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 22, 2025
@mabulgu mabulgu force-pushed the feature/bmo-per-host-oci-auth branch from 898fffc to 9594af2 Compare October 22, 2025 10:52
@mabulgu mabulgu marked this pull request as ready for review October 27, 2025 09:48
@mabulgu mabulgu changed the title [WIP] ✨ Accept per-host pull secrets for customer OCI registries ✨ Accept per-host pull secrets for customer OCI registries Oct 27, 2025
@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 Oct 27, 2025
@dtantsur
Copy link
Copy Markdown
Member

/ok-to-test

@metal3-io-bot metal3-io-bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 27, 2025
Comment thread apis/metal3.io/v1alpha1/baremetalhost_types.go Outdated
Comment thread internal/controller/metal3.io/baremetalhost_controller.go Outdated
Comment thread pkg/secretutils/validator.go Outdated
Comment thread internal/controller/metal3.io/baremetalhost_controller.go Outdated
Comment thread pkg/secretutils/dockerconfig.go
@MahnoorAsghar
Copy link
Copy Markdown
Contributor

It might be too much code to be squashing into one commit...what do we think?

@tuminoid
Copy link
Copy Markdown
Member

tuminoid commented Oct 28, 2025

It might be too much code to be squashing into one commit...what do we think?

10 is definitely not right either. One for implementation, one for tesrts, and one for docs? There is plenty of work left though, given the conditions need work per Dmitry's review.

@mabulgu
Copy link
Copy Markdown
Contributor Author

mabulgu commented Nov 3, 2025

Thanks for your comments!

@MahnoorAsghar > It might be too much code to be squashing into one commit...what do we think?

As soon as we itemize them and they are in the same context, I don't think so. I am going to remove everything related to conditions as they were like extra, but everything else share the same context and can be in the same squash commit IMO.

@tuminoid > One for implementation, one for tesrts, and one for docs

+1 for docs -1 for tests as tests are a part of the "implementation". Without tests, I would not count it as implemented.

What I will do is: seperating the code commit (which will have less changes than the current changes because of the condition revert) and the commit for docs.

@tuminoid
Copy link
Copy Markdown
Member

tuminoid commented Nov 3, 2025

Thanks for your comments!
@tuminoid > One for implementation, one for tesrts, and one for docs

+1 for docs -1 for tests as tests are a part of the "implementation". Without tests, I would not count it as implemented.

This is fine as well, but its quite common to implement tests in separate commit in same PR. Makes it maybe easier to manage, but like said, I'm 100% fine with code+tests in same commit.

@tuminoid
Copy link
Copy Markdown
Member

tuminoid commented Nov 9, 2025

/retest

@mabulgu mabulgu force-pushed the feature/bmo-per-host-oci-auth branch 2 times, most recently from b18570e to 27c9860 Compare November 11, 2025 15:19
@mabulgu mabulgu requested a review from dtantsur November 11, 2025 15:22
@mabulgu
Copy link
Copy Markdown
Contributor Author

mabulgu commented Nov 11, 2025

@dtantsur I applied your suggestions. pls check when you have time. You will find the relevant commetns resolved but pls feel free to reopen them if you feel any of them are not implemented the way you suggested

@mabulgu
Copy link
Copy Markdown
Contributor Author

mabulgu commented Nov 12, 2025

Not sure if the e2e test filure is related to my changes as it seems to be related to the BMC management credentials

@mabulgu
Copy link
Copy Markdown
Contributor Author

mabulgu commented Mar 4, 2026

@mabulgu the linter failure looks real from a quick glance + a couple of minor comments

Thanks @dtantsur. I addressed the feedback + commented for one. Pls check when available.

Comment thread internal/controller/metal3.io/baremetalhost_controller.go Outdated
Comment thread internal/controller/metal3.io/image_auth_validator.go Outdated
Comment thread internal/controller/metal3.io/image_auth_validator.go Outdated
@tuminoid
Copy link
Copy Markdown
Member

This needs rebase due linter updates.
/hold

@metal3-io-bot metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 18, 2026
@mabulgu mabulgu force-pushed the feature/bmo-per-host-oci-auth branch from 645c8b1 to 35ed9a3 Compare March 18, 2026 10:34
@mabulgu
Copy link
Copy Markdown
Contributor Author

mabulgu commented Mar 18, 2026

/retest

@mabulgu mabulgu force-pushed the feature/bmo-per-host-oci-auth branch from 35ed9a3 to 8a5cc14 Compare March 18, 2026 10:55
@mabulgu mabulgu requested a review from dtantsur March 18, 2026 11:24
@mabulgu
Copy link
Copy Markdown
Contributor Author

mabulgu commented Mar 18, 2026

@tuminoid @dtantsur I will need an approval/lgtm for merging this along.

Copy link
Copy Markdown

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.

Pull request overview

Copilot reviewed 15 out of 16 changed files in this pull request and generated 5 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/controller/metal3.io/image_auth_validator.go Outdated
Comment thread pkg/provisioner/ironic/ironic.go
Comment thread pkg/provisioner/ironic/ironic.go
Comment thread internal/controller/metal3.io/baremetalhost_controller.go
Comment thread apis/metal3.io/v1alpha1/baremetalhost_types.go
Copy link
Copy Markdown
Member

@tuminoid tuminoid left a comment

Choose a reason for hiding this comment

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

Please check Copilot nits.

I don't have other blockers, except the importing of basically unmaintained and not widely used dockercfg.

Comment thread go.mod
@metal3-io-bot metal3-io-bot added the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Apr 9, 2026
@mabulgu mabulgu force-pushed the feature/bmo-per-host-oci-auth branch from 8a5cc14 to 7056860 Compare April 20, 2026 11:28
@metal3-io-bot metal3-io-bot removed the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Apr 20, 2026
@mabulgu mabulgu force-pushed the feature/bmo-per-host-oci-auth branch from 7056860 to f02b28b Compare April 20, 2026 11:32
@mabulgu
Copy link
Copy Markdown
Contributor Author

mabulgu commented Apr 20, 2026

/retest

@mabulgu mabulgu force-pushed the feature/bmo-per-host-oci-auth branch from 4e09edc to 9ea9688 Compare April 20, 2026 12:03
@honza
Copy link
Copy Markdown
Member

honza commented Apr 20, 2026

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 20, 2026
@tuminoid tuminoid requested a review from Copilot April 21, 2026 04:23
Copy link
Copy Markdown

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.

Pull request overview

Copilot reviewed 17 out of 18 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pkg/provisioner/ironic/provision_test.go
Comment thread internal/controller/metal3.io/baremetalhost_controller.go
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: mabulgu <mabulgu@gmail.com>
@metal3-io-bot
Copy link
Copy Markdown
Contributor

New changes are detected. LGTM label has been removed.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: mabulgu <mabulgu@gmail.com>
@mabulgu
Copy link
Copy Markdown
Contributor Author

mabulgu commented Apr 22, 2026

@honza can I get a re- /lgtm when you can, as I had to rebase this + got some copilot feedback via @tuminoid

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants