From 96161c4321405c2a2a19af57800e7b0f0b6e0adc Mon Sep 17 00:00:00 2001 From: Baiju Muthukadan Date: Tue, 23 Aug 2022 21:56:44 +0530 Subject: [PATCH 1/3] Add optional key to the FieldExport target FieldExport generates the key name with an output structure like `.`. This default naming structure creates non-conflicting names as the keys. However, many applications expect short names as keys. This is important, especially when the Secret/ConfigMap is mounted as files; the file names are nothing but the key names. This PR adds an optional attribute (.spec.to.key) to specify the key name in the FieldExport resource. If this attribute is specified, use this key name instead of the default. Proposal: https://github.com/aws-controllers-k8s/community/issues/1410 Signed-off-by: Baiju Muthukadan --- apis/core/v1alpha1/field_export.go | 1 + apis/core/v1alpha1/zz_generated.deepcopy.go | 10 +++ .../bases/services.k8s.aws_fieldexports.yaml | 2 + pkg/runtime/field_export_reconciler.go | 6 ++ pkg/runtime/field_export_reconciler_test.go | 78 +++++++++++++++++++ 5 files changed, 97 insertions(+) diff --git a/apis/core/v1alpha1/field_export.go b/apis/core/v1alpha1/field_export.go index 796a82d..3b8a050 100644 --- a/apis/core/v1alpha1/field_export.go +++ b/apis/core/v1alpha1/field_export.go @@ -24,6 +24,7 @@ type FieldExportTarget struct { // Namespace is marked as optional, so we cannot compose `NamespacedName` Namespace *string `json:"namespace,omitempty"` Kind FieldExportOutputType `json:"kind"` + Key *string `json:"key,omitempty"` } // FieldExportSpec defines the desired state of the FieldExport. diff --git a/apis/core/v1alpha1/zz_generated.deepcopy.go b/apis/core/v1alpha1/zz_generated.deepcopy.go index cfd424e..0bb8ade 100644 --- a/apis/core/v1alpha1/zz_generated.deepcopy.go +++ b/apis/core/v1alpha1/zz_generated.deepcopy.go @@ -352,6 +352,11 @@ func (in *FieldExportTarget) DeepCopyInto(out *FieldExportTarget) { *out = new(string) **out = **in } + if in.Key != nil { + in, out := &in.Key, &out.Key + *out = new(string) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new FieldExportTarget. @@ -455,6 +460,11 @@ func (in *ResourceMetadata) DeepCopyInto(out *ResourceMetadata) { *out = new(AWSAccountID) **out = **in } + if in.Region != nil { + in, out := &in.Region, &out.Region + *out = new(AWSRegion) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ResourceMetadata. diff --git a/config/crd/bases/services.k8s.aws_fieldexports.yaml b/config/crd/bases/services.k8s.aws_fieldexports.yaml index 361e01f..b04e05f 100644 --- a/config/crd/bases/services.k8s.aws_fieldexports.yaml +++ b/config/crd/bases/services.k8s.aws_fieldexports.yaml @@ -66,6 +66,8 @@ spec: description: FieldExportTarget provides the values necessary to identify the output path for a field export. properties: + key: + type: string kind: description: FieldExportOutputType represents all types that can be produced by a field export operation diff --git a/pkg/runtime/field_export_reconciler.go b/pkg/runtime/field_export_reconciler.go index 136885c..68c77b6 100644 --- a/pkg/runtime/field_export_reconciler.go +++ b/pkg/runtime/field_export_reconciler.go @@ -296,6 +296,9 @@ func (r *fieldExportReconciler) writeToConfigMap( ) error { // Construct the data key key := fmt.Sprintf("%s.%s", desired.Namespace, desired.Name) + if desired.Spec.To.Key != nil { + key = *desired.Spec.To.Key + } // Get the initial configmap nsn := types.NamespacedName{ @@ -340,6 +343,9 @@ func (r *fieldExportReconciler) writeToSecret( ) error { // Construct the data key key := fmt.Sprintf("%s.%s", desired.Namespace, desired.Name) + if desired.Spec.To.Key != nil { + key = *desired.Spec.To.Key + } // Get the initial secret nsn := types.NamespacedName{ diff --git a/pkg/runtime/field_export_reconciler_test.go b/pkg/runtime/field_export_reconciler_test.go index 61aacba..d7ce419 100644 --- a/pkg/runtime/field_export_reconciler_test.go +++ b/pkg/runtime/field_export_reconciler_test.go @@ -163,6 +163,36 @@ func fieldExportWithPath(namespace, name string, kind ackv1alpha1.FieldExportOut } } +func fieldExportWithKey(namespace, name string, kind ackv1alpha1.FieldExportOutputType, key string) *ackv1alpha1.FieldExport { + path := ".spec.name" + return &ackv1alpha1.FieldExport{ + TypeMeta: v1.TypeMeta{}, + ObjectMeta: v1.ObjectMeta{ + Namespace: namespace, + Name: name, + Finalizers: []string{"finalizers.services.k8s.aws/FieldExport"}, + }, + Spec: ackv1alpha1.FieldExportSpec{ + From: &ackv1alpha1.ResourceFieldSelector{ + Path: &path, + Resource: ackv1alpha1.NamespacedResource{ + GroupKind: v1.GroupKind{ + Group: BookGVK.Group, + Kind: BookGVK.Kind, + }, + Name: strPtr(SourceResourceName), + }, + }, + To: &ackv1alpha1.FieldExportTarget{ + Name: strPtr("fake-export-output"), + Kind: kind, + Key: &key, + }, + }, + Status: ackv1alpha1.FieldExportStatus{}, + } +} + func mockFieldExportList() []ackv1alpha1.FieldExport { // Matching cases defaultConfigMap := fieldExportConfigMap(FieldExportNamespace, "export-1") @@ -452,6 +482,36 @@ func TestSync_HappyCaseResourceNoExports(t *testing.T) { require.Len(exports, 0) } +func TestSync_SetKeyNameExplicitly(t *testing.T) { + // Setup + require := require.New(t) + // Mock resource creation + r, kc, apiReader := mockFieldExportReconciler() + descriptor, res, _ := mockDescriptorAndAWSResource() + manager := mockManager() + fieldExport := fieldExportWithKey(FieldExportNamespace, FieldExportName, ackv1alpha1.FieldExportOutputTypeSecret, "new-key") + sourceResource, _, _ := mockSourceResource() + ctx := context.TODO() + statusWriter := &ctrlrtclientmock.StatusWriter{} + + //Mock behavior setup + setupMockClientForFieldExport(kc, statusWriter, ctx, fieldExport) + setupMockApiReaderForFieldExport(apiReader, ctx, res) + setupMockManager(manager, ctx, res) + setupMockDescriptor(descriptor, res) + setupMockUnstructuredConverter() + + // Call + latest, err := r.Sync(ctx, sourceResource, *fieldExport) + + //Assertions + require.Nil(err) + require.NotNil(latest.Status) + require.Len(latest.Status.Conditions, 0) + assertPatchedConfigMap(false, t, ctx, kc) + assertPatchedSecretWithKey(true, t, ctx, kc, "new-key") +} + // Assertions func assertPatchedConfigMap(expected bool, t *testing.T, ctx context.Context, kc *ctrlrtclientmock.Client) { @@ -492,6 +552,24 @@ func assertPatchedSecret(expected bool, t *testing.T, ctx context.Context, kc *c } } +func assertPatchedSecretWithKey(expected bool, t *testing.T, ctx context.Context, kc *ctrlrtclientmock.Client, key string) { + dataMatcher := mock.MatchedBy(func(cm *corev1.Secret) bool { + if cm.Data == nil { + return false + } + val, ok := cm.Data[key] + if !ok { + return false + } + return bytes.Equal(val, []byte("test-book-name")) + }) + if expected { + kc.AssertCalled(t, "Patch", ctx, dataMatcher, mock.Anything) + } else { + kc.AssertNotCalled(t, "Patch", ctx, dataMatcher, mock.Anything) + } +} + // assertRecoverableCondition asserts that 'ConditionTypeRecoverable' condition // is present in the resource's status and that it's value is equal to // 'conditionStatus' parameter From a974e46b904f7299f756fcd0a90d5a66522859fb Mon Sep 17 00:00:00 2001 From: Baiju Muthukadan Date: Wed, 24 Aug 2022 10:02:12 +0530 Subject: [PATCH 2/3] Fall back to default with empty string as key Signed-off-by: Baiju Muthukadan --- apis/core/v1alpha1/field_export.go | 3 +- .../bases/services.k8s.aws_fieldexports.yaml | 2 ++ pkg/runtime/field_export_reconciler.go | 4 +-- pkg/runtime/field_export_reconciler_test.go | 30 +++++++++++++++++++ 4 files changed, 36 insertions(+), 3 deletions(-) diff --git a/apis/core/v1alpha1/field_export.go b/apis/core/v1alpha1/field_export.go index 3b8a050..df1e53e 100644 --- a/apis/core/v1alpha1/field_export.go +++ b/apis/core/v1alpha1/field_export.go @@ -24,7 +24,8 @@ type FieldExportTarget struct { // Namespace is marked as optional, so we cannot compose `NamespacedName` Namespace *string `json:"namespace,omitempty"` Kind FieldExportOutputType `json:"kind"` - Key *string `json:"key,omitempty"` + // Key overrides the default value (`.`) for the FieldExport target + Key *string `json:"key,omitempty"` } // FieldExportSpec defines the desired state of the FieldExport. diff --git a/config/crd/bases/services.k8s.aws_fieldexports.yaml b/config/crd/bases/services.k8s.aws_fieldexports.yaml index b04e05f..a435de8 100644 --- a/config/crd/bases/services.k8s.aws_fieldexports.yaml +++ b/config/crd/bases/services.k8s.aws_fieldexports.yaml @@ -67,6 +67,8 @@ spec: the output path for a field export. properties: key: + description: Key overrides the default value (`.`) + for the FieldExport target type: string kind: description: FieldExportOutputType represents all types that can diff --git a/pkg/runtime/field_export_reconciler.go b/pkg/runtime/field_export_reconciler.go index 68c77b6..2c971c6 100644 --- a/pkg/runtime/field_export_reconciler.go +++ b/pkg/runtime/field_export_reconciler.go @@ -296,7 +296,7 @@ func (r *fieldExportReconciler) writeToConfigMap( ) error { // Construct the data key key := fmt.Sprintf("%s.%s", desired.Namespace, desired.Name) - if desired.Spec.To.Key != nil { + if desired.Spec.To.Key != nil && strings.TrimSpace(*desired.Spec.To.Key) != "" { key = *desired.Spec.To.Key } @@ -343,7 +343,7 @@ func (r *fieldExportReconciler) writeToSecret( ) error { // Construct the data key key := fmt.Sprintf("%s.%s", desired.Namespace, desired.Name) - if desired.Spec.To.Key != nil { + if desired.Spec.To.Key != nil && strings.TrimSpace(*desired.Spec.To.Key) != "" { key = *desired.Spec.To.Key } diff --git a/pkg/runtime/field_export_reconciler_test.go b/pkg/runtime/field_export_reconciler_test.go index d7ce419..d11d67f 100644 --- a/pkg/runtime/field_export_reconciler_test.go +++ b/pkg/runtime/field_export_reconciler_test.go @@ -512,6 +512,36 @@ func TestSync_SetKeyNameExplicitly(t *testing.T) { assertPatchedSecretWithKey(true, t, ctx, kc, "new-key") } +func TestSync_SetKeyNameExplicitlyWithEmptyString(t *testing.T) { + // Setup + require := require.New(t) + // Mock resource creation + r, kc, apiReader := mockFieldExportReconciler() + descriptor, res, _ := mockDescriptorAndAWSResource() + manager := mockManager() + fieldExport := fieldExportWithKey(FieldExportNamespace, FieldExportName, ackv1alpha1.FieldExportOutputTypeSecret, "") + sourceResource, _, _ := mockSourceResource() + ctx := context.TODO() + statusWriter := &ctrlrtclientmock.StatusWriter{} + + //Mock behavior setup + setupMockClientForFieldExport(kc, statusWriter, ctx, fieldExport) + setupMockApiReaderForFieldExport(apiReader, ctx, res) + setupMockManager(manager, ctx, res) + setupMockDescriptor(descriptor, res) + setupMockUnstructuredConverter() + + // Call + latest, err := r.Sync(ctx, sourceResource, *fieldExport) + + //Assertions + require.Nil(err) + require.NotNil(latest.Status) + require.Len(latest.Status.Conditions, 0) + assertPatchedConfigMap(false, t, ctx, kc) + assertPatchedSecret(true, t, ctx, kc) +} + // Assertions func assertPatchedConfigMap(expected bool, t *testing.T, ctx context.Context, kc *ctrlrtclientmock.Client) { From f6caeceb92749758b5b6200c83dd384091650f8c Mon Sep 17 00:00:00 2001 From: Baiju Muthukadan Date: Wed, 24 Aug 2022 16:04:37 +0530 Subject: [PATCH 3/3] Check for nil Signed-off-by: Baiju Muthukadan --- pkg/runtime/field_export_reconciler.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/runtime/field_export_reconciler.go b/pkg/runtime/field_export_reconciler.go index 2c971c6..e56ce95 100644 --- a/pkg/runtime/field_export_reconciler.go +++ b/pkg/runtime/field_export_reconciler.go @@ -296,7 +296,7 @@ func (r *fieldExportReconciler) writeToConfigMap( ) error { // Construct the data key key := fmt.Sprintf("%s.%s", desired.Namespace, desired.Name) - if desired.Spec.To.Key != nil && strings.TrimSpace(*desired.Spec.To.Key) != "" { + if desired.Spec.To != nil && desired.Spec.To.Key != nil && strings.TrimSpace(*desired.Spec.To.Key) != "" { key = *desired.Spec.To.Key } @@ -343,7 +343,7 @@ func (r *fieldExportReconciler) writeToSecret( ) error { // Construct the data key key := fmt.Sprintf("%s.%s", desired.Namespace, desired.Name) - if desired.Spec.To.Key != nil && strings.TrimSpace(*desired.Spec.To.Key) != "" { + if desired.Spec.To != nil && desired.Spec.To.Key != nil && strings.TrimSpace(*desired.Spec.To.Key) != "" { key = *desired.Spec.To.Key }