Secretsmanager: Implement LateInitialize of a K8s secret when AWS secret already exists #669

MisterMX · 2021-05-17T13:15:31Z

Description of your changes

Fixes #668

The controller now creates a K8s with the value from AWS if it does not already exist.

I have:

Read and followed Crossplane's [contribution process].
Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

Manually

MisterMX · 2021-11-11T08:48:30Z

@haarchri done

haarchri · 2021-11-15T14:44:22Z

@MisterMX sorry we merged #907 can you rebase again

MisterMX · 2021-11-16T11:17:06Z

@haarchri Done ;-)

muvaf

We haven't done late-init for an input Secret before but I think it makes sense given that it's very similar to other params except that it's stored differently. Thanks @MisterMX !

pkg/controller/secretsmanager/secret/setup.go

muvaf · 2022-01-31T16:35:29Z

pkg/controller/secretsmanager/secret/setup.go

+	}
+
+	payload := map[string][]byte{}
+	if v, exists := parsed[awsclients.StringValue(ref.Key)]; exists {


We send the data as a string if the key is given, I'd expect the reverse of this to happen in late-init, i.e. look for a string only if the key is given. If not, then we should always treat that the whole map is requested. So, I think we should only iterate here and not account for the case where there is a key, blob in AWS is a map and user actually meant a specific key in that whole blob in AWS.

Oh in fact, it will never be non-nil anyway since we checked it in the beginning.

Yes, I think we can go for always iterating to keep the external and local data in sync.

Though it will be overwritten in the next reconcile if a ref.Key is set (because payload will be different).

muvaf · 2022-01-31T16:36:31Z

pkg/controller/secretsmanager/secret/setup.go

-	default:
-		return nil, errors.New("neither binarySecretRef nor stringSecretRef is given")
+func (e *hooks) getPayload(ctx context.Context, cr *svcapitypes.SecretParameters) ([]byte, error) {
+	ref, err := getSecretRef(cr)


I think it's easier to follow what's happening if we have duplication for that switch case instead of calling another function.

I would prefer to keep code encapsulated that is used more than once.

I modified the function so it will return an error in case StringSecretRef and BinarySecretRef are both set. Before that, StringSecretRef took preference over BinarySecretRef and I don't think that was intended. I also added a note to the API docs.

muvaf · 2022-01-31T16:49:29Z

pkg/controller/secretsmanager/secret/setup.go

-		ref = cr.Spec.ForProvider.BinarySecretRef
-	default:
-		return nil, errors.New("neither binarySecretRef nor stringSecretRef is given")
+func (e *hooks) getPayload(ctx context.Context, cr *svcapitypes.SecretParameters) ([]byte, error) {


cr seems to be used to point to the whole Secret object in this file, i.e. as in (c)ustom (r)esource. So, I'd suggest either keeping this line as is or use params as variable name when it addresses SecretParameters.

Renamed to params.

MisterMX

Thanks @muvaf for the review. I added (most of) your requested changes and rebased on current master.

One last proposal before bumping this to v1beta1:

Currently we have a binarySecretRef and stringSecretRef next to each other. Wouldn't it be better if we'd have a single secretRef and a format property instead? This way we would be able to support different file formats, such as yaml, json etc.

MisterMX · 2022-02-01T11:53:34Z

pkg/controller/secretsmanager/secret/setup.go

+	}
+
+	payload := map[string][]byte{}
+	if v, exists := parsed[awsclients.StringValue(ref.Key)]; exists {


Yes, I think we can go for always iterating to keep the external and local data in sync.

Though it will be overwritten in the next reconcile if a ref.Key is set (because payload will be different).

MisterMX · 2022-02-01T12:16:51Z

pkg/controller/secretsmanager/secret/setup.go

-	default:
-		return nil, errors.New("neither binarySecretRef nor stringSecretRef is given")
+func (e *hooks) getPayload(ctx context.Context, cr *svcapitypes.SecretParameters) ([]byte, error) {
+	ref, err := getSecretRef(cr)


I would prefer to keep code encapsulated that is used more than once.

I modified the function so it will return an error in case StringSecretRef and BinarySecretRef are both set. Before that, StringSecretRef took preference over BinarySecretRef and I don't think that was intended. I also added a note to the API docs.

MisterMX · 2022-02-01T12:17:04Z

pkg/controller/secretsmanager/secret/setup.go

-		ref = cr.Spec.ForProvider.BinarySecretRef
-	default:
-		return nil, errors.New("neither binarySecretRef nor stringSecretRef is given")
+func (e *hooks) getPayload(ctx context.Context, cr *svcapitypes.SecretParameters) ([]byte, error) {


Renamed to params.

The controller now creates a K8s with the value from AWS if it does not already exist. Signed-off-by: Maximilian Blatt <maximilian.blatt-extern@deutschebahn.com> (external expert on behalf of DB Netz AG)

muvaf · 2022-02-07T11:10:53Z

pkg/controller/secretsmanager/secret/setup.go

@@ -113,6 +121,79 @@ type hooks struct {
 	kube   client.Client
 }

+func (e *hooks) lateInitialize(spec *svcapitypes.SecretParameters, resp *svcsdk.DescribeSecretOutput) error {
+	payload, err := e.getPayload(context.TODO(), spec)
+	if err := client.IgnoreNotFound(err); err != nil || payload == nil {


Suggested change

if err := client.IgnoreNotFound(err); err != nil || payload == nil {

if err := client.IgnoreNotFound(err); err != nil || payload != nil {

I think my initial code suggestion was the opposite of what you're trying to do in that if statement, sorry about that. We should return nil in case payload is full since we don't need to take any action.

muvaf

I'm merging this to unblock #1107 before the release. I'll open a follow-up PR if my concern about payload being nil is indeed a problem. Thanks @MisterMX !!

MisterMX force-pushed the secretsmanager-lateinitialize branch from 0bbe08c to 96a17f6 Compare June 22, 2021 09:25

AaronME assigned muvaf Sep 24, 2021

MisterMX force-pushed the secretsmanager-lateinitialize branch from 96a17f6 to c2bbe99 Compare November 11, 2021 08:24

MisterMX force-pushed the secretsmanager-lateinitialize branch from c2bbe99 to e523207 Compare November 16, 2021 11:02

haarchri mentioned this pull request Jan 31, 2022

Bump VPCLink, Secret and Function to v1beta1 #1107

Merged

2 tasks

muvaf reviewed Jan 31, 2022

View reviewed changes

MisterMX commented Feb 1, 2022

View reviewed changes

MisterMX force-pushed the secretsmanager-lateinitialize branch from 9239fe4 to baec13f Compare February 1, 2022 12:24

feat(secretsmanager): Add lateInitialize to secret

26ea6cb

The controller now creates a K8s with the value from AWS if it does not already exist. Signed-off-by: Maximilian Blatt <maximilian.blatt-extern@deutschebahn.com> (external expert on behalf of DB Netz AG)

MisterMX force-pushed the secretsmanager-lateinitialize branch from baec13f to 26ea6cb Compare February 1, 2022 12:25

muvaf reviewed Feb 7, 2022

View reviewed changes

muvaf approved these changes Feb 8, 2022

View reviewed changes

muvaf merged commit b4640f8 into crossplane-contrib:master Feb 8, 2022

muvaf mentioned this pull request Feb 8, 2022

secretsmanager.secret: make late init work only if the input secret does not exist #1127

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Secretsmanager: Implement LateInitialize of a K8s secret when AWS secret already exists #669

Secretsmanager: Implement LateInitialize of a K8s secret when AWS secret already exists #669

MisterMX commented May 17, 2021

MisterMX commented Nov 11, 2021

haarchri commented Nov 15, 2021

MisterMX commented Nov 16, 2021

muvaf left a comment

muvaf Jan 31, 2022

muvaf Jan 31, 2022

MisterMX Feb 1, 2022

muvaf Jan 31, 2022

MisterMX Feb 1, 2022

muvaf Jan 31, 2022

MisterMX Feb 1, 2022

MisterMX left a comment

MisterMX Feb 1, 2022

MisterMX Feb 1, 2022

MisterMX Feb 1, 2022

muvaf Feb 7, 2022

muvaf left a comment •

edited

Loading

	if err := client.IgnoreNotFound(err); err != nil \|\| payload == nil {
	if err := client.IgnoreNotFound(err); err != nil \|\| payload != nil {

Secretsmanager: Implement LateInitialize of a K8s secret when AWS secret already exists #669

Secretsmanager: Implement LateInitialize of a K8s secret when AWS secret already exists #669

Conversation

MisterMX commented May 17, 2021

Description of your changes

How has this code been tested

MisterMX commented Nov 11, 2021

haarchri commented Nov 15, 2021

MisterMX commented Nov 16, 2021

muvaf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MisterMX left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muvaf left a comment • edited Loading

Choose a reason for hiding this comment

muvaf left a comment •

edited

Loading