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

Backport of [Net-5510][Net-5455]: CRD controller should only patch the finalizer in the metadata of a CRD, rather than the whole object into release/1.1.x #3410

Conversation

hc-github-team-consul-core
Copy link
Collaborator

Backport

This PR is auto-generated from #3362 to be assessed for backporting due to the inclusion of the label backport/1.1.x.

🚨

Warning automatic cherry-pick of commits failed. If the first commit failed,
you will see a blank no-op commit below. If at least one commit succeeded, you
will see the cherry-picked commits up to, not including, the commit where
the merge conflict occurred.

The person who merged in the original PR is:
@ndhanushkodi
This person should manually cherry-pick the original PR into a new backport PR,
and close this one when the manual backport PR is merged in.

merge conflict error: POST https://api.github.com/repos/hashicorp/consul-k8s/merges: 409 Merge conflict []

The below text is copied from the body of the original PR.


Changes proposed in this PR

Issue: ArgoCD will flap when configured with replace mode, which causes zero values to get defaulted and the applied config to differ from the state in K8s.

This is how the ArgoCD flapping problem happens:

  1. Argo triggers a replace
  2. A replace operation will remove all finalizers, including the one that we add to all config entries in the config entry controller
  3. This triggers our config entry controller to run a reconcile to add the finalizer
  4. When our controller adds the finalizer, it updates the whole object, causing the zero values to get defaulted. (json omit empty does not kick in because the object is not being serialized/deserialized like it does when you first apply the config).

This PR:

  • Only patches finalizers in the metadata of the CRD, rather than updating the entire entry, which had caused 0 values to be defaulted when using kubectl with replace.

Added @hashi-derek as a coauthor for his work on a finalizer patcher POC that I used for this.

How I've tested this PR

From this repo https://github.com/ndhanushkodi/argocd-example-apps/tree/nd/debug-consul-config/consul-counting

Lightweight test steps with kubectl replace:
kubectl apply -f counting.yaml -f dashboard.yaml -f splitter.yaml

Test steps with Argo CD:

  1. Followed these instructions to setup argoCD, pointing to the consul-counting directory, set up the sync in manual mode with the "replace" setting.
  2. Hit the sync button in argoCD a few times.
  3. Before this PR, there would always be a diff that showed up in argoCD when using the "replace" setting with the config, because of the omitted values getting a default 0 value set.
  4. After this PR, there is no longer a diff in argoCD.

How I expect reviewers to test this PR

👀

Checklist


Overview of commits

@hc-github-team-consul-core hc-github-team-consul-core force-pushed the backport/nd/net-5510-net-5455-patch-config-entries/evenly-discrete-gelding branch 2 times, most recently from 5a55729 to ef2769e Compare December 20, 2023 01:13
@hashicorp-cla
Copy link

CLA assistant check

Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement

Learn more about why HashiCorp requires a CLA and what the CLA includes


temp seems not to be a GitHub user.
You need a GitHub account to be able to sign the CLA. If you already have a GitHub account, please add the email address used for this commit to your account.

Have you signed the CLA already but the status is still pending? Recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants