OCPBUGS-17035: fix KRP permissions for Thanos Querier

fix KRP configuration for thanos-querier in order to enforce restricted
access only to entities allowed to [http-verb] for pods.metrics.k8s.io

Signed-off-by: Pranshu Srivastava <[email protected]>

Author: rexagod

assets/thanos-querier/deployment.yaml

@@ -140,6 +140,7 @@ spec:
         - mountPath: /etc/proxy/secrets
           name: secret-thanos-querier-oauth-cookie
       - args:
+        - --v=12
         - --secure-listen-address=0.0.0.0:9092
         - --upstream=http://127.0.0.1:9095
         - --config-file=/etc/kube-rbac-proxy/config.yaml

assets/thanos-querier/kube-rbac-proxy-secret.yaml

@@ -15,7 +15,8 @@ stringData:
   config.yaml: |-
     "authorization":
       "resourceAttributes":
-        "apiVersion": "metrics.k8s.io/v1beta1"
+        "apiGroup": "metrics.k8s.io"
+        "apiVersion": "v1beta1"
         "namespace": "{{ .Value }}"
         "resource": "pods"
       "rewrites":

jsonnet/components/thanos-querier.libsonnet

@@ -144,7 +144,8 @@ function(params)
               },
             },
             resourceAttributes: {
-              apiVersion: 'metrics.k8s.io/v1beta1',
+              apiVersion: 'v1beta1',
+              apiGroup: 'metrics.k8s.io',
               resource: 'pods',
               namespace: '{{ .Value }}',
             },
@@ -465,6 +466,7 @@ function(params)
                   },
                 ],
                 args: [
+                  '--v=12',
                   '--secure-listen-address=0.0.0.0:9092',
                   '--upstream=http://127.0.0.1:9095',
                   '--config-file=/etc/kube-rbac-proxy/config.yaml',

test/e2e/framework/framework.go

@@ -82,7 +82,7 @@ type Framework struct {
 
 // New returns a new cluster monitoring operator end-to-end test framework and
 // triggers all the setup logic.
-func New(kubeConfigPath string) (*Framework, cleanUpFunc, error) {
+func New(kubeConfigPath string) (*Framework, CleanUpFunc, error) {
 	ctx := context.Background()
 	config, err := clientcmd.BuildConfigFromFlags("", kubeConfigPath)
 	if err != nil {
@@ -218,11 +218,11 @@ func New(kubeConfigPath string) (*Framework, cleanUpFunc, error) {
 	return f, cleanUp, nil
 }
 
-type cleanUpFunc func() error
+type CleanUpFunc func() error
 
 // setup creates everything necessary to use the test framework.
-func (f *Framework) setup() (cleanUpFunc, error) {
-	cleanUpFuncs := []cleanUpFunc{}
+func (f *Framework) setup() (CleanUpFunc, error) {
+	cleanUpFuncs := []CleanUpFunc{}
 
 	cf, err := f.CreateServiceAccount(f.Ns, E2eServiceAccount)
 	if err != nil {
@@ -273,7 +273,7 @@ func (f *Framework) setup() (cleanUpFunc, error) {
 	}, nil
 }
 
-func (f *Framework) CreateServiceAccount(namespace, serviceAccount string) (cleanUpFunc, error) {
+func (f *Framework) CreateServiceAccount(namespace, serviceAccount string) (CleanUpFunc, error) {
 	ctx := context.Background()
 	sa := &v1.ServiceAccount{
 		ObjectMeta: metav1.ObjectMeta{
@@ -334,7 +334,7 @@ func (f *Framework) GetLogs(namespace string, podName, containerName string) (st
 	return string(logs), err
 }
 
-func (f *Framework) CreateClusterRoleBinding(namespace, serviceAccount, clusterRole string) (cleanUpFunc, error) {
+func (f *Framework) CreateClusterRoleBinding(namespace, serviceAccount, clusterRole string) (CleanUpFunc, error) {
 	ctx := context.Background()
 	clusterRoleBinding := &rbacv1.ClusterRoleBinding{
 		ObjectMeta: metav1.ObjectMeta{
@@ -367,7 +367,87 @@ func (f *Framework) CreateClusterRoleBinding(namespace, serviceAccount, clusterR
 	}, nil
 }
 
-func (f *Framework) CreateRoleBindingFromClusterRole(namespace, serviceAccount, clusterRole string) (cleanUpFunc, error) {
+func (f *Framework) CreateRoleBindingFromTypedRole(namespace, serviceAccount string, typedRole *rbacv1.Role) (CleanUpFunc, error) {
+	ctx := context.Background()
+
+	role, err := f.KubeClient.RbacV1().Roles(namespace).Create(ctx, typedRole, metav1.CreateOptions{})
+	if err != nil {
+		if apierrors.IsAlreadyExists(err) {
+			return nil, errors.Errorf("role %s already exists", typedRole.Name)
+		}
+		return nil, err
+	}
+
+	roleBinding := &rbacv1.RoleBinding{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: fmt.Sprintf("%s-%s", serviceAccount, typedRole.Name),
+			Labels: map[string]string{
+				E2eTestLabelName: E2eTestLabelValue,
+			},
+		},
+		Subjects: []rbacv1.Subject{
+			{
+				Kind:      "ServiceAccount",
+				Name:      serviceAccount,
+				Namespace: namespace,
+			},
+		},
+		RoleRef: rbacv1.RoleRef{
+			Kind:     "Role",
+			Name:     role.Name,
+			APIGroup: "rbac.authorization.k8s.io",
+		},
+	}
+
+	roleBinding, err = f.KubeClient.RbacV1().RoleBindings(namespace).Create(ctx, roleBinding, metav1.CreateOptions{})
+	if err != nil {
+		if apierrors.IsAlreadyExists(err) {
+			return nil, errors.Errorf("%s %s already exists", roleBinding.GroupVersionKind(), roleBinding.Name)
+		}
+		return nil, err
+	}
+
+	// Wait for the role and role binding to be ready.
+	err = Poll(10*time.Second, time.Minute, func() error {
+		_, err := f.KubeClient.RbacV1().Roles(namespace).Get(ctx, role.Name, metav1.GetOptions{})
+		if err != nil {
+			return err
+		}
+		_, err = f.KubeClient.RbacV1().RoleBindings(namespace).Get(ctx, roleBinding.Name, metav1.GetOptions{})
+		if err != nil {
+			return err
+		}
+		return nil
+	})
+
+	return func() error {
+		err := f.KubeClient.RbacV1().Roles(namespace).Delete(ctx, role.Name, metav1.DeleteOptions{})
+		if err != nil {
+			return err
+		}
+		err = f.KubeClient.RbacV1().RoleBindings(namespace).Delete(ctx, roleBinding.Name, metav1.DeleteOptions{})
+		if err != nil {
+			return err
+		}
+
+		// Wait for the role and role binding to be deleted.
+		err = Poll(10*time.Second, time.Minute, func() error {
+			_, err := f.KubeClient.RbacV1().Roles(namespace).Get(ctx, role.Name, metav1.GetOptions{})
+			if err == nil {
+				return errors.Errorf("%s %s still exists", role.GroupVersionKind(), role.Name)
+			}
+			_, err = f.KubeClient.RbacV1().RoleBindings(namespace).Get(ctx, roleBinding.Name, metav1.GetOptions{})
+			if err == nil {
+				return errors.Errorf("%s %s still exists", roleBinding.GroupVersionKind(), roleBinding.Name)
+			}
+			return nil
+		})
+
+		return err
+	}, nil
+}
+
+func (f *Framework) CreateRoleBindingFromClusterRole(namespace, serviceAccount, clusterRole string) (CleanUpFunc, error) {
 	ctx := context.Background()
 	roleBinding := &rbacv1.RoleBinding{
 		ObjectMeta: metav1.ObjectMeta{
@@ -400,7 +480,7 @@ func (f *Framework) CreateRoleBindingFromClusterRole(namespace, serviceAccount,
 	}, nil
 }
 
-func (f *Framework) CreateRoleBindingFromRole(namespace, serviceAccount, role string) (cleanUpFunc, error) {
+func (f *Framework) CreateRoleBindingFromRole(namespace, serviceAccount, role string) (CleanUpFunc, error) {
 	ctx := context.Background()
 	roleBinding := &rbacv1.RoleBinding{
 		ObjectMeta: metav1.ObjectMeta{

test/e2e/user_workload_monitoring_test.go

@@ -26,15 +26,17 @@ import (
 	"time"
 
 	"github.com/Jeffail/gabs"
-	"github.com/openshift/cluster-monitoring-operator/test/e2e/framework"
 	"github.com/pkg/errors"
 	monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1"
 	appsv1 "k8s.io/api/apps/v1"
 	v1 "k8s.io/api/core/v1"
+	rbacv1 "k8s.io/api/rbac/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/client-go/util/cert"
+
+	"github.com/openshift/cluster-monitoring-operator/test/e2e/framework"
 )
 
 type scenario struct {
@@ -525,16 +527,10 @@ func assertTenancyForMetrics(t *testing.T) {
 
 	err := framework.Poll(2*time.Second, 10*time.Second, func() error {
 		_, err := f.CreateServiceAccount(userWorkloadTestNs, testAccount)
-		return err
-	})
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	// Grant enough permissions to the account so it can read metrics.
-	err = framework.Poll(2*time.Second, 10*time.Second, func() error {
-		_, err = f.CreateRoleBindingFromClusterRole(userWorkloadTestNs, testAccount, "admin")
-		return err
+		if !apierrors.IsAlreadyExists(err) {
+			return err
+		}
+		return nil
 	})
 	if err != nil {
 		t.Fatal(err)
@@ -576,6 +572,32 @@ func assertTenancyForMetrics(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			t.Logf("Running query %q", tc.query)
 
+			var cleanupFn func() error
+			// Grant just-enough permissions to the account so it can read metrics.
+			err = framework.Poll(2*time.Second, 10*time.Second, func() error {
+				cleanupFn, err = f.CreateRoleBindingFromTypedRole(userWorkloadTestNs, testAccount, &rbacv1.Role{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "tenancy-test-metrics",
+					},
+					Rules: []rbacv1.PolicyRule{
+						{
+							APIGroups: []string{"metrics.k8s.io"},
+							Resources: []string{"pods"},
+							Verbs:     []string{"get"},
+						},
+					},
+				})
+				return err
+			})
+			if err != nil {
+				t.Fatal(err)
+			}
+			defer func() {
+				if err := cleanupFn(); err != nil {
+					t.Fatal(err)
+				}
+			}()
+
 			err = framework.Poll(5*time.Second, time.Minute, func() error {
 				// The tenancy port (9092) is only exposed in-cluster so we need to use
 				// port forwarding to access kube-rbac-proxy.
@@ -681,6 +703,123 @@ func assertTenancyForMetrics(t *testing.T) {
 	if err != nil {
 		t.Fatalf("the account has access to the rules endpoint of Thanos querier: %v", err)
 	}
+
+	for _, tc := range []struct {
+		// NOTE: role verbs need to factor in the permissions metrics-server requires for the respective use-case.
+		role               rbacv1.Role
+		expectNotOKOnQuery bool
+		desc               string
+		method             string
+	}{
+
+		{
+			role: rbacv1.Role{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "tenancy-test-metrics",
+				},
+				Rules: []rbacv1.PolicyRule{
+					{
+						APIGroups: []string{"metrics.k8s.io"},
+						Resources: []string{"pods"},
+						Verbs:     []string{"get"},
+					},
+				},
+			},
+			method:             http.MethodPost,
+			expectNotOKOnQuery: true,
+			desc:               "should disallow POST queries to the endpoint for SA with no create permission",
+		},
+		{
+			role: rbacv1.Role{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "tenancy-test-metrics",
+				},
+				Rules: []rbacv1.PolicyRule{
+					{
+						APIGroups: []string{"metrics.k8s.io"},
+						Resources: []string{"pods"},
+						Verbs:     []string{"get"},
+					},
+				},
+			},
+			method: http.MethodGet,
+			desc:   "should allow GET queries to the endpoint for SA with get permission",
+		},
+		{
+			role: rbacv1.Role{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "tenancy-test-metrics",
+				},
+				Rules: []rbacv1.PolicyRule{
+					{
+						APIGroups: []string{"metrics.k8s.io"},
+						Resources: []string{"pods"},
+						Verbs:     []string{"get", "create"},
+					},
+				},
+			},
+			method: http.MethodPost,
+			desc:   "should allow POST queries to the endpoint for SA with get and create permission",
+		},
+	} {
+		t.Run(tc.desc, func(t *testing.T) {
+			var cleanupFn framework.CleanUpFunc
+
+			// Create a role binding for the test SA.
+			err = framework.Poll(time.Second, time.Minute, func() error {
+				cleanupFn, err = f.CreateRoleBindingFromTypedRole(userWorkloadTestNs, testAccount, &tc.role)
+				return err
+			})
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			// Remove associated artifacts.
+			defer func() {
+				if err := cleanupFn(); err != nil {
+					t.Fatal(err)
+				}
+			}()
+
+			// Forward the tenancy port.
+			host, cleanUp, err := f.ForwardPort(t, f.Ns, "thanos-querier", 9092)
+			if err != nil {
+				t.Fatal(err)
+			}
+			defer cleanUp()
+
+			// Create a Prometheus client with the test SA token.
+			client := framework.NewPrometheusClient(
+				host,
+				token,
+				&framework.QueryParameterInjector{
+					Name:  "namespace",
+					Value: userWorkloadTestNs,
+				},
+			)
+
+			resp, err := client.Do(tc.method, "/api/v1/query?namespace="+userWorkloadTestNs+"&query=up", nil)
+			if err != nil {
+				t.Fatal(err)
+			}
+			defer resp.Body.Close()
+			// Body: {"status":"success","data":{"resultType":"vector","result":[{"metric":{"__name__":"up",...},"value":[1695582946.784,"1"]}]}}
+			respBodyBytes, err := io.ReadAll(resp.Body)
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			if tc.expectNotOKOnQuery {
+				if resp.StatusCode == http.StatusOK {
+					t.Fatal("expected request to be rejected, but succeeded")
+				}
+			} else {
+				if resp.StatusCode != http.StatusOK {
+					t.Fatalf("expected request to be accepted, but got status code %d (%s)", resp.StatusCode, respBodyBytes)
+				}
+			}
+		})
+	}
 }
 
 // assertTenancyForRules ensures that a tenant can access rules from her namespace (and only from this one).