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.

Sign up for GitHub

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

Jump to bottom

Restrict cluster role of the amazon cloudwatch agent controller manager #83

Closed
wants to merge 6 commits into from
Closed

Restrict cluster role of the amazon cloudwatch agent controller manager #83

wants to merge 6 commits into from

Conversation

lisguo
Copy link
Contributor

@lisguo lisguo commented Aug 14, 2024 •
edited
Loading

Issue #, if available:
Related: #77

Description of changes:

  1. Remove unused privileges for leases, ingresses, and openshift routes
  2. only delete, patch, and update config maps used by the helm chart:
kubectl get configmaps -n amazon-cloudwatch -o name
configmap/cloudwatch-agent
configmap/cloudwatch-agent-windows
configmap/cwagent-clusterleader
configmap/dcgm-exporter-config-map
configmap/fluent-bit-config
configmap/fluent-bit-windows-config
configmap/kube-root-ca.crt
configmap/neuron-monitor-config-mapkh
  1. only delete, patch and update service accounts used by the helm chart:
kubectl get serviceaccounts -n amazon-cloudwatch -o name
serviceaccount/amazon-cloudwatch-observability-controller-manager
serviceaccount/cloudwatch-agent
serviceaccount/dcgm-exporter-service-acct
serviceaccount/default
serviceaccount/neuron-monitor-service-acct
  1. only delete, patch and update services used by the helm chart:
kubectl get services -n amazon-cloudwatch -o name
service/amazon-cloudwatch-observability-webhook-service
service/cloudwatch-agent
service/cloudwatch-agent-headless
service/cloudwatch-agent-monitoring
service/cloudwatch-agent-windows
service/cloudwatch-agent-windows-headless
service/cloudwatch-agent-windows-monitoring
service/dcgm-exporter-service
service/neuron-monitor-service

Testing:

kubectl describe clusterrole amazon-cloudwatch-observability-manager-role
Name:         amazon-cloudwatch-observability-manager-role
Labels:       <none>
Annotations:  <none>
PolicyRule:
  Resources                                                    Non-Resource URLs  Resource Names                                        Verbs
  ---------                                                    -----------------  --------------                                        -----
  daemonsets.apps                                              []                 []                                                    [create delete get list patch update watch]
  deployments.apps                                             []                 []                                                    [create delete get list patch update watch]
  statefulsets.apps                                            []                 []                                                    [create delete get list patch update watch]
  configmaps                                                   []                 []                                                    [create get list watch]
  serviceaccounts                                              []                 []                                                    [create get list watch]
  services                                                     []                 []                                                    [create get list watch]
  events                                                       []                 []                                                    [create patch]
  configmaps                                                   []                 [cloudwatch-agent-windows]                            [delete patch update]
  configmaps                                                   []                 [cloudwatch-agent]                                    [delete patch update]
  configmaps                                                   []                 [cwagent-clusterleader]                               [delete patch update]
  configmaps                                                   []                 [dcgm-exporter-config-map]                            [delete patch update]
  configmaps                                                   []                 [fluent-bit-config]                                   [delete patch update]
  configmaps                                                   []                 [fluent-bit-windows-config]                           [delete patch update]
  configmaps                                                   []                 [kube-root-ca.crt]                                    [delete patch update]
  configmaps                                                   []                 [neuron-monitor-config-map]                           [delete patch update]
  serviceaccounts                                              []                 [amazon-cloudwatch-observability-controller-manager]  [delete patch update]
  serviceaccounts                                              []                 [cloudwatch-agent]                                    [delete patch update]
  serviceaccounts                                              []                 [dcgm-exporter-service-acct]                          [delete patch update]
  serviceaccounts                                              []                 [neuron-monitor-service-acct]                         [delete patch update]
  services                                                     []                 [amazon-cloudwatch-observability-webhook-service]     [delete patch update]
  services                                                     []                 [cloudwatch-agent-headless]                           [delete patch update]
  services                                                     []                 [cloudwatch-agent-monitoring]                         [delete patch update]
  services                                                     []                 [cloudwatch-agent-windows-headless]                   [delete patch update]
  services                                                     []                 [cloudwatch-agent-windows-monitoring]                 [delete patch update]
  services                                                     []                 [cloudwatch-agent-windows]                            [delete patch update]
  services                                                     []                 [cloudwatch-agent]                                    [delete patch update]
  services                                                     []                 [dcgm-exporter-service]                               [delete patch update]
  services                                                     []                 [neuron-monitor-service]                              [delete patch update]
  namespaces                                                   []                 []                                                    [get list patch update watch]
  amazoncloudwatchagents.cloudwatch.aws.amazon.com             []                 []                                                    [get list patch update watch]
  dcgmexporters.cloudwatch.aws.amazon.com                      []                 []                                                    [get list patch update watch]
  instrumentations.cloudwatch.aws.amazon.com                   []                 []                                                    [get list patch update watch]
  neuronmonitors.cloudwatch.aws.amazon.com                     []                 []                                                    [get list patch update watch]
  replicasets.apps                                             []                 []                                                    [get list watch]
  amazoncloudwatchagents.cloudwatch.aws.amazon.com/finalizers  []                 []                                                    [get patch update]
  amazoncloudwatchagents.cloudwatch.aws.amazon.com/status      []                 []                                                    [get patch update]
  dcgmexporters.cloudwatch.aws.amazon.com/finalizers           []                 []                                                    [get patch update]
  dcgmexporters.cloudwatch.aws.amazon.com/status               []                 []                                                    [get patch update]
  neuronmonitors.cloudwatch.aws.amazon.com/finalizers          []                 []                                                    [get patch update]
  neuronmonitors.cloudwatch.aws.amazon.com/status              []                 []                                                    [get patch update]

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@lisguo lisguo mentioned this pull request Aug 15, 2024
Open
@lisguo
Copy link
Contributor Author

lisguo commented Aug 15, 2024

Change made to the operator repo to ensure integ tests pass: aws/amazon-cloudwatch-agent-operator#204

sky333999
sky333999 reviewed Aug 16, 2024
View reviewed changes
charts/amazon-cloudwatch-observability/templates/operator-clusterrole.yaml
verbs: [ "create","get","list", "watch" ]
- apiGroups: [ "" ]
resources: [ "configmaps" ]
resourceNames: ["cloudwatch-agent", "cloudwatch-agent-windows", "cwagent-clusterleader", "dcgm-exporter-config-map", "fluent-bit-config", "fluent-bit-windows-config", "neuron-monitor-config-map", "kube-root-ca.crt"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Restricting these by names is going to be tricky.
Customers installing their own AmazonCloudWatchAgent CR can set the name to be anything and the resources would get named accordingly per the regexes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, curious, why do we need the kube-root-ca.crt in this list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I was hoping there would be a way to do this by namespace, but it doesn't seem like thats possible. What if we pull the actual names from the values.yaml?

kube-root-ca.crt was found in the amazon-cloudwatch namespace, so I assume the operator should have domain over anything we create via helm/operator

Copy link
Contributor

Choose a reason for hiding this comment

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

What if we pull the actual names from the values.yaml?

In the scenario of customers installing their own CRs, it wouldnt come from the values. But perhaps thats just setting an expectation that they have to modify the RBAC accordingly when they do such customizations.

Copy link
Contributor

Choose a reason for hiding this comment

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

kube-root-ca.crt was found in the amazon-cloudwatch namespace, so I assume the operator should have domain over anything we create via helm/operator

Interesting. Is it actually created/modified by our service account though?

charts/amazon-cloudwatch-observability/templates/operator-clusterrole.yaml Show resolved Hide resolved
@sky333999
Copy link
Contributor

Perhaps a better way would be to expose a securityLevel in the values.yaml to allow specifying limited vs full-access.

The default limited level would mean we install a new Role scoped down to the amazon-cloudwatch namespace for most of the permissions above and use that in conjunction with a stripped down version of a ClusterRole (given we'd still need read, get, patch etc for the entire Cluster for usecases like AppSignals).

Users that would like to install custom CRs and/or are comfortable allowing the Operator to run with additional permissions could override to use the full-access mode which will just install our current ClusterRole we have today.

@lisguo
Copy link
Contributor Author

lisguo commented Aug 16, 2024

Yeah I think we will want something namespaced to amazon-cloudwatch which would essentially mean having a Role instead of a ClusterRole. Is it possible to have a service account bind to both a Role (amazon-cloudwatch) and a cluster role (for auto annotation / instrumentation purposes) ?

@sky333999
Copy link
Contributor

Is it possible to have a service account bind to both a Role (amazon-cloudwatch) and a cluster role (for auto annotation / instrumentation purposes) ?

Yes, this should be doable and what we would need with the limited level from my suggestion above.

@lisguo lisguo closed this Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sky333999 sky333999 sky333999 left review comments

@mitali-salvi mitali-salvi Awaiting requested review from mitali-salvi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lisguo @sky333999