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

use a NetworkPolicy instead of kube-rbac-proxy #1885

Open
estroz opened this issue Dec 8, 2020 · 25 comments · May be fixed by #3853
Open

use a NetworkPolicy instead of kube-rbac-proxy #1885

estroz opened this issue Dec 8, 2020 · 25 comments · May be fixed by #3853
Assignees
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature Categorizes issue or PR as related to a new feature. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Milestone

Comments

@estroz
Copy link
Contributor

estroz commented Dec 8, 2020

kube-rbac-proxy, while a nice feature, is a third-party workaround for a problem (restricting pod access to /metrics) that can be solved by a NetworkPolicy. For example, the following policy could be scaffolded by default so only manager-related pods could be selected:

apiVersion: networking.k8s.io/v1
kind: NetworkPolicy
metadata:
  name: network-policy
  namespace: system
spec:
  podSelector:
    matchLabels:
      control-plane: controller-manager
  policyTypes:
  - Ingress
  ingress:
  - from:
    - namespaceSelector:
        matchLabels:
          control-plane: controller-manager
    - podSelector:
        matchLabels:
          control-plane: controller-manager

Benefits:

  1. k8s-native resource.
  2. Reduces the number of external dependencies.
  3. Easy to use and modify.

/kind feature

@estroz estroz added the kind/feature Categorizes issue or PR as related to a new feature. label Dec 8, 2020
@camilamacedo86
Copy link
Member

I loved 👍 The only possible con here is check how if it would work nicely on other vendors such as OCP. Did you check that already?

@estroz
Copy link
Contributor Author

estroz commented Dec 8, 2020

NetworkPolicy has been around for a long time, so all full-featured k8s distros support it.

@camilamacedo86
Copy link
Member

camilamacedo86 commented Dec 9, 2020

Hi @estroz,

I raised this concern because in the past I remember some problems to work with Ingress on OCP. Regards OCP, I could check that this API networking.k8s.io/v1 is supported since 4.4 only : https://docs.openshift.com/container-platform/4.6/rest_api/network_apis/networkpolicy-networking-k8s-io-v1.html.

So, since Kubebuilder as SDK needs to support previous k8s cluster versions we would also need to check if we would need to allow scaffold it by using networking.k8s.io/v1beta1 as well

@prafull01
Copy link
Contributor

prafull01 commented Dec 9, 2020

Hi @camilamacedo86 @estroz

Network policies are implemented by CNI(container Network Interface) in the cluster. It means the users which are deploying kubebuilder's operator on such cluster where CNI doesn't support the Network Policy (For example: Flannel) this restriction policy will not work there. This con also needs to be considered while implementing this.

@estroz
Copy link
Contributor Author

estroz commented Dec 9, 2020

t means the clusters which are deploying kubebuilder's operator on such cluster where CNI doesn't support the Network Policy (For example: Flannel) this restriction policy will not work there

Right, and this potentially makes NetworkPolicy scaffolding (by default at least) a non-starter since it locks projects out of certain CNI's.

@camilamacedo86 camilamacedo86 added the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Dec 15, 2020
@fejta-bot

This comment was marked as outdated.

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 15, 2021
@Adirio

This comment was marked as outdated.

@k8s-ci-robot k8s-ci-robot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Mar 15, 2021
@camilamacedo86
Copy link
Member

Also, regards the motivations

kube-rbac-proxy, while a nice feature, is a third-party workaround for a problem (restricting pod access to /metrics)

Then, see the motivation for this project: https://github.com/brancz/kube-rbac-proxy/#motivation

I developed this proxy in order to be able to protect Prometheus metrics endpoints. In a scenario, where an attacker might obtain full control over a Pod, that attacker would have the ability to discover a lot of information about the workload as well as the current load of the respective workload. This information could originate for example from the node-exporter and kube-state-metrics. Both of those metric sources can commonly be found in Prometheus monitoring stacks on Kubernetes.
This project was created to specifically solve the above problem, however, I felt there is a larger need for such a proxy in general.

Then, see that the auth-proxy was built to:

In Kubernetes clusters without NetworkPolicies any Pod can perform requests to every other Pod in the cluster. This proxy was developed in order to restrict requests to only those Pods, that present a valid and RBAC authorized token or client TLS certificate.

However, I also align with the pros of using NetworkPolicy (remove thrid-party and reduce the deps). But we might want to better analyse and discuss this one.

@camilamacedo86

This comment was marked as resolved.

@camilamacedo86 camilamacedo86 added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. and removed lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. triage/accepted Indicates an issue or PR is ready to be actively worked on. labels Sep 11, 2022
@yashsingh74

This comment was marked as outdated.

@varshaprasad96

This comment was marked as outdated.

@yashsingh74 yashsingh74 removed their assignment Jan 9, 2023
@camilamacedo86 camilamacedo86 added this to the go/v5 milestone Feb 11, 2023
@camilamacedo86
Copy link
Member

camilamacedo86 commented May 1, 2023

Good argumentations to move forward within this one:

  • See those that cannot pull the image: Is it possible to remove kube-rbac-proxy #3337
  • Remove dependency in a project that is not donated to Kubernetes-sig (It has been in the process of donation for years but still not donated)
  • Less burn to maintain the project since we would no longer need to build those images.

More info: #1885 (comment)

@joejulian
Copy link

joejulian commented Aug 2, 2023

Another good reason, and a potential deadline, gcr.io container-registry is deprecated and is scheduled to be moved to Artifact Registry on May 15, 2024.

https://cloud.google.com/container-registry/docs/deprecations/container-registry-deprecation

@varshaprasad96
Copy link
Member

Since we have enough positive responses to move forward with this approach, I'm going to add this to the agenda for our next community meeting. Meanwhile, if anyone would like to take this up and contribute, please feel free to assign yourself.

@mjlshen
Copy link
Contributor

mjlshen commented Aug 2, 2023

I'm interested!
/assign mjlshen

@joejulian
Copy link

On the other hand, AWS VPC CNI does't support NetworkPolicy. There are a lot of kubernetes users that use EKS and have no choice but to use that CNI driver.

@fgiloux
Copy link
Contributor

fgiloux commented Aug 3, 2023

Sorry to bring some negative perspectives but to me NetworkPolicy and kube-rbac-proxy, although both security related, do not address the same use case:

  • NetworkPolicy is for controlling network traffic at level 3 or 4
  • kube-rbac-proxy is for controlling access through authentication and authorization

In practice it means that you can control with kube-rbac-proxy, which user or serviceaccount has access whereas you can control with networkpolicy, which network segment has access. Both may be useful but not interchangeable.

@mjlshen
Copy link
Contributor

mjlshen commented Aug 7, 2023

Thanks for bring that up, after some reading, I agree. Is it worth providing or documenting a NetworkPolicy with information similar to the above or do we think that we shouldn't pursue this issue any further?

@varshaprasad96
Copy link
Member

The other option as @camilamacedo86 mentioned in the community meeting is to build a plugin that adds the scaffolding to this to the main gaoling plugin. This will also reduce the burden of us not building the kube-rbac-proxy image again (since its not a part of k8s project: https://github.com/brancz/kube-rbac-proxy), as we could accept a user provided image to be added to the project.

@mjlshen
Copy link
Contributor

mjlshen commented Aug 30, 2023

We can also consider using this feature from controller-runtime 0.16.0 kubernetes-sigs/controller-runtime#2407 which I think perfectly fits our use-case of locking down the metrics endpoint with K8s RBAC without kube-rbac-proxy

@mjlshen
Copy link
Contributor

mjlshen commented Aug 31, 2023

I do prefer this approach and will start work in this direction:

We can also consider using this feature from controller-runtime 0.16.0 kubernetes-sigs/controller-runtime#2407 which I think perfectly fits our use-case of locking down the metrics endpoint with K8s RBAC without kube-rbac-proxy

but with regards to this:

On the other hand, AWS VPC CNI does't support NetworkPolicy. There are a lot of kubernetes users that use EKS and have no choice but to use that CNI driver.

I noticed that AWS VPC CNI now supports NetworkPolicy! https://aws.amazon.com/blogs/containers/amazon-vpc-cni-now-supports-kubernetes-network-policies/

@camilamacedo86
Copy link
Member

camilamacedo86 commented Nov 14, 2023

Hi @mjlshen

Your help is very required if you are interested in this one.
We really need to check this option.
In order to make easier what are the expected steps here (IHMO) and what we need to do next I added the bellow comment, see: #1885 (comment)

If you be unable to help us out, it will be sad but please remove the assign to you so maybe others can try to check this one.

Thank you a lot for all your help. 🥇

@camilamacedo86
Copy link
Member

camilamacedo86 commented Nov 14, 2023

Next Step: Evaluate NetworkPolicy Integration

Expected Outcome:

The outcome of this task is to make an informed decision regarding the adoption of NetworkPolicy in Kubebuilder. By evaluating its pros and cons and conducting testing, we aim to determine whether this change will enhance the project's functionality and align with community preferences.

Let's work together to evaluate this potential improvement and make an informed choice for the future of Kubebuilder.

Goals:

  1. Assess using NetworkPolicy instead of rbac-proxy in Kubebuilder.
  2. Test NetworkPolicy implementation via a PR with default scaffold changes.
  3. Share PR results in Kubebuilder channel for discussion.
  4. Evaluate the raised option:

We can also consider using this feature from controller-runtime 0.16.0 kubernetes-sigs/controller-runtime#2407 which I think perfectly fits our use-case of locking down the metrics endpoint with K8s RBAC without kube-rbac-proxy

We need to create such as markdown table factor | using NetwrokPolices | using auth-proxy | using Controller Runtime | . So that we can evaluate the 3 options raised.

Steps:
a) Evaluate Pros and Cons:

Review comments to capture NetworkPolicy's benefits and drawbacks.
Note that the argument against NetworkPolicy (CNI support) may no longer apply.

b) Create PR with Scaffold Changes:

  • Modify default scaffold to use NetworkPolicy.
  • Thoroughly test in e2e scenarios.
  • Summarize pros and cons in PR comment, linking it in the description. Ideally add a comment here in the issue and link it in the description so that we can easily check the summary with pros vs cons that you find out.

c) Community Discussion:

Share PR results in Kubebuilder channel so that we can collect feedback and insights as decide whether to merge the PR or retain rbac-proxy.

Your help here is very appreciated by the whole community 🥇

@camilamacedo86 camilamacedo86 added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Nov 14, 2023
@joejulian
Copy link

Note that the argument against NetworkPolicy (CNI support) may no longer apply

Is supporting NetworkPolicy a requirement for CNI, or is it just an option? If it's optional, should we be requiring it?

@camilamacedo86
Copy link
Member

camilamacedo86 commented Dec 5, 2023

Hi @mjlshen,

You wrote above:

We can also consider using this feature from controller-runtime 0.16.0 kubernetes-sigs/controller-runtime#2407 which I think perfectly fits our use-case of locking down the metrics endpoint with K8s RBAC without kube-rbac-proxy

That might be an option.
We need to create a proposal that compares the pros and cons of the three options.
Then, if whoever is checking it thinks that option X is better, then a PR with the changes to the default scaffold with the respective Option. That will prove that it works and show the required changes to be performed.

Regards controller runtime

  • how it will work
  • what are the limitations
  • will that do the same that we do with NetwrokPolicy or auth-proxy?
  • Is it required to use cert-manager by default? Is it required to pass the cert to use this feature?

See: #1885 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature Categorizes issue or PR as related to a new feature. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging a pull request may close this issue.