Skip to content

fix: add default value validation for AI policy configuration #11594

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

zhengkezhou1
Copy link
Contributor

Description

Fixes #11559

Change Type

/kind bug_fix

Changelog

NONE

Additional Notes

@Copilot Copilot AI review requested due to automatic review settings July 5, 2025 08:04
@github-actions github-actions bot added kind/fix Categorizes issue or PR as related to a bug. release-note-none labels Jul 5, 2025
Copilot

This comment was marked as outdated.

@zhengkezhou1 zhengkezhou1 requested a review from Copilot July 5, 2025 08:39
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enhances default value handling in AI policy configurations by validating JSON literals and refining template generation.

  • Introduces JSON validation for FieldDefault.Value when it appears to be an object or array
  • Updates applyDefaults to correctly build Inja templates for override and non-override cases
  • Adds comprehensive unit tests for default value processing in ai_policy_test.go

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
internal/kgateway/extensions2/plugins/trafficpolicy/ai_policy.go Add JSON validity checks and adjust template logic for defaults
internal/kgateway/extensions2/plugins/trafficpolicy/ai_policy_test.go Add TestDefault cases covering valid/invalid JSON defaults

@zhengkezhou1 zhengkezhou1 reopened this Jul 5, 2025
@zhengkezhou1 zhengkezhou1 force-pushed the add-validation-to-AI-Policy-field-defaults branch from 9ced92b to f255c4a Compare July 7, 2025 03:10
@zhengkezhou1 zhengkezhou1 reopened this Jul 7, 2025
@zhengkezhou1 zhengkezhou1 force-pushed the add-validation-to-AI-Policy-field-defaults branch from 3f45dc1 to 7fd6a25 Compare July 9, 2025 17:29
Comment on lines +226 to +235
Entry(
"TrafficPolicy with ai invalided default values",
translatorTestCase{
inputFile: "traffic-policy/ai-invalid-default-value.yaml",
outputFile: "traffic-policy/ai-invalid-default-value.yaml",
gwNN: types.NamespacedName{
Namespace: "infra",
Name: "example-gateway",
},
}),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there any way to verify the exception when translation fails?

Copy link
Contributor

Choose a reason for hiding this comment

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

You can potentially do a check similar to this if there's an error in the status:

Expect(translatortest.AreReportsSuccess(gwNN, reportsMap)).NotTo(HaveOccurred())

Copy link
Contributor

Choose a reason for hiding this comment

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

I think since the traffic policy doesn't have the error in the status, this can just confirm the output is as expected and the error assertions can happen in ai_policy_test.go

Copy link
Member

Choose a reason for hiding this comment

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

@npolshakova I think we'd see an error surface on the TP & HTTPRoute status (now that #11535 has landed), right? preProcessAITrafficPolicy is called during the builder's Translate method and we propagate those errors to the policy IR.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah confirmed locally the TP rejects the invalid config from the original issue:

...
conditions:
- lastTransitionTime: "2025-07-09T20:29:22Z"
  message: 'field custom_list contains invalid JSON string: [1,2,3'
  observedGeneration: 1
  reason: Invalid
  status: "False"
  type: Accepted

I think we'd need to fix the merge conflict & merge with main to see the HTTPRoute updated as well. Right now it's reporting an Accepted=true state on my local cluster.

Copy link
Contributor Author

@zhengkezhou1 zhengkezhou1 left a comment

Choose a reason for hiding this comment

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

@shashankram @npolshakova I've added test cases for non-JSON scenarios and included comments for the new code logic.

Signed-off-by: zhengkezhou1 <[email protected]>
@timflannagan
Copy link
Member

@zhengkezhou1 Looks like there's some merge conflicts with the gateway translator suite. Can we also merge with main? I think we'd see the HTTPRoute status get affected by this change in addition to the error reflecting on the TP status.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/fix Categorizes issue or PR as related to a bug. release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invalid JSON in AI Policy field defaults not caught by validation
4 participants