Skip to content

Commit a4f2ddb

Browse files
blublinskyblublinsky
committed
Update Tools Approval Configuration to always Include Config defaulting to tools annotations
1 parent c9efdb4 commit a4f2ddb

9 files changed

Lines changed: 98 additions & 70 deletions

File tree

‎api/v1alpha1/olsconfig_types.go‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -251,9 +251,6 @@ type OLSSpec struct {
251251
// +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Tool Filtering Configuration",xDescriptors={"urn:alm:descriptor:com.tectonic.ui:advanced"}
252252
ToolFilteringConfig *ToolFilteringConfig `json:"toolFilteringConfig,omitempty"`
253253
// Tool execution approval configuration. Controls whether tool calls require user approval before execution.
254-
// ⚠️ WARNING: This feature is not yet fully supported in the current OLS backend version.
255-
// The operator will generate the configuration, but tool approval behavior may not function as expected.
256-
// Please verify backend support before enabling.
257254
// +kubebuilder:validation:Optional
258255
// +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Tools Approval Configuration",xDescriptors={"urn:alm:descriptor:com.tectonic.ui:advanced"}
259256
ToolsApprovalConfig *ToolsApprovalConfig `json:"toolsApprovalConfig,omitempty"`
@@ -646,7 +643,7 @@ type ToolsApprovalConfig struct {
646643
// 'never' - tools execute without approval
647644
// 'always' - all tool calls require approval
648645
// 'tool_annotations' - approval based on per-tool annotations
649-
// +kubebuilder:default=never
646+
// +kubebuilder:default=tool_annotations
650647
// +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Approval Type"
651648
ApprovalType ApprovalType `json:"approvalType,omitempty"`
652649

‎bundle/manifests/lightspeed-operator.clusterserviceversion.yaml‎

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ metadata:
3838
]
3939
capabilities: Seamless Upgrades
4040
console.openshift.io/operator-monitoring-default: "true"
41-
createdAt: "2026-04-01T07:12:38Z"
41+
createdAt: "2026-04-09T08:26:34Z"
4242
features.operators.openshift.io/cnf: "false"
4343
features.operators.openshift.io/cni: "false"
4444
features.operators.openshift.io/csi: "false"
@@ -405,11 +405,7 @@ spec:
405405
- description: Number of tools to retrieve
406406
displayName: Top K
407407
path: ols.toolFilteringConfig.topK
408-
- description: |-
409-
Tool execution approval configuration. Controls whether tool calls require user approval before execution.
410-
⚠️ WARNING: This feature is not yet fully supported in the current OLS backend version.
411-
The operator will generate the configuration, but tool approval behavior may not function as expected.
412-
Please verify backend support before enabling.
408+
- description: Tool execution approval configuration. Controls whether tool calls require user approval before execution.
413409
displayName: Tools Approval Configuration
414410
path: ols.toolsApprovalConfig
415411
x-descriptors:

‎bundle/manifests/ols.openshift.io_olsconfigs.yaml‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4728,19 +4728,16 @@ spec:
47284728
- message: threshold must be between 0.0 and 1.0
47294729
rule: self.threshold >= 0.0 && self.threshold <= 1.0
47304730
toolsApprovalConfig:
4731-
description: |-
4732-
Tool execution approval configuration. Controls whether tool calls require user approval before execution.
4733-
⚠️ WARNING: This feature is not yet fully supported in the current OLS backend version.
4734-
The operator will generate the configuration, but tool approval behavior may not function as expected.
4735-
Please verify backend support before enabling.
4731+
description: Tool execution approval configuration. Controls whether
4732+
tool calls require user approval before execution.
47364733
properties:
47374734
approvalTimeout:
47384735
default: 600
47394736
description: Timeout in seconds for waiting for user approval
47404737
minimum: 1
47414738
type: integer
47424739
approvalType:
4743-
default: never
4740+
default: tool_annotations
47444741
description: |-
47454742
Approval strategy for tool execution.
47464743
'never' - tools execute without approval

‎config/crd/bases/ols.openshift.io_olsconfigs.yaml‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4728,19 +4728,16 @@ spec:
47284728
- message: threshold must be between 0.0 and 1.0
47294729
rule: self.threshold >= 0.0 && self.threshold <= 1.0
47304730
toolsApprovalConfig:
4731-
description: |-
4732-
Tool execution approval configuration. Controls whether tool calls require user approval before execution.
4733-
⚠️ WARNING: This feature is not yet fully supported in the current OLS backend version.
4734-
The operator will generate the configuration, but tool approval behavior may not function as expected.
4735-
Please verify backend support before enabling.
4731+
description: Tool execution approval configuration. Controls whether
4732+
tool calls require user approval before execution.
47364733
properties:
47374734
approvalTimeout:
47384735
default: 600
47394736
description: Timeout in seconds for waiting for user approval
47404737
minimum: 1
47414738
type: integer
47424739
approvalType:
4743-
default: never
4740+
default: tool_annotations
47444741
description: |-
47454742
Approval strategy for tool execution.
47464743
'never' - tools execute without approval

‎config/manifests/bases/lightspeed-operator.clusterserviceversion.yaml‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -382,11 +382,8 @@ spec:
382382
- description: Number of tools to retrieve
383383
displayName: Top K
384384
path: ols.toolFilteringConfig.topK
385-
- description: |-
386-
Tool execution approval configuration. Controls whether tool calls require user approval before execution.
387-
⚠️ WARNING: This feature is not yet fully supported in the current OLS backend version.
388-
The operator will generate the configuration, but tool approval behavior may not function as expected.
389-
Please verify backend support before enabling.
385+
- description: Tool execution approval configuration. Controls whether tool
386+
calls require user approval before execution.
390387
displayName: Tools Approval Configuration
391388
path: ols.toolsApprovalConfig
392389
x-descriptors:

‎internal/controller/appserver/assets.go‎

Lines changed: 19 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -492,25 +492,30 @@ func GenerateOLSConfigMap(r reconciler.Reconciler, ctx context.Context, cr *olsv
492492
appSrvConfigFile.OLSConfig.ToolFiltering = toolFilteringConfig
493493
}
494494

495-
// Add tools approval configuration if specified
496-
if cr.Spec.OLSConfig.ToolsApprovalConfig != nil {
497-
// Apply defaults for zero values (happens when user specifies toolsApprovalConfig: {})
498-
cfg := cr.Spec.OLSConfig.ToolsApprovalConfig
499-
approvalType := string(cfg.ApprovalType)
500-
approvalTimeout := cfg.ApprovalTimeout
501-
502-
// Apply defaults if not set
495+
// Add tools approval configuration (always present with defaults from CRD)
496+
var approvalType string
497+
var approvalTimeout int
498+
499+
if cr.Spec.OLSConfig.ToolsApprovalConfig == nil {
500+
// Use CRD defaults (must match +kubebuilder:default markers in ToolsApprovalConfig)
501+
approvalType = string(olsv1alpha1.ApprovalTypeToolAnnotations) // CRD default: tool_annotations
502+
approvalTimeout = 600 // CRD default: 600
503+
} else {
504+
// Use specified values, applying CRD defaults for zero values
505+
approvalType = string(cr.Spec.OLSConfig.ToolsApprovalConfig.ApprovalType)
506+
approvalTimeout = cr.Spec.OLSConfig.ToolsApprovalConfig.ApprovalTimeout
507+
503508
if approvalType == "" {
504-
approvalType = string(olsv1alpha1.ApprovalTypeNever)
509+
approvalType = string(olsv1alpha1.ApprovalTypeToolAnnotations) // CRD default: tool_annotations
505510
}
506511
if approvalTimeout == 0 {
507-
approvalTimeout = 600
512+
approvalTimeout = 600 // CRD default: 600
508513
}
514+
}
509515

510-
appSrvConfigFile.OLSConfig.ToolsApproval = &utils.ToolsApprovalConfig{
511-
ApprovalType: approvalType,
512-
ApprovalTimeout: approvalTimeout,
513-
}
516+
appSrvConfigFile.OLSConfig.ToolsApproval = &utils.ToolsApprovalConfig{
517+
ApprovalType: approvalType,
518+
ApprovalTimeout: approvalTimeout,
514519
}
515520

516521
// Marshal the configuration to YAML format

‎internal/controller/appserver/assets_test.go‎

Lines changed: 20 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,10 @@ var _ = Describe("App server assets", func() {
124124
"/etc/certs/ols-additional-ca/service-ca.crt",
125125
},
126126
CertificateDirectory: "/etc/certs/cert-bundle",
127+
ToolsApproval: &utils.ToolsApprovalConfig{
128+
ApprovalType: "tool_annotations",
129+
ApprovalTimeout: 600,
130+
},
127131
},
128132
LLMProviders: []utils.ProviderConfig{
129133
{
@@ -225,7 +229,7 @@ var _ = Describe("App server assets", func() {
225229
err = yaml.Unmarshal([]byte(cm.Data[utils.OLSConfigFilename]), &olsConfigMap)
226230
Expect(err).NotTo(HaveOccurred())
227231
Expect(olsConfigMap).To(HaveKeyWithValue("ols_config", HaveKeyWithValue("tools_approval", MatchAllKeys(Keys{
228-
"approval_type": Equal("never"),
232+
"approval_type": Equal("tool_annotations"),
229233
"approval_timeout": BeNumerically("==", 600),
230234
}))))
231235

@@ -257,15 +261,16 @@ var _ = Describe("App server assets", func() {
257261
"approval_timeout": BeNumerically("==", 120),
258262
}))))
259263

260-
By("not present when config is nil")
264+
By("with default values when config is nil")
261265
cr.Spec.OLSConfig.ToolsApprovalConfig = nil
262266
cm, err = GenerateOLSConfigMap(testReconcilerInstance, context.TODO(), cr)
263267
Expect(err).NotTo(HaveOccurred())
264268
err = yaml.Unmarshal([]byte(cm.Data[utils.OLSConfigFilename]), &olsConfigMap)
265269
Expect(err).NotTo(HaveOccurred())
266-
olsConfig, ok := olsConfigMap["ols_config"].(map[string]interface{})
267-
Expect(ok).To(BeTrue())
268-
Expect(olsConfig).NotTo(HaveKey("tools_approval"))
270+
Expect(olsConfigMap).To(HaveKeyWithValue("ols_config", HaveKeyWithValue("tools_approval", MatchAllKeys(Keys{
271+
"approval_type": Equal("tool_annotations"),
272+
"approval_timeout": BeNumerically("==", 600),
273+
}))))
269274
})
270275

271276
It("should generate configmap with token quota limiters", func() {
@@ -1286,6 +1291,9 @@ ols_config:
12861291
tls_config:
12871292
tls_certificate_path: /etc/certs/lightspeed-tls/tls.crt
12881293
tls_key_path: /etc/certs/lightspeed-tls/tls.key
1294+
tools_approval:
1295+
approval_timeout: 600
1296+
approval_type: tool_annotations
12891297
user_data_collection:
12901298
feedback_disabled: false
12911299
feedback_storage: /app-root/ols-user-data/feedback
@@ -1346,6 +1354,9 @@ ols_config:
13461354
tls_config:
13471355
tls_certificate_path: /etc/certs/lightspeed-tls/tls.crt
13481356
tls_key_path: /etc/certs/lightspeed-tls/tls.key
1357+
tools_approval:
1358+
approval_timeout: 600
1359+
approval_type: tool_annotations
13491360
user_data_collection:
13501361
feedback_disabled: true
13511362
feedback_storage: /app-root/ols-user-data/feedback
@@ -2045,11 +2056,11 @@ var _ = Describe("Helper function unit tests", func() {
20452056
It("should return error when proxy CA certificate ConfigMap does not exist", func() {
20462057
cr.Spec.OLSConfig.ProxyConfig = &olsv1alpha1.ProxyConfig{
20472058
ProxyURL: "http://proxy.example.com:8080",
2048-
ProxyCACertificateRef: &olsv1alpha1.ProxyCACertConfigMapRef{
2049-
LocalObjectReference: corev1.LocalObjectReference{
2050-
Name: "nonexistent-proxy-ca",
2059+
ProxyCACertificateRef: &olsv1alpha1.ProxyCACertConfigMapRef{
2060+
LocalObjectReference: corev1.LocalObjectReference{
2061+
Name: "nonexistent-proxy-ca",
2062+
},
20512063
},
2052-
},
20532064
}
20542065
// Don't create the ConfigMap - validation should fail
20552066
_, err := buildOLSConfig(testReconcilerInstance, ctx, cr, false)

‎internal/controller/lcore/config.go‎

Lines changed: 16 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -894,22 +894,24 @@ func buildLCoreQuotaHandlersConfig(r reconciler.Reconciler, cr *olsv1alpha1.OLSC
894894
// buildLCoreToolsApprovalConfig configures tool execution approval
895895
// Controls whether tool calls require user approval before execution
896896
func buildLCoreToolsApprovalConfig(_ reconciler.Reconciler, cr *olsv1alpha1.OLSConfig) map[string]interface{} {
897-
// If no tools approval config in CR, return nil (not configured)
898-
if cr.Spec.OLSConfig.ToolsApprovalConfig == nil {
899-
return nil
900-
}
897+
var approvalType string
898+
var approvalTimeout int
901899

902-
cfg := cr.Spec.OLSConfig.ToolsApprovalConfig
903-
904-
// Apply defaults if not set
905-
approvalType := string(cfg.ApprovalType)
906-
if approvalType == "" {
907-
approvalType = string(olsv1alpha1.ApprovalTypeNever)
908-
}
900+
if cr.Spec.OLSConfig.ToolsApprovalConfig == nil {
901+
// Use CRD defaults (must match +kubebuilder:default markers in ToolsApprovalConfig)
902+
approvalType = string(olsv1alpha1.ApprovalTypeToolAnnotations) // CRD default: tool_annotations
903+
approvalTimeout = 600 // CRD default: 600
904+
} else {
905+
// Use specified values, applying CRD defaults for zero values
906+
approvalType = string(cr.Spec.OLSConfig.ToolsApprovalConfig.ApprovalType)
907+
approvalTimeout = cr.Spec.OLSConfig.ToolsApprovalConfig.ApprovalTimeout
909908

910-
approvalTimeout := cfg.ApprovalTimeout
911-
if approvalTimeout == 0 {
912-
approvalTimeout = 600
909+
if approvalType == "" {
910+
approvalType = string(olsv1alpha1.ApprovalTypeToolAnnotations) // CRD default: tool_annotations
911+
}
912+
if approvalTimeout == 0 {
913+
approvalTimeout = 600 // CRD default: 600
914+
}
913915
}
914916

915917
return map[string]interface{}{

‎internal/controller/lcore/config_test.go‎

Lines changed: 32 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -510,7 +510,7 @@ func TestBuildLCoreToolsApprovalConfig_WithConfig(t *testing.T) {
510510
{
511511
name: "with empty config (defaults applied)",
512512
config: &olsv1alpha1.ToolsApprovalConfig{},
513-
expectedType: "never",
513+
expectedType: "tool_annotations",
514514
expectedTimeout: 600,
515515
},
516516
}
@@ -563,8 +563,24 @@ func TestBuildLCoreToolsApprovalConfig_WithoutConfig(t *testing.T) {
563563

564564
result := buildLCoreToolsApprovalConfig(r, cr)
565565

566-
if result != nil {
567-
t.Errorf("Expected nil result when ToolsApprovalConfig is nil, got %v", result)
566+
if result == nil {
567+
t.Fatal("Expected non-nil result when ToolsApprovalConfig is nil")
568+
}
569+
570+
approvalType, ok := result["approval_type"].(string)
571+
if !ok {
572+
t.Fatal("Expected approval_type to be string")
573+
}
574+
if approvalType != "tool_annotations" {
575+
t.Errorf("Expected approval_type %q, got %q", "tool_annotations", approvalType)
576+
}
577+
578+
approvalTimeout, ok := result["approval_timeout"].(int)
579+
if !ok {
580+
t.Fatal("Expected approval_timeout to be int")
581+
}
582+
if approvalTimeout != 600 {
583+
t.Errorf("Expected approval_timeout %d, got %d", 600, approvalTimeout)
568584
}
569585
}
570586

@@ -625,8 +641,18 @@ func TestBuildLCoreConfigYAML_WithoutToolsApproval(t *testing.T) {
625641
t.Fatalf("buildLCoreConfigYAML returned error: %v", err)
626642
}
627643

628-
// Verify YAML does NOT contain tools_approval section
629-
if strings.Contains(yamlStr, "tools_approval:") {
630-
t.Error("Expected YAML NOT to contain 'tools_approval:' section when not configured")
644+
// Verify YAML contains tools_approval section with defaults
645+
if !strings.Contains(yamlStr, "tools_approval:") {
646+
t.Error("Expected YAML to contain 'tools_approval:' section with defaults when not configured")
647+
}
648+
649+
// Verify default approval_type is present
650+
if !strings.Contains(yamlStr, "approval_type: tool_annotations") {
651+
t.Error("Expected YAML to contain 'approval_type: tool_annotations' as default")
652+
}
653+
654+
// Verify default approval_timeout is present
655+
if !strings.Contains(yamlStr, "approval_timeout: 600") {
656+
t.Error("Expected YAML to contain 'approval_timeout: 600' as default")
631657
}
632658
}

0 commit comments

Comments
 (0)