-
Notifications
You must be signed in to change notification settings - Fork 280
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
Adding RBAC support in kubernetes config (V2) #694
Conversation
Test FAILed. |
@simon-mo Tests fails with mxnet. I think the mxnet branch we merged is a little unstable? I will run a test again in case. |
Jenkins test this please |
Test PASSed. |
We received this error logs
|
@@ -0,0 +1,5 @@ | |||
apiVersion: v1 |
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.
Seems like we should bind our new ServiceAccount to prom_deployment? https://stackoverflow.com/questions/44505461/how-to-configure-a-non-default-serviceaccount-on-a-deployment
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.
@simon-mo So, I understand yaml file is correct, but don't we need to add serviceAccount to our pod -> https://github.com/ucbrise/clipper/blob/develop/clipper_admin/clipper_admin/kubernetes/prom_deployment.yaml?
I found this note from https://github.com/coreos/prometheus-operator/blob/master/Documentation/rbac.md,
Note: A cluster admin is required to create this ClusterRole and create a ClusterRoleBinding or RoleBinding to the ServiceAccount used by the Prometheus Pods. The ServiceAccount used by the Prometheus Pods can be specified in the Prometheus object.
Is it okay not to specify serviceAccount in our metric pod?
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.
You are correct, we need to use this serviceAccount in our prometheus deployment
There's nothing wrong the yaml file. but we do need to modify the version api call. |
Test PASSed. |
@@ -163,6 +168,8 @@ def __init__(self, | |||
configuration.assert_hostname = False | |||
self._k8s_v1 = client.CoreV1Api() | |||
self._k8s_beta = client.ExtensionsV1beta1Api() | |||
self._k8s_rbac=client.RbacAuthorizationV1beta1Api() |
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.
Nothing important, but we need spaces around =
for this line.
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! I'll make a lint PR formatting the entire code base.
Test PASSed. |
Test PASSed. |
Test PASSed. |
Test PASSed. |
This reverts commit 50dbf75.
Test FAILed. |
So, #687 merged this PR locally and still has an issue accessing the metric service through kube proxy. I think we should enable https://kubernetes.io/docs/reference/access-authn-authz/admission-controllers/, admission controller's ServiceAccount. -> This didn't resolve the issue. I am not sure if it is essential. |
Full output from the metrics pod for your information:
As you can see there is just one error for now. The connection through the browser still fails with:
I'll let you know if I get anything new. UPDATE: |
Test PASSed. |
Please refer to this issue. #687 |
Test PASSed. |
@simon-mo I bound default service account to prometheus cluster role, and it seems like it works fine. I think we can start from this and allow users to change settings later. |
@rkooo567 good work! since I authored this PR. I'll leave you to approve and merge. Thanks! |
@simon-mo Awesome. I will merge it tonight after cleaning up. Let's release the next version after that. @withsmilo @RehanSD Can you guys review the PR in them meantime? |
Test PASSed. |
Test PASSed. |
@@ -370,6 +377,7 @@ def _start_prometheus(self): | |||
CONFIG_FILES['metric']['deployment'], | |||
version=PROM_VERSION, | |||
cluster_name=self.cluster_name, | |||
service_account_name=self.cluster_name+"-prometheus" |
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 can't find service_account_name
in prom_deployment.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.
So, I decided to use default
as prom_deployment service account because
- it works
- There's no reason to setup specific service account for users. We can just leave it to users.
I will delete this line.
Test PASSed. |
Test PASSed. |
@simon-mo @withsmilo I will merge if one of you approve it |
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.
LGTM!
This build on top of #605