Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Limit scope of flux and flux operator in helm chart. #1928

Closed
wants to merge 1 commit into from

Conversation

arturo-c
Copy link
Contributor

Allows you to set clusterScope to false in helm chart so that rbac resources and arguments to both flux and helm operator are scoped only to the namespace where flux is deployed.

Partly addresses #1886 and #1085

@arturo-c
Copy link
Contributor Author

Similar to implementation in https://github.com/zeeZ/locked-down-flux

@hiddeco hiddeco requested a review from stefanprodan April 12, 2019 15:42
chart/flux/README.md Outdated Show resolved Hide resolved
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

Thanks @arturo-c please see my comments.

chart/flux/Chart.yaml Outdated Show resolved Hide resolved
chart/flux/templates/deployment.yaml Outdated Show resolved Hide resolved
@arturo-c arturo-c force-pushed the chart-namespace-rbac branch from bec8101 to 02a8e78 Compare April 12, 2019 16:19
@arturo-c
Copy link
Contributor Author

thanks @stefanprodan and @2opremio for the really quick review!, updated branch with the changes requested

@arturo-c
Copy link
Contributor Author

also maybe @zeeZ can take a look, since thats where I found the rbac example for flux -> #1830

Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

@arturo-c have you tested this with a namespace definition or some other cluster object? Will Flux stop function or it would keep applying the yamls that are allowed by RBAC?

@arturo-c
Copy link
Contributor Author

@stefanprodan have not tested by having resources out of the namespace for each operator, will try and do that today and report back

{{- $kind := "Role" -}}
{{- $bind := "RoleBinding" -}}
{{- if .Values.clusterScope -}}
{{ $kind = "ClusterRole" }}

Choose a reason for hiding this comment

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

I had to change the assignments in lines 4 and 5 to := from =

Suggested change
{{ $kind = "ClusterRole" }}
{{ $kind := "ClusterRole" }}

Maybe my helm client is too old?

Client: &version.Version{SemVer:"v2.9.1", GitCommit:"20adb27c7c5868466912eebdf6664e7390ebe710", GitTreeState:"clean"}
Server: &version.Version{SemVer:"v2.13.0", GitCommit:"79d07943b03aea2b76c12644b4b54733bc5958d6", GitTreeState:"clean"}

Copy link

@dranner-bgt dranner-bgt Apr 26, 2019

Choose a reason for hiding this comment

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

I also had to remove the nonResourceURLs from the Role (apparently they only work for ClusterRoles):

apiVersion: rbac.authorization.k8s.io/v1beta1
kind: {{ $kind }}
metadata:
  name: {{ template "flux.fullname" . }}
  {{- if not .Values.clusterScope }}
  namespace: {{ .Release.Namespace }}
  {{- end }}
  labels:
    app: {{ template "flux.name" . }}
    chart: {{ template "flux.chart" . }}
    release: {{ .Release.Name }}
    heritage: {{ .Release.Service }}
rules:
  - apiGroups: ["*"]
    resources: ["*"]
    verbs: ["*"]

Choose a reason for hiding this comment

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

Thank you for this effort. I'm using this PR as a base to deploy flux into a cluster with access to a limited list of namespaces. I think it might be best to create a new PR that contains my additional RBAC resources once this PR is merged.

@2opremio
Copy link
Contributor

@arturo-c any updates? Are you planning to continue working on this PR?

@arturo-c
Copy link
Contributor Author

arturo-c commented May 23, 2019

@2opremio sorry, haven't had time to work on this, not sure if best path forward is to close this pr for now?

@stefanprodan
Copy link
Member

Closing this in favour of #2206

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

Successfully merging this pull request may close these issues.

4 participants