Merge pull request #11620 from heww/fix-issue-11524

feat(scanner): make Clair and Trivy as reserved name for scanners

make/migrations/postgresql/0030_2.0.0_schema.up.sql

@@ -200,3 +200,7 @@ ALTER TABLE replication_task ALTER COLUMN dst_resource TYPE varchar(512);
 /*remove count from quota hard and quota_usage used json*/
 UPDATE quota SET hard = hard - 'count';
 UPDATE quota_usage SET used = used - 'count';
+
+/* make Clair and Trivy as reserved name for scanners in-tree */
+UPDATE scanner_registration SET name = concat_ws('-', name, uuid) WHERE name IN ('Clair', 'Trivy') AND immutable = FALSE;
+UPDATE scanner_registration SET name = split_part(name, '-', 1) WHERE immutable = TRUE;

src/controller/scanner/base_controller.go

@@ -17,6 +17,7 @@ package scanner
 import (
 	"github.com/goharbor/harbor/src/core/promgr/metamgr"
 	"github.com/goharbor/harbor/src/jobservice/logger"
+	lerrors "github.com/goharbor/harbor/src/lib/errors"
 	"github.com/goharbor/harbor/src/lib/log"
 	"github.com/goharbor/harbor/src/lib/q"
 	"github.com/goharbor/harbor/src/pkg/scan/dao/scanner"
@@ -65,6 +66,10 @@ func (bc *basicController) ListRegistrations(query *q.Query) ([]*scanner.Registr
 
 // CreateRegistration ...
 func (bc *basicController) CreateRegistration(registration *scanner.Registration) (string, error) {
+	if isReservedName(registration.Name) {
+		return "", lerrors.BadRequestError(nil).WithMessage(`name "%s" is reserved, please try a different name`, registration.Name)
+	}
+
 	// Check if the registration is available
 	if _, err := bc.Ping(registration); err != nil {
 		return "", errors.Wrap(err, "api controller: create registration")
@@ -115,6 +120,10 @@ func (bc *basicController) UpdateRegistration(registration *scanner.Registration
 		return errors.Errorf("default registration %s can not be marked to disabled", registration.UUID)
 	}
 
+	if isReservedName(registration.Name) {
+		return lerrors.BadRequestError(nil).WithMessage(`name "%s" is reserved, please try a different name`, registration.Name)
+	}
+
 	return bc.manager.Update(registration)
 }
 
@@ -316,3 +325,17 @@ func (bc *basicController) GetMetadata(registrationUUID string) (*v1.ScannerAdap
 
 	return bc.Ping(r)
 }
+
+var (
+	reservedNames = []string{"Clair", "Trivy"}
+)
+
+func isReservedName(name string) bool {
+	for _, reservedName := range reservedNames {
+		if name == reservedName {
+			return true
+		}
+	}
+
+	return false
+}

src/core/api/scanners.go

@@ -19,9 +19,9 @@ import (
 	"net/http"
 
 	s "github.com/goharbor/harbor/src/controller/scanner"
+	"github.com/goharbor/harbor/src/lib/errors"
 	"github.com/goharbor/harbor/src/lib/q"
 	"github.com/goharbor/harbor/src/pkg/scan/dao/scanner"
-	"github.com/pkg/errors"
 )
 
 // ScannerAPI provides the API for managing the plugin scanners
@@ -140,7 +140,7 @@ func (sa *ScannerAPI) Create() {
 
 	uuid, err := sa.c.CreateRegistration(r)
 	if err != nil {
-		sa.SendInternalServerError(errors.Wrap(err, "scanner API: create"))
+		sa.SendError(errors.Wrap(err, "scanner API: create"))
 		return
 	}
 

src/core/main.go

@@ -176,14 +176,19 @@ func main() {
 	beego.RunWithMiddleWares("", middlewares.MiddleWares()...)
 }
 
+const (
+	clairScanner = "Clair"
+	trivyScanner = "Trivy"
+)
+
 func registerScanners() {
 	wantedScanners := make([]scanner.Registration, 0)
-	uninstallURLs := make([]string, 0)
+	uninstallScannerNames := make([]string, 0)
 
 	if config.WithTrivy() {
 		log.Info("Registering Trivy scanner")
 		wantedScanners = append(wantedScanners, scanner.Registration{
-			Name:            "Trivy",
+			Name:            trivyScanner,
 			Description:     "The Trivy scanner adapter",
 			URL:             config.TrivyAdapterURL(),
 			UseInternalAddr: true,
@@ -191,7 +196,7 @@ func registerScanners() {
 		})
 	} else {
 		log.Info("Removing Trivy scanner")
-		uninstallURLs = append(uninstallURLs, config.TrivyAdapterURL())
+		uninstallScannerNames = append(uninstallScannerNames, trivyScanner)
 	}
 
 	if config.WithClair() {
@@ -205,7 +210,7 @@ func registerScanners() {
 
 		log.Info("Registering Clair scanner")
 		wantedScanners = append(wantedScanners, scanner.Registration{
-			Name:            "Clair",
+			Name:            clairScanner,
 			Description:     "The Clair scanner adapter",
 			URL:             config.ClairAdapterEndpoint(),
 			UseInternalAddr: true,
@@ -213,32 +218,32 @@ func registerScanners() {
 		})
 	} else {
 		log.Info("Removing Clair scanner")
-		uninstallURLs = append(uninstallURLs, config.ClairAdapterEndpoint())
+		uninstallScannerNames = append(uninstallScannerNames, clairScanner)
 	}
 
 	if err := scan.EnsureScanners(wantedScanners); err != nil {
 		log.Fatalf("failed to register scanners: %v", err)
 	}
 
-	if defaultScannerURL := getDefaultScannerURL(); defaultScannerURL != "" {
+	if defaultScannerName := getDefaultScannerName(); defaultScannerName != "" {
-		log.Infof("Setting %s as default scanner", defaultScannerURL)
+		log.Infof("Setting %s as default scanner", defaultScannerName)
-		if err := scan.EnsureDefaultScanner(defaultScannerURL); err != nil {
+		if err := scan.EnsureDefaultScanner(defaultScannerName); err != nil {
 			log.Fatalf("failed to set default scanner: %v", err)
 		}
 	}
 
-	if err := scan.RemoveImmutableScanners(uninstallURLs); err != nil {
+	if err := scan.RemoveImmutableScanners(uninstallScannerNames); err != nil {
 		log.Warningf("failed to remove scanners: %v", err)
 	}
 
 }
 
-func getDefaultScannerURL() string {
+func getDefaultScannerName() string {
 	if config.WithTrivy() {
-		return config.TrivyAdapterURL()
+		return trivyScanner
 	}
 	if config.WithClair() {
-		return config.ClairAdapterEndpoint()
+		return clairScanner
 	}
 	return ""
 }

src/pkg/scan/init.go

@@ -19,8 +19,6 @@ import (
 	"github.com/goharbor/harbor/src/lib/q"
 	"github.com/goharbor/harbor/src/pkg/scan/dao/scanner"
 	sc "github.com/goharbor/harbor/src/pkg/scan/scanner"
-	"github.com/goharbor/harbor/src/pkg/types"
-	"github.com/google/uuid"
 	"github.com/pkg/errors"
 )
 
@@ -33,100 +31,73 @@ func EnsureScanners(wantedScanners []scanner.Registration) (err error) {
 	if len(wantedScanners) == 0 {
 		return
 	}
-	endpointURLs := make([]string, len(wantedScanners))
+	names := make([]string, len(wantedScanners))
 	for i, ws := range wantedScanners {
-		endpointURLs[i] = ws.URL
+		names[i] = ws.Name
 	}
 
-	list, err := scannerManager.List(&q.Query{
+	list, err := scannerManager.List(q.New(q.KeyWords{"ex_name__in": names}))
-		Keywords: map[string]interface{}{
-			"ex_url__in": endpointURLs,
-		},
-	})
 	if err != nil {
 		return errors.Errorf("listing scanners: %v", err)
 	}
 	existingScanners := make(map[string]*scanner.Registration)
 	for _, li := range list {
-		existingScanners[li.URL] = li
+		existingScanners[li.Name] = li
 	}
 
 	for _, ws := range wantedScanners {
-		if _, exists := existingScanners[ws.URL]; exists {
+		scanner, exists := existingScanners[ws.Name]
+		if !exists {
+			if _, err := scannerManager.Create(&ws); err != nil {
+				return errors.Errorf("creating registration %s at %s failed: %v", ws.Name, ws.URL, err)
+			}
+			log.Infof("Successfully registered %s scanner at %s", ws.Name, ws.URL)
+		} else if scanner.URL != ws.URL {
+			scanner.URL = ws.URL
+			if err := scannerManager.Update(scanner); err != nil {
+				return errors.Errorf("updating registration %s to %s failed: %v", ws.Name, ws.URL, err)
+			}
+			log.Infof("Successfully updated %s scanner to %s", ws.Name, ws.URL)
+		} else {
 			log.Infof("Scanner registration already exists: %s", ws.URL)
-			continue
 		}
-		err = createRegistration(&ws, true)
-		if err != nil {
-			return errors.Errorf("creating registration: %s: %v", ws.URL, err)
-		}
-		log.Infof("Successfully registered %s scanner at %s", ws.Name, ws.URL)
 	}
 
 	return
 }
 
 // EnsureDefaultScanner ensures that the scanner with the specified URL is set as default in the system.
-func EnsureDefaultScanner(scannerURL string) (err error) {
+func EnsureDefaultScanner(scannerName string) (err error) {
 	defaultScanner, err := scannerManager.GetDefault()
 	if err != nil {
 		err = errors.Errorf("getting default scanner: %v", err)
 		return
 	}
 	if defaultScanner != nil {
-		log.Infof("Skipped setting %s as the default scanner. The default scanner is already set to %s", scannerURL, defaultScanner.URL)
+		log.Infof("Skipped setting %s as the default scanner. The default scanner is already set to %s", scannerName, defaultScanner.URL)
 		return
 	}
-	scanners, err := scannerManager.List(&q.Query{
+	scanners, err := scannerManager.List(q.New(q.KeyWords{"ex_name": scannerName}))
-		Keywords: map[string]interface{}{"url": scannerURL},
-	})
 	if err != nil {
 		err = errors.Errorf("listing scanners: %v", err)
 		return
 	}
 	if len(scanners) != 1 {
-		return errors.Errorf("expected only one scanner with URL %v but got %d", scannerURL, len(scanners))
+		return errors.Errorf("expected only one scanner with name %v but got %d", scannerName, len(scanners))
 	}
 	err = scannerManager.SetAsDefault(scanners[0].UUID)
 	if err != nil {
-		err = errors.Errorf("setting %s as default scanner: %v", scannerURL, err)
+		err = errors.Errorf("setting %s as default scanner: %v", scannerName, err)
-	}
-	return
-}
-
-func createRegistration(registration *scanner.Registration, resolveConflict bool) (err error) {
-	for {
-		_, err = scannerManager.Create(registration)
-		if err != nil {
-			if resolveConflict && errors.Cause(err) == types.ErrDupRows {
-				var id uuid.UUID
-				id, err = uuid.NewUUID()
-				if err != nil {
-					break
-				}
-
-				registration.Name = registration.Name + "-" + id.String()
-				resolveConflict = false
-				continue
-			}
-		}
-
-		break
 	}
 	return
 }
 
 // RemoveImmutableScanners removes immutable scanner Registrations with the specified endpoint URLs.
-func RemoveImmutableScanners(urls []string) error {
+func RemoveImmutableScanners(names []string) error {
-	if len(urls) == 0 {
+	if len(names) == 0 {
 		return nil
 	}
-	query := &q.Query{
+	query := q.New(q.KeyWords{"ex_immutable": true, "ex_name__in": names})
-		Keywords: map[string]interface{}{
-			"immutable":  true,
-			"ex_url__in": urls,
-		},
-	}
 
 	// TODO Instead of executing 1 to N SQL queries we might want to delete multiple rows with scannerManager.DeleteByImmutableAndURLIn(true, []string{})
 	registrations, err := scannerManager.List(query)

src/pkg/scan/init_test.go

@@ -15,12 +15,13 @@
 package scan
 
 import (
+	"testing"
+
 	"github.com/goharbor/harbor/src/lib/q"
 	"github.com/goharbor/harbor/src/pkg/scan/dao/scanner"
 	"github.com/goharbor/harbor/src/pkg/scan/scanner/mocks"
 	"github.com/pkg/errors"
 	"github.com/stretchr/testify/assert"
-	"testing"
 )
 
 func TestEnsureScanners(t *testing.T) {
@@ -36,12 +37,12 @@ func TestEnsureScanners(t *testing.T) {
 
 		mgr.On("List", &q.Query{
 			Keywords: map[string]interface{}{
-				"ex_url__in": []string{"http://scanner:8080"},
+				"ex_name__in": []string{"scanner"},
 			},
 		}).Return(nil, errors.New("DB error"))
 
 		err := EnsureScanners([]scanner.Registration{
-			{URL: "http://scanner:8080"},
+			{Name: "scanner", URL: "http://scanner:8080"},
 		})
 
 		assert.EqualError(t, err, "listing scanners: DB error")
@@ -54,21 +55,55 @@ func TestEnsureScanners(t *testing.T) {
 
 		mgr.On("List", &q.Query{
 			Keywords: map[string]interface{}{
-				"ex_url__in": []string{
+				"ex_name__in": []string{
-					"http://trivy:8080",
+					"trivy",
-					"http://clair:8080",
+					"clair",
 				},
 			},
 		}).Return([]*scanner.Registration{
-			{URL: "http://clair:8080"},
+			{Name: "clair", URL: "http://clair:8080"},
 		}, nil)
 		mgr.On("Create", &scanner.Registration{
-			URL: "http://trivy:8080",
+			Name: "trivy",
+			URL:  "http://trivy:8080",
 		}).Return("uuid-trivy", nil)
 
 		err := EnsureScanners([]scanner.Registration{
-			{URL: "http://trivy:8080"},
+			{Name: "trivy", URL: "http://trivy:8080"},
-			{URL: "http://clair:8080"},
+			{Name: "clair", URL: "http://clair:8080"},
+		})
+
+		assert.NoError(t, err)
+		mgr.AssertExpectations(t)
+	})
+
+	t.Run("Should update scanners", func(t *testing.T) {
+		mgr := &mocks.Manager{}
+		scannerManager = mgr
+
+		mgr.On("List", &q.Query{
+			Keywords: map[string]interface{}{
+				"ex_name__in": []string{
+					"trivy",
+					"clair",
+				},
+			},
+		}).Return([]*scanner.Registration{
+			{Name: "trivy", URL: "http://trivy:8080"},
+			{Name: "clair", URL: "http://clair:8080"},
+		}, nil)
+		mgr.On("Update", &scanner.Registration{
+			Name: "trivy",
+			URL:  "http://trivy:8443",
+		}).Return(nil)
+		mgr.On("Update", &scanner.Registration{
+			Name: "clair",
+			URL:  "http://clair:8443",
+		}).Return(nil)
+
+		err := EnsureScanners([]scanner.Registration{
+			{Name: "trivy", URL: "http://trivy:8443"},
+			{Name: "clair", URL: "http://clair:8443"},
 		})
 
 		assert.NoError(t, err)
@@ -85,7 +120,7 @@ func TestEnsureDefaultScanner(t *testing.T) {
 
 		mgr.On("GetDefault").Return(nil, errors.New("DB error"))
 
-		err := EnsureDefaultScanner("http://trivy:8080")
+		err := EnsureDefaultScanner("trivy")
 		assert.EqualError(t, err, "getting default scanner: DB error")
 		mgr.AssertExpectations(t)
 	})
@@ -95,10 +130,10 @@ func TestEnsureDefaultScanner(t *testing.T) {
 		scannerManager = mgr
 
 		mgr.On("GetDefault").Return(&scanner.Registration{
-			URL: "http://clair:8080",
+			Name: "clair",
 		}, nil)
 
-		err := EnsureDefaultScanner("http://trivy:8080")
+		err := EnsureDefaultScanner("trivy")
 		assert.NoError(t, err)
 		mgr.AssertExpectations(t)
 	})
@@ -109,10 +144,10 @@ func TestEnsureDefaultScanner(t *testing.T) {
 
 		mgr.On("GetDefault").Return(nil, nil)
 		mgr.On("List", &q.Query{
-			Keywords: map[string]interface{}{"url": "http://trivy:8080"},
+			Keywords: map[string]interface{}{"ex_name": "trivy"},
 		}).Return(nil, errors.New("DB error"))
 
-		err := EnsureDefaultScanner("http://trivy:8080")
+		err := EnsureDefaultScanner("trivy")
 		assert.EqualError(t, err, "listing scanners: DB error")
 		mgr.AssertExpectations(t)
 	})
@@ -123,14 +158,14 @@ func TestEnsureDefaultScanner(t *testing.T) {
 
 		mgr.On("GetDefault").Return(nil, nil)
 		mgr.On("List", &q.Query{
-			Keywords: map[string]interface{}{"url": "http://trivy:8080"},
+			Keywords: map[string]interface{}{"ex_name": "trivy"},
 		}).Return([]*scanner.Registration{
-			{URL: "http://trivy:8080"},
+			{Name: "trivy"},
-			{URL: "http://trivy:8080"},
+			{Name: "trivy"},
 		}, nil)
 
-		err := EnsureDefaultScanner("http://trivy:8080")
+		err := EnsureDefaultScanner("trivy")
-		assert.EqualError(t, err, "expected only one scanner with URL http://trivy:8080 but got 2")
+		assert.EqualError(t, err, "expected only one scanner with name trivy but got 2")
 		mgr.AssertExpectations(t)
 	})
 
@@ -140,16 +175,17 @@ func TestEnsureDefaultScanner(t *testing.T) {
 
 		mgr.On("GetDefault").Return(nil, nil)
 		mgr.On("List", &q.Query{
-			Keywords: map[string]interface{}{"url": "http://trivy:8080"},
+			Keywords: map[string]interface{}{"ex_name": "trivy"},
 		}).Return([]*scanner.Registration{
 			{
+				Name: "trivy",
 				UUID: "trivy-uuid",
 				URL:  "http://trivy:8080",
 			},
 		}, nil)
 		mgr.On("SetAsDefault", "trivy-uuid").Return(nil)
 
-		err := EnsureDefaultScanner("http://trivy:8080")
+		err := EnsureDefaultScanner("trivy")
 		assert.NoError(t, err)
 		mgr.AssertExpectations(t)
 	})
@@ -160,17 +196,18 @@ func TestEnsureDefaultScanner(t *testing.T) {
 
 		mgr.On("GetDefault").Return(nil, nil)
 		mgr.On("List", &q.Query{
-			Keywords: map[string]interface{}{"url": "http://trivy:8080"},
+			Keywords: map[string]interface{}{"ex_name": "trivy"},
 		}).Return([]*scanner.Registration{
 			{
+				Name: "trivy",
 				UUID: "trivy-uuid",
 				URL:  "http://trivy:8080",
 			},
 		}, nil)
 		mgr.On("SetAsDefault", "trivy-uuid").Return(errors.New("DB error"))
 
-		err := EnsureDefaultScanner("http://trivy:8080")
+		err := EnsureDefaultScanner("trivy")
-		assert.EqualError(t, err, "setting http://trivy:8080 as default scanner: DB error")
+		assert.EqualError(t, err, "setting trivy as default scanner: DB error")
 		mgr.AssertExpectations(t)
 	})
 
@@ -178,7 +215,7 @@ func TestEnsureDefaultScanner(t *testing.T) {
 
 func TestRemoveImmutableScanners(t *testing.T) {
 
-	t.Run("Should do nothing when list of URLs is empty", func(t *testing.T) {
+	t.Run("Should do nothing when list of names is empty", func(t *testing.T) {
 		mgr := &mocks.Manager{}
 		scannerManager = mgr
 
@@ -193,12 +230,12 @@ func TestRemoveImmutableScanners(t *testing.T) {
 
 		mgr.On("List", &q.Query{
 			Keywords: map[string]interface{}{
-				"immutable":  true,
+				"ex_immutable": true,
-				"ex_url__in": []string{"http://scanner:8080"},
+				"ex_name__in":  []string{"scanner"},
 			},
 		}).Return(nil, errors.New("DB error"))
 
-		err := RemoveImmutableScanners([]string{"http://scanner:8080"})
+		err := RemoveImmutableScanners([]string{"scanner"})
 		assert.EqualError(t, err, "listing scanners: DB error")
 		mgr.AssertExpectations(t)
 	})
@@ -209,20 +246,22 @@ func TestRemoveImmutableScanners(t *testing.T) {
 
 		registrations := []*scanner.Registration{
 			{
+				Name: "scanner-1",
 				UUID: "uuid-1",
 				URL:  "http://scanner-1",
 			},
 			{
+				Name: "scanner-2",
 				UUID: "uuid-2",
 				URL:  "http://scanner-2",
 			}}
 
 		mgr.On("List", &q.Query{
 			Keywords: map[string]interface{}{
-				"immutable": true,
+				"ex_immutable": true,
-				"ex_url__in": []string{
+				"ex_name__in": []string{
-					"http://scanner-1",
+					"scanner-1",
-					"http://scanner-2",
+					"scanner-2",
 				},
 			},
 		}).Return(registrations, nil)
@@ -230,8 +269,8 @@ func TestRemoveImmutableScanners(t *testing.T) {
 		mgr.On("Delete", "uuid-2").Return(nil)
 
 		err := RemoveImmutableScanners([]string{
-			"http://scanner-1",
+			"scanner-1",
-			"http://scanner-2",
+			"scanner-2",
 		})
 		assert.NoError(t, err)
 		mgr.AssertExpectations(t)
@@ -243,20 +282,22 @@ func TestRemoveImmutableScanners(t *testing.T) {
 
 		registrations := []*scanner.Registration{
 			{
+				Name: "scanner-1",
 				UUID: "uuid-1",
 				URL:  "http://scanner-1",
 			},
 			{
+				Name: "scanner-2",
 				UUID: "uuid-2",
 				URL:  "http://scanner-2",
 			}}
 
 		mgr.On("List", &q.Query{
 			Keywords: map[string]interface{}{
-				"immutable": true,
+				"ex_immutable": true,
-				"ex_url__in": []string{
+				"ex_name__in": []string{
-					"http://scanner-1",
+					"scanner-1",
-					"http://scanner-2",
+					"scanner-2",
 				},
 			},
 		}).Return(registrations, nil)
@@ -264,8 +305,8 @@ func TestRemoveImmutableScanners(t *testing.T) {
 		mgr.On("Delete", "uuid-2").Return(errors.New("DB error"))
 
 		err := RemoveImmutableScanners([]string{
-			"http://scanner-1",
+			"scanner-1",
-			"http://scanner-2",
+			"scanner-2",
 		})
 		assert.EqualError(t, err, "deleting scanner: uuid-2: DB error")
 		mgr.AssertExpectations(t)