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.

Sign up for GitHub

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

Jump to bottom

fix(policy): use new compute for rules and fix rules intersect #12340

Merged

Icarus9913 merged 13 commits into kumahq:master from Icarus9913:fix/wk/auto-reachable

Jan 10, 2025

Contributor

Icarus9913 commented Dec 19, 2024 •

edited

Loading

Motivation

When I went through the policy matching subset Intersect method, I found the behavior is incorrect with one case
Rewrite the rule's Compute method to use the new ContainsElement method instead of using the old IsSubset method

Implementation information

Introduce one new type type Element map[string]string for differentiation by IsSubset.

Supporting documentation

Fix #12319
Fix #12273


          fix(policy): fix policy compute

6578e0e

Signed-off-by: Icarus Wu <[email protected]>

Contributor

github-actions bot commented Dec 19, 2024

Reviewer Checklist

🔍 Each of these sections need to be checked by the reviewer of the PR 🔍:
If something doesn't apply please check the box and add a justification if the reason is non obvious.

Is the PR title satisfactory? Is this part of a larger feature and should be grouped using > Changelog?
PR description is clear and complete. It Links to relevant issue as well as docs and UI issues
This will not break child repos: it doesn't hardcode values (.e.g "kumahq" as an image registry)
IPv6 is taken into account (.e.g: no string concatenation of host port)
Tests (Unit test, E2E tests, manual test on universal and k8s)
- Don't forget ci/ labels to run additional/fewer tests
Does this contain a change that needs to be notified to users? In this case, UPGRADE.md should be updated.
Does it need to be backported according to the backporting policy? (this GH action will add "backport" label based on these file globs, if you want to prevent it from adding the "backport" label use no-backport-autolabel label)

Icarus9913 commented

View reviewed changes

pkg/plugins/policies/core/rules/rules.go Outdated Show resolved Hide resolved

Icarus9913 commented

View reviewed changes

pkg/plugins/policies/meshtrafficpermission/graph/backends/reachable_test.go Outdated Show resolved Hide resolved

Icarus9913 commented

View reviewed changes

pkg/plugins/policies/core/rules/rules_test.go Outdated Show resolved Hide resolved

Icarus9913 commented

View reviewed changes

pkg/plugins/policies/core/rules/rules.go Show resolved Hide resolved

fix

6f6e858

Signed-off-by: Icarus Wu <[email protected]>

This was linked to issues Dec 20, 2024

In policy matching subset intersect is incorrect #12319

Closed

The virtual_domains not generated correctly with AUTO_REACHABLE_SERVICES and MeshTrafficPermission #12273

Closed


          use golang lint rules to ignore some warnings for better reading the …

10c9bcf

…code

Signed-off-by: Icarus Wu <[email protected]>

lahabana previously requested changes

View reviewed changes

Contributor

lahabana left a comment

Seems like there's a refactor/cleanup and a bug fix. Is it possible to separate both changes as it's hard to review otherwise

.golangci.yml Outdated Show resolved Hide resolved

pkg/plugins/policies/core/rules/rules.go Outdated Show resolved Hide resolved

Icarus9913 added 3 commits

December 21, 2024 00:39


          intersect unit test

2d5bf1d

Signed-off-by: Icarus Wu <[email protected]>


          golangci-lint

8035df6

Signed-off-by: Icarus Wu <[email protected]>


          fix meshtimeout policy default timeouts on outbound

0516c46

Signed-off-by: Icarus Wu <[email protected]>

Icarus9913 force-pushed the fix/wk/auto-reachable branch from b2c9c11 to 0516c46 Compare

December 23, 2024 06:18

Icarus9913 added the ci/run-full-matrix label


          supplement empty set and empty element unit test case

51103f1

Signed-off-by: Icarus Wu <[email protected]>

lahabana reviewed

View reviewed changes

pkg/plugins/policies/core/rules/rules.go Outdated Show resolved Hide resolved

pkg/plugins/policies/core/rules/rules.go Outdated Show resolved Hide resolved

pkg/plugins/policies/core/rules/rules.go Outdated Show resolved Hide resolved

pkg/plugins/policies/core/rules/rules.go Outdated Show resolved Hide resolved

pkg/plugins/policies/meshtimeout/plugin/xds/configurer.go Outdated Show resolved Hide resolved

lobkovilya requested changes

View reviewed changes

pkg/plugins/policies/core/rules/rules.go Show resolved Hide resolved

pkg/plugins/policies/meshtimeout/plugin/xds/configurer.go Outdated Show resolved Hide resolved


          supplement comments for ContainsElement function

0485cd0

Signed-off-by: Icarus Wu <[email protected]>

lobkovilya reviewed

View reviewed changes

pkg/plugins/policies/core/rules/rules.go Outdated Show resolved Hide resolved

pkg/plugins/policies/core/rules/rules.go Show resolved Hide resolved

pkg/plugins/policies/core/rules/rules.go Show resolved Hide resolved

Icarus9913 added 3 commits

December 25, 2024 10:54


          fix comment

4005f28

Signed-off-by: Icarus Wu <[email protected]>


          optimize comments

611760f

Signed-off-by: Icarus Wu <[email protected]>


          supplement ContainsElement unit test

c11815d

Signed-off-by: Icarus Wu <[email protected]>

Icarus9913 marked this pull request as ready for review

December 25, 2024 07:09

Icarus9913 requested a review from a team as a code owner

December 25, 2024 07:09

Icarus9913 requested review from slonka and jakubdyszkiewicz and removed request for a team

December 25, 2024 07:09

lobkovilya reviewed

View reviewed changes

pkg/plugins/policies/core/rules/rules.go Outdated Show resolved Hide resolved

pkg/plugins/policies/core/rules/rules.go Outdated Show resolved Hide resolved

pkg/plugins/policies/core/rules/rules_test.go Outdated Show resolved Hide resolved

Icarus9913 and others added 2 commits

January 8, 2025 17:39


          fix negation rules edge case

6add814

Signed-off-by: Icarus Wu <[email protected]>


          replace multi line comment with single line

f194d7c

Signed-off-by: Ilya Lobkov <[email protected]>

lobkovilya approved these changes

View reviewed changes

Contributor

lobkovilya left a comment

LGTM

Icarus9913 dismissed lahabana’s stale review

January 10, 2025 02:15

solved

jijiechen approved these changes

View reviewed changes

Icarus9913 merged commit 3a00363 into kumahq:master

25 checks passed

Icarus9913 added the backport label

Icarus9913 added a commit to Icarus9913/kuma that referenced this pull request


          fix(policy): use new compute for rules and fix rules intersect (kumah…

7b698f9

…q#12340)

## Motivation

1. When I went through the policy matching subset `Intersect` method, I
found the behavior is incorrect with one case
2. Rewrite the rule's `Compute` method to use the new `ContainsElement`
method instead of using the old `IsSubset` method

## Implementation information

Introduce one new type `type Element map[string]string` for
differentiation by `IsSubset`.

## Supporting documentation

<!-- Is there a MADR? An Issue? A related PR? -->

Fix kumahq#12319
Fix kumahq#12273

<!--
> Changelog: skip
-->
<!--
Uncomment the above section to explicitly set a [`> Changelog:` entry
here](https://github.com/kumahq/kuma/blob/master/CONTRIBUTING.md#submitting-a-patch)?
-->

---------

Signed-off-by: Icarus Wu <[email protected]>
Signed-off-by: Ilya Lobkov <[email protected]>
Co-authored-by: Ilya Lobkov <[email protected]>

Icarus9913 added a commit to Icarus9913/kuma that referenced this pull request


          fix(policy): use new compute for rules and fix rules intersect (backp…

374bb3a

…ort of kumahq#12340)

1. When I went through the policy matching subset `Intersect` method, I
found the behavior is incorrect with one case
2. Rewrite the rule's `Compute` method to use the new `ContainsElement`
method instead of using the old `IsSubset` method

Introduce one new type `type Element map[string]string` for
differentiation by `IsSubset`.

<!-- Is there a MADR? An Issue? A related PR? -->

Fix kumahq#12319
Fix kumahq#12273

<!--
> Changelog: skip
-->
<!--
Uncomment the above section to explicitly set a [`> Changelog:` entry
here](https://github.com/kumahq/kuma/blob/master/CONTRIBUTING.md#submitting-a-patch)?
-->

---------

Signed-off-by: Icarus Wu <[email protected]>
Signed-off-by: Ilya Lobkov <[email protected]>
Co-authored-by: Ilya Lobkov <[email protected]>

Icarus9913 added a commit to Icarus9913/kuma that referenced this pull request


          fix(policy): use new compute for rules and fix rules intersect (kumah…

…q#12340)

1. When I went through the policy matching subset `Intersect` method, I
found the behavior is incorrect with one case
2. Rewrite the rule's `Compute` method to use the new `ContainsElement`
method instead of using the old `IsSubset` method

Introduce one new type `type Element map[string]string` for
differentiation by `IsSubset`.

<!-- Is there a MADR? An Issue? A related PR? -->

Fix kumahq#12319
Fix kumahq#12273

<!--
> Changelog: skip
-->
<!--
Uncomment the above section to explicitly set a [`> Changelog:` entry
here](https://github.com/kumahq/kuma/blob/master/CONTRIBUTING.md#submitting-a-patch)?
-->

---------

Signed-off-by: Icarus Wu <[email protected]>
Signed-off-by: Ilya Lobkov <[email protected]>
Co-authored-by: Ilya Lobkov <[email protected]>

Icarus9913 added a commit to Icarus9913/kuma that referenced this pull request


          fix(policy): use new compute for rules and fix rules intersect (backp…

809af86

…ort of kumahq#12340)

1. When I went through the policy matching subset `Intersect` method, I
found the behavior is incorrect with one case
2. Rewrite the rule's `Compute` method to use the new `ContainsElement`
method instead of using the old `IsSubset` method

Introduce one new type `type Element map[string]string` for
differentiation by `IsSubset`.

<!-- Is there a MADR? An Issue? A related PR? -->

Fix kumahq#12319
Fix kumahq#12273

<!--
> Changelog: skip
-->
<!--
Uncomment the above section to explicitly set a [`> Changelog:` entry
here](https://github.com/kumahq/kuma/blob/master/CONTRIBUTING.md#submitting-a-patch)?
-->

---------

Signed-off-by: Icarus Wu <[email protected]>
Signed-off-by: Ilya Lobkov <[email protected]>
Co-authored-by: Ilya Lobkov <[email protected]>

Icarus9913 added a commit to Icarus9913/kuma that referenced this pull request


          fix(policy): use new compute for rules and fix rules intersect (backp…

b6a81c0

…ort of kumahq#12340)

1. When I went through the policy matching subset `Intersect` method, I
found the behavior is incorrect with one case
2. Rewrite the rule's `Compute` method to use the new `ContainsElement`
method instead of using the old `IsSubset` method

Introduce one new type `type Element map[string]string` for
differentiation by `IsSubset`.

<!-- Is there a MADR? An Issue? A related PR? -->

Fix kumahq#12319
Fix kumahq#12273

<!--
> Changelog: skip
-->
<!--
Uncomment the above section to explicitly set a [`> Changelog:` entry
here](https://github.com/kumahq/kuma/blob/master/CONTRIBUTING.md#submitting-a-patch)?
-->

---------

Signed-off-by: Icarus Wu <[email protected]>
Signed-off-by: Ilya Lobkov <[email protected]>
Co-authored-by: Ilya Lobkov <[email protected]>

Icarus9913 added a commit that referenced this pull request


          fix(policy): use new compute for rules and fix rules intersect (backp…

ee51964

…ort of #12340) (#12515)

## Motivation

1. When I went through the policy matching subset `Intersect` method, I
found the behavior is incorrect with one case
2. Rewrite the rule's `Compute` method to use the new `ContainsElement`
method instead of using the old `IsSubset` method

## Implementation information

Introduce one new type `type Element map[string]string` for
differentiation by `IsSubset`.

<!--
> Changelog: skip
-->
<!--
Uncomment the above section to explicitly set a [`> Changelog:` entry
here](https://github.com/kumahq/kuma/blob/master/CONTRIBUTING.md#submitting-a-patch)?
-->

## Backport

cherry-picked commit 3a00363

Signed-off-by: Icarus Wu <[email protected]>
Signed-off-by: Ilya Lobkov <[email protected]>
Co-authored-by: Ilya Lobkov <[email protected]>

Icarus9913 added a commit that referenced this pull request


          fix(policy): use new compute for rules and fix rules intersect (backp…

5f9f5fe

…ort of #12340) (#12516)

## Motivation

1. When I went through the policy matching subset `Intersect` method, I
found the behavior is incorrect with one case
2. Rewrite the rule's `Compute` method to use the new `ContainsElement`
method instead of using the old `IsSubset` method

## Implementation information

Introduce one new type `type Element map[string]string` for
differentiation by `IsSubset`.

<!--
> Changelog: skip
-->
<!--
Uncomment the above section to explicitly set a [`> Changelog:` entry
here](https://github.com/kumahq/kuma/blob/master/CONTRIBUTING.md#submitting-a-patch)?
-->

## Backport

cherry-picked commit 3a00363

---------

Signed-off-by: Icarus Wu <[email protected]>
Signed-off-by: Ilya Lobkov <[email protected]>
Co-authored-by: Ilya Lobkov <[email protected]>

Icarus9913 added a commit that referenced this pull request


          fix(policy): use new compute for rules and fix rules intersect (backp…

26385a0

…ort of #12340) (#12517)

## Motivation

1. When I went through the policy matching subset `Intersect` method, I
found the behavior is incorrect with one case
2. Rewrite the rule's `Compute` method to use the new `ContainsElement`
method instead of using the old `IsSubset` method

## Implementation information

Introduce one new type `type Element map[string]string` for
differentiation by `IsSubset`.

<!--
> Changelog: skip
-->
<!--
Uncomment the above section to explicitly set a [`> Changelog:` entry
here](https://github.com/kumahq/kuma/blob/master/CONTRIBUTING.md#submitting-a-patch)?
-->

## Backport

cherry-picked commit 3a00363

---------

Signed-off-by: Icarus Wu <[email protected]>
Signed-off-by: Ilya Lobkov <[email protected]>
Co-authored-by: Ilya Lobkov <[email protected]>

Icarus9913 added a commit that referenced this pull request


          fix(policy): use new compute for rules and fix rules intersect (backp…

9303b61

…ort of #12340) (#12518)

## Motivation

1. When I went through the policy matching subset `Intersect` method, I
found the behavior is incorrect with one case
2. Rewrite the rule's `Compute` method to use the new `ContainsElement`
method instead of using the old `IsSubset` method

## Implementation information

Introduce one new type `type Element map[string]string` for
differentiation by `IsSubset`.

<!--
> Changelog: skip
-->
<!--
Uncomment the above section to explicitly set a [`> Changelog:` entry
here](https://github.com/kumahq/kuma/blob/master/CONTRIBUTING.md#submitting-a-patch)?
-->

## Backport

cherry-picked commit 3a00363

---------

Signed-off-by: Icarus Wu <[email protected]>
Signed-off-by: Ilya Lobkov <[email protected]>
Co-authored-by: Ilya Lobkov <[email protected]>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

lahabana lahabana left review comments

lobkovilya lobkovilya approved these changes

jijiechen jijiechen approved these changes

slonka Awaiting requested review from slonka slonka is a code owner automatically assigned from kumahq/kuma-maintainers

jakubdyszkiewicz Awaiting requested review from jakubdyszkiewicz jakubdyszkiewicz is a code owner automatically assigned from kumahq/kuma-maintainers

Labels

backport ci/run-full-matrix