Repository navigation
Conversation
github-actions
Bot
added
bug
iglin
changed the title
The plugin module has no parent POM and never ran JaCoCo, so Sonar got no coverage report for it and counted every new line as uncovered. Add the prepare-agent and report goals so verify writes target/site/jacoco/jacoco.xml, which the Sonar scanner picks up by default. Also resolve java:S2259 in RouteScanner: hierarchy() checked its argument for null, which made Sonar treat every caller's ClassInfo as nullable. Handle the missing superclass at the call site instead. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
|
| List<PlannedRule> rules = new ArrayList<>(); | ||
| groups.forEach((match, group) -> rules.add(prefixRule(match, group, problems))); | ||
| return rules; | ||
| } |
There was a problem hiding this comment.
A path variable before the service segment cuts the match down to a shared prefix. /api/{version}/svc/items/{id} becomes PathPrefix /api, and /{tenant}/svc/... becomes PathPrefix /. As a result:
On the internal gateway, which has no DENY rules, this service captures every /api/** request that doesn't match a longer prefix of another service, and rewrites it.
With autoGenerateAuthorizationPolicies=true, it generates a DENY rule ["/api", "/api/{**}"] on the public and private gateways. The notPaths list only this service's routes, so one service returns 403 for every other service on the shared gateway.
Consider to fail the build on short border prefixes
There was a problem hiding this comment.
There is no such route in any microservice so this check was dropped to simplify code changes.
| : " " + formatDuration(timeout))); | ||
| } | ||
| return path; | ||
| return new PlannedRule(widest, match, rewriteOf(match, routes.get(0)).orElse(null), timeout); |
There was a problem hiding this comment.
The group takes the first route's rewrite and silently drops the others. Example: /api/v1/svc/{id} → /v2/items/{id} and /api/v1/svc/{id}/x → /other/{id}/x merge into PathPrefix /api/v1/svc with ReplacePrefixMatch /v2/items. A request to /api/v1/svc/5/x then goes to /v2/items/5/x. The proposal promises "correct config or a failing build", but this doesn't even log a warning. describe() already computes each route's rewrite, so the check is nearly free:
if (routes.stream().map(r -> rewriteOf(match, r)).distinct().count() > 1) {
problems.error("Routes " + describe(match, routes) + " are cut to one PathPrefix " + match
+ " but need different rewrites");
}
| */ | ||
| private static Optional<String> rewriteOf(String match, HttpRoute route) { | ||
| String rewrite = RoutePaths.cut(route.path()); | ||
| return rewrite.equals(match) ? Optional.empty() : Optional.of(rewrite); |
There was a problem hiding this comment.
rewriteOf uses cut(route.path()) as the ReplacePrefixMatch value. That is only correct when the parts after the cut match between the gateway path and the service path, because ReplacePrefixMatch keeps everything after the match unchanged. Nothing checks this. Example: /api/v1/svc/{id}/details → /details/{id} gives PathPrefix /api/v1/svc → /details, so /api/v1/svc/5/details goes to /details/5/details.
| rules.addAll(facadeRules); | ||
| } else if (facadeRules.stream().anyMatch(r -> r.timeout() > 0)) { | ||
| problems.warn("No facade or composite route has a rewrite, so no service-bound HTTPRoute is generated, " | ||
| + "and the timeouts of the facade and composite routes are not applied"); |
There was a problem hiding this comment.
While this HTTPRoute exists, any direct call to http://:8080/... on a path that isn't declared as a facade route returns 404. In legacy, direct calls bypassed the facade gateway and worked. A PathPrefix / rule without a rewrite costs nothing, because longer prefixes still win, and it restores the legacy behavior:
java
if (facadeRules.stream().anyMatch(r -> r.rewrite() != null)) {
rules.addAll(facadeRules);
if (facadeRules.stream().noneMatch(r -> r.value().equals("/"))) {
rules.add(new PlannedRule(HttpRoute.Type.FACADE, "/", null, 0));
}
}
What scenario does a catch-all break? Design point D9 only says "user decision".
There was a problem hiding this comment.
No, in legacy any request on SERVICE_NAME goes to facade/composite gateway, so with no declared route on composite/facade gateway client gets 404.
| private String outputFile; | ||
|
|
||
| @Parameter(defaultValue = "{{ .Values.DEPLOYMENT_RESOURCE_NAME }}") | ||
| @Parameter(defaultValue = "{{ .Values.SERVICE_NAME }}") |
There was a problem hiding this comment.
The default changed from DEPLOYMENT_RESOURCE_NAME to SERVICE_NAME. This breaks blue-green deployments use versioned resource names. The change isn't mentioned in the proposal or in the README ("Differences from Legacy Routing", "Migrating Existing Services"). If it's intentional, please add a line to the migration section and explain how it works with blue-green.
There was a problem hiding this comment.
It is intentional for Istio. Service with DEPLOYMENT_RESOURCE_NAME name was needed only for composite/facade gateways, in Istio it does not exist.
iglin
changed the title
4 participants
Uh oh!
There was an error while loading. Please reload this page.