Exercise all three feature modes in placement shim e2e tests via header override#767
Merged
Merged
Conversation
…er override The middleware now reads X-Cortex-Feature-Mode from every request and, when valid, stores it in context. featureModeFromConfOrHeader resolves the effective mode per handler (override if present, configured default otherwise). All handlers use this helper instead of reading config directly. Each e2e test is wrapped with e2eWrapWithModes which iterates passthrough, hybrid, and crd. A custom RoundTripper (e2eModeTransport) auto-injects the header from context so individual tests need no changes. Passthrough-only endpoints probe for 501 via e2eProbeUnimplemented and skip gracefully. Tests that require CRD infrastructure (traits CRUD, RP traits writes) gate on the configured mode. The resourceLocker is now always initialized to avoid nil panics from mode overrides.
PhilippMatthes
requested review from
SoWieMarkus,
auhlig,
juliusclausnitzer,
mblos and
umswmayj
as code owners
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR introduces per-request feature mode resolution for the placement shim, enabling clients to override the configured feature mode (Passthrough/Hybrid/CRD) via the Changes
Sequence DiagramsequenceDiagram
participant Client
participant Middleware as wrapHandler<br/>(Middleware)
participant Handler
participant Context
participant Config
Client->>Middleware: HTTP Request<br/>(with optional<br/>X-Cortex-Feature-Mode)
Middleware->>Middleware: Extract header<br/>X-Cortex-Feature-Mode
Middleware->>Context: Inject featureModeOverride<br/>(if header valid)
Middleware->>Handler: Call handler with<br/>modified request
Handler->>Context: Retrieve override<br/>featureModeOverride?
Handler->>Config: Fallback to<br/>config.Features.*.orDefault()
Handler->>Handler: Resolve effective mode:<br/>Passthrough/Hybrid/CRD
alt Passthrough Mode
Handler->>Handler: Forward upstream
else Hybrid Mode
Handler->>Handler: Serve from local<br/>+ upstream
else CRD Mode
Handler->>Handler: Serve from local<br/>ConfigMaps only
end
Handler->>Client: Response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PhilippMatthes
changed the title
PhilippMatthes
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/shim/placement/shim_e2e.go (1)
141-169:e2eProbeUnimplementeddocs and behavior should be aligned.The comment says unexpected outcomes return an error, but the function currently returns
false, nilfor every non-501 status. Consider either tightening the status handling or relaxing the comment.Possible behavior-aligned fix
func e2eProbeUnimplemented(ctx context.Context, sc *gophercloud.ServiceClient, probeURL string) (bool, error) { @@ if resp.StatusCode == http.StatusNotImplemented { log.Info("Endpoint correctly returns 501 for unimplemented mode", "mode", mode) return true, nil } + if resp.StatusCode >= http.StatusBadRequest { + return false, fmt.Errorf("probe %s in mode %s returned unexpected status %d", probeURL, mode, resp.StatusCode) + } return false, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/shim/placement/shim_e2e.go` around lines 141 - 169, The function e2eProbeUnimplemented's comment says unexpected outcomes should return an error, but currently any non-501 response yields (false, nil); update e2eProbeUnimplemented to distinguish expected "works" responses from truly unexpected ones: after the HTTP call inspect resp.StatusCode and return (true, nil) for 501, return (false, nil) for accepted success codes the tests treat as "implemented" (e.g., 200/204), and return (false, error) for any other status (include the status code and optionally response body in the error). Keep using sc.HTTPClient.Do, req (with setFeatureModeHeader and token) and defer resp.Body.Close() as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/shim/placement/shim.go`:
- Around line 121-125: The helper featureModeFromConfOrHeader currently allows a
request header override to enable CRD/hybrid even when the configured
FeatureMode is passthrough; change it to reject or ignore request-time overrides
unless the configured mode already enables Versioning/Traits (i.e., only honor
override when configured.orDefault() != passthrough and/or when the backing
config (Versioning/Traits) is present). Concretely, update
featureModeFromConfOrHeader to check the configured mode (and presence of
backing config if available) first and return configured.orDefault() when
configured is passthrough or backing config is missing; only return
override.orDefault() when the configured mode permits those code paths (or when
you have validated the backing config), so handlers like HandleGetRoot and the
traits handlers cannot be routed into Versioning/Traits-dependent paths without
startup validation.
---
Nitpick comments:
In `@internal/shim/placement/shim_e2e.go`:
- Around line 141-169: The function e2eProbeUnimplemented's comment says
unexpected outcomes should return an error, but currently any non-501 response
yields (false, nil); update e2eProbeUnimplemented to distinguish expected
"works" responses from truly unexpected ones: after the HTTP call inspect
resp.StatusCode and return (true, nil) for 501, return (false, nil) for accepted
success codes the tests treat as "implemented" (e.g., 200/204), and return
(false, error) for any other status (include the status code and optionally
response body in the error). Keep using sc.HTTPClient.Do, req (with
setFeatureModeHeader and token) and defer resp.Body.Close() as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b8f4510d-4c0f-4b6d-b147-d6be018654d1
📒 Files selected for processing (21)
internal/shim/placement/handle_allocation_candidates_e2e.gointernal/shim/placement/handle_allocations_e2e.gointernal/shim/placement/handle_reshaper_e2e.gointernal/shim/placement/handle_resource_classes_e2e.gointernal/shim/placement/handle_resource_provider_aggregates_e2e.gointernal/shim/placement/handle_resource_provider_allocations_e2e.gointernal/shim/placement/handle_resource_provider_inventories_e2e.gointernal/shim/placement/handle_resource_provider_traits.gointernal/shim/placement/handle_resource_provider_traits_e2e.gointernal/shim/placement/handle_resource_provider_usages_e2e.gointernal/shim/placement/handle_resource_providers.gointernal/shim/placement/handle_resource_providers_e2e.gointernal/shim/placement/handle_root.gointernal/shim/placement/handle_root_e2e.gointernal/shim/placement/handle_traits.gointernal/shim/placement/handle_traits_e2e.gointernal/shim/placement/handle_usages_e2e.gointernal/shim/placement/shim.gointernal/shim/placement/shim_e2e.gointernal/shim/placement/shim_io.gointernal/shim/placement/shim_test.go
…be error handling Address review feedback: - featureModeFromConfOrHeader now rejects overrides to hybrid/crd when neither Versioning nor Traits config was validated at startup, preventing handlers from hitting nil-pointer paths. - e2eProbeUnimplemented now returns an error for unexpected 4xx/5xx status codes (other than 501) instead of silently treating them as success.
Member
Author
|
Addressed both review comments in 3f4e3b8:
|
Contributor
Test Coverage ReportTest Coverage 📊: 69.3% |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The middleware now reads X-Cortex-Feature-Mode from every request and, when valid, stores it in context. featureModeFromConfOrHeader resolves the effective mode per handler (override if present, configured default otherwise). All handlers use this helper instead of reading config directly.
Each e2e test is wrapped with e2eWrapWithModes which iterates passthrough, hybrid, and crd. A custom RoundTripper (e2eModeTransport) auto-injects the header from context so individual tests need no changes. Passthrough-only endpoints probe for 501 via e2eProbeUnimplemented and skip gracefully. Tests that require CRD infrastructure (traits CRUD, RP traits writes) gate on the configured mode. The resourceLocker is now always initialized to avoid nil panics from mode overrides.