Skip to content
This repository has been archived by the owner on Jul 11, 2023. It is now read-only.

Introducing per-service filtering #1725

Merged
merged 9 commits into from
Sep 25, 2020

Conversation

eduser25
Copy link
Contributor

@eduser25 eduser25 commented Sep 18, 2020

Introduces per-destination filter-chain matching on outbound

This change will allow setting specific network filtering on per service
destination, which enables finer configuration per-service level,
as well as the use of different network filters to allow not only L7 traffic
through.

Additional benefits:

  • Since we are filtering all permitted traffic, we can generalize and filter
    any remaining traffic as Egress (when enabled), which simplifies Egress
    to not need a CIDR anymore. (TODO: cleanup CIDR flags/code)
  • Additional work that might benefit from it: per-service route table
    on RDS, TCP routing, ....

Additionally:

  • Fixing the listener tests required adding the long-awaited catalog mock.
    Will add more tests in subsequent commits.

  • New Functionality [X]

  • Control Plane [X]

  • Networking [X]

  • SMI Policy [X]

  • Does this change contain code from or inspired by another project? If so, did you notify the maintainers and provide attribution?
    No

@eduser25 eduser25 requested a review from a team as a code owner September 18, 2020 00:02
pkg/envoy/cds/response.go Outdated Show resolved Hide resolved
pkg/envoy/lds/listener.go Outdated Show resolved Hide resolved
pkg/envoy/lds/listener.go Outdated Show resolved Hide resolved
pkg/envoy/cds/response.go Outdated Show resolved Hide resolved
pkg/envoy/cds/response.go Outdated Show resolved Hide resolved
pkg/envoy/lds/listener.go Outdated Show resolved Hide resolved
@eduser25
Copy link
Contributor Author

Will also address setting Permissive to TCP proxy, we do have some drawbacks to take care of.

@eduser25 eduser25 added the wip Work-in-Progress label Sep 18, 2020
@eduser25 eduser25 marked this pull request as draft September 18, 2020 00:14
@eduser25
Copy link
Contributor Author

Fixes for CI/demo expected values will need to be addressed

@codecov-commenter
Copy link

codecov-commenter commented Sep 18, 2020

Codecov Report

Merging #1725 into main will decrease coverage by 3.87%.
The diff coverage is 10.43%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1725      +/-   ##
==========================================
- Coverage   57.28%   53.40%   -3.88%     
==========================================
  Files         118      124       +6     
  Lines        5436     5065     -371     
==========================================
- Hits         3114     2705     -409     
- Misses       2319     2357      +38     
  Partials        3        3              
Impacted Files Coverage Δ
pkg/catalog/mock_catalog.go 0.00% <0.00%> (ø)
pkg/envoy/lds/response.go 0.00% <0.00%> (ø)
pkg/envoy/lds/listener.go 39.70% <28.78%> (-50.82%) ⬇️
pkg/health/health.go 20.51% <0.00%> (-11.41%) ⬇️
pkg/debugger/index.go 20.00% <0.00%> (-8.58%) ⬇️
pkg/debugger/policy.go 53.84% <0.00%> (-6.16%) ⬇️
pkg/catalog/service.go 72.72% <0.00%> (-5.85%) ⬇️
cmd/cli/util.go 53.33% <0.00%> (-5.50%) ⬇️
pkg/catalog/repeater.go 80.00% <0.00%> (-5.19%) ⬇️
... and 117 more

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 9104dbb...a6c478e. Read the comment docs.

Copy link
Member

@shashankram shashankram left a comment

Choose a reason for hiding this comment

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

Looks good overall. I'll pull this change and test it.

pkg/envoy/lds/listener.go Outdated Show resolved Hide resolved
pkg/envoy/lds/listener.go Outdated Show resolved Hide resolved
pkg/envoy/lds/listener.go Outdated Show resolved Hide resolved
pkg/envoy/lds/listener.go Outdated Show resolved Hide resolved
pkg/envoy/lds/listener.go Outdated Show resolved Hide resolved
pkg/envoy/lds/listener.go Show resolved Hide resolved
return nil, err
}

// Transform into set, when listing apex services we might face repetitions
Copy link
Member

Choose a reason for hiding this comment

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

Maybe use a mapset instead.

Copy link
Contributor Author

@eduser25 eduser25 Sep 21, 2020

Choose a reason for hiding this comment

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

mapset is a map underneath, I imagine you want it for readability at this point; I personally dislike adding deps if not strictly needed - more so if those are not golang official packages

Copy link
Member

Choose a reason for hiding this comment

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

for readability and helpers it provides. We already use it across the code.

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 know, and I never liked it. Want me to change it here though? I'm ok with either in any case

Copy link
Member

Choose a reason for hiding this comment

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

not necessarily for this PR but in a follow-up would be good.

pkg/envoy/lds/listener.go Show resolved Hide resolved
@eduser25 eduser25 removed the wip Work-in-Progress label Sep 21, 2020
@eduser25 eduser25 marked this pull request as ready for review September 21, 2020 22:56
snehachhabria
snehachhabria previously approved these changes Sep 25, 2020
shashankram
shashankram previously approved these changes Sep 25, 2020
Introduces per-destination filter-chain matching on outbound.
This change will allow setting specific L4 or L7 filtering, precursor
for TCP routing.

- Since we are filtering all permitted traffic, we can generalize the
remaining traffic and simplify Egress, which will not require a CIDR
anymore. (TODO: cleanup CIDR flags/code)
- Since we can match all destination traffic, Permissive mode can now
use TCP proxy (instead of wildcarded RDS) to allow also L4 protocols
between services.
- Additional work that might benefit from it: per-service route table
on RDS, TCP routing, ....

Additionally:
- Fixing the listener tests required adding the long-awaited catalog mock.
Will add more tests in subsequent commits.
eduser25 and others added 2 commits September 25, 2020 14:07
Co-authored-by: Sneha Chhabria <snchh@microsoft.com>
@eduser25 eduser25 merged commit dac3552 into openservicemesh:main Sep 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants