From 1b97d3a965b6e67379c8bfccab70903490370160 Mon Sep 17 00:00:00 2001 From: Jacob Aronoff Date: Tue, 28 Nov 2023 12:19:15 -0500 Subject: [PATCH 1/2] remove old autodetection, cleanup --- .chloggen/migrate-route-reconcilation.yaml | 16 + controllers/opampbridge_controller_test.go | 6 +- .../opentelemetrycollector_controller.go | 94 ------ .../opentelemetrycollector_controller_test.go | 179 ++++++++--- controllers/reconcile_test.go | 2 + {pkg => internal}/autodetect/main.go | 12 +- {pkg => internal}/autodetect/main_test.go | 9 +- .../openshift/routes.go} | 33 +- internal/config/change_handler.go | 63 ---- internal/config/main.go | 83 +---- internal/config/main_test.go | 57 +--- internal/config/options.go | 33 +- internal/manifests/collector/route.go | 3 +- internal/manifests/collector/suite_test.go | 7 +- main.go | 15 +- pkg/autodetect/openshiftroutes.go | 31 -- pkg/collector/reconcile/route.go | 152 --------- pkg/collector/reconcile/route_test.go | 106 ------ pkg/collector/reconcile/suit_test.go | 301 ------------------ .../reconcile/testdata/ingress_testdata.yaml | 16 - pkg/collector/reconcile/testdata/test.yaml | 22 -- 21 files changed, 220 insertions(+), 1020 deletions(-) create mode 100755 .chloggen/migrate-route-reconcilation.yaml rename {pkg => internal}/autodetect/main.go (82%) rename {pkg => internal}/autodetect/main_test.go (86%) rename internal/{config/change_handler_test.go => autodetect/openshift/routes.go} (56%) delete mode 100644 internal/config/change_handler.go delete mode 100644 pkg/autodetect/openshiftroutes.go delete mode 100644 pkg/collector/reconcile/route.go delete mode 100644 pkg/collector/reconcile/route_test.go delete mode 100644 pkg/collector/reconcile/suit_test.go delete mode 100644 pkg/collector/reconcile/testdata/ingress_testdata.yaml delete mode 100644 pkg/collector/reconcile/testdata/test.yaml diff --git a/.chloggen/migrate-route-reconcilation.yaml b/.chloggen/migrate-route-reconcilation.yaml new file mode 100755 index 0000000000..a710be5be5 --- /dev/null +++ b/.chloggen/migrate-route-reconcilation.yaml @@ -0,0 +1,16 @@ +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: enhancement + +# The name of the component, or a single word describing the area of concern, (e.g. operator, target allocator, github action) +component: operator + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: removes the old way of running autodetection for openshift routes being available + +# One or more tracking issues related to the change +issues: [2108] + +# (Optional) One or more lines of additional information to render under the primary note. +# These lines will be padded with 2 spaces and then inserted directly into the document. +# Use pipe (|) for multiline entries. +subtext: diff --git a/controllers/opampbridge_controller_test.go b/controllers/opampbridge_controller_test.go index 8ce76a35dd..127fddc149 100644 --- a/controllers/opampbridge_controller_test.go +++ b/controllers/opampbridge_controller_test.go @@ -35,14 +35,14 @@ import ( "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" "github.com/open-telemetry/opentelemetry-operator/controllers" + "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/openshift" "github.com/open-telemetry/opentelemetry-operator/internal/config" - "github.com/open-telemetry/opentelemetry-operator/pkg/autodetect" ) var opampBridgeLogger = logf.Log.WithName("opamp-bridge-controller-unit-tests") var opampBridgeMockAutoDetector = &mockAutoDetect{ - OpenShiftRoutesAvailabilityFunc: func() (autodetect.OpenShiftRoutesAvailability, error) { - return autodetect.OpenShiftRoutesAvailable, nil + OpenShiftRoutesAvailabilityFunc: func() (openshift.RoutesAvailability, error) { + return openshift.RoutesAvailable, nil }, } diff --git a/controllers/opentelemetrycollector_controller.go b/controllers/opentelemetrycollector_controller.go index 990482019d..72161f2460 100644 --- a/controllers/opentelemetrycollector_controller.go +++ b/controllers/opentelemetrycollector_controller.go @@ -17,8 +17,6 @@ package controllers import ( "context" - "fmt" - "sync" "github.com/go-logr/logr" monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1" @@ -36,8 +34,6 @@ import ( "github.com/open-telemetry/opentelemetry-operator/internal/config" "github.com/open-telemetry/opentelemetry-operator/internal/manifests" collectorStatus "github.com/open-telemetry/opentelemetry-operator/internal/status/collector" - "github.com/open-telemetry/opentelemetry-operator/pkg/autodetect" - "github.com/open-telemetry/opentelemetry-operator/pkg/collector/reconcile" "github.com/open-telemetry/opentelemetry-operator/pkg/featuregate" ) @@ -48,16 +44,6 @@ type OpenTelemetryCollectorReconciler struct { scheme *runtime.Scheme log logr.Logger config config.Config - - tasks []Task - muTasks sync.RWMutex -} - -// Task represents a reconciliation task to be executed by the reconciler. -type Task struct { - Do func(context.Context, manifests.Params) error - Name string - BailOnError bool } // Params is the set of options to build a new OpenTelemetryCollectorReconciler. @@ -66,55 +52,9 @@ type Params struct { Recorder record.EventRecorder Scheme *runtime.Scheme Log logr.Logger - Tasks []Task Config config.Config } -func (r *OpenTelemetryCollectorReconciler) onOpenShiftRoutesChange() error { - plt := r.config.OpenShiftRoutes() - var ( - routesIdx = -1 - ) - r.muTasks.Lock() - for i, t := range r.tasks { - // search for route reconciler - switch t.Name { - case "routes": - routesIdx = i - } - } - r.muTasks.Unlock() - - if err := r.addRouteTask(plt, routesIdx); err != nil { - return err - } - - return r.removeRouteTask(plt, routesIdx) -} - -func (r *OpenTelemetryCollectorReconciler) addRouteTask(ora autodetect.OpenShiftRoutesAvailability, routesIdx int) error { - r.muTasks.Lock() - defer r.muTasks.Unlock() - // if exists and openshift routes are available - if routesIdx == -1 && ora == autodetect.OpenShiftRoutesAvailable { - r.tasks = append([]Task{{reconcile.Routes, "routes", true}}, r.tasks...) - } - return nil -} - -func (r *OpenTelemetryCollectorReconciler) removeRouteTask(ora autodetect.OpenShiftRoutesAvailability, routesIdx int) error { - r.muTasks.Lock() - defer r.muTasks.Unlock() - if len(r.tasks) < routesIdx { - return fmt.Errorf("can not remove route task from reconciler") - } - // if exists and openshift routes are not available - if routesIdx != -1 && ora == autodetect.OpenShiftRoutesNotAvailable { - r.tasks = append(r.tasks[:routesIdx], r.tasks[routesIdx+1:]...) - } - return nil -} - func (r *OpenTelemetryCollectorReconciler) getParams(instance v1alpha1.OpenTelemetryCollector) manifests.Params { return manifests.Params{ Config: r.config, @@ -133,15 +73,8 @@ func NewReconciler(p Params) *OpenTelemetryCollectorReconciler { log: p.Log, scheme: p.Scheme, config: p.Config, - tasks: p.Tasks, recorder: p.Recorder, } - - if len(r.tasks) == 0 { - // TODO: put this in line with the rest of how we generate manifests - // https://github.com/open-telemetry/opentelemetry-operator/issues/2108 - r.config.RegisterOpenShiftRoutesChangeCallback(r.onOpenShiftRoutesChange) - } return r } @@ -185,9 +118,6 @@ func (r *OpenTelemetryCollectorReconciler) Reconcile(ctx context.Context, req ct } params := r.getParams(instance) - if err := r.RunTasks(ctx, params); err != nil { - return ctrl.Result{}, err - } desiredObjects, buildErr := BuildCollector(params) if buildErr != nil { @@ -197,32 +127,8 @@ func (r *OpenTelemetryCollectorReconciler) Reconcile(ctx context.Context, req ct return collectorStatus.HandleReconcileStatus(ctx, log, params, err) } -// RunTasks runs all the tasks associated with this reconciler. -func (r *OpenTelemetryCollectorReconciler) RunTasks(ctx context.Context, params manifests.Params) error { - r.muTasks.RLock() - defer r.muTasks.RUnlock() - for _, task := range r.tasks { - if err := task.Do(ctx, params); err != nil { - // If we get an error that occurs because a pod is being terminated, then exit this loop - if apierrors.IsForbidden(err) && apierrors.HasStatusCause(err, corev1.NamespaceTerminatingCause) { - r.log.V(2).Info("Exiting reconcile loop because namespace is being terminated", "namespace", params.OtelCol.Namespace) - return nil - } - r.log.Error(err, fmt.Sprintf("failed to reconcile %s", task.Name)) - if task.BailOnError { - return err - } - } - } - return nil -} - // SetupWithManager tells the manager what our controller is interested in. func (r *OpenTelemetryCollectorReconciler) SetupWithManager(mgr ctrl.Manager) error { - err := r.config.AutoDetect() // We need to call this, so we can get the correct autodetect version - if err != nil { - return err - } builder := ctrl.NewControllerManagedBy(mgr). For(&v1alpha1.OpenTelemetryCollector{}). Owns(&corev1.ConfigMap{}). diff --git a/controllers/opentelemetrycollector_controller_test.go b/controllers/opentelemetrycollector_controller_test.go index ec50a9a64a..6bb6a88a88 100644 --- a/controllers/opentelemetrycollector_controller_test.go +++ b/controllers/opentelemetrycollector_controller_test.go @@ -16,7 +16,6 @@ package controllers_test import ( "context" - "errors" "fmt" "testing" @@ -37,15 +36,15 @@ import ( "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" "github.com/open-telemetry/opentelemetry-operator/controllers" + "github.com/open-telemetry/opentelemetry-operator/internal/autodetect" + "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/openshift" "github.com/open-telemetry/opentelemetry-operator/internal/config" - "github.com/open-telemetry/opentelemetry-operator/internal/manifests" - "github.com/open-telemetry/opentelemetry-operator/pkg/autodetect" ) var logger = logf.Log.WithName("unit-tests") var mockAutoDetector = &mockAutoDetect{ - OpenShiftRoutesAvailabilityFunc: func() (autodetect.OpenShiftRoutesAvailability, error) { - return autodetect.OpenShiftRoutesAvailable, nil + OpenShiftRoutesAvailabilityFunc: func() (openshift.RoutesAvailability, error) { + return openshift.RoutesAvailable, nil }, } @@ -57,6 +56,7 @@ func TestNewObjectsOnReconciliation(t *testing.T) { config.WithAutoDetect(mockAutoDetector), ) nsn := types.NamespacedName{Name: "my-instance", Namespace: "default"} + require.NoError(t, cfg.AutoDetect()) reconciler := controllers.NewReconciler(controllers.Params{ Client: k8sClient, Log: logger, @@ -64,7 +64,6 @@ func TestNewObjectsOnReconciliation(t *testing.T) { Recorder: record.NewFakeRecorder(10), Config: cfg, }) - require.NoError(t, cfg.AutoDetect()) created := &v1alpha1.OpenTelemetryCollector{ ObjectMeta: metav1.ObjectMeta{ Name: nsn.Name, @@ -164,11 +163,22 @@ func TestNewObjectsOnReconciliation(t *testing.T) { assert.NoError(t, err) assert.Len(t, list.Items, 1) require.NoError(t, k8sClient.Delete(context.Background(), list.Items[0].DeepCopy())) + + // cleanup the route deliberately, otherwise a local tester will always fail as there is no gc event. + routeList := &routev1.RouteList{} + err = k8sClient.List(context.Background(), routeList, opts...) + assert.NoError(t, err) + assert.Len(t, routeList.Items, 1) + require.NoError(t, k8sClient.Delete(context.Background(), routeList.Items[0].DeepCopy())) } -func TestNewStatefulSetObjectsOnReconciliation(t *testing.T) { +func TestNoRoutesOnNotAvailablePlatform(t *testing.T) { // prepare - cfg := config.New(config.WithAutoDetect(mockAutoDetector)) + cfg := config.New( + config.WithCollectorImage("default-collector"), + config.WithTargetAllocatorImage("default-ta-allocator"), + config.WithOpenShiftRoutesAvailability(openshift.RoutesNotAvailable), + ) nsn := types.NamespacedName{Name: "my-instance", Namespace: "default"} reconciler := controllers.NewReconciler(controllers.Params{ Client: k8sClient, @@ -183,7 +193,19 @@ func TestNewStatefulSetObjectsOnReconciliation(t *testing.T) { Namespace: nsn.Namespace, }, Spec: v1alpha1.OpenTelemetryCollectorSpec{ - Mode: v1alpha1.ModeStatefulSet, + Mode: v1alpha1.ModeDeployment, + Ports: []corev1.ServicePort{ + { + Name: "telnet", + Port: 49935, + }, + }, + Ingress: v1alpha1.Ingress{ + Type: v1alpha1.IngressTypeRoute, + Route: v1alpha1.OpenShiftRoute{ + Termination: v1alpha1.TLSRouteTerminationTypeInsecure, + }, + }, }, } err := k8sClient.Create(context.Background(), created) @@ -229,60 +251,130 @@ func TestNewStatefulSetObjectsOnReconciliation(t *testing.T) { assert.NotEmpty(t, list.Items) } { - list := &appsv1.StatefulSetList{} + list := &appsv1.DeploymentList{} err = k8sClient.List(context.Background(), list, opts...) assert.NoError(t, err) assert.NotEmpty(t, list.Items) } { - list := &appsv1.DeploymentList{} + list := &appsv1.DaemonSetList{} err = k8sClient.List(context.Background(), list, opts...) assert.NoError(t, err) - // attention! we expect deployments to be empty when starting in the statefulset mode. + // attention! we expect daemonsets to be empty in the default configuration assert.Empty(t, list.Items) } { - list := &appsv1.DaemonSetList{} + list := &appsv1.StatefulSetList{} err = k8sClient.List(context.Background(), list, opts...) assert.NoError(t, err) - // attention! we expect daemonsets to be empty when starting in the statefulset mode. + // attention! we expect statefulsets to be empty in the default configuration + assert.Empty(t, list.Items) + } + { + list := &routev1.RouteList{} + err = k8sClient.List(context.Background(), list, opts...) + // routes should be empty in this test because we explicitly do not have openshift routes available. + assert.NoError(t, err) assert.Empty(t, list.Items) } // cleanup require.NoError(t, k8sClient.Delete(context.Background(), created)) + // cleanup the deployment deliberately, otherwise a local tester will always fail as there is no gc event. + list := &appsv1.DeploymentList{} + err = k8sClient.List(context.Background(), list, opts...) + assert.NoError(t, err) + assert.Len(t, list.Items, 1) + require.NoError(t, k8sClient.Delete(context.Background(), list.Items[0].DeepCopy())) } -func TestContinueOnRecoverableFailure(t *testing.T) { +func TestNewStatefulSetObjectsOnReconciliation(t *testing.T) { // prepare - taskCalled := false + cfg := config.New(config.WithAutoDetect(mockAutoDetector)) + nsn := types.NamespacedName{Name: "my-instance", Namespace: "default"} reconciler := controllers.NewReconciler(controllers.Params{ - Log: logger, - Tasks: []controllers.Task{ - { - Name: "should-fail", - Do: func(context.Context, manifests.Params) error { - return errors.New("should fail") - }, - BailOnError: false, - }, - { - Name: "should-be-called", - Do: func(context.Context, manifests.Params) error { - taskCalled = true - return nil - }, - }, - }, + Client: k8sClient, + Log: logger, + Scheme: testScheme, + Recorder: record.NewFakeRecorder(10), + Config: cfg, }) + created := &v1alpha1.OpenTelemetryCollector{ + ObjectMeta: metav1.ObjectMeta{ + Name: nsn.Name, + Namespace: nsn.Namespace, + }, + Spec: v1alpha1.OpenTelemetryCollectorSpec{ + Mode: v1alpha1.ModeStatefulSet, + }, + } + err := k8sClient.Create(context.Background(), created) + require.NoError(t, err) // test - err := reconciler.RunTasks(context.Background(), manifests.Params{}) + req := k8sreconcile.Request{ + NamespacedName: nsn, + } + _, err = reconciler.Reconcile(context.Background(), req) // verify - assert.NoError(t, err) - assert.True(t, taskCalled) + require.NoError(t, err) + + // the base query for the underlying objects + opts := []client.ListOption{ + client.InNamespace(nsn.Namespace), + client.MatchingLabels(map[string]string{ + "app.kubernetes.io/instance": fmt.Sprintf("%s.%s", nsn.Namespace, nsn.Name), + "app.kubernetes.io/managed-by": "opentelemetry-operator", + "app.kubernetes.io/component": "opentelemetry-collector", + }), + } + + // verify that we have at least one object for each of the types we create + // whether we have the right ones is up to the specific tests for each type + { + list := &corev1.ConfigMapList{} + err = k8sClient.List(context.Background(), list, opts...) + assert.NoError(t, err) + assert.NotEmpty(t, list.Items) + } + { + list := &corev1.ServiceAccountList{} + err = k8sClient.List(context.Background(), list, opts...) + assert.NoError(t, err) + assert.NotEmpty(t, list.Items) + } + { + list := &corev1.ServiceList{} + err = k8sClient.List(context.Background(), list, opts...) + assert.NoError(t, err) + assert.NotEmpty(t, list.Items) + } + { + list := &appsv1.StatefulSetList{} + err = k8sClient.List(context.Background(), list, opts...) + assert.NoError(t, err) + assert.NotEmpty(t, list.Items) + } + { + list := &appsv1.DeploymentList{} + err = k8sClient.List(context.Background(), list, opts...) + assert.NoError(t, err) + // attention! we expect deployments to be empty when starting in the statefulset mode. + assert.Empty(t, list.Items) + } + { + list := &appsv1.DaemonSetList{} + err = k8sClient.List(context.Background(), list, opts...) + assert.NoError(t, err) + // attention! we expect daemonsets to be empty when starting in the statefulset mode. + assert.Empty(t, list.Items) + } + + // cleanup + require.NoError(t, k8sClient.Delete(context.Background(), created)) + } func TestSkipWhenInstanceDoesNotExist(t *testing.T) { @@ -294,15 +386,6 @@ func TestSkipWhenInstanceDoesNotExist(t *testing.T) { Log: logger, Scheme: scheme.Scheme, Config: cfg, - Tasks: []controllers.Task{ - { - Name: "should-not-be-called", - Do: func(context.Context, manifests.Params) error { - assert.Fail(t, "should not have been called") - return nil - }, - }, - }, }) // test @@ -334,12 +417,12 @@ func TestRegisterWithManager(t *testing.T) { var _ autodetect.AutoDetect = (*mockAutoDetect)(nil) type mockAutoDetect struct { - OpenShiftRoutesAvailabilityFunc func() (autodetect.OpenShiftRoutesAvailability, error) + OpenShiftRoutesAvailabilityFunc func() (openshift.RoutesAvailability, error) } -func (m *mockAutoDetect) OpenShiftRoutesAvailability() (autodetect.OpenShiftRoutesAvailability, error) { +func (m *mockAutoDetect) OpenShiftRoutesAvailability() (openshift.RoutesAvailability, error) { if m.OpenShiftRoutesAvailabilityFunc != nil { return m.OpenShiftRoutesAvailabilityFunc() } - return autodetect.OpenShiftRoutesNotAvailable, nil + return openshift.RoutesNotAvailable, nil } diff --git a/controllers/reconcile_test.go b/controllers/reconcile_test.go index b83e165997..f10f21e5fd 100644 --- a/controllers/reconcile_test.go +++ b/controllers/reconcile_test.go @@ -38,6 +38,7 @@ import ( "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" "github.com/open-telemetry/opentelemetry-operator/controllers" + "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/openshift" "github.com/open-telemetry/opentelemetry-operator/internal/config" "github.com/open-telemetry/opentelemetry-operator/internal/manifests" ta "github.com/open-telemetry/opentelemetry-operator/internal/manifests/targetallocator/adapters" @@ -530,6 +531,7 @@ func TestOpenTelemetryCollectorReconciler_Reconcile(t *testing.T) { Config: config.New( config.WithCollectorImage("default-collector"), config.WithTargetAllocatorImage("default-ta-allocator"), + config.WithOpenShiftRoutesAvailability(openshift.RoutesAvailable), ), }) assert.True(t, len(tt.want) > 0, "must have at least one group of checks to run") diff --git a/pkg/autodetect/main.go b/internal/autodetect/main.go similarity index 82% rename from pkg/autodetect/main.go rename to internal/autodetect/main.go index d12d19b297..fb71adaa39 100644 --- a/pkg/autodetect/main.go +++ b/internal/autodetect/main.go @@ -18,13 +18,15 @@ package autodetect import ( "k8s.io/client-go/discovery" "k8s.io/client-go/rest" + + "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/openshift" ) var _ AutoDetect = (*autoDetect)(nil) // AutoDetect provides an assortment of routines that auto-detect traits based on the runtime. type AutoDetect interface { - OpenShiftRoutesAvailability() (OpenShiftRoutesAvailability, error) + OpenShiftRoutesAvailability() (openshift.RoutesAvailability, error) } type autoDetect struct { @@ -47,18 +49,18 @@ func New(restConfig *rest.Config) (AutoDetect, error) { } // OpenShiftRoutesAvailability checks if OpenShift Route are available. -func (a *autoDetect) OpenShiftRoutesAvailability() (OpenShiftRoutesAvailability, error) { +func (a *autoDetect) OpenShiftRoutesAvailability() (openshift.RoutesAvailability, error) { apiList, err := a.dcl.ServerGroups() if err != nil { - return OpenShiftRoutesNotAvailable, err + return openshift.RoutesNotAvailable, err } apiGroups := apiList.Groups for i := 0; i < len(apiGroups); i++ { if apiGroups[i].Name == "route.openshift.io" { - return OpenShiftRoutesAvailable, nil + return openshift.RoutesAvailable, nil } } - return OpenShiftRoutesNotAvailable, nil + return openshift.RoutesNotAvailable, nil } diff --git a/pkg/autodetect/main_test.go b/internal/autodetect/main_test.go similarity index 86% rename from pkg/autodetect/main_test.go rename to internal/autodetect/main_test.go index 18a9640f0b..6c18eee59a 100644 --- a/pkg/autodetect/main_test.go +++ b/internal/autodetect/main_test.go @@ -25,17 +25,18 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/rest" - "github.com/open-telemetry/opentelemetry-operator/pkg/autodetect" + "github.com/open-telemetry/opentelemetry-operator/internal/autodetect" + "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/openshift" ) func TestDetectPlatformBasedOnAvailableAPIGroups(t *testing.T) { for _, tt := range []struct { apiGroupList *metav1.APIGroupList - expected autodetect.OpenShiftRoutesAvailability + expected openshift.RoutesAvailability }{ { &metav1.APIGroupList{}, - autodetect.OpenShiftRoutesNotAvailable, + openshift.RoutesNotAvailable, }, { &metav1.APIGroupList{ @@ -45,7 +46,7 @@ func TestDetectPlatformBasedOnAvailableAPIGroups(t *testing.T) { }, }, }, - autodetect.OpenShiftRoutesAvailable, + openshift.RoutesAvailable, }, } { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { diff --git a/internal/config/change_handler_test.go b/internal/autodetect/openshift/routes.go similarity index 56% rename from internal/config/change_handler_test.go rename to internal/autodetect/openshift/routes.go index 069ffa3f8a..1872b9ba10 100644 --- a/internal/config/change_handler_test.go +++ b/internal/autodetect/openshift/routes.go @@ -12,30 +12,19 @@ // See the License for the specific language governing permissions and // limitations under the License. -// Package config contains the operator's runtime configuration. -package config +package openshift -import ( - "testing" +// RoutesAvailability holds the auto-detected OpenShift Routes availability API. +type RoutesAvailability int - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestChangeHandler(t *testing.T) { - // prepare - internal := 0 - callback := func() error { - internal += 1 - return nil - } - h := newOnChange() +const ( + // RoutesAvailable represents the route.openshift.io API is available. + RoutesAvailable RoutesAvailability = iota - h.Register(callback) + // RoutesNotAvailable represents the route.openshift.io API is not available. + RoutesNotAvailable +) - for i := 0; i < 5; i++ { - assert.Equal(t, i, internal) - require.NoError(t, h.Do()) - assert.Equal(t, i+1, internal) - } +func (p RoutesAvailability) String() string { + return [...]string{"Available", "NotAvailable"}[p] } diff --git a/internal/config/change_handler.go b/internal/config/change_handler.go deleted file mode 100644 index 46646c7129..0000000000 --- a/internal/config/change_handler.go +++ /dev/null @@ -1,63 +0,0 @@ -// Copyright The OpenTelemetry Authors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -// Package config contains the operator's runtime configuration. -package config - -import ( - "sync" - - "github.com/go-logr/logr" - logf "sigs.k8s.io/controller-runtime/pkg/log" -) - -// changeHandler is implemented by any structure that is able to register callbacks -// and call them using one single method. -type changeHandler interface { - // Do will call every registered callback. - Do() error - // Register this function as a callback that will be executed when Do() is called. - Register(f func() error) -} - -// newOnChange returns a thread-safe ChangeHandler. -func newOnChange() changeHandler { - return &onChange{ - logger: logf.Log.WithName("change-handler"), - } -} - -type onChange struct { - logger logr.Logger - - callbacks []func() error - muCallbacks sync.Mutex -} - -func (o *onChange) Do() error { - o.muCallbacks.Lock() - defer o.muCallbacks.Unlock() - for _, fn := range o.callbacks { - if err := fn(); err != nil { - o.logger.Error(err, "change callback failed") - } - } - return nil -} - -func (o *onChange) Register(f func() error) { - o.muCallbacks.Lock() - defer o.muCallbacks.Unlock() - o.callbacks = append(o.callbacks, f) -} diff --git a/internal/config/main.go b/internal/config/main.go index 7abb1296a3..414c58d165 100644 --- a/internal/config/main.go +++ b/internal/config/main.go @@ -16,14 +16,14 @@ package config import ( - "sync" "time" "github.com/go-logr/logr" logf "sigs.k8s.io/controller-runtime/pkg/log" + "github.com/open-telemetry/opentelemetry-operator/internal/autodetect" + "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/openshift" "github.com/open-telemetry/opentelemetry-operator/internal/version" - "github.com/open-telemetry/opentelemetry-operator/pkg/autodetect" ) const ( @@ -50,24 +50,20 @@ type Config struct { operatorOpAMPBridgeConfigMapEntry string autoInstrumentationNodeJSImage string autoInstrumentationJavaImage string - onOpenShiftRoutesChange changeHandler + openshiftRoutesAvailability openshift.RoutesAvailability labelsFilter []string - openshiftRoutes openshiftRoutesStore - autoDetectFrequency time.Duration } // New constructs a new configuration based on the given options. func New(opts ...Option) Config { // initialize with the default values o := options{ - autoDetectFrequency: defaultAutoDetectFrequency, + openshiftRoutesAvailability: openshift.RoutesNotAvailable, collectorConfigMapEntry: defaultCollectorConfigMapEntry, targetAllocatorConfigMapEntry: defaultTargetAllocatorConfigMapEntry, operatorOpAMPBridgeConfigMapEntry: defaultOperatorOpAMPBridgeConfigMapEntry, logger: logf.Log.WithName("config"), - openshiftRoutes: newOpenShiftRoutesWrapper(), version: version.Get(), - onOpenShiftRoutesChange: newOnChange(), } for _, opt := range opts { opt(&o) @@ -75,7 +71,6 @@ func New(opts ...Option) Config { return Config{ autoDetect: o.autoDetect, - autoDetectFrequency: o.autoDetectFrequency, collectorImage: o.collectorImage, collectorConfigMapEntry: o.collectorConfigMapEntry, targetAllocatorImage: o.targetAllocatorImage, @@ -83,8 +78,7 @@ func New(opts ...Option) Config { targetAllocatorConfigMapEntry: o.targetAllocatorConfigMapEntry, operatorOpAMPBridgeConfigMapEntry: o.operatorOpAMPBridgeConfigMapEntry, logger: o.logger, - openshiftRoutes: o.openshiftRoutes, - onOpenShiftRoutesChange: o.onOpenShiftRoutesChange, + openshiftRoutesAvailability: o.openshiftRoutesAvailability, autoInstrumentationJavaImage: o.autoInstrumentationJavaImage, autoInstrumentationNodeJSImage: o.autoInstrumentationNodeJSImage, autoInstrumentationPythonImage: o.autoInstrumentationPythonImage, @@ -99,20 +93,7 @@ func New(opts ...Option) Config { // StartAutoDetect attempts to automatically detect relevant information for this operator. This will block until the first // run is executed and will schedule periodic updates. func (c *Config) StartAutoDetect() error { - err := c.AutoDetect() - go c.periodicAutoDetect() - - return err -} - -func (c *Config) periodicAutoDetect() { - ticker := time.NewTicker(c.autoDetectFrequency) - - for range ticker.C { - if err := c.AutoDetect(); err != nil { - c.logger.Info("auto-detection failed", "error", err) - } - } + return c.AutoDetect() } // AutoDetect attempts to automatically detect relevant information for this operator. @@ -123,16 +104,7 @@ func (c *Config) AutoDetect() error { if err != nil { return err } - - if c.openshiftRoutes.Get() != ora { - c.logger.V(1).Info("openshift routes detected", "available", ora) - c.openshiftRoutes.Set(ora) - if err = c.onOpenShiftRoutesChange.Do(); err != nil { - // Don't fail if the callback failed, as auto-detection itself worked. - c.logger.Error(err, "configuration change notification failed for callback") - } - } - + c.openshiftRoutesAvailability = ora return nil } @@ -166,9 +138,9 @@ func (c *Config) OperatorOpAMPBridgeConfigMapEntry() string { return c.operatorOpAMPBridgeConfigMapEntry } -// OpenShiftRoutes represents the availability of the OpenShift Routes API. -func (c *Config) OpenShiftRoutes() autodetect.OpenShiftRoutesAvailability { - return c.openshiftRoutes.Get() +// OpenShiftRoutesAvailability represents the availability of the OpenShift Routes API. +func (c *Config) OpenShiftRoutesAvailability() openshift.RoutesAvailability { + return c.openshiftRoutesAvailability } // AutoInstrumentationJavaImage returns OpenTelemetry Java auto-instrumentation container image. @@ -210,38 +182,3 @@ func (c *Config) AutoInstrumentationNginxImage() string { func (c *Config) LabelsFilter() []string { return c.labelsFilter } - -// RegisterOpenShiftRoutesChangeCallback registers the given function as a callback that -// is called when the OpenShift Routes detection detects a change. -func (c *Config) RegisterOpenShiftRoutesChangeCallback(f func() error) { - c.onOpenShiftRoutesChange.Register(f) -} - -type openshiftRoutesStore interface { - Set(ora autodetect.OpenShiftRoutesAvailability) - Get() autodetect.OpenShiftRoutesAvailability -} - -func newOpenShiftRoutesWrapper() openshiftRoutesStore { - return &openshiftRoutesWrapper{ - current: autodetect.OpenShiftRoutesNotAvailable, - } -} - -type openshiftRoutesWrapper struct { - mu sync.Mutex - current autodetect.OpenShiftRoutesAvailability -} - -func (p *openshiftRoutesWrapper) Set(ora autodetect.OpenShiftRoutesAvailability) { - p.mu.Lock() - p.current = ora - p.mu.Unlock() -} - -func (p *openshiftRoutesWrapper) Get() autodetect.OpenShiftRoutesAvailability { - p.mu.Lock() - ora := p.current - p.mu.Unlock() - return ora -} diff --git a/internal/config/main_test.go b/internal/config/main_test.go index 0157b97f6c..44345bdf56 100644 --- a/internal/config/main_test.go +++ b/internal/config/main_test.go @@ -15,15 +15,14 @@ package config_test import ( - "sync" "testing" - "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/open-telemetry/opentelemetry-operator/internal/autodetect" + "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/openshift" "github.com/open-telemetry/opentelemetry-operator/internal/config" - "github.com/open-telemetry/opentelemetry-operator/pkg/autodetect" ) func TestNewConfig(t *testing.T) { @@ -31,78 +30,46 @@ func TestNewConfig(t *testing.T) { cfg := config.New( config.WithCollectorImage("some-image"), config.WithCollectorConfigMapEntry("some-config.yaml"), - config.WithPlatform(autodetect.OpenShiftRoutesNotAvailable), + config.WithOpenShiftRoutesAvailability(openshift.RoutesAvailable), ) // test assert.Equal(t, "some-image", cfg.CollectorImage()) assert.Equal(t, "some-config.yaml", cfg.CollectorConfigMapEntry()) - assert.Equal(t, autodetect.OpenShiftRoutesNotAvailable, cfg.OpenShiftRoutes()) + assert.Equal(t, openshift.RoutesAvailable, cfg.OpenShiftRoutesAvailability()) } -func TestOnPlatformChangeCallback(t *testing.T) { +func TestConfigChangesOnAutoDetect(t *testing.T) { // prepare - calledBack := false mock := &mockAutoDetect{ - OpenShiftRoutesAvailabilityFunc: func() (autodetect.OpenShiftRoutesAvailability, error) { - return autodetect.OpenShiftRoutesAvailable, nil + OpenShiftRoutesAvailabilityFunc: func() (openshift.RoutesAvailability, error) { + return openshift.RoutesAvailable, nil }, } cfg := config.New( config.WithAutoDetect(mock), - config.WithOnOpenShiftRoutesChangeCallback(func() error { - calledBack = true - return nil - }), ) // sanity check - require.Equal(t, autodetect.OpenShiftRoutesNotAvailable, cfg.OpenShiftRoutes()) + require.Equal(t, openshift.RoutesNotAvailable, cfg.OpenShiftRoutesAvailability()) // test err := cfg.AutoDetect() require.NoError(t, err) // verify - assert.Equal(t, autodetect.OpenShiftRoutesAvailable, cfg.OpenShiftRoutes()) - assert.True(t, calledBack) -} - -func TestAutoDetectInBackground(t *testing.T) { - // prepare - wg := &sync.WaitGroup{} - wg.Add(1) - mock := &mockAutoDetect{ - OpenShiftRoutesAvailabilityFunc: func() (autodetect.OpenShiftRoutesAvailability, error) { - wg.Done() - return autodetect.OpenShiftRoutesNotAvailable, nil - }, - } - cfg := config.New( - config.WithAutoDetect(mock), - config.WithAutoDetectFrequency(500*time.Second), - ) - - // sanity check - require.Equal(t, autodetect.OpenShiftRoutesNotAvailable, cfg.OpenShiftRoutes()) - - // test - err := cfg.StartAutoDetect() - require.NoError(t, err) - - // verify - wg.Wait() + assert.Equal(t, openshift.RoutesAvailable, cfg.OpenShiftRoutesAvailability()) } var _ autodetect.AutoDetect = (*mockAutoDetect)(nil) type mockAutoDetect struct { - OpenShiftRoutesAvailabilityFunc func() (autodetect.OpenShiftRoutesAvailability, error) + OpenShiftRoutesAvailabilityFunc func() (openshift.RoutesAvailability, error) } -func (m *mockAutoDetect) OpenShiftRoutesAvailability() (autodetect.OpenShiftRoutesAvailability, error) { +func (m *mockAutoDetect) OpenShiftRoutesAvailability() (openshift.RoutesAvailability, error) { if m.OpenShiftRoutesAvailabilityFunc != nil { return m.OpenShiftRoutesAvailabilityFunc() } - return autodetect.OpenShiftRoutesNotAvailable, nil + return openshift.RoutesNotAvailable, nil } diff --git a/internal/config/options.go b/internal/config/options.go index 5d172ccd60..225692cec4 100644 --- a/internal/config/options.go +++ b/internal/config/options.go @@ -17,12 +17,12 @@ package config import ( "regexp" "strings" - "time" "github.com/go-logr/logr" + "github.com/open-telemetry/opentelemetry-operator/internal/autodetect" + "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/openshift" "github.com/open-telemetry/opentelemetry-operator/internal/version" - "github.com/open-telemetry/opentelemetry-operator/pkg/autodetect" ) // Option represents one specific configuration option. @@ -45,10 +45,8 @@ type options struct { operatorOpAMPBridgeConfigMapEntry string targetAllocatorImage string operatorOpAMPBridgeImage string - onOpenShiftRoutesChange changeHandler + openshiftRoutesAvailability openshift.RoutesAvailability labelsFilter []string - openshiftRoutes openshiftRoutesStore - autoDetectFrequency time.Duration } func WithAutoDetect(a autodetect.AutoDetect) Option { @@ -56,11 +54,6 @@ func WithAutoDetect(a autodetect.AutoDetect) Option { o.autoDetect = a } } -func WithAutoDetectFrequency(t time.Duration) Option { - return func(o *options) { - o.autoDetectFrequency = t - } -} func WithTargetAllocatorImage(s string) Option { return func(o *options) { o.targetAllocatorImage = s @@ -96,20 +89,6 @@ func WithLogger(logger logr.Logger) Option { o.logger = logger } } - -func WithOnOpenShiftRoutesChangeCallback(f func() error) Option { - return func(o *options) { - if o.onOpenShiftRoutesChange == nil { - o.onOpenShiftRoutesChange = newOnChange() - } - o.onOpenShiftRoutesChange.Register(f) - } -} -func WithPlatform(ora autodetect.OpenShiftRoutesAvailability) Option { - return func(o *options) { - o.openshiftRoutes.Set(ora) - } -} func WithVersion(v version.Version) Option { return func(o *options) { o.version = v @@ -158,6 +137,12 @@ func WithAutoInstrumentationNginxImage(s string) Option { } } +func WithOpenShiftRoutesAvailability(os openshift.RoutesAvailability) Option { + return func(o *options) { + o.openshiftRoutesAvailability = os + } +} + func WithLabelFilters(labelFilters []string) Option { return func(o *options) { diff --git a/internal/manifests/collector/route.go b/internal/manifests/collector/route.go index a2e0325727..31901cd55c 100644 --- a/internal/manifests/collector/route.go +++ b/internal/manifests/collector/route.go @@ -22,12 +22,13 @@ import ( "k8s.io/apimachinery/pkg/util/intstr" "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" + "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/openshift" "github.com/open-telemetry/opentelemetry-operator/internal/manifests" "github.com/open-telemetry/opentelemetry-operator/internal/naming" ) func Routes(params manifests.Params) []*routev1.Route { - if params.OtelCol.Spec.Ingress.Type != v1alpha1.IngressTypeRoute { + if params.OtelCol.Spec.Ingress.Type != v1alpha1.IngressTypeRoute || params.Config.OpenShiftRoutesAvailability() != openshift.RoutesAvailable { return nil } diff --git a/internal/manifests/collector/suite_test.go b/internal/manifests/collector/suite_test.go index 7b6665aa46..fd5e275856 100644 --- a/internal/manifests/collector/suite_test.go +++ b/internal/manifests/collector/suite_test.go @@ -26,6 +26,7 @@ import ( logf "sigs.k8s.io/controller-runtime/pkg/log" "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" + "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/openshift" "github.com/open-telemetry/opentelemetry-operator/internal/config" "github.com/open-telemetry/opentelemetry-operator/internal/manifests" ) @@ -97,7 +98,11 @@ func newParams(taContainerImage string, file string) (manifests.Params, error) { return manifests.Params{}, fmt.Errorf("Error getting yaml file: %w", err) } - cfg := config.New(config.WithCollectorImage(defaultCollectorImage), config.WithTargetAllocatorImage(defaultTaAllocationImage)) + cfg := config.New( + config.WithCollectorImage(defaultCollectorImage), + config.WithTargetAllocatorImage(defaultTaAllocationImage), + config.WithOpenShiftRoutesAvailability(openshift.RoutesAvailable), + ) return manifests.Params{ Config: cfg, diff --git a/main.go b/main.go index 222a9d5253..11dd91cb13 100644 --- a/main.go +++ b/main.go @@ -45,10 +45,10 @@ import ( otelv1alpha1 "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" "github.com/open-telemetry/opentelemetry-operator/controllers" + "github.com/open-telemetry/opentelemetry-operator/internal/autodetect" "github.com/open-telemetry/opentelemetry-operator/internal/config" "github.com/open-telemetry/opentelemetry-operator/internal/version" "github.com/open-telemetry/opentelemetry-operator/internal/webhook/podmutation" - "github.com/open-telemetry/opentelemetry-operator/pkg/autodetect" collectorupgrade "github.com/open-telemetry/opentelemetry-operator/pkg/collector/upgrade" "github.com/open-telemetry/opentelemetry-operator/pkg/featuregate" "github.com/open-telemetry/opentelemetry-operator/pkg/instrumentation" @@ -186,6 +186,10 @@ func main() { config.WithAutoDetect(ad), config.WithLabelFilters(labelsFilter), ) + err = cfg.AutoDetect() + if err != nil { + setupLog.Error(err, "failed to autodetect config variables") + } watchNamespace, found := os.LookupEnv("WATCH_NAMESPACE") if found { @@ -310,15 +314,8 @@ func main() { } func addDependencies(_ context.Context, mgr ctrl.Manager, cfg config.Config, v version.Version) error { - // run the auto-detect mechanism for the configuration - err := mgr.Add(manager.RunnableFunc(func(_ context.Context) error { - return cfg.StartAutoDetect() - })) - if err != nil { - return fmt.Errorf("failed to start the auto-detect mechanism: %w", err) - } // adds the upgrade mechanism to be executed once the manager is ready - err = mgr.Add(manager.RunnableFunc(func(c context.Context) error { + err := mgr.Add(manager.RunnableFunc(func(c context.Context) error { up := &collectorupgrade.VersionUpgrade{ Log: ctrl.Log.WithName("collector-upgrade"), Version: v, diff --git a/pkg/autodetect/openshiftroutes.go b/pkg/autodetect/openshiftroutes.go deleted file mode 100644 index 19116a003e..0000000000 --- a/pkg/autodetect/openshiftroutes.go +++ /dev/null @@ -1,31 +0,0 @@ -// Copyright The OpenTelemetry Authors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -// Package autodetect is for auto-detecting traits from the environment (APIs, ...). -package autodetect - -// OpenShiftRoutesAvailability holds the auto-detected OpenShift Routes availability API. -type OpenShiftRoutesAvailability int - -const ( - // OpenShiftRoutesAvailable represents the route.openshift.io API is available. - OpenShiftRoutesAvailable OpenShiftRoutesAvailability = iota - - // OpenShiftRoutesNotAvailable represents the route.openshift.io API is not available. - OpenShiftRoutesNotAvailable -) - -func (p OpenShiftRoutesAvailability) String() string { - return [...]string{"Available", "NotAvailable"}[p] -} diff --git a/pkg/collector/reconcile/route.go b/pkg/collector/reconcile/route.go deleted file mode 100644 index a7cf081f7a..0000000000 --- a/pkg/collector/reconcile/route.go +++ /dev/null @@ -1,152 +0,0 @@ -// Copyright The OpenTelemetry Authors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package reconcile - -import ( - "context" - "fmt" - - "github.com/open-telemetry/opentelemetry-operator/internal/manifests" - "github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector" - - routev1 "github.com/openshift/api/route/v1" - k8serrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/types" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" - - "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" -) - -// Routes reconciles the route(s) required for the instance in the current context. -// TODO: This functionality should be put with the rest of reconciliation logic in the mutate.go -// https://github.com/open-telemetry/opentelemetry-operator/issues/2108 -func Routes(ctx context.Context, params manifests.Params) error { - if params.OtelCol.Spec.Ingress.Type != v1alpha1.IngressTypeRoute { - return nil - } - - isSupportedMode := true - if params.OtelCol.Spec.Mode == v1alpha1.ModeSidecar { - params.Log.V(3).Info("ingress settings are not supported in sidecar mode") - isSupportedMode = false - } - - var desired []*routev1.Route - if isSupportedMode { - if r := collector.Routes(params); r != nil { - desired = append(desired, r...) - } - } - - // first, handle the create/update parts - if err := expectedRoutes(ctx, params, desired); err != nil { - return fmt.Errorf("failed to reconcile the expected routes: %w", err) - } - - // then, delete the extra objects - if err := deleteRoutes(ctx, params, desired); err != nil { - return fmt.Errorf("failed to reconcile the routes to be deleted: %w", err) - } - - return nil -} - -func expectedRoutes(ctx context.Context, params manifests.Params, expected []*routev1.Route) error { - for _, obj := range expected { - desired := obj - - if err := controllerutil.SetControllerReference(¶ms.OtelCol, desired, params.Scheme); err != nil { - return fmt.Errorf("failed to set controller reference: %w", err) - } - - existing := &routev1.Route{} - 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) - } - params.Log.V(2).Info("created", "route.name", desired.Name, "route.namespace", desired.Namespace) - continue - } else if err != nil { - return fmt.Errorf("failed to get: %w", err) - } - - // it exists already, merge the two if the end result isn't identical to the existing one - updated := existing.DeepCopy() - if updated.Annotations == nil { - updated.Annotations = map[string]string{} - } - if updated.Labels == nil { - updated.Labels = map[string]string{} - } - updated.ObjectMeta.OwnerReferences = desired.ObjectMeta.OwnerReferences - updated.Spec.To = desired.Spec.To - updated.Spec.TLS = desired.Spec.TLS - updated.Spec.Port = desired.Spec.Port - updated.Spec.WildcardPolicy = desired.Spec.WildcardPolicy - - for k, v := range desired.ObjectMeta.Annotations { - updated.ObjectMeta.Annotations[k] = v - } - for k, v := range desired.ObjectMeta.Labels { - updated.ObjectMeta.Labels[k] = v - } - - patch := client.MergeFrom(existing) - - if err := params.Client.Patch(ctx, updated, patch); err != nil { - return fmt.Errorf("failed to apply changes: %w", err) - } - - params.Log.V(2).Info("applied", "route.name", desired.Name, "route.namespace", desired.Namespace) - } - return nil -} - -func deleteRoutes(ctx context.Context, params manifests.Params, expected []*routev1.Route) error { - opts := []client.ListOption{ - client.InNamespace(params.OtelCol.Namespace), - client.MatchingLabels(map[string]string{ - "app.kubernetes.io/instance": fmt.Sprintf("%s.%s", params.OtelCol.Namespace, params.OtelCol.Name), - "app.kubernetes.io/managed-by": "opentelemetry-operator", - }), - } - list := &routev1.RouteList{} - if err := params.Client.List(ctx, list, opts...); err != nil { - return fmt.Errorf("failed to list: %w", err) - } - - for i := range list.Items { - existing := list.Items[i] - del := true - for _, keep := range expected { - if keep.Name == existing.Name && keep.Namespace == existing.Namespace { - del = false - break - } - } - - if del { - if err := params.Client.Delete(ctx, &existing); err != nil { - return fmt.Errorf("failed to delete: %w", err) - } - params.Log.V(2).Info("deleted", "route.name", existing.Name, "route.namespace", existing.Namespace) - } - } - - return nil -} diff --git a/pkg/collector/reconcile/route_test.go b/pkg/collector/reconcile/route_test.go deleted file mode 100644 index 3d305ee2bb..0000000000 --- a/pkg/collector/reconcile/route_test.go +++ /dev/null @@ -1,106 +0,0 @@ -// Copyright The OpenTelemetry Authors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package reconcile - -import ( - "context" - _ "embed" - "strings" - "testing" - - "github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector" - - routev1 "github.com/openshift/api/route/v1" - "github.com/stretchr/testify/assert" - "k8s.io/apimachinery/pkg/types" - - "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" -) - -func TestExpectedRoutes(t *testing.T) { - t.Run("should create and update route entry", func(t *testing.T) { - ctx := context.Background() - - params, err := newParams("something:tag", testFileIngress) - if err != nil { - t.Fatal(err) - } - params.OtelCol.Spec.Ingress.Type = v1alpha1.IngressTypeRoute - params.OtelCol.Spec.Ingress.Route.Termination = v1alpha1.TLSRouteTerminationTypeInsecure - - routes := collector.Routes(params) - err = expectedRoutes(ctx, params, routes) - assert.NoError(t, err) - - nns := types.NamespacedName{Namespace: params.OtelCol.Namespace, Name: "otlp-grpc-test-route"} - exists, err := populateObjectIfExists(t, &routev1.Route{}, nns) - assert.NoError(t, err) - assert.True(t, exists) - - // update fields - const expectHostname = "something-else.com" - params.OtelCol.Spec.Ingress.Annotations = map[string]string{"blub": "blob"} - params.OtelCol.Spec.Ingress.Hostname = expectHostname - - routes = collector.Routes(params) - err = expectedRoutes(ctx, params, routes) - assert.NoError(t, err) - - got := &routev1.Route{} - err = params.Client.Get(ctx, nns, got) - assert.NoError(t, err) - - gotHostname := got.Spec.Host - if !strings.Contains(gotHostname, got.Spec.Host) { - t.Errorf("host name is not up-to-date. expect: %s, got: %s", expectHostname, gotHostname) - } - - if v, ok := got.Annotations["blub"]; !ok || v != "blob" { - t.Error("annotations are not up-to-date. Missing entry or value is invalid.") - } - }) -} - -func TestDeleteRoutes(t *testing.T) { - t.Run("should delete excess routes", func(t *testing.T) { - // create - ctx := context.Background() - - myParams, err := newParams("something:tag", testFileIngress) - if err != nil { - t.Fatal(err) - } - myParams.OtelCol.Spec.Ingress.Type = v1alpha1.IngressTypeRoute - - routes := collector.Routes(myParams) - err = expectedRoutes(ctx, myParams, routes) - assert.NoError(t, err) - - nns := types.NamespacedName{Namespace: "default", Name: "otlp-grpc-test-route"} - exists, err := populateObjectIfExists(t, &routev1.Route{}, nns) - assert.NoError(t, err) - assert.True(t, exists) - - // delete - if err = deleteRoutes(ctx, params(), []*routev1.Route{}); err != nil { - t.Error(err) - } - - // check - exists, err = populateObjectIfExists(t, &routev1.Route{}, nns) - assert.NoError(t, err) - assert.False(t, exists) - }) -} diff --git a/pkg/collector/reconcile/suit_test.go b/pkg/collector/reconcile/suit_test.go deleted file mode 100644 index cfe28fc28a..0000000000 --- a/pkg/collector/reconcile/suit_test.go +++ /dev/null @@ -1,301 +0,0 @@ -// Copyright The OpenTelemetry Authors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -// TODO: This file can be deleted when Routes is removed from this package. -// https://github.com/open-telemetry/opentelemetry-operator/issues/2108 -package reconcile - -import ( - "context" - "crypto/tls" - "fmt" - "net" - "os" - "path/filepath" - "sync" - "testing" - "time" - - routev1 "github.com/openshift/api/route/v1" - monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1" - v1 "k8s.io/api/core/v1" - apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" - "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/types" - "k8s.io/apimachinery/pkg/util/intstr" - "k8s.io/apimachinery/pkg/util/uuid" - "k8s.io/apimachinery/pkg/util/wait" - "k8s.io/client-go/kubernetes/scheme" - "k8s.io/client-go/tools/record" - "k8s.io/client-go/util/retry" - ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/envtest" - logf "sigs.k8s.io/controller-runtime/pkg/log" - metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" - "sigs.k8s.io/controller-runtime/pkg/webhook" - - "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" - "github.com/open-telemetry/opentelemetry-operator/internal/config" - "github.com/open-telemetry/opentelemetry-operator/internal/manifests" - "github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector/testdata" -) - -var ( - k8sClient client.Client - testEnv *envtest.Environment - testScheme *runtime.Scheme = scheme.Scheme - ctx context.Context - cancel context.CancelFunc - - logger = logf.Log.WithName("unit-tests") - - instanceUID = uuid.NewUUID() -) - -const ( - defaultCollectorImage = "default-collector" - defaultTaAllocationImage = "default-ta-allocator" - testFileIngress = "testdata/ingress_testdata.yaml" -) - -func TestMain(m *testing.M) { - ctx, cancel = context.WithCancel(context.TODO()) - defer cancel() - - testEnv = &envtest.Environment{ - CRDDirectoryPaths: []string{filepath.Join("..", "..", "..", "config", "crd", "bases")}, - CRDInstallOptions: envtest.CRDInstallOptions{ - CRDs: []*apiextensionsv1.CustomResourceDefinition{ - testdata.OpenShiftRouteCRD, - testdata.ServiceMonitorCRD, - }, - }, - WebhookInstallOptions: envtest.WebhookInstallOptions{ - Paths: []string{filepath.Join("..", "..", "..", "config", "webhook")}, - }, - } - cfg, err := testEnv.Start() - if err != nil { - fmt.Printf("failed to start testEnv: %v", err) - os.Exit(1) - } - - if err = routev1.AddToScheme(testScheme); err != nil { - fmt.Printf("failed to register scheme: %v", err) - os.Exit(1) - } - - if err = v1alpha1.AddToScheme(testScheme); err != nil { - fmt.Printf("failed to register scheme: %v", err) - os.Exit(1) - } - - if err = monitoringv1.AddToScheme(testScheme); err != nil { - fmt.Printf("failed to register scheme: %v", err) - os.Exit(1) - } - // +kubebuilder:scaffold:scheme - - k8sClient, err = client.New(cfg, client.Options{Scheme: testScheme}) - if err != nil { - fmt.Printf("failed to setup a Kubernetes client: %v", err) - os.Exit(1) - } - - // start webhook server using Manager - webhookInstallOptions := &testEnv.WebhookInstallOptions - mgr, mgrErr := ctrl.NewManager(cfg, ctrl.Options{ - Scheme: testScheme, - LeaderElection: false, - WebhookServer: webhook.NewServer(webhook.Options{ - Host: webhookInstallOptions.LocalServingHost, - Port: webhookInstallOptions.LocalServingPort, - CertDir: webhookInstallOptions.LocalServingCertDir, - }), - Metrics: metricsserver.Options{ - BindAddress: "0", - }, - }) - if mgrErr != nil { - fmt.Printf("failed to start webhook server: %v", mgrErr) - os.Exit(1) - } - - if err = v1alpha1.SetupCollectorWebhook(mgr, config.New()); err != nil { - fmt.Printf("failed to SetupWebhookWithManager: %v", err) - os.Exit(1) - } - - ctx, cancel = context.WithCancel(context.TODO()) - defer cancel() - go func() { - if err = mgr.Start(ctx); err != nil { - fmt.Printf("failed to start manager: %v", err) - os.Exit(1) - } - }() - - // wait for the webhook server to get ready - wg := &sync.WaitGroup{} - wg.Add(1) - dialer := &net.Dialer{Timeout: time.Second} - addrPort := fmt.Sprintf("%s:%d", webhookInstallOptions.LocalServingHost, webhookInstallOptions.LocalServingPort) - go func(wg *sync.WaitGroup) { - defer wg.Done() - if err = retry.OnError(wait.Backoff{ - Steps: 20, - Duration: 10 * time.Millisecond, - Factor: 1.5, - Jitter: 0.1, - Cap: time.Second * 30, - }, func(error) bool { - return true - }, func() error { - // #nosec G402 - conn, tlsErr := tls.DialWithDialer(dialer, "tcp", addrPort, &tls.Config{InsecureSkipVerify: true}) - if tlsErr != nil { - return tlsErr - } - _ = conn.Close() - return nil - }); err != nil { - fmt.Printf("failed to wait for webhook server to be ready: %v", err) - os.Exit(1) - } - }(wg) - wg.Wait() - - code := m.Run() - - err = testEnv.Stop() - if err != nil { - fmt.Printf("failed to stop testEnv: %v", err) - os.Exit(1) - } - - os.Exit(code) -} - -func params() manifests.Params { - return paramsWithMode(v1alpha1.ModeDeployment) -} - -func paramsWithMode(mode v1alpha1.Mode) manifests.Params { - replicas := int32(2) - configYAML, err := os.ReadFile("testdata/test.yaml") - if err != nil { - fmt.Printf("Error getting yaml file: %v", err) - } - return manifests.Params{ - Config: config.New(config.WithCollectorImage(defaultCollectorImage), config.WithTargetAllocatorImage(defaultTaAllocationImage)), - Client: k8sClient, - OtelCol: v1alpha1.OpenTelemetryCollector{ - TypeMeta: metav1.TypeMeta{ - Kind: "opentelemetry.io", - APIVersion: "v1", - }, - ObjectMeta: metav1.ObjectMeta{ - Name: "test", - Namespace: "default", - UID: instanceUID, - }, - Spec: v1alpha1.OpenTelemetryCollectorSpec{ - Image: "ghcr.io/open-telemetry/opentelemetry-operator/opentelemetry-operator:0.47.0", - Ports: []v1.ServicePort{{ - Name: "web", - Port: 80, - TargetPort: intstr.IntOrString{ - Type: intstr.Int, - IntVal: 80, - }, - NodePort: 0, - }}, - Replicas: &replicas, - Config: string(configYAML), - Mode: mode, - }, - }, - Scheme: testScheme, - Log: logger, - Recorder: record.NewFakeRecorder(10), - } -} - -func newParams(taContainerImage string, file string) (manifests.Params, error) { - replicas := int32(1) - var configYAML []byte - var err error - - if file == "" { - configYAML, err = os.ReadFile("testdata/test.yaml") - } else { - configYAML, err = os.ReadFile(file) - } - if err != nil { - return manifests.Params{}, fmt.Errorf("Error getting yaml file: %w", err) - } - - cfg := config.New(config.WithCollectorImage(defaultCollectorImage), config.WithTargetAllocatorImage(defaultTaAllocationImage)) - - return manifests.Params{ - Config: cfg, - Client: k8sClient, - OtelCol: v1alpha1.OpenTelemetryCollector{ - TypeMeta: metav1.TypeMeta{ - Kind: "opentelemetry.io", - APIVersion: "v1", - }, - ObjectMeta: metav1.ObjectMeta{ - Name: "test", - Namespace: "default", - UID: instanceUID, - }, - Spec: v1alpha1.OpenTelemetryCollectorSpec{ - Mode: v1alpha1.ModeStatefulSet, - Ports: []v1.ServicePort{{ - Name: "web", - Port: 80, - TargetPort: intstr.IntOrString{ - Type: intstr.Int, - IntVal: 80, - }, - NodePort: 0, - }}, - TargetAllocator: v1alpha1.OpenTelemetryTargetAllocator{ - Enabled: true, - Image: taContainerImage, - }, - Replicas: &replicas, - Config: string(configYAML), - }, - }, - Scheme: testScheme, - Log: logger, - }, nil -} - -func populateObjectIfExists(t testing.TB, object client.Object, namespacedName types.NamespacedName) (bool, error) { - t.Helper() - err := k8sClient.Get(context.Background(), namespacedName, object) - if errors.IsNotFound(err) { - return false, nil - } - if err != nil { - return false, err - } - return true, nil -} diff --git a/pkg/collector/reconcile/testdata/ingress_testdata.yaml b/pkg/collector/reconcile/testdata/ingress_testdata.yaml deleted file mode 100644 index 54f9342a76..0000000000 --- a/pkg/collector/reconcile/testdata/ingress_testdata.yaml +++ /dev/null @@ -1,16 +0,0 @@ ---- -receivers: - otlp: - protocols: - grpc: - endpoint: 0.0.0.0:12345 - otlp/test: - protocols: - grpc: - endpoint: 0.0.0.0:98765 - -service: - pipelines: - traces: - receivers: [otlp, otlp/test] - exporters: [nop] diff --git a/pkg/collector/reconcile/testdata/test.yaml b/pkg/collector/reconcile/testdata/test.yaml deleted file mode 100644 index c79ab5288b..0000000000 --- a/pkg/collector/reconcile/testdata/test.yaml +++ /dev/null @@ -1,22 +0,0 @@ -processors: -receivers: - jaeger: - protocols: - grpc: - prometheus: - config: - scrape_configs: - - job_name: otel-collector - scrape_interval: 10s - static_configs: - - targets: [ '0.0.0.0:8888', '0.0.0.0:9999' ] - -exporters: - debug: - -service: - pipelines: - metrics: - receivers: [prometheus, jaeger] - processors: [] - exporters: [debug] \ No newline at end of file From 2b5e6a88f87566c55c5f7a7abb838648ea6c76e0 Mon Sep 17 00:00:00 2001 From: Jacob Aronoff Date: Tue, 28 Nov 2023 15:11:26 -0500 Subject: [PATCH 2/2] remove unused --- internal/config/main.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/internal/config/main.go b/internal/config/main.go index 414c58d165..8c16226d6a 100644 --- a/internal/config/main.go +++ b/internal/config/main.go @@ -90,12 +90,6 @@ func New(opts ...Option) Config { } } -// StartAutoDetect attempts to automatically detect relevant information for this operator. This will block until the first -// run is executed and will schedule periodic updates. -func (c *Config) StartAutoDetect() error { - return c.AutoDetect() -} - // AutoDetect attempts to automatically detect relevant information for this operator. func (c *Config) AutoDetect() error { c.logger.V(2).Info("auto-detecting the configuration based on the environment")