Skip to content
This repository has been archived by the owner on Jul 26, 2022. It is now read-only.

feat: Upsert secrets only when needed #782

Merged
merged 1 commit into from
Jul 2, 2021

Conversation

stephenthedev
Copy link
Contributor

This implements a rudimentary data check on secrets polled to only call put/post actions when the data of the secret has changed.

This is aimed at resolving #737

@stephenthedev
Copy link
Contributor Author

stephenthedev commented Jun 25, 2021

@moolen @Flydiverny Can you help kick off CI? Thanks!

lib/poller.js Outdated Show resolved Hide resolved
@stephenthedev stephenthedev force-pushed the rag-737 branch 2 times, most recently from 6eb4cce to f601147 Compare June 30, 2021 09:25
@moolen
Copy link
Member

moolen commented Jun 30, 2021

LGTM so far, thank you! I want to check out your branch and do some manual testing.

@stephenthedev
Copy link
Contributor Author

@moolen awesome thanks! Looking forward to this landing for our use case :-)

Copy link
Member

@moolen moolen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested it locally but found that it wasn't working because some metadata fields were present:

// new metadata (from `secretManifest`)
{
    "labels": {
        "secretLabel": "Hellofoo123"
    },
    "name": "e2e-secretmanager-template-sql65h62hxarnqjmpgkj4",
    "ownerReferences": [
        {
            "apiVersion": "kubernetes-client.io/v1",
            "controller": true,
            "kind": "ExternalSecret",
            "name": "e2e-secretmanager-template-sql65h62hxarnqjmpgkj4",
            "uid": "b13c2a96-5258-4ec5-be37-7217aec71220"
        }
    ]
}
// current metadata (from `kubeSecret`)
{
    "name": "e2e-secretmanager-template-sql65h62hxarnqjmpgkj4",
    "labels": {
        "secretLabel": "Hellofoo123"
    },
    "managedFields": [
        {
            // ...
        }
    ]
}

I suggested two things in the comments. it would be great to have e2e tests for that (i did them manually today) but i don't have an approach for your right now that from the top of my head. Maybe you can figure something out if you have the time for it.

lib/poller.js Outdated Show resolved Hide resolved
lib/poller.js Outdated Show resolved Hide resolved
@stephenthedev
Copy link
Contributor Author

@moolen made the change. As for the e2e test, we don't have any existing instrumentation to trap calls made to the API so that'd be a bit of a lift to assert some of that. Given its now an allow list, I think the vector for that becomes pretty small. What do you think?

@stephenthedev
Copy link
Contributor Author

Thinking about this more, the allowList also makes the unit test a lot more accurate in asserting the behavior, so maybe the e2e is overkill?

lib/poller.js Outdated Show resolved Hide resolved
lib/poller.js Outdated Show resolved Hide resolved
lib/poller.test.js Outdated Show resolved Hide resolved
@moolen
Copy link
Member

moolen commented Jul 1, 2021

Given its now an allow list, I think the vector for that becomes pretty small. What do you think?

Sure, that works for me!

Copy link
Member

@moolen moolen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for your contribution 🎉

@moolen moolen merged commit 48db901 into external-secrets:master Jul 2, 2021
@stephenthedev
Copy link
Contributor Author

@moolen Awesome thank you for the review! What is the release cadence for this?

@Flydiverny
Copy link
Member

@stephenthedev I made a release now :)
Thanks for the contribution!

@stephenthedev
Copy link
Contributor Author

Amazing thank you both!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants