Skip to content

Conversation

@sabrina-ngrok
Copy link
Contributor

#K8SOP-147

What

it looks like we don't handle traffic policy with an inbound phase appropriately. Instead we just discard all of it, leading to no user supplied policy being merged with our generated traffic policy.

This could just be an edge case that you only hit with the inbound key

How

Amp suggested that we create a new JSON deserializer that checks the valid traffic policy keys (on_http_request, on_http_response, and on_tcp_connect) and have a bunch of error handling for edge cases such as this.

Breaking Changes

¯\(ツ)/¯

@github-actions github-actions bot added area/controller Issues dealing with the controller size/L Denotes a PR that changes 100-499 lines labels Dec 1, 2025
@sabrina-ngrok sabrina-ngrok changed the title Create NewTrafficPolicyFromJson deserializer checking for unknown keys s and implement it in translating ingresses Create NewTrafficPolicyFromJson deserializer checking for unknown keys and implement it in translating ingresses Dec 1, 2025
@codecov
Copy link

codecov bot commented Dec 1, 2025

Codecov Report

❌ Patch coverage is 70.83333% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 49.14%. Comparing base (029fd83) to head (e7c146e).

Files with missing lines Patch % Lines
pkg/managerdriver/translate_ingresses.go 37.50% 3 Missing and 2 partials ⚠️
internal/trafficpolicy/policy.go 87.50% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #718      +/-   ##
==========================================
+ Coverage   49.13%   49.14%   +0.01%     
==========================================
  Files          96       96              
  Lines       10604    10616      +12     
==========================================
+ Hits         5210     5217       +7     
- Misses       5037     5038       +1     
- Partials      357      361       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Collaborator

@jonstacks jonstacks left a comment

Choose a reason for hiding this comment

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

Overall this looks good. Just a few changes around error messaging.

The place where this could break someone is if they had a traffic policy like:

bad_key: "I'm ignored today"
on_http_request:
   (fully valid policy here)

Today we ignore the bad_key completely. This would now throw an error for the bad key. I think this is expected behavior, but something we'll want to call out in the release notes.

@sabrina-ngrok sabrina-ngrok marked this pull request as ready for review December 4, 2025 20:13
@sabrina-ngrok sabrina-ngrok requested a review from a team as a code owner December 4, 2025 20:13
@sabrina-ngrok sabrina-ngrok dismissed jonstacks’s stale review December 4, 2025 20:28

Did the changes requested in the last commit of this PR

@sabrina-ngrok
Copy link
Contributor Author

I tested the changes in my local, but I was having issues with SSH to push my commit and it was a small change so I just did a commit via suggestions on here.

name: "invalid JSON should error",
input: `{invalid}`,
expectError: true,
errContains: "failed to unmarshal traffic policy: invalid character 'i' looking for beginning of object key string. raw traffic policy: map[]",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we would expect the original traffic policy {invalid} to be printed so the user knows what wasn't able to be parsed. I think what you had before was close, it just needed to be cast as a string rather than a []byte

@sabrina-ngrok sabrina-ngrok force-pushed the sabrina/bad-traffic-policy-ingress-annotation branch from 99ef197 to e7c146e Compare December 22, 2025 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/controller Issues dealing with the controller size/L Denotes a PR that changes 100-499 lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants