Make vpn-select modular #8
No reviewers
Labels
No Label
RFC
RFT
WIP
blocked
bsp
bug
build/scripts/tools
duplicate
feature
fixed
layer3
mantis
more details required
needs changes
node
packages/fff
rejected
security
trivial
upstream
No Milestone
No Assignees
4 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: freifunk-franken/firmware#8
Loading…
Reference in New Issue
No description provided.
Delete Branch "rohammer:vxlan-node"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
vpn-select is an old relic and did not reflect the opportunities of our hoodfile.
This rewrite makes vpn-select modular to easely add new vpn-protocols.
The stuff dependent on the vpn-protocol is outsourced to files in /usr/lib/vpn-select.d/ and comes in with the respective vpn package.
vpn-stop is removed to use the protocol independent start/stop mechanism of vpn-select. Instead, a symlink is used.
We should avoid paralell vpns to the same gateway and not mix the protocol per hood. It is therefore necessary that all gateways offer the same protocols in one hood.
The most prefered protocol supported and avaylable in the hood is selected. The order for this is defined in "uci fff.vpnselect.protocol_order" eg. "vxlan l2tp fastd"
Signed-off-by: Robert Langhammer rlanghammer@web.de
c3d19ed4b7
to9cc338534f
Hallo, ein prefix für den Commit-Titel wäre schön, also "fff-vpn-select: make vpn-select modular" oder so.
@ -0,0 +1,5 @@
uci set fff.vpnselect=fff
uci set fff.vpnselect.protocol_order="vxlan fastd"
Eigenlich unterstützen wir noch kein vxlan, insofern würde ich das hier rausnehmen, bis es offiziell unterstützt wird.
jo, kann ich raus nehmen. Kommt dann mit dem vxlan Patch wieder rein.
@ -12,0 +27,4 @@
# compare $protocol_order with $supported_protocols
for proto in $protocol_order; do
echo "$supported_protocols" | grep $proto >&/dev/null || protocol_order=${protocol_order/$proto/}
Hier würde ggf. auch z.B. "fastd" mit "fastd2" gematcht. Ggf. sollte man word-boundaries in den grep mit einführen.
eventuell lohnt sich ein blick auf
man 1 comm
oder aehnliches@ -12,0 +28,4 @@
# compare $protocol_order with $supported_protocols
for proto in $protocol_order; do
echo "$supported_protocols" | grep $proto >&/dev/null || protocol_order=${protocol_order/$proto/}
done
Wenn man voraussetzt, das jedes Protokoll in protocol_order eingetragen werden muss, könnte man das etwas vereinfachen:
Gefaellt mir gut. Tut genau was ich moechte. Das grep ist weg und es wird ev. nur gesourced, was gebraucht wird.
Vielen Dank!
Ach bloed, geht doch nicht. Ich brauche alle supported_protocols zum clear. Sonst können configs uebrig bleiben, wenn man ein protocol aus der Liste protocol_order loescht.
Wie wichtig ist es denn in der Praxis, dass man die Reihenfolge per uci ändern kann?
Könnte man nicht einfach die Reihenfolge per prefix vorgeben, wie bei uci-default auch?
Also 10-fastd und 20-vxlan?
Wir haben ja ohnehin schon zwei Orte (vpn-select.d und hood file), wo wir die Protokolle behandeln müssen. Ich weiß nicht, ob das Ganze besser wird, wenn man jetzt drei hat.
Alternativ könnte man sagen, das man das uci-Setting zum an-/abschalten behält, aber zumindest die Reihenfolge von den Dateien nimmt. Das wäre immer noch deutlich einfacher zu implementieren als mit einer echten protocol order.
Ich hab das ganze umgebaut um den vxlan Client zu machen. Darum habe ich darauf geachtet, dass es voellig unabhaengig voneinander ist. Zum Um/Ein/Ausschalten reicht eine uci Zeile. So kann ich zum testen auch einfach einen tarball auf den Router werfen. Die protocol_order koennte man auch als Hoodparameter vom KeyX beziehen. Wenn man das statischer macht, wird es nur auf den ersten Blick einfacher. Es war echt schick mit diesem Unterbau das vxlan Ding zu bauen. Man muss in keine bestehende Datei rein. Nur die protocol_order. Und die ist konfigurierbar.
Ich wuerde es gerne so offen wie moeglich lassen.
Ich kuck später noch mal, ob man da einen Kompromiss finden kann. Die Entropie ist ja hier recht hoch ...
Also in jedem Fall kann man die for-Schleife "#clear old config" wegmachen und vorziehen:
Den Rest muss ich mir noch überlegen, mir ist das insgesamt einfach eine Nummer zu kompliziert ...
@ -0,0 +32,4 @@
([ -s "$pidfile" ] && [ -d "/proc/$(cat "$pidfile")" ]) && /etc/init.d/fastd stop
fi
}
Unneeded empty line at EOF.
@ -4,0 +8,4 @@
# The function ${protocol}_addpeer() is called for every selected peer in hoodfile.
# The function ${protocol}_start_stop() is called at the end once per installed protocol.
#
# The variable $protocol_order defines the order of preference. The most preffered protocol of the avaylable and offered protocols is selected.
Typo: "available"
Hier könnte man auch dann zur Klarstellung darauf hinweisen, dass ein Protokoll hier genannt sein muss, damit es funktioniert.
@ -0,0 +5,4 @@
}
fastd_addpeer() {
[ -d /tmp/fastd_fff_peers ] || mkdir /tmp/fastd_fff_peers
Hmm, gibts es da theoretische Vor- oder Nachteile im Vergleich zu
mkdir -p /tmp/fastd_fff_peers ohne test?
mkdir -p
ist definitiv besser hier.Das fastd fasse ich in diesem Patch nicht an. Braucht einen eigenen.
@ -0,0 +17,4 @@
json_get_var port port
echo "remote \"${address}\" port ${port};" >> "$filename"
echo "" >> "$filename"
echo "float yes;" >> "$filename"
Ich frage mich gerade, ob es klug ist, hier zeilenweise zu schreiben. Da die Dateien aber auf /tmp im RAM liegen, ist es an der Stelle wahrscheinlich wurscht. Keine Ahnung ob die Buffer da Effekte haben...
Im Prinzip würde das ja genauso funktionieren?
echo "#name \"${servername}\";\nkey \"${key}\";\nremote \"${address}\" port ${port};\n\nfloat yes;" > "$filename"
Bitte aber nur als Diskussionsanregung verstehen. Sowas würde ich wenn überhaupt nur in einem separaten Patch anfassen wollen ...
Sowas kann man auch mal mit einem here-document loesen. Dann hat man nur ein cat und kann es trotzdem lesen, im Gegensatz zu diesem Einzeiler.
Der fastd Kram ist mehr oder weniger eine Kopie von der alten version. Das wollte ich nicht anfassen. Gehoert in einen extra Patch.
@ -50,2 +57,2 @@
make_config
/etc/init.d/fastd reload
# select most preferred protocol for this hood
selected_protocol=$(for proto in $protocol_order; do echo $offered_protocols | grep $proto >&/dev/null && echo $proto && break; done)
Statt prüfen, Ausgabe unterdrücken, und dann echo könnte man auch einfach den grep direkt das richtige ausgeben lassen (-o). Den Fehler kann man dann auch gleich mit (-q) unterdrücken:
selected_protocol=$(for proto in $protocol_order; do echo $offered_protocols | grep -o -q $proto && break; done)
Ansonsten gäbe es noch die klassische Version, hier mal primär als Diskussionsbeitrag:
grep -o -q , sehr schoen. Danke!
-o und -q kann man leider nicht kombinieren. -q gewinnt. Es kommt nix raus.
Ich hab es jetzt nur mit -o gemacht
Interessant, ich dachte -q wäre für stderr. Offenbar ist es für stdout ... Dann muss man ggf. noch den stderr auch abfangen (bzw. das mal testen).
Ich habe es getestet. stderr muss man nicht abfangen. Ein "no match" ist kein Fehler.
echo blob | grep foo liefert nie eine Ausgabe auf stderr.
I see, ich hatte das mit uci -q verwechselt...
Auch das -o muss ich wieder zurueck nehmen.
Bei 2 peers mit z.B. fastd.
und das matcht natuerlich nicht mit "fastd"
geht da -m1?
Nö, schade. Aber ich finde sowieso, dass das Ganze irgendwie auch eleganter gehen muss.
@ -65,0 +63,4 @@
json_get_var protocol protocol
[ "$protocol" = "$selected_protocol" ] && "${protocol}_addpeer"
json_select ".." # back to vpn
done
Der Ordnung halber könnte man hier noch das
json_select ".." # back to root
wieder einfügen, funktional dürfte es egal sein.Kurz danach ist das Skript zu ende. Spielt keine Rolle. Ich lass es so.
@ -0,0 +30,4 @@
([ -s "$pidfile" ] && [ -d "/proc/$(cat "$pidfile")" ]) || /etc/init.d/fastd start
else
([ -s "$pidfile" ] && [ -d "/proc/$(cat "$pidfile")" ]) && /etc/init.d/fastd stop
fi
Hmm, subshells. Ob das wieder ich war? Das sollte man auch mal ordentlich machen, aber auch hier separat.
PR #13
Also persönlich würde ich vpn-stop töten und das in configurehood anpassen.
@ -3,1 +3,4 @@
# Usage: vpn-select <path-to-hood-file>
# To add a new protocol, put a file with three functions to /usr/lib/vpn-select.d/ .
# The file must start with protocol=name. It is most important to use the same name here and in hoodfile.
# Some parameters of the hood are avaylable via $hood_id, $hood_name and $hood_protocol.
Typo: available
@ -16,0 +43,4 @@
json_select hood
json_get_var hood_id id
json_get_var hood_name name
json_get_var hood_protocol protocol
Benutzen wir die für irgendwas?
noch nicht. Die id braucht dann vxlan.
Ist zwar kleinkariert, aber würde ich dann auch erst dann einfügen. Macht auch die Änderungen in diesem Patch einfacher nachzuvollziehen.
Die Idee war, eine Basis zu schaffen, auf der jemand ein neues Protokoll hinzufuegen kann, ohne das package vpn-select anfassen zu muessen. Aber wenn es dann besser durchs Karo passt, kommt es weg.
Ich wuerde das gerne lassen, dann muss der vxlan Patch nicht in die vpn-select rein greifen.
Aber wenn der vxlan Patch diese Parameter plötzlich nutzt, dann ist doch der vxlan-Patch der Grund, warum die überhaupt da sind. Dann gehören die auch in den vxlan-Patch rein, damit man sie dort findet. D.h. du fasst ja doch im vxlan-Patch vpn-select an, weil du neue Abhängigkeiten erzeugt, du würdest es nur nicht hinschreiben.
Alternativ könnte man höchstens noch nur diese drei Zeilen in einen Extra-Patch tun, als separate "Vorbereitung" für vxlan.
Man koennte auch sagen, die Daten werden angeboten und koennen bei Bedaf genutzt werden. So ne Art Schnittstelle. Die VPNs koennen das nutzen was sie brauchen und sich konfigurieren.
Sieht gut aus. Die Remarks sind ja im Wesentlichen Gestaltungsfragen, grundsätzlich erscheint mir das erstrebenswert und sollte auch funktionieren.
@ -12,0 +25,4 @@
supported_protocols="$supported_protocols $protocol"
done
# compare $protocol_order with $supported_protocols
Also zumindest das würde ich mir gerne sparen:
Erstens würde ich erwarten, das man die Protokolle in protocol_order richtig eintragen kann. Wenn nicht, sollte das nicht silently entfernt werden, sondern dann später im Skript tatsächlich einen Fehler erzeugen, weil die Funktion/Datei nicht gefunden werden kann. So merkt man dann ggf. auch, das man Quatsch eingetragen hat (was in meinen Augen gut ist).
Damit haben wir schon mal eine for-Schleife weniger, die bei jedem Run von vpn-select läuft. Als Nebeneffekt können wir dann im Prinzip sogar die ganze supported_protocols Variable entfernen, und einfach am Anfang (clear protocols) und Schluss (apply protocols) jeweils einmal über /usr/lib/vpn-select.d/* iterieren. Das sollte dann auch insgesamt deutlich einfacher zu lesen und verstehen sein. Funktional ändert sich dadurch erstmal nichts, außer dass eben "falsche" Einträge in protocol_order dann Fehler erzeugen, und nicht einfach weggelöscht werden.
Ich versuch das mal genauer zu erklaeren, wie ich zu dieser Variante kam.
Der Mechanismus mit der protocol_order kam spaeter rein, als mir bewusst wurde, dass ein protokoll, in dem Fall vxlan, protokollspezifische Voraussetzungen pruefen muss. Sind diese nicht erfuellt, ist es noetig, dass es sich aus den moeglichen Protokollen entfernt.
In protocol_order steht also drin, was wir uns wuenschen. Wir wissen nicht, was in der Hood angeboten wird, und ob die technichen Voraussetzungen am Anschluss erfuellt sind. (bei vxlan ist es ipv6) Darum habe ich diese Variable auch als Schalter konzepiert, der durch uci aber auch von den Protokollen bedient werden kann und die Priorisierung wiederspiegelt.
Wenn man das einfacher hin bekommt waere das klasse.
Natuerlich kann man jedes mal die liste der supported_protocols mit ls * holen. Ich mag das gerne einmal in einer Variablen. Aber das ist Geschmacksache.
Aber welche Protokolle in vpn-select.d liegen wird doch an gleicher Stelle kontrolliert wie der uci Eintrag. Insofern hat der Eintragende ja immer über beides Kontrolle. Entsprechend finde ich das automatische Löschen immer noch unnötig und würde hier einen Fehler erwarten.
Ich wollte es auch so haben, dass man bei der Firmware einfach fastd oder vxlan raus werfen kann, ohne irgendwas an der Konfiguration aendern zu muessen. Also ist protocol_order durchaus ungleich supported_protocols. Und an der Stelle gleiche ich das ab.
Vielleicht kann man das auch ohne den ganzen foo machen. Ich sehe es aber nicht.
Aber das Löschen ist doch nur dafür da, wenn du zusätzliche Eintrage in protocol_order hast. In der Praxis löscht doch niemand ein Skript aus vpn-select.d und lässt den Eintrag in protocol_order; wenn das so wäre, bräuchten wir protocol_order ja nicht. Insofern muss davon ausgegangen werden, das jeder zusätzliche Eintrag in protocol_order ohne passenden vpn-select.d File einen Fehler darstellt.
Wir haben doch jedes protocol in einem package, das an- oder abgewaehlt werden kann. Wie soll man dann eine Reihenfolge festlegen. Ich muss doch irgendwo sagen, falls fastd da ist, bevorzuge fastd. Und falls es vxlan gibt, setze es an 2. Stelle. Und wenn l2tp vohanden, nimm das vor fastd. usw. Auch wenn man mal die Reihenfolg vom KeyX bezieht, kann man nicht voraussagen, welche protocols auf dem Router vorhanden sind. Meine Loesung gleicht das ab und reagiert auf alle moeglichen Varianten.
Das hab ich auch getestet und funzt.
Und falls das gewuenschte protocol zwar da ist, aber nicht von den Gateways angeboten wird, muss man auch darauf reagieren und das naechste aus der Liste nehmen.
Das ist schon alles ziemlich verzwickt.
Ausserdem muss man auch testen ob ein protocol funktionieren kann, also ob z.B ipv6 da ist. Falls nicht, das naechste aus der Liste nehmen. Das kann aber vpn-select nicht wissen. Das muss das protocol mitbringen. Und ueber das manipulieren der protocol_order habe ich das geloest.
Ich versuche das vxlan Ding fertig zu machen. Dann sieht man vielleicht noch etwas mehr.
Verstehe ich das eigentlich richtig: Mit der neuen Implementierung gibt es ein "preferred protocol", und nur das wird dann mit allen Gateways gesprochen? Man kann also nicht mehr wie früher z.B. mit einem Gateway fastd und dem anderen l2tp sprechen, je nachdem was diese anbieten?
Wenn ja, gibt es dafür einen technischen Grund oder ist das nur wegen der Implementierung im Code so gewählt (in letzterem Fall könnte man ggf. überlegen, ob man das nicht einfach besser im json lösen kann)?
Wir hatten im Meehaeusle mal besprochen, das pro Gateway zu machen und z.B vxlan zu bevorzugen und fastd als fallback. Das ist aber so kompliziert geworden, dass es mein kleiner Geist nicht erfassen konnte. Darum habe ich beschlossen, eine Variante zu bauen, bei der pro Hood nur ein Protokoll benutzt wird. Das setzt voraus, dass alle Gateways einer Hood die gleichen Protokolle anbieten. Das sollte aber moeglich sein.
Ich wuerde immer noch eine Variante pro Gateway toll finden. Wenn das jeman hin bekommt waere das super.
9cc338534f
to2eccadc3ce
@ -4,0 +8,4 @@
# The function ${protocol}_addpeer() is called for every selected peer in hoodfile.
# The function ${protocol}_start_stop() is called at the end once per installed protocol.
#
# The variable $protocol_order defines the order of preference. The most preffered protocol of the available and offered protocols is selected.
Typo: preferred
Noch ein anderes Thema: Ich finde für diesen Fall die Includes weniger zweckdienlich als bei gateway.d. Ich wundere mich, ob es nicht vielleicht besser wäre, daraus ausführbare Dateien zu machen:
/etc/vpn-select.d/fastd clear
/etc/vpn-select.d/fastd start-stop
/etc/vpn-select.d/vxlan configure
Dadurch würde man diese Redundanz bei den Funktionsnamen los, und könnte die im vpn-select.d Skript einfach start-stop, clear etc. nennen, ohne das Prefix. Nebenbei würde das auch das Testen/Debuggen vereinfachen, wenn man mit den einzelnen Skripten direkt arbeiten kann. Und im Prinzip sind die einzelnen "Protokolle" ja schon so was wie separate Dienste ...
Mittelfristig könnte man dann sogar überlegen, ob man noch irgendwelches status-Gedöns oder so anflanscht ...
ich hab den Patch jetzt nicht komplett verfolgt aber wären das nicht Kanditaten für /etc/init.d? Irgendwie klingt das so als würde man das gerne dort haben wollen.
Es wird eine neue, simplere Version geben. PR geschlossen
Pull request closed