diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 628a63a59..036f4c4ed 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -20,7 +20,7 @@ permissions: env: NFPM_VERSION: 'v2.35.3' - GOPROXY: "direct" + GOPROXY: "https://${{ secrets.ARTIFACTORY_USER }}:${{ secrets.ARTIFACTORY_TOKEN }}@azr.artifactory.f5net.com/artifactory/api/go/f5-nginx-go-dev" jobs: proxy-sanity-check: diff --git a/api/grpc/mpi/v1/command.pb.go b/api/grpc/mpi/v1/command.pb.go index b5f0346f4..31e94ea7d 100644 --- a/api/grpc/mpi/v1/command.pb.go +++ b/api/grpc/mpi/v1/command.pb.go @@ -8,7 +8,7 @@ // Code generated by protoc-gen-go. DO NOT EDIT. // versions: -// protoc-gen-go v1.36.9 +// protoc-gen-go v1.36.10 // protoc (unknown) // source: mpi/v1/command.proto diff --git a/api/grpc/mpi/v1/common.pb.go b/api/grpc/mpi/v1/common.pb.go index e6d06cf37..b59f47b5a 100644 --- a/api/grpc/mpi/v1/common.pb.go +++ b/api/grpc/mpi/v1/common.pb.go @@ -5,7 +5,7 @@ // Code generated by protoc-gen-go. DO NOT EDIT. // versions: -// protoc-gen-go v1.36.9 +// protoc-gen-go v1.36.10 // protoc (unknown) // source: mpi/v1/common.proto diff --git a/api/grpc/mpi/v1/files.pb.go b/api/grpc/mpi/v1/files.pb.go index 47d1362b7..0223bae3a 100644 --- a/api/grpc/mpi/v1/files.pb.go +++ b/api/grpc/mpi/v1/files.pb.go @@ -5,7 +5,7 @@ // Code generated by protoc-gen-go. DO NOT EDIT. // versions: -// protoc-gen-go v1.36.9 +// protoc-gen-go v1.36.10 // protoc (unknown) // source: mpi/v1/files.proto diff --git a/internal/datasource/config/nginx_config_parser.go b/internal/datasource/config/nginx_config_parser.go index 5b0593f28..0c011aa64 100644 --- a/internal/datasource/config/nginx_config_parser.go +++ b/internal/datasource/config/nginx_config_parser.go @@ -67,6 +67,15 @@ type ( current *crossplane.Directive, apiType string) []*model.APIDetails ) +type createAPIDetailsParams struct { + locationDirectiveName string + address string + path string + caCertLocation string + isSSL bool + writeEnabled bool +} + func NewNginxConfigParser(agentConfig *config.Config) *NginxConfigParser { return &NginxConfigParser{ agentConfig: agentConfig, @@ -251,6 +260,8 @@ func (ncp *NginxConfigParser) createNginxConfigContext( nginxConfigContext.PlusAPIs = append(nginxConfigContext.PlusAPIs, plusAPIs...) } + nginxConfigContext.PlusAPIs = ncp.sortPlusAPIs(ctx, nginxConfigContext.PlusAPIs) + if len(napSyslogServersFound) > 0 { var napSyslogServer []string for server := range napSyslogServersFound { @@ -377,7 +388,6 @@ func (ncp *NginxConfigParser) crossplaneConfigTraverseAPIDetails( callback crossplaneTraverseCallbackAPIDetails, apiType string, ) []*model.APIDetails { - stop := false var responses []*model.APIDetails for _, dir := range root.Parsed { @@ -386,7 +396,7 @@ func (ncp *NginxConfigParser) crossplaneConfigTraverseAPIDetails( responses = append(responses, response...) continue } - response = traverseAPIDetails(ctx, dir, callback, &stop, apiType) + response = traverseAPIDetails(ctx, dir, callback, apiType) if response != nil { responses = append(responses, response...) } @@ -399,26 +409,23 @@ func traverseAPIDetails( ctx context.Context, root *crossplane.Directive, callback crossplaneTraverseCallbackAPIDetails, - stop *bool, apiType string, ) (response []*model.APIDetails) { - if *stop { - return nil - } + var collectedResponses []*model.APIDetails for _, child := range root.Block { - response = callback(ctx, root, child, apiType) - if len(response) > 0 { - *stop = true - return response + currentResponse := callback(ctx, root, child, apiType) + if len(currentResponse) > 0 { + collectedResponses = append(collectedResponses, currentResponse...) } - response = traverseAPIDetails(ctx, child, callback, stop, apiType) - if *stop { - return response + + recursiveResponse := traverseAPIDetails(ctx, child, callback, apiType) + if len(recursiveResponse) > 0 { + collectedResponses = append(collectedResponses, recursiveResponse...) } } - return response + return collectedResponses } func (ncp *NginxConfigParser) formatMap(directive *crossplane.Directive) map[string]string { @@ -699,11 +706,22 @@ func (ncp *NginxConfigParser) apiDetailsFromLocationDirective( addresses := ncp.parseAddressFromServerDirective(parent) path := ncp.parsePathFromLocationDirective(current) + writeEnabled := ncp.isWriteEnabled(locChild) + if locChild.Directive == locationDirectiveName { for _, address := range addresses { + params := createAPIDetailsParams{ + locationDirectiveName: locationDirectiveName, + address: address, + path: path, + caCertLocation: caCertLocation, + isSSL: isSSL, + writeEnabled: writeEnabled, + } + details = append( details, - ncp.createAPIDetails(locationDirectiveName, address, path, caCertLocation, isSSL), + ncp.createAPIDetails(params), ) } } @@ -713,28 +731,30 @@ func (ncp *NginxConfigParser) apiDetailsFromLocationDirective( } func (ncp *NginxConfigParser) createAPIDetails( - locationDirectiveName, address, path, caCertLocation string, isSSL bool, + params createAPIDetailsParams, ) (details *model.APIDetails) { - if strings.HasPrefix(address, "unix:") { + if strings.HasPrefix(params.address, "unix:") { format := unixStubStatusFormat - if locationDirectiveName == plusAPIDirective { + if params.locationDirectiveName == plusAPIDirective { format = unixPlusAPIFormat } details = &model.APIDetails{ - URL: fmt.Sprintf(format, path), - Listen: address, - Location: path, - Ca: caCertLocation, + URL: fmt.Sprintf(format, params.path), + Listen: params.address, + Location: params.path, + Ca: params.caCertLocation, + WriteEnabled: params.writeEnabled, } } else { details = &model.APIDetails{ - URL: fmt.Sprintf("%s://%s%s", map[bool]string{true: "https", false: "http"}[isSSL], - address, path), - Listen: address, - Location: path, - Ca: caCertLocation, + URL: fmt.Sprintf("%s://%s%s", map[bool]string{true: "https", false: "http"}[params.isSSL], + params.address, params.path), + Listen: params.address, + Location: params.path, + Ca: params.caCertLocation, + WriteEnabled: params.writeEnabled, } } @@ -888,3 +908,47 @@ func (ncp *NginxConfigParser) isDuplicateFile(nginxConfigContextFiles []*mpi.Fil return false } + +func (ncp *NginxConfigParser) isWriteEnabled(locChild *crossplane.Directive) bool { + if locChild.Directive != plusAPIDirective { + return false + } + + for _, arg := range locChild.Args { + if strings.EqualFold(arg, "write=on") { + return true + } + } + + return false +} + +// sort the API endpoints by prioritizing any API that has 'write=on'. +func (ncp *NginxConfigParser) sortPlusAPIs(ctx context.Context, apis []*model.APIDetails) []*model.APIDetails { + foundWriteEnabled := false + for _, api := range apis { + if api.WriteEnabled { + foundWriteEnabled = true + break + } + } + + if !foundWriteEnabled && len(apis) > 0 { + slog.InfoContext(ctx, "No write-enabled NGINX Plus API found. Defaulting to read-only API") + return apis + } + + slices.SortFunc(apis, func(a, b *model.APIDetails) int { + if a.WriteEnabled && !b.WriteEnabled { + return -1 + } + + if b.WriteEnabled && !a.WriteEnabled { + return 1 + } + + return 0 + }) + + return apis +} diff --git a/internal/datasource/config/nginx_config_parser_test.go b/internal/datasource/config/nginx_config_parser_test.go index 8051a5200..289def7e2 100644 --- a/internal/datasource/config/nginx_config_parser_test.go +++ b/internal/datasource/config/nginx_config_parser_test.go @@ -150,7 +150,7 @@ var ( listen [::]:8000; server_name _; location /api/ { - api write=on; + api write=off; allow 127.0.0.1; deny all; } @@ -275,7 +275,7 @@ var ( } location /api { - api write=on; + api write=off; } } @@ -286,6 +286,62 @@ server { return 418; } ` + testConf23 = `server { + listen 127.0.0.1:9090; + location /writeapi { + api write=on; + allow 127.0.0.1; + deny all; + } +}` + testConf24 = `server { + listen 127.0.0.1:9090; + location /writeapi { + api write=off; + allow 127.0.0.1; + deny all; + } +} +server { + listen 127.0.0.1:9091; + location /writeapi { + api write=off; + allow 127.0.0.1; + deny all; + } +}` + testConf25 = `server { + listen 127.0.0.1:9090; + location /writeapi { + api write=off; + allow 127.0.0.1; + deny all; + } +} + server { + listen 127.0.0.1:9091; + location /writeapi { + api write=off; + allow 127.0.0.1; + deny all; + } +} + server { + listen 127.0.0.1:9092; + location /writeapi { + api write=on; + allow 127.0.0.1; + deny all; + } +} + server { + listen 127.0.0.1:9093; + location /writeapi { + api write=off; + allow 127.0.0.1; + deny all; + } +}` ) //nolint:maintidx // The test cannot be refactored @@ -805,118 +861,132 @@ func TestNginxConfigParser_checkLog(t *testing.T) { } } +//nolint:maintidx // Does not make sense to add a new test case to do the exact same checks. func TestNginxConfigParser_urlsForLocationDirective(t *testing.T) { tmpDir := t.TempDir() tests := []struct { - oss *model.APIDetails - plus *model.APIDetails - name string - conf string + oss *model.APIDetails + plus *model.APIDetails + name string + conf string + expectedLog string }{ { plus: &model.APIDetails{ - URL: "http://localhost:80/api/", - Listen: "localhost:80", - Location: "/api/", + URL: "http://localhost:80/api/", + Listen: "localhost:80", + Location: "/api/", + WriteEnabled: true, }, name: "Test 1: listen localhost 80, allow 127.0.0.1 - Plus", conf: testConf01, }, { plus: &model.APIDetails{ - URL: "http://localhost:80/api/", - Listen: "localhost:80", - Location: "/api/", + URL: "http://localhost:80/api/", + Listen: "localhost:80", + Location: "/api/", + WriteEnabled: true, }, name: "Test 2: listen *:80 - Plus", conf: testConf02, }, { plus: &model.APIDetails{ - URL: "http://localhost:80/api/", - Listen: "localhost:80", - Location: "/api/", + URL: "http://localhost:80/api/", + Listen: "localhost:80", + Location: "/api/", + WriteEnabled: true, }, name: "Test 3: server_name _ - Plus", conf: testConf03, }, { plus: &model.APIDetails{ - URL: "http://localhost:8888/api/", - Listen: "localhost:8888", - Location: "/api/", + URL: "http://localhost:8888/api/", + Listen: "localhost:8888", + Location: "/api/", + WriteEnabled: true, }, name: "Test 4: server_name status.internal.com - Plus", conf: testConf04, }, { plus: &model.APIDetails{ - URL: "http://localhost:8080/privateapi", - Listen: "localhost:8080", - Location: "/privateapi", + URL: "http://localhost:8080/privateapi", + Listen: "localhost:8080", + Location: "/privateapi", + WriteEnabled: true, }, name: "Test 5: location /privateapi - Plus", conf: testConf05, }, { plus: &model.APIDetails{ - URL: "http://localhost:80/api/", - Listen: "localhost:80", - Location: "/api/", + URL: "http://localhost:80/api/", + Listen: "localhost:80", + Location: "/api/", + WriteEnabled: true, }, name: "Test 6: listen [::]:80 default_server - Plus", conf: testConf06, }, { plus: &model.APIDetails{ - URL: "http://localhost:80/api/", - Listen: "localhost:80", - Location: "/api/", + URL: "http://localhost:80/api/", + Listen: "localhost:80", + Location: "/api/", + WriteEnabled: true, }, name: "Test 7: listen 127.0.0.1, server_name _ - Plus", conf: testConf07, }, { plus: &model.APIDetails{ - URL: "http://localhost:80/api/", - Listen: "localhost:80", - Location: "/api/", + URL: "http://localhost:80/api/", + Listen: "localhost:80", + Location: "/api/", + WriteEnabled: true, }, name: "Test 8: location = /api/, listen 127.0.0.1 - Plus", conf: testConf08, }, { plus: &model.APIDetails{ - URL: "http://localhost:80/api/", - Listen: "localhost:80", - Location: "/api/", + URL: "http://localhost:80/api/", + Listen: "localhost:80", + Location: "/api/", + WriteEnabled: true, }, name: "Test 9: location = /api/ , listen 80 - Plus", conf: testConf09, }, { plus: &model.APIDetails{ - URL: "http://localhost:80/api/", - Listen: "localhost:80", - Location: "/api/", + URL: "http://localhost:80/api/", + Listen: "localhost:80", + Location: "/api/", + WriteEnabled: true, }, name: "Test 10: listen :80 - Plus", conf: testConf10, }, { plus: &model.APIDetails{ - URL: "http://localhost:80/api/", - Listen: "localhost:80", - Location: "/api/", + URL: "http://localhost:80/api/", + Listen: "localhost:80", + Location: "/api/", + WriteEnabled: true, }, name: "Test 11: listen localhost - Plus", conf: testConf11, }, { plus: &model.APIDetails{ - URL: "http://localhost:80/api/", - Listen: "localhost:80", - Location: "/api/", + URL: "http://localhost:80/api/", + Listen: "localhost:80", + Location: "/api/", + WriteEnabled: true, }, name: "Test 12: listen [::1] - Plus", conf: testConf12, @@ -973,9 +1043,10 @@ func TestNginxConfigParser_urlsForLocationDirective(t *testing.T) { Location: "/stub_status", }, plus: &model.APIDetails{ - URL: "http://localhost:80/api/", - Listen: "localhost:80", - Location: "/api/", + URL: "http://localhost:80/api/", + Listen: "localhost:80", + Location: "/api/", + WriteEnabled: true, }, name: "Test 18: listen 80 - OSS & Plus", conf: testConf18, @@ -1000,9 +1071,10 @@ func TestNginxConfigParser_urlsForLocationDirective(t *testing.T) { }, { plus: &model.APIDetails{ - URL: "http://nginx-plus-api/api", - Listen: "unix:/var/run/nginx/nginx-plus-api.sock", - Location: "/api", + URL: "http://nginx-plus-api/api", + Listen: "unix:/var/run/nginx/nginx-plus-api.sock", + Location: "/api", + WriteEnabled: true, }, name: "Test 21: listen unix:/var/run/nginx/nginx-plus-api.sock - Plus Unix Socket", conf: testConf21, @@ -1016,11 +1088,44 @@ func TestNginxConfigParser_urlsForLocationDirective(t *testing.T) { name: "Test 22: Multiple Plus Unix Sockets", conf: testConf22, }, + { + plus: &model.APIDetails{ + URL: "http://localhost:9090/writeapi", + Listen: "localhost:9090", + Location: "/writeapi", + WriteEnabled: true, + }, + name: "Test 23: Explicitly Write-Enabled Plus API", + conf: testConf23, + }, + { + plus: &model.APIDetails{ + URL: "http://localhost:9090/writeapi", + Listen: "localhost:9090", + Location: "/writeapi", + WriteEnabled: false, + }, + name: "Test 24: Multiple Plus APIs, all with Write=off, keep the order", + expectedLog: "No write-enabled NGINX Plus API found. Defaulting to read-only API", + conf: testConf24, + }, + { + plus: &model.APIDetails{ + URL: "http://localhost:9092/writeapi", + Listen: "localhost:9092", + Location: "/writeapi", + WriteEnabled: true, + }, + name: "Test 25: Multiple Plus APIs, Prioritize Write-Enabled (9092)", + conf: testConf25, + }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { ctx := context.Background() + logBuf := &bytes.Buffer{} + stub.StubLoggerWith(logBuf) f, err := os.CreateTemp(tmpDir, "conf") require.NoError(t, err) parseOptions := &crossplane.ParseOptions{ @@ -1042,6 +1147,9 @@ func TestNginxConfigParser_urlsForLocationDirective(t *testing.T) { if test.plus != nil { assert.Equal(t, test.plus, plus[0]) } + helpers.ValidateLog(t, test.expectedLog, logBuf) + + logBuf.Reset() }) } } @@ -1053,6 +1161,8 @@ func traverseConfigForAPIs( ncp := NewNginxConfigParser(types.AgentConfig()) + allPlusAPIs := []*model.APIDetails{} + assert.Len(t, payload.Config, 1) for _, xpConf := range payload.Config { assert.Len(t, xpConf.Parsed, 1) @@ -1064,7 +1174,7 @@ func traverseConfigForAPIs( } _plus := ncp.apiDetailsFromLocationDirective(ctx, parent, directive, plusAPIDirective) if _plus != nil { - plus = _plus + allPlusAPIs = append(allPlusAPIs, _plus...) } return nil @@ -1072,6 +1182,11 @@ func traverseConfigForAPIs( require.NoError(t, err) } + if len(allPlusAPIs) > 0 { + sortedAPIs := ncp.sortPlusAPIs(ctx, allPlusAPIs) + plus = []*model.APIDetails{sortedAPIs[0]} + } + return oss, plus } diff --git a/internal/model/config.go b/internal/model/config.go index 1aaf95c97..5455f61d1 100644 --- a/internal/model/config.go +++ b/internal/model/config.go @@ -26,10 +26,11 @@ type NginxConfigContext struct { } type APIDetails struct { - URL string - Listen string - Location string - Ca string + URL string + Listen string + Location string + Ca string + WriteEnabled bool } type ManifestFile struct { @@ -95,7 +96,8 @@ func (ncc *NginxConfigContext) Equal(otherNginxConfigContext *NginxConfigContext if ncc.PlusAPI != nil && otherNginxConfigContext.PlusAPI != nil { if ncc.PlusAPI.URL != otherNginxConfigContext.PlusAPI.URL || ncc.PlusAPI.Listen != otherNginxConfigContext.PlusAPI.Listen || ncc.PlusAPI.Location != - otherNginxConfigContext.PlusAPI.Location { + otherNginxConfigContext.PlusAPI.Location || + ncc.PlusAPI.WriteEnabled != otherNginxConfigContext.PlusAPI.WriteEnabled { return false } }