nic-planning

# Planning and Task Approach ## Before Writing Code 1. **Read the requirement** — Understand what's being asked. Check linked issues, specs, or PRs for full context. 2. **Write acceptance criteria** — Before implementing, state what "done" looks like in testable terms. What should work? What should be rejected? What must NOT break? 3. **Identify security impact** — Does this change: - Accept new external/untrusted input? → Requires input validation at the trust boundary - Touch credential or secret paths? → Requires human review - Generate config from user data? → Must pass through `containsDangerousChars()` - Change RBAC or access control? → Requires security review 4. **Identify affected layers** — Determine which architectural layers are touched: - Data Model (`pkg/apis/configuration/v1/types.go`) - Validation (`pkg/apis/configuration/validation/`) - Controller (`internal/k8s/`) - Config Generation (`internal/configs/`) - Templates (`internal/configs/version1/` or `version2/`) - Process Management (`internal/nginx/`) - Helm Chart (`charts/nginx-ingress/`) 5. **Check invariants** — Review the Key Invariants section in AGENTS.md: - Security: `containsDangerousChars()` on user strings reaching NGINX config - Codegen: Never edit `zz_generated.deepcopy.go` manually - Templates: Update BOTH OSS and Plus variants for shared directives; Plus-only directives go in the Plus template only - CRD fields: Every new field needs kubebuilder markers + validation + template + tests 6. **Identify test surface** — What tests need adding or updating? - Unit tests for validation logic - Negative tests for input rejection (security) - Snapshot tests for template output - Helm tests if chart changes - Integration tests if behaviour changes 7. **Produce a plan** — State your approach before coding. List files to change in order. Include what this change must NOT break. ## Layer Impact Checklist For any change, ask: - [ ] Does it accept new external input? → Add validation with `containsDangerousChars()` or appropriate sanitizer - [ ] Does it touch `types.go`? → Run `make update-codegen` then `make update-crds` - [ ] Does it add a template directive? → Update BOTH `nginx.ingress.tmpl` AND `nginx-plus.ingress.tmpl` (or v2 equivalents) if the directive is shared; Plus-only directives go in the Plus template alone - [ ] Does it add a CRD field? → Add kubebuilder markers, validation, template struct, rendering, tests - [ ] Does it touch Helm values? → Update `values.yaml`, `values.schema.json`, and helmunit tests - [ ] Does it affect config generation? → Add a snapshot fixture that exercises the change, then run `make test-update-snaps` - [ ] Does it change telemetry data types? → Run `make telemetry-schema` - [ ] Does user-controlled data reach NGINX config? → Verify sanitization path exists and is tested ## Definition of Done Do not report a task as complete until every applicable box is ticked. These are the steps most often skipped. | Condition | Required action | Verification | | --- | --- | --- | | Edited any `.tmpl` | Snapshot fixture added **and** regenerated | `git diff -- '**/__snapshots__/**'` is non-empty and shows the new directive | | Edited a template struct (`version1/config.go`, `version2/http.go`, `version2/stream.go`) | Fixture populates the field, snapshots regenerated | Same as above | | Edited `pkg/apis/**/types.go` | `make update-codegen && make update-crds` | `git status` shows regenerated `pkg/**`, `config/crd/bases`, `deploy/crds*.yaml`, `docs/crd/` | | Edited telemetry `Data` / `NICResourceCounts` | `make telemetry-schema` | No diff on re-run | | Added/changed imports | `go mod tidy` | `go.mod` / `go.sum` clean | | Edited chart templates or values | testdata + helmunit case | `charts/tests/__snapshots__/` diff is non-empty | | Added a pytest marker | Registered in `pyproject.toml` | Suite runs under `--strict-markers` | | Any of the above | `make test` then `make lint` | Both pass | **Snapshot rule:** regenerating without adding a fixture produces an empty diff, which is a *silent failure*, not a success. If `make test-update-snaps` changes nothing after a template edit, you have not tested the feature. ## Scope Assessment | Scope | Indicators | Action | | --- | --- | --- | | Trivial | Typo, docs, comment fix | Fix directly, no plan needed | | Small | Single layer, <50 lines, no API change | Brief plan → implement → test | | Medium | 2-3 layers, new field or annotation | Detailed plan → implement layer by layer → test each | | Large | New subsystem, new policy type, cross-cutting | Write plan document → get approval → implement in stages | ## Common Planning Mistakes - Starting implementation before understanding the full scope of affected files - Forgetting to update BOTH OSS and Plus templates for a shared directive -- or the inverse, leaking a Plus-only directive into the OSS template - Changing `types.go` without running codegen - Adding a VirtualServer feature without checking if Ingress (v1) also needs it - Adding Helm values without updating the JSON schema - Not checking if the feature already exists as an annotation when adding a CRD field - Skipping snapshot regeneration after template changes -- or regenerating without adding a fixture, which silently produces no diff - Forgetting `make telemetry-schema` after touching telemetry data types - Assuming a directive exists or behaves a certain way without checking <https://nginx.org/en/docs/> -- verify NGINX semantics before wiring a template - Not identifying where untrusted input enters — a new CRD field IS user input that reaches NGINX config - Skipping negative tests — only testing the happy path leaves injection vectors undiscovered ## Ordering Rules for Multi-Layer Changes When a change spans multiple layers, implement in this order: 1. **Data model** — Define types/fields in `types.go` 2. **Codegen** — `make update-codegen && make update-crds` 3. **Validation** — Add validation rules in `pkg/apis/configuration/validation/` 4. **Config structs** — Add fields to template structs in `version1/` or `version2/` 5. **Config generation** — Wire the new field into config builders in `internal/configs/` 6. **Templates** — Add NGINX directives to `.tmpl` files (OSS + Plus) 7. **Controller** — Wire into sync handlers if needed 8. **Helm** — Update chart values, schema, templates 9. **Tests** — Unit, negative, snapshot (fixture + regenerate), helm, integration 10. **Regenerate** — `make update-codegen`, `make update-crds`, `make telemetry-schema`, `make test-update-snaps`, then `make test` and `make lint`