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

Restrict source namespaces in flagger-loadtester #1119

Merged
merged 2 commits into from
Mar 8, 2022
Merged

Restrict source namespaces in flagger-loadtester #1119

merged 2 commits into from
Mar 8, 2022

Conversation

connesc
Copy link
Contributor

@connesc connesc commented Feb 24, 2022

In a multi-tenant cluster, isolating namespaces using network policies quickly becomes a requirement.
In this context, mutualizing a Flagger instance (in a dedicated namespace) is challenging, because flagger-loadtester generates cross-namespace traffic.

A simple approach is to let each tenant deploy its custom flagger-loadtester in the same namespace as its target canaries. This way, the only cross-namespace traffic is between flagger and flagger-loadtester and can be easily whitelisted by network policies.

                    ┌──────────────────────────────────┐
                    │                                  │
                    │                           ┌────┐ │
┌───────────┐       │                    ┌─────►│app1│ │
│           │       │                    │      └────┘ │
│ ┌───────┐ │       │  ┌─────────────────┴┐            │
│ │flagger├─┼───────┼─►│flagger-loadtester│            │
│ └───────┘ │       │  └─────────────────┬┘            │
│           │       │                    │      ┌────┐ │
└───────────┘       │                    └─────►│app2│ │
 flagger ns         │                           └────┘ │
                    │                                  │
                    └──────────────────────────────────┘
                                tenant1 ns

However, nothing can prevent a Canary from tenant2 namespace to declare flagger-loadtester.tenant1 as a webhook, and thus to execute arbitrary commands in tenant1 namespace.
This creates a serious isolation issue.

This PR allows flagger-loadtester to whitelist namespaces from which canaries are allowed.
The proposed -namespace-regex flag looks like a simple way to address the problem, but feel free to challenge it 😉.

This allows to forbid access from canaries in non-whitelisted
namespaces.
In a multi-tenant context, this can be combined with network policies to
maintain isolation between namespaces.

Signed-off-by: Cédric Connes <cedric.connes@gmail.com>
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

@connesc can you please add this flag to the loadtester Helm chart and document there what it does?

@stefanprodan stefanprodan added the kind/enhancement Improvement request for an existing feature label Mar 8, 2022
Signed-off-by: Cédric Connes <cedric.connes@gmail.com>
@codecov-commenter
Copy link

Codecov Report

Merging #1119 (96fd359) into main (65e3bcb) will decrease coverage by 0.22%.
The diff coverage is 9.37%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1119      +/-   ##
==========================================
- Coverage   57.21%   56.99%   -0.23%     
==========================================
  Files          78       79       +1     
  Lines        6344     6376      +32     
==========================================
+ Hits         3630     3634       +4     
- Misses       2172     2199      +27     
- Partials      542      543       +1     
Impacted Files Coverage Δ
pkg/loadtester/server.go 11.66% <0.00%> (-1.29%) ⬇️
pkg/loadtester/authorizer.go 100.00% <100.00%> (ø)
pkg/controller/finalizer.go 37.66% <0.00%> (+0.82%) ⬆️

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 65e3bcb...96fd359. Read the comment docs.

Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @connesc 🏅

@stefanprodan stefanprodan merged commit 090329d into fluxcd:main Mar 8, 2022
@connesc
Copy link
Contributor Author

connesc commented Mar 8, 2022

Thanks for the review & merge @stefanprodan! 👍

@connesc connesc deleted the authorizer branch March 8, 2022 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Improvement request for an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants