[chore] Refactor Manifest Generation to internal package (#1965)

* parent 2998ccc
author Jacob Aronoff <[email protected]> 1690402542 -0400
committer Jacob Aronoff <[email protected]> 1691001855 -0400

Merge upstream, squash to main

* Fix missing import

* fix whoopsie

apis/v1alpha1/opentelemetrycollector_webhook.go

@@ -27,8 +27,8 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 
+	ta "github.com/open-telemetry/opentelemetry-operator/internal/manifests/targetallocator/adapters"
 	"github.com/open-telemetry/opentelemetry-operator/pkg/featuregate"
-	ta "github.com/open-telemetry/opentelemetry-operator/pkg/targetallocator/adapters"
 )
 
 // log is for logging in this package.

controllers/opentelemetrycollector_controller.go

@@ -34,6 +34,7 @@ import (
 
 	"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/pkg/autodetect"
 	"github.com/open-telemetry/opentelemetry-operator/pkg/collector/reconcile"
 	"github.com/open-telemetry/opentelemetry-operator/pkg/featuregate"
@@ -53,7 +54,7 @@ type OpenTelemetryCollectorReconciler struct {
 
 // Task represents a reconciliation task to be executed by the reconciler.
 type Task struct {
-	Do          func(context.Context, reconcile.Params) error
+	Do          func(context.Context, manifests.Params) error
 	Name        string
 	BailOnError bool
 }
@@ -212,7 +213,7 @@ func (r *OpenTelemetryCollectorReconciler) Reconcile(ctx context.Context, req ct
 		return ctrl.Result{}, nil
 	}
 
-	params := reconcile.Params{
+	params := manifests.Params{
 		Config:   r.config,
 		Client:   r.Client,
 		Instance: instance,
@@ -229,7 +230,7 @@ func (r *OpenTelemetryCollectorReconciler) Reconcile(ctx context.Context, req ct
 }
 
 // RunTasks runs all the tasks associated with this reconciler.
-func (r *OpenTelemetryCollectorReconciler) RunTasks(ctx context.Context, params reconcile.Params) error {
+func (r *OpenTelemetryCollectorReconciler) RunTasks(ctx context.Context, params manifests.Params) error {
 	r.muTasks.RLock()
 	defer r.muTasks.RUnlock()
 	for _, task := range r.tasks {

controllers/opentelemetrycollector_controller_test.go

@@ -37,8 +37,8 @@ import (
 	"github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1"
 	"github.com/open-telemetry/opentelemetry-operator/controllers"
 	"github.com/open-telemetry/opentelemetry-operator/internal/config"
+	"github.com/open-telemetry/opentelemetry-operator/internal/manifests"
 	"github.com/open-telemetry/opentelemetry-operator/pkg/autodetect"
-	"github.com/open-telemetry/opentelemetry-operator/pkg/collector/reconcile"
 )
 
 var logger = logf.Log.WithName("unit-tests")
@@ -254,14 +254,14 @@ func TestContinueOnRecoverableFailure(t *testing.T) {
 		Tasks: []controllers.Task{
 			{
 				Name: "should-fail",
-				Do: func(context.Context, reconcile.Params) error {
+				Do: func(context.Context, manifests.Params) error {
 					return errors.New("should fail")
 				},
 				BailOnError: false,
 			},
 			{
 				Name: "should-be-called",
-				Do: func(context.Context, reconcile.Params) error {
+				Do: func(context.Context, manifests.Params) error {
 					taskCalled = true
 					return nil
 				},
@@ -270,7 +270,7 @@ func TestContinueOnRecoverableFailure(t *testing.T) {
 	})
 
 	// test
-	err := reconciler.RunTasks(context.Background(), reconcile.Params{})
+	err := reconciler.RunTasks(context.Background(), manifests.Params{})
 
 	// verify
 	assert.NoError(t, err)
@@ -291,15 +291,15 @@ func TestBreakOnUnrecoverableError(t *testing.T) {
 		Tasks: []controllers.Task{
 			{
 				Name: "should-fail",
-				Do: func(context.Context, reconcile.Params) error {
+				Do: func(context.Context, manifests.Params) error {
 					taskCalled = true
 					return expectedErr
 				},
 				BailOnError: true,
 			},
 			{
 				Name: "should-not-be-called",
-				Do: func(context.Context, reconcile.Params) error {
+				Do: func(context.Context, manifests.Params) error {
 					assert.Fail(t, "should not have been called")
 					return nil
 				},
@@ -341,7 +341,7 @@ func TestSkipWhenInstanceDoesNotExist(t *testing.T) {
 		Tasks: []controllers.Task{
 			{
 				Name: "should-not-be-called",
-				Do: func(context.Context, reconcile.Params) error {
+				Do: func(context.Context, manifests.Params) error {
 					assert.Fail(t, "should not have been called")
 					return nil
 				},

controllers/suite_test.go

@@ -37,7 +37,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/envtest"
 
 	"github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1"
-	"github.com/open-telemetry/opentelemetry-operator/pkg/collector/testdata"
+	"github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector/testdata"
 	// +kubebuilder:scaffold:imports
 )
 

internal/manifests/builder.go

@@ -0,0 +1,41 @@
+// 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 manifests
+
+import (
+	"github.com/go-logr/logr"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+
+	"github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1"
+	"github.com/open-telemetry/opentelemetry-operator/internal/config"
+)
+
+type Builder func(params Params) ([]client.Object, error)
+
+type ManifestFactory[T client.Object] func(cfg config.Config, logger logr.Logger, otelcol v1alpha1.OpenTelemetryCollector) (T, error)
+type SimpleManifestFactory[T client.Object] func(cfg config.Config, logger logr.Logger, otelcol v1alpha1.OpenTelemetryCollector) T
+type K8sManifestFactory ManifestFactory[client.Object]
+
+func FactoryWithoutError[T client.Object](f SimpleManifestFactory[T]) K8sManifestFactory {
+	return func(cfg config.Config, logger logr.Logger, otelcol v1alpha1.OpenTelemetryCollector) (client.Object, error) {
+		return f(cfg, logger, otelcol), nil
+	}
+}
+
+func Factory[T client.Object](f ManifestFactory[T]) K8sManifestFactory {
+	return func(cfg config.Config, logger logr.Logger, otelcol v1alpha1.OpenTelemetryCollector) (client.Object, error) {
+		return f(cfg, logger, otelcol)
+	}
+}

pkg/collector/adapters/config_from_test.go → internal/manifests/collector/adapters/config_from_test.go

@@ -19,7 +19,7 @@ import (
 
 	"github.com/stretchr/testify/assert"
 
-	"github.com/open-telemetry/opentelemetry-operator/pkg/collector/adapters"
+	"github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector/adapters"
 )
 
 func TestInvalidYAML(t *testing.T) {

pkg/collector/adapters/config_to_ports.go → internal/manifests/collector/adapters/config_to_ports.go

@@ -24,7 +24,7 @@ import (
 	"github.com/mitchellh/mapstructure"
 	corev1 "k8s.io/api/core/v1"
 
-	"github.com/open-telemetry/opentelemetry-operator/pkg/collector/parser"
+	"github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector/parser"
 )
 
 var (

pkg/collector/adapters/config_to_ports_test.go → internal/manifests/collector/adapters/config_to_ports_test.go

@@ -25,8 +25,8 @@ import (
 	"k8s.io/apimachinery/pkg/util/intstr"
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
 
-	"github.com/open-telemetry/opentelemetry-operator/pkg/collector/adapters"
-	"github.com/open-telemetry/opentelemetry-operator/pkg/collector/parser"
+	"github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector/adapters"
+	"github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector/parser"
 )
 
 var logger = logf.Log.WithName("unit-tests")

pkg/collector/adapters/config_to_probe.go → internal/manifests/collector/adapters/config_to_probe.go

@@ -31,10 +31,10 @@ var (
 
 	errNoExtensionHealthCheck = errors.New("extensions property in the configuration does not contain the expected health_check extension")
 
-	errNoServiceExtensions = errors.New("service property in the configuration doesn't contain extensions")
+	ErrNoServiceExtensions = errors.New("service property in the configuration doesn't contain extensions")
 
 	errServiceExtensionsNotSlice     = errors.New("service extensions property in the configuration does not contain valid extensions")
-	errNoServiceExtensionHealthCheck = errors.New("no healthcheck extension available in service extension configuration")
+	ErrNoServiceExtensionHealthCheck = errors.New("no healthcheck extension available in service extension configuration")
 )
 
 type probeConfiguration struct {
@@ -60,7 +60,7 @@ func ConfigToContainerProbe(config map[interface{}]interface{}) (*corev1.Probe,
 
 	serviceExtensionsProperty, withExtension := service["extensions"]
 	if !withExtension {
-		return nil, errNoServiceExtensions
+		return nil, ErrNoServiceExtensions
 	}
 
 	serviceExtensions, withExtProperty := serviceExtensionsProperty.([]interface{})
@@ -76,7 +76,7 @@ func ConfigToContainerProbe(config map[interface{}]interface{}) (*corev1.Probe,
 	}
 
 	if len(healthCheckServiceExtensions) == 0 {
-		return nil, errNoServiceExtensionHealthCheck
+		return nil, ErrNoServiceExtensionHealthCheck
 	}
 
 	extensionsProperty, ok := config["extensions"]

pkg/collector/adapters/config_to_probe_test.go → internal/manifests/collector/adapters/config_to_probe_test.go

@@ -146,7 +146,7 @@ service:
 			desc: "NoHealthCheckInServiceExtensions",
 			config: `service:
   extensions: [pprof]`,
-			expectedErr: errNoServiceExtensionHealthCheck,
+			expectedErr: ErrNoServiceExtensionHealthCheck,
 		}, {
 			desc: "BadlyFormattedServiceExtensions",
 			config: `service:
@@ -159,7 +159,7 @@ service:
   pipelines:
     traces:
       receivers: [otlp]`,
-			expectedErr: errNoServiceExtensions,
+			expectedErr: ErrNoServiceExtensions,
 		}, {
 			desc: "BadlyFormattedService",
 			config: `extensions:

internal/manifests/collector/collector.go

@@ -0,0 +1,69 @@
+// 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 collector
+
+import (
+	"sigs.k8s.io/controller-runtime/pkg/client"
+
+	"github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1"
+	"github.com/open-telemetry/opentelemetry-operator/internal/manifests"
+	"github.com/open-telemetry/opentelemetry-operator/pkg/featuregate"
+)
+
+// Build is currently unused, but will be implemented to solve
+// https://github.com/open-telemetry/opentelemetry-operator/issues/1876
+func Build(params manifests.Params) ([]client.Object, error) {
+	var resourceManifests []client.Object
+	var manifestFactories []manifests.K8sManifestFactory
+	switch params.Instance.Spec.Mode {
+	case v1alpha1.ModeDeployment:
+		manifestFactories = append(manifestFactories, manifests.FactoryWithoutError(Deployment))
+	case v1alpha1.ModeStatefulSet:
+		manifestFactories = append(manifestFactories, manifests.FactoryWithoutError(StatefulSet))
+	case v1alpha1.ModeDaemonSet:
+		manifestFactories = append(manifestFactories, manifests.FactoryWithoutError(DaemonSet))
+	case v1alpha1.ModeSidecar:
+		params.Log.V(5).Info("not building sidecar...")
+	}
+	manifestFactories = append(manifestFactories, []manifests.K8sManifestFactory{
+		manifests.FactoryWithoutError(ConfigMap),
+		manifests.FactoryWithoutError(HorizontalPodAutoscaler),
+		manifests.FactoryWithoutError(ServiceAccount),
+		manifests.FactoryWithoutError(Service),
+		manifests.FactoryWithoutError(HeadlessService),
+		manifests.FactoryWithoutError(MonitoringService),
+		manifests.FactoryWithoutError(Ingress),
+	}...)
+	if params.Instance.Spec.Observability.Metrics.EnableMetrics && featuregate.PrometheusOperatorIsAvailable.IsEnabled() {
+		manifestFactories = append(manifestFactories, manifests.Factory(ServiceMonitor))
+	}
+	for _, factory := range manifestFactories {
+		res, err := factory(params.Config, params.Log, params.Instance)
+		if err != nil {
+			return nil, err
+		} else if res != nil {
+			// because of pointer semantics, res is still nil-able here as this is an interface pointer
+			// read here for details:
+			// https://github.com/open-telemetry/opentelemetry-operator/pull/1965#discussion_r1281705719
+			resourceManifests = append(resourceManifests, res)
+		}
+	}
+	routes := Routes(params.Config, params.Log, params.Instance)
+	// NOTE: we cannot just unpack the slice, the type checker doesn't coerce the type correctly.
+	for _, route := range routes {
+		resourceManifests = append(resourceManifests, route)
+	}
+	return resourceManifests, nil
+}

pkg/collector/reconcile/config_replace.go → internal/manifests/collector/config_replace.go

@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package reconcile
+package collector
 
 import (
 	"time"
@@ -22,10 +22,10 @@ import (
 	"gopkg.in/yaml.v2"
 
 	"github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1"
-	"github.com/open-telemetry/opentelemetry-operator/pkg/collector/adapters"
+	"github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector/adapters"
+	ta "github.com/open-telemetry/opentelemetry-operator/internal/manifests/targetallocator/adapters"
+	"github.com/open-telemetry/opentelemetry-operator/internal/naming"
 	"github.com/open-telemetry/opentelemetry-operator/pkg/featuregate"
-	"github.com/open-telemetry/opentelemetry-operator/pkg/naming"
-	ta "github.com/open-telemetry/opentelemetry-operator/pkg/targetallocator/adapters"
 )
 
 type targetAllocator struct {

pkg/collector/reconcile/config_replace_test.go → internal/manifests/collector/config_replace_test.go

@@ -12,24 +12,23 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package reconcile
+package collector
 
 import (
 	"os"
 	"testing"
 
-	colfeaturegate "go.opentelemetry.io/collector/featuregate"
-
 	"github.com/prometheus/prometheus/discovery/http"
 	"github.com/stretchr/testify/assert"
+	colfeaturegate "go.opentelemetry.io/collector/featuregate"
 	"gopkg.in/yaml.v2"
 
+	ta "github.com/open-telemetry/opentelemetry-operator/internal/manifests/targetallocator/adapters"
 	"github.com/open-telemetry/opentelemetry-operator/pkg/featuregate"
-	ta "github.com/open-telemetry/opentelemetry-operator/pkg/targetallocator/adapters"
 )
 
 func TestPrometheusParser(t *testing.T) {
-	param, err := newParams("test/test-img", "../testdata/http_sd_config_test.yaml")
+	param, err := newParams("test/test-img", "testdata/http_sd_config_test.yaml")
 	assert.NoError(t, err)
 
 	t.Run("should update config with http_sd_config", func(t *testing.T) {
@@ -130,12 +129,12 @@ func TestPrometheusParser(t *testing.T) {
 }
 
 func TestReplaceConfig(t *testing.T) {
-	param, err := newParams("test/test-img", "../testdata/relabel_config_original.yaml")
+	param, err := newParams("test/test-img", "testdata/relabel_config_original.yaml")
 	assert.NoError(t, err)
 
 	t.Run("should not modify config when TargetAllocator is disabled", func(t *testing.T) {
 		param.Instance.Spec.TargetAllocator.Enabled = false
-		expectedConfigBytes, err := os.ReadFile("../testdata/relabel_config_original.yaml")
+		expectedConfigBytes, err := os.ReadFile("testdata/relabel_config_original.yaml")
 		assert.NoError(t, err)
 		expectedConfig := string(expectedConfigBytes)
 
@@ -148,7 +147,7 @@ func TestReplaceConfig(t *testing.T) {
 	t.Run("should rewrite scrape configs with SD config when TargetAllocator is enabled and feature flag is not set", func(t *testing.T) {
 		param.Instance.Spec.TargetAllocator.Enabled = true
 
-		expectedConfigBytes, err := os.ReadFile("../testdata/relabel_config_expected_with_sd_config.yaml")
+		expectedConfigBytes, err := os.ReadFile("testdata/relabel_config_expected_with_sd_config.yaml")
 		assert.NoError(t, err)
 		expectedConfig := string(expectedConfigBytes)