Uh oh!
There was an error while loading. Please reload this page.
✨ Certificate support for image registry#956
Uh oh!
There was an error while loading. Please reload this page.
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -43,6 +43,25 @@ const ( | ||
| UpgradeConstraintPolicyIgnore UpgradeConstraintPolicy = "Ignore" | ||
| ) | ||
| // Similar to NamespacedName, but with json | ||
| type ClusterExtensionSecretRef struct { | ||
| // Name of the secret | ||
| Name string `json:"name"` | ||
| // Namespace of the secret | ||
| Namespace string `json:"namespace,omitempty"` | ||
Contributor There was a problem hiding this comment. 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? ContributorAuthor There was a problem hiding this comment. 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. Member There was a problem hiding this comment.
I think my comment about cert configuration being on TLDR: I think we should not ask users to specify certs on every installation. ContributorAuthor There was a problem hiding this comment. 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. Member There was a problem hiding this comment.
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 ContributorAuthor There was a problem hiding this comment. 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. | ||
| } | ||
| type ClusterExtensionTLS struct { | ||
| //+optional | ||
| // InsecureSkipTLSVerify allows the HTTPS client to ignore the server certificate | ||
| InsecureSkipTLSVerify bool `json:"insecureSkipTLSVerify,omitempty"` | ||
| // +optional | ||
| // CertificateSecretRef references a Secret that contains the tls.crt certificate that | ||
| // can verify the server certificate. | ||
| // This fits the definition of NamespacedName (but that doesn't have json tags) | ||
| CertificateSecretRef *ClusterExtensionSecretRef `json:"certificateSecretRef,omitempty"` | ||
| } | ||
| // ClusterExtensionSpec defines the desired state of ClusterExtension | ||
| type ClusterExtensionSpec struct { | ||
| //+kubebuilder:validation:MaxLength:=48 | ||
| @@ -78,6 +97,10 @@ type ClusterExtensionSpec struct { | ||
| // the bundle may contain resources that are cluster-scoped or that are | ||
| // installed in a different namespace. This namespace is expected to exist. | ||
| InstallNamespace string `json:"installNamespace"` | ||
| //+optional | ||
| // RegistryTLS defines the connection parameters to retrieve an image from a registry | ||
| RegistryTLS *ClusterExtensionTLS `json:"registryTLS,omitempty"` | ||
Comment on lines
+100
to
+103
Member There was a problem hiding this comment. It feels like it does not belong in We need to think about UX for the following use cases:
ContributorAuthor There was a problem hiding this comment. 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. Member There was a problem hiding this comment. The more I think about this - the more I think we should not have this on I can totally see that some users are not going to know what to specific in
I understand that. But whoever configures
Not sure that I'm following this. ContributorAuthor There was a problem hiding this comment. 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? Member There was a problem hiding this comment.
I'm not suggesting to put a certificate into catalog metadata. I'm suggesting to potentially do what you did for My idea is that people using 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.
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:
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. ContributorAuthor There was a problem hiding this comment. 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 Member There was a problem hiding this comment. Sounds good!
Could you please create a new PR for this instead? This one has quite a lot comments about the original | ||
| } | ||
| const ( | ||
Uh oh!
There was an error while loading. Please reload this page.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -150,7 +150,6 @@ func main() { | ||
| setupLog.Error(err, "unable to start manager") | ||
| os.Exit(1) | ||
| } | ||
| httpClient, err := httputil.BuildHTTPClient(caCert) | ||
| if err != nil { | ||
| setupLog.Error(err, "unable to create catalogd http client") | ||
| @@ -210,6 +209,7 @@ func main() { | ||
| if err = (&controllers.ClusterExtensionReconciler{ | ||
| Client: cl, | ||
| Reader: mgr.GetAPIReader(), | ||
tmshort marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| BundleProvider: catalogClient, | ||
| ActionClientGetter: acg, | ||
| Unpacker: unpacker, | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -36,6 +36,7 @@ import ( | ||
| "helm.sh/helm/v3/pkg/postrender" | ||
| "helm.sh/helm/v3/pkg/release" | ||
| "helm.sh/helm/v3/pkg/storage/driver" | ||
| corev1 "k8s.io/api/core/v1" | ||
| "k8s.io/apimachinery/pkg/api/equality" | ||
| apimeta "k8s.io/apimachinery/pkg/api/meta" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| @@ -79,6 +80,7 @@ import ( | ||
| // ClusterExtensionReconciler reconciles a ClusterExtension object | ||
| type ClusterExtensionReconciler struct { | ||
| client.Client | ||
| client.Reader | ||
| BundleProvider BundleProvider | ||
| Unpacker rukpaksource.Unpacker | ||
| ActionClientGetter helmclient.ActionClientGetter | ||
| @@ -96,10 +98,6 @@ type InstalledBundleGetter interface { | ||
| GetInstalledBundle(ctx context.Context, ext *ocv1alpha1.ClusterExtension) (*ocv1alpha1.BundleMetadata, error) | ||
| } | ||
| //+kubebuilder:rbac:groups=olm.operatorframework.io,resources=clusterextensions,verbs=get;list;watch | ||
| //+kubebuilder:rbac:groups=olm.operatorframework.io,resources=clusterextensions/status,verbs=update;patch | ||
| //+kubebuilder:rbac:groups=olm.operatorframework.io,resources=clusterextensions/finalizers,verbs=update | ||
| @@ -249,7 +247,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp | ||
| // Generate a BundleDeployment from the ClusterExtension to Unpack. | ||
| // Note: The BundleDeployment here is not a k8s API, its a simple Go struct which | ||
| // necessary embedded values. | ||
| bd := r.generateBundleDeploymentForUnpack(ctx, bundle.Image, ext) | ||
| unpackResult, err := r.Unpacker.Unpack(ctx, bd) | ||
| if err != nil { | ||
| setStatusUnpackFailed(ext, err.Error()) | ||
| @@ -533,7 +531,11 @@ func SetDeprecationStatus(ext *ocv1alpha1.ClusterExtension, bundle *catalogmetad | ||
| } | ||
| } | ||
| func (r *ClusterExtensionReconciler) generateBundleDeploymentForUnpack(ctx context.Context, bundlePath string, ce *ocv1alpha1.ClusterExtension) *rukpakv1alpha2.BundleDeployment { | ||
| certData, err := r.getCertificateData(ctx, ce) | ||
| if err != nil { | ||
| log.FromContext(ctx).WithName("operator-controller").WithValues("cluster-extension", ce.GetName()).Error(err, "unable to get TLS certificate") | ||
| } | ||
| return &rukpakv1alpha2.BundleDeployment{ | ||
| TypeMeta: metav1.TypeMeta{ | ||
| Kind: ce.Kind, | ||
| @@ -550,21 +552,54 @@ func (r *ClusterExtensionReconciler) generateBundleDeploymentForUnpack(bundlePat | ||
| Image: &rukpakv1alpha2.ImageSource{ | ||
| Ref: bundlePath, | ||
| InsecureSkipTLSVerify: isInsecureSkipTLSVerifySet(ce), | ||
| CertificateData: certData, | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
| } | ||
| func isInsecureSkipTLSVerifySet(ce *ocv1alpha1.ClusterExtension) bool { | ||
| if ce == nil || ce.Spec.RegistryTLS == nil { | ||
| return false | ||
| } | ||
| return ce.Spec.RegistryTLS.InsecureSkipTLSVerify | ||
| } | ||
| func (r *ClusterExtensionReconciler) getCertificateData(ctx context.Context, ce *ocv1alpha1.ClusterExtension) (string, error) { | ||
| if ce == nil || ce.Spec.RegistryTLS == nil || ce.Spec.RegistryTLS.CertificateSecretRef == nil { | ||
| return "", nil | ||
tmshort marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| } | ||
| secretName := getNamespacedName(*ce.Spec.RegistryTLS.CertificateSecretRef) | ||
| var secret = &corev1.Secret{} | ||
| if err := r.Reader.Get(ctx, secretName, secret); err != nil { | ||
| return "", fmt.Errorf("unable to get secret %v: %w", secretName, err) | ||
| } | ||
tmshort marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| if secret.Type != corev1.SecretTypeTLS { | ||
| return "", fmt.Errorf("invalid type in secret %v: %v", secretName, secret.Type) | ||
| } | ||
| var certs []string | ||
| // Get any 'ca.crt' | ||
| data, ok := secret.Data[corev1.ServiceAccountRootCAKey] | ||
| if ok && len(data) > 0 { | ||
| certs = append(certs, string(data)) | ||
| } | ||
| // Get any 'tls.crt' | ||
| data, ok = secret.Data[corev1.TLSCertKey] | ||
| if ok && len(data) > 0 { | ||
| certs = append(certs, string(data)) | ||
| } | ||
| if len(certs) == 0 { | ||
| return "", fmt.Errorf("no data found in secret: %v", secretName) | ||
| } | ||
| return strings.Join(certs, "\n"), nil | ||
| } | ||
| func getNamespacedName(name ocv1alpha1.ClusterExtensionSecretRef) types.NamespacedName { | ||
| if name.Namespace == "" { | ||
| return types.NamespacedName{Namespace: "default", Name: name.Name} | ||
| } | ||
| return types.NamespacedName{Namespace: name.Namespace, Name: name.Name} | ||
| } | ||
| // SetupWithManager sets up the controller with the Manager. | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we force this to be the install namespace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually disagree, the registry and it's secret are likely in a separate namespace from the installation namespace.