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

feat(bgp): logger & enhance nadProvider config #4352

Merged
merged 5 commits into from
Aug 21, 2024

Conversation

SkalaNetworks
Copy link
Contributor

This PR makes the configuration of the BGP NAT GW easier, we're dropping the nadName field in the configmap and using only the provider instead. Name of the NAD is inferred from the name of the provider.

I also added a custom logger to GoBGP so we can change the verbosity of GoBGP on the fly.
Klog verbosity over 3 makes GoBGP go to trace levels. This is useful to see what's going on when peering is not working.

Signed-off-by: SkalaNetworks <[email protected]>
Signed-off-by: SkalaNetworks <[email protected]>
@SkalaNetworks
Copy link
Contributor Author

The "make lint" doesn't fix the indentation by running gofumpt (which has been removed), so I had to do it manually. I think the developper experience could be improved by making the Makefile do that automatically.

@oilbeater
Copy link
Collaborator

The "make lint" doesn't fix the indentation by running gofumpt (which has been removed), so I had to do it manually. I think the developper experience could be improved by making the Makefile do that automatically.

The golangci-lint has a --fix option, I will check if it can work for local dev and ci (We still need to failed the ci if lint issue exists).

@ah8ad3
Copy link

ah8ad3 commented Aug 9, 2024

Thanks @SkalaNetworks for amazing work, i have a quick question. I tried using the last PR you implemented.
After deploying the nat gw with dns configuration it has 4 interfaces, local, external, internal, api-server right?
I couldn't expose eip via bgp, because i read the code there might be some parts that i am missing, can you provide some docs on what was the design?
Thanks.

@SkalaNetworks
Copy link
Contributor Author

@ah8ad3 would this help you? kubeovn/docs#186
What exactly did you fail to do? Announce the EIPs?

@SkalaNetworks
Copy link
Contributor Author

Also @oilbeater is this PR mergeable? @ah8ad3 you'll need this patch to follow the documentation I sent you, the configmap changed a bit compared to what's currently merged in master

@ah8ad3
Copy link

ah8ad3 commented Aug 11, 2024

@ah8ad3 would this help you? kubeovn/docs#186
What exactly did you fail to do? Announce the EIPs?

Thanks, I think the most problem with my setup was that the speaker couldn't reach the neighbour and there was no log emphasize that problem, I will try with verbose arg that you provided. But out of curiosity can neighbour be the node speaker? Or should it be a router outside node?

@SkalaNetworks
Copy link
Contributor Author

It should be the router outside the node, the same one used by the node speakers. It needs to be on the external network added to it.

There's indeed 4 interfaces:

  • lo
  • net1 aka the external network, on which the BGP speaker will connect to your router speaker
  • eth0 aka the network behind the NAT GW hosting your pods
  • net2 aka the API network, used by the BGP speaker to poll for EIPs

@ah8ad3
Copy link

ah8ad3 commented Aug 11, 2024

Please ignore if my ideas are not correct i'm new here.
I have a setup which 1 nat-gw with 1 eip has been created.
The eip is 10.0.13.7 and the default ip for nat-gw is 10.0.13.6

  1. When we create the nat-gw, 1 ip will be added to the net1 by default. I don't think that is necessary anymore, am i right?
 142: net1@if143: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1400 qdisc noqueue state UP group default 
    link/ether 62:82:11:f9:21:71 brd ff:ff:ff:ff:ff:ff link-netnsid 0
    inet 10.0.13.6/24 brd 10.0.13.255 scope global net1
       valid_lft forever preferred_lft forever
    inet 10.0.13.7/24 scope global secondary net1
       valid_lft forever preferred_lft forever
    inet6 fe80::6082:11ff:fef9:2171/64 scope link proto kernel_ll 
       valid_lft forever preferred_lft forever
  1. When net1 in nat-gw has an predefined ip the next hop for the eip bgp is that ip
    announcing route with prefix 10.0.13.7/32 and nexthop: 10.0.13.6
    But when you create a subnet it will add the routing to 10.0.13.0/24 via 100.64.0.1 dev ovn0

So when you ping the 10.0.13.7 from host it reaches.

ping 10.0.13.7
PING 10.0.13.7 (10.0.13.7) 56(84) bytes of data.
64 bytes from 10.0.13.7: icmp_seq=1 ttl=63 time=2.29 ms
64 bytes from 10.0.13.7: icmp_seq=2 ttl=63 time=1.60 ms

And the route goes through

traceroute 10.0.13.7
traceroute to 10.0.13.7 (10.0.13.7), 30 hops max, 60 byte packets
 1  100.64.0.1 (100.64.0.1)  7.255 ms  7.514 ms  7.845 ms
 2  10.0.13.7 (10.0.13.7)  6.472 ms  6.452 ms  6.462 ms

So maybe next hop there can be fixed, or we can remove the default ip in nat-gw?

@SkalaNetworks
Copy link
Contributor Author

  1. It's still added for legacy purposes, if the NAT GW is not in BGP mode, the bash script adding the EIP to the interface is still called. If your BGP speaker is connected to a daemon like FRR's zebra, the route for the /32s should be appended to your RIB, so obviously adding the IPs to respond to ARP is unnecessary, but it does no harm.
  2. I don't exactly see the problem here, the next hop announced for your EIP is indeed the IP of the NAT GW (the .6), which is correct considering it is gonna do the NAT of the EIP to redirect the traffic to the pod that has a SNAT/DNAT on that EIP. This pod is in the "private" subnet attached to the NAT GW.

One important thing to note that might not be clear in the VPC doc:

  • The default GW in the subnet is "invisible", and is not replaced by the NAT GW when you put one. The NAT GW should run on a different IP from the default GW of the subnet.
  • The traffic will always go to the default GW, and then a custom route needs to be appended to reroute everything to the NAT GW
    Something like that in the VPC, with .70 being the IP of my NAT GW.
image

@SkalaNetworks
Copy link
Contributor Author

@oilbeater @zhangzujian just pinging you to know if something is holding this PR, it's in line with the documentation already pushed in the main branch (on the repo doc), but the code is not yet merged.

@oilbeater
Copy link
Collaborator

LGTM @zhangzujian do you have any other comment

pkg/speaker/config.go Outdated Show resolved Hide resolved
@zhangzujian zhangzujian merged commit 6bfbfe8 into kubeovn:master Aug 21, 2024
60 of 62 checks passed
bobz965 pushed a commit that referenced this pull request Aug 22, 2024
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.

4 participants