Skip to content

[installer] Support adding image pull secrets even when the internal (in-cluster) registry is used #10792

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

Closed
corneliusludmann opened this issue Jun 21, 2022 · 8 comments
Labels
team: delivery Issue belongs to the self-hosted team type: improvement Improves an existing feature or existing code

Comments

@corneliusludmann
Copy link
Contributor

corneliusludmann commented Jun 21, 2022

Currently, we can add image pull secrets only when an external registry is used like this:

containerRegistry:
  inCluster: false
  external:
    url: my-registry.example.com
    certificate:
      kind: secret
      name: my-registry-credentials

However, when we use an in-cluster registry, customers would probably still want to add pull secrets for workspace base images.

I would suggest to deprecate the certificate block under external and moving it directly under containerRegistry. Like this:

containerRegistry:
  inCluster: false
  external:
    url: my-registry.example.com
  certificate:
    kind: secret
    name: my-registry-credentials

(probably rename “certificate” to “credentials”, however, we probably want to keep this consistent to other occurences of “credentials”, e.g. for the object storage)

That would also allow adding workspace pull secrets when the in-cluster registry is used like this (see also #10791):

containerRegistry:
  inCluster: true
  certificate:
    kind: secret
    name: my-registry-credentials
  privateBaseImageRegistries:
    - my-registry.example.com
    - my-other-registry.example.com

In this case, we need to merge the in-cluster container secret with the secret given here to allow accessing both registries.

@corneliusludmann corneliusludmann added type: improvement Improves an existing feature or existing code team: delivery Issue belongs to the self-hosted team labels Jun 21, 2022
@lucasvaltl
Copy link
Contributor

  • We should do this soon - we created tech debt to solve a customer problem, but should solve this properly via this issue.

@lucasvaltl
Copy link
Contributor

Internal mention of this here

@mrzarquon
Copy link
Contributor

Allow certificate or a dockerconfig, since uploading a dockerconfig directly from a file means they can validate that they've set the correct credentials with a docker push/pull, before uploading the file.

Allow for multiple secrets and we merge them for one dockerconfig

@Pothulapati Pothulapati self-assigned this Aug 10, 2022
@Pothulapati Pothulapati moved this from 📓Scheduled to ⚒In Progress in 🚚 Security, Infrastructure, and Delivery Team (SID) Aug 10, 2022
@Pothulapati
Copy link
Contributor

Pothulapati commented Aug 10, 2022

Started working on this. The planned resolution is to move credentials to be under containerRegistry so that it can specified irrespective of the type of container registry i.e internal or external.

Coming to the requirements that @mrzarquon specified,

Allow certificate or a dockerconfig, since uploading a dockerconfig directly from a file means they can validate that they've set the correct credentials with a docker push/pull, before uploading the file.

We can only specify setting a certificate (aka secret) as taking a dockerconfig directly means that the we might be storing auth credentials in the config (which has complexities, even if we mask them). But, As the certificate is created by the user using the .dockerconfig itself, They can always try doing the push/pull before creating the secret itself.

@Pothulapati
Copy link
Contributor

Created #12060

@Pothulapati Pothulapati moved this from ⚒In Progress to 📓Scheduled in 🚚 Security, Infrastructure, and Delivery Team (SID) Aug 11, 2022
@lucasvaltl
Copy link
Contributor

@Pothulapati does #12060 replace this one in that case, let's close this one and prioritise the other one :)

@Pothulapati
Copy link
Contributor

Pothulapati commented Aug 11, 2022

@lucasvaltl This is still needed, as #12060 only tackles the components side of things, but we still need to move this config in the installer (but only after that is done)

@Pothulapati
Copy link
Contributor

After we merged #12174, This is supported now through kots!

There is #12060 that helps us to also add this option to the installer but not too important. Closing this hence.

Repository owner moved this from 📓Scheduled to ✨Done in 🚚 Security, Infrastructure, and Delivery Team (SID) Sep 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team: delivery Issue belongs to the self-hosted team type: improvement Improves an existing feature or existing code
Projects
No open projects
Development

No branches or pull requests

4 participants