-
Notifications
You must be signed in to change notification settings - Fork 100
NGINX Configs to Reference Remote External Files #1389
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
ae7fa14
14f1190
84b2310
d31b2df
b7a5c70
501e08f
b7eaef7
a83e45b
529c839
2a51d22
f22834f
4a2b917
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -157,6 +157,7 @@ func ResolveConfig() (*Config, error) { | |||||||||
| Features: viperInstance.GetStringSlice(FeaturesKey), | ||||||||||
| Labels: resolveLabels(), | ||||||||||
| LibDir: viperInstance.GetString(LibDirPathKey), | ||||||||||
| ExternalDataSource: resolveExternalDataSource(), | ||||||||||
| SyslogServer: resolveSyslogServer(), | ||||||||||
| } | ||||||||||
|
|
||||||||||
|
|
@@ -475,6 +476,7 @@ func registerFlags() { | |||||||||
| registerCollectorFlags(fs) | ||||||||||
| registerClientFlags(fs) | ||||||||||
| registerDataPlaneFlags(fs) | ||||||||||
| registerExternalDataSourceFlags(fs) | ||||||||||
|
|
||||||||||
| fs.SetNormalizeFunc(normalizeFunc) | ||||||||||
|
|
||||||||||
|
|
@@ -489,6 +491,24 @@ func registerFlags() { | |||||||||
| }) | ||||||||||
| } | ||||||||||
|
|
||||||||||
| func registerExternalDataSourceFlags(fs *flag.FlagSet) { | ||||||||||
| fs.String( | ||||||||||
| ExternalDataSourceProxyUrlKey, | ||||||||||
| DefExternalDataSourceProxyUrl, | ||||||||||
| "Url to the proxy service to fetch the external file.", | ||||||||||
| ) | ||||||||||
| fs.StringSlice( | ||||||||||
| ExternalDataSourceAllowDomainsKey, | ||||||||||
| []string{}, | ||||||||||
| "List of allowed domains for external data sources.", | ||||||||||
| ) | ||||||||||
| fs.Int64( | ||||||||||
| ExternalDataSourceMaxBytesKey, | ||||||||||
| DefExternalDataSourceMaxBytes, | ||||||||||
| "Maximum size in bytes for external data sources.", | ||||||||||
| ) | ||||||||||
| } | ||||||||||
|
|
||||||||||
| func registerDataPlaneFlags(fs *flag.FlagSet) { | ||||||||||
| fs.Duration( | ||||||||||
| NginxReloadMonitoringPeriodKey, | ||||||||||
|
|
@@ -628,6 +648,11 @@ func registerClientFlags(fs *flag.FlagSet) { | |||||||||
| DefMaxFileSize, | ||||||||||
| "Max file size in bytes.", | ||||||||||
| ) | ||||||||||
| fs.Duration( | ||||||||||
| ClientFileDownloadTimeoutKey, | ||||||||||
| DefClientFileDownloadTimeout, | ||||||||||
| "Timeout value in seconds, to downloading file for config apply.", | ||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
| ) | ||||||||||
|
|
||||||||||
| fs.Int( | ||||||||||
| ClientGRPCMaxParallelFileOperationsKey, | ||||||||||
|
|
@@ -1120,6 +1145,7 @@ func resolveClient() *Client { | |||||||||
| RandomizationFactor: viperInstance.GetFloat64(ClientBackoffRandomizationFactorKey), | ||||||||||
| Multiplier: viperInstance.GetFloat64(ClientBackoffMultiplierKey), | ||||||||||
| }, | ||||||||||
| FileDownloadTimeout: viperInstance.GetDuration(ClientFileDownloadTimeoutKey), | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
|
|
@@ -1560,3 +1586,35 @@ func areCommandServerProxyTLSSettingsSet() bool { | |||||||||
| viperInstance.IsSet(CommandServerProxyTLSSkipVerifyKey) || | ||||||||||
| viperInstance.IsSet(CommandServerProxyTLSServerNameKey) | ||||||||||
| } | ||||||||||
|
|
||||||||||
| func resolveExternalDataSource() *ExternalDataSource { | ||||||||||
| proxyURLStruct := ProxyURL{ | ||||||||||
| URL: viperInstance.GetString(ExternalDataSourceProxyUrlKey), | ||||||||||
| } | ||||||||||
| externalDataSource := &ExternalDataSource{ | ||||||||||
| ProxyURL: proxyURLStruct, | ||||||||||
| AllowedDomains: viperInstance.GetStringSlice(ExternalDataSourceAllowDomainsKey), | ||||||||||
| MaxBytes: viperInstance.GetInt64(ExternalDataSourceMaxBytesKey), | ||||||||||
| } | ||||||||||
|
|
||||||||||
| if err := validateAllowedDomains(externalDataSource.AllowedDomains); err != nil { | ||||||||||
| return nil | ||||||||||
| } | ||||||||||
|
|
||||||||||
| return externalDataSource | ||||||||||
| } | ||||||||||
|
|
||||||||||
| func validateAllowedDomains(domains []string) error { | ||||||||||
| if len(domains) == 0 { | ||||||||||
| return nil | ||||||||||
| } | ||||||||||
|
|
||||||||||
| for _, domain := range domains { | ||||||||||
| if strings.ContainsAny(domain, "/\\ ") || domain == "" { | ||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a better way to validate if the string is a valid domain name or not? Maybe there is a regex we could use? |
||||||||||
| slog.Error("domain is not specified in allowed_domains") | ||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no need to log error here if you are returning the error anyways |
||||||||||
| return errors.New("invalid domain found in allowed_domains") | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| return nil | ||||||||||
| } | ||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1425,6 +1425,13 @@ func createConfig() *Config { | |
| config.FeatureCertificates, config.FeatureFileWatcher, config.FeatureMetrics, | ||
| config.FeatureAPIAction, config.FeatureLogsNap, | ||
| }, | ||
| ExternalDataSource: &ExternalDataSource{ | ||
| ProxyURL: ProxyURL{ | ||
| URL: "", | ||
| }, | ||
| AllowedDomains: nil, | ||
| MaxBytes: 0, | ||
| }, | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -1569,3 +1576,73 @@ func TestValidateLabel(t *testing.T) { | |
| }) | ||
| } | ||
| } | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a test for external Data source and add the external data source to TestResolveConfig() which checks all the values that can be set in the agent config |
||
| func TestValidateAllowedDomains(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| domains []string | ||
| wantErr bool | ||
| }{ | ||
| { | ||
| name: "Test 1: Success: Empty slice", | ||
| domains: []string{}, | ||
| wantErr: false, | ||
| }, | ||
| { | ||
| name: "Test 2: Success: Nil slice", | ||
| domains: nil, | ||
| wantErr: false, | ||
| }, | ||
| { | ||
| name: "Test 3: Success: Valid domains", | ||
| domains: []string{"example.com", "api.nginx.com", "sub.domain.io"}, | ||
| wantErr: false, | ||
| }, | ||
| { | ||
| name: "Test 4: Failure: Domain contains space", | ||
| domains: []string{"valid.com", "bad domain.com"}, | ||
| wantErr: true, | ||
| }, | ||
| { | ||
| name: "Test 5: Failure: Empty string domain", | ||
| domains: []string{"valid.com", ""}, | ||
| wantErr: true, | ||
| }, | ||
| { | ||
| name: "Test 6: Failure: Domain contains forward slash /", | ||
| domains: []string{"domain.com/path"}, | ||
| wantErr: true, | ||
| }, | ||
| { | ||
| name: "Test 7: Failure: Domain contains backward slash \\", | ||
| domains: []string{"domain.com\\path"}, | ||
| wantErr: true, | ||
| }, | ||
| { | ||
| name: "Test 8: Failure: Mixed valid and invalid (first is invalid)", | ||
| domains: []string{" only.com", "good.com"}, | ||
| wantErr: true, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| var logBuffer bytes.Buffer | ||
| logHandler := slog.NewTextHandler(&logBuffer, &slog.HandlerOptions{Level: slog.LevelError}) | ||
|
|
||
| originalLogger := slog.Default() | ||
| slog.SetDefault(slog.New(logHandler)) | ||
| defer slog.SetDefault(originalLogger) | ||
|
|
||
| actualErr := validateAllowedDomains(tt.domains) | ||
|
|
||
| if tt.wantErr { | ||
| require.Error(t, actualErr, "Expected an error but got nil.") | ||
| assert.Contains(t, logBuffer.String(), "domain is not specified in allowed_domains", | ||
| "Expected the error log message to be present in the output.") | ||
| } else { | ||
| assert.NoError(t, actualErr, "Did not expect an error but got one: %v", actualErr) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -82,6 +82,8 @@ const ( | |
| DefBackoffMaxInterval = 20 * time.Second | ||
| DefBackoffMaxElapsedTime = 1 * time.Minute | ||
|
|
||
| DefClientFileDownloadTimeout = 60 * time.Second | ||
|
|
||
| // Watcher defaults | ||
| DefInstanceWatcherMonitoringFrequency = 5 * time.Second | ||
| DefInstanceHealthWatcherMonitoringFrequency = 5 * time.Second | ||
|
|
@@ -114,6 +116,9 @@ const ( | |
|
|
||
| // File defaults | ||
| DefLibDir = "/var/lib/nginx-agent" | ||
|
|
||
| DefExternalDataSourceProxyUrl = "" | ||
| DefExternalDataSourceMaxBytes = 100 * 1024 * 1024 | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you add a comment to say what the default size is in MB |
||
| ) | ||
|
|
||
| func DefaultFeatures() []string { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.