[feat] add support for reserved IPv4 range in NodeBalancer backend allocation - #632
vsilva-akamai wants to merge 5 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #632 +/- ##
==========================================
- Coverage 75.91% 75.15% -0.77%
==========================================
Files 18 19 +1
Lines 2811 2938 +127
==========================================
+ Hits 2134 2208 +74
- Misses 464 493 +29
- Partials 213 237 +24 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| {{- else if hasKey .Values "configureCloudRoutes" }} | ||
| - --configure-cloud-routes={{ .Values.configureCloudRoutes }} |
There was a problem hiding this comment.
Doesn't this make it so --configure-cloud-routes is passed twice if values has routeController.configureCloudRoutes and configureCloudRoutes set?
If we want to add support to configure cloud routes outside of enabling the route controller, we might want to deprecate/remove the option from the route controller itself.
There was a problem hiding this comment.
I teested this with both keys set to opposite values to check which one would be the winner, only one --configure-cloud-routes renders, and it's the routeController one.
L146 is the else if arm of the {{- if .Values.routeController }} at L124, so the two branches don't both fire. Letting the top-level false pass would render --enable-route-controller=true --configure-cloud-routes=false, which stops the route controller from start.
There was a problem hiding this comment.
$ helm template ccm ./deploy/chart \
--set configureCloudRoutes=false \
--set routeController.configureCloudRoutes=true \
--set routeController.vpcNames=vpc1 --set routeController.clusterCIDR=10.0.0.0/8
name: ccm-linode
args:
- --leader-elect-resource-lock=leases
- --v=3
- --secure-port=10253
- --webhook-secure-port=0
- --enable-route-controller=true
- --configure-cloud-routes=true
- --vpc-names=vpc1
- --cluster-cidr=10.0.0.0/8
There was a problem hiding this comment.
I did some digging and long story short, I believe we actually don’t want/need to have configureCloudRoutes at all in the chart.
Instead, the template should always pass - --configure-cloud-routes=true unless the new nodeBalancerBackendIPv4ReservedRange is set, in which case we’ll want the value to be false. If I’m understanding correctly, this new feature needs configuration of cloud routes to be disabled, but correct me if I’m wrong.
Upstream kube-controller-manager is defaulting configure-cloud-routes to true so it’s the same behavior whether we specify true or just omit the flag entirely.
To summarize, I think off of main this is the only diff needed on this file (and no changes on values.yaml vs what's on main):
diff --git a/deploy/chart/templates/daemonset.yaml b/deploy/chart/templates/daemonset.yaml
index 2cca11c..1bf10cb 100644
--- a/deploy/chart/templates/daemonset.yaml
+++ b/deploy/chart/templates/daemonset.yaml
@@ -135,7 +135,12 @@ spec:
{{- if not $clusterCIDR }}
{{- fail "clusterCIDR is required if route-controller is enabled" }}
{{- end }}
+ {{- if .Values.nodeBalancerBackendIPv4ReservedRange }}
+ - --nodebalancer-backend-ipv4-reserved-range={{ .Values.nodeBalancerBackendIPv4ReservedRange }}
+ - --configure-cloud-routes=false
+ {{- else }}
- --configure-cloud-routes={{ default true .Values.routeController.configureCloudRoutes }}
+ {{- end }}
{{- with .Values.routeController.routeReconciliationPeriod }}
- --route-reconciliation-period={{ . }}
{{- end }}
I can do the change in a follow-up PR to update the chart to remove routeController.configureCloudRoutes and just have it explicitly set to true in line with what's already happening but we shouldn't need a new top-level configureCloudRoutes field.
|
Can you document the new feature on |
I updated the docs as requested, and now I'm working on getting the Chainsaw tests implemented and working. |
Tests implemented. |
Summary
--nodebalancer-backend-ipv4-reserved-rangeand the matching Helm value for reserving the highest/30in a configured NodeBalancer backend subnet./30below the reserved range by reading the subnet's existing NodeBalancer assignments.Rationale
Reserving the highest
/30prevents CCM-managed NodeBalancer backend allocation from consuming an address range that must remain available for networking.Validation
go test ./cloud/linode -count=1make helm-templateGeneral
Pull Request Guidelines