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

Update MatchesConsul to normalize partitions during comparison. #3284

Merged
merged 2 commits into from
Dec 1, 2023

Conversation

thisisnotashwin
Copy link
Contributor

@thisisnotashwin thisisnotashwin commented Nov 29, 2023

Changes proposed in this PR:

When setup as a secondary in WAN federation, Consul migrates config entries from the primary datacenter into the kubernetes secondaries. When the config entries are saved, the partition field in the spec of the config entries that have them, gets normalized to the value default if it was previously "" in Consul Enterprise. CRDs for these resources then constantly re-sync themselves since the MatchesConsul method, which is responsible for identifying diffs, evaluates the match incorrectly.

To normalize this behavior, the comparison operation for the impacted CRDs has been updated so that when the Partition fields are compared, empty strings are normalized to the value default. This should ensure that those two values are considered symmetrical and hence should prevent the constant resync of the CRDs into Consul.

How I've tested this PR:

  • Unit tests.
  • Tested on env where this behavior was previously observed and has now stopped occurring.

How I expect reviewers to test this PR:

  • Kind eyes and a whimsical smile. Also, suggestions for additional test cases if you come across them.

Checklist:


specialEquality := cmp.Options{
cmp.FilterPath(func(path cmp.Path) bool {
return path.String() == "Services.Consumers.Partition"
Copy link
Member

Choose a reason for hiding this comment

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

Exports are kinda weird -- I'd be surprised if they need to be normalized. Was it actually being modified during the wanfed sync?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i didnt check.. based on the conversation with Dhia, i kinda blanket changed everywhere that one could specify a partition.


specialEquality := cmp.Options{
cmp.FilterPath(func(path cmp.Path) bool {
return path.String() == "Members.Partition"
Copy link
Member

@hashi-derek hashi-derek Nov 30, 2023

Choose a reason for hiding this comment

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

Sameness groups are enterprise-only, so I'd be surprised if they actually sync through wanfed and modify fields. I guess it doesn't hurt to leave this in place, though.

@@ -96,6 +98,7 @@ func TestServiceResolver_MatchesConsul(t *testing.T) {
Targets: []ServiceResolverFailoverTarget{
{Peer: "failover_peer3"},
{Partition: "failover_partition3", Namespace: "failover_namespace3"},
{Partition: "default", Namespace: "default"},
Copy link
Member

Choose a reason for hiding this comment

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

To make this a little bit more clear on it being a valid config entry, can we assign another field to this? Perhaps Service or Peer? That way, we won't have an empty {} in the comparison, which isn't really valid data server-side. I suppose the same applies to all of these config entries that are comparing empty {}, if possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good suggestion. ill update the tests to reflect this.

@david-yu david-yu linked an issue Nov 30, 2023 that may be closed by this pull request
@thisisnotashwin thisisnotashwin merged commit 6e9f63d into main Dec 1, 2023
48 checks passed
@thisisnotashwin thisisnotashwin deleted the ashwin/NET-6700-normalize-partitions branch December 1, 2023 16:12
@thisisnotashwin thisisnotashwin removed the backport/1.1.x Backport to release/1.1.x branch label Dec 1, 2023
sarahalsmiller pushed a commit that referenced this pull request Jan 5, 2024
* Update MatchesConsul to normalize partitions during comparison.
* Update test cases with valid datasets
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.2.x This release branch is no longer active. backport/1.3.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ServiceRouter constantly being synced when using enterprise Consul
2 participants