8000 BackendTLSPolicy conformance tests for conflict resolution by snorwin · Pull Request #4043 · kubernetes-sigs/gateway-api · GitHub
[go: up one dir, main page]
Skip to content

BackendTLSPolicy conformance tests for conflict resolution #4043

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
merged 3 commits into from
Sep 2, 2025
Merged

BackendTLSPolicy conformance tests for conflict resolution #4043

merged 3 commits into from
Sep 2, 2025

Conversation

snorwin
Copy link
Member
@snorwin snorwin commented Aug 28, 2025

What type of PR is this?

/kind test
/area conformance-test

What this PR does / why we need it:
This PR introduces some basic conformance tests to ensure that the status condition Accepted on a BackendTLSPolicy is set to false with Reason: Conflicted when a policy cannot be accepted due to another policy 8000 already targeting the same target (including the sectionName).

Which issue(s) this PR fixes:

Fixes #3953

Does this PR introduce a user-facing change?:

NONE

@snorwin
BackendTLSPolicy conformance tests for conflict resolution
d6a6f99 
Signed-off-by: Norwin Schnyder <norwin.schnyder+github@gmail.com>
@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/test area/conformance-test Issues or PRs related to Conformance tests. labels Aug 28, 2025
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 28, 2025
@snorwin
Copy link
Member Author
snorwin commented Aug 28, 2025

/cc @rikatz @kl52752

@k8s-ci-robot k8s-ci-robot requested review from kl52752 and rikatz August 28, 2025 14:58
@snorwin
Copy link
Member Author
snorwin commented Aug 28, 2025

/cc @kubernetes-sigs/gateway-api-admins

@k8s-ci-robot k8s-ci-robot requested a review from a team August 28, 2025 15:03
@snorwin snorwin mentioned this pull request Aug 26, 2025
Closed
3 tasks
@shaneutt shaneutt self-assigned this Aug 28, 2025
@shaneutt shaneutt added this to the v1.4.0 milestone Aug 28, 2025
@shaneutt shaneutt moved this to Review in Release v1.4.0 Aug 28, 2025
@snorwin
Copy link
Member Author
snorwin commented Aug 28, 2025

I tested this with Envoy Gateway, and all assertions passed except that the status condition of the second policy is set to False with reason Conflicted. It appears that Envoy completely ignores the conflicted policy and does not set any status for it.

@snorwin snorwin force-pushed the btlsp-conflict-resolution-tests branch from ac6a825 to 8b995c7 Compare August 28, 2025 15:44
@snorwin
fix linting errors
d962aa0 
Signed-off-by: Norwin Schnyder <norwin.schnyder+github@gmail.com>
@snorwin snorwin force-pushed the btlsp-conflict-resolution-tests branch from 8b995c7 to d962aa0 Compare August 28, 2025 15:44
@rikatz
Copy link
Member
rikatz commented Aug 29, 2025

/assign

@rikatz
Copy link
Member
rikatz commented Aug 29, 2025

hum wait: I was reading the GEP (https://gateway-api.sigs.k8s.io/geps/gep-1897/) but this condition is not clear there. Is this something actually related to how policies are attached and not to BackendTLSPolicy?

@kl52752
Copy link
Contributor
kl52752 commented Aug 29, 2025

hum wait: I was reading the GEP (https://gateway-api.sigs.k8s.io/geps/gep-1897/) but this condition is not clear there. Is this something actually related to how policies are attached and not to BackendTLSPolicy?

yes this is specific to DirectPolicy attachment https://gateway-api.sigs.k8s.io/geps/gep-2648/

@snorwin
Copy link
Member Author
snorwin commented Aug 29, 2025

If the section names differ, it is still possible to merge the policies. However, if two distinct hostnames are defined for the same Service and port, it becomes impossible to accept both policies.

I agree that it would be better if this behavior is explicitly defined in the GEP of the BackendTLSPolicy. Since it is currently missing there, I assumed it is inferred from the Direct Policy Attachment, as @kl52752 mentioned:

  // PolicyReasonConflicted is used with the "Accepted" condition when the policy has not
  // been accepted by a targeted resource because there is another policy that targets the same
  // resource and a merge is not possible.

@rikatz
Copy link
Member
rikatz commented Aug 29, 2025

ok, thanks for the clarification folks.

@snorwin as a request, can you just add to your code then, maybe as a comment that this is not BackendTLSPolicy specific, and point to the DAP GEP?

I think this would just make things slightly less confusing for someone implementing the conformance and trying to understand why this is required

@rikatz
Copy link
Member
rikatz commented Aug 29, 2025

/lgtm

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

/hold

If possible please just add the reference to the DAP GEP, as a comment, to make it clear that this is a DirectPolicy Attachment requirement.

Thanks!

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 29, 2025
@snorwin
Copy link
Member Author
snorwin commented Aug 29, 2025 •
edited
Loading

@rikatz I was thinking if we should update the godoc (https://github.com/kubernetes-sigs/gateway-api/blob/main/apis/v1alpha3/backendtlspolicy_types.go#L71) in a separate PR to clarify that targetRefs are expected to be unique across different policies, and that the implementation must mark them as conflicted if this condition isn’t met.

@rikatz
Copy link
Member
rikatz commented Aug 29, 2025

@rikatz I was thinking if we should update the godoc (main/apis/v1alpha3/backendtlspolicy_types.go#L71) in a separate PR to clarify that targetRefs are expected to be unique across different policies, and that the implementation must mark them as conflicted if this condition isn’t met.

I think any information would be good. If this is implementation specific, let's just add that marker that avoids overloading the CRD with implementation specific information, but yeah, feel free to open a new PR to update it!

@snorwin
apply PR feedback
baa925f 
Signed-off-by: Norwin Schnyder <norwin.schnyder+github@gmail.com>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 29, 2025
@snorwin snorwin requested a review from rikatz August 29, 2025 20:29
@rikatz
Copy link
Member
rikatz commented Aug 29, 2025

/lgtm

Thanks.
At some future, if the team agrees, I will set the GEP numbers on the test suite struct, so one can actually refer directly what may be the related GEPs to a conformance test

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 29, 2025
@snorwin snorwin mentioned this pull request Aug 29, 2025
Merged
@rikatz
8000
Copy link
Member
rikatz commented Sep 2, 2025

/cc @shaneutt

shaneutt
shaneutt approved these changes Sep 2, 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.

/approve
/unhold

@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 Sep 2, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 2, 2025
@k8s-ci-robot k8s-ci-robot merged commit 59a1a4e into kubernetes-sigs:main Sep 2, 2025
19 checks passed
@github-project-automation github-project-automation bot moved this from Review to Done in Release v1.4.0 Sep 2, 2025
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

@LiorLieberman LiorLieberman Awaiting requested review from LiorLieberman

@sunjayBhatia sunjayBhatia Awaiting requested review from sunjayBhatia

@kl52752 kl52752 Awaiting requested review from kl52752

@rikatz rikatz Awaiting requested review from rikatz

Assignees

@shaneutt shaneutt

@rikatz rikatz

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/conformance-test Issues or PRs related to Conformance tests. 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. 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.

BackendTLSPolicy conformance test for PolicyAncestorStatus
5 participants
@snorwin @rikatz @kl52752 @k8s-ci-robot @shaneutt
0