From 973f4c42afaa57e9138d2834b260d676f5741e24 Mon Sep 17 00:00:00 2001 From: Vlastimil Zeman Date: Fri, 24 Apr 2026 15:59:37 +0100 Subject: [PATCH 1/2] feat: support all-in-one request.yaml, enforce gold files, improve README --- README.md | 248 +++++++++++------- internal/evaluator/evaluator.go | 8 + internal/loader/request.go | 111 +++----- internal/loader/request_test.go | 112 ++++++++ internal/loader/suite.go | 44 +++- internal/loader/suite_test.go | 70 ++++- ...dd-default-labels.missing-gold.object.yaml | 17 ++ ...ditional.object-conflict.allow.object.yaml | 18 ++ ...itional.object-conflict.allow.request.yaml | 27 ++ .../add-label.all-in-one.allowed.gold.yaml | 12 + .../add-label.all-in-one.allowed.request.yaml | 22 ++ ...ting-test.prod-namespace.mutate.gold.yaml} | 0 .../conditional.all-in-one.allow.request.yaml | 28 ++ ...-limit-params.all-in-one.deny.request.yaml | 27 ++ testdata/fail_policies.golden | 4 + testdata/json_output.golden | 2 + testdata/recursion_dot.golden | 4 + 17 files changed, 578 insertions(+), 176 deletions(-) create mode 100644 test-policies-fail/add-default-labels/tests/add-default-labels.missing-gold.object.yaml create mode 100644 test-policies-fail/conditional-policy/tests/conditional.object-conflict.allow.object.yaml create mode 100644 test-policies-fail/conditional-policy/tests/conditional.object-conflict.allow.request.yaml create mode 100644 test-policies-pass/mutating/mutating-with-binding/tests/add-label.all-in-one.allowed.gold.yaml create mode 100644 test-policies-pass/mutating/mutating-with-binding/tests/add-label.all-in-one.allowed.request.yaml rename test-policies-pass/mutating/namespace-selector-binding-mutating/tests/{namespace-selector-binding-mutating-test.prod-namespace.mutate.expected.yaml => namespace-selector-binding-mutating-test.prod-namespace.mutate.gold.yaml} (100%) create mode 100644 test-policies-pass/validating/conditional-policy/tests/conditional.all-in-one.allow.request.yaml create mode 100644 test-policies-pass/validating/replica-limit-with-params/tests/replica-limit-params.all-in-one.deny.request.yaml diff --git a/README.md b/README.md index b418e65..9474045 100644 --- a/README.md +++ b/README.md @@ -13,17 +13,46 @@ `kat` is a lightweight, local testing tool for Kubernetes Admission Policies (ValidatingAdmissionPolicy and MutatingAdmissionPolicy). It allows you to write test cases using standard Kubernetes manifests and verify your policies' behavior without needing a running cluster. -## Features +## Quick Start + +Given a policy like this: + +```text +my-policy/ +├── policy.yaml +└── tests/ + ├── my-policy.good-pod.allow.object.yaml + └── my-policy.bad-pod.deny.object.yaml +``` + +The simplest test is just a Kubernetes object: + +```yaml +# tests/my-policy.good-pod.allow.object.yaml +apiVersion: v1 +kind: Pod +metadata: + name: good-pod + labels: + owner: platform-team +``` -- **Standard Kubernetes YAML**: Write tests using plain K8s manifests - no new DSL to learn. -- **Full CEL Support**: Uses the official Kubernetes CEL libraries for 100% accurate evaluation. -- **Comprehensive Policy Support**: - - `ValidatingAdmissionPolicy` (Allow, Deny, Warn, Audit) - - `MutatingAdmissionPolicy` (Mutate, No-op) -- **All Operations**: Supports CREATE, UPDATE, DELETE, and CONNECT operations. -- **Golden File Testing**: Automatically verifies mutated objects against expected golden files. -- **Rich Context**: Simulate complex scenarios with `userInfo`, `namespaceObject`, and `matchConditions`. -- **Parameter Testing**: Test parameter-driven policies (`paramKind`/`paramRef`). +```yaml +# tests/my-policy.bad-pod.deny.object.yaml +apiVersion: v1 +kind: Pod +metadata: + name: bad-pod + # Missing required label +``` + +Run it: + +```bash +kat . +``` + +That's it. `kat` discovers the policy, finds the tests, and evaluates them. The filename tells `kat` everything: which policy to test (`my-policy`), what to expect (`allow` or `deny`), and what the file contains (`object`). ## Installation @@ -41,19 +70,13 @@ go install ## Usage -The recommended way to use `kat` is to run it from the root of your repository. It will automatically discover and execute all tests found in `tests/` directories recursively. +Run from the root of your repository — `kat` will automatically discover and execute all tests found in `tests/` directories recursively: ```bash kat . ``` -This commands will: - -1. Find all `tests` directories. -2. Locate the corresponding Policy and Binding for each test (by looking up in the directory tree). -3. Execute all found tests. - -You can also target specific directories or files: +Target specific directories or files: ```bash # Run tests for a specific policy @@ -90,10 +113,6 @@ Supported filenames include: **Note:** You can define multiple policies and bindings in a single file (separated by `---`), or split them across multiple files. The tool loads all valid policy/binding resources found in the directory. -This allows you to keep your tests co-located with your policy definitions. You just need to add a `tests/` folder alongside your manifests. - -**Example Layout:** - ```text policies/ ├── team-label-policy/ @@ -106,119 +125,168 @@ policies/ │ └── ... ``` -In this setup, running `kat .` at the root will automatically find the `tests` directory, associate it with the policy in the parent `team-label-policy` directory, and execute the tests. +Running `kat .` at the root will automatically find the `tests` directory, associate it with the policy in the parent directory, and execute the tests. ## Writing Tests -Tests are defined by file naming conventions. The filename structure determines the test type and expectations. - ### File Naming Convention Pattern: `....yaml` -**Requirement:** The `` prefix must match the `metadata.name` of the policy being tested. -*(If a directory contains only a single policy, kats automatically associates all tests with that policy).* +The `` prefix must match the `metadata.name` of the policy being tested. If a directory contains only a single policy, `kat` automatically associates all tests with that policy. -- **expect**: `allow`, `deny`, `warn`, `audit` (for Validating) -- **type**: `object`, `oldObject`, `request`, `params` +| Part | Values | Description | +|------|--------|-------------| +| **expect** | `allow`, `deny`, `warn`, `audit` | Expected admission outcome | +| **type** | `object`, `oldObject`, `request`, `params`, `namespaceObject`, `authorizer`, `annotations`, `warnings` | What the file contains | -### Validating Admission Policy +Multiple files with the same `..` prefix are merged into a single test case. -**1. Expect Allow:** -Create a file ending in `.allow.object.yaml`. +### Two Ways to Write Tests -```yaml -# my-policy.test-1.allow.object.yaml -apiVersion: v1 -kind: Pod -metadata: - name: allowed-pod - labels: - cost-center: "123" -``` +You can provide test inputs as **a single `.request.yaml`** file or as **separate files per field** — or a mix of both. -**2. Expect Deny:** -Create a file ending in `.deny.object.yaml`. +#### All-in-One: `.request.yaml` + +A `.request.yaml` file can contain the object, params, namespace context, and user info all in one place. This is the simplest way to write tests that need more than just an object. ```yaml -# my-policy.test-2.deny.object.yaml -apiVersion: v1 -kind: Pod -metadata: - name: denied-pod - # Missing required labels +# my-policy.dev-deploy.allow.request.yaml +operation: CREATE +namespace: development +object: + apiVersion: apps/v1 + kind: Deployment + metadata: + name: dev-deployment + namespace: development + spec: + replicas: 1 + selector: + matchLabels: + app: test + template: + metadata: + labels: + app: test + spec: + containers: + - name: nginx + image: nginx +namespaceObject: + apiVersion: v1 + kind: Namespace + metadata: + name: development + labels: + environment: dev +params: + apiVersion: v1 + kind: ConfigMap + metadata: + name: policy-config + data: + maxReplicas: "10" +userInfo: + username: "developer@example.com" ``` -**3. Expect Specific Message:** -Add a `.message.txt` file side-by-side. +Available fields in `.request.yaml`: + +| Field | Description | +|-------|-------------| +| `operation` | `CREATE`, `UPDATE`, `DELETE`, or `CONNECT` | +| `object` | The object being admitted | +| `oldObject` | Previous version (for UPDATE/DELETE) | +| `params` | Parameter resource (`paramKind`/`paramRef`) | +| `namespaceObject` | Namespace context with labels/annotations | +| `userInfo` | User making the request | +| `namespace` | Shorthand for request namespace | +| `name` | Shorthand for request name | +| `subResource` | Sub-resource being accessed (e.g., `status`) | +| `options` | Additional options for the request | + +#### Split Files + +For simpler tests, you can use separate files — each containing just one piece: ```text -# my-policy.test-2.deny.message.txt -Pod must have a cost-center label +my-policy.deploy-test.allow.object.yaml # The object being admitted +my-policy.deploy-test.allow.params.yaml # Policy parameters +my-policy.deploy-test.allow.request.yaml # Additional context (userInfo, namespace, etc.) ``` -### Mutating Admission Policy - -**1. Mutation Test:** -Provide the input object and the expected output (golden file). +This keeps individual files small and readable. All files sharing the same base name (`my-policy.deploy-test.allow`) are merged into one test case. -- Input: `my-policy.test-1.object.yaml` -- Expected: `my-policy.test-1.gold.yaml` +**Conflict detection:** If the same field (e.g., `object`) is defined in both `.request.yaml` and a separate `.object.yaml`, `kat` reports an error. -If the actual mutation result differs from the golden file, the test fails and prints a diff. +### Expected Outcomes -### Advanced Scenarios +**Allow** — the object is admitted: +```yaml +# my-policy.good-pod.allow.object.yaml +``` -#### Request Context (`.request.yaml`) +**Deny** — the request is rejected. Add a `.message.txt` to verify the exact error: +```text +# my-policy.bad-pod.deny.message.txt +All workloads must have an 'owner' label +``` -Use a `.request.yaml` file to provide additional admission context like user info or namespace details. +**Warn** — admitted with warnings. Add a `.warnings.txt`: +```text +# my-policy.old-api.warn.warnings.txt +Deprecated API version, migrate to apps/v1 +``` +**Audit** — admitted with audit annotations. Add an `.annotations.yaml`: ```yaml -# my-policy.test-1.allow.request.yaml -operation: CREATE -userInfo: - username: "system:serviceaccount:kube-system:job-controller" -namespaceObject: - metadata: - labels: - environment: production +# my-policy.flagged.audit.annotations.yaml +audit-annotation-key: "violation detected" ``` -#### Parameters (`.params.yaml`) +### Mutating Policies -For policies using `paramKind`, provide the parameter resource. +For mutating policies, provide a **golden file** (`.gold.yaml`) with the expected output: -```yaml -# my-policy.test-1.allow.params.yaml -apiVersion: v1 -kind: ConfigMap -metadata: - name: policy-config -data: - excludedNamespaces: "kube-system,monitoring" +```text +my-policy.add-labels.object.yaml # Input object +my-policy.add-labels.gold.yaml # Expected object after mutation ``` -#### Authorizer Mocking (`.authorizer.yaml`) +If the actual mutation result differs from the golden file, the test fails with a diff. This also works with all-in-one `.request.yaml` files — just place a `.gold.yaml` alongside it. -You can mock Kubernetes Authorizer responses (SubjectAccessReview) for policies that use `authorizer` checks in CEL. -Create a `.authorizer.yaml` file side-by-side with your test files. +### Authorizer Mocking + +Mock Kubernetes Authorizer responses (SubjectAccessReview) for policies using `authorizer` in CEL: ```yaml -# my-policy.test-1.allow.authorizer.yaml +# my-policy.check-perms.deny.authorizer.yaml - group: "" resource: "pods" - subresource: "" namespace: "default" verb: "create" decision: "allow" ``` -The mock matches requests based on group, resource, subresource, namespace, and verb. By default, any check not explicitly mocked will return "NoOpinion" (which usually results in a denial or failed check depending on policy logic). +Any check not explicitly mocked returns "NoOpinion". + +### Operations -#### Operations (UPDATE / DELETE) +- **CREATE** (default): Provide `.object.yaml` (or `object` in `.request.yaml`). +- **UPDATE**: Provide both `.object.yaml` (new) and `.oldObject.yaml` (old). Operation is inferred automatically. +- **DELETE**: Provide only `.oldObject.yaml`. Operation is inferred automatically. +- **CONNECT**: Set `operation: CONNECT` in `.request.yaml`. + +## Features -- **UPDATE**: Provide both `.object.yaml` (new) and `.oldObject.yaml` (old). -- **DELETE**: Provide only `.oldObject.yaml` (resource being deleted). +- **Standard Kubernetes YAML** — no new DSL to learn +- **Full CEL Support** — uses official Kubernetes CEL libraries for 100% accurate evaluation +- **Comprehensive Policy Support** — `ValidatingAdmissionPolicy` and `MutatingAdmissionPolicy` +- **All Operations** — CREATE, UPDATE, DELETE, CONNECT +- **Golden File Testing** — verifies mutated objects against expected output +- **Rich Context** — `userInfo`, `namespaceObject`, `matchConditions`, authorizer mocking +- **Parameter Testing** — `paramKind`/`paramRef` with ConfigMaps or custom resources ## Examples diff --git a/internal/evaluator/evaluator.go b/internal/evaluator/evaluator.go index 9b6b94a..9a98cb7 100644 --- a/internal/evaluator/evaluator.go +++ b/internal/evaluator/evaluator.go @@ -217,6 +217,14 @@ func validateTestResult(result *TestResult, expected *TestExpectation, actual *T return chk } + // If the policy mutated the object but no .gold.yaml was provided, fail. + if result.PatchedObject != nil && expected.Object == nil { + result.Passed = false + result.Message = "policy mutated the object but no .gold.yaml file was provided" + + return result + } + result.Passed = true return result diff --git a/internal/loader/request.go b/internal/loader/request.go index 88851f7..3517263 100644 --- a/internal/loader/request.go +++ b/internal/loader/request.go @@ -44,6 +44,8 @@ func parseTestRequestFile(testReq *testRequest) error { return parseObjectYAML(testReq, data) case strings.HasSuffix(testReq.FilePath, ".oldObject.yaml"): return parseOldObjectYAML(testReq, data) + case strings.HasSuffix(testReq.FilePath, ".namespaceObject.yaml"): + return parseNamespaceObjectYAML(testReq, data) case strings.HasSuffix(testReq.FilePath, ".params.yaml"): return parseParamsYAML(testReq, data) case strings.HasSuffix(testReq.FilePath, ".annotations.yaml"): @@ -67,6 +69,7 @@ type simplifiedRequest struct { UserInfo *authenticationv1.UserInfo `json:"userInfo,omitempty"` Object map[string]interface{} `json:"object,omitempty"` OldObject map[string]interface{} `json:"oldObject,omitempty"` + Params map[string]interface{} `json:"params,omitempty"` Options map[string]interface{} `json:"options,omitempty"` } @@ -93,6 +96,15 @@ func parseRequestYAML(testReq *testRequest, data []byte) error { testReq.NamespaceObj = &unstructured.Unstructured{Object: req.NamespaceObject} } + if req.Params != nil { + testReq.Params = &unstructured.Unstructured{Object: req.Params} + } + + // Load gold file if present (for mutating policies tested via request.yaml). + if err := loadGoldFile(testReq); err != nil { + return err + } + return nil } @@ -267,31 +279,23 @@ func buildCreateRequestFromObject(testName string, obj *unstructured.Unstructure } func loadAuxiliaryFiles(testReq *testRequest) error { - // Look for corresponding .gold.yaml file (expected mutated object) + // Look for corresponding .gold.yaml file (expected mutated object). + // .params.yaml and .authorizer.yaml are registered file types handled by the suite loop. if err := loadGoldFile(testReq); err != nil { return err } - // Look for corresponding .params.yaml file - if err := loadParamsFile(testReq); err != nil { - return err - } - // Look for corresponding .message.txt file (expected error message) if err := loadMessageFile(testReq); err != nil { return err } - // Look for corresponding .authorizer.yaml file - if err := loadAuthorizerFile(testReq); err != nil { - return err - } - return nil } func loadGoldFile(testReq *testRequest) error { goldPath := strings.Replace(testReq.FilePath, ".object.yaml", ".gold.yaml", 1) + goldPath = strings.Replace(goldPath, ".request.yaml", ".gold.yaml", 1) if _, err := os.Stat(goldPath); err != nil { if os.IsNotExist(err) { return nil @@ -316,33 +320,6 @@ func loadGoldFile(testReq *testRequest) error { return nil } -func loadParamsFile(testReq *testRequest) error { - paramsPath := strings.Replace(testReq.FilePath, ".object.yaml", ".params.yaml", 1) - paramsPath = strings.Replace(paramsPath, ".request.yaml", ".params.yaml", 1) - - if _, err := os.Stat(paramsPath); err != nil { - if os.IsNotExist(err) { - return nil - } - - return fmt.Errorf("stat params file: %w", err) - } - - paramsData, err := os.ReadFile(paramsPath) - if err != nil { - return fmt.Errorf("failed to read params file: %w", err) - } - - var paramsObj map[string]interface{} - if err := yaml.Unmarshal(paramsData, ¶msObj); err != nil { - return fmt.Errorf("failed to unmarshal params object: %w", err) - } - - testReq.Params = &unstructured.Unstructured{Object: paramsObj} - - return nil -} - func loadMessageFile(testReq *testRequest) error { messagePath := strings.Replace(testReq.FilePath, ".object.yaml", ".message.txt", 1) messagePath = strings.Replace(messagePath, ".request.yaml", ".message.txt", 1) @@ -365,26 +342,6 @@ func loadMessageFile(testReq *testRequest) error { return nil } -func loadAuthorizerFile(testReq *testRequest) error { - authPath := strings.Replace(testReq.FilePath, ".object.yaml", ".authorizer.yaml", 1) - authPath = strings.Replace(authPath, ".request.yaml", ".authorizer.yaml", 1) - - if _, err := os.Stat(authPath); err != nil { - if os.IsNotExist(err) { - return nil - } - - return fmt.Errorf("stat authorizer file: %w", err) - } - - authData, err := os.ReadFile(authPath) - if err != nil { - return fmt.Errorf("failed to read authorizer file: %w", err) - } - - return parseAuthorizerYAML(testReq, authData) -} - // parseOldObjectYAML parses a raw Kubernetes object and creates an AdmissionRequest for DELETE operation. // This is used for testing deletion policies where only oldObject is relevant. func parseOldObjectYAML(testReq *testRequest, data []byte) error { @@ -422,23 +379,8 @@ func parseOldObjectYAML(testReq *testRequest, data []byte) error { testReq.Request = admReq testReq.NamespaceName = unstruct.GetNamespace() - // Look for corresponding .params.yaml file - paramsPath := strings.Replace(testReq.FilePath, ".oldObject.yaml", ".params.yaml", 1) - if _, err := os.Stat(paramsPath); err == nil { - paramsData, err := os.ReadFile(paramsPath) - if err != nil { - return fmt.Errorf("failed to read params file: %w", err) - } - - var paramsObj map[string]interface{} - if err := yaml.Unmarshal(paramsData, ¶msObj); err != nil { - return fmt.Errorf("failed to unmarshal params object: %w", err) - } - - testReq.Params = &unstructured.Unstructured{Object: paramsObj} - } - - // Look for corresponding .message.txt file (expected error message) + // Look for corresponding .message.txt file (expected error message). + // .params.yaml is a registered file type handled by the suite loop. messagePath := strings.Replace(testReq.FilePath, ".oldObject.yaml", ".message.txt", 1) if _, err := os.Stat(messagePath); err == nil { messageData, err := os.ReadFile(messagePath) @@ -452,6 +394,25 @@ func parseOldObjectYAML(testReq *testRequest, data []byte) error { return nil } +// parseNamespaceObjectYAML parses a Kubernetes Namespace object providing namespace context for the admission request. +func parseNamespaceObjectYAML(testReq *testRequest, data []byte) error { + var obj map[string]interface{} + if err := yaml.Unmarshal(data, &obj); err != nil { + return fmt.Errorf("failed to unmarshal namespaceObject: %w", err) + } + + nsGVK := schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Namespace"} + if err := validateWithScheme(obj, "namespaceObject", &nsGVK); err != nil { + return err + } + + unstruct := &unstructured.Unstructured{Object: obj} + testReq.NamespaceObj = unstruct + testReq.NamespaceName = unstruct.GetName() + + return nil +} + // loadGoldFile loads the expected object from a .gold.yaml file. // parseParamsYAML parses a policy parameters file (ConfigMap or custom resource). func parseParamsYAML(testReq *testRequest, data []byte) error { diff --git a/internal/loader/request_test.go b/internal/loader/request_test.go index 2fb8d41..120aa4a 100644 --- a/internal/loader/request_test.go +++ b/internal/loader/request_test.go @@ -1,6 +1,8 @@ package loader import ( + "os" + "path/filepath" "testing" "k8s.io/apimachinery/pkg/runtime/schema" @@ -227,3 +229,113 @@ func TestInferOperation(t *testing.T) { }) } } + +func TestParseRequestYAML_Params(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + requestFile := filepath.Join(dir, "test.allow.request.yaml") + content := ` +operation: CREATE +object: + apiVersion: v1 + kind: Pod + metadata: + name: test-pod + spec: + containers: + - name: nginx + image: nginx +params: + apiVersion: v1 + kind: ConfigMap + metadata: + name: my-config + data: + maxReplicas: "5" +` + if err := os.WriteFile(requestFile, []byte(content), 0o600); err != nil { + t.Fatal(err) + } + + req := &testRequest{Name: "test.allow", FilePath: requestFile} + data, err := os.ReadFile(requestFile) + if err != nil { + t.Fatal(err) + } + + if err := parseRequestYAML(req, data); err != nil { + t.Fatalf("parseRequestYAML() error = %v", err) + } + + if req.Params == nil { + t.Fatal("expected Params to be set, got nil") + } + + if req.Params.GetName() != "my-config" { + t.Errorf("Params.Name = %q, want %q", req.Params.GetName(), "my-config") + } +} + +func TestParseRequestYAML_GoldFile(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + requestFile := filepath.Join(dir, "test.request.yaml") + goldFile := filepath.Join(dir, "test.gold.yaml") + + requestContent := ` +operation: CREATE +object: + apiVersion: v1 + kind: Pod + metadata: + name: test-pod + spec: + containers: + - name: nginx + image: nginx +` + goldContent := ` +apiVersion: v1 +kind: Pod +metadata: + name: test-pod + labels: + injected: "true" +spec: + containers: + - name: nginx + image: nginx +` + + if err := os.WriteFile(requestFile, []byte(requestContent), 0o600); err != nil { + t.Fatal(err) + } + + if err := os.WriteFile(goldFile, []byte(goldContent), 0o600); err != nil { + t.Fatal(err) + } + + req := &testRequest{Name: "test", FilePath: requestFile} + data, err := os.ReadFile(requestFile) + if err != nil { + t.Fatal(err) + } + + if err := parseRequestYAML(req, data); err != nil { + t.Fatalf("parseRequestYAML() error = %v", err) + } + + if !req.ExpectMutated { + t.Error("expected ExpectMutated=true when .gold.yaml is present") + } + + if req.ExpectedObject == nil { + t.Fatal("expected ExpectedObject to be set") + } + + if req.ExpectedObject.GetName() != "test-pod" { + t.Errorf("ExpectedObject.Name = %q, want %q", req.ExpectedObject.GetName(), "test-pod") + } +} diff --git a/internal/loader/suite.go b/internal/loader/suite.go index 368c7e1..31b0498 100644 --- a/internal/loader/suite.go +++ b/internal/loader/suite.go @@ -380,6 +380,7 @@ func isTestFile(name string) bool { return strings.HasSuffix(name, ".request.yaml") || strings.HasSuffix(name, ".object.yaml") || strings.HasSuffix(name, ".oldObject.yaml") || + strings.HasSuffix(name, ".namespaceObject.yaml") || strings.HasSuffix(name, ".params.yaml") || strings.HasSuffix(name, ".annotations.yaml") || strings.HasSuffix(name, ".warnings.txt") || @@ -390,6 +391,7 @@ func testBaseName(name string) string { baseName := strings.TrimSuffix(name, ".request.yaml") baseName = strings.TrimSuffix(baseName, ".object.yaml") baseName = strings.TrimSuffix(baseName, ".oldObject.yaml") + baseName = strings.TrimSuffix(baseName, ".namespaceObject.yaml") baseName = strings.TrimSuffix(baseName, ".params.yaml") baseName = strings.TrimSuffix(baseName, ".annotations.yaml") baseName = strings.TrimSuffix(baseName, ".warnings.txt") @@ -424,7 +426,11 @@ func buildTestRequest(baseName string, filePaths []string, policyNames []string) return testReq } - mergeTestRequests(testReq, tempReq) + if err := mergeTestRequests(testReq, tempReq); err != nil { + testReq.Error = err + + return testReq + } } if !hasExplicitRequest && testReq.Request != nil { @@ -474,23 +480,43 @@ func newTempTestRequest(filePath, policyName string, expectAllowed bool) *testRe } //nolint:cyclop // Merge function with many fields -func mergeTestRequests(testReq, tempReq *testRequest) { +func mergeTestRequests(testReq, tempReq *testRequest) error { if tempReq.Object != nil { + if testReq.Object != nil { + return fmt.Errorf("conflict: object defined in multiple files") + } + testReq.Object = tempReq.Object } if tempReq.OldObject != nil { - testReq.OldObject = tempReq.OldObject - } + if testReq.OldObject != nil { + return fmt.Errorf("conflict: oldObject defined in multiple files") + } - if tempReq.Request != nil { - mergeRequest(testReq, tempReq) + testReq.OldObject = tempReq.OldObject } if tempReq.NamespaceObj != nil { + if testReq.NamespaceObj != nil { + return fmt.Errorf("conflict: namespaceObject defined in multiple files") + } + testReq.NamespaceObj = tempReq.NamespaceObj } + if tempReq.Params != nil { + if testReq.Params != nil { + return fmt.Errorf("conflict: params defined in multiple files") + } + + testReq.Params = tempReq.Params + } + + if tempReq.Request != nil { + mergeRequest(testReq, tempReq) + } + if tempReq.NamespaceName != "" { testReq.NamespaceName = tempReq.NamespaceName } @@ -499,10 +525,6 @@ func mergeTestRequests(testReq, tempReq *testRequest) { testReq.UserInfo = tempReq.UserInfo } - if tempReq.Params != nil { - testReq.Params = tempReq.Params - } - if tempReq.ExpectAuditAnnotations != nil { testReq.ExpectAuditAnnotations = tempReq.ExpectAuditAnnotations } @@ -526,6 +548,8 @@ func mergeTestRequests(testReq, tempReq *testRequest) { if len(tempReq.Authorizer) > 0 { testReq.Authorizer = tempReq.Authorizer } + + return nil } // mergeRequest merges fields from tempReq into testReq (tempReq takes precedence). diff --git a/internal/loader/suite_test.go b/internal/loader/suite_test.go index b7d6a5a..26ea8cc 100644 --- a/internal/loader/suite_test.go +++ b/internal/loader/suite_test.go @@ -530,7 +530,9 @@ func TestMergeTestRequests(t *testing.T) { ExpectMessage: "merged", } - mergeTestRequests(testReq, tempReq) + if err := mergeTestRequests(testReq, tempReq); err != nil { + t.Fatalf("mergeTestRequests() unexpected error: %v", err) + } if testReq.Request.Operation != "UPDATE" { t.Errorf("Expected Operation merged to UPDATE, got %s", testReq.Request.Operation) @@ -648,6 +650,72 @@ func TestMergeRequest_AllFields(t *testing.T) { } } +func TestMergeTestRequests_Conflicts(t *testing.T) { + t.Parallel() + + obj := &unstructured.Unstructured{Object: map[string]interface{}{"kind": "Pod"}} + + tests := []struct { + name string + base testRequest + overlay testRequest + wantErr string + }{ + { + name: "no conflict – base empty", + overlay: testRequest{Object: obj}, + wantErr: "", + }, + { + name: "conflict – object in both", + base: testRequest{Object: obj}, + overlay: testRequest{Object: obj}, + wantErr: "conflict: object defined in multiple files", + }, + { + name: "conflict – oldObject in both", + base: testRequest{OldObject: obj}, + overlay: testRequest{OldObject: obj}, + wantErr: "conflict: oldObject defined in multiple files", + }, + { + name: "conflict – namespaceObject in both", + base: testRequest{NamespaceObj: obj}, + overlay: testRequest{NamespaceObj: obj}, + wantErr: "conflict: namespaceObject defined in multiple files", + }, + { + name: "conflict – params in both", + base: testRequest{Params: obj}, + overlay: testRequest{Params: obj}, + wantErr: "conflict: params defined in multiple files", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + base := tt.base + overlay := tt.overlay + err := mergeTestRequests(&base, &overlay) + + if tt.wantErr == "" { + if err != nil { + t.Errorf("mergeTestRequests() unexpected error: %v", err) + } + } else { + if err == nil { + t.Fatalf("mergeTestRequests() expected error %q, got nil", tt.wantErr) + } + if err.Error() != tt.wantErr { + t.Errorf("mergeTestRequests() error = %q, want %q", err.Error(), tt.wantErr) + } + } + }) + } +} + func testGroupVersionResource(version, resource string) metav1.GroupVersionResource { return metav1.GroupVersionResource{Version: version, Resource: resource} } diff --git a/test-policies-fail/add-default-labels/tests/add-default-labels.missing-gold.object.yaml b/test-policies-fail/add-default-labels/tests/add-default-labels.missing-gold.object.yaml new file mode 100644 index 0000000..7eeb2f2 --- /dev/null +++ b/test-policies-fail/add-default-labels/tests/add-default-labels.missing-gold.object.yaml @@ -0,0 +1,17 @@ +apiVersion: apps/v1 +kind: Deployment +metadata: + name: test-deployment-no-gold +spec: + replicas: 1 + selector: + matchLabels: + app: test + template: + metadata: + labels: + app: test + spec: + containers: + - name: nginx + image: nginx diff --git a/test-policies-fail/conditional-policy/tests/conditional.object-conflict.allow.object.yaml b/test-policies-fail/conditional-policy/tests/conditional.object-conflict.allow.object.yaml new file mode 100644 index 0000000..5096184 --- /dev/null +++ b/test-policies-fail/conditional-policy/tests/conditional.object-conflict.allow.object.yaml @@ -0,0 +1,18 @@ +apiVersion: apps/v1 +kind: Deployment +metadata: + name: dev-deployment + namespace: development +spec: + replicas: 1 + selector: + matchLabels: + app: test + template: + metadata: + labels: + app: test + spec: + containers: + - name: nginx + image: nginx diff --git a/test-policies-fail/conditional-policy/tests/conditional.object-conflict.allow.request.yaml b/test-policies-fail/conditional-policy/tests/conditional.object-conflict.allow.request.yaml new file mode 100644 index 0000000..25a33b1 --- /dev/null +++ b/test-policies-fail/conditional-policy/tests/conditional.object-conflict.allow.request.yaml @@ -0,0 +1,27 @@ +namespace: development +namespaceObject: + apiVersion: v1 + kind: Namespace + metadata: + name: development + labels: + environment: dev +object: + apiVersion: apps/v1 + kind: Deployment + metadata: + name: dev-deployment-from-request + namespace: development + spec: + replicas: 1 + selector: + matchLabels: + app: test + template: + metadata: + labels: + app: test + spec: + containers: + - name: nginx + image: nginx diff --git a/test-policies-pass/mutating/mutating-with-binding/tests/add-label.all-in-one.allowed.gold.yaml b/test-policies-pass/mutating/mutating-with-binding/tests/add-label.all-in-one.allowed.gold.yaml new file mode 100644 index 0000000..d56d011 --- /dev/null +++ b/test-policies-pass/mutating/mutating-with-binding/tests/add-label.all-in-one.allowed.gold.yaml @@ -0,0 +1,12 @@ +apiVersion: v1 +kind: Pod +metadata: + name: test-pod-allinone + namespace: default + labels: + app: test + managed-by: kat-test +spec: + containers: + - name: nginx + image: nginx:latest diff --git a/test-policies-pass/mutating/mutating-with-binding/tests/add-label.all-in-one.allowed.request.yaml b/test-policies-pass/mutating/mutating-with-binding/tests/add-label.all-in-one.allowed.request.yaml new file mode 100644 index 0000000..c8bd60b --- /dev/null +++ b/test-policies-pass/mutating/mutating-with-binding/tests/add-label.all-in-one.allowed.request.yaml @@ -0,0 +1,22 @@ +operation: CREATE +object: + apiVersion: v1 + kind: Pod + metadata: + name: test-pod-allinone + namespace: default + labels: + app: test + spec: + containers: + - name: nginx + image: nginx:latest +params: + apiVersion: v1 + kind: ConfigMap + metadata: + name: label-config + namespace: default + data: + labelKey: "managed-by" + labelValue: "kat-test" diff --git a/test-policies-pass/mutating/namespace-selector-binding-mutating/tests/namespace-selector-binding-mutating-test.prod-namespace.mutate.expected.yaml b/test-policies-pass/mutating/namespace-selector-binding-mutating/tests/namespace-selector-binding-mutating-test.prod-namespace.mutate.gold.yaml similarity index 100% rename from test-policies-pass/mutating/namespace-selector-binding-mutating/tests/namespace-selector-binding-mutating-test.prod-namespace.mutate.expected.yaml rename to test-policies-pass/mutating/namespace-selector-binding-mutating/tests/namespace-selector-binding-mutating-test.prod-namespace.mutate.gold.yaml diff --git a/test-policies-pass/validating/conditional-policy/tests/conditional.all-in-one.allow.request.yaml b/test-policies-pass/validating/conditional-policy/tests/conditional.all-in-one.allow.request.yaml new file mode 100644 index 0000000..6d290bd --- /dev/null +++ b/test-policies-pass/validating/conditional-policy/tests/conditional.all-in-one.allow.request.yaml @@ -0,0 +1,28 @@ +operation: CREATE +namespace: development +namespaceObject: + apiVersion: v1 + kind: Namespace + metadata: + name: development + labels: + environment: dev +object: + apiVersion: apps/v1 + kind: Deployment + metadata: + name: dev-deployment-allinone + namespace: development + spec: + replicas: 1 + selector: + matchLabels: + app: test + template: + metadata: + labels: + app: test + spec: + containers: + - name: nginx + image: nginx diff --git a/test-policies-pass/validating/replica-limit-with-params/tests/replica-limit-params.all-in-one.deny.request.yaml b/test-policies-pass/validating/replica-limit-with-params/tests/replica-limit-params.all-in-one.deny.request.yaml new file mode 100644 index 0000000..e292eb4 --- /dev/null +++ b/test-policies-pass/validating/replica-limit-with-params/tests/replica-limit-params.all-in-one.deny.request.yaml @@ -0,0 +1,27 @@ +operation: CREATE +object: + apiVersion: apps/v1 + kind: Deployment + metadata: + name: large-deployment-allinone + spec: + replicas: 10 + selector: + matchLabels: + app: test + template: + metadata: + labels: + app: test + spec: + containers: + - name: nginx + image: nginx +params: + apiVersion: v1 + kind: ConfigMap + metadata: + name: replica-limit-config + namespace: default + data: + maxReplicas: "5" diff --git a/testdata/fail_policies.golden b/testdata/fail_policies.golden index 46a2133..45432f6 100644 --- a/testdata/fail_policies.golden +++ b/testdata/fail_policies.golden @@ -1,4 +1,6 @@ +--- FAIL: add-default-labels/add-default-labels.missing-gold.yaml (0.00s) + policy mutated the object but no .gold.yaml file was provided --- FAIL: add-default-labels/add-default-labels.no-labels.yaml (0.00s) mutated object does not match expected: --- Expected @@ -33,6 +35,8 @@ FAIL block-team-ci-service-accounts 0.000s --- FAIL: conditional-policy/conditional.dev-single-replica.allow.yaml (0.00s) expected allowed=true, got allowed=false +--- FAIL: conditional-policy/conditional.object-conflict.allow.yaml (0.00s) + test loading error: conflict: object defined in multiple files --- FAIL: conditional-policy/conditional.prod-ha.deny.yaml (0.00s) test loading error: failed to parse test file test-policies-fail/conditional-policy/tests/conditional.prod-ha.deny.object.yaml: object: invalid kubernetes object: strict decoding error: unknown field "spec.template.spec.container" FAIL conditional-policy 0.000s diff --git a/testdata/json_output.golden b/testdata/json_output.golden index a366094..2e4d6c3 100644 --- a/testdata/json_output.golden +++ b/testdata/json_output.golden @@ -5,6 +5,8 @@ {"time":"2000-01-01T00:00:00Z","action":"pass","package":"add-default-labels","test":"add-default-labels.no-labels.yaml","elapsed":0} {"time":"2000-01-01T00:00:00Z","action":"pass","package":"add-default-labels","elapsed":0} {"time":"2000-01-01T00:00:00Z","action":"run","package":"mutating-with-binding"} +{"time":"2000-01-01T00:00:00Z","action":"run","package":"mutating-with-binding","test":"add-label.all-in-one.allowed.yaml"} +{"time":"2000-01-01T00:00:00Z","action":"pass","package":"mutating-with-binding","test":"add-label.all-in-one.allowed.yaml","elapsed":0} {"time":"2000-01-01T00:00:00Z","action":"run","package":"mutating-with-binding","test":"add-label.allowed.yaml"} {"time":"2000-01-01T00:00:00Z","action":"pass","package":"mutating-with-binding","test":"add-label.allowed.yaml","elapsed":0} {"time":"2000-01-01T00:00:00Z","action":"run","package":"mutating-with-binding","test":"no-params.allowed.yaml"} diff --git a/testdata/recursion_dot.golden b/testdata/recursion_dot.golden index 102ab18..687991b 100644 --- a/testdata/recursion_dot.golden +++ b/testdata/recursion_dot.golden @@ -1,4 +1,6 @@ +--- FAIL: add-default-labels/add-default-labels.missing-gold.yaml (0.00s) + policy mutated the object but no .gold.yaml file was provided --- FAIL: add-default-labels/add-default-labels.no-labels.yaml (0.00s) mutated object does not match expected: --- Expected @@ -33,6 +35,8 @@ FAIL block-team-ci-service-accounts 0.000s --- FAIL: conditional-policy/conditional.dev-single-replica.allow.yaml (0.00s) expected allowed=true, got allowed=false +--- FAIL: conditional-policy/conditional.object-conflict.allow.yaml (0.00s) + test loading error: conflict: object defined in multiple files --- FAIL: conditional-policy/conditional.prod-ha.deny.yaml (0.00s) test loading error: failed to parse test file test-policies-fail/conditional-policy/tests/conditional.prod-ha.deny.object.yaml: object: invalid kubernetes object: strict decoding error: unknown field "spec.template.spec.container" FAIL conditional-policy 0.000s From 5bd71907deb28dedd6ac05adc3c7b74f26d7e3fd Mon Sep 17 00:00:00 2001 From: Vlastimil Zeman Date: Fri, 24 Apr 2026 16:15:33 +0100 Subject: [PATCH 2/2] fix: resolve golangci-lint errors (cyclop, err113, funlen, wsl) --- internal/loader/errors.go | 4 +++ internal/loader/request.go | 6 ++--- internal/loader/request_test.go | 4 +++ internal/loader/suite.go | 31 +++++++++++++++-------- internal/loader/suite_test.go | 45 +++++++-------------------------- 5 files changed, 41 insertions(+), 49 deletions(-) diff --git a/internal/loader/errors.go b/internal/loader/errors.go index b04cb0d..282959b 100644 --- a/internal/loader/errors.go +++ b/internal/loader/errors.go @@ -14,4 +14,8 @@ var ( ErrUnsupportedV1Beta1Binding = errors.New("ValidatingAdmissionPolicyBinding v1beta1 not supported, use v1") ErrUnsupportedV1Beta1MutPolicy = errors.New("MutatingAdmissionPolicy v1beta1 not supported, use v1") ErrUnsupportedV1Beta1MutBinding = errors.New("MutatingAdmissionPolicyBinding v1beta1 not supported, use v1") + ErrConflictObject = errors.New("conflict: object defined in multiple files") + ErrConflictOldObject = errors.New("conflict: oldObject defined in multiple files") + ErrConflictNamespaceObject = errors.New("conflict: namespaceObject defined in multiple files") + ErrConflictParams = errors.New("conflict: params defined in multiple files") ) diff --git a/internal/loader/request.go b/internal/loader/request.go index 3517263..bd516f0 100644 --- a/internal/loader/request.go +++ b/internal/loader/request.go @@ -28,9 +28,8 @@ var ( ) // parseTestRequestFile parses a test request file and populates the TestRequest. -// Handles *.request.yaml (simplified AdmissionRequest format), *.object.yaml (raw Kubernetes object), -// *.oldObject.yaml (object for DELETE operations), *.params.yaml (policy parameters), -// *.annotations.yaml (expected audit annotations), and *.warnings.txt (expected warnings). +// +//nolint:cyclop // Simple dispatch switch, each case is a one-liner. func parseTestRequestFile(testReq *testRequest) error { data, err := os.ReadFile(testReq.FilePath) if err != nil { @@ -296,6 +295,7 @@ func loadAuxiliaryFiles(testReq *testRequest) error { func loadGoldFile(testReq *testRequest) error { goldPath := strings.Replace(testReq.FilePath, ".object.yaml", ".gold.yaml", 1) goldPath = strings.Replace(goldPath, ".request.yaml", ".gold.yaml", 1) + if _, err := os.Stat(goldPath); err != nil { if os.IsNotExist(err) { return nil diff --git a/internal/loader/request_test.go b/internal/loader/request_test.go index 120aa4a..b1cfa7b 100644 --- a/internal/loader/request_test.go +++ b/internal/loader/request_test.go @@ -254,11 +254,13 @@ params: data: maxReplicas: "5" ` + if err := os.WriteFile(requestFile, []byte(content), 0o600); err != nil { t.Fatal(err) } req := &testRequest{Name: "test.allow", FilePath: requestFile} + data, err := os.ReadFile(requestFile) if err != nil { t.Fatal(err) @@ -277,6 +279,7 @@ params: } } +//nolint:funlen // Test function length is due to YAML test data. func TestParseRequestYAML_GoldFile(t *testing.T) { t.Parallel() @@ -318,6 +321,7 @@ spec: } req := &testRequest{Name: "test", FilePath: requestFile} + data, err := os.ReadFile(requestFile) if err != nil { t.Fatal(err) diff --git a/internal/loader/suite.go b/internal/loader/suite.go index 31b0498..dc6c9db 100644 --- a/internal/loader/suite.go +++ b/internal/loader/suite.go @@ -479,11 +479,24 @@ func newTempTestRequest(filePath, policyName string, expectAllowed bool) *testRe } } -//nolint:cyclop // Merge function with many fields func mergeTestRequests(testReq, tempReq *testRequest) error { + if err := mergeConflictFields(testReq, tempReq); err != nil { + return err + } + + if tempReq.Request != nil { + mergeRequest(testReq, tempReq) + } + + mergeSimpleFields(testReq, tempReq) + + return nil +} + +func mergeConflictFields(testReq, tempReq *testRequest) error { if tempReq.Object != nil { if testReq.Object != nil { - return fmt.Errorf("conflict: object defined in multiple files") + return ErrConflictObject } testReq.Object = tempReq.Object @@ -491,7 +504,7 @@ func mergeTestRequests(testReq, tempReq *testRequest) error { if tempReq.OldObject != nil { if testReq.OldObject != nil { - return fmt.Errorf("conflict: oldObject defined in multiple files") + return ErrConflictOldObject } testReq.OldObject = tempReq.OldObject @@ -499,7 +512,7 @@ func mergeTestRequests(testReq, tempReq *testRequest) error { if tempReq.NamespaceObj != nil { if testReq.NamespaceObj != nil { - return fmt.Errorf("conflict: namespaceObject defined in multiple files") + return ErrConflictNamespaceObject } testReq.NamespaceObj = tempReq.NamespaceObj @@ -507,16 +520,16 @@ func mergeTestRequests(testReq, tempReq *testRequest) error { if tempReq.Params != nil { if testReq.Params != nil { - return fmt.Errorf("conflict: params defined in multiple files") + return ErrConflictParams } testReq.Params = tempReq.Params } - if tempReq.Request != nil { - mergeRequest(testReq, tempReq) - } + return nil +} +func mergeSimpleFields(testReq, tempReq *testRequest) { if tempReq.NamespaceName != "" { testReq.NamespaceName = tempReq.NamespaceName } @@ -548,8 +561,6 @@ func mergeTestRequests(testReq, tempReq *testRequest) error { if len(tempReq.Authorizer) > 0 { testReq.Authorizer = tempReq.Authorizer } - - return nil } // mergeRequest merges fields from tempReq into testReq (tempReq takes precedence). diff --git a/internal/loader/suite_test.go b/internal/loader/suite_test.go index 26ea8cc..b7dc38b 100644 --- a/internal/loader/suite_test.go +++ b/internal/loader/suite_test.go @@ -659,37 +659,13 @@ func TestMergeTestRequests_Conflicts(t *testing.T) { name string base testRequest overlay testRequest - wantErr string + wantErr error }{ - { - name: "no conflict – base empty", - overlay: testRequest{Object: obj}, - wantErr: "", - }, - { - name: "conflict – object in both", - base: testRequest{Object: obj}, - overlay: testRequest{Object: obj}, - wantErr: "conflict: object defined in multiple files", - }, - { - name: "conflict – oldObject in both", - base: testRequest{OldObject: obj}, - overlay: testRequest{OldObject: obj}, - wantErr: "conflict: oldObject defined in multiple files", - }, - { - name: "conflict – namespaceObject in both", - base: testRequest{NamespaceObj: obj}, - overlay: testRequest{NamespaceObj: obj}, - wantErr: "conflict: namespaceObject defined in multiple files", - }, - { - name: "conflict – params in both", - base: testRequest{Params: obj}, - overlay: testRequest{Params: obj}, - wantErr: "conflict: params defined in multiple files", - }, + {name: "no conflict – base empty", overlay: testRequest{Object: obj}}, + {name: "conflict – object", base: testRequest{Object: obj}, overlay: testRequest{Object: obj}, wantErr: ErrConflictObject}, + {name: "conflict – oldObject", base: testRequest{OldObject: obj}, overlay: testRequest{OldObject: obj}, wantErr: ErrConflictOldObject}, + {name: "conflict – namespaceObject", base: testRequest{NamespaceObj: obj}, overlay: testRequest{NamespaceObj: obj}, wantErr: ErrConflictNamespaceObject}, + {name: "conflict – params", base: testRequest{Params: obj}, overlay: testRequest{Params: obj}, wantErr: ErrConflictParams}, } for _, tt := range tests { @@ -700,16 +676,13 @@ func TestMergeTestRequests_Conflicts(t *testing.T) { overlay := tt.overlay err := mergeTestRequests(&base, &overlay) - if tt.wantErr == "" { + if tt.wantErr == nil { if err != nil { t.Errorf("mergeTestRequests() unexpected error: %v", err) } } else { - if err == nil { - t.Fatalf("mergeTestRequests() expected error %q, got nil", tt.wantErr) - } - if err.Error() != tt.wantErr { - t.Errorf("mergeTestRequests() error = %q, want %q", err.Error(), tt.wantErr) + if !errors.Is(err, tt.wantErr) { + t.Errorf("mergeTestRequests() error = %v, want %v", err, tt.wantErr) } } })