-
Notifications
You must be signed in to change notification settings - Fork 95
Fix: Move away from kube-rbac-proxy sidecar for metric server auth. #605
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
base: master
Are you sure you want to change the base?
Fix: Move away from kube-rbac-proxy sidecar for metric server auth. #605
Conversation
Signed-off-by: codeknight03 <[email protected]>
BREAKING CHANGE: Defaults to HTTPS instead of HTTP for auth to work. Signed-off-by: codeknight03 <[email protected]>
Signed-off-by: codeknight03 <[email protected]>
Yay, your first pull request! 👍 A contributor will be by to give feedback soon. In the meantime, please review the Layer5 Community Welcome Guide and sure to join the community Slack. |
Blocker before merging:
Is failing because I am on |
@codeknight03 you might try running on codespaces . I am having some trouble with kubernetes on my linux |
@codeknight03 you can custom build the binaries for this using go. |
- meshSync | ||
links: | ||
- name: Meshery Operator | ||
url: https://meshery-operator.domain |
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.
not sure this is a valid url
kind: MeshSync | ||
name: meshsyncs.meshery.io | ||
version: v1alpha1 | ||
description: Meshery Operator is the multi-service mesh operator and implementation |
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.
meshery-operator is more than a multi-service mesh
annotations: | ||
controller-gen.kubebuilder.io/version: v0.17.1 | ||
creationTimestamp: null | ||
name: meshsyncs.meshery.io |
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.
name: meshsyncs.meshery.io | |
name: meshsync.meshery.io |
if secureMetrics { | ||
// Add the auth filter *only* if secure serving is enabled | ||
metricsServerOptions.FilterProvider = filters.WithAuthenticationAndAuthorization | ||
setupLog.Info( | ||
"Metrics endpoint protection enabled using controller-runtime filters", | ||
"address", | ||
metricsAddr, | ||
) | ||
} else { | ||
// Log if metrics are insecure (should be rare if default is true) | ||
setupLog.Info( | ||
"Metrics endpoint is serving insecurely", | ||
"address", | ||
metricsAddr, | ||
) | ||
} |
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.
if secureMetrics { | |
// Add the auth filter *only* if secure serving is enabled | |
metricsServerOptions.FilterProvider = filters.WithAuthenticationAndAuthorization | |
setupLog.Info( | |
"Metrics endpoint protection enabled using controller-runtime filters", | |
"address", | |
metricsAddr, | |
) | |
} else { | |
// Log if metrics are insecure (should be rare if default is true) | |
setupLog.Info( | |
"Metrics endpoint is serving insecurely", | |
"address", | |
metricsAddr, | |
) | |
} | |
// Log if metrics are insecure (should be rare if default is true) | |
metricsEndpointLog := "Metrics endpoint is serving insecurely" | |
if secureMetrics { | |
// Add the auth filter *only* if secure serving is enabled | |
metricsServerOptions.FilterProvider = filters.WithAuthenticationAndAuthorization | |
metricsEndpointLog = "Metrics endpoint is serving with protection enabled using controller-runtime filters" // perhaps reword this one to make it a little more similar to the insecure message | |
} | |
setupLog.Info( | |
metricsEndpointLog, | |
"address", | |
metricsAddr, | |
) | |
# - use environment variables to overwrite this value (e.g export VERSION=0.0.2) | ||
# Current Operator version | ||
VERSION ?= 0.0.1 | ||
VERSION ?= 0.0.2 |
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.
this changes looks more than a patch. it should be a major upgrade or at least minor one ?
Yeah this helps, but actually the source of my troubles is
is downloading all the binaries that are required for the test env, based on my platform i.e. darwin/arm64. But
needs binaries place at the path |
@codeknight03, are you still chewing on this one? |
This one is complete as it is I am working on the test cases. But haven't had a lot of progress to be honest. |
Description
This PR fixes ##573
Notes for Reviewers
Impact
port:8443
Tests Done
** Local Run ( Outside Cluster)**
Out side cluster run with
Without Token : Returned Unauthorized as expected

With secureMetrics=false: Returned Metrics as expected

With Token: Can not be tested for Local as without a k8s cluster can not generate token.
Within Cluster run
After running the following commands
Also had to set
ImagePullPolicy:IfNotPresent
inconfig/manager/manager.yaml
Performed the following tests
Without Token : Returned Unauth as expected

With SA Token:
Credits
Signed commits