Skip to content

Conversation

snorwin
Copy link
Member

@snorwin snorwin commented Aug 21, 2025

What type of PR is this?

/kind test
/area conformance-test

What this PR does / why we need it:
This test covers the following scenarios:

  • Invalid Kind
  • Malformed CA certificate ConfigMap
  • Missing (nonexistent) CA certificate ConfigMap
  • Valid CA certificate (implemented as part of the normative tests)

Which issue(s) this PR fixes:

Fixes #3993

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 area/conformance-test Issues or PRs related to Conformance tests. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 21, 2025
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 21, 2025
@snorwin
Copy link
Member Author

snorwin commented Aug 21, 2025

/cc @candita @kl52752

@snorwin snorwin force-pushed the btlsp-resolvedrefs-conformance branch from a6c17e9 to 8dd5b74 Compare August 21, 2025 12:51
@snorwin snorwin moved this to Review in Release v1.4.0 Aug 21, 2025
@snorwin snorwin force-pushed the btlsp-resolvedrefs-conformance branch from 8dd5b74 to 5ea56fc Compare August 21, 2025 13:19
Signed-off-by: Norwin Schnyder <[email protected]>
@snorwin snorwin requested a review from kl52752 August 22, 2025 06:14
@snorwin snorwin requested a review from kl52752 August 23, 2025 06:55
@snorwin snorwin force-pushed the btlsp-resolvedrefs-conformance branch from a9f5c10 to 357342b Compare August 23, 2025 12:02
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 PR, LGTM

@kl52752
Copy link
Contributor

kl52752 commented Aug 25, 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 25, 2025
@snorwin
Copy link
Member Author

snorwin commented Aug 25, 2025

/assign @LiorLieberman

@snorwin
Copy link
Member Author

snorwin commented Aug 25, 2025

/assign @shaneutt

@shaneutt shaneutt added this to the v1.4.0 milestone Aug 25, 2025
@snorwin snorwin force-pushed the btlsp-resolvedrefs-conformance branch from 357342b to 64ce80a Compare August 25, 2025 19:17
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 25, 2025
@snorwin snorwin requested a review from rikatz August 25, 2025 19:20
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.

Thanks for taking the time to do this @snorwin 👍

My initial pass looked good, have we tried to run these tests on an implementation yet?

@snorwin
Copy link
Member Author

snorwin commented Aug 25, 2025

@shaneutt it's not possible to verify this with an actual implementation, as the required API changes will be released in v1.4.0. However, I tested it using a patched version of the Airlock Microgateway implementation.

@shaneutt shaneutt requested a review from kl52752 August 25, 2025 20:02
@rikatz
Copy link
Member

rikatz commented Aug 25, 2025

I was testing against envoy gateway, but due to the lack of resolvedRefs conditions it is not passing as well.


kubernetes.NamespacesMustBeReady(t, suite.Client, suite.TimeoutConfig, []string{ns})
gwAddr := kubernetes.GatewayAndRoutesMustBeAccepted(t, suite.Client, suite.TimeoutConfig, suite.ControllerName, kubernetes.NewGatewayRef(gwNN), &gatewayv1.HTTPRoute{}, false, routeNN)
kubernetes.HTTPRouteMustHaveResolvedRefsConditionsTrue(t, suite.Client, suite.TimeoutConfig, routeNN, gwNN)
Copy link
Member

Choose a reason for hiding this comment

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

Is this right? Per the manifest, you are applying a backend that has failure already on backendtlspolicy, resulting on the following condition on HTTPRoute

    - lastTransitionTime: "2025-08-25T20:27:03Z"
      message: |-
        Failed to process route rule 0 backendRef 0: configmap nonexistent-ca-certificate not found in namespace gateway-conformance-infra.
        Failed to process route rule 1 backendRef 0: no ca found in configmap malformed-ca-certificate.
      observedGeneration: 1
      reason: InvalidBackendTLS
      status: "False"
      type: ResolvedRefs

Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty sure that is implementation specific behavior, I don't remember defining that in the BackendTLSPolicy GEP at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

@rikatz yes, there are many implementation-specific behaviors. That’s why we updated the GEP in #3909, and we’re now enforcing this new behavior through these conformance tests.

Copy link
Member Author

@snorwin snorwin Aug 26, 2025

Choose a reason for hiding this comment

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

I removed the assertion on the HTTPRoute since it is not specified whether the invalid ResolvedRefs status condition on the BackendTLSPolicy should be propagated to the HTTPRoute or not.

Should I add an assertion that the HTTPRoute is at least accepted, or does an invalid BackendTLSPolicy also influence its acceptance? - Already done.

/cc @kl52752

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for removing that.This question requires new GEP. I think that you can just leave TODO to address this issue later.

@rikatz
Copy link
Member

rikatz commented Aug 25, 2025

@snorwin a comment on the new cases, the initial test is already failing because at least on envoy gateway, the backendtlspolicy failure to resolve the configmap causes the httproute to have a problem on resolvedrefs.

@snorwin snorwin force-pushed the btlsp-resolvedrefs-conformance branch from 8c5ec61 to d400d84 Compare August 26, 2025 11:33
@snorwin snorwin requested a review from rikatz August 26, 2025 11:39
@snorwin snorwin force-pushed the btlsp-resolvedrefs-conformance branch from d400d84 to a8a865f Compare August 26, 2025 12:03
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.

Looks good to me, again :)

@rikatz
Copy link
Member

rikatz commented Aug 26, 2025

/lgtm

Thank you

/assign @candita @shaneutt

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 26, 2025
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.

Thanks @snorwin I think this looks good.

/approve

It shouldn't block us merging this, but as I understand it it's difficult to test this with implementations right now because no implementations actually follow the resolvedRefs correctly....

So what I would like to do @snorwin is have some kind of follow up where we engage with implementations and get more feedback as soon as we can. Would you mind putting an item on the agenda for our community call today to ask for implementations to implement and try the tests? (if you wont be there today though, I can field it just let me know)

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kl52752, 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 Aug 26, 2025
@k8s-ci-robot k8s-ci-robot merged commit c15b792 into kubernetes-sigs:main Aug 26, 2025
19 checks passed
@github-project-automation github-project-automation bot moved this from Review to Done in Release v1.4.0 Aug 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Status: Done
Development

Successfully merging this pull request may close these issues.

Conformance Tests for ResolvedRefs Condition in BackendTLSPolicy
8 participants