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

Isolate private ksvc using different envoy listener ports #852

Merged
merged 23 commits into from
Jul 11, 2022

Conversation

lionelvillard
Copy link
Contributor

@lionelvillard lionelvillard commented Jun 1, 2022

Add support for multiple private ingresses backed by multiple envoy listeners (each with a unique port).

This is an experimental Kourier-only feature guarded by a flag in config-kourier:

# Control the desired level of incoming traffic isolation.
#
# When set to the empty value (default), all incoming traffic flows through
# a shared ingress and listeners.
#
# When set to "port", incoming traffic is isolated by using different
# listener ports.
#
# NOTE: This flag is in an alpha state.
traffic-isolation: ""

The selection of which envoy listener to use is done at the namespace-level. The traffic of all internal services within the same namespace is redirected to the same envoy listener. Here are the annotation for mapping a namespace to a particular envoy listener:

// ListenerPortAnnotationKey is the annotation key for assigning the ingress to a particular
// envoy listener port. Only applicable to internal services.
ListenerPortAnnotationKey = "kourier.knative.dev/listener-port"

The allocation/assignment of a namespace to an envoy listener is an orthogonal concern and not address in this PR. This can be done by manually editing namespace annotations or automatically by a Tenant controller.

Each envoy listener is exposed using a k8s service (e.g kourier-isolation-9000). This PR does not automatically create/delete these services since this concern is also related to tenant management.

Additional context:

  • Feature Track
  • PoC showing how Network Policies with this PR can be used to isolate tenants.

Changes

  • Add a new configuration parameter for enabling port-level internal services isolation
  • Generate an envoy configuration containing multiple internal services

/kind enhancement

Fixes #

Release Note

Add a new configuration parameter for enabling port-level internal services isolation. See documentation for more details.

Docs


Remaining tasks are tracked in this issue.

/cc @evankanderson @nak3 @rhuss

@knative-prow knative-prow bot requested review from evankanderson and nak3 June 1, 2022 19:35
@knative-prow knative-prow bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/enhancement labels Jun 1, 2022
@knative-prow knative-prow bot requested a review from rhuss June 1, 2022 19:35
@knative-prow knative-prow bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jun 1, 2022
@codecov
Copy link

codecov bot commented Jun 6, 2022

Codecov Report

Merging #852 (8791e3f) into main (34458cb) will decrease coverage by 0.82%.
The diff coverage is 62.74%.

@@            Coverage Diff             @@
##             main     #852      +/-   ##
==========================================
- Coverage   81.60%   80.77%   -0.83%     
==========================================
  Files          18       18              
  Lines        1185     1233      +48     
==========================================
+ Hits          967      996      +29     
- Misses        174      190      +16     
- Partials       44       47       +3     
Impacted Files Coverage Δ
pkg/config/config.go 0.00% <0.00%> (ø)
pkg/generator/ingress_translator.go 82.53% <26.66%> (-3.03%) ⬇️
pkg/generator/caches.go 72.13% <81.25%> (+0.89%) ⬆️
pkg/config/configmap.go 100.00% <100.00%> (ø)

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 34458cb...8791e3f. Read the comment docs.

@knative-prow knative-prow bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 6, 2022
@knative-prow knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 6, 2022
@lionelvillard lionelvillard force-pushed the mt-ingresses branch 2 times, most recently from 1d9aa7e to 20795c3 Compare June 8, 2022 21:04
@skonto
Copy link
Contributor

skonto commented Jun 9, 2022

@lionelvillard hi, could you add a link to the ft doc in the description?

@lionelvillard
Copy link
Contributor Author

@lionelvillard hi, could you add a link to the ft doc in the description?

done.

@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 16, 2022
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 20, 2022
@lionelvillard lionelvillard changed the title WIP: isolate private ksvc using different envoy listener ports Isolate private ksvc using different envoy listener ports Jun 20, 2022
@knative-prow knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 20, 2022
@lionelvillard
Copy link
Contributor Author

@skonto can you take a look at this PR? I'm trying to see why the upgrade tests are failing. The log is not very helpful and these tests cannot run on a Mac.

@lionelvillard
Copy link
Contributor Author

/hold

@knative-prow knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 20, 2022
@lionelvillard lionelvillard force-pushed the mt-ingresses branch 2 times, most recently from 150e2f5 to 4a99326 Compare June 20, 2022 20:43
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 11, 2022
@lionelvillard
Copy link
Contributor Author

@skonto

Can we run knative/serving#13094 also with the feature enabled?

Yes. knative/serving#13112

We need to have an e2e test somewhere, could you add one here at least.

I'm looking at this now, it seems easy enough but due to the time constraint, I would rather do this in a separate PR.

IMO, as soon as both knative/serving#13094 and knative/serving#13112 are green, it should be safe to merge this PR.

Also is there a plan for the tenant controller here?

No plans. This is a much bigger topic!

Let's create a list of the remaining tasks pls (tracking issue). In this description of this issue all tasks seem done. Let's add the tenant controller idea and the webhook validation too. Also since we plan to iterate on this let's add the question about label vs annotation there. @lionelvillard could you do this before merging?

Yes I can create an Github issue tracking all remaining tasks

@lionelvillard
Copy link
Contributor Author

/hold I need to verify the PoC is still working.

@knative-prow knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 11, 2022
@lionelvillard
Copy link
Contributor Author

/unhold PoC is working.

@lionelvillard
Copy link
Contributor Author

@evankanderson @skonto @nak3 @rhuss Please can you take another look? Since the last round of reviews I:

  • removed the listener label. The Kourier services exposing the envoy listeners are now called kourier-isolation-<listener-port>
  • remove "none" in the configmap in favor of the empty string value
  • opened various issues to track the remaining tasks

I'm tracking knative/serving#13094 and knative/serving#13112. So far it looks good.

I'm working on e2e tests (in a separate PR). I don't know if I will have time to finish before the 1.6 release (which is tomorrow).

Copy link
Contributor

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

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

A few notes for the next PR, but happy to get this in for 1.6.

/lgtm
/approve

pkg/config/configmap.go Outdated Show resolved Hide resolved
pkg/config/configmap_test.go Outdated Show resolved Hide resolved
pkg/generator/caches.go Outdated Show resolved Hide resolved
pkg/reconciler/ingress/ingress.go Show resolved Hide resolved
@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jul 11, 2022
@knative-prow
Copy link

knative-prow bot commented Jul 11, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson, lionelvillard

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow knative-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 11, 2022
@lionelvillard
Copy link
Contributor Author

/unhold

@knative-prow knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 11, 2022
@lionelvillard
Copy link
Contributor Author

@evankanderson can you please TAL? All tests are green (including the Serving ones).

@evankanderson
Copy link
Contributor

/lgtm

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jul 11, 2022
@knative-prow knative-prow bot merged commit 2b580bb into knative-extensions:main Jul 11, 2022
ReToCode added a commit to ReToCode/net-kourier that referenced this pull request Aug 30, 2023
knative-prow bot pushed a commit that referenced this pull request Sep 1, 2023
knative-automation added a commit to knative-automation/serving that referenced this pull request Sep 1, 2023
bumping knative.dev/net-kourier 0d68ef5...9b26dcb:
  > 9b26dcb Revert changes from knative-extensions/net-kourier#852 (# 1099)
  > d696408 Update community files (# 1101)
  > 36c41d7 upgrade to latest dependencies (# 1100)

Signed-off-by: Knative Automation <[email protected]>
knative-prow bot pushed a commit to knative/serving that referenced this pull request Sep 4, 2023
bumping knative.dev/net-kourier 0d68ef5...9b26dcb:
  > 9b26dcb Revert changes from knative-extensions/net-kourier#852 (# 1099)
  > d696408 Update community files (# 1101)
  > 36c41d7 upgrade to latest dependencies (# 1100)

Signed-off-by: Knative Automation <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/enhancement lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants