Derive the load balancer scheme from what the controller decides it with - #247
Merged
Conversation
`inject-adopt-lb-subnets` injects private or public subnet ids according to the
scheme it believes a load balancer will have. It read one annotation per object
kind and defaulted to internal, which made that belief a pattern rather than a
derivation — and the AWS Load Balancer Controller decides the same thing from
several inputs, in an order, none of which the policy consulted past the first.
──────────────── What the controller actually decides with ────────────────
Read out of the controller source at the version the chart pin installs, not
from memory:
Service scheme aws-load-balancer-scheme
→ aws-load-balancer-internal (a bool; "false" means
INTERNET-FACING)
→ the scheme of a load balancer that already exists in AWS
→ --default-load-balancer-scheme
Ingress scheme IngressClassParams.spec.scheme, for any member of the
Ingress group
→ alb.ingress.kubernetes.io/scheme, for any member
→ --default-load-balancer-scheme
Whose Service spec.loadBalancerClass equal to the controller's class
→ aws-load-balancer-type: nlb-ip
→ aws-load-balancer-type: external with an
aws-load-balancer-nlb-target-type
The comparison the runtime makes is the comparison the policy now makes.
──────────────── What the policy does ────────────────
The Service scheme is resolved in that order, so a Service carrying only the
legacy spelling is internet-facing here as it is to the controller. It used to
read as the default and take the private-subnet list for a load balancer the
controller puts on public subnets.
The Service population is narrowed to Services the controller provisions. A
plain type=LoadBalancer Service is not one: the chart sets
enableServiceMutatorWebhook: false, so nothing stamps a loadBalancerClass on a
Service that does not ask for it, and the plain shape belongs to the in-tree
cloud provider, which builds a Classic Load Balancer and reads neither the
scheme annotation nor the subnets annotation this policy writes. The catalog was
writing to nobody, and the test fixture asserted that as coverage.
An Ingress naming a group is left alone. One load balancer serves every member
and one scheme is decided across all of them, so the object being admitted does
not carry the answer even though it looks like it does.
──────────────── Why a gate and not a longer list ────────────────
Adding the legacy annotation fixes the instance. It does nothing about the next
spelling, and the next spelling is the one nobody adds. So
scripts/check-lb-scheme-inputs.py derives the population from the code that
decides it: it reads `buildLoadBalancerScheme` (Service and Ingress) and
`IsServiceSupported`, follows the functions they call within their own files, and
requires every symbol those bodies consult to be accounted for in
scripts/lb-scheme-inputs.json — READ, with a string the policy must contain;
UNREAD, with the reason it cannot be consulted; or PLUMBING, with the reason it
decides nothing. A symbol that is none of those fails --sync, so a new deciding
input cannot be recorded without somebody choosing what to do about it.
Two are recorded UNREAD rather than closed, each with its reason.
IngressClassParams.spec.scheme needs two cluster lookups, and Kyverno evaluates a
rule's context before its preconditions — so they would run on every Ingress
admission on every cluster in the fleet, adopt or not, for a resource Kyverno's
admission controller is not granted. The scheme of an already-existing load
balancer is read from the AWS API and is not in an admission request at all.
Both leave the same loud failure at provisioning that an adopt cluster had
before any injection existed; neither is now something nobody wrote down.
The record is keyed on the chart pin, so moving the controller makes it stale and
fails the blocking gate on the pull request that moves it. The scheduled workflow
re-derives from source and re-renders the chart, because a controller can grow an
input between bumps, and because the policy's own constants — the load balancer
class, the two type values, the default scheme — are held equal to the flags the
chart actually renders rather than assumed.
──────────────── A false green in the test tool ────────────────
`kyverno test` reports a policy the engine REJECTS as "Invalid Policy" and then
skips every rule — which is exactly what every row expecting a skip asserts. The
CLI counts those rows as passes, so a variable expression Kyverno will not accept
printed the same summary as a suite that evaluated everything. Only rows
asserting a patched resource noticed. scripts/kyverno-test.sh now fails on any
such row, alongside its existing floor on tests executed.
──────────────── Proof ────────────────
Twelve adopt-mode cases, ordered as the controller's own decision procedure:
whether the object is one it provisions for, then which scheme it computes.
Reverting each behaviour in turn — the legacy spelling unread, the legacy flag
consulted ahead of the newer one, ownership narrowed, ownership dropped, a
grouped Ingress treated as carrying its own scheme — fails the case that names
it. Forty-seven unit tests cover the Go reading and both verdicts, and twelve
mutants of the gate were each killed by the test that names the behaviour; one
survivor found a containment test reading `aws-load-balancer-internal` as present
inside `aws-load-balancer-internal-renamed`, now a whole-token match.
Co-authored-by: stxkxsbot <275011021+stxkxsbot@users.noreply.github.com>
CI Results
All checks passed. |
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.
Closes #135.
The premise, checked against the merged tree
Both gaps the issue names still hold at the pinned controller (chart
3.5.0, appVersionv3.5.0), read out of the source rather than assumed:service.beta.kubernetes.io/aws-load-balancer-internalis still honoured and still sits ahead of the default."false"means internet-facing.IngressClassParams.spec.schemestill overrides the object annotation entirely.Reading
buildLoadBalancerSchemeandIsServiceSupportedalso turned up three more inputs the issue does not name, and one thing about this fleet that changes what the policy was doing at all.What the controller actually decides with
aws-load-balancer-scheme→aws-load-balancer-internal(bool) → the scheme of an existing LB, read from AWS →--default-load-balancer-schemeIngressClassParams.spec.scheme(any group member) →alb.ingress.kubernetes.io/scheme(any member) →--default-load-balancer-scheme; members disagreeing is an errorspec.loadBalancerClass== the controller's class →aws-load-balancer-type: nlb-ip→aws-load-balancer-type: externaland anaws-load-balancer-nlb-target-typeThe abort comparison the runtime makes is the comparison the policy now makes: a scheme is resolved down that chain, not read from one annotation with
internalas the fallback for everything unrecognised.The one the issue did not have
enableServiceMutatorWebhook: falsein this catalog's values, and the chart renders no service mutating webhook. So nothing stamps aloadBalancerClassonto a plaintype: LoadBalancerService — it belongs to the in-tree cloud provider, which builds a Classic Load Balancer and reads neitheraws-load-balancer-schemenoraws-load-balancer-subnets. The policy was writing an annotation nobody reads onto the most common Service shape on the fleet, andpatched/svc-plain.yamlasserted that inert mutation as coverage.The Service population is now the set the controller provisions for.
svc-plainis askip, and the reason is in the policy.Why a gate rather than a longer list
Adding the second annotation fixes the instance, not the next one.
scripts/check-lb-scheme-inputs.pyderives the population from the code that decides it — it reads the two entry functions at the pinned appVersion, follows the calls they make within their own files, and requires every symbol those bodies consult to be accounted for inscripts/lb-scheme-inputs.json:A symbol that is none of those fails
--sync. 22 symbols today: 10 read, 4 unread, 8 plumbing.Two are recorded unread rather than closed, each with its reason:
IngressClassParams.spec.scheme— two cluster lookups, and Kyverno evaluates a rule'scontextbefore its preconditions, so they would run on every Ingress admission on every cluster in the fleet, adopt or not, for a resource Kyverno's admission controller is not granted (its core ClusterRole reaches configmaps and namespaces, nothing innetworking.k8s.ioorelbv2.k8s.aws).Both leave the loud failure at provisioning that an adopt cluster had before any injection existed. Neither is now something nobody wrote down.
The record is keyed on the chart pin, so moving the controller makes it stale and fails the blocking gate on the PR that moves it.
--livere-derives from source and re-renders the chart on the schedule — a controller can grow an input between bumps, and the policy's own constants (the LB class, the two type values, the default scheme) are held equal to the flags the chart renders rather than assumed.A false green in the test tool
kyverno testreports a policy the engine rejects asInvalid Policyand then skips every rule — which is exactly what every row expecting a skip asserts. The CLI counts those as passes: a variable expression Kyverno will not accept printed29 tests passed and 0 tests failed. Only rows asserting a patched resource noticed.scripts/kyverno-test.shnow fails on any such row.Found by mutation, not by reading.
Proof
Twelve adopt-mode cases ordered as the controller's own procedure. Reverting each behaviour fails the case that names it:
svc-legacy-internal-falsesvc-scheme-beats-legacyloadBalancerClassalonesvc-plaining-grouped47 unit tests over the Go reading and both verdicts; 12 gate mutants each killed by the test that names the behaviour. One survivor found a containment check reading
aws-load-balancer-internalas present insideaws-load-balancer-internal-renamed— now a whole-token match.task validategreen,controls.py22/22,reverify-gates.sh60/0,empty-corpus.py35/35,kyverno test29/0, ruff + mypy + yamllint + zizmor clean.