Skip to content

trafficpolicy: Adopt policy IR pattern, define Equals methods, etc #11353

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 2 commits into
base: main
Choose a base branch
from

Conversation

timflannagan
Copy link
Member

Description

Follow-up to the re-organization trafficpolicy plugin PR

Further standardizes the per-policy-IR pattern and ensures that each policy type define it's own IR, Equals, handleX methods, etc

Make all sub IR types unexported for consistency

Change Type

/kind cleanup

Changelog

NONE

Additional Notes

Follow-up to the re-organization trafficpolicy plugin PR

Further standardizes the per-policy-IR pattern and ensures that each policy type define it's own IR, Equals, handleX methods, etc

Make all sub IR types unexported for consistency

Signed-off-by: timflannagan <[email protected]>
@Copilot Copilot AI review requested due to automatic review settings June 6, 2025 01:01
@github-actions github-actions bot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. release-note-none labels Jun 6, 2025
@timflannagan
Copy link
Member Author

TODO: handleAI method. Wanted to double check the state of CI after this change since it's invasive first.

Copilot

This comment was marked as outdated.

@timflannagan timflannagan force-pushed the refactor/traffic-policy-sub-ir branch 2 times, most recently from 251ccae to 22eb2d1 Compare June 6, 2025 01:22
@timflannagan timflannagan requested a review from Copilot June 6, 2025 01:22
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 standardizes the policy IR pattern by introducing unexported IR types with custom Equals methods and refactoring the translation logic for various policy components. Key changes include the introduction of transformationIR, rustformationIR, aiPolicyIR, extprocIR and corresponding updates in rate limiting and ext auth components.

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.

Show a summary per file
File Description
internal/kgateway/extensions2/plugins/trafficpolicy/transformation_plugin.go Refactored transformation and rustformation translation with new IR types and Equals methods.
internal/kgateway/extensions2/plugins/trafficpolicy/traffic_policy_plugin.go Updated references to new IR names and method calls for AI, transformation, ext proc, rate limit, and CORS.
internal/kgateway/extensions2/plugins/trafficpolicy/local_rate_limit_plugin.go Updated local rate limit translation to use new IR.
internal/kgateway/extensions2/plugins/trafficpolicy/global_rate_limit_plugin.go Updated global rate limit translation with new IR type and error handling.
internal/kgateway/extensions2/plugins/trafficpolicy/extproc_policy.go Refactored ExtProc IR and translation logic under new naming conventions.
internal/kgateway/extensions2/plugins/trafficpolicy/extauth_policy.go Updated ExtAuth translation to use new naming conventions.
internal/kgateway/extensions2/plugins/trafficpolicy/cors_policy.go Adjusted CORS policy translation consistent with other policy IR changes.
internal/kgateway/extensions2/plugins/trafficpolicy/builder.go Updated overall builder logic to integrate new IR methods for different policies.
internal/kgateway/extensions2/plugins/trafficpolicy/ai_policy.go Renamed and updated AI policy translation and Equals method.
Comments suppressed due to low confidence (2)

internal/kgateway/extensions2/plugins/trafficpolicy/global_rate_limit_plugin.go:75

  • The error check for the global rate limit extension compares against v1alpha1.GatewayExtensionTypeExtAuth instead of the expected GatewayExtensionTypeRateLimit. Please update the expected type for clarity.
return pluginutils.ErrInvalidExtensionType(v1alpha1.GatewayExtensionTypeExtAuth, gwExtIR.ExtType)

internal/kgateway/extensions2/plugins/trafficpolicy/extproc_policy.go:55

  • The extproc translation error check is using v1alpha1.GatewayExtensionTypeExtAuth as the expected type instead of the appropriate GatewayExtensionTypeExtProc. Please update this to reflect the correct extension type.
return pluginutils.ErrInvalidExtensionType(v1alpha1.GatewayExtensionTypeExtAuth, gatewayExtension.ExtType)

@timflannagan timflannagan force-pushed the refactor/traffic-policy-sub-ir branch 2 times, most recently from 01df310 to 022d099 Compare June 6, 2025 02:23
if spec.Transformation == nil {
return nil
type transformationIR struct {
transformation *transformationpb.RouteTransformations
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: transformationIR.transformation seems clunky.
is there a world where it could/should be policy or something else?

Comment on lines +378 to +379
plugins.BeforeStage(plugins.AcceptedStage),
)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a pretty brutal set of opinions on text formatting.
If we are going do it can we find or make a linter for this? Otherwise someone else is going come around and revert this and also add tons of lines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants