Reimplement getting load balancer source ranges #1896
Conversation
k8s-ci-robot
added
cncf-cla: yes
panslava
force-pushed
the
rework-source-ranges
branch
from
d6c1beb
to
2e39784
Compare
|
/lgtm |
|
@mmamczur: changing LGTM is restricted to collaborators In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
- Introduce custom error type, to identify user errors - Cover with unit tests - Needed to add IPv6 support later (in next PR)
panslava
force-pushed
the
rework-source-ranges
branch
from
2e39784
to
3ddbfec
Compare
| @@ -482,15 +482,15 @@ func (l4 *L4) ensureIPv4NodesFirewall(nodeNames []string, ipAddress string, resu | |||
| }() | |||
|
|
|||
| // ensure firewalls | |||
| sourceRanges, err := helpers.GetLoadBalancerSourceRanges(l4.Service) | |||
There was a problem hiding this comment.
IMHO, Ideally we should remove all these helper dependencies from the cloud-provider
grep -r "k8s.io/cloud-provider/service/helpers" pkg/
pkg/l4lb/l4netlbcontroller_test.go: "k8s.io/cloud-provider/service/helpers"
pkg/l4lb/l4lbcommon.go: "k8s.io/cloud-provider/service/helpers"
pkg/utils/patch/patch.go: svchelpers "k8s.io/cloud-provider/service/helpers"
pkg/neg/controller.go: "k8s.io/cloud-provider/service/helpers"
pkg/loadbalancers/l4netlb_test.go: servicehelper "k8s.io/cloud-provider/service/helpers"
pkg/loadbalancers/l4netlb.go: "k8s.io/cloud-provider/service/helpers"
pkg/loadbalancers/l4.go: "k8s.io/cloud-provider/service/helpers"
pkg/loadbalancers/l4_test.go: servicehelper "k8s.io/cloud-provider/service/helpers"
pkg/healthchecksl4/healthchecksl4.go: "k8s.io/cloud-provider/service/helpers"
cc: @swetharepakula
| }, | ||
| { | ||
| desc: "Should parse ranges from spec", | ||
| specRanges: []string{" 192.168.0.1/10", " 132.8.0.1/8 "}, |
There was a problem hiding this comment.
this was a kubernetes API validation mistake, but the ecosystem moved on allowing them
kubernetes/kubernetes#94107
so it seems correct to allow it here too
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aojea, mmamczur, panslava The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/assign cezarygerard