Skip to content

Conversation

mtnbikenc
Copy link
Member

Fixes 1431583

@mtnbikenc mtnbikenc self-assigned this Mar 14, 2017
@mtnbikenc mtnbikenc requested review from sdodson and kwoodson March 14, 2017 12:27
namespace: "{{ item.namespace }}"
resource_kind: cluster-role
resource_name: cluster-reader
when: item.namespace == 'default'
Copy link
Member Author

@mtnbikenc mtnbikenc Mar 14, 2017

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.

Copy link
Contributor

@kwoodson kwoodson Mar 14, 2017

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.

@sdodson
Copy link
Member

sdodson commented Mar 14, 2017

aos-ci-test

@openshift-bot
Copy link

@sdodson
Copy link
Member

sdodson commented Mar 14, 2017

[merge]

@openshift-bot
Copy link

Evaluated for openshift ansible merge up to a3f2626

@openshift-bot
Copy link

[test]ing while waiting on the merge queue

@openshift-bot
Copy link

@openshift-bot
Copy link

@openshift-bot
Copy link

Evaluated for openshift ansible test up to a3f2626

@openshift-bot
Copy link

@openshift-bot
Copy link

@openshift-bot
Copy link

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)

@mtnbikenc
Copy link
Member Author

@sdodson should I be expecting this testing error? I've not changed anything in that area. Or do I need to rebase?

No package matching 'origin-docker-excluder' found available, installed or updated

@sdodson
Copy link
Member

sdodson commented Mar 14, 2017

@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.

@openshift-bot
Copy link

openshift-bot commented Mar 15, 2017

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_openshift_ansible/37/) (Base Commit: 4c6d052)

@openshift-bot openshift-bot merged commit 15130f0 into openshift:master Mar 15, 2017
@sdodson
Copy link
Member

sdodson commented Mar 15, 2017

@mtnbikenc can you pick this into release-1.5 too?

@smarterclayton
Copy link
Contributor

Whoa whoa whoa - why is the router a cluster-reader?

@smarterclayton
Copy link
Contributor

@liggitt

@kwoodson
Copy link
Contributor

@smarterclayton, I think it is in response to this bug: https://bugzilla.redhat.com/show_bug.cgi?id=1431583

@mtnbikenc
Copy link
Member Author

@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.

@sdodson
Copy link
Member

sdodson commented Mar 15, 2017

Looks like we're also granding cluster-reader to nuage, logging, metrics, and manageiq integrations.

@smarterclayton
Copy link
Contributor

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.

@mtnbikenc mtnbikenc deleted the fix-router branch March 15, 2017 14:14
@mwringe
Copy link
Contributor

mwringe commented Mar 15, 2017

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.

@liggitt
Copy link
Contributor

liggitt commented Mar 15, 2017

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.

system:node-reader gives read access to node API objects, metrics, stats, and spec info from the nodes

@kwoodson
Copy link
Contributor

This is referring to the router. Is there a lesser permission we should be using to grant the router access to read namespaces?

@knobunc
Copy link
Contributor

knobunc commented Mar 15, 2017

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).

@jcantrill
Copy link
Contributor

The fluentd component of logging uses cluster-reader to get pod spec info (e.g. annotations, labels) for enriching log messages.

@mtnbikenc
Copy link
Member Author

@knobunc Could you point me to where read-namespace is documented? I'm trying to follow-up on implementation and want to make sure I'm doing it right.

@knobunc
Copy link
Contributor

knobunc commented Mar 16, 2017

@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.

@knobunc
Copy link
Contributor

knobunc commented Mar 16, 2017

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants