tuenti / secrets-manager

A daemon to sync Vault secrets to Kubernetes secrets
Apache License 2.0
171 stars 26 forks source link

K8s labels annotations #78

Closed eduardogr closed 3 years ago

eduardogr commented 3 years ago

Status

READY

Migrations

Yes, Migrating from apiextensions.k8s.io/v1beta1 to apiextensions.k8s.io/v1

Description

List of fixes # (issue)

Type of change

How Has This Been Tested?

with secrets-manager tests

Checklist:

eduardogr commented 3 years ago

@stevendborrelli i have here your original commits.

After your commits i've added some refactor and a fix for the test you've added. Are you Ok with this?

Thanks a lot for your changes.

stevendborrelli commented 3 years ago

Looks good to me! Thanks for fixing the merge conflict.

eduardogr commented 3 years ago

Thanks a lot @stevendborrelli :)

raelga commented 3 years ago

Thanks for implementing this!

We have been testing it and we faced a couple of issues. The first one is the usage of the v1 without updating the schema nor the controller toolchain (discussed here.

Regarding this feature implementation using metadata propagation, for our particular use case, it leads to an undesired behaviour that we didn't expected.

In some scenarios, labels are used to determine ownership of resources, especially when they are managed from outside the cluster. For example, some GitOps operators, like ArgoCD, use labels to identify what resources are part of a particular ArgoCD application. Propagating the labels to the resources created by the SecretDefinition instance will transfer the ownership of the Secrets created to ArgoCD.

When the SecretDefinition, created by the GitOps operator, propagates the labels, it adds metadata to the Secret resource that will end up with ArgoCD taking the "ownership" of the resource. In a GitOps scenario, that leads to the deletion of the resources, as is not present in the repository. That triggers the recreation of the secret by the secrets-manager operator, and the loop goes on. Using proper ownerReferences is not viable as in multi-cluster scenarios, the GitOps operator manage the resources from an external cluster. This is one of the scenarios that we found, put probably there are other scenarios where this propagation may lead to an undesired behaviour.