Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG: Controller never reconciles on update even if secret sync has changed #83

Open
AlexanderStocks opened this issue Sep 10, 2024 · 3 comments · May be fixed by #84 or #90
Open

BUG: Controller never reconciles on update even if secret sync has changed #83

AlexanderStocks opened this issue Sep 10, 2024 · 3 comments · May be fixed by #84 or #90
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@AlexanderStocks
Copy link
Contributor

What steps did you take and what happened:
When updating a secretsync, the controller will not re-reconcile a secret on an update.

What did you expect to happen:
I expected the controller to re-reconcile if the secretsync is updated.

Anything else you would like to add:
[Miscellaneous information that will assist in solving the issue.]
shouldReconcilePredicate is called and then calls processIfSecretChanged which always returns false because the syncHash is never updated.
Proposed solution is to return true in shouldReconcilePredicate in the case of an UpdateEvent and not call processIfSecretChanged

Which provider are you using:
[e.g. Azure Key Vault, HashiCorp Vault, etc. Have you checked out the provider's repo for more help?]

Environment:

  • Secrets Store Sync Controller version: (use the image tag):
  • Kubernetes version: (use kubectl version):
@AlexanderStocks AlexanderStocks added the kind/bug Categorizes issue or PR as related to a bug. label Sep 10, 2024
@AlexanderStocks AlexanderStocks linked a pull request Sep 10, 2024 that will close this issue
3 tasks
@AlexanderStocks
Copy link
Contributor Author

We can either always reconcile on the update or we can separate out the logic for calculating the hash in the reconcile function and then also call it in the shouldReconcile function to calculate the hash of the new object.

@marcelocyreno
Copy link

/assign

@AlexanderStocks
Copy link
Contributor Author

@nilekhc simon on my team proposed this

UpdateFunc: func(e event.UpdateEvent) bool {
  old, ok := e.ObjectOld.(*secretsyncv1alpha1.SecretSync)
  if !ok {
    return true
  }
  new, ok := e.ObjectNew.(*secretsyncv1alpha1.SecretSync)
  if !ok {
    return true
  }
  return !reflect.DeepEqual(old.Spec, new.Spec)
},

Given the function to calculate the secret hash requires us to also fetch the secret data, this seems like the best option. What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
2 participants