From 405460b2ed3a101942bbfe629bbd492f92881c08 Mon Sep 17 00:00:00 2001 From: Rick Rossi Date: Mon, 2 Dec 2024 15:33:15 -0500 Subject: [PATCH 1/4] Fix linter --- Makefile | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index 7532691c3..c105070e6 100644 --- a/Makefile +++ b/Makefile @@ -54,9 +54,11 @@ CONTROLLER_GEN ?= $(LOCALBIN)/controller-gen ENVTEST ?= $(LOCALBIN)/setup-envtest CHLOGGEN ?= $(LOCALBIN)/chloggen ADDLICENSE ?= $(LOCALBIN)/addlicense +GOLANGCI_LINT ?= $(LOCALBIN)/golangci-lint KUSTOMIZE_VERSION ?= v5.0.3 CONTROLLER_TOOLS_VERSION ?= v0.14.0 +GOLANGCI_LINT_VERSION ?= v1.57.2 ALL_SRC := $(shell find . -name '*.go' -type f | sort) CW_AGENT_OPERATOR_IMPORT_PATH = "github.com/aws/amazon-cloudwatch-agent-operator" @@ -202,8 +204,8 @@ impi: @echo "Check import order/grouping finished" .PHONY: lint -lint: simple-lint - $(LOCALBIN)/golangci-lint run ./... || (curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(LOCALBIN) v1.51.1) +lint: golangci-lint simple-lint + $(GOLANGCI_LINT) run simple-lint: checklicense impi @@ -233,6 +235,9 @@ checklicense: install-addlicense echo "Check License finished successfully"; \ fi +.PHONY: golangci-lint +golangci-lint: ## Download golangci-lint locally if necessary. + $(call go-get-tool,$(GOLANGCI_LINT),github.com/golangci/golangci-lint/cmd/golangci-lint,$(GOLANGCI_LINT_VERSION)) .PHONY: envtest envtest: $(ENVTEST) ## Download envtest-setup locally if necessary. From 47ae065e3454868ff9dfcd116b03400d75b7cfd0 Mon Sep 17 00:00:00 2001 From: Rick Rossi Date: Mon, 2 Dec 2024 15:33:34 -0500 Subject: [PATCH 2/4] Pin setup-envtest to compatible version --- Makefile | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index c105070e6..d141d3294 100644 --- a/Makefile +++ b/Makefile @@ -242,7 +242,8 @@ golangci-lint: ## Download golangci-lint locally if necessary. .PHONY: envtest envtest: $(ENVTEST) ## Download envtest-setup locally if necessary. $(ENVTEST): $(LOCALBIN) - test -s $(LOCALBIN)/setup-envtest || GOBIN=$(LOCALBIN) go install sigs.k8s.io/controller-runtime/tools/setup-envtest@latest + ## Pin to release-0.18 since we are not yet on Go1.23 + test -s $(LOCALBIN)/setup-envtest || GOBIN=$(LOCALBIN) go install sigs.k8s.io/controller-runtime/tools/setup-envtest@release-0.18 # go-get-tool will 'go get' any package $2 and install it to $1. PROJECT_DIR := $(shell dirname $(abspath $(lastword $(MAKEFILE_LIST)))) From e256a83deafbd9daafb406caca215711d4710cbd Mon Sep 17 00:00:00 2001 From: Rick Rossi Date: Mon, 2 Dec 2024 15:47:56 -0500 Subject: [PATCH 3/4] Fix linting issues --- .../watcher/promOperator.go | 4 +- .../watcher/promOperator_test.go | 11 +++-- controllers/common.go | 3 ++ .../validate_annotation_methods.go | 7 ++- .../validate_annotations_namespace_test.go | 6 ++- .../cmd/validate_instrumentation_vars.go | 3 +- internal/manifests/collector/ports.go | 4 +- internal/manifests/collector/ports_test.go | 15 +++--- internal/manifests/collector/suite_test.go | 49 ------------------- pkg/instrumentation/podmutator.go | 1 - 10 files changed, 29 insertions(+), 74 deletions(-) diff --git a/cmd/amazon-cloudwatch-agent-target-allocator/watcher/promOperator.go b/cmd/amazon-cloudwatch-agent-target-allocator/watcher/promOperator.go index b85e809fd..51bfca250 100644 --- a/cmd/amazon-cloudwatch-agent-target-allocator/watcher/promOperator.go +++ b/cmd/amazon-cloudwatch-agent-target-allocator/watcher/promOperator.go @@ -286,7 +286,7 @@ func (w *PrometheusCRWatcher) addStoreAssetsForServiceMonitor( for i, endp := range endps { objKey := fmt.Sprintf("serviceMonitor/%s/%s/%d", smNamespace, smName, i) - if err = store.AddBearerToken(ctx, smNamespace, endp.BearerTokenSecret, objKey); err != nil { + if err = store.AddSafeAuthorizationCredentials(ctx, smNamespace, endp.Authorization, objKey); err != nil { break } @@ -329,7 +329,7 @@ func (w *PrometheusCRWatcher) addStoreAssetsForPodMonitor( for i, endp := range podMetricsEndps { objKey := fmt.Sprintf("podMonitor/%s/%s/%d", pmNamespace, pmName, i) - if err = store.AddBearerToken(ctx, pmNamespace, &endp.BearerTokenSecret, objKey); err != nil { + if err = store.AddSafeAuthorizationCredentials(ctx, pmNamespace, endp.Authorization, objKey); err != nil { break } diff --git a/cmd/amazon-cloudwatch-agent-target-allocator/watcher/promOperator_test.go b/cmd/amazon-cloudwatch-agent-target-allocator/watcher/promOperator_test.go index 17a4ed46a..e0de772f9 100644 --- a/cmd/amazon-cloudwatch-agent-target-allocator/watcher/promOperator_test.go +++ b/cmd/amazon-cloudwatch-agent-target-allocator/watcher/promOperator_test.go @@ -189,11 +189,14 @@ func TestLoadConfig(t *testing.T) { PodMetricsEndpoints: []monitoringv1.PodMetricsEndpoint{ { Port: "web", - BearerTokenSecret: v1.SecretKeySelector{ - LocalObjectReference: v1.LocalObjectReference{ - Name: "bearer", + Authorization: &monitoringv1.SafeAuthorization{ + Type: "Bearer", + Credentials: &v1.SecretKeySelector{ + LocalObjectReference: v1.LocalObjectReference{ + Name: "bearer", + }, + Key: "token", }, - Key: "token", }, }, }, diff --git a/controllers/common.go b/controllers/common.go index 72f4849d8..979ded8e4 100644 --- a/controllers/common.go +++ b/controllers/common.go @@ -122,6 +122,9 @@ func reconcileDesiredObjectsWPrune(ctx context.Context, kubeClient client.Client } desiredObjectMap, err := reconcileDesiredObjectUIDs(ctx, kubeClient, logger, &owner, scheme, desiredObjects...) + if err != nil { + return fmt.Errorf("failed to reconcile desired objects: %w", err) + } // Pruning owned objects in the cluster which are not should not be present after the reconciliation. err = pruneStaleObjects(ctx, kubeClient, logger, previouslyOwnedObjects, desiredObjectMap) diff --git a/integration-tests/manifests/annotations/validate_annotation_methods.go b/integration-tests/manifests/annotations/validate_annotation_methods.go index 9e95ef950..305858b42 100644 --- a/integration-tests/manifests/annotations/validate_annotation_methods.go +++ b/integration-tests/manifests/annotations/validate_annotation_methods.go @@ -186,7 +186,7 @@ func checkNameSpaceAnnotations(t *testing.T, clientSet *kubernetes.Clientset, ex } } - if correct == true { + if correct { fmt.Println("Namespace annotations are correct!") return true } @@ -201,6 +201,10 @@ func updateOperator(t *testing.T, clientSet *kubernetes.Clientset, deployment *a args := deployment.Spec.Template.Spec.Containers[0].Args deployment, err = clientSet.AppsV1().Deployments(amazonCloudwatchNamespace).Get(context.TODO(), amazonControllerManager, metav1.GetOptions{}) + if err != nil { + t.Errorf("Failed to get deployment: %v\n", err) + return false + } deployment.Spec.Template.Spec.Containers[0].Args = args _, err = clientSet.AppsV1().Deployments(amazonCloudwatchNamespace).Update(context.TODO(), deployment, metav1.UpdateOptions{}) @@ -293,7 +297,6 @@ func updateAnnotationConfig(deployment *appsV1.Deployment, jsonStr string) *apps indexOfAutoAnnotationConfigString := findIndexOfPrefix("--auto-annotation-config=", args) if indexOfAutoAnnotationConfigString < 0 { deployment.Spec.Template.Spec.Containers[0].Args = append(deployment.Spec.Template.Spec.Containers[0].Args, "--auto-annotation-config="+jsonStr) - indexOfAutoAnnotationConfigString = len(deployment.Spec.Template.Spec.Containers[0].Args) - 1 } else { deployment.Spec.Template.Spec.Containers[0].Args[indexOfAutoAnnotationConfigString] = "--auto-annotation-config=" + jsonStr } diff --git a/integration-tests/manifests/annotations/validate_annotations_namespace_test.go b/integration-tests/manifests/annotations/validate_annotations_namespace_test.go index 57edc1e22..b9dcbff36 100644 --- a/integration-tests/manifests/annotations/validate_annotations_namespace_test.go +++ b/integration-tests/manifests/annotations/validate_annotations_namespace_test.go @@ -267,7 +267,6 @@ func TestAnnotationsOnMultipleResources(t *testing.T) { } func TestAutoAnnotationForManualAnnotationRemoval(t *testing.T) { - startTime := time.Now() clientSet, uniqueNamespace := setupFunction(t, "manual-annotation-removal", []string{sampleDeploymentYamlNameRelPath}) annotationConfig := auto.AnnotationConfig{ Java: auto.AnnotationResources{ @@ -279,7 +278,7 @@ func TestAutoAnnotationForManualAnnotationRemoval(t *testing.T) { if err != nil { t.Error("Error:", err) } - startTime = time.Now() + startTime := time.Now() updateTheOperator(t, clientSet, string(jsonStr)) if err != nil { t.Errorf("Failed to get deployment app: %s", err.Error()) @@ -308,6 +307,9 @@ func TestAutoAnnotationForManualAnnotationRemoval(t *testing.T) { time.Sleep(5 * time.Second) } deployment, err := clientSet.AppsV1().Deployments(uniqueNamespace).Get(ctx, deploymentName, metav1.GetOptions{}) + if err != nil { + t.Fatalf("Error getting deployment: %v\n", err) + } //Removing all annotations deployment.ObjectMeta.Annotations = nil diff --git a/integration-tests/manifests/cmd/validate_instrumentation_vars.go b/integration-tests/manifests/cmd/validate_instrumentation_vars.go index 9bb4a6b9b..51aeef38f 100644 --- a/integration-tests/manifests/cmd/validate_instrumentation_vars.go +++ b/integration-tests/manifests/cmd/validate_instrumentation_vars.go @@ -7,7 +7,6 @@ import ( "context" "encoding/json" "fmt" - "io/ioutil" "os" "path/filepath" "strings" @@ -85,7 +84,7 @@ func verifyInstrumentationEnvVariables(clientset *kubernetes.Clientset, namespac } fmt.Println("Pod environment variables:", envMap) - fileData, err := ioutil.ReadFile(jsonPath) + fileData, err := os.ReadFile(jsonPath) if err != nil { fmt.Println("Error reading JSON file:", err) return false diff --git a/internal/manifests/collector/ports.go b/internal/manifests/collector/ports.go index 6a137c701..60099db4e 100644 --- a/internal/manifests/collector/ports.go +++ b/internal/manifests/collector/ports.go @@ -70,9 +70,7 @@ var AppSignalsPortToServicePortMap = map[int32][]corev1.ServicePort{ func PortMapToServicePortList(portMap map[int32][]corev1.ServicePort) []corev1.ServicePort { ports := make([]corev1.ServicePort, 0, len(portMap)) for _, plist := range portMap { - for _, p := range plist { - ports = append(ports, p) - } + ports = append(ports, plist...) } sort.Slice(ports, func(i, j int) bool { return ports[i].Name < ports[j].Name diff --git a/internal/manifests/collector/ports_test.go b/internal/manifests/collector/ports_test.go index 2bd6ab618..16c3944db 100644 --- a/internal/manifests/collector/ports_test.go +++ b/internal/manifests/collector/ports_test.go @@ -125,10 +125,10 @@ func TestXRayGetContainerPorts(t *testing.T) { func TestXRayWithBindAddressDefaultGetContainerPorts(t *testing.T) { cfg := getStringFromFile("./test-resources/xrayAgentConfig.json") - strings.Replace(cfg, "2800", "2000", 1) + cfg = strings.Replace(cfg, "2800", "2000", 1) // set Xray trace port to 2000 containerPorts := getContainerPorts(logger, cfg, "", []corev1.ServicePort{}) assert.Equal(t, 2, len(containerPorts)) - assert.Equal(t, int32(2800), containerPorts[CWA+XrayTraces].ContainerPort) + assert.Equal(t, int32(2000), containerPorts[CWA+XrayTraces].ContainerPort) assert.Equal(t, CWA+XrayTraces, containerPorts[CWA+XrayTraces].Name) assert.Equal(t, int32(2900), containerPorts[CWA+XrayProxy].ContainerPort) assert.Equal(t, CWA+XrayProxy, containerPorts[CWA+XrayProxy].Name) @@ -137,11 +137,13 @@ func TestXRayWithBindAddressDefaultGetContainerPorts(t *testing.T) { func TestXRayWithTCPProxyBindAddressDefaultGetContainerPorts(t *testing.T) { cfg := getStringFromFile("./test-resources/xrayAgentConfig.json") - strings.Replace(cfg, "2900", "2000", 1) + cfg = strings.Replace(cfg, "2900", "2000", 1) // set Xray proxy port to 2000 containerPorts := getContainerPorts(logger, cfg, "", []corev1.ServicePort{}) assert.Equal(t, 2, len(containerPorts)) assert.Equal(t, int32(2800), containerPorts[CWA+XrayTraces].ContainerPort) assert.Equal(t, CWA+XrayTraces, containerPorts[CWA+XrayTraces].Name) + assert.Equal(t, int32(2000), containerPorts[CWA+XrayProxy].ContainerPort) + assert.Equal(t, CWA+XrayProxy, containerPorts[CWA+XrayProxy].Name) assert.Equal(t, corev1.ProtocolUDP, containerPorts[CWA+XrayTraces].Protocol) } @@ -153,7 +155,7 @@ func TestNilMetricsGetContainerPorts(t *testing.T) { func TestMultipleReceiversGetContainerPorts(t *testing.T) { cfg := getStringFromFile("./test-resources/multipleReceiversAgentConfig.json") - strings.Replace(cfg, "2900", "2000", 1) + cfg = strings.Replace(cfg, "2900", "2000", 1) // set Xray proxy to port 2000 wantPorts := []corev1.ContainerPort{ { Name: CWA + Server, @@ -200,11 +202,6 @@ func TestMultipleReceiversGetContainerPorts(t *testing.T) { Protocol: corev1.ProtocolUDP, ContainerPort: int32(2800), }, - { - Name: CWA + XrayProxy, - Protocol: corev1.ProtocolTCP, - ContainerPort: int32(2900), - }, { Name: OtlpGrpc + "-4327", Protocol: corev1.ProtocolTCP, diff --git a/internal/manifests/collector/suite_test.go b/internal/manifests/collector/suite_test.go index 33eaa54fb..ee91c7b29 100644 --- a/internal/manifests/collector/suite_test.go +++ b/internal/manifests/collector/suite_test.go @@ -118,52 +118,3 @@ func paramsWithOtelConfig(otelCfg string) manifests.Params { Recorder: record.NewFakeRecorder(10), } } - -func newParams(taContainerImage string, file string) (manifests.Params, error) { - replicas := int32(1) - var configJSON []byte - var err error - - if file == "" { - configJSON, err = os.ReadFile("testdata/test.json") - } else { - configJSON, err = os.ReadFile(file) - } - if err != nil { - return manifests.Params{}, fmt.Errorf("Error getting json file: %w", err) - } - - cfg := config.New( - config.WithCollectorImage(defaultCollectorImage), - ) - - return manifests.Params{ - Config: cfg, - OtelCol: v1alpha1.AmazonCloudWatchAgent{ - TypeMeta: metav1.TypeMeta{ - Kind: "cloudwatch.aws.amazon.com", - APIVersion: "v1", - }, - ObjectMeta: metav1.ObjectMeta{ - Name: "test", - Namespace: "default", - UID: instanceUID, - }, - Spec: v1alpha1.AmazonCloudWatchAgentSpec{ - Mode: v1alpha1.ModeStatefulSet, - Ports: []v1.ServicePort{{ - Name: "web", - Port: 80, - TargetPort: intstr.IntOrString{ - Type: intstr.Int, - IntVal: 80, - }, - NodePort: 0, - }}, - Replicas: &replicas, - Config: string(configJSON), - }, - }, - Log: logger, - }, nil -} diff --git a/pkg/instrumentation/podmutator.go b/pkg/instrumentation/podmutator.go index ba67a55c1..16ea40e94 100644 --- a/pkg/instrumentation/podmutator.go +++ b/pkg/instrumentation/podmutator.go @@ -29,7 +29,6 @@ const ( var ( errMultipleInstancesPossible = errors.New("multiple OpenTelemetry Instrumentation instances available, cannot determine which one to select") - errNoInstancesAvailable = errors.New("no OpenTelemetry Instrumentation instances available") ) type instPodMutator struct { From 415f3e76a385f807bf363da698cfb1f02cedf391 Mon Sep 17 00:00:00 2001 From: Rick Rossi Date: Mon, 2 Dec 2024 17:08:55 -0500 Subject: [PATCH 4/4] Add linter to github workflow --- .github/workflows/pr-build-test.yml | 32 ++++++++++++++++++- Makefile | 3 +- .../validate_annotation_methods.go | 1 - 3 files changed, 32 insertions(+), 4 deletions(-) diff --git a/.github/workflows/pr-build-test.yml b/.github/workflows/pr-build-test.yml index d121e6161..d04c883b4 100644 --- a/.github/workflows/pr-build-test.yml +++ b/.github/workflows/pr-build-test.yml @@ -35,4 +35,34 @@ jobs: - name: Upload coverage to Codecov uses: codecov/codecov-action@v3 with: - verbose: true \ No newline at end of file + verbose: true + + lint: + name: Code standards (linting) + runs-on: ubuntu-22.04 + steps: + - name: Check out code into the Go module directory + uses: actions/checkout@v4 + + - name: Set up Go + uses: actions/setup-go@v5 + with: + go-version: "~1.22.4" + + - name: Cache tools + uses: actions/cache@v4 + id: setup-go + with: + path: bin + key: ${{ runner.os }}-${{ runner.arch }}-${{ hashFiles('Makefile') }}-${{ steps.setup-go.outputs.go-version }} + + - uses: actions/cache@v4 + with: + path: | + /home/runner/.cache/golangci-lint + key: golangcilint-${{ hashFiles('**/go.sum') }} + restore-keys: | + golangcilint- + + - name: Lint + run: make lint \ No newline at end of file diff --git a/Makefile b/Makefile index d141d3294..07c5f0ea1 100644 --- a/Makefile +++ b/Makefile @@ -205,7 +205,7 @@ impi: .PHONY: lint lint: golangci-lint simple-lint - $(GOLANGCI_LINT) run + $(GOLANGCI_LINT) run --timeout 5m simple-lint: checklicense impi @@ -242,7 +242,6 @@ golangci-lint: ## Download golangci-lint locally if necessary. .PHONY: envtest envtest: $(ENVTEST) ## Download envtest-setup locally if necessary. $(ENVTEST): $(LOCALBIN) - ## Pin to release-0.18 since we are not yet on Go1.23 test -s $(LOCALBIN)/setup-envtest || GOBIN=$(LOCALBIN) go install sigs.k8s.io/controller-runtime/tools/setup-envtest@release-0.18 # go-get-tool will 'go get' any package $2 and install it to $1. diff --git a/integration-tests/manifests/annotations/validate_annotation_methods.go b/integration-tests/manifests/annotations/validate_annotation_methods.go index 305858b42..b2dcf209c 100644 --- a/integration-tests/manifests/annotations/validate_annotation_methods.go +++ b/integration-tests/manifests/annotations/validate_annotation_methods.go @@ -1,6 +1,5 @@ // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 - package annotations import (