fix: reconcile on namespace changes #5898

simonpasquier · 2023-09-07T12:32:56Z

Description

When the operator was configured to select only a limited number of
namespaces, it would not watch for namespace changes. It means that the
operator may not reconcile when a namespace label is added/removed
(affecting which objects should be selected or not).

This change enables the operator to use a privileged namespace
lister/watcher whenever the service account has the needed permissions.

IMPORTANT: it also requires Kubernetes >= 1.22 to be effective but the
operator will degrade to the less efficient implementation if this condition isn't met.

xref #3847 (one more PR required to fix it completely).

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.

Reconcile monitoring resources on namespace updates.

This change is a pre-requisite for prometheus-operator#5898. Signed-off-by: Simon Pasquier <spasquie@redhat.com>

simonpasquier · 2023-09-14T13:30:54Z

test/e2e/prometheus_instance_namespaces_test.go

@@ -302,11 +331,11 @@ func testPrometheusInstanceNamespacesAllowList(t *testing.T) {
 		// Remove the selecting label on the "allowed" namespace and check that
 		// the target is removed.
 		// See https://github.com/prometheus-operator/prometheus-operator/issues/3847
-		//if err := testFramework.RemoveLabelsFromNamespace(framework.KubeClient, allowedNs, "monitored"); err != nil {
+		//if err := framework.RemoveLabelsFromNamespace(context.Background(), allowedNs, "monitored"); err != nil {


this part is still commented because the service account isn't configured with list/watch permissions on namespaces. I have another change in flight that would fix it by implementing a poller based watcher but to keep this PR reviewable, I'll send as a follow-up.

simonpasquier · 2023-09-14T14:50:09Z

cmd/operator/main.go

@@ -303,6 +303,15 @@ func run() int {
 		return 1
 	}

+	kubernetesVersion, err := kclient.Discovery().ServerVersion()


Instead of repeating the same check in each controller, let's do it once on startup.

ArthurSens

Sorry for taking this long to review, the subject here is a bit out of my comfort zone 😬 I'm still going through the codebase and trying to understand everything.

A few doubts came up while reading the PR

ArthurSens · 2023-09-19T20:34:47Z

pkg/listwatch/listwatch.go


-		return cache.NewFilteredListWatchFromClient(c, "namespaces", metav1.NamespaceAll, tweak)
+		level.Debug(l).Log("msg", "using privileged namespace lister/watcher")


this debug log seems out of place 🤔

hmm not sure to understand but I can remove all the "using (un)privileged namespace lister/watcher" debug logs as they were more for me to check the code path.

It's fine to keep it! I missed the return literally in the next line and thought you left duplicated logs 😅

ArthurSens · 2023-09-19T20:44:14Z

pkg/listwatch/listwatch.go

-				level.Info(l).Log("msg", "namespace not found", "namespace", name)
-				continue
-			}
+			result, err := corev1Client.Namespaces().Get(ctx, name, metav1.GetOptions{})


Should we put this inside an if?

if getAllowed { result, err := corev1Client.Namespaces().Get(ctx, name, metav1.GetOptions{}) . . . }

I kept the existing behavior to minimize the changes. The SA might have get permissions on a subset of the configured namespaces but getAllowed will be false.

ArthurSens · 2023-09-19T20:47:13Z

pkg/listwatch/listwatch.go

+			err = fmt.Errorf("%w: %w", err, r)
+		}
+
+		level.Warn(l).Log("msg", "the operator lacks required permissions which may result in degraded functionalities", "err", err)
 	}

 	listFunc := func(options metav1.ListOptions) (runtime.Object, error) {


Suggested change

listFunc := func(options metav1.ListOptions) (runtime.Object, error) {

listFunc := func(_ metav1.ListOptions) (runtime.Object, error) {

Is it intended to not use the options provided here?

yes it's intended and the PR didn't change anything in that regard.

simonpasquier

Thanks for the review @ArthurSens. I understand that the change isn't easy to review, fortunately we have quite good e2e test coverage which helped me find bugs in the first iterations :)

simonpasquier · 2023-09-20T09:41:58Z

pkg/listwatch/listwatch.go


-		return cache.NewFilteredListWatchFromClient(c, "namespaces", metav1.NamespaceAll, tweak)
+		level.Debug(l).Log("msg", "using privileged namespace lister/watcher")


hmm not sure to understand but I can remove all the "using (un)privileged namespace lister/watcher" debug logs as they were more for me to check the code path.

simonpasquier · 2023-09-20T09:43:50Z

pkg/listwatch/listwatch.go

+			err = fmt.Errorf("%w: %w", err, r)
+		}
+
+		level.Warn(l).Log("msg", "the operator lacks required permissions which may result in degraded functionalities", "err", err)
 	}

 	listFunc := func(options metav1.ListOptions) (runtime.Object, error) {


yes it's intended and the PR didn't change anything in that regard.

simonpasquier · 2023-09-20T09:46:39Z

pkg/listwatch/listwatch.go

-				level.Info(l).Log("msg", "namespace not found", "namespace", name)
-				continue
-			}
+			result, err := corev1Client.Namespaces().Get(ctx, name, metav1.GetOptions{})


I kept the existing behavior to minimize the changes. The SA might have get permissions on a subset of the configured namespaces but getAllowed will be false.

simonpasquier · 2023-09-20T11:27:16Z

@ArthurSens in case you missed my earlier comment: 5ada40d is the next step but I didn't want to increase the scope of this PR even more.

ArthurSens

Alright, LGTM!

Thanks for working on this, reviewing it also helped me understand better what kubebuilder usually abstracts away from other projects I work with :P

ArthurSens · 2023-09-20T19:37:19Z

pkg/listwatch/listwatch.go


-		return cache.NewFilteredListWatchFromClient(c, "namespaces", metav1.NamespaceAll, tweak)
+		level.Debug(l).Log("msg", "using privileged namespace lister/watcher")


It's fine to keep it! I missed the return literally in the next line and thought you left duplicated logs 😅

ArthurSens · 2023-09-20T19:47:30Z

test/e2e/alertmanager_instance_namespaces_test.go

-	am = framework.MakeBasicAlertmanager(instanceNs, "instance", 3)
+	am = framework.MakeBasicAlertmanager(instanceNs, "instance", 2)


Any special reason to reduce the number of replicas?

I assume it is just because the test is valid for both 3 or 2 replicas and 2 replicas is just faster, but I wanted to ask anyway

yes, the test doesn't care if it's 3, 2 or 1 replicas. But I'll revert it to avoid unrelated changes. We can have a look at all e2e tests globally and decide whether they really need multi replicas: in most cases, I think that the answer is no and switching to single replicas might save a few resources.

ArthurSens · 2023-09-20T20:12:31Z

@ArthurSens in case you missed my earlier comment: 5ada40d is the next step but I didn't want to increase the scope of this PR even more.

I've just seen it and looks 👌 !

When the operator was configured to select only a limited number of namespaces, it would not watch for namespace changes. It means that the operator may not reconcile when a namespace label is added/removed (affecting which objects should be selected or not). This change enables the operator to use a privileged namespace lister/watcher whenever the service account has the needed permissions. **IMPORTANT:** it also requires Kubernetes >= 1.22 to be effective but the operator will degrade to the suboptimal implementation without watch if this condition isn't met. Closes prometheus-operator#3847 Signed-off-by: Simon Pasquier <spasquie@redhat.com>

pull-request-size bot added the size/L label Sep 7, 2023

simonpasquier force-pushed the improve-ns-listerwatcher branch from d8e91df to 3a7c436 Compare September 7, 2023 14:03

pull-request-size bot added size/XL and removed size/L labels Sep 7, 2023

simonpasquier force-pushed the improve-ns-listerwatcher branch 4 times, most recently from ae11f2a to fe98cee Compare September 11, 2023 07:27

simonpasquier added a commit to simonpasquier/prometheus-operator that referenced this pull request Sep 12, 2023

chore: refactor CRD and RBAC checks

4adc7cf

This change is a pre-requisite for prometheus-operator#5898. Signed-off-by: Simon Pasquier <spasquie@redhat.com>

simonpasquier mentioned this pull request Sep 12, 2023

enhancement: deploy operator in single namespace without ClusterRole #5807

Closed

5 tasks

simonpasquier added a commit to simonpasquier/prometheus-operator that referenced this pull request Sep 13, 2023

chore: refactor CRD and RBAC checks

52870d0

This change is a pre-requisite for prometheus-operator#5898. Signed-off-by: Simon Pasquier <spasquie@redhat.com>

simonpasquier mentioned this pull request Sep 13, 2023

chore: refactor CRD and RBAC checks #5910

Merged

5 tasks

simonpasquier added a commit to simonpasquier/prometheus-operator that referenced this pull request Sep 13, 2023

chore: refactor CRD and RBAC checks

56ebd4c

This change is a pre-requisite for prometheus-operator#5898. Signed-off-by: Simon Pasquier <spasquie@redhat.com>

simonpasquier added a commit to simonpasquier/prometheus-operator that referenced this pull request Sep 13, 2023

chore: refactor CRD and RBAC checks

521b1ad

This change is a pre-requisite for prometheus-operator#5898. Signed-off-by: Simon Pasquier <spasquie@redhat.com>

simonpasquier force-pushed the improve-ns-listerwatcher branch 3 times, most recently from 467307b to d7d5c85 Compare September 14, 2023 11:56

simonpasquier marked this pull request as ready for review September 14, 2023 13:24

simonpasquier requested a review from a team as a code owner September 14, 2023 13:24

simonpasquier force-pushed the improve-ns-listerwatcher branch from d7d5c85 to bd2b1bc Compare September 14, 2023 13:29

simonpasquier commented Sep 14, 2023

View reviewed changes

simonpasquier force-pushed the improve-ns-listerwatcher branch from bd2b1bc to 889e3db Compare September 14, 2023 13:31

simonpasquier requested review from philipgough and ArthurSens September 14, 2023 14:49

simonpasquier commented Sep 14, 2023

View reviewed changes

ArthurSens reviewed Sep 19, 2023

View reviewed changes

simonpasquier commented Sep 20, 2023

View reviewed changes

ArthurSens approved these changes Sep 20, 2023

View reviewed changes

simonpasquier force-pushed the improve-ns-listerwatcher branch from 889e3db to e274af7 Compare September 21, 2023 08:14

simonpasquier enabled auto-merge September 21, 2023 08:15

simonpasquier merged commit 561eb52 into prometheus-operator:main Sep 21, 2023
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: reconcile on namespace changes #5898

fix: reconcile on namespace changes #5898

simonpasquier commented Sep 7, 2023 •

edited

simonpasquier Sep 14, 2023

simonpasquier Sep 19, 2023

simonpasquier Sep 14, 2023

ArthurSens left a comment

ArthurSens Sep 19, 2023

simonpasquier Sep 20, 2023

ArthurSens Sep 20, 2023

ArthurSens Sep 19, 2023

simonpasquier Sep 20, 2023

ArthurSens Sep 19, 2023

simonpasquier Sep 20, 2023

simonpasquier left a comment

simonpasquier Sep 20, 2023

simonpasquier Sep 20, 2023

simonpasquier Sep 20, 2023

simonpasquier commented Sep 20, 2023

ArthurSens left a comment

ArthurSens Sep 20, 2023

ArthurSens Sep 20, 2023

simonpasquier Sep 21, 2023

ArthurSens commented Sep 20, 2023 •

edited


		return cache.NewFilteredListWatchFromClient(c, "namespaces", metav1.NamespaceAll, tweak)
		level.Debug(l).Log("msg", "using privileged namespace lister/watcher")

	listFunc := func(options metav1.ListOptions) (runtime.Object, error) {
	listFunc := func(_ metav1.ListOptions) (runtime.Object, error) {

		am = framework.MakeBasicAlertmanager(instanceNs, "instance", 3)
		am = framework.MakeBasicAlertmanager(instanceNs, "instance", 2)

fix: reconcile on namespace changes #5898

fix: reconcile on namespace changes #5898

Conversation

simonpasquier commented Sep 7, 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

ArthurSens left a 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

Choose a reason for hiding this comment

simonpasquier left a 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 commented Sep 20, 2023

ArthurSens left a 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

ArthurSens commented Sep 20, 2023 • edited

simonpasquier commented Sep 7, 2023 •

edited

ArthurSens commented Sep 20, 2023 •

edited