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

✨ Ensure docker registry CA is trusted in e2e tests #377

Merged
merged 2 commits into from
Sep 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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 Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -113,10 +113,12 @@ verify: tidy fmt vet generate ## Verify the current code generation and lint
lint: $(GOLANGCI_LINT) ## Run golangci linter.
$(GOLANGCI_LINT) run $(GOLANGCI_LINT_ARGS)

## image-registry target has to come after run-latest-release,
## because the image-registry depends on the olm-ca issuer.
.PHONY: test-upgrade-e2e
test-upgrade-e2e: export TEST_CLUSTER_CATALOG_NAME := test-catalog
test-upgrade-e2e: export TEST_CLUSTER_CATALOG_IMAGE := docker-registry.catalogd-e2e.svc:5000/test-catalog:e2e
test-upgrade-e2e: kind-cluster cert-manager build-container kind-load image-registry run-latest-release pre-upgrade-setup only-deploy-manifest wait post-upgrade-checks kind-cluster-cleanup ## Run upgrade e2e tests on a local kind cluster
test-upgrade-e2e: kind-cluster cert-manager build-container kind-load run-latest-release image-registry pre-upgrade-setup only-deploy-manifest wait post-upgrade-checks kind-cluster-cleanup ## Run upgrade e2e tests on a local kind cluster
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: reordering the targets just seems like churn?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The image-registry now depends on the OLM-ca Issuer being created before.
Otherwise the image-registry cert can't be created, which means the secret is missing, which means the image-registry Deployment never gets ready and the installation timeouts while waiting:

see:

error: timed out waiting for the condition on deployments/docker-registry
make: *** [Makefile:102: image-registry] Error 1
https://github.com/operator-framework/catalogd/actions/runs/10768994795/job/29859251956

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, maybe we could use a comment about these target ordering needs then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added.


pre-upgrade-setup:
./test/tools/imageregistry/pre-upgrade-setup.sh ${TEST_CLUSTER_CATALOG_IMAGE} ${TEST_CLUSTER_CATALOG_NAME}
Expand Down
11 changes: 10 additions & 1 deletion cmd/manager/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ import (
corecontrollers "github.com/operator-framework/catalogd/internal/controllers/core"
"github.com/operator-framework/catalogd/internal/features"
"github.com/operator-framework/catalogd/internal/garbagecollection"
"github.com/operator-framework/catalogd/internal/httputil"
catalogdmetrics "github.com/operator-framework/catalogd/internal/metrics"
"github.com/operator-framework/catalogd/internal/serverutil"
"github.com/operator-framework/catalogd/internal/source"
Expand Down Expand Up @@ -81,6 +82,7 @@ func main() {
certFile string
keyFile string
webhookPort int
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.")
Expand All @@ -97,6 +99,7 @@ func main() {
flag.StringVar(&certFile, "tls-cert", "", "The certificate file used for serving catalog contents over HTTPS. Requires tls-key.")
flag.StringVar(&keyFile, "tls-key", "", "The key file used for serving catalog contents over HTTPS. Requires tls-cert.")
flag.IntVar(&webhookPort, "webhook-server-port", 9443, "The port that the mutating webhook server serves at.")
flag.StringVar(&caCertDir, "ca-certs-dir", "", "The directory of TLS certificate to use for verifying HTTPS connections to the Catalogd and docker-registry web servers.")
opts := zap.Options{
Development: true,
}
Expand Down Expand Up @@ -175,7 +178,13 @@ func main() {
os.Exit(1)
}

unpacker, err := source.NewDefaultUnpacker(systemNamespace, cacheDir)
certPool, err := httputil.NewCertPool(caCertDir, ctrl.Log.WithName("cert-pool"))
if err != nil {
setupLog.Error(err, "unable to create CA certificate pool")
os.Exit(1)
}

unpacker, err := source.NewDefaultUnpacker(systemNamespace, cacheDir, certPool)
if err != nil {
setupLog.Error(err, "unable to create unpacker")
os.Exit(1)
Expand Down
1 change: 1 addition & 0 deletions config/base/manager/manager.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ spec:
matchLabels:
control-plane: catalogd-controller-manager
replicas: 1
minReadySeconds: 5
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this, because of frequent timing issues with webhooks coming online:

Error from server (InternalError): error when creating "./config/base/default/clustercatalogs/default-catalogs.yaml": Internal error occurred: failed calling webhook "inject-metadata-name.olm.operatorframework.io": failed to call webhook: Post "https://catalogd-webhook-service.olmv1-system.svc:443/mutate-olm-operatorframework-io-v1alpha1-clustercatalog?timeout=10s": context deadline exceeded

https://github.com/operator-framework/catalogd/actions/runs/10769410253/job/29860558691?pr=377

Copy link
Contributor

Choose a reason for hiding this comment

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

That does seem like a similar problem...

template:
metadata:
annotations:
Expand Down
3 changes: 3 additions & 0 deletions config/components/tls/patches/manager_deployment_certs.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,6 @@
- op: add
path: /spec/template/spec/containers/1/args/-
value: "--tls-key=/var/certs/tls.key"
- op: add
path: /spec/template/spec/containers/1/args/-
value: "--ca-certs-dir=/var/certs"
57 changes: 57 additions & 0 deletions internal/httputil/certutil.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
package httputil

import (
"crypto/x509"
"fmt"
"os"
"path/filepath"

"github.com/go-logr/logr"
)

// Should share code from operator-controller.
// see: https://issues.redhat.com/browse/OPRUN-3535
func NewCertPool(caDir string, log logr.Logger) (*x509.CertPool, error) {
caCertPool, err := x509.SystemCertPool()
if err != nil {
return nil, err
}
if caDir == "" {
return caCertPool, nil
}

dirEntries, err := os.ReadDir(caDir)
if err != nil {
return nil, err
}
count := 0

for _, e := range dirEntries {
file := filepath.Join(caDir, e.Name())
// These might be symlinks pointing to directories, so use Stat() to resolve
fi, err := os.Stat(file)
if err != nil {
return nil, err
}
if fi.IsDir() {
log.Info("skip directory", "name", e.Name())
continue
}
log.Info("load certificate", "name", e.Name())
data, err := os.ReadFile(file)
if err != nil {
return nil, fmt.Errorf("error reading cert file %q: %w", file, err)
}

if ok := caCertPool.AppendCertsFromPEM(data); ok {
count++
}
}

// Found no certs!
if count == 0 {
return nil, fmt.Errorf("no certificates found in %q", caDir)
}

return caCertPool, nil
}
73 changes: 73 additions & 0 deletions internal/httputil/main_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
package httputil

import (
"crypto/rand"
"crypto/rsa"
"crypto/x509"
"crypto/x509/pkix"
"encoding/pem"
"math/big"
"os"
"testing"
"time"

"github.com/go-logr/logr/testr"
"github.com/stretchr/testify/require"
)

func TestNewCertPool(t *testing.T) {
t.Parallel()

// set up our CA certificate
ca := &x509.Certificate{
SerialNumber: big.NewInt(2019),
Subject: pkix.Name{
Organization: []string{"Company, INC."},
Country: []string{"US"},
Province: []string{""},
Locality: []string{"San Francisco"},
StreetAddress: []string{"Golden Gate Bridge"},
PostalCode: []string{"94016"},
},
NotBefore: time.Now(),
NotAfter: time.Now().AddDate(10, 0, 0),
IsCA: true,
ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth, x509.ExtKeyUsageServerAuth},
KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign,
BasicConstraintsValid: true,
}

// create our private and public key
caPrivKey, err := rsa.GenerateKey(rand.Reader, 4096)
require.NoError(t, err)

// create the CA
caBytes, err := x509.CreateCertificate(rand.Reader, ca, ca, &caPrivKey.PublicKey, caPrivKey)
require.NoError(t, err)

// pem encode
err = os.MkdirAll("testdata/newCertPool/subfolder", 0700)
require.NoError(t, err)
t.Cleanup(func() {
require.NoError(t, os.RemoveAll("testdata/newCertPool"))
})

caPEM, err := os.Create("testdata/newCertPool/my.pem")
require.NoError(t, err)
err = pem.Encode(caPEM, &pem.Block{
Type: "CERTIFICATE",
Bytes: caBytes,
})
require.NoError(t, err)

_, err = NewCertPool("testdata/newCertPool", testr.New(t))
require.NoError(t, err)
}

func Test_newCertPool_empty(t *testing.T) {
err := os.MkdirAll("testdata/newCertPoolEmpty", 0700)
require.NoError(t, err)

_, err = NewCertPool("testdata/newCertPoolEmpty", testr.New(t))
require.EqualError(t, err, `no certificates found in "testdata/newCertPoolEmpty"`)
}
15 changes: 15 additions & 0 deletions internal/source/image_registry_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"archive/tar"
"context"
"crypto/tls"
"crypto/x509"
"errors"
"fmt"
"io/fs"
Expand Down Expand Up @@ -31,6 +32,7 @@ import (
type ImageRegistry struct {
BaseCachePath string
AuthNamespace string
CertPool *x509.CertPool
}

const ConfigDirLabel = "operators.operatorframework.io.index.configs.v1"
Expand All @@ -51,6 +53,17 @@ func (i *ImageRegistry) Unpack(ctx context.Context, catalog *catalogdv1alpha1.Cl
}

remoteOpts := []remote.Option{}
tlsTransport := remote.DefaultTransport.(*http.Transport).Clone()
if tlsTransport.TLSClientConfig == nil {
tlsTransport.TLSClientConfig = &tls.Config{
InsecureSkipVerify: false,
MinVersion: tls.VersionTLS12,
} // nolint:gosec
}
if i.CertPool != nil {
tlsTransport.TLSClientConfig.RootCAs = i.CertPool
}

if catalog.Spec.Source.Image.PullSecret != "" {
chainOpts := k8schain.Options{
ImagePullSecrets: []string{catalog.Spec.Source.Image.PullSecret},
Expand All @@ -75,6 +88,8 @@ func (i *ImageRegistry) Unpack(ctx context.Context, catalog *catalogdv1alpha1.Cl
}
insecureTransport.TLSClientConfig.InsecureSkipVerify = true // nolint:gosec
remoteOpts = append(remoteOpts, remote.WithTransport(insecureTransport))
} else {
remoteOpts = append(remoteOpts, remote.WithTransport(tlsTransport))
}

digest, isDigest := imgRef.(name.Digest)
Expand Down
4 changes: 3 additions & 1 deletion internal/source/unpacker.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package source

import (
"context"
"crypto/x509"
"fmt"
"io/fs"
"os"
Expand Down Expand Up @@ -110,7 +111,7 @@ const UnpackCacheDir = "unpack"
// source types.
//
// TODO: refactor NewDefaultUnpacker due to growing parameter list
func NewDefaultUnpacker(namespace, cacheDir string) (Unpacker, error) {
func NewDefaultUnpacker(namespace, cacheDir string, certPool *x509.CertPool) (Unpacker, error) {
unpackPath := path.Join(cacheDir, UnpackCacheDir)
if err := os.MkdirAll(unpackPath, 0700); err != nil {
return nil, fmt.Errorf("creating unpack cache directory: %w", err)
Expand All @@ -120,6 +121,7 @@ func NewDefaultUnpacker(namespace, cacheDir string) (Unpacker, error) {
catalogdv1alpha1.SourceTypeImage: &ImageRegistry{
BaseCachePath: unpackPath,
AuthNamespace: namespace,
CertPool: certPool,
},
}), nil
}
3 changes: 1 addition & 2 deletions test/e2e/unpack_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,7 @@ var _ = Describe("ClusterCatalog Unpacking", func() {
Source: catalogd.CatalogSource{
Type: catalogd.SourceTypeImage,
Image: &catalogd.ImageSource{
Ref: catalogImageRef(),
InsecureSkipTLSVerify: true,
Ref: catalogImageRef(),
},
},
},
Expand Down
12 changes: 2 additions & 10 deletions test/tools/imageregistry/imgreg.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,6 @@ metadata:
name: catalogd-e2e
---
apiVersion: cert-manager.io/v1
kind: Issuer
metadata:
name: selfsigned-issuer
namespace: catalogd-e2e
spec:
selfSigned: {}
---
apiVersion: cert-manager.io/v1
kind: Certificate
metadata:
name: catalogd-e2e-registry
Expand All @@ -25,8 +17,8 @@ spec:
algorithm: ECDSA
size: 256
issuerRef:
name: selfsigned-issuer
kind: Issuer
name: olmv1-ca
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

kind: ClusterIssuer
group: cert-manager.io
---
apiVersion: apps/v1
Expand Down