Pre-existing secret not being updated on Kubernetes v1.20
pauloeliasjr opened this issue · 11 comments
Describe the bug
Contents from a pre-existing secret is not being overwritten/merged with source secret on Kubernetes v1.20.
Working fine on v1.18.18 and v1.19.10.
To Reproduce
- Deployed kubernetes v1.20 using Minikube
- Deployed kubernetes-replication using manifests
- Created two namespaces
kubectl create ns test-secret-1
kubectl create ns test-secret-2
- Created secret at namespace 1
kubectl create secret docker-registry docker-registry \
--namespace test-secret-1 \
--docker-username=DOCKER_USERNAME \
--docker-password=DOCKER_PASSWORD
- Created secret at namespace 2 (same name, different content)
kubectl create secret docker-registry docker-registry \
--namespace test-secret-2 \
--docker-username=ANOTHER_DOCKERUSER \
--docker-password=ANOTHER_DOCKERPASSWORD
- Annotate secret at namespace 1
kubectl annotate secret docker-registry -n test-secret-1 "replicator.v1.mittwald.de/replicate-to"="*"`
- Check kubernetes-replicator logs
could not replicate object to other namespaces" error="Replicated test-secret-1/docker-registry to 1 out of 1 namespaces: 1 error occurred:\n\t* Failed to replicate Secret test-secret-1/docker-registry -> test-secret-2: Failed to update secret test-secret-2/docker-registry: secrets \"docker-registry\" already exists:`
Expected behavior
Secret contents should be merged, as defined here
Environment:
- Kubernetes version: [e.g. 1.20]
- kubernetes-replicator version: [e.g. v2.6.1]
Thanks for the report! 👍 Off the top of my head, I don't have a really good idea as to what could cause this, so this'll need some investigation... ⏳
Did the replicator log anything of note before the could not replicate [...] error message?
I did some testing, and while I was unable to replicate the issue using both Kubernetes 1.20.0 and 1.20.2 on minikube, I think I identified a possible scenario in which the error might occur. Could you provide us with the log-line where the operator checks if the secret already exists in the target ns? It should be the line directly above the error, Checking if test-secret-2/docker-registry exists? [...]
In the tests in the k8s.io/client-go an error with the matching message "<resource-type> <resource-name> already exists" is returned when attempting to add an existing object without a replaceExisting flag set. Therefore, my hypothesis would be that the replicator tried to create the secret in test-secret-2 using POST as opposed to PUT, because the operator's existence check on the secret in test-secret-2 returned false for some reason, but, by the time replication was performed, the secret existed in that ns.
There has not been any activity to this issue in the last 14 days. It will automatically be closed after 7 more days. Remove the stale label to prevent this.
Hi Guys, sorry for my delay.
I was unable to reproduce my own issue. :)
I'll assign that one to some weird, undetectable setting on my environment.
Closing...
I believe that I found the underlying issue. The replicator is relying on a informer (cache.NewInformer) to get informed about updates and then reacts on those by doing the actual replication. It however also uses the cache from that same informer to figure out if an object already exists or not, and then based on the does a create or an update.
This will however not work properly when things start up, as the cache does not contain all objects when the replicator handles objects. So it might determine that an object needs to be replicated to a non-existing object, while the target object is already existing and this knowledge is only gained AFTER the source object has been processed. This can currently be mitigated by using a shorter --resync-period (30m by default), as a forced full sync fixes the issue then...this will then however lead to a lot of load on the api servers (and might even fail to finish fast enough, see #132).
I believe that this is a general architecture issue, where a simple fix might be possible but a little bit hackish. Maybe the better solution is to reconsider the architecture and split bookkeeping of existing objects and the actual update logic as part of a reconciliation loop. This way you could start the informer/reflector and wait for the initial sync to finish. The informer would really only keep track of objects. A parallel reconciliation loop would then start and periodically reconcile the to-be-replicated objects.
Another issue I found, which would be indirectly fixed as well with an architecture change: The store returned by cache.NewInformer is not allowed to be modified. Only read operations are allowed, as it otherwise might corrupt the state of the cache/informer. See the comment above the cache.NewInformer method.
@pauloeliasjr Maybe you can reopen this issue?
@martin-helmich As you seem to be one of the devs here, maybe you can look into my analysis?
@codablock Sorry for the delay 🙄🙏 and thanks for the detailed analysis! 👍 I'll reopen this for now and give it a detailed look as soon as I find the time (probably some time next week).
There has not been any activity to this issue in the last 14 days. It will automatically be closed after 7 more days. Remove the stale label to prevent this.
Was there any followup about this problem? We believe we just experienced this behavior. We are on aks v1.24.9
and give it a detailed look as soon as I find the time (probably some time next week).
...I wrote, almost two years ago. 🙈 That didn't work out very well. I'll reopen this issue for now; we've got rid of the stale workflow since then, as it caused more problems that it solved (this issue being the best example). However, I cannot make any promises as to when I'll get around to look into this -- in the meantime, PRs are of course very welcome.
I believe I'm running into the same issue, and bumping up the initialDelaySeconds to a value greater than the resync period worked to get the pod to be being in a "crash loop". (It wasn't actually crashing but Kubernetes doesn't know the difference.)