You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@solr.apache.org by st...@apache.org on 2023/09/06 22:18:09 UTC
[solr] branch main updated: SOLR-16955 Tracing v2 apis breaks SecurityConfHandler (#1884)
This is an automated email from the ASF dual-hosted git repository.
stillalex pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/solr.git
The following commit(s) were added to refs/heads/main by this push:
new bc7c8190de0 SOLR-16955 Tracing v2 apis breaks SecurityConfHandler (#1884)
bc7c8190de0 is described below
commit bc7c8190de0d91fcbef835018aa183f1d8f250ca
Author: Alex D <st...@apache.org>
AuthorDate: Wed Sep 6 15:18:03 2023 -0700
SOLR-16955 Tracing v2 apis breaks SecurityConfHandler (#1884)
---
solr/CHANGES.txt | 2 +
.../src/java/org/apache/solr/api/V2HttpCall.java | 18 +--
.../solr/handler/admin/SecurityConfHandler.java | 8 ++
.../apache/solr/servlet/SolrDispatchFilter.java | 8 +-
.../org/apache/solr/util/tracing/TraceUtils.java | 10 ++
.../BasicAuthIntegrationTracingTest.java | 121 +++++++++++++++++++++
.../solr/opentelemetry/TestDistributedTracing.java | 2 +-
7 files changed, 150 insertions(+), 19 deletions(-)
diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
index 5013377536d..a8fdf86d7ae 100644
--- a/solr/CHANGES.txt
+++ b/solr/CHANGES.txt
@@ -157,6 +157,8 @@ Bug Fixes
* SOLR-16958: Fix spurious warning about LATEST luceneMatchVersion (Colvin Cowie)
+* SOLR-16955: Tracing v2 apis breaks SecurityConfHandler (Alex Deparvu, David Smiley)
+
Dependency Upgrades
---------------------
diff --git a/solr/core/src/java/org/apache/solr/api/V2HttpCall.java b/solr/core/src/java/org/apache/solr/api/V2HttpCall.java
index 92008542a60..a57b286a9a3 100644
--- a/solr/core/src/java/org/apache/solr/api/V2HttpCall.java
+++ b/solr/core/src/java/org/apache/solr/api/V2HttpCall.java
@@ -44,7 +44,6 @@ import org.apache.solr.common.SolrException;
import org.apache.solr.common.cloud.DocCollection;
import org.apache.solr.common.cloud.ZkStateReader;
import org.apache.solr.common.params.CommonParams;
-import org.apache.solr.common.util.CommandOperation;
import org.apache.solr.common.util.JsonSchemaValidator;
import org.apache.solr.common.util.PathTrie;
import org.apache.solr.common.util.SuppressForbidden;
@@ -503,22 +502,7 @@ public class V2HttpCall extends HttpSolrCall {
// Get the templatize-ed path, ex: "/c/{collection}"
final String path = computeEndpointPath();
-
- String verb = null;
- // if this api has commands ...
- final Map<String, JsonSchemaValidator> validators = getValidators(); // should be cached
- if (validators != null && validators.isEmpty() == false && solrReq != null) {
- boolean validateInput = true; // because getCommands caches it; and we want it validated later
- // does this request have one command?
- List<CommandOperation> cmds = solrReq.getCommands(validateInput);
- if (cmds.size() == 1) {
- verb = cmds.get(0).name;
- }
- }
- if (verb == null) {
- verb = req.getMethod().toLowerCase(Locale.ROOT);
- }
-
+ final String verb = req.getMethod().toLowerCase(Locale.ROOT);
span.updateName(verb + ":" + path);
}
diff --git a/solr/core/src/java/org/apache/solr/handler/admin/SecurityConfHandler.java b/solr/core/src/java/org/apache/solr/handler/admin/SecurityConfHandler.java
index 9142449cba6..95e1d10f351 100644
--- a/solr/core/src/java/org/apache/solr/handler/admin/SecurityConfHandler.java
+++ b/solr/core/src/java/org/apache/solr/handler/admin/SecurityConfHandler.java
@@ -28,6 +28,7 @@ import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;
+import java.util.stream.Collectors;
import org.apache.solr.api.AnnotatedApi;
import org.apache.solr.api.Api;
import org.apache.solr.api.ApiBag;
@@ -52,6 +53,7 @@ import org.apache.solr.security.AuthorizationContext;
import org.apache.solr.security.AuthorizationPlugin;
import org.apache.solr.security.ConfigEditablePlugin;
import org.apache.solr.security.PermissionNameProvider;
+import org.apache.solr.util.tracing.TraceUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -147,6 +149,7 @@ public abstract class SecurityConfHandler extends RequestHandlerBase
if (persistConf(securityConfig)) {
securityConfEdited();
+ updateTraceOps(req, configEditablePlugin.getClass().getSimpleName(), commandsCopy);
return;
}
}
@@ -156,6 +159,11 @@ public abstract class SecurityConfHandler extends RequestHandlerBase
SERVER_ERROR, "Failed to persist security config after 3 attempts. Giving up");
}
+ private void updateTraceOps(SolrQueryRequest req, String clazz, List<CommandOperation> commands) {
+ TraceUtils.setOperations(
+ req, clazz, commands.stream().map(c -> c.name).collect(Collectors.toUnmodifiableList()));
+ }
+
/** Hook where you can do stuff after a config has been edited. Defaults to NOP */
protected void securityConfEdited() {}
diff --git a/solr/core/src/java/org/apache/solr/servlet/SolrDispatchFilter.java b/solr/core/src/java/org/apache/solr/servlet/SolrDispatchFilter.java
index 7b9c2b7e90c..4bb869ca1ec 100644
--- a/solr/core/src/java/org/apache/solr/servlet/SolrDispatchFilter.java
+++ b/solr/core/src/java/org/apache/solr/servlet/SolrDispatchFilter.java
@@ -239,7 +239,13 @@ public class SolrDispatchFilter extends BaseSolrFilter implements PathExcluder {
if (log.isDebugEnabled()) {
log.debug("User principal: {}", request.getUserPrincipal());
}
- TraceUtils.setUser(span, String.valueOf(request.getUserPrincipal()));
+ final String principalName;
+ if (request.getUserPrincipal() != null) {
+ principalName = request.getUserPrincipal().getName();
+ } else {
+ principalName = null;
+ }
+ TraceUtils.setUser(span, String.valueOf(principalName));
}
HttpSolrCall call = getHttpSolrCall(request, response, retry);
diff --git a/solr/core/src/java/org/apache/solr/util/tracing/TraceUtils.java b/solr/core/src/java/org/apache/solr/util/tracing/TraceUtils.java
index 3505f4daac6..e63d8c6cbde 100644
--- a/solr/core/src/java/org/apache/solr/util/tracing/TraceUtils.java
+++ b/solr/core/src/java/org/apache/solr/util/tracing/TraceUtils.java
@@ -25,6 +25,7 @@ import io.opentelemetry.api.trace.Tracer;
import io.opentelemetry.api.trace.TracerProvider;
import io.opentelemetry.context.Context;
import io.opentelemetry.context.propagation.TextMapPropagator;
+import java.util.List;
import java.util.function.Consumer;
import javax.servlet.http.HttpServletRequest;
import org.apache.solr.client.solrj.SolrRequest;
@@ -52,6 +53,8 @@ public class TraceUtils {
public static final AttributeKey<String> TAG_RESPONSE_WRITER =
AttributeKey.stringKey("responseWriter");
public static final AttributeKey<String> TAG_CONTENT_TYPE = AttributeKey.stringKey("contentType");
+ public static final AttributeKey<List<String>> TAG_OPS = AttributeKey.stringArrayKey("ops");
+ public static final AttributeKey<String> TAG_CLASS = AttributeKey.stringKey("class");
@Deprecated
private static final AttributeKey<String> TAG_HTTP_METHOD_DEP =
@@ -140,4 +143,11 @@ public class TraceUtils {
spanBuilder.setAttribute(TAG_DB_TYPE, TAG_DB_TYPE_SOLR);
return spanBuilder.startSpan();
}
+
+ public static void setOperations(SolrQueryRequest req, String clazz, List<String> ops) {
+ if (!ops.isEmpty()) {
+ req.getSpan().setAttribute(TAG_OPS, ops);
+ req.getSpan().setAttribute(TAG_CLASS, clazz);
+ }
+ }
}
diff --git a/solr/modules/opentelemetry/src/test/org/apache/solr/opentelemetry/BasicAuthIntegrationTracingTest.java b/solr/modules/opentelemetry/src/test/org/apache/solr/opentelemetry/BasicAuthIntegrationTracingTest.java
new file mode 100644
index 00000000000..cdbf9f27141
--- /dev/null
+++ b/solr/modules/opentelemetry/src/test/org/apache/solr/opentelemetry/BasicAuthIntegrationTracingTest.java
@@ -0,0 +1,121 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.opentelemetry;
+
+import static java.util.Collections.singletonList;
+import static java.util.Collections.singletonMap;
+import static org.apache.solr.opentelemetry.TestDistributedTracing.getAndClearSpans;
+import static org.apache.solr.security.Sha256AuthenticationProvider.getSaltedHashedValue;
+
+import io.opentelemetry.api.GlobalOpenTelemetry;
+import io.opentelemetry.api.trace.TracerProvider;
+import java.util.List;
+import java.util.Map;
+import org.apache.solr.client.solrj.SolrRequest;
+import org.apache.solr.client.solrj.impl.CloudSolrClient;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.client.solrj.request.V2Request;
+import org.apache.solr.cloud.SolrCloudTestCase;
+import org.apache.solr.common.util.Utils;
+import org.apache.solr.security.BasicAuthPlugin;
+import org.apache.solr.security.RuleBasedAuthorizationPlugin;
+import org.apache.solr.util.tracing.TraceUtils;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+public class BasicAuthIntegrationTracingTest extends SolrCloudTestCase {
+
+ private static final String COLLECTION = "collection1";
+ private static final String USER = "solr";
+ private static final String PASS = "SolrRocksAgain";
+ private static final String SECURITY_JSON =
+ Utils.toJSONString(
+ Map.of(
+ "authorization",
+ Map.of(
+ "class",
+ RuleBasedAuthorizationPlugin.class.getName(),
+ "user-role",
+ singletonMap(USER, "admin"),
+ "permissions",
+ singletonList(Map.of("name", "all", "role", "admin"))),
+ "authentication",
+ Map.of(
+ "class",
+ BasicAuthPlugin.class.getName(),
+ "blockUnknown",
+ true,
+ "credentials",
+ singletonMap(USER, getSaltedHashedValue(PASS)))));
+
+ @BeforeClass
+ public static void setupCluster() throws Exception {
+ // force early init
+ CustomTestOtelTracerConfigurator.prepareForTest();
+
+ configureCluster(4)
+ .addConfig("config", TEST_PATH().resolve("collection1").resolve("conf"))
+ .withSolrXml(TEST_PATH().resolve("solr.xml"))
+ .withTraceIdGenerationDisabled()
+ .withSecurityJson(SECURITY_JSON)
+ .configure();
+
+ assertNotEquals(
+ "Expecting active otel, not noop impl",
+ TracerProvider.noop(),
+ GlobalOpenTelemetry.get().getTracerProvider());
+
+ CollectionAdminRequest.createCollection(COLLECTION, "config", 2, 2)
+ .setBasicAuthCredentials(USER, PASS)
+ .process(cluster.getSolrClient());
+ cluster.waitForActiveCollection(COLLECTION, 2, 4);
+ }
+
+ @AfterClass
+ public static void afterClass() {
+ CustomTestOtelTracerConfigurator.resetForTest();
+ }
+
+ /** See SOLR-16955 */
+ @Test
+ public void testSetupBasicAuth() throws Exception {
+ getAndClearSpans(); // reset
+
+ CloudSolrClient cloudClient = cluster.getSolrClient();
+ Map<String, Object> ops =
+ Map.of(
+ "set-user", Map.of("harry", "HarryIsCool"),
+ "set-property", Map.of("blockUnknown", true));
+ V2Request req =
+ new V2Request.Builder("/cluster/security/authentication")
+ .withMethod(SolrRequest.METHOD.POST)
+ .withPayload(Utils.toJSONString(ops))
+ .build();
+ req.setBasicAuthCredentials(USER, PASS);
+ assertEquals(0, req.process(cloudClient, COLLECTION).getStatus());
+
+ var finishedSpans = getAndClearSpans();
+ assertEquals(1, finishedSpans.size());
+ var span = finishedSpans.get(0);
+ assertEquals("post:/cluster/security/authentication", span.getName());
+ assertEquals("solr", span.getAttributes().get(TraceUtils.TAG_USER));
+ assertEquals(
+ BasicAuthPlugin.class.getSimpleName(), span.getAttributes().get(TraceUtils.TAG_CLASS));
+ assertEquals(List.copyOf(ops.keySet()), span.getAttributes().get(TraceUtils.TAG_OPS));
+ }
+}
diff --git a/solr/modules/opentelemetry/src/test/org/apache/solr/opentelemetry/TestDistributedTracing.java b/solr/modules/opentelemetry/src/test/org/apache/solr/opentelemetry/TestDistributedTracing.java
index f6e088e5168..23cf76f1cbf 100644
--- a/solr/modules/opentelemetry/src/test/org/apache/solr/opentelemetry/TestDistributedTracing.java
+++ b/solr/modules/opentelemetry/src/test/org/apache/solr/opentelemetry/TestDistributedTracing.java
@@ -201,7 +201,7 @@ public class TestDistributedTracing extends SolrCloudTestCase {
assertEquals(child.getTraceId(), parent.getTraceId());
}
- private List<SpanData> getAndClearSpans() {
+ static List<SpanData> getAndClearSpans() {
InMemorySpanExporter exporter = CustomTestOtelTracerConfigurator.getInMemorySpanExporter();
List<SpanData> result = new ArrayList<>(exporter.getFinishedSpanItems());
Collections.reverse(result); // nicer to see spans chronologically