From d11d7a5b94b789531213c272b5a442fa360753c8 Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Tue, 30 Sep 2025 15:41:01 +0100 Subject: [PATCH 01/15] selecting API with write access --- api/grpc/mpi/v1/command.pb.go | 79 ++++++++++++++-------- api/grpc/mpi/v1/command.pb.validate.go | 36 ++++++++++ api/grpc/mpi/v1/command.proto | 4 ++ docs/proto/protos.md | 2 + internal/model/config.go | 9 +-- internal/resource/resource_service.go | 37 +++++++--- internal/resource/resource_service_test.go | 35 ++++++---- 7 files changed, 146 insertions(+), 56 deletions(-) diff --git a/api/grpc/mpi/v1/command.pb.go b/api/grpc/mpi/v1/command.pb.go index b5f0346f4..63448d66c 100644 --- a/api/grpc/mpi/v1/command.pb.go +++ b/api/grpc/mpi/v1/command.pb.go @@ -2180,7 +2180,9 @@ type NGINXPlusRuntimeInfo struct { // List of NGINX dynamic modules. DynamicModules []string `protobuf:"bytes,5,rep,name=dynamic_modules,json=dynamicModules,proto3" json:"dynamic_modules,omitempty"` // the plus API details - PlusApi *APIDetails `protobuf:"bytes,6,opt,name=plus_api,json=plusApi,proto3" json:"plus_api,omitempty"` + PlusApi *APIDetails `protobuf:"bytes,6,opt,name=plus_api,json=plusApi,proto3" json:"plus_api,omitempty"` + // to store all the endpoints details + PlusApis []*APIDetails `protobuf:"bytes,7,rep,name=plus_apis,json=plusApis,proto3" json:"plus_apis,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache } @@ -2257,6 +2259,13 @@ func (x *NGINXPlusRuntimeInfo) GetPlusApi() *APIDetails { return nil } +func (x *NGINXPlusRuntimeInfo) GetPlusApis() []*APIDetails { + if x != nil { + return x.PlusApis + } + return nil +} + type APIDetails struct { state protoimpl.MessageState `protogen:"open.v1"` // the API location directive @@ -2264,7 +2273,9 @@ type APIDetails struct { // the API listen directive Listen string `protobuf:"bytes,2,opt,name=listen,proto3" json:"listen,omitempty"` // the API CA file path - Ca string `protobuf:"bytes,3,opt,name=Ca,proto3" json:"Ca,omitempty"` + Ca string `protobuf:"bytes,3,opt,name=Ca,proto3" json:"Ca,omitempty"` + // flag to know if this API location was configured with 'api write=on;' + WriteEnabled bool `protobuf:"varint,4,opt,name=write_enabled,json=writeEnabled,proto3" json:"write_enabled,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache } @@ -2320,6 +2331,13 @@ func (x *APIDetails) GetCa() string { return "" } +func (x *APIDetails) GetWriteEnabled() bool { + if x != nil { + return x.WriteEnabled + } + return false +} + // A set of runtime NGINX App Protect settings type NGINXAppProtectRuntimeInfo struct { state protoimpl.MessageState `protogen:"open.v1"` @@ -2871,7 +2889,7 @@ const file_mpi_v1_command_proto_rawDesc = "" + "\n" + "error_logs\x18\x03 \x03(\tR\terrorLogs\x12)\n" + "\x10loadable_modules\x18\x04 \x03(\tR\x0floadableModules\x12'\n" + - "\x0fdynamic_modules\x18\x05 \x03(\tR\x0edynamicModules\"\x8e\x02\n" + + "\x0fdynamic_modules\x18\x05 \x03(\tR\x0edynamicModules\"\xbf\x02\n" + "\x14NGINXPlusRuntimeInfo\x123\n" + "\vstub_status\x18\x01 \x01(\v2\x12.mpi.v1.APIDetailsR\n" + "stubStatus\x12\x1f\n" + @@ -2881,12 +2899,14 @@ const file_mpi_v1_command_proto_rawDesc = "" + "error_logs\x18\x03 \x03(\tR\terrorLogs\x12)\n" + "\x10loadable_modules\x18\x04 \x03(\tR\x0floadableModules\x12'\n" + "\x0fdynamic_modules\x18\x05 \x03(\tR\x0edynamicModules\x12-\n" + - "\bplus_api\x18\x06 \x01(\v2\x12.mpi.v1.APIDetailsR\aplusApi\"P\n" + + "\bplus_api\x18\x06 \x01(\v2\x12.mpi.v1.APIDetailsR\aplusApi\x12/\n" + + "\tplus_apis\x18\a \x03(\v2\x12.mpi.v1.APIDetailsR\bplusApis\"u\n" + "\n" + "APIDetails\x12\x1a\n" + "\blocation\x18\x01 \x01(\tR\blocation\x12\x16\n" + "\x06listen\x18\x02 \x01(\tR\x06listen\x12\x0e\n" + - "\x02Ca\x18\x03 \x01(\tR\x02Ca\"\xe0\x01\n" + + "\x02Ca\x18\x03 \x01(\tR\x02Ca\x12#\n" + + "\rwrite_enabled\x18\x04 \x01(\bR\fwriteEnabled\"\xe0\x01\n" + "\x1aNGINXAppProtectRuntimeInfo\x12\x18\n" + "\arelease\x18\x01 \x01(\tR\arelease\x128\n" + "\x18attack_signature_version\x18\x02 \x01(\tR\x16attackSignatureVersion\x126\n" + @@ -3030,30 +3050,31 @@ var file_mpi_v1_command_proto_depIdxs = []int32{ 34, // 43: mpi.v1.NGINXRuntimeInfo.stub_status:type_name -> mpi.v1.APIDetails 34, // 44: mpi.v1.NGINXPlusRuntimeInfo.stub_status:type_name -> mpi.v1.APIDetails 34, // 45: mpi.v1.NGINXPlusRuntimeInfo.plus_api:type_name -> mpi.v1.APIDetails - 38, // 46: mpi.v1.AgentConfig.command:type_name -> mpi.v1.CommandServer - 40, // 47: mpi.v1.AgentConfig.metrics:type_name -> mpi.v1.MetricsServer - 41, // 48: mpi.v1.AgentConfig.file:type_name -> mpi.v1.FileServer - 45, // 49: mpi.v1.AgentConfig.labels:type_name -> google.protobuf.Struct - 39, // 50: mpi.v1.AgentConfig.auxiliary_command:type_name -> mpi.v1.AuxiliaryCommandServer - 46, // 51: mpi.v1.CommandServer.server:type_name -> mpi.v1.ServerSettings - 47, // 52: mpi.v1.CommandServer.auth:type_name -> mpi.v1.AuthSettings - 48, // 53: mpi.v1.CommandServer.tls:type_name -> mpi.v1.TLSSettings - 46, // 54: mpi.v1.AuxiliaryCommandServer.server:type_name -> mpi.v1.ServerSettings - 47, // 55: mpi.v1.AuxiliaryCommandServer.auth:type_name -> mpi.v1.AuthSettings - 48, // 56: mpi.v1.AuxiliaryCommandServer.tls:type_name -> mpi.v1.TLSSettings - 2, // 57: mpi.v1.CommandService.CreateConnection:input_type -> mpi.v1.CreateConnectionRequest - 8, // 58: mpi.v1.CommandService.UpdateDataPlaneStatus:input_type -> mpi.v1.UpdateDataPlaneStatusRequest - 11, // 59: mpi.v1.CommandService.UpdateDataPlaneHealth:input_type -> mpi.v1.UpdateDataPlaneHealthRequest - 13, // 60: mpi.v1.CommandService.Subscribe:input_type -> mpi.v1.DataPlaneResponse - 7, // 61: mpi.v1.CommandService.CreateConnection:output_type -> mpi.v1.CreateConnectionResponse - 9, // 62: mpi.v1.CommandService.UpdateDataPlaneStatus:output_type -> mpi.v1.UpdateDataPlaneStatusResponse - 12, // 63: mpi.v1.CommandService.UpdateDataPlaneHealth:output_type -> mpi.v1.UpdateDataPlaneHealthResponse - 14, // 64: mpi.v1.CommandService.Subscribe:output_type -> mpi.v1.ManagementPlaneRequest - 61, // [61:65] is the sub-list for method output_type - 57, // [57:61] is the sub-list for method input_type - 57, // [57:57] is the sub-list for extension type_name - 57, // [57:57] is the sub-list for extension extendee - 0, // [0:57] is the sub-list for field type_name + 34, // 46: mpi.v1.NGINXPlusRuntimeInfo.plus_apis:type_name -> mpi.v1.APIDetails + 38, // 47: mpi.v1.AgentConfig.command:type_name -> mpi.v1.CommandServer + 40, // 48: mpi.v1.AgentConfig.metrics:type_name -> mpi.v1.MetricsServer + 41, // 49: mpi.v1.AgentConfig.file:type_name -> mpi.v1.FileServer + 45, // 50: mpi.v1.AgentConfig.labels:type_name -> google.protobuf.Struct + 39, // 51: mpi.v1.AgentConfig.auxiliary_command:type_name -> mpi.v1.AuxiliaryCommandServer + 46, // 52: mpi.v1.CommandServer.server:type_name -> mpi.v1.ServerSettings + 47, // 53: mpi.v1.CommandServer.auth:type_name -> mpi.v1.AuthSettings + 48, // 54: mpi.v1.CommandServer.tls:type_name -> mpi.v1.TLSSettings + 46, // 55: mpi.v1.AuxiliaryCommandServer.server:type_name -> mpi.v1.ServerSettings + 47, // 56: mpi.v1.AuxiliaryCommandServer.auth:type_name -> mpi.v1.AuthSettings + 48, // 57: mpi.v1.AuxiliaryCommandServer.tls:type_name -> mpi.v1.TLSSettings + 2, // 58: mpi.v1.CommandService.CreateConnection:input_type -> mpi.v1.CreateConnectionRequest + 8, // 59: mpi.v1.CommandService.UpdateDataPlaneStatus:input_type -> mpi.v1.UpdateDataPlaneStatusRequest + 11, // 60: mpi.v1.CommandService.UpdateDataPlaneHealth:input_type -> mpi.v1.UpdateDataPlaneHealthRequest + 13, // 61: mpi.v1.CommandService.Subscribe:input_type -> mpi.v1.DataPlaneResponse + 7, // 62: mpi.v1.CommandService.CreateConnection:output_type -> mpi.v1.CreateConnectionResponse + 9, // 63: mpi.v1.CommandService.UpdateDataPlaneStatus:output_type -> mpi.v1.UpdateDataPlaneStatusResponse + 12, // 64: mpi.v1.CommandService.UpdateDataPlaneHealth:output_type -> mpi.v1.UpdateDataPlaneHealthResponse + 14, // 65: mpi.v1.CommandService.Subscribe:output_type -> mpi.v1.ManagementPlaneRequest + 62, // [62:66] is the sub-list for method output_type + 58, // [58:62] is the sub-list for method input_type + 58, // [58:58] is the sub-list for extension type_name + 58, // [58:58] is the sub-list for extension extendee + 0, // [0:58] is the sub-list for field type_name } func init() { file_mpi_v1_command_proto_init() } diff --git a/api/grpc/mpi/v1/command.pb.validate.go b/api/grpc/mpi/v1/command.pb.validate.go index 81f716548..e3d480eb1 100644 --- a/api/grpc/mpi/v1/command.pb.validate.go +++ b/api/grpc/mpi/v1/command.pb.validate.go @@ -4787,6 +4787,40 @@ func (m *NGINXPlusRuntimeInfo) validate(all bool) error { } } + for idx, item := range m.GetPlusApis() { + _, _ = idx, item + + if all { + switch v := interface{}(item).(type) { + case interface{ ValidateAll() error }: + if err := v.ValidateAll(); err != nil { + errors = append(errors, NGINXPlusRuntimeInfoValidationError{ + field: fmt.Sprintf("PlusApis[%v]", idx), + reason: "embedded message failed validation", + cause: err, + }) + } + case interface{ Validate() error }: + if err := v.Validate(); err != nil { + errors = append(errors, NGINXPlusRuntimeInfoValidationError{ + field: fmt.Sprintf("PlusApis[%v]", idx), + reason: "embedded message failed validation", + cause: err, + }) + } + } + } else if v, ok := interface{}(item).(interface{ Validate() error }); ok { + if err := v.Validate(); err != nil { + return NGINXPlusRuntimeInfoValidationError{ + field: fmt.Sprintf("PlusApis[%v]", idx), + reason: "embedded message failed validation", + cause: err, + } + } + } + + } + if len(errors) > 0 { return NGINXPlusRuntimeInfoMultiError(errors) } @@ -4895,6 +4929,8 @@ func (m *APIDetails) validate(all bool) error { // no validation rules for Ca + // no validation rules for WriteEnabled + if len(errors) > 0 { return APIDetailsMultiError(errors) } diff --git a/api/grpc/mpi/v1/command.proto b/api/grpc/mpi/v1/command.proto index 84fb6a020..532d170c4 100644 --- a/api/grpc/mpi/v1/command.proto +++ b/api/grpc/mpi/v1/command.proto @@ -345,6 +345,8 @@ message NGINXPlusRuntimeInfo { repeated string dynamic_modules = 5; // the plus API details APIDetails plus_api = 6; + // to store all the endpoints details + repeated APIDetails plus_apis = 7; } message APIDetails { @@ -354,6 +356,8 @@ message APIDetails { string listen = 2; // the API CA file path string Ca = 3; + // flag to know if this API location was configured with 'api write=on;' + bool write_enabled = 4; } // A set of runtime NGINX App Protect settings diff --git a/docs/proto/protos.md b/docs/proto/protos.md index 3ae57f5e4..c19baa46b 100644 --- a/docs/proto/protos.md +++ b/docs/proto/protos.md @@ -697,6 +697,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 file path | +| write_enabled | [bool](#bool) | | flag to know if this API location was configured with 'api write=on;' | @@ -1131,6 +1132,7 @@ A set of runtime NGINX Plus settings | loadable_modules | [string](#string) | repeated | List of NGINX potentially loadable modules (installed but not loaded). | | dynamic_modules | [string](#string) | repeated | List of NGINX dynamic modules. | | plus_api | [APIDetails](#mpi-v1-APIDetails) | | the plus API details | +| plus_apis | [APIDetails](#mpi-v1-APIDetails) | repeated | to store all the endpoints details | diff --git a/internal/model/config.go b/internal/model/config.go index 1aaf95c97..e983d8708 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 { diff --git a/internal/resource/resource_service.go b/internal/resource/resource_service.go index 1357bda17..5140aabb6 100644 --- a/internal/resource/resource_service.go +++ b/internal/resource/resource_service.go @@ -361,21 +361,42 @@ func convertToStreamUpstreamServer(streamUpstreams []*structpb.Struct) []client. } func (r *ResourceService) createPlusClient(ctx context.Context, instance *mpi.Instance) (*client.NginxClient, error) { - plusAPI := instance.GetInstanceRuntime().GetNginxPlusRuntimeInfo().GetPlusApi() + plusAPIs := instance.GetInstanceRuntime().GetNginxPlusRuntimeInfo().GetPlusApis() var endpoint string + var selectedAPI *mpi.APIDetails - if plusAPI.GetLocation() == "" || plusAPI.GetListen() == "" { + if len(plusAPIs) == 0 { return nil, errors.New("failed to preform API action, NGINX Plus API is not configured") } - if strings.HasPrefix(plusAPI.GetListen(), "unix:") { - endpoint = fmt.Sprintf(unixPlusAPIFormat, plusAPI.GetLocation()) + for _, api := range plusAPIs { + if api.GetWriteEnabled() { + selectedAPI = api + slog.DebugContext(ctx, "Selected write-enabled NGINX Plus API for action", + "url", selectedAPI.GetLocation(), "listen", selectedAPI.GetListen()) + + break + } + } + + if selectedAPI == nil { + selectedAPI = plusAPIs[0] + slog.InfoContext(ctx, "No write-enabled NGINX Plus API found. Write operations may fail.", + "url", selectedAPI.GetLocation(), "listen", selectedAPI.GetListen()) + } + + if selectedAPI.GetLocation() == "" || selectedAPI.GetListen() == "" { + return nil, errors.New("failed to preform API action, NGINX Plus API is not configured") + } + + if strings.HasPrefix(selectedAPI.GetListen(), "unix:") { + endpoint = fmt.Sprintf(unixPlusAPIFormat, selectedAPI.GetLocation()) } else { - endpoint = fmt.Sprintf(apiFormat, plusAPI.GetListen(), plusAPI.GetLocation()) + endpoint = fmt.Sprintf(apiFormat, selectedAPI.GetListen(), selectedAPI.GetLocation()) } httpClient := http.DefaultClient - caCertLocation := plusAPI.GetCa() + caCertLocation := selectedAPI.GetCa() if caCertLocation != "" { slog.DebugContext(ctx, "Reading CA certificate", "file_path", caCertLocation) caCert, err := os.ReadFile(caCertLocation) @@ -394,8 +415,8 @@ func (r *ResourceService) createPlusClient(ctx context.Context, instance *mpi.In }, } } - if strings.HasPrefix(plusAPI.GetListen(), "unix:") { - httpClient = socketClient(ctx, strings.TrimPrefix(plusAPI.GetListen(), "unix:")) + if strings.HasPrefix(selectedAPI.GetListen(), "unix:") { + httpClient = socketClient(ctx, strings.TrimPrefix(selectedAPI.GetListen(), "unix:")) } return client.NewNginxClient(endpoint, diff --git a/internal/resource/resource_service_test.go b/internal/resource/resource_service_test.go index 8a8cd45a9..5a76bb6aa 100644 --- a/internal/resource/resource_service_test.go +++ b/internal/resource/resource_service_test.go @@ -246,24 +246,29 @@ func TestResourceService_createPlusClient(t *testing.T) { err := os.WriteFile(caFile, []byte("-----BEGIN CERTIFICATE-----\nMII...\n-----END CERTIFICATE-----"), 0o600) require.NoError(t, err) - instanceWithAPI := protos.NginxPlusInstance([]string{}) - instanceWithAPI.InstanceRuntime.GetNginxPlusRuntimeInfo().PlusApi = &v1.APIDetails{ - Location: "/api", - Listen: "localhost:80", - } + createPlusInstanceWithApis := func(details []*v1.APIDetails) *v1.Instance { + inst := protos.NginxPlusInstance([]string{}) + if inst.GetInstanceRuntime().GetNginxPlusRuntimeInfo() == nil { + inst.InstanceRuntime.Details = &v1.InstanceRuntime_NginxPlusRuntimeInfo{ + NginxPlusRuntimeInfo: &v1.NGINXPlusRuntimeInfo{}, + } + } + inst.InstanceRuntime.GetNginxPlusRuntimeInfo().PlusApis = details - instanceWithUnixAPI := protos.NginxPlusInstance([]string{}) - instanceWithUnixAPI.InstanceRuntime.GetNginxPlusRuntimeInfo().PlusApi = &v1.APIDetails{ - Listen: "unix:/var/run/nginx-status.sock", - Location: "/api", + return inst } - instanceWithCACert := protos.NginxPlusInstance([]string{}) - instanceWithCACert.InstanceRuntime.GetNginxPlusRuntimeInfo().PlusApi = &v1.APIDetails{ - Location: "/api", - Listen: "localhost:443", - Ca: caFile, - } + instanceWithAPI := createPlusInstanceWithApis([]*v1.APIDetails{ + {Location: "/api", Listen: "localhost:80"}, + }) + + instanceWithUnixAPI := createPlusInstanceWithApis([]*v1.APIDetails{ + {Listen: "unix:/var/run/nginx-status.sock", Location: "/api"}, + }) + + instanceWithCACert := createPlusInstanceWithApis([]*v1.APIDetails{ + {Location: "/api", Listen: "localhost:443", Ca: caFile}, + }) ctx := context.Background() tests := []struct { From 99d0db970670439fb5ac85d117301301901e4747 Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Wed, 1 Oct 2025 16:23:52 +0100 Subject: [PATCH 02/15] parsing the plusAPIs --- .../config/configfakes/fake_config_parser.go | 76 +++++++++++ .../datasource/config/nginx_config_parser.go | 42 ++++-- .../config/nginx_config_parser_test.go | 120 +++++++++++------- internal/resource/resource_service_test.go | 42 +++++- 4 files changed, 225 insertions(+), 55 deletions(-) diff --git a/internal/datasource/config/configfakes/fake_config_parser.go b/internal/datasource/config/configfakes/fake_config_parser.go index c4c7ad6ec..865833d38 100644 --- a/internal/datasource/config/configfakes/fake_config_parser.go +++ b/internal/datasource/config/configfakes/fake_config_parser.go @@ -11,6 +11,18 @@ import ( ) type FakeConfigParser struct { + FindAllPlusAPIsStub func(context.Context, *model.NginxConfigContext) []*model.APIDetails + findAllPlusAPIsMutex sync.RWMutex + findAllPlusAPIsArgsForCall []struct { + arg1 context.Context + arg2 *model.NginxConfigContext + } + findAllPlusAPIsReturns struct { + result1 []*model.APIDetails + } + findAllPlusAPIsReturnsOnCall map[int]struct { + result1 []*model.APIDetails + } FindPlusAPIStub func(context.Context, *model.NginxConfigContext) *model.APIDetails findPlusAPIMutex sync.RWMutex findPlusAPIArgsForCall []struct { @@ -53,6 +65,68 @@ type FakeConfigParser struct { invocationsMutex sync.RWMutex } +func (fake *FakeConfigParser) FindAllPlusAPIs(arg1 context.Context, arg2 *model.NginxConfigContext) []*model.APIDetails { + fake.findAllPlusAPIsMutex.Lock() + ret, specificReturn := fake.findAllPlusAPIsReturnsOnCall[len(fake.findAllPlusAPIsArgsForCall)] + fake.findAllPlusAPIsArgsForCall = append(fake.findAllPlusAPIsArgsForCall, struct { + arg1 context.Context + arg2 *model.NginxConfigContext + }{arg1, arg2}) + stub := fake.FindAllPlusAPIsStub + fakeReturns := fake.findAllPlusAPIsReturns + fake.recordInvocation("FindAllPlusAPIs", []interface{}{arg1, arg2}) + fake.findAllPlusAPIsMutex.Unlock() + if stub != nil { + return stub(arg1, arg2) + } + if specificReturn { + return ret.result1 + } + return fakeReturns.result1 +} + +func (fake *FakeConfigParser) FindAllPlusAPIsCallCount() int { + fake.findAllPlusAPIsMutex.RLock() + defer fake.findAllPlusAPIsMutex.RUnlock() + return len(fake.findAllPlusAPIsArgsForCall) +} + +func (fake *FakeConfigParser) FindAllPlusAPIsCalls(stub func(context.Context, *model.NginxConfigContext) []*model.APIDetails) { + fake.findAllPlusAPIsMutex.Lock() + defer fake.findAllPlusAPIsMutex.Unlock() + fake.FindAllPlusAPIsStub = stub +} + +func (fake *FakeConfigParser) FindAllPlusAPIsArgsForCall(i int) (context.Context, *model.NginxConfigContext) { + fake.findAllPlusAPIsMutex.RLock() + defer fake.findAllPlusAPIsMutex.RUnlock() + argsForCall := fake.findAllPlusAPIsArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2 +} + +func (fake *FakeConfigParser) FindAllPlusAPIsReturns(result1 []*model.APIDetails) { + fake.findAllPlusAPIsMutex.Lock() + defer fake.findAllPlusAPIsMutex.Unlock() + fake.FindAllPlusAPIsStub = nil + fake.findAllPlusAPIsReturns = struct { + result1 []*model.APIDetails + }{result1} +} + +func (fake *FakeConfigParser) FindAllPlusAPIsReturnsOnCall(i int, result1 []*model.APIDetails) { + fake.findAllPlusAPIsMutex.Lock() + defer fake.findAllPlusAPIsMutex.Unlock() + fake.FindAllPlusAPIsStub = nil + if fake.findAllPlusAPIsReturnsOnCall == nil { + fake.findAllPlusAPIsReturnsOnCall = make(map[int]struct { + result1 []*model.APIDetails + }) + } + fake.findAllPlusAPIsReturnsOnCall[i] = struct { + result1 []*model.APIDetails + }{result1} +} + func (fake *FakeConfigParser) FindPlusAPI(arg1 context.Context, arg2 *model.NginxConfigContext) *model.APIDetails { fake.findPlusAPIMutex.Lock() ret, specificReturn := fake.findPlusAPIReturnsOnCall[len(fake.findPlusAPIArgsForCall)] @@ -245,6 +319,8 @@ func (fake *FakeConfigParser) ParseReturnsOnCall(i int, result1 *model.NginxConf func (fake *FakeConfigParser) Invocations() map[string][][]interface{} { fake.invocationsMutex.RLock() defer fake.invocationsMutex.RUnlock() + fake.findAllPlusAPIsMutex.RLock() + defer fake.findAllPlusAPIsMutex.RUnlock() fake.findPlusAPIMutex.RLock() defer fake.findPlusAPIMutex.RUnlock() fake.findStubStatusAPIMutex.RLock() diff --git a/internal/datasource/config/nginx_config_parser.go b/internal/datasource/config/nginx_config_parser.go index 5b0593f28..fd736f5ca 100644 --- a/internal/datasource/config/nginx_config_parser.go +++ b/internal/datasource/config/nginx_config_parser.go @@ -57,6 +57,7 @@ type ConfigParser interface { Parse(ctx context.Context, instance *mpi.Instance) (*model.NginxConfigContext, error) FindStubStatusAPI(ctx context.Context, nginxConfigContext *model.NginxConfigContext) *model.APIDetails FindPlusAPI(ctx context.Context, nginxConfigContext *model.NginxConfigContext) *model.APIDetails + FindAllPlusAPIs(ctx context.Context, nginxConfigContext *model.NginxConfigContext) []*model.APIDetails } var _ ConfigParser = (*NginxConfigParser)(nil) @@ -144,6 +145,17 @@ func (ncp *NginxConfigParser) FindPlusAPI( } } +func (ncp *NginxConfigParser) FindAllPlusAPIs( + ctx context.Context, nginxConfigContext *model.NginxConfigContext, +) []*model.APIDetails { + // This function returns the list populated by createNginxConfigContext/Parse + if nginxConfigContext.PlusAPIs == nil { + return []*model.APIDetails{} + } + + return nginxConfigContext.PlusAPIs +} + //nolint:gocognit,gocyclo,revive,cyclop // cognitive complexity is 51, cyclomatic complexity is 24 func (ncp *NginxConfigParser) createNginxConfigContext( ctx context.Context, @@ -695,6 +707,17 @@ func (ncp *NginxConfigParser) apiDetailsFromLocationDirective( if locChild.Directive != plusAPIDirective && locChild.Directive != stubStatusAPIDirective { continue } + + isWriteEnabled := false + if locChild.Directive == plusAPIDirective { + for _, arg := range locChild.Args { + if arg == "write=on" { + isWriteEnabled = true + slog.DebugContext(ctx, "Found NGINX Plus API with write=on", "location", current.Args[0]) + break + } + } + } addresses := ncp.parseAddressFromServerDirective(parent) path := ncp.parsePathFromLocationDirective(current) @@ -703,7 +726,7 @@ func (ncp *NginxConfigParser) apiDetailsFromLocationDirective( for _, address := range addresses { details = append( details, - ncp.createAPIDetails(locationDirectiveName, address, path, caCertLocation, isSSL), + ncp.createAPIDetails(locationDirectiveName, address, path, caCertLocation, isSSL, isWriteEnabled), ) } } @@ -714,6 +737,7 @@ func (ncp *NginxConfigParser) apiDetailsFromLocationDirective( func (ncp *NginxConfigParser) createAPIDetails( locationDirectiveName, address, path, caCertLocation string, isSSL bool, + isWriteEnabled bool, ) (details *model.APIDetails) { if strings.HasPrefix(address, "unix:") { format := unixStubStatusFormat @@ -723,18 +747,20 @@ func (ncp *NginxConfigParser) createAPIDetails( } details = &model.APIDetails{ - URL: fmt.Sprintf(format, path), - Listen: address, - Location: path, - Ca: caCertLocation, + URL: fmt.Sprintf(format, path), + Listen: address, + Location: path, + Ca: caCertLocation, + WriteEnabled: isWriteEnabled, } } 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, + Listen: address, + Location: path, + Ca: caCertLocation, + WriteEnabled: isWriteEnabled, } } diff --git a/internal/datasource/config/nginx_config_parser_test.go b/internal/datasource/config/nginx_config_parser_test.go index 8051a5200..a9aa617ac 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,14 @@ server { return 418; } ` + testConf23 = `server { + listen 127.0.0.1:9090; + location /writeapi { + api write=on; + allow 127.0.0.1; + deny all; + } +}` ) //nolint:maintidx // The test cannot be refactored @@ -815,108 +823,120 @@ func TestNginxConfigParser_urlsForLocationDirective(t *testing.T) { }{ { 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 +993,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 +1021,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,6 +1038,16 @@ 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, + }, } for _, test := range tests { diff --git a/internal/resource/resource_service_test.go b/internal/resource/resource_service_test.go index 5a76bb6aa..f1301909a 100644 --- a/internal/resource/resource_service_test.go +++ b/internal/resource/resource_service_test.go @@ -6,9 +6,11 @@ package resource import ( + "bytes" "context" "errors" "fmt" + "log/slog" "os" "path/filepath" "testing" @@ -270,11 +272,19 @@ func TestResourceService_createPlusClient(t *testing.T) { {Location: "/api", Listen: "localhost:443", Ca: caFile}, }) + var logBuffer bytes.Buffer + + tempHandler := slog.NewTextHandler(&logBuffer, &slog.HandlerOptions{ + Level: slog.LevelDebug, + }) + tempLogger := slog.New(tempHandler) + ctx := context.Background() tests := []struct { - err error - instance *v1.Instance - name string + err error + instance *v1.Instance + name string + expectedLog string }{ { name: "Test 1: Create Plus Client", @@ -296,10 +306,32 @@ func TestResourceService_createPlusClient(t *testing.T) { instance: protos.NginxPlusInstance([]string{}), err: errors.New("failed to preform API action, NGINX Plus API is not configured"), }, + { + name: "Test 5: Fallback to First API (No Write-Enabled)", + instance: createPlusInstanceWithApis([]*v1.APIDetails{ + {Location: "/read1", Listen: "localhost:80"}, + {Location: "/read2", Listen: "localhost:8080"}, + }), + err: nil, + expectedLog: "No write-enabled NGINX Plus API found. Write operations may fail.", + }, + { + name: "Test 6: Prioritize Write-Enabled API", + instance: createPlusInstanceWithApis([]*v1.APIDetails{ + {Location: "/read", Listen: "localhost:80"}, + {Location: "/write", Listen: "localhost:8080", WriteEnabled: true}, + }), + err: nil, + expectedLog: "Selected write-enabled NGINX Plus API for action", + }, } for _, test := range tests { t.Run(test.name, func(tt *testing.T) { + logBuffer.Reset() + originalLogger := slog.Default() + slog.SetDefault(tempLogger) + defer slog.SetDefault(originalLogger) resourceService := NewResourceService(ctx, types.AgentConfig()) resourceService.resource.Instances = []*v1.Instance{ protos.NginxOssInstance([]string{}), @@ -315,6 +347,10 @@ func TestResourceService_createPlusClient(t *testing.T) { // For the CA cert test, we can't easily verify the internal http.Client configuration // without exporting it or adding test hooks, so we'll just verify no error is returned } + if test.expectedLog != "" { + logOutput := logBuffer.String() + assert.Contains(tt, logOutput, test.expectedLog) + } }) } } From 14977b6dcd9319b1584f5f91f5624d0483039009 Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Wed, 1 Oct 2025 16:42:37 +0100 Subject: [PATCH 03/15] reducing Cognitive Complexity --- .../datasource/config/nginx_config_parser.go | 43 ++++++++++++------- 1 file changed, 28 insertions(+), 15 deletions(-) diff --git a/internal/datasource/config/nginx_config_parser.go b/internal/datasource/config/nginx_config_parser.go index fd736f5ca..98110e6c3 100644 --- a/internal/datasource/config/nginx_config_parser.go +++ b/internal/datasource/config/nginx_config_parser.go @@ -703,25 +703,18 @@ func (ncp *NginxConfigParser) apiDetailsFromLocationDirective( return nil } + isWriteEnabled := false for _, locChild := range current.Block { - if locChild.Directive != plusAPIDirective && locChild.Directive != stubStatusAPIDirective { - continue - } - - isWriteEnabled := false - if locChild.Directive == plusAPIDirective { - for _, arg := range locChild.Args { - if arg == "write=on" { - isWriteEnabled = true - slog.DebugContext(ctx, "Found NGINX Plus API with write=on", "location", current.Args[0]) - break - } - } + if ncp.isPlusAPIWriteEnabled(ctx, locChild, current.Args[0]) { + isWriteEnabled = true + break } + } - addresses := ncp.parseAddressFromServerDirective(parent) - path := ncp.parsePathFromLocationDirective(current) + addresses := ncp.parseAddressFromServerDirective(parent) + path := ncp.parsePathFromLocationDirective(current) + for _, locChild := range current.Block { if locChild.Directive == locationDirectiveName { for _, address := range addresses { details = append( @@ -735,6 +728,7 @@ func (ncp *NginxConfigParser) apiDetailsFromLocationDirective( return details } +//nolint:revive // isWriteEnabled flag is required for selecting Plus API func (ncp *NginxConfigParser) createAPIDetails( locationDirectiveName, address, path, caCertLocation string, isSSL bool, isWriteEnabled bool, @@ -914,3 +908,22 @@ func (ncp *NginxConfigParser) isDuplicateFile(nginxConfigContextFiles []*mpi.Fil return false } + +func (ncp *NginxConfigParser) isPlusAPIWriteEnabled(ctx context.Context, + directive *crossplane.Directive, + locationPath string, +) bool { + // Only check plus_api directives + if directive.Directive != plusAPIDirective { + return false + } + + for _, arg := range directive.Args { + if arg == "write=on" { + slog.DebugContext(ctx, "Found NGINX Plus API with write=on", "location", locationPath) + return true + } + } + + return false +} From 555223025c500a7b421b282e8f1247894af30aab Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Fri, 3 Oct 2025 12:13:25 +0100 Subject: [PATCH 04/15] fixed integration test, reduced complexity --- api/grpc/mpi/v1/command.pb.go | 6 +- api/grpc/mpi/v1/command.proto | 4 +- api/grpc/mpi/v1/common.pb.go | 2 +- api/grpc/mpi/v1/files.pb.go | 2 +- docs/proto/protos.md | 4 +- .../datasource/config/nginx_config_parser.go | 71 +++++++++++++------ 6 files changed, 60 insertions(+), 29 deletions(-) diff --git a/api/grpc/mpi/v1/command.pb.go b/api/grpc/mpi/v1/command.pb.go index 63448d66c..743eb72d8 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 @@ -2181,7 +2181,7 @@ type NGINXPlusRuntimeInfo struct { DynamicModules []string `protobuf:"bytes,5,rep,name=dynamic_modules,json=dynamicModules,proto3" json:"dynamic_modules,omitempty"` // the plus API details PlusApi *APIDetails `protobuf:"bytes,6,opt,name=plus_api,json=plusApi,proto3" json:"plus_api,omitempty"` - // to store all the endpoints details + // to parse all the plus API PlusApis []*APIDetails `protobuf:"bytes,7,rep,name=plus_apis,json=plusApis,proto3" json:"plus_apis,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache @@ -2274,7 +2274,7 @@ type APIDetails struct { Listen string `protobuf:"bytes,2,opt,name=listen,proto3" json:"listen,omitempty"` // the API CA file path Ca string `protobuf:"bytes,3,opt,name=Ca,proto3" json:"Ca,omitempty"` - // flag to know if this API location was configured with 'api write=on;' + // flag to know API is configured with 'write=on;' WriteEnabled bool `protobuf:"varint,4,opt,name=write_enabled,json=writeEnabled,proto3" json:"write_enabled,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache diff --git a/api/grpc/mpi/v1/command.proto b/api/grpc/mpi/v1/command.proto index 532d170c4..5b0d9233b 100644 --- a/api/grpc/mpi/v1/command.proto +++ b/api/grpc/mpi/v1/command.proto @@ -345,7 +345,7 @@ message NGINXPlusRuntimeInfo { repeated string dynamic_modules = 5; // the plus API details APIDetails plus_api = 6; - // to store all the endpoints details + // to parse all the plus API repeated APIDetails plus_apis = 7; } @@ -356,7 +356,7 @@ message APIDetails { string listen = 2; // the API CA file path string Ca = 3; - // flag to know if this API location was configured with 'api write=on;' + // flag to know API is configured with 'write=on;' bool write_enabled = 4; } 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/docs/proto/protos.md b/docs/proto/protos.md index c19baa46b..767aa1e21 100644 --- a/docs/proto/protos.md +++ b/docs/proto/protos.md @@ -697,7 +697,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 file path | -| write_enabled | [bool](#bool) | | flag to know if this API location was configured with 'api write=on;' | +| write_enabled | [bool](#bool) | | flag to know API is configured with 'write=on;' | @@ -1132,7 +1132,7 @@ A set of runtime NGINX Plus settings | loadable_modules | [string](#string) | repeated | List of NGINX potentially loadable modules (installed but not loaded). | | dynamic_modules | [string](#string) | repeated | List of NGINX dynamic modules. | | plus_api | [APIDetails](#mpi-v1-APIDetails) | | the plus API details | -| plus_apis | [APIDetails](#mpi-v1-APIDetails) | repeated | to store all the endpoints details | +| plus_apis | [APIDetails](#mpi-v1-APIDetails) | repeated | to parse all the plus API | diff --git a/internal/datasource/config/nginx_config_parser.go b/internal/datasource/config/nginx_config_parser.go index 98110e6c3..7fba5bb47 100644 --- a/internal/datasource/config/nginx_config_parser.go +++ b/internal/datasource/config/nginx_config_parser.go @@ -68,6 +68,14 @@ type ( current *crossplane.Directive, apiType string) []*model.APIDetails ) +type apiCreationParams struct { + locationDirectiveName string + path string + caCertLocation string + isSSL bool + isWriteEnabled bool +} + func NewNginxConfigParser(agentConfig *config.Config) *NginxConfigParser { return &NginxConfigParser{ agentConfig: agentConfig, @@ -148,7 +156,6 @@ func (ncp *NginxConfigParser) FindPlusAPI( func (ncp *NginxConfigParser) FindAllPlusAPIs( ctx context.Context, nginxConfigContext *model.NginxConfigContext, ) []*model.APIDetails { - // This function returns the list populated by createNginxConfigContext/Parse if nginxConfigContext.PlusAPIs == nil { return []*model.APIDetails{} } @@ -714,47 +721,55 @@ func (ncp *NginxConfigParser) apiDetailsFromLocationDirective( addresses := ncp.parseAddressFromServerDirective(parent) path := ncp.parsePathFromLocationDirective(current) + params := apiCreationParams{ + locationDirectiveName: locationDirectiveName, + path: path, + caCertLocation: caCertLocation, + isSSL: isSSL, + isWriteEnabled: isWriteEnabled, + } + for _, locChild := range current.Block { + if locChild.Directive != plusAPIDirective && locChild.Directive != stubStatusAPIDirective { + continue + } + if locChild.Directive == locationDirectiveName { - for _, address := range addresses { - details = append( - details, - ncp.createAPIDetails(locationDirectiveName, address, path, caCertLocation, isSSL, isWriteEnabled), - ) - } + details = append(details, ncp.createAPIDetailsForAddresses( + params, + addresses, + )...) } } return details } -//nolint:revive // isWriteEnabled flag is required for selecting Plus API func (ncp *NginxConfigParser) createAPIDetails( - locationDirectiveName, address, path, caCertLocation string, isSSL bool, - isWriteEnabled bool, + params apiCreationParams, address string, ) (details *model.APIDetails) { if strings.HasPrefix(address, "unix:") { format := unixStubStatusFormat - if locationDirectiveName == plusAPIDirective { + if params.locationDirectiveName == plusAPIDirective { format = unixPlusAPIFormat } details = &model.APIDetails{ - URL: fmt.Sprintf(format, path), + URL: fmt.Sprintf(format, params.path), Listen: address, - Location: path, - Ca: caCertLocation, - WriteEnabled: isWriteEnabled, + Location: params.path, + Ca: params.caCertLocation, + WriteEnabled: params.isWriteEnabled, } } else { details = &model.APIDetails{ - URL: fmt.Sprintf("%s://%s%s", map[bool]string{true: "https", false: "http"}[isSSL], - address, path), + URL: fmt.Sprintf("%s://%s%s", map[bool]string{true: "https", false: "http"}[params.isSSL], + address, params.path), Listen: address, - Location: path, - Ca: caCertLocation, - WriteEnabled: isWriteEnabled, + Location: params.path, + Ca: params.caCertLocation, + WriteEnabled: params.isWriteEnabled, } } @@ -909,6 +924,22 @@ func (ncp *NginxConfigParser) isDuplicateFile(nginxConfigContextFiles []*mpi.Fil return false } +func (ncp *NginxConfigParser) createAPIDetailsForAddresses( + params apiCreationParams, + addresses []string, +) (details []*model.APIDetails) { + for _, address := range addresses { + details = append(details, + ncp.createAPIDetails( + params, + address, + ), + ) + } + + return details +} + func (ncp *NginxConfigParser) isPlusAPIWriteEnabled(ctx context.Context, directive *crossplane.Directive, locationPath string, From ed28bf1144fb057f417054a79773f49937adc89e Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Fri, 3 Oct 2025 17:02:50 +0100 Subject: [PATCH 05/15] fixing integration test --- .../datasource/config/nginx_config_parser.go | 26 +++++++------------ 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/internal/datasource/config/nginx_config_parser.go b/internal/datasource/config/nginx_config_parser.go index 7fba5bb47..3e1c3f302 100644 --- a/internal/datasource/config/nginx_config_parser.go +++ b/internal/datasource/config/nginx_config_parser.go @@ -710,31 +710,25 @@ func (ncp *NginxConfigParser) apiDetailsFromLocationDirective( return nil } - isWriteEnabled := false - for _, locChild := range current.Block { - if ncp.isPlusAPIWriteEnabled(ctx, locChild, current.Args[0]) { - isWriteEnabled = true - break - } - } - addresses := ncp.parseAddressFromServerDirective(parent) path := ncp.parsePathFromLocationDirective(current) - params := apiCreationParams{ - locationDirectiveName: locationDirectiveName, - path: path, - caCertLocation: caCertLocation, - isSSL: isSSL, - isWriteEnabled: isWriteEnabled, - } - for _, locChild := range current.Block { if locChild.Directive != plusAPIDirective && locChild.Directive != stubStatusAPIDirective { continue } if locChild.Directive == locationDirectiveName { + isWriteEnabled := ncp.isPlusAPIWriteEnabled(ctx, locChild, current.Args[0]) + + params := apiCreationParams{ + locationDirectiveName: locationDirectiveName, + path: path, + caCertLocation: caCertLocation, + isSSL: isSSL, + isWriteEnabled: isWriteEnabled, + } + details = append(details, ncp.createAPIDetailsForAddresses( params, addresses, From 6a55e6907a91bde39b931ec8b72d79a25689efa3 Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Fri, 3 Oct 2025 18:30:00 +0100 Subject: [PATCH 06/15] fixing integration test --- internal/datasource/proto/instance.go | 41 +++++++++++ internal/datasource/proto/instance_test.go | 79 +++++++++++++++++++++- 2 files changed, 117 insertions(+), 3 deletions(-) diff --git a/internal/datasource/proto/instance.go b/internal/datasource/proto/instance.go index 38c802030..3a79ee62e 100644 --- a/internal/datasource/proto/instance.go +++ b/internal/datasource/proto/instance.go @@ -15,6 +15,17 @@ import ( func NginxPlusRuntimeInfoEqual(nginxPlusRuntimeInfo *mpi.NGINXPlusRuntimeInfo, nginxConfigContext *model.NginxConfigContext, accessLogs, errorLogs []string, ) bool { + convertedPlusAPIs := convertToMpiAPIDetailsSlice(nginxConfigContext.PlusAPIs) + + if !reflect.DeepEqual(nginxPlusRuntimeInfo.GetPlusApis(), convertedPlusAPIs) { + return false + } + + if nginxPlusRuntimeInfo.GetPlusApi().GetWriteEnabled() != nginxConfigContext.PlusAPI.WriteEnabled || + nginxPlusRuntimeInfo.GetPlusApi().GetCa() != nginxConfigContext.PlusAPI.Ca { + return false + } + if !reflect.DeepEqual(nginxPlusRuntimeInfo.GetAccessLogs(), accessLogs) || !reflect.DeepEqual(nginxPlusRuntimeInfo.GetErrorLogs(), errorLogs) || nginxPlusRuntimeInfo.GetStubStatus().GetListen() != nginxConfigContext.StubStatus.Listen || @@ -59,6 +70,11 @@ func UpdateNginxInstanceRuntime( nginxPlusRuntimeInfo.PlusApi.Listen = nginxConfigContext.PlusAPI.Listen nginxPlusRuntimeInfo.StubStatus.Location = nginxConfigContext.StubStatus.Location nginxPlusRuntimeInfo.PlusApi.Location = nginxConfigContext.PlusAPI.Location + + nginxPlusRuntimeInfo.PlusApi.WriteEnabled = nginxConfigContext.PlusAPI.WriteEnabled + nginxPlusRuntimeInfo.PlusApi.Ca = nginxConfigContext.PlusAPI.Ca + + nginxPlusRuntimeInfo.PlusApis = convertToMpiAPIDetailsSlice(nginxConfigContext.PlusAPIs) updatesRequired = true } } else { @@ -75,3 +91,28 @@ func UpdateNginxInstanceRuntime( return updatesRequired } + +func convertToMpiAPIDetails(modelAPI *model.APIDetails) *mpi.APIDetails { + if modelAPI == nil { + return nil + } + + return &mpi.APIDetails{ + Listen: modelAPI.Listen, + Location: modelAPI.Location, + Ca: modelAPI.Ca, + WriteEnabled: modelAPI.WriteEnabled, + } +} + +func convertToMpiAPIDetailsSlice(modelAPIs []*model.APIDetails) []*mpi.APIDetails { + if modelAPIs == nil { + return nil + } + mpiAPIs := make([]*mpi.APIDetails, 0, len(modelAPIs)) + for _, api := range modelAPIs { + mpiAPIs = append(mpiAPIs, convertToMpiAPIDetails(api)) + } + + return mpiAPIs +} diff --git a/internal/datasource/proto/instance_test.go b/internal/datasource/proto/instance_test.go index 0ad5c25a8..81130c959 100644 --- a/internal/datasource/proto/instance_test.go +++ b/internal/datasource/proto/instance_test.go @@ -14,6 +14,54 @@ import ( "github.com/stretchr/testify/assert" ) +var plusAPIs = []*model.APIDetails{ + { + URL: "http://127.0.0.1:8081/api", + Listen: "127.0.0.1:8081", + Location: "/api", + WriteEnabled: false, + Ca: "", + }, + { + URL: "unix:/var/run/nginx/api.sock", + Listen: "unix:/var/run/nginx/api.sock", + Location: "/api", + WriteEnabled: true, // Crucial for selection logic + Ca: "/etc/certs/my_ca.pem", + }, +} + +var nginxPlusConfigContextForUpdate = &model.NginxConfigContext{ + AccessLogs: []*model.AccessLog{ + { + Name: "/usr/local/var/log/nginx/access.log", + }, + }, + ErrorLogs: []*model.ErrorLog{ + { + Name: "/usr/local/var/log/nginx/error.log", + }, + }, + PlusAPI: plusAPIs[1], + StubStatus: &model.APIDetails{ + URL: "http://127.0.0.1:8081/status", + Listen: "127.0.0.1:8081", + }, + PlusAPIs: plusAPIs, +} + +func convertAPIDetailsSliceForTest(modelAPIs []*model.APIDetails) []*mpi.APIDetails { + if modelAPIs == nil { + return nil + } + mpiAPIs := make([]*mpi.APIDetails, 0, len(modelAPIs)) + for _, api := range modelAPIs { + mpiAPIs = append(mpiAPIs, convertToMpiAPIDetails(api)) + } + + return mpiAPIs +} + func TestInstanceWatcherService_updateNginxInstanceRuntime(t *testing.T) { nginxOSSConfigContext := &model.NginxConfigContext{ AccessLogs: []*model.AccessLog{ @@ -68,12 +116,32 @@ func TestInstanceWatcherService_updateNginxInstanceRuntime(t *testing.T) { nginxConfigContext: nginxPlusConfigContext, instance: protos.NginxPlusInstance([]string{}), }, + { + name: "Test 3: Plus Instance - PlusAPIs Update", + nginxConfigContext: nginxPlusConfigContextForUpdate, + instance: protos.NginxPlusInstance([]string{}), + }, } for _, test := range tests { - t.Run(test.name, func(tt *testing.T) { + t.Run(test.name, func(t *testing.T) { + updatesRequired := UpdateNginxInstanceRuntime(test.instance, test.nginxConfigContext) UpdateNginxInstanceRuntime(test.instance, test.nginxConfigContext) - if test.name == "Test 2: Plus Instance" { + switch test.name { + case "Test 3: Plus Instance - PlusAPIs Update": + assert.True(t, updatesRequired, + "UpdateNginxInstanceRuntime should return true when PlusAPIs are updated") + expectedAPIs := convertAPIDetailsSliceForTest(test.nginxConfigContext.PlusAPIs) + assert.ElementsMatch(t, expectedAPIs, + test.instance.GetInstanceRuntime().GetNginxPlusRuntimeInfo().GetPlusApis()) + assert.Equal(t, test.nginxConfigContext.PlusAPI.WriteEnabled, test.instance.GetInstanceRuntime(). + GetNginxPlusRuntimeInfo().GetPlusApi().GetWriteEnabled()) + assert.Equal(t, test.nginxConfigContext.PlusAPI.Ca, test.instance.GetInstanceRuntime(). + GetNginxPlusRuntimeInfo().GetPlusApi().GetCa()) + assert.Equal(t, test.nginxConfigContext.PlusAPI.Listen, test.instance.GetInstanceRuntime(). + GetNginxPlusRuntimeInfo().GetPlusApi().GetListen()) + + case "Test 2: Plus Instance": assert.Equal(t, test.nginxConfigContext.AccessLogs[0].Name, test.instance.GetInstanceRuntime(). GetNginxPlusRuntimeInfo().GetAccessLogs()[0]) assert.Equal(t, test.nginxConfigContext.ErrorLogs[0].Name, test.instance.GetInstanceRuntime(). @@ -86,7 +154,12 @@ func TestInstanceWatcherService_updateNginxInstanceRuntime(t *testing.T) { GetNginxPlusRuntimeInfo().GetStubStatus().GetListen()) assert.Equal(t, test.nginxConfigContext.PlusAPI.Listen, test.instance.GetInstanceRuntime(). GetNginxPlusRuntimeInfo().GetPlusApi().GetListen()) - } else { + assert.Equal(t, test.nginxConfigContext.PlusAPI.WriteEnabled, test.instance.GetInstanceRuntime(). + GetNginxPlusRuntimeInfo().GetPlusApi().GetWriteEnabled()) + assert.Equal(t, test.nginxConfigContext.PlusAPI.Ca, test.instance.GetInstanceRuntime(). + GetNginxPlusRuntimeInfo().GetPlusApi().GetCa()) + + default: assert.Equal(t, test.nginxConfigContext.AccessLogs[0].Name, test.instance.GetInstanceRuntime(). GetNginxRuntimeInfo().GetAccessLogs()[0]) assert.Equal(t, test.nginxConfigContext.ErrorLogs[0].Name, test.instance.GetInstanceRuntime(). From 66cc63bf8104e941e05ca105f1cdd75c6e2b8cd8 Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Fri, 3 Oct 2025 19:05:26 +0100 Subject: [PATCH 07/15] fixing integration test --- internal/datasource/proto/instance_test.go | 29 +++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/internal/datasource/proto/instance_test.go b/internal/datasource/proto/instance_test.go index 81130c959..71d10cd5c 100644 --- a/internal/datasource/proto/instance_test.go +++ b/internal/datasource/proto/instance_test.go @@ -121,12 +121,16 @@ func TestInstanceWatcherService_updateNginxInstanceRuntime(t *testing.T) { nginxConfigContext: nginxPlusConfigContextForUpdate, instance: protos.NginxPlusInstance([]string{}), }, + { + name: "Test 4: Plus Instance - No Update Required", + nginxConfigContext: nginxPlusConfigContextForUpdate, + instance: createPopulatedNginxPlusInstance(nginxPlusConfigContextForUpdate), + }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { updatesRequired := UpdateNginxInstanceRuntime(test.instance, test.nginxConfigContext) - UpdateNginxInstanceRuntime(test.instance, test.nginxConfigContext) switch test.name { case "Test 3: Plus Instance - PlusAPIs Update": assert.True(t, updatesRequired, @@ -140,6 +144,9 @@ func TestInstanceWatcherService_updateNginxInstanceRuntime(t *testing.T) { GetNginxPlusRuntimeInfo().GetPlusApi().GetCa()) assert.Equal(t, test.nginxConfigContext.PlusAPI.Listen, test.instance.GetInstanceRuntime(). GetNginxPlusRuntimeInfo().GetPlusApi().GetListen()) + case "Test 4: Plus Instance - No Update Required": + assert.False(t, updatesRequired, + "UpdateNginxInstanceRuntime should return false when runtime already matches config") case "Test 2: Plus Instance": assert.Equal(t, test.nginxConfigContext.AccessLogs[0].Name, test.instance.GetInstanceRuntime(). @@ -172,3 +179,23 @@ func TestInstanceWatcherService_updateNginxInstanceRuntime(t *testing.T) { }) } } + +func createPopulatedNginxPlusInstance(configContext *model.NginxConfigContext) *mpi.Instance { + instance := protos.NginxPlusInstance([]string{}) + runtimeInfo := instance.GetInstanceRuntime().GetNginxPlusRuntimeInfo() + runtimeInfo.PlusApi.Listen = configContext.PlusAPI.Listen + + runtimeInfo.PlusApi.Location = configContext.PlusAPI.Location + runtimeInfo.PlusApi.WriteEnabled = configContext.PlusAPI.WriteEnabled + runtimeInfo.PlusApi.Ca = configContext.PlusAPI.Ca + + runtimeInfo.PlusApis = convertAPIDetailsSliceForTest(configContext.PlusAPIs) + + runtimeInfo.AccessLogs = model.ConvertAccessLogs(configContext.AccessLogs) + runtimeInfo.ErrorLogs = model.ConvertErrorLogs(configContext.ErrorLogs) + + runtimeInfo.StubStatus.Listen = configContext.StubStatus.Listen + runtimeInfo.StubStatus.Location = configContext.StubStatus.Location + + return instance +} From aeb6c68017a67a4069d633c402fbe4590dcbd34a Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Mon, 6 Oct 2025 16:40:34 +0100 Subject: [PATCH 08/15] sorting while creating context --- api/grpc/mpi/v1/command.pb.go | 65 ++++---- api/grpc/mpi/v1/command.pb.validate.go | 34 ----- api/grpc/mpi/v1/command.proto | 2 - docs/proto/protos.md | 1 - .../config/configfakes/fake_config_parser.go | 76 --------- .../datasource/config/nginx_config_parser.go | 117 +++++++------- internal/datasource/proto/instance.go | 43 +----- internal/datasource/proto/instance_test.go | 144 +++++------------- internal/model/config.go | 3 +- internal/resource/resource_service.go | 37 +---- internal/resource/resource_service_test.go | 77 +++------- 11 files changed, 145 insertions(+), 454 deletions(-) diff --git a/api/grpc/mpi/v1/command.pb.go b/api/grpc/mpi/v1/command.pb.go index 743eb72d8..fbceea13b 100644 --- a/api/grpc/mpi/v1/command.pb.go +++ b/api/grpc/mpi/v1/command.pb.go @@ -2180,9 +2180,7 @@ type NGINXPlusRuntimeInfo struct { // List of NGINX dynamic modules. DynamicModules []string `protobuf:"bytes,5,rep,name=dynamic_modules,json=dynamicModules,proto3" json:"dynamic_modules,omitempty"` // the plus API details - PlusApi *APIDetails `protobuf:"bytes,6,opt,name=plus_api,json=plusApi,proto3" json:"plus_api,omitempty"` - // to parse all the plus API - PlusApis []*APIDetails `protobuf:"bytes,7,rep,name=plus_apis,json=plusApis,proto3" json:"plus_apis,omitempty"` + PlusApi *APIDetails `protobuf:"bytes,6,opt,name=plus_api,json=plusApi,proto3" json:"plus_api,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache } @@ -2259,13 +2257,6 @@ func (x *NGINXPlusRuntimeInfo) GetPlusApi() *APIDetails { return nil } -func (x *NGINXPlusRuntimeInfo) GetPlusApis() []*APIDetails { - if x != nil { - return x.PlusApis - } - return nil -} - type APIDetails struct { state protoimpl.MessageState `protogen:"open.v1"` // the API location directive @@ -2889,7 +2880,7 @@ const file_mpi_v1_command_proto_rawDesc = "" + "\n" + "error_logs\x18\x03 \x03(\tR\terrorLogs\x12)\n" + "\x10loadable_modules\x18\x04 \x03(\tR\x0floadableModules\x12'\n" + - "\x0fdynamic_modules\x18\x05 \x03(\tR\x0edynamicModules\"\xbf\x02\n" + + "\x0fdynamic_modules\x18\x05 \x03(\tR\x0edynamicModules\"\x8e\x02\n" + "\x14NGINXPlusRuntimeInfo\x123\n" + "\vstub_status\x18\x01 \x01(\v2\x12.mpi.v1.APIDetailsR\n" + "stubStatus\x12\x1f\n" + @@ -2899,8 +2890,7 @@ const file_mpi_v1_command_proto_rawDesc = "" + "error_logs\x18\x03 \x03(\tR\terrorLogs\x12)\n" + "\x10loadable_modules\x18\x04 \x03(\tR\x0floadableModules\x12'\n" + "\x0fdynamic_modules\x18\x05 \x03(\tR\x0edynamicModules\x12-\n" + - "\bplus_api\x18\x06 \x01(\v2\x12.mpi.v1.APIDetailsR\aplusApi\x12/\n" + - "\tplus_apis\x18\a \x03(\v2\x12.mpi.v1.APIDetailsR\bplusApis\"u\n" + + "\bplus_api\x18\x06 \x01(\v2\x12.mpi.v1.APIDetailsR\aplusApi\"u\n" + "\n" + "APIDetails\x12\x1a\n" + "\blocation\x18\x01 \x01(\tR\blocation\x12\x16\n" + @@ -3050,31 +3040,30 @@ var file_mpi_v1_command_proto_depIdxs = []int32{ 34, // 43: mpi.v1.NGINXRuntimeInfo.stub_status:type_name -> mpi.v1.APIDetails 34, // 44: mpi.v1.NGINXPlusRuntimeInfo.stub_status:type_name -> mpi.v1.APIDetails 34, // 45: mpi.v1.NGINXPlusRuntimeInfo.plus_api:type_name -> mpi.v1.APIDetails - 34, // 46: mpi.v1.NGINXPlusRuntimeInfo.plus_apis:type_name -> mpi.v1.APIDetails - 38, // 47: mpi.v1.AgentConfig.command:type_name -> mpi.v1.CommandServer - 40, // 48: mpi.v1.AgentConfig.metrics:type_name -> mpi.v1.MetricsServer - 41, // 49: mpi.v1.AgentConfig.file:type_name -> mpi.v1.FileServer - 45, // 50: mpi.v1.AgentConfig.labels:type_name -> google.protobuf.Struct - 39, // 51: mpi.v1.AgentConfig.auxiliary_command:type_name -> mpi.v1.AuxiliaryCommandServer - 46, // 52: mpi.v1.CommandServer.server:type_name -> mpi.v1.ServerSettings - 47, // 53: mpi.v1.CommandServer.auth:type_name -> mpi.v1.AuthSettings - 48, // 54: mpi.v1.CommandServer.tls:type_name -> mpi.v1.TLSSettings - 46, // 55: mpi.v1.AuxiliaryCommandServer.server:type_name -> mpi.v1.ServerSettings - 47, // 56: mpi.v1.AuxiliaryCommandServer.auth:type_name -> mpi.v1.AuthSettings - 48, // 57: mpi.v1.AuxiliaryCommandServer.tls:type_name -> mpi.v1.TLSSettings - 2, // 58: mpi.v1.CommandService.CreateConnection:input_type -> mpi.v1.CreateConnectionRequest - 8, // 59: mpi.v1.CommandService.UpdateDataPlaneStatus:input_type -> mpi.v1.UpdateDataPlaneStatusRequest - 11, // 60: mpi.v1.CommandService.UpdateDataPlaneHealth:input_type -> mpi.v1.UpdateDataPlaneHealthRequest - 13, // 61: mpi.v1.CommandService.Subscribe:input_type -> mpi.v1.DataPlaneResponse - 7, // 62: mpi.v1.CommandService.CreateConnection:output_type -> mpi.v1.CreateConnectionResponse - 9, // 63: mpi.v1.CommandService.UpdateDataPlaneStatus:output_type -> mpi.v1.UpdateDataPlaneStatusResponse - 12, // 64: mpi.v1.CommandService.UpdateDataPlaneHealth:output_type -> mpi.v1.UpdateDataPlaneHealthResponse - 14, // 65: mpi.v1.CommandService.Subscribe:output_type -> mpi.v1.ManagementPlaneRequest - 62, // [62:66] is the sub-list for method output_type - 58, // [58:62] is the sub-list for method input_type - 58, // [58:58] is the sub-list for extension type_name - 58, // [58:58] is the sub-list for extension extendee - 0, // [0:58] is the sub-list for field type_name + 38, // 46: mpi.v1.AgentConfig.command:type_name -> mpi.v1.CommandServer + 40, // 47: mpi.v1.AgentConfig.metrics:type_name -> mpi.v1.MetricsServer + 41, // 48: mpi.v1.AgentConfig.file:type_name -> mpi.v1.FileServer + 45, // 49: mpi.v1.AgentConfig.labels:type_name -> google.protobuf.Struct + 39, // 50: mpi.v1.AgentConfig.auxiliary_command:type_name -> mpi.v1.AuxiliaryCommandServer + 46, // 51: mpi.v1.CommandServer.server:type_name -> mpi.v1.ServerSettings + 47, // 52: mpi.v1.CommandServer.auth:type_name -> mpi.v1.AuthSettings + 48, // 53: mpi.v1.CommandServer.tls:type_name -> mpi.v1.TLSSettings + 46, // 54: mpi.v1.AuxiliaryCommandServer.server:type_name -> mpi.v1.ServerSettings + 47, // 55: mpi.v1.AuxiliaryCommandServer.auth:type_name -> mpi.v1.AuthSettings + 48, // 56: mpi.v1.AuxiliaryCommandServer.tls:type_name -> mpi.v1.TLSSettings + 2, // 57: mpi.v1.CommandService.CreateConnection:input_type -> mpi.v1.CreateConnectionRequest + 8, // 58: mpi.v1.CommandService.UpdateDataPlaneStatus:input_type -> mpi.v1.UpdateDataPlaneStatusRequest + 11, // 59: mpi.v1.CommandService.UpdateDataPlaneHealth:input_type -> mpi.v1.UpdateDataPlaneHealthRequest + 13, // 60: mpi.v1.CommandService.Subscribe:input_type -> mpi.v1.DataPlaneResponse + 7, // 61: mpi.v1.CommandService.CreateConnection:output_type -> mpi.v1.CreateConnectionResponse + 9, // 62: mpi.v1.CommandService.UpdateDataPlaneStatus:output_type -> mpi.v1.UpdateDataPlaneStatusResponse + 12, // 63: mpi.v1.CommandService.UpdateDataPlaneHealth:output_type -> mpi.v1.UpdateDataPlaneHealthResponse + 14, // 64: mpi.v1.CommandService.Subscribe:output_type -> mpi.v1.ManagementPlaneRequest + 61, // [61:65] is the sub-list for method output_type + 57, // [57:61] is the sub-list for method input_type + 57, // [57:57] is the sub-list for extension type_name + 57, // [57:57] is the sub-list for extension extendee + 0, // [0:57] is the sub-list for field type_name } func init() { file_mpi_v1_command_proto_init() } diff --git a/api/grpc/mpi/v1/command.pb.validate.go b/api/grpc/mpi/v1/command.pb.validate.go index e3d480eb1..d35936df3 100644 --- a/api/grpc/mpi/v1/command.pb.validate.go +++ b/api/grpc/mpi/v1/command.pb.validate.go @@ -4787,40 +4787,6 @@ func (m *NGINXPlusRuntimeInfo) validate(all bool) error { } } - for idx, item := range m.GetPlusApis() { - _, _ = idx, item - - if all { - switch v := interface{}(item).(type) { - case interface{ ValidateAll() error }: - if err := v.ValidateAll(); err != nil { - errors = append(errors, NGINXPlusRuntimeInfoValidationError{ - field: fmt.Sprintf("PlusApis[%v]", idx), - reason: "embedded message failed validation", - cause: err, - }) - } - case interface{ Validate() error }: - if err := v.Validate(); err != nil { - errors = append(errors, NGINXPlusRuntimeInfoValidationError{ - field: fmt.Sprintf("PlusApis[%v]", idx), - reason: "embedded message failed validation", - cause: err, - }) - } - } - } else if v, ok := interface{}(item).(interface{ Validate() error }); ok { - if err := v.Validate(); err != nil { - return NGINXPlusRuntimeInfoValidationError{ - field: fmt.Sprintf("PlusApis[%v]", idx), - reason: "embedded message failed validation", - cause: err, - } - } - } - - } - if len(errors) > 0 { return NGINXPlusRuntimeInfoMultiError(errors) } diff --git a/api/grpc/mpi/v1/command.proto b/api/grpc/mpi/v1/command.proto index 5b0d9233b..894e64acd 100644 --- a/api/grpc/mpi/v1/command.proto +++ b/api/grpc/mpi/v1/command.proto @@ -345,8 +345,6 @@ message NGINXPlusRuntimeInfo { repeated string dynamic_modules = 5; // the plus API details APIDetails plus_api = 6; - // to parse all the plus API - repeated APIDetails plus_apis = 7; } message APIDetails { diff --git a/docs/proto/protos.md b/docs/proto/protos.md index 767aa1e21..529629ff3 100644 --- a/docs/proto/protos.md +++ b/docs/proto/protos.md @@ -1132,7 +1132,6 @@ A set of runtime NGINX Plus settings | loadable_modules | [string](#string) | repeated | List of NGINX potentially loadable modules (installed but not loaded). | | dynamic_modules | [string](#string) | repeated | List of NGINX dynamic modules. | | plus_api | [APIDetails](#mpi-v1-APIDetails) | | the plus API details | -| plus_apis | [APIDetails](#mpi-v1-APIDetails) | repeated | to parse all the plus API | diff --git a/internal/datasource/config/configfakes/fake_config_parser.go b/internal/datasource/config/configfakes/fake_config_parser.go index 865833d38..c4c7ad6ec 100644 --- a/internal/datasource/config/configfakes/fake_config_parser.go +++ b/internal/datasource/config/configfakes/fake_config_parser.go @@ -11,18 +11,6 @@ import ( ) type FakeConfigParser struct { - FindAllPlusAPIsStub func(context.Context, *model.NginxConfigContext) []*model.APIDetails - findAllPlusAPIsMutex sync.RWMutex - findAllPlusAPIsArgsForCall []struct { - arg1 context.Context - arg2 *model.NginxConfigContext - } - findAllPlusAPIsReturns struct { - result1 []*model.APIDetails - } - findAllPlusAPIsReturnsOnCall map[int]struct { - result1 []*model.APIDetails - } FindPlusAPIStub func(context.Context, *model.NginxConfigContext) *model.APIDetails findPlusAPIMutex sync.RWMutex findPlusAPIArgsForCall []struct { @@ -65,68 +53,6 @@ type FakeConfigParser struct { invocationsMutex sync.RWMutex } -func (fake *FakeConfigParser) FindAllPlusAPIs(arg1 context.Context, arg2 *model.NginxConfigContext) []*model.APIDetails { - fake.findAllPlusAPIsMutex.Lock() - ret, specificReturn := fake.findAllPlusAPIsReturnsOnCall[len(fake.findAllPlusAPIsArgsForCall)] - fake.findAllPlusAPIsArgsForCall = append(fake.findAllPlusAPIsArgsForCall, struct { - arg1 context.Context - arg2 *model.NginxConfigContext - }{arg1, arg2}) - stub := fake.FindAllPlusAPIsStub - fakeReturns := fake.findAllPlusAPIsReturns - fake.recordInvocation("FindAllPlusAPIs", []interface{}{arg1, arg2}) - fake.findAllPlusAPIsMutex.Unlock() - if stub != nil { - return stub(arg1, arg2) - } - if specificReturn { - return ret.result1 - } - return fakeReturns.result1 -} - -func (fake *FakeConfigParser) FindAllPlusAPIsCallCount() int { - fake.findAllPlusAPIsMutex.RLock() - defer fake.findAllPlusAPIsMutex.RUnlock() - return len(fake.findAllPlusAPIsArgsForCall) -} - -func (fake *FakeConfigParser) FindAllPlusAPIsCalls(stub func(context.Context, *model.NginxConfigContext) []*model.APIDetails) { - fake.findAllPlusAPIsMutex.Lock() - defer fake.findAllPlusAPIsMutex.Unlock() - fake.FindAllPlusAPIsStub = stub -} - -func (fake *FakeConfigParser) FindAllPlusAPIsArgsForCall(i int) (context.Context, *model.NginxConfigContext) { - fake.findAllPlusAPIsMutex.RLock() - defer fake.findAllPlusAPIsMutex.RUnlock() - argsForCall := fake.findAllPlusAPIsArgsForCall[i] - return argsForCall.arg1, argsForCall.arg2 -} - -func (fake *FakeConfigParser) FindAllPlusAPIsReturns(result1 []*model.APIDetails) { - fake.findAllPlusAPIsMutex.Lock() - defer fake.findAllPlusAPIsMutex.Unlock() - fake.FindAllPlusAPIsStub = nil - fake.findAllPlusAPIsReturns = struct { - result1 []*model.APIDetails - }{result1} -} - -func (fake *FakeConfigParser) FindAllPlusAPIsReturnsOnCall(i int, result1 []*model.APIDetails) { - fake.findAllPlusAPIsMutex.Lock() - defer fake.findAllPlusAPIsMutex.Unlock() - fake.FindAllPlusAPIsStub = nil - if fake.findAllPlusAPIsReturnsOnCall == nil { - fake.findAllPlusAPIsReturnsOnCall = make(map[int]struct { - result1 []*model.APIDetails - }) - } - fake.findAllPlusAPIsReturnsOnCall[i] = struct { - result1 []*model.APIDetails - }{result1} -} - func (fake *FakeConfigParser) FindPlusAPI(arg1 context.Context, arg2 *model.NginxConfigContext) *model.APIDetails { fake.findPlusAPIMutex.Lock() ret, specificReturn := fake.findPlusAPIReturnsOnCall[len(fake.findPlusAPIArgsForCall)] @@ -319,8 +245,6 @@ func (fake *FakeConfigParser) ParseReturnsOnCall(i int, result1 *model.NginxConf func (fake *FakeConfigParser) Invocations() map[string][][]interface{} { fake.invocationsMutex.RLock() defer fake.invocationsMutex.RUnlock() - fake.findAllPlusAPIsMutex.RLock() - defer fake.findAllPlusAPIsMutex.RUnlock() fake.findPlusAPIMutex.RLock() defer fake.findPlusAPIMutex.RUnlock() fake.findStubStatusAPIMutex.RLock() diff --git a/internal/datasource/config/nginx_config_parser.go b/internal/datasource/config/nginx_config_parser.go index 3e1c3f302..1fdef8268 100644 --- a/internal/datasource/config/nginx_config_parser.go +++ b/internal/datasource/config/nginx_config_parser.go @@ -57,7 +57,6 @@ type ConfigParser interface { Parse(ctx context.Context, instance *mpi.Instance) (*model.NginxConfigContext, error) FindStubStatusAPI(ctx context.Context, nginxConfigContext *model.NginxConfigContext) *model.APIDetails FindPlusAPI(ctx context.Context, nginxConfigContext *model.NginxConfigContext) *model.APIDetails - FindAllPlusAPIs(ctx context.Context, nginxConfigContext *model.NginxConfigContext) []*model.APIDetails } var _ ConfigParser = (*NginxConfigParser)(nil) @@ -68,12 +67,13 @@ type ( current *crossplane.Directive, apiType string) []*model.APIDetails ) -type apiCreationParams struct { +type createAPIDetailsParams struct { locationDirectiveName string + address string path string caCertLocation string isSSL bool - isWriteEnabled bool + writeEnabled bool } func NewNginxConfigParser(agentConfig *config.Config) *NginxConfigParser { @@ -153,16 +153,6 @@ func (ncp *NginxConfigParser) FindPlusAPI( } } -func (ncp *NginxConfigParser) FindAllPlusAPIs( - ctx context.Context, nginxConfigContext *model.NginxConfigContext, -) []*model.APIDetails { - if nginxConfigContext.PlusAPIs == nil { - return []*model.APIDetails{} - } - - return nginxConfigContext.PlusAPIs -} - //nolint:gocognit,gocyclo,revive,cyclop // cognitive complexity is 51, cyclomatic complexity is 24 func (ncp *NginxConfigParser) createNginxConfigContext( ctx context.Context, @@ -270,6 +260,8 @@ func (ncp *NginxConfigParser) createNginxConfigContext( nginxConfigContext.PlusAPIs = append(nginxConfigContext.PlusAPIs, plusAPIs...) } + nginxConfigContext.PlusAPIs = ncp.sortPlusAPIs(nginxConfigContext.PlusAPIs) + if len(napSyslogServersFound) > 0 { var napSyslogServer []string for server := range napSyslogServersFound { @@ -693,6 +685,7 @@ func validateAPIResponse(apiType string, bodyBytes []byte) error { return nil } +//nolint:revive //need to reduce cognitive complexity func (ncp *NginxConfigParser) apiDetailsFromLocationDirective( ctx context.Context, parent, current *crossplane.Directive, locationDirectiveName string, @@ -710,29 +703,40 @@ func (ncp *NginxConfigParser) apiDetailsFromLocationDirective( return nil } - addresses := ncp.parseAddressFromServerDirective(parent) - path := ncp.parsePathFromLocationDirective(current) - for _, locChild := range current.Block { if locChild.Directive != plusAPIDirective && locChild.Directive != stubStatusAPIDirective { continue } - if locChild.Directive == locationDirectiveName { - isWriteEnabled := ncp.isPlusAPIWriteEnabled(ctx, locChild, current.Args[0]) - - params := apiCreationParams{ - locationDirectiveName: locationDirectiveName, - path: path, - caCertLocation: caCertLocation, - isSSL: isSSL, - isWriteEnabled: isWriteEnabled, + addresses := ncp.parseAddressFromServerDirective(parent) + path := ncp.parsePathFromLocationDirective(current) + + writeEnabled := false + if locChild.Directive == plusAPIDirective { + for _, arg := range locChild.Args { + if strings.EqualFold(arg, "write=on") { + writeEnabled = true + break + } } + } - details = append(details, ncp.createAPIDetailsForAddresses( - params, - addresses, - )...) + 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(params), + ) + } } } @@ -740,9 +744,9 @@ func (ncp *NginxConfigParser) apiDetailsFromLocationDirective( } func (ncp *NginxConfigParser) createAPIDetails( - params apiCreationParams, address string, + params createAPIDetailsParams, ) (details *model.APIDetails) { - if strings.HasPrefix(address, "unix:") { + if strings.HasPrefix(params.address, "unix:") { format := unixStubStatusFormat if params.locationDirectiveName == plusAPIDirective { @@ -751,19 +755,19 @@ func (ncp *NginxConfigParser) createAPIDetails( details = &model.APIDetails{ URL: fmt.Sprintf(format, params.path), - Listen: address, + Listen: params.address, Location: params.path, Ca: params.caCertLocation, - WriteEnabled: params.isWriteEnabled, + WriteEnabled: params.writeEnabled, } } else { details = &model.APIDetails{ URL: fmt.Sprintf("%s://%s%s", map[bool]string{true: "https", false: "http"}[params.isSSL], - address, params.path), - Listen: address, + params.address, params.path), + Listen: params.address, Location: params.path, Ca: params.caCertLocation, - WriteEnabled: params.isWriteEnabled, + WriteEnabled: params.writeEnabled, } } @@ -918,37 +922,18 @@ func (ncp *NginxConfigParser) isDuplicateFile(nginxConfigContextFiles []*mpi.Fil return false } -func (ncp *NginxConfigParser) createAPIDetailsForAddresses( - params apiCreationParams, - addresses []string, -) (details []*model.APIDetails) { - for _, address := range addresses { - details = append(details, - ncp.createAPIDetails( - params, - address, - ), - ) - } - - return details -} - -func (ncp *NginxConfigParser) isPlusAPIWriteEnabled(ctx context.Context, - directive *crossplane.Directive, - locationPath string, -) bool { - // Only check plus_api directives - if directive.Directive != plusAPIDirective { - return false - } +func (ncp *NginxConfigParser) sortPlusAPIs(apis []*model.APIDetails) []*model.APIDetails { + slices.SortFunc(apis, func(a, b *model.APIDetails) int { + if a.WriteEnabled && !b.WriteEnabled { + return -1 + } - for _, arg := range directive.Args { - if arg == "write=on" { - slog.DebugContext(ctx, "Found NGINX Plus API with write=on", "location", locationPath) - return true + if b.WriteEnabled && !a.WriteEnabled { + return 1 } - } - return false + return 0 + }) + + return apis } diff --git a/internal/datasource/proto/instance.go b/internal/datasource/proto/instance.go index 3a79ee62e..61f54a68b 100644 --- a/internal/datasource/proto/instance.go +++ b/internal/datasource/proto/instance.go @@ -15,23 +15,13 @@ import ( func NginxPlusRuntimeInfoEqual(nginxPlusRuntimeInfo *mpi.NGINXPlusRuntimeInfo, nginxConfigContext *model.NginxConfigContext, accessLogs, errorLogs []string, ) bool { - convertedPlusAPIs := convertToMpiAPIDetailsSlice(nginxConfigContext.PlusAPIs) - - if !reflect.DeepEqual(nginxPlusRuntimeInfo.GetPlusApis(), convertedPlusAPIs) { - return false - } - - if nginxPlusRuntimeInfo.GetPlusApi().GetWriteEnabled() != nginxConfigContext.PlusAPI.WriteEnabled || - nginxPlusRuntimeInfo.GetPlusApi().GetCa() != nginxConfigContext.PlusAPI.Ca { - return false - } - if !reflect.DeepEqual(nginxPlusRuntimeInfo.GetAccessLogs(), accessLogs) || !reflect.DeepEqual(nginxPlusRuntimeInfo.GetErrorLogs(), errorLogs) || nginxPlusRuntimeInfo.GetStubStatus().GetListen() != nginxConfigContext.StubStatus.Listen || nginxPlusRuntimeInfo.GetPlusApi().GetListen() != nginxConfigContext.PlusAPI.Listen || nginxPlusRuntimeInfo.GetStubStatus().GetLocation() != nginxConfigContext.StubStatus.Location || - nginxPlusRuntimeInfo.GetPlusApi().GetLocation() != nginxConfigContext.PlusAPI.Location { + nginxPlusRuntimeInfo.GetPlusApi().GetLocation() != nginxConfigContext.PlusAPI.Location || + nginxPlusRuntimeInfo.GetPlusApi().GetWriteEnabled() != nginxConfigContext.PlusAPI.WriteEnabled { return false } @@ -70,11 +60,7 @@ func UpdateNginxInstanceRuntime( nginxPlusRuntimeInfo.PlusApi.Listen = nginxConfigContext.PlusAPI.Listen nginxPlusRuntimeInfo.StubStatus.Location = nginxConfigContext.StubStatus.Location nginxPlusRuntimeInfo.PlusApi.Location = nginxConfigContext.PlusAPI.Location - nginxPlusRuntimeInfo.PlusApi.WriteEnabled = nginxConfigContext.PlusAPI.WriteEnabled - nginxPlusRuntimeInfo.PlusApi.Ca = nginxConfigContext.PlusAPI.Ca - - nginxPlusRuntimeInfo.PlusApis = convertToMpiAPIDetailsSlice(nginxConfigContext.PlusAPIs) updatesRequired = true } } else { @@ -91,28 +77,3 @@ func UpdateNginxInstanceRuntime( return updatesRequired } - -func convertToMpiAPIDetails(modelAPI *model.APIDetails) *mpi.APIDetails { - if modelAPI == nil { - return nil - } - - return &mpi.APIDetails{ - Listen: modelAPI.Listen, - Location: modelAPI.Location, - Ca: modelAPI.Ca, - WriteEnabled: modelAPI.WriteEnabled, - } -} - -func convertToMpiAPIDetailsSlice(modelAPIs []*model.APIDetails) []*mpi.APIDetails { - if modelAPIs == nil { - return nil - } - mpiAPIs := make([]*mpi.APIDetails, 0, len(modelAPIs)) - for _, api := range modelAPIs { - mpiAPIs = append(mpiAPIs, convertToMpiAPIDetails(api)) - } - - return mpiAPIs -} diff --git a/internal/datasource/proto/instance_test.go b/internal/datasource/proto/instance_test.go index 71d10cd5c..90e1cf303 100644 --- a/internal/datasource/proto/instance_test.go +++ b/internal/datasource/proto/instance_test.go @@ -14,54 +14,6 @@ import ( "github.com/stretchr/testify/assert" ) -var plusAPIs = []*model.APIDetails{ - { - URL: "http://127.0.0.1:8081/api", - Listen: "127.0.0.1:8081", - Location: "/api", - WriteEnabled: false, - Ca: "", - }, - { - URL: "unix:/var/run/nginx/api.sock", - Listen: "unix:/var/run/nginx/api.sock", - Location: "/api", - WriteEnabled: true, // Crucial for selection logic - Ca: "/etc/certs/my_ca.pem", - }, -} - -var nginxPlusConfigContextForUpdate = &model.NginxConfigContext{ - AccessLogs: []*model.AccessLog{ - { - Name: "/usr/local/var/log/nginx/access.log", - }, - }, - ErrorLogs: []*model.ErrorLog{ - { - Name: "/usr/local/var/log/nginx/error.log", - }, - }, - PlusAPI: plusAPIs[1], - StubStatus: &model.APIDetails{ - URL: "http://127.0.0.1:8081/status", - Listen: "127.0.0.1:8081", - }, - PlusAPIs: plusAPIs, -} - -func convertAPIDetailsSliceForTest(modelAPIs []*model.APIDetails) []*mpi.APIDetails { - if modelAPIs == nil { - return nil - } - mpiAPIs := make([]*mpi.APIDetails, 0, len(modelAPIs)) - for _, api := range modelAPIs { - mpiAPIs = append(mpiAPIs, convertToMpiAPIDetails(api)) - } - - return mpiAPIs -} - func TestInstanceWatcherService_updateNginxInstanceRuntime(t *testing.T) { nginxOSSConfigContext := &model.NginxConfigContext{ AccessLogs: []*model.AccessLog{ @@ -92,9 +44,32 @@ func TestInstanceWatcherService_updateNginxInstanceRuntime(t *testing.T) { }, }, PlusAPI: &model.APIDetails{ + URL: "http://127.0.0.1:8081/api", + Listen: "", + WriteEnabled: false, + }, + StubStatus: &model.APIDetails{ URL: "http://127.0.0.1:8081/api", Listen: "", }, + } + + nginxPlusConfigContextWrite := &model.NginxConfigContext{ + AccessLogs: []*model.AccessLog{ + { + Name: "/usr/local/var/log/nginx/access.log", + }, + }, + ErrorLogs: []*model.ErrorLog{ + { + Name: "/usr/local/var/log/nginx/error.log", + }, + }, + PlusAPI: &model.APIDetails{ + URL: "http://127.0.0.1:8081/api/write", + Listen: "8080", + WriteEnabled: true, + }, StubStatus: &model.APIDetails{ URL: "http://127.0.0.1:8081/api", Listen: "", @@ -117,38 +92,25 @@ func TestInstanceWatcherService_updateNginxInstanceRuntime(t *testing.T) { instance: protos.NginxPlusInstance([]string{}), }, { - name: "Test 3: Plus Instance - PlusAPIs Update", - nginxConfigContext: nginxPlusConfigContextForUpdate, + name: "Test 3: Plus Instance (Write-Enabled)", + nginxConfigContext: nginxPlusConfigContextWrite, instance: protos.NginxPlusInstance([]string{}), }, - { - name: "Test 4: Plus Instance - No Update Required", - nginxConfigContext: nginxPlusConfigContextForUpdate, - instance: createPopulatedNginxPlusInstance(nginxPlusConfigContextForUpdate), - }, } for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - updatesRequired := UpdateNginxInstanceRuntime(test.instance, test.nginxConfigContext) - switch test.name { - case "Test 3: Plus Instance - PlusAPIs Update": - assert.True(t, updatesRequired, - "UpdateNginxInstanceRuntime should return true when PlusAPIs are updated") - expectedAPIs := convertAPIDetailsSliceForTest(test.nginxConfigContext.PlusAPIs) - assert.ElementsMatch(t, expectedAPIs, - test.instance.GetInstanceRuntime().GetNginxPlusRuntimeInfo().GetPlusApis()) - assert.Equal(t, test.nginxConfigContext.PlusAPI.WriteEnabled, test.instance.GetInstanceRuntime(). - GetNginxPlusRuntimeInfo().GetPlusApi().GetWriteEnabled()) - assert.Equal(t, test.nginxConfigContext.PlusAPI.Ca, test.instance.GetInstanceRuntime(). - GetNginxPlusRuntimeInfo().GetPlusApi().GetCa()) - assert.Equal(t, test.nginxConfigContext.PlusAPI.Listen, test.instance.GetInstanceRuntime(). - GetNginxPlusRuntimeInfo().GetPlusApi().GetListen()) - case "Test 4: Plus Instance - No Update Required": - assert.False(t, updatesRequired, - "UpdateNginxInstanceRuntime should return false when runtime already matches config") - - case "Test 2: Plus Instance": + t.Run(test.name, func(tt *testing.T) { + UpdateNginxInstanceRuntime(test.instance, test.nginxConfigContext) + if test.name == "Test 1: OSS Instance" { + assert.Equal(t, test.nginxConfigContext.AccessLogs[0].Name, test.instance.GetInstanceRuntime(). + GetNginxRuntimeInfo().GetAccessLogs()[0]) + assert.Equal(t, test.nginxConfigContext.ErrorLogs[0].Name, test.instance.GetInstanceRuntime(). + GetNginxRuntimeInfo().GetErrorLogs()[0]) + assert.Equal(t, test.nginxConfigContext.StubStatus.Location, test.instance.GetInstanceRuntime(). + GetNginxRuntimeInfo().GetStubStatus().GetLocation()) + assert.Equal(t, test.nginxConfigContext.StubStatus.Listen, test.instance.GetInstanceRuntime(). + GetNginxRuntimeInfo().GetStubStatus().GetListen()) + } else { assert.Equal(t, test.nginxConfigContext.AccessLogs[0].Name, test.instance.GetInstanceRuntime(). GetNginxPlusRuntimeInfo().GetAccessLogs()[0]) assert.Equal(t, test.nginxConfigContext.ErrorLogs[0].Name, test.instance.GetInstanceRuntime(). @@ -163,39 +125,7 @@ func TestInstanceWatcherService_updateNginxInstanceRuntime(t *testing.T) { GetNginxPlusRuntimeInfo().GetPlusApi().GetListen()) assert.Equal(t, test.nginxConfigContext.PlusAPI.WriteEnabled, test.instance.GetInstanceRuntime(). GetNginxPlusRuntimeInfo().GetPlusApi().GetWriteEnabled()) - assert.Equal(t, test.nginxConfigContext.PlusAPI.Ca, test.instance.GetInstanceRuntime(). - GetNginxPlusRuntimeInfo().GetPlusApi().GetCa()) - - default: - assert.Equal(t, test.nginxConfigContext.AccessLogs[0].Name, test.instance.GetInstanceRuntime(). - GetNginxRuntimeInfo().GetAccessLogs()[0]) - assert.Equal(t, test.nginxConfigContext.ErrorLogs[0].Name, test.instance.GetInstanceRuntime(). - GetNginxRuntimeInfo().GetErrorLogs()[0]) - assert.Equal(t, test.nginxConfigContext.StubStatus.Location, test.instance.GetInstanceRuntime(). - GetNginxRuntimeInfo().GetStubStatus().GetLocation()) - assert.Equal(t, test.nginxConfigContext.StubStatus.Listen, test.instance.GetInstanceRuntime(). - GetNginxRuntimeInfo().GetStubStatus().GetListen()) } }) } } - -func createPopulatedNginxPlusInstance(configContext *model.NginxConfigContext) *mpi.Instance { - instance := protos.NginxPlusInstance([]string{}) - runtimeInfo := instance.GetInstanceRuntime().GetNginxPlusRuntimeInfo() - runtimeInfo.PlusApi.Listen = configContext.PlusAPI.Listen - - runtimeInfo.PlusApi.Location = configContext.PlusAPI.Location - runtimeInfo.PlusApi.WriteEnabled = configContext.PlusAPI.WriteEnabled - runtimeInfo.PlusApi.Ca = configContext.PlusAPI.Ca - - runtimeInfo.PlusApis = convertAPIDetailsSliceForTest(configContext.PlusAPIs) - - runtimeInfo.AccessLogs = model.ConvertAccessLogs(configContext.AccessLogs) - runtimeInfo.ErrorLogs = model.ConvertErrorLogs(configContext.ErrorLogs) - - runtimeInfo.StubStatus.Listen = configContext.StubStatus.Listen - runtimeInfo.StubStatus.Location = configContext.StubStatus.Location - - return instance -} diff --git a/internal/model/config.go b/internal/model/config.go index e983d8708..5455f61d1 100644 --- a/internal/model/config.go +++ b/internal/model/config.go @@ -96,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 } } diff --git a/internal/resource/resource_service.go b/internal/resource/resource_service.go index 5140aabb6..1357bda17 100644 --- a/internal/resource/resource_service.go +++ b/internal/resource/resource_service.go @@ -361,42 +361,21 @@ func convertToStreamUpstreamServer(streamUpstreams []*structpb.Struct) []client. } func (r *ResourceService) createPlusClient(ctx context.Context, instance *mpi.Instance) (*client.NginxClient, error) { - plusAPIs := instance.GetInstanceRuntime().GetNginxPlusRuntimeInfo().GetPlusApis() + plusAPI := instance.GetInstanceRuntime().GetNginxPlusRuntimeInfo().GetPlusApi() var endpoint string - var selectedAPI *mpi.APIDetails - if len(plusAPIs) == 0 { + if plusAPI.GetLocation() == "" || plusAPI.GetListen() == "" { return nil, errors.New("failed to preform API action, NGINX Plus API is not configured") } - for _, api := range plusAPIs { - if api.GetWriteEnabled() { - selectedAPI = api - slog.DebugContext(ctx, "Selected write-enabled NGINX Plus API for action", - "url", selectedAPI.GetLocation(), "listen", selectedAPI.GetListen()) - - break - } - } - - if selectedAPI == nil { - selectedAPI = plusAPIs[0] - slog.InfoContext(ctx, "No write-enabled NGINX Plus API found. Write operations may fail.", - "url", selectedAPI.GetLocation(), "listen", selectedAPI.GetListen()) - } - - if selectedAPI.GetLocation() == "" || selectedAPI.GetListen() == "" { - return nil, errors.New("failed to preform API action, NGINX Plus API is not configured") - } - - if strings.HasPrefix(selectedAPI.GetListen(), "unix:") { - endpoint = fmt.Sprintf(unixPlusAPIFormat, selectedAPI.GetLocation()) + if strings.HasPrefix(plusAPI.GetListen(), "unix:") { + endpoint = fmt.Sprintf(unixPlusAPIFormat, plusAPI.GetLocation()) } else { - endpoint = fmt.Sprintf(apiFormat, selectedAPI.GetListen(), selectedAPI.GetLocation()) + endpoint = fmt.Sprintf(apiFormat, plusAPI.GetListen(), plusAPI.GetLocation()) } httpClient := http.DefaultClient - caCertLocation := selectedAPI.GetCa() + caCertLocation := plusAPI.GetCa() if caCertLocation != "" { slog.DebugContext(ctx, "Reading CA certificate", "file_path", caCertLocation) caCert, err := os.ReadFile(caCertLocation) @@ -415,8 +394,8 @@ func (r *ResourceService) createPlusClient(ctx context.Context, instance *mpi.In }, } } - if strings.HasPrefix(selectedAPI.GetListen(), "unix:") { - httpClient = socketClient(ctx, strings.TrimPrefix(selectedAPI.GetListen(), "unix:")) + if strings.HasPrefix(plusAPI.GetListen(), "unix:") { + httpClient = socketClient(ctx, strings.TrimPrefix(plusAPI.GetListen(), "unix:")) } return client.NewNginxClient(endpoint, diff --git a/internal/resource/resource_service_test.go b/internal/resource/resource_service_test.go index f1301909a..8a8cd45a9 100644 --- a/internal/resource/resource_service_test.go +++ b/internal/resource/resource_service_test.go @@ -6,11 +6,9 @@ package resource import ( - "bytes" "context" "errors" "fmt" - "log/slog" "os" "path/filepath" "testing" @@ -248,43 +246,30 @@ func TestResourceService_createPlusClient(t *testing.T) { err := os.WriteFile(caFile, []byte("-----BEGIN CERTIFICATE-----\nMII...\n-----END CERTIFICATE-----"), 0o600) require.NoError(t, err) - createPlusInstanceWithApis := func(details []*v1.APIDetails) *v1.Instance { - inst := protos.NginxPlusInstance([]string{}) - if inst.GetInstanceRuntime().GetNginxPlusRuntimeInfo() == nil { - inst.InstanceRuntime.Details = &v1.InstanceRuntime_NginxPlusRuntimeInfo{ - NginxPlusRuntimeInfo: &v1.NGINXPlusRuntimeInfo{}, - } - } - inst.InstanceRuntime.GetNginxPlusRuntimeInfo().PlusApis = details - - return inst + instanceWithAPI := protos.NginxPlusInstance([]string{}) + instanceWithAPI.InstanceRuntime.GetNginxPlusRuntimeInfo().PlusApi = &v1.APIDetails{ + Location: "/api", + Listen: "localhost:80", } - instanceWithAPI := createPlusInstanceWithApis([]*v1.APIDetails{ - {Location: "/api", Listen: "localhost:80"}, - }) - - instanceWithUnixAPI := createPlusInstanceWithApis([]*v1.APIDetails{ - {Listen: "unix:/var/run/nginx-status.sock", Location: "/api"}, - }) - - instanceWithCACert := createPlusInstanceWithApis([]*v1.APIDetails{ - {Location: "/api", Listen: "localhost:443", Ca: caFile}, - }) - - var logBuffer bytes.Buffer + instanceWithUnixAPI := protos.NginxPlusInstance([]string{}) + instanceWithUnixAPI.InstanceRuntime.GetNginxPlusRuntimeInfo().PlusApi = &v1.APIDetails{ + Listen: "unix:/var/run/nginx-status.sock", + Location: "/api", + } - tempHandler := slog.NewTextHandler(&logBuffer, &slog.HandlerOptions{ - Level: slog.LevelDebug, - }) - tempLogger := slog.New(tempHandler) + instanceWithCACert := protos.NginxPlusInstance([]string{}) + instanceWithCACert.InstanceRuntime.GetNginxPlusRuntimeInfo().PlusApi = &v1.APIDetails{ + Location: "/api", + Listen: "localhost:443", + Ca: caFile, + } ctx := context.Background() tests := []struct { - err error - instance *v1.Instance - name string - expectedLog string + err error + instance *v1.Instance + name string }{ { name: "Test 1: Create Plus Client", @@ -306,32 +291,10 @@ func TestResourceService_createPlusClient(t *testing.T) { instance: protos.NginxPlusInstance([]string{}), err: errors.New("failed to preform API action, NGINX Plus API is not configured"), }, - { - name: "Test 5: Fallback to First API (No Write-Enabled)", - instance: createPlusInstanceWithApis([]*v1.APIDetails{ - {Location: "/read1", Listen: "localhost:80"}, - {Location: "/read2", Listen: "localhost:8080"}, - }), - err: nil, - expectedLog: "No write-enabled NGINX Plus API found. Write operations may fail.", - }, - { - name: "Test 6: Prioritize Write-Enabled API", - instance: createPlusInstanceWithApis([]*v1.APIDetails{ - {Location: "/read", Listen: "localhost:80"}, - {Location: "/write", Listen: "localhost:8080", WriteEnabled: true}, - }), - err: nil, - expectedLog: "Selected write-enabled NGINX Plus API for action", - }, } for _, test := range tests { t.Run(test.name, func(tt *testing.T) { - logBuffer.Reset() - originalLogger := slog.Default() - slog.SetDefault(tempLogger) - defer slog.SetDefault(originalLogger) resourceService := NewResourceService(ctx, types.AgentConfig()) resourceService.resource.Instances = []*v1.Instance{ protos.NginxOssInstance([]string{}), @@ -347,10 +310,6 @@ func TestResourceService_createPlusClient(t *testing.T) { // For the CA cert test, we can't easily verify the internal http.Client configuration // without exporting it or adding test hooks, so we'll just verify no error is returned } - if test.expectedLog != "" { - logOutput := logBuffer.String() - assert.Contains(tt, logOutput, test.expectedLog) - } }) } } From a5e6be4ffc2ff1283087a90e90cfd3fa6b6f7f28 Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Tue, 7 Oct 2025 10:29:05 +0100 Subject: [PATCH 09/15] reduced Lint cognitive complexity --- .../datasource/config/nginx_config_parser.go | 25 +++++++++++-------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/internal/datasource/config/nginx_config_parser.go b/internal/datasource/config/nginx_config_parser.go index 1fdef8268..7653d4174 100644 --- a/internal/datasource/config/nginx_config_parser.go +++ b/internal/datasource/config/nginx_config_parser.go @@ -685,7 +685,6 @@ func validateAPIResponse(apiType string, bodyBytes []byte) error { return nil } -//nolint:revive //need to reduce cognitive complexity func (ncp *NginxConfigParser) apiDetailsFromLocationDirective( ctx context.Context, parent, current *crossplane.Directive, locationDirectiveName string, @@ -711,15 +710,7 @@ func (ncp *NginxConfigParser) apiDetailsFromLocationDirective( addresses := ncp.parseAddressFromServerDirective(parent) path := ncp.parsePathFromLocationDirective(current) - writeEnabled := false - if locChild.Directive == plusAPIDirective { - for _, arg := range locChild.Args { - if strings.EqualFold(arg, "write=on") { - writeEnabled = true - break - } - } - } + writeEnabled := ncp.isWriteEnabled(locChild) if locChild.Directive == locationDirectiveName { for _, address := range addresses { @@ -922,6 +913,20 @@ 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 +} + func (ncp *NginxConfigParser) sortPlusAPIs(apis []*model.APIDetails) []*model.APIDetails { slices.SortFunc(apis, func(a, b *model.APIDetails) int { if a.WriteEnabled && !b.WriteEnabled { From c5bbfecf366142f382c2785ef55a6ff883209a43 Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Wed, 8 Oct 2025 12:22:14 +0100 Subject: [PATCH 10/15] worked on PR comments --- .../datasource/config/nginx_config_parser.go | 18 +++- .../config/nginx_config_parser_test.go | 93 ++++++++++++++++++- 2 files changed, 104 insertions(+), 7 deletions(-) diff --git a/internal/datasource/config/nginx_config_parser.go b/internal/datasource/config/nginx_config_parser.go index 7653d4174..a88088c1e 100644 --- a/internal/datasource/config/nginx_config_parser.go +++ b/internal/datasource/config/nginx_config_parser.go @@ -260,7 +260,7 @@ func (ncp *NginxConfigParser) createNginxConfigContext( nginxConfigContext.PlusAPIs = append(nginxConfigContext.PlusAPIs, plusAPIs...) } - nginxConfigContext.PlusAPIs = ncp.sortPlusAPIs(nginxConfigContext.PlusAPIs) + nginxConfigContext.PlusAPIs = ncp.sortPlusAPIs(ctx, nginxConfigContext.PlusAPIs) if len(napSyslogServersFound) > 0 { var napSyslogServer []string @@ -927,7 +927,21 @@ func (ncp *NginxConfigParser) isWriteEnabled(locChild *crossplane.Directive) boo return false } -func (ncp *NginxConfigParser) sortPlusAPIs(apis []*model.APIDetails) []*model.APIDetails { +// sort the API endpoints by prioritising 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.WarnContext(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 diff --git a/internal/datasource/config/nginx_config_parser_test.go b/internal/datasource/config/nginx_config_parser_test.go index a9aa617ac..289def7e2 100644 --- a/internal/datasource/config/nginx_config_parser_test.go +++ b/internal/datasource/config/nginx_config_parser_test.go @@ -293,6 +293,54 @@ server { 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; + } }` ) @@ -813,13 +861,15 @@ 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{ @@ -1048,11 +1098,34 @@ func TestNginxConfigParser_urlsForLocationDirective(t *testing.T) { 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{ @@ -1074,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() }) } } @@ -1085,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) @@ -1096,7 +1174,7 @@ func traverseConfigForAPIs( } _plus := ncp.apiDetailsFromLocationDirective(ctx, parent, directive, plusAPIDirective) if _plus != nil { - plus = _plus + allPlusAPIs = append(allPlusAPIs, _plus...) } return nil @@ -1104,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 } From a740b6912dfd7d50effa8435874abdddb3b6f790 Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Wed, 8 Oct 2025 13:06:37 +0100 Subject: [PATCH 11/15] fixed lint error --- internal/datasource/config/nginx_config_parser.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/datasource/config/nginx_config_parser.go b/internal/datasource/config/nginx_config_parser.go index a88088c1e..1f17335eb 100644 --- a/internal/datasource/config/nginx_config_parser.go +++ b/internal/datasource/config/nginx_config_parser.go @@ -927,7 +927,7 @@ func (ncp *NginxConfigParser) isWriteEnabled(locChild *crossplane.Directive) boo return false } -// sort the API endpoints by prioritising any API that has 'write=on'. +// 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 { From a38d37fe1813affb27680b9a0efe2b624dda9c73 Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Tue, 14 Oct 2025 14:35:29 +0100 Subject: [PATCH 12/15] Modified log level to info --- internal/datasource/config/nginx_config_parser.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/datasource/config/nginx_config_parser.go b/internal/datasource/config/nginx_config_parser.go index 1f17335eb..271ca4123 100644 --- a/internal/datasource/config/nginx_config_parser.go +++ b/internal/datasource/config/nginx_config_parser.go @@ -938,7 +938,7 @@ func (ncp *NginxConfigParser) sortPlusAPIs(ctx context.Context, apis []*model.AP } if !foundWriteEnabled && len(apis) > 0 { - slog.WarnContext(ctx, "No write-enabled NGINX Plus API found. Defaulting to read-only API") + slog.InfoContext(ctx, "No write-enabled NGINX Plus API found. Defaulting to read-only API") return apis } From 46aaa894c7d552444121268b59b72521d3ee1aa4 Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Wed, 15 Oct 2025 12:09:04 +0100 Subject: [PATCH 13/15] removing the write-enable from MPI --- api/grpc/mpi/v1/command.pb.go | 16 ++----- api/grpc/mpi/v1/command.pb.validate.go | 2 - api/grpc/mpi/v1/command.proto | 2 - docs/proto/protos.md | 1 - internal/datasource/proto/instance.go | 4 +- internal/datasource/proto/instance_test.go | 50 +++++----------------- 6 files changed, 14 insertions(+), 61 deletions(-) diff --git a/api/grpc/mpi/v1/command.pb.go b/api/grpc/mpi/v1/command.pb.go index fbceea13b..31e94ea7d 100644 --- a/api/grpc/mpi/v1/command.pb.go +++ b/api/grpc/mpi/v1/command.pb.go @@ -2264,9 +2264,7 @@ type APIDetails struct { // the API listen directive Listen string `protobuf:"bytes,2,opt,name=listen,proto3" json:"listen,omitempty"` // the API CA file path - Ca string `protobuf:"bytes,3,opt,name=Ca,proto3" json:"Ca,omitempty"` - // flag to know API is configured with 'write=on;' - WriteEnabled bool `protobuf:"varint,4,opt,name=write_enabled,json=writeEnabled,proto3" json:"write_enabled,omitempty"` + Ca string `protobuf:"bytes,3,opt,name=Ca,proto3" json:"Ca,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache } @@ -2322,13 +2320,6 @@ func (x *APIDetails) GetCa() string { return "" } -func (x *APIDetails) GetWriteEnabled() bool { - if x != nil { - return x.WriteEnabled - } - return false -} - // A set of runtime NGINX App Protect settings type NGINXAppProtectRuntimeInfo struct { state protoimpl.MessageState `protogen:"open.v1"` @@ -2890,13 +2881,12 @@ const file_mpi_v1_command_proto_rawDesc = "" + "error_logs\x18\x03 \x03(\tR\terrorLogs\x12)\n" + "\x10loadable_modules\x18\x04 \x03(\tR\x0floadableModules\x12'\n" + "\x0fdynamic_modules\x18\x05 \x03(\tR\x0edynamicModules\x12-\n" + - "\bplus_api\x18\x06 \x01(\v2\x12.mpi.v1.APIDetailsR\aplusApi\"u\n" + + "\bplus_api\x18\x06 \x01(\v2\x12.mpi.v1.APIDetailsR\aplusApi\"P\n" + "\n" + "APIDetails\x12\x1a\n" + "\blocation\x18\x01 \x01(\tR\blocation\x12\x16\n" + "\x06listen\x18\x02 \x01(\tR\x06listen\x12\x0e\n" + - "\x02Ca\x18\x03 \x01(\tR\x02Ca\x12#\n" + - "\rwrite_enabled\x18\x04 \x01(\bR\fwriteEnabled\"\xe0\x01\n" + + "\x02Ca\x18\x03 \x01(\tR\x02Ca\"\xe0\x01\n" + "\x1aNGINXAppProtectRuntimeInfo\x12\x18\n" + "\arelease\x18\x01 \x01(\tR\arelease\x128\n" + "\x18attack_signature_version\x18\x02 \x01(\tR\x16attackSignatureVersion\x126\n" + diff --git a/api/grpc/mpi/v1/command.pb.validate.go b/api/grpc/mpi/v1/command.pb.validate.go index d35936df3..81f716548 100644 --- a/api/grpc/mpi/v1/command.pb.validate.go +++ b/api/grpc/mpi/v1/command.pb.validate.go @@ -4895,8 +4895,6 @@ func (m *APIDetails) validate(all bool) error { // no validation rules for Ca - // no validation rules for WriteEnabled - if len(errors) > 0 { return APIDetailsMultiError(errors) } diff --git a/api/grpc/mpi/v1/command.proto b/api/grpc/mpi/v1/command.proto index 894e64acd..84fb6a020 100644 --- a/api/grpc/mpi/v1/command.proto +++ b/api/grpc/mpi/v1/command.proto @@ -354,8 +354,6 @@ message APIDetails { string listen = 2; // the API CA file path string Ca = 3; - // flag to know API is configured with 'write=on;' - bool write_enabled = 4; } // A set of runtime NGINX App Protect settings diff --git a/docs/proto/protos.md b/docs/proto/protos.md index 529629ff3..3ae57f5e4 100644 --- a/docs/proto/protos.md +++ b/docs/proto/protos.md @@ -697,7 +697,6 @@ 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 file path | -| write_enabled | [bool](#bool) | | flag to know API is configured with 'write=on;' | diff --git a/internal/datasource/proto/instance.go b/internal/datasource/proto/instance.go index 61f54a68b..38c802030 100644 --- a/internal/datasource/proto/instance.go +++ b/internal/datasource/proto/instance.go @@ -20,8 +20,7 @@ func NginxPlusRuntimeInfoEqual(nginxPlusRuntimeInfo *mpi.NGINXPlusRuntimeInfo, nginxPlusRuntimeInfo.GetStubStatus().GetListen() != nginxConfigContext.StubStatus.Listen || nginxPlusRuntimeInfo.GetPlusApi().GetListen() != nginxConfigContext.PlusAPI.Listen || nginxPlusRuntimeInfo.GetStubStatus().GetLocation() != nginxConfigContext.StubStatus.Location || - nginxPlusRuntimeInfo.GetPlusApi().GetLocation() != nginxConfigContext.PlusAPI.Location || - nginxPlusRuntimeInfo.GetPlusApi().GetWriteEnabled() != nginxConfigContext.PlusAPI.WriteEnabled { + nginxPlusRuntimeInfo.GetPlusApi().GetLocation() != nginxConfigContext.PlusAPI.Location { return false } @@ -60,7 +59,6 @@ func UpdateNginxInstanceRuntime( nginxPlusRuntimeInfo.PlusApi.Listen = nginxConfigContext.PlusAPI.Listen nginxPlusRuntimeInfo.StubStatus.Location = nginxConfigContext.StubStatus.Location nginxPlusRuntimeInfo.PlusApi.Location = nginxConfigContext.PlusAPI.Location - nginxPlusRuntimeInfo.PlusApi.WriteEnabled = nginxConfigContext.PlusAPI.WriteEnabled updatesRequired = true } } else { diff --git a/internal/datasource/proto/instance_test.go b/internal/datasource/proto/instance_test.go index 90e1cf303..0ad5c25a8 100644 --- a/internal/datasource/proto/instance_test.go +++ b/internal/datasource/proto/instance_test.go @@ -44,32 +44,9 @@ func TestInstanceWatcherService_updateNginxInstanceRuntime(t *testing.T) { }, }, PlusAPI: &model.APIDetails{ - URL: "http://127.0.0.1:8081/api", - Listen: "", - WriteEnabled: false, - }, - StubStatus: &model.APIDetails{ URL: "http://127.0.0.1:8081/api", Listen: "", }, - } - - nginxPlusConfigContextWrite := &model.NginxConfigContext{ - AccessLogs: []*model.AccessLog{ - { - Name: "/usr/local/var/log/nginx/access.log", - }, - }, - ErrorLogs: []*model.ErrorLog{ - { - Name: "/usr/local/var/log/nginx/error.log", - }, - }, - PlusAPI: &model.APIDetails{ - URL: "http://127.0.0.1:8081/api/write", - Listen: "8080", - WriteEnabled: true, - }, StubStatus: &model.APIDetails{ URL: "http://127.0.0.1:8081/api", Listen: "", @@ -91,26 +68,12 @@ func TestInstanceWatcherService_updateNginxInstanceRuntime(t *testing.T) { nginxConfigContext: nginxPlusConfigContext, instance: protos.NginxPlusInstance([]string{}), }, - { - name: "Test 3: Plus Instance (Write-Enabled)", - nginxConfigContext: nginxPlusConfigContextWrite, - instance: protos.NginxPlusInstance([]string{}), - }, } for _, test := range tests { t.Run(test.name, func(tt *testing.T) { UpdateNginxInstanceRuntime(test.instance, test.nginxConfigContext) - if test.name == "Test 1: OSS Instance" { - assert.Equal(t, test.nginxConfigContext.AccessLogs[0].Name, test.instance.GetInstanceRuntime(). - GetNginxRuntimeInfo().GetAccessLogs()[0]) - assert.Equal(t, test.nginxConfigContext.ErrorLogs[0].Name, test.instance.GetInstanceRuntime(). - GetNginxRuntimeInfo().GetErrorLogs()[0]) - assert.Equal(t, test.nginxConfigContext.StubStatus.Location, test.instance.GetInstanceRuntime(). - GetNginxRuntimeInfo().GetStubStatus().GetLocation()) - assert.Equal(t, test.nginxConfigContext.StubStatus.Listen, test.instance.GetInstanceRuntime(). - GetNginxRuntimeInfo().GetStubStatus().GetListen()) - } else { + if test.name == "Test 2: Plus Instance" { assert.Equal(t, test.nginxConfigContext.AccessLogs[0].Name, test.instance.GetInstanceRuntime(). GetNginxPlusRuntimeInfo().GetAccessLogs()[0]) assert.Equal(t, test.nginxConfigContext.ErrorLogs[0].Name, test.instance.GetInstanceRuntime(). @@ -123,8 +86,15 @@ func TestInstanceWatcherService_updateNginxInstanceRuntime(t *testing.T) { GetNginxPlusRuntimeInfo().GetStubStatus().GetListen()) assert.Equal(t, test.nginxConfigContext.PlusAPI.Listen, test.instance.GetInstanceRuntime(). GetNginxPlusRuntimeInfo().GetPlusApi().GetListen()) - assert.Equal(t, test.nginxConfigContext.PlusAPI.WriteEnabled, test.instance.GetInstanceRuntime(). - GetNginxPlusRuntimeInfo().GetPlusApi().GetWriteEnabled()) + } else { + assert.Equal(t, test.nginxConfigContext.AccessLogs[0].Name, test.instance.GetInstanceRuntime(). + GetNginxRuntimeInfo().GetAccessLogs()[0]) + assert.Equal(t, test.nginxConfigContext.ErrorLogs[0].Name, test.instance.GetInstanceRuntime(). + GetNginxRuntimeInfo().GetErrorLogs()[0]) + assert.Equal(t, test.nginxConfigContext.StubStatus.Location, test.instance.GetInstanceRuntime(). + GetNginxRuntimeInfo().GetStubStatus().GetLocation()) + assert.Equal(t, test.nginxConfigContext.StubStatus.Listen, test.instance.GetInstanceRuntime(). + GetNginxRuntimeInfo().GetStubStatus().GetListen()) } }) } From af70f5d598129e4bc6afe1fe645d273d7efecd9b Mon Sep 17 00:00:00 2001 From: Aphral Griffin Date: Thu, 16 Oct 2025 15:54:09 +0100 Subject: [PATCH 14/15] fix pipeline --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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: From 32f010c4ec84a5cb44163e52bf400eedef13d8b0 Mon Sep 17 00:00:00 2001 From: Akshay Chawla Date: Wed, 22 Oct 2025 16:12:18 +0100 Subject: [PATCH 15/15] updated traverseAPI to read all APIs --- .../datasource/config/nginx_config_parser.go | 24 ++++++++----------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/internal/datasource/config/nginx_config_parser.go b/internal/datasource/config/nginx_config_parser.go index 271ca4123..0c011aa64 100644 --- a/internal/datasource/config/nginx_config_parser.go +++ b/internal/datasource/config/nginx_config_parser.go @@ -388,7 +388,6 @@ func (ncp *NginxConfigParser) crossplaneConfigTraverseAPIDetails( callback crossplaneTraverseCallbackAPIDetails, apiType string, ) []*model.APIDetails { - stop := false var responses []*model.APIDetails for _, dir := range root.Parsed { @@ -397,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...) } @@ -410,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 {