daemon: add BackendSlot to Service6Key.String and Service4Key.String

[xyz-li]

Please ensure your pull request adheres to the following guidelines:

• For first time contributors, read Submitting a pull request
• All code is covered by unit and/or runtime tests where feasible.
• All commits contain a well written commit description including a title,
  description and a Fixes: #XXX line if the commit addresses a particular
  GitHub issue.
• If your commit description contains a Fixes: <commit-id> tag, then
  please add the commit author[s] as reviewer[s] to this issue.
• All commits are signed off. See the section Developer’s Certificate of Origin
• Provide a title or release-note blurb suitable for the release notes.
• Are you a user of Cilium? Please add yourself to the Users doc
• Thanks for contributing!

Fixes: #29580

Fix overlapping keys in agent-side service BPF map cache used for retries. In rare cases this bug may have caused retrying of a failed BPF map update for a services entry to be skipped leading to a missing entry. This may have, for example, adversely affected recovering from a full BPF service map after excess services were removed.

[maintainer-s-little-helper]

Commit 0103bf3 does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

[aanm]

/test

[ti-mo]

@xyz-li What is the effect of removing the service entry? Given there are no backends to respond, is this a problem?

[xyz-li]

@xyz-li What is the effect of removing the service entry? Given there are no backends to respond, is this a problem?

Deleting the service entry will not affect the data-path. But it is very confusing when using cilium cli.
The output of cilium map get cilium_lb4_services_v2 does not contain my service IP. I spent one day to find the cause.

[aanm]

/test

[joestringer]

I think it might actually be useful to keep the service frontend in the datapath, so that when the LB decision is made, the datapath can emit drop notifications with DROP_NO_SERVICE ("Service backend not found").

[joamaki]

This does not look right at all. It's effectively undoing the delete for the obsolete backend slots, so we'll have entries in the service map that aren't used by the BPF datapath since slot > len(backends) of the master service. Unless I'm missing something, to me it seems like this is just effectively leaving garbage entries in the service map and bloating it.

Looking at the code, the agent-side cache should still have the master service in there as it's not deleted even when there's 0 backends.

Could you validate the cache against the actual BPF map (cilium-dbg bpf lb list) and see if there's any difference? I'm wondering if there's a bug causing the cache to not be in sync.

At least on my kind environment everything looks OK:

root@kind-control-plane:/home/cilium# cilium-dbg map get cilium_lb4_services_v2
Key                 Value                State   Error
10.96.142.154:80    0 1 (9) [0x0 0x0]    sync    
...

root@kind-control-plane:/home/cilium# cilium-dbg bpf lb list
SERVICE ADDRESS     BACKEND ADDRESS (REVNAT_ID) (SLOT)                           
10.96.142.154:80    0.0.0.0:0 (9) (0) [ClusterIP, non-routable]                  
                    10.244.1.21:9376 (9) (1) 

After deleting the backend pod:

root@kind-control-plane:/home/cilium# cilium-dbg map get cilium_lb4_services_v2
Key                 Value                State   Error
10.96.142.154:80    0 0 (9) [0x0 0x0]    sync    
root@kind-control-plane:/home/cilium# cilium-dbg bpf lb list
SERVICE ADDRESS     BACKEND ADDRESS (REVNAT_ID) (SLOT)
...                               
10.96.142.154:80    0.0.0.0:0 (9) (0) [ClusterIP, non-routable]                  

[xyz-li]

1. kubectl get svc

NAME         TYPE        CLUSTER-IP      EXTERNAL-IP   PORT(S)   AGE
kubernetes   ClusterIP   10.96.0.1       <none>        443/TCP   36d
nginx        ClusterIP   10.96.158.224   <none>        80/TCP    12d

2. cilium map get cilium_lb4_services_v2

10.96.114.15:443   0 1 (8) [0x0 0x10]   sync
10.96.0.10:53      0 2 (2) [0x0 0x0]    sync
10.96.0.1:443      0 1 (3) [0x0 0x0]    sync
10.96.158.224:80   0 2 (4) [0x0 0x0]    sync // 10.96.158.224 is here now.
10.96.0.10:9153    0 2 (1) [0x0 0x0]    sync

3. kubectl scale deploy nginx --replicas=1 # now service has one endpoint
4. kubectl describe svc nginx

Name:              nginx
Namespace:         default
Labels:            app=nginx
Annotations:       <none>
Selector:          app=nginx
Type:              ClusterIP
IP Family Policy:  SingleStack
IP Families:       IPv4
IP:                10.96.158.224
IPs:               10.96.158.224
Port:              80-80  80/TCP
TargetPort:        80/TCP
Endpoints:         10.0.0.249:80
Session Affinity:  None
Events:            <none>

5. cilium map get cilium_lb4_services_v2 # the item in cache not exists.

Key                Value                State   Error
10.96.114.15:443   0 1 (8) [0x0 0x10]   sync
10.96.0.10:53      0 2 (2) [0x0 0x0]    sync
10.96.0.1:443      0 1 (3) [0x0 0x0]    sync
10.96.0.10:9153    0 2 (1) [0x0 0x0]    sync

6. cilium bpf lb list

SERVICE ADDRESS    BACKEND ADDRESS (REVNAT_ID) (SLOT)
10.96.0.10:9153    0.0.0.0:0 (1) (0) [ClusterIP, non-routable]
                   10.0.0.39:9153 (1) (1)
                   10.0.1.179:9153 (1) (2)
10.96.0.10:53      10.0.0.39:53 (2) (1)
                   0.0.0.0:0 (2) (0) [ClusterIP, non-routable]
                   10.0.1.179:53 (2) (2)
10.96.114.15:443   0.0.0.0:0 (8) (0) [ClusterIP, InternalLocal, non-routable]
                   10.11.0.11:4244 (8) (1)
10.96.0.1:443      0.0.0.0:0 (3) (0) [ClusterIP, non-routable]
                   10.11.0.11:6443 (3) (1)
10.96.158.224:80   10.0.0.249:80 (4) (1)     # 10.96.158.224 exists 
                   0.0.0.0:0 (4) (0) [ClusterIP, non-routable]

After scale down my nginx workload, one endpoint will be deleted. At last the func deleteCacheEntry is called.

cilium/pkg/bpf/map_linux.go

Lines 883 to 890 in fa00376

	func (m *Map) deleteCacheEntry(key MapKey, err error) {
	if m.cache == nil {
	return
	}
	k := key.String()
	if err == nil {
	delete(m.cache, k)

And then the String function.

cilium/pkg/maps/lbmap/ipv4.go

Lines 250 to 257 in fa00376

	func (k *Service4Key) String() string {
	kHost := k.ToHost().(*Service4Key)
	addr := net.JoinHostPort(kHost.Address.String(), fmt.Sprintf("%d", kHost.Port))
	if kHost.Scope == loadbalancer.ScopeInternal {
	addr += "/i"
	}
	return addr
	}

[joamaki]

1. kubectl get svc

NAME         TYPE        CLUSTER-IP      EXTERNAL-IP   PORT(S)   AGE
kubernetes   ClusterIP   10.96.0.1       <none>        443/TCP   36d
nginx        ClusterIP   10.96.158.224   <none>        80/TCP    12d

2. cilium map get cilium_lb4_services_v2

10.96.114.15:443   0 1 (8) [0x0 0x10]   sync
10.96.0.10:53      0 2 (2) [0x0 0x0]    sync
10.96.0.1:443      0 1 (3) [0x0 0x0]    sync
10.96.158.224:80   0 2 (4) [0x0 0x0]    sync // 10.96.158.224 is here now.
10.96.0.10:9153    0 2 (1) [0x0 0x0]    sync

3. kubectl  scale deploy nginx --replicas=1 # now service has one endpoint

4. kubectl describe svc nginx

Name:              nginx
Namespace:         default
Labels:            app=nginx
Annotations:       <none>
Selector:          app=nginx
Type:              ClusterIP
IP Family Policy:  SingleStack
IP Families:       IPv4
IP:                10.96.158.224
IPs:               10.96.158.224
Port:              80-80  80/TCP
TargetPort:        80/TCP
Endpoints:         10.0.0.249:80
Session Affinity:  None
Events:            <none>

5. cilium map get cilium_lb4_services_v2  # the item in cache not exists.

Key                Value                State   Error
10.96.114.15:443   0 1 (8) [0x0 0x10]   sync
10.96.0.10:53      0 2 (2) [0x0 0x0]    sync
10.96.0.1:443      0 1 (3) [0x0 0x0]    sync
10.96.0.10:9153    0 2 (1) [0x0 0x0]    sync

6. cilium bpf lb list

SERVICE ADDRESS    BACKEND ADDRESS (REVNAT_ID) (SLOT)
10.96.0.10:9153    0.0.0.0:0 (1) (0) [ClusterIP, non-routable]
                   10.0.0.39:9153 (1) (1)
                   10.0.1.179:9153 (1) (2)
10.96.0.10:53      10.0.0.39:53 (2) (1)
                   0.0.0.0:0 (2) (0) [ClusterIP, non-routable]
                   10.0.1.179:53 (2) (2)
10.96.114.15:443   0.0.0.0:0 (8) (0) [ClusterIP, InternalLocal, non-routable]
                   10.11.0.11:4244 (8) (1)
10.96.0.1:443      0.0.0.0:0 (3) (0) [ClusterIP, non-routable]
                   10.11.0.11:6443 (3) (1)
10.96.158.224:80   10.0.0.249:80 (4) (1)     # 10.96.158.224 exists 
                   0.0.0.0:0 (4) (0) [ClusterIP, non-routable]

After scale down my nginx workload, one endpoint will be deleted. At last the func deleteCacheEntry is called.

cilium/pkg/bpf/map_linux.go

Lines 883 to 890 in fa00376

	func (m *Map) deleteCacheEntry(key MapKey, err error) {
	if m.cache == nil {
	return
	}
	k := key.String()
	if err == nil {
	delete(m.cache, k)

And then the String function.

cilium/pkg/maps/lbmap/ipv4.go

Lines 250 to 257 in fa00376

	func (k *Service4Key) String() string {
	kHost := k.ToHost().(*Service4Key)
	addr := net.JoinHostPort(kHost.Address.String(), fmt.Sprintf("%d", kHost.Port))
	if kHost.Scope == loadbalancer.ScopeInternal {
	addr += "/i"
	}
	return addr
	}

Ah the String function is incorrect! It's not taking into account the BackendSlot which causes multiple service entries to have the same key, hence we delete the "master" service when we're deleting the "backend slot" entries. What's worse here is that the "cache" is used to retry failed operations in resolveErrors and thus we may fail to retry some BPF updates since we've overwritten the entry in the cache.

A short term fix to this issue is to make sure Service4Key.String() fully reflects the Service4Key value, e.g. by having say (N) at the end where N is the BackendSlot. Could you try this and verify if that results in the correct behavior?

[joestringer]

/test