✨ Certificate support for image registry #956

tmshort · 2024-06-18T14:11:49Z

Updates the ClusterExtension API to support a certificate to retreive a bundle from an image registry.

Fixes #921

Description

Reviewer Checklist

API Go Documentation
Tests: Unit Tests (and E2E Tests, if appropriate)
Comprehensive Commit Messages
Links to related GitHub Issue(s)

netlify · 2024-06-18T14:12:05Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`c210e85`
🔍 Latest deploy log	https://app.netlify.com/sites/olmv1/deploys/6671aca079e1040008a3eb29
😎 Deploy Preview	https://deploy-preview-956--olmv1.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codecov · 2024-06-18T14:26:37Z

Codecov Report

Attention: Patch coverage is 57.89474% with 24 lines in your changes missing coverage. Please review.

Project coverage is 79.07%. Comparing base (bfd4142) to head (c210e85).

Files	Patch %	Lines
api/v1alpha1/zz_generated.deepcopy.go	44.00%	14 Missing ⚠️
...nternal/controllers/clusterextension_controller.go	67.74%	5 Missing and 5 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #956      +/-   ##
==========================================
- Coverage   80.14%   79.07%   -1.07%     
==========================================
  Files          16       16              
  Lines        1103     1152      +49     
==========================================
+ Hits          884      911      +27     
- Misses        152      169      +17     
- Partials       67       72       +5

Flag	Coverage Δ
e2e	`57.55% <52.63%> (-0.66%)`	⬇️
unit	`51.90% <14.03%> (-1.95%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

everettraven · 2024-06-18T14:45:14Z

api/v1alpha1/clusterextension_types.go

+	// Name of the secret
+	Name string `json:"name"`
+	// Namespace of the secret
+	Namespace string `json:"namespace,omitempty"`


Should we allow for specification of the namespace here? I'm wondering if using the install namespace as a point where all other resources referenced should exist is more reasonable?

The secret is associated with the image registry, not the installation of the bundle itself. This secret is independent of the installation namespace. If we do not allow specifying the namespace, it would possibly require copying the secret to various namespaces, which then causes issues when the secret (which is a TLS certificate) needs to be updated.

Also, the service account for installation does need access to the secret.

The secret is associated with the image registry

I think my comment about cert configuration being on ClusterExtension is relevant to this discussion: #956 (comment)

TLDR: I think we should not ask users to specify certs on every installation.

To be clear, it's not required on every installation.

If the docker-registry's certificate is signed by a system CA, this option is not required. This requires additional setup (i.e. cert-manager/trust-manager/deployment updates).

Also, if the docker-registry is public, and uses a certificate signed by a "public CA", then this option is not necessary.

If the operator-controller's deployment includes mounted certificates for the system CAs, then those will be used.

To be clear, it's not required on every installation.

You are right. But it is required on every installation if you use a registry with self-signed certs (like we do in our own tests: see addCertToSpec in this PR). I would expect it to be quite a common pattern.

I would actually expect it to be an exception, not a rule. The operator-controller deployment can be modified to include CAs used by the cluster.

varshaprasad96 · 2024-06-18T14:38:58Z

api/v1alpha1/clusterextension_types.go

+	// Name of the secret
+	Name string `json:"name"`
+	// Namespace of the secret
+	Namespace string `json:"namespace,omitempty"`


Should we force this to be the install namespace?

I actually disagree, the registry and it's secret are likely in a separate namespace from the installation namespace.

cmd/manager/main.go

go.mod

m1kola · 2024-06-18T14:36:51Z

api/v1alpha1/clusterextension_types.go

+
+	//+optional
+	// RegistryTLS defines the connection parameters to retrieve an image from a registry
+	RegistryTLS *ClusterExtensionTLS `json:"registryTLS,omitempty"`


It feels like it does not belong in ClusterExtension. At least in the use case when we install packages from a catalog it feels like specifying this every time we create a ClusterExtension is excessive.

We need to think about UX for the following use cases:

We install extensions from a catalog. I think in this case we probably don't want users to specify certs in ClusterExtension every time they install something from a catalog. Feels like some configuration should be done on ClusterCatalog level once and re-used.

We install something directly (like future support for direct helm installation).

Yes, this was the intent. This is the certificate used to verify the connection to the image-registry from which we get the bundle. The certificate can't be part of the catalog, as it may expire, and it could be an external repository or other location as you point out.

HOWEVER, this is optional, as the operator-controller will load the system certificates, and if the image registry's certificate is signed by a system certificate, then it's not necessary.

The more I think about this - the more I think we should not have this on ClusterExtension. Another reason - we do not specify specific catalog in ClusterExtension. If on ClusterExtension you say you want to install argocd-operator - it can come from any catalog installed on the cluster: it might come from a catalog which uses an image registry where certs are issued by a public CA, but it might also come from a catalog which uses an image registry with a self-signed cert. We don't know on the ClusterExtension level.

I can totally see that some users are not going to know what to specific in registryTLS because they don't know how argocd-operator will resolve.

This is the certificate used to verify the connection to the image-registry from which we get the bundle.

I understand that. But whoever configures ClusterCatalog probably knows better about what image registries the catalog uses. So they can provide a ref to a relevant CA on ClusterCatalog level.

The certificate can't be part of the catalog, as it may expire, and it could be an external repository or other location as you point out.

Not sure that I'm following this.

If the certificate is part of the catalog metadata, it could easily become stale - especially if a separate image-registry is used. And as you point out, the argocd could come from any catalog, and subsequently any docker-registry.

Again, this is optional, and is intended for those specific docker-registries which may be private (e.g. our e2e docker-registries) or for which don't use a system certificate.

This should really be considered an exception, not the rule.

@joelanford, I believe it was your idea to add a certificate to the ClusterExtension, do you have an additional response?

If the certificate is part of the catalog metadata

I'm not suggesting to put a certificate into catalog metadata. I'm suggesting to potentially do what you did for ClusterExtension, but for catalogd's ClusterCatalog API instead.

My idea is that people using ClusterExtension should not be forced to specify CA every time they install something on a cluster where self-signed certs are used. The cluster admins should be able to specify refs to secrets containing required CAs once (when adding a catalog to their cluster) and ClusterExtension API can remain the same.

What I suggest might also not be the right approach, but it is clear to me that we as a group need to spend some more time on the design for this.

This should really be considered an exception, not the rule.

If we provide this API - we and our users will be stuck with it for long time. We will have to support it and users will be forced to use it.

I agree with you - it won't affect people who use image registry with certs signed by public CAs. But it will result in a terrible user experience for people who install something on a cluster with self-signed certs. For them I see at least the following UX issues:

They will have to specify registryTLS for every ClusterExtension.

It is not clear what CA to specify on ClusterExtension. You have to know in advance how your package & version are going to resolve (is it going to use a registry with self-signed certs?). I think it is going to be more of a pain if people using ClusterExtension are not the same people configuring catalogs on the cluster.

IMO - we should either not support this use case at all or design so that we provide nice UX for all the use cases.

Let me know if you want to discuss this on a call. We should probably invite @joelanford as the author of the original idea.

I spoke with Joe Tuesday night and we are going to abandon the addition to the API for now and re-evaluate it later. This PR will still remove the annotations and fix the e2e to properly use certificates (and not use InsecureSkipTLSVerify).

Sounds good!

This PR will still remove the annotations and fix the e2e to properly use certificates

Could you please create a new PR for this instead? This one has quite a lot comments about the original registryTLS implementation. I think it will be easier to review in a fresh PR.

internal/controllers/clusterextension_controller.go

Updates the ClusterExtension API to support a certificate to retreive a bundle from an image registry. Signed-off-by: Todd Short <tshort@redhat.com>

Signed-off-by: Todd Short <tshort@redhat.com>

tmshort · 2024-06-20T17:23:49Z

See #960 for a different approach

tmshort requested a review from a team as a code owner June 18, 2024 14:11

tmshort force-pushed the cert-update branch 2 times, most recently from 9b67afd to d5af748 Compare June 18, 2024 14:14

everettraven reviewed Jun 18, 2024

View reviewed changes

varshaprasad96 reviewed Jun 18, 2024

View reviewed changes

m1kola reviewed Jun 18, 2024

View reviewed changes

Certificate support for image registry

756cd8a

Updates the ClusterExtension API to support a certificate to retreive a bundle from an image registry. Signed-off-by: Todd Short <tshort@redhat.com>

tmshort force-pushed the cert-update branch 2 times, most recently from 85728aa to 858efe0 Compare June 18, 2024 15:36

fixup! Certificate support for image registry

c210e85

Signed-off-by: Todd Short <tshort@redhat.com>

tmshort force-pushed the cert-update branch from 858efe0 to c210e85 Compare June 18, 2024 15:49

tmshort closed this Jun 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Certificate support for image registry #956

✨ Certificate support for image registry #956

tmshort commented Jun 18, 2024 •

edited

Loading

netlify bot commented Jun 18, 2024 •

edited

Loading

codecov bot commented Jun 18, 2024 •

edited

Loading

everettraven Jun 18, 2024

tmshort Jun 18, 2024 •

edited

Loading

m1kola Jun 18, 2024

tmshort Jun 18, 2024 •

edited

Loading

m1kola Jun 18, 2024

tmshort Jun 18, 2024

varshaprasad96 Jun 18, 2024

tmshort Jun 18, 2024

m1kola Jun 18, 2024 •

edited

Loading

tmshort Jun 18, 2024

m1kola Jun 18, 2024

tmshort Jun 18, 2024

m1kola Jun 19, 2024

tmshort Jun 19, 2024

m1kola Jun 20, 2024

tmshort commented Jun 20, 2024

✨ Certificate support for image registry #956

✨ Certificate support for image registry #956

Conversation

tmshort commented Jun 18, 2024 • edited Loading

Description

Reviewer Checklist

netlify bot commented Jun 18, 2024 • edited Loading

✅ Deploy Preview for olmv1 ready!

codecov bot commented Jun 18, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

tmshort Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmshort Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m1kola Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmshort commented Jun 20, 2024

tmshort commented Jun 18, 2024 •

edited

Loading

netlify bot commented Jun 18, 2024 •

edited

Loading

codecov bot commented Jun 18, 2024 •

edited

Loading

tmshort Jun 18, 2024 •

edited

Loading

tmshort Jun 18, 2024 •

edited

Loading

m1kola Jun 18, 2024 •

edited

Loading