Add support for ssl in v3 #1175
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
chore
aphralG
reviewed
Jul 21, 2025
|
|
||
| func TestStubStatusScraperTLS(t *testing.T) { | ||
| // Create a test CA certificate and key | ||
| ca := &x509.Certificate{ |
There was a problem hiding this comment.
Could the GenerateSelfSignedCert helper in test/helpers/certs_utils.go be used here
aphralG
reviewed
Jul 21, 2025
| require.NoError(t, caBytesErr) | ||
|
|
||
| // Create a test server certificate signed by the CA | ||
| cert := &x509.Certificate{ |
aphralG
reviewed
Jul 21, 2025
| tempDir := t.TempDir() | ||
|
|
||
| // Save CA certificate to a file | ||
| caFile := filepath.Join(tempDir, "ca.crt") |
There was a problem hiding this comment.
Can WriteCertFiles from test/helpers/certs_utils.go be used here
aphralG
reviewed
Jul 21, 2025
| defer listener.Close() | ||
|
|
||
| // Start a simple HTTP server on the Unix socket | ||
| server := &http.Server{ |
There was a problem hiding this comment.
Can httptest be used here instead
aphralG
reviewed
Jul 21, 2025
|
|
||
| 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") |
There was a problem hiding this comment.
Can t.TempDir be used here instead of os.TempDir()
aphralG
reviewed
Jul 21, 2025
| } | ||
|
|
||
| // Create a test server with TLS | ||
| server := httptest.NewUnstartedServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { |
There was a problem hiding this comment.
Can httptest.NewTLSServer be used here
internal/config/types.go
Outdated
| @@ -61,6 +61,7 @@ type ( | |||
| } | |||
|
|
|||
| NginxDataPlaneConfig struct { | |||
| ApiTls TLSConfig `yaml:"api_tls" mapstructure:"api_tls"` | |||
There was a problem hiding this comment.
Suggested change
| ApiTls TLSConfig `yaml:"api_tls" mapstructure:"api_tls"` | |
| APITls TLSConfig `yaml:"api_tls" mapstructure:"api_tls"` |
dhurley
reviewed
Jul 21, 2025
api/grpc/mpi/v1/command.proto
Outdated
Comment on lines
355
to
356
| // the API Ca directive | ||
| string Ca = 3; |
There was a problem hiding this comment.
Suggested change
| // the API Ca directive | |
| string Ca = 3; | |
| // the API CA file path | |
| string ca = 3; |
| @@ -63,6 +66,28 @@ func (s *NginxStubStatusScraper) ID() component.ID { | |||
| func (s *NginxStubStatusScraper) Start(_ context.Context, _ component.Host) error { | |||
| s.logger.Info("Starting NGINX stub status scraper") | |||
| httpClient := http.DefaultClient | |||
| caCertLocation := s.cfg.APIDetails.Ca | |||
| if caCertLocation != "" { | |||
| s.settings.Logger.Debug("Reading from Location for Ca Cert : ", zap.Any(caCertLocation, caCertLocation)) | |||
There was a problem hiding this comment.
Suggested change
| 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)) |
Comment on lines
74
to
75
| s.settings.Logger.Error("Error starting NGINX stub scraper. "+ | ||
| "Failed to read CA certificate : ", zap.Error(err)) |
There was a problem hiding this comment.
Suggested change
| 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)) |
| @@ -82,6 +85,26 @@ func (nps *NginxPlusScraper) ID() component.ID { | |||
| func (nps *NginxPlusScraper) Start(_ context.Context, _ component.Host) error { | |||
| endpoint := strings.TrimPrefix(nps.cfg.APIDetails.URL, "unix:") | |||
| httpClient := http.DefaultClient | |||
| caCertLocation := nps.cfg.APIDetails.Ca | |||
| if caCertLocation != "" { | |||
| nps.logger.Debug("Reading from Location for Ca Cert : ", zap.Any(caCertLocation, caCertLocation)) | |||
There was a problem hiding this comment.
Suggested change
| nps.logger.Debug("Reading from Location for Ca Cert : ", zap.Any(caCertLocation, caCertLocation)) | |
| nps.logger.Debug("Reading CA certificate", zap.Any("file_path", caCertLocation)) |
| nps.logger.Debug("Reading from Location for Ca Cert : ", zap.Any(caCertLocation, 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)) |
There was a problem hiding this comment.
Suggested change
| 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)) |
|
|
||
| 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.") |
There was a problem hiding this comment.
Can you use WarnContext function instead?
| slog.Debug("Reading from Location for Ca Cert : ", "cacertlocation", caCertLocation) | ||
| caCert, err := os.ReadFile(caCertLocation) | ||
| if err != nil { | ||
| slog.Error("Unable to Create NGINX Plus client. Failed to read CA certificate : ", "err", err) |
There was a problem hiding this comment.
No need to log error since you are returning the error anyways
| slog.DebugContext(ctx, "Reading from Location for Ca Cert : ", "cacertlocation", caCertLocation) | ||
| caCert, err := os.ReadFile(caCertLocation) | ||
| if err != nil { | ||
| slog.ErrorContext(ctx, "Failed to read CA certificate", "error", err) |
There was a problem hiding this comment.
No need to log error since you are returning the error anyways
| @@ -348,6 +351,26 @@ func (r *ResourceService) createPlusClient(instance *mpi.Instance) (*client.Ngin | |||
| } | |||
|
|
|||
| httpClient := http.DefaultClient | |||
| caCertLocation := plusAPI.GetCa() | |||
| if caCertLocation != "" { | |||
| slog.Debug("Reading from Location for Ca Cert : ", "cacertlocation", caCertLocation) | |||
There was a problem hiding this comment.
Suggested change
| slog.Debug("Reading from Location for Ca Cert : ", "cacertlocation", caCertLocation) | |
| slog.Debug("Reading CA certificate", zap.Any("file_path", caCertLocation) |
| caCertLocation := ncp.agentConfig.DataPlaneConfig.Nginx.ApiTls.Ca | ||
|
|
||
| if caCertLocation != "" && ncp.agentConfig.IsDirectoryAllowed(caCertLocation) { | ||
| slog.DebugContext(ctx, "Reading from Location for Ca Cert : ", "cacertlocation", caCertLocation) |
There was a problem hiding this comment.
Suggested change
| slog.DebugContext(ctx, "Reading from Location for Ca Cert : ", "cacertlocation", caCertLocation) | |
| slog.DebugContext(ctx, "Reading CA certificate", zap.Any("file_path", caCertLocation) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed changes
Describe the use case and detail of the change. If this PR addresses an issue on GitHub, make sure to include a link to that issue using one of the supported keywords here in this description (not in the title of the PR).
Checklist
Before creating a PR, run through this checklist and mark each as complete.
CONTRIBUTINGdocumentmake install-toolsand have attached any dependency changes to this pull requestREADME.md)