Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
title: "A single node system property can be fetched at GET /api/node/properties/{propertyName} (v1 still uses ?name=). SolrJ now provides NodeApi.GetNodeProperties and NodeApi.GetNodeProperty."
type: added
authors:
- name: Prithvi S
nick: iprithv
links:
- name: SOLR-16458
url: https://issues.apache.org/jira/browse/SOLR-16458
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
/*
* 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.client.api.endpoint;

import io.swagger.v3.oas.annotations.Operation;
import io.swagger.v3.oas.annotations.Parameter;
import jakarta.ws.rs.GET;
import jakarta.ws.rs.Path;
import jakarta.ws.rs.PathParam;
import org.apache.solr.client.api.model.NodePropertiesResponse;

/** V2 API definition for listing JRE system properties on a Solr node. */
@Path("/node/properties")
public interface NodePropertiesApi {

@GET
@Operation(
summary = "List system properties for the target Solr node.",
tags = {"node"})
NodePropertiesResponse getNodeProperties();
Comment thread
epugh marked this conversation as resolved.

@GET
@Path("/{propertyName}")
@Operation(
summary = "Get a single system property for the target Solr node.",
tags = {"node"})
NodePropertiesResponse getNodeProperty(
@Parameter(description = "Name of the system property to return.") @PathParam("propertyName")
String propertyName);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/*
* 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.client.api.model;

import com.fasterxml.jackson.annotation.JsonProperty;
import io.swagger.v3.oas.annotations.media.Schema;
import java.util.Map;

/**
* Response body for {@code GET /api/node/properties} and {@code GET
* /api/node/properties/{propertyName}}.
*/
public class NodePropertiesResponse extends SolrJerseyResponse {

public static final String SYSTEM_PROPERTIES = "system.properties";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Q] Does this field name pre-exist this PR? In general we prefer camelCase, so if this is "new" then we should standardize on that. But if it's pre-existing,let's not worry about it...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, v1 already used system.properties (Admin UI, NodeValueFetcher, etc.)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so, now is a good time to make these changes... how hard would it be to keep system.properties in v1, but have systemProperties in v2? And, are there a whole bunch of other weridly named fields like this? Or is this the only one?


@Schema(description = "JRE system properties for the Solr node. Secret values are redacted.")
@JsonProperty(SYSTEM_PROPERTIES)
public Map<String, String> systemProperties;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it really as simple as a string/string? Great.

}
Original file line number Diff line number Diff line change
Expand Up @@ -20,20 +20,22 @@

import java.io.IOException;
import java.util.Collection;
import java.util.Enumeration;
import org.apache.solr.api.AnnotatedApi;
import java.util.List;
import org.apache.solr.api.Api;
import org.apache.solr.common.util.NamedList;
import org.apache.solr.common.util.SimpleOrderedMap;
import org.apache.solr.api.JerseyResource;
import org.apache.solr.client.api.model.NodePropertiesResponse;
import org.apache.solr.core.CoreContainer;
import org.apache.solr.core.NodeConfig;
import org.apache.solr.handler.RequestHandlerBase;
import org.apache.solr.handler.admin.api.NodePropertiesAPI;
import org.apache.solr.handler.admin.api.GetNodeProperties;
import org.apache.solr.handler.api.V2ApiUtils;
import org.apache.solr.request.SolrQueryRequest;
import org.apache.solr.response.SolrQueryResponse;
import org.apache.solr.security.AuthorizationContext;

/**
* v1 implementation of {@code GET /admin/info/properties}. Business logic lives in {@link
Comment thread
epugh marked this conversation as resolved.
* GetNodeProperties}.
*
* @since solr 1.2
*/
public class PropertiesRequestHandler extends RequestHandlerBase {
Expand All @@ -51,21 +53,12 @@ public PropertiesRequestHandler(CoreContainer cc) {

@Override
public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp) throws IOException {
NamedList<String> props = new SimpleOrderedMap<>();
String name = req.getParams().get(NAME);
NodeConfig nodeConfig = getCoreContainer(req).getNodeConfig();
if (name != null) {
String property = nodeConfig.getRedactedSysPropValue(name);
props.add(name, property);
} else {
Enumeration<?> enumeration = System.getProperties().propertyNames();
while (enumeration.hasMoreElements()) {
name = (String) enumeration.nextElement();
props.add(name, nodeConfig.getRedactedSysPropValue(name));
}
}
rsp.add("system.properties", props);
rsp.setHttpCaching(false);
final GetNodeProperties api = new GetNodeProperties(getCoreContainer(req));
final NodePropertiesResponse response = new NodePropertiesResponse();
// v1 ?name= returns the key even if unset; the v2 path form 404s for unknown names.
response.systemProperties = api.collectProperties(req.getParams().get(NAME));
V2ApiUtils.squashIntoSolrResponseWithoutHeader(rsp, response);
}

//////////////////////// SolrInfoMBeans methods //////////////////////
Expand All @@ -82,7 +75,12 @@ public Category getCategory() {

@Override
public Collection<Api> getApis() {
return AnnotatedApi.getApis(new NodePropertiesAPI(this));
return List.of();
}

@Override
public Collection<Class<? extends JerseyResource>> getJerseyResources() {
return List.of(GetNodeProperties.class);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
/*
* 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.handler.admin.api;

import jakarta.inject.Inject;
import java.util.Enumeration;
import java.util.LinkedHashMap;
import java.util.Map;
import org.apache.solr.api.JerseyResource;
import org.apache.solr.client.api.endpoint.NodePropertiesApi;
import org.apache.solr.client.api.model.NodePropertiesResponse;
import org.apache.solr.common.SolrException;
import org.apache.solr.core.CoreContainer;
import org.apache.solr.core.NodeConfig;
import org.apache.solr.jersey.PermissionName;
import org.apache.solr.security.PermissionNameProvider;

/**
* V2 API for listing system properties on the receiving node.
*
* <p>GET /api/node/properties lists all properties. GET /api/node/properties/{propertyName} returns
* a single property. Both are analogous to v1 /admin/info/properties, which still uses a {@code
* name} query parameter for the single-property form.
*
* <p>The v1 {@link org.apache.solr.handler.admin.PropertiesRequestHandler} delegates to this class.
*/
public class GetNodeProperties extends JerseyResource implements NodePropertiesApi {

private final CoreContainer coreContainer;

@Inject
public GetNodeProperties(CoreContainer coreContainer) {
this.coreContainer = coreContainer;
}

@Override
@PermissionName(PermissionNameProvider.Name.CONFIG_READ_PERM)
public NodePropertiesResponse getNodeProperties() {
return buildResponse(null);
}

@Override
@PermissionName(PermissionNameProvider.Name.CONFIG_READ_PERM)
public NodePropertiesResponse getNodeProperty(String propertyName) {
final NodeConfig nodeConfig = coreContainer.getNodeConfig();
// Hidden names always return 200 + a redacted value, even if unset, so callers cannot

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is super helpful, thank you! 🎉

// probe whether a secret is configured.
if (!System.getProperties().containsKey(propertyName)
&& !nodeConfig.isSysPropHidden(propertyName)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Q] Why does the redacted-ness factor into whether we throw a 404 here or not? Might be missing it, but I don't see this logic in the original API...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

404 + hidden, original API never 404’d. On the v2 path we 404 unknown names, but hidden names always return --REDACTED-- so you can’t tell whether a secret is set. I added a comment and a test for that.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha - that makes sense, thanks for explaining!

throw new SolrException(
SolrException.ErrorCode.NOT_FOUND,
"No system property named '" + propertyName + "' exists on this node.");
}
return buildResponse(propertyName);
}

private NodePropertiesResponse buildResponse(String name) {
final NodePropertiesResponse response = instantiateJerseyResponse(NodePropertiesResponse.class);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[0] It's nbd but I don't think we strictly need instantiateJerseyResponse here. That's primarily useful on APIs with many sub-steps where we may want the response to contain information about both the steps that succeeded and those that failed (think collection creation)

Having it here won't do any harm though afaik, so purely a preference thing 🤷

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left it for now, mostly to match the other node APIs. happy to drop it if you’d rather :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, that's fine 👍

response.systemProperties = collectProperties(name);
return response;
Comment on lines +71 to +74

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this "disable http caching" is actually a red herring, and we don't need it. @gerlowskija I would be curious if you have any sense that we "need" this caching? Claude, when I dug into it loclaly, didn't think so, and I can't figure out why it would be needed. In fact, curious if GetNodeSystemInfo does...?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is something that ended up in some of our JAX-RS APIs because someone decided on it at some point on the v1 codepath. For example, see us disabling caching in the CollectionsHandler here.

So in general I think the AI is probably right here that giving hints about cacheability is valid and appropriate. Whether that should be done in individual APIs or as some sort of "ContainerResponseFilter" (see PostRequestLoggingFilter), I'm not really sure.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, so I think I read things backwards... I thought it was enforcing caching (when I talked to you @gerlowskija) but the recommendation is to prevent caching. I will open a seperate JIRA for saying "lets look at cahcing for admin v2 apis".. I don't really want to try to one off solve it in this one PR since hoenstly we haven' teven seen this as an issue! So let's solve it indepdent of this one.. also, so I can merge thisn ;-).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to influence the caching in this class, the way to do it is to have the constructor inject a "SolrQueryResponse" object and then call the appropriate caching method on that instance.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will open a seperate JIRA for saying "lets look at cahcing for admin v2 apis".

IMO we're not really consistent on the v1 side either, so I'd use a broader scope of: "Re-evaluate use of caching headers in all Solr APIs". We may decide to rip them out across the board, who knows 🤷

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you, I poked at this a bit more, GetNodeSystemInfo does call setHttpCaching(false), but it doesn’t actually do anything.. that flag only turns into Cache-Control headers on the core request path, not admin/node APIs. same for CollectionsHandler. so we aren’t dropping any headers that used to be there.

If we do want real no-cache on admin GETs, I think a filter or handleAdminRequest would be a better place for it than this API.. happy to leave that for a follow-up unless you’d rather we do it here?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}

/** Collect redacted system properties, optionally limited to a single named property. */
public Map<String, String> collectProperties(String name) {
final NodeConfig nodeConfig = coreContainer.getNodeConfig();
final Map<String, String> props = new LinkedHashMap<>();
if (name != null) {
props.put(name, nodeConfig.getRedactedSysPropValue(name));
} else {
Enumeration<?> enumeration = System.getProperties().propertyNames();
while (enumeration.hasMoreElements()) {
String propertyName = (String) enumeration.nextElement();
props.put(propertyName, nodeConfig.getRedactedSysPropValue(propertyName));
}
}
return props;
}
}

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,13 @@
*/
package org.apache.solr.handler.admin;

import java.util.Map;
import org.apache.solr.SolrTestCaseJ4;
import org.apache.solr.client.solrj.SolrClient;
import org.apache.solr.client.solrj.SolrRequest;
import org.apache.solr.client.solrj.embedded.EmbeddedSolrServer;
import org.apache.solr.client.solrj.request.GenericSolrRequest;
import org.apache.solr.common.params.ModifiableSolrParams;
import org.apache.solr.common.util.NamedList;
import org.apache.solr.core.NodeConfig;
import org.junit.BeforeClass;
Expand All @@ -45,7 +47,7 @@ public void testRedaction() throws Exception {
"some.Secret"
}) {
System.setProperty(propName, PASSWORD);
NamedList<Object> properties = readProperties();
Map<String, Object> properties = readProperties();

assertEquals(
"Failed to redact " + propName,
Expand All @@ -54,13 +56,41 @@ public void testRedaction() throws Exception {
}
}

@Test
public void testSingleProperty() throws Exception {
System.setProperty("GetNodeProperties.v1.visible", "hello");
try {
Map<String, Object> properties = readProperties("GetNodeProperties.v1.visible");
assertEquals(1, properties.size());
assertEquals("hello", properties.get("GetNodeProperties.v1.visible"));
} finally {
System.clearProperty("GetNodeProperties.v1.visible");
}
}

@Test
public void testMissingPropertyStillReturned() throws Exception {
Map<String, Object> properties = readProperties("GetNodeProperties.v1.doesNotExist");
assertEquals(1, properties.size());
assertTrue(properties.containsKey("GetNodeProperties.v1.doesNotExist"));
assertNull(properties.get("GetNodeProperties.v1.doesNotExist"));
}

private Map<String, Object> readProperties() throws Exception {
return readProperties(null);
}

@SuppressWarnings({"unchecked"})
private NamedList<Object> readProperties() throws Exception {
private Map<String, Object> readProperties(String name) throws Exception {
SolrClient client = new EmbeddedSolrServer(h.getCore());

ModifiableSolrParams params = new ModifiableSolrParams();
if (name != null) {
params.set("name", name);
}
NamedList<Object> properties =
client.request(new GenericSolrRequest(SolrRequest.METHOD.GET, "/admin/info/properties"));
client.request(
new GenericSolrRequest(SolrRequest.METHOD.GET, "/admin/info/properties", params));

return (NamedList<Object>) properties.get("system.properties");
return (Map<String, Object>) properties.get("system.properties");
}
}
Loading