Skip to content
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

Name ClusterRole objects to be namespace-specific #1295

Merged
merged 1 commit into from
Jul 10, 2018
Merged

Conversation

siggy
Copy link
Member

@siggy siggy commented Jul 7, 2018

The control-plane's ClusterRole and ClusterRoleBinding objects are
global. Because their names did not vary across multiple control-plane
deployments, it prevented multiple control-planes from coexisting (when
RBAC is enabled).

Modify the ClusterRole and ClusterRoleBinding objects to include the
control-plane's namespace in their names. Also modify the integration
test to first install two control-planes, and then perform its full
suite of tests, to prevent regression.

Fixes #1292.

Signed-off-by: Andrew Seigner siggy@buoyant.io

@siggy
Copy link
Member Author

siggy commented Jul 7, 2018

FWIW the relevant integration tests passed on the community cluster, though failed with something unrelated, confirmed this also happens on master:

--- FAIL: TestInstall (61.69s)
	install_test.go:105: Error validating deploy [grafana]:
		Expected deployment [grafana] in namespace [conduit-tls] to have [1] replicas, but found [0]

@klingerf
Copy link
Member

klingerf commented Jul 7, 2018

@siggy Interesting. I think that integration test failure that you're seeing is: #1265 (comment) -- my guess is that the grafana pod is taking longer than 1-minute to enter the Ready state.

@briansmith
Copy link
Contributor

Why are these ClusterRoles in the first place? Can't they be just normal roles in the namespace of the control plane?

@briansmith
Copy link
Contributor

Also, I think every object we create should have the product name in it, so that we can minimize the likelihood that our objects will clash with users' objects if/when they install the conduit control plane into a namespace where they have other objects (e.g. because they are only able to create objects in one namespace).

@siggy
Copy link
Member Author

siggy commented Jul 9, 2018

Why are these ClusterRoles in the first place? Can't they be just normal roles in the namespace of the control plane?

Unfortunately we need ClusterRole's because our ServiceAccount's (conduit-controller, conduit-prometheus, conduit-ca) need access to all k8s objects, cluster-wide. AFAICT the concepts of cluster-wide ClusterRole/ClusterRoleBinding objects is conflated with access to the whole k8s cluster. There is no way to have a Role/RoleBinding in a namespace that can provide access to objects outside of that namespace.

Also, I think every object we create should have the product name in it, so that we can minimize the likelihood that our objects will clash with users' objects if/when they install the conduit control plane into a namespace where they have other objects (e.g. because they are only able to create objects in one namespace).

Yeah, because we don't want objects conflicting between multiple control-plane namespaces, I ended up putting the namespace in the names, but you are right that this would create objects like myns-controller which may already be taken. I can format the names as conduit-[ns]-controller. This would make the default conduit-conduit-controller, which isn't the greatest naming but it's functional. Open to other ideas.

/cc @briansmith

@briansmith
Copy link
Contributor

briansmith commented Jul 9, 2018

@siggy Thanks for explaining that. I think that in the future we may be able to do something more fine-grained than ClusterRoleBindings, and I think we more-or-less have to do something better than ClusterRoleBinding for the read/write roles (and role bindings) for the "CA distributor" component, but that would require some experimentation and that shouldn't block this PR.

I do think that $productName-$controllerNamespaceName-$componentName or any similar naming convention makes sense. In the near future (outside of this PR) we should agree and document what naming convention we're using. I know that the Conduit CA uses a different convention, a -conduit-io suffix, and we probably should be a bit more consistent. I don't have a strong opinion about which specific scheme we pick though.

Copy link
Member

@klingerf klingerf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⭐️ Looks good to me. Thanks for fixing this!

The control-plane's `ClusterRole` and `ClusterRoleBinding` objects are
global. Because their names did not vary across multiple control-plane
deployments, it prevented multiple control-planes from coexisting (when
RBAC is enabled).

Modify the `ClusterRole` and `ClusterRoleBinding` objects to include the
control-plane's namespace in their names. Also modify the integration
test to first install two control-planes, and then perform its full
suite of tests, to prevent regression.

Fixes #1292.

Signed-off-by: Andrew Seigner <siggy@buoyant.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants