Flpath 4720 4721 rfc9457 e2e contract tests - #47
Conversation
Verify deployed service providers emit problem+json with project-controlled type URIs, matching status fields, and container multi-error validation. Signed-off-by: Vladislav Kolodny <vkolodny@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
…0/4721 Update container and ACM cluster SP test plans with RFC 9457 references, upstream PR links, and new TC-19/TC-20 and TC-14/TC-15 E2E cases. Signed-off-by: Vladislav Kolodny <vkolodny@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
PR Summary by QodoAdd RFC 9457 E2E contracts for container and ACM providers
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Parse Content-Type with mime.ParseMediaType, compare status as float64, require >=2 multi-errors with CPU and reserved-label coverage in details. Signed-off-by: Vladislav Kolodny <vkolodny@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
Code review by qodo was updated up to the latest commit b9f790f |
|
@vkolodny I just noticed the PR description was empty. |
| }) | ||
|
|
||
| It("returns errors array on multi-field validation failure", func() { | ||
| body := `{ |
There was a problem hiding this comment.
Why unstructured data? can't we use the go structure here?
There was a problem hiding this comment.
Agreed — addressed in 8ae2c94.
Added local typed structs in tests/e2e/problem_detail_test.go:
ProblemDetail— RFC 9457 core fields shared by all SPsContainerProblemDetail— addserrors[]for k8s-container SP only
They mirror the OpenAPI Error schema at the DCM boundary but are owned in dcm-utilities (no SP repo import). Contract tests now decode into these structs via expectRFC9457Problem() / readContainerProblemDetail() instead of map[string]interface{}.
Request bodies for the multi-error case stay as JSON strings for now since we're testing the deployed HTTP wire format; happy to type those too if you prefer.
There was a problem hiding this comment.
why use a json in a string? this is very brittle and will break if there are changes to the json structure and you will have to manually refactor this. Why not use a go structure and marshal it? It will ensure it survives refactoring and reduce cost maintenance.
There was a problem hiding this comment.
Valid payloads already use struct→marshal (containerSpec()). The multi-error body intentionally sends invalid values (cpu.min > max, reserved label) that a typed struct would reject at compile time, so inline JSON is simpler for these negative-path contract tests.
| } | ||
| } | ||
| }` | ||
| resp, err := doContainerSPRequest(http.MethodPost, "/containers", body) |
There was a problem hiding this comment.
I'm curious, we don't have a go client bindings available that we have to use pure http client?
There was a problem hiding this comment.
Good question. Today there is no published Go HTTP client for the container or ACM cluster SPs that dcm-utilities can import.
In the SP repos, oapi-codegen generates server stubs (ServerInterface, handlers) and shared types (api/v1alpha1/types.gen.go), but not a client package. By contrast, control-plane subsystem tests do use generated clients — but that is for the control-plane APIs, not the standalone SP HTTP endpoints we exercise here.
These E2E tests are intentionally black-box against the deployed SP ports (DCM_CONTAINER_SP_URL / DCM_ACM_CLUSTER_SP_URL, default :8082/:8083). The whole sp_* suite uses the same raw http.Client pattern (doContainerSPRequest, doAcmClusterSPRequest) so we validate the wire contract of whatever image compose publishes, without taking a compile-time dependency on a specific SP module version.
For the response side, we added local typed structs in problem_detail_test.go (ProblemDetail, ContainerProblemDetail) that mirror the OpenAPI Error schema at the DCM boundary — typed decoding without importing the SP repo.
A shared generated client could make sense later (e.g. SP SDK / UDLM alignment), but it is out of scope for this RFC 9457 contract PR. Happy to track a follow-up if we want client codegen for E2E.
There was a problem hiding this comment.
That's not quite right — both repos do generate a client. k8s-container-service-provider and acm-cluster-service-provider each have pkg/client/client.gen.go (oapi-codegen, generate: client: true), wired into generate-api and enforced by check-generate-api in CI — same pattern as control-plane's policy/catalog/sp clients and osac-service-provider's own pkg/client.
Given that, hand-rolling doContainerSPRequest/doAcmClusterSPRequest against raw http.Client is reinventing what's already generated and CI-verified. Can we import the generated clients here instead? Rough sketch using acm-cluster-service-provider:
import (
acmclient "github.com/dcm-project/acm-cluster-service-provider/pkg/client"
)
client, err := acmclient.NewClientWithResponses(acmClusterSPBaseURL)
Expect(err).NotTo(HaveOccurred())
resp, err := client.CreateClusterWithResponse(ctx, &acmclient.CreateClusterParams{}, acmclient.CreateClusterJSONRequestBody{ /* ... */ })
Expect(err).NotTo(HaveOccurred())
Expect(resp.StatusCode()).To(Equal(http.StatusCreated))Same shape for k8s-container-service-provider's client. Would need adding both as go.mod deps in tests/e2e/.
There was a problem hiding this comment.
You're right, apologies — both SPs do have generated clients. For these contract tests raw HTTP is intentional (validating wire format independent of client abstractions), but migrating the broader E2E suite to generated clients makes sense as a follow-up PR.
| problem := expectRFC9457Problem(resp, http.StatusBadRequest, "invalid-argument", "Invalid argument") | ||
| errors, ok := problem["errors"].([]interface{}) | ||
| Expect(ok).To(BeTrue(), "expected errors array in multi-field validation response, got %#v", problem["errors"]) | ||
| Expect(len(errors)).To(BeNumerically(">=", 2)) |
There was a problem hiding this comment.
Can you be more deterministic and account for the actual number of failures you expect (not < or > or similar, but equal to). This will make the test capture regressions in the future.
You can also validate which errors are happening, that's a stretch goal worth considering.
There was a problem hiding this comment.
Addressed in 8ae2c94.
Expect(problem.Errors).To(HaveLen(2))— exact count, not>=- Validates which errors: index 0 is CPU range (
#/spec/resources/cpu/min), index 1 is reserved label (#/spec/metadata/labels/dcm.project~1managed-by), matching SP unit test TC-U092 ordering - Detail strings use
ContainSubstringfor now; happy to tighten to full exact messages if you prefer
|
|
||
| var detailsBlob strings.Builder | ||
| for i, e := range errors { | ||
| entry, ok := e.(map[string]interface{}) |
There was a problem hiding this comment.
we don't have an existing go structure for errors? Using unstructured data like you're doing here is brittle and increases maintenance burden because refactoring is not possible.
There was a problem hiding this comment.
Same fix as #1 — addressed in 8ae2c94.
The SP repos do generate Error / ErrorDetail in api/v1alpha1/types.gen.go, but we intentionally mirror them locally in tests/e2e/problem_detail_test.go rather than importing the SP module into dcm-utilities (keeps E2E decoupled from SP release/version).
Contract tests no longer decode errors into map[string]interface{} — they use ProblemDetail / ContainerProblemDetail with field-level assertions, so renames to JSON tags would fail at compile time in the test package.
| decodeJSON(resp, &problem) | ||
|
|
||
| Expect(problem).To(HaveKeyWithValue("type", problemTypeBaseURI+wantTypeSuffix)) | ||
| Expect(problem).To(HaveKeyWithValue("title", wantTitle)) |
There was a problem hiding this comment.
this works, but I'd be more throughout and actually expect the full message, rather than a contains. Is that possible?
There was a problem hiding this comment.
Yes — partially addressed in 8ae2c94.
Already exact match today:
title—Expect(problem.Title).To(Equal(want.Title))type,status— exact- Multi-error top-level
detail— exactcontainerMultiErrorDetailconstant (matches SPhttperror.InvalidArgumentMultiDetail)
problemDetailExpectation.Detail: when set, the helper asserts full exact detail; when empty (e.g. simple POST {} validation), we only require non-empty because the message can come from OpenAPI middleware vs handler and may vary slightly.
Per-entry errors[].detail: still ContainSubstring in the multi-error test — can tighten to the full strings from validation.go if you want maximum strictness:
cpu.min (10) must not exceed cpu.max (5)label "dcm.project/managed-by" is reserved by DCM and cannot be set by the user
Happy to do that in a follow-up commit.
|
|
||
| Expect(problem).To(HaveKeyWithValue("type", problemTypeBaseURI+wantTypeSuffix)) | ||
| Expect(problem).To(HaveKeyWithValue("title", wantTitle)) | ||
| Expect(problem).To(HaveKey("detail")) |
There was a problem hiding this comment.
do you check the value of the detail key?.
There was a problem hiding this comment.
Partially — when detail is stable we assert the exact value.
Exact today:
- Multi-error test: top-level
detailisEqual(containerMultiErrorDetail)via whole-struct comparison onProblemDetail
Non-empty only today:
- Simple
POST {}validation (container + ACM) and ACM 404 —expectRFC9457Problemrequiresdetailis present and non-empty whenwant.Detailis left empty
Why not exact for those cases:
- Container SP has no single-error
detailconstant (onlyInvalidArgumentMultiDetailfor multi-error) - SP integration tests (TC-U014 for
POST {}) asserttype/title/statusbut not exactdetail— messages come from OpenAPI middleware and can vary with schema/codegen - ACM 404 similarly has no exported canonical
detailstring
The helper already supports tightening: set want.Detail when we have a shared constant or after a lab E2E capture pins the strings. Happy to do that in a follow-up if you want maximum strictness.
| } | ||
|
|
||
| // expectRFC9457Problem asserts an RFC 9457 problem+json response (FLPATH-4720/4721). | ||
| func expectRFC9457Problem(resp *http.Response, wantStatus int, wantTypeSuffix, wantTitle string) map[string]interface{} { |
There was a problem hiding this comment.
have you considered passing a structure that contains the error and doing the comparison directly against the structure rather than each field at a time. The problem with the current approach is that if the structure is extended with new fields, you will not catch the drift using this approach.
There was a problem hiding this comment.
Multi-error test already uses whole-struct Equal on ProblemDetail (commit 8ae2c94). Note that Go's json.Unmarshal silently drops unknown fields, so struct comparison won't catch new SP fields either — drift protection comes from keeping the local types aligned with the OpenAPI schema. Could add MatchJSON on raw bytes for full-body comparison if needed.
Add DCM-boundary ProblemDetail types (RFC 9457 core plus container errors[] extension), refactor helpers to decode into structs, and assert exact multi-error pointers matching SP unit test TC-U092. Signed-off-by: Vladislav Kolodny <vkolodny@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
| }) | ||
|
|
||
| It("returns errors array on multi-field validation failure", func() { | ||
| body := `{ |
There was a problem hiding this comment.
why use a json in a string? this is very brittle and will break if there are changes to the json structure and you will have to manually refactor this. Why not use a go structure and marshal it? It will ensure it survives refactoring and reduce cost maintenance.
| requireKubectl() | ||
| _, err := runKubectl("get", "crd", "hostedclusters.hypershift.openshift.io") | ||
| if err != nil { | ||
| Skip("HyperShift CRDs required — without them GET returns 500 instead of 404") |
There was a problem hiding this comment.
why not install the CRDs as part of the test fixtures and avoid this step?
There was a problem hiding this comment.
HyperShift CRDs are installed by the HyperShift operator, not standalone manifests we can apply as fixtures. The ACM cluster SP requires a real HyperShift environment to return 404 vs 500. The Skip keeps the test safe to run on any cluster.
| if want.Detail != "" { | ||
| Expect(problem.Detail).To(Equal(want.Detail)) | ||
| } else { | ||
| Expect(problem.Detail).NotTo(BeEmpty()) |
There was a problem hiding this comment.
why not validate the contents of the field here if it's not empty?
There was a problem hiding this comment.
Same rationale as my reply on #6 above — no stable SP constant for single-error detail. The helper supports exact match via want.Detail when we have one; will tighten after a lab capture or if the SPs export the strings.
4 participants
No description provided.