BackendTLSPolicy conformance tests for observedGeneration bump #3997
Conversation
Signed-off-by: Norwin Schnyder <[email protected]>
k8s-ci-robot
added
release-note-none
k8s-ci-robot
added
the
size/L
|
/cc @kl52752 |
|
@snorwin: GitHub didn't allow me to request PR reviews from the following users: kl52752. Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
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. |
|
|
||
| // BackendTLSPolicyMustHaveLatestConditions will fail the test if there are | ||
| // conditions that were not updated | ||
| func BackendTLSPolicyMustHaveLatestConditions(t *testing.T, r *v1alpha3.BackendTLSPolicy) { |
There was a problem hiding this comment.
Isn't this a subset of BackendTLSPolicyMustHaveCondition?
There was a problem hiding this comment.
Not exactly, it doesn’t explicitly assert the observedGeneration bump, but I agree it’s implicitly covered in the BackendTLSPolicyMustHaveCondition since the results of ConditionsHaveLatestObservedGeneration is used as a retry condition.
Signed-off-by: Norwin Schnyder <[email protected]>
kl52752
left a comment
kl52752
left a comment
There was a problem hiding this comment.
Thanks for the PR, LGTM :)
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/assign @arkodg |
|
Thanks @snorwin! /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kl52752, rikatz, robscott, 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 |
k8s-ci-robot
added
the
approved
| namespace: gateway-conformance-infra | ||
| spec: | ||
| selector: | ||
| app: observed-generation-bump-test |
There was a problem hiding this comment.
Any reason we use a service without endpoints here?
Why not use tls-backned directly?
There was a problem hiding this comment.
The idea was simply to use a dummy Service to link the BackendTLSPolicy to an HTTPRoute. Does this cause any issues in your testing infrastructure?
There was a problem hiding this comment.
yeah, EG return 503 directly, which means the process of BackendTLSPolicy is skipped.
is it better to use a valid servcie here?
There was a problem hiding this comment.
This test only verifies how the status is updated, i.e., that the observed generation is set and updated.
How does a direct 503 response from the data plane influence this test? Sorry for asking again, but I’m trying to understand whether this is truly an issue.
There was a problem hiding this comment.
EG skips to process TLSPolicy for a servcie when there's no valid endpoint behide it.
We can move the process before the endpoint checking, just want to understand better why not using a valid service?
There was a problem hiding this comment.
Understood. Could you please open a PR for it? However, I don’t expect it will be backported and released before v1.5.
…netes-sigs#3997) * BackendTLSPolicy conformance tests for observedGeneration bump Signed-off-by: Norwin Schnyder <[email protected]> * Apply PR feedback Signed-off-by: Norwin Schnyder <[email protected]> --------- Signed-off-by: Norwin Schnyder <[email protected]>
7 participants
What type of PR is this?
/kind test
/area conformance-test
What this PR does / why we need it:
Add a conformance test for BackendTLSPolicy to verify that the
observedGenerationis correctly updated for all status conditions, similar to theGatewayClassObservedGenerationBumporHTTPRouteObservedGenerationBumptest.Which issue(s) this PR fixes:
N/A
Does this PR introduce a user-facing change?: