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

Allow for scoping TA to watch namespace #3763

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

Conversation

CharlieTLe
Copy link

@CharlieTLe CharlieTLe commented Mar 3, 2025

When the target allocator is configured to watch Prometheus custom
resources in the cluster to discover targets, it is currently hard-coded
to require a ClusterRole with a policy rule of listing namespaces. This
prevents usage in environments where configuring ClusterRoles is not
permitted i.e. in namespace-as-a-service setups where only Roles can be
created.

This change introduces two fields in the prometheusCR specification to
allow configuring the namespaces that can be interacted with by the
target allocator.

  • allowNamespaces is a comma-separated list of namespaces for the target
    allocator to watch. If set to an empty string, it will list all
    list all namespaces in the cluster. This is mutually exclusive with
    denyNamespaces. Defaults to an empty string.
  • denyNamespaces is a comma-separated list of namespaces for the target
    allocator to not watch. If set to an empty string, it will not exclude
    any namespaces. This is mutually exclusive with allowNamespaces.
    Defaults to an empty string.

Fixes: #3086

@CharlieTLe CharlieTLe requested a review from a team as a code owner March 3, 2025 22:28
factory := informers.NewMonitoringInformerFactories(map[string]struct{}{v1.NamespaceAll: {}}, map[string]struct{}{}, mClient, allocatorconfig.DefaultResyncTime, nil) //TODO decide what strategy to use regarding namespaces
// Check env var for WATCH_NAMESPACE and use it if its set, else use v1.NamespaceAll
// This is to allow the operator to watch only a specific namespace
watchNamespace, found := os.LookupEnv("WATCH_NAMESPACE")
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we instead add this to the config, allow setting it via the config file, and also this env variable? I'd like to avoid reading environment variables all over the codebase, and instead do all of it in the config package.

Copy link
Contributor

@jaronoff97 jaronoff97 Mar 6, 2025

Choose a reason for hiding this comment

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

I also wonder if we should follow the namespace selector pattern prometheus (and as a result the TA) uses?

Copy link
Contributor

Choose a reason for hiding this comment

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

That is also true. @CharlieTLe could you check out #3573 and see if it would address your need?

Copy link
Author

Choose a reason for hiding this comment

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

I don’t think it would, I was trying to get the TA to run without the ability to list namespaces so that it wouldn’t require a ClusterRole.

Copy link
Author

Choose a reason for hiding this comment

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

When

https://github.com/open-telemetry/opentelemetry-operator/pull/3763/files#diff-c946a35cc8b11bb5df8b703eef3fb16c8676aa37948d54e63bf2bd5b04859f69R65

is used here, there’s an assumption that the service account that the TA uses has a cluster role with the permission to list namespaces in the cluster.

Copy link
Author

@CharlieTLe CharlieTLe Mar 10, 2025

Choose a reason for hiding this comment

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

The error that gets printed when the TA attempts to list all namespace without the ClusterRole.

missing list/watch permissions on the 'namespaces' resource: missing "list" permission on resource "namespaces" (group: "") for all namespaces: missing "watch" permission on resource "namespaces" (group: "") for all namespaces

The needed cluster role would look something like this

apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
  name: ta
rules:
- apiGroups:
  - ""
  resources:
  - namespaces
  verbs:
  - watch
  - list

So the main change in this PR is to not use v1.NamespaceAll so that the TA can run in a namespace-scoped Role when the watch_namespace field is set.

Copy link
Contributor

Choose a reason for hiding this comment

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

Allright, I can see this being useful. We like to align these kinds of options with how prometheus-operator does it. There, this is a command-line argument for the operator, which won't work in our case, but we can use the same terminology. So this would be named AllowNamespaces, and we could also add DenyNamespaces to let this set be constrained from the other side. WDYT @jaronoff97 ?

When the target allocator is configured to watch Prometheus custom
resources in the cluster to discover targets, it is currently hard-coded
to require a ClusterRole with a policy rule of listing namespaces. This
prevents usage in environments where configuring ClusterRoles is not
permitted i.e. in namespace-as-a-service setups where only Roles can be
created.

This change introduces two fields in the prometheusCR specification to
allow configuring the namespaces that can be interacted with by the
target allocator.

- allowNamespaces is a comma-separated list of namespaces for the target
  allocator to watch. If set to an empty string, it will list all
  list all namespaces in the cluster. This is mutually exclusive with
  denyNamespaces. Defaults to an empty string.
- denyNamespaces is a comma-separated list of namespaces for the target
  allocator to not watch. If set to an empty string, it will not exclude
  any namespaces. This is mutually exclusive with allowNamespaces.
  Defaults to an empty string.

Fixes: open-telemetry#3086

Signed-off-by: Charlie Le <[email protected]>
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.

Unable to run the targetAllocator in namespace mode
3 participants