Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 64 additions & 3 deletions components/operator/internal/trigger/trigger.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,11 @@ import (
"strings"
"time"

"k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/client-go/dynamic"
"k8s.io/client-go/kubernetes"
"k8s.io/client-go/rest"
"k8s.io/client-go/util/retry"

Expand Down Expand Up @@ -244,8 +246,67 @@ func resumeSessionWithPrompt(dynamicClient dynamic.Interface, namespace, session
})
}

// applyFeatureFlagOverrides reads workspace feature flag overrides from the
// feature-flag-overrides ConfigMap and applies them to the session template.
// Currently supports: jira-write flag → JIRA_READ_ONLY_MODE environment variable.
func applyFeatureFlagOverrides(ctx context.Context, k8sClient kubernetes.Interface, namespace string, template map[string]interface{}) error {
// Read feature-flag-overrides ConfigMap
cm, err := k8sClient.CoreV1().ConfigMaps(namespace).Get(ctx, "feature-flag-overrides", metav1.GetOptions{})
if errors.IsNotFound(err) {
// No overrides ConfigMap - this is normal, nothing to do
return nil
}
if err != nil {
// Log warning but don't fail - same pattern as backend
log.Printf("WARNING: failed to read feature flag overrides for namespace %s: %v", namespace, err)
return nil
}

// Check jira-write flag
if val, exists := cm.Data["jira-write"]; exists && val == "true" {
// Get or create environmentVariables map
spec, ok := template["spec"].(map[string]interface{})
if !ok {
spec = map[string]interface{}{}
template["spec"] = spec
}

envVars, ok := spec["environmentVariables"].(map[string]interface{})
if !ok {
envVars = map[string]interface{}{}
}

// Set JIRA_READ_ONLY_MODE to false to enable write operations
envVars["JIRA_READ_ONLY_MODE"] = "false"
spec["environmentVariables"] = envVars

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Feature-flag override is written to the wrong object level (spec.spec).

template is already the AgenticSession spec (Line 337 sets "spec": template), but this function writes to template["spec"]["environmentVariables"]. That produces spec.spec.environmentVariables, so JIRA_READ_ONLY_MODE won’t be applied where runtime expects it.

Suggested fix
-		// Get or create environmentVariables map
-		spec, ok := template["spec"].(map[string]interface{})
-		if !ok {
-			spec = map[string]interface{}{}
-			template["spec"] = spec
-		}
-
-		envVars, ok := spec["environmentVariables"].(map[string]interface{})
+		// template is already the session spec; write env vars at this level.
+		envVars, ok := template["environmentVariables"].(map[string]interface{})
 		if !ok {
 			envVars = map[string]interface{}{}
 		}
 
 		// Set JIRA_READ_ONLY_MODE to false to enable write operations
 		envVars["JIRA_READ_ONLY_MODE"] = "false"
-		spec["environmentVariables"] = envVars
+		template["environmentVariables"] = envVars

Also applies to: 337-337

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@components/operator/internal/trigger/trigger.go` around lines 268 - 282, The
code is writing environmentVariables into template["spec"] producing
spec.spec.environmentVariables; treat template as the AgenticSession spec
instead and set environmentVariables directly on template. Update the logic that
creates/reads spec and envVars (the variables named template, spec and envVars)
so you either assign spec = template.(map[string]interface{}) or read envVars
from template["environmentVariables"] (not
template["spec"]["environmentVariables"]), then set
envVars["JIRA_READ_ONLY_MODE"]="false" and store it back to
template["environmentVariables"] (or spec["environmentVariables"] if you
switched spec to reference template).

log.Printf("Applied jira-write feature flag: JIRA_READ_ONLY_MODE=false")
}

return nil
}

// createNewSession creates a new AgenticSession CR (original behavior).
func createNewSession(dynamicClient dynamic.Interface, namespace, scheduledSessionName string, template map[string]interface{}) {
// Apply feature flag overrides to template before creating the session
cfg, err := rest.InClusterConfig()
if err != nil {
log.Printf("WARNING: failed to get cluster config for feature flag check: %v", err)
// Continue without feature flags - degraded but not fatal
} else {
k8sClient, err := kubernetes.NewForConfig(cfg)
if err != nil {
log.Printf("WARNING: failed to create k8s client for feature flag check: %v", err)
} else {
// Apply feature flag overrides to template
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
defer cancel()
if err := applyFeatureFlagOverrides(ctx, k8sClient, namespace, template); err != nil {
log.Printf("WARNING: failed to apply feature flag overrides: %v", err)
}
}
}

// Build session name and display name.
// The most restrictive derived K8s resource name is the Service:
// "session-" (8 chars) + sessionName ≤ 63 → sessionName ≤ 55
Expand Down Expand Up @@ -279,9 +340,9 @@ func createNewSession(dynamicClient dynamic.Interface, namespace, scheduledSessi

// Create via dynamic client
gvr := types.GetAgenticSessionResource()
ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
defer cancel()
_, err := dynamicClient.Resource(gvr).Namespace(namespace).Create(ctx, session, metav1.CreateOptions{})
ctx2, cancel2 := context.WithTimeout(context.Background(), 30*time.Second)
defer cancel2()
_, err = dynamicClient.Resource(gvr).Namespace(namespace).Create(ctx2, session, metav1.CreateOptions{})
if err != nil {
log.Fatalf("Failed to create AgenticSession %s in namespace %s: %v", sessionName, namespace, err)
}
Expand Down
122 changes: 122 additions & 0 deletions components/operator/internal/trigger/trigger_test.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
package trigger

import (
"context"
"testing"

corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/kubernetes/fake"
)

func TestSanitizeName(t *testing.T) {
Expand Down Expand Up @@ -104,3 +109,120 @@ func TestSanitizeName_TruncationPreservesValidSuffix(t *testing.T) {
t.Errorf("sanitizeName(%q) ends with hyphen: %q", input, result)
}
}

func TestApplyFeatureFlagOverrides(t *testing.T) {
tests := []struct {
name string
configMapData map[string]string
existingEnvVars map[string]interface{}
expectedEnvVars map[string]interface{}
}{
{
name: "jira-write enabled sets JIRA_READ_ONLY_MODE to false",
configMapData: map[string]string{"jira-write": "true"},
existingEnvVars: nil,
expectedEnvVars: map[string]interface{}{"JIRA_READ_ONLY_MODE": "false"},
},
{
name: "jira-write disabled does not set env var",
configMapData: map[string]string{"jira-write": "false"},
existingEnvVars: nil,
expectedEnvVars: nil,
},
{
name: "no overrides ConfigMap does not set env var",
configMapData: nil,
existingEnvVars: nil,
expectedEnvVars: nil,
},
{
name: "preserves existing env vars",
configMapData: map[string]string{"jira-write": "true"},
existingEnvVars: map[string]interface{}{"CUSTOM_VAR": "value"},
expectedEnvVars: map[string]interface{}{
"CUSTOM_VAR": "value",
"JIRA_READ_ONLY_MODE": "false",
},
},
{
name: "other flags do not affect env vars",
configMapData: map[string]string{"other-flag": "true"},
existingEnvVars: nil,
expectedEnvVars: nil,
},
{
name: "jira-write with non-true value does not set env var",
configMapData: map[string]string{"jira-write": "yes"},
existingEnvVars: nil,
expectedEnvVars: nil,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
// Create fake K8s client
var k8sClient *fake.Clientset
if tt.configMapData != nil {
configMap := &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: "feature-flag-overrides",
Namespace: "test-namespace",
},
Data: tt.configMapData,
}
k8sClient = fake.NewSimpleClientset(configMap)
} else {
k8sClient = fake.NewSimpleClientset()
}

// Create template with optional existing env vars
template := map[string]interface{}{
"spec": map[string]interface{}{},
}
if tt.existingEnvVars != nil {
spec := template["spec"].(map[string]interface{})
spec["environmentVariables"] = tt.existingEnvVars
}
Comment on lines +181 to +187
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Test fixture uses a nested spec shape that does not match runtime input.

applyFeatureFlagOverrides is called with the session spec map in production, but this test passes {"spec": {...}} and asserts inside template["spec"]. That can mask real regressions and currently validates the wrong contract.

Suggested fix
-			template := map[string]interface{}{
-				"spec": map[string]interface{}{},
-			}
+			template := map[string]interface{}{}
 			if tt.existingEnvVars != nil {
-				spec := template["spec"].(map[string]interface{})
-				spec["environmentVariables"] = tt.existingEnvVars
+				template["environmentVariables"] = tt.existingEnvVars
 			}
@@
-			spec, ok := template["spec"].(map[string]interface{})
-			if !ok {
-				t.Fatal("template[spec] is not a map")
-			}
-
-			envVars, ok := spec["environmentVariables"].(map[string]interface{})
+			envVars, ok := template["environmentVariables"].(map[string]interface{})
 			if tt.expectedEnvVars == nil {

Also applies to: 195-210

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@components/operator/internal/trigger/trigger_test.go` around lines 179 - 185,
The test fixture nests the session spec under "spec" which doesn't match the
real runtime input consumed by applyFeatureFlagOverrides; update the test to
pass the spec map directly (not wrapped as {"spec": ...}) and adjust assertions
to inspect top-level keys (e.g., check environmentVariables on the map passed to
applyFeatureFlagOverrides), replacing usages of template["spec"] and any other
occurrences in this file (also update the similar block around the other failing
assertions) so the test exercises the same contract as
applyFeatureFlagOverrides.


// Apply feature flag overrides
ctx := context.Background()
err := applyFeatureFlagOverrides(ctx, k8sClient, "test-namespace", template)
if err != nil {
t.Fatalf("applyFeatureFlagOverrides() unexpected error: %v", err)
}

// Verify environment variables
spec, ok := template["spec"].(map[string]interface{})
if !ok {
t.Fatal("template[spec] is not a map")
}

envVars, ok := spec["environmentVariables"].(map[string]interface{})
if tt.expectedEnvVars == nil {
if envVars != nil && len(envVars) > 0 {
t.Errorf("Expected no environmentVariables, got %v", envVars)
}
return
}

if !ok {
t.Fatal("spec[environmentVariables] is not a map")
}

if len(envVars) != len(tt.expectedEnvVars) {
t.Errorf("environmentVariables count = %d, want %d", len(envVars), len(tt.expectedEnvVars))
}

for key, expectedVal := range tt.expectedEnvVars {
actualVal, exists := envVars[key]
if !exists {
t.Errorf("environmentVariables[%q] missing, want %q", key, expectedVal)
continue
}
if actualVal != expectedVal {
t.Errorf("environmentVariables[%q] = %q, want %q", key, actualVal, expectedVal)
}
}
})
}
}