diff --git a/changelog/@unreleased/pr-1286.v2.yml b/changelog/@unreleased/pr-1286.v2.yml new file mode 100644 index 000000000..a6bfed281 --- /dev/null +++ b/changelog/@unreleased/pr-1286.v2.yml @@ -0,0 +1,6 @@ +type: improvement +improvement: + description: Conjure User-Agents supports arbitrary comment metadata, for observability + metadata similar to the existing `nodeId` parameter. + links: + - https://github.com/palantir/conjure-java-runtime-api/pull/1286 diff --git a/service-config/src/main/java/com/palantir/conjure/java/api/config/service/UserAgent.java b/service-config/src/main/java/com/palantir/conjure/java/api/config/service/UserAgent.java index 7255ee94c..5c90273ee 100644 --- a/service-config/src/main/java/com/palantir/conjure/java/api/config/service/UserAgent.java +++ b/service-config/src/main/java/com/palantir/conjure/java/api/config/service/UserAgent.java @@ -16,11 +16,13 @@ package com.palantir.conjure.java.api.config.service; -import com.palantir.logsafe.Preconditions; +import com.google.common.collect.ImmutableList; +import com.palantir.logsafe.Safe; import com.palantir.logsafe.SafeArg; import com.palantir.logsafe.exceptions.SafeIllegalArgumentException; import java.util.List; import java.util.Optional; +import java.util.stream.Stream; import org.immutables.value.Value; /** @@ -33,7 +35,18 @@ public interface UserAgent { /** Identifies the node (e.g., IP address, container identifier, etc) on which this user agent was constructed. */ - Optional nodeId(); + @Value.Lazy + default Optional<@Safe String> nodeId() { + if (primary().comments().isEmpty()) { + // fast path to avoid stream overhead + return Optional.empty(); + } + return primary().comments().stream() + .filter(item -> item.startsWith("nodeId:")) + .map(item -> item.substring(7).trim()) + .filter(UserAgents::isValidNodeId) + .findFirst(); + } /** The primary user agent, typically the name/version of the service initiating an RPC call. */ Agent primary(); @@ -45,8 +58,15 @@ public interface UserAgent { List informational(); /** Creates a new {@link UserAgent} with the given {@link #primary} agent and originating node id. */ - static UserAgent of(Agent agent, String nodeId) { - return ImmutableUserAgent.builder().nodeId(nodeId).primary(agent).build(); + static UserAgent of(Agent agent, @Safe String nodeId) { + UserAgents.checkNodeId(nodeId); + List comments = Stream.concat( + agent.comments().stream().filter(item -> !item.startsWith("nodeId:")), + Stream.of("nodeId:" + nodeId)) + .toList(); + return ImmutableUserAgent.builder() + .primary(Agent.of(agent.name(), agent.version(), comments)) + .build(); } /** @@ -75,26 +95,28 @@ default UserAgent addAgent(Agent agent) { return ImmutableUserAgent.builder().from(this).addInformational(agent).build(); } - @Value.Check - default void check() { - if (nodeId().isPresent()) { - Preconditions.checkArgument( - UserAgents.isValidNodeId(nodeId().get()), - "Illegal node id format", - SafeArg.of("nodeId", nodeId().get())); - } - } - /** Specifies an agent that participates (client-side) in an RPC call in terms of its name and version. */ @Value.Immutable @ImmutablesStyle + @Safe interface Agent { String DEFAULT_VERSION = "0.0.0"; + @Safe String name(); + @Safe String version(); + /** + * rfc7230 section-3.2.6) + * for additional diagnostic information. Note that this library provides a much stricter set of allowed + * characters within comments than the linked RFCs to reduce complexity. + */ + List<@Safe String> comments(); + @Value.Check default void check() { if (!UserAgents.isValidName(name())) { @@ -107,10 +129,23 @@ default void check() { } } - static Agent of(String name, String version) { + static Agent of(@Safe String name, @Safe String version) { + return ImmutableAgent.builder() + .name(name) + .version(UserAgents.isValidVersion(version) ? version : DEFAULT_VERSION) + .build(); + } + + static Agent of(@Safe String name, @Safe String version, @Safe Iterable<@Safe String> comments) { + ImmutableList immutableComments = ImmutableList.copyOf(comments); + for (int i = 0; i < immutableComments.size(); i++) { + String comment = immutableComments.get(i); + UserAgents.checkComment(comment); + } return ImmutableAgent.builder() .name(name) .version(UserAgents.isValidVersion(version) ? version : DEFAULT_VERSION) + .comments(immutableComments) .build(); } } diff --git a/service-config/src/main/java/com/palantir/conjure/java/api/config/service/UserAgents.java b/service-config/src/main/java/com/palantir/conjure/java/api/config/service/UserAgents.java index ec359be32..e24aabbd8 100644 --- a/service-config/src/main/java/com/palantir/conjure/java/api/config/service/UserAgents.java +++ b/service-config/src/main/java/com/palantir/conjure/java/api/config/service/UserAgents.java @@ -18,13 +18,14 @@ import com.google.common.base.CharMatcher; import com.google.common.base.Splitter; +import com.google.common.collect.ImmutableList; import com.palantir.logsafe.Preconditions; +import com.palantir.logsafe.Safe; import com.palantir.logsafe.SafeArg; import com.palantir.logsafe.exceptions.SafeIllegalArgumentException; import com.palantir.logsafe.logger.SafeLogger; import com.palantir.logsafe.logger.SafeLoggerFactory; -import java.util.HashMap; -import java.util.Map; +import java.util.List; import java.util.Optional; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -45,18 +46,21 @@ public final class UserAgents { Pattern.compile("^[0-9]+(?:\\.[0-9]+)*(?:-rc[0-9]+)?(?:-[0-9]+-g[a-f0-9]+)?$"); private static final Pattern SEGMENT_PATTERN = Pattern.compile(String.format("(%s)/(%s)( \\((.+?)\\))?", NAME_REGEX, LENIENT_VERSION_REGEX)); - private static final Splitter COMMA_OR_SEMICOLON_SPLITTER = - Splitter.on(CharMatcher.anyOf(",;").precomputed()).trimResults().omitEmptyStrings(); + private static final Splitter SEMICOLON_SPLITTER = + Splitter.on(CharMatcher.is(';').precomputed()).trimResults().omitEmptyStrings(); + private static final CharMatcher COMMENT_VALID_CHARS = CharMatcher.inRange('a', 'z') + .or(CharMatcher.inRange('A', 'Z')) + .or(CharMatcher.inRange('0', '9')) + .or(CharMatcher.anyOf(".-:_/ ,")) + .precomputed(); private UserAgents() {} /** Returns the canonical string format for the given {@link UserAgent}. */ - public static String format(UserAgent userAgent) { + @Safe + public static String format(@Safe UserAgent userAgent) { StringBuilder formatted = new StringBuilder(64); // preallocate larger buffer for longer agents formatSimpleAgent(userAgent.primary(), formatted); - if (userAgent.nodeId().isPresent()) { - formatted.append(" (nodeId:").append(userAgent.nodeId().get()).append(')'); - } for (UserAgent.Agent informationalAgent : userAgent.informational()) { formatted.append(' '); formatSimpleAgent(informationalAgent, formatted); @@ -68,6 +72,17 @@ private static void formatSimpleAgent(UserAgent.Agent agent, StringBuilder outpu output.ensureCapacity( output.length() + 1 + agent.name().length() + agent.version().length()); output.append(agent.name()).append('/').append(agent.version()); + List comments = agent.comments(); + if (!comments.isEmpty()) { + output.append(" ("); + for (int i = 0; i < comments.size(); i++) { + if (i > 0) { + output.append("; "); + } + output.append(comments.get(i)); + } + output.append(')'); + } } /** @@ -76,7 +91,7 @@ private static void formatSimpleAgent(UserAgent.Agent agent, StringBuilder outpu * *

Valid user agent strings loosely follow RFC 7230 (https://tools.ietf.org/html/rfc7230#section-3.2.6). */ - public static UserAgent parse(String userAgent) { + public static UserAgent parse(@Safe String userAgent) { Preconditions.checkNotNull(userAgent, "userAgent must not be null"); return parseInternal(userAgent, false /* strict */); } @@ -85,7 +100,7 @@ public static UserAgent parse(String userAgent) { * Like {@link #parse}, but never fails and returns the primary agent {@code unknown/0.0.0} if no valid primary * agent can be parsed. */ - public static UserAgent tryParse(String userAgent) { + public static UserAgent tryParse(@Safe String userAgent) { return parseInternal(userAgent == null ? "" : userAgent, true /* lenient */); } @@ -97,7 +112,7 @@ public static UserAgent tryParse(String userAgent) { * This is functionally similar to calling `tryParse(userAgent).primary().name()`, but optimized to not use * regular expressions. */ - public static String tryParsePrimaryName(String userAgent) { + public static String tryParsePrimaryName(@Safe String userAgent) { if (userAgent == null) { return "unknown"; } @@ -132,7 +147,7 @@ public static String tryParsePrimaryName(String userAgent) { return userAgent.substring(lindex, split); } - private static UserAgent parseInternal(String userAgent, boolean lenient) { + private static UserAgent parseInternal(@Safe String userAgent, boolean lenient) { ImmutableUserAgent.Builder builder = ImmutableUserAgent.builder(); Matcher matcher = SEGMENT_PATTERN.matcher(userAgent); @@ -142,18 +157,14 @@ private static UserAgent parseInternal(String userAgent, boolean lenient) { String version = matcher.group(2); Optional comments = Optional.ofNullable(matcher.group(4)); + UserAgent.Agent agent = UserAgent.Agent.of( + name, version, comments.map(UserAgents::parseComments).orElseGet(ImmutableList::of)); if (!foundFirst) { // primary - builder.primary(UserAgent.Agent.of(name, version)); - comments.ifPresent(c -> { - Map parsedComments = parseComments(c); - if (parsedComments.containsKey("nodeId")) { - builder.nodeId(parsedComments.get("nodeId")); - } - }); + builder.primary(agent); } else { // informational - builder.addInformational(UserAgent.Agent.of(name, version)); + builder.addInformational(agent); } foundFirst = true; @@ -177,17 +188,46 @@ private static UserAgent parseInternal(String userAgent, boolean lenient) { return builder.build(); } - private static Map parseComments(String commentsString) { - Map comments = new HashMap<>(); - for (String comment : COMMA_OR_SEMICOLON_SPLITTER.split(commentsString)) { - String[] fields = comment.split(":"); - if (fields.length == 2) { - comments.put(fields[0], fields[1]); - } else { - comments.put(comment, comment); + private static List parseComments(String commentsString) { + List results = SEMICOLON_SPLITTER.splitToList(commentsString); + for (int i = 0; i < results.size(); ++i) { + // In most cases, all comments will be valid, so we avoid stream overhead. + if (!isValidComment(results.get(i))) { + return results.stream().filter(UserAgents::isValidComment).toList(); } } - return comments; + return results; + } + + static void checkComment(@Safe String comment) { + if (comment == null) { + throw new SafeIllegalArgumentException("Comment must not be null"); + } + if (comment.isEmpty()) { + throw new SafeIllegalArgumentException("Comment must not be empty"); + } + if (comment.startsWith(" ")) { + throw new SafeIllegalArgumentException( + "Comment must not start with whitespace", SafeArg.of("comment", comment)); + } + if (comment.endsWith(" ")) { + throw new SafeIllegalArgumentException( + "Comment must not end with whitespace", SafeArg.of("comment", comment)); + } + if (!COMMENT_VALID_CHARS.matchesAllOf(comment)) { + throw new SafeIllegalArgumentException( + "Comment contains disallowed characters", + SafeArg.of("allowed", "a-zA-Z0-9.-:_/ "), + SafeArg.of("comment", comment)); + } + } + + static boolean isValidComment(@Safe String comment) { + return comment != null + && !comment.isEmpty() + && !comment.startsWith(" ") + && !comment.endsWith(" ") + && COMMENT_VALID_CHARS.matchesAllOf(comment); } static boolean isValidName(String name) { @@ -227,7 +267,13 @@ static boolean isValidNodeId(String instanceId) { return NODE_REGEX.matcher(instanceId).matches(); } - static boolean isValidVersion(String version) { + static void checkNodeId(@Safe String instanceId) { + if (!isValidNodeId(instanceId)) { + throw new SafeIllegalArgumentException("Illegal node id format", SafeArg.of("nodeId", instanceId)); + } + } + + static boolean isValidVersion(@Safe String version) { if (VersionParser.countNumericDotGroups(version) >= 2 // fast path for numeric & dot only version numbers || versionMatchesRegex(version)) { return true; diff --git a/service-config/src/test/java/com/palantir/conjure/java/api/config/service/UserAgentTest.java b/service-config/src/test/java/com/palantir/conjure/java/api/config/service/UserAgentTest.java index 919914b31..048e8d2b1 100644 --- a/service-config/src/test/java/com/palantir/conjure/java/api/config/service/UserAgentTest.java +++ b/service-config/src/test/java/com/palantir/conjure/java/api/config/service/UserAgentTest.java @@ -18,13 +18,17 @@ import static com.palantir.logsafe.testing.Assertions.assertThatLoggableExceptionThrownBy; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; import com.palantir.conjure.java.api.config.service.UserAgent.Agent; import com.palantir.logsafe.SafeArg; import java.util.List; +import org.assertj.core.api.SoftAssertions; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; public class UserAgentTest { @@ -119,14 +123,15 @@ public void parse_handlesPrimaryAgent() { "service/10.20.30", "service/10.20.30 (nodeId:myNode)", }) { - assertThat(UserAgents.format(UserAgents.parse(agent))) - .withFailMessage(agent) - .isEqualTo(agent); + assertThat(UserAgents.format(UserAgents.parse(agent))).isEqualTo(agent); } - // Formatting ignores non-nodeId comments + // Formatting ignores invalid comments + assertThat(UserAgents.format(UserAgents.parse("service/1.2.3 (())"))).isEqualTo("service/1.2.3"); + + // Formatting retains valid comments assertThat(UserAgents.format(UserAgents.parse("service/1.2.3 (foo:bar)"))) - .isEqualTo("service/1.2.3"); + .isEqualTo("service/1.2.3 (foo:bar)"); // Finds primary agent even when there is a prefix assertThat(UserAgents.format(UserAgents.parse(" service/1.2.3"))).isEqualTo("service/1.2.3"); @@ -156,9 +161,12 @@ public void parse_handlesInformationalAgents() { .isEqualTo(agent); } - // nodeId on informational agents is omitted - assertThat(UserAgents.format(UserAgents.parse("serviceA/1.2.3 serviceB/4.5.6 (nodeId:myNode)"))) - .isEqualTo("serviceA/1.2.3 serviceB/4.5.6"); + // nodeId on informational agents is retained + UserAgent nodeIdOnInformational = UserAgents.parse("serviceA/1.2.3 serviceB/4.5.6 (nodeId:myNode)"); + assertThat(UserAgents.format(nodeIdOnInformational)).isEqualTo("serviceA/1.2.3 serviceB/4.5.6 (nodeId:myNode)"); + assertThat(nodeIdOnInformational.nodeId()) + .as("Only primary agents nodeId should be reported") + .isEmpty(); // Malformed informational agents are omitted assertThat(UserAgents.format(UserAgents.parse("serviceA/1.2.3 serviceB|4.5.6"))) @@ -169,16 +177,16 @@ public void parse_handlesInformationalAgents() { public void parse_canParseBrowserAgent() { String chrome = "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) " + "Chrome/61.0.3163.100 Safari/537.36"; - String expected = "Mozilla/5.0 AppleWebKit/537.36 Chrome/61.0.3163.100 Safari/537.36"; + String expected = "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) " + + "Chrome/61.0.3163.100 Safari/537.36"; assertThat(UserAgents.format(UserAgents.tryParse(chrome))).isEqualTo(expected); assertThat(UserAgents.format(UserAgents.parse(chrome))).isEqualTo(expected); } @Test public void parse_canParseBrowserAgentWithEmptyComment() { - String chrome = - "Mozilla/5.0 ( ) AppleWebKit/537.36 (KHTML, like Gecko) " + "Chrome/61.0.3163.100 Safari/537.36"; - String expected = "Mozilla/5.0 AppleWebKit/537.36 Chrome/61.0.3163.100 Safari/537.36"; + String chrome = "Mozilla/5.0 ( ) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.100 Safari/537.36"; + String expected = "Mozilla/5.0 AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.100 Safari/537.36"; assertThat(UserAgents.format(UserAgents.tryParse(chrome))).isEqualTo(expected); assertThat(UserAgents.format(UserAgents.parse(chrome))).isEqualTo(expected); } @@ -263,6 +271,63 @@ public void invalid_names() { assertThat("service_name").satisfies(UserAgentTest::isNotValidName); } + @Test + public void invalid_comment_messages() { + assertThatThrownBy(() -> UserAgents.checkComment(";")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Comment contains disallowed characters"); + assertThat(UserAgents.isValidComment(";")).isFalse(); + assertThatThrownBy(() -> UserAgents.checkComment(null)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Comment must not be null"); + assertThat(UserAgents.isValidComment(null)).isFalse(); + assertThatThrownBy(() -> UserAgents.checkComment("")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Comment must not be empty"); + assertThat(UserAgents.isValidComment("")).isFalse(); + assertThatThrownBy(() -> UserAgents.checkComment(" leading whitespace")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Comment must not start with whitespace"); + assertThat(UserAgents.isValidComment(" leading whitespace")).isFalse(); + assertThatThrownBy(() -> UserAgents.checkComment("trailing whitespace ")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Comment must not end with whitespace"); + assertThat(UserAgents.isValidComment("trailing whitespace ")).isFalse(); + } + + @ParameterizedTest + @ValueSource(strings = {"nodeId:nodeId", "MyResource/ri.abc.def.ghi-jkl", "KHTML, like Gecko"}) + public void valid_comments(String comment) { + SoftAssertions softly = new SoftAssertions(); + softly.assertThat(UserAgents.isValidComment(comment)).isTrue(); + softly.assertThatCode(() -> UserAgents.checkComment(comment)).doesNotThrowAnyException(); + softly.assertAll(); + } + + @ParameterizedTest + @ValueSource(strings = {";", "", " leading", "trailing ", "(parens)"}) + public void invalid_comments(String comment) { + SoftAssertions softly = new SoftAssertions(); + softly.assertThat(UserAgents.isValidComment(comment)).isFalse(); + softly.assertThatThrownBy(() -> UserAgents.checkComment(comment)).isInstanceOf(IllegalArgumentException.class); + softly.assertAll(); + } + + @Test + public void testNodeIdReplacement() { + UserAgent original = UserAgent.of(Agent.of("name", "0.0.0"), "nodeId1"); + UserAgent replacement = UserAgent.of(original.primary(), "nodeId2"); + assertThat(UserAgents.format(replacement)).isEqualTo("name/0.0.0 (nodeId:nodeId2)"); + } + + @Test + public void testNodeIdOnInformationalAgent() { + UserAgent original = UserAgent.of(Agent.of("primary", "0.0.0"), "nodeId1"); + UserAgent updated = original.addAgent( + UserAgent.of(Agent.of("info", "0.0.0"), "nodeId2").primary()); + assertThat(UserAgents.format(updated)).isEqualTo("primary/0.0.0 (nodeId:nodeId1) info/0.0.0 (nodeId:nodeId2)"); + } + private static void isValidName(String name) { assertThat(UserAgents.isValidName(name)).isTrue(); assertThat(name)