Skip to content
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.

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

conformance: gateway tests expecting wrong ListenerConditionReason #1297

Closed
mlavacca opened this issue Aug 8, 2022 · 4 comments · Fixed by #1305
Closed

conformance: gateway tests expecting wrong ListenerConditionReason #1297

mlavacca opened this issue Aug 8, 2022 · 4 comments · Fixed by #1305
Assignees
Labels
area/conformance kind/bug Categorizes issue or PR as related to a bug.

Comments

@mlavacca
Copy link
Member

mlavacca commented Aug 8, 2022

What happened:

In the conformance tests for the gateway there are two different tests [GatewaySecretInvalidReferenceGrant, GatewaySecretMissingReferenceGrant] that expect a listener to have the ReasonCondition as ListenerReasonInvalidCertificateRef. Both tests create a Gateway with a listener that refers to a secret in another namespace and there is no ReferenceGrant granting that reference.

For what concerns theListenerReasonInvalidCertificateRef, the godoc says that:

// This reason is used with the "ResolvedRefs" condition when the
// Listener has a TLS configuration with at least one TLS CertificateRef
// that is invalid or cannot be resolved.

while for what concerns theListenerReasonRefNotPermitted , the godoc says that:

// This reason is used with the "ResolvedRefs" condition when
// one of the Listener's Routes has a BackendRef to an object in
// another namespace, where the object in the other namespace does
// not have a ReferenceGrant explicitly allowing the reference.

Since the listener cannot reference the secret as there is no ReferenceGrant that allows that reference, the right reason to use for the ListenerCondition is ListenerReasonRefNotPermitted instead of ListenerReasonInvalidCertificateRef.

@mlavacca mlavacca added the kind/bug Categorizes issue or PR as related to a bug. label Aug 8, 2022
@mlavacca
Copy link
Member Author

mlavacca commented Aug 8, 2022

I'd be glad to work on a fix for this issue. If this is effectively a bug, I ask to be assigned to fix it.

@robscott
Copy link
Member

robscott commented Aug 8, 2022

I think you're right, thanks for catching this @mlavacca!

/assign @mlavacca

@skriss
Copy link
Contributor

skriss commented Aug 23, 2022

Just coming back to this and while I'm fine with using the RefNotPermitted reason for this scenario, I do think the API docs need some modifications to cover this more explicitly, since as pointed out above, currently the RefNotPermitted reason only talks about routes and backends (nothing to do with listeners and secrets). It might be worth also clarifying for InvalidCertificateRef, exactly what an "invalid" certificate ref is, and that it excludes this case of a disallowed cross-namespace reference.

@robscott
Copy link
Member

Good call @skriss, filed #1362 to track that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/conformance kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants