Repository navigation
[POC] Destination Service EndpointSlice support - #4563
mateiidavid wants to merge 5 commits into
Conversation
Signed-off-by: Matei David <matei.david.35@gmail.com>
Signed-off-by: Matei David <matei.david.35@gmail.com>
Signed-off-by: Matei David <matei.david.35@gmail.com>
…slices Signed-off-by: Matei David <matei.david.35@gmail.com>
| return | ||
| } | ||
|
|
||
| func endpointSliceAccess(k8sClient kubernetes.Interface) error { |
There was a problem hiding this comment.
does it actually make sense to retrn an error from here. From what I see you just want to know whether you can use slices or not, right? I think a bool should suffice there WDYT ?
There was a problem hiding this comment.
I see your point and I agree. Initially, I thought returning an error makes more sense because:
ServiceProfileAccessfunctions in a similar fashion, so thought it's a good idea for the sake of consistency.- By returning an error there was no need to log/handle the error in the function.
On second thought, handling the error in the function may not be necessary. I changed it to return a boolean. Some of the errors that can happen when calling ServerResourcesForGroupVersion(gv) on the discovery client are NotFound, IsForbidden and another error when the gv is empty. Based on this, I think logging/handling the error is trivial, we can just return false instead.
| return strings.Count(address, ":") >= 2 | ||
| } | ||
|
|
||
| func isTargetPortInSlice(targetPort namedPort, slicePorts []dv1beta1.EndpointPort) bool { |
There was a problem hiding this comment.
TIOLI: We can save on writing that for loop twice I guess:
func isTargetPortInSlice(targetPort namedPort, slicePorts []dv1beta1.EndpointPort) bool {
for _, p := range slicePorts {
switch targetPort.Type {
case intstr.Int:
if *p.Port == targetPort.IntVal {
return true
}
case intstr.String:
if *p.Name == targetPort.StrVal {
return true
}
}
}
return false
}
There was a problem hiding this comment.
That looks better so went ahead and changed it, thanks 👍
|
|
||
| if endpoint.TargetRef.Kind == "Pod" { | ||
| for _, addr := range endpoint.Addresses { | ||
| if isIPv6Address(addr) { |
There was a problem hiding this comment.
I think instead of doing so you can simply check the AddressType on the EndpointsSlice object. A slice will only have addresses of one type in its endpoints unless the documentation is incorrect.
| if endpoint.TargetRef == nil { | ||
| serviceName := es.Labels[dv1beta1.LabelServiceName] | ||
| for _, addr := range endpoint.Addresses { | ||
| if isIPv6Address(addr) { |
There was a problem hiding this comment.
Hm, I think this might cause some trouble here and there. So currently an EndpointsSlice has an address type defined such as:
const (
// AddressTypeIP represents an IP Address.
// This address type has been deprecated and has been replaced by the IPv4
// and IPv6 adddress types. New resources with this address type will be
// considered invalid. This will be fully removed in 1.18.
// +deprecated
AddressTypeIP = AddressType("IP")
// AddressTypeIPv4 represents an IPv4 Address.
AddressTypeIPv4 = AddressType(v1.IPv4Protocol)
// AddressTypeIPv6 represents an IPv6 Address.
AddressTypeIPv6 = AddressType(v1.IPv6Protocol)
// AddressTypeFQDN represents a FQDN.
AddressTypeFQDN = AddressType("FQDN")
)
This means that the address can actually be a domain name like google.com. In this case this function will return false:
func isIPv6Address(address string) bool {
return strings.Count(address, ":") >= 2
}
Not sure what happens next but I think it is not what we want. Nevertheless the fact that there is an address type that can be a concrete netwrok address (IPv4 or IPv6) OR a domain name is a great feature that I think will solve some of the headscratchers we are having here at the moment
There was a problem hiding this comment.
Yup, this was an error on my part, so apologies! When I was researching EndpointSlices, I came across a slice that had both IPv4 and IPv6 in the address type, but I think I must've made a mistake. It is obvious from the docs that a slice only has one address type:
// addressType specifies the type of address carried by this EndpointSlice.
// All addresses in this slice must be the same type. This field is
// immutable after creation. The following address types are currently
// supported:
// * IPv4: Represents an IPv4 Address.
// * IPv6: Represents an IPv6 Address.
// * FQDN: Represents a Fully Qualified Domain Name.
Based on your comment, I have:
- Removed the
isIPv6Addressfunction - Added a check in the
EndpointSlice/addEndpointshandler, if the address type is not IPv4 the resource won't be cached. This is until we can work out what should happen when a slice has an IPv6/FQDN address type.
There was a problem hiding this comment.
@Matei207 This is a great start !!! and overall I think the direction is the right one. I think one thing we need to think about is how do we want to take advantage of some of the extra features that endpoints slices provide. For example, do we want to ignore addresses of tupq FQDN the same way we do for services with ExternalName type?. Left some comments/questions. Also yes, a good set of unit tests once some of the more loose ideas are made concrete is a good thing to have for this kind of critical change
Signed-off-by: Matei David <matei.david.35@gmail.com>
|
@zaharidichev thank you for the review and the comments! I have addressed all of them and made changes where applicable.
Great point, one of the main things that I want to take away from this poc is what else can be done and built on top of the slices feature.
I have looked at #4582 and #4535 to try and get some ideas on how this could be handled, but I'm a bit stumped. WDYT? So far, it seems to me at the very least it should be decided what we are going to do when the address type is
Also, |
| id := PodID{ | ||
| Name: endpoint.TargetRef.Name, | ||
| Namespace: endpoint.TargetRef.Namespace, | ||
| } | ||
| pod, err := pp.k8sAPI.Pod().Lister().Pods(id.Namespace).Get(id.Name) | ||
| if err != nil { | ||
| pp.log.Errorf("Unable to fetch pod %v: %s", id, err) | ||
| continue | ||
| } | ||
| ownerKind, ownerName := pp.k8sAPI.GetOwnerKindAndName(pod, false) | ||
| addresses[id] = Address{ | ||
| IP: addr, | ||
| Port: resolvedPort, | ||
| Pod: pod, | ||
| OwnerName: ownerName, | ||
| OwnerKind: ownerKind, | ||
| } |
There was a problem hiding this comment.
This loggic here seems quite similar to the one in endpointsToAddresses. Can we extract it somehow?
| // Consumers must handle different types of addresses | ||
| // An "addresses" set must contain at least 1 but no more than 100 addresses | ||
| if endpoint.TargetRef == nil { | ||
| serviceName := es.Labels[dv1beta1.LabelServiceName] |
There was a problem hiding this comment.
What happens if this name is empty? Are we guaranteed that there will be a value for this label?
| } | ||
| } | ||
|
|
||
| return false |
There was a problem hiding this comment.
I think we can move that somewhere else indeed. Not sure it is worth it before we actually need it outside of this file.
| } | ||
| case *dv1beta1.EndpointSlice: | ||
| { | ||
| serviceName, ns = res.Labels[dv1beta1.LabelServiceName], res.Namespace |
There was a problem hiding this comment.
Again, not sure what we need to do in the case when this is not present. I kind of feel this is an invalid scenario and we should just skip processing the slice. Is this the only way to associate an EndpointSlice with a service? I assume so. If this label is non present.. then I guess this is an endpoint that is an "orphan" we cannot really do anything with it, so why go through the trouble of processing it at all. Correct me if I am wrong
There was a problem hiding this comment.
Labels are not the only way of associating an endpointslice to a service. According to the endpointslice docs, ownership is established by an ownership reference and/or a label.
e.g
ownerReferences:
- apiVersion: v1
blockOwnerDeletion: true
controller: true
kind: Service
name: web-svc
uid: f8d63059-f3ef-4a6f-9bfd-d9742ddb70deThere is no resource creation validation happening, so theoretically endpoints can be orphaned if they are created manually without specifying either a label or an ownership ref to the service. While it is unlikely to happen, think there should be a safeguard in place.
To cope with this, I will add a check to the addEndpoints function, a slice that:
- has an addressType != IPv4 (so IPv6 or FQDN)
- or does not have an ownership ref
- or does not have a service label
will not be processed.
| func endpointSliceAccess(k8sClient kubernetes.Interface) bool { | ||
| gv := dv1beta1.SchemeGroupVersion.String() | ||
| res, err := k8sClient.Discovery().ServerResourcesForGroupVersion(gv) | ||
| if err != nil { |
There was a problem hiding this comment.
maybe we can log the error here
There was a problem hiding this comment.
Yup, sounds good. Will change this.
|
@Matei207 As already mentioned I think you are on the right track. Lets abstract ourselves from how thi sall will work with multicluster as there are a few unknowns there at the moment. Instead we can focus on simply suporting the slices as if they were Endpoints and go with that for now. I think this is the right approach to integrate this new feature. Also at that point I think you can spend a bit of time trying to put together some unit tests that make sure everything works as expected :) |
|
Thank you for the comments and feedback! It seems that the general direction has been found and the next steps are a bit clearer. Will add the additional checks substantiated in the comments, add some unit tests and open a new PR with the actual implementation. |
Issue: #4501
After a few attempts at adding support for
EndpointSliceresources, I came up with this poc to show one way of doing this.EndpointSlice access
The first prerequisite to supporting
EndpointSlicesis to make sure that the cluster has access to the resource, since it sits behind a feature gate. To do this, we are using a similar check toServiceProfile. First, we check to see what resources are registered with the api-server under the group-versiondiscovery/v1beta1. Second, we check to see ifkind: EndpointSliceis a registered api resource, if it is, we return from the function with no error.When we create a new
EndpointWatcher, we check if the cluster has anyEndpointSlice. If we get an error, we start theEndpointsshared informer, otherwise, we start anEndpointSliceshared informer.Difference between EndpointSlice and Endpoints
Endpoints contain ALL endpoints of a service. An endpoint contains a subset -- a group of addresses with common ports. The set of all endpoints is the union of all subsets.EndpointSlicesARE a subset. Instead of grouping all subsets into a single resource, the slice contains only a set of addresses with common parts.Other notable differences:
Endpointswithip:Because of these differences,
EndpointSlicesas resources need to be processed a bit differently thanEndpoints. Will go more in-depth in the next sections.Handler functions
When implementing new handler functions for
EndpointSlice, I tried to reuse as much code as possible. Not everything can be reused because of the type system, where applicable I tried to reuse logic by doing type assertions.Add:
EndpointSlicein my mind there was one thing that needs to be addressed and is different fromEndpoints. Specifically, only update those ports whose addresses are in the slice.Endpointsresource has 2 subsets, one which includes addresses associated with 8080 and one with addresses associated with 6000, all ports would have to be updated when a change to theEndpointsobject is made, since E = Subset1 U Subset2. A slice IS a subset, however, so we want to update only theportPublisherthat has a targetPort of 8080, if our slice contains addresses associated with 8080. I'm not sure if this would be a case often met in a real production cluster, I've personally never seen anEndpointswith more than 1 subset, but I thought mathematically this should hold (?)isTargetPortInSlicethat checks whether the currentportPublisher's targetPort is part of the slice, if it is, update the endpoint.AddressSetsfor a port. Because each address has aconditionsfield associated with it, we only want to add an address when it is ready (to avoid duplicate ADDs being sent to the proxy).Delete:
Add-- when we want the delete the addresses of aportPublisher, only delete those addresses which were part of the slice, since other slices can be in existence it does not make sense to delete all endpoints.isTargetPortInSlice. If the addressSet is empty when we delete the slice addresses, we do whatnoEndpoints()does -- set the existence to false and tear down theportPublisher(along with its listener).Misc
I'm thinking that the resource access for
EndpointSlicesshould be done before on the server-side and passed as an argument toNewEndpointsWatcherto make it easier to write tests.I planned on adding new tests for the actual PR where we can check conditions such as ensuring a slice's addresses have been completely updated, a slice that does not correspond to a
portPublisherdoes not change anything, etc.When we create a new portPublisher we make another check for each resource to use when we populate it. If we have slices, we will populate the port with it. Not sure what to do when a port is updated, however.
I have tested this with the destination client script and it seems to be working fine. I have tested if
EndpointSlicescan be deleted, updated, added and thatNoEndpointsis returned when all pods in a slice have been deleted (to test if the address set is empty the functionality is similar tonoEndpoints).Let me know if I've missed anything!