Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[chore] Refactor Manifest Generation to internal package #1965

Merged
merged 3 commits into from
Aug 2, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion apis/v1alpha1/opentelemetrycollector_webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
7 changes: 4 additions & 3 deletions controllers/opentelemetrycollector_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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
}
Expand Down Expand Up @@ -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,
Expand All @@ -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 {
Expand Down
14 changes: 7 additions & 7 deletions controllers/opentelemetrycollector_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down Expand Up @@ -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
},
Expand All @@ -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)
Expand All @@ -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
},
Expand Down Expand Up @@ -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
},
Expand Down
2 changes: 1 addition & 1 deletion controllers/suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
)

Expand Down
41 changes: 41 additions & 0 deletions internal/manifests/builder.go
Original file line number Diff line number Diff line change
@@ -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)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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{})
Expand All @@ -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"]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ service:
desc: "NoHealthCheckInServiceExtensions",
config: `service:
extensions: [pprof]`,
expectedErr: errNoServiceExtensionHealthCheck,
expectedErr: ErrNoServiceExtensionHealthCheck,
}, {
desc: "BadlyFormattedServiceExtensions",
config: `service:
Expand All @@ -159,7 +159,7 @@ service:
pipelines:
traces:
receivers: [otlp]`,
expectedErr: errNoServiceExtensions,
expectedErr: ErrNoServiceExtensions,
}, {
desc: "BadlyFormattedService",
config: `extensions:
Expand Down
69 changes: 69 additions & 0 deletions internal/manifests/collector/collector.go
Original file line number Diff line number Diff line change
@@ -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
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.

package reconcile
package collector

import (
"time"
Expand All @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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)

Expand All @@ -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)

Expand Down
Loading
Loading