feat: add support for pushing whole Secret resources to a secret store#2288
Conversation
There was a problem hiding this comment.
Greetings!
Thank you for contributing to this project!
If this is your first time contributing, please make
sure to read the Developer and Contributing Process guides.
Please also mind and follow our Code of Conduct.
Useful commands:
make fmt: Formats the codemake check-diff: Ensures the branch is cleanmake reviewable: Ensures a PR is ready for review
bb7d1fa to
62de73a
Compare
|
@moolen this can be something to consider (besides having a PushSecret dataFrom) 🤔 |
62de73a to
1312f1e
Compare
|
IMO I'd prefer keeping the secret-key mandatory and having the |
I'd be willing to implement it, could you describe what kind of schema you have in mind? |
|
Right now you have to write something like this in your PushSecret data:
- match:
secretKey: key
remoteRef:
remoteKey: my-pushed-secretIf we add the dataFrom:
- find:
name:
regexp: "^secret-" # <-- this regex would be applied to every key in the source secretSo if the regexp is What does everybody else think? |
|
Regarding As for this PR i wouldn't mind to make it optional, that's simple enough and provides great value. But that's just my 5ct. Let me summon @gusfcarvalho @knelasevero |
|
I took a look at the other providers on how they implement PushSecret, my only concern would be some incompatibility that we may cause (i.e. instead of generating an actual key/Value secret for users to see, we generate a JSON that's not readable). The two providers that we may run in this situation are AWS SM and Kubernetes (from what I've seen), and for those:
I think we could have it as an addition indeed, if we are ok with the "weird kubernetes behavior" I've just described. |
e2d79d4 to
a6dac1c
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
|
Hi @mhannache-equativ ! Sorry for the long delay. We discussed this one on our last community meeting. We can proceed with it as proposed. Can you just bump main on this PR please? 🙈 we can also do that if you're busy. Again ,sorry for the delay. |
|
Hi @gusfcarvalho, I just rebased on main. |
a6dac1c to
80d858d
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
|
/ok-to-test sha=80d858d |
|
any news? |
| if err != nil { | ||
| return out, fmt.Errorf("error marshaling vault secret: %w", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Instead of marshaling the secret data we should change the PushSecret() interface. Then the provider can decide on how to handle that data.
Reason:
- the unneccessary marshal/unmarshal is error prone: binary data isn't well supported (needs encoding, e.g. base64) and then you can't differentiate the data: is it a string, binary-base64 encoded, a jwt 🤷 ?
- the provider doesn't know about the
ref.Match, hence can not differentiate whether this belongs to amatch.seretKeyor this is the whole secret.
Sorry for the late reply, we're under water with PR reviews 😞
Do you have to to finish this up? If not we can take it over
There was a problem hiding this comment.
Hi @moolen, unfortunately I will not have time to refactor the PushSecret() interface, feel free to take over.
Good luck with the other PR reviews!
Signed-off-by: Marin Hannache <mhannache@equativ.com>
80d858d to
6318bde
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
|
Superseded by #2862. |
Problem Statement
It is not currently possible to use a PushSecret to push a whole Secret resource to a secret store. Since the
secretKeyfield is mandatory, only a subset of the secret can be pushed.Related Issue
Related to #2166
Proposed Changes
This MR introduce the possibility to not provide a
secretKey, in this case the Secret resource's data field is converted to a JSON string and pushed to the secret store.Checklist
git commit --signoffmake testmake reviewable