From 678177c524c897d5e40773bd20072ee4ddc8bb32 Mon Sep 17 00:00:00 2001 From: Nic Cope Date: Thu, 3 Jul 2025 12:51:13 -0700 Subject: [PATCH] Run golangci-lint run --fix This commit is entirely generated by earthly +reviewable Signed-off-by: Nic Cope --- apis/common/v1/condition.go | 4 + apis/common/v1/connection_details.go | 2 + apis/common/v1/merge.go | 2 + apis/common/v1/merge_test.go | 3 + apis/common/v1/resource.go | 2 + pkg/certificates/certificates.go | 2 + pkg/certificates/certificates_test.go | 10 ++- pkg/conditions/manager.go | 1 + pkg/conditions/manager_test.go | 2 + pkg/connection/fake/mocks.go | 3 + pkg/connection/manager.go | 4 + pkg/connection/manager_test.go | 40 +++++----- pkg/connection/store/kubernetes/store.go | 13 ++++ pkg/connection/store/kubernetes/store_test.go | 41 ++++++---- pkg/connection/store/plugin/store.go | 5 ++ pkg/connection/store/plugin/store_test.go | 27 ++++--- pkg/connection/store/store.go | 6 ++ pkg/connection/stores.go | 1 + pkg/errors/errors.go | 4 + pkg/errors/errors_test.go | 2 + pkg/errors/reconcile.go | 1 + pkg/errors/reconcile_test.go | 3 + pkg/event/event.go | 4 + pkg/event/event_test.go | 1 + pkg/feature/feature.go | 4 + pkg/fieldpath/fieldpath.go | 13 ++++ pkg/fieldpath/fieldpath_test.go | 1 + pkg/fieldpath/merge.go | 7 ++ pkg/fieldpath/merge_test.go | 6 ++ pkg/fieldpath/paved.go | 55 ++++++++++++++ pkg/fieldpath/paved_test.go | 23 ++++++ pkg/logging/klog.go | 1 + pkg/meta/meta.go | 24 ++++++ pkg/meta/meta_test.go | 10 ++- pkg/parser/fsreader.go | 15 ++++ pkg/parser/linter.go | 7 ++ pkg/parser/linter_test.go | 2 - pkg/parser/parser.go | 25 +++++++ pkg/parser/parser_test.go | 6 ++ pkg/password/password.go | 2 + pkg/password/password_test.go | 3 +- pkg/ratelimiter/default.go | 1 + pkg/ratelimiter/reconciler.go | 3 + pkg/ratelimiter/reconciler_test.go | 2 + pkg/reconciler/managed/api.go | 13 ++++ pkg/reconciler/managed/api_test.go | 11 +++ pkg/reconciler/managed/changelogger.go | 1 + pkg/reconciler/managed/changelogger_test.go | 3 + pkg/reconciler/managed/metrics.go | 3 + pkg/reconciler/managed/policies.go | 6 ++ pkg/reconciler/managed/publisher.go | 6 ++ pkg/reconciler/managed/publisher_test.go | 6 ++ pkg/reconciler/managed/reconciler.go | 75 ++++++++++++++++++- pkg/reconciler/managed/reconciler_test.go | 12 ++- pkg/reconciler/managed/reconciler_typed.go | 6 ++ pkg/reconciler/providerconfig/reconciler.go | 8 ++ .../providerconfig/reconciler_test.go | 5 +- pkg/reference/reference.go | 27 +++++++ pkg/reference/reference_test.go | 8 ++ pkg/resource/api.go | 7 ++ pkg/resource/api_test.go | 9 +++ pkg/resource/fake/mocks.go | 29 +++++++ pkg/resource/late_initializer.go | 9 +++ pkg/resource/late_initializer_test.go | 36 +++++++-- pkg/resource/predicates.go | 3 + pkg/resource/predicates_test.go | 8 +- pkg/resource/providerconfig.go | 7 ++ pkg/resource/providerconfig_test.go | 4 + pkg/resource/resource.go | 22 ++++++ pkg/resource/resource_test.go | 16 ++-- pkg/resource/unstructured/claim/claim.go | 14 ++++ pkg/resource/unstructured/claim/claim_test.go | 12 +++ pkg/resource/unstructured/client.go | 12 +++ pkg/resource/unstructured/client_test.go | 20 +++++ .../unstructured/composed/composed.go | 6 ++ .../unstructured/composed/composed_test.go | 2 + .../unstructured/composite/composite.go | 25 +++++++ .../unstructured/composite/composite_test.go | 10 +++ pkg/statemetrics/mr_state_metrics.go | 2 + pkg/test/cmp.go | 1 + pkg/test/fake.go | 12 +++ pkg/webhook/mutator.go | 2 + pkg/webhook/mutator_test.go | 7 +- pkg/webhook/validator.go | 10 +++ pkg/webhook/validator_test.go | 30 +++++--- 85 files changed, 812 insertions(+), 86 deletions(-) diff --git a/apis/common/v1/condition.go b/apis/common/v1/condition.go index 48195b3..5a2cc4e 100644 --- a/apis/common/v1/condition.go +++ b/apis/common/v1/condition.go @@ -126,6 +126,7 @@ func IsSystemConditionType(t ConditionType) bool { case TypeReady, TypeSynced, TypeHealthy: return true } + return false } @@ -150,6 +151,7 @@ type ConditionedStatus struct { func NewConditionedStatus(c ...Condition) *ConditionedStatus { s := &ConditionedStatus{} s.SetConditions(c...) + return s } @@ -171,6 +173,7 @@ func (s *ConditionedStatus) GetCondition(ct ConditionType) Condition { func (s *ConditionedStatus) SetConditions(c ...Condition) { for _, cond := range c { exists := false + for i, existing := range s.Conditions { if existing.Type != cond.Type { continue @@ -184,6 +187,7 @@ func (s *ConditionedStatus) SetConditions(c ...Condition) { s.Conditions[i] = cond exists = true } + if !exists { s.Conditions = append(s.Conditions, cond) } diff --git a/apis/common/v1/connection_details.go b/apis/common/v1/connection_details.go index 9a2ffdf..db77776 100644 --- a/apis/common/v1/connection_details.go +++ b/apis/common/v1/connection_details.go @@ -72,6 +72,7 @@ func (in *ConnectionSecretMetadata) SetOwnerUID(uid types.UID) { if in.Labels == nil { in.Labels = map[string]string{} } + in.Labels[LabelKeyOwnerUID] = string(uid) } @@ -80,6 +81,7 @@ func (in *ConnectionSecretMetadata) GetOwnerUID() string { if u, ok := in.Labels[LabelKeyOwnerUID]; ok { return u } + return "" } diff --git a/apis/common/v1/merge.go b/apis/common/v1/merge.go index 9ad4006..929d8f3 100644 --- a/apis/common/v1/merge.go +++ b/apis/common/v1/merge.go @@ -40,9 +40,11 @@ func (mo *MergeOptions) MergoConfiguration() []func(*mergo.Config) { if mo.KeepMapValues != nil && *mo.KeepMapValues { config = config[:0] } + if mo.AppendSlice != nil && *mo.AppendSlice { config = append(config, mergo.WithAppendSlice) } + return config } diff --git a/apis/common/v1/merge_test.go b/apis/common/v1/merge_test.go index 01a8f31..8f4caa3 100644 --- a/apis/common/v1/merge_test.go +++ b/apis/common/v1/merge_test.go @@ -33,12 +33,15 @@ func (arr mergoOptArr) names() []string { for i, opt := range arr { names[i] = runtime.FuncForPC(reflect.ValueOf(opt).Pointer()).Name() } + sort.Strings(names) + return names } func TestMergoConfiguration(t *testing.T) { valTrue := true + tests := map[string]struct { mo *MergeOptions want mergoOptArr diff --git a/apis/common/v1/resource.go b/apis/common/v1/resource.go index 9919c5f..b76dd66 100644 --- a/apis/common/v1/resource.go +++ b/apis/common/v1/resource.go @@ -101,6 +101,7 @@ func (p *Policy) IsResolutionPolicyOptional() bool { if p == nil || p.Resolution == nil { return false } + return *p.Resolution == ResolutionPolicyOptional } @@ -109,6 +110,7 @@ func (p *Policy) IsResolvePolicyAlways() bool { if p == nil || p.Resolve == nil { return false } + return *p.Resolve == ResolvePolicyAlways } diff --git a/pkg/certificates/certificates.go b/pkg/certificates/certificates.go index e4f3c1d..33565db 100644 --- a/pkg/certificates/certificates.go +++ b/pkg/certificates/certificates.go @@ -36,12 +36,14 @@ const ( 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) } caCertFilePath := filepath.Clean(caPath) + ca, err := os.ReadFile(caCertFilePath) if err != nil { return nil, errors.Wrap(err, errLoadCA) diff --git a/pkg/certificates/certificates_test.go b/pkg/certificates/certificates_test.go index 3c854f5..83aad9a 100644 --- a/pkg/certificates/certificates_test.go +++ b/pkg/certificates/certificates_test.go @@ -27,10 +27,12 @@ func TestLoad(t *testing.T) { certsFolderPath string requireClientValidation bool } + type want struct { err error out *tls.Config } + cases := map[string]struct { reason string args @@ -92,16 +94,16 @@ func TestLoad(t *testing.T) { } for name, tc := range cases { t.Run(name, func(t *testing.T) { - certsFolderPath := tc.args.certsFolderPath - requireClient := tc.args.requireClientValidation + certsFolderPath := tc.certsFolderPath + requireClient := tc.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.err, err, test.EquateErrors()); diff != "" { t.Errorf("\n%s\nLoad(...): -want error, +got error:\n%s", tc.reason, diff) } if requireClient { - if diff := cmp.Diff(tc.want.out.ClientAuth, cfg.ClientAuth); diff != "" { + if diff := cmp.Diff(tc.out.ClientAuth, cfg.ClientAuth); diff != "" { t.Errorf("\n%s\nLoad(...): -want, +got:\n%s", tc.reason, diff) } } diff --git a/pkg/conditions/manager.go b/pkg/conditions/manager.go index 26afa1d..645549f 100644 --- a/pkg/conditions/manager.go +++ b/pkg/conditions/manager.go @@ -68,5 +68,6 @@ func (c *observedGenerationPropagationConditionSet) MarkConditions(condition ... for i := range condition { condition[i].ObservedGeneration = c.o.GetGeneration() } + c.o.SetConditions(condition...) } diff --git a/pkg/conditions/manager_test.go b/pkg/conditions/manager_test.go index 382cf31..2f1b1c5 100644 --- a/pkg/conditions/manager_test.go +++ b/pkg/conditions/manager_test.go @@ -80,6 +80,7 @@ func TestOGConditionSetMark(t *testing.T) { ut := newManaged(42, tt.start...) c := manager.For(ut) c.MarkConditions(tt.mark...) + if diff := cmp.Diff(tt.want, ut.Conditions, test.EquateConditions(), cmpopts.EquateApproxTime(1*time.Second)); diff != "" { t.Errorf("\nReason: %s\n-want, +got:\n%s", tt.reason, diff) } @@ -129,5 +130,6 @@ func newManaged(generation int64, conditions ...xpv1.Condition) *fake.Managed { mg := &fake.Managed{} mg.Generation = generation mg.SetConditions(conditions...) + return mg } diff --git a/pkg/connection/fake/mocks.go b/pkg/connection/fake/mocks.go index 5cd6862..0a397bb 100644 --- a/pkg/connection/fake/mocks.go +++ b/pkg/connection/fake/mocks.go @@ -74,10 +74,13 @@ func (s *StoreConfig) GetObjectKind() schema.ObjectKind { // DeepCopyObject returns a copy of the object as runtime.Object. func (s *StoreConfig) DeepCopyObject() runtime.Object { out := &StoreConfig{} + j, err := json.Marshal(s) if err != nil { panic(err) } + _ = json.Unmarshal(j, out) + return out } diff --git a/pkg/connection/manager.go b/pkg/connection/manager.go index 5263ab0..9e7b30e 100644 --- a/pkg/connection/manager.go +++ b/pkg/connection/manager.go @@ -115,6 +115,7 @@ func (m *DetailsManager) PublishConnection(ctx context.Context, so resource.Conn } changed, err := ss.WriteKeyValues(ctx, store.NewSecret(so, store.KeyValues(conn)), SecretToWriteMustBeOwnedBy(so)) + return changed, errors.Wrap(err, errWriteStore) } @@ -149,6 +150,7 @@ func (m *DetailsManager) FetchConnection(ctx context.Context, so resource.Connec } s := &store.Secret{} + return managed.ConnectionDetails(s.Data), errors.Wrap(ss.ReadKeyValues(ctx, store.ScopedName{Name: p.Name, Scope: so.GetNamespace()}, s), errReadStore) } @@ -185,6 +187,7 @@ func (m *DetailsManager) PropagateConnection(ctx context.Context, to resource.Lo } changed, err := ssTo.WriteKeyValues(ctx, store.NewSecret(to, sFrom.Data), SecretToWriteMustBeOwnedBy(to)) + return changed, errors.Wrap(err, errWriteStore) } @@ -217,5 +220,6 @@ 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 nil } diff --git a/pkg/connection/manager_test.go b/pkg/connection/manager_test.go index 39841b6..f7e29c2 100644 --- a/pkg/connection/manager_test.go +++ b/pkg/connection/manager_test.go @@ -144,10 +144,10 @@ func TestManagerConnectStore(t *testing.T) { } for name, tc := range cases { t.Run(name, func(t *testing.T) { - m := NewDetailsManager(tc.args.c, resourcefake.GVK(&fake.StoreConfig{}), WithStoreBuilder(tc.args.sb)) + m := NewDetailsManager(tc.c, resourcefake.GVK(&fake.StoreConfig{}), WithStoreBuilder(tc.sb)) - _, err := m.connectStore(context.Background(), tc.args.p) - if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + _, err := m.connectStore(context.Background(), tc.p) + if diff := cmp.Diff(tc.err, err, test.EquateErrors()); diff != "" { t.Errorf("\nReason: %s\nm.connectStore(...): -want error, +got error:\n%s", tc.reason, diff) } }) @@ -289,13 +289,14 @@ func TestManagerPublishConnection(t *testing.T) { } for name, tc := range cases { t.Run(name, func(t *testing.T) { - m := NewDetailsManager(tc.args.c, resourcefake.GVK(&fake.StoreConfig{}), WithStoreBuilder(tc.args.sb)) + m := NewDetailsManager(tc.c, resourcefake.GVK(&fake.StoreConfig{}), WithStoreBuilder(tc.sb)) - published, err := m.PublishConnection(context.Background(), tc.args.so, tc.args.conn) - if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + published, err := m.PublishConnection(context.Background(), tc.so, tc.conn) + if diff := cmp.Diff(tc.err, err, test.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 != "" { + + if diff := cmp.Diff(tc.published, published); diff != "" { t.Errorf("\nReason: %s\nm.publishConnection(...): -want published, +got published:\n%s", tc.reason, diff) } }) @@ -489,10 +490,10 @@ func TestManagerUnpublishConnection(t *testing.T) { } for name, tc := range cases { t.Run(name, func(t *testing.T) { - m := NewDetailsManager(tc.args.c, resourcefake.GVK(&fake.StoreConfig{}), WithStoreBuilder(tc.args.sb)) + m := NewDetailsManager(tc.c, resourcefake.GVK(&fake.StoreConfig{}), WithStoreBuilder(tc.sb)) - err := m.UnpublishConnection(context.Background(), tc.args.so, tc.args.conn) - if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + err := m.UnpublishConnection(context.Background(), tc.so, tc.conn) + if diff := cmp.Diff(tc.err, err, test.EquateErrors()); diff != "" { t.Errorf("\nReason: %s\nm.unpublishConnection(...): -want error, +got error:\n%s", tc.reason, diff) } }) @@ -632,13 +633,14 @@ func TestManagerFetchConnection(t *testing.T) { } for name, tc := range cases { t.Run(name, func(t *testing.T) { - m := NewDetailsManager(tc.args.c, resourcefake.GVK(&fake.StoreConfig{}), WithStoreBuilder(tc.args.sb)) + m := NewDetailsManager(tc.c, resourcefake.GVK(&fake.StoreConfig{}), WithStoreBuilder(tc.sb)) - got, err := m.FetchConnection(context.Background(), tc.args.so) - if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + got, err := m.FetchConnection(context.Background(), tc.so) + if diff := cmp.Diff(tc.err, err, test.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 != "" { + + if diff := cmp.Diff(tc.conn, got); diff != "" { t.Errorf("\nReason: %s\nm.FetchConnection(...): -want connDetails, +got connDetails:\n%s", tc.reason, diff) } }) @@ -1213,13 +1215,14 @@ func TestManagerPropagateConnection(t *testing.T) { } for name, tc := range cases { t.Run(name, func(t *testing.T) { - m := NewDetailsManager(tc.args.c, resourcefake.GVK(&fake.StoreConfig{}), WithStoreBuilder(tc.args.sb)) + m := NewDetailsManager(tc.c, resourcefake.GVK(&fake.StoreConfig{}), WithStoreBuilder(tc.sb)) - got, err := m.PropagateConnection(context.Background(), tc.args.to, tc.args.from) - if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + got, err := m.PropagateConnection(context.Background(), tc.to, tc.from) + if diff := cmp.Diff(tc.err, err, test.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 != "" { + + if diff := cmp.Diff(tc.propagated, got); diff != "" { t.Errorf("\nReason: %s\nm.PropagateConnection(...): -want propagated, +got propagated:\n%s", tc.reason, diff) } }) @@ -1231,6 +1234,7 @@ func fakeStoreBuilderFn(ss fake.SecretStore) StoreBuilderFn { if *cfg.Type == fakeStore { return &ss, nil } + return nil, errors.Errorf(errFmtUnknownSecretStore, *cfg.Type) } } diff --git a/pkg/connection/store/kubernetes/store.go b/pkg/connection/store/kubernetes/store.go index 4852986..249f3e1 100644 --- a/pkg/connection/store/kubernetes/store.go +++ b/pkg/connection/store/kubernetes/store.go @@ -83,10 +83,12 @@ func buildClient(ctx context.Context, local client.Client, cfg v1.SecretStoreCon if err != nil { return nil, errors.Wrap(err, errExtractKubernetesAuthCreds) } + config, err := clientcmd.RESTConfigFromKubeConfig(kfg) if err != nil { return nil, errors.Wrap(err, errBuildRestConfig) } + return client.New(config, client.Options{}) } @@ -96,12 +98,14 @@ func (ss *SecretStore) ReadKeyValues(ctx context.Context, n store.ScopedName, s 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) } + s.Data = ks.Data s.Metadata = &v1.ConnectionSecretMetadata{ Labels: ks.Labels, Annotations: ks.Annotations, Type: &ks.Type, } + return nil } @@ -118,6 +122,7 @@ func (ss *SecretStore) WriteKeyValues(ctx context.Context, s *store.Secret, wo . if s.Metadata != nil { ks.Labels = s.Metadata.Labels + ks.Annotations = s.Metadata.Annotations if s.Metadata.Type != nil { ks.Type = *s.Metadata.Type @@ -136,9 +141,11 @@ func (ss *SecretStore) WriteKeyValues(ctx context.Context, s *store.Secret, wo . // The update was not allowed because it was a no-op. return false, nil } + if err != nil { return false, errors.Wrap(err, errApplySecret) } + return true, nil } @@ -156,11 +163,13 @@ func (ss *SecretStore) DeleteKeyValues(ctx context.Context, s *store.Secret, do // deletion, I opted for unifying both instead of adding conditional logic // like add owner references if not remote and not call delete etc. ks := &corev1.Secret{} + err := ss.client.Get(ctx, types.NamespacedName{Name: s.Name, Namespace: ss.namespaceForSecret(s.ScopedName)}, ks) if kerrors.IsNotFound(err) { // Secret already deleted, nothing to do. return nil } + if err != nil { return errors.Wrap(err, errGetSecret) } @@ -175,6 +184,7 @@ func (ss *SecretStore) DeleteKeyValues(ctx context.Context, s *store.Secret, do for k := range s.Data { delete(ks.Data, k) } + if len(s.Data) == 0 || len(ks.Data) == 0 { // Secret is deleted only if: // - No kv to delete specified as input @@ -189,6 +199,7 @@ func (ss *SecretStore) namespaceForSecret(n store.ScopedName) string { if n.Scope == "" { return ss.defaultNamespace } + return n.Scope } @@ -244,6 +255,7 @@ func applyOptions(wo ...store.WriteOption) []resource.ApplyOption { desiredSecret.Data = ds.Data desiredSecret.Labels = ds.Metadata.Labels + desiredSecret.Annotations = ds.Metadata.Annotations if ds.Metadata.Type != nil { desiredSecret.Type = *ds.Metadata.Type @@ -252,5 +264,6 @@ func applyOptions(wo ...store.WriteOption) []resource.ApplyOption { return nil } } + return ao } diff --git a/pkg/connection/store/kubernetes/store_test.go b/pkg/connection/store/kubernetes/store_test.go index 2a1a2c8..85b7d69 100644 --- a/pkg/connection/store/kubernetes/store_test.go +++ b/pkg/connection/store/kubernetes/store_test.go @@ -71,6 +71,7 @@ func TestSecretStoreReadKeyValues(t *testing.T) { client resource.ClientApplicator n store.ScopedName } + type want struct { result store.KeyValues err error @@ -139,17 +140,18 @@ func TestSecretStoreReadKeyValues(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { ss := &SecretStore{ - client: tc.args.client, + client: tc.client, } 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 != "" { + s.ScopedName = tc.n + + err := ss.ReadKeyValues(context.Background(), tc.n, s) + if diff := cmp.Diff(tc.err, err, test.EquateErrors()); diff != "" { t.Errorf("\n%s\nss.ReadKeyValues(...): -want error, +got error:\n%s", tc.reason, diff) } - if diff := cmp.Diff(tc.want.result, s.Data); diff != "" { + if diff := cmp.Diff(tc.result, s.Data); diff != "" { t.Errorf("\n%s\nss.ReadKeyValues(...): -want, +got:\n%s", tc.reason, diff) } }) @@ -158,6 +160,7 @@ func TestSecretStoreReadKeyValues(t *testing.T) { func TestSecretStoreWriteKeyValues(t *testing.T) { secretTypeOpaque := corev1.SecretTypeOpaque + type args struct { client resource.ClientApplicator defaultNamespace string @@ -165,6 +168,7 @@ func TestSecretStoreWriteKeyValues(t *testing.T) { wo []store.WriteOption } + type want struct { changed bool err error @@ -465,14 +469,16 @@ func TestSecretStoreWriteKeyValues(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { ss := &SecretStore{ - client: tc.args.client, - defaultNamespace: tc.args.defaultNamespace, + client: tc.client, + defaultNamespace: tc.defaultNamespace, } - changed, err := ss.WriteKeyValues(context.Background(), tc.args.secret, tc.args.wo...) - if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + + changed, err := ss.WriteKeyValues(context.Background(), tc.secret, tc.wo...) + if diff := cmp.Diff(tc.err, err, test.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 != "" { + + if diff := cmp.Diff(tc.changed, changed); diff != "" { t.Errorf("\n%s\nss.WriteKeyValues(...): -want changed, +got changed:\n%s", tc.reason, diff) } }) @@ -487,6 +493,7 @@ func TestSecretStoreDeleteKeyValues(t *testing.T) { do []store.DeleteOption } + type want struct { err error } @@ -655,11 +662,12 @@ func TestSecretStoreDeleteKeyValues(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { ss := &SecretStore{ - client: tc.args.client, - defaultNamespace: tc.args.defaultNamespace, + client: tc.client, + defaultNamespace: tc.defaultNamespace, } - err := ss.DeleteKeyValues(context.Background(), tc.args.secret, tc.args.do...) - if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + + err := ss.DeleteKeyValues(context.Background(), tc.secret, tc.do...) + if diff := cmp.Diff(tc.err, err, test.EquateErrors()); diff != "" { t.Errorf("\n%s\nss.DeleteKeyValues(...): -want error, +got error:\n%s", tc.reason, diff) } }) @@ -671,6 +679,7 @@ func TestNewSecretStore(t *testing.T) { client resource.ClientApplicator cfg v1.SecretStoreConfig } + type want struct { err error } @@ -836,8 +845,8 @@ 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 != "" { + _, err := NewSecretStore(context.Background(), tc.client, nil, tc.cfg) + if diff := cmp.Diff(tc.err, err, test.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 3594f17..8c29fc8 100644 --- a/pkg/connection/store/plugin/store.go +++ b/pkg/connection/store/plugin/store.go @@ -53,6 +53,7 @@ type SecretStore struct { // NewSecretStore returns a new External SecretStore. func NewSecretStore(_ context.Context, kube client.Client, tcfg *tls.Config, cfg v1.SecretStoreConfig) (*SecretStore, error) { creds := credentials.NewTLS(tcfg) + conn, err := grpc.NewClient(cfg.Plugin.Endpoint, grpc.WithTransportCredentials(creds)) if err != nil { return nil, errors.Wrapf(err, errFmtCannotDial, cfg.Plugin.Endpoint) @@ -81,6 +82,7 @@ func (ss *SecretStore) ReadKeyValues(ctx context.Context, n store.ScopedName, s } respSecretData := respSecret.GetData() + s.Data = make(map[string][]byte, len(respSecretData)) for d := range respSecretData { s.Data[d] = respSecretData[d] @@ -89,6 +91,7 @@ func (ss *SecretStore) ReadKeyValues(ctx context.Context, n store.ScopedName, s respSecretMetadata := respSecret.GetMetadata() if len(respSecretMetadata) != 0 { s.Metadata = new(v1.ConnectionSecretMetadata) + s.Metadata.Labels = make(map[string]string, len(respSecretMetadata)) for k, v := range respSecretMetadata { s.Metadata.Labels[k] = v @@ -102,6 +105,7 @@ func (ss *SecretStore) ReadKeyValues(ctx context.Context, n store.ScopedName, s func (ss *SecretStore) WriteKeyValues(ctx context.Context, s *store.Secret, _ ...store.WriteOption) (changed bool, err error) { sec := &essproto.Secret{} sec.ScopedName = ss.getScopedName(s.ScopedName) + sec.Data = make(map[string][]byte, len(s.Data)) for k, v := range s.Data { sec.Data[k] = v @@ -141,5 +145,6 @@ func (ss *SecretStore) getScopedName(n store.ScopedName) string { if n.Scope == "" { n.Scope = ss.defaultScope } + return filepath.Join(n.Scope, n.Name) } diff --git a/pkg/connection/store/plugin/store_test.go b/pkg/connection/store/plugin/store_test.go index fcf6802..659517a 100644 --- a/pkg/connection/store/plugin/store_test.go +++ b/pkg/connection/store/plugin/store_test.go @@ -44,10 +44,12 @@ func TestReadKeyValues(t *testing.T) { sn store.ScopedName client ess.ExternalSecretStorePluginServiceClient } + type want struct { out *store.Secret err error } + cases := map[string]struct { reason string args @@ -123,7 +125,7 @@ func TestReadKeyValues(t *testing.T) { t.Run(name, func(t *testing.T) { ctx := context.Background() ss := &SecretStore{ - client: tc.args.client, + client: tc.client, config: &v1.Config{ APIVersion: "v1alpha1", Kind: "VaultConfig", @@ -132,13 +134,12 @@ func TestReadKeyValues(t *testing.T) { } s := &store.Secret{} - err := ss.ReadKeyValues(ctx, tc.args.sn, s) - - if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + err := ss.ReadKeyValues(ctx, tc.sn, s) + if diff := cmp.Diff(tc.err, err, test.EquateErrors()); diff != "" { t.Errorf("\n%s\nss.ReadKeyValues(...): -want error, +got error:\n%s", tc.reason, diff) } - if diff := cmp.Diff(tc.want.out, s); diff != "" { + if diff := cmp.Diff(tc.out, s); diff != "" { t.Errorf("\n%s\nss.ReadKeyValues(...): -want, +got:\n%s", tc.reason, diff) } }) @@ -149,10 +150,12 @@ func TestWriteKeyValues(t *testing.T) { type args struct { client ess.ExternalSecretStorePluginServiceClient } + type want struct { isChanged bool err error } + cases := map[string]struct { reason string args @@ -195,7 +198,7 @@ func TestWriteKeyValues(t *testing.T) { t.Run(name, func(t *testing.T) { ctx := context.Background() ss := &SecretStore{ - client: tc.args.client, + client: tc.client, config: &v1.Config{ APIVersion: "v1alpha1", Kind: "VaultConfig", @@ -205,12 +208,11 @@ func TestWriteKeyValues(t *testing.T) { s := &store.Secret{} isChanged, err := ss.WriteKeyValues(ctx, s) - - if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.err, err, test.EquateErrors()); diff != "" { t.Errorf("\n%s\nss.WriteKeyValues(...): -want error, +got error:\n%s", tc.reason, diff) } - if diff := cmp.Diff(tc.want.isChanged, isChanged); diff != "" { + if diff := cmp.Diff(tc.isChanged, isChanged); diff != "" { t.Errorf("\n%s\nss.WriteKeyValues(...): -want, +got:\n%s", tc.reason, diff) } }) @@ -221,9 +223,11 @@ func TestDeleteKeyValues(t *testing.T) { type args struct { client ess.ExternalSecretStorePluginServiceClient } + type want struct { err error } + cases := map[string]struct { reason string args @@ -260,7 +264,7 @@ func TestDeleteKeyValues(t *testing.T) { t.Run(name, func(t *testing.T) { ctx := context.Background() ss := &SecretStore{ - client: tc.args.client, + client: tc.client, config: &v1.Config{ APIVersion: "v1alpha1", Kind: "VaultConfig", @@ -270,8 +274,7 @@ func TestDeleteKeyValues(t *testing.T) { s := &store.Secret{} err := ss.DeleteKeyValues(ctx, s) - - if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.err, err, test.EquateErrors()); diff != "" { t.Errorf("\n%s\nss.DeletKeyValues(...): -want error, +got error:\n%s", tc.reason, diff) } }) diff --git a/pkg/connection/store/store.go b/pkg/connection/store/store.go index f383002..9eee14b 100644 --- a/pkg/connection/store/store.go +++ b/pkg/connection/store/store.go @@ -43,6 +43,7 @@ type ScopedName struct { // A Secret is an entity representing a set of sensitive Key Values. type Secret struct { ScopedName + Metadata *v1.ConnectionSecretMetadata Data KeyValues } @@ -53,11 +54,14 @@ func NewSecret(so SecretOwner, data KeyValues) *Secret { if so.GetPublishConnectionDetailsTo() == nil { return nil } + p := so.GetPublishConnectionDetailsTo() if p.Metadata == nil { p.Metadata = &v1.ConnectionSecretMetadata{} } + p.Metadata.SetOwnerUID(so.GetUID()) + return &Secret{ ScopedName: ScopedName{ Name: p.Name, @@ -73,6 +77,7 @@ func (s *Secret) GetOwner() string { if s.Metadata == nil { return "" } + return s.Metadata.GetOwnerUID() } @@ -81,6 +86,7 @@ func (s *Secret) GetLabels() map[string]string { if s.Metadata == nil { return nil } + return s.Metadata.Labels } diff --git a/pkg/connection/stores.go b/pkg/connection/stores.go index 924d800..0185495 100644 --- a/pkg/connection/stores.go +++ b/pkg/connection/stores.go @@ -43,5 +43,6 @@ 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) } diff --git a/pkg/errors/errors.go b/pkg/errors/errors.go index 2c6c493..1e18833 100644 --- a/pkg/errors/errors.go +++ b/pkg/errors/errors.go @@ -85,6 +85,7 @@ func WithMessage(err error, message string) error { if err == nil { return nil } + return fmt.Errorf("%s: %w", message, err) } @@ -94,6 +95,7 @@ func WithMessagef(err error, format string, args ...any) error { if err == nil { return nil } + return fmt.Errorf("%s: %w", fmt.Sprintf(format, args...), err) } @@ -120,6 +122,7 @@ func Cause(err error) error { if !ok { return err } + err = w.Unwrap() } @@ -146,6 +149,7 @@ func Join(errs ...error) MultiError { if err == nil { return nil } + return multiError{aggregate: err} } diff --git a/pkg/errors/errors_test.go b/pkg/errors/errors_test.go index 479a724..a762593 100644 --- a/pkg/errors/errors_test.go +++ b/pkg/errors/errors_test.go @@ -29,6 +29,7 @@ func TestWrap(t *testing.T) { err error message string } + cases := map[string]struct { args args want error @@ -65,6 +66,7 @@ func TestWrapf(t *testing.T) { message string args []any } + cases := map[string]struct { args args want error diff --git a/pkg/errors/reconcile.go b/pkg/errors/reconcile.go index 2417d64..562d5c6 100644 --- a/pkg/errors/reconcile.go +++ b/pkg/errors/reconcile.go @@ -30,6 +30,7 @@ func SilentlyRequeueOnConflict(result reconcile.Result, err error) (reconcile.Re if kerrors.IsConflict(Cause(err)) { return reconcile.Result{Requeue: true}, nil } + return result, err } diff --git a/pkg/errors/reconcile_test.go b/pkg/errors/reconcile_test.go index 02b8241..a93a58c 100644 --- a/pkg/errors/reconcile_test.go +++ b/pkg/errors/reconcile_test.go @@ -33,10 +33,12 @@ func TestSilentlyRequeueOnConflict(t *testing.T) { result reconcile.Result err error } + type want struct { result reconcile.Result err error } + tests := []struct { reason string args args @@ -91,6 +93,7 @@ func TestSilentlyRequeueOnConflict(t *testing.T) { if diff := cmp.Diff(tt.want.err, err, test.EquateErrors()); diff != "" { t.Errorf("\n%s\nIgnoreConflict(...): -want error, +got error:\n%s", tt.reason, diff) } + if diff := cmp.Diff(tt.want.result, got); diff != "" { t.Errorf("\n%s\nIgnoreConflict(...): -want result, +got result:\n%s", tt.reason, diff) } diff --git a/pkg/event/event.go b/pkg/event/event.go index a0c8da0..b193942 100644 --- a/pkg/event/event.go +++ b/pkg/event/event.go @@ -52,6 +52,7 @@ func Normal(r Reason, message string, keysAndValues ...string) Event { Annotations: map[string]string{}, } sliceMap(keysAndValues, e.Annotations) + return e } @@ -64,6 +65,7 @@ func Warning(r Reason, err error, keysAndValues ...string) Event { Annotations: map[string]string{}, } sliceMap(keysAndValues, e.Annotations) + return e } @@ -97,7 +99,9 @@ func (r *APIRecorder) WithAnnotations(keysAndValues ...string) Recorder { for k, v := range r.annotations { ar.annotations[k] = v } + sliceMap(keysAndValues, ar.annotations) + return ar } diff --git a/pkg/event/event_test.go b/pkg/event/event_test.go index b33e500..995bee9 100644 --- a/pkg/event/event_test.go +++ b/pkg/event/event_test.go @@ -28,6 +28,7 @@ func TestSliceMap(t *testing.T) { from []string to map[string]string } + cases := map[string]struct { reason string args args diff --git a/pkg/feature/feature.go b/pkg/feature/feature.go index e88a5a0..a073802 100644 --- a/pkg/feature/feature.go +++ b/pkg/feature/feature.go @@ -33,9 +33,11 @@ type Flags struct { // Enable a feature flag. func (fs *Flags) Enable(f Flag) { fs.m.Lock() + if fs.enabled == nil { fs.enabled = make(map[Flag]bool) } + fs.enabled[f] = true fs.m.Unlock() } @@ -45,7 +47,9 @@ func (fs *Flags) Enabled(f Flag) bool { if fs == nil { return false } + fs.m.RLock() defer fs.m.RUnlock() + return fs.enabled[f] } diff --git a/pkg/fieldpath/fieldpath.go b/pkg/fieldpath/fieldpath.go index aa633fb..d917326 100644 --- a/pkg/fieldpath/fieldpath.go +++ b/pkg/fieldpath/fieldpath.go @@ -82,6 +82,7 @@ func (sg Segments) String() string { b.WriteString(fmt.Sprintf("[%s]", s.Field)) continue } + b.WriteString(fmt.Sprintf(".%s", s.Field)) case SegmentIndex: b.WriteString(fmt.Sprintf("[%d]", s.Index)) @@ -120,6 +121,7 @@ func Parse(path string) (Segments, error) { go l.run() segments := make(Segments, 0, 1) + for i := range l.items { switch i.typ { //nolint:exhaustive // We're only worried about names, not separators. case itemField: @@ -130,6 +132,7 @@ func Parse(path string) (Segments, error) { return nil, errors.Errorf("%s at position %d", i.val, i.pos) } } + return segments, nil } @@ -174,6 +177,7 @@ func (l *lexer) run() { for state := lexField; state != nil; { state = state(l) } + close(l.items) } @@ -182,7 +186,9 @@ func (l *lexer) emit(t itemType) { if l.pos <= l.start { return } + l.items <- item{typ: t, pos: l.start, val: l.input[l.start:l.pos]} + l.start = l.pos } @@ -202,12 +208,14 @@ func lexField(l *lexer) stateFn { case leftBracket: l.pos += i l.emit(itemField) + return lexLeftBracket // A period indicates the end of the field name. case period: l.pos += i l.emit(itemField) + return lexPeriod } } @@ -216,6 +224,7 @@ func lexField(l *lexer) stateFn { l.pos = len(l.input) l.emit(itemField) l.emit(itemEOL) + return nil } @@ -234,6 +243,7 @@ func lexPeriod(l *lexer) stateFn { if r == period { return l.errorf(l.pos, "unexpected %q", period) } + if r == leftBracket { return l.errorf(l.pos, "unexpected %q", leftBracket) } @@ -249,6 +259,7 @@ func lexLeftBracket(l *lexer) stateFn { l.pos += utf8.RuneLen(leftBracket) l.emit(itemLeftBracket) + return lexFieldOrIndex } @@ -272,11 +283,13 @@ func lexFieldOrIndex(l *lexer) stateFn { // Periods are not considered field separators when we're inside brackets. l.pos += rbi l.emit(itemFieldOrIndex) + return lexRightBracket } func lexRightBracket(l *lexer) stateFn { l.pos += utf8.RuneLen(rightBracket) l.emit(itemRightBracket) + return lexField } diff --git a/pkg/fieldpath/fieldpath_test.go b/pkg/fieldpath/fieldpath_test.go index a366278..c2ac346 100644 --- a/pkg/fieldpath/fieldpath_test.go +++ b/pkg/fieldpath/fieldpath_test.go @@ -299,6 +299,7 @@ func TestParse(t *testing.T) { if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); 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 != "" { t.Errorf("\nParse(%s): %s: -want, +got:\n%s", tc.path, tc.reason, diff) } diff --git a/pkg/fieldpath/merge.go b/pkg/fieldpath/merge.go index dab88d4..2317e69 100644 --- a/pkg/fieldpath/merge.go +++ b/pkg/fieldpath/merge.go @@ -57,6 +57,7 @@ func merge(dst, src any, mergeOptions *xpv1.MergeOptions) (any, error) { // because mergo currently supports merging only maps or structs, // we wrap the argument to be passed to mergo.Merge in a map. const keyArg = "arg" + argWrap := func(arg any) map[string]any { return map[string]any{ keyArg: arg, @@ -77,6 +78,7 @@ func merge(dst, src any, mergeOptions *xpv1.MergeOptions) (any, error) { if err := mergo.Merge(&mDst, argWrap(src), mergeOptions.MergoConfiguration()...); err != nil { return nil, errors.Wrap(err, errInvalidMerge) } + return mDst[keyArg], nil } @@ -85,9 +87,11 @@ func removeSourceDuplicates(dst, src any) any { if sliceDst.Kind() == reflect.Ptr { sliceDst = sliceDst.Elem() } + if sliceSrc.Kind() == reflect.Ptr { sliceSrc = sliceSrc.Elem() } + if sliceDst.Kind() != reflect.Slice || sliceSrc.Kind() != reflect.Slice { return src } @@ -95,6 +99,7 @@ func removeSourceDuplicates(dst, src any) any { result := reflect.New(sliceSrc.Type()).Elem() // we will not modify src for i := range sliceSrc.Len() { itemSrc := sliceSrc.Index(i) + found := false for j := 0; j < sliceDst.Len() && !found; j++ { // if src item is found in the dst array @@ -102,10 +107,12 @@ func removeSourceDuplicates(dst, src any) any { found = true } } + if !found { // then put src item into result result = reflect.Append(result, itemSrc) } } + return result.Interface() } diff --git a/pkg/fieldpath/merge_test.go b/pkg/fieldpath/merge_test.go index 5753a39..55530c2 100644 --- a/pkg/fieldpath/merge_test.go +++ b/pkg/fieldpath/merge_test.go @@ -36,6 +36,7 @@ func TestMergeValue(t *testing.T) { valSrc2 = "e1-from-source-2" valDst = "e1-from-destination" ) + formatArr := func(arr []string) string { return fmt.Sprintf(`{"%s": ["%s"]}`, pathTest, strings.Join(arr, `", "`)) } @@ -59,15 +60,18 @@ func TestMergeValue(t *testing.T) { type fields struct { object map[string]any } + type args struct { path string value any mo *xpv1.MergeOptions } + type want struct { serialized string err error } + tests := map[string]struct { reason string fields fields @@ -199,11 +203,13 @@ func TestMergeValue(t *testing.T) { p := &Paved{ 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 != "" { t.Fatalf("\np.MergeValue(%s, %v): %s: -want error, +got error:\n%s", tc.args.path, tc.args.value, tc.reason, diff) } + if diff := cmp.Diff(want, p.object); diff != "" { t.Fatalf("\np.MergeValue(%s, %v): %s: -want, +got:\n%s", tc.args.path, tc.args.value, tc.reason, diff) diff --git a/pkg/fieldpath/paved.go b/pkg/fieldpath/paved.go index 5cd3020..7bb3a32 100644 --- a/pkg/fieldpath/paved.go +++ b/pkg/fieldpath/paved.go @@ -44,6 +44,7 @@ func IsNotFound(err error) bool { _, ok := cause.(interface { IsNotFound() bool }) + return ok } @@ -100,6 +101,7 @@ func (p *Paved) UnstructuredContent() map[string]any { if p.object == nil { return make(map[string]any) } + return p.object } @@ -115,18 +117,22 @@ func (p *Paved) getValue(s Segments) (any, error) { func getValueFromInterface(it any, s Segments) (any, error) { for i, current := range s { final := i == len(s)-1 + switch current.Type { case SegmentIndex: array, ok := it.([]any) if !ok { return nil, errors.Errorf("%s: not an array", s[:i]) } + if current.Index >= uint(len(array)) { return nil, notFoundError{errors.Errorf("%s: no such element", s[:i+1])} } + if final { return array[current.Index], nil } + it = array[current.Index] case SegmentField: switch object := it.(type) { @@ -135,9 +141,11 @@ func getValueFromInterface(it any, s Segments) (any, error) { if !ok { return nil, notFoundError{errors.Errorf("%s: no such field", s[:i+1])} } + if final { return v, nil } + it = object[current.Field] case nil: return nil, notFoundError{errors.Errorf("%s: expected map, got nil", s[:i])} @@ -165,14 +173,17 @@ func (p *Paved) ExpandWildcards(path string) ([]string, error) { if err != nil { return nil, errors.Wrapf(err, "cannot parse path %q", path) } + segmentsArray, err := expandWildcards(p.object, segments) if err != nil { return nil, errors.Wrapf(err, "cannot expand wildcards for segments: %q", segments) } + paths := make([]string, len(segmentsArray)) for i, s := range segmentsArray { paths[i] = s.String() } + return paths, nil } @@ -180,7 +191,9 @@ func expandWildcards(data any, segments Segments) ([]Segments, error) { //nolint // Even complexity turns out to be high, it is mostly because we have duplicate // logic for arrays and maps and a couple of error handling. var res []Segments + it := data + for i, current := range segments { // wildcards are regular fields with "*" as string if current.Type == SegmentField && current.Field == wildcard { @@ -190,10 +203,12 @@ func expandWildcards(data any, segments Segments) ([]Segments, error) { //nolint expanded := make(Segments, len(segments)) copy(expanded, segments) expanded = append(append(expanded[:i], FieldOrIndex(strconv.Itoa(ix))), expanded[i+1:]...) + r, err := expandWildcards(data, expanded) if err != nil { return nil, errors.Wrapf(err, "%q: cannot expand wildcards", expanded) } + res = append(res, r...) } case map[string]any: @@ -201,10 +216,12 @@ func expandWildcards(data any, segments Segments) ([]Segments, error) { //nolint expanded := make(Segments, len(segments)) copy(expanded, segments) expanded = append(append(expanded[:i], Field(k)), expanded[i+1:]...) + r, err := expandWildcards(data, expanded) if err != nil { return nil, errors.Wrapf(err, "%q: cannot expand wildcards", expanded) } + res = append(res, r...) } case nil: @@ -212,17 +229,22 @@ func expandWildcards(data any, segments Segments) ([]Segments, error) { //nolint default: return nil, errors.Errorf("%q: unexpected wildcard usage", segments[:i]) } + return res, nil } + var err error + it, err = getValueFromInterface(data, segments[:i+1]) if IsNotFound(err) { return nil, nil } + if err != nil { return nil, err } } + return append(res, segments), nil } @@ -242,10 +264,12 @@ func (p *Paved) GetValueInto(path string, out any) error { if err != nil { return err } + js, err := json.Marshal(val) if err != nil { return errors.Wrap(err, "cannot marshal value to JSON") } + return errors.Wrap(json.Unmarshal(js, out), "cannot unmarshal value from JSON") } @@ -260,6 +284,7 @@ func (p *Paved) GetString(path string) (string, error) { if !ok { return "", errors.Errorf("%s: not a string", path) } + return s, nil } @@ -281,6 +306,7 @@ func (p *Paved) GetStringArray(path string) ([]string, error) { if !ok { return nil, errors.Errorf("%s: not an array of strings", path) } + sa[i] = s } @@ -305,6 +331,7 @@ func (p *Paved) GetStringObject(path string) (map[string]string, error) { if !ok { return nil, errors.Errorf("%s: not an object with string field values", path) } + so[k] = s } @@ -322,6 +349,7 @@ func (p *Paved) GetBool(path string) (bool, error) { if !ok { return false, errors.Errorf("%s: not a bool", path) } + return b, nil } @@ -336,6 +364,7 @@ func (p *Paved) GetInteger(path string) (int64, error) { if !ok { return 0, errors.Errorf("%s: not a (int64) number", path) } + return f, nil } @@ -354,6 +383,7 @@ func (p *Paved) setValue(s Segments, value any) error { } var in any = p.object + for i, current := range s { final := i == len(s)-1 @@ -393,13 +423,16 @@ func (p *Paved) setValue(s Segments, value any) error { func toValidJSON(value any) (any, error) { var v any + j, err := json.Marshal(value) if err != nil { return nil, errors.Wrap(err, "cannot marshal value to JSON") } + if err := json.Unmarshal(j, &v); err != nil { return nil, errors.Wrap(err, "cannot unmarshal value from JSON") } + return v, nil } @@ -413,6 +446,7 @@ func prepareElement(array []any, current, next Segment) { case SegmentField: array[current.Index] = make(map[string]any) } + return } @@ -444,6 +478,7 @@ func prepareField(object map[string]any, current, next Segment) { case SegmentField: object[current.Field] = make(map[string]any) } + return } @@ -471,6 +506,7 @@ func (p *Paved) SetValue(path string, value any) error { if err != nil { return errors.Wrapf(err, "cannot parse path %q", path) } + return p.setValue(segments, value) } @@ -478,11 +514,13 @@ func (p *Paved) validateSegments(s Segments) error { if !p.maxFieldPathIndexEnabled() { return nil } + for _, segment := range s { if segment.Type == SegmentIndex && segment.Index > p.maxFieldPathIndex { return errors.Errorf("index %v is greater than max allowed index %d", segment.Index, p.maxFieldPathIndex) } } + return nil } @@ -511,6 +549,7 @@ func (p *Paved) DeleteField(path string) error { if err != nil { return errors.Wrapf(err, "cannot parse path %q", path) } + return p.delete(segments) } @@ -522,10 +561,14 @@ func (p *Paved) delete(segments Segments) error { //nolint:gocognit // See note if err != nil { return errors.Wrapf(err, "cannot delete %s", segments) } + p.object = o.(map[string]any) //nolint:forcetypeassert // We're deleting from the root of the paved object, which is always a map[string]any. + return nil } + var in any = p.object + for i, current := range segments { // beforeLast is true for the element before the last one because // slices cannot be changed in place and Go does not allow @@ -535,6 +578,7 @@ func (p *Paved) delete(segments Segments) error { //nolint:gocognit // See note // until the element before the last one as opposed to // Set/Get functions in this file. beforeLast := i == len(segments)-2 + switch current.Type { case SegmentIndex: array, ok := in.([]any) @@ -552,7 +596,9 @@ func (p *Paved) delete(segments Segments) error { //nolint:gocognit // See note if err != nil { return errors.Wrapf(err, "cannot delete %s", segments) } + array[current.Index] = o + return nil } @@ -573,13 +619,16 @@ func (p *Paved) delete(segments Segments) error { //nolint:gocognit // See note if err != nil { return errors.Wrapf(err, "cannot delete %s", segments) } + object[current.Field] = o + return nil } in = object[current.Field] } } + return nil } @@ -593,20 +642,26 @@ func deleteField(obj any, s Segment) (any, error) { if !ok { return nil, errors.New("not an array") } + if len(array) == 0 || uint(len(array)) <= s.Index { return array, nil } + for i := s.Index; i < uint(len(array))-1; i++ { array[i] = array[i+1] } + return array[:len(array)-1], nil case SegmentField: object, ok := obj.(map[string]any) if !ok { return nil, errors.New("not an object") } + delete(object, s.Field) + return object, nil } + return nil, nil } diff --git a/pkg/fieldpath/paved_test.go b/pkg/fieldpath/paved_test.go index d82568d..6a13333 100644 --- a/pkg/fieldpath/paved_test.go +++ b/pkg/fieldpath/paved_test.go @@ -68,6 +68,7 @@ func TestGetValue(t *testing.T) { value any err error } + cases := map[string]struct { reason string path string @@ -181,6 +182,7 @@ func TestGetValue(t *testing.T) { if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); 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 != "" { t.Errorf("\np.GetValue(%s): %s: -want, +got:\n%s", tc.path, tc.reason, diff) } @@ -201,10 +203,12 @@ func TestGetValueInto(t *testing.T) { path string out any } + type want struct { out any err error } + cases := map[string]struct { reason string data []byte @@ -257,6 +261,7 @@ func TestGetValueInto(t *testing.T) { if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); 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 != "" { t.Errorf("\np.GetValueInto(%s): %s: -want, +got:\n%s", tc.args.path, tc.reason, diff) } @@ -269,6 +274,7 @@ func TestGetString(t *testing.T) { value string err error } + cases := map[string]struct { reason string path string @@ -310,6 +316,7 @@ func TestGetString(t *testing.T) { if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); 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 != "" { t.Errorf("\np.GetString(%s): %s: -want, +got:\n%s", tc.path, tc.reason, diff) } @@ -322,6 +329,7 @@ func TestGetStringArray(t *testing.T) { value []string err error } + cases := map[string]struct { reason string path string @@ -371,6 +379,7 @@ func TestGetStringArray(t *testing.T) { if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); 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 != "" { t.Errorf("\np.GetStringArray(%s): %s: -want, +got:\n%s", tc.path, tc.reason, diff) } @@ -383,6 +392,7 @@ func TestGetStringObject(t *testing.T) { value map[string]string err error } + cases := map[string]struct { reason string path string @@ -432,6 +442,7 @@ func TestGetStringObject(t *testing.T) { if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); 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 != "" { t.Errorf("\np.GetStringObject(%s): %s: -want, +got:\n%s", tc.path, tc.reason, diff) } @@ -444,6 +455,7 @@ func TestGetBool(t *testing.T) { value bool err error } + cases := map[string]struct { reason string path string @@ -485,6 +497,7 @@ func TestGetBool(t *testing.T) { if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); 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 != "" { t.Errorf("\np.GetBool(%s): %s: -want, +got:\n%s", tc.path, tc.reason, diff) } @@ -497,6 +510,7 @@ func TestGetInteger(t *testing.T) { value int64 err error } + cases := map[string]struct { reason string path string @@ -538,6 +552,7 @@ func TestGetInteger(t *testing.T) { if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); 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 != "" { t.Errorf("\np.GetNumber(%s): %s: -want, +got:\n%s", tc.path, tc.reason, diff) } @@ -551,10 +566,12 @@ func TestSetValue(t *testing.T) { value any opts []PavedOption } + type want struct { object map[string]any err error } + cases := map[string]struct { reason string data []byte @@ -814,6 +831,7 @@ func TestSetValue(t *testing.T) { if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); 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 != "" { t.Fatalf("\np.SetValue(%s, %v): %s: -want, +got:\n%s", tc.args.path, tc.args.value, tc.reason, diff) } @@ -826,6 +844,7 @@ func TestExpandWildcards(t *testing.T) { expanded []string err error } + cases := map[string]struct { reason string path string @@ -979,6 +998,7 @@ func TestExpandWildcards(t *testing.T) { if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); 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 { return x < y })); diff != "" { @@ -992,10 +1012,12 @@ func TestDeleteField(t *testing.T) { type args struct { path string } + type want struct { object map[string]any err error } + cases := map[string]struct { reason string data []byte @@ -1267,6 +1289,7 @@ func TestDeleteField(t *testing.T) { if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); 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 != "" { t.Fatalf("\np.DeleteField(%s): %s: -want, +got:\n%s", tc.args.path, tc.reason, diff) } diff --git a/pkg/logging/klog.go b/pkg/logging/klog.go index 8e85e68..65477cd 100644 --- a/pkg/logging/klog.go +++ b/pkg/logging/klog.go @@ -49,6 +49,7 @@ func (l *requestThrottlingFilter) klogToLogrLevel(klogLvl int) int { if klogLvl >= 3 { return klogLvl - 3 } + return 0 } diff --git a/pkg/meta/meta.go b/pkg/meta/meta.go index 7528ea0..f8494d3 100644 --- a/pkg/meta/meta.go +++ b/pkg/meta/meta.go @@ -70,6 +70,7 @@ const ( // See https://github.com/crossplane/crossplane-runtime/issues/49 func ReferenceTo(o metav1.Object, of schema.GroupVersionKind) *corev1.ObjectReference { v, k := of.ToAPIVersionAndKind() + return &corev1.ObjectReference{ APIVersion: v, Kind: k, @@ -83,6 +84,7 @@ func ReferenceTo(o metav1.Object, of schema.GroupVersionKind) *corev1.ObjectRefe // presumed to be of the supplied group, version, and kind. func TypedReferenceTo(o metav1.Object, of schema.GroupVersionKind) *xpv1.TypedReference { v, k := of.ToAPIVersionAndKind() + return &xpv1.TypedReference{ APIVersion: v, Kind: k, @@ -108,6 +110,7 @@ func AsController(r *xpv1.TypedReference) metav1.OwnerReference { ref := AsOwner(r) ref.Controller = &t ref.BlockOwnerDeletion = &t + return ref } @@ -139,9 +142,11 @@ func AddOwnerReference(o metav1.Object, r metav1.OwnerReference) { if refs[i].UID == r.UID { refs[i] = r o.SetOwnerReferences(refs) + return } } + o.SetOwnerReferences(append(refs, r)) } @@ -154,6 +159,7 @@ func AddControllerReference(o metav1.Object, r metav1.OwnerReference) error { } AddOwnerReference(o, r) + return nil } @@ -165,6 +171,7 @@ func AddFinalizer(o metav1.Object, finalizer string) { return } } + o.SetFinalizers(append(f, finalizer)) } @@ -176,6 +183,7 @@ func RemoveFinalizer(o metav1.Object, finalizer string) { f = append(f[:i], f[i+1:]...) } } + o.SetFinalizers(f) } @@ -187,6 +195,7 @@ func FinalizerExists(o metav1.Object, finalizer string) bool { return true } } + return false } @@ -197,9 +206,11 @@ func AddLabels(o metav1.Object, labels map[string]string) { o.SetLabels(labels) return } + for k, v := range labels { l[k] = v } + o.SetLabels(l) } @@ -209,9 +220,11 @@ func RemoveLabels(o metav1.Object, labels ...string) { if l == nil { return } + for _, k := range labels { delete(l, k) } + o.SetLabels(l) } @@ -222,9 +235,11 @@ func AddAnnotations(o metav1.Object, annotations map[string]string) { o.SetAnnotations(annotations) return } + for k, v := range annotations { a[k] = v } + o.SetAnnotations(a) } @@ -234,9 +249,11 @@ func RemoveAnnotations(o metav1.Object, annotations ...string) { if a == nil { return } + for _, k := range annotations { delete(a, k) } + o.SetAnnotations(a) } @@ -267,10 +284,12 @@ func SetExternalName(o metav1.Object, name string) { // was most recently pending creation. func GetExternalCreatePending(o metav1.Object) time.Time { a := o.GetAnnotations()[AnnotationKeyExternalCreatePending] + t, err := time.Parse(time.RFC3339, a) if err != nil { return time.Time{} } + return t } @@ -284,10 +303,12 @@ func SetExternalCreatePending(o metav1.Object, t time.Time) { // was most recently created. func GetExternalCreateSucceeded(o metav1.Object) time.Time { a := o.GetAnnotations()[AnnotationKeyExternalCreateSucceeded] + t, err := time.Parse(time.RFC3339, a) if err != nil { return time.Time{} } + return t } @@ -301,10 +322,12 @@ func SetExternalCreateSucceeded(o metav1.Object, t time.Time) { // recently failed to create. func GetExternalCreateFailed(o metav1.Object) time.Time { a := o.GetAnnotations()[AnnotationKeyExternalCreateFailed] + t, err := time.Parse(time.RFC3339, a) if err != nil { return time.Time{} } + return t } @@ -344,6 +367,7 @@ func ExternalCreateSucceededDuring(o metav1.Object, d time.Duration) bool { if t.IsZero() { return false } + return time.Since(t) < d } diff --git a/pkg/meta/meta_test.go b/pkg/meta/meta_test.go index 9759983..f6554fc 100644 --- a/pkg/meta/meta_test.go +++ b/pkg/meta/meta_test.go @@ -46,6 +46,7 @@ func TestReferenceTo(t *testing.T) { o metav1.Object of schema.GroupVersionKind } + tests := map[string]struct { args want *corev1.ObjectReference @@ -77,7 +78,7 @@ func TestReferenceTo(t *testing.T) { for name, tc := range tests { t.Run(name, func(t *testing.T) { - got := ReferenceTo(tc.args.o, tc.args.of) + got := ReferenceTo(tc.o, tc.of) if diff := cmp.Diff(tc.want, got); diff != "" { t.Errorf("ReferenceTo(): -want, +got:\n%s", diff) } @@ -90,6 +91,7 @@ func TestTypedReferenceTo(t *testing.T) { o metav1.Object of schema.GroupVersionKind } + tests := map[string]struct { args want *xpv1.TypedReference @@ -120,7 +122,7 @@ func TestTypedReferenceTo(t *testing.T) { for name, tc := range tests { t.Run(name, func(t *testing.T) { - got := TypedReferenceTo(tc.args.o, tc.args.of) + got := TypedReferenceTo(tc.o, tc.of) if diff := cmp.Diff(tc.want, got); diff != "" { t.Errorf("TypedReferenceTo(): -want, +got:\n%s", diff) } @@ -895,6 +897,7 @@ func TestSetExternalName(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { SetExternalName(tc.o, tc.name) + if diff := cmp.Diff(tc.want, tc.o); diff != "" { t.Errorf("SetExternalName(...): -want, +got:\n%s", diff) } @@ -947,6 +950,7 @@ func TestSetExternalCreatePending(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { SetExternalCreatePending(tc.o, tc.t) + if diff := cmp.Diff(tc.want, tc.o); diff != "" { t.Errorf("SetExternalCreatePending(...): -want, +got:\n%s", diff) } @@ -999,6 +1003,7 @@ func TestSetExternalCreateSucceeded(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { SetExternalCreateSucceeded(tc.o, tc.t) + if diff := cmp.Diff(tc.want, tc.o); diff != "" { t.Errorf("SetExternalCreateSucceeded(...): -want, +got:\n%s", diff) } @@ -1051,6 +1056,7 @@ func TestSetExternalCreateFailed(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { SetExternalCreateFailed(tc.o, tc.t) + if diff := cmp.Diff(tc.want, tc.o); diff != "" { t.Errorf("SetExternalCreateFailed(...): -want, +got:\n%s", diff) } diff --git a/pkg/parser/fsreader.go b/pkg/parser/fsreader.go index 126bce2..6023123 100644 --- a/pkg/parser/fsreader.go +++ b/pkg/parser/fsreader.go @@ -62,6 +62,7 @@ func SkipDirs() FilterFn { if info.IsDir() { return true, nil } + return false, nil } } @@ -79,6 +80,7 @@ func SkipNotYAML() FilterFn { if filepath.Ext(path) != ".yaml" && filepath.Ext(path) != ".yml" { return true, nil } + return false, nil } } @@ -92,18 +94,23 @@ func NewFsReadCloser(fs afero.Fs, dir string, fns ...FilterFn) (*FsReadCloser, e if err != nil { return err } + for _, fn := range fns { filter, err := fn(path, info) if err != nil { return err } + if filter { return nil } } + paths = append(paths, path) + return nil }) + return &FsReadCloser{ fs: fs, dir: dir, @@ -121,24 +128,31 @@ func (r *FsReadCloser) Read(p []byte) (n int, err error) { r.position = 0 r.wroteBreak = false n = copy(p, "\n---\n") + return n, nil } + if r.index == len(r.paths) { return 0, io.EOF } + if r.writeBreak { n = copy(p, "\n...\n") r.writeBreak = false r.wroteBreak = true + return n, nil } + b, err := afero.ReadFile(r.fs, r.paths[r.index]) n = copy(p, b[r.position:]) + r.position += n if errors.Is(err, io.EOF) || n == 0 { r.writeBreak = true err = nil } + return n, err } @@ -155,6 +169,7 @@ func (r *FsReadCloser) Annotate() any { if index == len(r.paths) { index-- } + return FsReadCloserAnnotation{ path: r.paths[index], position: r.position, diff --git a/pkg/parser/linter.go b/pkg/parser/linter.go index f9e8a9b..a3211cf 100644 --- a/pkg/parser/linter.go +++ b/pkg/parser/linter.go @@ -78,6 +78,7 @@ func (l *PackageLinter) Lint(pkg Lintable) error { return err } } + for _, o := range pkg.GetMeta() { for _, fn := range l.perMeta { if err := fn(o); err != nil { @@ -85,6 +86,7 @@ func (l *PackageLinter) Lint(pkg Lintable) error { } } } + for _, o := range pkg.GetObjects() { for _, fn := range l.perObject { if err := fn(o); err != nil { @@ -92,6 +94,7 @@ func (l *PackageLinter) Lint(pkg Lintable) error { } } } + return nil } @@ -100,16 +103,20 @@ func (l *PackageLinter) Lint(pkg Lintable) error { func Or(linters ...ObjectLinterFn) ObjectLinterFn { return func(o runtime.Object) error { var errs []string + for _, l := range linters { if l == nil { return errors.New(errNilLinterFn) } + err := l(o) if err == nil { return nil } + errs = append(errs, err.Error()) } + return errors.Errorf(errOrFmt, strings.Join(errs, ", ")) } } diff --git a/pkg/parser/linter_test.go b/pkg/parser/linter_test.go index da32fb2..a5144a5 100644 --- a/pkg/parser/linter_test.go +++ b/pkg/parser/linter_test.go @@ -119,7 +119,6 @@ func TestLinter(t *testing.T) { for name, tc := range cases { 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 != "" { t.Errorf("\n%s\nl.Lint(...): -want error, +got error:\n%s", tc.reason, diff) } @@ -175,7 +174,6 @@ func TestOr(t *testing.T) { for name, tc := range cases { 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 != "" { t.Errorf("\n%s\nOr(...): -want error, +got error:\n%s", tc.reason, diff) } diff --git a/pkg/parser/parser.go b/pkg/parser/parser.go index 8cc387b..ce2c27f 100644 --- a/pkg/parser/parser.go +++ b/pkg/parser/parser.go @@ -105,21 +105,27 @@ func (p *PackageParser) Parse(_ context.Context, reader io.ReadCloser) (*Package if reader == nil { return pkg, nil } + defer func() { _ = reader.Close() }() + yr := yaml.NewYAMLReader(bufio.NewReader(reader)) dm := json.NewSerializerWithOptions(json.DefaultMetaFactory, p.metaScheme, p.metaScheme, json.SerializerOptions{Yaml: true}) do := json.NewSerializerWithOptions(json.DefaultMetaFactory, p.objScheme, p.objScheme, json.SerializerOptions{Yaml: true}) + for { content, err := yr.Read() if err != nil && !errors.Is(err, io.EOF) { return pkg, err } + if errors.Is(err, io.EOF) { break } + if isEmptyYAML(content) { continue } + m, _, err := dm.Decode(content, nil, nil) if err != nil { // NOTE(hasheddan): we only try to decode with object scheme if the @@ -127,15 +133,20 @@ func (p *PackageParser) Parse(_ context.Context, reader io.ReadCloser) (*Package if !runtime.IsNotRegisteredError(err) { return pkg, annotateErr(err, reader) } + o, _, err := do.Decode(content, nil, nil) if err != nil { return pkg, annotateErr(err, reader) } + pkg.objects = append(pkg.objects, o) + continue } + pkg.meta = append(pkg.meta, m) } + return pkg, nil } @@ -151,6 +162,7 @@ func isEmptyYAML(y []byte) bool { return false } } + return true } @@ -159,6 +171,7 @@ func annotateErr(err error, reader io.ReadCloser) error { if anno, ok := reader.(AnnotatedReadCloser); ok { return errors.Wrapf(err, "%+v", anno.Annotate()) } + return err } @@ -184,6 +197,7 @@ func NewPodLogBackend(bo ...BackendOption) *PodLogBackend { for _, o := range bo { o(p) } + return p } @@ -192,11 +206,14 @@ func (p *PodLogBackend) Init(ctx context.Context, bo ...BackendOption) (io.ReadC for _, o := range bo { o(p) } + logs := p.client.CoreV1().Pods(p.namespace).GetLogs(p.name, &corev1.PodLogOptions{}) + reader, err := logs.Stream(ctx) if err != nil { return nil, err } + return reader, nil } @@ -207,6 +224,7 @@ func PodName(name string) BackendOption { if !ok { return } + pl.name = name } } @@ -218,6 +236,7 @@ func PodNamespace(namespace string) BackendOption { if !ok { return } + pl.namespace = namespace } } @@ -229,6 +248,7 @@ func PodClient(client kubernetes.Interface) BackendOption { if !ok { return } + pl.client = client } } @@ -261,6 +281,7 @@ func NewFsBackend(fs afero.Fs, bo ...BackendOption) *FsBackend { for _, o := range bo { o(f) } + return f } @@ -269,6 +290,7 @@ func (p *FsBackend) Init(_ context.Context, bo ...BackendOption) (io.ReadCloser, for _, o := range bo { o(p) } + return NewFsReadCloser(p.fs, p.dir, p.skips...) } @@ -279,6 +301,7 @@ func FsDir(dir string) BackendOption { if !ok { return } + f.dir = dir } } @@ -290,6 +313,7 @@ func FsFilters(skips ...FilterFn) BackendOption { if !ok { return } + f.skips = skips } } @@ -311,5 +335,6 @@ func (p *EchoBackend) Init(_ context.Context, bo ...BackendOption) (io.ReadClose for _, o := range bo { o(p) } + return io.NopCloser(strings.NewReader(p.echo)), nil } diff --git a/pkg/parser/parser_test.go b/pkg/parser/parser_test.go index c5d56d9..94d84d4 100644 --- a/pkg/parser/parser_test.go +++ b/pkg/parser/parser_test.go @@ -195,18 +195,22 @@ func TestParser(t *testing.T) { if err != nil { t.Errorf("backend.Init(...): unexpected error: %s", err) } + pkg, err := tc.parser.Parse(context.TODO(), r) if err != nil && !tc.wantErr { t.Errorf("parser.Parse(...): unexpected error: %s", err) } + if tc.wantErr { return } + if diff := cmp.Diff(tc.pkg.GetObjects(), pkg.GetObjects(), cmpopts.SortSlices(func(i, j runtime.Object) bool { return i.GetObjectKind().GroupVersionKind().String() > j.GetObjectKind().GroupVersionKind().String() })); diff != "" { t.Errorf("Objects: -want, +got:\n%s", diff) } + if diff := cmp.Diff(tc.pkg.GetMeta(), pkg.GetMeta(), cmpopts.SortSlices(func(i, j runtime.Object) bool { return i.GetObjectKind().GroupVersionKind().String() > j.GetObjectKind().GroupVersionKind().String() })); diff != "" { @@ -220,9 +224,11 @@ func TestCleanYAML(t *testing.T) { type args struct { in []byte } + type want struct { out bool } + cases := map[string]struct { reason string args args diff --git a/pkg/password/password.go b/pkg/password/password.go index bdf31a2..abc18aa 100644 --- a/pkg/password/password.go +++ b/pkg/password/password.go @@ -53,7 +53,9 @@ func (s Settings) Generate() (string, error) { if err != nil { return "", err } + pw[i] = s.CharacterSet[n.Int64()] } + return string(pw), nil } diff --git a/pkg/password/password_test.go b/pkg/password/password_test.go index 52b2f71..661b932 100644 --- a/pkg/password/password_test.go +++ b/pkg/password/password_test.go @@ -8,12 +8,13 @@ import ( func TestGenerate(t *testing.T) { // ¯\_(ツ)_/¯ - want := "aaa" + got, err := Settings{CharacterSet: "a", Length: 3}.Generate() if diff := cmp.Diff(want, got); diff != "" { t.Errorf("Generate(): -want, +got:\n%s", diff) } + if err != nil { t.Errorf("Generate: %s\n", err) } diff --git a/pkg/ratelimiter/default.go b/pkg/ratelimiter/default.go index cf2c340..ea4975b 100644 --- a/pkg/ratelimiter/default.go +++ b/pkg/ratelimiter/default.go @@ -52,5 +52,6 @@ func LimitRESTConfig(cfg *rest.Config, rps int) *rest.Config { out := rest.CopyConfig(cfg) out.QPS = float32(rps * 5) out.Burst = rps * 10 + return out } diff --git a/pkg/ratelimiter/reconciler.go b/pkg/ratelimiter/reconciler.go index 0f79067..8ff44bf 100644 --- a/pkg/ratelimiter/reconciler.go +++ b/pkg/ratelimiter/reconciler.go @@ -56,7 +56,9 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco if d := r.when(req); d > 0 { return reconcile.Result{RequeueAfter: d}, nil } + r.limit.Forget(item) + return r.inner.Reconcile(ctx, req) } @@ -78,6 +80,7 @@ func (r *Reconciler) when(req reconcile.Request) time.Duration { r.limitedL.Lock() delete(r.limited, item) r.limitedL.Unlock() + return 0 } diff --git a/pkg/ratelimiter/reconciler_test.go b/pkg/ratelimiter/reconciler_test.go index dc742e6..287e476 100644 --- a/pkg/ratelimiter/reconciler_test.go +++ b/pkg/ratelimiter/reconciler_test.go @@ -41,6 +41,7 @@ func TestReconcile(t *testing.T) { ctx context.Context req reconcile.Request } + type want struct { res reconcile.Result err error @@ -101,6 +102,7 @@ func TestReconcile(t *testing.T) { if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { t.Errorf("%s\nr.Reconcile(...): -want, +got error:\n%s", tc.reason, diff) } + if diff := cmp.Diff(tc.want.res, got); diff != "" { t.Errorf("%s\nr.Reconcile(...): -want, +got result:\n%s", tc.reason, diff) } diff --git a/pkg/reconciler/managed/api.go b/pkg/reconciler/managed/api.go index e6c546f..b1b4a30 100644 --- a/pkg/reconciler/managed/api.go +++ b/pkg/reconciler/managed/api.go @@ -70,7 +70,9 @@ func (a *NameAsExternalName) Initialize(ctx context.Context, mg resource.Managed if meta.GetExternalName(mg) != "" { return nil } + meta.SetExternalName(mg, mg.GetName()) + return errors.Wrap(a.client.Update(ctx, mg), errUpdateManaged) } @@ -103,6 +105,7 @@ func (a *APISecretPublisher) PublishConnection(ctx context.Context, o resource.C s := resource.ConnectionSecretFor(o, resource.MustGetKind(o, a.typer)) s.Data = c + err := a.secret.Apply(ctx, s, resource.ConnectionSecretMustBeControllableBy(o.GetUID()), resource.AllowUpdateIf(func(current, desired runtime.Object) bool { @@ -116,6 +119,7 @@ func (a *APISecretPublisher) PublishConnection(ctx context.Context, o resource.C // The update was not allowed because it was a no-op. return false, nil } + if err != nil { return false, errors.Wrap(err, errCreateOrUpdateSecret) } @@ -153,15 +157,19 @@ func prepareJSONMerge(existing, resolved runtime.Object) ([]byte, error) { // in the first place, instead of an unmarshal/marshal from the prepared // patch []byte later. existing.GetObjectKind().SetGroupVersionKind(schema.GroupVersionKind{}) + eBuff, err := json.Marshal(existing) if err != nil { return nil, errors.Wrap(err, errMarshalExisting) } + rBuff, err := json.Marshal(resolved) if err != nil { return nil, errors.Wrap(err, errMarshalResolved) } + patch, err := jsonpatch.CreateMergePatch(eBuff, rBuff) + return patch, errors.Wrap(err, errPreparePatch) } @@ -177,6 +185,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) } @@ -190,6 +199,7 @@ func (a *APISimpleReferenceResolver) ResolveReferences(ctx context.Context, mg r if err != nil { return err } + return errors.Wrap(a.client.Patch(ctx, mg, client.RawPatch(types.ApplyPatchType, patch), client.FieldOwner(fieldOwnerAPISimpleRefResolver), client.ForceOwnership), errPatchManaged) } @@ -221,9 +231,12 @@ func (u *RetryingCriticalAnnotationUpdater) UpdateCriticalAnnotations(ctx contex if getErr := u.client.Get(ctx, client.ObjectKeyFromObject(o), o); getErr != nil { return getErr } + meta.AddAnnotations(o, a) } + return err }) + return errors.Wrap(err, errUpdateCriticalAnnotations) } diff --git a/pkg/reconciler/managed/api_test.go b/pkg/reconciler/managed/api_test.go index 83c4353..9a9310e 100644 --- a/pkg/reconciler/managed/api_test.go +++ b/pkg/reconciler/managed/api_test.go @@ -106,10 +106,12 @@ func TestNameAsExternalName(t *testing.T) { for name, tc := range cases { 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 != "" { t.Errorf("api.Initialize(...): -want error, +got error:\n%s", diff) } + if diff := cmp.Diff(tc.want.mg, tc.args.mg, test.EquateConditions()); diff != "" { t.Errorf("api.Initialize(...) Managed: -want, +got:\n%s", diff) } @@ -144,6 +146,7 @@ func TestAPISecretPublisher(t *testing.T) { err error published bool } + cases := map[string]struct { reason string fields fields @@ -223,10 +226,12 @@ func TestAPISecretPublisher(t *testing.T) { for name, tc := range cases { 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 != "" { t.Errorf("\n%s\nPublish(...): -wantErr, +gotErr:\n%s", tc.reason, diff) } + if diff := cmp.Diff(tc.want.published, got); diff != "" { t.Errorf("\n%s\nPublish(...): -wantPublished, +gotPublished:\n%s", tc.reason, diff) } @@ -347,6 +352,7 @@ func TestResolveReferences(t *testing.T) { for name, tc := range cases { 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 != "" { t.Errorf("\n%s\nr.ResolveReferences(...): -want, +got:\n%s", tc.reason, diff) @@ -360,6 +366,7 @@ func TestPrepareJSONMerge(t *testing.T) { existing runtime.Object resolved runtime.Object } + type want struct { patch string err error @@ -392,6 +399,7 @@ func TestPrepareJSONMerge(t *testing.T) { if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { t.Errorf("\n%s\nprepareJSONMerge(...): -wantErr, +gotErr:\n%s", tc.reason, diff) } + if diff := cmp.Diff(tc.want.patch, string(patch)); diff != "" { t.Errorf("\n%s\nprepareJSONMerge(...): -want, +got:\n%s", tc.reason, diff) } @@ -406,6 +414,7 @@ func TestRetryingCriticalAnnotationUpdater(t *testing.T) { ctx context.Context o client.Object } + type want struct { err error o client.Object @@ -494,10 +503,12 @@ func TestRetryingCriticalAnnotationUpdater(t *testing.T) { for name, tc := range cases { 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.err, got, test.EquateErrors()); diff != "" { t.Errorf("\n%s\nu.UpdateCriticalAnnotations(...): -want, +got:\n%s", tc.reason, diff) } + if diff := cmp.Diff(tc.want.o, tc.args.o); diff != "" { t.Errorf("\n%s\nu.UpdateCriticalAnnotations(...): -want, +got:\n%s", tc.reason, diff) } diff --git a/pkg/reconciler/managed/changelogger.go b/pkg/reconciler/managed/changelogger.go index 050ff58..50dcbdf 100644 --- a/pkg/reconciler/managed/changelogger.go +++ b/pkg/reconciler/managed/changelogger.go @@ -119,6 +119,7 @@ func (g *GRPCChangeLogger) Log(ctx context.Context, managed resource.Managed, op // send everything we've got to the change log service _, err = g.client.SendChangeLog(sendCtx, &v1alpha1.SendChangeLogRequest{Entry: entry}, grpc.WaitForReady(true)) + return errors.Wrap(err, "cannot send change log entry") } diff --git a/pkg/reconciler/managed/changelogger_test.go b/pkg/reconciler/managed/changelogger_test.go index 5b4d562..706ca22 100644 --- a/pkg/reconciler/managed/changelogger_test.go +++ b/pkg/reconciler/managed/changelogger_test.go @@ -52,6 +52,7 @@ func (c *changeLogServiceClient) SendChangeLog(ctx context.Context, in *v1alpha1 if c.sendFn != nil { return c.sendFn(ctx, in, opts...) } + return nil, nil } @@ -167,6 +168,7 @@ func mustObjectAsProtobufStruct(o runtime.Object) *structpb.Struct { if err != nil { panic(err) } + return s } @@ -183,6 +185,7 @@ func equateApproxTimepb(margin time.Duration) []cmp.Option { a, b := p.Last().Values() return msgIsTimestamp(a) && msgIsTimestamp(b) } + return false }, cmp.Transformer("timestamppb", func(t protocmp.Message) time.Time { diff --git a/pkg/reconciler/managed/metrics.go b/pkg/reconciler/managed/metrics.go index fe5f717..5329156 100644 --- a/pkg/reconciler/managed/metrics.go +++ b/pkg/reconciler/managed/metrics.go @@ -116,10 +116,12 @@ func (r *MRMetricRecorder) recordFirstTimeReconciled(managed resource.Managed) { func (r *MRMetricRecorder) recordDrift(managed resource.Managed) { name := managed.GetName() + last, ok := r.lastObservation.Load(name) if !ok { return } + lt, ok := last.(time.Time) if !ok { return @@ -142,6 +144,7 @@ func (r *MRMetricRecorder) recordFirstTimeReady(managed resource.Managed) { if !ok { return } + r.mrFirstTimeReady.With(getLabels(managed)).Observe(time.Since(managed.GetCreationTimestamp().Time).Seconds()) r.firstObservation.Delete(managed.GetName()) } diff --git a/pkg/reconciler/managed/policies.go b/pkg/reconciler/managed/policies.go index 0876df9..10e2c91 100644 --- a/pkg/reconciler/managed/policies.go +++ b/pkg/reconciler/managed/policies.go @@ -139,6 +139,7 @@ func (m *ManagementPoliciesResolver) Validate() error { return nil } } + return fmt.Errorf(errFmtManagementPolicyNotSupported, m.managementPolicies.UnsortedList()) } @@ -148,6 +149,7 @@ func (m *ManagementPoliciesResolver) IsPaused() bool { if !m.enabled { return false } + return m.managementPolicies.Len() == 0 } @@ -157,6 +159,7 @@ func (m *ManagementPoliciesResolver) ShouldCreate() bool { if !m.enabled { return true } + return m.managementPolicies.HasAny(xpv1.ManagementActionCreate, xpv1.ManagementActionAll) } @@ -166,6 +169,7 @@ func (m *ManagementPoliciesResolver) ShouldUpdate() bool { if !m.enabled { return true } + return m.managementPolicies.HasAny(xpv1.ManagementActionUpdate, xpv1.ManagementActionAll) } @@ -175,6 +179,7 @@ func (m *ManagementPoliciesResolver) ShouldLateInitialize() bool { if !m.enabled { return true } + return m.managementPolicies.HasAny(xpv1.ManagementActionLateInitialize, xpv1.ManagementActionAll) } @@ -185,6 +190,7 @@ func (m *ManagementPoliciesResolver) ShouldOnlyObserve() bool { if !m.enabled { return false } + return m.managementPolicies.Equal(sets.New[xpv1.ManagementAction](xpv1.ManagementActionObserve)) } diff --git a/pkg/reconciler/managed/publisher.go b/pkg/reconciler/managed/publisher.go index f0a65c1..f49801a 100644 --- a/pkg/reconciler/managed/publisher.go +++ b/pkg/reconciler/managed/publisher.go @@ -32,15 +32,18 @@ type PublisherChain []ConnectionPublisher // encounters, if any. func (pc PublisherChain) PublishConnection(ctx context.Context, o resource.ConnectionSecretOwner, c ConnectionDetails) (bool, error) { published := false + for _, p := range pc { pb, err := p.PublishConnection(ctx, o, c) if err != nil { return published, err } + if pb { published = true } } + return published, nil } @@ -52,6 +55,7 @@ func (pc PublisherChain) UnpublishConnection(ctx context.Context, o resource.Con return err } } + return nil } @@ -65,6 +69,7 @@ func (m *DisabledSecretStoreManager) PublishConnection(_ context.Context, so res if so.GetPublishConnectionDetailsTo() != nil { return false, errors.New(errSecretStoreDisabled) } + return false, nil } @@ -74,5 +79,6 @@ func (m *DisabledSecretStoreManager) UnpublishConnection(_ context.Context, so r if so.GetPublishConnectionDetailsTo() != nil { return errors.New(errSecretStoreDisabled) } + return nil } diff --git a/pkg/reconciler/managed/publisher_test.go b/pkg/reconciler/managed/publisher_test.go index 5326907..87ba413 100644 --- a/pkg/reconciler/managed/publisher_test.go +++ b/pkg/reconciler/managed/publisher_test.go @@ -109,6 +109,7 @@ func TestPublisherChain(t *testing.T) { if diff := cmp.Diff(tc.want.err, gotErr, test.EquateErrors()); diff != "" { t.Errorf("Publish(...): -want, +got:\n%s", diff) } + if diff := cmp.Diff(tc.want.published, got); diff != "" { t.Errorf("Publish(...): -wantPublished, +gotPublished:\n%s", diff) } @@ -120,6 +121,7 @@ func TestDisabledSecretStorePublish(t *testing.T) { type args struct { mg resource.Managed } + type want struct { published bool err error @@ -151,10 +153,12 @@ func TestDisabledSecretStorePublish(t *testing.T) { for name, tc := range cases { 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 != "" { t.Errorf("Publish(...): -want, +got:\n%s", diff) } + if diff := cmp.Diff(tc.want.published, got); diff != "" { t.Errorf("Publish(...): -wantPublished, +gotPublished:\n%s", diff) } @@ -166,6 +170,7 @@ func TestDisabledSecretStoreUnpublish(t *testing.T) { type args struct { mg resource.Managed } + type want struct { err error } @@ -196,6 +201,7 @@ func TestDisabledSecretStoreUnpublish(t *testing.T) { for name, tc := range cases { 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 != "" { t.Errorf("Publish(...): -want, +got:\n%s", diff) diff --git a/pkg/reconciler/managed/reconciler.go b/pkg/reconciler/managed/reconciler.go index 49fc3a9..4d51888 100644 --- a/pkg/reconciler/managed/reconciler.go +++ b/pkg/reconciler/managed/reconciler.go @@ -199,6 +199,7 @@ func (cc InitializerChain) Initialize(ctx context.Context, mg resource.Managed) return err } } + return nil } @@ -823,7 +824,6 @@ func NewReconciler(m manager.Manager, of resource.ManagedKind, o ...ReconcilerOp func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (result reconcile.Result, err error) { //nolint:gocognit // See note below. // NOTE(negz): This method is a well over our cyclomatic complexity goal. // Be wary of adding additional complexity. - defer func() { result, err = errors.SilentlyRequeueOnConflict(result, err) }() log := r.log.WithValues("request", req) @@ -886,11 +886,14 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu // that is not supported by the controller. if err := policy.Validate(); err != nil { log.Debug(err.Error()) + if kerrors.IsConflict(err) { return reconcile.Result{Requeue: true}, nil } + record.Event(managed, event.Warning(reasonManagementPolicyInvalid, err)) status.MarkConditions(xpv1.ReconcileError(err)) + return reconcile.Result{}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) } @@ -911,23 +914,30 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu // condition. If not, we requeue explicitly, which will trigger // backoff. log.Debug("Cannot unpublish connection details", "error", err) + if kerrors.IsConflict(err) { return reconcile.Result{Requeue: true}, nil } + record.Event(managed, event.Warning(reasonCannotUnpublish, err)) status.MarkConditions(xpv1.Deleting(), xpv1.ReconcileError(err)) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) } + if err := r.managed.RemoveFinalizer(ctx, managed); err != nil { // If this is the first time we encounter this issue we'll be // requeued implicitly when we update our status with the new error // condition. If not, we requeue explicitly, which will trigger // backoff. log.Debug("Cannot remove managed resource finalizer", "error", err) + if kerrors.IsConflict(err) { return reconcile.Result{Requeue: true}, nil } + status.MarkConditions(xpv1.Deleting(), xpv1.ReconcileError(err)) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) } @@ -937,6 +947,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu // longer exist and thus there is no point trying to update its status. r.metricRecorder.recordDeleted(managed) log.Debug("Successfully deleted managed resource") + return reconcile.Result{Requeue: false}, nil } @@ -945,11 +956,14 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu // implicitly when we update our status with the new error condition. If // not, we requeue explicitly, which will trigger backoff. log.Debug("Cannot initialize managed resource", "error", err) + if kerrors.IsConflict(err) { return reconcile.Result{Requeue: true}, nil } + record.Event(managed, event.Warning(reasonCannotInitialize, err)) status.MarkConditions(xpv1.ReconcileError(err)) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) } @@ -963,8 +977,10 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu log.Debug(errCreateIncomplete) record.Event(managed, event.Warning(reasonCannotInitialize, errors.New(errCreateIncomplete))) status.MarkConditions(xpv1.Creating(), xpv1.ReconcileError(errors.New(errCreateIncomplete))) + return reconcile.Result{Requeue: false}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) } + log.Debug("Cannot determine creation result, but proceeding due to deterministic external name") } @@ -985,11 +1001,14 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu // requeued implicitly due to the status update. If not, we want // requeue explicitly, which will trigger backoff. log.Debug("Cannot resolve managed resource references", "error", err) + if kerrors.IsConflict(err) { return reconcile.Result{Requeue: true}, nil } + record.Event(managed, event.Warning(reasonCannotResolveRefs, err)) status.MarkConditions(xpv1.ReconcileError(err)) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) } } @@ -1002,13 +1021,17 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu // condition. If not, we requeue explicitly, which will trigger // backoff. log.Debug("Cannot connect to provider", "error", err) + if kerrors.IsConflict(err) { return reconcile.Result{Requeue: true}, nil } + record.Event(managed, event.Warning(reasonCannotConnect, err)) status.MarkConditions(xpv1.ReconcileError(errors.Wrap(err, errReconcileConnect))) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) } + defer func() { if err := r.external.Disconnect(ctx); err != nil { log.Debug("Cannot disconnect from provider", "error", err) @@ -1030,11 +1053,14 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu // error condition. If not, we requeue explicitly, which will // trigger backoff. log.Debug("Cannot observe external resource", "error", err) + if kerrors.IsConflict(err) { return reconcile.Result{Requeue: true}, nil } + record.Event(managed, event.Warning(reasonCannotObserve, err)) status.MarkConditions(xpv1.ReconcileError(errors.Wrap(err, errReconcileObserve))) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) } @@ -1043,6 +1069,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu if !observation.ResourceExists && policy.ShouldOnlyObserve() { record.Event(managed, event.Warning(reasonCannotObserve, errors.New(errExternalResourceNotExist))) status.MarkConditions(xpv1.ReconcileError(errors.Wrap(errors.New(errExternalResourceNotExist), errReconcileObserve))) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) } @@ -1054,6 +1081,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu if !observation.ResourceExists && meta.ExternalCreateSucceededDuring(managed, r.creationGracePeriod) { log.Debug("Waiting for external resource existence to be confirmed") record.Event(managed, event.Normal(reasonPending, "Waiting for external resource existence to be confirmed")) + return reconcile.Result{Requeue: true}, nil } @@ -1076,11 +1104,14 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu // status with the new error condition. If not, we want requeue // explicitly, which will trigger backoff. log.Debug("Cannot delete external resource", "error", err) + if err := r.change.Log(ctx, managedPreOp, v1alpha1.OperationType_OPERATION_TYPE_DELETE, err, deletion.AdditionalDetails); err != nil { log.Info(errRecordChangeLog, "error", err) } + record.Event(managed, event.Warning(reasonCannotDelete, err)) status.MarkConditions(xpv1.Deleting(), xpv1.ReconcileError(errors.Wrap(err, errReconcileDelete))) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) } @@ -1092,36 +1123,47 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu // unpublish and finalize. If it still exists we'll re-enter this // block and try again. log.Debug("Successfully requested deletion of external resource") + if err := r.change.Log(ctx, managedPreOp, v1alpha1.OperationType_OPERATION_TYPE_DELETE, nil, deletion.AdditionalDetails); err != nil { log.Info(errRecordChangeLog, "error", err) } + record.Event(managed, event.Normal(reasonDeleted, "Successfully requested deletion of external resource")) status.MarkConditions(xpv1.Deleting(), xpv1.ReconcileSuccess()) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) } + if err := r.managed.UnpublishConnection(ctx, managed, observation.ConnectionDetails); err != nil { // If this is the first time we encounter this issue we'll be // requeued implicitly when we update our status with the new error // condition. If not, we requeue explicitly, which will trigger // backoff. log.Debug("Cannot unpublish connection details", "error", err) + if kerrors.IsConflict(err) { return reconcile.Result{Requeue: true}, nil } + record.Event(managed, event.Warning(reasonCannotUnpublish, err)) status.MarkConditions(xpv1.Deleting(), xpv1.ReconcileError(err)) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) } + if err := r.managed.RemoveFinalizer(ctx, managed); err != nil { // If this is the first time we encounter this issue we'll be // requeued implicitly when we update our status with the new error // condition. If not, we requeue explicitly, which will trigger // backoff. log.Debug("Cannot remove managed resource finalizer", "error", err) + if kerrors.IsConflict(err) { return reconcile.Result{Requeue: true}, nil } + status.MarkConditions(xpv1.Deleting(), xpv1.ReconcileError(err)) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) } @@ -1131,6 +1173,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu // thus there is no point trying to update its status. r.metricRecorder.recordDeleted(managed) log.Debug("Successfully deleted managed resource") + return reconcile.Result{Requeue: false}, nil } @@ -1139,11 +1182,14 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu // implicitly when we update our status with the new error condition. If // not, we requeue explicitly, which will trigger backoff. log.Debug("Cannot publish connection details", "error", err) + if kerrors.IsConflict(err) { return reconcile.Result{Requeue: true}, nil } + record.Event(managed, event.Warning(reasonCannotPublish, err)) status.MarkConditions(xpv1.ReconcileError(err)) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) } @@ -1152,10 +1198,13 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu // implicitly when we update our status with the new error condition. If // not, we requeue explicitly, which will trigger backoff. log.Debug("Cannot add finalizer", "error", err) + if kerrors.IsConflict(err) { return reconcile.Result{Requeue: true}, nil } + status.MarkConditions(xpv1.ReconcileError(err)) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) } @@ -1168,13 +1217,17 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu // don't use the CriticalAnnotationUpdater because we _want_ the // 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) + if kerrors.IsConflict(err) { return reconcile.Result{Requeue: true}, nil } + record.Event(managed, event.Warning(reasonCannotUpdateManaged, errors.Wrap(err, errUpdateManaged))) status.MarkConditions(xpv1.Creating(), xpv1.ReconcileError(errors.Wrap(err, errUpdateManaged))) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) } @@ -1186,6 +1239,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu // issue we'll be requeued implicitly when we update our status with // the new error condition. If not, we requeue explicitly, which will trigger backoff. log.Debug("Cannot create external resource", "error", err) + if !kerrors.IsConflict(err) { record.Event(managed, event.Warning(reasonCannotCreate, err)) } @@ -1197,6 +1251,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu // won't know whether or not it created an external // 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))) @@ -1211,7 +1266,9 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu if err := r.change.Log(ctx, managedPreOp, v1alpha1.OperationType_OPERATION_TYPE_CREATE, err, creation.AdditionalDetails); err != nil { log.Info(errRecordChangeLog, "error", err) } + status.MarkConditions(xpv1.Creating(), xpv1.ReconcileError(errors.Wrap(err, errReconcileCreate))) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) } @@ -1234,13 +1291,17 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu // Create implementations are advised not to alter status, but // 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) + if kerrors.IsConflict(err) { return reconcile.Result{Requeue: true}, nil } + record.Event(managed, event.Warning(reasonCannotUpdateManaged, errors.Wrap(err, errUpdateManagedAnnotations))) status.MarkConditions(xpv1.Creating(), xpv1.ReconcileError(errors.Wrap(err, errUpdateManagedAnnotations))) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) } @@ -1249,11 +1310,14 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu // requeued implicitly when we update our status with the new error // condition. If not, we requeue explicitly, which will trigger backoff. log.Debug("Cannot publish connection details", "error", err) + if kerrors.IsConflict(err) { return reconcile.Result{Requeue: true}, nil } + record.Event(managed, event.Warning(reasonCannotPublish, err)) status.MarkConditions(xpv1.Creating(), xpv1.ReconcileError(err)) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) } @@ -1264,6 +1328,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")) status.MarkConditions(xpv1.Creating(), xpv1.ReconcileSuccess()) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) } @@ -1279,6 +1344,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu log.Debug(errUpdateManaged, "error", err) record.Event(managed, event.Warning(reasonCannotUpdateManaged, err)) status.MarkConditions(xpv1.ReconcileError(errors.Wrap(err, errUpdateManaged))) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) } } @@ -1313,6 +1379,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu reconcileAfter := r.pollIntervalHook(managed, r.pollInterval) log.Debug("Skipping update due to managementPolicies. Reconciliation succeeded", "requeue-after", time.Now().Add(reconcileAfter)) status.MarkConditions(xpv1.ReconcileSuccess()) + return reconcile.Result{RequeueAfter: reconcileAfter}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) } @@ -1324,16 +1391,20 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu // requeued implicitly when we update our status with the new error // condition. If not, we requeue explicitly, which will trigger backoff. log.Debug("Cannot update external resource") + if err := r.change.Log(ctx, managedPreOp, v1alpha1.OperationType_OPERATION_TYPE_UPDATE, err, update.AdditionalDetails); err != nil { log.Info(errRecordChangeLog, "error", err) } + record.Event(managed, event.Warning(reasonCannotUpdate, err)) status.MarkConditions(xpv1.ReconcileError(errors.Wrap(err, errReconcileUpdate))) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) } // record the drift after the successful update. r.metricRecorder.recordDrift(managed) + if err := r.change.Log(ctx, managedPreOp, v1alpha1.OperationType_OPERATION_TYPE_UPDATE, nil, update.AdditionalDetails); err != nil { log.Info(errRecordChangeLog, "error", err) } @@ -1345,6 +1416,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)) status.MarkConditions(xpv1.ReconcileError(err)) + return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) } @@ -1357,5 +1429,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu log.Debug("Successfully requested update of external resource", "requeue-after", time.Now().Add(reconcileAfter)) record.Event(managed, event.Normal(reasonUpdated, "Successfully requested update of external resource")) status.MarkConditions(xpv1.ReconcileSuccess()) + return reconcile.Result{RequeueAfter: reconcileAfter}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) } diff --git a/pkg/reconciler/managed/reconciler_test.go b/pkg/reconciler/managed/reconciler_test.go index 249422a..3fc6df4 100644 --- a/pkg/reconciler/managed/reconciler_test.go +++ b/pkg/reconciler/managed/reconciler_test.go @@ -2020,8 +2020,8 @@ func TestReconciler(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { r := NewReconciler(tc.args.m, tc.args.mg, tc.args.o...) - got, err := r.Reconcile(context.Background(), reconcile.Request{}) + got, err := r.Reconcile(context.Background(), reconcile.Request{}) if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { t.Errorf("\nReason: %s\nr.Reconcile(...): -want error, +got error:\n%s", tc.reason, diff) } @@ -2038,6 +2038,7 @@ func TestTestManagementPoliciesResolverIsPaused(t *testing.T) { enabled bool policy xpv1.ManagementPolicies } + cases := map[string]struct { reason string args args @@ -2083,6 +2084,7 @@ func TestManagementPoliciesResolverValidate(t *testing.T) { enabled bool policy xpv1.ManagementPolicies } + cases := map[string]struct { reason string args args @@ -2144,6 +2146,7 @@ func TestManagementPoliciesResolverShouldCreate(t *testing.T) { managementPoliciesEnabled bool policy xpv1.ManagementPolicies } + cases := map[string]struct { reason string args args @@ -2196,6 +2199,7 @@ func TestManagementPoliciesResolverShouldUpdate(t *testing.T) { managementPoliciesEnabled bool policy xpv1.ManagementPolicies } + cases := map[string]struct { reason string args args @@ -2248,6 +2252,7 @@ func TestManagementPoliciesResolverShouldLateInitialize(t *testing.T) { managementPoliciesEnabled bool policy xpv1.ManagementPolicies } + cases := map[string]struct { reason string args args @@ -2300,6 +2305,7 @@ func TestManagementPoliciesResolverOnlyObserve(t *testing.T) { managementPoliciesEnabled bool policy xpv1.ManagementPolicies } + cases := map[string]struct { reason string args args @@ -2344,9 +2350,11 @@ func TestShouldDelete(t *testing.T) { managementPoliciesEnabled bool managed resource.Managed } + type want struct { delete bool } + cases := map[string]struct { reason string args args @@ -2747,12 +2755,14 @@ func TestReconcilerChangeLogs(t *testing.T) { func asManaged(obj client.Object, generation int64) *fake.Managed { mg := obj.(*fake.Managed) mg.Generation = generation + return mg } func newManaged(generation int64) *fake.Managed { mg := &fake.Managed{} mg.Generation = generation + return mg } diff --git a/pkg/reconciler/managed/reconciler_typed.go b/pkg/reconciler/managed/reconciler_typed.go index 7992212..02b0acb 100644 --- a/pkg/reconciler/managed/reconciler_typed.go +++ b/pkg/reconciler/managed/reconciler_typed.go @@ -20,10 +20,12 @@ func (c *typedExternalConnectDisconnectorWrapper[managed]) Connect(ctx context.C if !ok { return nil, errors.Errorf(errFmtUnexpectedObjectType, mg) } + external, err := c.c.Connect(ctx, cr) if err != nil { return nil, err } + return &typedExternalClientWrapper[managed]{c: external}, nil } @@ -42,6 +44,7 @@ func (c *typedExternalClientWrapper[managed]) Observe(ctx context.Context, mg re if !ok { return ExternalObservation{}, errors.Errorf(errFmtUnexpectedObjectType, mg) } + return c.c.Observe(ctx, cr) } @@ -50,6 +53,7 @@ func (c *typedExternalClientWrapper[managed]) Create(ctx context.Context, mg res if !ok { return ExternalCreation{}, errors.Errorf(errFmtUnexpectedObjectType, mg) } + return c.c.Create(ctx, cr) } @@ -58,6 +62,7 @@ func (c *typedExternalClientWrapper[managed]) Update(ctx context.Context, mg res if !ok { return ExternalUpdate{}, errors.Errorf(errFmtUnexpectedObjectType, mg) } + return c.c.Update(ctx, cr) } @@ -66,6 +71,7 @@ func (c *typedExternalClientWrapper[managed]) Delete(ctx context.Context, mg res if !ok { return ExternalDelete{}, errors.Errorf(errFmtUnexpectedObjectType, mg) } + return c.c.Delete(ctx, cr) } diff --git a/pkg/reconciler/providerconfig/reconciler.go b/pkg/reconciler/providerconfig/reconciler.go index 3b78402..07f2ce3 100644 --- a/pkg/reconciler/providerconfig/reconciler.go +++ b/pkg/reconciler/providerconfig/reconciler.go @@ -168,6 +168,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco 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))) + return reconcile.Result{RequeueAfter: shortWait}, nil } @@ -181,11 +182,14 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco 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))) + return reconcile.Result{RequeueAfter: shortWait}, nil } + users-- } } + log = log.WithValues("usages", users) if meta.WasDeleted(pc) { @@ -198,10 +202,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) } meta.RemoveFinalizer(pc, finalizer) + if err := r.client.Update(ctx, pc); err != nil { r.log.Debug(errUpdate, "error", err) return reconcile.Result{RequeueAfter: shortWait}, nil @@ -212,6 +218,7 @@ 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) return reconcile.Result{RequeueAfter: shortWait}, nil @@ -219,5 +226,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco // 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) } diff --git a/pkg/reconciler/providerconfig/reconciler_test.go b/pkg/reconciler/providerconfig/reconciler_test.go index 9d84f76..cf1ff99 100644 --- a/pkg/reconciler/providerconfig/reconciler_test.go +++ b/pkg/reconciler/providerconfig/reconciler_test.go @@ -50,11 +50,14 @@ func (p *ProviderConfigUsageList) GetObjectKind() schema.ObjectKind { func (p *ProviderConfigUsageList) DeepCopyObject() runtime.Object { out := &ProviderConfigUsageList{} + j, err := json.Marshal(p) //nolint:musttag // We're just using this to round-trip convert. if err != nil { panic(err) } + _ = json.Unmarshal(j, out) //nolint:musttag // We're just using this to round-trip convert. + return out } @@ -320,8 +323,8 @@ func TestReconciler(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { r := NewReconciler(tc.args.m, tc.args.of) - got, err := r.Reconcile(context.Background(), reconcile.Request{}) + got, err := r.Reconcile(context.Background(), reconcile.Request{}) if diff := cmp.Diff(tc.want.err, err, test.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 2f1983e..25d9a23 100644 --- a/pkg/reference/reference.go +++ b/pkg/reference/reference.go @@ -51,6 +51,7 @@ func FromPtrValue(v *string) string { if v == nil { return "" } + return *v } @@ -59,6 +60,7 @@ func FromFloatPtrValue(v *float64) string { if v == nil { return "" } + return strconv.FormatFloat(*v, 'f', 0, 64) } @@ -67,6 +69,7 @@ func FromIntPtrValue(v *int64) string { if v == nil { return "" } + return strconv.FormatInt(*v, 10) } @@ -75,6 +78,7 @@ func ToPtrValue(v string) *string { if v == "" { return nil } + return &v } @@ -83,10 +87,12 @@ func ToFloatPtrValue(v string) *float64 { if v == "" { return nil } + vParsed, err := strconv.ParseFloat(v, 64) if err != nil { return nil } + return &vParsed } @@ -95,10 +101,12 @@ func ToIntPtrValue(v string) *int64 { if v == "" { return nil } + vParsed, err := strconv.ParseInt(v, 10, 64) if err != nil { return nil } + return &vParsed } @@ -112,6 +120,7 @@ func FromPtrValues(v []*string) []string { for i := range v { res[i] = FromPtrValue(v[i]) } + return res } @@ -121,6 +130,7 @@ func FromFloatPtrValues(v []*float64) []string { for i := range v { res[i] = FromFloatPtrValue(v[i]) } + return res } @@ -130,6 +140,7 @@ func FromIntPtrValues(v []*int64) []string { for i := range v { res[i] = FromIntPtrValue(v[i]) } + return res } @@ -143,6 +154,7 @@ func ToPtrValues(v []string) []*string { for i := range v { res[i] = ToPtrValue(v[i]) } + return res } @@ -152,6 +164,7 @@ func ToFloatPtrValues(v []string) []*float64 { for i := range v { res[i] = ToFloatPtrValue(v[i]) } + return res } @@ -161,6 +174,7 @@ func ToIntPtrValues(v []string) []*int64 { for i := range v { res[i] = ToIntPtrValue(v[i]) } + return res } @@ -197,6 +211,7 @@ type ResolutionRequest struct { // processed. func (rr *ResolutionRequest) IsNoOp() bool { isAlways := false + if rr.Selector != nil { if rr.Selector.Policy.IsResolvePolicyAlways() { rr.Reference = nil @@ -252,6 +267,7 @@ type MultiResolutionRequest struct { // not be processed. func (rr *MultiResolutionRequest) IsNoOp() bool { isAlways := false + if rr.Selector != nil { if rr.Selector.Policy.IsResolvePolicyAlways() { rr.References = nil @@ -331,10 +347,12 @@ func (r *APIResolver) Resolve(ctx context.Context, req ResolutionRequest) (Resol if kerrors.IsNotFound(err) { return ResolutionResponse{}, getResolutionError(req.Reference.Policy, errors.Wrap(err, errGetManaged)) } + return ResolutionResponse{}, errors.Wrap(err, errGetManaged) } rsp := ResolutionResponse{ResolvedValue: req.Extract(req.To.Managed), ResolvedReference: req.Reference} + return rsp, getResolutionError(req.Reference.Policy, rsp.Validate()) } @@ -350,6 +368,7 @@ func (r *APIResolver) Resolve(ctx context.Context, req ResolutionRequest) (Resol } rsp := ResolutionResponse{ResolvedValue: req.Extract(to), ResolvedReference: &xpv1.Reference{Name: to.GetName()}} + return rsp, getResolutionError(req.Selector.Policy, rsp.Validate()) } @@ -375,14 +394,17 @@ func (r *APIResolver) ResolveMultiple(ctx context.Context, req MultiResolutionRe if kerrors.IsNotFound(err) { return MultiResolutionResponse{}, getResolutionError(req.References[i].Policy, errors.Wrap(err, errGetManaged)) } + return MultiResolutionResponse{}, errors.Wrap(err, errGetManaged) } + valueMap[req.Extract(req.To.Managed)] = req.References[i] } sortedKeys, sortedRefs := sortMapByKeys(valueMap) rsp := MultiResolutionResponse{ResolvedValues: sortedKeys, ResolvedReferences: sortedRefs} + return rsp, rsp.Validate() } @@ -403,6 +425,7 @@ func (r *APIResolver) ResolveMultiple(ctx context.Context, req MultiResolutionRe sortedKeys, sortedRefs := sortMapByKeys(valueMap) rsp := MultiResolutionResponse{ResolvedValues: sortedKeys, ResolvedReferences: sortedRefs} + return rsp, getResolutionError(req.Selector.Policy, rsp.Validate()) } @@ -410,15 +433,18 @@ func getResolutionError(p *xpv1.Policy, err error) error { if !p.IsResolutionPolicyOptional() { return err } + return nil } func sortMapByKeys(m map[string]xpv1.Reference) ([]string, []xpv1.Reference) { keys := slices.Sorted(maps.Keys(m)) + values := make([]xpv1.Reference, 0, len(keys)) for _, k := range keys { values = append(values, m[k]) } + return keys, values } @@ -429,5 +455,6 @@ func ControllersMustMatch(s *xpv1.Selector) bool { if s == nil { return false } + return s.MatchControllerRef != nil && *s.MatchControllerRef } diff --git a/pkg/reference/reference_test.go b/pkg/reference/reference_test.go index fe3961c..dd4c1d3 100644 --- a/pkg/reference/reference_test.go +++ b/pkg/reference/reference_test.go @@ -157,10 +157,12 @@ func TestResolve(t *testing.T) { ctx context.Context req ResolutionRequest } + type want struct { rsp ResolutionResponse err error } + cases := map[string]struct { reason string c client.Reader @@ -494,10 +496,12 @@ func TestResolve(t *testing.T) { for name, tc := range cases { 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 != "" { t.Errorf("\n%s\nControllersMustMatch(...): -want error, +got error:\n%s", tc.reason, diff) } + if diff := cmp.Diff(tc.want.rsp, got); diff != "" { t.Errorf("\n%s\nControllersMustMatch(...): -want, +got:\n%s", tc.reason, diff) } @@ -530,10 +534,12 @@ func TestResolveMultiple(t *testing.T) { ctx context.Context req MultiResolutionRequest } + type want struct { rsp MultiResolutionResponse err error } + cases := map[string]struct { reason string c client.Reader @@ -899,10 +905,12 @@ func TestResolveMultiple(t *testing.T) { for name, tc := range cases { 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 != "" { 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 != "" { t.Errorf("\n%s\nControllersMustMatch(...): -want, +got:\n%s", tc.reason, diff) } diff --git a/pkg/resource/api.go b/pkg/resource/api.go index 55fd6fd..b66c60b 100644 --- a/pkg/resource/api.go +++ b/pkg/resource/api.go @@ -67,6 +67,7 @@ func (a *APIPatchingApplicator) Apply(ctx context.Context, o client.Object, ao . // TODO(negz): Apply ApplyOptions here too? return errors.Wrap(a.client.Create(ctx, o), "cannot create object") } + if err != nil { return errors.Wrap(err, "cannot get object") } @@ -118,6 +119,7 @@ func (a *APIUpdatingApplicator) Apply(ctx context.Context, o client.Object, ao . // TODO(negz): Apply ApplyOptions here too? return errors.Wrap(a.client.Create(ctx, m), "cannot create object") } + if err != nil { return errors.Wrap(err, "cannot get object") } @@ -131,6 +133,7 @@ func (a *APIUpdatingApplicator) Apply(ctx context.Context, o client.Object, ao . // NOTE(hasheddan): we must set the resource version of the desired object // to that of the current or the update will always fail. m.SetResourceVersion(current.GetResourceVersion()) + return errors.Wrap(a.client.Update(ctx, m), "cannot update object") } @@ -163,7 +166,9 @@ func (a *APIFinalizer) AddFinalizer(ctx context.Context, obj Object) error { if meta.FinalizerExists(obj, a.finalizer) { return nil } + meta.AddFinalizer(obj, a.finalizer) + return errors.Wrap(a.client.Update(ctx, obj), errUpdateObject) } @@ -172,7 +177,9 @@ func (a *APIFinalizer) RemoveFinalizer(ctx context.Context, obj Object) error { if !meta.FinalizerExists(obj, a.finalizer) { return nil } + meta.RemoveFinalizer(obj, a.finalizer) + return errors.Wrap(IgnoreNotFound(a.client.Update(ctx, obj)), errUpdateObject) } diff --git a/pkg/resource/api_test.go b/pkg/resource/api_test.go index 731cee3..02aa996 100644 --- a/pkg/resource/api_test.go +++ b/pkg/resource/api_test.go @@ -142,10 +142,12 @@ func TestAPIPatchingApplicator(t *testing.T) { for name, tc := range cases { 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 != "" { 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 != "" { t.Errorf("\n%s\nApply(...): -want, +got\n%s\n", tc.reason, diff) } @@ -157,6 +159,7 @@ func TestAPIUpdatingApplicator(t *testing.T) { errBoom := errors.New("boom") desired := &object{} desired.SetName("desired") + current := &object{} current.SetName("current") @@ -270,10 +273,12 @@ func TestAPIUpdatingApplicator(t *testing.T) { for name, tc := range cases { 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 != "" { 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 != "" { t.Errorf("\n%s\nApply(...): -want, +got\n%s\n", tc.reason, diff) } @@ -328,10 +333,12 @@ func TestManagedRemoveFinalizer(t *testing.T) { for name, tc := range cases { 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 != "" { t.Errorf("api.RemoveFinalizer(...): -want error, +got error:\n%s", diff) } + if diff := cmp.Diff(tc.want.obj, tc.args.obj, test.EquateConditions()); diff != "" { t.Errorf("api.RemoveFinalizer(...) Managed: -want, +got:\n%s", diff) } @@ -386,10 +393,12 @@ func TestAPIFinalizerAdder(t *testing.T) { for name, tc := range cases { 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 != "" { t.Errorf("api.Initialize(...): -want error, +got error:\n%s", diff) } + if diff := cmp.Diff(tc.want.obj, tc.args.obj, test.EquateConditions()); diff != "" { t.Errorf("api.Initialize(...) Managed: -want, +got:\n%s", diff) } diff --git a/pkg/resource/fake/mocks.go b/pkg/resource/fake/mocks.go index ae8f80e..e80db16 100644 --- a/pkg/resource/fake/mocks.go +++ b/pkg/resource/fake/mocks.go @@ -315,11 +315,14 @@ func (o *Object) GetObjectKind() schema.ObjectKind { // DeepCopyObject returns a copy of the object as runtime.Object. func (o *Object) DeepCopyObject() runtime.Object { out := &Object{} + j, err := json.Marshal(o) if err != nil { panic(err) } + _ = json.Unmarshal(j, out) + return out } @@ -342,11 +345,14 @@ func (m *Managed) GetObjectKind() schema.ObjectKind { // DeepCopyObject returns a copy of the object as runtime.Object. func (m *Managed) DeepCopyObject() runtime.Object { out := &Managed{} + j, err := json.Marshal(m) if err != nil { panic(err) } + _ = json.Unmarshal(j, out) + return out } @@ -376,11 +382,14 @@ func (m *Composite) GetObjectKind() schema.ObjectKind { // DeepCopyObject returns a copy of the object as runtime.Object. func (m *Composite) DeepCopyObject() runtime.Object { out := &Composite{} + j, err := json.Marshal(m) if err != nil { panic(err) } + _ = json.Unmarshal(j, out) + return out } @@ -400,11 +409,14 @@ func (m *Composed) GetObjectKind() schema.ObjectKind { // DeepCopyObject returns a copy of the object as runtime.Object. func (m *Composed) DeepCopyObject() runtime.Object { out := &Composed{} + j, err := json.Marshal(m) if err != nil { panic(err) } + _ = json.Unmarshal(j, out) + return out } @@ -433,11 +445,14 @@ func (m *CompositeClaim) GetObjectKind() schema.ObjectKind { // DeepCopyObject returns a copy of the object as runtime.Object. func (m *CompositeClaim) DeepCopyObject() runtime.Object { out := &CompositeClaim{} + j, err := json.Marshal(m) if err != nil { panic(err) } + _ = json.Unmarshal(j, out) + return out } @@ -457,6 +472,7 @@ type Manager struct { func (m *Manager) Elected() <-chan struct{} { e := make(chan struct{}) close(e) + return e } @@ -490,6 +506,7 @@ func GVK(o runtime.Object) schema.GroupVersionKind { func SchemeWith(o ...runtime.Object) *runtime.Scheme { s := runtime.NewScheme() s.AddKnownTypes(GV, o...) + return s } @@ -531,11 +548,14 @@ func (m *MockConnectionSecretOwner) GetObjectKind() schema.ObjectKind { // DeepCopyObject returns a copy of the object as runtime.Object. func (m *MockConnectionSecretOwner) DeepCopyObject() runtime.Object { out := &MockConnectionSecretOwner{} + j, err := json.Marshal(m) if err != nil { panic(err) } + _ = json.Unmarshal(j, out) + return out } @@ -577,11 +597,14 @@ func (m *MockLocalConnectionSecretOwner) GetObjectKind() schema.ObjectKind { // DeepCopyObject returns a copy of the object as runtime.Object. func (m *MockLocalConnectionSecretOwner) DeepCopyObject() runtime.Object { out := &MockLocalConnectionSecretOwner{} + j, err := json.Marshal(m) if err != nil { panic(err) } + _ = json.Unmarshal(j, out) + return out } @@ -601,11 +624,14 @@ func (p *ProviderConfig) GetObjectKind() schema.ObjectKind { // DeepCopyObject returns a copy of the object as runtime.Object. func (p *ProviderConfig) DeepCopyObject() runtime.Object { out := &ProviderConfig{} + j, err := json.Marshal(p) if err != nil { panic(err) } + _ = json.Unmarshal(j, out) + return out } @@ -626,10 +652,13 @@ func (p *ProviderConfigUsage) GetObjectKind() schema.ObjectKind { // DeepCopyObject returns a copy of the object as runtime.Object. func (p *ProviderConfigUsage) DeepCopyObject() runtime.Object { out := &ProviderConfigUsage{} + j, err := json.Marshal(p) if err != nil { panic(err) } + _ = json.Unmarshal(j, out) + return out } diff --git a/pkg/resource/late_initializer.go b/pkg/resource/late_initializer.go index 0bcbb7c..8c8d3a4 100644 --- a/pkg/resource/late_initializer.go +++ b/pkg/resource/late_initializer.go @@ -52,7 +52,9 @@ func (li *LateInitializer) LateInitializeStringPtr(org *string, from *string) *s if org != nil || from == nil { return org } + li.SetChanged() + return from } @@ -61,7 +63,9 @@ func (li *LateInitializer) LateInitializeInt64Ptr(org *int64, from *int64) *int6 if org != nil || from == nil { return org } + li.SetChanged() + return from } @@ -70,7 +74,9 @@ func (li *LateInitializer) LateInitializeBoolPtr(org *bool, from *bool) *bool { if org != nil || from == nil { return org } + li.SetChanged() + return from } @@ -80,7 +86,10 @@ func (li *LateInitializer) LateInitializeTimePtr(org *metav1.Time, from *time.Ti if org != nil || from == nil { return org } + li.SetChanged() + t := metav1.NewTime(*from) + return &t } diff --git a/pkg/resource/late_initializer_test.go b/pkg/resource/late_initializer_test.go index fc1828d..b3dd762 100644 --- a/pkg/resource/late_initializer_test.go +++ b/pkg/resource/late_initializer_test.go @@ -27,14 +27,17 @@ import ( func TestLateInitializeStringPtr(t *testing.T) { s1 := "desired" s2 := "observed" + type args struct { org *string from *string } + type want struct { result *string changed bool } + cases := map[string]struct { args want @@ -74,11 +77,13 @@ func TestLateInitializeStringPtr(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { li := NewLateInitializer() + got := li.LateInitializeStringPtr(tc.org, tc.from) - if diff := cmp.Diff(tc.want.result, got); diff != "" { + if diff := cmp.Diff(tc.result, got); diff != "" { t.Errorf("LateInitializeStringPtr(...): -want, +got:\n%s", diff) } - if diff := cmp.Diff(tc.want.changed, li.IsChanged()); diff != "" { + + if diff := cmp.Diff(tc.changed, li.IsChanged()); diff != "" { t.Errorf("IsChanged(...): -want, +got:\n%s", diff) } }) @@ -88,14 +93,17 @@ func TestLateInitializeStringPtr(t *testing.T) { func TestLateInitializeInt64Ptr(t *testing.T) { i1 := int64(10) i2 := int64(20) + type args struct { org *int64 from *int64 } + type want struct { result *int64 changed bool } + cases := map[string]struct { args want @@ -135,11 +143,13 @@ func TestLateInitializeInt64Ptr(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { li := NewLateInitializer() + got := li.LateInitializeInt64Ptr(tc.org, tc.from) - if diff := cmp.Diff(tc.want.result, got); diff != "" { + if diff := cmp.Diff(tc.result, got); diff != "" { t.Errorf("LateInitializeBoolPtr(...): -want, +got:\n%s", diff) } - if diff := cmp.Diff(tc.want.changed, li.IsChanged()); diff != "" { + + if diff := cmp.Diff(tc.changed, li.IsChanged()); diff != "" { t.Errorf("IsChanged(...): -want, +got:\n%s", diff) } }) @@ -149,14 +159,17 @@ func TestLateInitializeInt64Ptr(t *testing.T) { func TestLateInitializeBoolPtr(t *testing.T) { trueVal := true falseVal := false + type args struct { org *bool from *bool } + type want struct { result *bool changed bool } + cases := map[string]struct { args want @@ -196,11 +209,13 @@ func TestLateInitializeBoolPtr(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { li := NewLateInitializer() + got := li.LateInitializeBoolPtr(tc.org, tc.from) - if diff := cmp.Diff(tc.want.result, got); diff != "" { + if diff := cmp.Diff(tc.result, got); diff != "" { t.Errorf("LateInitializeBoolPtr(...): -want, +got:\n%s", diff) } - if diff := cmp.Diff(tc.want.changed, li.IsChanged()); diff != "" { + + if diff := cmp.Diff(tc.changed, li.IsChanged()); diff != "" { t.Errorf("IsChanged(...): -want, +got:\n%s", diff) } }) @@ -211,14 +226,17 @@ func TestLateInitializeTimePtr(t *testing.T) { t1 := metav1.Now() t2 := time.Now().Add(time.Minute) t2m := metav1.NewTime(t2) + type args struct { org *metav1.Time from *time.Time } + type want struct { result *metav1.Time changed bool } + cases := map[string]struct { args want @@ -258,11 +276,13 @@ func TestLateInitializeTimePtr(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { li := NewLateInitializer() + got := li.LateInitializeTimePtr(tc.org, tc.from) - if diff := cmp.Diff(tc.want.result, got); diff != "" { + if diff := cmp.Diff(tc.result, got); diff != "" { t.Errorf("LateInitializeTimePtr(...): -want, +got:\n%s", diff) } - if diff := cmp.Diff(tc.want.changed, li.IsChanged()); diff != "" { + + if diff := cmp.Diff(tc.changed, li.IsChanged()); diff != "" { t.Errorf("IsChanged(...): -want, +got:\n%s", diff) } }) diff --git a/pkg/resource/predicates.go b/pkg/resource/predicates.go index df8457e..a77debd 100644 --- a/pkg/resource/predicates.go +++ b/pkg/resource/predicates.go @@ -72,6 +72,7 @@ func DesiredStateChanged() predicate.Predicate { // being able to ignore certain annotations. type AnnotationChangedPredicate struct { predicate.Funcs + ignored []string } @@ -80,6 +81,7 @@ func copyAnnotations(an map[string]string) map[string]string { for k, v := range an { r[k] = v } + return r } @@ -89,6 +91,7 @@ func (a AnnotationChangedPredicate) Update(e event.UpdateEvent) bool { // Update event has no old object to update return false } + if e.ObjectNew == nil { // Update event has no new object for update return false diff --git a/pkg/resource/predicates_test.go b/pkg/resource/predicates_test.go index 2354cf7..13e0b7d 100644 --- a/pkg/resource/predicates_test.go +++ b/pkg/resource/predicates_test.go @@ -34,9 +34,11 @@ func TestDesiredStateChanged(t *testing.T) { old client.Object new client.Object } + type want struct { desiredStateChanged bool } + cases := map[string]struct { args want @@ -142,11 +144,11 @@ func TestDesiredStateChanged(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { got := DesiredStateChanged().Update(event.UpdateEvent{ - ObjectOld: tc.args.old, - ObjectNew: tc.args.new, + ObjectOld: tc.old, + ObjectNew: tc.new, }) - if diff := cmp.Diff(tc.want.desiredStateChanged, got); diff != "" { + if diff := cmp.Diff(tc.desiredStateChanged, got); diff != "" { t.Errorf("DesiredStateChanged(...): -want, +got:\n%s", diff) } }) diff --git a/pkg/resource/providerconfig.go b/pkg/resource/providerconfig.go index b486a8d..7b2585d 100644 --- a/pkg/resource/providerconfig.go +++ b/pkg/resource/providerconfig.go @@ -52,6 +52,7 @@ func IsMissingReference(err error) bool { _, ok := err.(interface { MissingReference() bool }) + return ok } @@ -63,6 +64,7 @@ func ExtractEnv(_ context.Context, e EnvLookupFn, s xpv1.CommonCredentialSelecto if s.Env == nil { return nil, errors.New(errExtractEnv) } + return []byte(e(s.Env.Name)), nil } @@ -71,6 +73,7 @@ func ExtractFs(_ context.Context, fs afero.Fs, s xpv1.CommonCredentialSelectors) if s.Fs == nil { return nil, errors.New(errExtractFs) } + return afero.ReadFile(fs, s.Fs.Path) } @@ -79,10 +82,12 @@ func ExtractSecret(ctx context.Context, client client.Client, s xpv1.CommonCrede if s.SecretRef == nil { return nil, errors.New(errExtractSecretKey) } + 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 secret.Data[s.SecretRef.Key], nil } @@ -141,6 +146,7 @@ func (u *ProviderConfigUsageTracker) Track(ctx context.Context, mg Managed) erro //nolint:forcetypeassert // Will always be a PCU. pcu := u.of.DeepCopyObject().(ProviderConfigUsage) gvk := mg.GetObjectKind().GroupVersionKind() + ref := mg.GetProviderConfigReference() if ref == nil { return missingRefError{errors.New(errMissingPCRef)} @@ -163,5 +169,6 @@ func (u *ProviderConfigUsageTracker) Track(ctx context.Context, mg Managed) erro return current.(ProviderConfigUsage).GetProviderConfigReference() != pcu.GetProviderConfigReference() }), ) + return errors.Wrap(Ignore(IsNotAllowed, err), errApplyPCU) } diff --git a/pkg/resource/providerconfig_test.go b/pkg/resource/providerconfig_test.go index d3b31c7..4952576 100644 --- a/pkg/resource/providerconfig_test.go +++ b/pkg/resource/providerconfig_test.go @@ -79,6 +79,7 @@ func TestExtractEnv(t *testing.T) { if diff := cmp.Diff(tc.want.err, err, test.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 != "" { t.Errorf("\n%s\npc.ExtractEnv(...): -want, +got:\n%s\n", tc.reason, diff) } @@ -138,6 +139,7 @@ func TestExtractFs(t *testing.T) { if diff := cmp.Diff(tc.want.err, err, test.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 != "" { t.Errorf("\n%s\npc.ExtractFs(...): -want, +got:\n%s\n", tc.reason, diff) } @@ -226,6 +228,7 @@ func TestExtractSecret(t *testing.T) { if diff := cmp.Diff(tc.want.err, err, test.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 != "" { t.Errorf("\n%s\npc.ExtractSecret(...): -want, +got:\n%s\n", tc.reason, diff) } @@ -318,6 +321,7 @@ func TestTrack(t *testing.T) { for name, tc := range cases { 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 != "" { t.Errorf("\n%s\nut.Track(...): -want error, +got error:\n%s\n", tc.reason, diff) diff --git a/pkg/resource/resource.go b/pkg/resource/resource.go index 32ca71f..710360e 100644 --- a/pkg/resource/resource.go +++ b/pkg/resource/resource.go @@ -128,6 +128,7 @@ func MustCreateObject(kind schema.GroupVersionKind, oc runtime.ObjectCreater) ru if err != nil { panic(err) } + return obj } @@ -139,12 +140,15 @@ func GetKind(obj runtime.Object, ot runtime.ObjectTyper) (schema.GroupVersionKin if err != nil { return schema.GroupVersionKind{}, errors.Wrap(err, "cannot get kind of supplied object") } + if unversioned { return schema.GroupVersionKind{}, errors.New("supplied object is unversioned") } + if len(kinds) != 1 { return schema.GroupVersionKind{}, errors.New("supplied object does not have exactly one kind") } + return kinds[0], nil } @@ -156,6 +160,7 @@ func MustGetKind(obj runtime.Object, ot runtime.ObjectTyper) schema.GroupVersion if err != nil { panic(err) } + return gvk } @@ -168,6 +173,7 @@ func Ignore(is ErrorIs, err error) error { if is(err) { return nil } + return err } @@ -180,6 +186,7 @@ func IgnoreAny(err error, is ...ErrorIs) error { return nil } } + return err } @@ -215,6 +222,7 @@ type shouldRetryFunc func(error) bool // An ApplicatorWithRetry applies changes to an object, retrying on transient failures. type ApplicatorWithRetry struct { Applicator + shouldRetry shouldRetryFunc backoff wait.Backoff } @@ -285,6 +293,7 @@ func IsNotControllable(err error) bool { _, ok := err.(interface { NotControllable() bool }) + return ok } @@ -299,6 +308,7 @@ func MustBeControllableBy(u types.UID) ApplyOption { if !ok { return notControllableError{errors.Errorf("existing object is missing object metadata")} } + c := metav1.GetControllerOf(mo) if c == nil { return nil @@ -307,6 +317,7 @@ func MustBeControllableBy(u types.UID) ApplyOption { if c.UID != u { return notControllableError{errors.Errorf("existing object is not controlled by UID %q", u)} } + return nil } } @@ -329,6 +340,7 @@ func ConnectionSecretMustBeControllableBy(u types.UID) ApplyOption { if !ok { return errors.New("current resource is not a Secret") } + c := metav1.GetControllerOf(s) switch { @@ -361,6 +373,7 @@ func IsNotAllowed(err error) bool { _, ok := err.(interface { NotAllowed() bool }) + return ok } @@ -373,6 +386,7 @@ func AllowUpdateIf(fn func(current, desired runtime.Object) bool) ApplyOption { if fn(current, desired) { return nil } + return notAllowedError{errors.New("update not allowed")} } } @@ -386,7 +400,9 @@ func StoreCurrentRV(origRV *string) ApplyOption { if !ok { return errors.New("current resource is missing object metadata") } + *origRV = mo.GetResourceVersion() + return nil } } @@ -402,6 +418,7 @@ func GetExternalTags(mg Managed) map[string]string { if mg.GetProviderConfigReference() != nil && mg.GetProviderConfigReference().Name != "" { tags[ExternalResourceTagKeyProvider] = mg.GetProviderConfigReference().Name } + return tags } @@ -416,12 +433,15 @@ func FirstNAndSomeMore(n int, names []string) string { if n <= 0 { return fmt.Sprintf("%d", len(names)) } + if len(names) > n { return fmt.Sprintf("%s, and %d more", strings.Join(names[:n], ", "), len(names)-n) } + if len(names) == n { return fmt.Sprintf("%s, and %s", strings.Join(names[:n-1], ", "), names[n-1]) } + return strings.Join(names, ", ") } @@ -431,6 +451,7 @@ func StableNAndSomeMore(n int, names []string) string { cpy := make([]string, len(names)) copy(cpy, names) sort.Strings(cpy) + return FirstNAndSomeMore(n, cpy) } @@ -458,5 +479,6 @@ func AsProtobufStruct(o runtime.Object) (*structpb.Struct, error) { } s := &structpb.Struct{} + return s, errors.Wrap(s.UnmarshalJSON(b), errUnmarshalJSON) } diff --git a/pkg/resource/resource_test.go b/pkg/resource/resource_test.go index aa07848..ee4fc81 100644 --- a/pkg/resource/resource_test.go +++ b/pkg/resource/resource_test.go @@ -181,6 +181,7 @@ func TestGetKind(t *testing.T) { obj runtime.Object ot runtime.ObjectTyper } + type want struct { kind schema.GroupVersionKind err error @@ -243,6 +244,7 @@ func TestGetKind(t *testing.T) { if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { t.Errorf("GetKind(...): -want error, +got error:\n%s", diff) } + if diff := cmp.Diff(tc.want.kind, got); diff != "" { t.Errorf("GetKind(...): -want, +got:\n%s", diff) } @@ -255,6 +257,7 @@ func TestMustCreateObject(t *testing.T) { kind schema.GroupVersionKind oc runtime.ObjectCreater } + cases := map[string]struct { args args want runtime.Object @@ -285,6 +288,7 @@ func TestIgnore(t *testing.T) { is ErrorIs err error } + cases := map[string]struct { args args want error @@ -322,6 +326,7 @@ func TestIgnoreAny(t *testing.T) { is []ErrorIs err error } + cases := map[string]struct { args args want error @@ -401,7 +406,7 @@ type object struct { } func (o *object) DeepCopyObject() runtime.Object { - return &object{ObjectMeta: *o.ObjectMeta.DeepCopy()} + return &object{ObjectMeta: *o.DeepCopy()} } func TestIsNotControllable(t *testing.T) { @@ -486,8 +491,8 @@ func TestMustBeControllableBy(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { ao := MustBeControllableBy(tc.u) - err := ao(tc.args.ctx, tc.args.current, tc.args.desired) + err := ao(tc.args.ctx, tc.args.current, tc.args.desired) if diff := cmp.Diff(tc.want, err, test.EquateErrors()); diff != "" { t.Errorf("\n%s\nMustBeControllableBy(...)(...): -want error, +got error\n%s\n", tc.reason, diff) } @@ -558,8 +563,8 @@ func TestConnectionSecretMustBeControllableBy(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { ao := ConnectionSecretMustBeControllableBy(tc.u) - err := ao(tc.args.ctx, tc.args.current, tc.args.desired) + err := ao(tc.args.ctx, tc.args.current, tc.args.desired) if diff := cmp.Diff(tc.want, err, test.EquateErrors()); diff != "" { t.Errorf("\n%s\nConnectionSecretMustBeControllableBy(...)(...): -want error, +got error\n%s\n", tc.reason, diff) } @@ -600,8 +605,8 @@ func TestAllowUpdateIf(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { ao := AllowUpdateIf(tc.fn) - err := ao(tc.args.ctx, tc.args.current, tc.args.desired) + err := ao(tc.args.ctx, tc.args.current, tc.args.desired) if diff := cmp.Diff(tc.want, err, test.EquateErrors()); diff != "" { t.Errorf("\n%s\nAllowUpdateIf(...)(...): -want error, +got error\n%s\n", tc.reason, diff) } @@ -645,7 +650,6 @@ func Test_notControllableError_NotControllable(t *testing.T) { err := notControllableError{ errors.New("test-error"), } - if !err.NotControllable() { t.Errorf("NotControllable(): false") } @@ -656,7 +660,6 @@ func Test_notAllowedError_NotAllowed(t *testing.T) { err := notAllowedError{ errors.New("test-error"), } - if !err.NotAllowed() { t.Errorf("NotAllowed(): false") } @@ -884,6 +887,7 @@ func TestFirstNAndSomeMore(t *testing.T) { n int names []string } + tests := []struct { name string args args diff --git a/pkg/resource/unstructured/claim/claim.go b/pkg/resource/unstructured/claim/claim.go index b0c7283..d3793be 100644 --- a/pkg/resource/unstructured/claim/claim.go +++ b/pkg/resource/unstructured/claim/claim.go @@ -53,6 +53,7 @@ func New(opts ...Option) *Unstructured { for _, f := range opts { f(c) } + return c } @@ -75,6 +76,7 @@ func (c *Unstructured) GetCompositionSelector() *metav1.LabelSelector { if err := fieldpath.Pave(c.Object).GetValueInto("spec.compositionSelector", out); err != nil { return nil } + return out } @@ -89,6 +91,7 @@ func (c *Unstructured) GetCompositionReference() *corev1.ObjectReference { if err := fieldpath.Pave(c.Object).GetValueInto("spec.compositionRef", out); err != nil { return nil } + return out } @@ -103,6 +106,7 @@ func (c *Unstructured) GetCompositionRevisionReference() *corev1.LocalObjectRefe if err := fieldpath.Pave(c.Object).GetValueInto("spec.compositionRevisionRef", out); err != nil { return nil } + return out } @@ -117,6 +121,7 @@ func (c *Unstructured) GetCompositionRevisionSelector() *metav1.LabelSelector { if err := fieldpath.Pave(c.Object).GetValueInto("spec.compositionRevisionSelector", out); err != nil { return nil } + return out } @@ -136,7 +141,9 @@ func (c *Unstructured) GetCompositionUpdatePolicy() *xpv1.UpdatePolicy { if err != nil { return nil } + out := xpv1.UpdatePolicy(p) + return &out } @@ -151,7 +158,9 @@ func (c *Unstructured) GetCompositeDeletePolicy() *xpv1.CompositeDeletePolicy { if err != nil { return nil } + out := xpv1.CompositeDeletePolicy(p) + return &out } @@ -161,6 +170,7 @@ func (c *Unstructured) GetResourceReference() *reference.Composite { if err := fieldpath.Pave(c.Object).GetValueInto("spec.resourceRef", out); err != nil { return nil } + return out } @@ -185,6 +195,7 @@ func (c *Unstructured) GetWriteConnectionSecretToReference() *xpv1.LocalSecretRe if err := fieldpath.Pave(c.Object).GetValueInto("spec.writeConnectionSecretToRef", out); err != nil { return nil } + return out } @@ -200,6 +211,7 @@ func (c *Unstructured) GetCondition(ct xpv1.ConditionType) xpv1.Condition { if err := fieldpath.Pave(c.Object).GetValueInto("status", &conditioned); err != nil { return xpv1.Condition{} } + return conditioned.GetCondition(ct) } @@ -218,6 +230,7 @@ func (c *Unstructured) GetConnectionDetailsLastPublishedTime() *metav1.Time { if err := fieldpath.Pave(c.Object).GetValueInto("status.connectionDetails.lastPublishedTime", out); err != nil { return nil } + return out } @@ -238,5 +251,6 @@ func (c *Unstructured) SetObservedGeneration(generation int64) { func (c *Unstructured) GetObservedGeneration() int64 { status := &xpv1.ObservedStatus{} _ = fieldpath.Pave(c.Object).GetValueInto("status", status) + return status.GetObservedGeneration() } diff --git a/pkg/resource/unstructured/claim/claim_test.go b/pkg/resource/unstructured/claim/claim_test.go index b267cac..ef77a34 100644 --- a/pkg/resource/unstructured/claim/claim_test.go +++ b/pkg/resource/unstructured/claim/claim_test.go @@ -114,6 +114,7 @@ func TestConditions(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { tc.u.SetConditions(tc.set...) + got := tc.u.GetCondition(tc.get) if diff := cmp.Diff(tc.want, got); diff != "" { t.Errorf("\n%s\nu.GetCondition(%s): -want, +got:\n%s", tc.reason, tc.get, diff) @@ -139,6 +140,7 @@ func TestCompositionSelector(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { tc.u.SetCompositionSelector(tc.set) + got := tc.u.GetCompositionSelector() if diff := cmp.Diff(tc.want, got); diff != "" { t.Errorf("\nu.GetCompositionSelector(): -want, +got:\n%s", diff) @@ -164,6 +166,7 @@ func TestCompositionReference(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { tc.u.SetCompositionReference(tc.set) + got := tc.u.GetCompositionReference() if diff := cmp.Diff(tc.want, got); diff != "" { t.Errorf("\nu.GetCompositionReference(): -want, +got:\n%s", diff) @@ -189,6 +192,7 @@ func TestCompositionRevisionReference(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { tc.u.SetCompositionRevisionReference(tc.set) + got := tc.u.GetCompositionRevisionReference() if diff := cmp.Diff(tc.want, got); diff != "" { t.Errorf("\nu.GetCompositionRevisionReference(): -want, +got:\n%s", diff) @@ -214,6 +218,7 @@ func TestCompositionRevisionSelector(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { tc.u.SetCompositionRevisionSelector(tc.set) + got := tc.u.GetCompositionRevisionSelector() if diff := cmp.Diff(tc.want, got); diff != "" { t.Errorf("\nu.GetCompositionRevisionSelector(): -want, +got:\n%s", diff) @@ -239,6 +244,7 @@ func TestCompositionUpdatePolicy(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { tc.u.SetCompositionUpdatePolicy(tc.set) + got := tc.u.GetCompositionUpdatePolicy() if diff := cmp.Diff(tc.want, got); diff != "" { t.Errorf("\nu.GetCompositionUpdatePolicy(): -want, +got:\n%s", diff) @@ -264,6 +270,7 @@ func TestCompositeDeletePolicy(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { tc.u.SetCompositeDeletePolicy(tc.set) + got := tc.u.GetCompositeDeletePolicy() if diff := cmp.Diff(tc.want, got); diff != "" { t.Errorf("\nu.GetCompositeDeletePolicy(): -want, +got:\n%s", diff) @@ -289,6 +296,7 @@ func TestResourceReference(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { tc.u.SetResourceReference(tc.set) + got := tc.u.GetResourceReference() if diff := cmp.Diff(tc.want, got); diff != "" { t.Errorf("\nu.GetResourceReference(): -want, +got:\n%s", diff) @@ -304,6 +312,7 @@ func TestClaimReference(t *testing.T) { u.SetNamespace(ref.Namespace) u.SetAPIVersion(ref.APIVersion) u.SetKind(ref.Kind) + got := u.GetReference() if diff := cmp.Diff(ref, got); diff != "" { t.Errorf("\nu.GetClaimReference(): -want, +got:\n%s", diff) @@ -327,6 +336,7 @@ func TestWriteConnectionSecretToReference(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { tc.u.SetWriteConnectionSecretToReference(tc.set) + got := tc.u.GetWriteConnectionSecretToReference() if diff := cmp.Diff(tc.want, got); diff != "" { t.Errorf("\nu.GetWriteConnectionSecretToReference(): -want, +got:\n%s", diff) @@ -344,6 +354,7 @@ func TestConnectionDetailsLastPublishedTime(t *testing.T) { out := &metav1.Time{} j, _ := json.Marshal(t) //nolint:errchkjson // No encoding issue in practice. _ = json.Unmarshal(j, out) + return out } @@ -362,6 +373,7 @@ func TestConnectionDetailsLastPublishedTime(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { tc.u.SetConnectionDetailsLastPublishedTime(tc.set) + got := tc.u.GetConnectionDetailsLastPublishedTime() if diff := cmp.Diff(tc.want, got); diff != "" { t.Errorf("\nu.GetConnectionDetailsLastPublishedTime(): -want, +got:\n%s", diff) diff --git a/pkg/resource/unstructured/client.go b/pkg/resource/unstructured/client.go index 655ef1b..3af3a02 100644 --- a/pkg/resource/unstructured/client.go +++ b/pkg/resource/unstructured/client.go @@ -59,6 +59,7 @@ func (c *WrapperClient) Get(ctx context.Context, key client.ObjectKey, obj clien if u, ok := obj.(Wrapper); ok { return c.kube.Get(ctx, key, u.GetUnstructured(), opts...) } + return c.kube.Get(ctx, key, obj, opts...) } @@ -69,6 +70,7 @@ func (c *WrapperClient) List(ctx context.Context, list client.ObjectList, opts . if u, ok := list.(ListWrapper); ok { return c.kube.List(ctx, u.GetUnstructuredList(), opts...) } + return c.kube.List(ctx, list, opts...) } @@ -77,6 +79,7 @@ func (c *WrapperClient) Create(ctx context.Context, obj client.Object, opts ...c if u, ok := obj.(Wrapper); ok { return c.kube.Create(ctx, u.GetUnstructured(), opts...) } + return c.kube.Create(ctx, obj, opts...) } @@ -85,6 +88,7 @@ func (c *WrapperClient) Delete(ctx context.Context, obj client.Object, opts ...c if u, ok := obj.(Wrapper); ok { return c.kube.Delete(ctx, u.GetUnstructured(), opts...) } + return c.kube.Delete(ctx, obj, opts...) } @@ -94,6 +98,7 @@ func (c *WrapperClient) Update(ctx context.Context, obj client.Object, opts ...c if u, ok := obj.(Wrapper); ok { return c.kube.Update(ctx, u.GetUnstructured(), opts...) } + return c.kube.Update(ctx, obj, opts...) } @@ -103,6 +108,7 @@ func (c *WrapperClient) Patch(ctx context.Context, obj client.Object, patch clie if u, ok := obj.(Wrapper); ok { return c.kube.Patch(ctx, u.GetUnstructured(), patch, opts...) } + return c.kube.Patch(ctx, obj, patch, opts...) } @@ -111,6 +117,7 @@ func (c *WrapperClient) DeleteAllOf(ctx context.Context, obj client.Object, opts if u, ok := obj.(Wrapper); ok { return c.kube.DeleteAllOf(ctx, u.GetUnstructured(), opts...) } + return c.kube.DeleteAllOf(ctx, obj, opts...) } @@ -142,6 +149,7 @@ func (c *WrapperClient) GroupVersionKindFor(obj runtime.Object) (schema.GroupVer if u, ok := obj.(Wrapper); ok { return c.kube.GroupVersionKindFor(u.GetUnstructured()) } + return c.kube.GroupVersionKindFor(obj) } @@ -150,6 +158,7 @@ func (c *WrapperClient) IsObjectNamespaced(obj runtime.Object) (bool, error) { if u, ok := obj.(Wrapper); ok { return c.kube.IsObjectNamespaced(u.GetUnstructured()) } + return c.kube.IsObjectNamespaced(obj) } @@ -165,6 +174,7 @@ func (c *wrapperStatusClient) Create(ctx context.Context, obj, subResource clien if u, ok := obj.(Wrapper); ok { return c.kube.Create(ctx, u.GetUnstructured(), subResource, opts...) } + return c.kube.Create(ctx, obj, subResource, opts...) } @@ -175,6 +185,7 @@ func (c *wrapperStatusClient) Update(ctx context.Context, obj client.Object, opt if u, ok := obj.(Wrapper); ok { return c.kube.Update(ctx, u.GetUnstructured(), opts...) } + return c.kube.Update(ctx, obj, opts...) } @@ -185,5 +196,6 @@ func (c *wrapperStatusClient) Patch(ctx context.Context, obj client.Object, patc if u, ok := obj.(Wrapper); ok { return c.kube.Patch(ctx, u.GetUnstructured(), patch, opts...) } + return c.kube.Patch(ctx, obj, patch, opts...) } diff --git a/pkg/resource/unstructured/client_test.go b/pkg/resource/unstructured/client_test.go index a68c30d..dbef7f4 100644 --- a/pkg/resource/unstructured/client_test.go +++ b/pkg/resource/unstructured/client_test.go @@ -79,6 +79,7 @@ func TestGet(t *testing.T) { key client.ObjectKey obj client.Object } + cases := map[string]struct { c client.Client args args @@ -107,6 +108,7 @@ func TestGet(t *testing.T) { for name, tc := range cases { 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 != "" { t.Errorf("\nc.Get(...): -want error, +got error:\n %s", diff) @@ -120,6 +122,7 @@ func TestList(t *testing.T) { ctx context.Context obj client.ObjectList } + cases := map[string]struct { c client.Client args args @@ -150,6 +153,7 @@ func TestList(t *testing.T) { for name, tc := range cases { 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 != "" { t.Errorf("\nc.List(...): -want error, +got error:\n %s", diff) @@ -163,6 +167,7 @@ func TestCreate(t *testing.T) { ctx context.Context obj client.Object } + cases := map[string]struct { c client.Client args args @@ -191,6 +196,7 @@ func TestCreate(t *testing.T) { for name, tc := range cases { 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 != "" { t.Errorf("\nc.Create(...): -want error, +got error:\n %s", diff) @@ -204,6 +210,7 @@ func TestDelete(t *testing.T) { ctx context.Context obj client.Object } + cases := map[string]struct { c client.Client args args @@ -232,6 +239,7 @@ func TestDelete(t *testing.T) { for name, tc := range cases { 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 != "" { t.Errorf("\nc.Delete(...): -want error, +got error:\n %s", diff) @@ -245,6 +253,7 @@ func TestUpdate(t *testing.T) { ctx context.Context obj client.Object } + cases := map[string]struct { c client.Client args args @@ -273,6 +282,7 @@ func TestUpdate(t *testing.T) { for name, tc := range cases { 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 != "" { t.Errorf("\nc.Update(...): -want error, +got error:\n %s", diff) @@ -287,6 +297,7 @@ func TestPatch(t *testing.T) { obj client.Object patch client.Patch } + cases := map[string]struct { c client.Client args args @@ -315,6 +326,7 @@ func TestPatch(t *testing.T) { for name, tc := range cases { 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 != "" { t.Errorf("\nc.Patch(...): -want error, +got error:\n %s", diff) @@ -328,6 +340,7 @@ func TestDeleteAllOf(t *testing.T) { ctx context.Context obj client.Object } + cases := map[string]struct { c client.Client args args @@ -356,6 +369,7 @@ func TestDeleteAllOf(t *testing.T) { for name, tc := range cases { 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 != "" { t.Errorf("\nc.DeleteAllOf(...): -want error, +got error:\n %s", diff) @@ -370,6 +384,7 @@ func TestStatusCreate(t *testing.T) { obj client.Object sub client.Object } + cases := map[string]struct { c client.Client args args @@ -398,6 +413,7 @@ func TestStatusCreate(t *testing.T) { for name, tc := range cases { 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 != "" { t.Errorf("\nc.Status().Create(...): -want error, +got error:\n %s", diff) @@ -411,6 +427,7 @@ func TestStatusUpdate(t *testing.T) { ctx context.Context obj client.Object } + cases := map[string]struct { c client.Client args args @@ -439,6 +456,7 @@ func TestStatusUpdate(t *testing.T) { for name, tc := range cases { 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 != "" { t.Errorf("\nc.Status().Update(...): -want error, +got error:\n %s", diff) @@ -453,6 +471,7 @@ func TestStatusPatch(t *testing.T) { obj client.Object patch client.Patch } + cases := map[string]struct { c client.Client args args @@ -481,6 +500,7 @@ func TestStatusPatch(t *testing.T) { for name, tc := range cases { 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 != "" { t.Errorf("\nc.StatusPatch(...): -want error, +got error:\n %s", diff) diff --git a/pkg/resource/unstructured/composed/composed.go b/pkg/resource/unstructured/composed/composed.go index 2b7a408..daf37ca 100644 --- a/pkg/resource/unstructured/composed/composed.go +++ b/pkg/resource/unstructured/composed/composed.go @@ -54,6 +54,7 @@ func New(opts ...Option) *Unstructured { for _, f := range opts { f(cr) } + return cr } @@ -77,6 +78,7 @@ func (cr *Unstructured) GetCondition(ct xpv1.ConditionType) xpv1.Condition { if err := fieldpath.Pave(cr.Object).GetValueInto("status", &conditioned); err != nil { return xpv1.Condition{} } + return conditioned.GetCondition(ct) } @@ -95,6 +97,7 @@ func (cr *Unstructured) GetWriteConnectionSecretToReference() *xpv1.SecretRefere if err := fieldpath.Pave(cr.Object).GetValueInto("spec.writeConnectionSecretToRef", out); err != nil { return nil } + return out } @@ -110,6 +113,7 @@ func (cr *Unstructured) OwnedBy(u types.UID) bool { return true } } + return false } @@ -142,6 +146,7 @@ func NewList(opts ...ListOption) *UnstructuredList { for _, f := range opts { f(cr) } + return cr } @@ -167,5 +172,6 @@ func (cr *Unstructured) SetObservedGeneration(generation int64) { func (cr *Unstructured) GetObservedGeneration() int64 { status := &xpv1.ObservedStatus{} _ = fieldpath.Pave(cr.Object).GetValueInto("status", status) + return status.GetObservedGeneration() } diff --git a/pkg/resource/unstructured/composed/composed_test.go b/pkg/resource/unstructured/composed/composed_test.go index e54e691..c8313a8 100644 --- a/pkg/resource/unstructured/composed/composed_test.go +++ b/pkg/resource/unstructured/composed/composed_test.go @@ -114,6 +114,7 @@ func TestConditions(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { tc.u.SetConditions(tc.set...) + got := tc.u.GetCondition(tc.get) if diff := cmp.Diff(tc.want, got); diff != "" { t.Errorf("\n%s\nu.GetCondition(%s): -want, +got:\n%s", tc.reason, tc.get, diff) @@ -139,6 +140,7 @@ func TestWriteConnectionSecretToReference(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { tc.u.SetWriteConnectionSecretToReference(tc.set) + got := tc.u.GetWriteConnectionSecretToReference() if diff := cmp.Diff(tc.want, got); diff != "" { t.Errorf("\nu.GetWriteConnectionSecretToReference(): -want, +got:\n%s", diff) diff --git a/pkg/resource/unstructured/composite/composite.go b/pkg/resource/unstructured/composite/composite.go index ea83189..dc7d11c 100644 --- a/pkg/resource/unstructured/composite/composite.go +++ b/pkg/resource/unstructured/composite/composite.go @@ -76,6 +76,7 @@ func New(opts ...Option) *Unstructured { for _, f := range opts { f(c) } + return c } @@ -105,6 +106,7 @@ func (c *Unstructured) GetCompositionSelector() *metav1.LabelSelector { if err := fieldpath.Pave(c.Object).GetValueInto(path, out); err != nil { return nil } + return out } @@ -129,6 +131,7 @@ func (c *Unstructured) GetCompositionReference() *corev1.ObjectReference { if err := fieldpath.Pave(c.Object).GetValueInto(path, out); err != nil { return nil } + return out } @@ -153,6 +156,7 @@ func (c *Unstructured) GetCompositionRevisionReference() *corev1.LocalObjectRefe if err := fieldpath.Pave(c.Object).GetValueInto(path, out); err != nil { return nil } + return out } @@ -177,6 +181,7 @@ func (c *Unstructured) GetCompositionRevisionSelector() *metav1.LabelSelector { if err := fieldpath.Pave(c.Object).GetValueInto(path, out); err != nil { return nil } + return out } @@ -211,7 +216,9 @@ func (c *Unstructured) GetCompositionUpdatePolicy() *xpv1.UpdatePolicy { if err != nil { return nil } + out := xpv1.UpdatePolicy(p) + return &out } @@ -226,6 +233,7 @@ func (c *Unstructured) GetClaimReference() *reference.Claim { if err := fieldpath.Pave(c.Object).GetValueInto("spec.claimRef", out); err != nil { return nil } + return out } @@ -248,6 +256,7 @@ func (c *Unstructured) GetResourceReferences() []corev1.ObjectReference { out := &[]corev1.ObjectReference{} _ = fieldpath.Pave(c.Object).GetValueInto(path, out) + return *out } @@ -259,6 +268,7 @@ func (c *Unstructured) SetResourceReferences(refs []corev1.ObjectReference) { } empty := corev1.ObjectReference{} + filtered := make([]corev1.ObjectReference, 0, len(refs)) for _, ref := range refs { // TODO(negz): Ask muvaf to explain what this is working around. :) @@ -266,8 +276,10 @@ func (c *Unstructured) SetResourceReferences(refs []corev1.ObjectReference) { if ref.String() == empty.String() { continue } + filtered = append(filtered, ref) } + _ = fieldpath.Pave(c.Object).SetValue(path, filtered) } @@ -301,6 +313,7 @@ func (c *Unstructured) GetWriteConnectionSecretToReference() *xpv1.SecretReferen if err := fieldpath.Pave(c.Object).GetValueInto("spec.writeConnectionSecretToRef", out); err != nil { return nil } + return out } @@ -321,6 +334,7 @@ func (c *Unstructured) GetCondition(ct xpv1.ConditionType) xpv1.Condition { if err := fieldpath.Pave(c.Object).GetValueInto("status", &conditioned); err != nil { return xpv1.Condition{} } + return conditioned.GetCondition(ct) } @@ -338,6 +352,7 @@ func (c *Unstructured) GetConditions() []xpv1.Condition { conditioned := xpv1.ConditionedStatus{} // The path is directly `status` because conditions are inline. _ = fieldpath.Pave(c.Object).GetValueInto("status", &conditioned) + return conditioned.Conditions } @@ -352,6 +367,7 @@ func (c *Unstructured) GetConnectionDetailsLastPublishedTime() *metav1.Time { if err := fieldpath.Pave(c.Object).GetValueInto(path, out); err != nil { return nil } + return out } @@ -377,6 +393,7 @@ func (c *Unstructured) SetObservedGeneration(generation int64) { func (c *Unstructured) GetObservedGeneration() int64 { status := &xpv1.ObservedStatus{} _ = fieldpath.Pave(c.Object).GetValueInto("status", status) + return status.GetObservedGeneration() } @@ -387,7 +404,9 @@ func (c *Unstructured) SetClaimConditionTypes(in ...xpv1.ConditionType) error { if c.Schema != SchemaLegacy { return nil } + ts := c.GetClaimConditionTypes() + m := make(map[xpv1.ConditionType]bool, len(ts)) for _, t := range ts { m[t] = true @@ -397,13 +416,17 @@ func (c *Unstructured) SetClaimConditionTypes(in ...xpv1.ConditionType) error { if xpv1.IsSystemConditionType(t) { return errors.Errorf("cannot set system condition %s as a claim condition", t) } + if m[t] { continue } + m[t] = true ts = append(ts, t) } + _ = fieldpath.Pave(c.Object).SetValue("status.claimConditionTypes", ts) + return nil } @@ -413,7 +436,9 @@ func (c *Unstructured) GetClaimConditionTypes() []xpv1.ConditionType { if c.Schema != SchemaLegacy { return nil } + cs := []xpv1.ConditionType{} _ = fieldpath.Pave(c.Object).GetValueInto("status.claimConditionTypes", &cs) + return cs } diff --git a/pkg/resource/unstructured/composite/composite_test.go b/pkg/resource/unstructured/composite/composite_test.go index 4f85471..c6032be 100644 --- a/pkg/resource/unstructured/composite/composite_test.go +++ b/pkg/resource/unstructured/composite/composite_test.go @@ -231,6 +231,7 @@ func TestCompositionSelector(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { tc.u.SetCompositionSelector(tc.set) + got := tc.u.GetCompositionSelector() if diff := cmp.Diff(tc.want, got); diff != "" { t.Errorf("\nu.GetCompositionSelector(): -want, +got:\n%s", diff) @@ -256,6 +257,7 @@ func TestCompositionReference(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { tc.u.SetCompositionReference(tc.set) + got := tc.u.GetCompositionReference() if diff := cmp.Diff(tc.want, got); diff != "" { t.Errorf("\nu.GetCompositionReference(): -want, +got:\n%s", diff) @@ -281,6 +283,7 @@ func TestCompositionRevisionReference(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { tc.u.SetCompositionRevisionReference(tc.set) + got := tc.u.GetCompositionRevisionReference() if diff := cmp.Diff(tc.want, got); diff != "" { t.Errorf("\nu.GetCompositionRevisionReference(): -want, +got:\n%s", diff) @@ -306,6 +309,7 @@ func TestCompositionRevisionSelector(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { tc.u.SetCompositionRevisionSelector(tc.set) + got := tc.u.GetCompositionRevisionSelector() if diff := cmp.Diff(tc.want, got); diff != "" { t.Errorf("\nu.GetCompositionRevisionSelector(): -want, +got:\n%s", diff) @@ -331,6 +335,7 @@ func TestCompositionUpdatePolicy(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { tc.u.SetCompositionUpdatePolicy(tc.set) + got := tc.u.GetCompositionUpdatePolicy() if diff := cmp.Diff(tc.want, got); diff != "" { t.Errorf("\nu.GetCompositionUpdatePolicy(): -want, +got:\n%s", diff) @@ -356,6 +361,7 @@ func TestClaimReference(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { tc.u.SetClaimReference(tc.set) + got := tc.u.GetClaimReference() if diff := cmp.Diff(tc.want, got); diff != "" { t.Errorf("\nu.GetClaimReference(): -want, +got:\n%s", diff) @@ -381,6 +387,7 @@ func TestResourceReferences(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { tc.u.SetResourceReferences(tc.set) + got := tc.u.GetResourceReferences() if diff := cmp.Diff(tc.want, got); diff != "" { t.Errorf("\nu.GetResourceReferences(): -want, +got:\n%s", diff) @@ -406,6 +413,7 @@ func TestWriteConnectionSecretToReference(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { tc.u.SetWriteConnectionSecretToReference(tc.set) + got := tc.u.GetWriteConnectionSecretToReference() if diff := cmp.Diff(tc.want, got); diff != "" { t.Errorf("\nu.GetWriteConnectionSecretToReference(): -want, +got:\n%s", diff) @@ -423,6 +431,7 @@ func TestConnectionDetailsLastPublishedTime(t *testing.T) { out := &metav1.Time{} j, _ := json.Marshal(t) //nolint:errchkjson // No encoding error in practice. _ = json.Unmarshal(j, out) + return out } @@ -441,6 +450,7 @@ func TestConnectionDetailsLastPublishedTime(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { tc.u.SetConnectionDetailsLastPublishedTime(tc.set) + got := tc.u.GetConnectionDetailsLastPublishedTime() if diff := cmp.Diff(tc.want, got); diff != "" { t.Errorf("\nu.GetConnectionDetailsLastPublishedTime(): -want, +got:\n%s", diff) diff --git a/pkg/statemetrics/mr_state_metrics.go b/pkg/statemetrics/mr_state_metrics.go index a42d735..e082e63 100644 --- a/pkg/statemetrics/mr_state_metrics.go +++ b/pkg/statemetrics/mr_state_metrics.go @@ -114,6 +114,7 @@ func (r *MRStateRecorder) Record(ctx context.Context) error { r.metrics.Exists.With(labels).Set(float64(len(mrs))) var numReady, numSynced float64 = 0, 0 + for _, o := range mrs { if o.GetCondition(xpv1.TypeReady).Status == corev1.ConditionTrue { numReady++ @@ -133,6 +134,7 @@ func (r *MRStateRecorder) Record(ctx context.Context) error { // Start records state of managed resources with given interval. func (r *MRStateRecorder) Start(ctx context.Context) error { ticker := time.NewTicker(r.interval) + for { select { case <-ticker.C: diff --git a/pkg/test/cmp.go b/pkg/test/cmp.go index a7414c1..5d31568 100644 --- a/pkg/test/cmp.go +++ b/pkg/test/cmp.go @@ -40,6 +40,7 @@ func EquateErrors() cmp.Option { } av := reflect.ValueOf(a) + bv := reflect.ValueOf(b) if av.Type() != bv.Type() { return false diff --git a/pkg/test/fake.go b/pkg/test/fake.go index 4f1677e..89dd0c4 100644 --- a/pkg/test/fake.go +++ b/pkg/test/fake.go @@ -89,6 +89,7 @@ func NewMockGetFn(err error, ofn ...ObjectFn) MockGetFn { return err } } + return err } } @@ -101,6 +102,7 @@ func NewMockListFn(err error, ofn ...ObjectListFn) MockListFn { return err } } + return err } } @@ -113,6 +115,7 @@ func NewMockCreateFn(err error, ofn ...ObjectFn) MockCreateFn { return err } } + return err } } @@ -125,6 +128,7 @@ func NewMockDeleteFn(err error, ofn ...ObjectFn) MockDeleteFn { return err } } + return err } } @@ -137,6 +141,7 @@ func NewMockDeleteAllOfFn(err error, ofn ...ObjectFn) MockDeleteAllOfFn { return err } } + return err } } @@ -149,6 +154,7 @@ func NewMockUpdateFn(err error, ofn ...ObjectFn) MockUpdateFn { return err } } + return err } } @@ -161,6 +167,7 @@ func NewMockPatchFn(err error, ofn ...ObjectFn) MockPatchFn { return err } } + return err } } @@ -173,6 +180,7 @@ func NewMockSubResourceCreateFn(err error, ofn ...ObjectFn) MockSubResourceCreat return err } } + return err } } @@ -185,6 +193,7 @@ func NewMockSubResourceUpdateFn(err error, ofn ...ObjectFn) MockSubResourceUpdat return err } } + return err } } @@ -197,6 +206,7 @@ func NewMockSubResourcePatchFn(err error, ofn ...ObjectFn) MockSubResourcePatchF return err } } + return err } } @@ -216,6 +226,7 @@ func NewMockGroupVersionKindForFn(err error, gvk schema.GroupVersionKind, rofn . return gvk, err } } + return gvk, err } } @@ -228,6 +239,7 @@ func NewMockIsObjectNamespacedFn(err error, isNamespaced bool, rofn ...RuntimeOb return isNamespaced, err } } + return isNamespaced, err } } diff --git a/pkg/webhook/mutator.go b/pkg/webhook/mutator.go index 9a7b67d..7622748 100644 --- a/pkg/webhook/mutator.go +++ b/pkg/webhook/mutator.go @@ -45,6 +45,7 @@ func NewMutator(opts ...MutatorOption) *Mutator { for _, f := range opts { f(m) } + return m } @@ -62,5 +63,6 @@ func (m *Mutator) Default(ctx context.Context, obj runtime.Object) error { return err } } + return nil } diff --git a/pkg/webhook/mutator_test.go b/pkg/webhook/mutator_test.go index 8ad7e4c..14f165a 100644 --- a/pkg/webhook/mutator_test.go +++ b/pkg/webhook/mutator_test.go @@ -36,9 +36,11 @@ func TestDefault(t *testing.T) { obj runtime.Object fns []MutateFn } + type want struct { err error } + cases := map[string]struct { reason string args @@ -71,8 +73,9 @@ func TestDefault(t *testing.T) { for name, tc := range cases { 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 != "" { + + err := v.Default(context.TODO(), tc.obj) + if diff := cmp.Diff(tc.err, err, test.EquateErrors()); diff != "" { t.Errorf("\n%s\nDefault(...): -want, +got\n%s\n", tc.reason, diff) } }) diff --git a/pkg/webhook/validator.go b/pkg/webhook/validator.go index a474401..4e41f99 100644 --- a/pkg/webhook/validator.go +++ b/pkg/webhook/validator.go @@ -69,6 +69,7 @@ func NewValidator(opts ...ValidatorOption) *Validator { for _, f := range opts { f(vc) } + return vc } @@ -82,38 +83,47 @@ type Validator struct { // ValidateCreate runs functions in creation chain in order. func (vc *Validator) ValidateCreate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { warnings := []string{} + for _, f := range vc.CreationChain { warns, err := f(ctx, obj) if err != nil { return append(warnings, warns...), err } + warnings = append(warnings, warns...) } + return warnings, nil } // ValidateUpdate runs functions in update chain in order. func (vc *Validator) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) { warnings := []string{} + for _, f := range vc.UpdateChain { warns, err := f(ctx, oldObj, newObj) if err != nil { return append(warnings, warns...), err } + warnings = append(warnings, warns...) } + return warnings, nil } // ValidateDelete runs functions in deletion chain in order. func (vc *Validator) ValidateDelete(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { warnings := []string{} + for _, f := range vc.DeletionChain { warns, err := f(ctx, obj) if err != nil { return append(warnings, warns...), err } + warnings = append(warnings, warns...) } + return warnings, nil } diff --git a/pkg/webhook/validator_test.go b/pkg/webhook/validator_test.go index 63481f6..ee40b32 100644 --- a/pkg/webhook/validator_test.go +++ b/pkg/webhook/validator_test.go @@ -44,10 +44,12 @@ func TestValidateCreate(t *testing.T) { obj runtime.Object fns []ValidateCreateFn } + type want struct { err error warnings admission.Warnings } + cases := map[string]struct { reason string args @@ -107,11 +109,13 @@ func TestValidateCreate(t *testing.T) { for name, tc := range cases { 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 != "" { + + warn, err := v.ValidateCreate(context.TODO(), tc.obj) + if diff := cmp.Diff(tc.err, err, test.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 != "" { + + if diff := cmp.Diff(tc.warnings, warn, cmpopts.EquateEmpty()); diff != "" { t.Errorf("\n%s\nValidateCreate(...): -want warnings, +got warnings\n%s\n", tc.reason, diff) } }) @@ -124,10 +128,12 @@ func TestValidateUpdate(t *testing.T) { newObj runtime.Object fns []ValidateUpdateFn } + type want struct { err error warnings admission.Warnings } + cases := map[string]struct { reason string args @@ -187,11 +193,13 @@ func TestValidateUpdate(t *testing.T) { for name, tc := range cases { 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 != "" { + + warn, err := v.ValidateUpdate(context.TODO(), tc.oldObj, tc.newObj) + if diff := cmp.Diff(tc.err, err, test.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 != "" { + + if diff := cmp.Diff(tc.warnings, warn, cmpopts.EquateEmpty()); diff != "" { t.Errorf("\n%s\nValidateUpdate(...): -want warnings, +got warnings\n%s\n", tc.reason, diff) } }) @@ -203,10 +211,12 @@ func TestValidateDelete(t *testing.T) { obj runtime.Object fns []ValidateDeleteFn } + type want struct { err error warnings admission.Warnings } + cases := map[string]struct { reason string args @@ -266,11 +276,13 @@ func TestValidateDelete(t *testing.T) { for name, tc := range cases { 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 != "" { + + warn, err := v.ValidateDelete(context.TODO(), tc.obj) + if diff := cmp.Diff(tc.err, err, test.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 != "" { + + if diff := cmp.Diff(tc.warnings, warn, cmpopts.EquateEmpty()); diff != "" { t.Errorf("\n%s\nValidateDelete(...): -want warnings, +got warnings\n%s\n", tc.reason, diff) } })