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

feat: Adding extra permissions to the cluster's default service account #1943

Merged
merged 2 commits into from
May 30, 2024

Conversation

jullianow
Copy link
Contributor

Recently, I started testing a component suggested by Google to consume GMP metrics (Google Manager Prometheus).
Ref: https://cloud.google.com/stackdriver/docs/managed-prometheus/hpa

This component, until the version of this module in 28.0.0, worked normally, and after I updated to version 30.2.0, it stopped working.
Trying to understand better, I realized that one of the main changes in the module version was due to this PR.

What happens is that the role roles/container.nodeServiceAccount does not have all the necessary permissions for the Adapter to work, especially the monitoring.metricDescriptors.get permission.
This caused me to get this error when I updated the service account permissions.

image

Therefore, the objective of this change is to revert at least two roles that were removed (roles/monitoring.metricWriter and roles/stackdriver.resourceMetadata.writer) so that the GMP Adapter works normally again.

@jullianow jullianow requested review from ericyz, gtsorbo and a team as code owners May 14, 2024 20:27
@jullianow jullianow force-pushed the sa_bindings_roles branch 2 times, most recently from ba1bbe8 to f985310 Compare May 14, 2024 20:29
@jullianow jullianow changed the title Adding extra permissions to the cluster's default service account feat: Adding extra permissions to the cluster's default service account May 14, 2024
@apeabody
Copy link
Contributor

/gcbrun

Copy link
Contributor

@apeabody apeabody left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @jullianow!

From the Integration Test:

Error: Reference to undeclared resource

  on ../../../sa.tf line 57, in resource "google_project_iam_member" "cluster_service_account-metric_writer":
  57:   project = google_project_iam_member.cluster_service_account-log_writer[0].project

A managed resource "google_project_iam_member"
"cluster_service_account-log_writer" has not been declared in
module.example.module.gke.

Error: Reference to undeclared resource

  on ../../../sa.tf line 64, in resource "google_project_iam_member" "cluster_service_account-resourceMetadata-writer":
  64:   project = google_project_iam_member.cluster_service_account-monitoring_viewer[0].project

A managed resource "google_project_iam_member"
"cluster_service_account-monitoring_viewer" has not been declared in
module.example.module.gke.}

@jullianow jullianow force-pushed the sa_bindings_roles branch from 8cb45e7 to 0155eaf Compare May 29, 2024 21:37
@jullianow jullianow force-pushed the sa_bindings_roles branch from 0155eaf to d99f8fa Compare May 29, 2024 21:38
@jullianow
Copy link
Contributor Author

@apeabody Adjusted.

@jullianow jullianow requested a review from apeabody May 29, 2024 21:38
@apeabody
Copy link
Contributor

/gcbrun

Copy link
Contributor

@apeabody apeabody left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @jullianow!

Confirmed description of these roles here: https://cloud.google.com/iam/docs/understanding-roles

@apeabody apeabody merged commit 4fab404 into terraform-google-modules:master May 30, 2024
4 checks passed
@gorge511
Copy link
Contributor

gorge511 commented Jul 8, 2024

I think the correct approach is to use Workload Identity for the GMP and the HPA Adapter. Please read how to set up the identity in step 4 of the docs.

Nodes should have as little permissions as possible.

@oliverboehme-ida
Copy link

Hm I just saw this change when trying to upgrade to v31.1.0.

Wouldn't it have been sufficient for solving your problem to just assign roles/monitoring.viewer (a read-only role).
This would also be a less invasive way in my eyes.

Also I don't see at all why it needs roles/stackdriver.resourceMetadata.writer.

CPL-markus pushed a commit to WALTER-GROUP/terraform-google-kubernetes-engine that referenced this pull request Jul 15, 2024
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.

4 participants