Solr
  1. Solr
  2. SOLR-8299

ConfigSet DELETE should not allow deletion of a a configset that's currently being used

    Details

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

      Description

      The ConfigSet DELETE API currently doesn't check if the configuration directory being deleted is being used by an active Collection. We should add a check for the same.

      1. SOLR-8299.patch
        4 kB
        Anshum Gupta
      2. SOLR-8299.patch
        2 kB
        Anshum Gupta

        Activity

        Hide
        Anshum Gupta added a comment -

        Here's a patch without the test. I've allowed deletion of configset when force=true, but I really believe we shouldn't allow that.

        I'll add a test and if someone has a suggestion about respecting 'force' in this case, kindly chime in.

        Show
        Anshum Gupta added a comment - Here's a patch without the test. I've allowed deletion of configset when force=true, but I really believe we shouldn't allow that. I'll add a test and if someone has a suggestion about respecting 'force' in this case, kindly chime in.
        Hide
        Shai Erera added a comment -

        "Can not delete ConfigSet as it is being used by another collection"

        I would add the collection name to the message to make it more helpful?

        I've allowed deletion of configset when force=true, but I really believe we shouldn't allow that.

        I tend to agree with you on that. The problem is, from what I've seen, that the collection becomes unusable after you delete its configuration, even if it's later restored. Perhaps it was a point-in-time behavior of the code and has been fixed since then, but I prefer that we don't allow forcing the deletion of a configuration.

        Why would a user want to force-delete a configuration, when there are collections that use it? What good comes out of it? Perhaps we should add this 'force' support after we see an actual use case?

        Show
        Shai Erera added a comment - "Can not delete ConfigSet as it is being used by another collection" I would add the collection name to the message to make it more helpful? I've allowed deletion of configset when force=true, but I really believe we shouldn't allow that. I tend to agree with you on that. The problem is, from what I've seen, that the collection becomes unusable after you delete its configuration, even if it's later restored. Perhaps it was a point-in-time behavior of the code and has been fixed since then, but I prefer that we don't allow forcing the deletion of a configuration. Why would a user want to force-delete a configuration, when there are collections that use it? What good comes out of it? Perhaps we should add this 'force' support after we see an actual use case?
        Hide
        Anshum Gupta added a comment -

        Thanks Shai.

        I would add the collection name to the message to make it more helpful?

        I thought about that too, but just wanted to be sure that I'm not overlooking any multi-tenant security requirement.

        Why would a user want to force-delete a configuration, when there are collections that use it? What good comes out of it? Perhaps we should add this 'force' support after we see an actual use case?

        I completely agree with that but again, wanted to be sure that I was not missing a use-case here. Also, the collection does stay in an unusable state once you delete the config that's in-use and a ton of errors get logged too.

        But I'm now more convinced to make both of those changes before I commit.

        Show
        Anshum Gupta added a comment - Thanks Shai. I would add the collection name to the message to make it more helpful? I thought about that too, but just wanted to be sure that I'm not overlooking any multi-tenant security requirement. Why would a user want to force-delete a configuration, when there are collections that use it? What good comes out of it? Perhaps we should add this 'force' support after we see an actual use case? I completely agree with that but again, wanted to be sure that I was not missing a use-case here. Also, the collection does stay in an unusable state once you delete the config that's in-use and a ton of errors get logged too. But I'm now more convinced to make both of those changes before I commit.
        Hide
        Shai Erera added a comment -

        I thought about that too, but just wanted to be sure that I'm not overlooking any multi-tenant security requirement.

        Yes, that's a good point, but at the moment I think that whoever will hit this message, will have no idea what collection uses the config. Also, if we want to address multi-tenant and security stuff, I think that the app should be able to handle that. Maybe if we return a proper error code, or throw a specific exception. Since Solr does not support multi-tenancy natively, I don't think that we should try to protect it.

        Show
        Shai Erera added a comment - I thought about that too, but just wanted to be sure that I'm not overlooking any multi-tenant security requirement. Yes, that's a good point, but at the moment I think that whoever will hit this message, will have no idea what collection uses the config. Also, if we want to address multi-tenant and security stuff, I think that the app should be able to handle that. Maybe if we return a proper error code, or throw a specific exception. Since Solr does not support multi-tenancy natively, I don't think that we should try to protect it.
        Hide
        Anshum Gupta added a comment -

        Updated patch with a test.

        If a collection is in use, the call would now fail, irrespective of the force param.

        Show
        Anshum Gupta added a comment - Updated patch with a test. If a collection is in use, the call would now fail, irrespective of the force param.
        Hide
        Jason Gerlowski added a comment - - edited

        It's got my +1, fwiw.

        Though out of curiosity, are there any sort of docs that should be updated for this. (Still getting used to where Solr keeps tabs on various things. So that's not a leading question; I actually don't know.)

        Show
        Jason Gerlowski added a comment - - edited It's got my +1, fwiw. Though out of curiosity, are there any sort of docs that should be updated for this. (Still getting used to where Solr keeps tabs on various things. So that's not a leading question; I actually don't know.)
        Hide
        ASF subversion and git services added a comment -

        Commit 1716223 from Anshum Gupta in branch 'dev/trunk'
        [ https://svn.apache.org/r1716223 ]

        SOLR-8299: ConfigSet DELETE operation no longer allows deletion of config sets that are currently in use by other collections

        Show
        ASF subversion and git services added a comment - Commit 1716223 from Anshum Gupta in branch 'dev/trunk' [ https://svn.apache.org/r1716223 ] SOLR-8299 : ConfigSet DELETE operation no longer allows deletion of config sets that are currently in use by other collections
        Hide
        ASF subversion and git services added a comment -

        Commit 1716230 from Anshum Gupta in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1716230 ]

        SOLR-8299: ConfigSet DELETE operation no longer allows deletion of config sets that are currently in use by other collections (merge from trunk)

        Show
        ASF subversion and git services added a comment - Commit 1716230 from Anshum Gupta in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1716230 ] SOLR-8299 : ConfigSet DELETE operation no longer allows deletion of config sets that are currently in use by other collections (merge from trunk)
        Hide
        Anshum Gupta added a comment -

        Jason Gerlowski All the user-specific information should be in the reference guide. Other than that, it's just javadocs.
        In this particular case, this should go into the ref guide.

        Show
        Anshum Gupta added a comment - Jason Gerlowski All the user-specific information should be in the reference guide. Other than that, it's just javadocs. In this particular case, this should go into the ref guide.
        Hide
        Anshum Gupta added a comment -

        Fixing fix version and marking this as resolved.

        Show
        Anshum Gupta added a comment - Fixing fix version and marking this as resolved.

          People

          • Assignee:
            Anshum Gupta
            Reporter:
            Anshum Gupta
          • Votes:
            1 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development