Add value to configure control plane load balancer allow list #343
Conversation
|
Skipping CI for Draft Pull Request. |
fiunchinho
force-pushed
the
cp-lb-allow-list
branch
from
e4809a6 to
1763b00
Compare
fiunchinho
force-pushed
the
cp-lb-allow-list
branch
from
190ce05 to
3e6444a
Compare
|
/run cluster-test-suites |
cluster-test-suites
📋 View full results in Tekton Dashboard Rerun trigger: |
|
I think schemadocs needs updating, too |
fiunchinho
force-pushed
the
cp-lb-allow-list
branch
from
695bdc6 to
b1cc9a5
Compare
|
The e2e tests on this PR will fail until CAPA |
| fromPort: 6443 | ||
| toPort: 6443 | ||
| cidrBlocks: | ||
| {{- toYaml .Values.controlPlane.allowList | nindent 6 }} |
There was a problem hiding this comment.
Indentation needs to match the existing list item for the Kubernetes API, so shouldn't it be nindent 4?
There was a problem hiding this comment.
If I use nindent 4 it would look like this, and it wouldn't work
- description: "Kubernetes API"
protocol: tcp
fromPort: 6443
toPort: 6443
cidrBlocks:
- 1.2.3.4/32
There was a problem hiding this comment.
See #343 (comment) – I understood the goal now
CHANGELOG.md
Outdated
| @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
|
|
|||
| ## [Unreleased] | |||
|
|
|||
| ### Added | |||
|
|
|||
| - Add `controlPlane.allowList` to configure control plane load balancer ingress rules. | |||
There was a problem hiding this comment.
The name is quite unclear. What about controlPlane.loadBalancerIngressRules which matches CAPA naming? Also, it looks like the default value ["0.0.0.0/0"] shouldn't work because []IngressRule is expected, not []string.
The description shouldn't say anything about IPv4. The IngressRule type supports IPv6 as well.
Also, should we move this into .Values.providerSpecific?
There was a problem hiding this comment.
We are not exposing CAPA values directly. We are only exposing the cidrBlock field of the IngressRule, so it expects a string.
The helm value we expose it's not AWS specific. I don't know how firewall rules are managed on other providers, but I don't see why we couldn't use the same helm value for all providers, just used differently depending on the provider. What do you think?
There was a problem hiding this comment.
Regarding IPv4, we are currently only setting cidrBlocks field, which is IPv4. I don't know if we support clusters using IPv6 at this point.
There was a problem hiding this comment.
I was confused by
cidrBlocks:
{{- toYaml .Values.controlPlane.loadBalancerIngressRules | ... }}
because the name controlPlane.loadBalancerIngressRules reads as if users can pass additional rules. Therefore I commented as if that was the goal.
However, what you currently implement is rather something like controlPlane.loadBalancerIngressAllowCidrBlocks, right? I'm fine to only allow configuring IPv4 blocks, defaulting to 0.0.0.0/0, but then we need to give the value a good name. We can still migrate to supporting the configuration of full rules later.
fiunchinho
force-pushed
the
cp-lb-allow-list
branch
from
d3f1e2f to
dc0d93d
Compare
fiunchinho
force-pushed
the
cp-lb-allow-list
branch
from
dc0d93d to
c1c1eec
Compare
|
/run cluster-test-suites |
(helm/cluster-aws/ci/test-wc-minimal-values.yaml) rendered manifest diff/spec/controlPlaneLoadBalancer (AWSCluster/org-giantswarm/test-wc-minimal)
+ one map entry added:
ingressRules:
- description: "Kubernetes API"
protocol: tcp
fromPort: 6443
toPort: 6443
cidrBlocks:
- 0.0.0.0/0
|
cluster-test-suites
📋 View full results in Tekton Dashboard Rerun trigger: |
What this PR does / why we need it
Towards giantswarm/roadmap#2351
Checklist