worked on review comments

Author: Akshay2191

internal/collector/otel_collector_plugin.go

@@ -232,7 +232,9 @@ func (oc *Collector) bootup(ctx context.Context) error {
 			return
 		}
 
-		oc.setProxyIfNeeded(ctx)
+		if oc.config.IsCommandServerProxyConfigured() {
+			oc.setProxyIfNeeded(ctx)
+		}
 
 		appErr := oc.service.Run(ctx)
 		if appErr != nil {
@@ -392,7 +394,10 @@ func (oc *Collector) restartCollector(ctx context.Context) {
 	if !oc.tryCreateCollector(ctx) {
 		return
 	}
-	oc.setProxyIfNeeded(ctx)
+
+	if oc.config.IsCommandServerProxyConfigured() {
+		oc.setProxyIfNeeded(ctx)
+	}
 	var runCtx context.Context
 	runCtx, oc.cancel = context.WithCancel(ctx)
 	if !oc.stopped {
@@ -778,29 +783,17 @@ func escapeString(input string) string {
 }
 
 func (oc *Collector) setExporterProxyEnvVars(ctx context.Context) {
-	// Validate proxy fields
-	if oc.config.Command.Server.Proxy == nil {
-		slog.InfoContext(ctx, "Proxy configuration is not setup; skipping Proxy setup")
-		return
-	}
-
 	proxy := oc.config.Command.Server.Proxy
-	if proxy.URL == "" {
-		slog.WarnContext(ctx, "Proxy URL is empty; skipping Proxy setup")
-		return
-	}
 	proxyURL := proxy.URL
-	u, err := url.Parse(proxyURL)
+	parsedProxyURL, err := url.Parse(proxyURL)
 	if err != nil {
 		slog.ErrorContext(ctx, "Malformed proxy URL; skipping Proxy setup", "url", proxyURL, "error", err)
 		return
 	}
 
 	auth := ""
-	if oc.config.Command.Server.Proxy.AuthMethod != "" &&
-		strings.TrimSpace(oc.config.Command.Server.Proxy.AuthMethod) != "" {
-		auth = strings.TrimSpace(oc.config.Command.Server.Proxy.AuthMethod)
-		slog.DebugContext(ctx, "auth string", "auth", auth)
+	if proxy.AuthMethod != "" && strings.TrimSpace(proxy.AuthMethod) != "" {
+		auth = strings.TrimSpace(proxy.AuthMethod)
 	}
 
 	// Use the standalone setProxyWithBasicAuth function
@@ -809,9 +802,8 @@ func (oc *Collector) setExporterProxyEnvVars(ctx context.Context) {
 		return
 	}
 	authLower := strings.ToLower(auth)
-	slog.DebugContext(ctx, "auth To Lower string", "authlower", authLower)
 	if authLower == "basic" {
-		setProxyWithBasicAuth(ctx, proxy, u)
+		setProxyWithBasicAuth(ctx, proxy, parsedProxyURL)
 	} else {
 		slog.ErrorContext(ctx, "Unknown auth type for proxy; Aborting Proxy Setup", "auth", auth, "url", proxyURL)
 	}
@@ -829,14 +821,14 @@ func setProxyEnvs(ctx context.Context, proxyEnvURL, msg string) {
 }
 
 // setProxyWithBasicAuth sets the proxy environment variables with basic auth credentials.
-func setProxyWithBasicAuth(ctx context.Context, proxy *config.Proxy, u *url.URL) {
+func setProxyWithBasicAuth(ctx context.Context, proxy *config.Proxy, parsedProxyURL *url.URL) {
 	username := proxy.Username
 	password := proxy.Password
 	if username == "" || password == "" {
 		slog.ErrorContext(ctx, "Username or password missing for basic auth")
 		return
 	}
-	u.User = url.UserPassword(username, password)
-	proxyURL := u.String()
+	parsedProxyURL.User = url.UserPassword(username, password)
+	proxyURL := parsedProxyURL.String()
 	setProxyEnvs(ctx, proxyURL, "Setting Proxy with basic auth")
 }

internal/collector/otel_collector_plugin_test.go

@@ -842,13 +842,7 @@ func TestSetExporterProxyEnvVars(t *testing.T) {
 		{
 			name:        "No proxy config",
 			proxy:       nil,
-			expectedLog: "Proxy configuration is not setup; skipping Proxy setup",
-			setEnv:      false,
-		},
-		{
-			name:        "Empty proxy URL",
-			proxy:       &config.Proxy{URL: ""},
-			expectedLog: "Proxy URL is empty; skipping Proxy setup",
+			expectedLog: "Proxy configuration is not setup. Unable to configure proxy for OTLP exporter",
 			setEnv:      false,
 		},
 		{
@@ -885,24 +879,38 @@ func TestSetExporterProxyEnvVars(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			logBuf.Reset()
+
+			_ = os.Unsetenv("HTTP_PROXY")
+			_ = os.Unsetenv("HTTPS_PROXY")
+
 			tmpDir := t.TempDir()
 			cfg := types.OTelConfig(t)
 			cfg.Collector.Log.Path = filepath.Join(tmpDir, "otel-collector-test.log")
 			cfg.Command.Server.Proxy = tt.proxy
+
+			// If the proxy is nil, the production code would never call the setter functions.
+			// added this check to prevent the panic error in UT.
+			if cfg.Command.Server.Proxy == nil {
+				// For the nil proxy case, we expect nothing to happen.
+				assert.Empty(t, os.Getenv("HTTP_PROXY"))
+				assert.Empty(t, os.Getenv("HTTPS_PROXY"))
+
+				return
+			}
+
 			collector, err := NewCollector(cfg)
 			require.NoError(t, err)
+
 			collector.setExporterProxyEnvVars(ctx)
+
 			helpers.ValidateLog(t, tt.expectedLog, logBuf)
+
 			if tt.setEnv {
-				// Check that HTTP_PROXY and HTTPS_PROXY are set
-				httpProxy := os.Getenv("HTTP_PROXY")
-				httpsProxy := os.Getenv("HTTPS_PROXY")
-				assert.NotEmpty(t, httpProxy)
-				assert.NotEmpty(t, httpsProxy)
+				assert.NotEmpty(t, os.Getenv("HTTP_PROXY"))
+				assert.NotEmpty(t, os.Getenv("HTTPS_PROXY"))
 			} else {
-				// Unset for next test
-				os.Unsetenv("HTTP_PROXY")
-				os.Unsetenv("HTTPS_PROXY")
+				assert.Empty(t, os.Getenv("HTTP_PROXY"))
+				assert.Empty(t, os.Getenv("HTTPS_PROXY"))
 			}
 		})
 	}

internal/config/types.go

@@ -472,6 +472,14 @@ func (c *Config) NewContextWithLabels(ctx context.Context) context.Context {
 	return metadata.NewOutgoingContext(ctx, md)
 }
 
+func (c *Config) IsCommandServerProxyConfigured() bool {
+	if c.Command.Server.Proxy == nil {
+		return false
+	}
+
+	return c.Command.Server.Proxy.URL != ""
+}
+
 // isAllowedDir checks if the given path is in the list of allowed directories.
 // It returns true if the path is allowed, false otherwise.
 // If the path is allowed but does not exist, it also logs a warning.