-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
commands/.../new.go;pkg/scaffold: adding support for ClusterRole and ClusterRoleBinding #747
Conversation
…ClusterRoleBinding
commands/operator-sdk/cmd/new.go
Outdated
Repo: filepath.Join(projutil.CheckAndGetProjectGoPkg(), projectName), | ||
AbsProjectPath: filepath.Join(projutil.MustGetwd(), projectName), | ||
ProjectName: projectName, | ||
IsClusterScoped: isClusterScoped, |
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.
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.
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 👍
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.
pkg/scaffold/rolebinding.go
Outdated
apiVersion: rbac.authorization.k8s.io/v1 | ||
metadata: | ||
name: {{.ProjectName}} | ||
subjects: | ||
- kind: ServiceAccount | ||
name: {{.ProjectName}} | ||
{{- if .IsClusterScoped }} | ||
# Replace this with the operator 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.
Nit but maybe this might make things a bit clearer:
Replace this with the namespace the operator is deployed in.
Quick question, but if a user tells us that their operator is cluster-wide, we probably want the |
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
Overall SGTM. |
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 |
Description of the change:
operator-sdk new
command called--cluster-scoped
IsClusterScoped
field to theinput.Input
struct and other helper structsClusterRole
andClusterRoleBinding
whenIsClusterScoped
is set to true.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 ininput.Input
(which I did) or if we should limit it to justscaffold.Role
andscaffold.RoleBinding
. Thoughts?