Fix 301 redirect of non-trailing-slash GETs breaking /openapi/v2 and /openapi/v3 - #32
Fix 301 redirect of non-trailing-slash GETs breaking /openapi/v2 and /openapi/v3#32shahab96 wants to merge 2 commits into
Conversation
UpgradeAwareHandler 301-redirects any GET or HEAD request to its trailing-slash form when the target location path is empty, as a workaround for https://issue.k8s.io/4958. When the proxy is configured with a kubernetes-api-server URL without a trailing slash (the default in kubernetes-operator <= v0.7.0 ClusterProxy CRs), every such request is redirected instead of proxied. Most API discovery endpoints tolerate this because the API server also serves their trailing-slash variants (/api/, /apis/, /version/), but /openapi/v2 and /openapi/v3 are exact-path handlers, so the redirected requests return 404. This breaks clients that need the OpenAPI document through the proxy, e.g. Terraform's kubernetes_manifest resource ("failed get OpenAPI spec") and kubectl apply client-side validation. Ensure the location passed to UpgradeAwareHandler always has a non-empty path, and update the proxy handler test to use an empty-path target URL as regression coverage.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe proxy handler copies and normalizes the Kubernetes API server URL before constructing the upgrade-aware handler. Tests preserve an empty target path and verify exact OpenAPI endpoints proxy without trailing-slash redirects. ChangesProxy path normalization
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/proxy/proxy_test.go (1)
81-86: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd explicit OpenAPI endpoint cases.
The regression test currently exercises only
/version. Add requests for/openapi/v2and/openapi/v3(and HEAD if applicable) so the stated client-facing contract is covered directly, not only via a generic path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/proxy/proxy_test.go` around lines 81 - 86, Add explicit regression-test requests for /openapi/v2 and /openapi/v3 alongside the existing /version coverage in the test using kubeAPIServerURL and UpgradeAwareHandler; include corresponding HEAD requests if the test covers HEAD behavior, and assert they are served without trailing-slash redirects.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/proxy/proxy_test.go`:
- Around line 81-86: Add explicit regression-test requests for /openapi/v2 and
/openapi/v3 alongside the existing /version coverage in the test using
kubeAPIServerURL and UpgradeAwareHandler; include corresponding HEAD requests if
the test covers HEAD behavior, and assert they are served without trailing-slash
redirects.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 831939c4-f2aa-4aff-8a38-97aad5fe4a19
📒 Files selected for processing (2)
internal/proxy/proxy.gointernal/proxy/proxy_test.go
/version alone does not mirror the production failure mode, since the API server also serves its trailing-slash variant. The OpenAPI endpoints are exact-path handlers, so extend the test backend to serve /openapi/v2 and /openapi/v3 only at their exact paths and add cases asserting they are proxied without a trailing-slash redirect.
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Summary
Fixes #31
When the proxy is configured with a
--kubernetes-api-serverURL that has an empty path (e.g.https://kubernetes.default.svc.cluster.local— theClusterProxyCRD default in kubernetes-operator <= v0.7.0),UpgradeAwareHandler.ServeHTTP301-redirects every GET/HEAD whose path does not end in/to its trailing-slash form, instead of proxying it. This comes fromproxyRedirectsforRootPathink8s.io/apimachinery/pkg/util/proxy/upgradeaware.go(a legacy workaround for https://issue.k8s.io/4958), which fires wheneverLocation.Pathis empty.Most API discovery endpoints tolerate this because the API server also serves their trailing-slash variants (
/api/,/apis/,/version/), but/openapi/v2and/openapi/v3are exact-path handlers, so the redirected requests 404. This breaks clients that need the OpenAPI document through the proxy, e.g. Terraform'skubernetes_manifestresource ("failed get OpenAPI spec") andkubectl applyclient-side validation.Changes
internal/proxy/proxy.go: normalize the target URL inproxyHandlerso theLocationpassed toUpgradeAwareHandleralways has a non-empty path (/when empty), which avoids the redirect branch entirely. Done at the use site so it is robust regardless of how the URL was provided (flag default, operator CR, etc.).internal/proxy/proxy_test.go: the proxy handler test now parses the test server URL without a manually appended trailing slash, providing regression coverage. Without the fix, the test fails with301responses instead of200.Testing
go test ./...— passesgo vet ./...— cleangofmt— cleanNeed help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by CodeRabbit
/openapi/v2and/openapi/v3are proxied correctly without being redirected to trailing-slash routes.