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 babel routing protocol #934

Merged
merged 5 commits into from
Aug 1, 2018
Merged

add support for babel routing protocol #934

merged 5 commits into from
Aug 1, 2018

Conversation

christf
Copy link
Member

@christf christf commented Nov 12, 2016

this PR contains first support for a network as described in the l3 master plan.
this requires preliminary packages for l3roamd, babel (git version), mmfd which are all contained in the frankfurt main package repository on https://github.com/freifunk-ffm/packages

This PR contains init scripts for mmfd, l3roamd, babel, a firewall and the necessary integration of these components.

This set now (2017-06-17) includes a proof of concept implementation of prefixd including a gui.

https://wiki.ffm.freifunk.net/firmware:babel

this replaces #732

@christf
Copy link
Member Author

christf commented Nov 18, 2016

this depends on #938

@christf
Copy link
Member Author

christf commented Nov 19, 2016

this also depends on #939

@christf christf mentioned this pull request Nov 19, 2016
8 tasks
@rotanid rotanid added the 3. topic: babel Topic: Babel Layer 3 Routing label Nov 23, 2016
@christf
Copy link
Member Author

christf commented Nov 25, 2016

this depends in #946

@christf christf changed the title add support for babel routing protocol WIP add support for babel routing protocol Nov 29, 2016
@christf christf changed the title WIP add support for babel routing protocol [WIP] add support for babel routing protocol Nov 29, 2016
@christf
Copy link
Member Author

christf commented Nov 30, 2016

test was successful. there is a new status in the wiki.

Copy link
Contributor

@T-X T-X left a comment

Choose a reason for hiding this comment

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

Why disable bridge multicast snooping?

@christf
Copy link
Member Author

christf commented Nov 30, 2016

@T-X unfortunately roaming does not work because the IPv6 address of the clients cannot be properly detected when igmp snooping is on. IPv6 prefixes also are not properly announced making the network next to unusable.
with igmp snooping off, things suddenly start to work. am guessing this has something to do with https://bugzilla.kernel.org/show_bug.cgi?id=99081

@T-X
Copy link
Contributor

T-X commented Nov 30, 2016

For that thing on bugzilla, no one was able to name a way to reproduce the alleged problem in over 1.5 years. So I do not see any similarities yet.

How are IPv6 addresses detected by l3roamd? Is the issue no detection upon roaming or a delayed detection?

@christf
Copy link
Member Author

christf commented Nov 30, 2016

l3roamd will query for IPv6 addresses if it receives a packet directed towards an address it does not already know. It does this by using a multicast to ff02:... and this never reaches all necessary devices.

@T-X
Copy link
Contributor

T-X commented Nov 30, 2016

Can you creature a capture of these multicast probing packets send by l3roamd and any MLD+ND messages with tcpdump?

"this never reaches all necessary devices" means no to my previous, second question? Does l3roamd send the probing packets once only, without any retries?

Edit: If you perform a tcpdump on the bridge or a bridge port, do it with "tcpdump -p" (= --no-promiscuous-mode) please.

@christf
Copy link
Member Author

christf commented Dec 8, 2016

squashing all commits as this log will not rebase properly due to internal conflicts.

@jplitza
Copy link
Member

jplitza commented Dec 9, 2016

Because of the changes in #972, package/gluon-mesh-babel/files/lib/gluon/respondd/client.dev now needs to contain a uci name of the interface.

@christf
Copy link
Member Author

christf commented Dec 13, 2016

@jplitza thank you for the hint, the file is now adapted to #972

@christf christf mentioned this pull request Dec 21, 2016
19 tasks
case "$ACTION"
in
ifup)
echo 1 > /sys/devices/virtual/net/br-client/bridge/multicast_snooping
Copy link
Contributor

@T-X T-X Jan 20, 2017

Choose a reason for hiding this comment

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

File name seems to need an update, too. Also, multicast_snooping is enabled by default in the kernel (and OpenWRT+LEDE too), does it need to be set explicitly?

However, multicast_querier is not enabled by default (and I think the gluon-batman-adv packages do this?).

(Edit: on the other hand, while multicast_querier is not enabled by default in the vanilla kernel, netifd enables it by default in OpenWRT/LEDE. So probably not needed to be set explicitly either)

Copy link
Member

Choose a reason for hiding this comment

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

The latest netifd version in LEDE has disabled multicast_snooping for the LEDE release because it causes too many issues. I think we should do the same in Gluon for now... :/

Copy link
Contributor

Choose a reason for hiding this comment

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

What, why did nobody tell me? (ok, now you did :D)

Can you point me to some ticket?

Copy link
Member

Choose a reason for hiding this comment

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

@christf
Copy link
Member Author

christf commented Feb 16, 2017

since some of the changes in the pr are already merged I am compiling a list... :)

@neocturne neocturne added this to the 2017.1 milestone Feb 21, 2017
proto = 'udp',
target = 'ACCEPT',
}
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we assume that the incoming fastd connections are limited to ports 100010-10010?
Are any other communities using different ports?

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 is about outgoing connections, other than that we probably cannot assume this.....
maybe the ports should be generated from site.conf

Good catch!

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 is done now.

sprintf(mac1, "%02x:%02x:%02x:%02x:%02x:%02x",
orig[0], orig[1], orig[2], orig[3], orig[4], orig[5]);
sprintf(mac1, "%02x:%02x:%02x:%02x:%02x:%02x",
orig[0], orig[1], orig[2], orig[3], orig[4], orig[5]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this debug or an actual output?

Copy link
Member Author

Choose a reason for hiding this comment

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

none of the above.. this writes to a variable instead of stdout or stderr....

Copy link
Contributor

@Brother-Lal Brother-Lal Mar 21, 2017

Choose a reason for hiding this comment

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

see, thats why i said i write, but never understand ...
Edit: i'm missing the head->wall smiley on github...

count_stations(&wifi24, &wifi5);

// size_t total = atoi(get_line_from_run("exec sh -c 'ip -6 r s t 0 |grep 2a06|grep -v mesh-vpn|grep -v unreachable|grep -v /|wc -l'"));
size_t total = get_all_client_count();
Copy link
Contributor

@Brother-Lal Brother-Lal Mar 21, 2017

Choose a reason for hiding this comment

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

I have a funny phenomenon here:
Sample from respondd output:
data: {"clients":{"total":4,"wifi":2,"wifi24":2,"wifi5":0}
I only have 2 clients connected via wifi, but the total seems to be 4.
Might it be that get_all_client_count actually counts all ipv6 adresses on the interfaces, not clients, and as all my devices have 2 ipv6 adresses if connected to the network -> 4 instead of 2.
Is this intended?

Copy link
Contributor

Choose a reason for hiding this comment

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

Additional:
data: {"clients":{"total":0,"wifi":1,"wifi24":1,"wifi5":0}
Opposite seems to be true too, it might be that the wifi connection is still open, but the route might be gone?
Mobile phone was logged in for ~10 min, probably gone to sleep/energy save.
As soon as i switched the screen on, the info was suddenly correct:
{"clients":{"total":1,"wifi":1,"wifi24":1,"wifi5":0}

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 indeed is a bug due to the way clients are counted. at the moment the logic is counting only ip-addresses. Some smarter mechanism should be found for that.

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 is done now. respondd will query l3roamd in the latest version.

@christf
Copy link
Member Author

christf commented Apr 25, 2017

@NeoRaider would df76c3d be something you feel could fit into master? The babel branch introduces a firewall that seems to be usable in the batman-world as well.

@neocturne
Copy link
Member

@christf Hmm, a similar change would have to be done for the tunneldigger support. I'd love to just use the gluon-mesh-vpn user group for the firewall rule instead of dynamically creating port rules... but that won't work for tunneldigger either, as the actual tunnels are in the kernel and will not have their sockets opened by the correct group.

In any case, the whole WAN firewall question is completely independent of Babel: both Babel and batman-adv can work with the current (unrestricted) firewall, or with a more restrictive firewall, so a separate PR to discuss this in would be great.

@neocturne neocturne removed this from the 2017.1 milestone May 30, 2017
@rotanid rotanid added the 2. status: merge conflict The merge has a conflict and needs rebasing label Jul 11, 2018
@@ -16,6 +16,13 @@ uci:delete('dhcp', dnsmasq, 'cachesize')

uci:delete('firewall', 'client_dns')
if dns.servers then
localipv6 = uci:get("network", "loopback", "ip6addr")
Copy link
Member

Choose a reason for hiding this comment

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

indentation wrong?

include ../gluon.mk

define Package/gluon-mesh-babel
SECTION:=gluon
Copy link
Member

Choose a reason for hiding this comment

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

please change all packages in your PR according to 994c949

@@ -0,0 +1,105 @@
#!/usr/bin/lua

-- TODO: replace REJECT by DROP. only use REJECT for debugging
Copy link
Member

Choose a reason for hiding this comment

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

do we really want to merge code that contains TODO statements?

@@ -0,0 +1,23 @@
#!/usr/bin/lua
-- TODO: Migrate this into uci
Copy link
Member

Choose a reason for hiding this comment

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

do we really want to merge code that contains TODO statements?

Copy link
Member Author

Choose a reason for hiding this comment

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

well, I have to say that I am not sure about the uci strategy. 2 years ago I learned from neoraider that we would be moving away from it. I left this TODO he to force a decision. In this case I think it is ok to not do uci.

Copy link
Member

Choose a reason for hiding this comment

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

my two review-comments about the TODOs aren't saying what you have to do, only that you have to decide one way or the other and if you want to solve it later, remove the TODO and add a new issue to gluon about this rather than abusing comments for it

CFLAGS += -Wall -g -fPIC -D_GNU_SOURCE

ifeq ($(origin PKG_CONFIG), undefined)
PKG_CONFIG = pkg-config
Copy link
Member

Choose a reason for hiding this comment

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

please compare this Makefile's indentation to that of other gluon packages

return true;
}


Copy link
Member

Choose a reason for hiding this comment

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

some of your new files contain one or even two newlines at the end, others don't ?

@rotanid rotanid removed the 2. status: merge conflict The merge has a conflict and needs rebasing label Jul 16, 2018
endef

define Build/Prepare
mkdir -p $(PKG_BUILD_DIR)
Copy link
Member

Choose a reason for hiding this comment

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

a space too much



define Package/$(PKG_NAME)
SECTION:=gluon
Copy link
Member

Choose a reason for hiding this comment

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

please change all packages in your PR according to 994c949

Package working around icmp blackholes in the internet.
endef

define Build/Configure
Copy link
Member

Choose a reason for hiding this comment

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

can be removed as done by 934221b treewide

$(CP) ./src/* $(PKG_BUILD_DIR)/
endef

define Build/Configure
Copy link
Member

Choose a reason for hiding this comment

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

can be removed as done by 934221b treewide

PKG_VERSION:=$(if $(DUMP),x,$(GLUON_VERSION))


PKG_BUILD_DIR := $(BUILD_DIR)/$(PKG_NAME)
Copy link
Member

Choose a reason for hiding this comment

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

see 934221b


define Build/Prepare
mkdir -p $(PKG_BUILD_DIR)
$(CP) ./src/* $(PKG_BUILD_DIR)/
Copy link
Member

Choose a reason for hiding this comment

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

can be removed as done by 934221b treewide

endef

define Build/Compile
$(call GluonSrcDiet,./luasrc,$(PKG_BUILD_DIR)/luadest/)
Copy link
Member

Choose a reason for hiding this comment

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

see 60522ee

endef

define Package/$(PKG_NAME)/install
$(CP) ./files/* $(1)/
Copy link
Member

Choose a reason for hiding this comment

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

see 60522ee

PKG_VERSION:=1

PKG_BUILD_DIR := $(BUILD_DIR)/$(PKG_NAME)
PKG_BUILD_DEPENDS := respondd
Copy link
Member

Choose a reason for hiding this comment

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

endef

define Build/Compile
$(call Build/Compile/Default)
Copy link
Member

Choose a reason for hiding this comment

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

$(INSTALL_BIN) $(PKG_BUILD_DIR)/neighbours-babel $(1)/lib/gluon/status-page/providers/
endef

define Package/gluon-mesh-babel/postinst
Copy link
Member

Choose a reason for hiding this comment

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

endef

define Package/gluon-mesh-babel/install
$(CP) ./files/* $(1)/
Copy link
Member

Choose a reason for hiding this comment

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

@rotanid
Copy link
Member

rotanid commented Jul 30, 2018

in addition to what i commented before i noticed that some of the treewide changes that went into Gluon 2018.1 weren't applied to this PR yet.
i made these changes from 994c949 and 934221b and 60522ee
and also fixed the other change requests i had, see commit log.

so this PR has two things left:

  1. check/build with these changes
  2. resolve the TODO regarding DROP/REJECT in firewall

@christf
Copy link
Member Author

christf commented Jul 30, 2018

(2) is done. I was not sure in the first place if we really should use DROP, looking at gluon-core, REJECT is used there so I have removed this TODO to match policies.

@christf
Copy link
Member Author

christf commented Aug 1, 2018

tested and ready for merge. /1) is done.

@rotanid rotanid merged commit a304814 into freifunk-gluon:master Aug 1, 2018
@christf christf deleted the n_babel-work branch August 1, 2018 23:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. topic: babel Topic: Babel Layer 3 Routing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants