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

Deletion of individual NodePortLocal (NPL) mapping rules not working correctly #6281

Closed
antoninbas opened this issue May 2, 2024 · 0 comments · Fixed by #6284
Closed

Deletion of individual NodePortLocal (NPL) mapping rules not working correctly #6281

antoninbas opened this issue May 2, 2024 · 0 comments · Fixed by #6284
Assignees
Labels
area/proxy/nodeportlocal Issues or PRs related to the NodePortLocal feature kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.

Comments

@antoninbas
Copy link
Contributor

Describe the bug
It is possible to create a situation where the NPL controller in the Antrea Agent cannot delete NPL rules appropriately when needed (on Linux Nodes). The DNAT rule gets deleted correctly from iptables, but the Pod annotation is not updated, and we get stuck in an error loop when trying to handle Pod updates.

To Reproduce
On a cluster running Antrea with NPL enabled, do the following:

  1. Apply the following YAML manifest. You will get a Pod with 2 containers, both running web servers on different ports. You will also get 2 Services, one for each web server, with NPL enabled.
apiVersion: apps/v1
kind: Deployment
metadata:
  name: web-deployment
spec:
  replicas: 1
  selector:
    matchLabels:
      app: web-server
  template:
    metadata:
      labels:
        app: web-server
    spec:
      containers:
      - name: web-server-1
        image: registry.k8s.io/e2e-test-images/agnhost:2.29
        command: ["/agnhost", "test-webserver", "--port=9090"]
        ports:
        - containerPort: 9090
      - name: web-server-2
        image: registry.k8s.io/e2e-test-images/agnhost:2.29
        command: ["/agnhost", "test-webserver", "--port=9000"]
        ports:
        - containerPort: 9000

---
apiVersion: v1
kind: Service
metadata:
  name: web-service-9090
  annotations:
    nodeportlocal.antrea.io/enabled: "true"
spec:
  selector:
    app: web-server
  ports:
  - protocol: TCP
    port: 80
    targetPort: 9090

---
apiVersion: v1
kind: Service
metadata:
  name: web-service-9000
  annotations:
    nodeportlocal.antrea.io/enabled: "true"
spec:
  selector:
    app: web-server
  ports:
  - protocol: TCP
    port: 80
    targetPort: 9000
  1. Check the Pod NPL annotation, it should have 2 mappings. For example:
$ kubectl get pod/web-deployment-7fd7879d77-b96jf -o=jsonpath='{.metadata.annotations}'
{"nodeportlocal.antrea.io":"[{\"podPort\":9090,\"nodeIP\":\"172.18.0.3\",\"nodePort\":61001,\"protocol\":\"tcp\"},{\"podPort\":9000,\"nodeIP\":\"172.18.0.3\",\"nodePort\":61000,\"protocol\":\"tcp\"}]"}
  1. Edit one of the Services to remove the NPL annotation and disable NPL.

Expected

After a short while, the Pod's NPL annotation should be updated to show a single mapping.

Actual behavior

The Pod's NPL annotation is never updated, and the antrea-agent logs (on the Node where the Pod is running) show the following errors:

I0502 23:03:21.694528       1 iptable_rule.go:124] "Successfully deleted DNAT rule" podAddr="10.10.1.2:9090" nodePort=61001 protocol="tcp"
E0502 23:03:21.694730       1 npl_controller.go:375] Error syncing Pod default/web-deployment-7fd7879d77-b96jf, requeuing. Error: failed to delete rule for Pod IP 10.10.1.2, Pod Port 9090, Protocol tcp: protocol tcp is still in use, cannot release socket
I0502 23:03:23.705246       1 iptable_rule.go:124] "Successfully deleted DNAT rule" podAddr="10.10.1.2:9090" nodePort=61001 protocol="tcp"
E0502 23:03:23.705867       1 npl_controller.go:375] Error syncing Pod default/web-deployment-7fd7879d77-b96jf, requeuing. Error: failed to delete rule for Pod IP 10.10.1.2, Pod Port 9090, Protocol tcp: protocol tcp is still in use, cannot release socket
I0502 23:03:27.714051       1 iptable_rule.go:124] "Successfully deleted DNAT rule" podAddr="10.10.1.2:9090" nodePort=61001 protocol="tcp"
E0502 23:03:27.714315       1 npl_controller.go:375] Error syncing Pod default/web-deployment-7fd7879d77-b96jf, requeuing. Error: failed to delete rule for Pod IP 10.10.1.2, Pod Port 9090, Protocol tcp: protocol tcp is still in use, cannot release socket
I0502 23:03:35.719107       1 iptable_rule.go:124] "Successfully deleted DNAT rule" podAddr="10.10.1.2:9090" nodePort=61001 protocol="tcp"
E0502 23:03:35.719317       1 npl_controller.go:375] Error syncing Pod default/web-deployment-7fd7879d77-b96jf, requeuing. Error: failed to delete rule for Pod IP 10.10.1.2, Pod Port 9090, Protocol tcp: protocol tcp is still in use, cannot release socket
I0502 23:03:51.724275       1 iptable_rule.go:124] "Successfully deleted DNAT rule" podAddr="10.10.1.2:9090" nodePort=61001 protocol="tcp"

This issue only exists when we (the NPL controller in the Agent) need to edit an existing NPL Pod annotation to remove one of the rules / mappings. When the Pod is deleted, or when NPL is completely "disabled" for a Pod (all mappings are removed), the issue does not arise. This is because we use different functions to handle these 2 different situations:

  • delete all NPL rules for the Pod (this works):
    func (pt *PortTable) DeleteRulesForPod(podIP string) error {
    pt.tableLock.Lock()
    defer pt.tableLock.Unlock()
    podEntries := pt.getDataForPodIP(podIP)
    for _, podEntry := range podEntries {
    protocolSocketData := podEntry.Protocol
    if err := pt.PodPortRules.DeleteRule(podEntry.NodePort, podIP, podEntry.PodPort, protocolSocketData.Protocol); err != nil {
    return err
    }
    if err := protocolSocketData.socket.Close(); err != nil {
    return fmt.Errorf("error when releasing local port %d with protocol %s: %v", podEntry.NodePort, protocolSocketData.Protocol, err)
    }
    pt.deletePortTableCache(podEntry)
    }
    return nil
    }
  • delete a single NPL rule for the Pod (this does not work):
    func (pt *PortTable) DeleteRule(podIP string, podPort int, protocol string) error {
    pt.tableLock.Lock()
    defer pt.tableLock.Unlock()
    data := pt.getEntryByPodIPPortProto(podIP, podPort, protocol)
    if data == nil {
    // Delete not required when the PortTable entry does not exist
    return nil
    }
    if err := pt.PodPortRules.DeleteRule(data.NodePort, podIP, podPort, protocol); err != nil {
    return err
    }
    if err := data.CloseSockets(); err != nil {
    return err
    }
    // We don't need to delete cache from different indexes repeatedly because they map to the same entry.
    pt.deletePortTableCache(data)
    return nil
    }

Versions:
All "recent" Antrea versions.
This issue was reported for Antrea v1.11.3, and I confirmed that the issue is still present in Antrea v2.0.0.

@antoninbas antoninbas added kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. area/proxy/nodeportlocal Issues or PRs related to the NodePortLocal feature labels May 2, 2024
@antoninbas antoninbas self-assigned this May 2, 2024
antoninbas added a commit to antoninbas/antrea that referenced this issue May 3, 2024
The logic for deleting an individual NPL mapping was broken. It
incorrectly believed that the protocol socket was still in use, and the
mapping could never be deleted, putting the NPL controller in an endless
error loop.

The State field in ProtocolSocketData was left over from pre Antrea
v1.7, back when we would always use the same port number for multiple
protocols, for a give Pod IP + port. With the current version of the NPL
implementation, this field is not needed and should be removed. By
removing the field, we avoid the deletion issue.

This patch also ensures that if a rule is only partially cleaned-up, we
can attempt to delete it again, by making DeleteRule idempotent. To
identify that a prior deletion attempt failed, we introduce a "defunct"
field in the NPL rule data. If this field is set, the controller knows
that the rule has been partially deleted and deletion needs to be
attempted again. Without this, it would be possible for the controller
(with the right sequence of updates) to assume that a partially-deleted
rule is still valid, which would break the datapath. I plan on improving
the NPL code further with a follow-up patch, but in order to keep this
patch small (for back-porting), I went with the simplest solution I
could think of.

Fixes antrea-io#6281

Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>
antoninbas added a commit to antoninbas/antrea that referenced this issue May 6, 2024
The logic for deleting an individual NPL mapping was broken. It
incorrectly believed that the protocol socket was still in use, and the
mapping could never be deleted, putting the NPL controller in an endless
error loop.

The State field in ProtocolSocketData was left over from pre Antrea
v1.7, back when we would always use the same port number for multiple
protocols, for a give Pod IP + port. With the current version of the NPL
implementation, this field is not needed and should be removed. By
removing the field, we avoid the deletion issue.

This patch also ensures that if a rule is only partially cleaned-up, we
can attempt to delete it again, by making DeleteRule idempotent. To
identify that a prior deletion attempt failed, we introduce a "defunct"
field in the NPL rule data. If this field is set, the controller knows
that the rule has been partially deleted and deletion needs to be
attempted again. Without this, it would be possible for the controller
(with the right sequence of updates) to assume that a partially-deleted
rule is still valid, which would break the datapath. I plan on improving
the NPL code further with a follow-up patch, but in order to keep this
patch small (for back-porting), I went with the simplest solution I
could think of.

Fixes antrea-io#6281

Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>
antoninbas added a commit that referenced this issue May 7, 2024
The logic for deleting an individual NPL mapping was broken. It
incorrectly believed that the protocol socket was still in use, and the
mapping could never be deleted, putting the NPL controller in an endless
error loop.

The State field in ProtocolSocketData was left over from pre Antrea
v1.7, back when we would always use the same port number for multiple
protocols, for a give Pod IP + port. With the current version of the NPL
implementation, this field is not needed and should be removed. By
removing the field, we avoid the deletion issue.

This patch also ensures that if a rule is only partially cleaned-up, we
can attempt to delete it again, by making DeleteRule idempotent. To
identify that a prior deletion attempt failed, we introduce a "defunct"
field in the NPL rule data. If this field is set, the controller knows
that the rule has been partially deleted and deletion needs to be
attempted again. Without this, it would be possible for the controller
(with the right sequence of updates) to assume that a partially-deleted
rule is still valid, which would break the datapath. I plan on improving
the NPL code further with a follow-up patch, but in order to keep this
patch small (for back-porting), I went with the simplest solution I
could think of.

Fixes #6281

Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>
antoninbas added a commit to antoninbas/antrea that referenced this issue May 7, 2024
The logic for deleting an individual NPL mapping was broken. It
incorrectly believed that the protocol socket was still in use, and the
mapping could never be deleted, putting the NPL controller in an endless
error loop.

The State field in ProtocolSocketData was left over from pre Antrea
v1.7, back when we would always use the same port number for multiple
protocols, for a give Pod IP + port. With the current version of the NPL
implementation, this field is not needed and should be removed. By
removing the field, we avoid the deletion issue.

This patch also ensures that if a rule is only partially cleaned-up, we
can attempt to delete it again, by making DeleteRule idempotent. To
identify that a prior deletion attempt failed, we introduce a "defunct"
field in the NPL rule data. If this field is set, the controller knows
that the rule has been partially deleted and deletion needs to be
attempted again. Without this, it would be possible for the controller
(with the right sequence of updates) to assume that a partially-deleted
rule is still valid, which would break the datapath. I plan on improving
the NPL code further with a follow-up patch, but in order to keep this
patch small (for back-porting), I went with the simplest solution I
could think of.

Fixes antrea-io#6281

Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>
antoninbas added a commit to antoninbas/antrea that referenced this issue May 7, 2024
The logic for deleting an individual NPL mapping was broken. It
incorrectly believed that the protocol socket was still in use, and the
mapping could never be deleted, putting the NPL controller in an endless
error loop.

The State field in ProtocolSocketData was left over from pre Antrea
v1.7, back when we would always use the same port number for multiple
protocols, for a give Pod IP + port. With the current version of the NPL
implementation, this field is not needed and should be removed. By
removing the field, we avoid the deletion issue.

This patch also ensures that if a rule is only partially cleaned-up, we
can attempt to delete it again, by making DeleteRule idempotent. To
identify that a prior deletion attempt failed, we introduce a "defunct"
field in the NPL rule data. If this field is set, the controller knows
that the rule has been partially deleted and deletion needs to be
attempted again. Without this, it would be possible for the controller
(with the right sequence of updates) to assume that a partially-deleted
rule is still valid, which would break the datapath. I plan on improving
the NPL code further with a follow-up patch, but in order to keep this
patch small (for back-porting), I went with the simplest solution I
could think of.

Fixes antrea-io#6281

Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>
antoninbas added a commit to antoninbas/antrea that referenced this issue May 7, 2024
The logic for deleting an individual NPL mapping was broken. It
incorrectly believed that the protocol socket was still in use, and the
mapping could never be deleted, putting the NPL controller in an endless
error loop.

The State field in ProtocolSocketData was left over from pre Antrea
v1.7, back when we would always use the same port number for multiple
protocols, for a give Pod IP + port. With the current version of the NPL
implementation, this field is not needed and should be removed. By
removing the field, we avoid the deletion issue.

This patch also ensures that if a rule is only partially cleaned-up, we
can attempt to delete it again, by making DeleteRule idempotent. To
identify that a prior deletion attempt failed, we introduce a "defunct"
field in the NPL rule data. If this field is set, the controller knows
that the rule has been partially deleted and deletion needs to be
attempted again. Without this, it would be possible for the controller
(with the right sequence of updates) to assume that a partially-deleted
rule is still valid, which would break the datapath. I plan on improving
the NPL code further with a follow-up patch, but in order to keep this
patch small (for back-porting), I went with the simplest solution I
could think of.

Fixes antrea-io#6281

Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>
antoninbas added a commit to antoninbas/antrea that referenced this issue May 7, 2024
The logic for deleting an individual NPL mapping was broken. It
incorrectly believed that the protocol socket was still in use, and the
mapping could never be deleted, putting the NPL controller in an endless
error loop.

The State field in ProtocolSocketData was left over from pre Antrea
v1.7, back when we would always use the same port number for multiple
protocols, for a give Pod IP + port. With the current version of the NPL
implementation, this field is not needed and should be removed. By
removing the field, we avoid the deletion issue.

This patch also ensures that if a rule is only partially cleaned-up, we
can attempt to delete it again, by making DeleteRule idempotent. To
identify that a prior deletion attempt failed, we introduce a "defunct"
field in the NPL rule data. If this field is set, the controller knows
that the rule has been partially deleted and deletion needs to be
attempted again. Without this, it would be possible for the controller
(with the right sequence of updates) to assume that a partially-deleted
rule is still valid, which would break the datapath. I plan on improving
the NPL code further with a follow-up patch, but in order to keep this
patch small (for back-porting), I went with the simplest solution I
could think of.

Fixes antrea-io#6281

Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>
antoninbas added a commit that referenced this issue May 8, 2024
The logic for deleting an individual NPL mapping was broken. It
incorrectly believed that the protocol socket was still in use, and the
mapping could never be deleted, putting the NPL controller in an endless
error loop.

The State field in ProtocolSocketData was left over from pre Antrea
v1.7, back when we would always use the same port number for multiple
protocols, for a give Pod IP + port. With the current version of the NPL
implementation, this field is not needed and should be removed. By
removing the field, we avoid the deletion issue.

This patch also ensures that if a rule is only partially cleaned-up, we
can attempt to delete it again, by making DeleteRule idempotent. To
identify that a prior deletion attempt failed, we introduce a "defunct"
field in the NPL rule data. If this field is set, the controller knows
that the rule has been partially deleted and deletion needs to be
attempted again. Without this, it would be possible for the controller
(with the right sequence of updates) to assume that a partially-deleted
rule is still valid, which would break the datapath. I plan on improving
the NPL code further with a follow-up patch, but in order to keep this
patch small (for back-porting), I went with the simplest solution I
could think of.

Fixes #6281

Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>
antoninbas added a commit that referenced this issue May 8, 2024
The logic for deleting an individual NPL mapping was broken. It
incorrectly believed that the protocol socket was still in use, and the
mapping could never be deleted, putting the NPL controller in an endless
error loop.

The State field in ProtocolSocketData was left over from pre Antrea
v1.7, back when we would always use the same port number for multiple
protocols, for a give Pod IP + port. With the current version of the NPL
implementation, this field is not needed and should be removed. By
removing the field, we avoid the deletion issue.

This patch also ensures that if a rule is only partially cleaned-up, we
can attempt to delete it again, by making DeleteRule idempotent. To
identify that a prior deletion attempt failed, we introduce a "defunct"
field in the NPL rule data. If this field is set, the controller knows
that the rule has been partially deleted and deletion needs to be
attempted again. Without this, it would be possible for the controller
(with the right sequence of updates) to assume that a partially-deleted
rule is still valid, which would break the datapath. I plan on improving
the NPL code further with a follow-up patch, but in order to keep this
patch small (for back-porting), I went with the simplest solution I
could think of.

Fixes #6281

Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>
antoninbas added a commit that referenced this issue May 11, 2024
The logic for deleting an individual NPL mapping was broken. It
incorrectly believed that the protocol socket was still in use, and the
mapping could never be deleted, putting the NPL controller in an endless
error loop.

The State field in ProtocolSocketData was left over from pre Antrea
v1.7, back when we would always use the same port number for multiple
protocols, for a give Pod IP + port. With the current version of the NPL
implementation, this field is not needed and should be removed. By
removing the field, we avoid the deletion issue.

This patch also ensures that if a rule is only partially cleaned-up, we
can attempt to delete it again, by making DeleteRule idempotent. To
identify that a prior deletion attempt failed, we introduce a "defunct"
field in the NPL rule data. If this field is set, the controller knows
that the rule has been partially deleted and deletion needs to be
attempted again. Without this, it would be possible for the controller
(with the right sequence of updates) to assume that a partially-deleted
rule is still valid, which would break the datapath. I plan on improving
the NPL code further with a follow-up patch, but in order to keep this
patch small (for back-porting), I went with the simplest solution I
could think of.

Fixes #6281

Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>
antoninbas added a commit that referenced this issue May 13, 2024
The logic for deleting an individual NPL mapping was broken. It
incorrectly believed that the protocol socket was still in use, and the
mapping could never be deleted, putting the NPL controller in an endless
error loop.

The State field in ProtocolSocketData was left over from pre Antrea
v1.7, back when we would always use the same port number for multiple
protocols, for a give Pod IP + port. With the current version of the NPL
implementation, this field is not needed and should be removed. By
removing the field, we avoid the deletion issue.

This patch also ensures that if a rule is only partially cleaned-up, we
can attempt to delete it again, by making DeleteRule idempotent. To
identify that a prior deletion attempt failed, we introduce a "defunct"
field in the NPL rule data. If this field is set, the controller knows
that the rule has been partially deleted and deletion needs to be
attempted again. Without this, it would be possible for the controller
(with the right sequence of updates) to assume that a partially-deleted
rule is still valid, which would break the datapath. I plan on improving
the NPL code further with a follow-up patch, but in order to keep this
patch small (for back-porting), I went with the simplest solution I
could think of.

Fixes #6281

Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/proxy/nodeportlocal Issues or PRs related to the NodePortLocal feature kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant