-
Notifications
You must be signed in to change notification settings - Fork 32
fix: remove backend-ingress to resolve auth token routing conflict #296
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
The backend-ingress was intercepting /api/* requests before they could reach the Next.js frontend, causing authentication failures. The frontend serves both the UI and API routes, proxying authenticated requests to the backend internally via backend-service:8080/api. This eliminates the "Invalid or missing token" error users saw when accessing the UI after running setup-kind.sh and deploy.sh. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - frontend-ingress.yaml | ||
| - backend-ingress.yaml | ||
| # backend-ingress.yaml removed - frontend handles all routing and proxies to backend internally |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing backend ingress never cleans up existing deployments
Commenting out backend-ingress.yaml from the e2e kustomization prevents new deployments from creating the ingress, but e2e/scripts/deploy.sh still uses kubectl apply -k without --prune. Any cluster that previously deployed this overlay will keep the old backend-ingress resource, so /api requests will continue to bypass the frontend and the “Invalid or missing token” problem remains. The fix only works on brand‑new clusters. Consider deleting the old ingress explicitly or switching to a pruned apply so existing environments also receive the change.
Useful? React with 👍 / 👎.
| - test-user.yaml | ||
| - frontend-ingress.yaml | ||
| - backend-ingress.yaml | ||
| # backend-ingress.yaml removed - frontend handles all routing and proxies to backend internally |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documentation Enhancement Suggestion
Great explanatory comment! To make it even more helpful for future developers, consider adding a reference to where this architectural pattern is documented:
| # backend-ingress.yaml removed - frontend handles all routing and proxies to backend internally | |
| # backend-ingress.yaml removed - frontend handles all routing and proxies to backend internally | |
| # Architecture: Next.js API Gateway pattern - see components/frontend/src/lib/auth.ts for auth flow |
This helps developers understand where to look for the authentication/proxy implementation details.
Claude Code ReviewSummaryThis is a critical bug fix that resolves authentication failures in the e2e test environment. The change correctly removes the Overall Assessment: ✅ APPROVED - Ready to merge with one minor documentation enhancement suggestion. Issues by Severity🔵 Minor IssuesDocumentation Enhancement
Positive HighlightsArchitecture & Design Excellence✅ Correct API Gateway Pattern: This change properly implements the recommended architecture where Next.js acts as the unified entry point, handling authentication and proxying to backend services. ✅ Root Cause Analysis: The PR description demonstrates excellent understanding of the problem - the dual ingress configuration was creating routing ambiguity where ✅ Minimal Impact: This is a surgical fix - only one line changed (comment added), with no code modifications required. The architecture was already correct; the extra ingress was the only problem. Security & Authentication✅ Preserves Auth Flow: The fix maintains the proper authentication chain:
✅ No Security Regression: Removing the direct backend ingress actually improves security by ensuring all requests flow through the authentication layer in the Next.js API routes. Testing & Validation✅ Comprehensive Test Plan: PR description shows thorough manual testing of the critical paths ✅ E2E Test Coverage: The existing Cypress tests validate the exact scenarios affected by this bug, providing excellent regression protection. Code Quality✅ Clear Documentation: Inline comment explains not just what was removed, but why - the architectural reasoning. ✅ Excellent PR Description: Clear problem statement, root cause analysis with routing diagram, solution explanation, and impact assessment. Architecture ValidationI reviewed the implementation and confirmed this fix aligns with the established patterns. The correct routing architecture is now: The previous broken architecture had dual ingress points causing /api/* requests to bypass the frontend auth layer. Comparison with Other Overlayslocal-dev overlay: Uses OpenShift Routes and has both frontend-route.yaml AND backend-route.yaml. May have similar routing conflict worth investigating. RecommendationsBefore Merge (Optional)
Future Work (Not Blocking)
Technical Review Notes
Verdict: This is an exemplary bug fix PR - clear problem identification, correct solution, minimal change scope, and thorough documentation. 🚢 Ready to merge! |
|
Tracked in Jira: https://issues.redhat.com/browse/RHOAIENG-39124 |
Summary
Fixes the "Invalid or missing token" authentication error users encounter when accessing the UI after running
setup-kind.shanddeploy.sh.Problem
The
backend-ingresswas intercepting all/api/*requests and routing them directly to the Go backend, bypassing the Next.js frontend's API routes. This caused:/api/me,/api/cluster-info) to return 404OC_TOKENenvironment variable for authenticationSolution
Removed
backend-ingress.yamlfrom the e2e kustomization overlay. The correct architecture is:```
Browser → frontend-ingress → Next.js Frontend
↓
/api/* routes (handles auth)
↓
backend-service:8080/api (internal proxy)
```
This follows the API Gateway pattern where Next.js acts as the unified entry point for both UI and API traffic.
Changes
Test Plan
Impact
🤖 Generated with Claude Code