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
gateway-api: Support for BackendRef filters #30090
Conversation
/test |
Confirmed this fix works! Tested by deploying the 1.15.0-rc0 helm chart and using the container image generated in the PR workflow (https://github.com/cilium/cilium/actions/runs/7399979212/job/20132600608). values
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change looks good with one small query.
However, could you take this chance to add tests?
I think we need a test for one backend with Backend filters, multiple backends with different backend filters, and multiple backends with some with backend filters and some not.
The fixtures for the Gateway will need to be added in operator/pkg/model/ingestion/gateway_fixture_test.go
, with the tests in operator/pkg/model/ingestion/gateway_test.go
.
Then, you can take the []model.HTTPRoute
that you get from that test, and use it as a fixture in operator/pkg/model/translation/fixture_test.go
, and test it in operator/pkg/model/translation/translation_test.go
.
Sorry to ask for a lot of tests here, but this feature is implementation-specific, so there aren't any tests upstream yet (I think that the tests you add here would be a great basis for optional upstream conformance tests that could move this feature to Extended there).
Ok, I will try to add some unit tests soon. |
a199ad6
to
a40aebc
Compare
Fixes: cilium#29080 Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
a40aebc
to
f396316
Compare
/test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work on the tests! LGTM.
/ci-ipsec-upgrade |
/ci-aks |
/ci-ipsec-upgrade |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot
Hey! Can this be added to the 1.15.2 release? Thanks! |
Fixes: #29080
Please ensure your pull request adheres to the following guidelines: