SOLR-16980 Connect to SOLR standalone with basic authentication #1957

stillalex · 2023-09-25T15:22:30Z

https://issues.apache.org/jira/browse/SOLR-16980

Description

Adding support for passing basic auth details to cli command

Solution

Please provide a short description of the approach taken to implement your solution.

Tests

Please describe the tests you've developed or run to confirm this patch implements the feature or solves the problem.

Checklist

Please review the following and check all that apply:

I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
I have created a Jira issue and added the issue ID to my pull request title.
I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
I have developed this patch against the main branch.
I have run ./gradlew check.
I have added tests for my changes.
I have added documentation for the Reference Guide

epugh · 2023-09-25T15:44:16Z

Awesome work @stillalex .... Have you looked at #1954 (SOLR-14496) where I took a stab at adding credentials to the SolrCLI? It would be great if we used the same patterns in both there AND here... For example, I went with -u eric:password instead of seperate username and password params, mostly because curl does it that way!

Sigh, in looking, looks like we use net.sourceforge.argparse4j instead of commons-cli... Wish we picked one CLI arg parser for them all!

Lastly, are there any improvements here that could be reused in better handling of the CLI stuff? Would that change any of this code?

epugh · 2023-09-25T15:45:35Z

solr/prometheus-exporter/src/java/org/apache/solr/prometheus/exporter/SolrClientFactory.java

Do we need this class? Probably, but I wonder if it actually is required with all the other builders we have for Solr Clients? Seems a shame to have "YASCB". yet another solr client builder..... ;-)

it wires the existing builders with params passed via cli, it didn't fee like a big problem to me.

Maybe I just wished we could use one of the other factory/builders we have.. You are right that this probably isn't a big deal ;-)...

epugh · 2023-09-25T15:46:18Z

solr/prometheus-exporter/src/java/org/apache/solr/prometheus/exporter/SolrExporter.java

@@ -96,6 +96,20 @@ public class SolrExporter {
          + ARG_NUM_THREADS_DEFAULT
          + ".";

+  private static final String[] ARG_USER_FLAGS = {"-u", "--user"};


at least we have --user instead of -user, over on SolrCLI, we want to get to the -- pattern!

I moved it all to 'credentials' similar to the example PR.

epugh · 2023-09-25T15:46:44Z

solr/prometheus-exporter/src/java/org/apache/solr/prometheus/exporter/SolrExporter.java

@@ -242,6 +256,22 @@ public static void main(String[] args) {
        .setDefault(ARG_CLUSTER_ID_DEFAULT)
        .help(ARG_CLUSTER_ID_HELP);

+    parser


Curious if we love this parser or not for CLI args??? THoughts on this versus the commons-cli ?

I used what was there already :( I can take a stab at rewriting this cli code on top of commons-cli

epugh · 2023-09-25T15:47:31Z

solr/prometheus-exporter/src/java/org/apache/solr/prometheus/exporter/SolrExporter.java

+      if (!res.getString(ARG_USER_DEST).isEmpty() && !res.getString(ARG_PASSWORD_DEST).isEmpty()) {
+        String user = res.getString(ARG_USER_DEST);
+        String password = res.getString(ARG_PASSWORD_DEST);
+        scrapeConfiguration.withBasicAuthCredentials(user, password);


i ended up littering the #1954 code iwth lots of things that look like this too ;-).

epugh · 2023-09-25T15:49:03Z

solr/prometheus-exporter/src/test-files/solr/security.json

have you seen the new "sane defaults" security.json? It declares four roles, index, search, admin, and superadmin. I wonder if it would make sense to add a "monitor" role to that as part of this? So you do:

bin/solr auth...

and now you are ready for Prometheus?

similar to the kubernetes setup (I think)? sounds like a good idea

Here is the "default" security.json that we now have! https://github.com/apache/solr/blob/main/solr/core/src/resources/security.json

I see a metrics-read permission here. is this what you were hinting to? like maybe add a role to include this one

yeah, a "metrics" role or a "monitor" role that has what you would need, that way you don't have to figure that stuff out just to use Prometheus... (assuming there are some logical permissions you need)

epugh · 2023-09-25T15:49:58Z

...exporter/src/test/org/apache/solr/prometheus/scraper/SolrStandaloneScraperBasicAuthTest.java

+  public static void setupSolrHome() throws Exception {
+    Path solrHome = LuceneTestCase.createTempDir();
+    Files.copy(
+        SolrTestCaseJ4.TEST_PATH().resolve("security.json"), solrHome.resolve("security.json"));


a thought... that if security.json isn't special for your test, just use the one that now ships with Solr?

it only has a user+pass. if one already exists and is available to this module I can try to use it, sure

trying to find an example for this. would you happen to have one that I could use as a reference?

https://github.com/apache/solr/blob/main/solr/core/src/resources/security.json is the one that was introduced...

I thin the trouble is prometheus module does not depend on core (not even for testing)? bringing that in just for access to the security file might be overkill.

found a cleaner way. removed the file and I am recreating it as needed.

epugh · 2023-09-25T15:51:19Z

...exporter/src/test/org/apache/solr/prometheus/scraper/SolrStandaloneScraperBasicAuthTest.java

+            25, new SolrNamedThreadFactory("solr-cloud-scraper-tests"));
+    solrScraper = new SolrStandaloneScraper(solrClient, executor, "test");
+
+    Helpers.indexAllDocs(solrClient);


don't love this class name? Helpers feels very generic... Also, we don't have the equivaletn of indexAllDocs somewhere already?

yeah. class name is the existing test class name plus BasicAuth :) I can change if you think there is a better name

Actually, the class name I didn't love was Helpers... ;-) I feel like that method indexAllDocs must exist already!

ah right. this was already there. I picked up the SolrStandaloneScraperTest test (cleaned it up a bit) and added basic auth to it. indexAllDocs will actually index a local copy of the solr examples files. I'm pretty sure this was not strictly needed for this, but yeah I didn't add any of it.

epugh · 2023-09-25T15:52:13Z

...ometheus-exporter/src/test/org/apache/solr/prometheus/scraper/SolrStandaloneScraperTest.java

+    Files.copy(top.resolve("managed-schema.xml"), subHome.resolve("schema.xml"));
+    Files.copy(top.resolve("solrconfig.xml"), subHome.resolve("solrconfig.xml"));
+
+    Files.copy(top.resolve("stopwords.txt"), subHome.resolve("stopwords.txt"));


wish we had a different more minimal config that dind't drag along stopwords and synnymos....

trouble with using the minimal config is that the data indexed here uses multi value strings heavily which is not allowed in the existing min config. it will fail indexing, so this is the best I could come up with

makes sense. Maybe someday we figure out how many tests actually want stopwords, or which ones want stopwords, and only have those tests depend on it!

epugh · 2023-09-25T15:52:24Z

...ometheus-exporter/src/test/org/apache/solr/prometheus/scraper/SolrStandaloneScraperTest.java

@@ -127,7 +136,7 @@ public void metricsForHost() throws Exception {
    assertEquals(1, metricsByHost.size());

    List<Collector.MetricFamilySamples> replicaSamples =
-        metricsByHost.get(restTestHarness.getAdminURL()).asList();
+        metricsByHost.get(solrRule.getBaseUrl()).asList();


stillalex · 2023-09-25T16:29:09Z

thank you for the review @epugh!

Awesome work @stillalex .... Have you looked at #1954 (SOLR-14496) where I took a stab at adding credentials to the SolrCLI? It would be great if we used the same patterns in both there AND here... For example, I went with -u eric:password instead of seperate username and password params, mostly because curl does it that way!

I have not, no. will take a look and try to make it consistent with #1954. -u style works too I don't have a strong preference here.

Sigh, in looking, looks like we use net.sourceforge.argparse4j instead of commons-cli... Wish we picked one CLI arg parser for them all!

strange. did not even notice this until you said it. will try to see how complicated the refactor to commons-cli would be

epugh · 2023-09-25T17:36:10Z

thank you for the review @epugh!

Awesome work @stillalex .... Have you looked at #1954 (SOLR-14496) where I took a stab at adding credentials to the SolrCLI? It would be great if we used the same patterns in both there AND here... For example, I went with -u eric:password instead of seperate username and password params, mostly because curl does it that way!

I have not, no. will take a look and try to make it consistent with #1954. -u style works too I don't have a strong preference here.

Sigh, in looking, looks like we use net.sourceforge.argparse4j instead of commons-cli... Wish we picked one CLI arg parser for them all!

strange. did not even notice this until you said it. will try to see how complicated the refactor to commons-cli would be

Also, not wedded to commons-cli... I think if the community likes argparse4j, then happy to go that way as well ;-)

epugh · 2023-09-25T18:05:14Z

solr/prometheus-exporter/src/java/org/apache/solr/prometheus/exporter/SolrExporter.java

@@ -96,6 +96,13 @@ public class SolrExporter {
          + ARG_NUM_THREADS_DEFAULT
          + ".";

+  private static final String[] ARG_CREDENTIALS_FLAGS = {"-u", "--credentials"};


stillalex · 2023-09-25T18:06:25Z

@epugh re. commons-cli vs argparse4j I want to take a stab at rewriting, but I would like to do it as a part of a different PR as an improvement. for this PR i am focusing on resolving a regression: not being able to use prometheus with basic auth. if that works for you I will create another Jira to track it so it doesn't get lost.

epugh · 2023-09-25T18:12:02Z

@epugh re. commons-cli vs argparse4j I want to take a stab at rewriting, but I would like to do it as a part of a different PR as an improvement. for this PR i am focusing on resolving a regression: not being able to use prometheus with basic auth. if that works for you I will create another Jira to track it so it doesn't get lost.

Awesome! Totally agree! You could link it to https://issues.apache.org/jira/browse/SOLR-16757 which is what I'm using to guide me in trying to get our varous CLI's to a better place.

stillalex · 2023-09-25T18:25:51Z

sure, created SOLR-16996 and linked to umbrella ticket.

…-basicauth

(cherry picked from commit 61d41e5)

stillalex added 2 commits September 25, 2023 08:21

SOLR-16980 Connect to SOLR standalone with basic authentication

69d6a92

added new params to solr guide

47067fb

epugh reviewed Sep 25, 2023

View reviewed changes

fixed test dependencies

0ee5898

epugh reviewed Sep 25, 2023

View reviewed changes

unified credentials arg

af6f628

epugh reviewed Sep 25, 2023

View reviewed changes

removed security file

8ff91b4

stillalex added 5 commits September 25, 2023 11:29

updated guide again

ce6c801

split w limit 2

be3d616

Merge remote-tracking branch 'apache/main' into SOLR-16980-prometheus…

74120fa

…-basicauth

import cleanup

9c554b6

changes.txt

de9eb3f

stillalex merged commit 61d41e5 into apache:main Sep 26, 2023
2 checks passed

stillalex deleted the SOLR-16980-prometheus-basicauth branch September 26, 2023 13:14

stillalex added a commit that referenced this pull request Sep 26, 2023

SOLR-16980 Connect to SOLR standalone with basic authentication (#1957)

43a6161

(cherry picked from commit 61d41e5)

SOLR-16980 Connect to SOLR standalone with basic authentication #1957

SOLR-16980 Connect to SOLR standalone with basic authentication #1957

Conversation

stillalex commented Sep 25, 2023

Description

Solution

Tests

Checklist

epugh commented Sep 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stillalex Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stillalex commented Sep 25, 2023

epugh commented Sep 25, 2023

Choose a reason for hiding this comment

stillalex commented Sep 25, 2023

epugh commented Sep 25, 2023

stillalex commented Sep 25, 2023

stillalex Sep 25, 2023 •

edited

Loading