feat: configurable data-plane Service shape via GatewayClassParameters (#70)#71
Open
oysteins10 wants to merge 17 commits into
Open
feat: configurable data-plane Service shape via GatewayClassParameters (#70)#71oysteins10 wants to merge 17 commits into
oysteins10 wants to merge 17 commits into
Conversation
Pure function resolving GatewayClassParameters.spec.service + Gateway.spec.infrastructure into a fully-defaulted ResolvedServiceConfig. Defaults Type to LoadBalancer when omitted; overlays Gateway-level annotations/labels on top of class-level defaults without aliasing inputs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…l merger Implements the sentinel-based merge function that writes desired keys onto the existing map, prunes previously-managed keys no longer in desired, leaves unmanaged external keys untouched, and enforces protected-key policy. Eight tests cover first-apply, drift-add, drift-remove, external key preservation, empty desired pruning, sorted sentinel, protected key dropping, and nil existing map handling. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wire the ResolvedServiceConfig produced by resolveServiceConfig through buildService: service type, LoadBalancer knobs, ExternalTrafficPolicy, and user annotations/labels (via managed-keys sentinel). Selector is built from controller-managed labels only so user labels can never break pod selection. Update all five call sites (4 in resources_test, 1 in gateway_controller_test) and add a placeholder in reconcileResources (Task 6 replaces with real resolver invocation). Add controllerManagedLabelKeys helper and 5 new TestBuildService_* tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ce shape Extend needsServiceUpdate to compare Type, LoadBalancerClass, LoadBalancerSourceRanges, ExternalTrafficPolicy, and managed annotations/labels (via sentinel) in addition to ports. Add stringPtrEqual, stringSliceEqual, and managedMapEqual helpers. Replace the Service update branch in reconcileResource to apply all operator-owned fields when drift is detected, using desiredAnnotationsFromSentinel, desiredLabelsFromSentinel, and buildLabelsForService helpers to preserve cloud-controller-managed state during updates. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Hoist params out of the short-variable-declaration if-block so it is in scope at the buildService call site, then replace the hardcoded LoadBalancer default with resolveServiceConfig(gateway, params). The resolver is nil-safe, so gateways without GatewayClassParameters continue to produce an identical LoadBalancer Service. Add TestInfraHash_ServiceConfigDoesNotAffectHash to pin the design invariant that Service shape (Type, annotations, labels, LB knobs) is deliberately excluded from InfrastructureConfig and must never trigger pod restarts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Author
Conformance run (local, k3d)Ran
Profile: 🤖 Generated with Claude Code |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #71 +/- ##
==========================================
+ Coverage 68.02% 68.77% +0.74%
==========================================
Files 39 40 +1
Lines 6096 6267 +171
==========================================
+ Hits 4147 4310 +163
- Misses 1618 1620 +2
- Partials 331 337 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Three findings from review of the configurable Service shape feature. 1. Labels sentinel was written into .metadata.labels with a comma-separated key list. Commas and slashes are not valid in Kubernetes label values, so the API server rejected any Service produced by a config with >= 2 user labels or any DNS-prefixed label key (e.g. app.kubernetes.io/component). The Gateway reconcile failed every loop. Both sentinels now live in .metadata.annotations; the user-supplied label values still live in .Labels and are looked up via the annotation-borne key list. 2. The Service update path replaced Spec.Ports wholesale with the desired slice, which has NodePort: 0. On every reconcile for a NodePort/LoadBalancer Service the API server would reallocate node ports, disconnecting clients connected via the old allocation. Added mergeServicePorts to carry existing NodePort allocations over by port name. 3. Protected-label collisions were silently dropped despite the API doc claiming a log warning. buildService now logs a Warn entry per collision, naming the gateway and the dropped key. Tests: - TestBuildService_MultipleLabels_SentinelIsValidLabelValue - TestServiceShape_MultipleLabels_Accepted_Envtest (would fail with API-server "Invalid value" prior to the fix) - TestMergeServicePorts_* (preserve, desired-wins, renamed) Co-Authored-By: Claude <noreply@anthropic.com>
Author
|
@perbu hva er status her? |
Contributor
|
@oysteins10 I've cleaned up the PR, it is ready to merged. This is a not a bug fix, so it will require a major release. I have a couple of bugs I wanna squash in this cycle before I merge this. |
Author
|
Makes sense, crd changes should be major 👍 |
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
2 participants
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.
Summary
Fixes #70 — exposes the data-plane Service shape through
GatewayClassParameters.spec.service, unblocking gateway-behind-gateway, bare-metal MetalLB, and internal-cache deployment patterns.ServiceConfigCRD type with six fields:type,annotations,labels,loadBalancerClass,loadBalancerSourceRanges,externalTrafficPolicyloadBalancerClassonly withtype: LoadBalancer)Gateway.spec.infrastructurefield; other knobs stay GatewayClass-levelvarnish.io/managed-{annotations,labels}) preserves annotations added by cloud controllers (AWS LB Controller, MetalLB, etc.) across reconcilesservicefield produces an identicalType: LoadBalancerService to beforeArchitecture
resolveServiceConfig(gateway, params)layers defaults → GatewayClass config →Gateway.spec.infrastructureoverlay for labels/annotationsbuildServiceconsumes the resolved config;Spec.Selectoris built only from controller-managed labels (user labels can never break pod selection)needsServiceUpdateextended to detect drift in all owned fields plus the managed subset of annotations/labels; cloud-controller-added keys are ignoredTest plan
needsServiceUpdatecases, 1 controller-managed-labels invariant, 1 infra-hash regression (reflection)make build-go,make test-go,make test-envtest,make vet,make verify-manifestsall greenmake test-conformance) not run — requires a live cluster. Recommend running before merge.LoadBalancer → ClusterIP → LoadBalancercycle preservesclusterIPand re-populatesloadBalancer.ingressNotes
make staticcheckfailures (deprecation warnings forRequeueandfake.NewSimpleClientset) are in untouched files; none of the new/modified files trigger any lint findings.🤖 Generated with Claude Code