diff --git a/pkg/certificates/certificates.go b/pkg/certificates/certificates.go index e4f3c1d05..1e54b19ff 100644 --- a/pkg/certificates/certificates.go +++ b/pkg/certificates/certificates.go @@ -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{ diff --git a/pkg/certificates/certificates_test.go b/pkg/certificates/certificates_test.go index 3c854f57d..8017b768f 100644 --- a/pkg/certificates/certificates_test.go +++ b/pkg/certificates/certificates_test.go @@ -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 ( @@ -42,7 +36,7 @@ func TestLoad(t *testing.T) { certsFolderPath: "invalid/path", }, want: want{ - err: errors.Wrap(errNoSuchFile, errLoadCert), + err: os.ErrNotExist, out: nil, }, }, @@ -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, }, }, @@ -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, }, }, @@ -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) } diff --git a/pkg/connection/manager.go b/pkg/connection/manager.go index 3bc017187..e5f512500 100644 --- a/pkg/connection/manager.go +++ b/pkg/connection/manager.go @@ -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) @@ -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 @@ -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. @@ -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. @@ -160,7 +148,7 @@ 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{} @@ -168,29 +156,29 @@ func (m *DetailsManager) PropagateConnection(ctx context.Context, to resource.Lo 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()) @@ -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())) } return nil } diff --git a/pkg/connection/manager_test.go b/pkg/connection/manager_test.go index d5da750f6..c04cf8d0a 100644 --- a/pkg/connection/manager_test.go +++ b/pkg/connection/manager_test.go @@ -22,6 +22,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" kerrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" @@ -43,10 +44,6 @@ const ( testUID = "e8587e99-15c9-4069-a530-1d2205032848" ) -const ( - errBuildStore = "cannot build store" -) - var ( fakeStore = secretStoreFake @@ -87,7 +84,8 @@ func TestManagerConnectStore(t *testing.T) { }, }, want: want{ - err: errors.Wrapf(kerrors.NewNotFound(schema.GroupResource{}, fakeConfig), errGetStoreConfig), + // TODO(negz): Can we test that this is kerrors.NewNotFound? + err: cmpopts.AnyError, }, }, "BuildStoreError": { @@ -101,7 +99,7 @@ func TestManagerConnectStore(t *testing.T) { MockScheme: test.NewMockSchemeFn(resourcefake.SchemeWith(&fake.StoreConfig{})), }, sb: func(ctx context.Context, local client.Client, tCfg *tls.Config, cfg v1.SecretStoreConfig) (Store, error) { - return nil, errors.New(errBuildStore) + return nil, errBoom }, p: &v1.PublishConnectionDetailsTo{ SecretStoreConfigRef: &v1.Reference{ @@ -110,7 +108,7 @@ func TestManagerConnectStore(t *testing.T) { }, }, want: want{ - err: errors.New(errBuildStore), + err: errBoom, }, }, "SuccessfulConnect": { @@ -147,7 +145,7 @@ func TestManagerConnectStore(t *testing.T) { m := NewDetailsManager(tc.args.c, resourcefake.GVK(&fake.StoreConfig{}), WithStoreBuilder(tc.args.sb)) _, err := m.connectStore(context.Background(), tc.args.p) - if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want.err, err, cmpopts.EquateErrors()); diff != "" { t.Errorf("\nReason: %s\nm.connectStore(...): -want error, +got error:\n%s", tc.reason, diff) } }) @@ -208,7 +206,8 @@ func TestManagerPublishConnection(t *testing.T) { }, }, want: want{ - err: errors.Wrap(errors.Wrapf(kerrors.NewNotFound(schema.GroupResource{}, "non-existing"), errGetStoreConfig), errConnectStore), + // TODO(negz): Can we test that this is kerrors.NewNotFound? + err: cmpopts.AnyError, }, }, "CannotPublishTo": { @@ -242,7 +241,7 @@ func TestManagerPublishConnection(t *testing.T) { }, }, want: want{ - err: errors.Wrap(errBoom, errWriteStore), + err: errBoom, }, }, "SuccessfulPublishWithOwnerUID": { @@ -292,7 +291,7 @@ func TestManagerPublishConnection(t *testing.T) { m := NewDetailsManager(tc.args.c, resourcefake.GVK(&fake.StoreConfig{}), WithStoreBuilder(tc.args.sb)) published, err := m.PublishConnection(context.Background(), tc.args.so, tc.args.conn) - if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want.err, err, cmpopts.EquateErrors()); diff != "" { t.Errorf("\nReason: %s\nm.publishConnection(...): -want error, +got error:\n%s", tc.reason, diff) } if diff := cmp.Diff(tc.want.published, published); diff != "" { @@ -355,7 +354,8 @@ func TestManagerUnpublishConnection(t *testing.T) { }, }, want: want{ - err: errors.Wrap(errors.Wrapf(kerrors.NewNotFound(schema.GroupResource{}, "non-existing"), errGetStoreConfig), errConnectStore), + // TODO(negz): Can we test that this is kerrors.NewNotFound? + err: cmpopts.AnyError, }, }, "CannotUnpublish": { @@ -389,7 +389,7 @@ func TestManagerUnpublishConnection(t *testing.T) { }, }, want: want{ - err: errors.Wrap(errBoom, errDeleteFromStore), + err: errBoom, }, }, "CannotUnpublishUnowned": { @@ -436,7 +436,8 @@ func TestManagerUnpublishConnection(t *testing.T) { }, }, want: want{ - err: errors.Wrap(errors.Errorf(errFmtNotOwnedBy, testUID), errDeleteFromStore), + // TODO(negz): Can we be more specific? + err: cmpopts.AnyError, }, }, "SuccessfulUnpublish": { @@ -492,7 +493,7 @@ func TestManagerUnpublishConnection(t *testing.T) { m := NewDetailsManager(tc.args.c, resourcefake.GVK(&fake.StoreConfig{}), WithStoreBuilder(tc.args.sb)) err := m.UnpublishConnection(context.Background(), tc.args.so, tc.args.conn) - if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want.err, err, cmpopts.EquateErrors()); diff != "" { t.Errorf("\nReason: %s\nm.unpublishConnection(...): -want error, +got error:\n%s", tc.reason, diff) } }) @@ -552,7 +553,8 @@ func TestManagerFetchConnection(t *testing.T) { }, }, want: want{ - err: errors.Wrap(errors.Wrapf(kerrors.NewNotFound(schema.GroupResource{}, "non-existing"), errGetStoreConfig), errConnectStore), + // TODO(negz): Can we test that this is kerrors.NewNotFound? + err: cmpopts.AnyError, }, }, "CannotFetch": { @@ -586,7 +588,7 @@ func TestManagerFetchConnection(t *testing.T) { }, }, want: want{ - err: errors.Wrap(errBoom, errReadStore), + err: errBoom, }, }, "SuccessfulFetch": { @@ -635,7 +637,7 @@ func TestManagerFetchConnection(t *testing.T) { m := NewDetailsManager(tc.args.c, resourcefake.GVK(&fake.StoreConfig{}), WithStoreBuilder(tc.args.sb)) got, err := m.FetchConnection(context.Background(), tc.args.so) - if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want.err, err, cmpopts.EquateErrors()); diff != "" { t.Errorf("\nReason: %s\nm.FetchConnection(...): -want error, +got error:\n%s", tc.reason, diff) } if diff := cmp.Diff(tc.want.conn, got); diff != "" { @@ -713,7 +715,8 @@ func TestManagerPropagateConnection(t *testing.T) { to: &resourcefake.MockLocalConnectionSecretOwner{To: &v1.PublishConnectionDetailsTo{}}, }, want: want{ - err: errors.Wrap(errors.Wrapf(kerrors.NewNotFound(schema.GroupResource{}, "non-existing"), errGetStoreConfig), errConnectStore), + // TODO(negz): Can we test that this is kerrors.NewNotFound? + err: cmpopts.AnyError, }, }, "CannotFetch": { @@ -748,7 +751,7 @@ func TestManagerPropagateConnection(t *testing.T) { to: &resourcefake.MockLocalConnectionSecretOwner{To: &v1.PublishConnectionDetailsTo{}}, }, want: want{ - err: errors.Wrap(errBoom, errReadStore), + err: errBoom, }, }, "CannotEstablishControlOfUnowned": { @@ -798,7 +801,9 @@ func TestManagerPropagateConnection(t *testing.T) { }, }, want: want{ - err: errors.New(errSecretConflict), + // TODO(negz): Can we be more specific? Do we want a sentinel + // error? + err: cmpopts.AnyError, }, }, "CannotEstablishControlOfAnotherOwner": { @@ -852,7 +857,9 @@ func TestManagerPropagateConnection(t *testing.T) { }, }, want: want{ - err: errors.New(errSecretConflict), + // TODO(negz): Can we be more specific? Do we want a sentinel + // error? + err: cmpopts.AnyError, }, }, "CannotConnectDestination": { @@ -906,7 +913,8 @@ func TestManagerPropagateConnection(t *testing.T) { }, }, want: want{ - err: errors.Wrap(errors.Wrapf(kerrors.NewNotFound(schema.GroupResource{}, "non-existing"), errGetStoreConfig), errConnectStore), + // TODO(negz): Can we test that this is kerrors.NewNotFound? + err: cmpopts.AnyError, }, }, "CannotPublish": { @@ -958,7 +966,7 @@ func TestManagerPropagateConnection(t *testing.T) { }, }, want: want{ - err: errors.Wrap(errBoom, errWriteStore), + err: errBoom, }, }, "DestinationSecretCannotBeOwned": { @@ -1024,7 +1032,8 @@ func TestManagerPropagateConnection(t *testing.T) { }, }, want: want{ - err: errors.Wrap(errors.Errorf(errFmtNotOwnedBy, testUID), errWriteStore), + // TODO(negz): Can we be more specific? + err: cmpopts.AnyError, }, }, "SuccessfulPropagateCreated": { @@ -1138,7 +1147,8 @@ func TestManagerPropagateConnection(t *testing.T) { }, want: want{ propagated: false, - err: errors.Wrap(errors.Errorf(errFmtNotOwnedBy, ""), errWriteStore), + // TODO(negz): Can we be more specific? + err: cmpopts.AnyError, }, }, "SuccessfulPropagateUpdated": { @@ -1216,7 +1226,7 @@ func TestManagerPropagateConnection(t *testing.T) { m := NewDetailsManager(tc.args.c, resourcefake.GVK(&fake.StoreConfig{}), WithStoreBuilder(tc.args.sb)) got, err := m.PropagateConnection(context.Background(), tc.args.to, tc.args.from) - if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want.err, err, cmpopts.EquateErrors()); diff != "" { t.Errorf("\nReason: %s\nm.PropagateConnection(...): -want error, +got error:\n%s", tc.reason, diff) } if diff := cmp.Diff(tc.want.propagated, got); diff != "" { @@ -1231,6 +1241,6 @@ func fakeStoreBuilderFn(ss fake.SecretStore) StoreBuilderFn { if *cfg.Type == fakeStore { return &ss, nil } - return nil, errors.Errorf(errFmtUnknownSecretStore, *cfg.Type) + return nil, errors.Errorf("unknown secret store type: %q", *cfg.Type) } } diff --git a/pkg/connection/store/kubernetes/store.go b/pkg/connection/store/kubernetes/store.go index 72623efb7..bc6b6d778 100644 --- a/pkg/connection/store/kubernetes/store.go +++ b/pkg/connection/store/kubernetes/store.go @@ -37,18 +37,6 @@ import ( "github.com/crossplane/crossplane-runtime/pkg/resource" ) -// Error strings. -const ( - errGetSecret = "cannot get secret" - errDeleteSecret = "cannot delete secret" - errUpdateSecret = "cannot update secret" - errApplySecret = "cannot apply secret" - - errExtractKubernetesAuthCreds = "cannot extract kubernetes auth credentials" - errBuildRestConfig = "cannot build rest config kubeconfig" - errBuildClient = "cannot build Kubernetes client" -) - // SecretStore is a Kubernetes Secret Store. type SecretStore struct { client resource.ClientApplicator @@ -60,7 +48,7 @@ type SecretStore struct { func NewSecretStore(ctx context.Context, local client.Client, _ *tls.Config, cfg v1.SecretStoreConfig) (*SecretStore, error) { kube, err := buildClient(ctx, local, cfg) if err != nil { - return nil, errors.Wrap(err, errBuildClient) + return nil, errors.Wrap(err, "cannot build Kubernetes client") } return &SecretStore{ @@ -81,11 +69,11 @@ func buildClient(ctx context.Context, local client.Client, cfg v1.SecretStoreCon // Configure client for an external API server with a given Kubeconfig. kfg, err := resource.CommonCredentialExtractor(ctx, cfg.Kubernetes.Auth.Source, local, cfg.Kubernetes.Auth.CommonCredentialSelectors) if err != nil { - return nil, errors.Wrap(err, errExtractKubernetesAuthCreds) + return nil, errors.Wrap(err, "cannot extract Kubernetes auth credentials") } config, err := clientcmd.RESTConfigFromKubeConfig(kfg) if err != nil { - return nil, errors.Wrap(err, errBuildRestConfig) + return nil, errors.Wrap(err, "cannot build REST config kubeconfig") } return client.New(config, client.Options{}) } @@ -94,7 +82,7 @@ func buildClient(ctx context.Context, local client.Client, cfg v1.SecretStoreCon func (ss *SecretStore) ReadKeyValues(ctx context.Context, n store.ScopedName, s *store.Secret) error { ks := &corev1.Secret{} if err := ss.client.Get(ctx, types.NamespacedName{Name: n.Name, Namespace: ss.namespaceForSecret(n)}, ks); resource.IgnoreNotFound(err) != nil { - return errors.Wrap(err, errGetSecret) + return errors.Wrap(err, "cannot get secret") } s.Data = ks.Data s.Metadata = &v1.ConnectionSecretMetadata{ @@ -137,7 +125,7 @@ func (ss *SecretStore) WriteKeyValues(ctx context.Context, s *store.Secret, wo . return false, nil } if err != nil { - return false, errors.Wrap(err, errApplySecret) + return false, errors.Wrap(err, "cannot apply secret") } return true, nil } @@ -162,7 +150,7 @@ func (ss *SecretStore) DeleteKeyValues(ctx context.Context, s *store.Secret, do return nil } if err != nil { - return errors.Wrap(err, errGetSecret) + return errors.Wrap(err, "cannot get secret") } for _, o := range do { @@ -179,10 +167,10 @@ func (ss *SecretStore) DeleteKeyValues(ctx context.Context, s *store.Secret, do // Secret is deleted only if: // - No kv to delete specified as input // - No data left in the secret - return errors.Wrapf(ss.client.Delete(ctx, ks), errDeleteSecret) + return errors.Wrapf(ss.client.Delete(ctx, ks), "cannot delete secret") } // If there are still keys left, update the secret with the remaining. - return errors.Wrapf(ss.client.Update(ctx, ks), errUpdateSecret) + return errors.Wrapf(ss.client.Update(ctx, ks), "cannot update secret") } func (ss *SecretStore) namespaceForSecret(n store.ScopedName) string { diff --git a/pkg/connection/store/kubernetes/store_test.go b/pkg/connection/store/kubernetes/store_test.go index b5afd10e8..c5f25f2ca 100644 --- a/pkg/connection/store/kubernetes/store_test.go +++ b/pkg/connection/store/kubernetes/store_test.go @@ -21,6 +21,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" corev1 "k8s.io/api/core/v1" kerrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -94,7 +95,7 @@ func TestSecretStoreReadKeyValues(t *testing.T) { }, }, want: want{ - err: errors.Wrap(errBoom, errGetSecret), + err: errBoom, }, }, "SuccessfulRead": { @@ -145,7 +146,7 @@ func TestSecretStoreReadKeyValues(t *testing.T) { s := &store.Secret{} s.ScopedName = tc.args.n err := ss.ReadKeyValues(context.Background(), tc.args.n, s) - 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\nss.ReadKeyValues(...): -want error, +got error:\n%s", tc.reason, diff) } @@ -192,7 +193,7 @@ func TestSecretStoreWriteKeyValues(t *testing.T) { }, }, want: want{ - err: errors.Wrap(errBoom, errApplySecret), + err: errBoom, }, }, "FailedWriteOption": { @@ -222,7 +223,7 @@ func TestSecretStoreWriteKeyValues(t *testing.T) { }, }, want: want{ - err: errors.Wrap(errBoom, errApplySecret), + err: errBoom, }, }, "SuccessfulWriteOption": { @@ -469,7 +470,7 @@ func TestSecretStoreWriteKeyValues(t *testing.T) { defaultNamespace: tc.args.defaultNamespace, } changed, err := ss.WriteKeyValues(context.Background(), tc.args.secret, tc.args.wo...) - 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\nss.WriteKeyValues(...): -want error, +got error:\n%s", tc.reason, diff) } if diff := cmp.Diff(tc.want.changed, changed); diff != "" { @@ -512,7 +513,7 @@ func TestSecretStoreDeleteKeyValues(t *testing.T) { }, }, want: want{ - err: errors.Wrap(errBoom, errGetSecret), + err: errBoom, }, }, "SecretUpdatedWithRemainingKeys": { @@ -567,7 +568,7 @@ func TestSecretStoreDeleteKeyValues(t *testing.T) { }, }, want: want{ - err: errors.Wrap(errBoom, errDeleteSecret), + err: errBoom, }, }, "SecretAlreadyDeleted": { @@ -659,7 +660,7 @@ func TestSecretStoreDeleteKeyValues(t *testing.T) { defaultNamespace: tc.args.defaultNamespace, } err := ss.DeleteKeyValues(context.Background(), tc.args.secret, tc.args.do...) - 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\nss.DeleteKeyValues(...): -want error, +got error:\n%s", tc.reason, diff) } }) @@ -723,7 +724,8 @@ func TestNewSecretStore(t *testing.T) { }, }, want: want{ - err: errors.Wrap(errors.Wrap(errors.Wrap(kerrors.NewNotFound(schema.GroupResource{}, "kube-conn"), "cannot get credentials secret"), errExtractKubernetesAuthCreds), errBuildClient), + // TODO(negz): Can we test that this is a *kerrors.StatusError? + err: cmpopts.AnyError, }, }, "InvalidRestConfigForRemote": { @@ -769,7 +771,8 @@ malformed }, }, want: want{ - err: errors.Wrap(errors.Wrap(errors.New("yaml: line 5: could not find expected ':'"), errBuildRestConfig), errBuildClient), + // TODO(negz): Can we be more specific? + err: cmpopts.AnyError, }, }, "InvalidKubeconfigForRemote": { @@ -829,7 +832,8 @@ users: }, }, want: want{ - err: errors.Wrap(errors.New("unable to load root certificates: unable to parse bytes as PEM block"), errBuildClient), + // TODO(negz): Can we be more specific? + err: cmpopts.AnyError, }, }, } @@ -837,7 +841,7 @@ users: for name, tc := range cases { t.Run(name, func(t *testing.T) { _, err := NewSecretStore(context.Background(), tc.args.client, nil, tc.args.cfg) - 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\nNewSecretStore(...): -want error, +got error:\n%s", tc.reason, diff) } }) diff --git a/pkg/connection/store/plugin/store.go b/pkg/connection/store/plugin/store.go index fa6b3078c..cbec36a43 100644 --- a/pkg/connection/store/plugin/store.go +++ b/pkg/connection/store/plugin/store.go @@ -32,15 +32,6 @@ import ( "github.com/crossplane/crossplane-runtime/pkg/errors" ) -// Error strings. -const ( - errGet = "cannot get secret" - errApply = "cannot apply secret" - errDelete = "cannot delete secret" - - errFmtCannotDial = "cannot dial to the endpoint: %s" -) - // SecretStore is an External Secret Store. type SecretStore struct { client essproto.ExternalSecretStorePluginServiceClient @@ -55,7 +46,7 @@ func NewSecretStore(_ context.Context, kube client.Client, tcfg *tls.Config, cfg creds := credentials.NewTLS(tcfg) conn, err := grpc.Dial(cfg.Plugin.Endpoint, grpc.WithTransportCredentials(creds)) if err != nil { - return nil, errors.Wrapf(err, errFmtCannotDial, cfg.Plugin.Endpoint) + return nil, errors.Wrapf(err, "cannot dial to the endpoint: %s", cfg.Plugin.Endpoint) } return &SecretStore{ @@ -70,7 +61,7 @@ func NewSecretStore(_ context.Context, kube client.Client, tcfg *tls.Config, cfg func (ss *SecretStore) ReadKeyValues(ctx context.Context, n store.ScopedName, s *store.Secret) error { resp, err := ss.client.GetSecret(ctx, &essproto.GetSecretRequest{Secret: &essproto.Secret{ScopedName: ss.getScopedName(n)}, Config: ss.getConfigReference()}) if err != nil { - return errors.Wrap(err, errGet) + return errors.Wrap(err, "cannot get secret") } s.ScopedName = n @@ -107,7 +98,7 @@ func (ss *SecretStore) WriteKeyValues(ctx context.Context, s *store.Secret, _ .. resp, err := ss.client.ApplySecret(ctx, &essproto.ApplySecretRequest{Secret: sec, Config: ss.getConfigReference()}) if err != nil { - return false, errors.Wrap(err, errApply) + return false, errors.Wrap(err, "cannot apply secret") } return resp.Changed, nil @@ -117,7 +108,7 @@ func (ss *SecretStore) WriteKeyValues(ctx context.Context, s *store.Secret, _ .. func (ss *SecretStore) DeleteKeyValues(ctx context.Context, s *store.Secret, _ ...store.DeleteOption) error { _, err := ss.client.DeleteKeys(ctx, &essproto.DeleteKeysRequest{Secret: &essproto.Secret{ScopedName: ss.getScopedName(s.ScopedName)}, Config: ss.getConfigReference()}) - return errors.Wrap(err, errDelete) + return errors.Wrap(err, "cannot delete secret") } func (ss *SecretStore) getConfigReference() *essproto.ConfigReference { diff --git a/pkg/connection/store/plugin/store_test.go b/pkg/connection/store/plugin/store_test.go index 5219fd5c8..b693392c5 100644 --- a/pkg/connection/store/plugin/store_test.go +++ b/pkg/connection/store/plugin/store_test.go @@ -22,6 +22,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "google.golang.org/grpc" v1 "github.com/crossplane/crossplane-runtime/apis/common/v1" @@ -29,7 +30,6 @@ import ( "github.com/crossplane/crossplane-runtime/pkg/connection/store" "github.com/crossplane/crossplane-runtime/pkg/connection/store/plugin/fake" "github.com/crossplane/crossplane-runtime/pkg/errors" - "github.com/crossplane/crossplane-runtime/pkg/test" ) const ( @@ -66,7 +66,7 @@ func TestReadKeyValues(t *testing.T) { }, want: want{ out: &store.Secret{}, - err: errors.Wrap(errBoom, errGet), + err: errBoom, }, }, "SuccessfulGet": { @@ -136,7 +136,7 @@ func TestReadKeyValues(t *testing.T) { err := ss.ReadKeyValues(ctx, tc.args.sn, s) - 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\nss.ReadKeyValues(...): -want error, +got error:\n%s", tc.reason, diff) } @@ -171,7 +171,7 @@ func TestWriteKeyValues(t *testing.T) { }, want: want{ isChanged: false, - err: errors.Wrap(errBoom, errApply), + err: errBoom, }, }, "SuccessfulWrite": { @@ -208,7 +208,7 @@ func TestWriteKeyValues(t *testing.T) { isChanged, err := ss.WriteKeyValues(ctx, s) - 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\nss.WriteKeyValues(...): -want error, +got error:\n%s", tc.reason, diff) } @@ -240,7 +240,7 @@ func TestDeleteKeyValues(t *testing.T) { }}, }, want: want{ - err: errors.Wrap(errBoom, errDelete), + err: errBoom, }, }, "SuccessfulDelete": { @@ -272,7 +272,7 @@ func TestDeleteKeyValues(t *testing.T) { err := ss.DeleteKeyValues(ctx, s) - 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\nss.DeletKeyValues(...): -want error, +got error:\n%s", tc.reason, diff) } }) diff --git a/pkg/connection/stores.go b/pkg/connection/stores.go index 924d800f3..e351e52b0 100644 --- a/pkg/connection/stores.go +++ b/pkg/connection/stores.go @@ -28,10 +28,6 @@ import ( "github.com/crossplane/crossplane-runtime/pkg/errors" ) -const ( - errFmtUnknownSecretStore = "unknown secret store type: %q" -) - // RuntimeStoreBuilder builds and returns a Store for any supported Store type // in a given config. // @@ -43,5 +39,5 @@ func RuntimeStoreBuilder(ctx context.Context, local client.Client, tcfg *tls.Con case v1.SecretStorePlugin: return plugin.NewSecretStore(ctx, local, tcfg, cfg) } - return nil, errors.Errorf(errFmtUnknownSecretStore, *cfg.Type) + return nil, errors.Errorf("unknown secret store type: %q", *cfg.Type) } diff --git a/pkg/controller/engine.go b/pkg/controller/engine.go index ac470e776..40c128839 100644 --- a/pkg/controller/engine.go +++ b/pkg/controller/engine.go @@ -33,15 +33,6 @@ import ( "github.com/crossplane/crossplane-runtime/pkg/errors" ) -// Error strings -const ( - errCreateCache = "cannot create new cache" - errCreateController = "cannot create new controller" - errCrashCache = "cache error" - errCrashController = "controller error" - errWatch = "cannot setup watch" -) - // A NewCacheFn creates a new controller-runtime cache. type NewCacheFn func(cfg *rest.Config, o cache.Options) (cache.Cache, error) @@ -182,27 +173,27 @@ func (e *Engine) Start(name string, o controller.Options, w ...Watch) error { // work around this by stopping the entire cache. ca, err := e.newCache(e.mgr.GetConfig(), cache.Options{Scheme: e.mgr.GetScheme(), Mapper: e.mgr.GetRESTMapper()}) if err != nil { - return errors.Wrap(err, errCreateCache) + return errors.Wrap(err, "cannot create new cache") } ctrl, err := e.newCtrl(name, e.mgr, o) if err != nil { - return errors.Wrap(err, errCreateController) + return errors.Wrap(err, "cannot create new controller") } for _, wt := range w { if err := ctrl.Watch(source.Kind(ca, wt.kind), wt.handler, wt.predicates...); err != nil { - return errors.Wrap(err, errWatch) + return errors.Wrap(err, "cannot setup watch") } } go func() { <-e.mgr.Elected() - e.done(name, errors.Wrap(ca.Start(ctx), errCrashCache)) + e.done(name, errors.Wrap(ca.Start(ctx), "cache error")) }() go func() { <-e.mgr.Elected() - e.done(name, errors.Wrap(ctrl.Start(ctx), errCrashController)) + e.done(name, errors.Wrap(ctrl.Start(ctx), "controller error")) }() return nil diff --git a/pkg/controller/engine_test.go b/pkg/controller/engine_test.go index 1abca9f36..b3084688d 100644 --- a/pkg/controller/engine_test.go +++ b/pkg/controller/engine_test.go @@ -22,6 +22,7 @@ import ( "time" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/cache" "sigs.k8s.io/controller-runtime/pkg/controller" @@ -32,7 +33,6 @@ import ( "github.com/crossplane/crossplane-runtime/pkg/errors" "github.com/crossplane/crossplane-runtime/pkg/resource/fake" - "github.com/crossplane/crossplane-runtime/pkg/test" ) type MockCache struct { @@ -87,7 +87,7 @@ func TestEngine(t *testing.T) { name: "coolcontroller", }, want: want{ - err: errors.Wrap(errBoom, errCreateCache), + err: errBoom, }, }, "NewControllerError": { @@ -100,7 +100,7 @@ func TestEngine(t *testing.T) { name: "coolcontroller", }, want: want{ - err: errors.Wrap(errBoom, errCreateController), + err: errBoom, }, }, "WatchError": { @@ -117,7 +117,7 @@ func TestEngine(t *testing.T) { w: []Watch{For(&fake.Managed{}, nil)}, }, want: want{ - err: errors.Wrap(errBoom, errWatch), + err: errBoom, }, }, "CacheCrashError": { @@ -138,7 +138,7 @@ func TestEngine(t *testing.T) { name: "coolcontroller", }, want: want{ - crash: errors.Wrap(errBoom, errCrashCache), + crash: errBoom, }, }, "ControllerCrashError": { @@ -161,7 +161,7 @@ func TestEngine(t *testing.T) { name: "coolcontroller", }, want: want{ - crash: errors.Wrap(errBoom, errCrashController), + crash: errBoom, }, }, } @@ -169,7 +169,7 @@ func TestEngine(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { err := tc.e.Start(tc.args.name, tc.args.o, tc.args.w...) - 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\ne.Start(...): -want error, +got error:\n%s", tc.reason, diff) } @@ -178,7 +178,7 @@ func TestEngine(t *testing.T) { time.Sleep(100 * time.Millisecond) tc.e.Stop(tc.args.name) - if diff := cmp.Diff(tc.want.crash, tc.e.Err(tc.args.name), test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want.crash, tc.e.Err(tc.args.name), cmpopts.EquateErrors()); diff != "" { t.Errorf("\n%s\ne.Err(...): -want error, +got error:\n%s", tc.reason, diff) } }) diff --git a/pkg/errors/errors_test.go b/pkg/errors/errors_test.go index 479a72470..697a032dc 100644 --- a/pkg/errors/errors_test.go +++ b/pkg/errors/errors_test.go @@ -20,11 +20,12 @@ import ( "testing" "github.com/google/go-cmp/cmp" - - "github.com/crossplane/crossplane-runtime/pkg/test" + "github.com/google/go-cmp/cmp/cmpopts" ) func TestWrap(t *testing.T) { + errBoom := New("boom") + type args struct { err error message string @@ -42,17 +43,17 @@ func TestWrap(t *testing.T) { }, "NonNilError": { args: args{ - err: New("boom"), + err: errBoom, message: "very useful context", }, - want: Errorf("very useful context: %w", New("boom")), + want: errBoom, }, } for name, tc := range cases { t.Run(name, func(t *testing.T) { got := Wrap(tc.args.err, tc.args.message) - if diff := cmp.Diff(tc.want, got, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want, got, cmpopts.EquateErrors()); diff != "" { t.Errorf("Wrap(...): -want, +got:\n%s", diff) } }) @@ -60,6 +61,8 @@ func TestWrap(t *testing.T) { } func TestWrapf(t *testing.T) { + errBoom := New("boom") + type args struct { err error message string @@ -78,18 +81,18 @@ func TestWrapf(t *testing.T) { }, "NonNilError": { args: args{ - err: New("boom"), + err: errBoom, message: "very useful context about %s", args: []any{"ducks"}, }, - want: Errorf("very useful context about %s: %w", "ducks", New("boom")), + want: errBoom, }, } for name, tc := range cases { t.Run(name, func(t *testing.T) { got := Wrapf(tc.args.err, tc.args.message, tc.args.args...) - if diff := cmp.Diff(tc.want, got, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want, got, cmpopts.EquateErrors()); diff != "" { t.Errorf("Wrapf(...): -want, +got:\n%s", diff) } }) @@ -97,6 +100,8 @@ func TestWrapf(t *testing.T) { } func TestCause(t *testing.T) { + errBoom := New("boom") + cases := map[string]struct { err error want error @@ -106,19 +111,19 @@ func TestCause(t *testing.T) { want: nil, }, "BareError": { - err: New("boom"), - want: New("boom"), + err: errBoom, + want: errBoom, }, "WrappedError": { - err: Wrap(Wrap(New("boom"), "interstitial context"), "very important context"), - want: New("boom"), + err: Wrap(Wrap(errBoom, "interstitial context"), "very important context"), + want: errBoom, }, } for name, tc := range cases { t.Run(name, func(t *testing.T) { got := Cause(tc.err) - if diff := cmp.Diff(tc.want, got, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want, got, cmpopts.EquateErrors()); diff != "" { t.Errorf("Cause(...): -want, +got:\n%s", diff) } }) diff --git a/pkg/fieldpath/fieldpath_test.go b/pkg/fieldpath/fieldpath_test.go index 3b76f139e..f62752e2b 100644 --- a/pkg/fieldpath/fieldpath_test.go +++ b/pkg/fieldpath/fieldpath_test.go @@ -24,9 +24,28 @@ import ( "github.com/google/go-cmp/cmp" "github.com/crossplane/crossplane-runtime/pkg/errors" - "github.com/crossplane/crossplane-runtime/pkg/test" ) +// EquateErrorStrings() returns true if the supplied errors produce identical +// strings. +// +// This differs from cmpopts.EquateErrorStrings(), which does not test for error strings +// and instead returns whether one error 'is' (in the errors.Is sense) the +// other. +// +// In the case of this package we do really care about error strings, since +// they're being used to provide feedback for humans about where a syntax error +// occurred. +func EquateErrorStrings() cmp.Option { + return cmp.Comparer(func(a, b error) bool { + if a == nil || b == nil { + return a == nil && b == nil + } + + return a.Error() == b.Error() + }) +} + func TestSegments(t *testing.T) { cases := map[string]struct { s Segments @@ -297,7 +316,7 @@ func TestParse(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { got, err := Parse(tc.path) - if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want.err, err, EquateErrorStrings()); diff != "" { t.Fatalf("\nParse(%s): %s: -want error, +got error:\n%s", tc.path, tc.reason, diff) } if diff := cmp.Diff(tc.want.s, got); diff != "" { diff --git a/pkg/fieldpath/merge.go b/pkg/fieldpath/merge.go index 88b7d532b..2e600bc8d 100644 --- a/pkg/fieldpath/merge.go +++ b/pkg/fieldpath/merge.go @@ -25,10 +25,6 @@ import ( "github.com/crossplane/crossplane-runtime/pkg/errors" ) -const ( - errInvalidMerge = "failed to merge values" -) - // MergeValue of the receiver p at the specified field path with the supplied // value according to supplied merge options func (p *Paved) MergeValue(path string, value any, mo *xpv1.MergeOptions) error { @@ -75,7 +71,7 @@ func merge(dst, src any, mergeOptions *xpv1.MergeOptions) (any, error) { mDst := argWrap(dst) // use merge semantics with the configured merge options to obtain the target dst value if err := mergo.Merge(&mDst, argWrap(src), mergeOptions.MergoConfiguration()...); err != nil { - return nil, errors.Wrap(err, errInvalidMerge) + return nil, errors.Wrap(err, "failed to merge values") } return mDst[keyArg], nil } diff --git a/pkg/fieldpath/merge_test.go b/pkg/fieldpath/merge_test.go index 5753a3989..bb3751104 100644 --- a/pkg/fieldpath/merge_test.go +++ b/pkg/fieldpath/merge_test.go @@ -26,7 +26,6 @@ import ( "k8s.io/apimachinery/pkg/util/json" xpv1 "github.com/crossplane/crossplane-runtime/apis/common/v1" - "github.com/crossplane/crossplane-runtime/pkg/test" ) func TestMergeValue(t *testing.T) { @@ -200,7 +199,7 @@ func TestMergeValue(t *testing.T) { object: tc.fields.object, } err := p.MergeValue(tc.args.path, tc.args.value, tc.args.mo) - if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want.err, err, EquateErrorStrings()); diff != "" { t.Fatalf("\np.MergeValue(%s, %v): %s: -want error, +got error:\n%s", tc.args.path, tc.args.value, tc.reason, diff) } diff --git a/pkg/fieldpath/paved_test.go b/pkg/fieldpath/paved_test.go index db00c3050..5f9fc0c28 100644 --- a/pkg/fieldpath/paved_test.go +++ b/pkg/fieldpath/paved_test.go @@ -27,7 +27,6 @@ import ( "k8s.io/apimachinery/pkg/util/json" "github.com/crossplane/crossplane-runtime/pkg/errors" - "github.com/crossplane/crossplane-runtime/pkg/test" ) func TestIsNotFound(t *testing.T) { @@ -170,7 +169,7 @@ func TestGetValue(t *testing.T) { p := Pave(in) got, err := p.GetValue(tc.path) - if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want.err, err, EquateErrorStrings()); diff != "" { t.Fatalf("\np.GetValue(%s): %s: -want error, +got error:\n%s", tc.path, tc.reason, diff) } if diff := cmp.Diff(tc.want.value, got); diff != "" { @@ -246,7 +245,7 @@ func TestGetValueInto(t *testing.T) { p := Pave(in) err := p.GetValueInto(tc.args.path, tc.args.out) - if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want.err, err, EquateErrorStrings()); diff != "" { t.Fatalf("\np.GetValueInto(%s): %s: -want error, +got error:\n%s", tc.args.path, tc.reason, diff) } if diff := cmp.Diff(tc.want.out, tc.args.out); diff != "" { @@ -299,7 +298,7 @@ func TestGetString(t *testing.T) { p := Pave(in) got, err := p.GetString(tc.path) - if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want.err, err, EquateErrorStrings()); diff != "" { t.Fatalf("\np.GetString(%s): %s: -want error, +got error:\n%s", tc.path, tc.reason, diff) } if diff := cmp.Diff(tc.want.value, got); diff != "" { @@ -360,7 +359,7 @@ func TestGetStringArray(t *testing.T) { p := Pave(in) got, err := p.GetStringArray(tc.path) - if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want.err, err, EquateErrorStrings()); diff != "" { t.Fatalf("\np.GetStringArray(%s): %s: -want error, +got error:\n%s", tc.path, tc.reason, diff) } if diff := cmp.Diff(tc.want.value, got); diff != "" { @@ -421,7 +420,7 @@ func TestGetStringObject(t *testing.T) { p := Pave(in) got, err := p.GetStringObject(tc.path) - if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want.err, err, EquateErrorStrings()); diff != "" { t.Fatalf("\np.GetStringObject(%s): %s: -want error, +got error:\n%s", tc.path, tc.reason, diff) } if diff := cmp.Diff(tc.want.value, got); diff != "" { @@ -474,7 +473,7 @@ func TestGetBool(t *testing.T) { p := Pave(in) got, err := p.GetBool(tc.path) - if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want.err, err, EquateErrorStrings()); diff != "" { t.Fatalf("\np.GetBool(%s): %s: -want error, +got error:\n%s", tc.path, tc.reason, diff) } if diff := cmp.Diff(tc.want.value, got); diff != "" { @@ -527,7 +526,7 @@ func TestGetInteger(t *testing.T) { p := Pave(in) got, err := p.GetInteger(tc.path) - if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want.err, err, EquateErrorStrings()); diff != "" { t.Fatalf("\np.GetNumber(%s): %s: -want error, +got error:\n%s", tc.path, tc.reason, diff) } if diff := cmp.Diff(tc.want.value, got); diff != "" { @@ -801,7 +800,7 @@ func TestSetValue(t *testing.T) { p := Pave(in, tc.args.opts...) err := p.SetValue(tc.args.path, tc.args.value) - if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want.err, err, EquateErrorStrings()); diff != "" { t.Fatalf("\np.SetValue(%s, %v): %s: -want error, +got error:\n%s", tc.args.path, tc.args.value, tc.reason, diff) } if diff := cmp.Diff(tc.want.object, p.object); diff != "" { @@ -958,7 +957,7 @@ func TestExpandWildcards(t *testing.T) { p := Pave(in) got, err := p.ExpandWildcards(tc.path) - if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want.err, err, EquateErrorStrings()); diff != "" { t.Fatalf("\np.ExpandWildcards(%s): %s: -want error, +got error:\n%s", tc.path, tc.reason, diff) } if diff := cmp.Diff(tc.want.expanded, got, cmpopts.SortSlices(func(x, y string) bool { @@ -1246,7 +1245,7 @@ func TestDeleteField(t *testing.T) { p := Pave(in) err := p.DeleteField(tc.args.path) - if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want.err, err, EquateErrorStrings()); diff != "" { t.Fatalf("\np.DeleteField(%s): %s: -want error, +got error:\n%s", tc.args.path, tc.reason, diff) } if diff := cmp.Diff(tc.want.object, p.object); diff != "" { diff --git a/pkg/meta/meta_test.go b/pkg/meta/meta_test.go index 4936292e7..632d1633e 100644 --- a/pkg/meta/meta_test.go +++ b/pkg/meta/meta_test.go @@ -21,14 +21,13 @@ import ( "time" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" xpv1 "github.com/crossplane/crossplane-runtime/apis/common/v1" - "github.com/crossplane/crossplane-runtime/pkg/errors" - "github.com/crossplane/crossplane-runtime/pkg/test" ) const ( @@ -418,7 +417,7 @@ func TestAddControllerReference(t *testing.T) { }, want: want{ owners: []metav1.OwnerReference{otrlr}, - err: errors.Errorf("%s is already controlled by %s %s (UID %s)", name, otrlr.Kind, otrlr.Name, otrlr.UID), + err: cmpopts.AnyError, }, }, } @@ -426,7 +425,7 @@ func TestAddControllerReference(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { err := AddControllerReference(tc.args.o, tc.args.r) - if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want.err, err, cmpopts.EquateErrors()); diff != "" { t.Errorf("AddControllerReference(...): -want error, +got error:\n%s", diff) } diff --git a/pkg/parser/linter.go b/pkg/parser/linter.go index a9fdce09b..e72ce52cf 100644 --- a/pkg/parser/linter.go +++ b/pkg/parser/linter.go @@ -24,12 +24,6 @@ import ( "github.com/crossplane/crossplane-runtime/pkg/errors" ) -const ( - errNilLinterFn = "linter function is nil" - - errOrFmt = "object did not pass any of the linters with following errors: %s" -) - // A Linter lints packages. type Linter interface { Lint(*Package) error @@ -102,7 +96,7 @@ func Or(linters ...ObjectLinterFn) ObjectLinterFn { var errs []string for _, l := range linters { if l == nil { - return errors.New(errNilLinterFn) + return errors.New("linter function is nil") } err := l(o) if err == nil { @@ -110,6 +104,6 @@ func Or(linters ...ObjectLinterFn) ObjectLinterFn { } errs = append(errs, err.Error()) } - return errors.Errorf(errOrFmt, strings.Join(errs, ", ")) + return errors.Errorf("object did not pass any of the linters with following errors: %s", strings.Join(errs, ", ")) } } diff --git a/pkg/parser/linter_test.go b/pkg/parser/linter_test.go index 10e640490..26ccfc556 100644 --- a/pkg/parser/linter_test.go +++ b/pkg/parser/linter_test.go @@ -20,10 +20,10 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "k8s.io/apimachinery/pkg/runtime" "github.com/crossplane/crossplane-runtime/pkg/errors" - "github.com/crossplane/crossplane-runtime/pkg/test" ) var _ Linter = &PackageLinter{} @@ -112,7 +112,7 @@ func TestLinter(t *testing.T) { objects: []runtime.Object{crd}, }, }, - err: errors.Errorf(errOrFmt, errBoom.Error()+", "+errBoom.Error()), + err: cmpopts.AnyError, }, } @@ -120,7 +120,7 @@ func TestLinter(t *testing.T) { t.Run(name, func(t *testing.T) { err := tc.args.linter.Lint(tc.args.pkg) - if diff := cmp.Diff(tc.err, err, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.err, err, cmpopts.EquateErrors()); diff != "" { t.Errorf("\n%s\nl.Lint(...): -want error, +got error:\n%s", tc.reason, diff) } }) @@ -160,7 +160,7 @@ func TestOr(t *testing.T) { one: objFail, two: objFail, }, - err: errors.Errorf(errOrFmt, errBoom.Error()+", "+errBoom.Error()), + err: cmpopts.AnyError, }, "ErrNilLinter": { reason: "Passing a nil linter will should always return error.", @@ -168,7 +168,7 @@ func TestOr(t *testing.T) { one: nil, two: objPass, }, - err: errors.New(errNilLinterFn), + err: cmpopts.AnyError, }, } @@ -176,7 +176,7 @@ func TestOr(t *testing.T) { t.Run(name, func(t *testing.T) { err := Or(tc.args.one, tc.args.two)(crd) - if diff := cmp.Diff(tc.err, err, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.err, err, cmpopts.EquateErrors()); diff != "" { t.Errorf("\n%s\nOr(...): -want error, +got error:\n%s", tc.reason, diff) } }) diff --git a/pkg/ratelimiter/reconciler_test.go b/pkg/ratelimiter/reconciler_test.go index c8cdf5963..7e5ea8cad 100644 --- a/pkg/ratelimiter/reconciler_test.go +++ b/pkg/ratelimiter/reconciler_test.go @@ -22,11 +22,10 @@ import ( "time" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/ratelimiter" "sigs.k8s.io/controller-runtime/pkg/reconcile" - - "github.com/crossplane/crossplane-runtime/pkg/test" ) var _ ratelimiter.RateLimiter = &predictableRateLimiter{} @@ -100,7 +99,7 @@ func TestReconcile(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { got, err := tc.r.Reconcile(tc.args.ctx, tc.args.req) - if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want.err, err, cmpopts.EquateErrors()); diff != "" { t.Errorf("%s\nr.Reconcile(...): -want, +got error:\n%s", tc.reason, diff) } if diff := cmp.Diff(tc.want.res, got); diff != "" { diff --git a/pkg/reconciler/managed/api.go b/pkg/reconciler/managed/api.go index 6fd981442..035318d1c 100644 --- a/pkg/reconciler/managed/api.go +++ b/pkg/reconciler/managed/api.go @@ -32,15 +32,6 @@ import ( "github.com/crossplane/crossplane-runtime/pkg/resource" ) -// Error strings. -const ( - errCreateOrUpdateSecret = "cannot create or update connection secret" - errUpdateManaged = "cannot update managed resource" - errUpdateManagedStatus = "cannot update managed resource status" - errResolveReferences = "cannot resolve references" - errUpdateCriticalAnnotations = "cannot update critical annotations" -) - // NameAsExternalName writes the name of the managed resource to // the external name annotation field in order to be used as name of // the external resource in provider. @@ -57,7 +48,7 @@ func (a *NameAsExternalName) Initialize(ctx context.Context, mg resource.Managed return nil } meta.SetExternalName(mg, mg.GetName()) - return errors.Wrap(a.client.Update(ctx, mg), errUpdateManaged) + return errors.Wrap(a.client.Update(ctx, mg), "cannot update managed resource") } // An APISecretPublisher publishes ConnectionDetails by submitting a Secret to a @@ -102,7 +93,7 @@ func (a *APISecretPublisher) PublishConnection(ctx context.Context, o resource.C return false, nil } if err != nil { - return false, errors.Wrap(err, errCreateOrUpdateSecret) + return false, errors.Wrap(err, "cannot create or update connection secret") } return true, nil @@ -142,7 +133,7 @@ func (a *APISimpleReferenceResolver) ResolveReferences(ctx context.Context, mg r existing := mg.DeepCopyObject() if err := rr.ResolveReferences(ctx, a.client); err != nil { - return errors.Wrap(err, errResolveReferences) + return errors.Wrap(err, "cannot resolve references") } if cmp.Equal(existing, mg) { @@ -150,7 +141,7 @@ func (a *APISimpleReferenceResolver) ResolveReferences(ctx context.Context, mg r return nil } - return errors.Wrap(a.client.Update(ctx, mg), errUpdateManaged) + return errors.Wrap(a.client.Update(ctx, mg), "cannot update managed resource") } // A RetryingCriticalAnnotationUpdater is a CriticalAnnotationUpdater that @@ -180,5 +171,5 @@ func (u *RetryingCriticalAnnotationUpdater) UpdateCriticalAnnotations(ctx contex meta.AddAnnotations(o, a) return u.client.Update(ctx, o) }) - return errors.Wrap(err, errUpdateCriticalAnnotations) + return errors.Wrap(err, "cannot update critical annotations") } diff --git a/pkg/reconciler/managed/api_test.go b/pkg/reconciler/managed/api_test.go index 47b491b14..5c282da57 100644 --- a/pkg/reconciler/managed/api_test.go +++ b/pkg/reconciler/managed/api_test.go @@ -21,6 +21,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -64,7 +65,7 @@ func TestNameAsExternalName(t *testing.T) { mg: &fake.Managed{ObjectMeta: metav1.ObjectMeta{Name: testExternalName}}, }, want: want{ - err: errors.Wrap(errBoom, errUpdateManaged), + err: errBoom, mg: &fake.Managed{ObjectMeta: metav1.ObjectMeta{ Name: testExternalName, Annotations: map[string]string{meta.AnnotationKeyExternalName: testExternalName}, @@ -107,7 +108,7 @@ func TestNameAsExternalName(t *testing.T) { t.Run(name, func(t *testing.T) { api := NewNameAsExternalName(tc.client) err := api.Initialize(tc.args.ctx, tc.args.mg) - if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want.err, err, cmpopts.EquateErrors()); diff != "" { t.Errorf("api.Initialize(...): -want error, +got error:\n%s", diff) } if diff := cmp.Diff(tc.want.mg, tc.args.mg, test.EquateConditions()); diff != "" { @@ -168,7 +169,7 @@ func TestAPISecretPublisher(t *testing.T) { mg: mg, }, want: want{ - err: errors.Wrap(errBoom, errCreateOrUpdateSecret), + err: errBoom, }, }, "AlreadyPublished": { @@ -224,7 +225,7 @@ func TestAPISecretPublisher(t *testing.T) { t.Run(name, func(t *testing.T) { a := &APISecretPublisher{tc.fields.secret, tc.fields.typer} got, gotErr := a.PublishConnection(tc.args.ctx, tc.args.mg, tc.args.c) - if diff := cmp.Diff(tc.want.err, gotErr, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want.err, gotErr, cmpopts.EquateErrors()); diff != "" { t.Errorf("\n%s\nPublish(...): -wantErr, +gotErr:\n%s", tc.reason, diff) } if diff := cmp.Diff(tc.want.published, got); diff != "" { @@ -290,7 +291,7 @@ func TestResolveReferences(t *testing.T) { }, }, }, - want: errors.Wrap(errBoom, errResolveReferences), + want: errBoom, }, "SuccessfulNoop": { reason: "Should return without error when resolution does not change the managed resource.", @@ -340,7 +341,7 @@ func TestResolveReferences(t *testing.T) { }, }, }, - want: errors.Wrap(errBoom, errUpdateManaged), + want: errBoom, }, } @@ -348,7 +349,7 @@ func TestResolveReferences(t *testing.T) { t.Run(name, func(t *testing.T) { r := NewAPISimpleReferenceResolver(tc.c) got := r.ResolveReferences(tc.args.ctx, tc.args.mg) - if diff := cmp.Diff(tc.want, got, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want, got, cmpopts.EquateErrors()); diff != "" { t.Errorf("\n%s\nr.ResolveReferences(...): -want, +got:\n%s", tc.reason, diff) } }) @@ -378,7 +379,7 @@ func TestRetryingCriticalAnnotationUpdater(t *testing.T) { args: args{ o: &fake.Managed{}, }, - want: errors.Wrap(errBoom, errUpdateCriticalAnnotations), + want: errBoom, }, "UpdateError": { reason: "We should return any error we encounter updating the supplied object", @@ -389,7 +390,7 @@ func TestRetryingCriticalAnnotationUpdater(t *testing.T) { args: args{ o: &fake.Managed{}, }, - want: errors.Wrap(errBoom, errUpdateCriticalAnnotations), + want: errBoom, }, "Success": { reason: "We should return without error if we successfully update our annotations", @@ -400,7 +401,7 @@ func TestRetryingCriticalAnnotationUpdater(t *testing.T) { args: args{ o: &fake.Managed{}, }, - want: errors.Wrap(errBoom, errUpdateCriticalAnnotations), + want: errBoom, }, } @@ -408,7 +409,7 @@ func TestRetryingCriticalAnnotationUpdater(t *testing.T) { t.Run(name, func(t *testing.T) { u := NewRetryingCriticalAnnotationUpdater(tc.c) got := u.UpdateCriticalAnnotations(tc.args.ctx, tc.args.o) - if diff := cmp.Diff(tc.want, got, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want, got, cmpopts.EquateErrors()); diff != "" { t.Errorf("\n%s\nu.UpdateCriticalAnnotations(...): -want, +got:\n%s", tc.reason, diff) } }) diff --git a/pkg/reconciler/managed/policies.go b/pkg/reconciler/managed/policies.go index 24de639bb..fee150641 100644 --- a/pkg/reconciler/managed/policies.go +++ b/pkg/reconciler/managed/policies.go @@ -112,7 +112,7 @@ func (m *ManagementPoliciesResolver) Validate() error { // check if its disabled, but uses a non-default value. if !m.enabled { if !m.managementPolicies.Equal(sets.New[xpv1.ManagementAction](xpv1.ManagementActionAll)) && m.managementPolicies.Len() != 0 { - return fmt.Errorf(errFmtManagementPolicyNonDefault, m.managementPolicies.UnsortedList()) + return fmt.Errorf("`spec.managementPolicies` is set to a non-default value but the feature is not enabled: %s", m.managementPolicies.UnsortedList()) } // if its just disabled we don't care about supported policies return nil @@ -124,7 +124,7 @@ func (m *ManagementPoliciesResolver) Validate() error { return nil } } - return fmt.Errorf(errFmtManagementPolicyNotSupported, m.managementPolicies.UnsortedList()) + return fmt.Errorf("`spec.managementPolicies` is set to a value(%s) which is not supported. Check docs for supported policies", m.managementPolicies.UnsortedList()) } // IsPaused returns true if the management policy is empty and the diff --git a/pkg/reconciler/managed/policies_test.go b/pkg/reconciler/managed/policies_test.go new file mode 100644 index 000000000..64d26446d --- /dev/null +++ b/pkg/reconciler/managed/policies_test.go @@ -0,0 +1,457 @@ +/* +Copyright 2023 The Crossplane Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package managed + +import ( + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + + xpv1 "github.com/crossplane/crossplane-runtime/apis/common/v1" + "github.com/crossplane/crossplane-runtime/pkg/resource" + "github.com/crossplane/crossplane-runtime/pkg/resource/fake" +) + +func TestTestManagementPoliciesResolverIsPaused(t *testing.T) { + type args struct { + enabled bool + policy xpv1.ManagementPolicies + } + cases := map[string]struct { + reason string + args args + want bool + }{ + "Disabled": { + reason: "Should return false if management policies are disabled", + args: args{ + enabled: false, + policy: xpv1.ManagementPolicies{}, + }, + want: false, + }, + "EnabledEmptyPolicies": { + reason: "Should return true if the management policies are enabled and empty", + args: args{ + enabled: true, + policy: xpv1.ManagementPolicies{}, + }, + want: true, + }, + "EnabledNonEmptyPolicies": { + reason: "Should return true if the management policies are enabled and non empty", + args: args{ + enabled: true, + policy: xpv1.ManagementPolicies{xpv1.ManagementActionAll}, + }, + want: false, + }, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + r := NewManagementPoliciesResolver(tc.args.enabled, tc.args.policy, xpv1.DeletionDelete) + if diff := cmp.Diff(tc.want, r.IsPaused()); diff != "" { + t.Errorf("\nReason: %s\nIsPaused(...): -want, +got:\n%s", tc.reason, diff) + } + }) + } +} + +func TestManagementPoliciesResolverValidate(t *testing.T) { + type args struct { + enabled bool + policy xpv1.ManagementPolicies + } + cases := map[string]struct { + reason string + args args + want error + }{ + "Enabled": { + reason: "Should return nil if the management policy is enabled.", + args: args{ + enabled: true, + policy: xpv1.ManagementPolicies{}, + }, + want: nil, + }, + "DisabledNonDefault": { + reason: "Should return error if the management policy is non-default and disabled.", + args: args{ + enabled: false, + policy: xpv1.ManagementPolicies{xpv1.ManagementActionCreate}, + }, + want: cmpopts.AnyError, + }, + "DisabledDefault": { + reason: "Should return nil if the management policy is default and disabled.", + args: args{ + enabled: false, + policy: xpv1.ManagementPolicies{xpv1.ManagementActionAll}, + }, + want: nil, + }, + "EnabledSupported": { + reason: "Should return nil if the management policy is supported.", + args: args{ + enabled: true, + policy: xpv1.ManagementPolicies{xpv1.ManagementActionAll}, + }, + want: nil, + }, + "EnabledNotSupported": { + reason: "Should return err if the management policy is not supported.", + args: args{ + enabled: true, + policy: xpv1.ManagementPolicies{xpv1.ManagementActionDelete}, + }, + want: cmpopts.AnyError, + }, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + r := NewManagementPoliciesResolver(tc.args.enabled, tc.args.policy, xpv1.DeletionDelete) + if diff := cmp.Diff(tc.want, r.Validate(), cmpopts.EquateErrors()); diff != "" { + t.Errorf("\nReason: %s\nIsNonDefault(...): -want, +got:\n%s", tc.reason, diff) + } + }) + } +} + +func TestManagementPoliciesResolverShouldCreate(t *testing.T) { + type args struct { + managementPoliciesEnabled bool + policy xpv1.ManagementPolicies + } + cases := map[string]struct { + reason string + args args + want bool + }{ + "ManagementPoliciesDisabled": { + reason: "Should return true if management policies are disabled", + args: args{ + managementPoliciesEnabled: false, + }, + want: true, + }, + "ManagementPoliciesEnabledHasCreate": { + reason: "Should return true if management policies are enabled and managementPolicies has action Create", + args: args{ + managementPoliciesEnabled: true, + policy: xpv1.ManagementPolicies{xpv1.ManagementActionCreate}, + }, + want: true, + }, + "ManagementPoliciesEnabledHasCreateAll": { + reason: "Should return true if management policies are enabled and managementPolicies has action All", + args: args{ + managementPoliciesEnabled: true, + policy: xpv1.ManagementPolicies{xpv1.ManagementActionAll}, + }, + want: true, + }, + "ManagementPoliciesEnabledActionNotAllowed": { + reason: "Should return false if management policies are enabled and managementPolicies does not have Create", + args: args{ + managementPoliciesEnabled: true, + policy: xpv1.ManagementPolicies{xpv1.ManagementActionObserve}, + }, + want: false, + }, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + r := NewManagementPoliciesResolver(tc.args.managementPoliciesEnabled, tc.args.policy, xpv1.DeletionOrphan) + if diff := cmp.Diff(tc.want, r.ShouldCreate()); diff != "" { + t.Errorf("\nReason: %s\nShouldCreate(...): -want, +got:\n%s", tc.reason, diff) + } + }) + } +} + +func TestManagementPoliciesResolverShouldUpdate(t *testing.T) { + type args struct { + managementPoliciesEnabled bool + policy xpv1.ManagementPolicies + } + cases := map[string]struct { + reason string + args args + want bool + }{ + "ManagementPoliciesDisabled": { + reason: "Should return true if management policies are disabled", + args: args{ + managementPoliciesEnabled: false, + }, + want: true, + }, + "ManagementPoliciesEnabledHasUpdate": { + reason: "Should return true if management policies are enabled and managementPolicies has action Update", + args: args{ + managementPoliciesEnabled: true, + policy: xpv1.ManagementPolicies{xpv1.ManagementActionUpdate}, + }, + want: true, + }, + "ManagementPoliciesEnabledHasUpdateAll": { + reason: "Should return true if management policies are enabled and managementPolicies has action All", + args: args{ + managementPoliciesEnabled: true, + policy: xpv1.ManagementPolicies{xpv1.ManagementActionAll}, + }, + want: true, + }, + "ManagementPoliciesEnabledActionNotAllowed": { + reason: "Should return false if management policies are enabled and managementPolicies does not have Update", + args: args{ + managementPoliciesEnabled: true, + policy: xpv1.ManagementPolicies{xpv1.ManagementActionObserve}, + }, + want: false, + }, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + r := NewManagementPoliciesResolver(tc.args.managementPoliciesEnabled, tc.args.policy, xpv1.DeletionOrphan) + if diff := cmp.Diff(tc.want, r.ShouldUpdate()); diff != "" { + t.Errorf("\nReason: %s\nShouldUpdate(...): -want, +got:\n%s", tc.reason, diff) + } + }) + } +} + +func TestManagementPoliciesResolverShouldLateInitialize(t *testing.T) { + type args struct { + managementPoliciesEnabled bool + policy xpv1.ManagementPolicies + } + cases := map[string]struct { + reason string + args args + want bool + }{ + "ManagementPoliciesDisabled": { + reason: "Should return true if management policies are disabled", + args: args{ + managementPoliciesEnabled: false, + }, + want: true, + }, + "ManagementPoliciesEnabledHasLateInitialize": { + reason: "Should return true if management policies are enabled and managementPolicies has action LateInitialize", + args: args{ + managementPoliciesEnabled: true, + policy: xpv1.ManagementPolicies{xpv1.ManagementActionLateInitialize}, + }, + want: true, + }, + "ManagementPoliciesEnabledHasLateInitializeAll": { + reason: "Should return true if management policies are enabled and managementPolicies has action All", + args: args{ + managementPoliciesEnabled: true, + policy: xpv1.ManagementPolicies{xpv1.ManagementActionAll}, + }, + want: true, + }, + "ManagementPoliciesEnabledActionNotAllowed": { + reason: "Should return false if management policies are enabled and managementPolicies does not have LateInitialize", + args: args{ + managementPoliciesEnabled: true, + policy: xpv1.ManagementPolicies{xpv1.ManagementActionObserve}, + }, + want: false, + }, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + r := NewManagementPoliciesResolver(tc.args.managementPoliciesEnabled, tc.args.policy, xpv1.DeletionOrphan) + if diff := cmp.Diff(tc.want, r.ShouldLateInitialize()); diff != "" { + t.Errorf("\nReason: %s\nShouldLateInitialize(...): -want, +got:\n%s", tc.reason, diff) + } + }) + } +} + +func TestManagementPoliciesResolverOnlyObserve(t *testing.T) { + type args struct { + managementPoliciesEnabled bool + policy xpv1.ManagementPolicies + } + cases := map[string]struct { + reason string + args args + want bool + }{ + "ManagementPoliciesDisabled": { + reason: "Should return false if management policies are disabled", + args: args{ + managementPoliciesEnabled: false, + }, + want: false, + }, + "ManagementPoliciesEnabledHasOnlyObserve": { + reason: "Should return true if management policies are enabled and managementPolicies has action LateInitialize", + args: args{ + managementPoliciesEnabled: true, + policy: xpv1.ManagementPolicies{xpv1.ManagementActionObserve}, + }, + want: true, + }, + "ManagementPoliciesEnabledHasMultipleActions": { + reason: "Should return false if management policies are enabled and managementPolicies has multiple actions", + args: args{ + managementPoliciesEnabled: true, + policy: xpv1.ManagementPolicies{xpv1.ManagementActionLateInitialize, xpv1.ManagementActionObserve}, + }, + want: false, + }, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + r := NewManagementPoliciesResolver(tc.args.managementPoliciesEnabled, tc.args.policy, xpv1.DeletionOrphan) + if diff := cmp.Diff(tc.want, r.ShouldOnlyObserve()); diff != "" { + t.Errorf("\nReason: %s\nShouldOnlyObserve(...): -want, +got:\n%s", tc.reason, diff) + } + }) + } +} + +func TestShouldDelete(t *testing.T) { + type args struct { + managementPoliciesEnabled bool + managed resource.Managed + } + type want struct { + delete bool + } + cases := map[string]struct { + reason string + args args + want want + }{ + "DeletionOrphan": { + reason: "Should orphan if management policies are disabled and deletion policy is set to Orphan.", + args: args{ + managementPoliciesEnabled: false, + managed: &fake.Managed{ + Orphanable: fake.Orphanable{ + Policy: xpv1.DeletionOrphan, + }, + }, + }, + want: want{delete: false}, + }, + "DeletionDelete": { + reason: "Should delete if management policies are disabled and deletion policy is set to Delete.", + args: args{ + managementPoliciesEnabled: false, + managed: &fake.Managed{ + Orphanable: fake.Orphanable{ + Policy: xpv1.DeletionDelete, + }, + }, + }, + want: want{delete: true}, + }, + "DeletionDeleteManagementActionAll": { + reason: "Should delete if management policies are enabled and deletion policy is set to Delete and management policy is set to All.", + args: args{ + managementPoliciesEnabled: true, + managed: &fake.Managed{ + Orphanable: fake.Orphanable{ + Policy: xpv1.DeletionDelete, + }, + Manageable: fake.Manageable{ + Policy: xpv1.ManagementPolicies{xpv1.ManagementActionAll}, + }, + }, + }, + want: want{delete: true}, + }, + "DeletionOrphanManagementActionAll": { + reason: "Should orphan if management policies are enabled and deletion policy is set to Orphan and management policy is set to All.", + args: args{ + managementPoliciesEnabled: true, + managed: &fake.Managed{ + Orphanable: fake.Orphanable{ + Policy: xpv1.DeletionOrphan, + }, + Manageable: fake.Manageable{ + Policy: xpv1.ManagementPolicies{xpv1.ManagementActionAll}, + }, + }, + }, + want: want{delete: false}, + }, + "DeletionDeleteManagementActionDelete": { + reason: "Should delete if management policies are enabled and deletion policy is set to Delete and management policy has action Delete.", + args: args{ + managementPoliciesEnabled: true, + managed: &fake.Managed{ + Orphanable: fake.Orphanable{ + Policy: xpv1.DeletionDelete, + }, + Manageable: fake.Manageable{ + Policy: xpv1.ManagementPolicies{xpv1.ManagementActionDelete}, + }, + }, + }, + want: want{delete: true}, + }, + "DeletionOrphanManagementActionDelete": { + reason: "Should delete if management policies are enabled and deletion policy is set to Orphan and management policy has action Delete.", + args: args{ + managementPoliciesEnabled: true, + managed: &fake.Managed{ + Orphanable: fake.Orphanable{ + Policy: xpv1.DeletionOrphan, + }, + Manageable: fake.Manageable{ + Policy: xpv1.ManagementPolicies{xpv1.ManagementActionDelete}, + }, + }, + }, + want: want{delete: true}, + }, + "DeletionDeleteManagementActionNoDelete": { + reason: "Should orphan if management policies are enabled and deletion policy is set to Delete and management policy does not have action Delete.", + args: args{ + managementPoliciesEnabled: true, + managed: &fake.Managed{ + Orphanable: fake.Orphanable{ + Policy: xpv1.DeletionDelete, + }, + Manageable: fake.Manageable{ + Policy: xpv1.ManagementPolicies{xpv1.ManagementActionObserve}, + }, + }, + }, + want: want{delete: false}, + }, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + r := NewManagementPoliciesResolver(tc.args.managementPoliciesEnabled, tc.args.managed.GetManagementPolicies(), tc.args.managed.GetDeletionPolicy()) + if diff := cmp.Diff(tc.want.delete, r.ShouldDelete()); diff != "" { + t.Errorf("\nReason: %s\nShouldDelete(...): -want, +got:\n%s", tc.reason, diff) + } + }) + } +} diff --git a/pkg/reconciler/managed/publisher.go b/pkg/reconciler/managed/publisher.go index 07db0b841..417a3b379 100644 --- a/pkg/reconciler/managed/publisher.go +++ b/pkg/reconciler/managed/publisher.go @@ -23,8 +23,6 @@ import ( "github.com/crossplane/crossplane-runtime/pkg/resource" ) -const errSecretStoreDisabled = "cannot publish to secret store, feature is not enabled" - // A PublisherChain chains multiple ManagedPublishers. type PublisherChain []ConnectionPublisher @@ -64,7 +62,7 @@ type DisabledSecretStoreManager struct { // not enabled. func (m *DisabledSecretStoreManager) PublishConnection(_ context.Context, so resource.ConnectionSecretOwner, _ ConnectionDetails) (bool, error) { if so.GetPublishConnectionDetailsTo() != nil { - return false, errors.New(errSecretStoreDisabled) + return false, errors.New("cannot publish to secret store, feature is not enabled") } return false, nil } @@ -73,7 +71,7 @@ func (m *DisabledSecretStoreManager) PublishConnection(_ context.Context, so res // not enabled. func (m *DisabledSecretStoreManager) UnpublishConnection(_ context.Context, so resource.ConnectionSecretOwner, _ ConnectionDetails) error { if so.GetPublishConnectionDetailsTo() != nil { - return errors.New(errSecretStoreDisabled) + return errors.New("cannot publish to secret store, feature is not enabled") } return nil } diff --git a/pkg/reconciler/managed/publisher_test.go b/pkg/reconciler/managed/publisher_test.go index 928b8fde2..3dc269356 100644 --- a/pkg/reconciler/managed/publisher_test.go +++ b/pkg/reconciler/managed/publisher_test.go @@ -21,12 +21,12 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" xpv1 "github.com/crossplane/crossplane-runtime/apis/common/v1" "github.com/crossplane/crossplane-runtime/pkg/errors" "github.com/crossplane/crossplane-runtime/pkg/resource" "github.com/crossplane/crossplane-runtime/pkg/resource/fake" - "github.com/crossplane/crossplane-runtime/pkg/test" ) var ( @@ -106,7 +106,7 @@ func TestPublisherChain(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { got, gotErr := tc.p.PublishConnection(tc.args.ctx, tc.args.mg, tc.args.c) - if diff := cmp.Diff(tc.want.err, gotErr, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want.err, gotErr, cmpopts.EquateErrors()); diff != "" { t.Errorf("Publish(...): -want, +got:\n%s", diff) } if diff := cmp.Diff(tc.want.published, got); diff != "" { @@ -143,7 +143,7 @@ func TestDisabledSecretStorePublish(t *testing.T) { }, }, want: want{ - err: errors.New(errSecretStoreDisabled), + err: cmpopts.AnyError, }, }, } @@ -152,7 +152,7 @@ func TestDisabledSecretStorePublish(t *testing.T) { t.Run(name, func(t *testing.T) { ss := &DisabledSecretStoreManager{} got, gotErr := ss.PublishConnection(context.Background(), tc.args.mg, nil) - if diff := cmp.Diff(tc.want.err, gotErr, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want.err, gotErr, cmpopts.EquateErrors()); diff != "" { t.Errorf("Publish(...): -want, +got:\n%s", diff) } if diff := cmp.Diff(tc.want.published, got); diff != "" { @@ -188,7 +188,7 @@ func TestDisabledSecretStoreUnpublish(t *testing.T) { }, }, want: want{ - err: errors.New(errSecretStoreDisabled), + err: cmpopts.AnyError, }, }, } @@ -197,7 +197,7 @@ func TestDisabledSecretStoreUnpublish(t *testing.T) { t.Run(name, func(t *testing.T) { ss := &DisabledSecretStoreManager{} gotErr := ss.UnpublishConnection(context.Background(), tc.args.mg, nil) - if diff := cmp.Diff(tc.want.err, gotErr, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want.err, gotErr, cmpopts.EquateErrors()); diff != "" { t.Errorf("Publish(...): -want, +got:\n%s", diff) } }) diff --git a/pkg/reconciler/managed/reconciler.go b/pkg/reconciler/managed/reconciler.go index 3723b5ceb..3d2648540 100644 --- a/pkg/reconciler/managed/reconciler.go +++ b/pkg/reconciler/managed/reconciler.go @@ -49,23 +49,6 @@ const ( defaultGracePeriod = 30 * time.Second ) -// Error strings. -const ( - errFmtManagementPolicyNonDefault = "`spec.managementPolicies` is set to a non-default value but the feature is not enabled: %s" - errFmtManagementPolicyNotSupported = "`spec.managementPolicies` is set to a value(%s) which is not supported. Check docs for supported policies" - - errGetManaged = "cannot get managed resource" - errUpdateManagedAnnotations = "cannot update managed resource annotations" - errCreateIncomplete = "cannot determine creation result - remove the " + meta.AnnotationKeyExternalCreatePending + " annotation if it is safe to proceed" - errReconcileConnect = "connect failed" - errReconcileObserve = "observe failed" - errReconcileCreate = "create failed" - errReconcileUpdate = "update failed" - errReconcileDelete = "delete failed" - - errExternalResourceNotExist = "external resource does not exist" -) - // Event reasons. const ( reasonCannotConnect event.Reason = "CannotConnectToProvider" @@ -708,7 +691,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu // There's no need to requeue if we no longer exist. Otherwise we'll be // requeued implicitly because we return an error. log.Debug("Cannot get managed resource", "error", err) - return reconcile.Result{}, errors.Wrap(resource.IgnoreNotFound(err), errGetManaged) + return reconcile.Result{}, errors.Wrap(resource.IgnoreNotFound(err), "cannot get managed resource") } record := r.record.WithAnnotations("external-name", meta.GetExternalName(managed)) @@ -738,7 +721,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu managed.SetConditions(xpv1.ReconcilePaused()) // if the pause annotation is removed or the management policies changed, we will have a chance to reconcile // again and resume and if status update fails, we will reconcile again to retry to update the status - return reconcile.Result{}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) + return reconcile.Result{}, errors.Wrap(r.client.Status().Update(ctx, managed), "cannot update managed resource status") } // Check if the ManagementPolicies is set to a non-default value while the @@ -753,7 +736,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu log.Debug(err.Error()) record.Event(managed, event.Warning(reasonManagementPolicyInvalid, err)) managed.SetConditions(xpv1.ReconcileError(err)) - return reconcile.Result{}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) + return reconcile.Result{}, errors.Wrap(r.client.Status().Update(ctx, managed), "cannot update managed resource status") } // If managed resource has a deletion timestamp and a deletion policy of @@ -775,7 +758,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu log.Debug("Cannot unpublish connection details", "error", err) record.Event(managed, event.Warning(reasonCannotUnpublish, err)) managed.SetConditions(xpv1.Deleting(), xpv1.ReconcileError(err)) - return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), "cannot update managed resource status") } if err := r.managed.RemoveFinalizer(ctx, managed); err != nil { // If this is the first time we encounter this issue we'll be @@ -784,7 +767,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu // backoff. log.Debug("Cannot remove managed resource finalizer", "error", err) managed.SetConditions(xpv1.Deleting(), xpv1.ReconcileError(err)) - return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), "cannot update managed resource status") } // We've successfully unpublished our managed resource's connection @@ -802,7 +785,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu log.Debug("Cannot initialize managed resource", "error", err) record.Event(managed, event.Warning(reasonCannotInitialize, err)) managed.SetConditions(xpv1.ReconcileError(err)) - return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), "cannot update managed resource status") } // If we started but never completed creation of an external resource we @@ -810,10 +793,11 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu // an updated external name we've leaked a resource. The safest thing to // do is to refuse to proceed. if meta.ExternalCreateIncomplete(managed) { - log.Debug(errCreateIncomplete) - record.Event(managed, event.Warning(reasonCannotInitialize, errors.New(errCreateIncomplete))) - managed.SetConditions(xpv1.Creating(), xpv1.ReconcileError(errors.New(errCreateIncomplete))) - return reconcile.Result{Requeue: false}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) + msg := "cannot determine creation result - remove the " + meta.AnnotationKeyExternalCreatePending + " annotation if it is safe to proceed" + log.Debug(msg) + record.Event(managed, event.Warning(reasonCannotInitialize, errors.New(msg))) + managed.SetConditions(xpv1.Creating(), xpv1.ReconcileError(errors.New(msg))) + return reconcile.Result{Requeue: false}, errors.Wrap(r.client.Status().Update(ctx, managed), "cannot update managed resource status") } // We resolve any references before observing our external resource because @@ -835,7 +819,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu log.Debug("Cannot resolve managed resource references", "error", err) record.Event(managed, event.Warning(reasonCannotResolveRefs, err)) managed.SetConditions(xpv1.ReconcileError(err)) - return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), "cannot update managed resource status") } } @@ -848,8 +832,8 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu // backoff. log.Debug("Cannot connect to provider", "error", err) record.Event(managed, event.Warning(reasonCannotConnect, err)) - managed.SetConditions(xpv1.ReconcileError(errors.Wrap(err, errReconcileConnect))) - return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) + managed.SetConditions(xpv1.ReconcileError(errors.Wrap(err, "connect failed"))) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), "cannot update managed resource status") } defer func() { if err := r.external.Disconnect(ctx); err != nil { @@ -868,16 +852,16 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu // trigger backoff. log.Debug("Cannot observe external resource", "error", err) record.Event(managed, event.Warning(reasonCannotObserve, err)) - managed.SetConditions(xpv1.ReconcileError(errors.Wrap(err, errReconcileObserve))) - return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) + managed.SetConditions(xpv1.ReconcileError(errors.Wrap(err, "observe failed"))) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), "cannot update managed resource status") } // In the observe-only mode, !observation.ResourceExists will be an error // case, and we will explicitly return this information to the user. if !observation.ResourceExists && policy.ShouldOnlyObserve() { - record.Event(managed, event.Warning(reasonCannotObserve, errors.New(errExternalResourceNotExist))) - managed.SetConditions(xpv1.ReconcileError(errors.Wrap(errors.New(errExternalResourceNotExist), errReconcileObserve))) - return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) + record.Event(managed, event.Warning(reasonCannotObserve, errors.New("external resource does not exist"))) + managed.SetConditions(xpv1.ReconcileError(errors.New("observe failed: external resource does not exist"))) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), "cannot update managed resource status") } // If this resource has a non-zero creation grace period we want to wait @@ -904,8 +888,8 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu // explicitly, which will trigger backoff. log.Debug("Cannot delete external resource", "error", err) record.Event(managed, event.Warning(reasonCannotDelete, err)) - managed.SetConditions(xpv1.Deleting(), xpv1.ReconcileError(errors.Wrap(err, errReconcileDelete))) - return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) + managed.SetConditions(xpv1.Deleting(), xpv1.ReconcileError(errors.Wrap(err, "delete failed"))) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), "cannot update managed resource status") } // We've successfully requested deletion of our external resource. @@ -918,7 +902,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu log.Debug("Successfully requested deletion of external resource") record.Event(managed, event.Normal(reasonDeleted, "Successfully requested deletion of external resource")) managed.SetConditions(xpv1.Deleting(), xpv1.ReconcileSuccess()) - return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), "cannot update managed resource status") } if err := r.managed.UnpublishConnection(ctx, managed, observation.ConnectionDetails); err != nil { // If this is the first time we encounter this issue we'll be @@ -928,7 +912,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu log.Debug("Cannot unpublish connection details", "error", err) record.Event(managed, event.Warning(reasonCannotUnpublish, err)) managed.SetConditions(xpv1.Deleting(), xpv1.ReconcileError(err)) - return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), "cannot update managed resource status") } if err := r.managed.RemoveFinalizer(ctx, managed); err != nil { // If this is the first time we encounter this issue we'll be @@ -937,7 +921,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu // backoff. log.Debug("Cannot remove managed resource finalizer", "error", err) managed.SetConditions(xpv1.Deleting(), xpv1.ReconcileError(err)) - return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), "cannot update managed resource status") } // We've successfully deleted our external resource (if necessary) and @@ -955,7 +939,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu log.Debug("Cannot publish connection details", "error", err) record.Event(managed, event.Warning(reasonCannotPublish, err)) managed.SetConditions(xpv1.ReconcileError(err)) - return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), "cannot update managed resource status") } if err := r.managed.AddFinalizer(ctx, managed); err != nil { @@ -964,7 +948,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu // not, we requeue explicitly, which will trigger backoff. log.Debug("Cannot add finalizer", "error", err) managed.SetConditions(xpv1.ReconcileError(err)) - return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), "cannot update managed resource status") } if !observation.ResourceExists && policy.ShouldCreate() { @@ -977,10 +961,10 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu // update to fail if we get a 409 due to a stale version. meta.SetExternalCreatePending(managed, time.Now()) if err := r.client.Update(ctx, managed); err != nil { - log.Debug(errUpdateManaged, "error", err) - record.Event(managed, event.Warning(reasonCannotUpdateManaged, errors.Wrap(err, errUpdateManaged))) - managed.SetConditions(xpv1.Creating(), xpv1.ReconcileError(errors.Wrap(err, errUpdateManaged))) - return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) + log.Debug("cannot update managed resource", "error", err) + record.Event(managed, event.Warning(reasonCannotUpdateManaged, errors.Wrap(err, "cannot update managed resource"))) + managed.SetConditions(xpv1.Creating(), xpv1.ReconcileError(errors.Wrap(err, "cannot update managed resource"))) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), "cannot update managed resource status") } creation, err := external.Create(externalCtx, managed) @@ -1001,8 +985,8 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu // resource. meta.SetExternalCreateFailed(managed, time.Now()) if err := r.managed.UpdateCriticalAnnotations(ctx, managed); err != nil { - log.Debug(errUpdateManagedAnnotations, "error", err) - record.Event(managed, event.Warning(reasonCannotUpdateManaged, errors.Wrap(err, errUpdateManagedAnnotations))) + log.Debug("cannot update managed resource annotations", "error", err) + record.Event(managed, event.Warning(reasonCannotUpdateManaged, errors.Wrap(err, "cannot update managed resource annotations"))) // We only log and emit an event here rather // than setting a status condition and returning @@ -1011,8 +995,8 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu // create failed. } - managed.SetConditions(xpv1.Creating(), xpv1.ReconcileError(errors.Wrap(err, errReconcileCreate))) - return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) + managed.SetConditions(xpv1.Creating(), xpv1.ReconcileError(errors.Wrap(err, "create failed"))) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), "cannot update managed resource status") } // In some cases our external-name may be set by Create above. @@ -1031,10 +1015,10 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu // we may revisit this in future. meta.SetExternalCreateSucceeded(managed, time.Now()) if err := r.managed.UpdateCriticalAnnotations(ctx, managed); err != nil { - log.Debug(errUpdateManagedAnnotations, "error", err) - record.Event(managed, event.Warning(reasonCannotUpdateManaged, errors.Wrap(err, errUpdateManagedAnnotations))) - managed.SetConditions(xpv1.Creating(), xpv1.ReconcileError(errors.Wrap(err, errUpdateManagedAnnotations))) - return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) + log.Debug("cannot update managed resource annotations", "error", err) + record.Event(managed, event.Warning(reasonCannotUpdateManaged, errors.Wrap(err, "cannot update managed resource annotations"))) + managed.SetConditions(xpv1.Creating(), xpv1.ReconcileError(errors.Wrap(err, "cannot update managed resource annotations"))) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), "cannot update managed resource status") } if _, err := r.managed.PublishConnection(ctx, managed, creation.ConnectionDetails); err != nil { @@ -1044,7 +1028,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu log.Debug("Cannot publish connection details", "error", err) record.Event(managed, event.Warning(reasonCannotPublish, err)) managed.SetConditions(xpv1.Creating(), xpv1.ReconcileError(err)) - return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), "cannot update managed resource status") } // We've successfully created our external resource. In many cases the @@ -1054,7 +1038,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu log.Debug("Successfully requested creation of external resource") record.Event(managed, event.Normal(reasonCreated, "Successfully requested creation of external resource")) managed.SetConditions(xpv1.Creating(), xpv1.ReconcileSuccess()) - return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), "cannot update managed resource status") } if observation.ResourceLateInitialized && policy.ShouldLateInitialize() { @@ -1066,10 +1050,10 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu // an immediate reconcile which should re-observe the external resource // and persist its status. if err := r.client.Update(ctx, managed); err != nil { - log.Debug(errUpdateManaged, "error", err) + log.Debug("cannot update managed resource", "error", err) record.Event(managed, event.Warning(reasonCannotUpdateManaged, err)) - managed.SetConditions(xpv1.ReconcileError(errors.Wrap(err, errUpdateManaged))) - return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) + managed.SetConditions(xpv1.ReconcileError(errors.Wrap(err, "cannot update managed resource"))) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), "cannot update managed resource status") } } @@ -1082,7 +1066,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu // https://github.com/crossplane/crossplane/issues/289 log.Debug("External resource is up to date", "requeue-after", time.Now().Add(r.pollInterval)) managed.SetConditions(xpv1.ReconcileSuccess()) - return reconcile.Result{RequeueAfter: r.pollInterval}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) + return reconcile.Result{RequeueAfter: r.pollInterval}, errors.Wrap(r.client.Status().Update(ctx, managed), "cannot update managed resource status") } if observation.Diff != "" { @@ -1093,7 +1077,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu if !policy.ShouldUpdate() { log.Debug("Skipping update due to managementPolicies. Reconciliation succeeded", "requeue-after", time.Now().Add(r.pollInterval)) managed.SetConditions(xpv1.ReconcileSuccess()) - return reconcile.Result{RequeueAfter: r.pollInterval}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) + return reconcile.Result{RequeueAfter: r.pollInterval}, errors.Wrap(r.client.Status().Update(ctx, managed), "cannot update managed resource status") } update, err := external.Update(externalCtx, managed) @@ -1105,8 +1089,8 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu // condition. If not, we requeue explicitly, which will trigger backoff. log.Debug("Cannot update external resource") record.Event(managed, event.Warning(reasonCannotUpdate, err)) - managed.SetConditions(xpv1.ReconcileError(errors.Wrap(err, errReconcileUpdate))) - return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) + managed.SetConditions(xpv1.ReconcileError(errors.Wrap(err, "update failed"))) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), "cannot update managed resource status") } if _, err := r.managed.PublishConnection(ctx, managed, update.ConnectionDetails); err != nil { @@ -1116,7 +1100,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu log.Debug("Cannot publish connection details", "error", err) record.Event(managed, event.Warning(reasonCannotPublish, err)) managed.SetConditions(xpv1.ReconcileError(err)) - return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), "cannot update managed resource status") } // We've successfully updated our external resource. Per the below issue @@ -1127,5 +1111,5 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu log.Debug("Successfully requested update of external resource", "requeue-after", time.Now().Add(r.pollInterval)) record.Event(managed, event.Normal(reasonUpdated, "Successfully requested update of external resource")) managed.SetConditions(xpv1.ReconcileSuccess()) - return reconcile.Result{RequeueAfter: r.pollInterval}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) + return reconcile.Result{RequeueAfter: r.pollInterval}, errors.Wrap(r.client.Status().Update(ctx, managed), "cannot update managed resource status") } diff --git a/pkg/reconciler/managed/reconciler_test.go b/pkg/reconciler/managed/reconciler_test.go index 9e4930a71..f6f865253 100644 --- a/pkg/reconciler/managed/reconciler_test.go +++ b/pkg/reconciler/managed/reconciler_test.go @@ -71,7 +71,9 @@ func TestReconciler(t *testing.T) { }, mg: resource.ManagedKind(fake.GVK(&fake.Managed{})), }, - want: want{err: errors.Wrap(errBoom, errGetManaged)}, + want: want{ + err: errBoom, + }, }, "ManagedNotFound": { reason: "Not found errors encountered while getting the resource under reconciliation should be ignored.", @@ -214,7 +216,7 @@ func TestReconciler(t *testing.T) { MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := &fake.Managed{} meta.SetExternalCreatePending(want, now.Time) - want.SetConditions(xpv1.Creating(), xpv1.ReconcileError(errors.New(errCreateIncomplete))) + want.SetConditions(xpv1.Creating(), xpv1.ReconcileError(errors.New("cannot determine creation result - remove the "+meta.AnnotationKeyExternalCreatePending+" annotation if it is safe to proceed"))) if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" { reason := "We should update our status when we're asked to reconcile a managed resource that is pending creation." t.Errorf("\nReason: %s\n-want, +got:\n%s", reason, diff) @@ -267,7 +269,7 @@ func TestReconciler(t *testing.T) { MockGet: test.NewMockGetFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, got client.Object, _ ...client.SubResourceUpdateOption) error { want := &fake.Managed{} - want.SetConditions(xpv1.ReconcileError(errors.Wrap(errBoom, errReconcileConnect))) + want.SetConditions(xpv1.ReconcileError(errors.Wrap(errBoom, "connect failed"))) if diff := cmp.Diff(want, got, test.EquateConditions()); diff != "" { reason := "Errors connecting to the provider should be reported as a conditioned status." t.Errorf("\nReason: %s\n-want, +got:\n%s", reason, diff) @@ -334,7 +336,7 @@ func TestReconciler(t *testing.T) { MockGet: test.NewMockGetFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := &fake.Managed{} - want.SetConditions(xpv1.ReconcileError(errors.Wrap(errBoom, errReconcileObserve))) + want.SetConditions(xpv1.ReconcileError(errors.Wrap(errBoom, "observe failed"))) if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" { reason := "Errors observing the managed resource should be reported as a conditioned status." t.Errorf("\nReason: %s\n-want, +got:\n%s", reason, diff) @@ -402,7 +404,7 @@ func TestReconciler(t *testing.T) { want := &fake.Managed{} want.SetDeletionTimestamp(&now) want.SetDeletionPolicy(xpv1.DeletionDelete) - want.SetConditions(xpv1.ReconcileError(errors.Wrap(errBoom, errReconcileDelete))) + want.SetConditions(xpv1.ReconcileError(errors.Wrap(errBoom, "delete failed"))) want.SetConditions(xpv1.Deleting()) if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" { reason := "An error deleting an external resource should be reported as a conditioned status." @@ -669,7 +671,7 @@ func TestReconciler(t *testing.T) { MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := &fake.Managed{} meta.SetExternalCreatePending(want, time.Now()) - want.SetConditions(xpv1.Creating(), xpv1.ReconcileError(errors.Wrap(errBoom, errUpdateManaged))) + want.SetConditions(xpv1.Creating(), xpv1.ReconcileError(errors.Wrap(errBoom, "cannot update managed resource"))) if diff := cmp.Diff(want, obj, test.EquateConditions(), cmpopts.EquateApproxTime(1*time.Second)); diff != "" { reason := "Errors while creating an external resource should be reported as a conditioned status." t.Errorf("\nReason: %s\n-want, +got:\n%s", reason, diff) @@ -711,7 +713,7 @@ func TestReconciler(t *testing.T) { want := &fake.Managed{} meta.SetExternalCreatePending(want, time.Now()) meta.SetExternalCreateFailed(want, time.Now()) - want.SetConditions(xpv1.ReconcileError(errors.Wrap(errBoom, errReconcileCreate))) + want.SetConditions(xpv1.ReconcileError(errors.Wrap(errBoom, "create failed"))) want.SetConditions(xpv1.Creating()) if diff := cmp.Diff(want, obj, test.EquateConditions(), cmpopts.EquateApproxTime(1*time.Second)); diff != "" { reason := "Errors while creating an external resource should be reported as a conditioned status." @@ -757,7 +759,7 @@ func TestReconciler(t *testing.T) { want := &fake.Managed{} meta.SetExternalCreatePending(want, time.Now()) meta.SetExternalCreateSucceeded(want, time.Now()) - want.SetConditions(xpv1.ReconcileError(errors.Wrap(errBoom, errUpdateManagedAnnotations))) + want.SetConditions(xpv1.ReconcileError(errors.Wrap(errBoom, "cannot update managed resource annotations"))) want.SetConditions(xpv1.Creating()) if diff := cmp.Diff(want, obj, test.EquateConditions(), cmpopts.EquateApproxTime(1*time.Second)); diff != "" { reason := "Errors updating critical annotations after creation should be reported as a conditioned status." @@ -887,7 +889,7 @@ func TestReconciler(t *testing.T) { MockUpdate: test.NewMockUpdateFn(errBoom), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := &fake.Managed{} - want.SetConditions(xpv1.ReconcileError(errors.Wrap(errBoom, errUpdateManaged))) + want.SetConditions(xpv1.ReconcileError(errors.Wrap(errBoom, "cannot update managed resource"))) if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" { reason := "Errors updating a managed resource should be reported as a conditioned status." t.Errorf("\nReason: %s\n-want, +got:\n%s", reason, diff) @@ -959,7 +961,7 @@ func TestReconciler(t *testing.T) { MockGet: test.NewMockGetFn(nil), MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := &fake.Managed{} - want.SetConditions(xpv1.ReconcileError(errors.Wrap(errBoom, errReconcileUpdate))) + want.SetConditions(xpv1.ReconcileError(errors.Wrap(errBoom, "update failed"))) if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" { reason := "Errors while updating an external resource should be reported as a conditioned status." t.Errorf("\nReason: %s\n-want, +got:\n%s", reason, diff) @@ -1202,7 +1204,9 @@ func TestReconciler(t *testing.T) { }, mg: resource.ManagedKind(fake.GVK(&fake.Managed{})), }, - want: want{err: errors.Wrap(errBoom, errUpdateManagedStatus)}, + want: want{ + err: errBoom, + }, }, "ManagementPoliciesUsedButNotEnabled": { reason: `If management policies tried to be used without enabling the feature, we should throw an error.`, @@ -1217,7 +1221,7 @@ func TestReconciler(t *testing.T) { MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := &fake.Managed{} want.SetManagementPolicies(xpv1.ManagementPolicies{xpv1.ManagementActionCreate}) - want.SetConditions(xpv1.ReconcileError(fmt.Errorf(errFmtManagementPolicyNonDefault, xpv1.ManagementPolicies{xpv1.ManagementActionCreate}))) + want.SetConditions(xpv1.ReconcileError(fmt.Errorf("`spec.managementPolicies` is set to a non-default value but the feature is not enabled: %s", xpv1.ManagementPolicies{xpv1.ManagementActionCreate}))) if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" { reason := `If managed resource has a non default management policy but feature not enabled, it should return a proper error.` t.Errorf("\nReason: %s\n-want, +got:\n%s", reason, diff) @@ -1244,7 +1248,7 @@ func TestReconciler(t *testing.T) { MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := &fake.Managed{} want.SetManagementPolicies(xpv1.ManagementPolicies{xpv1.ManagementActionCreate}) - want.SetConditions(xpv1.ReconcileError(fmt.Errorf(errFmtManagementPolicyNotSupported, xpv1.ManagementPolicies{xpv1.ManagementActionCreate}))) + want.SetConditions(xpv1.ReconcileError(fmt.Errorf("`spec.managementPolicies` is set to a value(%s) which is not supported. Check docs for supported policies", xpv1.ManagementPolicies{xpv1.ManagementActionCreate}))) if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" { reason := `If managed resource has non supported management policy, it should return a proper error.` t.Errorf("\nReason: %s\n-want, +got:\n%s", reason, diff) @@ -1274,7 +1278,7 @@ func TestReconciler(t *testing.T) { MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := &fake.Managed{} want.SetManagementPolicies(xpv1.ManagementPolicies{xpv1.ManagementActionAll}) - want.SetConditions(xpv1.ReconcileError(fmt.Errorf(errFmtManagementPolicyNotSupported, xpv1.ManagementPolicies{xpv1.ManagementActionAll}))) + want.SetConditions(xpv1.ReconcileError(fmt.Errorf("`spec.managementPolicies` is set to a value(%s) which is not supported. Check docs for supported policies", xpv1.ManagementPolicies{xpv1.ManagementActionAll}))) if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" { reason := `If managed resource has non supported management policy, it should return a proper error.` t.Errorf("\nReason: %s\n-want, +got:\n%s", reason, diff) @@ -1305,7 +1309,7 @@ func TestReconciler(t *testing.T) { MockStatusUpdate: test.MockSubResourceUpdateFn(func(_ context.Context, obj client.Object, _ ...client.SubResourceUpdateOption) error { want := &fake.Managed{} want.SetManagementPolicies(xpv1.ManagementPolicies{xpv1.ManagementActionObserve}) - want.SetConditions(xpv1.ReconcileError(errors.Wrap(errors.New(errExternalResourceNotExist), errReconcileObserve))) + want.SetConditions(xpv1.ReconcileError(errors.New("observe failed: external resource does not exist"))) if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" { reason := "Resource does not exist should be reported as a conditioned status when ObserveOnly." t.Errorf("\nReason: %s\n-want, +got:\n%s", reason, diff) @@ -1686,7 +1690,7 @@ func TestReconciler(t *testing.T) { r := NewReconciler(tc.args.m, tc.args.mg, tc.args.o...) got, err := r.Reconcile(context.Background(), reconcile.Request{}) - if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want.err, err, cmpopts.EquateErrors()); diff != "" { t.Errorf("\nReason: %s\nr.Reconcile(...): -want error, +got error:\n%s", tc.reason, diff) } @@ -1696,432 +1700,3 @@ func TestReconciler(t *testing.T) { }) } } - -func TestTestManagementPoliciesResolverIsPaused(t *testing.T) { - type args struct { - enabled bool - policy xpv1.ManagementPolicies - } - cases := map[string]struct { - reason string - args args - want bool - }{ - "Disabled": { - reason: "Should return false if management policies are disabled", - args: args{ - enabled: false, - policy: xpv1.ManagementPolicies{}, - }, - want: false, - }, - "EnabledEmptyPolicies": { - reason: "Should return true if the management policies are enabled and empty", - args: args{ - enabled: true, - policy: xpv1.ManagementPolicies{}, - }, - want: true, - }, - "EnabledNonEmptyPolicies": { - reason: "Should return true if the management policies are enabled and non empty", - args: args{ - enabled: true, - policy: xpv1.ManagementPolicies{xpv1.ManagementActionAll}, - }, - want: false, - }, - } - for name, tc := range cases { - t.Run(name, func(t *testing.T) { - r := NewManagementPoliciesResolver(tc.args.enabled, tc.args.policy, xpv1.DeletionDelete) - if diff := cmp.Diff(tc.want, r.IsPaused()); diff != "" { - t.Errorf("\nReason: %s\nIsPaused(...): -want, +got:\n%s", tc.reason, diff) - } - }) - } -} - -func TestManagementPoliciesResolverValidate(t *testing.T) { - type args struct { - enabled bool - policy xpv1.ManagementPolicies - } - cases := map[string]struct { - reason string - args args - want error - }{ - "Enabled": { - reason: "Should return nil if the management policy is enabled.", - args: args{ - enabled: true, - policy: xpv1.ManagementPolicies{}, - }, - want: nil, - }, - "DisabledNonDefault": { - reason: "Should return error if the management policy is non-default and disabled.", - args: args{ - enabled: false, - policy: xpv1.ManagementPolicies{xpv1.ManagementActionCreate}, - }, - want: fmt.Errorf(errFmtManagementPolicyNonDefault, []xpv1.ManagementAction{xpv1.ManagementActionCreate}), - }, - "DisabledDefault": { - reason: "Should return nil if the management policy is default and disabled.", - args: args{ - enabled: false, - policy: xpv1.ManagementPolicies{xpv1.ManagementActionAll}, - }, - want: nil, - }, - "EnabledSupported": { - reason: "Should return nil if the management policy is supported.", - args: args{ - enabled: true, - policy: xpv1.ManagementPolicies{xpv1.ManagementActionAll}, - }, - want: nil, - }, - "EnabledNotSupported": { - reason: "Should return err if the management policy is not supported.", - args: args{ - enabled: true, - policy: xpv1.ManagementPolicies{xpv1.ManagementActionDelete}, - }, - want: fmt.Errorf(errFmtManagementPolicyNotSupported, []xpv1.ManagementAction{xpv1.ManagementActionDelete}), - }, - } - for name, tc := range cases { - t.Run(name, func(t *testing.T) { - r := NewManagementPoliciesResolver(tc.args.enabled, tc.args.policy, xpv1.DeletionDelete) - if diff := cmp.Diff(tc.want, r.Validate(), test.EquateErrors()); diff != "" { - t.Errorf("\nReason: %s\nIsNonDefault(...): -want, +got:\n%s", tc.reason, diff) - } - }) - } -} - -func TestManagementPoliciesResolverShouldCreate(t *testing.T) { - type args struct { - managementPoliciesEnabled bool - policy xpv1.ManagementPolicies - } - cases := map[string]struct { - reason string - args args - want bool - }{ - "ManagementPoliciesDisabled": { - reason: "Should return true if management policies are disabled", - args: args{ - managementPoliciesEnabled: false, - }, - want: true, - }, - "ManagementPoliciesEnabledHasCreate": { - reason: "Should return true if management policies are enabled and managementPolicies has action Create", - args: args{ - managementPoliciesEnabled: true, - policy: xpv1.ManagementPolicies{xpv1.ManagementActionCreate}, - }, - want: true, - }, - "ManagementPoliciesEnabledHasCreateAll": { - reason: "Should return true if management policies are enabled and managementPolicies has action All", - args: args{ - managementPoliciesEnabled: true, - policy: xpv1.ManagementPolicies{xpv1.ManagementActionAll}, - }, - want: true, - }, - "ManagementPoliciesEnabledActionNotAllowed": { - reason: "Should return false if management policies are enabled and managementPolicies does not have Create", - args: args{ - managementPoliciesEnabled: true, - policy: xpv1.ManagementPolicies{xpv1.ManagementActionObserve}, - }, - want: false, - }, - } - for name, tc := range cases { - t.Run(name, func(t *testing.T) { - r := NewManagementPoliciesResolver(tc.args.managementPoliciesEnabled, tc.args.policy, xpv1.DeletionOrphan) - if diff := cmp.Diff(tc.want, r.ShouldCreate()); diff != "" { - t.Errorf("\nReason: %s\nShouldCreate(...): -want, +got:\n%s", tc.reason, diff) - } - }) - } -} - -func TestManagementPoliciesResolverShouldUpdate(t *testing.T) { - type args struct { - managementPoliciesEnabled bool - policy xpv1.ManagementPolicies - } - cases := map[string]struct { - reason string - args args - want bool - }{ - "ManagementPoliciesDisabled": { - reason: "Should return true if management policies are disabled", - args: args{ - managementPoliciesEnabled: false, - }, - want: true, - }, - "ManagementPoliciesEnabledHasUpdate": { - reason: "Should return true if management policies are enabled and managementPolicies has action Update", - args: args{ - managementPoliciesEnabled: true, - policy: xpv1.ManagementPolicies{xpv1.ManagementActionUpdate}, - }, - want: true, - }, - "ManagementPoliciesEnabledHasUpdateAll": { - reason: "Should return true if management policies are enabled and managementPolicies has action All", - args: args{ - managementPoliciesEnabled: true, - policy: xpv1.ManagementPolicies{xpv1.ManagementActionAll}, - }, - want: true, - }, - "ManagementPoliciesEnabledActionNotAllowed": { - reason: "Should return false if management policies are enabled and managementPolicies does not have Update", - args: args{ - managementPoliciesEnabled: true, - policy: xpv1.ManagementPolicies{xpv1.ManagementActionObserve}, - }, - want: false, - }, - } - for name, tc := range cases { - t.Run(name, func(t *testing.T) { - r := NewManagementPoliciesResolver(tc.args.managementPoliciesEnabled, tc.args.policy, xpv1.DeletionOrphan) - if diff := cmp.Diff(tc.want, r.ShouldUpdate()); diff != "" { - t.Errorf("\nReason: %s\nShouldUpdate(...): -want, +got:\n%s", tc.reason, diff) - } - }) - } -} - -func TestManagementPoliciesResolverShouldLateInitialize(t *testing.T) { - type args struct { - managementPoliciesEnabled bool - policy xpv1.ManagementPolicies - } - cases := map[string]struct { - reason string - args args - want bool - }{ - "ManagementPoliciesDisabled": { - reason: "Should return true if management policies are disabled", - args: args{ - managementPoliciesEnabled: false, - }, - want: true, - }, - "ManagementPoliciesEnabledHasLateInitialize": { - reason: "Should return true if management policies are enabled and managementPolicies has action LateInitialize", - args: args{ - managementPoliciesEnabled: true, - policy: xpv1.ManagementPolicies{xpv1.ManagementActionLateInitialize}, - }, - want: true, - }, - "ManagementPoliciesEnabledHasLateInitializeAll": { - reason: "Should return true if management policies are enabled and managementPolicies has action All", - args: args{ - managementPoliciesEnabled: true, - policy: xpv1.ManagementPolicies{xpv1.ManagementActionAll}, - }, - want: true, - }, - "ManagementPoliciesEnabledActionNotAllowed": { - reason: "Should return false if management policies are enabled and managementPolicies does not have LateInitialize", - args: args{ - managementPoliciesEnabled: true, - policy: xpv1.ManagementPolicies{xpv1.ManagementActionObserve}, - }, - want: false, - }, - } - for name, tc := range cases { - t.Run(name, func(t *testing.T) { - r := NewManagementPoliciesResolver(tc.args.managementPoliciesEnabled, tc.args.policy, xpv1.DeletionOrphan) - if diff := cmp.Diff(tc.want, r.ShouldLateInitialize()); diff != "" { - t.Errorf("\nReason: %s\nShouldLateInitialize(...): -want, +got:\n%s", tc.reason, diff) - } - }) - } -} - -func TestManagementPoliciesResolverOnlyObserve(t *testing.T) { - type args struct { - managementPoliciesEnabled bool - policy xpv1.ManagementPolicies - } - cases := map[string]struct { - reason string - args args - want bool - }{ - "ManagementPoliciesDisabled": { - reason: "Should return false if management policies are disabled", - args: args{ - managementPoliciesEnabled: false, - }, - want: false, - }, - "ManagementPoliciesEnabledHasOnlyObserve": { - reason: "Should return true if management policies are enabled and managementPolicies has action LateInitialize", - args: args{ - managementPoliciesEnabled: true, - policy: xpv1.ManagementPolicies{xpv1.ManagementActionObserve}, - }, - want: true, - }, - "ManagementPoliciesEnabledHasMultipleActions": { - reason: "Should return false if management policies are enabled and managementPolicies has multiple actions", - args: args{ - managementPoliciesEnabled: true, - policy: xpv1.ManagementPolicies{xpv1.ManagementActionLateInitialize, xpv1.ManagementActionObserve}, - }, - want: false, - }, - } - for name, tc := range cases { - t.Run(name, func(t *testing.T) { - r := NewManagementPoliciesResolver(tc.args.managementPoliciesEnabled, tc.args.policy, xpv1.DeletionOrphan) - if diff := cmp.Diff(tc.want, r.ShouldOnlyObserve()); diff != "" { - t.Errorf("\nReason: %s\nShouldOnlyObserve(...): -want, +got:\n%s", tc.reason, diff) - } - }) - } -} - -func TestShouldDelete(t *testing.T) { - type args struct { - managementPoliciesEnabled bool - managed resource.Managed - } - type want struct { - delete bool - } - cases := map[string]struct { - reason string - args args - want want - }{ - "DeletionOrphan": { - reason: "Should orphan if management policies are disabled and deletion policy is set to Orphan.", - args: args{ - managementPoliciesEnabled: false, - managed: &fake.Managed{ - Orphanable: fake.Orphanable{ - Policy: xpv1.DeletionOrphan, - }, - }, - }, - want: want{delete: false}, - }, - "DeletionDelete": { - reason: "Should delete if management policies are disabled and deletion policy is set to Delete.", - args: args{ - managementPoliciesEnabled: false, - managed: &fake.Managed{ - Orphanable: fake.Orphanable{ - Policy: xpv1.DeletionDelete, - }, - }, - }, - want: want{delete: true}, - }, - "DeletionDeleteManagementActionAll": { - reason: "Should delete if management policies are enabled and deletion policy is set to Delete and management policy is set to All.", - args: args{ - managementPoliciesEnabled: true, - managed: &fake.Managed{ - Orphanable: fake.Orphanable{ - Policy: xpv1.DeletionDelete, - }, - Manageable: fake.Manageable{ - Policy: xpv1.ManagementPolicies{xpv1.ManagementActionAll}, - }, - }, - }, - want: want{delete: true}, - }, - "DeletionOrphanManagementActionAll": { - reason: "Should orphan if management policies are enabled and deletion policy is set to Orphan and management policy is set to All.", - args: args{ - managementPoliciesEnabled: true, - managed: &fake.Managed{ - Orphanable: fake.Orphanable{ - Policy: xpv1.DeletionOrphan, - }, - Manageable: fake.Manageable{ - Policy: xpv1.ManagementPolicies{xpv1.ManagementActionAll}, - }, - }, - }, - want: want{delete: false}, - }, - "DeletionDeleteManagementActionDelete": { - reason: "Should delete if management policies are enabled and deletion policy is set to Delete and management policy has action Delete.", - args: args{ - managementPoliciesEnabled: true, - managed: &fake.Managed{ - Orphanable: fake.Orphanable{ - Policy: xpv1.DeletionDelete, - }, - Manageable: fake.Manageable{ - Policy: xpv1.ManagementPolicies{xpv1.ManagementActionDelete}, - }, - }, - }, - want: want{delete: true}, - }, - "DeletionOrphanManagementActionDelete": { - reason: "Should delete if management policies are enabled and deletion policy is set to Orphan and management policy has action Delete.", - args: args{ - managementPoliciesEnabled: true, - managed: &fake.Managed{ - Orphanable: fake.Orphanable{ - Policy: xpv1.DeletionOrphan, - }, - Manageable: fake.Manageable{ - Policy: xpv1.ManagementPolicies{xpv1.ManagementActionDelete}, - }, - }, - }, - want: want{delete: true}, - }, - "DeletionDeleteManagementActionNoDelete": { - reason: "Should orphan if management policies are enabled and deletion policy is set to Delete and management policy does not have action Delete.", - args: args{ - managementPoliciesEnabled: true, - managed: &fake.Managed{ - Orphanable: fake.Orphanable{ - Policy: xpv1.DeletionDelete, - }, - Manageable: fake.Manageable{ - Policy: xpv1.ManagementPolicies{xpv1.ManagementActionObserve}, - }, - }, - }, - want: want{delete: false}, - }, - } - for name, tc := range cases { - t.Run(name, func(t *testing.T) { - r := NewManagementPoliciesResolver(tc.args.managementPoliciesEnabled, tc.args.managed.GetManagementPolicies(), tc.args.managed.GetDeletionPolicy()) - if diff := cmp.Diff(tc.want.delete, r.ShouldDelete()); diff != "" { - t.Errorf("\nReason: %s\nShouldDelete(...): -want, +got:\n%s", tc.reason, diff) - } - }) - } -} diff --git a/pkg/reconciler/providerconfig/reconciler.go b/pkg/reconciler/providerconfig/reconciler.go index dca195049..dc45da5aa 100644 --- a/pkg/reconciler/providerconfig/reconciler.go +++ b/pkg/reconciler/providerconfig/reconciler.go @@ -41,12 +41,6 @@ const ( finalizer = "in-use.crossplane.io" shortWait = 30 * time.Second timeout = 2 * time.Minute - - errGetPC = "cannot get ProviderConfig" - errListPCUs = "cannot list ProviderConfigUsages" - errDeletePCU = "cannot delete ProviderConfigUsage" - errUpdate = "cannot update ProviderConfig" - errUpdateStatus = "cannot update ProviderConfig status" ) // Event reasons. @@ -152,8 +146,8 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco // In case object is not found, most likely the object was deleted and // then disappeared while the event was in the processing queue. We // don't need to take any action in that case. - log.Debug(errGetPC, "error", err) - return reconcile.Result{}, errors.Wrap(resource.IgnoreNotFound(err), errGetPC) + log.Debug("cannot get ProviderConfig", "error", err) + return reconcile.Result{}, errors.Wrap(resource.IgnoreNotFound(err), "cannot get ProviderConfig") } log = log.WithValues( @@ -164,8 +158,8 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco l := r.newUsageList() if err := r.client.List(ctx, l, client.MatchingLabels{xpv1.LabelKeyProviderName: pc.GetName()}); err != nil { - log.Debug(errListPCUs, "error", err) - r.record.Event(pc, event.Warning(reasonAccount, errors.Wrap(err, errListPCUs))) + log.Debug("cannot list ProviderConfigUsages", "error", err) + r.record.Event(pc, event.Warning(reasonAccount, errors.Wrap(err, "cannot list ProviderConfigUsages"))) return reconcile.Result{RequeueAfter: shortWait}, nil } @@ -177,8 +171,8 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco // We can safely delete it - it's either stale, or will be recreated // next time the relevant managed resource connects. if err := r.client.Delete(ctx, pcu); resource.IgnoreNotFound(err) != nil { - log.Debug(errDeletePCU, "error", err) - r.record.Event(pc, event.Warning(reasonAccount, errors.Wrap(err, errDeletePCU))) + log.Debug("cannot delete ProviderConfigUsage", "error", err) + r.record.Event(pc, event.Warning(reasonAccount, errors.Wrap(err, "cannot delete ProviderConfigUsage"))) return reconcile.Result{RequeueAfter: shortWait}, nil //nolint:nilerr // Returning err would make us requeue instantly. } users-- @@ -196,12 +190,12 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco // We're watching our usages, so we'll be requeued when they go. pc.SetUsers(users) pc.SetConditions(Terminating().WithMessage(msg)) - return reconcile.Result{Requeue: false}, errors.Wrap(r.client.Status().Update(ctx, pc), errUpdateStatus) + return reconcile.Result{Requeue: false}, errors.Wrap(r.client.Status().Update(ctx, pc), "cannot update ProviderConfig status") } meta.RemoveFinalizer(pc, finalizer) if err := r.client.Update(ctx, pc); err != nil { - r.log.Debug(errUpdate, "error", err) + r.log.Debug("cannot update ProviderConfig", "error", err) return reconcile.Result{RequeueAfter: shortWait}, nil } @@ -211,11 +205,11 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco meta.AddFinalizer(pc, finalizer) if err := r.client.Update(ctx, pc); err != nil { - r.log.Debug(errUpdate, "error", err) + r.log.Debug("cannot update ProviderConfig", "error", err) return reconcile.Result{RequeueAfter: shortWait}, nil } // There's no need to requeue explicitly - we're watching all PCs. pc.SetUsers(users) - return reconcile.Result{Requeue: false}, errors.Wrap(r.client.Status().Update(ctx, pc), errUpdateStatus) + return reconcile.Result{Requeue: false}, errors.Wrap(r.client.Status().Update(ctx, pc), "cannot update ProviderConfig status") } diff --git a/pkg/reconciler/providerconfig/reconciler_test.go b/pkg/reconciler/providerconfig/reconciler_test.go index ef4332ef7..fc42a2d94 100644 --- a/pkg/reconciler/providerconfig/reconciler_test.go +++ b/pkg/reconciler/providerconfig/reconciler_test.go @@ -22,6 +22,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" kerrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -99,7 +100,7 @@ func TestReconciler(t *testing.T) { }, want: want{ result: reconcile.Result{}, - err: errors.Wrap(errBoom, errGetPC), + err: errBoom, }, }, "ProviderConfigNotFound": { @@ -291,7 +292,7 @@ func TestReconciler(t *testing.T) { }, want: want{ result: reconcile.Result{Requeue: false}, - err: errors.Wrap(errBoom, errUpdateStatus), + err: errBoom, }, }, "SuccessfulSetUsers": { @@ -322,7 +323,7 @@ func TestReconciler(t *testing.T) { r := NewReconciler(tc.args.m, tc.args.of) got, err := r.Reconcile(context.Background(), reconcile.Request{}) - 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\nr.Reconcile(...): -want error, +got error:\n%s", tc.reason, diff) } diff --git a/pkg/reference/reference.go b/pkg/reference/reference.go index 38e905c35..77d0e117f 100644 --- a/pkg/reference/reference.go +++ b/pkg/reference/reference.go @@ -32,14 +32,6 @@ import ( "github.com/crossplane/crossplane-runtime/pkg/resource" ) -// Error strings. -const ( - errGetManaged = "cannot get referenced resource" - errListManaged = "cannot list resources that match selector" - errNoMatches = "no resources matched selector" - errNoValue = "referenced field was empty (referenced resource may not yet be ready)" -) - // NOTE(negz): There are many equivalents of FromPtrValue and ToPtrValue // throughout the Crossplane codebase. We duplicate them here to reduce the // number of packages our API types have to import to support references. @@ -190,7 +182,7 @@ type ResolutionResponse struct { // Validate this ResolutionResponse. func (rr ResolutionResponse) Validate() error { if rr.ResolvedValue == "" { - return errors.New(errNoValue) + return errors.New("referenced field was empty (referenced resource may not yet be ready)") } return nil @@ -249,12 +241,12 @@ type MultiResolutionResponse struct { // Validate this MultiResolutionResponse. func (rr MultiResolutionResponse) Validate() error { if len(rr.ResolvedValues) == 0 { - return errors.New(errNoMatches) + return errors.New("no resources matched selector") } for i, v := range rr.ResolvedValues { if v == "" { - return getResolutionError(rr.ResolvedReferences[i].Policy, errors.New(errNoValue)) + return getResolutionError(rr.ResolvedReferences[i].Policy, errors.New("referenced field was empty (referenced resource may not yet be ready)")) } } @@ -287,9 +279,9 @@ func (r *APIResolver) Resolve(ctx context.Context, req ResolutionRequest) (Resol if req.Reference != nil { if err := r.client.Get(ctx, types.NamespacedName{Name: req.Reference.Name}, req.To.Managed); err != nil { if kerrors.IsNotFound(err) { - return ResolutionResponse{}, getResolutionError(req.Reference.Policy, errors.Wrap(err, errGetManaged)) + return ResolutionResponse{}, getResolutionError(req.Reference.Policy, errors.Wrap(err, "cannot get referenced resource")) } - return ResolutionResponse{}, errors.Wrap(err, errGetManaged) + return ResolutionResponse{}, errors.Wrap(err, "cannot get referenced resource") } rsp := ResolutionResponse{ResolvedValue: req.Extract(req.To.Managed), ResolvedReference: req.Reference} @@ -298,7 +290,7 @@ func (r *APIResolver) Resolve(ctx context.Context, req ResolutionRequest) (Resol // The reference was not set, but a selector was. Select a reference. if err := r.client.List(ctx, req.To.List, client.MatchingLabels(req.Selector.MatchLabels)); err != nil { - return ResolutionResponse{}, errors.Wrap(err, errListManaged) + return ResolutionResponse{}, errors.Wrap(err, "cannot list resources that match selector") } for _, to := range req.To.List.GetItems() { @@ -311,7 +303,7 @@ func (r *APIResolver) Resolve(ctx context.Context, req ResolutionRequest) (Resol } // We couldn't resolve anything. - return ResolutionResponse{}, getResolutionError(req.Selector.Policy, errors.New(errNoMatches)) + return ResolutionResponse{}, getResolutionError(req.Selector.Policy, errors.New("no resources matched selector")) } @@ -330,9 +322,9 @@ func (r *APIResolver) ResolveMultiple(ctx context.Context, req MultiResolutionRe for i := range req.References { if err := r.client.Get(ctx, types.NamespacedName{Name: req.References[i].Name}, req.To.Managed); err != nil { if kerrors.IsNotFound(err) { - return MultiResolutionResponse{}, getResolutionError(req.References[i].Policy, errors.Wrap(err, errGetManaged)) + return MultiResolutionResponse{}, getResolutionError(req.References[i].Policy, errors.Wrap(err, "cannot get referenced resource")) } - return MultiResolutionResponse{}, errors.Wrap(err, errGetManaged) + return MultiResolutionResponse{}, errors.Wrap(err, "cannot get referenced resource") } vals[i] = req.Extract(req.To.Managed) } @@ -343,7 +335,7 @@ func (r *APIResolver) ResolveMultiple(ctx context.Context, req MultiResolutionRe // No references were set, but a selector was. Select and resolve references. if err := r.client.List(ctx, req.To.List, client.MatchingLabels(req.Selector.MatchLabels)); err != nil { - return MultiResolutionResponse{}, errors.Wrap(err, errListManaged) + return MultiResolutionResponse{}, errors.Wrap(err, "cannot list resources that match selector") } items := req.To.List.GetItems() diff --git a/pkg/reference/reference_test.go b/pkg/reference/reference_test.go index 99f1a1733..81749891e 100644 --- a/pkg/reference/reference_test.go +++ b/pkg/reference/reference_test.go @@ -227,7 +227,7 @@ func TestResolve(t *testing.T) { }, }, want: want{ - err: errors.Wrap(errBoom, errGetManaged), + err: errBoom, }, }, "ResolvedNoValue": { @@ -247,7 +247,7 @@ func TestResolve(t *testing.T) { rsp: ResolutionResponse{ ResolvedReference: ref, }, - err: errors.New(errNoValue), + err: cmpopts.AnyError, }, }, "SuccessfulResolve": { @@ -306,7 +306,7 @@ func TestResolve(t *testing.T) { }, want: want{ rsp: ResolutionResponse{}, - err: errors.Wrap(errBoom, errListManaged), + err: errBoom, }, }, "NoMatches": { @@ -323,7 +323,7 @@ func TestResolve(t *testing.T) { }, want: want{ rsp: ResolutionResponse{}, - err: errors.New(errNoMatches), + err: cmpopts.AnyError, }, }, "OptionalSelector": { @@ -431,7 +431,7 @@ func TestResolve(t *testing.T) { t.Run(name, func(t *testing.T) { r := NewAPIResolver(tc.c, tc.from) got, err := r.Resolve(tc.args.ctx, tc.args.req) - 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\nControllersMustMatch(...): -want error, +got error:\n%s", tc.reason, diff) } if diff := cmp.Diff(tc.want.rsp, got); diff != "" { @@ -542,7 +542,7 @@ func TestResolveMultiple(t *testing.T) { }, }, want: want{ - err: errors.Wrap(errBoom, errGetManaged), + err: errBoom, }, }, "ResolvedNoValue": { @@ -563,7 +563,7 @@ func TestResolveMultiple(t *testing.T) { ResolvedValues: []string{""}, ResolvedReferences: []xpv1.Reference{ref}, }, - err: errors.New(errNoValue), + err: cmpopts.AnyError, }, }, "SuccessfulResolve": { @@ -623,7 +623,7 @@ func TestResolveMultiple(t *testing.T) { }, want: want{ rsp: MultiResolutionResponse{}, - err: errors.Wrap(errBoom, errListManaged), + err: errBoom, }, }, "NoMatches": { @@ -640,7 +640,7 @@ func TestResolveMultiple(t *testing.T) { }, want: want{ rsp: MultiResolutionResponse{}, - err: errors.New(errNoMatches), + err: cmpopts.AnyError, }, }, "OptionalSelector": { @@ -748,7 +748,7 @@ func TestResolveMultiple(t *testing.T) { t.Run(name, func(t *testing.T) { r := NewAPIResolver(tc.c, tc.from) got, err := r.ResolveMultiple(tc.args.ctx, tc.args.req) - 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\nControllersMustMatch(...): -want error, +got error:\n%s", tc.reason, diff) } if diff := cmp.Diff(tc.want.rsp, got, cmpopts.EquateEmpty()); diff != "" { diff --git a/pkg/resource/api.go b/pkg/resource/api.go index 8c60dd3e4..5e1d59195 100644 --- a/pkg/resource/api.go +++ b/pkg/resource/api.go @@ -30,11 +30,6 @@ import ( "github.com/crossplane/crossplane-runtime/pkg/meta" ) -// Error strings. -const ( - errUpdateObject = "cannot update object" -) - // An APIPatchingApplicator applies changes to an object by either creating or // patching it in a Kubernetes API server. type APIPatchingApplicator struct { @@ -162,7 +157,7 @@ func (a *APIFinalizer) AddFinalizer(ctx context.Context, obj Object) error { return nil } meta.AddFinalizer(obj, a.finalizer) - return errors.Wrap(a.client.Update(ctx, obj), errUpdateObject) + return errors.Wrap(a.client.Update(ctx, obj), "cannot update object") } // RemoveFinalizer from the supplied Managed resource. @@ -171,7 +166,7 @@ func (a *APIFinalizer) RemoveFinalizer(ctx context.Context, obj Object) error { return nil } meta.RemoveFinalizer(obj, a.finalizer) - return errors.Wrap(IgnoreNotFound(a.client.Update(ctx, obj)), errUpdateObject) + return errors.Wrap(IgnoreNotFound(a.client.Update(ctx, obj)), "cannot update object") } // A FinalizerFns satisfy the Finalizer interface. diff --git a/pkg/resource/api_test.go b/pkg/resource/api_test.go index 731cee31e..7f7781f9b 100644 --- a/pkg/resource/api_test.go +++ b/pkg/resource/api_test.go @@ -21,6 +21,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" kerrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -62,7 +63,7 @@ func TestAPIPatchingApplicator(t *testing.T) { }, want: want{ o: &object{}, - err: errors.Wrap(errBoom, "cannot get object"), + err: errBoom, }, }, "CreateError": { @@ -76,7 +77,7 @@ func TestAPIPatchingApplicator(t *testing.T) { }, want: want{ o: &object{}, - err: errors.Wrap(errBoom, "cannot create object"), + err: errBoom, }, }, "ApplyOptionError": { @@ -102,7 +103,7 @@ func TestAPIPatchingApplicator(t *testing.T) { }, want: want{ o: &object{}, - err: errors.Wrap(errBoom, "cannot patch object"), + err: errBoom, }, }, "Created": { @@ -143,7 +144,7 @@ func TestAPIPatchingApplicator(t *testing.T) { t.Run(name, func(t *testing.T) { a := NewAPIPatchingApplicator(tc.c) err := a.Apply(tc.args.ctx, tc.args.o, tc.args.ao...) - 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\nApply(...): -want error, +got error\n%s\n", tc.reason, diff) } if diff := cmp.Diff(tc.want.o, tc.args.o); diff != "" { @@ -185,7 +186,7 @@ func TestAPIUpdatingApplicator(t *testing.T) { }, want: want{ o: &object{}, - err: errors.Wrap(errBoom, "cannot get object"), + err: errBoom, }, }, "CreateError": { @@ -199,7 +200,7 @@ func TestAPIUpdatingApplicator(t *testing.T) { }, want: want{ o: &object{}, - err: errors.Wrap(errBoom, "cannot create object"), + err: errBoom, }, }, "ApplyOptionError": { @@ -225,7 +226,7 @@ func TestAPIUpdatingApplicator(t *testing.T) { }, want: want{ o: &object{}, - err: errors.Wrap(errBoom, "cannot update object"), + err: errBoom, }, }, "Created": { @@ -271,7 +272,7 @@ func TestAPIUpdatingApplicator(t *testing.T) { t.Run(name, func(t *testing.T) { a := NewAPIUpdatingApplicator(tc.c) err := a.Apply(tc.args.ctx, tc.args.o, tc.args.ao...) - 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\nApply(...): -want error, +got error\n%s\n", tc.reason, diff) } if diff := cmp.Diff(tc.want.o, tc.args.o); diff != "" { @@ -308,7 +309,7 @@ func TestManagedRemoveFinalizer(t *testing.T) { obj: &fake.Object{ObjectMeta: metav1.ObjectMeta{Finalizers: []string{finalizer}}}, }, want: want{ - err: errors.Wrap(errBoom, errUpdateObject), + err: errBoom, obj: &fake.Object{ObjectMeta: metav1.ObjectMeta{Finalizers: []string{}}}, }, }, @@ -329,7 +330,7 @@ func TestManagedRemoveFinalizer(t *testing.T) { t.Run(name, func(t *testing.T) { api := NewAPIFinalizer(tc.client, finalizer) err := api.RemoveFinalizer(tc.args.ctx, tc.args.obj) - if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want.err, err, cmpopts.EquateErrors()); diff != "" { t.Errorf("api.RemoveFinalizer(...): -want error, +got error:\n%s", diff) } if diff := cmp.Diff(tc.want.obj, tc.args.obj, test.EquateConditions()); diff != "" { @@ -366,7 +367,7 @@ func TestAPIFinalizerAdder(t *testing.T) { obj: &fake.Object{ObjectMeta: metav1.ObjectMeta{Finalizers: []string{}}}, }, want: want{ - err: errors.Wrap(errBoom, errUpdateObject), + err: errBoom, obj: &fake.Object{ObjectMeta: metav1.ObjectMeta{Finalizers: []string{finalizer}}}, }, }, @@ -387,7 +388,7 @@ func TestAPIFinalizerAdder(t *testing.T) { t.Run(name, func(t *testing.T) { api := NewAPIFinalizer(tc.client, finalizer) err := api.AddFinalizer(tc.args.ctx, tc.args.obj) - if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want.err, err, cmpopts.EquateErrors()); diff != "" { t.Errorf("api.Initialize(...): -want error, +got error:\n%s", diff) } if diff := cmp.Diff(tc.want.obj, tc.args.obj, test.EquateConditions()); diff != "" { diff --git a/pkg/resource/providerconfig.go b/pkg/resource/providerconfig.go index 5c1269f12..c14216e5d 100644 --- a/pkg/resource/providerconfig.go +++ b/pkg/resource/providerconfig.go @@ -32,16 +32,6 @@ import ( "github.com/crossplane/crossplane-runtime/pkg/meta" ) -const ( - errExtractEnv = "cannot extract from environment variable when none specified" - errExtractFs = "cannot extract from filesystem when no path specified" - errExtractSecretKey = "cannot extract from secret key when none specified" - errGetCredentialsSecret = "cannot get credentials secret" - errNoHandlerForSourceFmt = "no extraction handler registered for source: %s" - errMissingPCRef = "managed resource does not reference a ProviderConfig" - errApplyPCU = "cannot apply ProviderConfigUsage" -) - type errMissingRef struct{ error } func (m errMissingRef) MissingReference() bool { return true } @@ -61,7 +51,7 @@ type EnvLookupFn func(string) string // ExtractEnv extracts credentials from an environment variable. func ExtractEnv(_ context.Context, e EnvLookupFn, s xpv1.CommonCredentialSelectors) ([]byte, error) { if s.Env == nil { - return nil, errors.New(errExtractEnv) + return nil, errors.New("cannot extract from environment variable when none specified") } return []byte(e(s.Env.Name)), nil } @@ -69,7 +59,7 @@ func ExtractEnv(_ context.Context, e EnvLookupFn, s xpv1.CommonCredentialSelecto // ExtractFs extracts credentials from the filesystem. func ExtractFs(_ context.Context, fs afero.Fs, s xpv1.CommonCredentialSelectors) ([]byte, error) { if s.Fs == nil { - return nil, errors.New(errExtractFs) + return nil, errors.New("cannot extract from filesystem when no path specified") } return afero.ReadFile(fs, s.Fs.Path) } @@ -77,11 +67,11 @@ func ExtractFs(_ context.Context, fs afero.Fs, s xpv1.CommonCredentialSelectors) // ExtractSecret extracts credentials from a Kubernetes secret. func ExtractSecret(ctx context.Context, client client.Client, s xpv1.CommonCredentialSelectors) ([]byte, error) { if s.SecretRef == nil { - return nil, errors.New(errExtractSecretKey) + return nil, errors.New("cannot extract from secret key when none specified") } secret := &corev1.Secret{} if err := client.Get(ctx, types.NamespacedName{Namespace: s.SecretRef.Namespace, Name: s.SecretRef.Name}, secret); err != nil { - return nil, errors.Wrap(err, errGetCredentialsSecret) + return nil, errors.Wrap(err, "cannot get credentials secret") } return secret.Data[s.SecretRef.Key], nil } @@ -102,7 +92,7 @@ func CommonCredentialExtractor(ctx context.Context, source xpv1.CredentialsSourc // implement their own. fallthrough default: - return nil, errors.Errorf(errNoHandlerForSourceFmt, source) + return nil, errors.Errorf("no extraction handler registered for source: %s", source) } } @@ -142,7 +132,7 @@ func (u *ProviderConfigUsageTracker) Track(ctx context.Context, mg Managed) erro gvk := mg.GetObjectKind().GroupVersionKind() ref := mg.GetProviderConfigReference() if ref == nil { - return errMissingRef{errors.New(errMissingPCRef)} + return errMissingRef{errors.New("managed resource does not reference a ProviderConfig")} } pcu.SetName(string(mg.GetUID())) @@ -161,5 +151,5 @@ func (u *ProviderConfigUsageTracker) Track(ctx context.Context, mg Managed) erro return current.(ProviderConfigUsage).GetProviderConfigReference() != pcu.GetProviderConfigReference() }), ) - return errors.Wrap(Ignore(IsNotAllowed, err), errApplyPCU) + return errors.Wrap(Ignore(IsNotAllowed, err), "cannot apply ProviderConfigUsage") } diff --git a/pkg/resource/providerconfig_test.go b/pkg/resource/providerconfig_test.go index eb973bb6f..14e2817e1 100644 --- a/pkg/resource/providerconfig_test.go +++ b/pkg/resource/providerconfig_test.go @@ -21,6 +21,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "github.com/spf13/afero" corev1 "k8s.io/api/core/v1" "sigs.k8s.io/controller-runtime/pkg/client" @@ -69,14 +70,14 @@ func TestExtractEnv(t *testing.T) { e: func(string) string { return string(credentials) }, }, want: want{ - err: errors.New(errExtractEnv), + err: cmpopts.AnyError, }, }, } for name, tc := range cases { t.Run(name, func(t *testing.T) { got, err := ExtractEnv(context.TODO(), tc.args.e, tc.args.creds) - 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\npc.ExtractEnv(...): -want error, +got error:\n%s\n", tc.reason, diff) } if diff := cmp.Diff(tc.want.b, got); diff != "" { @@ -128,14 +129,14 @@ func TestExtractFs(t *testing.T) { fs: mockFs, }, want: want{ - err: errors.New(errExtractFs), + err: cmpopts.AnyError, }, }, } for name, tc := range cases { t.Run(name, func(t *testing.T) { got, err := ExtractFs(context.TODO(), tc.args.fs, tc.args.creds) - 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\npc.ExtractFs(...): -want error, +got error:\n%s\n", tc.reason, diff) } if diff := cmp.Diff(tc.want.b, got); diff != "" { @@ -194,7 +195,7 @@ func TestExtractSecret(t *testing.T) { reason: "Failed extraction of credentials from Secret when key not defined", args: args{}, want: want{ - err: errors.New(errExtractSecretKey), + err: cmpopts.AnyError, }, }, "SecretFailureGet": { @@ -216,14 +217,14 @@ func TestExtractSecret(t *testing.T) { }, }, want: want{ - err: errors.Wrap(errBoom, errGetCredentialsSecret), + err: errBoom, }, }, } for name, tc := range cases { t.Run(name, func(t *testing.T) { got, err := ExtractSecret(context.TODO(), tc.args.client, tc.args.creds) - 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\npc.ExtractSecret(...): -want error, +got error:\n%s\n", tc.reason, diff) } if diff := cmp.Diff(tc.want.b, got); diff != "" { @@ -261,7 +262,8 @@ func TestTrack(t *testing.T) { args: args{ mg: &fake.Managed{}, }, - want: errMissingRef{errors.New(errMissingPCRef)}, + // TODO(negz): Actually test that this satisfies IsMissingReference :/ + want: cmpopts.AnyError, }, "NopUpdate": { reason: "No error should be returned if the apply fails because it would be a no-op", @@ -311,7 +313,7 @@ func TestTrack(t *testing.T) { }, }, }, - want: errors.Wrap(errBoom, errApplyPCU), + want: errBoom, }, } @@ -319,7 +321,7 @@ func TestTrack(t *testing.T) { t.Run(name, func(t *testing.T) { ut := &ProviderConfigUsageTracker{c: tc.fields.c, of: tc.fields.of} got := ut.Track(tc.args.ctx, tc.args.mg) - if diff := cmp.Diff(tc.want, got, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want, got, cmpopts.EquateErrors()); diff != "" { t.Errorf("\n%s\nut.Track(...): -want error, +got error:\n%s\n", tc.reason, diff) } }) diff --git a/pkg/resource/resource_test.go b/pkg/resource/resource_test.go index 8a506dea8..d6557e0eb 100644 --- a/pkg/resource/resource_test.go +++ b/pkg/resource/resource_test.go @@ -22,6 +22,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" corev1 "k8s.io/api/core/v1" kerrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -35,7 +36,6 @@ import ( xpv1 "github.com/crossplane/crossplane-runtime/apis/common/v1" "github.com/crossplane/crossplane-runtime/pkg/errors" "github.com/crossplane/crossplane-runtime/pkg/resource/fake" - "github.com/crossplane/crossplane-runtime/pkg/test" ) const ( @@ -205,7 +205,7 @@ func TestGetKind(t *testing.T) { ot: MockTyper{Error: errBoom}, }, want: want{ - err: errors.Wrap(errBoom, "cannot get kind of supplied object"), + err: errBoom, }, }, "KindIsUnversioned": { @@ -213,7 +213,7 @@ func TestGetKind(t *testing.T) { ot: MockTyper{Unversioned: true}, }, want: want{ - err: errors.New("supplied object is unversioned"), + err: cmpopts.AnyError, }, }, "NotEnoughKinds": { @@ -221,7 +221,7 @@ func TestGetKind(t *testing.T) { ot: MockTyper{}, }, want: want{ - err: errors.New("supplied object does not have exactly one kind"), + err: cmpopts.AnyError, }, }, "TooManyKinds": { @@ -232,7 +232,7 @@ func TestGetKind(t *testing.T) { }}, }, want: want{ - err: errors.New("supplied object does not have exactly one kind"), + err: cmpopts.AnyError, }, }, } @@ -240,7 +240,7 @@ func TestGetKind(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { got, err := GetKind(tc.args.obj, tc.args.ot) - if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want.err, err, cmpopts.EquateErrors()); diff != "" { t.Errorf("GetKind(...): -want error, +got error:\n%s", diff) } if diff := cmp.Diff(tc.want.kind, got); diff != "" { @@ -307,7 +307,7 @@ func TestIgnore(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { got := Ignore(tc.args.is, tc.args.err) - if diff := cmp.Diff(tc.want, got, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want, got, cmpopts.EquateErrors()); diff != "" { t.Errorf("Ignore(...): -want error, +got error:\n%s", diff) } }) @@ -354,7 +354,7 @@ func TestIgnoreAny(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { got := IgnoreAny(tc.args.err, tc.args.is...) - if diff := cmp.Diff(tc.want, got, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want, got, cmpopts.EquateErrors()); diff != "" { t.Errorf("Ignore(...): -want error, +got error:\n%s", diff) } }) @@ -479,7 +479,8 @@ func TestMustBeControllableBy(t *testing.T) { Controller: &controller, }}}}, }, - want: errNotControllable{errors.Errorf("existing object is not controlled by UID %q", uid)}, + // TODO(negz): Test that this actually satisfies IsNotControllable :/ + want: cmpopts.AnyError, }, } @@ -488,7 +489,7 @@ func TestMustBeControllableBy(t *testing.T) { ao := MustBeControllableBy(tc.u) err := ao(tc.args.ctx, tc.args.current, tc.args.desired) - if diff := cmp.Diff(tc.want, err, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want, err, cmpopts.EquateErrors()); diff != "" { t.Errorf("\n%s\nMustBeControllableBy(...)(...): -want error, +got error\n%s\n", tc.reason, diff) } }) @@ -543,7 +544,8 @@ func TestConnectionSecretMustBeControllableBy(t *testing.T) { Type: SecretTypeConnection, }, }, - want: errNotControllable{errors.Errorf("existing secret is not controlled by UID %q", uid)}, + // TODO(negz): Test that this satisfies IsNotControllable? + want: cmpopts.AnyError, }, "UncontrolledOpaqueSecret": { reason: "A Secret of corev1.SecretTypeOpqaue with no controller is not controllable", @@ -551,7 +553,8 @@ func TestConnectionSecretMustBeControllableBy(t *testing.T) { args: args{ current: &corev1.Secret{Type: corev1.SecretTypeOpaque}, }, - want: errNotControllable{errors.Errorf("refusing to modify uncontrolled secret of type %q", corev1.SecretTypeOpaque)}, + // TODO(negz): Test that this satisfies IsNotControllable? + want: cmpopts.AnyError, }, } @@ -560,7 +563,7 @@ func TestConnectionSecretMustBeControllableBy(t *testing.T) { ao := ConnectionSecretMustBeControllableBy(tc.u) err := ao(tc.args.ctx, tc.args.current, tc.args.desired) - if diff := cmp.Diff(tc.want, err, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want, err, cmpopts.EquateErrors()); diff != "" { t.Errorf("\n%s\nConnectionSecretMustBeControllableBy(...)(...): -want error, +got error\n%s\n", tc.reason, diff) } }) @@ -593,7 +596,8 @@ func TestAllowUpdateIf(t *testing.T) { args: args{ current: &object{}, }, - want: errNotAllowed{errors.New("update not allowed")}, + // TODO(negz): Actually test that this satisfies IsNotAllowed. :/ + want: cmpopts.AnyError, }, } @@ -602,7 +606,7 @@ func TestAllowUpdateIf(t *testing.T) { ao := AllowUpdateIf(tc.fn) err := ao(tc.args.ctx, tc.args.current, tc.args.desired) - if diff := cmp.Diff(tc.want, err, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want, err, cmpopts.EquateErrors()); diff != "" { t.Errorf("\n%s\nAllowUpdateIf(...)(...): -want error, +got error\n%s\n", tc.reason, diff) } }) @@ -811,7 +815,7 @@ func TestApplicatorWithRetry_Apply(t *testing.T) { backoff: tc.fields.backoff, } - if diff := cmp.Diff(tc.wantErr, awr.Apply(tc.args.ctx, tc.args.c, tc.args.opts...), test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.wantErr, awr.Apply(tc.args.ctx, tc.args.c, tc.args.opts...), cmpopts.EquateErrors()); diff != "" { t.Fatalf("ApplicatorWithRetry.Apply(...): -want, +got:\n%s", diff) } diff --git a/pkg/resource/unstructured/client_test.go b/pkg/resource/unstructured/client_test.go index a68c30d32..a9254b703 100644 --- a/pkg/resource/unstructured/client_test.go +++ b/pkg/resource/unstructured/client_test.go @@ -22,6 +22,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "sigs.k8s.io/controller-runtime/pkg/client" @@ -108,7 +109,7 @@ func TestGet(t *testing.T) { t.Run(name, func(t *testing.T) { c := NewClient(tc.c) got := c.Get(tc.args.ctx, tc.args.key, tc.args.obj) - if diff := cmp.Diff(tc.want, got, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want, got, cmpopts.EquateErrors()); diff != "" { t.Errorf("\nc.Get(...): -want error, +got error:\n %s", diff) } }) @@ -151,7 +152,7 @@ func TestList(t *testing.T) { t.Run(name, func(t *testing.T) { c := NewClient(tc.c) got := c.List(tc.args.ctx, tc.args.obj) - if diff := cmp.Diff(tc.want, got, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want, got, cmpopts.EquateErrors()); diff != "" { t.Errorf("\nc.List(...): -want error, +got error:\n %s", diff) } }) @@ -192,7 +193,7 @@ func TestCreate(t *testing.T) { t.Run(name, func(t *testing.T) { c := NewClient(tc.c) got := c.Create(tc.args.ctx, tc.args.obj) - if diff := cmp.Diff(tc.want, got, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want, got, cmpopts.EquateErrors()); diff != "" { t.Errorf("\nc.Create(...): -want error, +got error:\n %s", diff) } }) @@ -233,7 +234,7 @@ func TestDelete(t *testing.T) { t.Run(name, func(t *testing.T) { c := NewClient(tc.c) got := c.Delete(tc.args.ctx, tc.args.obj) - if diff := cmp.Diff(tc.want, got, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want, got, cmpopts.EquateErrors()); diff != "" { t.Errorf("\nc.Delete(...): -want error, +got error:\n %s", diff) } }) @@ -274,7 +275,7 @@ func TestUpdate(t *testing.T) { t.Run(name, func(t *testing.T) { c := NewClient(tc.c) got := c.Update(tc.args.ctx, tc.args.obj) - if diff := cmp.Diff(tc.want, got, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want, got, cmpopts.EquateErrors()); diff != "" { t.Errorf("\nc.Update(...): -want error, +got error:\n %s", diff) } }) @@ -316,7 +317,7 @@ func TestPatch(t *testing.T) { t.Run(name, func(t *testing.T) { c := NewClient(tc.c) got := c.Patch(tc.args.ctx, tc.args.obj, tc.args.patch) - if diff := cmp.Diff(tc.want, got, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want, got, cmpopts.EquateErrors()); diff != "" { t.Errorf("\nc.Patch(...): -want error, +got error:\n %s", diff) } }) @@ -357,7 +358,7 @@ func TestDeleteAllOf(t *testing.T) { t.Run(name, func(t *testing.T) { c := NewClient(tc.c) got := c.DeleteAllOf(tc.args.ctx, tc.args.obj) - if diff := cmp.Diff(tc.want, got, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want, got, cmpopts.EquateErrors()); diff != "" { t.Errorf("\nc.DeleteAllOf(...): -want error, +got error:\n %s", diff) } }) @@ -399,7 +400,7 @@ func TestStatusCreate(t *testing.T) { t.Run(name, func(t *testing.T) { c := NewClient(tc.c) got := c.Status().Create(tc.args.ctx, tc.args.obj, tc.args.sub) - if diff := cmp.Diff(tc.want, got, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want, got, cmpopts.EquateErrors()); diff != "" { t.Errorf("\nc.Status().Create(...): -want error, +got error:\n %s", diff) } }) @@ -440,7 +441,7 @@ func TestStatusUpdate(t *testing.T) { t.Run(name, func(t *testing.T) { c := NewClient(tc.c) got := c.Status().Update(tc.args.ctx, tc.args.obj) - if diff := cmp.Diff(tc.want, got, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want, got, cmpopts.EquateErrors()); diff != "" { t.Errorf("\nc.Status().Update(...): -want error, +got error:\n %s", diff) } }) @@ -482,7 +483,7 @@ func TestStatusPatch(t *testing.T) { t.Run(name, func(t *testing.T) { c := NewClient(tc.c) got := c.Status().Patch(tc.args.ctx, tc.args.obj, tc.args.patch) - if diff := cmp.Diff(tc.want, got, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.want, got, cmpopts.EquateErrors()); diff != "" { t.Errorf("\nc.StatusPatch(...): -want error, +got error:\n %s", diff) } }) diff --git a/pkg/test/cmp.go b/pkg/test/cmp.go index a7414c1bf..90ee15331 100644 --- a/pkg/test/cmp.go +++ b/pkg/test/cmp.go @@ -33,6 +33,8 @@ import ( // This differs from cmpopts.EquateErrors, which does not test for error strings // and instead returns whether one error 'is' (in the errors.Is sense) the // other. +// +// Deprecated: Use cmpopts.EquateErrors instead. func EquateErrors() cmp.Option { return cmp.Comparer(func(a, b error) bool { if a == nil || b == nil { diff --git a/pkg/webhook/mutator_test.go b/pkg/webhook/mutator_test.go index 8ad7e4c03..da13b4526 100644 --- a/pkg/webhook/mutator_test.go +++ b/pkg/webhook/mutator_test.go @@ -21,10 +21,9 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/webhook" - - "github.com/crossplane/crossplane-runtime/pkg/test" ) // Mutator has to satisfy CustomDefaulter interface so that it can be used by @@ -72,7 +71,7 @@ func TestDefault(t *testing.T) { t.Run(name, func(t *testing.T) { v := NewMutator(WithMutationFns(tc.fns...)) err := v.Default(context.TODO(), tc.args.obj) - 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\nDefault(...): -want, +got\n%s\n", tc.reason, diff) } }) diff --git a/pkg/webhook/validator_test.go b/pkg/webhook/validator_test.go index 8d2455526..7677a802e 100644 --- a/pkg/webhook/validator_test.go +++ b/pkg/webhook/validator_test.go @@ -27,7 +27,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/webhook/admission" "github.com/crossplane/crossplane-runtime/pkg/errors" - "github.com/crossplane/crossplane-runtime/pkg/test" ) // Validator has to satisfy CustomValidator interface so that it can be used by @@ -106,7 +105,7 @@ func TestValidateCreate(t *testing.T) { t.Run(name, func(t *testing.T) { v := NewValidator(WithValidateCreationFns(tc.fns...)) warn, err := v.ValidateCreate(context.TODO(), tc.args.obj) - 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\nValidateCreate(...): -want error, +got error\n%s\n", tc.reason, diff) } if diff := cmp.Diff(tc.want.warnings, warn, cmpopts.EquateEmpty()); diff != "" { @@ -186,7 +185,7 @@ func TestValidateUpdate(t *testing.T) { t.Run(name, func(t *testing.T) { v := NewValidator(WithValidateUpdateFns(tc.fns...)) warn, err := v.ValidateUpdate(context.TODO(), tc.args.oldObj, tc.args.newObj) - 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\nValidateUpdate(...): -want error, +got error\n%s\n", tc.reason, diff) } if diff := cmp.Diff(tc.want.warnings, warn, cmpopts.EquateEmpty()); diff != "" { @@ -265,7 +264,7 @@ func TestValidateDelete(t *testing.T) { t.Run(name, func(t *testing.T) { v := NewValidator(WithValidateDeletionFns(tc.fns...)) warn, err := v.ValidateDelete(context.TODO(), tc.args.obj) - 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\nValidateDelete(...): -want error, +got error\n%s\n", tc.reason, diff) } if diff := cmp.Diff(tc.want.warnings, warn, cmpopts.EquateEmpty()); diff != "" {