From cf6494e6d0e9b3eec83ea8d7f7039492f000d925 Mon Sep 17 00:00:00 2001 From: Tom Pantelis Date: Tue, 7 Jan 2025 11:10:15 -0500 Subject: [PATCH 1/8] Adjust golangci-lint linters Added new linters (some commented out if not needed) and removed deprecated linters. Also addressed violations reported by new linters (in separate commits). Related to submariner-io/enhancements#231 Signed-off-by: Tom Pantelis --- .golangci.yml | 42 +++++++++++++++++++++++++++++++++++------- 1 file changed, 35 insertions(+), 7 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 4c77d2fd..85511fb4 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -39,29 +39,40 @@ linters-settings: linters: disable-all: true enable: + - asasalint - asciicheck - bidichk - bodyclose + # - canonicalheader # This is a slow linter and we don't use the net/http.Header API + - containedctx - contextcheck - copyloopvar # - cyclop # This is equivalent to gocyclo + - decorder # - depguard # depguard now denies by default, it should only be enabled if we actually use it - dogsled - dupl + - dupword - durationcheck - errcheck + - errchkjson - errorlint - errname + # - execinquery # No SQ - exhaustive - # - exhaustivestruct # Not recommended for general use - meant to be used only for special cases + # - exhaustruct # This is too cumbersome as it requires all string, int, pointer et al fields to be initialized even when the + # type's default suffices, which is most of the time + - fatcontext # - forbidigo # We don't forbid any statements # - forcetypeassert # There are many unchecked type assertions that would be the result of a programming error so the # reasonable recourse would be to panic anyway if checked so this doesn't seem useful # - funlen # gocyclo is enabled which is generally a better metric than simply LOC. - gci - ginkgolinter + - gocheckcompilerdirectives # - gochecknoglobals # We don't want to forbid global variable constants # - gochecknoinits # We use init functions for valid reasons + # - gochecksumtype # The usefulness is very narrow - gocognit - goconst - gocritic @@ -73,24 +84,29 @@ linters: - gofumpt - goheader - goimports - # - golint # Deprecated since v1.41.0 - # - gomnd # It doesn't seem useful in general to enforce constants for all numeric values # - gomoddirectives # We don't want to forbid the 'replace' directive # - gomodguard # We don't block any modules # - goprintffuncname # This doesn't seem useful at all + # - gosmopolitan # This is related to internationalization which is not a concern for us - gosec - gosimple - govet - # - ifshort # This is a style preference and doesn't seem compelling + - grouper + - iface - importas + - inamedparam - ineffassign + # - interfacebloat # We track complexity elsewhere + - intrange # - ireturn # The argument to always "Return Concrete Types" doesn't seem compelling. It is perfectly valid to return # an interface to avoid exposing the entire underlying struct - # - interfacer # Deprecated since v1.38.0 - lll + - loggercheck + - maintidx - makezero - # - maligned # Deprecated since v1.38.0 + - mirror - misspell + # - mnd # It doesn't seem useful in general to enforce constants for all numeric values - nakedret # - nestif # This calculates cognitive complexity but we're doing that elsewhere - nilerr @@ -98,18 +114,28 @@ linters: # - nlreturn # This is reasonable with a block-size of 2 but setting it above isn't honored # - noctx # We don't send HTTP requests - nolintlint + - nonamedreturns + # - nosprintfhostport # The use of this is very narrow # - paralleltest # Not relevant for Ginkgo UTs + - perfsprint - prealloc - predeclared - promlinter + - protogetter + - reassign + - recvcheck - revive # - rowserrcheck # We don't use SQL - # - scopelint # Deprecated since v1.39.0 + # - sloglint # We don't use log/slog + # - spancheck # We don't use OpenTelemetry/OpenCensus # - sqlclosecheck # We don't use SQL - staticcheck - stylecheck + - tagalign # - tagliatelle # Inconsistent with stylecheck and not as good # - tenv # Not relevant for our Ginkgo UTs + # - testableexamples # We don't need this + # - testifylint # We don't use testify - testpackage # - thelper # Not relevant for our Ginkgo UTs # - tparallel # Not relevant for our Ginkgo UTs @@ -117,11 +143,13 @@ linters: - unconvert - unparam - unused + - usestdlibvars # - varnamelen # It doesn't seem necessary to enforce a minimum variable name length - wastedassign - whitespace - wrapcheck - wsl + # - zerologlint # We use zerolog indirectly so this isn't needed issues: exclude-rules: # Allow dot-imports for Gomega BDD directives per idiomatic Gomega From dbf3384396119f19af6bd29dc891da0b753fb9a1 Mon Sep 17 00:00:00 2001 From: Tom Pantelis Date: Tue, 7 Jan 2025 08:44:08 -0500 Subject: [PATCH 2/8] Address dupword linter violations Fix "Duplicate words (on) found". Signed-off-by: Tom Pantelis --- test/e2e/discovery/clusterset_ip_enabled.go | 2 +- test/e2e/discovery/service_discovery.go | 10 +++++----- test/e2e/internal/service_discovery.go | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/test/e2e/discovery/clusterset_ip_enabled.go b/test/e2e/discovery/clusterset_ip_enabled.go index bd966864..277762e3 100644 --- a/test/e2e/discovery/clusterset_ip_enabled.go +++ b/test/e2e/discovery/clusterset_ip_enabled.go @@ -45,7 +45,7 @@ func RunClusterSetIPTest(f *lhframework.Framework) { clusterAName := framework.TestContext.ClusterIDs[framework.ClusterA] clusterBName := framework.TestContext.ClusterIDs[framework.ClusterB] - framework.By(fmt.Sprintf("Creating an Nginx Deployment on on %q", clusterBName)) + framework.By(fmt.Sprintf("Creating an Nginx Deployment on %q", clusterBName)) f.NewNginxDeployment(framework.ClusterB) framework.By(fmt.Sprintf("Creating a Nginx Service on %q", clusterBName)) diff --git a/test/e2e/discovery/service_discovery.go b/test/e2e/discovery/service_discovery.go index b09e62b9..24b6698a 100644 --- a/test/e2e/discovery/service_discovery.go +++ b/test/e2e/discovery/service_discovery.go @@ -126,7 +126,7 @@ func RunServiceDiscoveryTest(f *lhframework.Framework) { clusterAName := framework.TestContext.ClusterIDs[framework.ClusterA] clusterBName := framework.TestContext.ClusterIDs[framework.ClusterB] - framework.By(fmt.Sprintf("Creating an Nginx Deployment on on %q", clusterBName)) + framework.By(fmt.Sprintf("Creating an Nginx Deployment on %q", clusterBName)) f.NewNginxDeployment(framework.ClusterB) framework.By(fmt.Sprintf("Creating a Nginx Service on %q", clusterBName)) @@ -250,7 +250,7 @@ func RunServiceExportTest(f *lhframework.Framework) { framework.By(fmt.Sprintf("Creating an Nginx ServiceExport on %q", clusterBName)) f.NewServiceExport(framework.ClusterB, "nginx-demo", f.Namespace) - framework.By(fmt.Sprintf("Creating an Nginx Deployment on on %q", clusterBName)) + framework.By(fmt.Sprintf("Creating an Nginx Deployment on %q", clusterBName)) f.NewNginxDeployment(framework.ClusterB) framework.By(fmt.Sprintf("Creating a Nginx Service on %q", clusterBName)) @@ -282,7 +282,7 @@ func RunServicesPodAvailabilityTest(f *lhframework.Framework) { clusterAName := framework.TestContext.ClusterIDs[framework.ClusterA] clusterBName := framework.TestContext.ClusterIDs[framework.ClusterB] - framework.By(fmt.Sprintf("Creating an Nginx Deployment on on %q", clusterBName)) + framework.By(fmt.Sprintf("Creating an Nginx Deployment on %q", clusterBName)) f.NewNginxDeployment(framework.ClusterB) framework.By(fmt.Sprintf("Creating a Nginx Service on %q", clusterBName)) @@ -518,7 +518,7 @@ func RunServicesClusterAvailabilityMultiClusterTest(f *lhframework.Framework) { clusterBName := framework.TestContext.ClusterIDs[framework.ClusterB] clusterCName := framework.TestContext.ClusterIDs[framework.ClusterC] - framework.By(fmt.Sprintf("Creating an Nginx Deployment on on %q", clusterBName)) + framework.By(fmt.Sprintf("Creating an Nginx Deployment on %q", clusterBName)) f.NewNginxDeployment(framework.ClusterB) framework.By(fmt.Sprintf("Creating a Nginx Service on %q", clusterBName)) @@ -540,7 +540,7 @@ func RunServicesClusterAvailabilityMultiClusterTest(f *lhframework.Framework) { f.AwaitEndpointSlices(framework.ClusterB, nginxServiceClusterB.Name, nginxServiceClusterB.Namespace, 1, 1) f.AwaitEndpointSlices(framework.ClusterA, nginxServiceClusterB.Name, nginxServiceClusterB.Namespace, 1, 1) - framework.By(fmt.Sprintf("Creating an Nginx Deployment on on %q", clusterCName)) + framework.By(fmt.Sprintf("Creating an Nginx Deployment on %q", clusterCName)) f.NewNginxDeployment(framework.ClusterC) framework.By(fmt.Sprintf("Creating a Nginx Service on %q", clusterCName)) diff --git a/test/e2e/internal/service_discovery.go b/test/e2e/internal/service_discovery.go index ca0ddf04..64ccf1c1 100644 --- a/test/e2e/internal/service_discovery.go +++ b/test/e2e/internal/service_discovery.go @@ -59,7 +59,7 @@ func RunNonexistentNamespaceDiscoveryTest(f *lhframework.Framework) { Expect(framework.KubeClients[framework.ClusterA].CoreV1().Namespaces().Delete(context.TODO(), f.Namespace, metav1.DeleteOptions{})).To(Succeed()) - framework.By(fmt.Sprintf("Creating an Nginx Deployment on on %q", clusterBName)) + framework.By(fmt.Sprintf("Creating an Nginx Deployment on %q", clusterBName)) f.NewNginxDeployment(framework.ClusterB) framework.By(fmt.Sprintf("Creating a Nginx Service on %q", clusterBName)) From 0e9fc0292429351faabc717931b2bb86122d8100 Mon Sep 17 00:00:00 2001 From: Tom Pantelis Date: Tue, 7 Jan 2025 08:49:25 -0500 Subject: [PATCH 3/8] Address errchkjson linter errchkjson Fix "Error return value of `encoding/json.MarshalIndent` is not checked" Signed-off-by: Tom Pantelis --- pkg/agent/controller/agent.go | 5 ++--- pkg/agent/controller/service_endpoint_slices.go | 7 +++---- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/pkg/agent/controller/agent.go b/pkg/agent/controller/agent.go index ab276be1..18be60e3 100644 --- a/pkg/agent/controller/agent.go +++ b/pkg/agent/controller/agent.go @@ -20,7 +20,6 @@ package controller import ( "context" - "encoding/json" "fmt" "reflect" "strconv" @@ -29,6 +28,7 @@ import ( "github.com/submariner-io/admiral/pkg/federate" "github.com/submariner-io/admiral/pkg/ipam" "github.com/submariner-io/admiral/pkg/log" + "github.com/submariner-io/admiral/pkg/resource" "github.com/submariner-io/admiral/pkg/syncer" "github.com/submariner-io/admiral/pkg/syncer/broker" "github.com/submariner-io/lighthouse/pkg/constants" @@ -496,6 +496,5 @@ type serviceImportStringer struct { } func (s serviceImportStringer) String() string { - spec, _ := json.MarshalIndent(&s.Spec, "", " ") - return "spec: " + string(spec) + return "spec: " + resource.ToJSON(&s.Spec) } diff --git a/pkg/agent/controller/service_endpoint_slices.go b/pkg/agent/controller/service_endpoint_slices.go index e7e977f3..4700b8a8 100644 --- a/pkg/agent/controller/service_endpoint_slices.go +++ b/pkg/agent/controller/service_endpoint_slices.go @@ -20,7 +20,6 @@ package controller import ( "context" - "encoding/json" "fmt" "strconv" "strings" @@ -380,9 +379,9 @@ type endpointSliceStringer struct { } func (s endpointSliceStringer) String() string { - labels, _ := json.MarshalIndent(&s.Labels, "", " ") - ports, _ := json.MarshalIndent(&s.Ports, "", " ") - endpoints, _ := json.MarshalIndent(&s.Endpoints, "", " ") + labels := resource.ToJSON(&s.Labels) + ports := resource.ToJSON(&s.Ports) + endpoints := resource.ToJSON(&s.Endpoints) return fmt.Sprintf("\nlabels: %s\naddressType: %s\nendpoints: %s\nports: %s", labels, s.AddressType, endpoints, ports) } From 072420b2eb22f4b81289ca882b6c728889ebb78c Mon Sep 17 00:00:00 2001 From: Tom Pantelis Date: Tue, 7 Jan 2025 09:43:51 -0500 Subject: [PATCH 4/8] Address intrange linter violations Fix "for loop can be changed to use an integer range" Signed-off-by: Tom Pantelis --- .../smooth_weighted_round_robin_test.go | 10 +++++----- coredns/resolver/clusterip_service_test.go | 18 +++++++++--------- coredns/resolver/resolver_suite_test.go | 4 ++-- coredns/resolver/service_info.go | 2 +- pkg/agent/controller/controller_suite_test.go | 2 +- test/e2e/discovery/service_discovery.go | 2 +- test/e2e/framework/framework.go | 2 +- 7 files changed, 20 insertions(+), 20 deletions(-) diff --git a/coredns/loadbalancer/smooth_weighted_round_robin_test.go b/coredns/loadbalancer/smooth_weighted_round_robin_test.go index 61107cd0..1ef54ccc 100644 --- a/coredns/loadbalancer/smooth_weighted_round_robin_test.go +++ b/coredns/loadbalancer/smooth_weighted_round_robin_test.go @@ -59,7 +59,7 @@ var _ = Describe("Smooth Weighted RR", func() { // Validations validateServerAdded := func(s server) { - for i := 0; i < 100; i++ { + for range 100 { if lb.Next() == s.name { return } @@ -75,7 +75,7 @@ var _ = Describe("Smooth Weighted RR", func() { validateEmptyLBState := func() { Expect(lb.ItemCount()).To(Equal(0)) - for i := 0; i < 100; i++ { + for range 100 { Expect(lb.Next()).To(BeNil()) } } @@ -83,7 +83,7 @@ var _ = Describe("Smooth Weighted RR", func() { validateLoadBalancingByCount := func(rounds int, servers []server) { totalServersWeight := getTotalServesWeight(servers) results := make(map[string]int64) - for i := 0; i < rounds; i++ { + for range rounds { s := lb.Next().(string) results[s]++ } @@ -170,7 +170,7 @@ var _ = Describe("Smooth Weighted RR", func() { It("Next() should return it all the time", func() { s := servers[0] addServer(s) - for i := 0; i < 10; i++ { + for range 10 { Expect(lb.Next()).To(Equal(s.name)) } }) @@ -192,7 +192,7 @@ var _ = Describe("Smooth Weighted RR", func() { err := lb.Add(s.name, s.weight) Expect(err).ToNot(HaveOccurred()) } - for i := 0; i < 100; i++ { + for range 100 { for _, s := range roundRobinServers { Expect(lb.Next().(string)).To(Equal(s.name)) } diff --git a/coredns/resolver/clusterip_service_test.go b/coredns/resolver/clusterip_service_test.go index b45fce7c..e489eb56 100644 --- a/coredns/resolver/clusterip_service_test.go +++ b/coredns/resolver/clusterip_service_test.go @@ -56,7 +56,7 @@ func testClusterIPServiceInOneCluster() { Context("and no specific cluster is requested", func() { It("should consistently return its DNS record", func() { - for i := 0; i < 5; i++ { + for range 5 { t.assertDNSRecordsFound(namespace1, service1, "", "", false, expDNSRecord) } }) @@ -64,7 +64,7 @@ func testClusterIPServiceInOneCluster() { Context("and the cluster is requested", func() { It("should consistently return its DNS record", func() { - for i := 0; i < 5; i++ { + for range 5 { t.assertDNSRecordsFound(namespace1, service1, clusterID1, "", false, expDNSRecord) } }) @@ -145,7 +145,7 @@ func testClusterIPServiceInTwoClusters() { }) It("should consistently return its DNS record", func() { - for i := 0; i < 10; i++ { + for range 10 { Expect(t.getNonHeadlessDNSRecord(namespace1, service1, "").IP).To(Equal(serviceIP1)) } }) @@ -164,7 +164,7 @@ func testClusterIPServiceInTwoClusters() { Context("and no specific cluster is requested", func() { It("should consistently return the DNS record of the connected cluster", func() { - for i := 0; i < 10; i++ { + for range 10 { t.assertDNSRecordsFound(namespace1, service1, "", "", false, expDNSRecord) } }) @@ -210,7 +210,7 @@ func testClusterIPServiceInTwoClusters() { Context("and no specific cluster is requested", func() { It("should consistently return the DNS record of the healthy cluster", func() { - for i := 0; i < 10; i++ { + for range 10 { t.assertDNSRecordsFound(namespace1, service1, "", "", false, expDNSRecord) } }) @@ -245,7 +245,7 @@ func testClusterIPServiceInTwoClusters() { }) It("should consistently return the DNS record of the remaining cluster", func() { - for i := 0; i < 10; i++ { + for range 10 { t.assertDNSRecordsFound(namespace1, service1, "", "", false, expDNSRecord) } }) @@ -279,7 +279,7 @@ func testClusterIPServiceInThreeClusters() { }) It("should consistently return the merged service ports", func() { - for i := 0; i < 10; i++ { + for range 10 { Expect(t.getNonHeadlessDNSRecord(namespace1, service1, "").Ports).To(Equal([]mcsv1a1.ServicePort{port1})) } }) @@ -293,7 +293,7 @@ func testClusterIPServiceInThreeClusters() { } It("should consistently return its DNS record", func() { - for i := 0; i < 10; i++ { + for range 10 { t.assertDNSRecordsFound(namespace1, service1, clusterID2, "", false, expDNSRecord) } }) @@ -320,7 +320,7 @@ func testClusterIPServiceInThreeClusters() { Context("and subsequently healthy again", func() { It("should consistently return the all DNS records round-robin", func() { - for i := 0; i < 5; i++ { + for range 5 { Expect(t.getNonHeadlessDNSRecord(namespace1, service1, "").IP).To(Or(Equal(serviceIP1), Equal(serviceIP3))) } diff --git a/coredns/resolver/resolver_suite_test.go b/coredns/resolver/resolver_suite_test.go index 5b906937..58a8304f 100644 --- a/coredns/resolver/resolver_suite_test.go +++ b/coredns/resolver/resolver_suite_test.go @@ -259,7 +259,7 @@ func (t *testDriver) testRoundRobin(ns, service string, serviceIPs ...string) { ipsCount := len(serviceIPs) rrIPs := make([]string, 0) - for i := 0; i < ipsCount; i++ { + for i := range ipsCount { r := t.getNonHeadlessDNSRecord(ns, service, "") rrIPs = append(rrIPs, r.IP) slice := rrIPs[0:i] @@ -267,7 +267,7 @@ func (t *testDriver) testRoundRobin(ns, service string, serviceIPs ...string) { Expect(serviceIPs).To(ContainElement(r.IP)) } - for i := 0; i < 5; i++ { + for range 5 { for _, ip := range rrIPs { testIP := t.getNonHeadlessDNSRecord(ns, service, "").IP Expect(testIP).To(Equal(ip)) diff --git a/coredns/resolver/service_info.go b/coredns/resolver/service_info.go index f3c8c985..506ebae6 100644 --- a/coredns/resolver/service_info.go +++ b/coredns/resolver/service_info.go @@ -75,7 +75,7 @@ func (si *serviceInfo) newRecordFrom(from *DNSRecord) *DNSRecord { func (si *serviceInfo) selectIP(checkCluster func(string) bool) *DNSRecord { queueLength := si.balancer.ItemCount() - for i := 0; i < queueLength; i++ { + for range queueLength { clusterID := si.balancer.Next().(string) clusterInfo := si.clusters[clusterID] diff --git a/pkg/agent/controller/controller_suite_test.go b/pkg/agent/controller/controller_suite_test.go index 48ac599c..2e575afa 100644 --- a/pkg/agent/controller/controller_suite_test.go +++ b/pkg/agent/controller/controller_suite_test.go @@ -510,7 +510,7 @@ func (c *cluster) awaitServiceExportCondition(expected ...*mcsv1a1.ServiceExport actual := make([]*mcsv1a1.ServiceExportCondition, len(expected)) lastIndex := -1 - for i := 0; i < len(expected)-1; i++ { + for i := range len(expected) - 1 { j := lastIndex + 1 Eventually(func() interface{} { diff --git a/test/e2e/discovery/service_discovery.go b/test/e2e/discovery/service_discovery.go index 24b6698a..addb64f5 100644 --- a/test/e2e/discovery/service_discovery.go +++ b/test/e2e/discovery/service_discovery.go @@ -645,7 +645,7 @@ func verifyRoundRobinWithDig(f *framework.Framework, srcCluster framework.Cluste var retIPs []string - for count := 0; count < 10; count++ { + for range 10 { framework.AwaitUntil("verify if service IP is discoverable", func() (interface{}, error) { stdout, _, err := f.ExecWithOptions(context.TODO(), &framework.ExecOptions{ Command: cmd, diff --git a/test/e2e/framework/framework.go b/test/e2e/framework/framework.go index 926c1b51..9b18c312 100644 --- a/test/e2e/framework/framework.go +++ b/test/e2e/framework/framework.go @@ -408,7 +408,7 @@ func (f *Framework) AwaitPodIngressIPs(targetCluster framework.ClusterIndex, svc hostNameList = make([]string, 0) ipList = make([]string, 0) - for i := 0; i < len(podList.Items); i++ { + for i := range len(podList.Items) { ingressIPName := fmt.Sprintf("pod-%s", podList.Items[i].Name) ingressIP := f.Framework.AwaitGlobalIngressIP(targetCluster, ingressIPName, svc.Namespace) From b436464a321fef4c76bb9317d7f03188d733ad47 Mon Sep 17 00:00:00 2001 From: Tom Pantelis Date: Tue, 7 Jan 2025 10:44:07 -0500 Subject: [PATCH 5/8] Address maintidx linter violations Signed-off-by: Tom Pantelis --- pkg/agent/controller/clusterip_service_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/agent/controller/clusterip_service_test.go b/pkg/agent/controller/clusterip_service_test.go index 6cf47660..d1578777 100644 --- a/pkg/agent/controller/clusterip_service_test.go +++ b/pkg/agent/controller/clusterip_service_test.go @@ -46,6 +46,7 @@ var _ = Describe("ClusterIP Service export", func() { Describe("with multiple service EndpointSlices", testClusterIPServiceWithMultipleEPS) }) +//nolint:maintidx // This function composes test cases so ignore low maintainability index. func testClusterIPServiceInOneCluster() { var t *testDriver @@ -502,6 +503,7 @@ func testClusterIPServiceInOneCluster() { }) } +//nolint:maintidx // This function composes test cases so ignore low maintainability index. func testClusterIPServiceInTwoClusters() { noConflictCondition := &mcsv1a1.ServiceExportCondition{ Type: mcsv1a1.ServiceExportConflict, From cd2da4b588706a4a601e4b68cd8a3c42864f083c Mon Sep 17 00:00:00 2001 From: Tom Pantelis Date: Tue, 7 Jan 2025 10:56:46 -0500 Subject: [PATCH 6/8] Addesss nonamedreturns linter violations Signed-off-by: Tom Pantelis --- coredns/gateway/controller.go | 4 ++- .../smooth_weighted_round_robin.go | 5 +-- coredns/resolver/resolver.go | 4 +-- test/e2e/discovery/headless_services.go | 6 ++-- test/e2e/framework/framework.go | 34 ++++++++++++------- 5 files changed, 32 insertions(+), 21 deletions(-) diff --git a/coredns/gateway/controller.go b/coredns/gateway/controller.go index 98f69de7..61c86786 100644 --- a/coredns/gateway/controller.go +++ b/coredns/gateway/controller.go @@ -204,7 +204,7 @@ func (c *Controller) updateLocalClusterIDIfNeeded(clusterID string) { } } -func getGatewayStatus(obj *unstructured.Unstructured) (connections []interface{}, clusterID string, gwStatus bool) { +func getGatewayStatus(obj *unstructured.Unstructured) ([]interface{}, string, bool) { status, found, err := unstructured.NestedMap(obj.Object, "status") if !found || err != nil { logger.Errorf(err, "status field not found in %#v, err was", obj) @@ -213,6 +213,8 @@ func getGatewayStatus(obj *unstructured.Unstructured) (connections []interface{} localClusterID, found, err := unstructured.NestedString(status, "localEndpoint", "cluster_id") + var connections []interface{} + if !found || err != nil { logger.Errorf(err, "localEndpoint->cluster_id not found in %#v, err was", status) diff --git a/coredns/loadbalancer/smooth_weighted_round_robin.go b/coredns/loadbalancer/smooth_weighted_round_robin.go index a30a6713..a84b3162 100644 --- a/coredns/loadbalancer/smooth_weighted_round_robin.go +++ b/coredns/loadbalancer/smooth_weighted_round_robin.go @@ -65,7 +65,7 @@ func (lb *smoothWeightedRR) ItemCount() int { } // Add - adds a new unique item to the list. -func (lb *smoothWeightedRR) Add(item interface{}, weight int64) (err error) { +func (lb *smoothWeightedRR) Add(item interface{}, weight int64) error { if item == nil { return fmt.Errorf("item cannot be nil") } @@ -115,7 +115,8 @@ func (lb *smoothWeightedRR) nextWeightedItem() *weightedItem { return nextSmoothWeightedItem(lb.items) } -func nextSmoothWeightedItem(items []*weightedItem) (best *weightedItem) { +func nextSmoothWeightedItem(items []*weightedItem) *weightedItem { + var best *weightedItem total := int64(0) for _, item := range items { diff --git a/coredns/resolver/resolver.go b/coredns/resolver/resolver.go index ac8d5153..c35e995f 100644 --- a/coredns/resolver/resolver.go +++ b/coredns/resolver/resolver.go @@ -30,7 +30,7 @@ func New(clusterStatus ClusterStatus, client dynamic.Interface) *Interface { } } -func (i *Interface) GetDNSRecords(namespace, name, clusterID, hostname string) (records []DNSRecord, isHeadless, found bool) { +func (i *Interface) GetDNSRecords(namespace, name, clusterID, hostname string) ([]DNSRecord, bool, bool) { i.mutex.RLock() defer i.mutex.RUnlock() @@ -48,7 +48,7 @@ func (i *Interface) GetDNSRecords(namespace, name, clusterID, hostname string) ( return nil, false, found } - records, found = i.getHeadlessRecords(serviceInfo, clusterID, hostname) + records, found := i.getHeadlessRecords(serviceInfo, clusterID, hostname) return records, true, found } diff --git a/test/e2e/discovery/headless_services.go b/test/e2e/discovery/headless_services.go index 9701d453..d91a70dc 100644 --- a/test/e2e/discovery/headless_services.go +++ b/test/e2e/discovery/headless_services.go @@ -379,10 +379,10 @@ func verifyHeadlessSRVRecordsWithDig(f *framework.Framework, cluster framework.C func createSRVQuery(f *framework.Framework, port *corev1.ServicePort, service *corev1.Service, domain string, clusterName string, withPort, withcluster bool, -) (cmd []string, domainName string) { - cmd = []string{"dig", "+short", "SRV"} +) ([]string, string) { + cmd := []string{"dig", "+short", "SRV"} - domainName = lhframework.BuildServiceDNSName("", service.Name, f.Namespace, domain) + domainName := lhframework.BuildServiceDNSName("", service.Name, f.Namespace, domain) clusterDNSName := domainName if withcluster { diff --git a/test/e2e/framework/framework.go b/test/e2e/framework/framework.go index 9b18c312..94c21ce8 100644 --- a/test/e2e/framework/framework.go +++ b/test/e2e/framework/framework.go @@ -359,7 +359,9 @@ func (f *Framework) NewEndpointForHeadlessService(cluster framework.ClusterIndex func (f *Framework) AwaitEndpointIPs(targetCluster framework.ClusterIndex, name, namespace string, count int, -) (ipList, hostNameList []string) { +) ([]string, []string) { + var ipList, hostNameList []string + client := framework.KubeClients[targetCluster].DiscoveryV1().EndpointSlices(namespace) framework.By(fmt.Sprintf("Retrieving Endpoints for %s on %q", name, framework.TestContext.ClusterIDs[targetCluster])) framework.AwaitUntil("retrieve Endpoints", func() (interface{}, error) { @@ -403,10 +405,10 @@ func (f *Framework) AwaitEndpointIPs(targetCluster framework.ClusterIndex, name, func (f *Framework) AwaitPodIngressIPs(targetCluster framework.ClusterIndex, svc *v1.Service, count int, isLocal bool, -) (ipList, hostNameList []string) { +) ([]string, []string) { podList := f.Framework.AwaitPodsByAppLabel(targetCluster, svc.Labels["app"], svc.Namespace, count) - hostNameList = make([]string, 0) - ipList = make([]string, 0) + hostNameList := make([]string, 0) + ipList := make([]string, 0) for i := range len(podList.Items) { ingressIPName := fmt.Sprintf("pod-%s", podList.Items[i].Name) @@ -431,7 +433,7 @@ func (f *Framework) AwaitPodIngressIPs(targetCluster framework.ClusterIndex, svc func (f *Framework) AwaitPodIPs(targetCluster framework.ClusterIndex, svc *v1.Service, count int, isLocal bool, -) (ipList, hostNameList []string) { +) ([]string, []string) { if framework.TestContext.GlobalnetEnabled { return f.AwaitPodIngressIPs(targetCluster, svc, count, isLocal) } @@ -439,13 +441,13 @@ func (f *Framework) AwaitPodIPs(targetCluster framework.ClusterIndex, svc *v1.Se return f.AwaitEndpointIPs(targetCluster, svc.Name, svc.Namespace, count) } -func (f *Framework) GetPodIPs(targetCluster framework.ClusterIndex, service *v1.Service, isLocal bool) (ipList, hostNameList []string) { +func (f *Framework) GetPodIPs(targetCluster framework.ClusterIndex, service *v1.Service, isLocal bool) ([]string, []string) { return f.AwaitPodIPs(targetCluster, service, anyCount, isLocal) } -func (f *Framework) AwaitEndpointIngressIPs(targetCluster framework.ClusterIndex, svc *v1.Service) (ipList, hostNameList []string) { - hostNameList = make([]string, 0) - ipList = make([]string, 0) +func (f *Framework) AwaitEndpointIngressIPs(targetCluster framework.ClusterIndex, svc *v1.Service) ([]string, []string) { + hostNameList := make([]string, 0) + ipList := make([]string, 0) endpoint := framework.AwaitUntil("retrieve Endpoints", func() (interface{}, error) { return framework.KubeClients[targetCluster].CoreV1().Endpoints(svc.Namespace).Get(context.TODO(), svc.Name, metav1.GetOptions{}) @@ -467,7 +469,7 @@ func (f *Framework) AwaitEndpointIngressIPs(targetCluster framework.ClusterIndex return ipList, hostNameList } -func (f *Framework) GetEndpointIPs(targetCluster framework.ClusterIndex, svc *v1.Service) (ipList, hostNameList []string) { +func (f *Framework) GetEndpointIPs(targetCluster framework.ClusterIndex, svc *v1.Service) ([]string, []string) { if framework.TestContext.GlobalnetEnabled { return f.AwaitEndpointIngressIPs(targetCluster, svc) } @@ -548,7 +550,9 @@ func create(f *Framework, cluster framework.ClusterIndex, statefulSet *appsv1.St func (f *Framework) AwaitEndpointSlices(targetCluster framework.ClusterIndex, name, namespace string, expSliceCount, expReadyCount int, -) (endpointSliceList *discovery.EndpointSliceList) { +) *discovery.EndpointSliceList { + var endpointSliceList *discovery.EndpointSliceList + ep := framework.KubeClients[targetCluster].DiscoveryV1().EndpointSlices(namespace) labelMap := map[string]string{ discovery.LabelManagedBy: constants.LabelValueManagedBy, @@ -604,7 +608,9 @@ func (f *Framework) SetNginxStatefulSetReplicas(cluster framework.ClusterIndex, return result } -func (f *Framework) GetHealthCheckIPInfo(cluster framework.ClusterIndex) (endpointName, healthCheckIP string) { +func (f *Framework) GetHealthCheckIPInfo(cluster framework.ClusterIndex) (string, string) { + var endpointName, healthCheckIP string + framework.AwaitUntil("Get healthCheckIP", func() (interface{}, error) { unstructuredEndpointList, err := EndpointClients[cluster].List(context.TODO(), metav1.ListOptions{}) return unstructuredEndpointList, err @@ -636,7 +642,9 @@ func (f *Framework) GetHealthCheckIPInfo(cluster framework.ClusterIndex) (endpoi return endpointName, healthCheckIP } -func (f *Framework) GetHealthCheckEnabledInfo(cluster framework.ClusterIndex) (healthCheckEnabled bool) { +func (f *Framework) GetHealthCheckEnabledInfo(cluster framework.ClusterIndex) bool { + var healthCheckEnabled bool + framework.AwaitUntil("Get healthCheckEnabled Configuration", func() (interface{}, error) { unstructuredSubmarinerConfig, err := SubmarinerClients[cluster].Get(context.TODO(), "submariner", metav1.GetOptions{}) From 4c83648c5b3eb09755fb7c13a4084da28dabb03c Mon Sep 17 00:00:00 2001 From: Tom Pantelis Date: Tue, 7 Jan 2025 11:04:37 -0500 Subject: [PATCH 7/8] Address perfsprint linter violations "fmt.Sprintf can be replaced with string concatenation" "fmt.Errorf can be replaced with errors.New" Signed-off-by: Tom Pantelis --- coredns/gateway/controller.go | 3 +-- coredns/loadbalancer/smooth_weighted_round_robin.go | 3 ++- pkg/agent/controller/controller_suite_test.go | 9 ++++----- test/e2e/framework/framework.go | 4 ++-- 4 files changed, 9 insertions(+), 10 deletions(-) diff --git a/coredns/gateway/controller.go b/coredns/gateway/controller.go index 61c86786..6e873cf7 100644 --- a/coredns/gateway/controller.go +++ b/coredns/gateway/controller.go @@ -20,7 +20,6 @@ package gateway import ( "context" - "fmt" "os" "sync/atomic" @@ -110,7 +109,7 @@ func (c *Controller) Start(client dynamic.Interface) error { go c.informer.Run(c.stopCh) if ok := cache.WaitForCacheSync(c.stopCh, c.informer.HasSynced); !ok { - return fmt.Errorf("failed to wait for informer cache to sync") + return errors.New("failed to wait for informer cache to sync") } go c.queue.Run(c.stopCh, c.processNextGateway) diff --git a/coredns/loadbalancer/smooth_weighted_round_robin.go b/coredns/loadbalancer/smooth_weighted_round_robin.go index a84b3162..1065f9e4 100644 --- a/coredns/loadbalancer/smooth_weighted_round_robin.go +++ b/coredns/loadbalancer/smooth_weighted_round_robin.go @@ -21,6 +21,7 @@ package loadbalancer import ( "fmt" + "github.com/pkg/errors" "github.com/submariner-io/admiral/pkg/log" logf "sigs.k8s.io/controller-runtime/pkg/log" ) @@ -67,7 +68,7 @@ func (lb *smoothWeightedRR) ItemCount() int { // Add - adds a new unique item to the list. func (lb *smoothWeightedRR) Add(item interface{}, weight int64) error { if item == nil { - return fmt.Errorf("item cannot be nil") + return errors.New("item cannot be nil") } if weight < 0 { diff --git a/pkg/agent/controller/controller_suite_test.go b/pkg/agent/controller/controller_suite_test.go index 2e575afa..b0e92ccf 100644 --- a/pkg/agent/controller/controller_suite_test.go +++ b/pkg/agent/controller/controller_suite_test.go @@ -534,7 +534,7 @@ func (c *cluster) awaitServiceExportCondition(expected ...*mcsv1a1.ServiceExport } return nil - }).ShouldNot(BeNil(), fmt.Sprintf("ServiceExport condition not received. Expected: %s", resource.ToJSON(expected[i]))) + }).ShouldNot(BeNil(), "ServiceExport condition not received. Expected: "+resource.ToJSON(expected[i])) } last := len(expected) - 1 @@ -551,7 +551,7 @@ func (c *cluster) awaitServiceExportCondition(expected ...*mcsv1a1.ServiceExport } return nil - }).ShouldNot(BeNil(), fmt.Sprintf("ServiceExport condition not found. Expected: %s", resource.ToJSON(expected[last]))) + }).ShouldNot(BeNil(), "ServiceExport condition not found. Expected: "+resource.ToJSON(expected[last])) for i := range expected { assertEquivalentConditions(actual[i], expected[i]) @@ -575,7 +575,7 @@ func (c *cluster) ensureLastServiceExportCondition(expected *mcsv1a1.ServiceExpo } } - Fail(fmt.Sprintf("ServiceExport condition not found. Expected: %s", resource.ToJSON(expected))) + Fail("ServiceExport condition not found. Expected: " + resource.ToJSON(expected)) return -1 } @@ -583,8 +583,7 @@ func (c *cluster) ensureLastServiceExportCondition(expected *mcsv1a1.ServiceExpo initialIndex := indexOfLastCondition() Consistently(func() int { return indexOfLastCondition() - }).Should(Equal(initialIndex), fmt.Sprintf("Expected ServiceExport condition to not change: %s", - resource.ToJSON(expected))) + }).Should(Equal(initialIndex), "Expected ServiceExport condition to not change: "+resource.ToJSON(expected)) } func (c *cluster) ensureNoServiceExportCondition(condType mcsv1a1.ServiceExportConditionType, serviceExports ...*mcsv1a1.ServiceExport) { diff --git a/test/e2e/framework/framework.go b/test/e2e/framework/framework.go index 94c21ce8..1cba9fc2 100644 --- a/test/e2e/framework/framework.go +++ b/test/e2e/framework/framework.go @@ -411,7 +411,7 @@ func (f *Framework) AwaitPodIngressIPs(targetCluster framework.ClusterIndex, svc ipList := make([]string, 0) for i := range len(podList.Items) { - ingressIPName := fmt.Sprintf("pod-%s", podList.Items[i].Name) + ingressIPName := "pod-" + podList.Items[i].Name ingressIP := f.Framework.AwaitGlobalIngressIP(targetCluster, ingressIPName, svc.Namespace) if isLocal { @@ -652,7 +652,7 @@ func (f *Framework) GetHealthCheckEnabledInfo(cluster framework.ClusterIndex) bo }, func(result interface{}) (bool, string, error) { unstructuredSubmarinerConfig := result.(*unstructured.Unstructured) - framework.By(fmt.Sprintf("Getting the Submariner Config, for cluster %s", framework.TestContext.ClusterIDs[cluster])) + framework.By("Getting the Submariner Config, for cluster " + framework.TestContext.ClusterIDs[cluster]) var found bool var err error From 923c17afe3514cb6cf2131d06d905b8a028f082c Mon Sep 17 00:00:00 2001 From: Tom Pantelis Date: Wed, 18 Sep 2024 22:49:49 -0400 Subject: [PATCH 8/8] Add GHA to run MCS conformance tests The GHA clones the "sigs.k8s.io/mcs-api" repo with the desired commit and runs `go test` directly in the directory. "v0.1.0" doesn't have the conformance tests so we can't run them via Go import and trying to import a later commit is problematic due incompatible changes to the MCS APIs. Also the conformance tests are really tied to the MCS spec and not specifically to the API version so they'll likely evolve separately anyway. The clusterset IP field in the ServiceImport is required by the spec for ClusterIP services so that conformance test will fail unless the clusterset IP feature is enabled in Lighthouse. However, with that enabled, the connectivity conformance tests will fail b/c Submariner doesn't perform routing in that case. Thus, two separate GHA jobs are configured, one with clusterset IP enabled and one without, and the appropriate conformance tests are run/skipped by specifying the appropriate Ginkgo label filter. Signed-off-by: Tom Pantelis --- .github/workflows/e2e.yml | 46 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/.github/workflows/e2e.yml b/.github/workflows/e2e.yml index e0373fdc..550aaef6 100644 --- a/.github/workflows/e2e.yml +++ b/.github/workflows/e2e.yml @@ -68,3 +68,49 @@ jobs: - name: Post mortem if: failure() uses: submariner-io/shipyard/gh-actions/post-mortem@devel + conformance-test: + name: MCS Conformance + needs: images + timeout-minutes: 60 + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + using: ['clusterset-ip', ''] + steps: + - name: Check out the repository + uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 + + - name: Check out the mcs-api repository + uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 + with: + ref: 2b1ebf0d1a12261b9982393a5b0e4abceefa437f + repository: kubernetes-sigs/mcs-api + path: mcs-api + + - name: Deploy Submariner + shell: bash + run: | + make deploy using="${{ matrix.using }}" + + - name: Run conformance tests + shell: bash + run: | + export KUBECONFIG=$(find $(git rev-parse --show-toplevel)/output/kubeconfigs/ -type f -printf %p:) + label_filter="(!ClusterIP || Connectivity) && !ExportedLabels" + if [[ "${{ matrix.using }}" =~ "clusterset-ip" ]]; then + label_filter="ClusterIP && !Connectivity" + fi + cd mcs-api/conformance + go test -v -timeout 30m -contexts cluster1,cluster2 -args -test.timeout 15m \ + --ginkgo.v --ginkgo.trace --ginkgo.label-filter "${label_filter}" + + - name: Print report.html + if: always() + shell: bash + run: | + cat mcs-api/conformance/report.html + + - name: Post mortem + if: failure() + uses: submariner-io/shipyard/gh-actions/post-mortem@devel