Optional loadBalancerIP input in Contour #336
Conversation
BostjanBozic
requested review from
stevesloka and
youngnick
and removed request for
a team
internal/objects/service/service.go
Outdated
| @@ -260,6 +260,9 @@ func DesiredEnvoyService(contour *operatorv1alpha1.Contour) *corev1.Service { | |||
| switch epType { | |||
| case operatorv1alpha1.LoadBalancerServicePublishingType: | |||
| svc.Spec.Type = corev1.ServiceTypeLoadBalancer | |||
| if len(contour.Spec.NetworkPublishing.Envoy.LoadBalancer.ProviderParameters.LoadBalancerIP) > 0 { | |||
There was a problem hiding this comment.
I think a test in service_test.go would be sufficient to ensure we have coverage over this change
There was a problem hiding this comment.
Unit test added with hardcoded IP address.
api/v1alpha1/contour_types.go
Outdated
| @@ -266,6 +266,11 @@ type ProviderLoadBalancerParameters struct { | |||
| // +unionDiscriminator | |||
| // +kubebuilder:default=AWS | |||
| Type LoadBalancerProviderType `json:"type,omitempty"` | |||
|
|
|||
| // loadBalancerIP contains IP for LoadBalancer service type, optional | |||
| // +kubebuilder:validation:Format=ipv4 | |||
There was a problem hiding this comment.
should we allow ipv6 ips as well?
There was a problem hiding this comment.
I was thinking about it, but I am not sure how to handle this. Problem is that of course format can only be specified one, so e.g. I can not do Format=ipv4;ipv6. So I was thinking, either we move everything to validation.go, we have separate field (e.g. LoadBalancerIPv6), or maybe simplest way would be to just use same spec as it is implemented into * corev1.ServiceSpec (json:"loadBalancerIP,omitempty" protobuf:"bytes,8,opt,name=loadBalancerIP").
What do you think, maybe last option would make the most sense as it follows same structure as corev1?
There was a problem hiding this comment.
Maybe we can leave it as IPv4 for now and if there is some requirement for IPv6, we can add support later?
There was a problem hiding this comment.
i would be on the side of letting ipv6 addresses be specified to start (probably via adding to the validation code we have today, probably simple enough to use net.ParseIP), though leaving things restricted and opening it up when needed make sense
I would like to get other @projectcontour/maintainers thoughts on this as well
There was a problem hiding this comment.
I would expect LoadBalancerIP to be a field of LoadBalancerStrategy or ProviderLoadBalancerParameters. @BostjanBozic a few questions:
- Currently
LoadBalancerStrategysupports LB scope. Must a loadbalancer have an external scope to specify an IP? - Do the major cloud providers support specifying a LB IP? From the Service spec: "This feature depends on whether the underlying cloud-provider supports specifying the loadBalancerIP when a load balancer is created. This field will be ignored if the cloud-provider does not support the feature." If this feature is not widely supported, it should be part of
ProviderLoadBalancerParameters.
There was a problem hiding this comment.
@danehans Cheers for feedback. I put that under ProviderLoadBalancerParameters, as I was thinking this is parameter of LB itself, just for naming semantics. If preferred I can move it to LoadBalancerStrategy as well.
Regarding questions:
- As far as I am aware, you can also use this when
externalTrafficPolicy: Local, but you need to provide IP in correct subnet - I did check AWS, Azure and GCP. For Azure and GCP they support explicitly configuring
loadBalancerIP(with Azure you might have to configure annotation in case public IP resource is configured in another resource group - Pass annotations to created resource #327). For AWS, you have to provide annotationservice.beta.kubernetes.io/aws-load-balancer-eip-allocations. Good thing is if cloud provider does not support settingloadBalancerIP, this field will be ignored.
There was a problem hiding this comment.
ProviderLoadBalancerParameters is meant for passing provider-specific LB config and why I'm trying to understand what cloud providers support specifying a LB IP. If the major cloud providers support the ability to specify an IP, then the field should be in LoadBalancerStrategy.
As far as I am aware, you can also use this when externalTrafficPolicy: Local, but you need to provide IP in correct subnet
I'm referring to this scope, which is used to specify an internal or external LB. All major cloud providers support specifying an internal load-balancer, hence why scope is part of LoadBalancerStrategy and not ProviderLoadBalancerParameters. I'm trying to understand how specifying loadBalancerIP will work with scope: internal. Do cloud providers allow an internal LB scope when loadBalancerIP is set? If so, so any limitations exist? For example, cloud provider foo allows loadBalancerIP with internally-scoped LB but the address must be from "10.x.x.x/8".
There was a problem hiding this comment.
It is only Azure and GCP that are currently supporting providing exact IP. In AWS's case, binding existing LB to a service requires annotating the service. So basically following is the statue:
- Azure: you can specify
loadBalancerIPparameter, in case of Public IP resource being in another resource group, you have to annotate service withservice.beta.kubernetes.io/azure-load-balancer-resource-group: myResourceGroup - GCP: you can specify
loadBalancerIPparameter, I think nothing else is required - AWS: specifying
loadBalancerIPparameter has no effect, service needs annotation (service.beta.kubernetes.io/aws-load-balancer-eip-allocations: eipalloc-<xxxxxxxxxxxxxxxxx>)
Regarding the scope, I was only able to test Azure. Using internal scope following results occur:
loadBalancerIPparameter can be configuredloadBalancerIPneeds to be in specific range- when creating Public IP on Azure, this will never be in range required above, hence preconfiguring
loadBalancerIPparameter forInternalscope makes no sense, at least for Azure
There was a problem hiding this comment.
I have used GCP in this way - allocate a regional static IP address, and that set that IP address to the loadBalancerIP in the service, and it works. We do it this way because we have all our networking created in terraform - so we let terraform create the IP addresses, and then also let terraform create the DNS records that depend on those IP addresses. Then when we run our k8s deployment, we get the IP from terraform and plug it into our k8s deploy scripts.
| @@ -112,6 +112,14 @@ func checkServiceHasExternalTrafficPolicy(t *testing.T, svc *corev1.Service, pol | |||
| } | |||
| } | |||
|
|
|||
| func checkServiceHasLoadBalancerIP(t *testing.T, svc *corev1.Service, loadBalancerIp string) { | |||
There was a problem hiding this comment.
added test seems good to me, thanks
@sunjayBhatia That is the confusing part, I actually did a rebase on my branch (as it is stated in dev guide that rebase is required before merge) and those changes crawled in. Am I missing something here/doing it wrong? |
|
@BostjanBozic it looks like you have #317 are part of this PR. Can you please clean this up so the PR is easier to review? |
@danehans Yes, it seems that after rebasing my fork, this mess occurred. Should I just checkout commit that I forked from and provide changes or actually just remove changes in my latest commit (I am worried that this PR would then overwrite work that was already done)? |
|
@BostjanBozic give these steps a try:
In the future, it's a good idea to create a topic branch when developing, instead of using "main/master". Check out the Kubernetes GitHub workflow guide for additional details. |
|
@BostjanBozic xref #327 (comment) for my thoughts on next steps to add this feature. I'm happy to help you through the process. |
|
@danehans Sounds good, I guess I will wait until we decide on the structure and I can review the implementation. As mentioned in #327 it would make sense to provide cloudProvider type under Discussion docs available here. Also thanks again for helping me reset changes on my PR, it did the trick :) |
|
I have set things up based on doc, just so we can take a look how this would work out. |
|
@projectcontour/maintainers @sunjayBhatia @danehans Once you have time, can you check the changes and let me know if that sort of implementation would be suitable or would we change the structure? |
Codecov Report
@@ Coverage Diff @@
## main #336 +/- ##
==========================================
+ Coverage 58.18% 58.75% +0.56%
==========================================
Files 19 19
Lines 1801 1862 +61
==========================================
+ Hits 1048 1094 +46
- Misses 727 741 +14
- Partials 26 27 +1
Continue to review full report at Codecov.
|
|
@BostjanBozic Thanks for this! It's exactly what I need! |
|
@BostjanBozic thanks again for working on this feature and for your patience to get #366 merged. I think this PR is very close. I provided a few comments that need to get resolved before I can /approve. We're cutting v1.16.0 tomorrow. I'll check this PR before cutting the release to see if it's ready to be included in the release. |
|
I don't think this one will make 1.16, pushing to 1.17. @danehans, please undo if you think otherwise. |
|
@danehans Thank you for feedback, I pushed latest changes, PTAL. Only thing that I am not sure about is equality for |
|
|
@danehans thank you for explanation. I am a bit lost though how can I fix this, can you maybe help me with this? |
|
@BostjanBozic please ignore #336 (comment). This PR looks good and should be ready to merge after fixing the merge conflicts and resolving #336 (comment). |
|
@BostjanBozic can you also add unit tests to cover the changes you made to the validation pkg? |
|
@BostjanBozic a few more comments above that need to get resolved... this PR is very close. Thanks again for your efforts and I look forward to getting this merged. |
|
@danehans Thanks for check again, I will get this fixed on Friday and rebase everything. For unit test they are configured in |
|
|
xref #379 for adding e2e tests (requires testing against cloud-specific clusters). |
Signed-off-by: Bostjan Bozic <[email protected]>
|
@danehans Thanks for looking into it. I went through your review and updated the code, PTAL. Maybe just a comment on changes within |
Signed-off-by: Bostjan Bozic <[email protected]>
| cntr.Spec.NetworkPublishing.Envoy.Type = operatorv1alpha1.LoadBalancerServicePublishingType | ||
| cntr.Spec.NetworkPublishing.Envoy.LoadBalancer.Scope = operatorv1alpha1.ExternalLoadBalancer | ||
| cntr.Spec.NetworkPublishing.Envoy.LoadBalancer.ProviderParameters.Type = operatorv1alpha1.AWSLoadBalancerProvider | ||
| if tc.description == "if load balancer IP changed" { |
There was a problem hiding this comment.
might be ok to leave now, but would be good to refactor this test suite a bit
we're currently taking a global contour variable and modifying it in each test case, then generating a service from that contour, would be good to not use that global variable if we are going to modify it and for this test just create a service object to check the equality against with the copy/mutate pattern which is there and nice to avoid duplication
There was a problem hiding this comment.
I was thinking the same. It would be quite a change, as it would affect whole equality_testo.go, so I thought we can set this up separately.
Adds support for configuring Load Balancer IP in Envoy service. Previously,
loadBalancerIPparameter of Envoy service was dynamically configured (from provider's side). Now user can preconfigure desired IP. This PR adds the following:Spec.NetworkPublishing.Envoy.LoadBalancer.ProviderParameters.<cloudProvider>Updates #326
Updates #327
Signed-off-by: Bostjan Bozic [email protected]