Conversation
…ons response; don't assume that read_basic_data is always true
…st body, correct nullability, correct invalid tests (don't assume RESET command if unable to parse request body)
…is guards against wire-level breaking changes when renaming properties and 2) the names are dictated by Spring, so configuring a global casing policy must not change them
|
Summary - All Code Coverage (ubuntu-latest)
|
TimHess
left a comment
TimHess
left a comment
There was a problem hiding this comment.
Generally LGTM, some nice improvements! Comments can be resolved if you don't think changes are needed for any of them
|
|
||
| [JsonSourceGenerationOptions] | ||
| [JsonSerializable(typeof(Application))] | ||
| [JsonSerializable(typeof(SpringBootAdminApiClient.RegistrationResult))] |
There was a problem hiding this comment.
Probably not a big deal, but the PR description says it adds [JsonPropertyName] to all serialized properties to guard against wire-level breaking changes. But RegistrationResult.Id doesn't have one.
There was a problem hiding this comment.
Hmm, Cursor initially generated a very detailed description for the PR, so I asked to simplify it. I'll update the description to indicate that this matters to users who pass their own casing convention via configuration, which is only possible for actuator endpoints.
| public Uri Uri { get; set; } = null!; | ||
| public Uri? NonSecureUri { get; set; } | ||
| public Uri? SecureUri { get; set; } | ||
| public IReadOnlyDictionary<string, string?> Metadata { get; set; } = null!; |
There was a problem hiding this comment.
Should these properties have [JsonPropertyName] set?
There was a problem hiding this comment.
Same, this only matters for data exposed at actuator endpoints. Serializer options are not configurable here.
| [Theory] | ||
| [InlineData(false, false, EndpointPermissions.None)] | ||
| [InlineData(false, true, EndpointPermissions.Restricted)] | ||
| [InlineData(true, false, EndpointPermissions.None)] |
There was a problem hiding this comment.
Were you able to find out if this scenario is possible? It does seem implausible, so probably doesn't need extensive research
There was a problem hiding this comment.
I haven't looked into it, just trying to be defensive by default.
| LogChangeRequest(loggerName, level ?? "RESET"); | ||
|
|
||
| if (!string.IsNullOrEmpty(loggerName)) | ||
| if (changes.TryGetValue("configuredLevel", out string? level)) |
There was a problem hiding this comment.
This seems like a good and safe change, but just wanted to confirm you validated that both SBA and Apps Manager send the expected request body and there aren't real cases of an actuator client sending an empty body here?
| catch (Exception exception) when (!exception.IsCancellation()) | ||
| { | ||
| throw new SecurityException($"Exception extracting permissions from json: {SecurityUtilities.SanitizeInput(json)}", exception); | ||
| return null; |
There was a problem hiding this comment.
If there's an exception during deserialization, it will be swallowed and the result will be Bad Gateway... While the scenario is pretty unlikely, and it will be pretty clear what happened, this could leave anybody trying to diagnose the situation with a more work to do trying to understand why it happened since they won't be able to see what that response body was
There was a problem hiding this comment.
Yes, they need to turn on debug logging. This is a scenario that should never happen, unless the Cloud Controller API changes or misbehaves. An API change requires a major version bump, while a misbehaving CC is a bug that Broadcom needs to investigate. I wouldn't expect end-users to go that deep.
2 participants
Description
This PR migrates System.Text.Json serialization from reflection-based to compile-time source-generated
JsonSerializerContextclasses across the non-actuator parts of the codebase (actuator endpoints are excluded because their serializer options are configurable at runtime).Key changes:
Source-generated JSON serialization -- Six new
JsonSerializerContextsubclasses replace staticJsonSerializerOptionsinstances and update (de)serialization calls throughout Eureka, HttpClients, Cloud Foundry middleware, Management port middleware, and SpringBootAdmin client.Explicit
[JsonPropertyName]on all serialized properties that are exposed at actuator endpoints -- Guards against wire-level breaking changes when users configure an alternate casing policy, since the property names are dictated by Spring.Cloud Foundry permissions reworked -- Switched from v2 to v3 cloud controller API, replaced loose
Dictionary<string, JsonElement>parsing with a strongly-typedPermissionsResponse, and improved error handling in parsing Cloud Controller responses (graceful failure instead of throwingSecurityException). Test mock scenarios were renamed for clarity ("success" -> "full-permissions", "no_sensitive_data" -> "restricted-permissions") and new scenarios added for "no-permissions" and "broken-response".Loggers actuator bug fixes -- Now respects configured serializer options when reading request bodies, corrects nullability, and no longer assumes RESET when the request body can't be parsed.
Test improvements -- Expanded serialization roundtrip tests for Eureka types, added
ToString()debug output coverage, updated CloudFoundry mock URLs and scenarios to match the new permission logic.Closes #970.
Quality checklist
If your change affects other repositories, such as Documentation, Samples and/or MainSite, add linked PRs here.