conformance: add a conformance test for BackendTLSPolicy

[Thealisyed]

Changing a ConfigMap content should be reconciled by the controller.

What type of PR is this?

/kind test
/area conformance-test

What this PR does / why we need it:

This PR adds a conformance test for BackendTLSPolicy so that when a ConfigMap contents are changed, it should be reconciled by the controller.

Which issue(s) this PR fixes:

Fixes #4338

Does this PR introduce a user-facing change?:

Adds a conformance test for BackendTLSPolicy so that when a ConfigMap contents are changed, it should be reconciled by the controller.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: Thealisyed / name: Ali Syed (c9f37ef)

[k8s-ci-robot]

Hi @Thealisyed. Thanks for your PR.

I'm waiting for a github.com 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.

Details

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.

[rikatz]

/ok-to-test

[rikatz]

/cc
/assign

[rikatz]

overall the test looks good to me, I was able to test it on envoygateway with almost full success (but the returned reason from resolvedRefs that seems to be wrong).

I will leave this for someone else to also review and approve, as me and Ali work for the same company!

[snorwin]

/cc @snorwin

[snorwin]

@rikatz I have no objection to adding conformance tests that verify that a parent resource is reconciled when a referenced resource is created, updated, or deleted (or when a ReferenceGrant affecting it is created, updated, or deleted).

However, I would expect this to be done consistently for all references, not just for BackendTLSPolicy, and in a way that avoids duplicating existing conformance tests. For that reason, I would limit such tests to control-plane behavior, i.e., verifying that updates are reflected in the parent resource’s status.

In addition, I would suggest first updating the implementation guide to clearly state that creating, updating, or deleting any referenced resource must trigger a reconcile, and to clarify the semantics around ReferenceGrants. From there, issues could be opened to add conformance tests covering all reference types. In general, this should be considered best practice when writing Kubernetes controllers. If specific implementations do not follow these practices, I would prefer opening issues in those repositories rather than over-complicating the conformance tests.

That said, I don’t think the current conformance suite, given its heavy use of shared resources, is ready for the kind of scenario-based tests proposed in this PR. The tests run in parallel and rely on many shared resources, which MUST NOT be modified, as this would interfere with other tests. This would likely lead to significant duplication just to isolate scenarios.
Additionally, test duration would increase substantially if we required dedicated infrastructure for these scenarios, which would negatively impact CI pipeline runtimes across all implementations.

[rikatz]

The tests run in parallel and rely on many shared resources, which MUST NOT be modified, as this would interfere with other tests.

agreed!

In addition, I would suggest first updating the implementation guide to clearly state that creating, updating, or deleting any referenced resource must trigger a reconcile

How is this different today from a Service, as a backendRef on a route, that gets a new endpoint added? Or a certificate on a listener that is updated? Is this just a lack of clarification on implementation guide, or do we miss something else?

Should we have an extra conditions field for the policies that say that any policy implementation MUST have a condition for any referenced resource, and with observedGeneration instead?

Again, my bigger concern here is about behavior. It would sucks to be a user that has everything working, decides upfront to update the certificate on a BTLS Policy CA chain just to figure out that my proxy will never work unless someone restarts the controller.

[snorwin]

Should we have an extra conditions field for the policies that say that any policy implementation MUST have a condition for any referenced resource, and with observedGeneration instead?

This information is usually already reflected in the ResolvedRefs status condition.

Just as an inspirtaion, this is how we typically test whether our controllers correctly reconcile resources, using HTTPRoute as an example:

1. Ensure the initial state, HTTPRoute has Accepted=true and ResolvedRefs=true.
2. Delete the referenced Service, this change should be reflected in the status: ResolvedRefs=false.
3. Re-create the referenced Service, the Route should recover to Accepted=true and ResolvedRefs=true.
4. Update the Service (remove the referenced port), the Route status should again reflect the error: ResolvedRefs=false.
5. Restore the Service to its original state, the Route should return to Accepted=true and ResolvedRefs=true.

While we could also validate the data plane at each step, we consider this unnecessary for reconciliation tests. Configuration changes and errors are already fully surfaced through the control plane, so verifying the control plane status is sufficient for these tests, without additionally checking the data plane.

[rikatz]

besides my comment on the "require" this lgtm and seems better contained.

I will still do some tests locally with envoygw, but will defer for @kl52752 and @snorwin for some final review as well.

[snorwin]

I successfully verified the updated conformance test with the Airlock Microgateway. However, I’ve added a few minor comments.

[rikatz]

@snorwin @kl52752 I did another pass, lgtm besides a nit. I have tested on EnvoyGW and it fails because of the condition. Can you test on Airlock please?

Thanks!

[snorwin]

I was able to successfully run the conformance test with the Airlock Microgateway. I’ll defer the final approval and lgtm to @rikatz.

[rikatz]

thanks for the validation @snorwin
/lgtm
/approve
thanks @Thealisyed

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rikatz, snorwin, Thealisyed

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• conformance/OWNERS [rikatz]

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

[rikatz]

/lgtm
After rebasing to upstream

[kl52752]

Thank for your work :)
/lgtm