From ee00d2cf3998d3164938b44bfbf630f315dc4483 Mon Sep 17 00:00:00 2001
From: Andrew Crump <andrew.crump@broadcom.com>
Date: Wed, 10 Jul 2024 02:50:51 +0000
Subject: [PATCH 1/2] Expose complete collector configuration

- Replaces the existing `metric_exporters` and `trace_exporters`
  properties with the new `config` property.
- Operators can now provide pipeline and processor configuration.
- The default otlp receiver will be set on all pipelines.
- The config can be validated with the following command:
  `/var/vcap/packages/otel-collector/otel-collector validate --config some-config.yml`

Signed-off-by: Ausaf Ahmed <ausaf.ahmed@broadcom.com>
---
 Gemfile                                       |   2 +-
 jobs/otel-collector-windows/spec              |  46 +--
 .../templates/config.yml.erb                  |  93 ++---
 jobs/otel-collector/spec                      |  40 ++-
 jobs/otel-collector/templates/config.yml.erb  |  93 ++---
 spec/jobs/otel-collector-windows_spec.rb      |   4 +-
 .../shared_examples_for_otel_collector.rb     | 317 +++++++++++++-----
 7 files changed, 386 insertions(+), 209 deletions(-)

diff --git a/Gemfile b/Gemfile
index a0cbb7bc..84366213 100644
--- a/Gemfile
+++ b/Gemfile
@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-source "https://rubygems.org"
+source 'https://rubygems.org'
 
 group :development, :test do
   gem 'bosh-template', '2.2.1'
diff --git a/jobs/otel-collector-windows/spec b/jobs/otel-collector-windows/spec
index 70279b73..6c6ebf54 100644
--- a/jobs/otel-collector-windows/spec
+++ b/jobs/otel-collector-windows/spec
@@ -16,12 +16,36 @@ properties:
   enabled:
     description: "Enable OTel Collector"
     default: true
-  ingress.grpc.port:
-    description: "Port the collector is listening on to receive OTLP over gRPC"
-    default: 9100
+  config:
+    description: "Collector configuration"
+    default: {}
+    example: |
+      receivers:
+        otlp/placeholder:
+
+      processors:
+        batch:
+
+      exporters:
+        otlp:
+          endpoint: otelcol:4317
+
+      service:
+        pipelines:
+          traces:
+            receivers: [otlp/placeholder]
+            processors: [batch]
+            exporters: [otlp]
+          metrics:
+            receivers: [otlp/placeholder]
+            processors: [batch]
+            exporters: [otlp]
   ingress.grpc.address:
     description: "Address to listen on to receive OTLP over gRPC"
     default: 127.0.0.1
+  ingress.grpc.port:
+    description: "Port the collector is listening on to receive OTLP over gRPC"
+    default: 9100
   ingress.grpc.tls.ca_cert:
     description: "CA root required for key/cert verification in gRPC ingress"
   ingress.grpc.tls.cert:
@@ -34,19 +58,3 @@ properties:
   telemetry.metrics.port:
     description: "Port to serve the collector's internal metrics"
     default: 14830
-  metric_exporters:
-    description: "Exporter configuration for aggregate metric egress"
-    default: {}
-    example: |
-      otlp:
-        endpoint: otelcol:4317
-      otlp/2:
-        endpoint: otelcol:4318
-  trace_exporters:
-    description: "Exporter configuration for aggregate trace egress"
-    default: {}
-    example: |
-      otlp/trace:
-        endpoint: otelcol:4317
-      otlp/trace2:
-        endpoint: otelcol:4318
diff --git a/jobs/otel-collector-windows/templates/config.yml.erb b/jobs/otel-collector-windows/templates/config.yml.erb
index 4646616b..8dffa802 100644
--- a/jobs/otel-collector-windows/templates/config.yml.erb
+++ b/jobs/otel-collector-windows/templates/config.yml.erb
@@ -1,61 +1,76 @@
 <%=
-metric_exporters = p('metric_exporters')
-unless metric_exporters.respond_to?(:keys)
-  metric_exporters = YAML::load(metric_exporters)
+def config
+  @config ||= begin
+    cfg = p('config')
+    cfg = YAML.safe_load(cfg) unless cfg.respond_to?(:keys)
+    cfg
+  end
 end
 
-trace_exporters = p('trace_exporters')
-unless trace_exporters.respond_to?(:keys)
-  trace_exporters = YAML::load(trace_exporters)
+def check_for_no_exporters!
+  raise 'Exporter configuration must be provided' unless config['exporters']
 end
 
-unless (trace_exporters.keys-metric_exporters.keys) == trace_exporters.keys
-  raise "Exporter names must be unique"
+def check_for_no_service_config!
+  raise 'Service configuration must be provided' unless config['service']
 end
 
-if (metric_exporters.keys + trace_exporters.keys).any?{|k| k.include?('/cf-internal')}
-  raise 'Exporters cannot be defined under cf-internal namespace'
+def check_for_use_of_reserved_prefix!
+  %w[exporters processors].each do |component|
+    if config[component] && config[component].keys.any? { |k| k.include?('/cf-internal') }
+      raise "#{component.capitalize} cannot be defined under cf-internal namespace"
+    end
+  end
 end
 
-if metric_exporters.any?{|k, v| k.start_with?('prometheus') && v['endpoint'] && v['endpoint'].end_with?(':8889')}
-  raise 'Cannot define prometheus exporter listening on port 8889 (reserved for BBS API port)'
+def check_for_use_of_reserved_bbs_api_port!
+  if config['exporters'] && config['exporters'].any? do |k, v|
+       k.start_with?('prometheus/') && v['endpoint'] && v['endpoint'].end_with?(':8889')
+     end
+    raise 'Cannot define prometheus exporter listening on port 8889 (reserved for BBS API port)'
+  end
 end
 
-config = {
-  "receivers"=> {
-    "otlp"=>{
-      "protocols"=>{
-        "grpc"=>{
-          "endpoint"=>"#{p('ingress.grpc.address')}:#{p('ingress.grpc.port')}",
-          "tls"=>{
-            "client_ca_file"=>"/var/vcap/jobs/otel-collector-windows/config/certs/otel-collector-ca.crt",
-             "cert_file"=>"/var/vcap/jobs/otel-collector-windows/config/certs/otel-collector.crt",
-             "key_file"=>"/var/vcap/jobs/otel-collector-windows/config/certs/otel-collector.key",
-             "min_version"=>"1.3"
+def set_internal_receiver_as_only_receiver
+  config['receivers'] = {
+    'otlp/cf-internal-local' => {
+      'protocols' => {
+        'grpc' => {
+          'endpoint' => "#{p('ingress.grpc.address')}:#{p('ingress.grpc.port')}",
+          'tls' => {
+            'client_ca_file' => '/var/vcap/jobs/otel-collector-windows/config/certs/otel-collector-ca.crt',
+            'cert_file' => '/var/vcap/jobs/otel-collector-windows/config/certs/otel-collector.crt',
+            'key_file' => '/var/vcap/jobs/otel-collector-windows/config/certs/otel-collector.key',
+            'min_version' => '1.3'
           }
         }
       }
     }
-  },
-  "exporters"=>metric_exporters.merge(trace_exporters),
-  "service"=>{
-    "telemetry"=>{
-      "metrics"=>{
-        "level"=>p('telemetry.metrics.level'),
-        "address"=>"127.0.0.1:#{p('telemetry.metrics.port')}"
-      }
-    },
-    "pipelines"=>{}
   }
-}
+end
 
-if metric_exporters.any?
-  config['service']['pipelines']['metrics'] = {"receivers"=>["otlp"], "exporters"=>metric_exporters.keys}
+def set_internal_receiver_on_all_pipelines
+  config['service']['pipelines'].each_value do |p|
+    p['receivers'] = ['otlp/cf-internal-local']
+  end
 end
 
-if trace_exporters.any?
-  config['service']['pipelines']['traces'] = {"receivers"=>["otlp"], "exporters"=>trace_exporters.keys}
+def expose_internal_telemetry
+  config['service']['telemetry'] = {
+    'metrics' => {
+      'address' => "127.0.0.1:#{p('telemetry.metrics.port')}",
+      'level' => p('telemetry.metrics.level')
+    }
+  }
 end
 
-YAML::dump(config)
+check_for_no_exporters!
+check_for_no_service_config!
+check_for_use_of_reserved_prefix!
+check_for_use_of_reserved_bbs_api_port!
+set_internal_receiver_as_only_receiver
+set_internal_receiver_on_all_pipelines
+expose_internal_telemetry
+
+YAML.dump(config)
 %>
diff --git a/jobs/otel-collector/spec b/jobs/otel-collector/spec
index 1903e51e..63c7966f 100644
--- a/jobs/otel-collector/spec
+++ b/jobs/otel-collector/spec
@@ -17,6 +17,30 @@ properties:
   enabled:
     description: "Enable OTel Collector"
     default: true
+  config:
+    description: "Collector configuration"
+    default: {}
+    example: |
+      receivers:
+        otlp/placeholder:
+
+      processors:
+        batch:
+
+      exporters:
+        otlp:
+          endpoint: otelcol:4317
+
+      service:
+        pipelines:
+          traces:
+            receivers: [otlp/placeholder]
+            processors: [batch]
+            exporters: [otlp]
+          metrics:
+            receivers: [otlp/placeholder]
+            processors: [batch]
+            exporters: [otlp]
   ingress.grpc.address:
     description: "Address to listen on to receive OTLP over gRPC"
     default: 127.0.0.1
@@ -35,19 +59,3 @@ properties:
   telemetry.metrics.port:
     description: "Port to serve the collector's internal metrics"
     default: 14830
-  metric_exporters:
-    description: "Exporter configuration for aggregate metric egress"
-    default: {}
-    example: |
-      otlp:
-        endpoint: otelcol:4317
-      otlp/2:
-        endpoint: otelcol:4318
-  trace_exporters:
-    description: "Exporter configuration for aggregate trace egress"
-    default: {}
-    example: |
-      otlp/trace:
-        endpoint: otelcol:4317
-      otlp/trace2:
-        endpoint: otelcol:4318
diff --git a/jobs/otel-collector/templates/config.yml.erb b/jobs/otel-collector/templates/config.yml.erb
index d023c51f..b02765f7 100644
--- a/jobs/otel-collector/templates/config.yml.erb
+++ b/jobs/otel-collector/templates/config.yml.erb
@@ -1,61 +1,76 @@
 <%=
-metric_exporters = p('metric_exporters')
-unless metric_exporters.respond_to?(:keys)
-  metric_exporters = YAML::load(metric_exporters)
+def config
+  @config ||= begin
+    cfg = p('config')
+    cfg = YAML.safe_load(cfg) unless cfg.respond_to?(:keys)
+    cfg
+  end
 end
 
-trace_exporters = p('trace_exporters')
-unless trace_exporters.respond_to?(:keys)
-  trace_exporters = YAML::load(trace_exporters)
+def check_for_no_exporters!
+  raise 'Exporter configuration must be provided' unless config['exporters']
 end
 
-unless (trace_exporters.keys-metric_exporters.keys) == trace_exporters.keys
-  raise "Exporter names must be unique"
+def check_for_no_service_config!
+  raise 'Service configuration must be provided' unless config['service']
 end
 
-if (metric_exporters.keys + trace_exporters.keys).any?{|k| k.include?('/cf-internal')}
-  raise 'Exporters cannot be defined under cf-internal namespace'
+def check_for_use_of_reserved_prefix!
+  %w[exporters processors].each do |component|
+    if config[component] && config[component].keys.any? { |k| k.include?('/cf-internal') }
+      raise "#{component.capitalize} cannot be defined under cf-internal namespace"
+    end
+  end
 end
 
-if metric_exporters.any?{|k, v| k.start_with?('prometheus') && v['endpoint'] && v['endpoint'].end_with?(':8889')}
-  raise 'Cannot define prometheus exporter listening on port 8889 (reserved for BBS API port)'
+def check_for_use_of_reserved_bbs_api_port!
+  if config['exporters'] && config['exporters'].any? do |k, v|
+       k.start_with?('prometheus/') && v['endpoint'] && v['endpoint'].end_with?(':8889')
+     end
+    raise 'Cannot define prometheus exporter listening on port 8889 (reserved for BBS API port)'
+  end
 end
 
-config = {
-  "receivers"=> {
-    "otlp"=>{
-      "protocols"=>{
-        "grpc"=>{
-          "endpoint"=>"#{p('ingress.grpc.address')}:#{p('ingress.grpc.port')}",
-          "tls"=>{
-            "client_ca_file"=>"/var/vcap/jobs/otel-collector/config/certs/otel-collector-ca.crt",
-             "cert_file"=>"/var/vcap/jobs/otel-collector/config/certs/otel-collector.crt",
-             "key_file"=>"/var/vcap/jobs/otel-collector/config/certs/otel-collector.key",
-             "min_version"=>"1.3"
+def set_internal_receiver_as_only_receiver
+  config['receivers'] = {
+    'otlp/cf-internal-local' => {
+      'protocols' => {
+        'grpc' => {
+          'endpoint' => "#{p('ingress.grpc.address')}:#{p('ingress.grpc.port')}",
+          'tls' => {
+            'client_ca_file' => '/var/vcap/jobs/otel-collector/config/certs/otel-collector-ca.crt',
+            'cert_file' => '/var/vcap/jobs/otel-collector/config/certs/otel-collector.crt',
+            'key_file' => '/var/vcap/jobs/otel-collector/config/certs/otel-collector.key',
+            'min_version' => '1.3'
           }
         }
       }
     }
-  },
-  "exporters"=>metric_exporters.merge(trace_exporters),
-  "service"=>{
-    "telemetry"=>{
-      "metrics"=>{
-        "level"=>p('telemetry.metrics.level'),
-        "address"=>"127.0.0.1:#{p('telemetry.metrics.port')}"
-      }
-    },
-    "pipelines"=>{}
   }
-}
+end
 
-if metric_exporters.any?
-  config['service']['pipelines']['metrics'] = {"receivers"=>["otlp"], "exporters"=>metric_exporters.keys}
+def set_internal_receiver_on_all_pipelines
+  config['service']['pipelines'].each_value do |p|
+    p['receivers'] = ['otlp/cf-internal-local']
+  end
 end
 
-if trace_exporters.any?
-  config['service']['pipelines']['traces'] = {"receivers"=>["otlp"], "exporters"=>trace_exporters.keys}
+def expose_internal_telemetry
+  config['service']['telemetry'] = {
+    'metrics' => {
+      'address' => "127.0.0.1:#{p('telemetry.metrics.port')}",
+      'level' => p('telemetry.metrics.level')
+    }
+  }
 end
 
-YAML::dump(config)
+check_for_no_exporters!
+check_for_no_service_config!
+check_for_use_of_reserved_prefix!
+check_for_use_of_reserved_bbs_api_port!
+set_internal_receiver_as_only_receiver
+set_internal_receiver_on_all_pipelines
+expose_internal_telemetry
+
+YAML.dump(config)
 %>
diff --git a/spec/jobs/otel-collector-windows_spec.rb b/spec/jobs/otel-collector-windows_spec.rb
index 73e3f325..8364fe4b 100644
--- a/spec/jobs/otel-collector-windows_spec.rb
+++ b/spec/jobs/otel-collector-windows_spec.rb
@@ -14,8 +14,8 @@
 
   describe 'spec' do
     it 'has only the specified differences from the linux spec' do
-      windows_spec = YAML.load(File.read(File.join(release_dir, 'jobs', 'otel-collector-windows', 'spec')))
-      linux_spec = YAML.load(File.read(File.join(release_dir, 'jobs', 'otel-collector', 'spec')))
+      windows_spec = YAML.safe_load(File.read(File.join(release_dir, 'jobs', 'otel-collector-windows', 'spec')))
+      linux_spec = YAML.safe_load(File.read(File.join(release_dir, 'jobs', 'otel-collector', 'spec')))
 
       windows_spec['name'] = 'otel-collector'
       windows_spec['packages'] = ['otel-collector']
diff --git a/spec/support/shared_examples_for_otel_collector.rb b/spec/support/shared_examples_for_otel_collector.rb
index 300dde53..bca7ac4d 100644
--- a/spec/support/shared_examples_for_otel_collector.rb
+++ b/spec/support/shared_examples_for_otel_collector.rb
@@ -7,120 +7,108 @@
 shared_examples_for 'common config.yml' do
   describe 'config/config.yml' do
     let(:template) { job.template('config/config.yml') }
-    let(:trace_exporters) { { 'otlp/traces' => { 'endpoint' => 'otelcol:4317' } } }
-    let(:metric_exporters) do
+    let(:config) do
       {
-        'otlp' => { 'endpoint' => 'otelcol:4317' },
-        'prometheus/tls' => {
-          'endpoint' => '1.2.3.4:1234',
-          'metric_expiration' => '60m'
+        'receivers' => {
+          'otlp/placeholder' => nil
+        },
+        'processors' => {
+          'batch' => nil
+        },
+        'exporters' => {
+          'otlp' => {
+            'endpoint' => 'otelcol:4317'
+          }
+        },
+        'extensions' => {
+          'pprof' => nil,
+          'zpages' => nil
+        },
+        'service' => {
+          'extensions' => %w[pprof zpages],
+          'pipelines' => {
+            'traces' => {
+              'receivers' => ['otlp/placeholder'],
+              'processors' => ['batch'],
+              'exporters' => ['otlp']
+            },
+            'metrics' => {
+              'receivers' => ['otlp/placeholder'],
+              'processors' => ['batch'],
+              'exporters' => ['otlp']
+            }
+          }
         }
       }
     end
-    let(:properties) do
-      {
-        'metric_exporters' => metric_exporters,
-        'trace_exporters' => trace_exporters
-      }
-    end
-    let(:rendered) { YAML.load(template.render(properties)) }
+    let(:properties) { { 'config' => config } }
+    let(:rendered) { YAML.safe_load(template.render(properties)) }
 
-    describe 'exporters' do
-      let(:exporters) { rendered['exporters'] }
+    context 'when the config is provided as a string, not a hash' do
+      let(:string_config) { YAML.dump(config) }
+      let(:rendered) { YAML.safe_load(template.render({ 'config' => string_config })) }
 
-      it 'has the right number of exporters' do
-        expect(exporters.count).to eq(3)
+      def without_receivers(cfg)
+        cfg.delete('receivers')
+        cfg['service']['pipelines']['metrics'].delete('receivers')
+        cfg['service']['pipelines']['traces'].delete('receivers')
+        cfg
       end
 
-      context 'when an exporter has a name collision' do
-        let(:trace_exporters) { { 'otlp' => { 'endpoint' => 'otelcol:4317' } } }
-        it 'raises an error' do
-          expect { rendered }.to raise_error(/Exporter names must be unique/)
-        end
+      def without_internal_telemetry(cfg)
+        cfg.tap { |c| c['service'].delete('telemetry') }
       end
 
-      describe 'trace exporters' do
-        it 'puts the trace exporters in the traces pipeline' do
-          expect(rendered['service']['pipelines']['traces']).to eq({ 'exporters' => ['otlp/traces'], 'receivers' => ['otlp'] })
-        end
-
-        context 'when exporters is a string and not a hash' do
-          let(:trace_exporters) { "{otlp/traces: {endpoint: 'otelcol:4317'}}" }
-          it 'parses it as YAML' do
-            expect(rendered['service']['pipelines']['traces']).to eq({ 'exporters' => ['otlp/traces'], 'receivers' => ['otlp'] })
-          end
-        end
+      it 'uses the config provided and parses it as YAML' do
+        expect(without_internal_telemetry(without_receivers(rendered))).to eq(
+          without_internal_telemetry(without_receivers(config))
+        )
+      end
+    end
 
-        context 'when an exporter uses the reserved namespace' do
-          let(:trace_exporters) { { 'otlp/cf-internal-foo' => { 'endpoint' => 'otelcol:4317' } } }
-          it 'raises an error' do
-            expect { rendered }.to raise_error(/Exporters cannot be defined under cf-internal namespace/)
-          end
-        end
+    context 'when only minimal valid config is provided' do
+      before do
+        config.delete('receivers')
+        config.delete('processors')
+        config.delete('extensions')
+      end
 
-        context 'when trace exporters is empty' do
-          let(:trace_exporters) { {} }
-          it 'does not create a trace pipeline, which would cause otel-collector to error' do
-            expect(rendered['service']['pipelines']['traces']).to be_nil
-          end
-        end
+      it 'renders successfully' do
+        expect(rendered.keys).to contain_exactly('receivers', 'exporters', 'service')
       end
+    end
 
-      describe 'metric exporters' do
-        it 'puts the metric exporters in the metrics pipeline' do
-          expect(rendered['service']['pipelines']['metrics']).to eq({ 'exporters' => ['otlp', 'prometheus/tls'], 'receivers' => ['otlp'] })
-        end
+    context 'receivers' do
+      let(:receivers) { rendered['receivers'] }
 
-        context 'when exporters is a string and not a hash' do
-          let(:metric_exporters) { "{otlp/metrics: {endpoint: 'otelcol:4317'}}" }
-          it 'parses it as YAML' do
-            expect(rendered['service']['pipelines']['metrics']).to eq({ 'exporters' => ['otlp/metrics'], 'receivers' => ['otlp'] })
-          end
-        end
+      it 'removes any receiver that the operator provided to keep the config well-formed' do
+        expect(receivers.keys).to_not include 'otlp/placeholder'
+      end
 
-        context 'when there is a prometheus exporter listening on 8889' do
-          let(:metric_exporters) do
-            {
-              'prometheus/tls' => {
-                'endpoint' => '1.2.3.4:8889',
-                'metric_expiration' => '60m'
+      context 'when the operator provides a real receiver' do
+        before do
+          config['receivers']['otlp/some-receiver'] = {
+            'protocols' => {
+              'grpc' => {
+                'endpoint' => '0.0.0.0:2345'
+              },
+              'http' => {
+                'endpoint' => '0.0.0.0:3456'
               }
             }
-          end
-
-          it 'raises an error' do
-            expect { rendered }.to raise_error(/Cannot define prometheus exporter listening on port 8889/)
-          end
-        end
-
-        context 'when an exporter uses the reserved namespace' do
-          let(:metric_exporters) { { 'otlp/cf-internal-foo' => { 'endpoint' => 'otelcol:4317' } } }
-          it 'raises an error' do
-            expect { rendered }.to raise_error(/Exporters cannot be defined under cf-internal namespace/)
-          end
+          }
         end
 
-        context 'when metric exporters is empty' do
-          let(:metric_exporters) { {} }
-          it 'does not create a metric pipeline, which would cause otel-collector to error' do
-            expect(rendered['service']['pipelines']['metrics']).to be_nil
-          end
+        it 'is ignored' do
+          expect(rendered['receivers'].keys).to_not include 'otlp/some-receiver'
         end
       end
-    end
-
-    context 'receivers' do
-      let(:receivers) { rendered['receivers'] }
-
-      it 'only has one receiver' do
-        expect(receivers.count).to eq(1)
-      end
 
-      context 'otlpreceiver' do
-        let(:otlpreceiver) { rendered['receivers']['otlp'] }
+      context 'built-in otlp receiver' do
+        let(:builtin_otlp_receiver) { rendered['receivers']['otlp/cf-internal-local'] }
 
         it 'is configured by default' do
-          expect(otlpreceiver).to eq(
+          expect(builtin_otlp_receiver).to eq(
             {
               'protocols' => {
                 'grpc' => {
@@ -137,22 +125,165 @@
           )
         end
 
+        context 'when multiple pipelines exist' do
+          before do
+            config['service']['pipelines'] = {
+              'traces' => {
+                'receivers' => ['otlp/placeholder'],
+                'processors' => ['batch'],
+                'exporters' => ['otlp']
+              },
+              'traces/2' => {
+                'receivers' => ['otlp/placeholder'],
+                'processors' => ['batch/test'],
+                'exporters' => ['otlp/2']
+              },
+              'metrics' => {
+                'receivers' => ['otlp/placeholder'],
+                'processors' => ['batch'],
+                'exporters' => ['otlp']
+              },
+              'metrics/foo' => {
+                'receivers' => ['otlp/placeholder'],
+                'processors' => ['batch'],
+                'exporters' => ['otlp']
+              }
+            }
+          end
+
+          it 'includes only the built-in receiver in every pipeline' do
+            expect(rendered['service']['pipelines']['traces']['receivers']).to eq(['otlp/cf-internal-local'])
+            expect(rendered['service']['pipelines']['traces/2']['receivers']).to eq(['otlp/cf-internal-local'])
+            expect(rendered['service']['pipelines']['metrics']['receivers']).to eq(['otlp/cf-internal-local'])
+            expect(rendered['service']['pipelines']['metrics/foo']['receivers']).to eq(['otlp/cf-internal-local'])
+          end
+        end
+
         context 'when ingress.grpc.port is set' do
-          let(:properties) { { 'ingress' => { 'grpc' => { 'port' => 1234 } } } }
+          before do
+            properties['ingress'] = { 'grpc' => { 'port' => 1234 } }
+          end
 
           it 'has an endpoint with that port' do
-            expect(otlpreceiver['protocols']['grpc']['endpoint']).to eq('127.0.0.1:1234')
+            expect(builtin_otlp_receiver['protocols']['grpc']['endpoint']).to eq('127.0.0.1:1234')
           end
         end
 
         context 'when ingress.grpc.listen_address is set' do
-          let(:properties) { { 'ingress' => { 'grpc' => { 'address' => '0.0.0.0' } } } }
+          before do
+            properties['ingress'] = { 'grpc' => { 'address' => '0.0.0.0' } }
+          end
 
           it 'has an endpoint with that address' do
-            expect(otlpreceiver['protocols']['grpc']['endpoint']).to eq('0.0.0.0:9100')
+            expect(builtin_otlp_receiver['protocols']['grpc']['endpoint']).to eq('0.0.0.0:9100')
           end
         end
       end
     end
+
+    describe 'processors' do
+      it 'includes the configured processors in the config' do
+        expect(rendered.keys).to include 'processors'
+        expect(rendered['processors']).to eq(config['processors'])
+      end
+      context 'when a processor uses the reserved namespace' do
+        before do
+          config['processors']['batch/cf-internal-foo'] = nil
+        end
+        it 'raises an error' do
+          expect { rendered }.to raise_error(/Processors cannot be defined under cf-internal namespace/)
+        end
+      end
+    end
+
+    describe 'exporters' do
+      it 'includes the configured exporters in the config' do
+        expect(rendered.keys).to include 'exporters'
+        expect(rendered['exporters']).to eq(config['exporters'])
+      end
+
+      context 'when there is a prometheus exporter listening on 8889' do
+        before do
+          config['exporters']['prometheus/tls'] = {
+            'endpoint' => '203.0.113.10:8889',
+            'metric_expiration' => '60m'
+          }
+        end
+
+        it 'raises an error' do
+          expect { rendered }.to raise_error(/Cannot define prometheus exporter listening on port 8889/)
+        end
+      end
+
+      context 'when an exporter uses the reserved namespace' do
+        before do
+          config['exporters']['otlp/cf-internal-foo'] = {
+            'endpoint' => '203.0.113.10:4317'
+          }
+        end
+        it 'raises an error' do
+          expect { rendered }.to raise_error(/Exporters cannot be defined under cf-internal namespace/)
+        end
+      end
+    end
+
+    describe 'extensions' do
+      context 'when extensions are specified' do
+        it 'includes the configured extensions in the config' do
+          expect(rendered.keys).to include 'extensions'
+          expect(rendered['extensions']).to eq(config['extensions'])
+        end
+      end
+    end
+
+    describe 'internal telemetry' do
+      it 'exposes telemetry at the default port' do
+        expect(rendered['service']['telemetry']['metrics']['address']).to eq('127.0.0.1:14830')
+      end
+      it 'provides basic level metrics by default' do
+        expect(rendered['service']['telemetry']['metrics']['level']).to eq('basic')
+      end
+
+      context 'when the port is specified' do
+        let(:properties) { { 'config' => config, 'telemetry' => { 'metrics' => { 'port' => 14_831 } } } }
+        it 'exposes telemetry at the specified port' do
+          expect(rendered['service']['telemetry']['metrics']['address']).to eq('127.0.0.1:14831')
+        end
+      end
+
+      context 'when the metrics level is specified' do
+        let(:properties) { { 'config' => config, 'telemetry' => { 'metrics' => { 'level' => 'detailed' } } } }
+        it 'applies the telemetry metrics level' do
+          expect(rendered['service']['telemetry']['metrics']['level']).to eq('detailed')
+        end
+      end
+    end
+
+    describe 'invalid config' do
+      context 'when the config does not provide exporters' do
+        before do
+          config.delete('exporters')
+        end
+        it 'errors' do
+          expect { rendered }.to raise_error(/Exporter configuration must be provided/)
+        end
+      end
+      context 'when the config has the exporters key but no value' do
+        before do
+          config['exporters'] = nil
+        end
+        it 'errors' do
+          expect { rendered }.to raise_error(/Exporter configuration must be provided/)
+        end
+      end
+      context 'when the config does not provide a service section' do
+        before do
+          config.delete('service')
+        end
+        it 'errors' do
+          expect { rendered }.to raise_error(/Service configuration must be provided/)
+        end
+      end
+    end
   end
 end

From e68a410cf52b6c971a4d01f42c6e88f014cbca11 Mon Sep 17 00:00:00 2001
From: Andrew Crump <andrew.crump@broadcom.com>
Date: Tue, 16 Jul 2024 01:28:04 +0000
Subject: [PATCH 2/2] Support metric_exporters and trace_exporters properties

- Continue to support the earlier properties to provide a migration
  path.
- Operators should prefer the new `config` property going forwards.

Signed-off-by: Matthew Kocher <matthew.kocher@broadcom.com>
---
 jobs/otel-collector-windows/spec              | 16 +++
 .../templates/config.yml.erb                  | 45 ++++++++-
 jobs/otel-collector/spec                      | 16 +++
 jobs/otel-collector/templates/config.yml.erb  | 45 ++++++++-
 .../shared_examples_for_otel_collector.rb     | 99 +++++++++++++++++++
 5 files changed, 217 insertions(+), 4 deletions(-)

diff --git a/jobs/otel-collector-windows/spec b/jobs/otel-collector-windows/spec
index 6c6ebf54..5a569977 100644
--- a/jobs/otel-collector-windows/spec
+++ b/jobs/otel-collector-windows/spec
@@ -58,3 +58,19 @@ properties:
   telemetry.metrics.port:
     description: "Port to serve the collector's internal metrics"
     default: 14830
+  metric_exporters:
+    description: "Exporter configuration for aggregate metric egress. Deprecated, please use 'config' property."
+    default: {}
+    example: |
+      otlp:
+        endpoint: otelcol:4317
+      otlp/2:
+        endpoint: otelcol:4318
+  trace_exporters:
+    description: "Exporter configuration for aggregate trace egress. Deprecated, please use 'config' property."
+    default: {}
+    example: |
+      otlp/trace:
+        endpoint: otelcol:4317
+      otlp/trace2:
+        endpoint: otelcol:4318
diff --git a/jobs/otel-collector-windows/templates/config.yml.erb b/jobs/otel-collector-windows/templates/config.yml.erb
index 8dffa802..bd673c25 100644
--- a/jobs/otel-collector-windows/templates/config.yml.erb
+++ b/jobs/otel-collector-windows/templates/config.yml.erb
@@ -1,12 +1,52 @@
 <%=
 def config
   @config ||= begin
-    cfg = p('config')
-    cfg = YAML.safe_load(cfg) unless cfg.respond_to?(:keys)
+    cfg = retrieve_property('config')
+    cfg = handle_old_properties(cfg) if cfg.empty?
     cfg
   end
 end
 
+def check_for_new_and_old_properties!
+  both_provided = !p('config').empty? && (!p('metric_exporters').empty? || !p('trace_exporters').empty?)
+  return unless both_provided
+
+  raise "Can not provide 'config' property when deprecated 'metric_exporters' or 'trace_exporters' properties are provided"
+end
+
+def retrieve_property(name)
+  if p(name).respond_to?(:keys)
+    p(name)
+  else
+    YAML.safe_load(p(name))
+  end
+end
+
+def handle_old_properties(cfg)
+  metric_exporters = retrieve_property('metric_exporters')
+  trace_exporters = retrieve_property('trace_exporters')
+  raise 'Exporter names must be unique' unless (trace_exporters.keys - metric_exporters.keys) == trace_exporters.keys
+
+  cfg['exporters'] = metric_exporters.merge(trace_exporters)
+  cfg['service'] = { 'pipelines' => {} }
+
+  if metric_exporters.any?
+    cfg['service']['pipelines']['metrics'] = {
+        'receivers' => ['otlp/cf-local'],
+        'exporters' => metric_exporters.keys
+      }
+  end
+
+  if trace_exporters.any?
+    cfg['service']['pipelines']['traces'] = {
+        'receivers' => ['otlp/cf-local'],
+        'exporters' => trace_exporters.keys
+      }
+  end
+
+  cfg
+end
+
 def check_for_no_exporters!
   raise 'Exporter configuration must be provided' unless config['exporters']
 end
@@ -64,6 +104,7 @@ def expose_internal_telemetry
   }
 end
 
+check_for_new_and_old_properties!
 check_for_no_exporters!
 check_for_no_service_config!
 check_for_use_of_reserved_prefix!
diff --git a/jobs/otel-collector/spec b/jobs/otel-collector/spec
index 63c7966f..c2c9eb4f 100644
--- a/jobs/otel-collector/spec
+++ b/jobs/otel-collector/spec
@@ -59,3 +59,19 @@ properties:
   telemetry.metrics.port:
     description: "Port to serve the collector's internal metrics"
     default: 14830
+  metric_exporters:
+    description: "Exporter configuration for aggregate metric egress. Deprecated, please use 'config' property."
+    default: {}
+    example: |
+      otlp:
+        endpoint: otelcol:4317
+      otlp/2:
+        endpoint: otelcol:4318
+  trace_exporters:
+    description: "Exporter configuration for aggregate trace egress. Deprecated, please use 'config' property."
+    default: {}
+    example: |
+      otlp/trace:
+        endpoint: otelcol:4317
+      otlp/trace2:
+        endpoint: otelcol:4318
diff --git a/jobs/otel-collector/templates/config.yml.erb b/jobs/otel-collector/templates/config.yml.erb
index b02765f7..af2d3614 100644
--- a/jobs/otel-collector/templates/config.yml.erb
+++ b/jobs/otel-collector/templates/config.yml.erb
@@ -1,12 +1,52 @@
 <%=
 def config
   @config ||= begin
-    cfg = p('config')
-    cfg = YAML.safe_load(cfg) unless cfg.respond_to?(:keys)
+    cfg = retrieve_property('config')
+    cfg = handle_old_properties(cfg) if cfg.empty?
     cfg
   end
 end
 
+def check_for_new_and_old_properties!
+  both_provided = !p('config').empty? && (!p('metric_exporters').empty? || !p('trace_exporters').empty?)
+  return unless both_provided
+
+  raise "Can not provide 'config' property when deprecated 'metric_exporters' or 'trace_exporters' properties are provided"
+end
+
+def retrieve_property(name)
+  if p(name).respond_to?(:keys)
+    p(name)
+  else
+    YAML.safe_load(p(name))
+  end
+end
+
+def handle_old_properties(cfg)
+  metric_exporters = retrieve_property('metric_exporters')
+  trace_exporters = retrieve_property('trace_exporters')
+  raise 'Exporter names must be unique' unless (trace_exporters.keys - metric_exporters.keys) == trace_exporters.keys
+
+  cfg['exporters'] = metric_exporters.merge(trace_exporters)
+  cfg['service'] = { 'pipelines' => {} }
+
+  if metric_exporters.any?
+    cfg['service']['pipelines']['metrics'] = {
+        'receivers' => ['otlp/cf-local'],
+        'exporters' => metric_exporters.keys
+      }
+  end
+
+  if trace_exporters.any?
+    cfg['service']['pipelines']['traces'] = {
+        'receivers' => ['otlp/cf-local'],
+        'exporters' => trace_exporters.keys
+      }
+  end
+
+  cfg
+end
+
 def check_for_no_exporters!
   raise 'Exporter configuration must be provided' unless config['exporters']
 end
@@ -64,6 +104,7 @@ def expose_internal_telemetry
   }
 end
 
+check_for_new_and_old_properties!
 check_for_no_exporters!
 check_for_no_service_config!
 check_for_use_of_reserved_prefix!
diff --git a/spec/support/shared_examples_for_otel_collector.rb b/spec/support/shared_examples_for_otel_collector.rb
index bca7ac4d..db07f034 100644
--- a/spec/support/shared_examples_for_otel_collector.rb
+++ b/spec/support/shared_examples_for_otel_collector.rb
@@ -285,5 +285,104 @@ def without_internal_telemetry(cfg)
         end
       end
     end
+
+    context 'when disabled and no other config properties are provided' do
+      let(:properties) { { 'enabled' => false } }
+
+      it "doesn't raise an error" do
+        expect { rendered }.to_not raise_error
+      end
+    end
+
+    context 'when the older config properties are provided' do
+      let(:properties) do
+        {
+          'metric_exporters' => {
+            'otlp' => { 'endpoint' => 'otelcol:4317' },
+            'prometheus/tls' => {
+              'endpoint' => '1.2.3.4:1234',
+              'metric_expiration' => '60m'
+            }
+          },
+          'trace_exporters' => {
+            'otlp/traces' => { 'endpoint' => 'otelcol:4317' }
+          }
+        }
+      end
+
+      it 'uses the exporters provided' do
+        expect(rendered['exporters']).to eq(
+          {
+            'otlp' => { 'endpoint' => 'otelcol:4317' },
+            'prometheus/tls' => {
+              'endpoint' => '1.2.3.4:1234',
+              'metric_expiration' => '60m'
+            },
+            'otlp/traces' => { 'endpoint' => 'otelcol:4317' }
+          }
+        )
+      end
+
+      it 'generates pipelines that include the exporters' do
+        metrics_pipeline = rendered['service']['pipelines']['metrics']
+        expect(metrics_pipeline['receivers']).to eq(['otlp/cf-internal-local'])
+        expect(metrics_pipeline['exporters']).to eq(['otlp', 'prometheus/tls'])
+
+        traces_pipeline = rendered['service']['pipelines']['traces']
+        expect(traces_pipeline['receivers']).to eq(['otlp/cf-internal-local'])
+        expect(traces_pipeline['exporters']).to eq(['otlp/traces'])
+      end
+
+      context 'when only a metrics pipeline is defined' do
+        before do
+          properties.delete('trace_exporters')
+        end
+        it 'does not generate a traces pipeline' do
+          expect(rendered['service']['pipelines'].keys).to_not include 'traces'
+        end
+      end
+
+      context 'when only a traces pipeline is defined' do
+        before do
+          properties.delete('metric_exporters')
+        end
+        it 'does not generate a metrics pipeline' do
+          expect(rendered['service']['pipelines'].keys).to_not include 'metrics'
+        end
+      end
+
+      context 'when an exporter has a name collision' do
+        before do
+          properties['trace_exporters'] = { 'otlp' => { 'endpoint' => 'otelcol:4317' } }
+        end
+
+        it 'raises an error' do
+          expect { rendered }.to raise_error(/Exporter names must be unique/)
+        end
+      end
+
+      context 'when trace_exporters is a string and not a hash' do
+        before do
+          properties['trace_exporters'] = YAML.dump(properties['trace_exporters'])
+        end
+
+        it 'parses it as YAML' do
+          expect(rendered['service']['pipelines']['traces']).to eq({ 'exporters' => ['otlp/traces'],
+                                                                     'receivers' => ['otlp/cf-internal-local'] })
+        end
+      end
+
+      describe 'and a normal configuration is also provided' do
+        before do
+          properties['config'] = { 'some' => 'configuration' }
+        end
+
+        it 'raises an error' do
+          expect do
+            rendered
+          end.to raise_error(/Can not provide 'config' property when deprecated 'metric_exporters' or 'trace_exporters' properties are provided/)
+        end
+      end
+    end
   end
 end