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

Permission check fixed for the serviceaccount of the target allocator #3391

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

davidhaja
Copy link
Contributor

Description:
Permission check fixed for the serviceaccount of the target allocator.
If the opentelemetrycollector.spec.targetAllocator.serviceAccount is not set, the operator
checks the system:serviceaccount:default:<EMPTY SA NAME> serviceaccount instead of the generated serviceaccount name.

Warning: missing the following rules for networking.k8s.io/ingresses: [get,system:serviceaccount:default:,list,system:serviceaccount:default:,watch,system:serviceaccount:default:]
Warning: missing the following rules for nodes: [get,system:serviceaccount:default:,list,system:serviceaccount:default:,watch,system:serviceaccount:default:]
Warning: missing the following rules for nodes/metrics: [get,system:serviceaccount:default:,list,system:serviceaccount:default:,watch,system:serviceaccount:default:]
Warning: missing the following rules for endpoints: [get,system:serviceaccount:default:,list,system:serviceaccount:default:,watch,system:serviceaccount:default:]
Warning: missing the following rules for pods: [get,system:serviceaccount:default:,list,system:serviceaccount:default:,watch,system:serviceaccount:default:]
Warning: missing the following rules for discovery.k8s.io/endpointslices: [get,system:serviceaccount:default:,list,system:serviceaccount:default:,watch,system:serviceaccount:default:]
Warning: missing the following rules for nonResourceURL: /metrics: [get,system:serviceaccount:default:]
Warning: missing the following rules for monitoring.coreos.com/servicemonitors: [*,system:serviceaccount:default:]
Warning: missing the following rules for monitoring.coreos.com/podmonitors: [*,system:serviceaccount:default:]
Warning: missing the following rules for services: [get,system:serviceaccount:default:,list,system:serviceaccount:default:,watch,system:serviceaccount:default:]
Warning: missing the following rules for namespaces: [get,system:serviceaccount:default:,list,system:serviceaccount:default:,watch,system:serviceaccount:default:]
Warning: missing the following rules for configmaps: [get,system:serviceaccount:default:]

This PR resolves this issue using the target allocator's serviceaccount naming logic.

Link to tracking Issue(s): 3380

Testing:

Documentation:

@davidhaja davidhaja requested a review from a team as a code owner October 25, 2024 06:49
@iblancasa iblancasa self-requested a review October 25, 2024 06:53
@iblancasa
Copy link
Contributor

checks the system:serviceaccount:default: serviceaccount instead of the generated serviceaccount name.

Is that the default service account the TA will use?

@davidhaja
Copy link
Contributor Author

checks the system:serviceaccount:default: serviceaccount instead of the generated serviceaccount name.

Is that the default service account the TA will use?

Yes.

Copy link
Contributor

@swiatekm swiatekm 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 fixing this! I'd love to see a test verifying that we call the function correctly. Changing the order of two string arguments in a function call is an easy mistake to make.

@jaronoff97
Copy link
Contributor

+1 to Mikolaj's suggestion though :)

@davidhaja
Copy link
Contributor Author

Thanks for fixing this! I'd love to see a test verifying that we call the function correctly. Changing the order of two string arguments in a function call is an easy mistake to make.

I'm wondering if it makes sense to include the serviceaccount name in the warning message?
So like:

Warning: missing the following rules for system:serviceaccount:default:metrics-targetallocator - nonResourceURL: /metrics: [get]
Warning: missing the following rules for system:serviceaccount:default:metrics-targetallocator - monitoring.coreos.com/podmonitors: [*]
Warning: missing the following rules for system:serviceaccount:default:metrics-targetallocator - nodes/metrics: [get,list,watch]
Warning: missing the following rules for system:serviceaccount:default:metrics-targetallocator - pods: [get,list,watch]
Warning: missing the following rules for system:serviceaccount:default:metrics-targetallocator - discovery.k8s.io/endpointslices: [get,list,watch]
Warning: missing the following rules for system:serviceaccount:default:metrics-targetallocator - namespaces: [get,list,watch]
Warning: missing the following rules for system:serviceaccount:default:metrics-targetallocator - configmaps: [get]
Warning: missing the following rules for system:serviceaccount:default:metrics-targetallocator - networking.k8s.io/ingresses: [get,list,watch]
Warning: missing the following rules for system:serviceaccount:default:metrics-targetallocator - monitoring.coreos.com/servicemonitors: [*]
Warning: missing the following rules for system:serviceaccount:default:metrics-targetallocator - nodes: [get,list,watch]
Warning: missing the following rules for system:serviceaccount:default:metrics-targetallocator - services: [get,list,watch]
Warning: missing the following rules for system:serviceaccount:default:metrics-targetallocator - endpoints: [get,list,watch]

@swiatekm @jaronoff97 wdyt?

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.

RBAC warnings when using custom service account/rbac
4 participants