diff --git a/.github/workflows/pr-linter-check.yml b/.github/workflows/pr-linter-check.yml index f2dc7c6e6c8..36badcaa14c 100644 --- a/.github/workflows/pr-linter-check.yml +++ b/.github/workflows/pr-linter-check.yml @@ -7,8 +7,16 @@ jobs: runs-on: ubuntu-latest steps: + - name: Set up Go + uses: actions/setup-go@v5 + with: + go-version: '1.22' + - name: Check out the code - uses: actions/checkout@v2 + uses: actions/checkout@v4 - - name: Linter check - run: make lint + - name: golangci-lint + uses: golangci/golangci-lint-action@v4 + with: + version: v1.57.2 + args: --out-format=colored-line-number diff --git a/golangci.yaml b/.golangci.yaml similarity index 98% rename from golangci.yaml rename to .golangci.yaml index c96c7428e06..3f7e0c11f5a 100644 --- a/golangci.yaml +++ b/.golangci.yaml @@ -12,14 +12,6 @@ run: # exit code when at least one issue was found, default is 1 issues-exit-code: 1 - # which dirs to skip: issues from them won't be reported; - # can use regexp here: generated.*, regexp is applied on full path; - # default value is empty list, but default dirs are skipped independently - # from this option's value (see skip-dirs-use-default). - # "/" will be replaced by current OS file path separator to properly work - # on Windows. - skip-dirs: - - pkg/plugin/generated/* # default is true. Enables skipping of directories: # vendor$, third_party$, testdata$, examples$, Godeps$, builtin$ @@ -44,7 +36,7 @@ run: # output configuration options output: # colored-line-number|line-number|json|tab|checkstyle|code-climate, default is "colored-line-number" - format: colored-line-number + formats: colored-line-number # print lines of code with issue, default is true print-issued-lines: true @@ -148,10 +140,8 @@ linters-settings: # minimal confidence for issues, default is 0.8 min-confidence: 0.8 gomnd: - settings: - mnd: - # the list of enabled checks, see https://github.com/tommy-muehle/go-mnd/#checks for description. - checks: argument,case,condition,operation,return,assign + # the list of enabled checks, see https://github.com/tommy-muehle/go-mnd/#checks for description. + checks: argument,case,condition,operation,return,assign gomodguard: allowed: modules: # List of allowed modules @@ -389,6 +379,15 @@ issues: # Show only new issues created after git revision `REV` # new-from-rev: origin/main + # which dirs to skip: issues from them won't be reported; + # can use regexp here: generated.*, regexp is applied on full path; + # default value is empty list, but default dirs are skipped independently + # from this option's value (see skip-dirs-use-default). + # "/" will be replaced by current OS file path separator to properly work + # on Windows. + exclude-dirs: + - pkg/plugin/generated/* + severity: # Default value is empty string. # Set the default severity for issues. If severity rules are defined and the issues diff --git a/changelogs/unreleased/7697-blackpiglet b/changelogs/unreleased/7697-blackpiglet index ba1b12a3677..a6c5bead0b5 100644 --- a/changelogs/unreleased/7697-blackpiglet +++ b/changelogs/unreleased/7697-blackpiglet @@ -1 +1 @@ -Modify namespace filter logic for backup with label selector \ No newline at end of file +When Included/ExcludedNamespaces are omitted, and LabelSelector or OrLabelSelector is used, namespaces without selected items are excluded from backup. \ No newline at end of file diff --git a/hack/build-image/Dockerfile b/hack/build-image/Dockerfile index 161a837f0b6..e2d966f73a3 100644 --- a/hack/build-image/Dockerfile +++ b/hack/build-image/Dockerfile @@ -93,7 +93,7 @@ RUN ARCH=$(go env GOARCH) && \ chmod +x /usr/bin/goreleaser # get golangci-lint -RUN curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v1.55.0 +RUN curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v1.57.2 # install kubectl RUN curl -LO https://storage.googleapis.com/kubernetes-release/release/$(curl -s https://storage.googleapis.com/kubernetes-release/release/stable.txt)/bin/linux/$(go env GOARCH)/kubectl diff --git a/hack/lint.sh b/hack/lint.sh index 13ed64caba7..737a6d6750f 100755 --- a/hack/lint.sh +++ b/hack/lint.sh @@ -14,14 +14,12 @@ # See the License for the specific language governing permissions and # limitations under the License. -HACK_DIR=$(dirname "${BASH_SOURCE[0]}") - # Printing out cache status golangci-lint cache status # Enable GL_DEBUG line below for debug messages for golangci-lint # export GL_DEBUG=loader,gocritic,env -CMD="golangci-lint run -c $HACK_DIR/../golangci.yaml" +CMD="golangci-lint run" echo "Running $CMD" eval $CMD diff --git a/internal/hook/item_hook_handler_test.go b/internal/hook/item_hook_handler_test.go index f60110a5de3..fccd511db6b 100644 --- a/internal/hook/item_hook_handler_test.go +++ b/internal/hook/item_hook_handler_test.go @@ -1976,7 +1976,7 @@ func TestValidateContainer(t *testing.T) { expectedError := fmt.Errorf("invalid InitContainer in restore hook, it doesn't have Command, Name or Image field") // valid string should return nil as result. - assert.Equal(t, nil, ValidateContainer([]byte(valid))) + assert.Nil(t, ValidateContainer([]byte(valid))) // noName string should return expected error as result. assert.Equal(t, expectedError, ValidateContainer([]byte(noName))) diff --git a/pkg/backup/backup_test.go b/pkg/backup/backup_test.go index 00ecdbd021d..59d00bd77fe 100644 --- a/pkg/backup/backup_test.go +++ b/pkg/backup/backup_test.go @@ -4334,8 +4334,7 @@ func TestBackupNamespaces(t *testing.T) { backup: defaultBackup().OrLabelSelector([]*metav1.LabelSelector{ {MatchLabels: map[string]string{"a": "b"}}, {MatchLabels: map[string]string{"c": "d"}}, - }). - Result(), + }).Result(), apiResources: []*test.APIResource{ test.Namespaces( builder.ForNamespace("ns-1").Result(), @@ -4379,6 +4378,27 @@ func TestBackupNamespaces(t *testing.T) { "resources/deployments.apps/v1-preferredversion/namespaces/ns-1/deploy-1.json", }, }, + { + name: "LabelSelector and Namespace exclude filtering test", + backup: defaultBackup().ExcludedNamespaces("ns-1", "ns-2").LabelSelector(&metav1.LabelSelector{MatchLabels: map[string]string{"a": "b"}}). + Result(), + apiResources: []*test.APIResource{ + test.Namespaces( + builder.ForNamespace("ns-1").ObjectMeta(builder.WithLabels("a", "b")).Result(), + builder.ForNamespace("ns-2").Result(), + builder.ForNamespace("ns-3").Result(), + ), + test.Deployments( + builder.ForDeployment("ns-1", "deploy-1").ObjectMeta(builder.WithLabels("a", "b")).Result(), + ), + }, + want: []string{ + "resources/namespaces/cluster/ns-1.json", + "resources/namespaces/v1-preferredversion/cluster/ns-1.json", + "resources/namespaces/cluster/ns-3.json", + "resources/namespaces/v1-preferredversion/cluster/ns-3.json", + }, + }, { name: "Empty namespace test", backup: defaultBackup().IncludedNamespaces("invalid*").Result(), diff --git a/pkg/backup/item_collector.go b/pkg/backup/item_collector.go index 594415048a1..f616171cc2e 100644 --- a/pkg/backup/item_collector.go +++ b/pkg/backup/item_collector.go @@ -55,9 +55,24 @@ type itemCollector struct { } // nsTracker is used to integrate several namespace filters together. -// 1. backup's namespace include and exclude filters; -// 2. backup's LabelSelector and OrLabelSelector selected namespace; -// 3. resources namespaces selected by label selectors. +// 1. Backup's namespace Include/Exclude filters; +// 2. Backup's (Or)LabelSelector selected namespace; +// 3. Backup's (Or)LabelSelector selected resources' namespaces. +// +// Rules: +// +// a. When backup has default namespace Include/Exclude filters, +// The namespaces, which do not have backup including resources, +// are not collected. +// +// b. If the namespace I/E filters and the (Or)LabelSelectors have +// different results. The tracker takes the union of them. +// +// c. If the namespace I/E filters and the (Or)LabelSelectors selected +// namespaces are different. The tracker takes the union of them. +// +// d. Namespace I/E filters should always have the same result with +// (Or)LabelSelector selected resources' namespaces. type nsTracker struct { singleLabelSelector labels.Selector orLabelSelector []labels.Selector @@ -67,6 +82,7 @@ type nsTracker struct { namespaceMap map[string]bool } +// track add the namespace into the namespaceMap. func (nt *nsTracker) track(ns string) { if nt.namespaceMap == nil { nt.namespaceMap = make(map[string]bool) @@ -77,6 +93,8 @@ func (nt *nsTracker) track(ns string) { } } +// isTracked check whether the namespace's name exists in +// namespaceMap. func (nt *nsTracker) isTracked(ns string) bool { if nt.namespaceMap != nil { return nt.namespaceMap[ns] @@ -84,6 +102,8 @@ func (nt *nsTracker) isTracked(ns string) bool { return false } +// init initialize the namespaceMap, and add elements according to +// namespace include/exclude filters and the backup label selectors. func (nt *nsTracker) init( unstructuredNSs []unstructured.Unstructured, singleLabelSelector labels.Selector, @@ -91,18 +111,21 @@ func (nt *nsTracker) init( namespaceFilter *collections.IncludesExcludes, logger logrus.FieldLogger, ) { - nt.namespaceMap = make(map[string]bool) + if nt.namespaceMap == nil { + nt.namespaceMap = make(map[string]bool) + } nt.singleLabelSelector = singleLabelSelector nt.orLabelSelector = orLabelSelector nt.namespaceFilter = namespaceFilter nt.logger = logger for _, namespace := range unstructuredNSs { - if nt.singleLabelSelector != nil && - nt.singleLabelSelector.Matches(labels.Set(namespace.GetLabels())) == true { - nt.logger.Debugf("Track namespace %s,", namespace.GetName(), - "because its labels match backup LabelSelector.") + nt.singleLabelSelector.Matches(labels.Set(namespace.GetLabels())) { + nt.logger.Debugf(`Track namespace %s, + because its labels match backup LabelSelector.`, + namespace.GetName(), + ) nt.track(namespace.GetName()) continue @@ -110,9 +133,11 @@ func (nt *nsTracker) init( if len(nt.orLabelSelector) > 0 { for _, selector := range nt.orLabelSelector { - if selector.Matches(labels.Set(namespace.GetLabels())) == true { - nt.logger.Debugf("Track namespace %s", namespace.GetName(), - "because its labels match the backup OrLabelSelector.") + if selector.Matches(labels.Set(namespace.GetLabels())) { + nt.logger.Debugf(`Track namespace %s", + "because its labels match the backup OrLabelSelector.`, + namespace.GetName(), + ) nt.track(namespace.GetName()) continue } @@ -120,25 +145,26 @@ func (nt *nsTracker) init( } // Skip the backup when the backup's namespace filter has - // default value(*), and the namespace doesn't match backup + // default value, and the namespace doesn't match backup // LabelSelector and OrLabelSelector. // https://github.com/vmware-tanzu/velero/issues/7105 - if nt.namespaceFilter.ShouldInclude("*") == true && + if nt.namespaceFilter.IncludeEverything() && (nt.singleLabelSelector != nil || len(nt.orLabelSelector) > 0) { - nt.logger.Debugf("Skip namespace %s,", namespace.GetName(), - "because backup's namespace filter is *, and backup has label selector.") continue } - if nt.namespaceFilter.ShouldInclude(namespace.GetName()) == true { - nt.logger.Debugf("Track namespace %s", namespace.GetName(), - "because its name match the backup namespace filter.") + if nt.namespaceFilter.ShouldInclude(namespace.GetName()) { + nt.logger.Debugf(`Track namespace %s, + because its name match the backup namespace filter.`, + namespace.GetName(), + ) nt.track(namespace.GetName()) } } } -// filterNamespaces only keeps namespace tracked by the nsTracker +// filterNamespaces filters the input resource list to remove the +// namespaces not tracked by the nsTracker. func (nt *nsTracker) filterNamespaces( resources []*kubernetesResource, ) []*kubernetesResource { @@ -146,7 +172,7 @@ func (nt *nsTracker) filterNamespaces( for _, resource := range resources { if resource.groupResource != kuberesource.Namespaces || - nt.isTracked(resource.name) == true { + nt.isTracked(resource.name) { result = append(result, resource) } } @@ -160,8 +186,8 @@ type kubernetesResource struct { namespace, name, path string } -// getItemsFromResourceIdentifiers converts ResourceIdentifiers to -// kubernetesResources +// getItemsFromResourceIdentifiers get the kubernetesResources +// specified by the input parameter resourceIDs. func (r *itemCollector) getItemsFromResourceIdentifiers( resourceIDs []velero.ResourceIdentifier, ) []*kubernetesResource { @@ -173,16 +199,20 @@ func (r *itemCollector) getItemsFromResourceIdentifiers( return r.getItems(grResourceIDsMap) } -// getAllItems gets all relevant items from all API groups. +// getAllItems gets all backup-relevant items from all API groups. func (r *itemCollector) getAllItems() []*kubernetesResource { resources := r.getItems(nil) return r.nsTracker.filterNamespaces(resources) } -// getItems gets all relevant items from all API groups. +// getItems gets all backup-relevant items from all API groups, +// // If resourceIDsMap is nil, then all items from the cluster are -// pulled for each API group, subject to include/exclude rules. +// pulled for each API group, subject to include/exclude rules, +// except the namespace, because the namespace filtering depends on +// all namespaced-scoped resources. +// // If resourceIDsMap is supplied, then only those resources are // returned, with the appropriate APIGroup information filled in. In // this case, include/exclude rules are not invoked, since we already @@ -409,10 +439,10 @@ func (r *itemCollector) getResourceItems( } // Handle namespace resource here. - // Namespace are only filtered by namespace include/exclude filters. - // Label selectors are not checked. + // Namespace are filtered by namespace include/exclude filters, + // backup LabelSelectors and OrLabelSelectors are checked too. if gr == kuberesource.Namespaces { - return r.backupNamespaces( + return r.collectNamespaces( resource, gv, gr, @@ -697,8 +727,8 @@ func (r *itemCollector) listItemsForLabel( return unstructuredItems, nil } -// backupNamespaces process namespace resource according to namespace filters. -func (r *itemCollector) backupNamespaces( +// collectNamespaces process namespace resource according to namespace filters. +func (r *itemCollector) collectNamespaces( resource metav1.APIResource, gv schema.GroupVersion, gr schema.GroupResource, @@ -750,17 +780,18 @@ func (r *itemCollector) backupNamespaces( var items []*kubernetesResource - for index, unstructured := range unstructuredList.Items { + for index := range unstructuredList.Items { path, err := r.writeToFile(&unstructuredList.Items[index]) if err != nil { - log.WithError(err).Error("Error writing item to file") + log.WithError(err).Errorf("Error writing item %s to file", + unstructuredList.Items[index].GetName()) continue } items = append(items, &kubernetesResource{ groupResource: gr, preferredGVR: preferredGVR, - name: unstructured.GetName(), + name: unstructuredList.Items[index].GetName(), path: path, }) } diff --git a/pkg/backup/item_collector_test.go b/pkg/backup/item_collector_test.go index dcb48dcc1cb..01c8dd21636 100644 --- a/pkg/backup/item_collector_test.go +++ b/pkg/backup/item_collector_test.go @@ -206,12 +206,23 @@ func TestItemCollectorBackupNamespaces(t *testing.T) { backup: builder.ForBackup("velero", "backup").OrLabelSelector([]*metav1.LabelSelector{ {MatchLabels: map[string]string{"name": "ns1"}}, }).Result(), - ie: collections.NewIncludesExcludes().Excludes("ns1"), + ie: collections.NewIncludesExcludes().Excludes("ns1", "ns2"), namespaces: []*corev1.Namespace{ builder.ForNamespace("ns1").ObjectMeta(builder.WithLabels("name", "ns1")).Result(), builder.ForNamespace("ns2").Result(), + builder.ForNamespace("ns3").Result(), }, - expectedTrackedNS: []string{"ns1"}, + expectedTrackedNS: []string{"ns1", "ns3"}, + }, + { + name: "No ns filters", + backup: builder.ForBackup("velero", "backup").Result(), + ie: collections.NewIncludesExcludes().Includes("*"), + namespaces: []*corev1.Namespace{ + builder.ForNamespace("ns1").ObjectMeta(builder.WithLabels("name", "ns1")).Result(), + builder.ForNamespace("ns2").Result(), + }, + expectedTrackedNS: []string{"ns1", "ns2"}, }, } @@ -248,7 +259,7 @@ func TestItemCollectorBackupNamespaces(t *testing.T) { dir: tempDir, } - r.backupNamespaces( + r.collectNamespaces( metav1.APIResource{ Name: "Namespace", Kind: "Namespace", diff --git a/pkg/client/client_test.go b/pkg/client/client_test.go index c09d90ba6fb..05c1e26e365 100644 --- a/pkg/client/client_test.go +++ b/pkg/client/client_test.go @@ -79,7 +79,7 @@ func TestConfig(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { client, err := Config(test.kubeconfig, test.kubecontext, "velero", test.QPS, test.burst) - assert.Equal(t, err, nil) + assert.Nil(t, err) assert.Equal(t, test.expectedHost, client.Host) assert.Equal(t, test.QPS, client.QPS) assert.Equal(t, test.burst, client.Burst) diff --git a/pkg/client/config_test.go b/pkg/client/config_test.go index b8e593d2165..126262fbda0 100644 --- a/pkg/client/config_test.go +++ b/pkg/client/config_test.go @@ -62,13 +62,13 @@ func TestConfigOperations(t *testing.T) { // Remove config file if it exists err := removeConfigfileName() - assert.Equal(t, err, nil) + assert.NoError(t, err) // Test LoadConfig: expect an empty velero config expectedConfig := VeleroConfig{} config, err := LoadConfig() - assert.Equal(t, err, nil) + assert.NoError(t, err) assert.True(t, reflect.DeepEqual(expectedConfig, config)) // Test savedConfig @@ -84,9 +84,9 @@ func TestConfigOperations(t *testing.T) { err = SaveConfig(config) - assert.Equal(t, err, nil) + assert.NoError(t, err) savedConfig, err := LoadConfig() - assert.Equal(t, err, nil) + assert.NoError(t, err) // Test Features feature := savedConfig.Features() @@ -107,7 +107,7 @@ func TestConfigOperations(t *testing.T) { t.Cleanup(func() { err = removeConfigfileName() - assert.Equal(t, err, nil) + assert.NoError(t, err) os.Unsetenv("HOME") os.Setenv("HOME", preHomeEnv) }) diff --git a/pkg/client/factory_test.go b/pkg/client/factory_test.go index 17569f1c850..3ab61c32306 100644 --- a/pkg/client/factory_test.go +++ b/pkg/client/factory_test.go @@ -95,7 +95,7 @@ func TestFactory(t *testing.T) { baseName := "velero-bn" config, err := LoadConfig() - assert.Equal(t, err, nil) + assert.NoError(t, err) for _, test := range tests { t.Run(test.name, func(t *testing.T) { f = NewFactory(baseName, config) diff --git a/pkg/cmd/cli/backup/delete_test.go b/pkg/cmd/cli/backup/delete_test.go index 0d34bc5a102..420523dd37c 100644 --- a/pkg/cmd/cli/backup/delete_test.go +++ b/pkg/cmd/cli/backup/delete_test.go @@ -62,15 +62,15 @@ func TestDeleteCommand(t *testing.T) { args := []string{backup1, backup2} e := o.Complete(f, args) - require.Equal(t, nil, e) + require.NoError(t, e) e = o.Validate(c, f, args) - require.Equal(t, nil, e) + require.NoError(t, e) Run(o) e = c.Execute() - require.Equal(t, nil, e) + require.NoError(t, e) if os.Getenv(cmdtest.CaptureFlag) == "1" { return diff --git a/pkg/cmd/cli/backuplocation/create_test.go b/pkg/cmd/cli/backuplocation/create_test.go index dc2cb1aae06..69c0bded46a 100644 --- a/pkg/cmd/cli/backuplocation/create_test.go +++ b/pkg/cmd/cli/backuplocation/create_test.go @@ -148,7 +148,7 @@ func TestCreateCommand_Run(t *testing.T) { o.Complete(args, f) e := o.Validate(c, args, f) - assert.Equal(t, e, nil) + assert.Nil(t, e) e = o.Run(c, f) assert.Contains(t, e.Error(), fmt.Sprintf("%s: no such file or directory", caCertFile)) diff --git a/pkg/cmd/cli/backuplocation/set_test.go b/pkg/cmd/cli/backuplocation/set_test.go index cac77d12925..481a9dd01cf 100644 --- a/pkg/cmd/cli/backuplocation/set_test.go +++ b/pkg/cmd/cli/backuplocation/set_test.go @@ -66,7 +66,7 @@ func TestNewSetCommand(t *testing.T) { args := []string{backupName} o.Complete(args, f) e := o.Validate(c, args, f) - assert.Equal(t, e, nil) + assert.Nil(t, e) e = o.Run(c, f) assert.Contains(t, e.Error(), fmt.Sprintf("%s: no such file or directory", cacert)) diff --git a/pkg/cmd/cli/restore/delete_test.go b/pkg/cmd/cli/restore/delete_test.go index b504e2c005c..187aad39d00 100644 --- a/pkg/cmd/cli/restore/delete_test.go +++ b/pkg/cmd/cli/restore/delete_test.go @@ -62,15 +62,15 @@ func TestDeleteCommand(t *testing.T) { args := []string{restore1, restore2} e := o.Complete(f, args) - require.Equal(t, nil, e) + require.NoError(t, e) e = o.Validate(c, f, args) - require.Equal(t, nil, e) + require.NoError(t, e) Run(o) e = c.Execute() - require.Equal(t, nil, e) + require.NoError(t, e) if os.Getenv(cmdtest.CaptureFlag) == "1" { return diff --git a/pkg/datapath/file_system_test.go b/pkg/datapath/file_system_test.go index e39f73fdcd6..8e496f1c04d 100644 --- a/pkg/datapath/file_system_test.go +++ b/pkg/datapath/file_system_test.go @@ -101,7 +101,7 @@ func TestAsyncBackup(t *testing.T) { fs.callbacks = test.callbacks err := fs.StartBackup(AccessPoint{ByPath: test.path}, "", "", false, nil, map[string]string{}) - require.Equal(t, nil, err) + require.NoError(t, err) <-finish @@ -184,7 +184,7 @@ func TestAsyncRestore(t *testing.T) { fs.callbacks = test.callbacks err := fs.StartRestore(test.snapshot, AccessPoint{ByPath: test.path}, map[string]string{}) - require.Equal(t, nil, err) + require.NoError(t, err) <-finish diff --git a/pkg/datapath/manager_test.go b/pkg/datapath/manager_test.go index ebe18d1a031..fda574400e1 100644 --- a/pkg/datapath/manager_test.go +++ b/pkg/datapath/manager_test.go @@ -36,7 +36,7 @@ func TestManager(t *testing.T) { assert.Equal(t, ConcurrentLimitExceed, err) ret := m.GetAsyncBR("job-0") - assert.Equal(t, nil, ret) + assert.Nil(t, ret) ret = m.GetAsyncBR("job-1") assert.Equal(t, async_job_1, ret) @@ -48,5 +48,5 @@ func TestManager(t *testing.T) { assert.Len(t, m.tracker, 1) ret = m.GetAsyncBR("job-1") - assert.Equal(t, nil, ret) + assert.Nil(t, ret) }