-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Add router svcacct cluster-reader role #3650
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
Conversation
namespace: "{{ item.namespace }}" | ||
resource_kind: cluster-role | ||
resource_name: cluster-reader | ||
when: item.namespace == 'default' |
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.
The only thing I'm not sure about is whether this needs to be run for all shards, or only on the 'default' namespace.
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.
@mtnbikenc, good question here. How we run it today is with the same service account and in the same namespace so a single call would probably suffice. The loop would get both use cases where someone wants to run with a different service account.
aos-ci-test |
a3f2626 - State: success - All Test Contexts: aos-ci-jenkins/OS_unit_tests - Logs: https://aos-ci.s3.amazonaws.com/openshift/openshift-ansible/jenkins-openshift-ansible-2-unit-tests-1120/a3f2626530456e9eae9492758900a30e8d641b8b.txt |
[merge] |
Evaluated for openshift ansible merge up to a3f2626 |
[test]ing while waiting on the merge queue |
a3f2626 - State: success - All Test Contexts: "aos-ci-jenkins/OS_3.4_NOT_containerized, aos-ci-jenkins/OS_3.4_NOT_containerized_e2e_tests" - Logs: https://aos-ci.s3.amazonaws.com/openshift/openshift-ansible/jenkins-openshift-ansible-3-test-matrix-CONTAINERIZED=_NOT_containerized,OSE_VER=3.4,PYTHON=System-CPython-2.7,TOPOLOGY=openshift-cluster,TargetBranch=master,nodes=openshift-ansible-slave-1124/a3f2626530456e9eae9492758900a30e8d641b8b.txt |
a3f2626 - State: success - All Test Contexts: "aos-ci-jenkins/OS_3.4_containerized, aos-ci-jenkins/OS_3.4_containerized_e2e_tests" - Logs: https://aos-ci.s3.amazonaws.com/openshift/openshift-ansible/jenkins-openshift-ansible-3-test-matrix-CONTAINERIZED=_containerized,OSE_VER=3.4,PYTHON=System-CPython-2.7,TOPOLOGY=openshift-cluster-containerized,TargetBranch=master,nodes=openshift-ansible-slave-1124/a3f2626530456e9eae9492758900a30e8d641b8b.txt |
Evaluated for openshift ansible test up to a3f2626 |
a3f2626 - State: success - All Test Contexts: "aos-ci-jenkins/OS_3.5_NOT_containerized, aos-ci-jenkins/OS_3.5_NOT_containerized_e2e_tests" - Logs: https://aos-ci.s3.amazonaws.com/openshift/openshift-ansible/jenkins-openshift-ansible-3-test-matrix-CONTAINERIZED=_NOT_containerized,OSE_VER=3.5,PYTHON=System-CPython-2.7,TOPOLOGY=openshift-cluster,TargetBranch=master,nodes=openshift-ansible-slave-1124/a3f2626530456e9eae9492758900a30e8d641b8b.txt |
a3f2626 - State: success - All Test Contexts: "aos-ci-jenkins/OS_3.5_containerized, aos-ci-jenkins/OS_3.5_containerized_e2e_tests" - Logs: https://aos-ci.s3.amazonaws.com/openshift/openshift-ansible/jenkins-openshift-ansible-3-test-matrix-CONTAINERIZED=_containerized,OSE_VER=3.5,PYTHON=System-CPython-2.7,TOPOLOGY=openshift-cluster-containerized,TargetBranch=master,nodes=openshift-ansible-slave-1124/a3f2626530456e9eae9492758900a30e8d641b8b.txt |
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_openshift_ansible_extended_conformance_install/21/) (Base Commit: 46d1efc) |
@sdodson should I be expecting this testing error? I've not changed anything in that area. Or do I need to rebase?
|
@mtnbikenc I think the origin excluder is broken and I've opened a PR to fix it. I think we can also work around it in openshift-ansible too. |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_openshift_ansible/37/) (Base Commit: 4c6d052) |
@mtnbikenc can you pick this into release-1.5 too? |
Whoa whoa whoa - why is the router a cluster-reader? |
@smarterclayton, I think it is in response to this bug: https://bugzilla.redhat.com/show_bug.cgi?id=1431583 |
@smarterclayton The above bug linked to this this bug comment which provided the guidance. https://bugzilla.redhat.com/show_bug.cgi?id=1332510#c1 Please correct me if there is newer guidance. |
Looks like we're also granding cluster-reader to nuage, logging, metrics, and manageiq integrations. |
We shouldn't given cluster reader to those - that's way too much permission. We should be giving out a much more restricted role. It's not good to give infrastructure components cluster-reader by default (maybe manageiq), because that makes those components have way more privileges than we want. In general, if we need a generic role that lets some data be accessible, we should describe it as a new role - not as giving them cluster-reader. Can we get the various folks involved in those components @jcantrill @mwringe @simon3z @knobunc to describes what permissions they actually need? This is a potential security issue, so I'd like to get resolution on steps forward very shortly. In the meantime, we need to hold off on merging the backport of this. |
For the metrics components, its only Heapster that currently requests cluster-reader permissions. I believe it needs cluster reader permissions because that was the only way it could access /stats and summary information from the nodes. Heapster also needs to be able to read pod definitions from each namespace, but that can be granted using a role with a lower level of authorisation. |
|
This is referring to the router. Is there a lesser permission we should be using to grant the router access to read namespaces? |
All the router needs is read-namespace so it can read the labels on the namespace to see if they match the subset the router is looking for. And that permission is only needed when the router is sharded (i.e. when the env NAMESPACE_LABELS is set to a label selector). |
The fluentd component of logging uses cluster-reader to get pod spec info (e.g. annotations, labels) for enriching log messages. |
@knobunc Could you point me to where |
@mtnbikenc sorry Russell. I mean that generically. It needs to be able to read the namespace objects. I'm not sure if we have something else we can grant yet if they want to do sharding. |
@mtnbikenc Here's the doc where we tell them to grant the cluster-reader permission: https://docs.openshift.com/enterprise/3.2/install_config/install/deploy_router.html#creating-the-router-service-account @smarterclayton is there a finer-graind role we can use? Or are you advocating that we make a new one? |
Fixes 1431583