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

Bug: Bundle image source should use ext.Spec.InstallNamespace, not systemNamespace #944

Closed
Tracked by #950
joelanford opened this issue Jun 15, 2024 · 4 comments · Fixed by #1180
Closed
Tracked by #950
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug. v1.0 Issues related to the initial stable release of OLMv1
Milestone

Comments

@joelanford
Copy link
Member

See:

unpacker, err := source.NewDefaultUnpacker(mgr, systemNamespace, filepath.Join(cachePath, "unpack"), (*x509.CertPool)(nil))

This likely means a slight refactoring of the ImageRegistry implementation.

Related: it probably makes sense to stop using the composite unpacker and instead just use ImageRegistry directly.

@joelanford joelanford added kind/bug Categorizes issue or PR as related to a bug. v1.0 Issues related to the initial stable release of OLMv1 labels Jun 15, 2024
@LalatenduMohanty
Copy link
Member

/assign

@everettraven everettraven added this to the v1.0.0 milestone Aug 20, 2024
@m1kola m1kola assigned m1kola and unassigned LalatenduMohanty Aug 27, 2024
@m1kola
Copy link
Member

m1kola commented Aug 27, 2024

ImageRegistry is now being used, but it looks like we still need to fix AuthNamespace:

unpacker := &source.ImageRegistry{
BaseCachePath: filepath.Join(cachePath, "unpack"),
// TODO: This needs to be derived per extension via ext.Spec.InstallNamespace
AuthNamespace: systemNamespace,
CertPoolWatcher: certPoolWatcher,
}

@m1kola
Copy link
Member

m1kola commented Aug 27, 2024

This namespace is only used when there is image pull secret specified and we currently do not specify it. So as far as I understand - this does not affect anything at the moment. @joelanford or am I missing something?

If we set AuthNamespace to ext.Spec.InstallNamespace this will mean that we will have to create a pull secret for image registry in the ext.Spec.InstallNamespace. Is this what we want to achieve? Oh, and we will also have to have a pull secret support (has to be in line with #1015, I suspect).

@joelanford
Copy link
Member Author

The original idea was that a user would be able to specify an image pull secret on the ClusterExtension object. We don't have that field on our API, which explains why we don't plumb it through to the library.

Perhaps for now, in order to reduce the likelihood of the current unused setup being cargo-culted in the future, we should strip out the unused plumbing and internal API fields (that we control) that accept image pull secret configuration?

@m1kola m1kola moved this to Ready for Review in OLM v1 Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. v1.0 Issues related to the initial stable release of OLMv1
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants