worked on PR comments

Author: Akshay2191

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

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,14 +1174,19 @@ func traverseConfigForAPIs(
 				}
 				_plus := ncp.apiDetailsFromLocationDirective(ctx, parent, directive, plusAPIDirective)
 				if _plus != nil {
-					plus = _plus
+					allPlusAPIs = append(allPlusAPIs, _plus...)
 				}
 
 				return nil
 			})
 		require.NoError(t, err)
 	}
 
+	if len(allPlusAPIs) > 0 {
+		sortedAPIs := ncp.sortPlusAPIs(ctx, allPlusAPIs)
+		plus = []*model.APIDetails{sortedAPIs[0]}
+	}
+
 	return oss, plus
 }