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

Mark test.EquateErrors as deprecated #529

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
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
12 changes: 3 additions & 9 deletions pkg/certificates/certificates.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,30 +26,24 @@ import (
"github.com/crossplane/crossplane-runtime/pkg/errors"
)

const (
errLoadCert = "cannot load certificate"
errLoadCA = "cannot load CA certificate"
errInvalidCA = "invalid CA certificate"
)

// LoadMTLSConfig loads TLS certificates in the given folder using well-defined filenames for certificates in a Kubernetes environment.
func LoadMTLSConfig(caPath, certPath, keyPath string, isServer bool) (*tls.Config, error) {
tlsCertFilePath := filepath.Clean(certPath)
tlsKeyFilePath := filepath.Clean(keyPath)
certificate, err := tls.LoadX509KeyPair(tlsCertFilePath, tlsKeyFilePath)
if err != nil {
return nil, errors.Wrap(err, errLoadCert)
return nil, errors.Wrap(err, "cannot load certificate")
}

caCertFilePath := filepath.Clean(caPath)
ca, err := os.ReadFile(caCertFilePath)
if err != nil {
return nil, errors.Wrap(err, errLoadCA)
return nil, errors.Wrap(err, "cannot load CA certificate")
}

pool := x509.NewCertPool()
if !pool.AppendCertsFromPEM(ca) {
return nil, errors.New(errInvalidCA)
return nil, errors.New("invalid CA certificate")
}

tlsConfig := &tls.Config{
Expand Down
20 changes: 8 additions & 12 deletions pkg/certificates/certificates_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,12 @@ package certificates

import (
"crypto/tls"
"os"
"path/filepath"
"testing"

"github.com/google/go-cmp/cmp"

"github.com/crossplane/crossplane-runtime/pkg/errors"
"github.com/crossplane/crossplane-runtime/pkg/test"
)

var (
errNoSuchFile = errors.New("open invalid/path/tls.crt: no such file or directory")
errNoCAFile = errors.New("open test-data/no-ca/ca.crt: no such file or directory")
"github.com/google/go-cmp/cmp/cmpopts"
)

const (
Expand Down Expand Up @@ -42,7 +36,7 @@ func TestLoad(t *testing.T) {
certsFolderPath: "invalid/path",
},
want: want{
err: errors.Wrap(errNoSuchFile, errLoadCert),
err: os.ErrNotExist,
out: nil,
},
},
Expand All @@ -52,7 +46,7 @@ func TestLoad(t *testing.T) {
certsFolderPath: "test-data/no-ca",
},
want: want{
err: errors.Wrap(errNoCAFile, errLoadCA),
err: os.ErrNotExist,
out: nil,
},
},
Expand All @@ -62,7 +56,9 @@ func TestLoad(t *testing.T) {
certsFolderPath: "test-data/invalid-certs/",
},
want: want{
err: errors.New(errInvalidCA),
// TODO(negz): Can we be more specific? Should we use a sentinel
// error?
err: cmpopts.AnyError,
out: nil,
},
},
Expand Down Expand Up @@ -96,7 +92,7 @@ func TestLoad(t *testing.T) {
requireClient := tc.args.requireClientValidation

cfg, err := LoadMTLSConfig(filepath.Join(certsFolderPath, caCertFileName), filepath.Join(certsFolderPath, tlsCertFileName), filepath.Join(certsFolderPath, tlsKeyFileName), requireClient)
if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" {
if diff := cmp.Diff(tc.want.err, err, cmpopts.EquateErrors()); diff != "" {
t.Errorf("\n%s\nLoad(...): -want error, +got error:\n%s", tc.reason, diff)
}

Expand Down
38 changes: 13 additions & 25 deletions pkg/connection/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,18 +33,6 @@ import (
"github.com/crossplane/crossplane-runtime/pkg/resource"
)

// Error strings.
const (
errConnectStore = "cannot connect to secret store"
errWriteStore = "cannot write to secret store"
errReadStore = "cannot read from secret store"
errDeleteFromStore = "cannot delete from secret store"
errGetStoreConfig = "cannot get store config"
errSecretConflict = "cannot establish control of existing connection secret"

errFmtNotOwnedBy = "existing secret is not owned by UID %q"
)

// StoreBuilderFn is a function that builds and returns a Store with a given
// store config.
type StoreBuilderFn func(ctx context.Context, local client.Client, tcfg *tls.Config, cfg v1.SecretStoreConfig) (Store, error)
Expand Down Expand Up @@ -110,11 +98,11 @@ func (m *DetailsManager) PublishConnection(ctx context.Context, so resource.Conn

ss, err := m.connectStore(ctx, p)
if err != nil {
return false, errors.Wrap(err, errConnectStore)
return false, errors.Wrap(err, "cannot connect to secret store")
}

changed, err := ss.WriteKeyValues(ctx, store.NewSecret(so, store.KeyValues(conn)), SecretToWriteMustBeOwnedBy(so))
return changed, errors.Wrap(err, errWriteStore)
return changed, errors.Wrap(err, "cannot write to secret store")
}

// UnpublishConnection deletes connection details secret to the configured
Expand All @@ -128,10 +116,10 @@ func (m *DetailsManager) UnpublishConnection(ctx context.Context, so resource.Co

ss, err := m.connectStore(ctx, p)
if err != nil {
return errors.Wrap(err, errConnectStore)
return errors.Wrap(err, "cannot connect to secret store")
}

return errors.Wrap(ss.DeleteKeyValues(ctx, store.NewSecret(so, store.KeyValues(conn)), SecretToDeleteMustBeOwnedBy(so)), errDeleteFromStore)
return errors.Wrap(ss.DeleteKeyValues(ctx, store.NewSecret(so, store.KeyValues(conn)), SecretToDeleteMustBeOwnedBy(so)), "cannot delete from secret store")
}

// FetchConnection fetches connection details of a given ConnectionSecretOwner.
Expand All @@ -144,11 +132,11 @@ func (m *DetailsManager) FetchConnection(ctx context.Context, so resource.Connec

ss, err := m.connectStore(ctx, p)
if err != nil {
return nil, errors.Wrap(err, errConnectStore)
return nil, errors.Wrap(err, "cannot connect to secret store")
}

s := &store.Secret{}
return managed.ConnectionDetails(s.Data), errors.Wrap(ss.ReadKeyValues(ctx, store.ScopedName{Name: p.Name, Scope: so.GetNamespace()}, s), errReadStore)
return managed.ConnectionDetails(s.Data), errors.Wrap(ss.ReadKeyValues(ctx, store.ScopedName{Name: p.Name, Scope: so.GetNamespace()}, s), "cannot read from secret store")
}

// PropagateConnection propagate connection details from one resource to another.
Expand All @@ -160,37 +148,37 @@ func (m *DetailsManager) PropagateConnection(ctx context.Context, to resource.Lo

ssFrom, err := m.connectStore(ctx, from.GetPublishConnectionDetailsTo())
if err != nil {
return false, errors.Wrap(err, errConnectStore)
return false, errors.Wrap(err, "cannot connect to secret store")
}

sFrom := &store.Secret{}
if err = ssFrom.ReadKeyValues(ctx, store.ScopedName{
Name: from.GetPublishConnectionDetailsTo().Name,
Scope: from.GetNamespace(),
}, sFrom); err != nil {
return false, errors.Wrap(err, errReadStore)
return false, errors.Wrap(err, "cannot read from secret store")
}

// Make sure 'from' is the controller of the connection secret it references
// before we propagate it. This ensures a resource cannot use Crossplane to
// circumvent RBAC by propagating a secret it does not own.
if sFrom.GetOwner() != string(from.GetUID()) {
return false, errors.New(errSecretConflict)
return false, errors.New("cannot establish control of existing connection secret")
}

ssTo, err := m.connectStore(ctx, to.GetPublishConnectionDetailsTo())
if err != nil {
return false, errors.Wrap(err, errConnectStore)
return false, errors.Wrap(err, "cannot connect to secret store")
}

changed, err := ssTo.WriteKeyValues(ctx, store.NewSecret(to, sFrom.Data), SecretToWriteMustBeOwnedBy(to))
return changed, errors.Wrap(err, errWriteStore)
return changed, errors.Wrap(err, "cannot write to secret store")
}

func (m *DetailsManager) connectStore(ctx context.Context, p *v1.PublishConnectionDetailsTo) (Store, error) {
sc := m.newConfig()
if err := m.client.Get(ctx, types.NamespacedName{Name: p.SecretStoreConfigRef.Name}, sc); err != nil {
return nil, errors.Wrap(err, errGetStoreConfig)
return nil, errors.Wrap(err, "cannot get store config")
}

return m.storeBuilder(ctx, m.client, m.tcfg, sc.GetStoreConfig())
Expand All @@ -214,7 +202,7 @@ func SecretToDeleteMustBeOwnedBy(so metav1.Object) store.DeleteOption {

func secretMustBeOwnedBy(so metav1.Object, secret *store.Secret) error {
if secret.Metadata == nil || secret.Metadata.GetOwnerUID() != string(so.GetUID()) {
return errors.Errorf(errFmtNotOwnedBy, string(so.GetUID()))
return errors.Errorf("existing secret is not woned by UID %q", string(so.GetUID()))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return errors.Errorf("existing secret is not woned by UID %q", string(so.GetUID()))
return errors.Errorf("existing secret is not owned by UID %q", string(so.GetUID()))

}
return nil
}
Loading
Loading