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

Record Status when EgressIP is not available on any Node incase of static Egress #6228

Open
Atish-iaf opened this issue Apr 16, 2024 · 8 comments · May be fixed by #6661
Open

Record Status when EgressIP is not available on any Node incase of static Egress #6228

Atish-iaf opened this issue Apr 16, 2024 · 8 comments · May be fixed by #6661
Assignees
Labels
area/transit/egress Issues or PRs related to Egress (SNAT for traffic egressing the cluster). kind/documentation Categorizes issue or PR as related to a documentation. kind/feature Categorizes issue or PR as related to a new feature.

Comments

@Atish-iaf
Copy link
Contributor

Atish-iaf commented Apr 16, 2024

Describe the problem/challenge you have
In case of static egress, sometimes user may provide EgressIP which isn't assigned to any Node by mistake or EgressIP was later removed from the assigned Node. Currently, there is no useful info for troubleshooting this.

kubectl get egress                     
NAME            EGRESSIP   AGE     NODE
egress-static              2m16s   
kubectl describe egress egress-static  
Name:         egress-static
Namespace:    
Labels:       app=egress
Annotations:  <none>
API Version:  crd.antrea.io/v1beta1
Kind:         Egress
Metadata:
  Creation Timestamp:  2024-04-16T06:10:11Z
  Generation:          1
  Resource Version:    52666
  UID:                 985974d8-b98f-443d-bcf3-c1b5c8cfc95c
Spec:
  Applied To:
    Pod Selector:
      Match Labels:
        App:  nginx1
  Egress IP:  172.18.0.6
Events:       <none>

Describe the solution you'd like
It may be useful to provide Status for static Egress case as well like we provide for Egress with externalippool.

Status:
  Conditions:
    Last Transition Time:  2024-04-16T05:57:47Z
    Message:               Failed to assign the IP to EgressNode: no Node available
    Reason:                AssignmentError
    Status:                False
    Type:                  IPAssigned

Anything else you would like to add?
For Egress with ExternalIPPool, when EgressIP isn't assigned to any Node, Egress is not applied to Pod. Pod to external traffic goes via host NodeIP SNAT (Normal encap mode).
But this is not the case for static Egress, when EgressIP isn't available on any Node, still Egress is applicable to Pod and Pod to external traffic is unsuccessful. We can take reference from egress with externalIPPool and not apply egress to pod until egressIP is available on a Node for static egress as well.

@Atish-iaf Atish-iaf added kind/feature Categorizes issue or PR as related to a new feature. area/transit/egress Issues or PRs related to Egress (SNAT for traffic egressing the cluster). labels Apr 16, 2024
@rajnkamr
Copy link
Contributor

rajnkamr commented Apr 17, 2024

Usually Egress with ExternalIPPool should be in same subnet as node's interface, however it is not mandatory case, and ExternalIPPool could also be in different subnet than node's network, However the egress interface and ip will be assigned to egress dummy interface with the given subnet which leads to traffic going via actual node interface which could be in different subnet. In that case, does egress apply to pod if dummy interface is created and down !
Same for static egress , there might be case where static egress ip might not be same as node's ip.

@tnqn
Copy link
Member

tnqn commented Apr 17, 2024

Currently, there is no useful info for troubleshooting this.

I think the empty EgressIP and EgressNode already mean the IP is not on any Node, which is everything users need to know?

It may be useful to provide Status for static Egress case as well like we provide for Egress with externalippool.

It's kind of by design and I can't think of how we can make it more friendly. The point of static Egress is that user have full control on the Egress IPs, and none of the agent knows which Node the Egress IP should be on, then how they know when and which agent should update the condition to be False? And the point of the condition is to reduce the ambiguity of empty Node and to distinguish between "agent haven't tried to assign the IP" and "agent fail to assign the IP". For static Egress, I think there is no ambiguity, empty Node just means users the IP isn't assigned to any Node and agents are not going to do anything with it.

For Egress with ExternalIPPool, when EgressIP isn't assigned to any Node, Egress is not applied to Pod. Pod to external traffic goes via host NodeIP SNAT (Normal encap mode).
But this is not the case for static Egress, when EgressIP isn't available on any Node, still Egress is applicable to Pod and Pod to external traffic is unsuccessful. We can take reference from egress with externalIPPool and not apply egress to pod until egressIP is available on a Node for static egress as well.

This sounds reasonable to me. I'm not sure which behavior is better but we should make them consistent, and perhaps even make the behavior configurable, as some users may expect strict enforcement while some may expect best-effort.

@rajnkamr
Copy link
Contributor

But this is not the case for static Egress, when EgressIP isn't available on any Node, still Egress is applicable to Pod and Pod to external traffic is unsuccessful. We can take reference from egress with externalIPPool and not apply egress to pod until egressIP is available on a Node for static egress as well.

As rightly pointed by @tnqn, It seems right to me to address this issue as for static egress even though static egress is not applied to any node, pod to external traffic remain unsuccessful , we shld only apply egress to pod when egress ip is available on node, same behaviour as in case of ExternlIPPools, to make it consistent across.

@rajnkamr rajnkamr added action/release-note Indicates a PR that should be included in release notes. kind/documentation Categorizes issue or PR as related to a documentation. labels Jun 11, 2024
@rajnkamr rajnkamr assigned jainpulkit22 and unassigned KMAnju-2021 Jul 26, 2024
@rajnkamr rajnkamr added this to the Antrea v2.2 release milestone Jul 30, 2024
@luolanzone luolanzone removed the action/release-note Indicates a PR that should be included in release notes. label Aug 5, 2024
@rajnkamr rajnkamr assigned KMAnju-2021 and unassigned jainpulkit22 Aug 12, 2024
@antoninbas
Copy link
Contributor

For Egress with ExternalIPPool, when EgressIP isn't assigned to any Node, Egress is not applied to Pod. Pod to external traffic goes via host NodeIP SNAT (Normal encap mode).
But this is not the case for static Egress, when EgressIP isn't available on any Node, still Egress is applicable to Pod and Pod to external traffic is unsuccessful. We can take reference from egress with externalIPPool and not apply egress to pod until egressIP is available on a Node for static egress as well.

This sounds reasonable to me. I'm not sure which behavior is better but we should make them consistent, and perhaps even make the behavior configurable, as some users may expect strict enforcement while some may expect best-effort.

I am trying to understand what we want to "correct" here.
It feels to me that the behavior is already consistent across both cases: as long as the .spec.EgressIP field is populated, we will program the datapath on all cluster Nodes, regardless of whether or not the IP has effectively been assigned to the Egress Node. Is that not correct?
The only difference is that in the HA case (with ExternalIPPool), .spec.EgressIP is not populated if the pool is exhausted, and in that case we will use Node SNAT. Is that the specific case we are trying to address?

@Atish-iaf
Copy link
Contributor Author

Atish-iaf commented Sep 25, 2024

It feels to me that the behavior is already consistent across both cases: as long as the .spec.EgressIP field is populated, we will program the datapath on all cluster Nodes, regardless of whether or not the IP has effectively been assigned to the Egress Node. Is that not correct?

This observation is correct in static egress case only.

Incase of egress with externalIPPool -

  1. Create an externalIPPool with some ipRanges and nodeSelector. Do not provide
    this nodeSelector label to any Node in the cluster.
  2. Create an egress and use above created externalIPPool in the egress.
  3. .spec.EgressIP gets some egressIP from externalIPPool.
  4. But actually egress is not applied to Pod, and Pod to external traffic works via Node SNAT. This is not the case with static egress and this is not consistent. In static egress, egress is effectively applied on Pod and Pod to external traffic fails in this case for static egress.
katish@R9CX49X7GC egress % kubectl describe egress
Name:         egress
Namespace:    
Labels:       <none>
Annotations:  <none>
API Version:  crd.antrea.io/v1beta1
Kind:         Egress
Metadata:
  Creation Timestamp:  2024-09-25T09:46:11Z
  Generation:          2
  Resource Version:    187909
  UID:                 ba21e865-f114-4f35-8102-31e466fcba3f
Spec:
  Applied To:
    Pod Selector:
      Match Labels:
        App:         nginx1
  Egress IP:         172.18.0.8
  External IP Pool:  external-ip-pool
Status:
  Conditions:
    Last Transition Time:  2024-09-25T09:46:11Z
    Message:               Failed to assign the IP to EgressNode: no Node available
    Reason:                AssignmentError
    Status:                False
    Type:                  IPAssigned
    Last Transition Time:  2024-09-25T09:46:11Z
    Message:               EgressIP is successfully allocated
    Reason:                Allocated
    Status:                True
    Type:                  IPAllocated
  Egress IP:               
  Egress Node:             
Events:                    <none>
katish@R9CX49X7GC egress % 
katish@R9CX49X7GC egress % kubectl get egress
NAME     EGRESSIP   AGE   NODE
egress              10m   

@rajnkamr
Copy link
Contributor

rajnkamr commented Sep 25, 2024

It feels to me that the behavior is already consistent across both cases: as long as the .spec.EgressIP field is populated, we will program the datapath on all cluster Nodes, regardless of whether or not the IP has effectively been assigned to the Egress Node. Is that not correct?

This observation is correct in static egress case only.

In externalIPPool case, egress is populated to .spec.EgressIP and later ip is also assigned to dummy interface (antrea-egress0).

In static egress case, .spec.EgressIP is populated but ip is never assigned to dummy egress interface(antrea-egress0) present on the node as ip is supposed to be already available on one of node's interface, also since user is already aware that ip is available on node.
Hence, in static egress, egress gets applied on Pod and Pod to external traffic fails.

@antoninbas
Copy link
Contributor

Thanks for the explanation @Atish-iaf.
Looking at the code more closely, it indeed confirms this behavior:

if isEgressSchedulable(egress) {
egressIP, egressNode, err, scheduled := c.egressIPScheduler.GetEgressIPAndNode(egressName)
if scheduled {
desiredEgressIP = egressIP
desiredNode = egressNode
} else {
scheduleErr = err
}
} else {
desiredEgressIP = egress.Spec.EgressIP
}

IMO, the one thing really worth doing is prevent Node SNAT (should be configurable, as Quan pointed out) for HA Egress, if the IP cannot be scheduled to any Node. That can help prevent unwanted behavior. Changing anything for static Egress may not be worth it. I don't think that "use Node SNAT for workloads selected by my static Egress until I actually assign the IP to one of the Nodes" is an actual behavior we want to support. It would also require the Egress controller (in the Agent) to consume the Status - in order to determine whether the static Egress IP has been manually assigned to a Node -, which may not be a big deal but this is not something the controller does at the moment (and controllers typically consume the Spec and update the Status). But if we really want to make the behavior consistent, then we would have to consume .Status.EgressNode and skip programming the Pod datapath until it becomes non empty. What do you think @tnqn ?

"prevent Node SNAT for HA Egress if the IP cannot be scheduled to any Node" can probably be addressed pretty easily. In any case @KMAnju-2021, all the changes should be in the Agent (pkg/agent/controller/egress/egress_controller.go). No change is required / desired in the Antrea Controller.

@KMAnju-2021
Copy link
Contributor

KMAnju-2021 commented Sep 25, 2024

Thanks for the explanation @Atish-iaf. Looking at the code more closely, it indeed confirms this behavior:

if isEgressSchedulable(egress) {
egressIP, egressNode, err, scheduled := c.egressIPScheduler.GetEgressIPAndNode(egressName)
if scheduled {
desiredEgressIP = egressIP
desiredNode = egressNode
} else {
scheduleErr = err
}
} else {
desiredEgressIP = egress.Spec.EgressIP
}

IMO, the one thing really worth doing is prevent Node SNAT (should be configurable, as Quan pointed out) for HA Egress, if the IP cannot be scheduled to any Node. That can help prevent unwanted behavior. Changing anything for static Egress may not be worth it. I don't think that "use Node SNAT for workloads selected by my static Egress until I actually assign the IP to one of the Nodes" is an actual behavior we want to support. It would also require the Egress controller (in the Agent) to consume the Status - in order to determine whether the static Egress IP has been manually assigned to a Node -, which may not be a big deal but this is not something the controller does at the moment (and controllers typically consume the Spec and update the Status). But if we really want to make the behavior consistent, then we would have to consume .Status.EgressNode and skip programming the Pod datapath until it becomes non empty. What do you think @tnqn ?

"prevent Node SNAT for HA Egress if the IP cannot be scheduled to any Node" can probably be addressed pretty easily. In any case @KMAnju-2021, all the changes should be in the Agent (pkg/agent/controller/egress/egress_controller.go). No change is required / desired in the Antrea Controller.

Thanks for the suggestions, i will look into it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/transit/egress Issues or PRs related to Egress (SNAT for traffic egressing the cluster). kind/documentation Categorizes issue or PR as related to a documentation. kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants