ref(certs): unexport methods on cert manager (#4742)

There are 2 exported methods on certmanager that are only used in unit
tests in other packages. This removes that need by leveraging interfaces
in the appropriate places where a test may be needed. The 2 methods are
GetRootCertificate and GetCertificate. This is especially useful for both
upcoming changes, and when installing certmanager, as it removes the 
burdensome step of needing to place the Root CA in a k8s secret

Signed-off-by: Sean Teeling <[email protected]>

Author: steeling

DESIGN.md

@@ -458,15 +458,9 @@ type Manager interface {
 	// IssueCertificate issues a new certificate.
 	IssueCertificate(CommonName, time.Duration) (*Certificate, error)
 
-	// GetCertificate returns a certificate given its Common Name (CN)
-	GetCertificate(CommonName) (*Certificate, error)
-
 	// RotateCertificate rotates an existing certificate.
 	RotateCertificate(CommonName) (*Certificate, error)
 
-	// GetRootCertificate returns the root certificate.
-	GetRootCertificate() (*Certificate, error)
-
 	// ListCertificates lists all certificates issued
 	ListCertificates() ([]*Certificate, error)
 

cmd/osm-bootstrap/osm-bootstrap.go

@@ -121,7 +121,6 @@ func getCertOptions() (providers.Options, error) {
 	case providers.VaultKind:
 		return vaultOptions, nil
 	case providers.CertManagerKind:
-		certManagerOptions.SecretName = caBundleSecretName
 		return certManagerOptions, nil
 	}
 	return nil, fmt.Errorf("unknown certificate provider kind: %s", certProviderKind)

cmd/osm-controller/osm-controller.go

@@ -130,7 +130,6 @@ func getCertOptions() (providers.Options, error) {
 	case providers.VaultKind:
 		return vaultOptions, nil
 	case providers.CertManagerKind:
-		certManagerOptions.SecretName = caBundleSecretName
 		return certManagerOptions, nil
 	}
 	return nil, fmt.Errorf("unknown certificate provider kind: %s", certProviderKind)

cmd/osm-injector/osm-injector.go

@@ -119,7 +119,6 @@ func getCertOptions() (providers.Options, error) {
 	case providers.VaultKind:
 		return vaultOptions, nil
 	case providers.CertManagerKind:
-		certManagerOptions.SecretName = caBundleSecretName
 		return certManagerOptions, nil
 	}
 	return nil, fmt.Errorf("unknown certificate provider kind: %s", certProviderKind)

docs/certificate_management.md

@@ -8,29 +8,13 @@ Currently there are three supported certificate managers:
 - [cert-manager](https://cert-manager.io/)
 - [HashiCorp Vault](https://www.hashicorp.com/products/vault)
 
-All certificate managers implement the `Manager` interface (located in `pkg/certificate`). Currently this interface is defined as:
+All certificate managers implement the `Issuer` interface (located in `pkg/certificate`). Currently this interface is defined as:
 
 ```go
-// Manager is the interface declaring the methods for the Certificate Manager.
-type Manager interface {
+// Issuer is the interface declaring the methods for the Certificate Manager.
+type Issuer interface {
 	// IssueCertificate issues a new certificate.
 	IssueCertificate(CommonName, time.Duration) (*Certificate, error)
-
-	// GetCertificate returns a certificate given its Common Name (CN)
-	GetCertificate(CommonName) (*Certificate, error)
-
-	// RotateCertificate rotates an existing certificate.
-	RotateCertificate(CommonName) (*Certificate, error)
-
-	// GetRootCertificate returns the root certificate in PEM format and its expiration.
-	GetRootCertificate() (*Certificate, error)
-
-	// ListCertificates lists all certificates issued
-	ListCertificates() ([]*Certificate, error)
-
-	// ReleaseCertificate informs the underlying certificate issuer that the given cert will no longer be needed.
-	// This method could be called when a given payload is terminated. Calling this should remove certs from cache and free memory if possible.
-	ReleaseCertificate(CommonName)
 }
 ```
 

pkg/certificate/fake_manager.go

@@ -10,10 +10,6 @@ import (
 )
 
 var (
-	caCert = &Certificate{
-		CommonName: "Test CA",
-		Expiration: time.Now().Add(time.Hour * 24),
-	}
 	validity = time.Hour
 )
 
@@ -43,10 +39,6 @@ func (i *fakeIssuer) IssueCertificate(cn CommonName, validityPeriod time.Duratio
 	}, nil
 }
 
-func (i *fakeIssuer) GetRootCertificate() *Certificate {
-	return caCert
-}
-
 // FakeCertManager is a testing helper that returns a *certificate.Manager
 func FakeCertManager() (*Manager, error) {
 	cm, err := NewManager(

pkg/certificate/manager.go

@@ -12,8 +12,6 @@ import (
 	"github.com/openservicemesh/osm/pkg/messaging"
 )
 
-var errCertNotFound = errors.New("certificate not found")
-
 // NewManager creates a new CertManager with the passed CA and CA Private Key
 func NewManager(mrcClient MRCClient, serviceCertValidityDuration time.Duration, msgBroker *messaging.Broker) (*Manager, error) {
 	// TODO(#4502): transition this call to a watch function that knows how to handle multiple MRC and can react to changes.
@@ -61,12 +59,7 @@ func (m *Manager) Start(checkInterval time.Duration, certRotation <-chan struct{
 }
 
 func (m *Manager) checkAndRotate() {
-	certs, err := m.ListCertificates()
-	if err != nil {
-		log.Error().Err(err).Msgf("Error listing all certificates")
-	}
-
-	for _, cert := range certs {
+	for _, cert := range m.ListIssuedCertificates() {
 		shouldRotate := cert.ShouldRotate()
 
 		word := map[bool]string{true: "will", false: "will not"}[shouldRotate]
@@ -131,14 +124,6 @@ func (m *Manager) ReleaseCertificate(cn CommonName) {
 	m.cache.Delete(cn)
 }
 
-// GetCertificate returns a certificate given its Common Name (CN)
-func (m *Manager) GetCertificate(cn CommonName) (*Certificate, error) {
-	if cert := m.getFromCache(cn); cert != nil {
-		return cert, nil
-	}
-	return nil, errCertNotFound
-}
-
 // RotateCertificate implements Manager and rotates an existing
 func (m *Manager) RotateCertificate(cn CommonName) (*Certificate, error) {
 	start := time.Now()
@@ -171,22 +156,6 @@ func (m *Manager) RotateCertificate(cn CommonName) (*Certificate, error) {
 	return newCert, nil
 }
 
-// ListCertificates lists all certificates issued
-func (m *Manager) ListCertificates() ([]*Certificate, error) {
-	var certs []*Certificate
-	m.cache.Range(func(_ interface{}, certInterface interface{}) bool {
-		certs = append(certs, certInterface.(*Certificate))
-		return true // continue the iteration
-	})
-	return certs, nil
-}
-
-// GetRootCertificate returns the root
-func (m *Manager) GetRootCertificate() *Certificate {
-	// TODO(#4502): determine client(s) to use based on the rotation stage(s) of the client(s)
-	return m.clients[0].GetRootCertificate()
-}
-
 // ListIssuedCertificates implements CertificateDebugger interface and returns the list of issued certificates.
 func (m *Manager) ListIssuedCertificates() []*Certificate {
 	var certs []*Certificate

pkg/certificate/manager_test.go

@@ -46,8 +46,7 @@ var _ = Describe("Test Certificate Manager", func() {
 			Expect(issueCertificateError).ToNot(HaveOccurred())
 			Expect(cert.GetCommonName()).To(Equal(CommonName(serviceFQDN)))
 
-			cachedCert, getCertificateError := m.GetCertificate(serviceFQDN)
-			Expect(getCertificateError).ToNot(HaveOccurred())
+			cachedCert := m.getFromCache(serviceFQDN)
 			Expect(cachedCert).To(Equal(cert))
 		})
 	})
@@ -110,70 +109,14 @@ func TestReleaseCertificate(t *testing.T) {
 			assert := tassert.New(t)
 
 			manager.ReleaseCertificate(tc.commonName)
-			_, err := manager.GetCertificate(tc.commonName)
+			cert := manager.getFromCache(tc.commonName)
 
-			assert.ErrorIs(err, errCertNotFound)
+			assert.Nil(cert)
 		})
 	}
 }
 
-func TestGetCertificate(t *testing.T) {
-	cn := CommonName("Test Cert")
-	cert := &Certificate{
-		CommonName: cn,
-		Expiration: time.Now().Add(1 * time.Hour),
-	}
-
-	expiredCn := CommonName("Expired Test Cert")
-	expiredCert := &Certificate{
-		CommonName: expiredCn,
-		Expiration: time.Now().Add(-1 * time.Hour),
-	}
-
-	manager := &Manager{}
-	manager.cache.Store(cn, cert)
-	manager.cache.Store(expiredCn, expiredCert)
-
-	testCases := []struct {
-		name                string
-		commonName          CommonName
-		expectedCertificate *Certificate
-		expectedErr         error
-	}{
-		{
-			name:                "cache hit",
-			commonName:          cn,
-			expectedCertificate: cert,
-		},
-		{
-			name:        "cache miss",
-			commonName:  CommonName("Wrong Cert"),
-			expectedErr: errCertNotFound,
-		},
-		{
-			name:        "certificate expiration",
-			commonName:  expiredCn,
-			expectedErr: errCertNotFound,
-		},
-	}
-
-	for _, tc := range testCases {
-		t.Run(tc.name, func(t *testing.T) {
-			assert := tassert.New(t)
-
-			c, err := manager.GetCertificate(tc.commonName)
-			if tc.expectedErr != nil {
-				assert.ErrorIs(err, tc.expectedErr)
-				return
-			}
-
-			assert.Nil(err)
-			assert.Equal(tc.expectedCertificate, c)
-		})
-	}
-}
-
-func TestListCertificate(t *testing.T) {
+func TestListIssuedCertificate(t *testing.T) {
 	assert := tassert.New(t)
 
 	cn := CommonName("Test Cert")
@@ -192,9 +135,7 @@ func TestListCertificate(t *testing.T) {
 	manager.cache.Store(cn, cert)
 	manager.cache.Store(anotherCn, anotherCert)
 
-	cs, err := manager.ListCertificates()
-
-	assert.Nil(err)
+	cs := manager.ListIssuedCertificates()
 	assert.Len(cs, 2)
 
 	for i, c := range cs {
@@ -212,13 +153,3 @@ func TestListCertificate(t *testing.T) {
 		}
 	}
 }
-
-func TestGetRootCertificate(t *testing.T) {
-	assert := tassert.New(t)
-
-	manager := &Manager{clients: []Issuer{&fakeIssuer{}}}
-
-	got := manager.GetRootCertificate()
-
-	assert.Equal(caCert, got)
-}