fix: never report a gateway with a dropped listener - #365
Open
ecv wants to merge 2 commits into
Open
Conversation
A listener the user asked for could be left out of the downstream gateway while the upstream gateway kept reporting Programmed=True. The flag that guards against this was re-derived from hostname claim status, so it only covered one of the three ways a listener can be withheld: a listener with an unset hostname vanished from the edge silently, and a listener held back by an unusable certificate was exempt on purpose. Derive the flag by comparing the listener set actually built against the upstream spec instead. A listener withheld for any reason, including a reason added later, now flips the aggregate Programmed condition. The certificate carve-out is removed: a listener waiting on a certificate is still missing from the edge, and the conditions it reports itself through are not visible to alerting. It is reported as Pending rather than ListenersNotValid so a transient issuance stays distinguishable from a listener that was genuinely dropped. This reverses the user-visible effect of 914467e for gateways waiting on a certificate, which will now report Programmed=False until the certificate is usable. HTTPProxy mirrors the gateway's Programmed condition, so those proxies report unprogrammed for the same window. Key changes: - Add summarizeDroppedListeners, comparing built downstream listeners against the upstream spec by name - Split the Programmed reason: Pending when every dropped listener is cert-withheld, ListenersNotValid otherwise - Log and skip a listener with an unset hostname explicitly instead of falling out of an unguarded nil check - Cover both drops with unit tests, plus an invariant asserting a gateway carrying fewer listeners than its spec never reports Programmed=True
ecv
marked this pull request as ready for review
August 13, 2026 03:16
Contributor
Author
|
@scotwells this screws with #361 but my brain isnt comprehending how to reconcile |
The e2e environment maps no cluster issuer, so the `auto` issuer the defaulting webhook stamps on the injected default-https listener resolves to nothing unless the gateway happens to declare a real issuer on another listener. No Certificate is minted, the listener is withheld from the downstream gateway, and it stays withheld for the life of the test. Production maps `auto` to its ACME issuer, so this gap is an environment fidelity problem rather than a behaviour the suite should encode. Add a CA-backed issuer to the downstream environment and map `auto` to it. The billing scenario is the one that exposed this: its gateway declares only an HTTP listener, so nothing existed for `auto` to inherit from, and its assertion that the gateway reports Programmed=True began failing once a withheld listener stopped being reported as programmed. Key changes: - Add the e2e-gateway-http self-signed root, CA certificate, and CA-backed ClusterIssuer to the downstream environment - Map the auto issuer sentinel to it in the e2e operator config
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A gateway could lose a listener on its way to the edge and still report
Programmed=True. ThelistenersDroppedflag was re-derived from hostname claim status rather than from the listener set actually built, so it covered only one of the three ways a listener gets withheld ingetDesiredDownstreamGateway:Programmedbeforegateway_controller.go:705ListenersNotValidif l.Hostname != nilwrapperListenersNotValidgateway_controller.go:713PendingsummarizeDroppedListenersnow compares the built downstream listeners against the upstream spec by name. A listener withheld for a reason added later is covered without anyone extending the check.The certificate carve-out is removed
#363 asks for this to be decided rather than assumed. A listener waiting on a certificate is still missing from the edge, and the certificate conditions it was said to report itself through are not visible to alerting. The metrics that would have carried it have never reached Prometheus (#359).
It reports
Pendingrather thanListenersNotValid, so a transient issuance stays distinguishable from a listener that was genuinely dropped.This reverses the user-visible effect of 914467e (#361), merged yesterday. A gateway waiting on a certificate reports
Programmed=Falseagain until that certificate is usable. Two consequences to weigh before merge:httpproxy_controller.go:411mirrors the gateway'sProgrammedcondition, so an HTTPProxy on a custom hostname reports unprogrammed for the issuance window too.gateway_controller.go:1211requeues every 5s while unprogrammed, so a permanently failing certificate becomes a standing 5s reconcile loop for that gateway.Test plan
TestReconcileGatewayStatus_DroppedListenerIsNotProgrammeddrives the real chain, fromgetDesiredDownstreamGatewaythroughsummarizeDroppedListenerstoreconcileGatewayStatus, across all three withholding reasons. Every case also asserts that a gateway carrying fewer listeners than its spec never reportsProgrammed=True.Patching the summary back to the pre-fix derivation fails the unset-hostname and certificate cases, so the coverage proves the gap rather than restating it.
TestGetDesiredDownstreamGateway_NilHostnameSkippedcharacterises the drop itself.make testandmake lintpass.Closes #363
Fixes #235
The e2e environment needed the
autoissuer resolvedbilling-http-meteringfailed on the first push, and the failure was real. Its gateway declares only an HTTP listener, so the injecteddefault-httpshad no real issuer to inherit and no Certificate was minted. That listener has been withheld from the downstream gateway for the life of the scenario, and the suite passed because the gateway reportedProgrammed=Trueanyway.Production maps
autoto its ACME issuer and also setsdefaultListenerTLSSecretName, so neither production nor staging can reach this state. The e2e environment set neither. The second commit adds a CA-backede2e-gateway-httpClusterIssuer toconfig/e2e-downstreamand mapsautoto it, which is the environment fidelity fix rather than a change to what the scenario asserts.