review - GW API conformance options

README.md

Adversarial Review: yaml-conformance-options Branch

Summary Verdict

This code should not be merged as-is. There is one critical correctness bug in TimeoutConfig.UnmarshalJSON that will cause silent data loss for valid YAML inputs, a behavioral regression for programmatic ConformanceOptions callers around Mode defaults, and several less severe issues around flag-append semantics, global test state mutation, and missing test coverage. The architectural direction is sound — separating configurable options from infrastructure options and supporting YAML configuration is a good move — but the implementation has gaps that need to be closed before merge.

---

Critical Findings

[conformance/utils/config/timeout.go:128-129] — UnmarshalJSON unmarshals into map[string]string, rejecting valid YAML inputs

The custom UnmarshalJSON does json.Unmarshal(data, &raw) where raw is map[string]string. Since sigs.k8s.io/yaml converts YAML to JSON internally, a YAML value like deleteTimeout: 30 (bare integer, no quotes) becomes JSON "deleteTimeout": 30 — a number, not a string. This causes json.Unmarshal into map[string]string to return an error, and the entire TimeoutConfig unmarshal fails, propagating up to the parent ConfigurableOptions unmarshal and killing the entire YAML load.

The test YAML uses quoted strings ("30s"), so this doesn't surface in tests, but it is entirely reasonable for a user to write deleteTimeout: 30s (works, YAML string) or deleteTimeout: 30 (breaks, YAML integer). The error message will be cryptic — a JSON type error from inside the timeout config unmarshaller.

A more robust approach: unmarshal into map[string]any and convert values to strings via fmt.Sprintf, or use json.RawMessage per field, or handle both string and numeric types.

[conformance/utils/config/timeout.go:150] — SetInt on reflect.Value will panic if a tagged non-Duration field is ever added

v.Field(i).SetInt(int64(d)) assumes every field with a json tag is a time.Duration (underlying int64). If someone adds a field like MaxRetries int \json:"maxRetries"`toTimeoutConfig, this line will panic at runtime with a type mismatch. There is no guard checking field.Type == reflect.TypeOf(time.Duration(0))before callingSetInt. The RequiredConsecutiveSuccesses` field currently dodges this because it has no json tag, but that's an undocumented invariant — nothing prevents a future contributor from adding a tag.

---

Significant Concerns

[conformance/utils/suite/suite.go:318 / conformance.go:74] — Mode default fallback removed, breaking programmatic callers

Previously, NewConformanceTestSuite had:

mode := flags.DefaultMode
if options.Mode != "" {
    mode = options.Mode
}

This was removed. Now options.Mode is used directly. If a downstream project constructs ConformanceOptions without setting Mode (which was perfectly valid before — the zero value "" was handled), the suite will now run with mode: "" instead of mode: "default". This is a silent behavioral regression for every programmatic caller that doesn't set Mode.

The DefaultOptions function sets the default, but programmatic callers using ConformanceOptions directly bypass DefaultOptions.

[conformance/utils/flags/flags.go:136-148] — usable-address and unusable-address flags append instead of replacing

Every other flag in the registry replaces the YAML value when the flag is explicitly set. But the address flags append:

o.UsableNetworkAddresses = append(o.UsableNetworkAddresses, parseAddress(v))

If a YAML file specifies two usable addresses and the user also passes --usable-address 10.0.0.5, they get three addresses (two from YAML + one from flag). The documented contract is "Command line flags will override the values in the file" — append is not override. This will surprise users.

[conformance/utils/flags/flags.go] — All exported flag variables removed — breaking public API

Every flags.GatewayClassName, flags.SupportedFeatures, flags.Mode, etc. is gone. Downstream conformance test implementations that reference these (and there are many in the Gateway API ecosystem) will fail to compile. Only flags.ConformanceOptionsFile and flags.ApplyAll are exported now, plus the Default* constants.

The ConformanceOptions struct shape also changed (embedding ConfigurableOptions), so any downstream code that constructs it with named fields needs updating.

This should be called out prominently in release notes or a migration guide.

[conformance/conformance_options_test.go:35-38] — Test mutates global flag state without cleanup

*flags.ConformanceOptionsFile = "data/test-conformance-options.yaml"
flag.CommandLine.Set("report-output", "test-output/override.yaml")
flag.CommandLine.Set("timeout-config-overrides", "GetTimeout:40;DefaultTestTimeout:45")

These mutations persist for the entire test process. If another test in the conformance package runs after this one and calls DefaultOptions(t), it will pick up the conformance-options-file and the timeout overrides. Since Go test ordering within a package is deterministic but not guaranteed across refactors, this is a landmine for future test additions. There is no t.Cleanup to reset these values.

[conformance/utils/flags/flags.go:128] — contact flag produces [""] for empty explicit value

strings.Split("", ",") returns [""], not nil. If a user explicitly passes --contact "", the Implementation gets Contact: [""] — a slice with one empty string. This may fail validation or produce garbage in the conformance report.

---

Test Gaps

1. No test for YAML-only operation (no flags set). The test always sets flags. There should be a test that loads only from YAML with no flag overrides, verifying that YAML values are used as-is and defaults are preserved for unset fields.

2. No test for debug: true from YAML. The YAML file sets debug: true but the test never asserts options.Debug == true.

3. No test for usable-address / unusable-address flag interaction with YAML addresses. The current test only verifies YAML-sourced addresses. It should verify whether flag-provided addresses replace or append to YAML addresses.

4. No test for invalid YAML values. What happens when timeoutConfig.deleteTimeout is set to "not-a-duration" or a bare integer 30? The error path from UnmarshalJSON is untested.

5. No test for an empty YAML file or a YAML file with only timeoutConfig. Partial YAML files are a common real-world scenario.

6. No test for Mode default when not set in YAML or flags. The default "default" is set in DefaultOptions but not tested.

7. RequiredConsecutiveSuccesses explicitly excluded from YAML configurability, but no test verifies this. A user might put it in YAML expecting it to work; it should be documented or tested that it's silently ignored.

---

Architectural Notes

• The json struct tags on ConfigurableOptions are correct for sigs.k8s.io/yaml (which uses JSON tags), but the decision to use json: tags for what is primarily a YAML-facing struct will confuse contributors. A comment explaining this design choice on the struct would prevent future contributors from "fixing" it to yaml: tags.

• The flag registry pattern (registerStringFlag/registerBoolFlag + ApplyAll using flag.Visit) is clean and solves the "only apply explicitly set flags" problem well. However, the init-time registration means the registry is not testable in isolation — you can't test ApplyAll without side-effecting global flag.CommandLine state.

• SetupTimeoutConfig is missing coverage for ListenerSetMustHaveCondition and ListenerSetListenersMustHaveConditions. These fields have defaults in DefaultTimeoutConfig() but SetupTimeoutConfig won't fill them in if they're zero. Pre-existing, but this branch should not make it worse by adding a new code path (UnmarshalJSON) that can zero them out.

• The change from sets.Set[T] to []T for SupportedFeatures, ExemptFeatures, and ConformanceProfiles in the options struct is correct for YAML/JSON serializability. The conversion back to sets happens at the right boundary (NewConformanceTestSuite). However, duplicates in slices from YAML are now silently accepted — sets.New() deduplicates, but the logging output (t.Logf("Supported Features: %v"...)) will show duplicates, which may confuse users.