diff --git a/internal/managementrouter/router.go b/internal/managementrouter/router.go index 34c468ba6..43a8ad352 100644 --- a/internal/managementrouter/router.go +++ b/internal/managementrouter/router.go @@ -11,6 +11,7 @@ import ( "github.com/gorilla/mux" "github.com/sirupsen/logrus" + apierrors "k8s.io/apimachinery/pkg/api/errors" "github.com/openshift/monitoring-plugin/pkg/k8s" "github.com/openshift/monitoring-plugin/pkg/management" @@ -62,6 +63,7 @@ func authMiddleware(next http.Handler) http.Handler { }) } +// writeError sends a JSON {"error": message} response with the given status code. func writeError(w http.ResponseWriter, statusCode int, message string) { w.Header().Set("Content-Type", "application/json") w.WriteHeader(statusCode) @@ -75,28 +77,37 @@ func writeError(w http.ResponseWriter, statusCode int, message string) { } } +// handleError maps err to an HTTP status via parseError and writes the response. func handleError(w http.ResponseWriter, err error) { status, message := parseError(err) writeError(w, status, message) } +// parseError inspects err and returns a (statusCode, userMessage) pair. +// Kubernetes auth errors are checked first to prevent information leakage; +// domain errors are then mapped to 4xx codes. func parseError(err error) (int, string) { - var nf *management.NotFoundError - if errors.As(err, &nf) { + var ( + notFound *management.NotFoundError + validation *management.ValidationError + notAllowed *management.NotAllowedError + conflict *management.ConflictError + ) + switch { + case apierrors.IsUnauthorized(err): + return http.StatusUnauthorized, "authentication failed" + case apierrors.IsForbidden(err): + return http.StatusForbidden, "insufficient permissions" + case errors.As(err, ¬Found): return http.StatusNotFound, err.Error() - } - var ve *management.ValidationError - if errors.As(err, &ve) { + case errors.As(err, &validation): return http.StatusBadRequest, err.Error() - } - var na *management.NotAllowedError - if errors.As(err, &na) { + case errors.As(err, ¬Allowed): return http.StatusMethodNotAllowed, err.Error() - } - var ce *management.ConflictError - if errors.As(err, &ce) { + case errors.As(err, &conflict): return http.StatusConflict, err.Error() + default: + log.WithError(err).Error("unexpected management API error") + return http.StatusInternalServerError, "An unexpected error occurred" } - log.WithError(err).Error("unexpected management API error") - return http.StatusInternalServerError, "An unexpected error occurred" } diff --git a/internal/managementrouter/router_test.go b/internal/managementrouter/router_test.go new file mode 100644 index 000000000..272a973d2 --- /dev/null +++ b/internal/managementrouter/router_test.go @@ -0,0 +1,88 @@ +package managementrouter + +import ( + "fmt" + "net/http" + "testing" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime/schema" + + "github.com/openshift/monitoring-plugin/pkg/management" +) + +func TestParseError(t *testing.T) { + tests := []struct { + name string + err error + expectedStatus int + expectedMsg string + }{ + { + name: "NotFoundError", + err: &management.NotFoundError{Resource: "AlertRule", Id: "abc"}, + expectedStatus: http.StatusNotFound, + }, + { + name: "NotFoundError wrapped", + err: fmt.Errorf("lookup failed: %w", &management.NotFoundError{Resource: "AlertRule", Id: "abc"}), + expectedStatus: http.StatusNotFound, + }, + { + name: "ValidationError", + err: &management.ValidationError{Message: "bad input"}, + expectedStatus: http.StatusBadRequest, + }, + { + name: "NotAllowedError", + err: &management.NotAllowedError{Message: "not allowed"}, + expectedStatus: http.StatusMethodNotAllowed, + }, + { + name: "ConflictError", + err: &management.ConflictError{Message: "conflict"}, + expectedStatus: http.StatusConflict, + }, + { + name: "Kubernetes Forbidden", + err: apierrors.NewForbidden(schema.GroupResource{ + Group: "monitoring.coreos.com", Resource: "prometheusrules", + }, "test-pr", fmt.Errorf("access denied")), + expectedStatus: http.StatusForbidden, + expectedMsg: "insufficient permissions", + }, + { + name: "Kubernetes Forbidden wrapped", + err: fmt.Errorf("failed to get PrometheusRule: %w", + apierrors.NewForbidden(schema.GroupResource{ + Group: "monitoring.coreos.com", Resource: "prometheusrules", + }, "test-pr", fmt.Errorf("access denied"))), + expectedStatus: http.StatusForbidden, + expectedMsg: "insufficient permissions", + }, + { + name: "Kubernetes Unauthorized", + err: apierrors.NewUnauthorized("token expired"), + expectedStatus: http.StatusUnauthorized, + expectedMsg: "authentication failed", + }, + { + name: "unknown error", + err: fmt.Errorf("something unexpected"), + expectedStatus: http.StatusInternalServerError, + expectedMsg: "An unexpected error occurred", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + status, msg := parseError(tt.err) + if status != tt.expectedStatus { + t.Errorf("expected status %d, got %d", tt.expectedStatus, status) + } + if tt.expectedMsg != "" && msg != tt.expectedMsg { + t.Errorf("expected message %q, got %q", tt.expectedMsg, msg) + } + }) + } +} diff --git a/pkg/k8s/user_scoped_client.go b/pkg/k8s/user_scoped_client.go index 448ba4d8e..d180945d5 100644 --- a/pkg/k8s/user_scoped_client.go +++ b/pkg/k8s/user_scoped_client.go @@ -13,14 +13,21 @@ type userScopedClientsets struct { osmV1 *osmv1client.Clientset } +// buildUserScopedConfig creates a rest.Config that authenticates exclusively +// with the given bearer token. It uses AnonymousClientConfig to strip all +// existing auth (certs, basic auth, auth/exec providers, impersonation) while +// preserving the server connection settings (host, TLS CA, proxy). +func buildUserScopedConfig(baseConfig *rest.Config, userToken string) *rest.Config { + cfg := rest.AnonymousClientConfig(baseConfig) + cfg.BearerToken = userToken + return cfg +} + // newUserScopedClientsets creates clientsets that carry the supplied bearer // token so that Kubernetes RBAC is enforced for the requesting user on all // mutating API calls. func newUserScopedClientsets(baseConfig *rest.Config, userToken string) (*userScopedClientsets, error) { - cfg := rest.CopyConfig(baseConfig) - // Override any SA token loaded from the file system with the user's token. - cfg.BearerToken = userToken - cfg.BearerTokenFile = "" + cfg := buildUserScopedConfig(baseConfig, userToken) monClient, err := monitoringv1client.NewForConfig(cfg) if err != nil { diff --git a/pkg/k8s/user_scoped_client_test.go b/pkg/k8s/user_scoped_client_test.go new file mode 100644 index 000000000..f10723f5e --- /dev/null +++ b/pkg/k8s/user_scoped_client_test.go @@ -0,0 +1,71 @@ +package k8s + +import ( + "testing" + + "k8s.io/client-go/rest" +) + +func TestBuildUserScopedConfig(t *testing.T) { + base := &rest.Config{ + Host: "https://api.example.com:6443", + BearerToken: "sa-token", + BearerTokenFile: "/var/run/secrets/kubernetes.io/serviceaccount/token", + Impersonate: rest.ImpersonationConfig{ + UserName: "system:admin", + Groups: []string{"system:masters"}, + }, + TLSClientConfig: rest.TLSClientConfig{ + Insecure: true, + CertData: []byte("admin-cert"), + KeyData: []byte("admin-key"), + CertFile: "/path/to/cert", + KeyFile: "/path/to/key", + }, + } + + cfg := buildUserScopedConfig(base, "user-token") + + // Derived config uses the user token exclusively. + if cfg.BearerToken != "user-token" { + t.Errorf("derived BearerToken = %q, want %q", cfg.BearerToken, "user-token") + } + if cfg.BearerTokenFile != "" { + t.Errorf("derived BearerTokenFile = %q, want empty", cfg.BearerTokenFile) + } + if cfg.CertData != nil { + t.Error("derived CertData should be nil") + } + if cfg.KeyData != nil { + t.Error("derived KeyData should be nil") + } + if cfg.CertFile != "" { + t.Errorf("derived CertFile = %q, want empty", cfg.CertFile) + } + if cfg.KeyFile != "" { + t.Errorf("derived KeyFile = %q, want empty", cfg.KeyFile) + } + if cfg.Impersonate.UserName != "" || len(cfg.Impersonate.Groups) != 0 { + t.Errorf("derived Impersonate = %+v, want empty", cfg.Impersonate) + } + if !cfg.Insecure { + t.Error("derived Insecure should be preserved as true") + } + if cfg.Host != base.Host { + t.Errorf("derived Host = %q, want %q", cfg.Host, base.Host) + } + + // Base config must not be mutated. + if base.CertData == nil { + t.Error("base CertData was mutated") + } + if base.KeyData == nil { + t.Error("base KeyData was mutated") + } + if base.BearerToken != "sa-token" { + t.Errorf("base BearerToken = %q, want %q", base.BearerToken, "sa-token") + } + if base.BearerTokenFile != "/var/run/secrets/kubernetes.io/serviceaccount/token" { + t.Errorf("base BearerTokenFile = %q, was mutated", base.BearerTokenFile) + } +} diff --git a/test/e2e/create_alert_rule_test.go b/test/e2e/create_alert_rule_test.go index ee312e9de..bc5c20669 100644 --- a/test/e2e/create_alert_rule_test.go +++ b/test/e2e/create_alert_rule_test.go @@ -3,9 +3,14 @@ package e2e import ( + "bytes" "context" + "encoding/json" "errors" "fmt" + "io" + "net/http" + "net/url" "testing" "time" @@ -16,6 +21,8 @@ import ( "github.com/openshift/monitoring-plugin/test/e2e/framework" ) +// TestCreateUserDefinedAlertRule covers create success (cluster-admin) and RBAC +// denials/allowances for anonymous and namespace-scoped personas as subtests. func TestCreateUserDefinedAlertRule(t *testing.T) { f, err := framework.New() if err != nil { @@ -24,66 +31,158 @@ func TestCreateUserDefinedAlertRule(t *testing.T) { ctx := context.Background() - testNamespace, cleanup, err := f.CreateUserNamespace(ctx, "test-create-rule") + nsY, cleanupY, err := f.CreateUserNamespace(ctx, "test-create-rule-y") if err != nil { - t.Fatalf("Failed to create test namespace: %v", err) + t.Fatalf("Failed to create namespace Y: %v", err) } - defer cleanup() + defer func() { _ = cleanupY() }() - createExpr := "vector(1) or vector(0)" - createAlertRuleRequest := managementrouter.CreateAlertRuleRequest{ + nsZ, cleanupZ, err := f.CreateUserNamespace(ctx, "test-create-rule-z") + if err != nil { + t.Fatalf("Failed to create namespace Z: %v", err) + } + defer func() { _ = cleanupZ() }() + + anonymousUser, err := f.CreateAnonymousUser(ctx, "e2e-create-anon", "default") + if err != nil { + t.Fatalf("Failed to create anonymous user: %v", err) + } + defer func() { _ = anonymousUser.Cleanup() }() + + scopedUser, err := f.CreateScopedUser(ctx, "e2e-create-scoped", nsY, + "monitoring.coreos.com", []string{"prometheusrules"}, []string{"get", "create", "update", "patch"}) + if err != nil { + t.Fatalf("Failed to create scoped user: %v", err) + } + defer func() { _ = scopedUser.Cleanup() }() + + t.Run("ClusterAdmin_CreatesAndPersists", func(t *testing.T) { + createExpr := "vector(1) or vector(0)" + createAlertRuleRequest := managementrouter.CreateAlertRuleRequest{ + AlertingRule: &managementrouter.AlertRuleSpec{ + Alert: new("E2ECreateAlert"), + Expr: &createExpr, + For: new("1m"), + Labels: &map[string]string{ + "severity": "info", + }, + Annotations: &map[string]string{ + "summary": "E2E test alert for create-rule", + }, + }, + PrometheusRule: &managementrouter.PrometheusRuleTarget{ + PrometheusRuleName: "e2e-create-pr", + PrometheusRuleNamespace: nsY, + }, + } + id, err := createRuleViaAPIWithRetry(ctx, f, createAlertRuleRequest) + require.NoError(t, err) + require.NotEmpty(t, id) + t.Logf("Created rule with ID: %s", id) + + err = framework.Poll(time.Second, time.Minute, func() error { + promRule, err := f.Monitoringv1clientset.MonitoringV1().PrometheusRules(nsY).Get( + ctx, "e2e-create-pr", metav1.GetOptions{}, + ) + if err != nil { + return fmt.Errorf("failed to get PrometheusRule: %w", err) + } + + for _, group := range promRule.Spec.Groups { + for _, rule := range group.Rules { + if rule.Alert == "E2ECreateAlert" { + if rule.Expr.String() != createExpr { + return fmt.Errorf("expected expr %q, got %q", createExpr, rule.Expr.String()) + } + if rule.For == nil || string(*rule.For) != "1m" { + return fmt.Errorf("expected for '1m', got %v", rule.For) + } + if rule.Labels["severity"] != "info" { + return fmt.Errorf("expected severity=info, got %q", rule.Labels["severity"]) + } + if rule.Annotations["summary"] != "E2E test alert for create-rule" { + return fmt.Errorf("expected summary annotation, got %q", rule.Annotations["summary"]) + } + return nil + } + } + } + return errors.New("alerting rule 'E2ECreateAlert' not found in PrometheusRule") + }) + require.NoError(t, err) + }) + + cases := []struct { + name string + token string + namespace string + alertName string + wantStatus int + }{ + {"AnonymousUser_FailsNamespaceY", anonymousUser.Token, nsY, "RBACAlertA", http.StatusForbidden}, + {"AnonymousUser_FailsNamespaceZ", anonymousUser.Token, nsZ, "RBACAlertAZ", http.StatusForbidden}, + {"ScopedUser_SucceedsNamespaceY", scopedUser.Token, nsY, "RBACAlertBY", http.StatusCreated}, + {"ScopedUser_FailsNamespaceZ", scopedUser.Token, nsZ, "RBACAlertBZ", http.StatusForbidden}, + {"ClusterAdmin_SucceedsNamespaceZ", f.BearerToken, nsZ, "RBACAlertCZ", http.StatusCreated}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + status := createAlertRuleWithToken(ctx, t, f, tc.token, tc.namespace, tc.alertName) + if status != tc.wantStatus { + t.Fatalf("Expected status %d, got %d", tc.wantStatus, status) + } + }) + } +} + +// createAlertRuleWithToken sends a create alert rule request using the given +// bearer token and returns the HTTP status code. +func createAlertRuleWithToken(ctx context.Context, t *testing.T, f *framework.Framework, token, namespace, alertName string) int { + t.Helper() + + expr := fmt.Sprintf("absent(nonexistent{e2e_rbac_create=%q})", alertName) + payload := managementrouter.CreateAlertRuleRequest{ AlertingRule: &managementrouter.AlertRuleSpec{ - Alert: new("E2ECreateAlert"), - Expr: &createExpr, - For: new("1m"), + Alert: &alertName, + Expr: &expr, Labels: &map[string]string{ "severity": "info", }, - Annotations: &map[string]string{ - "summary": "E2E test alert for create-rule", - }, }, PrometheusRule: &managementrouter.PrometheusRuleTarget{ - PrometheusRuleName: "e2e-create-pr", - PrometheusRuleNamespace: testNamespace, + PrometheusRuleName: "e2e-rbac-pr", + PrometheusRuleNamespace: namespace, }, } - id, err := createRuleViaAPIWithRetry(ctx, f, createAlertRuleRequest) - require.NoError(t, err) - require.NotEmpty(t, id) - - t.Logf("Created rule with ID: %s", id) - - err = poll(time.Second, time.Minute, func() error { - promRule, err := f.Monitoringv1clientset.MonitoringV1().PrometheusRules(testNamespace).Get( - ctx, "e2e-create-pr", metav1.GetOptions{}, - ) - if err != nil { - return fmt.Errorf("failed to get PrometheusRule: %w", err) - } - for _, group := range promRule.Spec.Groups { - for _, rule := range group.Rules { - if rule.Alert == "E2ECreateAlert" { - if rule.Expr.String() != createExpr { - return fmt.Errorf("expected expr %q, got %q", createExpr, rule.Expr.String()) - } - if rule.For == nil || string(*rule.For) != "1m" { - return fmt.Errorf("expected for '1m', got %v", rule.For) - } - if rule.Labels["severity"] != "info" { - return fmt.Errorf("expected severity=info, got %q", rule.Labels["severity"]) - } - if rule.Annotations["summary"] != "E2E test alert for create-rule" { - return fmt.Errorf("expected summary annotation, got %q", rule.Annotations["summary"]) - } + reqBody, err := json.Marshal(payload) + if err != nil { + t.Fatalf("Failed to marshal create request: %v", err) + } - return nil - } - } - } + createURL, err := url.JoinPath(f.PluginURL, "api/v1/alerting/rules") + if err != nil { + t.Fatalf("Failed to build URL: %v", err) + } - return errors.New("alerting rule 'E2ECreateAlert' not found in PrometheusRule") - }) - require.NoError(t, err) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, createURL, bytes.NewBuffer(reqBody)) + if err != nil { + t.Fatalf("Failed to create HTTP request: %v", err) + } + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Authorization", "Bearer "+token) + + resp, err := f.HTTPClient().Do(req) + if err != nil { + t.Fatalf("Failed to make create request: %v", err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusCreated { + body, _ := io.ReadAll(resp.Body) + t.Logf("Create %s in %s: status %d, body: %s", alertName, namespace, resp.StatusCode, string(body)) + } + + return resp.StatusCode } diff --git a/test/e2e/delete_alert_rule_test.go b/test/e2e/delete_alert_rule_test.go index 672e27753..7d355a80f 100644 --- a/test/e2e/delete_alert_rule_test.go +++ b/test/e2e/delete_alert_rule_test.go @@ -9,6 +9,7 @@ import ( "fmt" "io" "net/http" + "net/url" "testing" "time" @@ -19,6 +20,8 @@ import ( "github.com/openshift/monitoring-plugin/test/e2e/framework" ) +// TestDeleteAlertRule covers bulk-delete success (cluster-admin) and RBAC +// denials/allowances for anonymous and namespace-scoped personas as subtests. func TestDeleteAlertRule(t *testing.T) { f, err := framework.New() if err != nil { @@ -27,118 +30,293 @@ func TestDeleteAlertRule(t *testing.T) { ctx := context.Background() - testNamespace, cleanup, err := f.CreateUserNamespace(ctx, "test-delete-rule") + nsY, cleanupY, err := f.CreateUserNamespace(ctx, "test-delete-rule-y") if err != nil { - t.Fatalf("Failed to create test namespace: %v", err) - } - defer cleanup() - - ruleNames := []string{"DeleteAlert1", "DeleteAlert2", "KeepAlert3"} - ruleIDs := make([]string, 0, len(ruleNames)) - - for _, name := range ruleNames { - expr := fmt.Sprintf("absent(nonexistent{e2e_rule=%q})", name) - alertRuleRequest := managementrouter.CreateAlertRuleRequest{ - AlertingRule: &managementrouter.AlertRuleSpec{ - Alert: new(name), - Expr: &expr, - For: new("1m"), - Labels: &map[string]string{ - "severity": "info", - }, - }, - PrometheusRule: &managementrouter.PrometheusRuleTarget{ - PrometheusRuleName: "e2e-delete-pr", - PrometheusRuleNamespace: testNamespace, - }, - } - - id, err := createRuleViaAPIWithRetry(ctx, f, alertRuleRequest) - if err != nil { - t.Fatalf("failed to create alert rule %s: %v", name, err) - } - ruleIDs = append(ruleIDs, id) - + t.Fatalf("Failed to create namespace Y: %v", err) } + defer func() { _ = cleanupY() }() - t.Logf("Created 3 rules with IDs: %v", ruleIDs) + nsZ, cleanupZ, err := f.CreateUserNamespace(ctx, "test-delete-rule-z") + if err != nil { + t.Fatalf("Failed to create namespace Z: %v", err) + } + defer func() { _ = cleanupZ() }() - deleteReq := managementrouter.BulkDeleteAlertRulesRequest{ - RuleIds: []string{ruleIDs[0], ruleIDs[1]}, + anonymousUser, err := f.CreateAnonymousUser(ctx, "e2e-delete-anon", "default") + if err != nil { + t.Fatalf("Failed to create anonymous user: %v", err) } - reqBody, err := json.Marshal(deleteReq) + defer func() { _ = anonymousUser.Cleanup() }() + + scopedUser, err := f.CreateScopedUser(ctx, "e2e-delete-scoped", nsY, + "monitoring.coreos.com", []string{"prometheusrules"}, []string{"get", "create", "update", "patch", "delete"}) if err != nil { - t.Fatalf("Failed to marshal delete request: %v", err) + t.Fatalf("Failed to create scoped user: %v", err) } + defer func() { _ = scopedUser.Cleanup() }() - err = poll(time.Second, time.Minute, func() error { - deleteURL := f.PluginURL + "/api/v1/alerting/rules" - req, err := http.NewRequestWithContext(ctx, http.MethodDelete, deleteURL, bytes.NewBuffer(reqBody)) - if err != nil { - return fmt.Errorf("failed to create delete request: %w", err) - } - req.Header.Set("Content-Type", "application/json") - if f.BearerToken != "" { - req.Header.Set("Authorization", "Bearer "+f.BearerToken) + t.Run("ClusterAdmin_BulkDeleteAndVerifyRemaining", func(t *testing.T) { + ruleNames := []string{"DeleteAlert1", "DeleteAlert2", "KeepAlert3"} + ruleIDs := make([]string, 0, len(ruleNames)) + + for _, name := range ruleNames { + expr := fmt.Sprintf("absent(nonexistent{e2e_rule=%q})", name) + alertRuleRequest := managementrouter.CreateAlertRuleRequest{ + AlertingRule: &managementrouter.AlertRuleSpec{ + Alert: new(name), + Expr: &expr, + For: new("1m"), + Labels: &map[string]string{ + "severity": "info", + }, + }, + PrometheusRule: &managementrouter.PrometheusRuleTarget{ + PrometheusRuleName: "e2e-delete-pr", + PrometheusRuleNamespace: nsY, + }, + } + + id, err := createRuleViaAPIWithRetry(ctx, f, alertRuleRequest) + if err != nil { + t.Fatalf("failed to create alert rule %s: %v", name, err) + } + ruleIDs = append(ruleIDs, id) } - resp, err := f.HTTPClient().Do(req) + t.Logf("Created 3 rules with IDs: %v", ruleIDs) + + deleteReq := managementrouter.BulkDeleteAlertRulesRequest{ + RuleIds: []string{ruleIDs[0], ruleIDs[1]}, + } + reqBody, err := json.Marshal(deleteReq) if err != nil { - return fmt.Errorf("failed to make delete request: %w", err) + t.Fatalf("Failed to marshal delete request: %v", err) } - defer resp.Body.Close() - if resp.StatusCode != http.StatusOK { - body, err := io.ReadAll(resp.Body) + err = framework.Poll(time.Second, time.Minute, func() error { + deleteURL := f.PluginURL + "/api/v1/alerting/rules" + req, err := http.NewRequestWithContext(ctx, http.MethodDelete, deleteURL, bytes.NewBuffer(reqBody)) if err != nil { - return fmt.Errorf("failed to read body: %w", err) + return fmt.Errorf("failed to create delete request: %w", err) + } + req.Header.Set("Content-Type", "application/json") + if f.BearerToken != "" { + req.Header.Set("Authorization", "Bearer "+f.BearerToken) } - return fmt.Errorf("expected status 200, got %d (body: %s)", resp.StatusCode, string(body)) - } - var deleteResp managementrouter.BulkDeleteAlertRulesResponse - if err := json.NewDecoder(resp.Body).Decode(&deleteResp); err != nil { - return fmt.Errorf("failed to decode delete response: %w", err) - } + resp, err := f.HTTPClient().Do(req) + if err != nil { + return fmt.Errorf("failed to make delete request: %w", err) + } + defer resp.Body.Close() - if len(deleteResp.Rules) != 2 { - return fmt.Errorf("expected 2 results, got %d", len(deleteResp.Rules)) - } - for _, result := range deleteResp.Rules { - // http.StatusNotFound can be returned if a previous request was partially successful. - if result.StatusCode/100 != 2 && result.StatusCode != http.StatusNotFound { - return fmt.Errorf("rule %s deletion failed with status %d: %v", result.Id, result.StatusCode, result.Message) + if resp.StatusCode != http.StatusOK { + body, err := io.ReadAll(resp.Body) + if err != nil { + return fmt.Errorf("failed to read body: %w", err) + } + return fmt.Errorf("expected status 200, got %d (body: %s)", resp.StatusCode, string(body)) } - } - return nil + var deleteResp managementrouter.BulkDeleteAlertRulesResponse + if err := json.NewDecoder(resp.Body).Decode(&deleteResp); err != nil { + return fmt.Errorf("failed to decode delete response: %w", err) + } + + if len(deleteResp.Rules) != 2 { + return fmt.Errorf("expected 2 results, got %d", len(deleteResp.Rules)) + } + for _, result := range deleteResp.Rules { + // http.StatusNotFound can be returned if a previous request was partially successful. + if result.StatusCode/100 != 2 && result.StatusCode != http.StatusNotFound { + return fmt.Errorf("rule %s deletion failed with status %d: %v", result.Id, result.StatusCode, result.Message) + } + } + + return nil + }) + require.NoError(t, err) + + err = framework.Poll(time.Second, 20*time.Second, func() error { + promRule, err := f.Monitoringv1clientset.MonitoringV1().PrometheusRules(nsY).Get( + ctx, "e2e-delete-pr", metav1.GetOptions{}, + ) + if err != nil { + return fmt.Errorf("failed to get PrometheusRule after deletion: %w", err) + } + + var remainingAlerts []string + for _, group := range promRule.Spec.Groups { + for _, rule := range group.Rules { + remainingAlerts = append(remainingAlerts, rule.Alert) + } + } + + if len(remainingAlerts) != 1 { + return fmt.Errorf("expected 1 remaining rule, got %d: %v", len(remainingAlerts), remainingAlerts) + } + + if remainingAlerts[0] != "KeepAlert3" { + return fmt.Errorf("expected remaining rule 'KeepAlert3', got %q", remainingAlerts[0]) + } + + return nil + }) + require.NoError(t, err) }) - require.NoError(t, err) - err = poll(time.Second, 20*time.Second, func() error { - promRule, err := f.Monitoringv1clientset.MonitoringV1().PrometheusRules(testNamespace).Get( - ctx, "e2e-delete-pr", metav1.GetOptions{}, - ) - if err != nil { - return fmt.Errorf("failed to get PrometheusRule after deletion: %w", err) - } + ruleInY, err := createRuleViaAPI(ctx, f, managementrouter.CreateAlertRuleRequest{ + AlertingRule: &managementrouter.AlertRuleSpec{ + Alert: new("RBACDelAlertY"), + Expr: new(fmt.Sprintf("absent(nonexistent{e2e_rbac_del=%q})", "y")), + Labels: &map[string]string{ + "severity": "info", + }, + }, + PrometheusRule: &managementrouter.PrometheusRuleTarget{ + PrometheusRuleName: "e2e-rbac-del-pr", + PrometheusRuleNamespace: nsY, + }, + }) + if err != nil { + t.Fatalf("Failed to create rule in namespace Y: %v", err) + } + + ruleInZ, err := createRuleViaAPI(ctx, f, managementrouter.CreateAlertRuleRequest{ + AlertingRule: &managementrouter.AlertRuleSpec{ + Alert: new("RBACDelAlertZ"), + Expr: new(fmt.Sprintf("absent(nonexistent{e2e_rbac_del=%q})", "z")), + Labels: &map[string]string{ + "severity": "info", + }, + }, + PrometheusRule: &managementrouter.PrometheusRuleTarget{ + PrometheusRuleName: "e2e-rbac-del-pr", + PrometheusRuleNamespace: nsZ, + }, + }) + if err != nil { + t.Fatalf("Failed to create rule in namespace Z: %v", err) + } + + ruleInY2, err := createRuleViaAPI(ctx, f, managementrouter.CreateAlertRuleRequest{ + AlertingRule: &managementrouter.AlertRuleSpec{ + Alert: new("RBACDelAlertY2"), + Expr: new(fmt.Sprintf("absent(nonexistent{e2e_rbac_del=%q})", "y2")), + Labels: &map[string]string{ + "severity": "info", + }, + }, + PrometheusRule: &managementrouter.PrometheusRuleTarget{ + PrometheusRuleName: "e2e-rbac-del-pr", + PrometheusRuleNamespace: nsY, + }, + }) + if err != nil { + t.Fatalf("Failed to create second rule in namespace Y: %v", err) + } + + // Probe with the anonymous user so a successful sync check cannot + // accidentally delete the rule (expects 403 once the cache has the ID). + for _, ruleID := range []string{ruleInY, ruleInY2, ruleInZ} { + waitForCacheSync(ctx, t, f, anonymousUser.Token, ruleID) + } - var remainingAlerts []string - for _, group := range promRule.Spec.Groups { - for _, rule := range group.Rules { - remainingAlerts = append(remainingAlerts, rule.Alert) + cases := []struct { + name string + token string + ruleID string + wantStatus int + }{ + {"AnonymousUser_DeniedNamespaceY", anonymousUser.Token, ruleInY, http.StatusForbidden}, + {"AnonymousUser_DeniedNamespaceZ", anonymousUser.Token, ruleInZ, http.StatusForbidden}, + {"ScopedUser_SucceedsNamespaceY", scopedUser.Token, ruleInY, http.StatusNoContent}, + {"ScopedUser_DeniedNamespaceZ", scopedUser.Token, ruleInZ, http.StatusForbidden}, + {"ClusterAdmin_SucceedsNamespaceZ", f.BearerToken, ruleInZ, http.StatusNoContent}, + {"ClusterAdmin_SucceedsNamespaceY", f.BearerToken, ruleInY2, http.StatusNoContent}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + status := deleteAlertRuleWithToken(ctx, t, f, tc.token, tc.ruleID) + if status != tc.wantStatus { + t.Fatalf("Expected per-rule status %d, got %d", tc.wantStatus, status) } - } + }) + } +} - if len(remainingAlerts) != 1 { - return fmt.Errorf("expected 1 remaining rule, got %d: %v", len(remainingAlerts), remainingAlerts) +// waitForCacheSync polls until the relabeled-rules cache has synced by +// attempting a bulk-delete probe with a non-deleting token. A 403 +// (Forbidden) per-rule status indicates the rule was found in cache and +// RBAC was evaluated without mutating the rule. +func waitForCacheSync(ctx context.Context, t *testing.T, f *framework.Framework, token, ruleID string) { + t.Helper() + err := framework.Poll(time.Second, 30*time.Second, func() error { + status, err := tryDeleteAlertRule(ctx, f, token, ruleID) + if err != nil { + return err } - - if remainingAlerts[0] != "KeepAlert3" { - return fmt.Errorf("expected remaining rule 'KeepAlert3', got %q", remainingAlerts[0]) + if status == http.StatusForbidden { + return nil } - - return nil + return fmt.Errorf("per-rule status %d, waiting for cache sync", status) }) - require.NoError(t, err) + if err != nil { + t.Fatalf("Cache sync timed out for rule %s: %v", ruleID, err) + } +} + +// tryDeleteAlertRule attempts a single-rule bulk-delete and returns the per-rule +// status code without calling t.Fatal, making it suitable for polling loops. +func tryDeleteAlertRule(ctx context.Context, f *framework.Framework, token, ruleID string) (int, error) { + payload := managementrouter.BulkDeleteAlertRulesRequest{ + RuleIds: []string{ruleID}, + } + reqBody, err := json.Marshal(payload) + if err != nil { + return 0, fmt.Errorf("marshal delete request: %w", err) + } + deleteURL, err := url.JoinPath(f.PluginURL, "api/v1/alerting/rules") + if err != nil { + return 0, fmt.Errorf("build URL: %w", err) + } + req, err := http.NewRequestWithContext(ctx, http.MethodDelete, deleteURL, bytes.NewBuffer(reqBody)) + if err != nil { + return 0, fmt.Errorf("create HTTP request: %w", err) + } + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Authorization", "Bearer "+token) + + resp, err := f.HTTPClient().Do(req) + if err != nil { + return 0, fmt.Errorf("make delete request: %w", err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + body, _ := io.ReadAll(resp.Body) + return resp.StatusCode, fmt.Errorf("expected bulk response 200, got %d: %s", resp.StatusCode, string(body)) + } + + var deleteResp managementrouter.BulkDeleteAlertRulesResponse + if err := json.NewDecoder(resp.Body).Decode(&deleteResp); err != nil { + return 0, fmt.Errorf("decode delete response: %w", err) + } + if len(deleteResp.Rules) != 1 { + return 0, fmt.Errorf("expected 1 per-rule result, got %d", len(deleteResp.Rules)) + } + return deleteResp.Rules[0].StatusCode, nil +} + +// deleteAlertRuleWithToken sends a bulk-delete request for a single rule ID +// using the given bearer token and returns the per-rule HTTP status code. +func deleteAlertRuleWithToken(ctx context.Context, t *testing.T, f *framework.Framework, token, ruleID string) int { + t.Helper() + + status, err := tryDeleteAlertRule(ctx, f, token, ruleID) + if err != nil { + t.Fatalf("Delete request for rule %s failed: %v", ruleID, err) + } + return status } diff --git a/test/e2e/framework/framework.go b/test/e2e/framework/framework.go index 3e007152c..eb181d521 100644 --- a/test/e2e/framework/framework.go +++ b/test/e2e/framework/framework.go @@ -223,3 +223,145 @@ func createServiceAccountToken(clientset *kubernetes.Clientset) (string, error) } return resp.Status.Token, nil } + +// ScopedUser represents a ServiceAccount with specific RBAC permissions for testing. +type ScopedUser struct { + Token string + Cleanup CleanupFunc +} + +// requestServiceAccountToken creates a short-lived (1 hour) bearer token for +// the named ServiceAccount via the TokenRequest API. The call is retried to +// tolerate transient API failures. +func (f *Framework) requestServiceAccountToken(ctx context.Context, namespace, name string) (string, error) { + expSeconds := int64(3600) + treq := &authv1.TokenRequest{ + Spec: authv1.TokenRequestSpec{ExpirationSeconds: &expSeconds}, + } + var token string + err := Poll(time.Second, 3*time.Second, func() error { + tokenResp, err := f.Clientset.CoreV1().ServiceAccounts(namespace).CreateToken(ctx, name, treq, metav1.CreateOptions{}) + if err != nil { + return err + } + token = tokenResp.Status.Token + return nil + }) + if err != nil { + return "", fmt.Errorf("requesting token for %s/%s: %w", namespace, name, err) + } + return token, nil +} + +// CreateScopedUser creates a ServiceAccount in the given namespace with a Role +// granting the specified verbs on the specified resources. Returns a bearer token +// and a cleanup function. The apiGroup should be e.g. "monitoring.coreos.com". +// API calls are retried to tolerate transient failures. +func (f *Framework) CreateScopedUser(ctx context.Context, name, namespace, apiGroup string, resources, verbs []string) (*ScopedUser, error) { + rollback := func() { + _ = f.Clientset.RbacV1().RoleBindings(namespace).Delete(ctx, name, metav1.DeleteOptions{}) + _ = f.Clientset.RbacV1().Roles(namespace).Delete(ctx, name, metav1.DeleteOptions{}) + _ = f.Clientset.CoreV1().ServiceAccounts(namespace).Delete(ctx, name, metav1.DeleteOptions{}) + } + + sa := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{Name: name}, + } + err := Poll(time.Second, 3*time.Second, func() error { + _, err := f.Clientset.CoreV1().ServiceAccounts(namespace).Create(ctx, sa, metav1.CreateOptions{}) + if apierrors.IsAlreadyExists(err) { + return nil + } + return err + }) + if err != nil { + return nil, fmt.Errorf("creating service account %s/%s: %w", namespace, name, err) + } + + role := &rbacv1.Role{ + ObjectMeta: metav1.ObjectMeta{Name: name}, + Rules: []rbacv1.PolicyRule{{ + APIGroups: []string{apiGroup}, + Resources: resources, + Verbs: verbs, + }}, + } + err = Poll(time.Second, 3*time.Second, func() error { + _, err := f.Clientset.RbacV1().Roles(namespace).Create(ctx, role, metav1.CreateOptions{}) + if apierrors.IsAlreadyExists(err) { + return nil + } + return err + }) + if err != nil { + rollback() + return nil, fmt.Errorf("creating role %s/%s: %w", namespace, name, err) + } + + rb := &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{Name: name}, + Subjects: []rbacv1.Subject{{ + Kind: rbacv1.ServiceAccountKind, + Name: name, + Namespace: namespace, + }}, + RoleRef: rbacv1.RoleRef{ + APIGroup: rbacv1.GroupName, + Kind: "Role", + Name: name, + }, + } + err = Poll(time.Second, 3*time.Second, func() error { + _, err := f.Clientset.RbacV1().RoleBindings(namespace).Create(ctx, rb, metav1.CreateOptions{}) + if apierrors.IsAlreadyExists(err) { + return nil + } + return err + }) + if err != nil { + rollback() + return nil, fmt.Errorf("creating role binding %s/%s: %w", namespace, name, err) + } + + token, err := f.requestServiceAccountToken(ctx, namespace, name) + if err != nil { + rollback() + return nil, err + } + + return &ScopedUser{Token: token, Cleanup: func() error { rollback(); return nil }}, nil +} + +// CreateAnonymousUser creates a ServiceAccount with no RBAC permissions. +// The whole setup is retried to tolerate transient API failures. +func (f *Framework) CreateAnonymousUser(ctx context.Context, name, namespace string) (*ScopedUser, error) { + var user *ScopedUser + err := Poll(time.Second, 3*time.Second, func() error { + sa := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{Name: name}, + } + _, err := f.Clientset.CoreV1().ServiceAccounts(namespace).Create(ctx, sa, metav1.CreateOptions{}) + if err != nil && !apierrors.IsAlreadyExists(err) { + return fmt.Errorf("creating service account %s/%s: %w", namespace, name, err) + } + + token, err := f.requestServiceAccountToken(ctx, namespace, name) + if err != nil { + _ = f.Clientset.CoreV1().ServiceAccounts(namespace).Delete(ctx, name, metav1.DeleteOptions{}) + return err + } + + user = &ScopedUser{ + Token: token, + Cleanup: func() error { + _ = f.Clientset.CoreV1().ServiceAccounts(namespace).Delete(ctx, name, metav1.DeleteOptions{}) + return nil + }, + } + return nil + }) + if err != nil { + return nil, err + } + return user, nil +} diff --git a/test/e2e/framework/poll.go b/test/e2e/framework/poll.go new file mode 100644 index 000000000..67136505d --- /dev/null +++ b/test/e2e/framework/poll.go @@ -0,0 +1,27 @@ +//go:build e2e + +package framework + +import ( + "context" + "fmt" + "time" + + "k8s.io/apimachinery/pkg/util/wait" +) + +// Poll calls f every interval until it returns nil or timeout elapses. +// On timeout the last observed error is wrapped with wait.ErrWaitTimeout. +func Poll(interval, timeout time.Duration, f func() error) error { + var lastErr error + err := wait.PollUntilContextTimeout(context.Background(), interval, timeout, true, func(context.Context) (bool, error) { + if lastErr = f(); lastErr != nil { + return false, nil + } + return true, nil + }) + if err != nil && lastErr != nil { + return fmt.Errorf("%w: %w", err, lastErr) + } + return err +} diff --git a/test/e2e/helpers_test.go b/test/e2e/helpers_test.go index 481634de8..ad52b5d4c 100644 --- a/test/e2e/helpers_test.go +++ b/test/e2e/helpers_test.go @@ -12,35 +12,13 @@ import ( "net/url" "time" - "k8s.io/apimachinery/pkg/util/wait" - "github.com/openshift/monitoring-plugin/internal/managementrouter" "github.com/openshift/monitoring-plugin/test/e2e/framework" ) -// poll calls the given function f() every given interval -// until it returns no error or the given timeout occurs. -// When a timeout occurs, the last observed error is returned -// wrapped in a wait.ErrWaitTimeout. -func poll(interval, timeout time.Duration, f func() error) error { - var lastErr error - err := wait.PollUntilContextTimeout(context.Background(), interval, timeout, true, func(context.Context) (bool, error) { - if lastErr = f(); lastErr != nil { - return false, nil - } - - return true, nil - }) - if err != nil && lastErr != nil { - return fmt.Errorf("%w: %w", err, lastErr) - } - - return err -} - func createRuleViaAPIWithRetry(ctx context.Context, f *framework.Framework, createAlertRuleRequest managementrouter.CreateAlertRuleRequest) (string, error) { var id string - err := poll(time.Second, 20*time.Second, func() error { + err := framework.Poll(time.Second, 20*time.Second, func() error { var err error id, err = createRuleViaAPI(ctx, f, createAlertRuleRequest) if err != nil {