Skip to content

conformance: fix invalid BackendTLSPolicy conformance test #4105

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

conformance: fix invalid BackendTLSPolicy conformance test #4105

k8s-ci-robot merged 2 commits into from
Sep 23, 2025

Conversation

@howardjohn
Copy link
Contributor

@howardjohn howardjohn commented Sep 18, 2025

What type of PR is this?
/kind bug

What this PR does / why we need it:
The BackendTLSPolicyObservedGenerationBump has

  validation:
    caCertificateRefs:
      - group: ""
        kind: ConfigMap
        # This ConfigMap is generated dynamically by the test suite.
        name: "backend-tls-checks-certificate"
    hostname: "abc.example.com"

backend-tls-checks-certificate does not exist, nor is it created by the test suite (despite the comment).

Our implementation sets Accepted=false for this, because there were no valid caCertificateRefs, therefor we cannot accept the policy. This is in line with the specification:

	// If ALL CACertificateRefs are invalid, the implementation MUST also
	// ensure the `Accepted` Condition on the BackendTLSPolicy is set to
	// `status: False`, with a Reason `NoValidCACertificate`.

Which issue(s) this PR fixes:

Fixes #4103

Does this PR introduce a user-facing change?:

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 18, 2025
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 18, 2025
howardjohn
howardjohn commented Sep 18, 2025
View reviewed changes
conformance/tests/backendtlspolicy-observed-generation-bump.yaml Outdated
kind: ConfigMap
# This ConfigMap is generated dynamically by the test suite.
name: "backend-tls-checks-certificate"
wellKnownCACertificates: System
Copy link
Contributor Author

@howardjohn howardjohn Sep 18, 2025

Choose a reason for hiding this comment

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

The test has nothing to do with whether caCertificateRefs or wellKnownCACertificates is used; just using wellKnownCACertificates to make a simple policy that should be Accepted. The purpose of the test does not depend on this.

We could also create the CM but its more complex for no benefit

Copy link
Member

@snorwin snorwin Sep 19, 2025

Choose a reason for hiding this comment

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

I would suggest to use the tls-checks-ca-certificate ConfigMap (https://github.com/kubernetes-sigs/gateway-api/blob/main/conformance/tests/backendtlspolicy.yaml#L144) as in the normative tests, since the well-known system CA certificate isn’t considered required for Core conformance.

Copy link
Contributor

@candita candita Sep 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

That's correct the system CA certificate support is implementation-specific.

One generated by the test suite here:
https://github.com/kubernetes-sigs/gateway-api/blob/main/conformance/utils/suite/suite.go#L391-L392

@robscott
Copy link
Member

robscott commented Sep 19, 2025

Thanks @howardjohn!

/cc @candita @kl52752 @snorwin

snorwin
snorwin suggested changes Sep 19, 2025
View reviewed changes
Copy link
Member

@snorwin snorwin left a comment

Choose a reason for hiding this comment

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

@howardjohn good catch! I overlooked this one in #4016.

howardjohn
howardjohn commented Sep 22, 2025
View reviewed changes
Copy link
Contributor Author

@howardjohn howardjohn left a comment

Choose a reason for hiding this comment

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

updated, thanks @snorwin !

@youngnick youngnick added the release-note-none Denotes a PR that doesn't merit a release note. label Sep 23, 2025
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Sep 23, 2025
@youngnick
Copy link
Contributor

youngnick commented Sep 23, 2025

This LGTM now but I will leave it for other reviewers to re-review.

snorwin
snorwin approved these changes Sep 23, 2025
View reviewed changes
Copy link
Member

@snorwin snorwin left a comment

Choose a reason for hiding this comment

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

/lgtm

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

k8s-ci-robot commented Sep 23, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: howardjohn, 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 merged commit 4c403cf into kubernetes-sigs:main Sep 23, 2025
20 checks passed
kl52752
kl52752 reviewed Sep 23, 2025
View reviewed changes
Copy link
Contributor

@kl52752 kl52752 left a comment

Choose a reason for hiding this comment

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

Thanks for the fix,
/lgtm

@snorwin
Copy link
Member

snorwin commented Sep 23, 2025

/cherry-pick release-1.4

@k8s-infra-cherrypick-robot
Copy link
Contributor

k8s-infra-cherrypick-robot commented Sep 23, 2025

@snorwin: new pull request created: #4117

In response to this:

/cherry-pick release-1.4

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.

@k8s-infra-cherrypick-robot k8s-infra-cherrypick-robot mentioned this pull request Sep 23, 2025
Merged
tylerauerbeck pushed a commit to tylerauerbeck/gateway-api that referenced this pull request Nov 27, 2025
@howardjohn @tylerauerbeck
conformance: fix invalid BackendTLSPolicy conformance test (kubernete…
fa948b1 
…s-sigs#4105)

* conformance: fix invalid BackendTLSPolicy conformance test

* Move to a real one
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kflynn kflynn Awaiting requested review from kflynn

@mikemorris mikemorris Awaiting requested review from mikemorris

@candita candita Awaiting requested review from candita

2 more reviewers

@kl52752 kl52752 kl52752 left review comments

@snorwin snorwin snorwin approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@snorwin snorwin

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/bug Categorizes issue or PR as related to a bug. 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/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

v.1.4.0: new conformance test BackendTLSPolicyObservedGenerationBump makes invalid assertion (IMO)

8 participants

@howardjohn @robscott @youngnick @k8s-ci-robot @snorwin @k8s-infra-cherrypick-robot @candita @kl52752