dataclients/kubernetes: change GetEndpointAddresses to return addresses without scheme and port #2917

AlexanderYastrebov · 2024-02-09T12:31:45Z

use -kubernetes-redis-service-port flag to explicitly configure redis port
use separate cluster state cache for addresses since cache key does not contain port
rename helpers protocol argument to scheme

This is a breaking change for users that:

use kubernetes dataclient GetEndpointAddresses method
use dynamic redis shards discovery AND use non-standard redis port AND do not specify -kubernetes-redis-service-port

AlexanderYastrebov · 2024-02-09T12:37:28Z

dataclients/kubernetes/clusterstate.go

-	epID := endpointID{
-		ResourceID: newResourceID(namespace, name),
-		Protocol:   protocol,
-	}


This was logically wrong because it does not contain target port unlike other places that use state.cachedEndpoints

AlexanderYastrebov · 2024-02-09T12:38:11Z

dataclients/kubernetes/endpoints.go

@@ -22,18 +22,18 @@ type endpointList struct {
 	Items []*endpoint `json:"items"`
 }

-func formatEndpointString(ip, protocol string, port int) string {
-	return protocol + "://" + net.JoinHostPort(ip, strconv.Itoa(port))
+func formatEndpointString(ip, scheme string, port int) string {


This is not strictly required for this refactoring but I noticed the inconsistency and decided to fix it along.

dataclients/kubernetes/endpointslices_test.go

dataclients/kubernetes/endpointslices.go

AlexanderYastrebov · 2024-02-09T15:37:42Z

dataclients/kubernetes/clusterstate.go

 	state.mu.Lock()
 	defer state.mu.Unlock()
-	if cached, ok := state.cachedEndpoints[epID]; ok {
+	if cached, ok := state.cachedAddresses[rID]; ok {


I think we can even drop the cache as implementation became straightforward now

RomanZavodskikh · 2024-02-09T18:09:20Z

I would like to kindly ask why we should provide format different from what we already have in the routing.Route? Does it make sense to have the same format there and here for increasing readability?

https://github.com/zalando/skipper/pull/2824/files#diff-bacdef5a036ec850a0efe0adc1f0288bc03339074b068f5b9b87e7d8532aaa5bR274

AlexanderYastrebov · 2024-02-09T22:06:59Z

I would like to kindly ask why we should provide format different from what we already have in the routing.Route

Redis endpoints are used by redis client, these are not backend endpoints for the proxy.

AlexanderYastrebov · 2024-02-09T22:10:34Z

TestConcurrentKubernetesClusterStateAccessWithRemoteRedis fails because it creates an endpoint with a single address and multiple ports

skipper/redis_test.go

Lines 150 to 166 in be4ef50

    
           // apiserver1 
        
           specFmt := redisEpSpecFmt + redisPortFmt 
        
           redisSpec1 := fmt.Sprintf(specFmt, 0, host1, port1) 
        
           apiServer1, u1, err := createApiserver(kubeSpec + redisSpec1) 
        
           if err != nil { 
        
           	t.Fatalf("Failed to start apiserver1: %v", err) 
        
           } 
        
           defer apiServer1.Close() 
        
           // apiserver2 
        
           specFmt += redisPortFmt 
        
           redisSpec2 := fmt.Sprintf(specFmt, 0, host1, port1, port2) 
        
           apiServer2, u2, err := createApiserver(kubeSpec + redisSpec2) 
        
           if err != nil { 
        
           	t.Fatalf("Failed to start apiserver2: %v", err) 
        
           } 
        
           defer apiServer2.Close()

while in k8s cluster endpoints have multiple addresses and a single port.

I've created #2919 to address this.

…es without scheme and port * use -kubernetes-redis-service-port flag to explicitly configure redis port * use separate cluster state cache for addresses since cache key does not contain port * rename helpers protocol argument to scheme This is a breaking change for users that: * use kubernetes dataclient GetEndpointAddresses method * use dynamic redis shards discovery AND use non-standard redis port AND do not specify -kubernetes-redis-service-port For #2476 Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>

szuecs · 2024-02-12T13:54:40Z

👍

szuecs · 2024-02-12T13:55:50Z

Please make sure that the release notes contain all information what needs to be done to migrate (breaking change with simple migration path if documented).

AlexanderYastrebov · 2024-02-12T15:11:40Z

👍

Returning cached endpoint addreses from exported GetEndpointAddresses makes it possible for caller to modify cached values. Since the logic of obtaining endpoint addresses is lightweight this change simply drops caching altogether. Follow up on #2917 Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>

AlexanderYastrebov · 2024-02-12T16:08:31Z

routesrv/redishandler.go

 		for i := 0; i < len(a); i++ {
-			a[i] = strings.TrimPrefix(a[i], "http://")


Here is an obscure bug, see https://github.com/zalando/skipper/pull/2917/files#r1486405631

AlexanderYastrebov · 2024-02-12T16:08:57Z

skipper.go

-			a[i] = strings.TrimPrefix(a[i], "http://")
+			a[i] = net.JoinHostPort(a[i], port)


Here is an obscure bug - the code modifies cached value of a returned from kdc.GetEndpointAddresses, see #2929

It was not observed in production because k8s poll interval is 3 seconds and redis endpoints update interval is 10s

Returning cached endpoint addreses from exported GetEndpointAddresses makes it possible for caller to modify cached values. Since the logic of obtaining endpoint addresses is lightweight this change simply drops caching altogether. Follow up on #2917 Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>

AlexanderYastrebov added the minor no risk changes, for example new filters label Feb 9, 2024

AlexanderYastrebov mentioned this pull request Feb 9, 2024

Use kubernetes cluster client to discover redis shards #2915

Closed

4 tasks

AlexanderYastrebov force-pushed the dataclients/kubernetes/refactor-get-endpoint-addresses branch from 302dd49 to 4b0187f Compare February 9, 2024 12:36

AlexanderYastrebov commented Feb 9, 2024

View reviewed changes

szuecs reviewed Feb 9, 2024

View reviewed changes

dataclients/kubernetes/endpointslices_test.go Outdated Show resolved Hide resolved

szuecs reviewed Feb 9, 2024

View reviewed changes

dataclients/kubernetes/endpointslices.go Show resolved Hide resolved

AlexanderYastrebov force-pushed the dataclients/kubernetes/refactor-get-endpoint-addresses branch 2 times, most recently from 9b79d6c to 493c3fc Compare February 9, 2024 15:36

AlexanderYastrebov changed the title ~~dataclients/kubernetes: refactor GetEndpointAddresses~~ dataclients/kubernetes: change GetEndpointAddresses to return addresses without port Feb 9, 2024

AlexanderYastrebov commented Feb 9, 2024

View reviewed changes

AlexanderYastrebov force-pushed the dataclients/kubernetes/refactor-get-endpoint-addresses branch from 493c3fc to 471cfb4 Compare February 9, 2024 15:52

AlexanderYastrebov changed the title ~~dataclients/kubernetes: change GetEndpointAddresses to return addresses without port~~ dataclients/kubernetes: change GetEndpointAddresses to return addresses without scheme and port Feb 9, 2024

AlexanderYastrebov added major moderate risk, for example new API, small filter changes that have no risk like refactoring or logs and removed minor no risk changes, for example new filters labels Feb 9, 2024

AlexanderYastrebov force-pushed the dataclients/kubernetes/refactor-get-endpoint-addresses branch 2 times, most recently from a86199b to ce25bc5 Compare February 9, 2024 16:15

AlexanderYastrebov force-pushed the dataclients/kubernetes/refactor-get-endpoint-addresses branch from ce25bc5 to d05dd7b Compare February 12, 2024 11:57

AlexanderYastrebov merged commit 3c3f0c1 into master Feb 12, 2024
14 checks passed

AlexanderYastrebov deleted the dataclients/kubernetes/refactor-get-endpoint-addresses branch February 12, 2024 15:12

AlexanderYastrebov mentioned this pull request Feb 12, 2024

dataclients/kubernetes: remove endpoint addresses cache #2929

Merged

AlexanderYastrebov commented Feb 12, 2024

View reviewed changes

zalando-teapot-robot mentioned this pull request Sep 23, 2024

skipper: Update to version v0.21.208 zalando-incubator/kubernetes-on-aws#8164

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dataclients/kubernetes: change GetEndpointAddresses to return addresses without scheme and port #2917

dataclients/kubernetes: change GetEndpointAddresses to return addresses without scheme and port #2917

AlexanderYastrebov commented Feb 9, 2024 •

edited

Loading

AlexanderYastrebov Feb 9, 2024 •

edited

Loading

AlexanderYastrebov Feb 9, 2024 •

edited

Loading

AlexanderYastrebov Feb 9, 2024 •

edited

Loading

RomanZavodskikh commented Feb 9, 2024

AlexanderYastrebov commented Feb 9, 2024

AlexanderYastrebov commented Feb 9, 2024 •

edited

Loading

szuecs commented Feb 12, 2024

szuecs commented Feb 12, 2024

AlexanderYastrebov commented Feb 12, 2024

AlexanderYastrebov Feb 12, 2024 •

edited

Loading

AlexanderYastrebov Feb 12, 2024 •

edited

Loading

AlexanderYastrebov Feb 12, 2024 •

edited

Loading

		for i := 0; i < len(a); i++ {
		a[i] = strings.TrimPrefix(a[i], "http://")

		a[i] = strings.TrimPrefix(a[i], "http://")
		a[i] = net.JoinHostPort(a[i], port)

dataclients/kubernetes: change GetEndpointAddresses to return addresses without scheme and port #2917

dataclients/kubernetes: change GetEndpointAddresses to return addresses without scheme and port #2917

Conversation

AlexanderYastrebov commented Feb 9, 2024 • edited Loading

AlexanderYastrebov Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

AlexanderYastrebov Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

AlexanderYastrebov Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

RomanZavodskikh commented Feb 9, 2024

AlexanderYastrebov commented Feb 9, 2024

AlexanderYastrebov commented Feb 9, 2024 • edited Loading

szuecs commented Feb 12, 2024

szuecs commented Feb 12, 2024

AlexanderYastrebov commented Feb 12, 2024

AlexanderYastrebov Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

AlexanderYastrebov Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

AlexanderYastrebov Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

AlexanderYastrebov commented Feb 9, 2024 •

edited

Loading

AlexanderYastrebov Feb 9, 2024 •

edited

Loading

AlexanderYastrebov Feb 9, 2024 •

edited

Loading

AlexanderYastrebov Feb 9, 2024 •

edited

Loading

AlexanderYastrebov commented Feb 9, 2024 •

edited

Loading

AlexanderYastrebov Feb 12, 2024 •

edited

Loading

AlexanderYastrebov Feb 12, 2024 •

edited

Loading

AlexanderYastrebov Feb 12, 2024 •

edited

Loading