-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
⚠️ Discontinue Kube RBAC Proxy in Default Kubebuilder Scaffolding #3899
⚠️ Discontinue Kube RBAC Proxy in Default Kubebuilder Scaffolding #3899
Conversation
aa27c06
to
93a7dbf
Compare
- path: manager_auth_proxy_patch.yaml | ||
# [METRICS] The following patch will enable the metrics endpoint. Ensure that you also protect this endpoint. | ||
# If you want to expose the metric endpoint of your controller-manager uncomment the following line. | ||
#- path: manager_metrics_patch.yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Metrics should not be enable by default as it was before.
docs/book/src/getting-started/testdata/project/config/prometheus/monitor.yaml
Outdated
Show resolved
Hide resolved
92b585f
to
00f5ecf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a quick scan, and I think there is more to be cleaned up.
...ook/src/component-config-tutorial/testdata/project/config/default/manager_metrics_patch.yaml
Outdated
Show resolved
Hide resolved
...ook/src/component-config-tutorial/testdata/project/config/default/manager_metrics_patch.yaml
Outdated
Show resolved
Hide resolved
docs/book/src/component-config-tutorial/testdata/project/config/rbac/metrics_role.yaml
Outdated
Show resolved
Hide resolved
docs/book/src/component-config-tutorial/testdata/project/config/rbac/metrics_role_binding.yaml
Outdated
Show resolved
Hide resolved
docs/book/src/component-config-tutorial/testdata/project/config/rbac/kustomization.yaml
Outdated
Show resolved
Hide resolved
aba519b
to
cc85d34
Compare
Hi @erikgb Thank you a lot for your time and help. All points are addressed and I supplement the docs to let the users know how to protected the endpoint. |
containers: | ||
- name: manager | ||
args: | ||
- "--metrics-bind-address=0.0.0.0:8080" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this patch is needed to enable metrics. We have our metrics scraped by Prometheus without this arg set. Ref. controller-runtime argument help.
-metrics-bind-address string
The address the metric endpoint binds to. (default ":8080")
So if you want to have a patch to enable the metrics endpoint, you have to disable it by default by setting it to 0, ref. https://github.com/kubernetes-sigs/controller-runtime/blob/479b723944e34ae42c9911fe01228ff34eb5ca81/pkg/metrics/server/server.go#L120-L122
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh. thank you a lot for check and share it.
I think ideally we should not enable by default.
Not everybody want to use it and by enable the metrics is required to protect the endpoint
So, it would be better if that is a conscious decision.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done the changes and also added a test to ensure that the metrics endpoint will not be exposed in this case
It("should generate a runnable project without metrics exposed", func() {
kbc.IsRestricted = false
GenerateV4WithoutMetrics(kbc)
Run(kbc, true, false, false)
})
In this case, we are using the curl pod to ensure that the connection will be refused such as:
$ kubectl logs curl -n e2e-vbau-system
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0* Trying 10.96.136.215:8080...
* connect to 10.96.136.215 port 8080 failed: Connection refused
* Failed to connect to e2e-vbau-controller-manager-metrics-service.e2e-vbau-system.svc.cluster.local port 8080 after 3 ms: Connection refused
0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0
* Closing connection 0
curl: (7) Failed to connect to e2e-vbau-controller-manager-metrics-service.e2e-vbau-system.svc.cluster.local port 8080 after 3 ms: Connection refused
Thank you a lot.
cc85d34
to
1aee283
Compare
- --leader-elect | ||
- --leader-elect | ||
- --health-probe-bind-address=:8081 | ||
- --metrics-bind-address=0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--metrics-bind-address=0 -> Used here to disable it by default
1aee283
to
f740067
Compare
ab83bd8
to
5d38a92
Compare
9efea29
to
b332e82
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for putting this through @camilamacedo86!
/lgtm
/approve
Just a few nits, can be handled later too!
docs/book/src/reference/metrics.md
Outdated
we now require the use of shared infrastructure. But as [`kube-rbac-proxy`](https://github.com/brancz/kube-rbac-proxy) is a process | ||
to be part of but not yet, sadly we cannot build and promote these images using the new |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we now require the use of shared infrastructure. But as [`kube-rbac-proxy`](https://github.com/brancz/kube-rbac-proxy) is a process | |
to be part of but not yet, sadly we cannot build and promote these images using the new | |
we now require the use of shared infrastructure. But as [`kube-rbac-proxy`](https://github.com/brancz/kube-rbac-proxy) is in a process | |
to be a part of it, but not yet, sadly we cannot build and promote these images using the new |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
fmt.Sprintf("\n- %[1]s_editor_role.yaml\n- %[1]s_viewer_role.yaml", crdName)) | ||
if err != nil { | ||
log.Errorf("Unable to add Editor and Viewer roles in the file "+ | ||
"%s.", rbacKustomizeFilePath) | ||
} | ||
// Add an empty line at the end of the file | ||
err = pluginutil.AppendCodeIfNotExist(rbacKustomizeFilePath, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, why is appending an empty line at the end of this brace file necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid breaking changes. So, it can work with scaffolds that has the kube-rbac-proxy or not.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86, varshaprasad96 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 |
b332e82
to
bbb155e
Compare
New changes are detected. LGTM label has been removed. |
8d39b62
to
6ddf84b
Compare
6ddf84b
to
973a600
Compare
c01af8f
into
kubernetes-sigs:master
* fix eks/gke semver restriction by `kubeVersion` * add pprof optional endpoint * deprecate kube-proxy due to [PR](kubernetes-sigs/kubebuilder#3899) * fix e2e pipeline
Remove the Kube RBAC Proxy and update the docs accordingly so that we can have a latest release in v3x to warning users about the changes. More info: https://github.com/kubernetes-sigs/kubebuilder/blob/master/designs/discontinue_usage_of_kube_rbac_proxy.md
**To understand why it is required see **: https://github.com/kubernetes-sigs/kubebuilder/blob/master/designs/discontinue_usage_of_kube_rbac_proxy.md
PS.: After the release we will add a discussion in this repo, prepare a guidance, communicate in the mailing list and channels.
More info: (Slack Threads)