Refact cwhub: item removal with shared dependencies (#2598)

* Iterate over sub-items in Remove(), not in disable() -- fix shared dependency issue
* Increase hub download timeout to 2 minutes

pkg/cwhub/cwhub.go

@@ -10,5 +10,5 @@ import (
 )
 
 var hubClient = &http.Client{
-	Timeout: 20 * time.Second,
+	Timeout: 120 * time.Second,
 }

pkg/cwhub/enable.go

@@ -146,35 +146,10 @@ func (i *Item) removeInstallLink() error {
 // disable removes the symlink to the downloaded content, also removes the content if purge is true
 func (i *Item) disable(purge bool, force bool) error {
 	// XXX: should return the number of disabled/purged items to inform the upper layer whether to reload or not
-	if i.IsLocal() {
-		return fmt.Errorf("%s isn't managed by hub. Please delete manually", i.Name)
-	}
-
-	if i.Tainted && !force {
-		return fmt.Errorf("%s is tainted, use '--force' to overwrite", i.Name)
-	}
-
-	for _, sub := range i.SubItems() {
-		// TODO XXX: if the other collection(s) are direct or indirect dependencies of the current one, it's good to go
-		if len(sub.BelongsToCollections) > 1 {
-			log.Infof("%s was not removed because it belongs to another collection", sub.Name)
-			continue
-		}
-
-		if err := sub.disable(purge, force); err != nil {
-			return fmt.Errorf("while disabling %s: %w", sub.Name, err)
-		}
-	}
-
-	if !i.Installed && !purge {
-		log.Infof("removing %s: not installed -- no need to remove", i.Name)
-		return nil
-	}
-
 	err := i.removeInstallLink()
 	if os.IsNotExist(err) {
 		if !purge && !force {
-			return fmt.Errorf("can't disable %s: %s doesn't exist", i.Name, i.installLink())
+			return fmt.Errorf("link %s does not exist (override with --force or --purge)", i.installLink())
 		}
 	} else if err != nil {
 		return err

pkg/cwhub/helpers.go

@@ -17,6 +17,7 @@ import (
 
 	"github.com/enescakir/emoji"
 	log "github.com/sirupsen/logrus"
+	"slices"
 )
 
 // Install installs the item from the hub, downloading it if needed
@@ -51,12 +52,70 @@ func (i *Item) Install(force bool, downloadOnly bool) error {
 	return nil
 }
 
+// allDependencies return a list of all dependencies and sub-dependencies of the item
+func (i *Item) allDependencies() []*Item {
+	var deps []*Item
+
+	for _, dep := range i.SubItems() {
+		if dep == i {
+			log.Errorf("circular dependency detected: %s depends on %s", dep.Name, i.Name)
+			continue
+		}
+
+		deps = append(deps, dep.allDependencies()...)
+	}
+
+	return append(deps, i)
+}
+
 // Remove disables the item, optionally removing the downloaded content
-func (i *Item) Remove(purge bool, forceAction bool) (bool, error) {
+func (i *Item) Remove(purge bool, force bool) (bool, error) {
+	if i.IsLocal() {
+		return false, fmt.Errorf("%s isn't managed by hub. Please delete manually", i.Name)
+	}
+
+	if i.Tainted && !force {
+		return false, fmt.Errorf("%s is tainted, use '--force' to remove", i.Name)
+	}
+
+	if !i.Installed && !purge {
+		log.Infof("removing %s: not installed -- no need to remove", i.Name)
+		return false, nil
+	}
+
 	removed := false
 
-	if err := i.disable(purge, forceAction); err != nil {
-		return false, fmt.Errorf("unable to disable %s: %w", i.Name, err)
+	allDeps := i.allDependencies()
+
+	for _, sub := range i.SubItems() {
+		if !sub.Installed {
+			continue
+		}
+
+		// if the other collection(s) are direct or indirect dependencies of the current one, it's good to go
+		// log parent collections
+		for _, subParent := range sub.parentCollections() {
+			if subParent == i {
+				continue
+			}
+
+			if !slices.Contains(allDeps, subParent) {
+				log.Infof("%s was not removed because it also belongs to %s", sub.Name, subParent.Name)
+				continue
+			}
+		}
+
+		subRemoved, err := sub.Remove(purge, force)
+		if err != nil {
+			return false, fmt.Errorf("unable to disable %s: %w", i.Name, err)
+		}
+
+		removed = removed || subRemoved
+	}
+
+	err := i.disable(purge, force)
+	if err != nil {
+		return false, fmt.Errorf("while removing %s: %w", i.Name, err)
 	}
 
 	// XXX: should take the value from disable()

pkg/cwhub/items.go

@@ -186,6 +186,21 @@ func (i *Item) logMissingSubItems() {
 	}
 }
 
+func (i *Item) parentCollections() []*Item {
+	ret := make([]*Item, 0)
+
+	for _, parentName := range i.BelongsToCollections {
+		parent := i.hub.GetItem(COLLECTIONS, parentName)
+		if parent == nil {
+			continue
+		}
+
+		ret = append(ret, parent)
+	}
+
+	return ret
+}
+
 // Status returns the status of the item as a string and an emoji
 // ie. "enabled,update-available" and emoji.Warning
 func (i *Item) Status() (string, emoji.Emoji) {

pkg/cwhub/sync.go

@@ -393,6 +393,8 @@ func (h *Hub) syncDir(dir string) ([]string, error) {
 
 	// For each, scan PARSERS, POSTOVERFLOWS, SCENARIOS and COLLECTIONS last
 	for _, scan := range ItemTypes {
+		// cpath: top-level item directory, either downloaded or installed items.
+		// i.e. /etc/crowdsec/parsers, /etc/crowdsec/hub/parsers, ...
 		cpath, err := filepath.Abs(fmt.Sprintf("%s/%s", dir, scan))
 		if err != nil {
 			log.Errorf("failed %s: %s", cpath, err)

test/bats/20_hub_collections_dep.bats

@@ -81,11 +81,34 @@ teardown() {
 
     # now we can't remove smb without --force
     rune -1 cscli collections remove crowdsecurity/smb
-    assert_stderr --partial "unable to disable crowdsecurity/smb: crowdsecurity/smb is tainted, use '--force' to overwrite"
+    assert_stderr --partial "crowdsecurity/smb is tainted, use '--force' to remove"
+}
 
+@test "cscli collections (dependencies II: the revenge)" {
     rune -0 cscli collections install crowdsecurity/wireguard baudneo/gotify
     rune -0 cscli collections remove crowdsecurity/wireguard
-    assert_stderr --partial "crowdsecurity/syslog-logs was not removed because it belongs to another collection"
+    assert_stderr --partial "crowdsecurity/syslog-logs was not removed because it also belongs to baudneo/gotify"
     rune -0 cscli collections inspect crowdsecurity/wireguard -o json
     rune -0 jq -e '.installed==false' <(output)
 }
+
+@test "cscli collections (dependencies III: origins)" {
+    # it is perfectly fine to remove an item belonging to a collection that we are removing anyway
+
+    # inject a dependency: sshd requires the syslog-logs parsers, but linux does too
+    hub_dep=$(jq <"$INDEX_PATH" '. * {collections:{"crowdsecurity/sshd":{parsers:["crowdsecurity/syslog-logs"]}}}')
+    echo "$hub_dep" >"$INDEX_PATH"
+
+    # verify that installing sshd brings syslog-logs
+    rune -0 cscli collections install crowdsecurity/sshd
+    rune -0 cscli parsers inspect crowdsecurity/syslog-logs -o json
+    rune -0 jq -e '.installed==true' <(output)
+
+    rune -0 cscli collections install crowdsecurity/linux
+
+    # removing linux should remove syslog-logs even though sshd depends on it
+    rune -0 cscli collections remove crowdsecurity/linux
+    refute_stderr --partial "crowdsecurity/syslog-logs was not removed"
+    rune -0 cscli parsers list -o json
+    rune -0 jq -e '.parsers | length == 0' <(output)
+}

test/bats/20_hub_items.bats

@@ -80,7 +80,7 @@ teardown() {
     rune -0 rm "$(output)"
 
     rune -0 cscli parsers remove crowdsecurity/syslog-logs --debug
-    assert_stderr --partial "Removed crowdsecurity/syslog-logs"
+    assert_stderr --partial "removing crowdsecurity/syslog-logs: not installed -- no need to remove"
 
     rune -0 cscli parsers inspect crowdsecurity/syslog-logs -o json
     rune -0 jq -r '.path' <(output)