From c7c4b5f97b346a57a1a6b07d30de40ba26a9c19c Mon Sep 17 00:00:00 2001 From: Yuri Sa <48062171+yuriolisa@users.noreply.github.com> Date: Wed, 7 Dec 2022 17:23:28 +0100 Subject: [PATCH] Added further lint work (#1303) * Added furhter lint work Signed-off-by: Yuri Sa * Fixed param ghosted vars Signed-off-by: Yuri Sa * add missing line Signed-off-by: Yuri Sa Signed-off-by: Yuri Sa --- .golangci.yaml | 44 ++++++++++++++++++- .../opentelemetrycollector_webhook.go | 2 +- cmd/otel-allocator/config/config.go | 4 +- cmd/otel-allocator/main.go | 11 ++--- controllers/suite_test.go | 21 +++++---- .../webhookhandler_suite_test.go | 21 +++++---- .../webhookhandler/webhookhandler_test.go | 12 ++--- pkg/collector/adapters/config_to_probe.go | 16 +++---- pkg/collector/adapters/config_validate.go | 20 ++++----- pkg/collector/reconcile/config_replace.go | 22 +++++----- pkg/collector/reconcile/configmap.go | 14 +++--- pkg/collector/reconcile/daemonset.go | 4 +- pkg/collector/reconcile/deployment.go | 4 +- pkg/collector/reconcile/deployment_test.go | 26 +++++------ .../reconcile/horizontalpodautoscaler.go | 4 +- .../reconcile/horizontalpodautoscaler_test.go | 27 +++++++----- pkg/collector/reconcile/ingress.go | 8 ++-- pkg/collector/reconcile/ingress_test.go | 4 +- pkg/collector/reconcile/service.go | 4 +- pkg/collector/reconcile/serviceaccount.go | 4 +- pkg/collector/reconcile/statefulset.go | 4 +- pkg/collector/reconcile/suite_test.go | 21 +++++---- pkg/collector/upgrade/suite_test.go | 21 +++++---- pkg/collector/upgrade/v0_36_0.go | 12 ++--- .../instrumentation_suite_test.go | 15 ++++--- .../upgrade/upgrade_suite_test.go | 7 ++- pkg/sidecar/podmutator.go | 2 +- 27 files changed, 207 insertions(+), 147 deletions(-) diff --git a/.golangci.yaml b/.golangci.yaml index e763616e1e..6a5007561e 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -1,5 +1,8 @@ run: + concurrency: 4 timeout: 5m + issues-exit-code: 1 + tests: true # all available settings of specific linters linters-settings: @@ -11,8 +14,42 @@ linters-settings: suggest-new: true misspell: locale: US + ignore-words: + - cancelled + - metre + - meter + - metres + - kilometre + - kilometres govet: - disable-all: true + # report about shadowed variables + check-shadowing: true + + # settings per analyzer + settings: + printf: # analyzer name, run `go tool vet help` to see all analyzers + funcs: # run `go tool vet help printf` to see available settings for `printf` analyzer + - (github.com/golangci/golangci-lint/pkg/logutils.Log).Infof + - (github.com/golangci/golangci-lint/pkg/logutils.Log).Warnf + - (github.com/golangci/golangci-lint/pkg/logutils.Log).Errorf + - (github.com/golangci/golangci-lint/pkg/logutils.Log).Fatalf + + enable-all: true + # TODO: Enable this and fix the alignment issues. + disable: + - fieldalignment + gofmt: + simplify: true + revive: + min-confidence: 0.8 + + depguard: + list-type: denylist + include-go-root: true + packages-with-error-message: + # See https://github.com/open-telemetry/opentelemetry-collector/issues/5200 for rationale + - sync/atomic: "Use go.uber.org/atomic instead of sync/atomic" + - github.com/pkg/errors: "Use 'errors' or 'fmt' instead of github.com/pkg/errors" linters: enable: @@ -29,4 +66,7 @@ linters: - staticcheck - ineffassign - typecheck - - unparam \ No newline at end of file + - unparam + - depguard + - errcheck + - errorlint diff --git a/apis/v1alpha1/opentelemetrycollector_webhook.go b/apis/v1alpha1/opentelemetrycollector_webhook.go index ea41f2b103..a4a1ee222c 100644 --- a/apis/v1alpha1/opentelemetrycollector_webhook.go +++ b/apis/v1alpha1/opentelemetrycollector_webhook.go @@ -132,7 +132,7 @@ func (r *OpenTelemetryCollector) validateCRDSpec() error { if r.Spec.TargetAllocator.Enabled { _, err := ta.ConfigToPromConfig(r.Spec.Config) if err != nil { - return fmt.Errorf("the OpenTelemetry Spec Prometheus configuration is incorrect, %s", err) + return fmt.Errorf("the OpenTelemetry Spec Prometheus configuration is incorrect, %w", err) } } diff --git a/cmd/otel-allocator/config/config.go b/cmd/otel-allocator/config/config.go index ed16eddce6..7185b5ce8a 100644 --- a/cmd/otel-allocator/config/config.go +++ b/cmd/otel-allocator/config/config.go @@ -15,6 +15,7 @@ package config import ( + "errors" "flag" "fmt" "io/fs" @@ -115,7 +116,8 @@ func ParseCLI() (CLIConfig, error) { clusterConfig, err := clientcmd.BuildConfigFromFlags("", *kubeconfigPath) cLIConf.KubeConfigFilePath = *kubeconfigPath if err != nil { - if _, ok := err.(*fs.PathError); !ok { + pathError := &fs.PathError{} + if ok := errors.As(err, &pathError); !ok { return CLIConfig{}, err } clusterConfig, err = rest.InClusterConfig() diff --git a/cmd/otel-allocator/main.go b/cmd/otel-allocator/main.go index 5cc9362d51..f15b4a47f9 100644 --- a/cmd/otel-allocator/main.go +++ b/cmd/otel-allocator/main.go @@ -17,6 +17,7 @@ package main import ( "context" "encoding/json" + "errors" "net/http" "net/url" "os" @@ -91,9 +92,9 @@ func main() { os.Exit(1) } defer func() { - err := watcher.Close() - if err != nil { - log.Error(err, "failed to close watcher") + watcherErr := watcher.Close() + if watcherErr != nil { + log.Error(watcherErr, "failed to close watcher") } }() @@ -115,7 +116,7 @@ func main() { signal.Notify(interrupts, os.Interrupt, syscall.SIGTERM) go func() { - if err := srv.Start(); err != http.ErrServerClosed { + if err := srv.Start(); !errors.Is(err, http.ErrServerClosed) { setupLog.Error(err, "Can't start the server") } }() @@ -139,7 +140,7 @@ func main() { } srv = newServer(log, allocator, discoveryManager, k8sclient, cliConf.ListenAddr) go func() { - if err := srv.Start(); err != http.ErrServerClosed { + if err := srv.Start(); !errors.Is(err, http.ErrServerClosed) { setupLog.Error(err, "Can't restart the server") } }() diff --git a/controllers/suite_test.go b/controllers/suite_test.go index 1068a79be3..9b79181ead 100644 --- a/controllers/suite_test.go +++ b/controllers/suite_test.go @@ -28,6 +28,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/client-go/kubernetes/scheme" + "k8s.io/client-go/rest" "k8s.io/client-go/util/retry" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -43,6 +44,8 @@ var ( testScheme *runtime.Scheme = scheme.Scheme ctx context.Context cancel context.CancelFunc + err error + cfg *rest.Config ) func TestMain(m *testing.M) { @@ -55,13 +58,13 @@ func TestMain(m *testing.M) { Paths: []string{filepath.Join("..", "config", "webhook")}, }, } - cfg, err := testEnv.Start() + cfg, err = testEnv.Start() if err != nil { fmt.Printf("failed to start testEnv: %v", err) os.Exit(1) } - if err := v1alpha1.AddToScheme(testScheme); err != nil { + if err = v1alpha1.AddToScheme(testScheme); err != nil { fmt.Printf("failed to register scheme: %v", err) os.Exit(1) } @@ -75,7 +78,7 @@ func TestMain(m *testing.M) { // start webhook server using Manager webhookInstallOptions := &testEnv.WebhookInstallOptions - mgr, err := ctrl.NewManager(cfg, ctrl.Options{ + mgr, mgrErr := ctrl.NewManager(cfg, ctrl.Options{ Scheme: testScheme, Host: webhookInstallOptions.LocalServingHost, Port: webhookInstallOptions.LocalServingPort, @@ -83,12 +86,12 @@ func TestMain(m *testing.M) { LeaderElection: false, MetricsBindAddress: "0", }) - if err != nil { - fmt.Printf("failed to start webhook server: %v", err) + if mgrErr != nil { + fmt.Printf("failed to start webhook server: %v", mgrErr) os.Exit(1) } - if err := (&v1alpha1.OpenTelemetryCollector{}).SetupWebhookWithManager(mgr); err != nil { + if err = (&v1alpha1.OpenTelemetryCollector{}).SetupWebhookWithManager(mgr); err != nil { fmt.Printf("failed to SetupWebhookWithManager: %v", err) os.Exit(1) } @@ -119,9 +122,9 @@ func TestMain(m *testing.M) { return true }, func() error { // #nosec G402 - conn, err := tls.DialWithDialer(dialer, "tcp", addrPort, &tls.Config{InsecureSkipVerify: true}) - if err != nil { - return err + conn, tlsErr := tls.DialWithDialer(dialer, "tcp", addrPort, &tls.Config{InsecureSkipVerify: true}) + if tlsErr != nil { + return tlsErr } _ = conn.Close() return nil diff --git a/internal/webhookhandler/webhookhandler_suite_test.go b/internal/webhookhandler/webhookhandler_suite_test.go index 4c0b1c6814..4e0d26be60 100644 --- a/internal/webhookhandler/webhookhandler_suite_test.go +++ b/internal/webhookhandler/webhookhandler_suite_test.go @@ -28,6 +28,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/client-go/kubernetes/scheme" + "k8s.io/client-go/rest" "k8s.io/client-go/util/retry" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -43,6 +44,8 @@ var ( testScheme *runtime.Scheme = scheme.Scheme ctx context.Context cancel context.CancelFunc + err error + cfg *rest.Config ) func TestMain(m *testing.M) { @@ -55,13 +58,13 @@ func TestMain(m *testing.M) { Paths: []string{filepath.Join("..", "..", "config", "webhook")}, }, } - cfg, err := testEnv.Start() + cfg, err = testEnv.Start() if err != nil { fmt.Printf("failed to start testEnv: %v", err) os.Exit(1) } - if err := v1alpha1.AddToScheme(testScheme); err != nil { + if err = v1alpha1.AddToScheme(testScheme); err != nil { fmt.Printf("failed to register scheme: %v", err) os.Exit(1) } @@ -75,7 +78,7 @@ func TestMain(m *testing.M) { // start webhook server using Manager webhookInstallOptions := &testEnv.WebhookInstallOptions - mgr, err := ctrl.NewManager(cfg, ctrl.Options{ + mgr, mgrErr := ctrl.NewManager(cfg, ctrl.Options{ Scheme: testScheme, Host: webhookInstallOptions.LocalServingHost, Port: webhookInstallOptions.LocalServingPort, @@ -83,12 +86,12 @@ func TestMain(m *testing.M) { LeaderElection: false, MetricsBindAddress: "0", }) - if err != nil { - fmt.Printf("failed to start webhook server: %v", err) + if mgrErr != nil { + fmt.Printf("failed to start webhook server: %v", mgrErr) os.Exit(1) } - if err := (&v1alpha1.OpenTelemetryCollector{}).SetupWebhookWithManager(mgr); err != nil { + if err = (&v1alpha1.OpenTelemetryCollector{}).SetupWebhookWithManager(mgr); err != nil { fmt.Printf("failed to SetupWebhookWithManager: %v", err) os.Exit(1) } @@ -119,9 +122,9 @@ func TestMain(m *testing.M) { return true }, func() error { // #nosec G402 - conn, err := tls.DialWithDialer(dialer, "tcp", addrPort, &tls.Config{InsecureSkipVerify: true}) - if err != nil { - return err + conn, tlsErr := tls.DialWithDialer(dialer, "tcp", addrPort, &tls.Config{InsecureSkipVerify: true}) + if tlsErr != nil { + return tlsErr } _ = conn.Close() return nil diff --git a/internal/webhookhandler/webhookhandler_test.go b/internal/webhookhandler/webhookhandler_test.go index 16b3543f4c..9987a06fc2 100644 --- a/internal/webhookhandler/webhookhandler_test.go +++ b/internal/webhookhandler/webhookhandler_test.go @@ -131,8 +131,8 @@ func TestShouldInjectSidecar(t *testing.T) { }() for i := range tt.otelcols { - err := k8sClient.Create(context.Background(), &tt.otelcols[i]) - require.NoError(t, err) + clientErr := k8sClient.Create(context.Background(), &tt.otelcols[i]) + require.NoError(t, clientErr) } encoded, err := json.Marshal(tt.pod) @@ -154,8 +154,8 @@ func TestShouldInjectSidecar(t *testing.T) { require.NoError(t, err) injector := NewWebhookHandler(cfg, logger, k8sClient, []PodMutator{sidecar.NewMutator(logger, cfg, k8sClient)}) - err = injector.InjectDecoder(decoder) - require.NoError(t, err) + injectErr := injector.InjectDecoder(decoder) + require.NoError(t, injectErr) // test res := injector.Handle(context.Background(), req) @@ -354,8 +354,8 @@ func TestPodShouldNotBeChanged(t *testing.T) { }() for i := range tt.otelcols { - err := k8sClient.Create(context.Background(), &tt.otelcols[i]) - require.NoError(t, err) + clientErr := k8sClient.Create(context.Background(), &tt.otelcols[i]) + require.NoError(t, clientErr) } encoded, err := json.Marshal(tt.pod) diff --git a/pkg/collector/adapters/config_to_probe.go b/pkg/collector/adapters/config_to_probe.go index 10dfa45cd8..84b65eb013 100644 --- a/pkg/collector/adapters/config_to_probe.go +++ b/pkg/collector/adapters/config_to_probe.go @@ -49,22 +49,22 @@ const ( // ConfigToContainerProbe converts the incoming configuration object into a container probe or returns an error. func ConfigToContainerProbe(config map[interface{}]interface{}) (*corev1.Probe, error) { - serviceProperty, ok := config["service"] - if !ok { + serviceProperty, withService := config["service"] + if !withService { return nil, errNoService } - service, ok := serviceProperty.(map[interface{}]interface{}) - if !ok { + service, withSvcProperty := serviceProperty.(map[interface{}]interface{}) + if !withSvcProperty { return nil, errServiceNotAMap } - serviceExtensionsProperty, ok := service["extensions"] - if !ok { + serviceExtensionsProperty, withExtension := service["extensions"] + if !withExtension { return nil, errNoServiceExtensions } - serviceExtensions, ok := serviceExtensionsProperty.([]interface{}) - if !ok { + serviceExtensions, withExtProperty := serviceExtensionsProperty.([]interface{}) + if !withExtProperty { return nil, errServiceExtensionsNotSlice } healthCheckServiceExtensions := make([]string, 0) diff --git a/pkg/collector/adapters/config_validate.go b/pkg/collector/adapters/config_validate.go index f34f36f2d3..37c6e6ec98 100644 --- a/pkg/collector/adapters/config_validate.go +++ b/pkg/collector/adapters/config_validate.go @@ -34,29 +34,29 @@ func GetEnabledReceivers(_ logr.Logger, config map[interface{}]interface{}) map[ for recvID := range receivers { //Safe Cast - receiverID, ok := recvID.(string) - if !ok { + receiverID, withReceiver := recvID.(string) + if !withReceiver { return nil } //Getting all receivers present in the receivers section and setting them to false. availableReceivers[receiverID] = false } - cfgService, ok := config["service"].(map[interface{}]interface{}) - if !ok { + cfgService, withService := config["service"].(map[interface{}]interface{}) + if !withService { return nil } - pipeline, ok := cfgService["pipelines"].(map[interface{}]interface{}) - if !ok { + pipeline, withPipeline := cfgService["pipelines"].(map[interface{}]interface{}) + if !withPipeline { return nil } availablePipelines := map[string]bool{} for pipID := range pipeline { //Safe Cast - pipelineID, ok := pipID.(string) - if !ok { + pipelineID, existsPipeline := pipID.(string) + if !existsPipeline { return nil } //Getting all the available pipelines. @@ -66,8 +66,8 @@ func GetEnabledReceivers(_ logr.Logger, config map[interface{}]interface{}) map[ if len(pipeline) > 0 { for pipelineID, pipelineCfg := range pipeline { //Safe Cast - pipelineV, ok := pipelineID.(string) - if !ok { + pipelineV, withPipelineCfg := pipelineID.(string) + if !withPipelineCfg { continue } //Condition will get information if there are multiple configured pipelines. diff --git a/pkg/collector/reconcile/config_replace.go b/pkg/collector/reconcile/config_replace.go index 7de12081bd..8be892b654 100644 --- a/pkg/collector/reconcile/config_replace.go +++ b/pkg/collector/reconcile/config_replace.go @@ -38,27 +38,27 @@ func ReplaceConfig(params Params) (string, error) { if !params.Instance.Spec.TargetAllocator.Enabled { return params.Instance.Spec.Config, nil } - config, err := adapters.ConfigFromString(params.Instance.Spec.Config) - if err != nil { - return "", err + config, getStringErr := adapters.ConfigFromString(params.Instance.Spec.Config) + if getStringErr != nil { + return "", getStringErr } - promCfgMap, err := ta.ConfigToPromConfig(params.Instance.Spec.Config) - if err != nil { - return "", err + promCfgMap, getCfgPromErr := ta.ConfigToPromConfig(params.Instance.Spec.Config) + if getCfgPromErr != nil { + return "", getCfgPromErr } // yaml marshaling/unsmarshaling is preferred because of the problems associated with the conversion of map to a struct using mapstructure - promCfg, err := yaml.Marshal(map[string]interface{}{ + promCfg, marshalErr := yaml.Marshal(map[string]interface{}{ "config": promCfgMap, }) - if err != nil { - return "", err + if marshalErr != nil { + return "", marshalErr } var cfg Config - if err = yaml.UnmarshalStrict(promCfg, &cfg); err != nil { - return "", fmt.Errorf("error unmarshaling YAML: %w", err) + if marshalErr = yaml.UnmarshalStrict(promCfg, &cfg); marshalErr != nil { + return "", fmt.Errorf("error unmarshaling YAML: %w", marshalErr) } for i := range cfg.PromConfig.ScrapeConfigs { diff --git a/pkg/collector/reconcile/configmap.go b/pkg/collector/reconcile/configmap.go index 9f874010c0..24a6f7cef3 100644 --- a/pkg/collector/reconcile/configmap.go +++ b/pkg/collector/reconcile/configmap.go @@ -161,10 +161,10 @@ func expectedConfigMaps(ctx context.Context, params Params, expected []corev1.Co existing := &corev1.ConfigMap{} nns := types.NamespacedName{Namespace: desired.Namespace, Name: desired.Name} - err := params.Client.Get(ctx, nns, existing) - if err != nil && errors.IsNotFound(err) { - if err := params.Client.Create(ctx, &desired); err != nil { - if errors.IsAlreadyExists(err) && retry { + clientGetErr := params.Client.Get(ctx, nns, existing) + if clientGetErr != nil && errors.IsNotFound(clientGetErr) { + if clientCreateErr := params.Client.Create(ctx, &desired); clientCreateErr != nil { + if errors.IsAlreadyExists(clientCreateErr) && retry { // let's try again? we probably had multiple updates at one, and now it exists already if err := expectedConfigMaps(ctx, params, expected, false); err != nil { // somethin else happened now... @@ -174,12 +174,12 @@ func expectedConfigMaps(ctx context.Context, params Params, expected []corev1.Co // we succeeded in the retry, exit this attempt return nil } - return fmt.Errorf("failed to create: %w", err) + return fmt.Errorf("failed to create: %w", clientCreateErr) } params.Log.V(2).Info("created", "configmap.name", desired.Name, "configmap.namespace", desired.Namespace) continue - } else if err != nil { - return fmt.Errorf("failed to get: %w", err) + } else if clientGetErr != nil { + return fmt.Errorf("failed to get: %w", clientGetErr) } // it exists already, merge the two if the end result isn't identical to the existing one diff --git a/pkg/collector/reconcile/daemonset.go b/pkg/collector/reconcile/daemonset.go index a14649c59c..8a665a194f 100644 --- a/pkg/collector/reconcile/daemonset.go +++ b/pkg/collector/reconcile/daemonset.go @@ -62,8 +62,8 @@ func expectedDaemonSets(ctx context.Context, params Params, expected []appsv1.Da nns := types.NamespacedName{Namespace: desired.Namespace, Name: desired.Name} err := params.Client.Get(ctx, nns, existing) if err != nil && k8serrors.IsNotFound(err) { - if err := params.Client.Create(ctx, &desired); err != nil { - return fmt.Errorf("failed to create: %w", err) + if clientErr := params.Client.Create(ctx, &desired); clientErr != nil { + return fmt.Errorf("failed to create: %w", clientErr) } params.Log.V(2).Info("created", "daemonset.name", desired.Name, "daemonset.namespace", desired.Namespace) continue diff --git a/pkg/collector/reconcile/deployment.go b/pkg/collector/reconcile/deployment.go index 5e4b4bd18d..6e2a8c3ecc 100644 --- a/pkg/collector/reconcile/deployment.go +++ b/pkg/collector/reconcile/deployment.go @@ -68,8 +68,8 @@ func expectedDeployments(ctx context.Context, params Params, expected []appsv1.D nns := types.NamespacedName{Namespace: desired.Namespace, Name: desired.Name} err := params.Client.Get(ctx, nns, existing) if err != nil && k8serrors.IsNotFound(err) { - if err := params.Client.Create(ctx, &desired); err != nil { - return fmt.Errorf("failed to create: %w", err) + if clientErr := params.Client.Create(ctx, &desired); clientErr != nil { + return fmt.Errorf("failed to create: %w", clientErr) } params.Log.V(2).Info("created", "deployment.name", desired.Name, "deployment.namespace", desired.Namespace) continue diff --git a/pkg/collector/reconcile/deployment_test.go b/pkg/collector/reconcile/deployment_test.go index ab146b469a..00c4e0bb9d 100644 --- a/pkg/collector/reconcile/deployment_test.go +++ b/pkg/collector/reconcile/deployment_test.go @@ -57,8 +57,7 @@ func TestExpectedDeployments(t *testing.T) { }) t.Run("should not create target allocator deployment when targetallocator is not enabled", func(t *testing.T) { - param := Params{ - Client: k8sClient, + paramTargetAllocator := Params{ Instance: v1alpha1.OpenTelemetryCollector{ TypeMeta: metav1.TypeMeta{ Kind: "opentelemetry.io", @@ -91,12 +90,11 @@ func TestExpectedDeployments(t *testing.T) { `, }, }, - Scheme: testScheme, - Log: logger, + Log: logger, } expected := []v1.Deployment{} - if param.Instance.Spec.TargetAllocator.Enabled { - expected = append(expected, targetallocator.Deployment(param.Config, param.Log, param.Instance)) + if paramTargetAllocator.Instance.Spec.TargetAllocator.Enabled { + expected = append(expected, targetallocator.Deployment(paramTargetAllocator.Config, paramTargetAllocator.Log, paramTargetAllocator.Instance)) } assert.Len(t, expected, 0) @@ -128,8 +126,7 @@ func TestExpectedDeployments(t *testing.T) { t.Run("should not update target allocator deployment replicas when collector max replicas is set", func(t *testing.T) { replicas, maxReplicas := int32(2), int32(10) oneReplica := int32(1) - param := Params{ - Client: k8sClient, + paramMaxReplicas := Params{ Instance: v1alpha1.OpenTelemetryCollector{ TypeMeta: metav1.TypeMeta{ Kind: "opentelemetry.io", @@ -168,11 +165,10 @@ func TestExpectedDeployments(t *testing.T) { `, }, }, - Scheme: testScheme, - Log: logger, + Log: logger, } expected := []v1.Deployment{} - allocator := targetallocator.Deployment(param.Config, param.Log, param.Instance) + allocator := targetallocator.Deployment(paramMaxReplicas.Config, paramMaxReplicas.Log, paramMaxReplicas.Instance) expected = append(expected, allocator) assert.Len(t, expected, 1) @@ -181,8 +177,7 @@ func TestExpectedDeployments(t *testing.T) { t.Run("should update target allocator deployment replicas when changed", func(t *testing.T) { initialReplicas, nextReplicas := int32(1), int32(2) - param := Params{ - Client: k8sClient, + paramReplicas := Params{ Instance: v1alpha1.OpenTelemetryCollector{ TypeMeta: metav1.TypeMeta{ Kind: "opentelemetry.io", @@ -220,11 +215,10 @@ func TestExpectedDeployments(t *testing.T) { `, }, }, - Scheme: testScheme, - Log: logger, + Log: logger, } expected := []v1.Deployment{} - allocator := targetallocator.Deployment(param.Config, param.Log, param.Instance) + allocator := targetallocator.Deployment(paramReplicas.Config, paramReplicas.Log, paramReplicas.Instance) expected = append(expected, allocator) assert.Len(t, expected, 1) diff --git a/pkg/collector/reconcile/horizontalpodautoscaler.go b/pkg/collector/reconcile/horizontalpodautoscaler.go index d5df719b0c..28f756e361 100644 --- a/pkg/collector/reconcile/horizontalpodautoscaler.go +++ b/pkg/collector/reconcile/horizontalpodautoscaler.go @@ -74,8 +74,8 @@ func expectedHorizontalPodAutoscalers(ctx context.Context, params Params, expect nns := types.NamespacedName{Namespace: desired.GetNamespace(), Name: desired.GetName()} err := params.Client.Get(ctx, nns, existing) if k8serrors.IsNotFound(err) { - if err := params.Client.Create(ctx, obj); err != nil { - return fmt.Errorf("failed to create: %w", err) + if clientErr := params.Client.Create(ctx, obj); clientErr != nil { + return fmt.Errorf("failed to create: %w", clientErr) } params.Log.V(2).Info("created", "hpa.name", desired.GetName(), "hpa.namespace", desired.GetNamespace()) continue diff --git a/pkg/collector/reconcile/horizontalpodautoscaler_test.go b/pkg/collector/reconcile/horizontalpodautoscaler_test.go index 3d23f29c70..1e978d9cb9 100644 --- a/pkg/collector/reconcile/horizontalpodautoscaler_test.go +++ b/pkg/collector/reconcile/horizontalpodautoscaler_test.go @@ -38,6 +38,9 @@ import ( "github.com/open-telemetry/opentelemetry-operator/pkg/platform" ) +var hpaUpdateErr error +var withHPA bool + func TestExpectedHPA(t *testing.T) { params := paramsWithHPA(autodetect.AutoscalingVersionV2Beta2) err := params.Config.AutoDetect() @@ -49,8 +52,8 @@ func TestExpectedHPA(t *testing.T) { err = expectedHorizontalPodAutoscalers(context.Background(), params, []client.Object{expectedHPA}) assert.NoError(t, err) - exists, err := populateObjectIfExists(t, &autoscalingv2beta2.HorizontalPodAutoscaler{}, types.NamespacedName{Namespace: "default", Name: "test-collector"}) - assert.NoError(t, err) + exists, hpaErr := populateObjectIfExists(t, &autoscalingv2beta2.HorizontalPodAutoscaler{}, types.NamespacedName{Namespace: "default", Name: "test-collector"}) + assert.NoError(t, hpaErr) assert.True(t, exists) }) @@ -65,27 +68,27 @@ func TestExpectedHPA(t *testing.T) { if autoscalingVersion == autodetect.AutoscalingVersionV2Beta2 { updatedAutoscaler := *updatedHPA.(*autoscalingv2beta2.HorizontalPodAutoscaler) createObjectIfNotExists(t, "test-collector", &updatedAutoscaler) - err := expectedHorizontalPodAutoscalers(context.Background(), updateParms, []client.Object{updatedHPA}) - assert.NoError(t, err) + hpaUpdateErr = expectedHorizontalPodAutoscalers(context.Background(), updateParms, []client.Object{updatedHPA}) + assert.NoError(t, hpaUpdateErr) actual := autoscalingv2beta2.HorizontalPodAutoscaler{} - exists, err := populateObjectIfExists(t, &actual, types.NamespacedName{Namespace: "default", Name: "test-collector"}) + withHPA, hpaUpdateErr = populateObjectIfExists(t, &actual, types.NamespacedName{Namespace: "default", Name: "test-collector"}) - assert.NoError(t, err) - assert.True(t, exists) + assert.NoError(t, hpaUpdateErr) + assert.True(t, withHPA) assert.Equal(t, int32(1), *actual.Spec.MinReplicas) assert.Equal(t, int32(3), actual.Spec.MaxReplicas) } else { updatedAutoscaler := *updatedHPA.(*autoscalingv2.HorizontalPodAutoscaler) createObjectIfNotExists(t, "test-collector", &updatedAutoscaler) - err := expectedHorizontalPodAutoscalers(context.Background(), updateParms, []client.Object{updatedHPA}) - assert.NoError(t, err) + hpaUpdateErr = expectedHorizontalPodAutoscalers(context.Background(), updateParms, []client.Object{updatedHPA}) + assert.NoError(t, hpaUpdateErr) actual := autoscalingv2.HorizontalPodAutoscaler{} - exists, err := populateObjectIfExists(t, &actual, types.NamespacedName{Namespace: "default", Name: "test-collector"}) + withHPA, hpaUpdateErr := populateObjectIfExists(t, &actual, types.NamespacedName{Namespace: "default", Name: "test-collector"}) - assert.NoError(t, err) - assert.True(t, exists) + assert.NoError(t, hpaUpdateErr) + assert.True(t, withHPA) assert.Equal(t, int32(1), *actual.Spec.MinReplicas) assert.Equal(t, int32(3), actual.Spec.MaxReplicas) } diff --git a/pkg/collector/reconcile/ingress.go b/pkg/collector/reconcile/ingress.go index ea82791120..9ed20fbd59 100644 --- a/pkg/collector/reconcile/ingress.go +++ b/pkg/collector/reconcile/ingress.go @@ -165,15 +165,15 @@ func expectedIngresses(ctx context.Context, params Params, expected []networking existing := &networkingv1.Ingress{} nns := types.NamespacedName{Namespace: desired.Namespace, Name: desired.Name} - err := params.Client.Get(ctx, nns, existing) - if err != nil && k8serrors.IsNotFound(err) { + clientGetErr := params.Client.Get(ctx, nns, existing) + if clientGetErr != nil && k8serrors.IsNotFound(clientGetErr) { if err := params.Client.Create(ctx, &desired); err != nil { return fmt.Errorf("failed to create: %w", err) } params.Log.V(2).Info("created", "ingress.name", desired.Name, "ingress.namespace", desired.Namespace) return nil - } else if err != nil { - return fmt.Errorf("failed to get: %w", err) + } else if clientGetErr != nil { + return fmt.Errorf("failed to get: %w", clientGetErr) } // it exists already, merge the two if the end result isn't identical to the existing one diff --git a/pkg/collector/reconcile/ingress_test.go b/pkg/collector/reconcile/ingress_test.go index 8608011bd2..9c03dd5650 100644 --- a/pkg/collector/reconcile/ingress_test.go +++ b/pkg/collector/reconcile/ingress_test.go @@ -240,8 +240,8 @@ func TestDeleteIngresses(t *testing.T) { assert.True(t, exists) // delete - if err := deleteIngresses(ctx, params(), []networkingv1.Ingress{}); err != nil { - t.Error(err) + if delIngressErr := deleteIngresses(ctx, params(), []networkingv1.Ingress{}); delIngressErr != nil { + t.Error(delIngressErr) } // check diff --git a/pkg/collector/reconcile/service.go b/pkg/collector/reconcile/service.go index 8bb8e85198..8589359a4c 100644 --- a/pkg/collector/reconcile/service.go +++ b/pkg/collector/reconcile/service.go @@ -209,8 +209,8 @@ func expectedServices(ctx context.Context, params Params, expected []corev1.Serv nns := types.NamespacedName{Namespace: desired.Namespace, Name: desired.Name} err := params.Client.Get(ctx, nns, existing) if err != nil && k8serrors.IsNotFound(err) { - if err := params.Client.Create(ctx, &desired); err != nil { - return fmt.Errorf("failed to create: %w", err) + if clientErr := params.Client.Create(ctx, &desired); clientErr != nil { + return fmt.Errorf("failed to create: %w", clientErr) } params.Log.V(2).Info("created", "service.name", desired.Name, "service.namespace", desired.Namespace) continue diff --git a/pkg/collector/reconcile/serviceaccount.go b/pkg/collector/reconcile/serviceaccount.go index b19c40aa10..77fadd7d78 100644 --- a/pkg/collector/reconcile/serviceaccount.go +++ b/pkg/collector/reconcile/serviceaccount.go @@ -71,8 +71,8 @@ func expectedServiceAccounts(ctx context.Context, params Params, expected []core nns := types.NamespacedName{Namespace: desired.Namespace, Name: desired.Name} err := params.Client.Get(ctx, nns, existing) if err != nil && k8serrors.IsNotFound(err) { - if err := params.Client.Create(ctx, &desired); err != nil { - return fmt.Errorf("failed to create: %w", err) + if clientErr := params.Client.Create(ctx, &desired); clientErr != nil { + return fmt.Errorf("failed to create: %w", clientErr) } params.Log.V(2).Info("created", "serviceaccount.name", desired.Name, "serviceaccount.namespace", desired.Namespace) continue diff --git a/pkg/collector/reconcile/statefulset.go b/pkg/collector/reconcile/statefulset.go index e663734329..d36f0acee7 100644 --- a/pkg/collector/reconcile/statefulset.go +++ b/pkg/collector/reconcile/statefulset.go @@ -63,8 +63,8 @@ func expectedStatefulSets(ctx context.Context, params Params, expected []appsv1. nns := types.NamespacedName{Namespace: desired.Namespace, Name: desired.Name} err := params.Client.Get(ctx, nns, existing) if err != nil && k8serrors.IsNotFound(err) { - if err := params.Client.Create(ctx, &desired); err != nil { - return fmt.Errorf("failed to create: %w", err) + if clientErr := params.Client.Create(ctx, &desired); clientErr != nil { + return fmt.Errorf("failed to create: %w", clientErr) } params.Log.V(2).Info("created", "statefulset.name", desired.Name, "statefulset.namespace", desired.Namespace) continue diff --git a/pkg/collector/reconcile/suite_test.go b/pkg/collector/reconcile/suite_test.go index c1640d6fe3..a83f3bf8fb 100644 --- a/pkg/collector/reconcile/suite_test.go +++ b/pkg/collector/reconcile/suite_test.go @@ -35,6 +35,7 @@ import ( "k8s.io/apimachinery/pkg/util/uuid" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/client-go/kubernetes/scheme" + "k8s.io/client-go/rest" "k8s.io/client-go/tools/record" "k8s.io/client-go/util/retry" ctrl "sigs.k8s.io/controller-runtime" @@ -56,6 +57,8 @@ var ( logger = logf.Log.WithName("unit-tests") instanceUID = uuid.NewUUID() + err error + cfg *rest.Config ) const ( @@ -73,13 +76,13 @@ func TestMain(m *testing.M) { Paths: []string{filepath.Join("..", "..", "..", "config", "webhook")}, }, } - cfg, err := testEnv.Start() + cfg, err = testEnv.Start() if err != nil { fmt.Printf("failed to start testEnv: %v", err) os.Exit(1) } - if err := v1alpha1.AddToScheme(testScheme); err != nil { + if err = v1alpha1.AddToScheme(testScheme); err != nil { fmt.Printf("failed to register scheme: %v", err) os.Exit(1) } @@ -93,7 +96,7 @@ func TestMain(m *testing.M) { // start webhook server using Manager webhookInstallOptions := &testEnv.WebhookInstallOptions - mgr, err := ctrl.NewManager(cfg, ctrl.Options{ + mgr, mgrErr := ctrl.NewManager(cfg, ctrl.Options{ Scheme: testScheme, Host: webhookInstallOptions.LocalServingHost, Port: webhookInstallOptions.LocalServingPort, @@ -101,12 +104,12 @@ func TestMain(m *testing.M) { LeaderElection: false, MetricsBindAddress: "0", }) - if err != nil { - fmt.Printf("failed to start webhook server: %v", err) + if mgrErr != nil { + fmt.Printf("failed to start webhook server: %v", mgrErr) os.Exit(1) } - if err := (&v1alpha1.OpenTelemetryCollector{}).SetupWebhookWithManager(mgr); err != nil { + if err = (&v1alpha1.OpenTelemetryCollector{}).SetupWebhookWithManager(mgr); err != nil { fmt.Printf("failed to SetupWebhookWithManager: %v", err) os.Exit(1) } @@ -137,9 +140,9 @@ func TestMain(m *testing.M) { return true }, func() error { // #nosec G402 - conn, err := tls.DialWithDialer(dialer, "tcp", addrPort, &tls.Config{InsecureSkipVerify: true}) - if err != nil { - return err + conn, tlsErr := tls.DialWithDialer(dialer, "tcp", addrPort, &tls.Config{InsecureSkipVerify: true}) + if tlsErr != nil { + return tlsErr } _ = conn.Close() return nil diff --git a/pkg/collector/upgrade/suite_test.go b/pkg/collector/upgrade/suite_test.go index 84b3e43e7c..e17404a261 100644 --- a/pkg/collector/upgrade/suite_test.go +++ b/pkg/collector/upgrade/suite_test.go @@ -28,6 +28,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/client-go/kubernetes/scheme" + "k8s.io/client-go/rest" "k8s.io/client-go/util/retry" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -43,6 +44,8 @@ var ( testScheme *runtime.Scheme = scheme.Scheme ctx context.Context cancel context.CancelFunc + err error + cfg *rest.Config ) func TestMain(m *testing.M) { @@ -56,13 +59,13 @@ func TestMain(m *testing.M) { }, } - cfg, err := testEnv.Start() + cfg, err = testEnv.Start() if err != nil { fmt.Printf("failed to start testEnv: %v", err) os.Exit(1) } - if err := v1alpha1.AddToScheme(testScheme); err != nil { + if err = v1alpha1.AddToScheme(testScheme); err != nil { fmt.Printf("failed to register scheme: %v", err) os.Exit(1) } @@ -76,7 +79,7 @@ func TestMain(m *testing.M) { // start webhook server using Manager webhookInstallOptions := &testEnv.WebhookInstallOptions - mgr, err := ctrl.NewManager(cfg, ctrl.Options{ + mgr, mgrErr := ctrl.NewManager(cfg, ctrl.Options{ Scheme: testScheme, Host: webhookInstallOptions.LocalServingHost, Port: webhookInstallOptions.LocalServingPort, @@ -84,12 +87,12 @@ func TestMain(m *testing.M) { LeaderElection: false, MetricsBindAddress: "0", }) - if err != nil { - fmt.Printf("failed to start webhook server: %v", err) + if mgrErr != nil { + fmt.Printf("failed to start webhook server: %v", mgrErr) os.Exit(1) } - if err := (&v1alpha1.OpenTelemetryCollector{}).SetupWebhookWithManager(mgr); err != nil { + if err = (&v1alpha1.OpenTelemetryCollector{}).SetupWebhookWithManager(mgr); err != nil { fmt.Printf("failed to SetupWebhookWithManager: %v", err) os.Exit(1) } @@ -120,9 +123,9 @@ func TestMain(m *testing.M) { return true }, func() error { // #nosec G402 - conn, err := tls.DialWithDialer(dialer, "tcp", addrPort, &tls.Config{InsecureSkipVerify: true}) - if err != nil { - return err + conn, tlsErr := tls.DialWithDialer(dialer, "tcp", addrPort, &tls.Config{InsecureSkipVerify: true}) + if tlsErr != nil { + return tlsErr } _ = conn.Close() return nil diff --git a/pkg/collector/upgrade/v0_36_0.go b/pkg/collector/upgrade/v0_36_0.go index 875b7a04c5..3ea3cc7785 100644 --- a/pkg/collector/upgrade/v0_36_0.go +++ b/pkg/collector/upgrade/v0_36_0.go @@ -49,24 +49,24 @@ func upgrade0_36_0(u VersionUpgrade, otelcol *v1alpha1.OpenTelemetryCollector) ( // Change tls config key from tls_settings to tls in otlp.protocols.grpc if strings.HasPrefix(k1.(string), "otlp") { - otlpConfig, ok := v1.(map[interface{}]interface{}) - if !ok { + otlpConfig, withOTLP := v1.(map[interface{}]interface{}) + if !withOTLP { // no otlpConfig? no need to fail because of that return otelcol, nil } for k2, v2 := range otlpConfig { // protocols config if k2 == "protocols" { - protocConfig, ok := v2.(map[interface{}]interface{}) - if !ok { + protocConfig, withProtocConfig := v2.(map[interface{}]interface{}) + if !withProtocConfig { // no protocolConfig? no need to fail because of that return otelcol, nil } for k3, v3 := range protocConfig { // grpc config if k3 == "grpc" || k3 == "http" { - grpcHTTPConfig, ok := v3.(map[interface{}]interface{}) - if !ok { + grpcHTTPConfig, withHTTPConfig := v3.(map[interface{}]interface{}) + if !withHTTPConfig { // no grpcHTTPConfig? no need to fail because of that return otelcol, nil } diff --git a/pkg/instrumentation/instrumentation_suite_test.go b/pkg/instrumentation/instrumentation_suite_test.go index a5fb3c46b0..4877f7355a 100644 --- a/pkg/instrumentation/instrumentation_suite_test.go +++ b/pkg/instrumentation/instrumentation_suite_test.go @@ -21,28 +21,33 @@ import ( "testing" "k8s.io/client-go/kubernetes/scheme" + "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/envtest" "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" ) -var k8sClient client.Client -var testEnv *envtest.Environment -var testScheme = scheme.Scheme +var ( + k8sClient client.Client + testEnv *envtest.Environment + testScheme = scheme.Scheme + err error + cfg *rest.Config +) func TestMain(m *testing.M) { testEnv = &envtest.Environment{ CRDDirectoryPaths: []string{filepath.Join("..", "..", "config", "crd", "bases")}, } - cfg, err := testEnv.Start() + cfg, err = testEnv.Start() if err != nil { fmt.Printf("failed to start testEnv: %v", err) os.Exit(1) } - if err := v1alpha1.AddToScheme(testScheme); err != nil { + if err = v1alpha1.AddToScheme(testScheme); err != nil { fmt.Printf("failed to register scheme: %v", err) os.Exit(1) } diff --git a/pkg/instrumentation/upgrade/upgrade_suite_test.go b/pkg/instrumentation/upgrade/upgrade_suite_test.go index fb3891f9ec..11f62ea128 100644 --- a/pkg/instrumentation/upgrade/upgrade_suite_test.go +++ b/pkg/instrumentation/upgrade/upgrade_suite_test.go @@ -21,6 +21,7 @@ import ( "testing" "k8s.io/client-go/kubernetes/scheme" + "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/envtest" @@ -30,6 +31,8 @@ import ( var k8sClient client.Client var testEnv *envtest.Environment var testScheme = scheme.Scheme +var err error +var cfg *rest.Config func TestMain(m *testing.M) { testEnv = &envtest.Environment{ @@ -38,13 +41,13 @@ func TestMain(m *testing.M) { }, } - cfg, err := testEnv.Start() + cfg, err = testEnv.Start() if err != nil { fmt.Printf("failed to start testEnv: %v", err) os.Exit(1) } - if err := v1alpha1.AddToScheme(testScheme); err != nil { + if err = v1alpha1.AddToScheme(testScheme); err != nil { fmt.Printf("failed to register scheme: %v", err) os.Exit(1) } diff --git a/pkg/sidecar/podmutator.go b/pkg/sidecar/podmutator.go index 55743db2e6..22302a44ea 100644 --- a/pkg/sidecar/podmutator.go +++ b/pkg/sidecar/podmutator.go @@ -79,7 +79,7 @@ func (p *sidecarPodMutator) Mutate(ctx context.Context, ns corev1.Namespace, pod // which instance should it talk to? otelcol, err := p.getCollectorInstance(ctx, ns, annValue) if err != nil { - if err == errMultipleInstancesPossible || err == errNoInstancesAvailable || err == errInstanceNotSidecar { + if errors.Is(err, errMultipleInstancesPossible) || errors.Is(err, errNoInstancesAvailable) || errors.Is(err, errInstanceNotSidecar) { // we still allow the pod to be created, but we log a message to the operator's logs logger.Error(err, "failed to select an OpenTelemetry Collector instance for this pod's sidecar") return pod, nil