Skip to content

Adds NetworkPolicies to all components of Kube-prometheus #1650

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

Merged
merged 7 commits into from
Apr 5, 2022
Merged

Adds NetworkPolicies to all components of Kube-prometheus #1650

merged 7 commits into from
Apr 5, 2022

Conversation

ArthurSens
Copy link
Member

@ArthurSens ArthurSens commented Feb 17, 2022

Description

This is continuing the work started at #1470.

It adds NetworkPolicies to all components of the stack:

  • Egress is allowed for all just for getting NetworkPolicies in quickly - We might want to revisit and be more restrict here
  • Ingress only allowed by Prometheus for all components, except Prometheus-adapter (HPAs need to send requests for metrics)

Extra changes to make sure we keep supporting NetworkPolicies without breaking anything:

  • CI was updated to disable default CNI and is installing Cilium to enable NetworkPolicies
  • Scripts used for development were also updated to enable NetworkPolicies with Cilium

Fixes #1596

Type of change

What type of changes does your code introduce to the kube-prometheus? Put an x in the box that apply.

  • CHANGE (fix or feature that would cause existing functionality to not work as expected)
  • FEATURE (non-breaking change which adds functionality)
  • BUGFIX (non-breaking change which fixes an issue)
  • ENHANCEMENT (non-breaking change which improves existing functionality)
  • NONE (if none of the other choices apply. Example, tooling, build system, CI, docs, etc.)

Changelog entry

Please put a one-line changelog entry below. Later this will be copied to the changelog file.

Add NetworkPolicies to follow security best-practices

@ArthurSens
Copy link
Member Author

Hmmm strange, this same setup worked when running locally 🤔
I'll try to take a look another day

@ArthurSens
Copy link
Member Author

ArthurSens commented Feb 23, 2022

I'm not really sure what is wrong here, testing locally works...

@prometheus-operator/kube-prometheus-reviewers any idea why e2e tests doesn't work with NetworkPolicies during CI?

@paulfantom
Copy link
Member

Does it work locally?

@ArthurSens
Copy link
Member Author

yes it does 🤔

@ArthurSens
Copy link
Member Author

Alright, after @simonpasquier's suggestions, I've created another branch that only adds Calico to the CI just to make sure that it wasn't the problem here.

Turns out that only by adding Calico, and not adding any network policy, our CI already fails. I'm not that knowledgeable about Kubernetes Networking, so I still haven't figured out why that breaks the CI.

@philipgough
Copy link
Contributor

@ArthurSens - I did take a look. Also no clue right now why it fails on CI with Calico installed. FWIW I checked out your branch and ran the tests locally on OS X and they passed too :/

ArthurSens and others added 7 commits March 9, 2022 07:42
(cherry picked from commit f8c00b9)
(cherry picked from commit f09b8e5)
(cherry picked from commit b4bf38b)
(cherry picked from commit c21bf4f)
…d prometheus

Signed-off-by: GitHub <[email protected]>
(cherry picked from commit 86e16b5)
Signed-off-by: GitHub <[email protected]>
(cherry picked from commit 233a8ac)
Signed-off-by: paulfantom <[email protected]>
Signed-off-by: Paweł Krupa (paulfantom) <[email protected]>
(cherry picked from commit d3ea314)
(cherry picked from commit d24c347)
@ArthurSens
Copy link
Member Author

OKEY! Replacing Calico with Cilium did the trick 🚀

I believe this PR is ready for review now :)

Copy link
Contributor

@philipgough philipgough left a comment

Choose a reason for hiding this comment

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

@ArthurSens - great job, looks good to me overall but I wonder if we should update the quickstart guide also?

@ArthurSens
Copy link
Member Author

ArthurSens commented Mar 11, 2022

@ArthurSens - great job, looks good to me overall but I wonder if we should update the quickstart guide also?

Good point! I'm not sure if that's needed though...

Network policies should be a no-op on Kubernetes without a plugin with support for it, so this should work out-of-the-box. I'm not entirely sure we want to add another step to the quickstart guide to enable this. I think quickstart should have the less amount of friction possible 🤔

image

@philipgough
Copy link
Contributor

@ArthurSens - thanks for the link, I wasn't aware of the no-op situation and in that case it make perfect sense to keep the quickstart as simple as possible.

All good my side then. Will wait for some eyes from @paulfantom /others

@ArthurSens
Copy link
Member Author

I'm merging since there were no oppositions so far 🙂

@ArthurSens ArthurSens merged commit 01004de into prometheus-operator:main Apr 5, 2022
@ArthurSens ArthurSens deleted the as/network-policies branch April 5, 2022 08:48
@ArthurSens ArthurSens mentioned this pull request Apr 5, 2022
5 tasks
kakkoyun added a commit that referenced this pull request Apr 5, 2022
@infernix infernix mentioned this pull request Apr 7, 2022
5 tasks
@werdnum
Copy link

werdnum commented Jul 15, 2022

FWIW, this seems to have completely broken my relatively ordinary setup on k3s (specifically, it broke my ingress rules) and I had to go digging to find out why. There doesn't seem to be a clear way to enable ingress (even to Grafana) without completely deleting the network policies. I'd hope you could add at least an escape hatch in 'values' to allow adding the ingress controller to the network policy.

@SISheogorath
Copy link

@werdnum you have to do the opposite: You have to create a NetworkPolicy that allows access from your ingress (controller) Pods to the Grafana Pod, deleting all NetworkPolicies removes any access restrictions for this namespace.

@werdnum
Copy link

werdnum commented Jul 16, 2022

@SISheogorath:

@werdnum you have to do the opposite: You have to create a NetworkPolicy that allows access from your ingress (controller) Pods to the Grafana Pod, deleting all NetworkPolicies removes any access restrictions for this namespace.

Is there some kind of affordance in the templates to do so? Ideally there's something in values that allows me to put in a PodSelector value that matches the ingress controller, instead of me having to learn what a NetworkPolicy is.

Seems like this should have been marked as a breaking change.

@roelandvanbatenburg
Copy link
Contributor

@SISheogorath:

@werdnum you have to do the opposite: You have to create a NetworkPolicy that allows access from your ingress (controller) Pods to the Grafana Pod, deleting all NetworkPolicies removes any access restrictions for this namespace.

Is there some kind of affordance in the templates to do so? Ideally there's something in values that allows me to put in a PodSelector value that matches the ingress controller, instead of me having to learn what a NetworkPolicy is.

Seems like this should have been marked as a breaking change.

I did some digging and provided an example of how to modify the NetworkPolicies in #1960. Not quite what you were asking for, but I have little experience with Helm.

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.

Investigate disabling ingress and egress traffic
6 participants