From 2c24b3ec416a25d03500e47c408b47db0799e191 Mon Sep 17 00:00:00 2001
From: William Dumont <william.dumont@grafana.com>
Date: Tue, 23 Jan 2024 15:45:08 +0100
Subject: [PATCH] prohibit the configuration of services within modules (#6215)

---
 CHANGELOG.md                           |  4 +++
 docs/sources/flow/release-notes.md     |  7 +++++
 pkg/flow/internal/controller/loader.go | 11 ++++++++
 pkg/flow/module_test.go                | 38 ++++++++++++++++++++++++++
 4 files changed, 60 insertions(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 366e24dc11ef..0bb46ed8df7b 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -10,6 +10,10 @@ internal API changes are not present.
 Main (unreleased)
 -----------------
 
+### Breaking changes
+
+- Prohibit the configuration of services within modules. (@wildum)
+
 ### Features
 
 - A new `discovery.process` component for discovering Linux OS processes on the current host. (@korniltsev)
diff --git a/docs/sources/flow/release-notes.md b/docs/sources/flow/release-notes.md
index f8053bf3c0b3..7124b4a9bb28 100644
--- a/docs/sources/flow/release-notes.md
+++ b/docs/sources/flow/release-notes.md
@@ -29,6 +29,13 @@ Other release notes for the different {{< param "PRODUCT_ROOT_NAME" >}} variants
 [release-notes-operator]: {{< relref "../operator/release-notes.md" >}}
 {{% /admonition %}}
 
+## v0.40
+
+### Breaking change: Prohibit the configuration of services within modules.
+
+Previously it was possible to configure the HTTP service via the [HTTP config block](https://grafana.com/docs/agent/v0.39/flow/reference/config-blocks/http/) inside of a module.
+This functionality is now only available in the main configuration.
+
 ## v0.39
 
 ### Breaking change: `otelcol.receiver.prometheus` will drop all `otel_scope_info` metrics when converting them to OTLP
diff --git a/pkg/flow/internal/controller/loader.go b/pkg/flow/internal/controller/loader.go
index 7673af3a2526..4a22b2419f0a 100644
--- a/pkg/flow/internal/controller/loader.go
+++ b/pkg/flow/internal/controller/loader.go
@@ -345,6 +345,17 @@ func (l *Loader) populateServiceNodes(g *dag.Graph, serviceBlocks []*ast.BlockSt
 	// Now, assign blocks to services.
 	for _, block := range serviceBlocks {
 		blockID := BlockComponentID(block).String()
+
+		if l.isModule() {
+			diags.Add(diag.Diagnostic{
+				Severity: diag.SeverityLevelError,
+				Message:  fmt.Sprintf("service blocks not allowed inside a module: %q", blockID),
+				StartPos: ast.StartPos(block).Position(),
+				EndPos:   ast.EndPos(block).Position(),
+			})
+			continue
+		}
+
 		node := g.GetByID(blockID).(*ServiceNode)
 
 		// Blocks assigned to services are reset to nil in the previous loop.
diff --git a/pkg/flow/module_test.go b/pkg/flow/module_test.go
index 4e4ddb9faaa8..c5f4417c84c3 100644
--- a/pkg/flow/module_test.go
+++ b/pkg/flow/module_test.go
@@ -7,8 +7,10 @@ import (
 	"time"
 
 	"github.com/grafana/agent/component"
+	"github.com/grafana/agent/pkg/flow/internal/controller"
 	"github.com/grafana/agent/pkg/flow/internal/worker"
 	"github.com/grafana/agent/pkg/flow/logging"
+	"github.com/grafana/agent/service"
 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/stretchr/testify/require"
 )
@@ -43,6 +45,9 @@ const exportDummy = `
 		value = "bob"
 	}`
 
+const serviceConfig = `
+	testservice {}`
+
 func TestModule(t *testing.T) {
 	tt := []struct {
 		name                  string
@@ -72,6 +77,12 @@ func TestModule(t *testing.T) {
 			exportModuleContent:   exportStringConfig,
 			expectedErrorContains: "tracing block not allowed inside a module",
 		},
+		{
+			name:                  "Service blocks not allowed in module config",
+			argumentModuleContent: argumentConfig + serviceConfig,
+			exportModuleContent:   exportStringConfig,
+			expectedErrorContains: "service blocks not allowed inside a module: \"testservice\"",
+		},
 		{
 			name:                  "Argument not defined in module source",
 			argumentModuleContent: `argument "different_argument" {}`,
@@ -245,12 +256,19 @@ func testModuleControllerOptions(t *testing.T) *moduleControllerOptions {
 	s, err := logging.New(os.Stderr, logging.DefaultOptions)
 	require.NoError(t, err)
 
+	services := []service.Service{
+		&testService{},
+	}
+
+	serviceMap := controller.NewServiceMap(services)
+
 	return &moduleControllerOptions{
 		Logger:         s,
 		DataPath:       t.TempDir(),
 		Reg:            prometheus.NewRegistry(),
 		ModuleRegistry: newModuleRegistry(),
 		WorkerPool:     worker.NewFixedWorkerPool(1, 100),
+		ServiceMap:     serviceMap,
 	}
 }
 
@@ -307,3 +325,23 @@ func (t *testModule) Run(ctx context.Context) error {
 func (t *testModule) Update(_ component.Arguments) error {
 	return nil
 }
+
+type testService struct{}
+
+func (t *testService) Definition() service.Definition {
+	return service.Definition{
+		Name: "testservice",
+	}
+}
+
+func (t *testService) Run(ctx context.Context, host service.Host) error {
+	return nil
+}
+
+func (t *testService) Update(newConfig any) error {
+	return nil
+}
+
+func (t *testService) Data() any {
+	return nil
+}