-
Notifications
You must be signed in to change notification settings - Fork 24
HYPERFLEET-1469 - feat: add tenant configuration and enforcement middleware #345
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
Changes from all commits
5857bac
cd92ccc
cc2600b
d85e6a9
71fae1b
2372094
a7ae4b7
64d2b67
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| package servecmd | ||
|
|
||
| import ( | ||
| "context" | ||
| "net/http" | ||
| "testing" | ||
| "time" | ||
|
|
||
| . "github.com/onsi/gomega" | ||
|
|
||
| "github.com/openshift-hyperfleet/hyperfleet-api/pkg/config" | ||
| "github.com/openshift-hyperfleet/hyperfleet-api/pkg/registry" | ||
| ) | ||
|
|
||
| // TestBuildAPIServer_TenantMiddlewareWiredWhenEnabled guards the tenant | ||
| // middleware's wiring into the composition root, since pkg/tenant's own tests | ||
| // never exercise it there and wouldn't catch a dropped/reordered append. | ||
| // | ||
| // resourceService, adapterStatusService, schemaValidator, and sessionFactory | ||
| // are nil: tenant middleware runs ahead of the handlers that would need them, | ||
| // so a rejected request never reaches code that dereferences them. | ||
| func TestBuildAPIServer_TenantMiddlewareWiredWhenEnabled(t *testing.T) { | ||
| RegisterTestingT(t) | ||
| registry.Reset() | ||
| t.Cleanup(registry.Reset) | ||
| registry.Register(registry.EntityDescriptor{Kind: "Channel", Plural: "channels"}) | ||
|
|
||
| cfg := config.NewApplicationConfig() | ||
| cfg.Server.Host = "127.0.0.1" | ||
| cfg.Server.Port = 0 // ephemeral port | ||
| cfg.Server.JWT.Enabled = false | ||
| cfg.Server.Tenant = config.TenantConfig{ | ||
| Enabled: true, | ||
| SystemHeader: "X-HyperFleet-System", | ||
| Dimensions: []config.TenantDimension{ | ||
| {Header: "X-HyperFleet-Org", Key: "org", Required: true}, | ||
| }, | ||
| } | ||
|
|
||
| apiServer, err := BuildAPIServer(cfg, nil, nil, nil, nil, nil) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| listener, err := apiServer.Listen() | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| // served closes once Serve returns, so cleanup can block until the | ||
| // goroutine has actually exited instead of racing the next test. | ||
| served := make(chan struct{}) | ||
| go func() { | ||
| defer close(served) | ||
| apiServer.Serve(listener) | ||
| }() | ||
| t.Cleanup(func() { | ||
| shutdownCtx, cancel := context.WithTimeout(t.Context(), time.Second) | ||
| defer cancel() | ||
| Expect(apiServer.Shutdown(shutdownCtx)).To(Succeed()) | ||
| <-served | ||
| }) | ||
|
|
||
| baseURL := "http://" + listener.Addr().String() | ||
|
|
||
| // Missing the required tenant dimension header: tenant middleware must | ||
| // reject with 403 before the request reaches any entity handler. | ||
| var resp *http.Response | ||
| Eventually(func() error { | ||
| var getErr error | ||
| resp, getErr = http.Get(baseURL + "/api/hyperfleet/v1/channels") | ||
| return getErr | ||
| }, "2s", "25ms").Should(Succeed()) | ||
| defer resp.Body.Close() | ||
|
|
||
| Expect(resp.StatusCode).To(Equal(http.StatusForbidden), | ||
| "request missing a required tenant header must be rejected by the tenant middleware") | ||
| } |
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder about the need of this file.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. dev.yaml and config.yaml.example serve different purposes: dev.yaml is the runnable config so make run works out of the box; config.yaml.example is the reference template with all options documented. Keeping both means contributors don't have to maintain their own local config, since dev.yaml stays current as config evolves. Updating both together when adding config (as done here) keeps them in sync. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| package response | ||
|
|
||
| import ( | ||
| "context" | ||
| "net/http" | ||
|
|
||
| "github.com/openshift-hyperfleet/hyperfleet-api/pkg/errors" | ||
| "github.com/openshift-hyperfleet/hyperfleet-api/pkg/logger" | ||
| ) | ||
|
|
||
| // WriteServiceErrorResponse writes err as an RFC 9457 Problem Details response, | ||
| // resolving the trace ID from ctx (via logger.GetRequestID) and using the | ||
| // request path as the problem instance. ctx is taken explicitly rather than | ||
| // derived from r.Context() so callers can still resolve a trace ID when r is | ||
| // nil. Callers are responsible for logging the error themselves beforehand, | ||
| // since the appropriate log level (e.g. Warn vs Info vs Error) varies by caller. | ||
| func WriteServiceErrorResponse(ctx context.Context, w http.ResponseWriter, r *http.Request, err *errors.ServiceError) { | ||
| traceID, ok := logger.GetRequestID(ctx) | ||
| if !ok { | ||
| traceID = "unknown" | ||
| } | ||
| instance := "" | ||
| if r != nil { | ||
| instance = r.URL.Path | ||
| } | ||
| WriteProblemDetailsResponse(w, r, err.HTTPCode, err.AsProblemDetails(instance, traceID)) | ||
| } |
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.
I find a bit confusing that we generate:
/tmpdev-jwks.jsonat./configsThere 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.
This is pre-existing behavior, not introduced here. The distinction: the token is ephemeral (regenerated on each make run, 8-hour expiry) so /tmp fits; the JWKS is referenced by dev.yaml and can persist across runs, so it lives in ./configs. Happy to revisit the locations in a follow-up if you feel strongly.