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.

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 FQDN Address Type in EndpointSlice #1922

Closed
arkodg opened this issue Oct 3, 2023 · 18 comments · Fixed by #2138
Closed

Support FQDN Address Type in EndpointSlice #1922

arkodg opened this issue Oct 3, 2023 · 18 comments · Fixed by #2138
Assignees
Labels
area/translator Issues related to Gateway's translation service, e.g. translating Gateway APIs into the IR. kind/enhancement New feature or request
Milestone

Comments

@arkodg
Copy link
Contributor

arkodg commented Oct 3, 2023

Description:
We've recently added support for EndpointSlice, and current only support Static IPv4 Endpoint Addresses
We should also add support for FQDN address types
https://kubernetes.io/docs/concepts/services-networking/endpoint-slices/#address-types

[optional Relevant Links:]

  • looks like no changes are needed in the gateway api layer
    for _, address := range endpoint.Addresses {
  • in the xds translator layer, we'd need to detect whether the endpoint is a static address or FQDN and pass the right endpoint type (Static or DNS)
    endpointType: Static,
  • Above step may require splitting up endpoints in multiple clusters if we need to handle mix endpoints (some static and some DNS)
@arkodg arkodg added kind/enhancement New feature or request area/translator Issues related to Gateway's translation service, e.g. translating Gateway APIs into the IR. help wanted Extra attention is needed labels Oct 3, 2023
@arkodg
Copy link
Contributor Author

arkodg commented Oct 3, 2023

this is a good alternative to supporting ExternalName in Service

@arkodg
Copy link
Contributor Author

arkodg commented Oct 3, 2023

relates to #873

@shawnh2
Copy link
Contributor

shawnh2 commented Oct 12, 2023

hi, interested in picking this one up, assign please.

@zirain zirain removed the help wanted Extra attention is needed label Oct 12, 2023
@arkodg
Copy link
Contributor Author

arkodg commented Oct 12, 2023

thanks @shawnh2, recommend working on top of the changes introduced by #1947

@arkodg arkodg modified the milestones: 0.6.0-rc1, Backlog Oct 14, 2023
@EltonzHu
Copy link

EltonzHu commented Oct 23, 2023

Hi @arkodg , I think this approach has the same security risks as using externalName service. Has Envoy gateway community approved this feature design?

Thank you!

@arkodg
Copy link
Contributor Author

arkodg commented Oct 23, 2023

can you elaborate on the security risk here ?

@EltonzHu
Copy link

can you elaborate on the security risk here ?

Sure.

These security issues are mainly related to ingress:

  • Any DNS name can be resolved to sensitive IPs that may expose content the proxy has access to but is not supposed to expose.
  • DNS resolution to 127.0.0.1 attack which can break into Envoy admin endpoint. Hopefully the Envoy community can fix this Admin Endpoint Security issue.
  • Because of the way kube DNS works, if you know the name of a service and the name of the namespace it's in, you can use a servicename.svc.namespace.cluster.local ExternalName to expose it, and, unless you have good NetworkPolicy and a CNI that enforces it correctly, the proxy will allow it.

Ref: Above are summarized from link.

@arkodg
Copy link
Contributor Author

arkodg commented Oct 23, 2023

Yes above security concerns have been mitigated because the definition of a FQDN endpoint has moved from a less privileged resource ( Service ) to a more privileged resource ( EndpointSlice) which only a few have access to edit

@EltonzHu
Copy link

EltonzHu commented Oct 23, 2023

Yes above security concerns have been mitigated because the definition of a FQDN endpoint has moved from a less privileged resource ( Service ) to a more privileged resource ( EndpointSlice) which only a few have access to edit

Thanks @arkodg for the confirmation.
Three more questions.

  1. If we go with this EndpointSlice solution, any K8S clusters version below 1.21 can not be supported. Any other work around for those clusters?
  2. @shawnh2 Any ETA for this feature implementation? Is this going to be included in the next release 0.6?
  3. I was looking into the code. It is doable as @arkodg mentioned it in the above. However, which Envoy cluster type we are going to pick for EndpointSlice with FQDN type? Stric DNS or Logical DNS? Either of them can be applied depends on the DNS configuration externally. I did not see any discussion about handlering Envoy cluster type above. I am thinking to use annotations to address this problem. What do you think? @arkodg @shawnh2

@shawnh2
Copy link
Contributor

shawnh2 commented Oct 23, 2023

  1. We have a compatibility matrix for Envoy Gateway, the lowest Kubernetes version we support is v1.24.
  2. I will take a further look into this issue about this week, but I shall assume this feature wont be in the next release 0.6.

@EltonzHu
Copy link

  1. We have a compatibility matrix for Envoy Gateway, the lowest Kubernetes version we support is v1.24.
  2. I will take a further look into this issue about this week, but I shall assume this feature wont be in the next release 0.6.

Thanks @shawnh2 .

  1. I have generally tested Envoy gateway in clusters below v1.24 which worked in features we want. Fair enough according to the compatibility matrix.
  2. Please let me know any ETA after you looking into this issue this week. BTW, I am happy to help on this development / review too.

@EltonzHu
Copy link

@arkodg @shawnh2 Looks like K8S community is still considering deprecate FQDN type from endpointslice as seen in the head of the main branch from K8S repo: https://github.com/kubernetes/kubernetes/blob/master/pkg/registry/discovery/endpointslice/strategy.go#L221

Are you guys aware of this?

@arkodg
Copy link
Contributor Author

arkodg commented Oct 27, 2023

looks like you already found the issue kubernetes/kubernetes#114210 @EltonzHu, its still being discussed

@EltonzHu
Copy link

looks like you already found the issue kubernetes/kubernetes#114210 @EltonzHu, its still being discussed

Yes. Thank you for the confirmation. @arkodg

@shawnh2
Copy link
Contributor

shawnh2 commented Nov 5, 2023

func getIREndpointsFromEndpointSlice(endpointSlices []*discoveryv1.EndpointSlice, portName string, portProtocol corev1.Protocol) []*ir.DestinationEndpoint {
endpoints := []*ir.DestinationEndpoint{}
for _, endpointSlice := range endpointSlices {
for _, endpoint := range endpointSlice.Endpoints {

assume you have two endpointslices that associate with one service, one is IP address type, the other is FQDN address type. all the endpoints in these two endpointslices will be mixed into one vector via above function.

in the xds translator layer, should we separate the endpoints by its address type? and create a different xds cluster for each address-type endpoints? in that case, how to associate these xds clusters with only one xds route?

IMHO, the static type endpoint is fine. the EDS cluster type supports the endpoint address field to be a FQDN type address. so maybe we don't have to detect whether the endpoint type is a static or FQDN.

if args.endpointType == Static {
cluster.ClusterDiscoveryType = &clusterv3.Cluster_Type{Type: clusterv3.Cluster_EDS}

cc @arkodg

@EltonzHu
Copy link

EltonzHu commented Nov 5, 2023

func getIREndpointsFromEndpointSlice(endpointSlices []*discoveryv1.EndpointSlice, portName string, portProtocol corev1.Protocol) []*ir.DestinationEndpoint {
endpoints := []*ir.DestinationEndpoint{}
for _, endpointSlice := range endpointSlices {
for _, endpoint := range endpointSlice.Endpoints {

assume you have two endpointslices that associate with one service, one is IP address type, the other is FQDN address type. all the endpoints in these two endpointslices will be mixed into one vector via above function.

in the xds translator layer, should we separate the endpoints by its address type? and create a different xds cluster for each address-type endpoints? in that case, how to associate these xds clusters with only one xds route?

IMHO, the static type endpoint is fine. the EDS cluster type supports the endpoint address field to be a FQDN type address. so maybe we don't have to detect whether the endpoint type is a static or FQDN.

if args.endpointType == Static {
cluster.ClusterDiscoveryType = &clusterv3.Cluster_Type{Type: clusterv3.Cluster_EDS}

cc @arkodg

From EDS cluster type
image
From endpoint address field
image

Above information show that address inside EDS must be an IP, so we can not use EDS for address type as an FQDN.

@EltonzHu
Copy link

EltonzHu commented Nov 5, 2023

Regarding this case "User have two endpointslices that associate with one service, one is IP address type, the other is FQDN address type. ",
we can use weight balancing from the routing layer in technical stand point. However, from the practice of using L7 proxy, we should not allow this kind of configuration.

@arkodg
Copy link
Contributor Author

arkodg commented Nov 10, 2023

agree with @EltonzHu
@shawnh2 lets keep it simple for now, if EndpointSlice contains mixed endpoint types (Static & FQDN), lets set the status ResolvedRefs=False and discontinue processing the backendRef

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/translator Issues related to Gateway's translation service, e.g. translating Gateway APIs into the IR. kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants