Support configuring loadBalancerIP in envoy svc #2017
Conversation
arkodg
requested review from
a team,
qicz,
LanceEa and
chauhanshubham
and removed request for
a team
Codecov Report
@@ Coverage Diff @@
## main #2017 +/- ##
==========================================
- Coverage 65.78% 65.66% -0.12%
==========================================
Files 93 93
Lines 13589 13599 +10
==========================================
- Hits 8939 8930 -9
- Misses 4104 4119 +15
- Partials 546 550 +4
|
| // LoadBalancerIP defines the IP Address of the underlying load balancer service. This field | ||
| // may be ignored if the load balancer provider does not support this feature. | ||
| // This field has been Deprecated, but it is still used for set the IP Address in some cloud | ||
| // providers such as GCP. |
There was a problem hiding this comment.
it's deprecated, maybe use annotations to support this?
There was a problem hiding this comment.
Not possible for GCP, the linked issue has all the details:)
There was a problem hiding this comment.
what will happen if someone config this by mistake on non-GCP?
There was a problem hiding this comment.
#1841 (comment)
has the details, looks like all still support it except AWS and I'm unsure what happens when it is set, any idea @LanceEa ?
There was a problem hiding this comment.
Thanks @LanceEa can you please review the API doc string in this PR
There was a problem hiding this comment.
My guess is either it is ignored or like GCP the admission controller would reject the fields it doesn't support.
There was a problem hiding this comment.
What I'm thinking is should EG auto-detect cloud provider, and reject/ignore this field if it's non-GCP?
There was a problem hiding this comment.
@zirain in the process of improving user experience, we will end up making it worse because this field is still supported in Azure and some AWS controllers, so im a -1 on any auto detect cloud logic
There was a problem hiding this comment.
@arkodg - I agree. At that point I think we would be over engineering it and would end up with more pain maintaining it. Instead I think leaning on documentation is a better approach.
api/v1alpha1/validation/validate.go
Outdated
| errs = append(errs, fmt.Errorf("loadBalancerIP can only be set for %v type", egv1a1.ServiceTypeLoadBalancer)) | ||
| } | ||
|
|
||
| if net.ParseIP(*serviceLoadBalancerIP) == nil { |
There was a problem hiding this comment.
Not a blocker for me but ParseIP will also parse an IPV6 address successfully so it could be nice to give the user earlier feedback.
But realistically most users setting this field have read the Kubernetes docs or can at least search them easily.
There was a problem hiding this comment.
thanks for flagging this, adding a check to only allow ipv4
Fixes: envoyproxy#1841 Signed-off-by: Arko Dasgupta <[email protected]>
Signed-off-by: Arko Dasgupta <[email protected]>
Signed-off-by: Arko Dasgupta <[email protected]>
Fixes: #1841