Changes as per PR review comments

Author: Akshay2191

api/grpc/mpi/v1/command.pb.go

@@ -352,7 +352,7 @@ message APIDetails {
     string location = 1;
     // the API listen directive
     string listen =  2;
-    // the API Ca directive
+    // the API CA file path
     string Ca = 3;
 }
 

api/grpc/mpi/v1/command.proto

@@ -678,7 +678,7 @@ Perform an associated API action on an instance
 | ----- | ---- | ----- | ----------- |
 | location | [string](#string) |  | the API location directive |
 | listen | [string](#string) |  | the API listen directive |
-| Ca | [string](#string) |  | the API Ca directive |
+| Ca | [string](#string) |  | the API CA file path |
 
 
 

docs/proto/protos.md

@@ -68,11 +68,11 @@ func (s *NginxStubStatusScraper) Start(_ context.Context, _ component.Host) erro
 	httpClient := http.DefaultClient
 	caCertLocation := s.cfg.APIDetails.Ca
 	if caCertLocation != "" {
-		s.settings.Logger.Debug("Reading from Location for Ca Cert : ", zap.Any(caCertLocation, caCertLocation))
+		s.settings.Logger.Debug("Reading CA certificate", zap.Any("file_path", caCertLocation))
 		caCert, err := os.ReadFile(caCertLocation)
 		if err != nil {
-			s.settings.Logger.Error("Error starting NGINX stub scraper. "+
-				"Failed to read CA certificate : ", zap.Error(err))
+			s.settings.Logger.Error("Error starting NGINX stub status scraper. "+
+				"Failed to read CA certificate", zap.Error(err))
 
 			return nil
 		}

internal/collector/nginxossreceiver/internal/scraper/stubstatus/stub_status_scraper.go

@@ -7,20 +7,13 @@ package stubstatus
 
 import (
 	"context"
-	"crypto/rand"
-	"crypto/rsa"
 	"crypto/tls"
 	"crypto/x509"
-	"crypto/x509/pkix"
-	"encoding/pem"
-	"math/big"
 	"net"
 	"net/http"
 	"net/http/httptest"
 	"os"
-	"path/filepath"
 	"testing"
-	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -29,71 +22,39 @@ import (
 	"go.opentelemetry.io/collector/receiver/receivertest"
 
 	"github.com/nginx/agent/v3/internal/collector/nginxossreceiver/internal/config"
+	"github.com/nginx/agent/v3/test/helpers"
 )
 
 func TestStubStatusScraperTLS(t *testing.T) {
-	// Create a test CA certificate and key
-	ca := &x509.Certificate{
-		SerialNumber: big.NewInt(1),
-		Subject: pkix.Name{
-			Organization: []string{"NGINX Agent Test CA"},
-		},
-		NotBefore:             time.Now(),
-		NotAfter:              time.Now().AddDate(10, 0, 0),
-		KeyUsage:              x509.KeyUsageCertSign | x509.KeyUsageDigitalSignature,
-		ExtKeyUsage:           []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth, x509.ExtKeyUsageServerAuth},
-		BasicConstraintsValid: true,
-		IsCA:                  true,
-	}
-
-	caPrivKey, caPrivKeyErr := rsa.GenerateKey(rand.Reader, 2048)
-	require.NoError(t, caPrivKeyErr)
-
-	caBytes, caBytesErr := x509.CreateCertificate(rand.Reader, ca, ca, &caPrivKey.PublicKey, caPrivKey)
-	require.NoError(t, caBytesErr)
-
-	// Create a test server certificate signed by the CA
-	cert := &x509.Certificate{
-		SerialNumber: big.NewInt(2),
-		Subject: pkix.Name{
-			Organization: []string{"NGINX Agent Test"},
-		},
-		NotBefore:    time.Now(),
-		NotAfter:     time.Now().AddDate(10, 0, 0),
-		SubjectKeyId: []byte{1, 2, 3, 4, 6},
-		ExtKeyUsage:  []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth, x509.ExtKeyUsageServerAuth},
-		KeyUsage:     x509.KeyUsageDigitalSignature,
-		IPAddresses:  []net.IP{net.IPv4(127, 0, 0, 1)},
-		DNSNames:     []string{"localhost"},
-	}
-
-	certPrivKey, certPrivKeyErr := rsa.GenerateKey(rand.Reader, 2048)
-	require.NoError(t, certPrivKeyErr)
-
-	certBytes, certBytesErr := x509.CreateCertificate(rand.Reader, cert, ca, &certPrivKey.PublicKey, caPrivKey)
-	require.NoError(t, certBytesErr)
+	// Generate self-signed certificate using helper
+	keyBytes, certBytes := helpers.GenerateSelfSignedCert(t)
 
 	// Create a temporary directory for test files
 	tempDir := t.TempDir()
 
-	// Save CA certificate to a file
-	caFile := filepath.Join(tempDir, "ca.crt")
-	caPEM := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: caBytes})
-	writeErr := os.WriteFile(caFile, caPEM, 0o600)
-	require.NoError(t, writeErr)
+	// Save certificate to a file
+	certFile := helpers.WriteCertFiles(t, tempDir, helpers.Cert{
+		Name:     "server.crt",
+		Type:     "CERTIFICATE",
+		Contents: certBytes,
+	})
+
+	// Parse the private key
+	key, err := x509.ParsePKCS1PrivateKey(keyBytes)
+	require.NoError(t, err)
+
+	// Create a TLS config with our self-signed certificate
+	tlsCert := tls.Certificate{
+		Certificate: [][]byte{certBytes},
+		PrivateKey:  key,
+	}
 
-	// Create a TLS config for the server
 	serverTLSConfig := &tls.Config{
-		MinVersion: tls.VersionTLS13,
-		Certificates: []tls.Certificate{
-			{
-				Certificate: [][]byte{certBytes},
-				PrivateKey:  certPrivKey,
-			},
-		},
+		MinVersion:   tls.VersionTLS13,
+		Certificates: []tls.Certificate{tlsCert},
 	}
 
-	// Create a test server with TLS
+	// Create a test server with our custom TLS config
 	server := httptest.NewUnstartedServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
 		if req.URL.Path == "/status" {
 			rw.WriteHeader(http.StatusOK)
@@ -112,56 +73,65 @@ Reading: 6 Writing: 179 Waiting: 106
 	server.StartTLS()
 	defer server.Close()
 
-	// Test with TLS configuration
-	t.Run("with TLS CA", func(t *testing.T) {
+	// Test with TLS configuration using our self-signed certificate
+	t.Run("with self-signed TLS", func(t *testing.T) {
 		cfg, ok := config.CreateDefaultConfig().(*config.Config)
 		require.True(t, ok)
 
 		cfg.APIDetails.URL = server.URL + "/status"
-		cfg.APIDetails.Ca = caFile
+		// Use the self-signed certificate for verification
+		cfg.APIDetails.Ca = certFile
 
 		scraper := NewScraper(receivertest.NewNopSettings(component.Type{}), cfg)
 
-		err := scraper.Start(context.Background(), componenttest.NewNopHost())
-		require.NoError(t, err)
+		startErr := scraper.Start(context.Background(), componenttest.NewNopHost())
+		require.NoError(t, startErr)
 
 		_, err = scraper.Scrape(context.Background())
-		assert.NoError(t, err)
+		assert.NoError(t, err, "Scraping with self-signed certificate should succeed")
 	})
 }
 
 func TestStubStatusScraperUnixSocket(t *testing.T) {
-	// Use a shorter path for the socket to avoid path length issues
-	socketPath := filepath.Join(os.TempDir(), "test-nginx.sock")
-	// Clean up the socket file after the test
-	t.Cleanup(func() { os.Remove(socketPath) })
-
-	// Create a Unix domain socket listener
-	listener, listenErr := net.Listen("unix", socketPath)
-	require.NoError(t, listenErr)
-	defer listener.Close()
-
-	// Start a simple HTTP server on the Unix socket
-	server := &http.Server{
-		Handler: http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
-			if req.URL.Path == "/status" {
-				rw.WriteHeader(http.StatusOK)
-				_, _ = rw.Write([]byte(`Active connections: 291
+	// Create a test server with a Unix domain socket
+	handler := http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
+		if req.URL.Path == "/status" {
+			rw.WriteHeader(http.StatusOK)
+			_, _ = rw.Write([]byte(`Active connections: 291
 server accepts handled requests
  16630948 16630946 31070465
 Reading: 6 Writing: 179 Waiting: 106
 `))
 
-				return
-			}
-			rw.WriteHeader(http.StatusNotFound)
-		}),
+			return
+		}
+		rw.WriteHeader(http.StatusNotFound)
+	})
+
+	// Create a socket file in a temporary directory with a shorter path
+	socketPath := "/tmp/nginx-test.sock"
+
+	// Clean up any existing socket file
+	os.Remove(socketPath)
+
+	// Create a listener for the Unix socket
+	listener, err := net.Listen("unix", socketPath)
+	require.NoError(t, err, "Failed to create Unix socket listener")
+
+	// Create a test server with our custom listener
+	server := &httptest.Server{
+		Listener: listener,
+		Config:   &http.Server{Handler: handler},
 	}
 
-	go func() {
-		_ = server.Serve(listener)
-	}()
-	defer server.Close()
+	// Start the server
+	server.Start()
+
+	// Ensure cleanup of the socket file
+	t.Cleanup(func() {
+		server.Close()
+		os.Remove(socketPath)
+	})
 
 	// Test with Unix socket
 	t.Run("with Unix socket", func(t *testing.T) {
@@ -173,8 +143,8 @@ Reading: 6 Writing: 179 Waiting: 106
 
 		scraper := NewScraper(receivertest.NewNopSettings(component.Type{}), cfg)
 
-		err := scraper.Start(context.Background(), componenttest.NewNopHost())
-		require.NoError(t, err)
+		startErr := scraper.Start(context.Background(), componenttest.NewNopHost())
+		require.NoError(t, startErr)
 
 		_, err = scraper.Scrape(context.Background())
 		assert.NoError(t, err)

internal/collector/nginxossreceiver/internal/scraper/stubstatus/stub_status_scraper_tls_test.go

@@ -87,10 +87,12 @@ func (nps *NginxPlusScraper) Start(_ context.Context, _ component.Host) error {
 	httpClient := http.DefaultClient
 	caCertLocation := nps.cfg.APIDetails.Ca
 	if caCertLocation != "" {
-		nps.logger.Debug("Reading from Location for Ca Cert : ", zap.Any(caCertLocation, caCertLocation))
+		nps.logger.Debug("Reading CA certificate", zap.Any("file_path", caCertLocation))
 		caCert, err := os.ReadFile(caCertLocation)
 		if err != nil {
-			nps.logger.Error("Unable to start NGINX Plus scraper. Failed to read CA certificate: %v", zap.Error(err))
+			nps.logger.Error("Error starting NGINX stub status scraper. "+
+				"Failed to read CA certificate", zap.Error(err))
+
 			return err
 		}
 		caCertPool := x509.NewCertPool()

internal/collector/nginxplusreceiver/scraper.go

@@ -792,7 +792,7 @@ func resolveDataPlaneConfig() *DataPlaneConfig {
 			ReloadMonitoringPeriod: viperInstance.GetDuration(NginxReloadMonitoringPeriodKey),
 			TreatWarningsAsErrors:  viperInstance.GetBool(NginxTreatWarningsAsErrorsKey),
 			ExcludeLogs:            viperInstance.GetStringSlice(NginxExcludeLogsKey),
-			ApiTls:                 TLSConfig{Ca: viperInstance.GetString(NginxApiTlsCa)},
+			APITls:                 TLSConfig{Ca: viperInstance.GetString(NginxApiTlsCa)},
 		},
 	}
 }

internal/config/config.go

@@ -61,7 +61,7 @@ type (
 	}
 
 	NginxDataPlaneConfig struct {
-		ApiTls                 TLSConfig     `yaml:"api_tls"                  mapstructure:"api_tls"`
+		APITls                 TLSConfig     `yaml:"api_tls"                  mapstructure:"api_tls"`
 		ExcludeLogs            []string      `yaml:"exclude_logs"             mapstructure:"exclude_logs"`
 		ReloadMonitoringPeriod time.Duration `yaml:"reload_monitoring_period" mapstructure:"reload_monitoring_period"`
 		TreatWarningsAsErrors  bool          `yaml:"treat_warnings_as_errors" mapstructure:"treat_warnings_as_errors"`

internal/config/types.go

@@ -554,7 +554,7 @@ func (ncp *NginxConfigParser) sslCert(ctx context.Context, file, rootDir string)
 func (ncp *NginxConfigParser) apiCallback(ctx context.Context, parent,
 	current *crossplane.Directive, apiType string,
 ) *model.APIDetails {
-	urls := ncp.urlsForLocationDirectiveAPIDetails(parent, current, apiType)
+	urls := ncp.urlsForLocationDirectiveAPIDetails(ctx, parent, current, apiType)
 	if len(urls) > 0 {
 		slog.DebugContext(ctx, fmt.Sprintf("%d potential %s urls", len(urls), apiType), "urls", urls)
 	}
@@ -643,7 +643,7 @@ func (ncp *NginxConfigParser) pingAPIEndpoint(ctx context.Context, statusAPIDeta
 
 // nolint: revive
 func (ncp *NginxConfigParser) urlsForLocationDirectiveAPIDetails(
-	parent, current *crossplane.Directive,
+	ctx context.Context, parent, current *crossplane.Directive,
 	locationDirectiveName string,
 ) []*model.APIDetails {
 	var urls []*model.APIDetails
@@ -652,7 +652,7 @@ func (ncp *NginxConfigParser) urlsForLocationDirectiveAPIDetails(
 	caCertLocation := ""
 	// If SSl is enabled, check if CA cert is provided and the location is allowed
 	if isSSL {
-		caCertLocation = ncp.getCACertLocation()
+		caCertLocation = ncp.getCACertLocation(ctx)
 	}
 	// process from the location block
 	if current.Directive != locationDirective {
@@ -834,13 +834,12 @@ func (ncp *NginxConfigParser) socketClient(socketPath string) *http.Client {
 // prepareHTTPClient handles TLS config
 func (ncp *NginxConfigParser) prepareHTTPClient(ctx context.Context) (*http.Client, error) {
 	httpClient := http.DefaultClient
-	caCertLocation := ncp.agentConfig.DataPlaneConfig.Nginx.ApiTls.Ca
+	caCertLocation := ncp.agentConfig.DataPlaneConfig.Nginx.APITls.Ca
 
 	if caCertLocation != "" && ncp.agentConfig.IsDirectoryAllowed(caCertLocation) {
-		slog.DebugContext(ctx, "Reading from Location for Ca Cert : ", "cacertlocation", caCertLocation)
+		slog.DebugContext(ctx, "Reading CA certificate", "file_path", caCertLocation)
 		caCert, err := os.ReadFile(caCertLocation)
 		if err != nil {
-			slog.ErrorContext(ctx, "Failed to read CA certificate", "error", err)
 			return nil, err
 		}
 		caCertPool := x509.NewCertPool()
@@ -860,12 +859,12 @@ func (ncp *NginxConfigParser) prepareHTTPClient(ctx context.Context) (*http.Clie
 }
 
 // Populate the CA cert location based ondirectory allowance.
-func (ncp *NginxConfigParser) getCACertLocation() string {
-	caCertLocation := ncp.agentConfig.DataPlaneConfig.Nginx.ApiTls.Ca
+func (ncp *NginxConfigParser) getCACertLocation(ctx context.Context) string {
+	caCertLocation := ncp.agentConfig.DataPlaneConfig.Nginx.APITls.Ca
 
 	if caCertLocation != "" && !ncp.agentConfig.IsDirectoryAllowed(caCertLocation) {
 		// If SSL is enabled but CA cert is provided and not allowed, treat it as if no CA cert
-		slog.Warn("CA certificate location is not allowed, treating as if no CA cert provided.")
+		slog.WarnContext(ctx, "CA certificate location is not allowed, treating as if no CA cert provided.")
 		return ""
 	}
 

internal/datasource/config/nginx_config_parser.go

@@ -1172,9 +1172,9 @@ func TestNginxConfigParser_urlsForLocationDirective(t *testing.T) {
 			assert.Len(t, xpConf.Parsed, 1)
 			err = ncp.crossplaneConfigTraverse(ctx, &xpConf,
 				func(ctx context.Context, parent, directive *crossplane.Directive) error {
-					_oss := ncp.urlsForLocationDirectiveAPIDetails(parent, directive,
+					_oss := ncp.urlsForLocationDirectiveAPIDetails(ctx, parent, directive,
 						stubStatusAPIDirective)
-					_plus := ncp.urlsForLocationDirectiveAPIDetails(parent, directive, plusAPIDirective)
+					_plus := ncp.urlsForLocationDirectiveAPIDetails(ctx, parent, directive, plusAPIDirective)
 					oss = append(oss, _oss...)
 					plus = append(plus, _plus...)