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

CNS-2801 - Minimize the operator's RBAC access #163

Merged
merged 22 commits into from
Jul 11, 2023

Conversation

ltsonov-cb
Copy link
Contributor

@ltsonov-cb ltsonov-cb commented Jul 4, 2023

Main PR change
The main change in this MR is to reduce the RBAC of the operator's roles. As it stands, the operator has cluster-wide access to many objects, including secrets and configmaps. This is not strictly required and came to be because that is the default of kubebuilder and the operator-sdk.

To implement this in a good way, we decided to tie the operator + agent together and always deploy in the same namespace. The namespace cannot be changed when using the setup wizard. It can be changed by a cluster admin when deploying via Helm, but not via the operator itself.

This was made because the operator could previously move the agent around based on the CRD's Namespace value. But to scope the RBAC in that case, we require ClusterRoles or elevate permission so the operator can "assume" the required permissions in any namespace. This was deemed as too high risk for no real value, as we don't see customers changing the agent's namespace post-deployment without good reason (or needing the operator to be in a separate namespace).

The current operator RBAC is split in two roles - ClusterRole for cluster-wide resources (nodes, webhooks, priorityclasses, etc.) and a Role for anything that has a namespace.
In addition, for cluster resources we try to restrict the operations by resource name as well. For create, list, watch verbs this is not possible so we have two sets of permissions - one for those verbs and one for the remaining ones. See k8s docs for additional info.

Note that this introduces three functional (but not API) breaking changes - the deprecated CRD Namespace value, the same-namespace enforcement and the restructured Helm charts. So it is planned to be bundled in the next major operator release.

Summarized changes

  1. Operator's namespace is mounted on the pod as env var and used to deploy all other agent resources in the same namespace. If one isn't found, a fallback is used (cbcontainers-dataplane)
  2. CRD Namespace field remains but is not used in any way anymore (previously the operator would use it to deploy agent components)
  3. The dataplane accounts+roles have been moved under the operator chart. This way all namespace items (except the agent secret) are controlled in 1 place and the flow is the same as-if one was using the setup-wizard APIs.
  4. The Helm Readme's have been updated. I also removed the references to Helm repositories as we don't currently plan to have a public helm repository for the chart, we will only support installing from source.
  5. Synced the helm's operator chart - it was missing some items from the last 3-4 months.
  6. Split the operator's role into ClusterRole+Role
  7. Removed the imagePullSecret from the dataplane accounts, it's not needed for them (they pull from cbartifactory)
  8. [Minor] Changed the operator's logger to display dates instead of milliseconds-since-epoch since the latter was useless to human readers (and our logs are currently design for them).
  9. [Minor] moved the dataplane roles in a subfolder under rbac to make it clearer. I think some more restructuring there would be nice in a separate PR.

Tests:

  • Manual test suite when applying via helm and createNamespace=false,operatorNamespace=X
  • Manual test suite when applying via helm and createNamespace=true
  • Manual test suite when applying via create_operator_spec

…tead of coming from agentSpec. Modify objects to take the value from their own fields.
…ng a newer operator. Rename the local RoleBinding instead to avoid conflicts.
…rences to repositories that we don't support. Added the missing labeling when pre-creating a namespace.
@ltsonov-cb ltsonov-cb requested review from BenRub and asankov-cb July 4, 2023 08:35
@ltsonov-cb ltsonov-cb marked this pull request as ready for review July 4, 2023 11:55
@asankov-cb
Copy link
Contributor

This PR will also resolve #117

main.go Show resolved Hide resolved
api/v1/cbcontainersagent_types.go Show resolved Hide resolved
@@ -35,12 +35,12 @@ func (obj *ConfigurationK8sObject) MutateK8sObject(k8sObject client.Object, agen
return fmt.Errorf("expected ConfigMap K8s object")
}

configMap.Namespace = agentSpec.Namespace
configMap.Namespace = obj.Namespace
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't really matter
Is this line necessary? (and in other files)
The function NamespacedName takes care on this.
Just so we won't have confusion in the future.

I also wrote a comment next to the c.SetNamespace() in state applier about passing the namespace as a parameter to DesiredK8sObjectes New functions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done - both the constructor and removing these lines.

main.go Show resolved Hide resolved
main.go Show resolved Hide resolved
cbcontainers/state/state_applier.go Outdated Show resolved Hide resolved
…e on the k8s object as it has no affect (NamespacedName is what sets the namespace).
@ltsonov-cb ltsonov-cb self-assigned this Jul 11, 2023
@ltsonov-cb ltsonov-cb merged commit b83d0be into main Jul 11, 2023
@ltsonov-cb ltsonov-cb deleted the CNS-2801-rbac-role-minimization branch September 5, 2023 07:57
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.

3 participants