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: add default route on customer vnets and provide default route from CNS to CNI #3027

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

paulyufan2
Copy link
Contributor

@paulyufan2 paulyufan2 commented Sep 23, 2024

Reason for Change:

This PR is to fix one issue in swiftv2 Windows scenario:
When a pod is created, the default route is added on infra vnet:
root@swiftv2-pod-3:/# ip route
default via 10.244.2.1 dev eth0 metric 1
It leads to ping a VM IP in the same VNET that cannot work.

There are two issues::
1.CNS does not provide the default route to CNI;
2.CNI should only add the default route to secondary interface customer vnet; on Swiftv2 scenario, skipDefaultRoutes is set to true for infraNIC interface and false for a secondary interface; so if !info.skipDefaultRoutes, then add default route.

Issue Fixed:

Requirements:

Notes:

@paulyufan2 paulyufan2 added cni Related to CNI. fix Fixes something. labels Sep 23, 2024
@paulyufan2 paulyufan2 marked this pull request as ready for review September 23, 2024 16:02
@paulyufan2 paulyufan2 requested a review from a team as a code owner September 23, 2024 16:02
Copy link
Member

@tamilmani1989 tamilmani1989 left a comment

Choose a reason for hiding this comment

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

how it works for linux not windows? why we need this change specifically for windows. also feel this will regress linux swiftv2

@paulyufan2
Copy link
Contributor Author

/azp run Azure Container Networking PR

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@paulyufan2 paulyufan2 changed the title fix: add default route on customer vnets fix: add default route on customer vnets and provide default route from CNS to CNI Sep 26, 2024
@paulyufan2 paulyufan2 added the cns Related to CNS. label Sep 26, 2024
@paulyufan2
Copy link
Contributor Author

/azp run Azure Container Networking PR

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@@ -428,7 +428,7 @@ func (nw *network) newEndpointImplHnsV2(cli apipaClient, epInfo *EndpointInfo) (
}

// Create the HCN endpoint.
logger.Info("Creating hcn endpoint", zap.Any("hcnEndpoint", hcnEndpoint), zap.String("computenetwork", hcnEndpoint.HostComputeNetwork))
logger.Info("Creating hcn endpoint", zap.Any("hcnEndpoint", hcnEndpoint), zap.String("computenetwork", hcnEndpoint.HostComputeNetwork), zap.Any("routes", hcnEndpoint.Routes))
Copy link
Member

Choose a reason for hiding this comment

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

can skip logging routes if hcnendpoint object already logs it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

zap log does not log Routes field from hcnEndpoint

@paulyufan2 paulyufan2 force-pushed the fixdefaultRouteSwiftv2windows branch 3 times, most recently from 24dc6b2 to 43cd9da Compare October 15, 2024 16:24
@paulyufan2 paulyufan2 force-pushed the fixdefaultRouteSwiftv2windows branch 4 times, most recently from a14aeff to 2a7940f Compare November 4, 2024 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cni Related to CNI. cns Related to CNS. fix Fixes something.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants