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

docs(*): new policy matching proposal #4474

Merged
merged 18 commits into from
Jul 13, 2022
Merged

Conversation

lobkovilya
Copy link
Contributor

Summary

Rendered version to read

Full changelog

  • add a new policy matching proposal

Issues resolved

Updates #3330

Documentation

Testing

  • Unit tests
  • E2E tests
  • Manual testing on Universal
  • Manual testing on Kubernetes

Backwards compatibility

  • Update UPGRADE.md with any steps users will need to take when upgrading.
  • Add backport-to-stable label if the code follows our backporting policy

Signed-off-by: Ilya Lobkov <[email protected]>
@lobkovilya lobkovilya requested a review from a team as a code owner June 14, 2022 13:07
Copy link
Contributor

@lahabana lahabana left a comment

Choose a reason for hiding this comment

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

Great start! I've left a few comments and I think we're missing:

  • Showing an example of a policy that's neither inbound nor outbound (did we have one in the end? If not thinking about one to prove our system works would be good).
  • Showing an example of a policy that's both inbound and outbound (trafficLog I believe is one of them)
  • Schema of TargetRef
  • Mention how we're going to roll this out (new policies that will replace existing policies)
  • Explain that follow MADR will come to describe each of the new policy
  • Explain that the top targetRef always identifies the set of DPP that is being modified

docs/madr/decisions/005-policy-matching.md Outdated Show resolved Hide resolved
* ProxyGroup
* Proxy
* MeshGatewayRoute
* HTTPRoute
Copy link
Contributor

Choose a reason for hiding this comment

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

Also didn't we talked about a ServiceGroup or ServiceSubset which is like a ProxyGroup except that kuma.io/service is required?

* ProxyGroup
* Proxy
* MeshGatewayRoute
* HTTPRoute
Copy link
Contributor

Choose a reason for hiding this comment

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

Also didn't we talked about a ServiceGroup or ServiceSubset which is like a ProxyGroup except that kuma.io/service is required?

docs/madr/decisions/005-policy-matching.md Outdated Show resolved Hide resolved
docs/madr/decisions/005-policy-matching.md Outdated Show resolved Hide resolved
docs/madr/decisions/005-policy-matching.md Show resolved Hide resolved
docs/madr/decisions/005-policy-matching.md Show resolved Hide resolved
docs/madr/decisions/005-policy-matching.md Show resolved Hide resolved
docs/madr/decisions/005-policy-matching.md Show resolved Hide resolved
docs/madr/decisions/005-policy-matching.md Show resolved Hide resolved
@lobkovilya
Copy link
Contributor Author

Also didn't we talked about a ServiceGroup or ServiceSubset which is like a ProxyGroup except that kuma.io/service is required?

Do you have an example of a policy that supports ServiceSubset but doesn't support ProxyGroup (or vice versa)?

@lahabana
Copy link
Contributor

Do you have an example of a policy that supports ServiceSubset but doesn't support ProxyGroup (or vice versa)?

  1. TrafficLog you might want to use the top TargetRef to just match any DP with a tag debug-log: enabled for example that's where a ProxyGroup without a service makes sense.

ServiceSubset is to make it clear that this can't be matched across services.

So:

kind: ServiceSubset
service: foo
tags:
  tag1: bar

I imagine things like HTTPRoute would want build a subset of a service (the slow rollout case with a version) That would need to be a ServiceSubset and not a ProxyGroup).

  1. Related but not exactly the same what replaces our current service: *? Like I believe we could have: service: *, tag1: foo which would create subgroups but based on the same service.

@codecov-commenter
Copy link

codecov-commenter commented Jun 16, 2022

Codecov Report

Merging #4474 (2f15d8b) into master (f4b94a0) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4474      +/-   ##
==========================================
+ Coverage   55.40%   55.42%   +0.01%     
==========================================
  Files         947      947              
  Lines       58041    58041              
==========================================
+ Hits        32157    32167      +10     
+ Misses      23328    23327       -1     
+ Partials     2556     2547       -9     
Impacted Files Coverage Δ
pkg/plugins/common/postgres/listener.go 53.84% <0.00%> (-15.39%) ⬇️
pkg/plugins/resources/postgres/events/listener.go 16.66% <0.00%> (-14.59%) ⬇️
pkg/xds/generator/direct_access_proxy_generator.go 89.65% <0.00%> (-1.15%) ⬇️
pkg/defaults/components.go 85.71% <0.00%> (ø)
pkg/plugins/resources/postgres/store.go 76.37% <0.00%> (+1.68%) ⬆️
pkg/core/resources/manager/cache.go 88.31% <0.00%> (+2.59%) ⬆️
pkg/core/runtime/component/component.go 93.54% <0.00%> (+9.67%) ⬆️
pkg/metrics/store/counter.go 85.45% <0.00%> (+10.90%) ⬆️
pkg/clusterid/creator.go 86.66% <0.00%> (+13.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f4b94a0...2f15d8b. Read the comment docs.

@lobkovilya
Copy link
Contributor Author

@lahabana I renamed ProxyGroup to ServiceSubset and added a new MeshSubset level, so now it looks like:

  • Mesh
  • MeshSubset
  • Service
  • ServiceSubset
  • Proxy
  • MeshGatewayRoute
  • HTTPRoute

Related but not exactly the same what replaces our current service: *?

yes, so now if you want service: * you'll create:

targetRef:
  kind: MeshSubset
  tags:
    key1: value1

@lobkovilya
Copy link
Contributor Author

lobkovilya commented Jun 27, 2022

todo:

  • Showing an example of a policy that's neither inbound nor outbound (did we have one in the end? If not thinking about one to prove our system works would be good).
  • Showing an example of a policy that's both inbound and outbound (trafficLog I believe is one of them)
  • Schema of TargetRef
  • Mention how we're going to roll this out (new policies that will replace existing policies)
  • Explain that follow MADR will come to describe each of the new policy
  • Explain that the top targetRef always identifies the set of DPP that is being modified

Copy link
Contributor

@lahabana lahabana left a comment

Choose a reason for hiding this comment

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

I don't see anywhere an explanation that states that "to" and "from" will have different valid subsets of "targetRef.kind"

docs/madr/decisions/005-policy-matching.md Show resolved Hide resolved
docs/madr/decisions/005-policy-matching.md Show resolved Hide resolved
docs/madr/decisions/005-policy-matching.md Show resolved Hide resolved
docs/madr/decisions/005-policy-matching.md Show resolved Hide resolved
@lahabana
Copy link
Contributor

lahabana commented Jun 27, 2022

Related but not exactly the same what replaces our current service: *?

So service subset will ALWAYS require a serviceName to be set?

Also MeshSubset will never be usable in to and from in the end no? Sounds like you can only pick subsets of a service in to and from.
Should we therefore disallow using ServiceSubset in top level? I'm unsure but it might be simpler to users

@lobkovilya
Copy link
Contributor Author

I don't see anywhere an explanation that states that "to" and "from" will have different valid subsets of "targetRef.kind"

That's down to policies, in usecases you can find allowed values for each policy

@lobkovilya
Copy link
Contributor Author

So service subset will ALWAYS require a serviceName to be set?

Yes, I think we can avoid having * at all, if you don't care about the service name you should use MeshSubset.

Also MeshSubset will never be usable in to and from in the end no? Sounds like you can only pick subsets of a service in to and from.

You can use MeshSubset in from array. In the to array you can use neither MeshSubset nor ServiceSubset.

Should we therefore disallow using ServiceSubset in top level? I'm unsure but it might be simpler to users

I'm not sure if more exceptions make it simpler. I'd allow both MeshSubset and ServiceSubset in top level, I think it's clear that the latter is more specific

@lobkovilya
Copy link
Contributor Author

lobkovilya commented Jun 28, 2022

Awaiting for decision:

  1. When it comes to conflict resolution, do we want to follow Gateway API? link
    answer: yes/no
  2. Do we want to rename kind: Service to MeshService, KumaService, etc? link
    answer: yes rename to MeshService
  3. When merging items inside to/from arrays do we want to change the order and have more specific items at the bottom? link
    answer: yes

Copy link
Contributor

@michaelbeaumont michaelbeaumont left a comment

Choose a reason for hiding this comment

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

Two comments about PolicyAttachment compliance

docs/madr/decisions/005-policy-matching.md Outdated Show resolved Hide resolved
docs/madr/decisions/005-policy-matching.md Outdated Show resolved Hide resolved
Copy link
Contributor

@bartsmykla bartsmykla left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@slonka slonka left a comment

Choose a reason for hiding this comment

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

some nitpicks and questions, nice work overall 👍

docs/madr/decisions/005-policy-matching.md Show resolved Hide resolved
docs/madr/decisions/005-policy-matching.md Outdated Show resolved Hide resolved
docs/madr/decisions/005-policy-matching.md Outdated Show resolved Hide resolved
docs/madr/decisions/005-policy-matching.md Show resolved Hide resolved
Co-authored-by: Krzysztof Słonka <[email protected]>

Signed-off-by: Ilya Lobkov <[email protected]>
Signed-off-by: Ilya Lobkov <[email protected]>
@lobkovilya lobkovilya merged commit eabf470 into master Jul 13, 2022
@lobkovilya lobkovilya deleted the docs/target-ref-proposal branch July 13, 2022 09:15
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.

9 participants