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

In policy matching subset intersect is incorrect #12319

Closed
Icarus9913 opened this issue Dec 18, 2024 · 9 comments · Fixed by #12340
Closed

In policy matching subset intersect is incorrect #12319

Icarus9913 opened this issue Dec 18, 2024 · 9 comments · Fixed by #12340
Assignees
Labels
area/policies kind/bug A bug triage/accepted The issue was reviewed and is complete enough to start working on it

Comments

@Icarus9913
Copy link
Contributor

What happened?

Went through the Subset Intersect method source codes, I found there's one case would be failed:

Entry("entry 66", testCase{
  s1: []core_rules.Tag{
    {Key: "key1", Value: "val1"},
    {Key: "key2", Value: "abc"},
  },
  s2: []core_rules.Tag{
    {Key: "key2", Value: "val2"},
    {Key: "key3", Value: "val3"},
  },
  intersect: false,
}),

In this case, there should not be one intersection but our codes would give us intersect: true

@Icarus9913 Icarus9913 added triage/pending This issue will be looked at on the next triage meeting kind/bug A bug labels Dec 18, 2024
@Icarus9913
Copy link
Contributor Author

if !ok {
return true
}

This should be continue

@lahabana lahabana changed the title A bug found in Subset intersect method Subset intersect method is sometimes wrong Dec 19, 2024
@lahabana
Copy link
Contributor

What's the breaking change risk here? How likely are people to be relying on this behaviour?

@lahabana lahabana changed the title Subset intersect method is sometimes wrong In policy matching subset intersect is incorrect Dec 19, 2024
@Icarus9913
Copy link
Contributor Author

What's the breaking change risk here? How likely are people to be relying on this behaviour?

I need to assess it

@Icarus9913
Copy link
Contributor Author

@lahabana I did lots of tests and analysis for this, but didn't find an impact on the customer usage side. I also used ci-full-matrix to run our e2e to test this and no errors were found.

But, this bug would generate the wrong rules for the issue: #12273 which is the sub-issue of the Compute function.

@lobkovilya
Copy link
Contributor

I think the code works as expected. In your example:

s1: []core_rules.Tag{
  {Key: "key1", Value: "val1"},
  {Key: "key2", Value: "abc"},
},
s2: []core_rules.Tag{
  {Key: "key2", Value: "val2"},
  {Key: "key3", Value: "val3"},
},

these two sets have an intersection {key1: val1}

  • {key1: val1} belongs to s1
  • {key1: val1} belongs to s2 (because s2 doesn't have key1)

@Icarus9913
Copy link
Contributor Author

Icarus9913 commented Dec 23, 2024

I think the code works as expected. In your example:

s1: []core_rules.Tag{
  {Key: "key1", Value: "val1"},
  {Key: "key2", Value: "abc"},
},
s2: []core_rules.Tag{
  {Key: "key2", Value: "val2"},
  {Key: "key3", Value: "val3"},
},

these two sets have an intersection {key1: val1}

  • {key1: val1} belongs to s1
  • {key1: val1} belongs to s2 (because s2 doesn't have key1)

This is a little bit confused, in sets s1 and s2, they have the same key key2 but with different value. Isn't it contradictory?

@lobkovilya
Copy link
Contributor

This is a little bit confused, in sets s1 and s2, they have the same key key2 but with different value. Isn't it contradictory?

Why is this contradictory? You need just 1 element that belongs to both s1 and s2 to have an intersection

@Icarus9913
Copy link
Contributor Author

Icarus9913 commented Dec 23, 2024

Also we have different behaviors once I modified the s1 key-values order. We can see the two Entry have the same tags-pair but just with different orders, in the end we got different results. Does the order matter?

	FEntry("entry 1", testCase{
		s1: []core_rules.Tag{
			{Key: "key2", Value: "abc"},
			{Key: "key1", Value: "val1"},
		},
		s2: []core_rules.Tag{
			{Key: "key2", Value: "val2"},
			{Key: "key3", Value: "val3"},
		},
		intersect: false,
	}),
	FEntry("entry 2", testCase{
		s1: []core_rules.Tag{
			{Key: "key1", Value: "val1"},
			{Key: "key2", Value: "abc"},
		},
		s2: []core_rules.Tag{
			{Key: "key2", Value: "val2"},
			{Key: "key3", Value: "val3"},
		},
		intersect: true,
	}),

@lahabana lahabana added triage/accepted The issue was reviewed and is complete enough to start working on it and removed triage/pending This issue will be looked at on the next triage meeting labels Dec 23, 2024
@lobkovilya
Copy link
Contributor

I think the code works as expected. In your example:

s1: []core_rules.Tag{
  {Key: "key1", Value: "val1"},
  {Key: "key2", Value: "abc"},
},
s2: []core_rules.Tag{
  {Key: "key2", Value: "val2"},
  {Key: "key3", Value: "val3"},
},

these two sets have an intersection {key1: val1}

  • {key1: val1} belongs to s1
  • {key1: val1} belongs to s2 (because s2 doesn't have key1)

Actually, you're right, my example here is incorrect. Since there are 3 keys in total (key1, key2 and key3) then the element of the intersection has to be in 3-dimensional space. But I have an example {key1: val1} of a 1-dimensional. And in 3-dimensional space there are actually no points (because key2 has different values), so no intersection between s1 and s2.

So the issue is correct, I'm going to review the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/policies kind/bug A bug triage/accepted The issue was reviewed and is complete enough to start working on it
Projects
None yet
3 participants