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

commands/.../new.go;pkg/scaffold: adding support for ClusterRole and ClusterRoleBinding #747

Merged
merged 7 commits into from
Nov 15, 2018

Conversation

joelanford
Copy link
Member

Description of the change:

  • Adds a new boolean flag to the operator-sdk new command called --cluster-scoped
  • Adds IsClusterScoped field to the input.Input struct and other helper structs
  • Updates Role and RoleBinding templates to be generated as ClusterRole and ClusterRoleBinding when IsClusterScoped is set to true.
  • Updates unit associated unit tests
  • Updates user guide to describe how to update the generated ClusterRoleBinding to set the namespace on the service account subject.

Motivation for the change:
See #736. This PR allows users to generate an operator that can be deployed with cluster scoped permissions by tweaking scaffolding of RBAC resources.

Given #236, I'm not sure if we want to do this or if there's more to do than just this. I also wasn't sure whether to put the IsClusterScoped field in input.Input (which I did) or if we should limit it to just scaffold.Role and scaffold.RoleBinding. Thoughts?

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 15, 2018
Repo: filepath.Join(projutil.CheckAndGetProjectGoPkg(), projectName),
AbsProjectPath: filepath.Join(projutil.MustGetwd(), projectName),
ProjectName: projectName,
IsClusterScoped: isClusterScoped,
Copy link
Member

Choose a reason for hiding this comment

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

isClusterScoped is specific to Roles and RoleBindings so it shouldn't go in input.Config or scaffold.Scaffold. I suggest adding an IsClusterScoped field and setting it in both scaffold.Role and scaffold.RoleBinding here.

Copy link
Member

@lilic lilic left a comment

Choose a reason for hiding this comment

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

lgtm 👍

One suggestion is to maybe add some docs on the flag and link to some cluster scoped docs, just to explain that it exists and the use case for it, as currently we only mention it after the user creates the operator.

apiVersion: rbac.authorization.k8s.io/v1
metadata:
name: {{.ProjectName}}
subjects:
- kind: ServiceAccount
name: {{.ProjectName}}
{{- if .IsClusterScoped }}
# Replace this with the operator namespace
Copy link
Member

Choose a reason for hiding this comment

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

Nit but maybe this might make things a bit clearer:
Replace this with the namespace the operator is deployed in.

@shawn-hurley
Copy link
Member

Quick question, but if a user tells us that their operator is cluster-wide, we probably want the WATCH_NAMESPACE in the deployment to be "" as well?

Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

LGTM

@hasbro17
Copy link
Contributor

Overall SGTM.
However I think we'll need to update the ansible user-guide as well for the steps of replacing the REPLACE_NAMESPACE for deploy/role_binding.yaml when cluster-scoped.
@shawn-hurley Can you double check what changes we need for the rbac setup in the ansible user-guide?
https://github.com/operator-framework/operator-sdk/blob/master/doc/ansible/user-guide.md#1-run-as-a-pod-inside-a-kubernetes-cluster

@joelanford
Copy link
Member Author

Good catch. Those sections are nearly identical, so I copied over the cluster-scoped stuff into the ansible docs. I did a quick test by scaffolding new go and ansible operators with the --cluster-scoped flag and deployed them along with a couple of CRs in different namespaces and everything deployed as expected.

@joelanford joelanford merged commit 8412ccb into operator-framework:master Nov 15, 2018
@joelanford joelanford deleted the cluster-scope branch November 15, 2018 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants