Skip to content

✨ Certificate support for image registry #960

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Jun 24, 2024
Merged

✨ Certificate support for image registry #960

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b128ce3
Certificate support for image registry
tmshort Jun 17, 2024
0c20937
fixup! Certificate support for image registry
tmshort Jun 21, 2024
6155df5
fixup! Certificate support for image registry
tmshort Jun 21, 2024
0693c6a
fixup! Certificate support for image registry
tmshort Jun 22, 2024
cf5b281
fixup! Certificate support for image registry
tmshort Jun 23, 2024
421d281
fixup! Certificate support for image registry
tmshort Jun 24, 2024
3ca71ba
fixup! Certificate support for image registry
tmshort Jun 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion Tiltfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
if not os.path.exists('../tilt-support'):
fail('Please clone https://github.com/operator-framework/tilt-support to ../tilt-support')

load('../tilt-support/Tiltfile', 'deploy_repo')
load('../tilt-support/Tiltfile', 'deploy_repo', 'process_yaml')

config.define_string_list('repos', args=True)
cfg = config.parse()
Expand All @@ -16,6 +16,8 @@ repo = {
'starting_debug_port': 30000,
}

process_yaml("testdata/certs/issuers.yaml")

for r in repos:
if r == 'operator-controller':
deploy_repo('operator-controller', repo)
Expand Down
7 changes: 4 additions & 3 deletions cmd/manager/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,11 +79,11 @@ func main() {
cachePath string
operatorControllerVersion bool
systemNamespace string
caCert string
caCertDir string
)
flag.StringVar(&metricsAddr, "metrics-bind-address", ":8080", "The address the metric endpoint binds to.")
flag.StringVar(&probeAddr, "health-probe-bind-address", ":8081", "The address the probe endpoint binds to.")
flag.StringVar(&caCert, "ca-cert", "", "The TLS certificate to use for verifying HTTPS connections to the Catalogd web server.")
flag.StringVar(&caCertDir, "ca-cert-dir", "", "The directory of TLS certificate to use for verifying HTTPS connections to the Catalogd and docker-registry web servers.")
flag.BoolVar(&enableLeaderElection, "leader-elect", false,
"Enable leader election for controller manager. "+
"Enabling this will ensure there is only one active controller manager.")
Expand Down Expand Up @@ -151,7 +151,7 @@ func main() {
os.Exit(1)
}

httpClient, err := httputil.BuildHTTPClient(caCert)
httpClient, err := httputil.BuildHTTPClient(caCertDir)
if err != nil {
setupLog.Error(err, "unable to create catalogd http client")
}
Expand Down Expand Up @@ -217,6 +217,7 @@ func main() {
InstalledBundleGetter: &controllers.DefaultInstalledBundleGetter{ActionClientGetter: acg},
Handler: registryv1handler.HandlerFunc(registry.HandleBundleDeployment),
Finalizers: clusterExtensionFinalizers,
CaCertDir: caCertDir,
}).SetupWithManager(mgr); err != nil {
setupLog.Error(err, "unable to create controller", "controller", "ClusterExtension")
os.Exit(1)
Expand Down
4 changes: 3 additions & 1 deletion config/overlays/tls/kustomization.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,11 @@ apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
resources:
- ../../base
- resources/manager_cert.yaml

patches:
- target:
kind: Deployment
name: controller-manager
path: patches/manager_deployment_cert.yaml
path: patches/manager_deployment_cert.yaml
Comment on lines +21 to +22
Copy link
Member

Choose a reason for hiding this comment

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

manager_deployment_cert.yaml and manager_cert_patch.yaml seem to patch the same deployment and same fields but in different ways. I think it would be good to standardise.

- path: patches/manager_cert_patch.yaml
23 changes: 23 additions & 0 deletions config/overlays/tls/patches/manager_cert_patch.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
apiVersion: apps/v1
kind: Deployment
metadata:
name: controller-manager
namespace: system
spec:
template:
spec:
containers:
- name: kube-rbac-proxy
- name: manager
volumeMounts:
- name: e2e-cert
mountPath: /var/certs/olm-ca.crt
subPath: olm-ca.crt
readOnly: true
volumes:
- name: e2e-cert
secret:
secretName: olmv1-cert
items:
- key: ca.crt
path: olm-ca.crt
6 changes: 3 additions & 3 deletions config/overlays/tls/patches/manager_deployment_cert.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
- op: add
path: /spec/template/spec/volumes/-
value: {"name":"ca-certificate", "secret":{"secretName":"catalogd-catalogserver-cert", "optional": false, "items": [{"key": "tls.crt", "path": "tls.crt"}]}}
value: {"name":"catalogd-certificate", "secret":{"secretName":"catalogd-catalogserver-cert", "optional": false, "items": [{"key": "ca.crt", "path": "catalogd.crt"}]}}
- op: add
path: /spec/template/spec/containers/0/volumeMounts/-
value: {"name":"ca-certificate", "readOnly": true, "mountPath":"/var/certs"}
value: {"name":"catalogd-certificate", "readOnly": true, "mountPath":"/var/certs/catalogd.crt", "subPath":"catalogd.crt"}
- op: add
path: /spec/template/spec/containers/0/args/-
value: "--ca-cert=/var/certs/tls.crt"
value: "--ca-cert-dir=/var/certs"
16 changes: 16 additions & 0 deletions config/overlays/tls/resources/manager_cert.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
apiVersion: cert-manager.io/v1
kind: Certificate
metadata:
name: olmv1-cert
spec:
secretName: olmv1-cert
dnsNames:
- operator-controller.olmv1-system.svc
- operator-controller.olmv1-system.svc.cluster.local
privateKey:
algorithm: ECDSA
size: 256
issuerRef:
name: olmv1-ca
kind: ClusterIssuer
group: cert-manager.io
Comment on lines +13 to +16
Copy link
Member

Choose a reason for hiding this comment

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

This introduces a dependency on ClusterIssuer which is only being created In install.tpl.sh/in Tilt.

This means it is no longer possible to do this to install:

kubectl apply -f https://github.com/operator-framework/catalogd/releases/download/$CATALOGD_VERSION/catalogd.yaml
kubectl apply -f https://github.com/operator-framework/operator-controller/releases/download/$VERSION/operator-controller.yaml

You have to checkout the repo run the install script.

Are we ok with that?

Copy link
Member

Choose a reason for hiding this comment

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

Also about related changes in scripts/install.tpl.sh where we create CA, etc.

As far as I understand - we need to create CA and patch deployment of the manger only for E2E setups. If it is the case - can we put this in config/overlays/e2e/?

I would like to avoid having to create all this on every cluster just because our E2E setup requires it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need cert-manager regardless, AFAICT, as catalogd now creates certificates. So, the method listed above no longer works (or at least it shouldn't).
I tried to put this into the e2e overlay, but could not get the namespace to work as expected. Please see #960 (comment); I have been unable to figure out how to apply resources into the cert-manager namespace. i.e. kustomize seems to be locking me into a single namespace...

29 changes: 10 additions & 19 deletions internal/controllers/clusterextension_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ import (
catalogfilter "github.com/operator-framework/operator-controller/internal/catalogmetadata/filter"
catalogsort "github.com/operator-framework/operator-controller/internal/catalogmetadata/sort"
"github.com/operator-framework/operator-controller/internal/conditionsets"
"github.com/operator-framework/operator-controller/internal/httputil"
"github.com/operator-framework/operator-controller/internal/labels"
)

Expand All @@ -90,16 +91,13 @@ type ClusterExtensionReconciler struct {
cache cache.Cache
InstalledBundleGetter InstalledBundleGetter
Finalizers crfinalizer.Finalizers
CaCertDir string
}

type InstalledBundleGetter interface {
GetInstalledBundle(ctx context.Context, ext *ocv1alpha1.ClusterExtension) (*ocv1alpha1.BundleMetadata, error)
}

const (
bundleConnectionAnnotation string = "bundle.connection.config/insecureSkipTLSVerify"
)

//+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
Expand Down Expand Up @@ -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(bundle.Image, ext)
bd := r.generateBundleDeploymentForUnpack(ctx, bundle.Image, ext)
unpackResult, err := r.Unpacker.Unpack(ctx, bd)
if err != nil {
setStatusUnpackFailed(ext, err.Error())
Expand Down Expand Up @@ -533,7 +531,11 @@ func SetDeprecationStatus(ext *ocv1alpha1.ClusterExtension, bundle *catalogmetad
}
}

func (r *ClusterExtensionReconciler) generateBundleDeploymentForUnpack(bundlePath string, ce *ocv1alpha1.ClusterExtension) *rukpakv1alpha2.BundleDeployment {
func (r *ClusterExtensionReconciler) generateBundleDeploymentForUnpack(ctx context.Context, bundlePath string, ce *ocv1alpha1.ClusterExtension) *rukpakv1alpha2.BundleDeployment {
certData, err := httputil.LoadCerts(r.CaCertDir)
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,
Expand All @@ -548,25 +550,14 @@ func (r *ClusterExtensionReconciler) generateBundleDeploymentForUnpack(bundlePat
Source: rukpakv1alpha2.BundleSource{
Type: rukpakv1alpha2.SourceTypeImage,
Image: &rukpakv1alpha2.ImageSource{
Ref: bundlePath,
InsecureSkipTLSVerify: isInsecureSkipTLSVerifySet(ce),
Ref: bundlePath,
CertificateData: certData,
},
},
},
}
}

func isInsecureSkipTLSVerifySet(ce *ocv1alpha1.ClusterExtension) bool {
if ce == nil {
return false
}
value, ok := ce.Annotations[bundleConnectionAnnotation]
if !ok {
return false
}
return value == "true"
}

// SetupWithManager sets up the controller with the Manager.
func (r *ClusterExtensionReconciler) SetupWithManager(mgr ctrl.Manager) error {
controller, err := ctrl.NewControllerManagedBy(mgr).
Expand Down
60 changes: 45 additions & 15 deletions internal/httputil/httputil.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,30 +5,60 @@ import (
"crypto/x509"
"net/http"
"os"
"path/filepath"
"strings"
"time"
)

func BuildHTTPClient(caCert string) (*http.Client, error) {
httpClient := &http.Client{Timeout: 10 * time.Second}

if caCert != "" {
// tlsFileWatcher, err := certwatcher.New(caCert, "")
func LoadCerts(caDir string) (string, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Seems like it would be a nicer abstraction if this was:

Suggested change
func LoadCerts(caDir string) (string, error) {
func AddCertificatesFromDirectory(certPool *x509.CertPool, caDir string) error { ... }

In which case, we could add each file's content, collect errors reading/adding each file, and then tell callers which specific files failed for which reasons.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not compatible with the current rukpak API, which expects a string of PEM. Maybe when rukpak is fully integrated into operator-controller? I agree that this would be better for a larger directory. There does not appear to be a way to extract certificates/PEM from a CertPool, so it can't be done with this signature, yet.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I was just looking at LoadCerts being called in BuildHTTPClient, which turns around and consumes the certs string by immediately adding to the caCertPool.

And that made me think we could pass the caCertPool into this function and call AppendCertsFromPEM for each file's contents. Then, when my suggested function signature returns, you end up with caCertPool having everything in caDir added to it.

Seems like I'm missing something though?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's also used when creating a BundleDeployment for a rukpak API.

operator-controller/internal/controllers/clusterextension_controller.go

Line 252 in ed4e40e

bd := r.generateBundleDeploymentForUnpack(bundle.Image, ext)

So, we need to change the rukpak API to accept a caPool first, then we can do as you suggest.

if caDir == "" {
return "", nil
}

cert, err := os.ReadFile(caCert)
var certs []string
err := filepath.Walk(caDir, func(path string, info os.FileInfo, err error) error {
if err != nil {
return nil, err
return err
}
caCertPool := x509.NewCertPool()
caCertPool.AppendCertsFromPEM(cert)
tlsConfig := &tls.Config{
RootCAs: caCertPool,
MinVersion: tls.VersionTLS12,
if info.IsDir() {
return nil
Copy link
Member

Choose a reason for hiding this comment

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

Is the intention to recurse through all subdirectories? I'm tempted to suggest that we stick to finding files in a single named directory without recursing, but I'm curious if you have a specific reason.

To avoid recursing, we can return filepath.SkipDir here.

Suggested change
return nil
return filepath.SkipDir

Copy link
Contributor Author

@tmshort tmshort Jun 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

No, it's intended to skip directories. I didn't see this in any examples.

Copy link
Contributor Author

@tmshort tmshort Jun 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This appears to skip "." as a directory, meaning, it skips the current directory... so the e2es fail.
(Or something equally nefarious.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, found one example, and the example has a comparison with a directory name, so we'd need to do that to properly use filepathSkipDir; not sure it's worth it?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe switch over to os.ReadDir(), which makes the intent more clear anyways?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will look

}
tlsTransport := &http.Transport{
TLSClientConfig: tlsConfig,
data, err := os.ReadFile(path)
if err != nil {
return err
}
httpClient.Transport = tlsTransport
certs = append(certs, string(data))
return nil
})
if err != nil {
return "", err
}
return strings.Join(certs, "\n"), nil
}

func BuildHTTPClient(caDir string) (*http.Client, error) {
httpClient := &http.Client{Timeout: 10 * time.Second}

// use the SystemCertPool as a default
caCertPool, err := x509.SystemCertPool()
if err != nil {
return nil, err
}

certs, err := LoadCerts(caDir)
if err != nil {
return nil, err
}

caCertPool.AppendCertsFromPEM([]byte(certs))
tlsConfig := &tls.Config{
RootCAs: caCertPool,
MinVersion: tls.VersionTLS12,
}
tlsTransport := &http.Transport{
TLSClientConfig: tlsConfig,
}
httpClient.Transport = tlsTransport

return httpClient, nil
}
36 changes: 36 additions & 0 deletions scripts/install.tpl.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,42 @@ function kubectl_wait() {
kubectl apply -f "https://github.com/cert-manager/cert-manager/releases/download/${cert_mgr_version}/cert-manager.yaml"
kubectl_wait "cert-manager" "deployment/cert-manager-webhook" "60s"

# Create a self-signed ClusterIssuer
kubectl apply -f - <<EOF
apiVersion: cert-manager.io/v1
kind: Issuer
metadata:
name: self-sign-issuer
namespace: cert-manager
spec:
selfSigned: {}
---
apiVersion: cert-manager.io/v1
kind: Certificate
metadata:
name: olmv1-ca
namespace: cert-manager
spec:
isCA: true
commonName: olmv1-ca
secretName: olmv1-ca
privateKey:
algorithm: ECDSA
size: 256
issuerRef:
name: self-sign-issuer
kind: Issuer
group: cert-manager.io
---
apiVersion: cert-manager.io/v1
kind: ClusterIssuer
metadata:
name: olmv1-ca
spec:
ca:
secretName: olmv1-ca
EOF

kubectl apply -f "https://github.com/operator-framework/catalogd/releases/download/${catalogd_version}/catalogd.yaml"
kubectl_wait "olmv1-system" "deployment/catalogd-controller-manager" "60s"

Expand Down
3 changes: 0 additions & 3 deletions test/e2e/cluster_extension_install_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,6 @@ func testInit(t *testing.T) (*ocv1alpha1.ClusterExtension, *catalogd.ClusterCata
clusterExtension := &ocv1alpha1.ClusterExtension{
ObjectMeta: metav1.ObjectMeta{
Name: clusterExtensionName,
Annotations: map[string]string{
"bundle.connection.config/insecureSkipTLSVerify": "true",
},
},
}
return clusterExtension, extensionCatalog
Expand Down
3 changes: 0 additions & 3 deletions test/extension-developer-e2e/extension_developer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,6 @@ func TestExtensionDeveloper(t *testing.T) {
{
ObjectMeta: metav1.ObjectMeta{
Name: "registryv1",
Annotations: map[string]string{
"bundle.connection.config/insecureSkipTLSVerify": "true",
},
},
Spec: ocv1alpha1.ClusterExtensionSpec{
PackageName: os.Getenv("REG_PKG_NAME"),
Expand Down
12 changes: 2 additions & 10 deletions test/tools/image-registry.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,6 @@ metadata:
name: ${namespace}
---
apiVersion: cert-manager.io/v1
kind: Issuer
metadata:
name: selfsigned-issuer
namespace: ${namespace}
spec:
selfSigned: {}
---
apiVersion: cert-manager.io/v1
kind: Certificate
metadata:
name: ${namespace}-registry
Expand All @@ -52,8 +44,8 @@ spec:
algorithm: ECDSA
size: 256
issuerRef:
name: selfsigned-issuer
kind: Issuer
name: olmv1-ca
kind: ClusterIssuer
group: cert-manager.io
---
apiVersion: apps/v1
Expand Down
32 changes: 32 additions & 0 deletions testdata/certs/issuers.yaml
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

I don't love that this is duplicated here and in the HEREDOC, but I also recognize that this setup is temporary. Can we just make sure that our follow-ups include a callout to remove this duplication?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #967 (comment)

Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
apiVersion: cert-manager.io/v1
kind: Issuer
metadata:
name: self-sign-issuer
namespace: cert-manager
spec:
selfSigned: {}
---
apiVersion: cert-manager.io/v1
kind: Certificate
metadata:
name: olmv1-ca
namespace: cert-manager
spec:
isCA: true
commonName: olmv1-ca
secretName: olmv1-ca
privateKey:
algorithm: ECDSA
size: 256
issuerRef:
name: self-sign-issuer
kind: Issuer
group: cert-manager.io
---
apiVersion: cert-manager.io/v1
kind: ClusterIssuer
metadata:
name: olmv1-ca
spec:
ca:
secretName: olmv1-ca
Loading