markhoerth commented on issue #12921:
URL: https://github.com/apache/gravitino/issues/12921#issuecomment-5538057712
@lasdf1234 Agreed on all five points. The direction is right: move it under
/api, authorize with serviceAdmin, and do not try to bolt an allowlist onto the
/configs mount. Concretely that is GET /configs/secrets/providers becoming GET
/api/secrets/providers, top level with no metalake in the path, since the
registry is process-global and a metalake segment would imply a scoping that
does not exist.
Five requirements for the fix.
1. Remove the old path rather than deprecating it. The endpoint exists only
on main and has never shipped in a release, so there is no compatibility
surface to protect. A deprecated anonymous route is a second unprotected entry
point that outlives the fix.
2. The new path goes into the Open API definition. Whatever we pick, it
ships as a specified, documented endpoint rather than another undocumented one
that consumers discover in devtools and then depend on.
3. Audit gets verified, not assumed. HttpAuditFilter is bound to
API_ANY_PATH in initializeRestApi(), so the move should pick it up, but the
acceptance criterion is an actual audit entry produced by a request to the new
path.
4. Scope the response body to what a named consumer needs.
SecretProviderInfo carries name, type and a nullable uri. Even for
serviceAdmin, uri is infrastructure topology and it is the only one of the
three a caller could not have guessed. If no consumer requires it, it does not
belong in the response.
5. It conforms to the /api conventions once it is there, not just the path.
Standard ErrorResponse body and error codes through the normal exception
mapping rather than the hand-written {"error": ...} map and blanket 500, and
correct method handling instead of a class that only overrides doGet. The
current implementation predates the auth question and does not meet these.
Moving it without rewriting the response handling leaves a non-conforming
endpoint inside the conforming surface. Deleting SecretProvidersConfigServlet
in favour of a resource class in org.apache.gravitino.server.web.rest gets all
of it for free.
/configs being anonymous is correct, and the fix is not a change to that.
ConfigServlet serves the pre-login UI and is bounded by visibleConfigs, which
is a deliberate and reviewed exposure. The provider registry does not belong in
that class, so the fix is moving it out, not gating the root path.
Stating the general rule so this does not recur: every standard external API
lives under /api. The root-mounted paths are a closed set with a specific
reason each, /configs for the pre-login UI and the health aliases for probes
that must sit at well-known root paths. Neither is a general-purpose place to
publish data, and adding to that set needs the same justification they had. New
endpoints go under /api by default, unless they are internal or UI APIs, in
which case they live somewhere else TBD. The secrets provider registry is not
one of those: it is operator-facing configuration, so it goes under /api now
and does not wait on where internal APIs land.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
