From 911508b75e15ffc57d1711d6a52fe6d87268af92 Mon Sep 17 00:00:00 2001 From: "Maximilian Blatt (external expert on behalf of DB Netz)" Date: Mon, 4 Sep 2023 11:48:03 +0200 Subject: [PATCH 1/4] fix(ec2): Possible nil derefs in ec2 controllers * vpcendpoint * vpcendpointconfiguration * vpcpeeringconnection Signed-off-by: Maximilian Blatt (external expert on behalf of DB Netz) --- pkg/controller/ec2/vpcendpoint/setup.go | 11 +- pkg/controller/ec2/vpcendpoint/setup_test.go | 157 +++++++++++++++ .../vpcendpointserviceconfiguration/setup.go | 3 + .../setup_test.go | 189 +++++++++++++++++ .../ec2/vpcpeeringconnection/setup.go | 3 + .../ec2/vpcpeeringconnection/setup_test.go | 190 ++++++++++++++++++ 6 files changed, 552 insertions(+), 1 deletion(-) create mode 100644 pkg/controller/ec2/vpcendpointserviceconfiguration/setup_test.go create mode 100644 pkg/controller/ec2/vpcpeeringconnection/setup_test.go diff --git a/pkg/controller/ec2/vpcendpoint/setup.go b/pkg/controller/ec2/vpcendpoint/setup.go index 4e2d887898..aa8f062449 100644 --- a/pkg/controller/ec2/vpcendpoint/setup.go +++ b/pkg/controller/ec2/vpcendpoint/setup.go @@ -372,12 +372,21 @@ func (t *tagger) Initialize(ctx context.Context, mgd cpresource.Managed) error { } var vpcEndpointTags svcapitypes.TagSpecification for _, tagSpecification := range cr.Spec.ForProvider.TagSpecifications { + if tagSpecification == nil { + continue + } if aws.StringValue(tagSpecification.ResourceType) == "vpc-endpoint" { vpcEndpointTags = *tagSpecification } } - tagMap := cr.Spec.ForProvider.Tags + var tagMap map[string]string + if cr.Spec.ForProvider.Tags != nil { + tagMap = cr.Spec.ForProvider.Tags + } else { + tagMap = map[string]string{} + } + tagMap["Name"] = cr.Name for k, v := range cpresource.GetExternalTags(mgd) { tagMap[k] = v diff --git a/pkg/controller/ec2/vpcendpoint/setup_test.go b/pkg/controller/ec2/vpcendpoint/setup_test.go index b5596be303..5a78578831 100644 --- a/pkg/controller/ec2/vpcendpoint/setup_test.go +++ b/pkg/controller/ec2/vpcendpoint/setup_test.go @@ -1,3 +1,19 @@ +/* +Copyright 2023 The Crossplane Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + package vpcendpoint import ( @@ -9,10 +25,12 @@ import ( "github.com/aws/aws-sdk-go/service/ec2" "github.com/crossplane/crossplane-runtime/pkg/meta" "github.com/crossplane/crossplane-runtime/pkg/reconciler/managed" + cpresource "github.com/crossplane/crossplane-runtime/pkg/resource" "github.com/crossplane/crossplane-runtime/pkg/test" "github.com/google/go-cmp/cmp" "github.com/pkg/errors" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/pointer" "sigs.k8s.io/controller-runtime/pkg/client" "github.com/crossplane-contrib/provider-aws/apis/ec2/v1alpha1" @@ -44,6 +62,10 @@ type args struct { type vpcEndpointModifier func(*v1alpha1.VPCEndpoint) +func withName(name string) vpcEndpointModifier { + return func(r *v1alpha1.VPCEndpoint) { r.SetName(name) } +} + func withExternalName(name string) vpcEndpointModifier { return func(r *v1alpha1.VPCEndpoint) { meta.SetExternalName(r, name) } } @@ -555,3 +577,138 @@ func TestObserve(t *testing.T) { }) } } + +func TestTagger(t *testing.T) { + type want struct { + cr *v1alpha1.VPCEndpoint + err error + } + + tag := func(k, v string) *v1alpha1.Tag { + return &v1alpha1.Tag{Key: pointer.String(k), Value: pointer.String(v)} + } + + cases := map[string]struct { + args + want + }{ + "ShouldAddTagsIfSpecIsNil": { + args: args{ + kube: &test.MockClient{ + MockUpdate: test.NewMockUpdateFn(nil), + }, + cr: vpcEndpoint( + withName("test"), + withSpec(v1alpha1.VPCEndpointParameters{}), + ), + }, + want: want{ + cr: vpcEndpoint( + withName("test"), + withSpec(v1alpha1.VPCEndpointParameters{ + TagSpecifications: []*v1alpha1.TagSpecification{ + { + ResourceType: aws.String("vpc-endpoint"), + Tags: []*v1alpha1.Tag{ + tag("Name", "test"), + tag(cpresource.ExternalResourceTagKeyKind, ""), + tag(cpresource.ExternalResourceTagKeyName, "test"), + }, + }, + }, + }), + ), + }, + }, + "ShouldOverwriteTags": { + args: args{ + kube: &test.MockClient{ + MockUpdate: test.NewMockUpdateFn(nil), + }, + cr: vpcEndpoint( + withName("test"), + withSpec(v1alpha1.VPCEndpointParameters{ + TagSpecifications: []*v1alpha1.TagSpecification{ + { + ResourceType: aws.String("vpc-endpoint"), + Tags: []*v1alpha1.Tag{ + tag(cpresource.ExternalResourceTagKeyName, "preset"), + }, + }, + }, + }), + ), + }, + want: want{ + cr: vpcEndpoint( + withName("test"), + withSpec(v1alpha1.VPCEndpointParameters{ + TagSpecifications: []*v1alpha1.TagSpecification{ + { + ResourceType: aws.String("vpc-endpoint"), + Tags: []*v1alpha1.Tag{ + tag("Name", "test"), + tag(cpresource.ExternalResourceTagKeyKind, ""), + tag(cpresource.ExternalResourceTagKeyName, "test"), + }, + }, + }, + }), + ), + }, + }, + "ShouldMergeTags": { + args: args{ + kube: &test.MockClient{ + MockUpdate: test.NewMockUpdateFn(nil), + }, + cr: vpcEndpoint( + withName("test"), + withSpec(v1alpha1.VPCEndpointParameters{ + TagSpecifications: []*v1alpha1.TagSpecification{ + { + ResourceType: aws.String("vpc-endpoint"), + Tags: []*v1alpha1.Tag{ + tag("Name", "test"), + tag(cpresource.ExternalResourceTagKeyKind, ""), + tag(cpresource.ExternalResourceTagKeyName, "test"), + }, + }, + }, + }), + ), + }, + want: want{ + cr: vpcEndpoint( + withName("test"), + withSpec(v1alpha1.VPCEndpointParameters{ + TagSpecifications: []*v1alpha1.TagSpecification{ + { + ResourceType: aws.String("vpc-endpoint"), + Tags: []*v1alpha1.Tag{ + tag("Name", "test"), + tag(cpresource.ExternalResourceTagKeyKind, ""), + tag(cpresource.ExternalResourceTagKeyName, "test"), + }, + }, + }, + }), + ), + }, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + ta := tagger{kube: tc.args.kube} + err := ta.Initialize(context.Background(), tc.args.cr) + + if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + t.Errorf("r: -want, +got:\n%s", diff) + } + if diff := cmp.Diff(tc.want.cr, tc.args.cr, test.EquateConditions()); diff != "" { + t.Errorf("r: -want, +got:\n%s", diff) + } + }) + } +} diff --git a/pkg/controller/ec2/vpcendpointserviceconfiguration/setup.go b/pkg/controller/ec2/vpcendpointserviceconfiguration/setup.go index 4f06421fe4..d73575419f 100644 --- a/pkg/controller/ec2/vpcendpointserviceconfiguration/setup.go +++ b/pkg/controller/ec2/vpcendpointserviceconfiguration/setup.go @@ -225,6 +225,9 @@ func (t *tagger) Initialize(ctx context.Context, mgd cpresource.Managed) error { } var vpcEndpointTags svcapitypes.TagSpecification for _, tagSpecification := range cr.Spec.ForProvider.TagSpecifications { + if tagSpecification == nil { + continue + } if aws.StringValue(tagSpecification.ResourceType) == "vpc-endpoint-service" { vpcEndpointTags = *tagSpecification } diff --git a/pkg/controller/ec2/vpcendpointserviceconfiguration/setup_test.go b/pkg/controller/ec2/vpcendpointserviceconfiguration/setup_test.go new file mode 100644 index 0000000000..962fe7c03f --- /dev/null +++ b/pkg/controller/ec2/vpcendpointserviceconfiguration/setup_test.go @@ -0,0 +1,189 @@ +/* +Copyright 2023 The Crossplane Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package vpcendpointserviceconfiguration + +import ( + "context" + "testing" + + cpresource "github.com/crossplane/crossplane-runtime/pkg/resource" + "github.com/crossplane/crossplane-runtime/pkg/test" + "github.com/google/go-cmp/cmp" + "k8s.io/utils/pointer" + "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/crossplane-contrib/provider-aws/apis/ec2/v1alpha1" + + aws "github.com/crossplane-contrib/provider-aws/pkg/clients" +) + +type args struct { + kube client.Client + cr *v1alpha1.VPCEndpointServiceConfiguration +} + +type vPCEndpointServiceConfigurationModifier func(*v1alpha1.VPCEndpointServiceConfiguration) + +func withName(name string) vPCEndpointServiceConfigurationModifier { + return func(r *v1alpha1.VPCEndpointServiceConfiguration) { r.SetName(name) } +} + +func withSpec(p v1alpha1.VPCEndpointServiceConfigurationParameters) vPCEndpointServiceConfigurationModifier { + return func(o *v1alpha1.VPCEndpointServiceConfiguration) { o.Spec.ForProvider = p } +} +func vPCEndpointServiceConfiguration(m ...vPCEndpointServiceConfigurationModifier) *v1alpha1.VPCEndpointServiceConfiguration { + cr := &v1alpha1.VPCEndpointServiceConfiguration{} + for _, f := range m { + f(cr) + } + return cr +} + +func TestTagger(t *testing.T) { + type want struct { + cr *v1alpha1.VPCEndpointServiceConfiguration + err error + } + + tag := func(k, v string) *v1alpha1.Tag { + return &v1alpha1.Tag{Key: pointer.String(k), Value: pointer.String(v)} + } + + cases := map[string]struct { + args + want + }{ + "ShouldAddTagsIfSpecIsNil": { + args: args{ + kube: &test.MockClient{ + MockUpdate: test.NewMockUpdateFn(nil), + }, + cr: vPCEndpointServiceConfiguration( + withName("test"), + withSpec(v1alpha1.VPCEndpointServiceConfigurationParameters{}), + ), + }, + want: want{ + cr: vPCEndpointServiceConfiguration( + withName("test"), + withSpec(v1alpha1.VPCEndpointServiceConfigurationParameters{ + TagSpecifications: []*v1alpha1.TagSpecification{ + { + ResourceType: aws.String("vpc-endpoint-service"), + Tags: []*v1alpha1.Tag{ + tag("Name", "test"), + tag(cpresource.ExternalResourceTagKeyKind, ""), + tag(cpresource.ExternalResourceTagKeyName, "test"), + }, + }, + }, + }), + ), + }, + }, + "ShouldOverwriteTags": { + args: args{ + kube: &test.MockClient{ + MockUpdate: test.NewMockUpdateFn(nil), + }, + cr: vPCEndpointServiceConfiguration( + withName("test"), + withSpec(v1alpha1.VPCEndpointServiceConfigurationParameters{ + TagSpecifications: []*v1alpha1.TagSpecification{ + { + ResourceType: aws.String("vpc-endpoint-service"), + Tags: []*v1alpha1.Tag{ + tag(cpresource.ExternalResourceTagKeyName, "preset"), + }, + }, + }, + }), + ), + }, + want: want{ + cr: vPCEndpointServiceConfiguration( + withName("test"), + withSpec(v1alpha1.VPCEndpointServiceConfigurationParameters{ + TagSpecifications: []*v1alpha1.TagSpecification{ + { + ResourceType: aws.String("vpc-endpoint-service"), + Tags: []*v1alpha1.Tag{ + tag("Name", "test"), + tag(cpresource.ExternalResourceTagKeyKind, ""), + tag(cpresource.ExternalResourceTagKeyName, "test"), + }, + }, + }, + }), + ), + }, + }, + "ShouldMergeTags": { + args: args{ + kube: &test.MockClient{ + MockUpdate: test.NewMockUpdateFn(nil), + }, + cr: vPCEndpointServiceConfiguration( + withName("test"), + withSpec(v1alpha1.VPCEndpointServiceConfigurationParameters{ + TagSpecifications: []*v1alpha1.TagSpecification{ + { + ResourceType: aws.String("vpc-endpoint-service"), + Tags: []*v1alpha1.Tag{ + tag("Name", "test"), + tag(cpresource.ExternalResourceTagKeyKind, ""), + tag(cpresource.ExternalResourceTagKeyName, "test"), + }, + }, + }, + }), + ), + }, + want: want{ + cr: vPCEndpointServiceConfiguration( + withName("test"), + withSpec(v1alpha1.VPCEndpointServiceConfigurationParameters{ + TagSpecifications: []*v1alpha1.TagSpecification{ + { + ResourceType: aws.String("vpc-endpoint-service"), + Tags: []*v1alpha1.Tag{ + tag("Name", "test"), + tag(cpresource.ExternalResourceTagKeyKind, ""), + tag(cpresource.ExternalResourceTagKeyName, "test"), + }, + }, + }, + }), + ), + }, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + ta := tagger{kube: tc.args.kube} + err := ta.Initialize(context.Background(), tc.args.cr) + + if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + t.Errorf("r: -want, +got:\n%s", diff) + } + if diff := cmp.Diff(tc.want.cr, tc.args.cr, test.EquateConditions()); diff != "" { + t.Errorf("r: -want, +got:\n%s", diff) + } + }) + } +} diff --git a/pkg/controller/ec2/vpcpeeringconnection/setup.go b/pkg/controller/ec2/vpcpeeringconnection/setup.go index ad9ad6fc30..0cbc641ba9 100644 --- a/pkg/controller/ec2/vpcpeeringconnection/setup.go +++ b/pkg/controller/ec2/vpcpeeringconnection/setup.go @@ -247,6 +247,9 @@ func (t *tagger) Initialize(ctx context.Context, mgd resource.Managed) error { } var vpcPeeringConnectionTags svcapitypes.TagSpecification for _, tagSpecification := range cr.Spec.ForProvider.TagSpecifications { + if tagSpecification == nil { + continue + } if aws.StringValue(tagSpecification.ResourceType) == "vpc-peering-connection" { vpcPeeringConnectionTags = *tagSpecification } diff --git a/pkg/controller/ec2/vpcpeeringconnection/setup_test.go b/pkg/controller/ec2/vpcpeeringconnection/setup_test.go new file mode 100644 index 0000000000..97e19c8104 --- /dev/null +++ b/pkg/controller/ec2/vpcpeeringconnection/setup_test.go @@ -0,0 +1,190 @@ +/* +Copyright 2023 The Crossplane Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package vpcpeeringconnection + +import ( + "context" + "testing" + + cpresource "github.com/crossplane/crossplane-runtime/pkg/resource" + "github.com/crossplane/crossplane-runtime/pkg/test" + "github.com/google/go-cmp/cmp" + "k8s.io/utils/pointer" + "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/crossplane-contrib/provider-aws/apis/ec2/v1alpha1" + + aws "github.com/crossplane-contrib/provider-aws/pkg/clients" +) + +type args struct { + kube client.Client + cr *v1alpha1.VPCPeeringConnection +} + +type vPCPeeringConnectionModifier func(*v1alpha1.VPCPeeringConnection) + +func withName(name string) vPCPeeringConnectionModifier { + return func(r *v1alpha1.VPCPeeringConnection) { r.SetName(name) } +} + +func withSpec(p v1alpha1.VPCPeeringConnectionParameters) vPCPeeringConnectionModifier { + return func(o *v1alpha1.VPCPeeringConnection) { o.Spec.ForProvider = p } +} + +func vPCPeeringConnection(m ...vPCPeeringConnectionModifier) *v1alpha1.VPCPeeringConnection { + cr := &v1alpha1.VPCPeeringConnection{} + for _, f := range m { + f(cr) + } + return cr +} + +func TestTagger(t *testing.T) { + type want struct { + cr *v1alpha1.VPCPeeringConnection + err error + } + + tag := func(k, v string) *v1alpha1.Tag { + return &v1alpha1.Tag{Key: pointer.String(k), Value: pointer.String(v)} + } + + cases := map[string]struct { + args + want + }{ + "ShouldAddTagsIfSpecIsNil": { + args: args{ + kube: &test.MockClient{ + MockUpdate: test.NewMockUpdateFn(nil), + }, + cr: vPCPeeringConnection( + withName("test"), + withSpec(v1alpha1.VPCPeeringConnectionParameters{}), + ), + }, + want: want{ + cr: vPCPeeringConnection( + withName("test"), + withSpec(v1alpha1.VPCPeeringConnectionParameters{ + TagSpecifications: []*v1alpha1.TagSpecification{ + { + ResourceType: aws.String("vpc-peering-connection"), + Tags: []*v1alpha1.Tag{ + tag("Name", "test"), + tag(cpresource.ExternalResourceTagKeyKind, ""), + tag(cpresource.ExternalResourceTagKeyName, "test"), + }, + }, + }, + }), + ), + }, + }, + "ShouldOverwriteTags": { + args: args{ + kube: &test.MockClient{ + MockUpdate: test.NewMockUpdateFn(nil), + }, + cr: vPCPeeringConnection( + withName("test"), + withSpec(v1alpha1.VPCPeeringConnectionParameters{ + TagSpecifications: []*v1alpha1.TagSpecification{ + { + ResourceType: aws.String("vpc-peering-connection"), + Tags: []*v1alpha1.Tag{ + tag(cpresource.ExternalResourceTagKeyName, "preset"), + }, + }, + }, + }), + ), + }, + want: want{ + cr: vPCPeeringConnection( + withName("test"), + withSpec(v1alpha1.VPCPeeringConnectionParameters{ + TagSpecifications: []*v1alpha1.TagSpecification{ + { + ResourceType: aws.String("vpc-peering-connection"), + Tags: []*v1alpha1.Tag{ + tag("Name", "test"), + tag(cpresource.ExternalResourceTagKeyKind, ""), + tag(cpresource.ExternalResourceTagKeyName, "test"), + }, + }, + }, + }), + ), + }, + }, + "ShouldMergeTags": { + args: args{ + kube: &test.MockClient{ + MockUpdate: test.NewMockUpdateFn(nil), + }, + cr: vPCPeeringConnection( + withName("test"), + withSpec(v1alpha1.VPCPeeringConnectionParameters{ + TagSpecifications: []*v1alpha1.TagSpecification{ + { + ResourceType: aws.String("vpc-peering-connection"), + Tags: []*v1alpha1.Tag{ + tag("Name", "test"), + tag(cpresource.ExternalResourceTagKeyKind, ""), + tag(cpresource.ExternalResourceTagKeyName, "test"), + }, + }, + }, + }), + ), + }, + want: want{ + cr: vPCPeeringConnection( + withName("test"), + withSpec(v1alpha1.VPCPeeringConnectionParameters{ + TagSpecifications: []*v1alpha1.TagSpecification{ + { + ResourceType: aws.String("vpc-peering-connection"), + Tags: []*v1alpha1.Tag{ + tag("Name", "test"), + tag(cpresource.ExternalResourceTagKeyKind, ""), + tag(cpresource.ExternalResourceTagKeyName, "test"), + }, + }, + }, + }), + ), + }, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + ta := tagger{kube: tc.args.kube} + err := ta.Initialize(context.Background(), tc.args.cr) + + if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + t.Errorf("r: -want, +got:\n%s", diff) + } + if diff := cmp.Diff(tc.want.cr, tc.args.cr, test.EquateConditions()); diff != "" { + t.Errorf("r: -want, +got:\n%s", diff) + } + }) + } +} From 9a25e36932f491b975275d4bbc5b603606d057e8 Mon Sep 17 00:00:00 2001 From: "Maximilian Blatt (external expert on behalf of DB Netz)" Date: Tue, 5 Sep 2023 09:18:32 +0200 Subject: [PATCH 2/4] fix(efs): Ignore AWS system tags Signed-off-by: Maximilian Blatt (external expert on behalf of DB Netz) --- pkg/controller/efs/utils/tags.go | 13 ++++ pkg/controller/efs/utils/tags_test.go | 87 +++++++++++++++++++++++++++ 2 files changed, 100 insertions(+) create mode 100644 pkg/controller/efs/utils/tags_test.go diff --git a/pkg/controller/efs/utils/tags.go b/pkg/controller/efs/utils/tags.go index 14ce60c677..cee40ebfaa 100644 --- a/pkg/controller/efs/utils/tags.go +++ b/pkg/controller/efs/utils/tags.go @@ -18,6 +18,7 @@ package utils import ( "sort" + "strings" svcsdk "github.com/aws/aws-sdk-go/service/efs" "github.com/aws/aws-sdk-go/service/efs/efsiface" @@ -98,6 +99,12 @@ func DiffTags(spec []*svcapitypes.Tag, current []*svcsdk.Tag) (addTags []*svcsdk specMap := make(map[string]string, len(spec)) for _, t := range spec { key := awsclient.StringValue(t.Key) + + // Ignore "aws:" internal tags since they cannot be added or removed. + if strings.HasPrefix(key, "aws:") { + continue + } + val := awsclient.StringValue(t.Value) specMap[key] = awsclient.StringValue(t.Value) @@ -119,6 +126,12 @@ func DiffTags(spec []*svcapitypes.Tag, current []*svcsdk.Tag) (addTags []*svcsdk for _, t := range current { key := awsclient.StringValue(t.Key) + + // Ignore "aws:" internal tags since they cannot be added or removed. + if strings.HasPrefix(key, "aws:") { + continue + } + if _, exists := specMap[key]; !exists { removeTags = append(removeTags, awsclient.String(key)) } diff --git a/pkg/controller/efs/utils/tags_test.go b/pkg/controller/efs/utils/tags_test.go new file mode 100644 index 0000000000..79683dce81 --- /dev/null +++ b/pkg/controller/efs/utils/tags_test.go @@ -0,0 +1,87 @@ +/* +Copyright 2023 The Crossplane Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package utils + +import ( + "testing" + + svcsdk "github.com/aws/aws-sdk-go/service/efs" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + + svcapitypes "github.com/crossplane-contrib/provider-aws/apis/efs/v1alpha1" + awsclient "github.com/crossplane-contrib/provider-aws/pkg/clients" +) + +func TestDiffTags(t *testing.T) { + type args struct { + spec []*svcapitypes.Tag + current []*svcsdk.Tag + } + type want struct { + add []*svcsdk.Tag + remove []*string + } + + sdkTag := func(k, v string) *svcsdk.Tag { + return &svcsdk.Tag{Key: &k, Value: &v} + } + + apiTag := func(k, v string) *svcapitypes.Tag { + return &svcapitypes.Tag{Key: &k, Value: &v} + } + + cases := map[string]struct { + args args + want want + }{ + "DoNotAddOrRemoveAWSystemTags": { + args: args{ + spec: []*svcapitypes.Tag{ + apiTag("foo", "bar"), + apiTag("testAdd", "val"), + apiTag("aws:some-system-tag", "enabled"), + }, + current: []*svcsdk.Tag{ + sdkTag("foo", "bar"), + sdkTag("testRemove", "val2"), + sdkTag("aws:elasticfilesystem:default-backup", "enabled"), + }, + }, + want: want{ + add: []*svcsdk.Tag{ + sdkTag("testAdd", "val"), + }, + remove: []*string{ + awsclient.String("testRemove"), + }, + }, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + add, remove := DiffTags(tc.args.spec, tc.args.current) + if diff := cmp.Diff(tc.want.add, add); diff != "" { + t.Errorf("add: -want, +got:\n%s", diff) + } + if diff := cmp.Diff(tc.want.remove, remove, cmpopts.SortSlices(func(a, b string) bool { return a > b })); diff != "" { + t.Errorf("remove: -want, +got:\n%s", diff) + } + }) + } +} From d62377479c39084eddc95f75c919c3362d515b3d Mon Sep 17 00:00:00 2001 From: "Maximilian Blatt (external expert on behalf of DB Netz)" Date: Tue, 5 Sep 2023 17:25:49 +0200 Subject: [PATCH 3/4] fix: Package names for mwaa and mq Signed-off-by: Maximilian Blatt (external expert on behalf of DB Netz) --- pkg/controller/mq/setup.go | 2 +- pkg/controller/mwaa/setup.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/controller/mq/setup.go b/pkg/controller/mq/setup.go index 9ddca13e37..199f19bb44 100644 --- a/pkg/controller/mq/setup.go +++ b/pkg/controller/mq/setup.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package mwaa +package mq import ( ctrl "sigs.k8s.io/controller-runtime" diff --git a/pkg/controller/mwaa/setup.go b/pkg/controller/mwaa/setup.go index 4bb6264736..77f6af5cb7 100644 --- a/pkg/controller/mwaa/setup.go +++ b/pkg/controller/mwaa/setup.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package mq +package mwaa import ( ctrl "sigs.k8s.io/controller-runtime" From 27686eeb4549a6d56eb963c86cba2287cbd85058 Mon Sep 17 00:00:00 2001 From: Kai Parry Date: Tue, 5 Sep 2023 15:08:42 +0200 Subject: [PATCH 4/4] feat: expose metrics for all AWS API calls Signed-off-by: Kai Parry --- cmd/provider/main.go | 2 ++ go.mod | 4 ++-- go.sum | 4 ++-- pkg/clients/config.go | 43 +++++++++++++++++++++++++------------- pkg/utils/metrics/setup.go | 24 +++++++++++++++++++++ 5 files changed, 58 insertions(+), 19 deletions(-) create mode 100644 pkg/utils/metrics/setup.go diff --git a/cmd/provider/main.go b/cmd/provider/main.go index e1be815a3f..f63ee2b809 100644 --- a/cmd/provider/main.go +++ b/cmd/provider/main.go @@ -41,6 +41,7 @@ import ( "github.com/crossplane-contrib/provider-aws/apis/v1alpha1" "github.com/crossplane-contrib/provider-aws/pkg/controller" "github.com/crossplane-contrib/provider-aws/pkg/features" + "github.com/crossplane-contrib/provider-aws/pkg/utils/metrics" ) func main() { @@ -123,6 +124,7 @@ func main() { log.Info("Alpha feature enabled", "flag", features.EnableAlphaManagementPolicies) } + kingpin.FatalIfError(metrics.SetupMetrics(), "Cannot setup AWS metrics hook") kingpin.FatalIfError(controller.Setup(mgr, o), "Cannot setup AWS controllers") kingpin.FatalIfError(mgr.Start(ctrl.SetupSignalHandler()), "Cannot start controller manager") diff --git a/go.mod b/go.mod index 8e446c8b62..eb865de76b 100644 --- a/go.mod +++ b/go.mod @@ -39,6 +39,7 @@ require ( github.com/mitchellh/copystructure v1.0.0 github.com/onsi/gomega v1.27.7 github.com/pkg/errors v0.9.1 + github.com/prometheus/client_golang v1.15.1 go.uber.org/zap v1.24.0 golang.org/x/net v0.12.0 gopkg.in/alecthomas/kingpin.v2 v2.2.6 @@ -121,10 +122,9 @@ require ( github.com/modern-go/reflect2 v1.0.2 // indirect github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect github.com/operator-framework/api v0.6.0 // indirect - github.com/prometheus/client_golang v1.15.1 // indirect github.com/prometheus/client_model v0.4.0 // indirect github.com/prometheus/common v0.44.0 // indirect - github.com/prometheus/procfs v0.10.0 // indirect + github.com/prometheus/procfs v0.10.1 // indirect github.com/ryanuber/go-glob v1.0.0 // indirect github.com/samber/lo v1.37.0 // indirect github.com/sergi/go-diff v1.0.0 // indirect diff --git a/go.sum b/go.sum index 9d9fd3ab76..b1590aa4e7 100644 --- a/go.sum +++ b/go.sum @@ -683,8 +683,8 @@ github.com/prometheus/procfs v0.0.0-20190507164030-5867b95ac084/go.mod h1:TjEm7z github.com/prometheus/procfs v0.0.2/go.mod h1:TjEm7ze935MbeOT/UhFTIMYKhuLP4wbCsTZCD3I8kEA= github.com/prometheus/procfs v0.1.3/go.mod h1:lV6e/gmhEcM9IjHGsFOCxxuZ+z1YqCvr4OA4YeYWdaU= github.com/prometheus/procfs v0.2.0/go.mod h1:lV6e/gmhEcM9IjHGsFOCxxuZ+z1YqCvr4OA4YeYWdaU= -github.com/prometheus/procfs v0.10.0 h1:UkG7GPYkO4UZyLnyXjaWYcgOSONqwdBqFUT95ugmt6I= -github.com/prometheus/procfs v0.10.0/go.mod h1:nwNm2aOCAYw8uTR/9bWRREkZFxAUcWzPHWJq+XBB/FM= +github.com/prometheus/procfs v0.10.1 h1:kYK1Va/YMlutzCGazswoHKo//tZVlFpKYh+PymziUAg= +github.com/prometheus/procfs v0.10.1/go.mod h1:nwNm2aOCAYw8uTR/9bWRREkZFxAUcWzPHWJq+XBB/FM= github.com/prometheus/tsdb v0.7.1/go.mod h1:qhTCs0VvXwvX/y3TZrWD7rabWM+ijKTux40TwIPHuXU= github.com/rogpeppe/fastuuid v0.0.0-20150106093220-6724a57986af/go.mod h1:XWv6SoW27p1b0cqNHllgS5HIMJraePCO15w5zCzIWYg= github.com/rogpeppe/fastuuid v1.2.0/go.mod h1:jVj6XXZzXRy/MSR5jhDC/2q6DgLz+nrA6LYCDYWNEvQ= diff --git a/pkg/clients/config.go b/pkg/clients/config.go index 82bf012040..542aae5258 100644 --- a/pkg/clients/config.go +++ b/pkg/clients/config.go @@ -45,6 +45,7 @@ import ( "github.com/crossplane/crossplane-runtime/pkg/resource" "github.com/crossplane-contrib/provider-aws/apis/v1beta1" + "github.com/crossplane-contrib/provider-aws/pkg/utils/metrics" "github.com/crossplane-contrib/provider-aws/pkg/version" ) @@ -74,9 +75,18 @@ const ( FieldRequired FieldOption = iota ) -// userAgentV2 constructs the Crossplane user agent for AWS v2 clients -var userAgentV2 = config.WithAPIOptions([]func(*middleware.Stack) error{ +// middlewareV2 constructs the AWS SDK v2 middleware +var middlewareV2 = config.WithAPIOptions([]func(*middleware.Stack) error{ awsmiddleware.AddUserAgentKeyValue("crossplane-provider-aws", version.Version), + func(s *middleware.Stack) error { + return s.Finalize.Add(recordRequestMetrics, middleware.After) + }, +}) + +// recordRequestMetrics records Prometheus metrics for requests to the AWS APIs +var recordRequestMetrics = middleware.FinalizeMiddlewareFunc("recordRequestMetrics", func(ctx context.Context, in middleware.FinalizeInput, next middleware.FinalizeHandler) (middleware.FinalizeOutput, middleware.Metadata, error) { + metrics.IncAWSAPICall(awsmiddleware.GetServiceID(ctx), awsmiddleware.GetOperationName(ctx), "2") + return next.HandleFinalize(ctx, in) }) // userAgentV1 constructs the Crossplane user agent for AWS v1 clients @@ -256,7 +266,7 @@ func UseProviderSecret(ctx context.Context, data []byte, profile, region string) config, err := config.LoadDefaultConfig( ctx, - userAgentV2, + middlewareV2, config.WithRegion(region), config.WithCredentialsProvider(credentials.StaticCredentialsProvider{ Value: creds, @@ -275,7 +285,7 @@ func UseProviderSecretAssumeRole(ctx context.Context, data []byte, profile, regi config, err := config.LoadDefaultConfig( ctx, - userAgentV2, + middlewareV2, config.WithRegion(region), config.WithCredentialsProvider(credentials.StaticCredentialsProvider{ Value: creds, @@ -319,7 +329,7 @@ func UsePodServiceAccountAssumeRole(ctx context.Context, _ []byte, _, region str stsAssumeRoleOptions := SetAssumeRoleOptions(pc) cnf, err := config.LoadDefaultConfig( ctx, - userAgentV2, + middlewareV2, config.WithRegion(cfg.Region), config.WithCredentialsProvider(aws.NewCredentialsCache( stscreds.NewAssumeRoleProvider( @@ -339,7 +349,7 @@ func UsePodServiceAccountAssumeRole(ctx context.Context, _ []byte, _, region str // configured via a ServiceAccount assume Cross account IAM roles // https://aws.amazon.com/blogs/containers/cross-account-iam-roles-for-kubernetes-service-accounts/ func UsePodServiceAccountAssumeRoleWithWebIdentity(ctx context.Context, _ []byte, _, region string, pc *v1beta1.ProviderConfig) (*aws.Config, error) { - cfg, err := config.LoadDefaultConfig(ctx, userAgentV2) + cfg, err := config.LoadDefaultConfig(ctx, middlewareV2) if err != nil { return nil, errors.Wrap(err, "failed to load default AWS config") } @@ -354,7 +364,7 @@ func UsePodServiceAccountAssumeRoleWithWebIdentity(ctx context.Context, _ []byte cnf, err := config.LoadDefaultConfig( ctx, - userAgentV2, + middlewareV2, config.WithRegion(region), config.WithCredentialsProvider(aws.NewCredentialsCache( stscreds.NewWebIdentityRoleProvider( @@ -386,13 +396,13 @@ func UsePodServiceAccount(ctx context.Context, _ []byte, _, region string) (*aws if region == GlobalRegion { cfg, err := config.LoadDefaultConfig( ctx, - userAgentV2, + middlewareV2, ) return &cfg, errors.Wrap(err, "failed to load default AWS config") } cfg, err := config.LoadDefaultConfig( ctx, - userAgentV2, + middlewareV2, config.WithRegion(region), ) if err != nil { @@ -468,6 +478,9 @@ func GetSessionV1(cfg *awsv1.Config) (*session.Session, error) { return nil, err } session.Handlers.Build.PushBackNamed(userAgentV1) + session.Handlers.Send.PushFront(func(r *requestv1.Request) { + metrics.IncAWSAPICall(r.ClientInfo.ServiceName, r.Operation.Name, "1") + }) return session, nil } @@ -481,7 +494,7 @@ func UseProviderSecretV1AssumeRole(ctx context.Context, data []byte, pc *v1beta1 config, err := config.LoadDefaultConfig( ctx, - userAgentV2, + middlewareV2, config.WithRegion(region), config.WithCredentialsProvider(credentials.StaticCredentialsProvider{ Value: creds, @@ -554,7 +567,7 @@ func UseProviderSecretV1(_ context.Context, data []byte, pc *v1beta1.ProviderCon // assume Cross account IAM role // https://aws.amazon.com/blogs/containers/cross-account-iam-roles-for-kubernetes-service-accounts/ func UsePodServiceAccountV1AssumeRole(ctx context.Context, _ []byte, pc *v1beta1.ProviderConfig, _, region string) (*awsv1.Config, error) { - cfg, err := config.LoadDefaultConfig(ctx, userAgentV2) + cfg, err := config.LoadDefaultConfig(ctx, middlewareV2) if err != nil { return nil, errors.Wrap(err, "failed to load default AWS config") } @@ -570,7 +583,7 @@ func UsePodServiceAccountV1AssumeRole(ctx context.Context, _ []byte, pc *v1beta1 } cnf, err := config.LoadDefaultConfig( ctx, - userAgentV2, + middlewareV2, config.WithRegion(region), config.WithCredentialsProvider(aws.NewCredentialsCache( stscreds.NewAssumeRoleProvider( @@ -598,7 +611,7 @@ func UsePodServiceAccountV1AssumeRole(ctx context.Context, _ []byte, pc *v1beta1 // assume Cross account IAM role // https://aws.amazon.com/blogs/containers/cross-account-iam-roles-for-kubernetes-service-accounts/ func UsePodServiceAccountV1AssumeRoleWithWebIdentity(ctx context.Context, _ []byte, pc *v1beta1.ProviderConfig, _, region string) (*awsv1.Config, error) { - cfg, err := config.LoadDefaultConfig(ctx, userAgentV2) + cfg, err := config.LoadDefaultConfig(ctx, middlewareV2) if err != nil { return nil, errors.Wrap(err, "failed to load default AWS config") } @@ -613,7 +626,7 @@ func UsePodServiceAccountV1AssumeRoleWithWebIdentity(ctx context.Context, _ []by cnf, err := config.LoadDefaultConfig( ctx, - userAgentV2, + middlewareV2, config.WithRegion(region), config.WithCredentialsProvider(aws.NewCredentialsCache( stscreds.NewWebIdentityRoleProvider( @@ -643,7 +656,7 @@ func UsePodServiceAccountV1AssumeRoleWithWebIdentity(ctx context.Context, _ []by func UsePodServiceAccountV1(ctx context.Context, _ []byte, pc *v1beta1.ProviderConfig, _, region string) (*awsv1.Config, error) { cfg, err := config.LoadDefaultConfig( ctx, - userAgentV2, + middlewareV2, ) if err != nil { return nil, errors.Wrap(err, "failed to load default AWS config") diff --git a/pkg/utils/metrics/setup.go b/pkg/utils/metrics/setup.go new file mode 100644 index 0000000000..66ec5ba566 --- /dev/null +++ b/pkg/utils/metrics/setup.go @@ -0,0 +1,24 @@ +package metrics + +import ( + "github.com/prometheus/client_golang/prometheus" + + k8smetrics "sigs.k8s.io/controller-runtime/pkg/metrics" +) + +var ( + metricAWSAPICalls = prometheus.NewCounterVec(prometheus.CounterOpts{ + Name: "aws_api_calls_total", + Help: "Number of API calls to the AWS API", + }, []string{"service", "operation", "api_version"}) +) + +// SetupMetrics will register the known Prometheus metrics with controller-runtime's metrics registry +func SetupMetrics() error { + return k8smetrics.Registry.Register(metricAWSAPICalls) +} + +// IncAWSAPICall will increment the aws_api_calls_total metric for the specified service, operation, and apiVersion tuple +func IncAWSAPICall(service, operation, apiVersion string) { + metricAWSAPICalls.WithLabelValues(service, operation, apiVersion).Inc() +}