enhancement: deploy operator in single namespace without `ClusterRole` #5807

zhuangzhi-cc · 2023-08-14T12:49:24Z

Description

We need deploy prometheus operator in single namespace without cluster role rabc. Follow simonpasquier's suggestion if the --namespaces is a single namespace, operator would running in none clusterrole mode.
eg. --namespaces=monitoring

Type of change

What type of changes does your code introduce to the Prometheus operator? Put an x in the box that apply.

CHANGE (fix or feature that would cause existing functionality to not work as expected)
FEATURE (non-breaking change which adds functionality)
BUGFIX (non-breaking change which fixes an issue)
ENHANCEMENT (non-breaking change which improves existing functionality)
NONE (if none of the other choices apply. Example, tooling, build system, CI, docs, etc.)

Changelog entry

Please put a one-line changelog entry below. This will be copied to the changelog file during the release process.

add single namespace to namespaces argument
Createo role rbac instead of ClusterRole
deploy prometheus operator, prometheus and application PODs in same namespace.

simonpasquier · 2023-08-25T14:32:14Z

pkg/alertmanager/operator.go

-			),
-			&v1.Namespace{}, nsResyncPeriod, cache.Indexers{},
-		)
+	if !operator.IsSingleNamespace(c.config.Namespaces) {


I think that the test should be more elaborated and dependent on the controller: e.g. Alertmanager can skip the namespace informers if AlertmanagerAllowList and AlertmanagerConfigAllowList have a single item (and different from v1.NamespaceAll).

For Prometheus and Thanos, the checks will be different.

ok, I would change it.

code changed

simonpasquier · 2023-08-25T14:37:31Z

pkg/prometheus/server/operator.go

@@ -969,11 +978,15 @@ func (c *Operator) handleConfigMapUpdate(old, cur interface{}) {
 }

 func (c *Operator) enqueueForPrometheusNamespace(nsName string) {
-	c.enqueueForNamespace(c.nsPromInf.GetStore(), nsName)
+	if !operator.IsSingleNamespace(c.config.Namespaces) {
+		c.enqueueForNamespace(c.nsPromInf.GetStore(), nsName)


We still need to run the reconciliation. I think that the change should rather be to enqueueForNamespace().

Yes, it woule be more simplefy, I would change it.

code changed

simonpasquier · 2023-08-25T14:37:49Z

pkg/prometheus/server/operator.go

@@ -521,7 +530,7 @@ func (c *Operator) Run(ctx context.Context) error {

 	c.addHandlers()

-	if c.kubeletSyncEnabled {
+	if c.kubeletSyncEnabled && !operator.IsSingleNamespace(c.config.Namespaces) {


why do we need this?

level=error ts=2023-09-01T11:39:50.560491434Z caller=operator.go:626 component=prometheusoperator msg="Syncing nodes into Endpoints object failed" err="listing nodes failed: nodes is forbidden: User "system:serviceaccount:rio:prometheus-operator" cannot list resource "nodes" in API group "" at the cluster scope"
That's the error log if disable this limit.

simonpasquier

Thinking more about it, I wonder if it wouldn't be simpler in terms of implementation to add a special "single namespace" lister/watcher. In other words, extend NewUnprivilegedNamespaceListWatchFromClient() to avoid the "get namespace" requests when a single namespace (not equal to all namespaces) is given.

Another thing to keep in mind: when running the operator in single namespace mode, the .spec.*NamespaceSelector fields become ineffective. When these selectors aren't nil, I wonder if the operator should report it via the "Reconciled=False" status configuration or if it should ignore the namespace selectors? I'd rather lean towards the first option as it's more explicit.

zhuangzhi-cc · 2023-09-04T12:13:00Z

Thinking more about it, I wonder if it wouldn't be simpler in terms of implementation to add a special "single namespace" lister/watcher. In other words, extend NewUnprivilegedNamespaceListWatchFromClient() to avoid the "get namespace" requests when a single namespace (not equal to all namespaces) is given.

Another thing to keep in mind: when running the operator in single namespace mode, the .spec.*NamespaceSelector fields become ineffective. When these selectors aren't nil, I wonder if the operator should report it via the "Reconciled=False" status configuration or if it should ignore the namespace selectors? I'd rather lean towards the first option as it's more explicit.

Do you means create a fake ListerWatcher in NewUnprivilegedNamespaceListWatchFromClient like:

func NewUnprivilegedNamespaceListWatchFromClient(
ctx context.Context,
l log.Logger,
c cache.Getter,
allowedNamespaces, deniedNamespaces map[string]struct{},
fieldSelector fields.Selector,
) cache.ListerWatcher {
......
if IsAllNamespaces(allowedNamespaces) {
....
}

  if IsSingleNamespace(allowedNamespaces) {

	listFunc := func(options metav1.ListOptions) (runtime.Object, error) {
		list := &v1.NamespaceList{}
		return list, nil
	}
	watchFunc := func(_ metav1.ListOptions) (watch.Interface, error) {
		// Since the client does not have Watch privileges, do not
		// actually watch anything. Use a watch.FakeWatcher here to
		// implement watch.Interface but not send any events.
		return watch.NewFake(), nil
	}
	return &cache.ListWatch{ListFunc: listFunc, WatchFunc: watchFunc}
 }

.....
}

simonpasquier · 2023-09-04T15:02:12Z

Yes something along those lines. It could even work when there are multiple namespaces in allowedNamespaces beccause the controllers only care about the namespace names after all.

func NewUnprivilegedNamespaceListWatchFromClient(
ctx context.Context,
l log.Logger,
c cache.Getter,
allowedNamespaces, deniedNamespaces map[string]struct{},
fieldSelector fields.Selector,
) cache.ListerWatcher {
...
if IsAllNamespaces(allowedNamespaces) {
   ....
}


listFunc := func(options metav1.ListOptions) (runtime.Object, error) {
                list := &v1.NamespaceList{}
                for name := range allowedNamespaces {
                        result := &v1.Namespace{}
                        list.Items = append(list.Items, &v1.Namespace{
                                ObjectMeta: metav1.ObjectMeta{Name: name},
                        })
                }
                return list, nil		
	})
}
watchFunc := func(_ metav1.ListOptions) (watch.Interface, error) {
		// Since the client does not have Watch privileges, do not
		// actually watch anything. Use a watch.FakeWatcher here to
		// implement watch.Interface but not send any events.
		return watch.NewFake(), nil
}

return &cache.ListWatch{ListFunc: listFunc, WatchFunc: watchFunc}
)

…s related logic create a fake watch list, and remove anyother change

simonpasquier · 2023-09-05T12:59:49Z

pkg/prometheus/server/operator.go

@@ -521,7 +523,7 @@ func (c *Operator) Run(ctx context.Context) error {

 	c.addHandlers()

-	if c.kubeletSyncEnabled {
+	if c.kubeletSyncEnabled && !operator.IsPrometheusInSingleNamespace(c.config.Namespaces) {


still not sure why this change is required :)

It's my misunderstand, I have remove the related check.

simonpasquier · 2023-09-05T14:04:06Z

pkg/operator/config.go

+	_, ok := ns[v1.NamespaceAll]
+	return !ok && len(ns) == 1


(nit)

Suggested change

_, ok := ns[v1.NamespaceAll]

return !ok && len(ns) == 1

_, allNamespaces := ns[v1.NamespaceAll]

return !allNamespaces && len(ns) == 1

this code has been removed.

simonpasquier · 2023-09-06T09:45:44Z

There's an issue with the testOperatorNSScope test (and more precisely the SingleNS subtest). The reason is that the namespace selection returns an empty list because the operator doesn't have access to the namespace's labels:

prometheus-operator/pkg/prometheus/server/rules.go

Lines 174 to 182 in 6d3714e

    
           ruleNamespaceSelector, err := metav1.LabelSelectorAsSelector(p.Spec.RuleNamespaceSelector) 
        
           if err != nil { 
        
           	return namespaces, errors.Wrap(err, "convert rule namespace label selector to selector") 
        
           } 
        
           namespaces, err = operator.ListMatchingNamespaces(ruleNamespaceSelector, c.nsMonInf) 
        
           if err != nil { 
        
           	return nil, err 
        
           }

We have the same "problem" in other places:

prometheus-operator/pkg/prometheus/resource_selector.go

Lines 78 to 91 in 6d3714e

    
           // If 'ServiceMonitorNamespaceSelector' is nil only check own namespace. 
        
           if cpf.ServiceMonitorNamespaceSelector == nil { 
        
           	namespaces = append(namespaces, objMeta.GetNamespace()) 
        
           } else { 
        
           	servMonNSSelector, err := metav1.LabelSelectorAsSelector(cpf.ServiceMonitorNamespaceSelector) 
        
           	if err != nil { 
        
           		return nil, err 
        
           	} 
        
           	namespaces, err = operator.ListMatchingNamespaces(servMonNSSelector, rs.namespaceInformers) 
        
           	if err != nil { 
        
           		return nil, err 
        
           	} 
        
           }

This is similar to what I said in a previous comment

Another thing to keep in mind: when running the operator in single namespace mode, the .spec.*NamespaceSelector fields become ineffective. When these selectors aren't nil, I wonder if the operator should report it via the "Reconciled=False" status configuration or if it should ignore the namespace selectors? I'd rather lean towards the first option as it's more explicit.

In retrospect, the controllers should rather default the namespace selectors to nil and report the situation via the Reconciled status condition with a dedicated reason and message:

  availableReplicas: 1
  conditions:
  - lastTransitionTime: "2023-09-05T12:01:49Z"
    observedGeneration: 5
    status: "True"
    type: Available
  - lastTransitionTime: "2023-09-05T12:01:49Z"
    observedGeneration: 5
    status: "True"
    type: Reconciled
    reason: "NamespaceSelectorsIgnored"
    message: "The resource specifies namespace selectors for the ServiceMonitor, PrometheusRule custom resources but the operator can only select resources from the 'foo' namespace."

simonpasquier · 2023-09-12T15:54:21Z

@zhuangzhi-cc thanks for keeping up the PR up-to-date! I'd advise to wait for #5898 which fixes a problem that's a bit different but after it (and with the refactoring), it should be even easier to implement single-namespace watcher.

simonpasquier · 2023-09-21T15:10:12Z

@zhuangzhi-cc now that #5898 and #5934 have merged, it's possible to run the operator watching a single namespace without ClusterRoleBinding. If you create a role binding to the prometheus-operator cluster role in the required namespace, it should just work. Does it satisfy your use case?

kc create rolebinding -n my_namespace prometheus-operator --clusterrole prometheus-operator --serviceaccount default:prometheus-operator
./operator --namespaces my_namespace --as system:serviceaccount:default:prometheus-operator

zhuangzhi-cc · 2023-09-22T07:13:55Z

@zhuangzhi-cc thanks for keeping up the PR up-to-date! I'd advise to wait for #5898 which fixes a problem that's a bit different but after it (and with the refactoring), it should be even easier to implement single-namespace watcher.

@simonpasquier I see that PR #5898 has alreay merged, is there any idea about single-namespace feature?

simonpasquier · 2023-09-22T10:28:13Z

@simonpasquier I see that PR #5898 has alreay merged, is there any idea about single-namespace feature?

@zhuangzhi-cc please check my comment right before (#5807 (comment)).

zhuangzhi-cc · 2023-09-22T13:31:33Z

@simonpasquier I see that PR #5898 has alreay merged, is there any idea about single-namespace feature?

@zhuangzhi-cc please check my comment right before (#5807 (comment)).

Sorry for my browser not sync the latest message because I am not connect to VPN :), I would make some test with the latest code. thanks a lot!

zhuangzhi-cc · 2023-09-22T13:32:16Z

@simonpasquier I see that PR #5898 has alreay merged, is there any idea about single-namespace feature?

@zhuangzhi-cc please check my comment right before (#5807 (comment)).

Sorry for my browser not sync the latest message because I am not connected to VPN :), I would make some test with the latest code. thanks a lot!

zhuangzhi-cc · 2023-10-09T07:00:34Z

this feature has supported at #5898 and #5934

simonpasquier · 2023-10-09T14:35:09Z

Thanks for confirming!

zhuangzhi-cc added 2 commits July 18, 2023 20:01

update

d9eaf9a

test operator in single namespace successful

c8a3666

zhuangzhi-cc requested a review from a team as a code owner August 14, 2023 12:49

pull-request-size bot added the size/L label Aug 14, 2023

zhuangzhi-cc changed the title ~~Singlens~~ deploy operator in single namespace without ClusterRole Aug 17, 2023

zhuangzhi-cc changed the title ~~deploy operator in single namespace without ClusterRole~~ enhancement: deploy operator in single namespace without ClusterRole Aug 17, 2023

simonpasquier reviewed Aug 25, 2023

View reviewed changes

fix review issues

e0e1600

pull-request-size bot added size/XL and removed size/L labels Aug 31, 2023

fix PR comments issues.

637a165

simonpasquier reviewed Sep 1, 2023

View reviewed changes

change NewUnprivilegedNamespaceListWatchFromClient logic add single n…

6e0389b

…s related logic create a fake watch list, and remove anyother change

pull-request-size bot added size/M and removed size/XL labels Sep 5, 2023

zhuangzhi-cc added 4 commits September 5, 2023 15:53

remove unused code

1c08396

recover some code

770466f

recover some code

491abdb

Merge branch 'main' into singlens

caa5858

simonpasquier reviewed Sep 5, 2023

View reviewed changes

remove unused code

1e806e8

pull-request-size bot added size/S and removed size/M labels Sep 6, 2023

recover code

536b594

debug e2e singlens error

6e205c3

pull-request-size bot added size/M and removed size/S labels Sep 12, 2023

debug e2e singlens error

9650da4

zhuangzhi-cc closed this Oct 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enhancement: deploy operator in single namespace without `ClusterRole` #5807

enhancement: deploy operator in single namespace without `ClusterRole` #5807

zhuangzhi-cc commented Aug 14, 2023 •

edited

simonpasquier Aug 25, 2023

zhuangzhi-cc Aug 29, 2023

zhuangzhi-cc Sep 1, 2023

simonpasquier Aug 25, 2023

zhuangzhi-cc Aug 29, 2023

zhuangzhi-cc Sep 1, 2023

simonpasquier Aug 25, 2023

zhuangzhi-cc Sep 1, 2023

simonpasquier left a comment

zhuangzhi-cc commented Sep 4, 2023

simonpasquier commented Sep 4, 2023

simonpasquier Sep 5, 2023

zhuangzhi-cc Sep 6, 2023 •

edited

simonpasquier Sep 5, 2023

zhuangzhi-cc Sep 6, 2023

simonpasquier commented Sep 6, 2023

simonpasquier commented Sep 12, 2023

simonpasquier commented Sep 21, 2023

zhuangzhi-cc commented Sep 22, 2023

simonpasquier commented Sep 22, 2023

zhuangzhi-cc commented Sep 22, 2023

zhuangzhi-cc commented Sep 22, 2023

zhuangzhi-cc commented Oct 9, 2023

simonpasquier commented Oct 9, 2023

enhancement: deploy operator in single namespace without ClusterRole #5807

enhancement: deploy operator in single namespace without ClusterRole #5807

Conversation

zhuangzhi-cc commented Aug 14, 2023 • edited

Description

Type of change

Changelog entry

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonpasquier left a comment

Choose a reason for hiding this comment

zhuangzhi-cc commented Sep 4, 2023

simonpasquier commented Sep 4, 2023

Choose a reason for hiding this comment

zhuangzhi-cc Sep 6, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonpasquier commented Sep 6, 2023

simonpasquier commented Sep 12, 2023

simonpasquier commented Sep 21, 2023

zhuangzhi-cc commented Sep 22, 2023

simonpasquier commented Sep 22, 2023

zhuangzhi-cc commented Sep 22, 2023

zhuangzhi-cc commented Sep 22, 2023

zhuangzhi-cc commented Oct 9, 2023

simonpasquier commented Oct 9, 2023

enhancement: deploy operator in single namespace without `ClusterRole` #5807

enhancement: deploy operator in single namespace without `ClusterRole` #5807

zhuangzhi-cc commented Aug 14, 2023 •

edited

zhuangzhi-cc Sep 6, 2023 •

edited