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

Add support for ipv6 #114

Merged
merged 5 commits into from
Aug 18, 2022
Merged

Add support for ipv6 #114

merged 5 commits into from
Aug 18, 2022

Conversation

dddddai
Copy link
Member

@dddddai dddddai commented Apr 8, 2022

To enable ipv6 only, just add the following env in the daemon set:

env:
  - name: ENABLE_IPV4
    value: "false"
  - name: ENABLE_IPV6
    value: "true"

Note that ipv6 requires cni-mode enabled

@dddddai dddddai requested review from a team as code owners April 8, 2022 03:03
@mergify
Copy link

mergify bot commented Apr 8, 2022

Welcome to the Merbridge OpenSource Community!👏

We're delighted to have you onboard 💘

bpf/mb_connect.c Outdated
struct origin_info origin = {.ip = ctx->user_ip4,
.port = ctx->user_port};
struct origin_info origin;
__builtin_memset(&origin, 0, sizeof(origin));
Copy link
Member Author

Choose a reason for hiding this comment

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

This pattern avoids padding issues, otherwise there would be a verifier error
See iovisor/bcc#2623 (comment)

Copy link
Member

@kebe7jun kebe7jun left a comment

Choose a reason for hiding this comment

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

I think maybe we don't need the ENABLE_IPV4 macro, it's probably there by default.

bpf/headers/helpers.h Outdated Show resolved Hide resolved
bpf/headers/maps.h Outdated Show resolved Hide resolved
bpf/mb_connect.c Outdated Show resolved Hide resolved
@dddddai
Copy link
Member Author

dddddai commented Apr 9, 2022

I think maybe we don't need the ENABLE_IPV4 macro, it's probably there by default.

I'm not sure if we are gonna support dual stack, if yes, the v4 macro would be helpful

ipv4 ipv6
enabled disabled
disabled enabled
enabled enabled

@dddddai dddddai force-pushed the ipv6 branch 3 times, most recently from 6e80ef6 to 3a59641 Compare April 9, 2022 14:55
Copy link
Member

@kebe7jun kebe7jun left a comment

Choose a reason for hiding this comment

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

lgtm

just some nits.

bpf/headers/helpers.h Outdated Show resolved Hide resolved
bpf/headers/helpers.h Outdated Show resolved Hide resolved
@dddddai dddddai force-pushed the ipv6 branch 3 times, most recently from 96007c7 to c307a37 Compare April 12, 2022 07:37
@kebe7jun
Copy link
Member

kebe7jun commented Apr 12, 2022

enable dual stack and run prog error:

clang -O2 -g  -Wall -target bpf -I/usr/include/x86_64-linux-gnu  -DMESH=1 -DENABLE_IPV4=1 -DENABLE_IPV6=1 -DDEBUG -DUSE_RECONNECT -c mb_connect.c -o mb_connect.o
clang -O2 -g  -Wall -target bpf -I/usr/include/x86_64-linux-gnu  -DMESH=1 -DENABLE_IPV4=1 -DENABLE_IPV6=1 -DDEBUG -DUSE_RECONNECT -c mb_get_sockopts.c -o mb_get_sockopts.o
mb_get_sockopts.c:76:29: error: redefinition of 'origin'
        struct origin_info *origin =
                            ^
mb_get_sockopts.c:49:29: note: previous definition is here
        struct origin_info *origin =
                            ^

@dddddai
Copy link
Member Author

dddddai commented Apr 12, 2022

enable dual stack and run prog error:

Fixed, BTW could you test this without iptables/ip6tables? I have some problems cleaning iptables...

@kebe7jun
Copy link
Member

enable dual stack and run prog error:

Fixed, BTW could you test this without iptables/ip6tables? I have some problems cleaning iptables...

I am testing it.

Copy link
Member Author

@dddddai dddddai left a comment

Choose a reason for hiding this comment

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

Hello @kebe7jun, it's ready for review now and I've tested it, PTAL when you get a chance

printk("write cookie_original_dst failed");
return 0;
}
// TODO(dddddai): add support for annotations
Copy link
Member Author

Choose a reason for hiding this comment

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

ipv6 annotations are not supported yet, will follow up

@kebe7jun
Copy link
Member

Hello @kebe7jun, it's ready for review now and I've tested it, PTAL when you get a chance

Great work!

conn4 := fmt.Sprintf("%s/%s/net/tcp", config.HostProc, pid)
return !findStr(conn4, []byte(fmt.Sprintf(": %0.8d:%0.4X %0.8d:%0.4X 0A", 0, 15001, 0, 0)))
if config.EnableIPV4 {
conn4 := fmt.Sprintf("%s/%s/net/tcp", config.HostProc, pid)
Copy link
Member

Choose a reason for hiding this comment

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

the listener should also consider ipv6?

bpf/mb_sockops.c Show resolved Hide resolved
return !findStr(conn4, []byte(fmt.Sprintf(": %0.8d:%0.4X %0.8d:%0.4X 0A", 0, 15001, 0, 0)))
if config.EnableIPV4 {
conn4 := fmt.Sprintf("%s/%s/net/tcp", config.HostProc, pid)
return !findStr(conn4, []byte(fmt.Sprintf(": %0.8d:%0.4X %0.8d:%0.4X 0A", 0, 15001, 0, 0)))
Copy link
Member

Choose a reason for hiding this comment

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

Is it not supposed to use 15001 directly, in kuma or linkerd will not work.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry I don't know much about kuma and linkerd...
Could we focus on istio in this pr and support others later?

Copy link
Member

Choose a reason for hiding this comment

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

that's ok.

Copy link
Contributor

Choose a reason for hiding this comment

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

Give me some time, I'll look at it

@@ -29,6 +33,8 @@ var (
UseReconnect = true
Debug = false
EnableCNI = false
EnableIPV4 = getEnvOrDefault("ENABLE_IPV4", "true") == "true"
EnableIPV6 = getEnvOrDefault("ENABLE_IPV6", "false") == "true"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe using command line parameters will better?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought so but I'm a bit concerned about:

lifecycle:
preStop:
exec:
command:
- make
- -k
- clean

It can't get flags from the command params, so I use env variables instead, though merbridge will clean up eBPF stuff when it exits
if err = ebpfs.UnLoadMBProgs(); err != nil {

@codecov-commenter
Copy link

Codecov Report

Merging #114 (284cfa5) into main (ffc42b4) will increase coverage by 0.72%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #114      +/-   ##
==========================================
+ Coverage   41.10%   41.83%   +0.72%     
==========================================
  Files           6        6              
  Lines         399      404       +5     
==========================================
+ Hits          164      169       +5     
  Misses        221      221              
  Partials       14       14              
Flag Coverage Δ
unittests 41.83% <100.00%> (+0.72%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/linux/ip.go 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@mergify mergify bot merged commit 7fd1fdd into merbridge:main Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants