Skip to content

Add Conformance test for Invalid BackendTLSPolicy TLS settings #3930

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.

Sign up for GitHub

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

Jump to bottom
Merged
k8s-ci-robot merged 1 commit into from
Aug 7, 2025
Merged

Add Conformance test for Invalid BackendTLSPolicy TLS settings #3930

k8s-ci-robot merged 1 commit into from
Aug 7, 2025

Conversation

@kl52752
Copy link
Contributor

@kl52752 kl52752 commented Jul 18, 2025 •
edited
Loading

What type of PR is this?
/kind test

What this PR does / why we need it:
This PR introduces Conformance tests for BackendTLSPolicy to verify behavior with invalid TLS settings.

Which issue(s) this PR fixes:

Fixes #3138

This covers test cases:

  1. Invalid: hostname doesn't match the hostname in the certificate served by the backend
  2. Invalid BackendTLSPolicy performs no default forwarding

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 18, 2025
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 18, 2025
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Jul 18, 2025

Hi @kl52752. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@kl52752 kl52752 marked this pull request as draft July 18, 2025 15:20
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 18, 2025
@kl52752
Copy link
Contributor Author

kl52752 commented Jul 18, 2025

Marking this PR as draft because it is based on unmerged changes from #3212

/cc @robscott @snorwin @candita

@kl52752 kl52752 force-pushed the bs-test branch 2 times, most recently from fb6c097 to 58279d3 Compare July 18, 2025 16:09
snorwin
snorwin reviewed Jul 21, 2025
View reviewed changes
@kl52752
Copy link
Contributor Author

kl52752 commented Jul 21, 2025

Thank you @snorwin for the review and running conformance tests.
Can you add me ok-to-test label? I'm not kubernetes-sig member (yet!) :)

@snorwin
Copy link
Member

snorwin commented Jul 21, 2025

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 21, 2025
@kl52752
Copy link
Contributor Author

kl52752 commented Jul 21, 2025

/retest

@kl52752
Copy link
Contributor Author

kl52752 commented Jul 21, 2025

/retest

@kl52752
Copy link
Contributor Author

kl52752 commented Jul 21, 2025

/retest

candita
candita reviewed Jul 22, 2025
View reviewed changes
candita
candita reviewed Jul 22, 2025
View reviewed changes
candita
candita reviewed Jul 22, 2025
View reviewed changes
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 6, 2025
candita
candita reviewed Aug 6, 2025
View reviewed changes
candita
candita reviewed Aug 6, 2025
View reviewed changes
candita
candita reviewed Aug 6, 2025
View reviewed changes
@candita
Copy link
Contributor

candita commented Aug 6, 2025

@kl52752 @shaneutt I see a few nits but would really like to see the yaml file name reverted to its original name, as mentioned in https://github.com/kubernetes-sigs/gateway-api/pull/3930/files#r2258524505.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 7, 2025
@shaneutt
Copy link
Member

shaneutt commented Aug 7, 2025

@kl52752 @shaneutt I see a few nits but would really like to see the yaml file name reverted to its original name, as mentioned in https://github.com/kubernetes-sigs/gateway-api/pull/3930/files#r2258524505.

Sounds good. @kl52752 once these are resolved, I'll give it the LGTM 👍

rikatz
rikatz reviewed Aug 7, 2025
View reviewed changes
rikatz
rikatz reviewed Aug 7, 2025
View reviewed changes
@rikatz
Copy link
Member

rikatz commented Aug 7, 2025

@kl52752 just for my own understanding, does this test already pass on some implementation? I was testing with envoy gateway but it fails on setting the observed generation on status ancestors, just wanted to check if something is already compliant with it :)

@kl52752
Copy link
Contributor Author

kl52752 commented Aug 7, 2025

@kl52752 just for my own understanding, does this test already pass on some implementation? I was testing with envoy gateway but it fails on setting the observed generation on status ancestors, just wanted to check if something is already compliant with it :)

yes, @snorwin confirmed that test passed on their impl.

shaneutt
shaneutt approved these changes Aug 7, 2025
View reviewed changes
Copy link
Member

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

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

/lgtm

but I'm keeping the hold for now while @rikatz is taking a look at potential issues with an implementation running these tests 🤔

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 7, 2025
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Aug 7, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kl52752, shaneutt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@rikatz
Copy link
Member

rikatz commented Aug 7, 2025

/lgtm
/hold cancel

Tested here removing just the ObservedGeneration assertion for envoy gateway and works fine, envoy gateway should probably fix it on their implementation.

Thanks @kl52752 !

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 7, 2025
@k8s-ci-robot k8s-ci-robot merged commit bee090d into kubernetes-sigs:main Aug 7, 2025
19 checks passed
@github-project-automation github-project-automation bot moved this from Review to Done in Release v1.4.0 Aug 7, 2025
@shaneutt
Copy link
Member

shaneutt commented Aug 7, 2025

Tested here removing just the ObservedGeneration assertion for envoy gateway and works fine, envoy gateway should probably fix it on their implementation.

/cc @arkodg

@k8s-ci-robot k8s-ci-robot requested a review from arkodg August 7, 2025 14:04
@arkodg
Copy link
Contributor

arkodg commented Aug 7, 2025

thanks for flagging the issue in Envoy Gateway, will get it addressed in the next release

@snorwin snorwin mentioned this pull request Aug 18, 2025
Closed
3 tasks
tylerauerbeck pushed a commit to tylerauerbeck/gateway-api that referenced this pull request Nov 27, 2025
@kl52752 @tylerauerbeck
Invalid BackendTLSPolicy conformance test (kubernetes-sigs#3930)
fc9a9d2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shaneutt shaneutt shaneutt approved these changes

@mikemorris mikemorris Awaiting requested review from mikemorris

@mlavacca mlavacca Awaiting requested review from mlavacca

@robscott robscott Awaiting requested review from robscott

@arkodg arkodg Awaiting requested review from arkodg

+3 more reviewers

@candita candita candita left review comments

@snorwin snorwin snorwin left review comments

@rikatz rikatz rikatz left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@shaneutt shaneutt

@rikatz rikatz

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/test lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

Release v1.4.0
Status: Done

Milestone

v1.4.0

Development

Successfully merging this pull request may close these issues.

Conformance tests for BackendTLSPolicy

7 participants

@kl52752 @k8s-ci-robot @snorwin @candita @shaneutt @rikatz @arkodg