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

feat: adding annotations support #139

Closed
wants to merge 1 commit into from
Closed

feat: adding annotations support #139

wants to merge 1 commit into from

Conversation

cabrinha
Copy link

@cabrinha cabrinha commented Jul 29, 2019

Resolves #137 by allowing annotations to be passed through the ExternalSecret and into the resulting Secret.

Not quite sure how to get the tests passing, but I've tested this on my cluster and it works with and without annotations added to the ExternalSecret object.

@jeffpearce
Copy link
Contributor

jeffpearce commented Jul 29, 2019

Thanks for the PR @cabrinha

Here's the failing test (from the Travis build).

image

I think that the annotations should be optional; if not provided in the external secret, we shouldn't try to pass them when we create the secret. That would probably fix that failing test.

Also, rather than modifying the existing tests, I think we should just add a new one that verifies that the code can pass annotations (if provided). The unmodified existing tests would verify that the behavior hasn't changed if there are no annotations.

Let me know if you have questions about any of that.

@cabrinha
Copy link
Author

cabrinha commented Jul 29, 2019

Thanks for the PR @cabrinha

Here's the failing test (from the Travis build).

image

I think that the annotations should be optional; if not provided in the external secret, we shouldn't try to pass them when we create the secret. That would probably fix that failing test.

Also, rather than modifying the existing tests, I think we should just add a new one that verifies that the code can pass annotations (if provided). The unmodified existing tests would verify that the behavior hasn't changed if there are no annotations.

Let me know if you have questions about any of that.

Yep, thanks for taking a look @jeffpearce ! I changed annotations to be optional and also altered the existing tests to expect an empty annotations key. If I need to add specific tests let me know, otherwise I think this could be good to go now.

Screen Shot 2019-07-29 at 10 13 24 AM

Copy link
Contributor

@silasbw silasbw left a comment

Choose a reason for hiding this comment

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

Hey, thanks for the PR! A have a high-level point worth discussing.

@@ -36,7 +36,7 @@ class Daemon {
* @returns {Object} Poller descriptor.
*/
_createPollerDescriptor (externalSecret) {
const { uid, name, namespace } = externalSecret.metadata
const { uid, name, namespace, annotations = {} } = externalSecret.metadata
Copy link
Contributor

Choose a reason for hiding this comment

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

In general, I worry about copying one object's annotations directly to another object. A more "kubernetes-like" approach would be to have a Secret template (analogous to the Pod template for Deployments) and set the annotation values there that you want the external secrets controller to apply to the corresponding Secret.

We've discussed this approach before, but had some concerns with reconciling in with other "frontends" that don't use Secret objects (see #46).

What's the use case for this? Maybe there's another way to support it?

@cabrinha
Copy link
Author

cabrinha commented Aug 5, 2019

@silasbw thanks for the consideration. The use-case is covered in #137.

I'd like to use the kubernetes-replicator deployment to copy external secrets (and maybe later configmaps) to other/all namespaces automatically.

What would be better is if I didn't have to use that project at all, but if there were an option to have the ExternalSecret replicated to every (or some) namespace on creation.

Create the secret once, but be able to consume it everywhere.

@silasbw
Copy link
Contributor

silasbw commented Aug 21, 2019

This fell off my radar @cabrinha. Is it still useful for you? If it us, would something like the approach I mention above work? For example, there'd be a secretDescriptor.template.metadata.annotations object you could add annotations to that the controller would copy to your resulting Secret object.

@cabrinha
Copy link
Author

This fell off my radar @cabrinha. Is it still useful for you? If it us, would something like the approach I mention above work? For example, there'd be a secretDescriptor.template.metadata.annotations object you could add annotations to that the controller would copy to your resulting Secret object.

That would be great. Yes, this feature would still be useful to me. I think others would also find a use for it along side kubernetes-replicator.

Let me know if there is anything I can do to help.

@iAnomaly
Copy link
Contributor

This is critical for us as well as we use Helm hook annotations on our Secrets currently. @silasbw @cabrinha What needs to happen to get this merged and how can I help? Thanks.

@silasbw
Copy link
Contributor

silasbw commented Oct 31, 2019

I think secretDescriptor.template.metadata.annotations is the way to go. An ExternalSecret using that might look like this, for example:

apiVersion: 'kubernetes-client.io/v1'
kind: ExternalSecret
metadata:
  name: hello-service
secretDescriptor:
  backendType: secretsManager
  data:
    - key: hello-service/password
      name: password
  template:
    metadata:
      labels:
        dog: farfel
      annotations:
        cat: cheese

The corresponding Secret would look like:

kind: Secret
metadata:
  labels:
    dog: farfel
  annotations:
    cat: cheese
...

@iAnomaly, want to take a pass at implementing that?

@iAnomaly
Copy link
Contributor

iAnomaly commented Nov 4, 2019

@silasbw @cabrinha #192

@silasbw
Copy link
Contributor

silasbw commented Nov 5, 2019

Closing in favor of #192

@silasbw silasbw closed this Nov 5, 2019
@cabrinha cabrinha deleted the annotations branch November 5, 2019 19:56
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.

Support for Annotations
4 participants