Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support configuring loadBalancerIP in envoy svc #2017

Merged
merged 3 commits into from
Oct 20, 2023
Merged

Support configuring loadBalancerIP in envoy svc #2017

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
71bd170
Support configuring loadBalancerIP in envoy svc
arkodg Oct 19, 2023
53d159e
typo
arkodg Oct 19, 2023
3834fe2
reject ipv6
arkodg Oct 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions api/v1alpha1/shared_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,13 @@ type KubernetesServiceSpec struct {
// +optional
AllocateLoadBalancerNodePorts *bool `json:"allocateLoadBalancerNodePorts,omitempty"`

// 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 in Kubernetes, but it is still used for setting the IP Address in some cloud
// providers such as GCP.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's deprecated, maybe use annotations to support this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not possible for GCP, the linked issue has all the details:)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for clearify.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what will happen if someone config this by mistake on non-GCP?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @LanceEa can you please review the API doc string in this PR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My guess is either it is ignored or like GCP the admission controller would reject the fields it doesn't support.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I'm thinking is should EG auto-detect cloud provider, and reject/ignore this field if it's non-GCP?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

// +optional
LoadBalancerIP *string `json:"loadBalancerIP,omitempty"`

// TODO: Expose config as use cases are better understood, e.g. labels.
}

Expand Down
10 changes: 10 additions & 0 deletions api/v1alpha1/validation/envoyproxy_validate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import (
"errors"
"fmt"
"net"
"reflect"

bootstrapv3 "github.com/envoyproxy/go-control-plane/envoy/config/bootstrap/v3"
Expand Down Expand Up @@ -95,6 +96,15 @@
errs = append(errs, fmt.Errorf("allocateLoadBalancerNodePorts can only be set for %v type", egv1a1.ServiceTypeLoadBalancer))
}
}
if serviceType, serviceLoadBalancerIP := spec.Provider.Kubernetes.EnvoyService.Type, spec.Provider.Kubernetes.EnvoyService.LoadBalancerIP; serviceType != nil && serviceLoadBalancerIP != nil {
if *serviceType != egv1a1.ServiceTypeLoadBalancer {
errs = append(errs, fmt.Errorf("loadBalancerIP can only be set for %v type", egv1a1.ServiceTypeLoadBalancer))
}

Check warning on line 102 in api/v1alpha1/validation/envoyproxy_validate.go

View check run for this annotation

Codecov / codecov/patch

api/v1alpha1/validation/envoyproxy_validate.go#L101-L102 

Added lines #L101 - L102 were not covered by tests

if ip := net.ParseIP(*serviceLoadBalancerIP); ip == nil || ip.To4() == nil {
errs = append(errs, fmt.Errorf("loadBalancerIP:%s is an invalid IPv4 address", *serviceLoadBalancerIP))
}
}
}
return errs
}
Expand Down
68 changes: 66 additions & 2 deletions api/v1alpha1/validation/envoyproxy_validate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ func TestValidateEnvoyProxy(t *testing.T) {
Kubernetes: &egv1a1.EnvoyProxyKubernetesProvider{
EnvoyService: &egv1a1.KubernetesServiceSpec{
Type: egv1a1.GetKubernetesServiceType(egv1a1.ServiceTypeLoadBalancer),
AllocateLoadBalancerNodePorts: ptr.To[bool](false),
AllocateLoadBalancerNodePorts: ptr.To(false),
},
},
},
Expand All @@ -205,14 +205,78 @@ func TestValidateEnvoyProxy(t *testing.T) {
Kubernetes: &egv1a1.EnvoyProxyKubernetesProvider{
EnvoyService: &egv1a1.KubernetesServiceSpec{
Type: egv1a1.GetKubernetesServiceType(egv1a1.ServiceTypeClusterIP),
AllocateLoadBalancerNodePorts: ptr.To[bool](false),
AllocateLoadBalancerNodePorts: ptr.To(false),
},
},
},
},
},
expected: false,
},
{
name: "envoy service type 'LoadBalancer' with valid loadBalancerIP",
proxy: &egv1a1.EnvoyProxy{
ObjectMeta: metav1.ObjectMeta{
Namespace: "test",
Name: "test",
},
Spec: egv1a1.EnvoyProxySpec{
Provider: &egv1a1.EnvoyProxyProvider{
Type: egv1a1.ProviderTypeKubernetes,
Kubernetes: &egv1a1.EnvoyProxyKubernetesProvider{
EnvoyService: &egv1a1.KubernetesServiceSpec{
Type: egv1a1.GetKubernetesServiceType(egv1a1.ServiceTypeLoadBalancer),
LoadBalancerIP: ptr.To("10.11.12.13"),
},
},
},
},
},
expected: true,
},
{
name: "envoy service type 'LoadBalancer' with invalid loadBalancerIP",
proxy: &egv1a1.EnvoyProxy{
ObjectMeta: metav1.ObjectMeta{
Namespace: "test",
Name: "test",
},
Spec: egv1a1.EnvoyProxySpec{
Provider: &egv1a1.EnvoyProxyProvider{
Type: egv1a1.ProviderTypeKubernetes,
Kubernetes: &egv1a1.EnvoyProxyKubernetesProvider{
EnvoyService: &egv1a1.KubernetesServiceSpec{
Type: egv1a1.GetKubernetesServiceType(egv1a1.ServiceTypeLoadBalancer),
LoadBalancerIP: ptr.To("invalid-ip"),
},
},
},
},
},
expected: false,
},
{
name: "envoy service type 'LoadBalancer' with ipv6 loadBalancerIP",
proxy: &egv1a1.EnvoyProxy{
ObjectMeta: metav1.ObjectMeta{
Namespace: "test",
Name: "test",
},
Spec: egv1a1.EnvoyProxySpec{
Provider: &egv1a1.EnvoyProxyProvider{
Type: egv1a1.ProviderTypeKubernetes,
Kubernetes: &egv1a1.EnvoyProxyKubernetesProvider{
EnvoyService: &egv1a1.KubernetesServiceSpec{
Type: egv1a1.GetKubernetesServiceType(egv1a1.ServiceTypeLoadBalancer),
LoadBalancerIP: ptr.To("2001:db8::68"),
},
},
},
},
},
expected: false,
},

{
name: "valid user bootstrap replace type",
proxy: &egv1a1.EnvoyProxy{
Expand Down
5 changes: 5 additions & 0 deletions api/v1alpha1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions charts/gateway-helm/crds/generated/gateway.envoyproxy.io_envoyproxies.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5258,6 +5258,14 @@ spec:
if more than one are available or is otherwise expected
to be specified
type: string
loadBalancerIP:
description: 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 in Kubernetes,
but it is still used for setting the IP Address in some
cloud providers such as GCP.
type: string
type:
default: LoadBalancer
description: Type determines how the Service is exposed.
Expand Down
3 changes: 3 additions & 0 deletions internal/infrastructure/kubernetes/resource/resource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ func ExpectedServiceSpec(service *egv1a1.KubernetesServiceSpec) corev1.ServiceSp
if service.AllocateLoadBalancerNodePorts != nil {
serviceSpec.AllocateLoadBalancerNodePorts = service.AllocateLoadBalancerNodePorts
}
if service.LoadBalancerIP != nil {
serviceSpec.LoadBalancerIP = *service.LoadBalancerIP
}
// Preserve the client source IP and avoid a second hop for LoadBalancer.
serviceSpec.ExternalTrafficPolicy = corev1.ServiceExternalTrafficPolicyTypeLocal
}
Expand Down
17 changes: 15 additions & 2 deletions internal/infrastructure/kubernetes/resource/resource_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,15 +56,28 @@ func TestExpectedServiceSpec(t *testing.T) {
name: "LoadBalancerWithAllocateLoadBalancerNodePorts",
args: args{service: &egv1a1.KubernetesServiceSpec{
Type: egv1a1.GetKubernetesServiceType(egv1a1.ServiceTypeLoadBalancer),
AllocateLoadBalancerNodePorts: ptr.To[bool](true),
AllocateLoadBalancerNodePorts: ptr.To(true),
}},
want: corev1.ServiceSpec{
Type: corev1.ServiceTypeLoadBalancer,
AllocateLoadBalancerNodePorts: ptr.To[bool](true),
AllocateLoadBalancerNodePorts: ptr.To(true),
SessionAffinity: corev1.ServiceAffinityNone,
ExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyTypeLocal,
},
},
{
name: "LoadBalancerWithLoadBalancerIP",
args: args{service: &egv1a1.KubernetesServiceSpec{
Type: egv1a1.GetKubernetesServiceType(egv1a1.ServiceTypeLoadBalancer),
LoadBalancerIP: ptr.To("10.11.12.13"),
}},
want: corev1.ServiceSpec{
Type: corev1.ServiceTypeLoadBalancer,
LoadBalancerIP: "10.11.12.13",
SessionAffinity: corev1.ServiceAffinityNone,
ExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyTypeLocal,
},
},
{
name: "ClusterIP",
args: args{service: &egv1a1.KubernetesServiceSpec{
Expand Down
1 change: 1 addition & 0 deletions site/content/en/latest/api/extension_types.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -847,6 +847,7 @@ _Appears in:_
| `type` _[ServiceType](#servicetype)_ | Type determines how the Service is exposed. Defaults to LoadBalancer. Valid options are ClusterIP, LoadBalancer and NodePort. "LoadBalancer" means a service will be exposed via an external load balancer (if the cloud provider supports it). "ClusterIP" means a service will only be accessible inside the cluster, via the cluster IP. "NodePort" means a service will be exposed on a static Port on all Nodes of the cluster. |
| `loadBalancerClass` _string_ | LoadBalancerClass, when specified, allows for choosing the LoadBalancer provider implementation if more than one are available or is otherwise expected to be specified |
| `allocateLoadBalancerNodePorts` _boolean_ | AllocateLoadBalancerNodePorts defines if NodePorts will be automatically allocated for services with type LoadBalancer. Default is "true". It may be set to "false" if the cluster load-balancer does not rely on NodePorts. If the caller requests specific NodePorts (by specifying a value), those requests will be respected, regardless of this field. This field may only be set for services with type LoadBalancer and will be cleared if the type is changed to any other type. |
| `loadBalancerIP` _string_ | 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 in Kubernetes, but it is still used for setting the IP Address in some cloud providers such as GCP. |


#### KubernetesWatchMode
Expand Down