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

Fix race condition in strict mode #306

Merged
merged 3 commits into from
Sep 25, 2024

Conversation

Pavani-Panakanti
Copy link
Contributor

Issue #, if available:
Fixing a race condition where shared ebpf maps are being deleted in strict mode

Description of changes:
Added new map to store progFD to pods mapping which will help us check if progFD's are being shared between pods

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@Pavani-Panakanti Pavani-Panakanti requested a review from a team as a code owner September 20, 2024 01:48
@achevuru
Copy link
Contributor

Generic Q: How is the BPF prog delete going through when the said prog is still (loaded &) attached to another active pod interface on the same node?

@@ -197,6 +197,33 @@ func (r *PolicyEndpointsReconciler) cleanUpPolicyEndpoint(ctx context.Context, r
return nil
}

func (r *PolicyEndpointsReconciler) isProgFdShared(ctx context.Context, targetPodName string,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can optimize this as a pod will always have both ingress and egress probes attached (i.e.,) there will never be a scenario where only one of the probes is attached for any particular pod. Probably can collapse the maps in to one unless there are plans to use them for something else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@achevuru Yeah using one just one map to store reverse mapping from progFds to Pods list makes sense. I will just add the ingress one and we can remove the new egress map

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We will update this optimization in a separate PR as this is change is blocking cx

@Pavani-Panakanti
Copy link
Contributor Author

Generic Q: How is the BPF prog delete going through when the said prog is still (loaded &) attached to another active pod interface on the same node?

We are assuming it is not being shared by any other active pod by verifying policy endpoint list. But when a new pod is created and not yet reconciled in strict mode, we are attaching the probes and updating the maps but pod is not yet present in policy endpoint list https://github.com/aws/aws-network-policy-agent/blob/main/pkg/rpc/rpc_handler.go#L74

{"level":"info","ts":"2024-09-17T16:14:51.158Z","caller":"ebpf/bpf_client.go:724","msg":"This can be deleted, not needed anymore..."}
{"level":"info","ts":"2024-09-17T16:14:51.158Z","caller":"maps/loader.go:505","msg":"Delete map entry done with fd : 0 and err errno 0"}

@jayanthvn
Copy link
Contributor

Nice to hear from you @achevuru :)

To summarize with Strict mode when we get pod 2 of the same replica while we have pod 1 on the node. We check there is pod 1 and reuse the FDs and update the local data structures i.e, pod -> FDs mapping. Now the issue happens if there is a pod 1 delete at the same time and the PE reconcile is out of order i.e, we get pod 1 delete in the first reconciliation loop and then pod 2 add in the second reconciliation loop. In the first reconciliation loop we don't do additional check (shared replicas) and mark the pod 1 and bpf file to be deleted. Now when second reconciliation is received we just check the local data structure i.e, the mapping and skip loading bpf prog/map..pod 2 ENI will have a probe with an invalid FD..

@@ -211,11 +238,10 @@ func (r *PolicyEndpointsReconciler) updatePolicyEnforcementStatusForPods(ctx con
deletePinPath := true
podIdentifier := utils.GetPodIdentifier(targetPod.Name, targetPod.Namespace, r.log)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need podIdentifier? Seems like only used in the below log?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah we can remove that one. I will add optimization and clean up of this line in next PR

Copy link
Contributor

@jayanthvn jayanthvn left a comment

Choose a reason for hiding this comment

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

We can do the cleanup post merge. We should also have a way to dump all these internal structures maybe via cli to debug any issues..

@Pavani-Panakanti
Copy link
Contributor Author

We can do the cleanup post merge. We should also have a way to dump all these internal structures maybe via cli to debug any issues..

Agree. Logging these structs will help us in debugging. I will add a task and follow up on this

@Pavani-Panakanti Pavani-Panakanti merged commit fb4dd87 into aws:main Sep 25, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants