Uploaded image for project: 'Solr'
  1. Solr
  2. SOLR-8677

SOLR allows creation of shards with invalid names.

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 6.0
    • Fix Version/s: 6.0
    • Component/s: None
    • Labels:
      None

      Description

      Solr currently has "recommendations" about what constitutes a valid identifier, but doesn't enforce these "recommendations" uniformly. Core (SOLR-8308) and collection (SOLR-8642) names are currently checked, but shards aren't.

      $ bin/solr -e cloud -noprompt
          ....
      $ curl -i -l -k -X GET "http://localhost:8983/solr/admin/collections?action=CREATE&name=coll1&router.name=implicit&numShards=1&shards=bad+shard+name"
      HTTP/1.1 200 OK
      Content-Type: application/xml; charset=UTF-8
      Transfer-Encoding: chunked
      
      <?xml version="1.0" encoding="UTF-8"?>
      <response>
      <lst name="responseHeader"><int name="status">0</int><int name="QTime">204</int></lst><lst name="failure"><str>org.apache.solr.client.solrj.impl.HttpSolrClient$RemoteSolrException:Error from server at http://127.0.1.1:8983/solr: Error CREATEing SolrCore 'coll1_bad shard name_replica1': Unable to create core [coll1_bad shard name_replica1] Caused by: Invalid name: 'coll1_bad shard name_replica1' Identifiers must consist entirely of periods, underscores and alphanumerics</str></lst>
      </response>
      

      (Note that the CREATE command above returned 200-OK, and the failure was only apparent when viewing the message.)

      A CLUSTERSTATUS shows that the shard was actually created, but has no underlying cores.

      $ curl -i -l -k -X GET "http://localhost:8983/solr/admin/collections?action=CLUSTERSTATUS&wt=json&indent=true"
      ...
          "collections":{
            "coll1":{
              "replicationFactor":"1",
              "shards":{"bad shard name":{
                  "range":null,
                  "state":"active",
                  "replicas":{}}},
              "router":{"name":"implicit"},
              "maxShardsPerNode":"1",
              "autoAddReplicas":"false",
              "znodeVersion":1,
              "configName":"gettingstarted"},
      ...
      

      This JIRA proposes adding a check to ensure that shard names meet SOLR's identifier "recommendations". This should prevent users from accidentally putting themselves in a bad state.

      1. SOLR-8677.patch
        34 kB
        Anshum Gupta
      2. SOLR-8677.patch
        31 kB
        Anshum Gupta
      3. SOLR-8677.patch
        28 kB
        Jason Gerlowski
      4. SOLR-8677.patch
        7 kB
        Jason Gerlowski
      5. SOLR-8677-5x-revert.patch
        34 kB
        Anshum Gupta

        Issue Links

          Activity

          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 297b193dec2213dd48cc308ae59ddc0c4845d4d0 in lucene-solr's branch refs/heads/branch_5x from anshum
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=297b193 ]

          Revert "SOLR-8677: Restrict creation of shards with invalid names"

          This reverts commit 96c01a2c885871f7d80beddc6e019547639ef71e.

          Show
          jira-bot ASF subversion and git services added a comment - Commit 297b193dec2213dd48cc308ae59ddc0c4845d4d0 in lucene-solr's branch refs/heads/branch_5x from anshum [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=297b193 ] Revert " SOLR-8677 : Restrict creation of shards with invalid names" This reverts commit 96c01a2c885871f7d80beddc6e019547639ef71e.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 69a8aa77e36216b329dc159c391ad2f0155de740 in lucene-solr's branch refs/heads/branch_5x from anshum
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=69a8aa7 ]

          Revert "SOLR-8677: Fix broken build"

          This reverts commit 55162f2255a1f707603cc81134996cfb2a5968ec.

          Show
          jira-bot ASF subversion and git services added a comment - Commit 69a8aa77e36216b329dc159c391ad2f0155de740 in lucene-solr's branch refs/heads/branch_5x from anshum [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=69a8aa7 ] Revert " SOLR-8677 : Fix broken build" This reverts commit 55162f2255a1f707603cc81134996cfb2a5968ec.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 7415d100ef09a9a10b45746ebb794452e288ca3a in lucene-solr's branch refs/heads/branch_5x from anshum
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=7415d10 ]

          Revert "SOLR-8677: Fix assert statement"

          This reverts commit e8acc04c68ac74ca5757285581c42457100c990c.

          Show
          jira-bot ASF subversion and git services added a comment - Commit 7415d100ef09a9a10b45746ebb794452e288ca3a in lucene-solr's branch refs/heads/branch_5x from anshum [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=7415d10 ] Revert " SOLR-8677 : Fix assert statement" This reverts commit e8acc04c68ac74ca5757285581c42457100c990c.
          Hide
          anshumg Anshum Gupta added a comment -

          Sorry about the delay, but here's the patch that reverts this change in 5x. I'll commit it later tonight so it would be good to have someone else take a look as well.
          ant test passes with this patch.

          Show
          anshumg Anshum Gupta added a comment - Sorry about the delay, but here's the patch that reverts this change in 5x. I'll commit it later tonight so it would be good to have someone else take a look as well. ant test passes with this patch.
          Hide
          erickerickson Erick Erickson added a comment -

          Anshum Gupta I ran across this, can we close it out now?

          Show
          erickerickson Erick Erickson added a comment - Anshum Gupta I ran across this, can we close it out now?
          Hide
          anshumg Anshum Gupta added a comment -

          This shouldn't have gone to 5x. My bad! I'll revert this so that this isn't released on the 5x line.

          Show
          anshumg Anshum Gupta added a comment - This shouldn't have gone to 5x. My bad! I'll revert this so that this isn't released on the 5x line.
          Hide
          anshumg Anshum Gupta added a comment -

          Done

          Show
          anshumg Anshum Gupta added a comment - Done
          Hide
          erickerickson Erick Erickson added a comment -

          Anshum Gupta Can this be closed out?

          Show
          erickerickson Erick Erickson added a comment - Anshum Gupta Can this be closed out?
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit e8acc04c68ac74ca5757285581c42457100c990c in lucene-solr's branch refs/heads/branch_5x from anshum
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=e8acc04 ]

          SOLR-8677: Fix assert statement

          Show
          jira-bot ASF subversion and git services added a comment - Commit e8acc04c68ac74ca5757285581c42457100c990c in lucene-solr's branch refs/heads/branch_5x from anshum [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=e8acc04 ] SOLR-8677 : Fix assert statement
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 9b9a64a9d6a6b334c93ae18dbc8da534f4198b22 in lucene-solr's branch refs/heads/branch_5x from anshum
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=9b9a64a ]

          SOLR-8677: Fix broken build

          Show
          jira-bot ASF subversion and git services added a comment - Commit 9b9a64a9d6a6b334c93ae18dbc8da534f4198b22 in lucene-solr's branch refs/heads/branch_5x from anshum [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=9b9a64a ] SOLR-8677 : Fix broken build
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 55162f2255a1f707603cc81134996cfb2a5968ec in lucene-solr's branch refs/heads/branch_5x from anshum
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=55162f2 ]

          SOLR-8677: Fix broken build

          Show
          jira-bot ASF subversion and git services added a comment - Commit 55162f2255a1f707603cc81134996cfb2a5968ec in lucene-solr's branch refs/heads/branch_5x from anshum [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=55162f2 ] SOLR-8677 : Fix broken build
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 96c01a2c885871f7d80beddc6e019547639ef71e in lucene-solr's branch refs/heads/branch_5x from anshum
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=96c01a2 ]

          SOLR-8677: Restrict creation of shards with invalid names

          Show
          jira-bot ASF subversion and git services added a comment - Commit 96c01a2c885871f7d80beddc6e019547639ef71e in lucene-solr's branch refs/heads/branch_5x from anshum [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=96c01a2 ] SOLR-8677 : Restrict creation of shards with invalid names
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit eb0e270043f7e83c06683043a4fb642b4f04b485 in lucene-solr's branch refs/heads/master from anshum
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=eb0e270 ]

          SOLR-8677: Fix assert statement

          Show
          jira-bot ASF subversion and git services added a comment - Commit eb0e270043f7e83c06683043a4fb642b4f04b485 in lucene-solr's branch refs/heads/master from anshum [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=eb0e270 ] SOLR-8677 : Fix assert statement
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit c7c5b8fe498408fb28911272986b119fc3ab563f in lucene-solr's branch refs/heads/master from anshum
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=c7c5b8f ]

          SOLR-8677: Fix broken build

          Show
          jira-bot ASF subversion and git services added a comment - Commit c7c5b8fe498408fb28911272986b119fc3ab563f in lucene-solr's branch refs/heads/master from anshum [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=c7c5b8f ] SOLR-8677 : Fix broken build
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit a54e819a6272830098cb50ec1abd75f2501d4993 in lucene-solr's branch refs/heads/master from anshum
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=a54e819 ]

          SOLR-8677: Fix broken build

          Show
          jira-bot ASF subversion and git services added a comment - Commit a54e819a6272830098cb50ec1abd75f2501d4993 in lucene-solr's branch refs/heads/master from anshum [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=a54e819 ] SOLR-8677 : Fix broken build
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit d01230d6394b29fa6fd42377404c0c03d6e8a4d9 in lucene-solr's branch refs/heads/master from anshum
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=d01230d ]

          SOLR-8677: Restrict creation of shards with invalid names

          Show
          jira-bot ASF subversion and git services added a comment - Commit d01230d6394b29fa6fd42377404c0c03d6e8a4d9 in lucene-solr's branch refs/heads/master from anshum [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=d01230d ] SOLR-8677 : Restrict creation of shards with invalid names
          Hide
          anshumg Anshum Gupta added a comment -

          There were a few issues with the last patch. Here's an updated patch.
          I'll commit once the entire test suite passes. Thanks everyone.

          Show
          anshumg Anshum Gupta added a comment - There were a few issues with the last patch. Here's an updated patch. I'll commit once the entire test suite passes. Thanks everyone.
          Hide
          gerlowskija Jason Gerlowski added a comment -

          Good changes. I'd thought about having SolrIdentifierValidator spit out booleans initially, but decided against it back in SOLR-8642, as it'd force callers in Solr to produce their own SolrExceptions by wrapping the call. Pros and cons either way.

          But now that it gets rid of the duplication I like it better this/your way.

          Thanks for taking the time to review Anshum.

          Show
          gerlowskija Jason Gerlowski added a comment - Good changes. I'd thought about having SolrIdentifierValidator spit out booleans initially, but decided against it back in SOLR-8642 , as it'd force callers in Solr to produce their own SolrExceptions by wrapping the call. Pros and cons either way. But now that it gets rid of the duplication I like it better this/your way. Thanks for taking the time to review Anshum.
          Hide
          anshumg Anshum Gupta added a comment -

          This obviously means some duplicated code, since the classes are largely the same other than the exception being thrown. If anyone has suggestions on better ways to structure this, I'd love to hear them.

          handled in my last patch.

          I created a new file for these tests (TestCollectionAdminRequest). Wanted to mention it, in case tests for those classes do already exist, and I just missed them. Pretty sure the new file was warranted though.

          You've added nice unit tests that don't require SolrCloud bootstrapping, so that's perfect. We can also add more tests to CollectionsAPISolrJTests class but it's not really needed so we're good.

          Show
          anshumg Anshum Gupta added a comment - This obviously means some duplicated code, since the classes are largely the same other than the exception being thrown. If anyone has suggestions on better ways to structure this, I'd love to hear them. handled in my last patch. I created a new file for these tests (TestCollectionAdminRequest). Wanted to mention it, in case tests for those classes do already exist, and I just missed them. Pretty sure the new file was warranted though. You've added nice unit tests that don't require SolrCloud bootstrapping, so that's perfect. We can also add more tests to CollectionsAPISolrJTests class but it's not really needed so we're good.
          Hide
          anshumg Anshum Gupta added a comment -

          Thanks Jason. Here's an updated patch.

          • Refactored the validator to a single class in SolrJ instead and the validation methods now return boolean instead. This allows for throwing the appropriate exception from the calling code.
          • Removed the new Exception to just throw IllegalArgumentException directly.

          I didn't have any strong feelings about the new Exception but I didn't find it adding any value so I let go of it. I'm still running the tests but this should be fine I think.

          Show
          anshumg Anshum Gupta added a comment - Thanks Jason. Here's an updated patch. Refactored the validator to a single class in SolrJ instead and the validation methods now return boolean instead. This allows for throwing the appropriate exception from the calling code. Removed the new Exception to just throw IllegalArgumentException directly. I didn't have any strong feelings about the new Exception but I didn't find it adding any value so I let go of it. I'm still running the tests but this should be fine I think.
          Hide
          gerlowskija Jason Gerlowski added a comment -

          Updated the patch with the changes to SolrJ that Shai suggested. A few notes:

          • SolrJ now throws an "InvalidIdentifierException" (extends IllegalArgumentException) when an invalid shard/core/collection/alias name is provided. I wasn't sure whether it's acceptable to add a new thrown exception type, or if SolrJ had conventions around this. Might be onerous to ask callers to add an additional catch block into their code. But grepping through SolrJ's code, I saw a variety of exceptions being thrown, so I took that as a sign that I was "in the clear".
          • The existing identifier-validation logic (on the server side) threw a SolrException directly. This makes sense on the server side, but less so in SolrJ. So I created a client-side validation class to throw a more appropriate exception (InvalidIdentifierException, as mentioned above). This obviously means some duplicated code, since the classes are largely the same other than the exception being thrown. If anyone has suggestions on better ways to structure this, I'd love to hear them.
          • I added tests for the additional SolrJ checks. I had a hard time finding any tests for the Collection-API related types in SolrJ (CollectionAdminRequest). So I created a new file for these tests (TestCollectionAdminRequest). Wanted to mention it, in case tests for those classes do already exist, and I just missed them. Pretty sure the new file was warranted though.

          Hope this covers what you were looking for Shai!

          Show
          gerlowskija Jason Gerlowski added a comment - Updated the patch with the changes to SolrJ that Shai suggested. A few notes: SolrJ now throws an "InvalidIdentifierException" (extends IllegalArgumentException) when an invalid shard/core/collection/alias name is provided. I wasn't sure whether it's acceptable to add a new thrown exception type, or if SolrJ had conventions around this. Might be onerous to ask callers to add an additional catch block into their code. But grepping through SolrJ's code, I saw a variety of exceptions being thrown, so I took that as a sign that I was "in the clear". The existing identifier-validation logic (on the server side) threw a SolrException directly. This makes sense on the server side, but less so in SolrJ. So I created a client-side validation class to throw a more appropriate exception (InvalidIdentifierException, as mentioned above). This obviously means some duplicated code, since the classes are largely the same other than the exception being thrown. If anyone has suggestions on better ways to structure this, I'd love to hear them. I added tests for the additional SolrJ checks. I had a hard time finding any tests for the Collection-API related types in SolrJ (CollectionAdminRequest). So I created a new file for these tests (TestCollectionAdminRequest). Wanted to mention it, in case tests for those classes do already exist, and I just missed them. Pretty sure the new file was warranted though. Hope this covers what you were looking for Shai!
          Hide
          gerlowskija Jason Gerlowski added a comment -

          I'll just roll it into this patch. Just wanted to make sure I was doing the right thing process-wise before I start in on it. If it doesn't bother you, it doesn't bother me.

          I'll push up a new revision shortly.

          Show
          gerlowskija Jason Gerlowski added a comment - I'll just roll it into this patch. Just wanted to make sure I was doing the right thing process-wise before I start in on it. If it doesn't bother you, it doesn't bother me. I'll push up a new revision shortly.
          Hide
          shaie Shai Erera added a comment -

          I don't mind either way Jason. Either you add it to this patch, and with that finish the collection/alias/shard name restrictions handling, or to a new issue, whatever works for you. I assume both will be released only post 5.5 anyway, and thus together. If you prefer to handle that separately, let me know and I'll add a CHANGES entry before committing this patch.

          Show
          shaie Shai Erera added a comment - I don't mind either way Jason. Either you add it to this patch, and with that finish the collection/alias/shard name restrictions handling, or to a new issue, whatever works for you. I assume both will be released only post 5.5 anyway, and thus together. If you prefer to handle that separately, let me know and I'll add a CHANGES entry before committing this patch.
          Hide
          gerlowskija Jason Gerlowski added a comment -

          Thanks for the review Shai. Do you want to see that SolrJ change as a part of this JIRA? I'd thought about adding that myself, but I had planned on it being its own JIRA, since as you mentioned it's not strictly related. But if there's no objection to adding it in this patch, I'll just save the overhead and add it here.

          Show
          gerlowskija Jason Gerlowski added a comment - Thanks for the review Shai. Do you want to see that SolrJ change as a part of this JIRA? I'd thought about adding that myself, but I had planned on it being its own JIRA, since as you mentioned it's not strictly related. But if there's no objection to adding it in this patch, I'll just save the overhead and add it here.
          Hide
          shaie Shai Erera added a comment -

          Also, would u mind adding a CHANGES.txt entry too?

          Show
          shaie Shai Erera added a comment - Also, would u mind adding a CHANGES.txt entry too?
          Hide
          shaie Shai Erera added a comment -

          Looks good to me. Jason Gerlowski I know it's not strictly related to this issue, but perhaps we can use the validator in SolrJ too, short-circuiting alias/collection/shard requests before they reach the server?

          Show
          shaie Shai Erera added a comment - Looks good to me. Jason Gerlowski I know it's not strictly related to this issue, but perhaps we can use the validator in SolrJ too, short-circuiting alias/collection/shard requests before they reach the server?
          Hide
          gerlowskija Jason Gerlowski added a comment -

          Attached patch prevents creation of shards with "invalid" names via the CREATESHARD and CREATE APIs. (There might be other places where shards can be created that I'm not aware of, though I'm reasonably certain I got them all).

          All tests pass locally for me.

          Show
          gerlowskija Jason Gerlowski added a comment - Attached patch prevents creation of shards with "invalid" names via the CREATESHARD and CREATE APIs. (There might be other places where shards can be created that I'm not aware of, though I'm reasonably certain I got them all). All tests pass locally for me.
          Hide
          gerlowskija Jason Gerlowski added a comment -

          This check would need to be put in at least two places:

          • the shards parameter of the CREATE collection call.
          • the shard parameter of the CREATESHARD call.

          There may be other places where users can specify the name of a new shard. These are just the two places I found after a quick look.

          Hoping to upload a patch shortly that can address this behavior.

          Show
          gerlowskija Jason Gerlowski added a comment - This check would need to be put in at least two places: the shards parameter of the CREATE collection call. the shard parameter of the CREATESHARD call. There may be other places where users can specify the name of a new shard. These are just the two places I found after a quick look. Hoping to upload a patch shortly that can address this behavior.

            People

            • Assignee:
              anshumg Anshum Gupta
              Reporter:
              gerlowskija Jason Gerlowski
            • Votes:
              1 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development