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

Without the list and watch permissions on secrets a log error is shown #14

Closed
paologallinaharbur opened this issue Apr 29, 2021 · 14 comments
Labels
bug Categorizes issue or PR as related to a bug.
Milestone

Comments

@paologallinaharbur
Copy link
Member

dev-newrelic… │ E0429 13:52:53.496860      12 reflector.go:138] sigs.k8s.io/controller-runtime/pkg/cache/internal/informers_map.go:229: Failed to watch *v1.Secret: failed to list *v1.Secret: secrets is forbidden: User "system:serviceaccount:default:dev-newrelic-infra-operators" cannot list resource "secrets" in API group "" at the cluster scope

kubernetes-sigs/controller-runtime#1156

@invidian
Copy link
Contributor

maybe we can disable the cache then? with ClientDisableCacheFor https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/manager. Not ideal though..

@paologallinaharbur
Copy link
Member Author

I tested with:

Client: client.New(mgr.GetConfig(), client.Options{Scheme: mgr.GetScheme(), Mapper: mgr.GetRESTMapper()})

Instead of mgr.GetClient() and seems to be working fine

This was the possible workaround mentioned in the issue

@invidian
Copy link
Contributor

invidian commented Apr 29, 2021

Client: client.New(mgr.GetConfig(), client.Options{Scheme: mgr.GetScheme(), Mapper: mgr.GetRESTMapper()})

Perhaps we would need a separate client for secrets then, something like uncachedClient or something.

See also kubernetes-sigs/controller-runtime#244.

EDIT: it seems it will be possible with new release: kubernetes-sigs/controller-runtime@cd065bf.

@paologallinaharbur
Copy link
Member Author

Moreover resourceName cannot be used when creating the secret. So the rbac for creating should embrace every secret. Not a big issue I believe

@paologallinaharbur
Copy link
Member Author

paologallinaharbur commented Apr 29, 2021

So far I got it working with the following permissions: @roobre

rules:
  - apiGroups: [""]
    resources:
      - "secrets"
    verbs: ["get", "update", "patch"]
    resourceNames: [{{ template "newrelic-infra-operator.fullname" . }}-config]
  - apiGroups: [""]
    resources:
      - "secrets"
    verbs: ["create"]
  - apiGroups: ["rbac.authorization.k8s.io"]
    resources: ["clusterrolebindings"]
    verbs: ["list", "watch", "get"]

I am using the different client:

Client: client.New(mgr.GetConfig(), client.Options{Scheme: mgr.GetScheme(), Mapper: mgr.GetRESTMapper()})

With the new method to reduce the size of the cache was still showing the same behaviour, not sure why

@roobre
Copy link
Contributor

roobre commented Apr 29, 2021

That ruleset is looking great to me!

@invidian
Copy link
Contributor

invidian commented May 2, 2021

Shouldn't it be more like the following:

rules:
  # To create license secret for infrastructure agent sidecar.
  - apiGroups: [""]
    resources:
      - "secrets"
    verbs: ["get", "update", "patch", "create"]
    resourceNames: [{{ template "newrelic-infra-operator.fullname" . }}-config]
  # To enable ClusterRoleBinding resources caching.
  - apiGroups: ["rbac.authorization.k8s.io"]
    resources: ["clusterrolebindings"]
    verbs: ["list", "watch", "get"]
  # To allow granting extra service accounts access to node metrics.
  - apiGroups: ["rbac.authorization.k8s.io"]
    resources: ["clusterrolebindings"]
    verbs: ["update", "patch"]
    resourceNames: [{{ template "newrelic-infra-operator.fullname" . }}-config]

@invidian
Copy link
Contributor

invidian commented May 3, 2021

So, what do we do with this issue? We can close it with proposed RBAC rules are added to the Helm chart?

@invidian invidian added the bug Categorizes issue or PR as related to a bug. label May 3, 2021
@invidian invidian added this to the v0.1.0 milestone May 3, 2021
@roobre
Copy link
Contributor

roobre commented May 3, 2021

I think @paologallinaharbur commented that create for secrets cannot be restricted to resourceNames, is this still the case?

@invidian
Copy link
Contributor

invidian commented May 3, 2021

I think @paologallinaharbur commented that create for secrets cannot be restricted to resourceNames, is this still the case?

You're right: kubernetes/kubernetes#80295 (comment).

This is working as designed. Authorization is based on the request path, not the content of the request body. That means resourceNames is not usable with the create verb. See https://kubernetes.io/docs/reference/access-authn-authz/rbac/

@paologallinaharbur
Copy link
Member Author

yes that is why I had a separate create, I would say we can close this once with Helm chart is enforcing these (or any working) permissions

@invidian
Copy link
Contributor

invidian commented May 3, 2021

yes that is why I had a separate create, I would say we can close this once with Helm chart is enforcing these (or any working) permissions

We should have a comment for that I think. It may not be obvious for newcomers.

@invidian
Copy link
Contributor

invidian commented May 7, 2021

Documented in newrelic/helm-charts@7dfd2e6.

@invidian invidian closed this as completed May 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

No branches or pull requests

3 participants