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

Remove permission to list secrets #607

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

Conversation

bernot-dev
Copy link
Collaborator

Permission to list secrets is no longer needed.

@bernot-dev bernot-dev self-assigned this Oct 5, 2023
@pintohutch pintohutch requested review from TheSpiritXIII and bwplotka and removed request for pintohutch October 6, 2023 01:03
@pintohutch
Copy link
Collaborator

Tagging in Bartek and Daniel to take a look and confirm this still works.

@bernot-dev
Copy link
Collaborator Author

It looks like the operator does not currently have any List calls that list secrets.

  1 e2e/collector_test.go|310 col 23| if err := t.Client().List(ctx, &pods, client.InNamespace(t.namespace), client.MatchingLabelsSelector{
  2 e2e/collector_test.go|404 col 23| if err := t.Client().List(ctx, &nodes); err != nil {
  3 e2e/kubeclient.go|75 col 16| return c.base.List(ctx, list, opts...)
  4 e2e/operator_context_test.go|454 col 23| if err := kubeClient.List(ctx, &namespaceList); err != nil {
  5 e2e/operator_context_test.go|585 col 23| if err := kubeClient.List(ctx, &namespaceList, &listOpts); err != nil {
  6 pkg/operator/collection.go|386 col 21| if err := r.client.List(ctx, &podMons); err != nil {
  7 pkg/operator/collection.go|427 col 21| if err := r.client.List(ctx, &clusterPodMons); err != nil {
  8 pkg/operator/collection_test.go|142 col 13| kubeClient.List(ctx, &podMonitorings)
  9 pkg/operator/rules.go|161 col 21| if err := r.client.List(ctx, &rulesList); err != nil {
 10 pkg/operator/rules.go|175 col 21| if err := r.client.List(ctx, &clusterRulesList); err != nil {
 11 pkg/operator/rules.go|189 col 21| if err := r.client.List(ctx, &globalRulesList); err != nil {
 12 pkg/operator/target_status.go|141 col 23| if err := kubeClient.List(ctx, &podMonitoringList); err != nil {
 13 pkg/operator/target_status.go|145 col 24| if err := kubeClient.List(ctx, &clusterPodMonitoringList); err != nil {
 14 pkg/operator/target_status.go|385 col 23| if err := kubeClient.List(ctx, &podList, client.InNamespace(opts.OperatorNamespace), client.MatchingLabelsSelector{
 15 pkg/operator/target_status_test.go|1268 col 25| if err := kubeClient.List(ctx, &podMonitorings); err != nil {

@TheSpiritXIII
Copy link
Member

We do not call List directly, but the cache normally call List before Watch: https://github.com/GoogleCloudPlatform/prometheus-engine/blob/e1b516eaf3a06263d07fa77bbfff1deaf0230b3f/pkg/operator/operator.go#L229C15-L229C15

See: https://kubernetes.io/docs/reference/using-api/api-concepts/#efficient-detection-of-changes

It looks like e2e tests passed so not sure if this change is okay or it silently failed (or our tests do not hit that execution path). We may have to manually deploy and look at logs to test this out.

@bwplotka
Copy link
Collaborator

bwplotka commented Oct 9, 2023

Thanks @bernot-dev!

Great finding @TheSpiritXIII - let's double check this manually first @bernot-dev ? 🤔 Thanks! (:

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.

None yet

4 participants