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

Ignore partition/namespace on SourceIntention list to match top-level compare logic #1804

Merged
merged 2 commits into from
Dec 19, 2022

Conversation

kyhavlov
Copy link
Contributor

@kyhavlov kyhavlov commented Dec 19, 2022

This PR updates the logic in MatchesConsul for intentions to add partition/namespace to the list of ignored fields for the SourceIntention list to match the ignored fields on the top-level config entry struct. This is to fix an issue where upgrading from Consul pre-1.12 to 1.12+ can trigger an update of all config entries due to the partition/namespace fields in the SourceIntentions from Consul being filled in as "default" instead of "".

Here's what the entry coming from consul 1.12+ looks like:

{
  "Kind": "service-intentions",
  "Name": "*",
  "Partition": "default",
  "Namespace": "default",
  "Sources": [
    {
      "Name": "foo",
      "Partition": "default",
      "Namespace": "default",
      "Action": "allow",
      ...
    }
  ],
  ...
}

And this is what consul-k8s generates via the ToConsul function:

{
  "Kind": "service-intentions",
  "Name": "*",
  "Sources": [
    {
      "Name": "foo",
      "Action": "allow",
      ...
    }
  ],
  ...
}

How I've tested this PR:
Created a consul 1.11.1+ent cluster alongside consul-k8s with a couple basic service intention config entries, then upgraded to 1.12.3+ent. Without this change there's an update to each intention because of the diff/MatchesConsul returns false.

How I expect reviewers to test this PR:

Checklist:

  • Tests added
  • CHANGELOG entry added

Copy link
Contributor

@ndhanushkodi ndhanushkodi left a comment

Choose a reason for hiding this comment

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

I think we just need a changelog, and are we planning to backport this? If so we'll need backport labels. 0.49.x of consul-k8s is supposed to work with 1.13.x and 1.12.x of Consul. I don't think we test with Consul 1.11.x anymore but in theory that should have worked with 0.49.x at some point.

Thanks for fixing, it looks great to me!

@david-yu david-yu added backport/0.49.x 0.49.x release branches backport/1.0.x labels Dec 19, 2022
@david-yu
Copy link
Contributor

Thanks I've added a backport label for 0.49.x and 1.0.x

@kyhavlov kyhavlov force-pushed the kyhavlov/diff-intention-sources branch from b0fcd67 to 7300245 Compare December 19, 2022 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/0.49.x 0.49.x release branches backport/1.0.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants