Remove URL-rewriting hack (#1519)

* Remove URL-rewriting hack This commit removes a hack that rewrites the collector URL for use with OpAmp. The collector now supports redirection, and we expect the Sumo Logic backend to correctly redirect the client. * Use default SumoLogic URL for OpAmp clients When the OpAmp URL is not explicitly configured, use the default SumoLogic URL. * overwrite GNU_SITE for now Signed-off-by: Justin Kolberg <[email protected]> * chore(ci): fix toolchain cache name Signed-off-by: Justin Kolberg <[email protected]> * fix(ci): fix path to Makefile in hash generation Signed-off-by: Justin Kolberg <[email protected]> --------- Signed-off-by: Eric Chlebek <[email protected]> Signed-off-by: Justin Kolberg <[email protected]> Co-authored-by: Justin Kolberg <[email protected]>
SumoLogic · Nov 8, 2024 · 76770d1 · 76770d1
1 parent 1b49351
commit 76770d1
Show file tree

Hide file tree

Showing 4 changed files with 65 additions and 127 deletions.
diff --git a/.github/workflows/workflow-build.yml b/.github/workflows/workflow-build.yml
@@ -112,7 +112,7 @@ jobs:
         run: |
           echo "cache-key=go-build-${{ env.GO_VERSION }}${OTELCOL_FIPS_SUFFIX}-${{inputs.arch_os}}-${{ hashFiles('pkg/**/go.sum', 'otelcolbuilder/.otelcol-builder.yaml') }}" >> $GITHUB_OUTPUT
           echo "restore-keys=go-build-${{ env.GO_VERSION }}${OTELCOL_FIPS_SUFFIX}-${{inputs.arch_os}}-" >> $GITHUB_OUTPUT
-          echo "toolchain-cache-key=toolchain-${{inputs.arch_os}}-${{ hashFiles('otelcolbuilder/build-fips/config.mak', 'otelcolbuilder/build-fips/Makefile') }}" >> $GITHUB_OUTPUT
+          echo "toolchain-cache-key=toolchain-${{inputs.arch_os}}-${{ hashFiles('toolchains/config.mak', 'toolchains/Makefile') }}" >> $GITHUB_OUTPUT
 
       - uses: actions/cache/restore@v4
         with:

diff --git a/pkg/extension/opampextension/opamp_agent.go b/pkg/extension/opampextension/opamp_agent.go
@@ -18,12 +18,10 @@ import (
 	"context"
 	"fmt"
 	"net/http"
-	"net/url"
 	"os"
 	"path/filepath"
 	"reflect"
 	"runtime"
-	"strings"
 
 	"go.opentelemetry.io/collector/component"
 	"go.opentelemetry.io/collector/confmap"
@@ -46,6 +44,8 @@ import (
 	"github.com/open-telemetry/opentelemetry-collector-contrib/extension/sumologicextension"
 )
 
+const DefaultSumoLogicOpAmpURL = "wss://opamp-events.sumologic.com/v1/opamp"
+
 type opampAgent struct {
 	cfg    *Config
 	host   component.Host
@@ -103,13 +103,9 @@ func (o *opampAgent) Start(ctx context.Context, host component.Host) error {
 		return err
 	}
 
-	var baseURL string
-	if o.authExtension != nil {
-		baseURL = o.authExtension.BaseURL()
-	}
-
-	if err := o.setEndpoint(baseURL); err != nil {
-		return err
+	o.endpoint = o.cfg.Endpoint
+	if o.endpoint == "" {
+		o.endpoint = DefaultSumoLogicOpAmpURL
 	}
 
 	if o.authExtension == nil {
@@ -156,7 +152,7 @@ func (o *opampAgent) Reload(ctx context.Context) error {
 	return o.Start(ctx, o.host)
 }
 
-func (o *opampAgent) startClient(ctx context.Context) error {
+func (o *opampAgent) startSettings() types.StartSettings {
 	settings := types.StartSettings{
 		Header:         o.authHeader,
 		OpAMPServerURL: o.endpoint,
@@ -184,6 +180,16 @@ func (o *opampAgent) startClient(ctx context.Context) error {
 		Capabilities:       o.getAgentCapabilities(),
 	}
 
+	if settings.OpAMPServerURL == "" {
+		settings.OpAMPServerURL = DefaultSumoLogicOpAmpURL
+	}
+
+	return settings
+}
+
+func (o *opampAgent) startClient(ctx context.Context) error {
+	settings := o.startSettings()
+
 	o.logger.Debug("Starting OpAMP client...")
 
 	if err := o.opampClient.Start(ctx, settings); err != nil {
@@ -252,33 +258,6 @@ func (o *opampAgent) watchCredentials(ctx context.Context, callback func(ctx con
 	return nil
 }
 
-// setEndpoint sets the OpAMP endpoint based on the collector endpoint.
-// This is a hack, and it should be removed when the backend is able to
-// correctly redirect our OpAMP client to the correct URL.
-func (o *opampAgent) setEndpoint(baseURL string) error {
-	if baseURL == "" {
-		o.endpoint = o.cfg.Endpoint
-		return nil
-	}
-	u, err := url.Parse(baseURL)
-	if err != nil {
-		return fmt.Errorf("url error, cannot set opamp endpoint: %s", err)
-	}
-
-	u.Scheme = "wss"
-	u.Path = "/v1/opamp"
-
-	// These replacements are specific to Sumo Logic's current domain naming,
-	// and are made provisionally for the OTRM beta. In the future, the backend
-	// will inform the agent of the correct OpAMP URL to use.
-	u.Host = strings.Replace(u.Host, "open-events", "opamp-events", 1)
-	u.Host = strings.Replace(u.Host, "open-collectors", "opamp-collectors", 1)
-
-	o.endpoint = u.String()
-
-	return nil
-}
-
 func newOpampAgent(cfg *Config, logger *zap.Logger, build component.BuildInfo, res pcommon.Resource) (*opampAgent, error) {
 	agentType := build.Command
 

diff --git a/pkg/extension/opampextension/opamp_agent_test.go b/pkg/extension/opampextension/opamp_agent_test.go
@@ -26,7 +26,6 @@ import (
 
 	"go.opentelemetry.io/collector/component"
 	"go.opentelemetry.io/collector/component/componenttest"
-	"go.opentelemetry.io/collector/config/confighttp"
 	"go.opentelemetry.io/collector/extension"
 	"go.opentelemetry.io/collector/extension/extensiontest"
 
@@ -203,68 +202,48 @@ func TestShutdown(t *testing.T) {
 	assert.NoError(t, o.Shutdown(context.Background()))
 }
 
-// To be removed when the OpAMP backend correctly advertises its URL.
-// This test is badly written, it reaches into unexported fields to test
-// their contents, but given that it will be removed in a few months, that
-// shouldn't matter too much from a big picture perspective.
-func TestHackSetEndpoint(t *testing.T) {
-	tests := []struct {
-		name         string
-		url          string
-		wantEndpoint string
-	}{
-		{
-			name:         "empty url defaults to config endpoint",
-			wantEndpoint: "wss://example.com",
-		},
-		{
-			name:         "url variant a",
-			url:          "https://sumo-open-events.example.com",
-			wantEndpoint: "wss://sumo-opamp-events.example.com/v1/opamp",
-		},
-		{
-			name:         "url variant b",
-			url:          "https://sumo-open-collectors.example.com",
-			wantEndpoint: "wss://sumo-opamp-collectors.example.com/v1/opamp",
-		},
-		{
-			name:         "url variant c",
-			url:          "https://example.com",
-			wantEndpoint: "wss://example.com/v1/opamp",
-		},
-		{
-			name:         "dev sumologic url",
-			url:          "https://long-open-events.sumologic.net/api/v1",
-			wantEndpoint: "wss://long-opamp-events.sumologic.net/v1/opamp",
-		},
-		{
-			name:         "prod sumologic url",
-			url:          "https://open-collectors.sumologic.com/api/v1",
-			wantEndpoint: "wss://opamp-collectors.sumologic.com/v1/opamp",
-		},
-		{
-
-			name:         "prod sumologic url with region",
-			url:          "https://open-collectors.au.sumologic.com/api/v1/",
-			wantEndpoint: "wss://opamp-collectors.au.sumologic.com/v1/opamp",
-		},
+func TestStart(t *testing.T) {
+	d, err := os.MkdirTemp("", "opamp.d")
+	assert.NoError(t, err)
+	defer os.RemoveAll(d)
+
+	cfg := createDefaultConfig().(*Config)
+	cfg.ClientConfig.Auth = nil
+	cfg.RemoteConfigurationDirectory = d
+	set := extensiontest.NewNopSettings()
+	o, err := newOpampAgent(cfg, set.Logger, set.BuildInfo, set.Resource)
+	assert.NoError(t, err)
+
+	assert.NoError(t, o.Start(context.Background(), componenttest.NewNopHost()))
+}
+
+func TestReload(t *testing.T) {
+	d, err := os.MkdirTemp("", "opamp.d")
+	assert.NoError(t, err)
+	defer os.RemoveAll(d)
+
+	cfg := createDefaultConfig().(*Config)
+	cfg.ClientConfig.Auth = nil
+	cfg.RemoteConfigurationDirectory = d
+	set := extensiontest.NewNopSettings()
+	o, err := newOpampAgent(cfg, set.Logger, set.BuildInfo, set.Resource)
+	assert.NoError(t, err)
+
+	ctx := context.Background()
+	assert.NoError(t, o.Start(ctx, componenttest.NewNopHost()))
+	assert.NoError(t, o.Reload(ctx))
+}
+
+func TestDefaultEndpointSetOnStart(t *testing.T) {
+	cfg := createDefaultConfig().(*Config)
+	set := extensiontest.NewNopSettings()
+	o, err := newOpampAgent(cfg, set.Logger, set.BuildInfo, set.Resource)
+	if err != nil {
+		t.Fatal(err)
 	}
-	for _, test := range tests {
-		t.Run(test.name, func(t *testing.T) {
-			agent := &opampAgent{cfg: &Config{
-				ClientConfig: confighttp.ClientConfig{
-					Endpoint: "wss://example.com",
-				},
-			}}
-			if err := agent.setEndpoint(test.url); err != nil {
-				// can only happen with an invalid URL, which is quite hard
-				// to even come up with for Go's URL package
-				t.Fatal(err)
-			}
-			if got, want := agent.endpoint, test.wantEndpoint; got != want {
-				t.Errorf("didn't get expected endpoint: got %q, want %q", got, want)
-			}
-		})
+	settings := o.startSettings()
+	if settings.OpAMPServerURL != DefaultSumoLogicOpAmpURL {
+		t.Error("expected unconfigured opamp endpoint to result in default sumo opamp url setting")
 	}
 }
 
@@ -290,30 +269,3 @@ func TestNewOpampAgentAttributes(t *testing.T) {
 	assert.Equal(t, "sumo.0", o.agentVersion)
 	assert.Equal(t, "7RK6DW2K4V8RCSQBKZ02EJ84FC", o.instanceId.String())
 }
-
-func TestStart(t *testing.T) {
-	d, err := os.MkdirTemp("", "opamp.d")
-	assert.NoError(t, err)
-	defer os.RemoveAll(d)
-	cfg, set := setupWithRemoteConfig(t, d)
-	cfg.ClientConfig.Auth = nil
-	cfg.RemoteConfigurationDirectory = d
-	o, err := newOpampAgent(cfg, set.Logger, set.BuildInfo, set.Resource)
-	assert.NoError(t, err)
-
-	assert.NoError(t, o.Start(context.Background(), componenttest.NewNopHost()))
-}
-
-func TestReload(t *testing.T) {
-	d, err := os.MkdirTemp("", "opamp.d")
-	assert.NoError(t, err)
-	defer os.RemoveAll(d)
-	cfg, set := setupWithRemoteConfig(t, d)
-	cfg.ClientConfig.Auth = nil
-	o, err := newOpampAgent(cfg, set.Logger, set.BuildInfo, set.Resource)
-	assert.NoError(t, err)
-
-	ctx := context.Background()
-	assert.NoError(t, o.Start(ctx, componenttest.NewNopHost()))
-	assert.NoError(t, o.Reload(ctx))
-}
diff --git a/toolchains/config.mak b/toolchains/config.mak
@@ -10,3 +10,10 @@ COMMON_CONFIG += CFLAGS="-g0 -Os" CXXFLAGS="-g0 -Os" LDFLAGS="-s"
 GCC_CONFIG += --enable-languages=c
 # add -nv to download command for tidy CI output
 DL_CMD = wget -nv -c -O
+
+# GNU_SITE defaults to https://ftpmirror.gnu.org/gnu which will redirect to
+# another mirror. At the time of this commit, one of the mirrors that is
+# frequently being picked has an expired certificate. Remove or comment out the
+# following line once https://mirror.us-midwest-1.nexcess.net has a valid
+# certificate.
+GNU_SITE = https://mirror.team-cymru.com/gnu