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

Prepare Collector reconciliation for TargetAllocator CRD #3065

Merged
merged 1 commit into from
Jun 24, 2024
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 controllers/builder_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1988,7 +1988,7 @@ prometheus_cr:
}
targetAllocator, err := collector.TargetAllocator(params)
require.NoError(t, err)
params.TargetAllocator = *targetAllocator
params.TargetAllocator = targetAllocator
if len(tt.featuregates) > 0 {
fg := strings.Join(tt.featuregates, ",")
flagset := featuregate.Flags(colfeaturegate.GlobalRegistry())
Expand Down
5 changes: 3 additions & 2 deletions controllers/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,15 +59,16 @@ func BuildCollector(params manifests.Params) ([]client.Object, error) {
}
resources = append(resources, objs...)
}
if params.OtelCol.Spec.TargetAllocator.Enabled {
// TODO: Remove this after TargetAllocator CRD is reconciled
if params.TargetAllocator != nil {
taParams := targetallocator.Params{
Client: params.Client,
Scheme: params.Scheme,
Recorder: params.Recorder,
Log: params.Log,
Config: params.Config,
Collector: params.OtelCol,
TargetAllocator: params.TargetAllocator,
TargetAllocator: *params.TargetAllocator,
}
taResources, err := BuildTargetAllocator(taParams)
if err != nil {
Expand Down
4 changes: 1 addition & 3 deletions controllers/opentelemetrycollector_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -183,9 +183,7 @@ func (r *OpenTelemetryCollectorReconciler) getParams(instance v1beta1.OpenTeleme
if err != nil {
return p, err
}
if targetAllocator != nil {
p.TargetAllocator = *targetAllocator
}
p.TargetAllocator = targetAllocator
return p, nil
}

Expand Down
15 changes: 9 additions & 6 deletions internal/manifests/collector/config_replace.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
_ "github.com/prometheus/prometheus/discovery/install" // Package install has the side-effect of registering all builtin.
"gopkg.in/yaml.v3"

"github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1"
"github.com/open-telemetry/opentelemetry-operator/apis/v1beta1"
"github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector/adapters"
ta "github.com/open-telemetry/opentelemetry-operator/internal/manifests/targetallocator/adapters"
Expand All @@ -41,13 +42,15 @@ type Config struct {
TargetAllocConfig *targetAllocator `yaml:"target_allocator,omitempty"`
}

func ReplaceConfig(instance v1beta1.OpenTelemetryCollector) (string, error) {
cfgStr, err := instance.Spec.Config.Yaml()
func ReplaceConfig(otelcol v1beta1.OpenTelemetryCollector, targetAllocator *v1alpha1.TargetAllocator) (string, error) {
collectorSpec := otelcol.Spec
taEnabled := targetAllocator != nil
cfgStr, err := collectorSpec.Config.Yaml()
if err != nil {
return "", err
}
// Check if TargetAllocator is enabled, if not, return the original config
if !instance.Spec.TargetAllocator.Enabled {
// Check if TargetAllocator is present, if not, return the original config
if !taEnabled {
return cfgStr, nil
}

Expand All @@ -61,14 +64,14 @@ func ReplaceConfig(instance v1beta1.OpenTelemetryCollector) (string, error) {
return "", getCfgPromErr
}

validateCfgPromErr := ta.ValidatePromConfig(promCfgMap, instance.Spec.TargetAllocator.Enabled)
validateCfgPromErr := ta.ValidatePromConfig(promCfgMap, taEnabled)
if validateCfgPromErr != nil {
return "", validateCfgPromErr
}

// To avoid issues caused by Prometheus validation logic, which fails regex validation when it encounters
// $$ in the prom config, we update the YAML file directly without marshaling and unmarshalling.
updPromCfgMap, getCfgPromErr := ta.AddTAConfigToPromConfig(promCfgMap, naming.TAService(instance.Name))
updPromCfgMap, getCfgPromErr := ta.AddTAConfigToPromConfig(promCfgMap, naming.TAService(targetAllocator.Name))
if getCfgPromErr != nil {
return "", getCfgPromErr
}
Expand Down
17 changes: 5 additions & 12 deletions internal/manifests/collector/config_replace_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,7 @@ func TestPrometheusParser(t *testing.T) {

t.Run("should update config with targetAllocator block if block not present", func(t *testing.T) {
// Set up the test scenario
param.OtelCol.Spec.TargetAllocator.Enabled = true
actualConfig, err := ReplaceConfig(param.OtelCol)
actualConfig, err := ReplaceConfig(param.OtelCol, param.TargetAllocator)
assert.NoError(t, err)

// Verify the expected changes in the config
Expand All @@ -57,9 +56,7 @@ func TestPrometheusParser(t *testing.T) {
paramTa, err := newParams("test/test-img", "testdata/http_sd_config_ta_test.yaml")
require.NoError(t, err)

paramTa.OtelCol.Spec.TargetAllocator.Enabled = true

actualConfig, err := ReplaceConfig(paramTa.OtelCol)
actualConfig, err := ReplaceConfig(paramTa.OtelCol, param.TargetAllocator)
assert.NoError(t, err)

// Verify the expected changes in the config
Expand All @@ -80,9 +77,7 @@ func TestPrometheusParser(t *testing.T) {
})

t.Run("should not update config with http_sd_config", func(t *testing.T) {
param.OtelCol.Spec.TargetAllocator.Enabled = false

actualConfig, err := ReplaceConfig(param.OtelCol)
actualConfig, err := ReplaceConfig(param.OtelCol, nil)
assert.NoError(t, err)

// prepare
Expand Down Expand Up @@ -120,25 +115,23 @@ func TestReplaceConfig(t *testing.T) {
assert.NoError(t, err)

t.Run("should not modify config when TargetAllocator is disabled", func(t *testing.T) {
param.OtelCol.Spec.TargetAllocator.Enabled = false
expectedConfigBytes, err := os.ReadFile("testdata/relabel_config_original.yaml")
assert.NoError(t, err)
expectedConfig := string(expectedConfigBytes)

actualConfig, err := ReplaceConfig(param.OtelCol)
actualConfig, err := ReplaceConfig(param.OtelCol, nil)
assert.NoError(t, err)

assert.YAMLEq(t, expectedConfig, actualConfig)
})

t.Run("should remove scrape configs if TargetAllocator is enabled", func(t *testing.T) {
param.OtelCol.Spec.TargetAllocator.Enabled = true

expectedConfigBytes, err := os.ReadFile("testdata/config_expected_targetallocator.yaml")
assert.NoError(t, err)
expectedConfig := string(expectedConfigBytes)

actualConfig, err := ReplaceConfig(param.OtelCol)
actualConfig, err := ReplaceConfig(param.OtelCol, param.TargetAllocator)
assert.NoError(t, err)

assert.YAMLEq(t, expectedConfig, actualConfig)
Expand Down
2 changes: 1 addition & 1 deletion internal/manifests/collector/configmap.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ func ConfigMap(params manifests.Params) (*corev1.ConfigMap, error) {
collectorName := naming.Collector(params.OtelCol.Name)
labels := manifestutils.Labels(params.OtelCol.ObjectMeta, collectorName, params.OtelCol.Spec.Image, ComponentOpenTelemetryCollector, []string{})

replacedConf, err := ReplaceConfig(params.OtelCol)
replacedConf, err := ReplaceConfig(params.OtelCol, params.TargetAllocator)
if err != nil {
params.Log.V(2).Info("failed to update prometheus config to use sharded targets: ", "err", err)
return nil, err
Expand Down
9 changes: 7 additions & 2 deletions internal/manifests/collector/suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ func newParams(taContainerImage string, file string, options ...config.Option) (
}
cfg := config.New(append(defaultOptions, options...)...)

return manifests.Params{
params := manifests.Params{
Config: cfg,
OtelCol: v1beta1.OpenTelemetryCollector{
TypeMeta: metav1.TypeMeta{
Expand Down Expand Up @@ -168,5 +168,10 @@ func newParams(taContainerImage string, file string, options ...config.Option) (
},
},
Log: logger,
}, nil
}
targetAllocator, err := TargetAllocator(params)
if err == nil {
params.TargetAllocator = targetAllocator
}
return params, nil
}
2 changes: 1 addition & 1 deletion internal/manifests/params.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ type Params struct {
Scheme *runtime.Scheme
Log logr.Logger
OtelCol v1beta1.OpenTelemetryCollector
TargetAllocator v1alpha1.TargetAllocator
TargetAllocator *v1alpha1.TargetAllocator
OpAMPBridge v1alpha1.OpAMPBridge
Config config.Config
}
4 changes: 2 additions & 2 deletions internal/naming/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -142,8 +142,8 @@ func ClusterRoleBinding(otelcol, namespace string) string {
}

// TAService returns the name to use for the TargetAllocator service.
func TAService(otelcol string) string {
return DNSName(Truncate("%s-targetallocator", 63, otelcol))
func TAService(taName string) string {
return DNSName(Truncate("%s-targetallocator", 63, taName))
}

// OpAMPBridgeService returns the name to use for the OpAMPBridge service.
Expand Down
2 changes: 1 addition & 1 deletion pkg/sidecar/pod.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ const (

// add a new sidecar container to the given pod, based on the given OpenTelemetryCollector.
func add(cfg config.Config, logger logr.Logger, otelcol v1beta1.OpenTelemetryCollector, pod corev1.Pod, attributes []corev1.EnvVar) (corev1.Pod, error) {
otelColCfg, err := collector.ReplaceConfig(otelcol)
otelColCfg, err := collector.ReplaceConfig(otelcol, nil)
if err != nil {
return pod, err
}
Expand Down
Loading