|
Herbststurm
Anmeldungsdatum: 27. Mai 2011
Beiträge: 244
|
Hallo Ich habe mir ein bash Skript gebastelt (es funktioiert ☺), um auf einem neuen Ubuntu Server microk8s zu starten. Leider ist meine bash Erfahrung nicht so wie sie sein sollte... ☹ und aus diesem Grund möchte ich darum möchte ich euch bitten folgendes auf Code Qualität zu prüfen, und mir zu sagen was ich anders, oder besser machen soll (und warum). Und ich habe eine Frage zu Zeile 131, wo die Firewall auf allow policy gestellt wird: Ich habe mehrere network interfaces in dem Server. Die Befehle die ich fand gaben mit immer alle interfaces zurück. Aber nur dieses eine, in meinem Fall venet0, konnte ich nicht bekommen. Wie mache ich das, um dies via Variable in das Skript einzubauen? Denn wenn ich auf einem anderen Server bin ist ja keineswegs sichergestellt, dass die Netzwerk Schnittstelle den selben Namen hat. 1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187 | #/bin/bash
echo -e $"\n==================================================================================================================================="
echo -e $'\nTL;DR'
echo -e $'\nThis script is preparing a new blank linux server into a production ready kubernetes cluster for hosting containerized apps.'
echo -e $'\nControl bools has to be defined, because this script will be used ( or abused :) ) for more universal linux configurations too.'
echo -e $"\n==================================================================================================================================="
# Define the control bools
dummyVGA="True" # if True a dummy X11 will be installed, to simulate a monitor which is useful in some docker applications
dummyXconf=/usr/share/X11/xorg.conf.d/xorg.conf
dockerCanonicalFork="True" # if True the docker fork from Canonical, and if False the official docker will be installed.
dockerDeamon=/etc/docker/daemon.json
kubernetes="True" # if True Canonical microk8s will be installed. Use Canonical docker, because microk8s is from Canonical too.
# root user creates a sudo user ubuntu if not already exists
if [ $USER = "root" ]; then
if id -u "ubuntu" >/dev/null 2>&1; then
echo -e $'\nUser ubuntu already has an user account'
else
echo -e $"\nInit new sudo user ubuntu"
apt-get update
apt-get install -y sudo
adduser -m -s /bin/bash ubuntu
adduser --gecos '' ubuntu
usermod -aG sudo ubuntu
echo "me ALL=(ALL) NOPASSWD:ALL" | sudo tee /etc/sudoers.d/ubuntu
fi #if ubuntu
echo -e $'\nLogout and login as ubuntu or pi user, and re-execute this script'
fi #if root
# configure ubuntu user environment
if [ $USER = "ubuntu" ] || [ $USER = "pi" ]; then
echo -e $"\nInstalling, or updating basic dependencies"
sudo apt-get update
sudo apt-get install -y ccache dos2unix bash-completion snapd fuse ufw nano nfs-common
sudo systemctl start snapd.socket
# Simulated monitor pluged in for dummy XServer
if test -f "$dummyXconf" && [[ $dummyVGA == "True" ]]; then
echo -e $"\n==================================================="
echo -e $"\nDummy XServer already installed"
else
if [[ $dummyVGA == "True" ]]; then
echo -e $"\n==================================================="
echo -e $"\nInstall dummy vga monitor"
sudo apt-get install -y xserver-xorg-video-dummy
sudo touch $dummyXconf
#ToDo sed mock X11 config into file
fi
fi
# Docker section
if [ -x "$(command -v docker)" ]; then
echo -e $"\n==================================================="
echo -e $"\ndocker already installed"
echo -e $"\n==================================================="
else
if [[ $dockerCanonicalFork == "False" ]]; then
echo -e $"\nInstall official docker"
echo -e $"\nRemove docker artefacts if they exist"
sudo apt-get remove docker docker-engine docker.io containerd runc
sudo apt-get purge docker-ce docker-ce-cli containerd.io
sudo rm -rf /var/lib/docker
sudo rm -rf /var/lib/containerd
echo -e $"\nSetup docker repository"
sudo apt-get install -y apt-transport-https ca-certificates curl gnupg lsb-release
curl -fsSL https://download.docker.com/linux/ubuntu/gpg | sudo gpg --dearmor -o /usr/share/keyrings/docker-archive-keyring.gpg
echo \
"deb [arch=amd64 signed-by=/usr/share/keyrings/docker-archive-keyring.gpg] https://download.docker.com/linux/ubuntu \
$(lsb_release -cs) stable" | sudo tee /etc/apt/sources.list.d/docker.list > /dev/null
sudo apt-get update
echo -e $"\nInstall official docker"
sudo apt-get install -y docker-ce docker-ce-cli containerd.io
echo -e $"\nManage docker as non-root user"
sudo groupadd docker
sudo usermod -aG docker $USER
sudo chown "$USER":"$USER" /home/"$USER"/.docker -R
sudo chmod g+rwx "$HOME/.docker" -R
echo -e $"\nConfigure docker to start on boot"
sudo systemctl enable docker.service
sudo systemctl enable containerd.service
echo -e "\nInstall docker-compose"
sudo curl -L "https://github.com/docker/compose/releases/download/1.29.2/docker-compose-$(uname -s)-$(uname -m)" -o /usr/local/bin/docker-compose
sudo chmod +x /usr/local/bin/docker-compose
sudo ln -s /usr/local/bin/docker-compose /usr/bin/docker-compose
echo -e "\nNext you have to setup systemd for docker!"
echo "See: Configuring remote access with systemd unit file in https://docs.docker.com/engine/install/linux-postinstall/"
echo -e "\ndocker successfully installed. please logout and login again"
echo -e $"\n==================================================="
else
echo -e $"\n==================================================="
echo -e $"\nInstall Canonical docker"
sudo apt-get install -y docker.io docker-compose docker-doc docker-registry
echo -e $"\nManage docker as non-root user"
sudo groupadd docker
sudo usermod -aG docker $USER
sudo chown ubuntu:docker /var/run/docker.sock
sudo chown -R ubuntu:docker /var/run/docker
sudo systemctl enable docker
echo -e $'\nLogin as ubuntu or pi user, and re-execute this script'
echo -e $'\nSystem will be rebootet now'
sudo reboot
fi
fi #if docker
# Kubernetes section
if [[ $kubernetes == "True" ]]; then
if [ -x "$(command -v microk8s)" ];
then
echo -e $"\n==================================================="
echo -e $"\nmicrok8s already installed"
echo -e $"\n==================================================="
else
if [ -f "$dockerDeamon" ]; then
echo -e $"\nDocker daemon already configured"
echo "$dockerDeamon exists."
else
echo "$dockerDeamon does not exist."
# because of microk8s inspect 'WARNING: Docker is installed.'
sudo touch /etc/docker/daemon.json
sudo tee /etc/docker/daemon.json <<<'{"insecure-registries":["localhost:32000"]}'
sudo systemctl restart docker
fi
echo -e $"\nConfigure firewall for Kubernetes"
sudo ufw allow in on venet0 && sudo ufw allow out on venet0
sudo ufw default allow routed
echo -e $"\n==================================================="
echo -e $"\nDisabeling swap for Kubernetes"
sudo swapoff -a
echo -e $"\n==================================================="
echo -e $"\nSee what versions of microk8s are currently published"
snap info microk8s
echo -e $"\n==================================================="
echo -e $"\nInstall microk8s - Canocical Kubernetes"
sudo snap install microk8s --classic --channel=1.18/stable
sudo usermod -a -G microk8s $USER
echo -e $"\nGenerate and show microk8s kube config"
mkdir -p ~/.kube
microk8s config > ~/.kube/config
microk8s config
sudo chown -R $USER. ~/.kube
echo -e $"\n==================================================="
echo -e $"\nShow kubelet service status"
systemctl status snap.microk8s.daemon-kubelet
echo -e $"\n==================================================="
echo -e $"\nEnable addons and basic services"
microk8s.enable dns dashboard storage registry
echo -e $"\nmicrok8s waiting for status"
microk8s status --wait-ready
echo -e $"\n==================================================="
echo -e $"\nGet microk8s namespaces"
microk8s.kubectl get all --namespace kube-system
echo -e $"\n==================================================="
echo -e $"\nmicrok8s inspect"
microk8s inspect
echo -e $"\n==================================================="
echo -e $"\nCheck kubernetes ports"
sudo netstat -lnpt|grep kube
echo -e $"\n==================================================="
echo -e $"\nCreate microk8s access token"
token=$(microk8s kubectl -n kube-system get secret | grep default-token | cut -d " " -f1)
microk8s kubectl -n kube-system describe secret $token
echo -e $"\nmicrok8s dashboard IP"
microk8s kubectl -n kube-system get service/kubernetes-dashboard -o go-template={{.spec.clusterIP}}
echo -e $"\n==================================================="
fi
fi #if kubernetes
fi #if ubuntu
|
Vielen Dank und viele Grüße
|
|
user_unknown
Anmeldungsdatum: 10. August 2005
Beiträge: 17630
|
Ich kann zum eigentlichen Job wenig sagen, da ich mit Docker/Kubernetes noch nichts gemacht habe. Aber zu manchen Kleinigkeiten kann ich mich äußern. Wieso setzt Du das Wort True (und viele andere) in Anführungsstriche?
Was sollen die Anführungsstriche bewirken? Was bewirken sie wirklich? Wann braucht man sie? Hier frage ich mich zweierlei:
| echo -e $'\nUser ubuntu already has an user account'
|
Wieso der Wechsel zu Apostrophen? Und was soll das führende Dollarzeichen bewirken? | echo -e $"\n==================================================="
echo -e $"\ndocker already installed"
echo -e $"\n==================================================="
|
Umrandung als Hervorhebung, und dennoch eine Extrazeile Abstand? Wenn man jede Zeile hervorhebt ist es keine Hervorhebung mehr. Dem Shebang fehlt ein Ausrufezeichen. Kommentierte fis sind ein Codesmell, und zwar riecht es nach zu viel Code in einer Verzweigung, wenn man am Ende den Anfang nicht mehr sieht. Eine mechanische Lösung wäre es, alle Blöcke in eigene Funktionen auszulagern, und die Namen dafür finden sich eigentlich schon in den sie begleitenden Blöcken: Z. 176-179:
|
echo -e $"\n==================================================="
echo -e $"\nCreate microk8s access token"
token=$(microk8s kubectl -n kube-system get secret | grep default-token | cut -d " " -f1)
microk8s kubectl -n kube-system describe secret $token
|
| createMicrok8sAccessToken () {
echo "==================================================="
echo "Create microk8s access token"
token=$(microk8s kubectl -n kube-system get secret | grep default-token | cut -d " " -f1)
microk8s kubectl -n kube-system describe secret $token
}
#
# ... viele Zeilen später:
#
createMicrok8sAccessToken ()
|
|
|
kB
Supporter, Wikiteam
Anmeldungsdatum: 4. Oktober 2007
Beiträge: 9837
|
Herbststurm schrieb: […] auf Code Qualität zu prüfen […]
[ $USER = "root" ]
Obwohl es in Deinem Skript problemlos funktioniert, ist dies grundsätzlich ein schwerer programmiertechnischer Kunstfehler! Ein Problem entsteht, wenn die Variable nicht gesetzt oder leer ist. (Das kann zwar explizit bei der bash mit der Variable USER nicht geschehen, aber in anderen Situationen schon.) Dann wird dieser Test einen syntaktischen Fehler erzeugen und Dein Programm auf jeden Fall mit dem folgenden Befehl weitermachen. Damit wird Code auch in Situationen ausgeführt, für die er nicht vorgesehen ist. Das richtige Idiom für Tests in der Shell (bash und andere) sieht so aus:
[ "$VARIABLE" = Suchstring ] bzw. in Deinem Fall [ "$USER" = root ] Wenn jetzt die Variable keinen Wert hat, lautet das explizit [ "" = root ] und das ergibt falsch. Damit wird der von diesem Test abhängige folgende Code sicher nicht ausgeführt. Quotierung des Suchstrings ist nur erforderlich, wenn er Leerzeichen oder andere problematische Sonderzeichen enthält. Quotierung der Variable ist dagegen immer erforderlich. Nun ist zwar USER ein Sonderfall, weil immer ein Wert vorliegt, aber Du hast ja nach Clean Code gefragt.
|
|
rklm
Projektleitung
Anmeldungsdatum: 16. Oktober 2011
Beiträge: 13242
|
Das erste was mir auffällt sind die Massen von sudo. Es erscheint, man führt das Skript besser als "root" aus. Das kann man z.B. so automatisieren: | #!/bin/sh
[ "$(id -u)" -eq 0 ] || exec sudo "$0" "$@"
# Als root
id
# Jetzt als der aufrufende User:
sudo -u "$SUDO_USER" id
|
Zum Debuggen ist set -x sehr nützlich. Ich schalte das gerne durch eine Umgebungsvariable ein: | #!/bin/sh
[ -z "$DEBUG" ] || set -x
[ $(id -u) -eq 0 ] || exec sudo "DEBUG=$DEBUG" "$0" "$@"
# Kommando als root
id
# Jetzt als der ursprünglich aufrufende User:
sudo -u "$SUDO_USER" id
|
id dient nur der Illustration, welcher User gerade aktiv ist. Zeile 32 muss dann natürlich SUDO_USER prüfen und nicht USER.
user_unknown schrieb:
Hier frage ich mich zweierlei:
| echo -e $'\nUser ubuntu already has an user account'
|
Wieso der Wechsel zu Apostrophen? Und was soll das führende Dollarzeichen bewirken?
Das ist eine spezielle Syntax der bash, die dazu führt, dass Sequenzen wie "\n" übersetzt werden (hier zu einem Newline): | $ echo 'foo\nbar' | od -t x1c
0000000 66 6f 6f 5c 6e 62 61 72 0a
f o o \ n b a r \n
0000011
$ echo $'foo\nbar' | od -t x1c
0000000 66 6f 6f 0a 62 61 72 0a
f o o \n b a r \n
0000010
|
| echo -e $"\n==================================================="
echo -e $"\ndocker already installed"
echo -e $"\n==================================================="
|
Bei so etwas würde sich auch eher ein here document anbieten. Man könnte solche Ausgaben auch in eine Funktion auslagern. Das vermindert Redundanz und macht eine spätere Änderung leichter (z.B. wenn einem "/////" besser gefällt als "====="). Insgesamt ist da sehr viel Wiederholung in dem Skript. Die würde ich auf jeden Fall loswerden. Dein Skript macht sehr verschiedene Dinge abhängig vom Nutzer, der das ausführt ("root" vs. "ubuntu" oder "pi"). Das halte ich für schlecht, weil es die Regel verletzt, dass ein Artefakt möglichst eine Sache machen soll. Ich würde das auf jeden Fall auf zwei Skripte verteilen. Und in dem Skript, das die Nicht-root-Arbeit macht, würde ich bei "falschem" Nutzer schnell aussteigen. Das macht den Riesen-if überflüssig: 1
2
3
4
5
6
7
8
9
10
11
12 | #!/bin/sh
abort() { echo "$*" >&2; exit 1; }
# Wenn man ein bisschen paranoid ist:
export USER=$(id -un)
if [ "$USER" != ubuntu -a "$USER" != pi ]; then
abort "ERROR: user needs to be 'ubuntu' or 'pi' but was '$USER'"
fi
...
|
|
|
user_unknown
Anmeldungsdatum: 10. August 2005
Beiträge: 17630
|
rklm schrieb:
user_unknown schrieb:
Hier frage ich mich zweierlei:
| echo -e $'\nUser ubuntu already has an user account'
|
Wieso der Wechsel zu Apostrophen? Und was soll das führende Dollarzeichen bewirken?
Das ist eine spezielle Syntax der bash, die dazu führt, dass Sequenzen wie "\n" übersetzt werden (hier zu einem Newline): | $ echo 'foo\nbar' | od -t x1c
0000000 66 6f 6f 5c 6e 62 61 72 0a
f o o \ n b a r \n
0000011
$ echo $'foo\nbar' | od -t x1c
0000000 66 6f 6f 0a 62 61 72 0a
f o o \n b a r \n
0000010
|
Da ist aber schon der -e-Schalter gesetzt, der ein einen Zeilenumbruch erzeugt: | t530:~/job/fbb/mysql 🐧> echo -e 'foo\nbar' | od -t x1c
0000000 66 6f 6f 0a 62 61 72 0a
f o o \n b a r \n
0000010
t530:~/job/fbb/mysql 🐧> echo -e $'foo\nbar' | od -t x1c
0000000 66 6f 6f 0a 62 61 72 0a
f o o \n b a r \n
0000010
|
|
|
user_unknown
Anmeldungsdatum: 10. August 2005
Beiträge: 17630
|
kB schrieb: Herbststurm schrieb: […] auf Code Qualität zu prüfen […]
[ $USER = "root" ]
Obwohl es in Deinem Skript problemlos funktioniert, ist dies grundsätzlich ein schwerer programmiertechnischer Kunstfehler!
Das sehe ich aber anders. Eine Regel stur anzuwenden, auch wo der Grund hinfällig ist, ist Dogmatismus und das Gegenteil von Kunst.
|
|
user_unknown
Anmeldungsdatum: 10. August 2005
Beiträge: 17630
|
Wo wir bei Kunst sind - In The art of unix programming hat Eric S. Raymond festgehalten: Rule of Silence: When a program has nothing surprising to say, it should say nothing. One of Unix's oldest and most persistent design rules is that when a program has nothing interesting or surprising to say, it should shut up. Well-behaved Unix programs do their jobs unobtrusively, with a minimum of fuss and bother. Silence is golden. This “silence is golden” rule evolved originally because Unix predates video displays. On the slow printing terminals of 1969, each line of unnecessary output was a serious drain on the user's time. That constraint is gone, but excellent reasons for terseness remain. I think that the terseness of Unix programs is a central feature of the style. When your program's output becomes another's input, it should be easy to pick out the needed bits. And for people it is a human-factors necessity — important information should not be mixed in with verbosity about internal program behavior. If all displayed information is important, important information is easy to find. – Ken Arnold Well-designed programs treat the user's attention and concentration as a precious and limited resource, only to be claimed when necessary. (We'll discuss the Rule of Silence and the reasons for it in more detail at the end of Chapter 11.)
Quelle: http://www.catb.org/~esr/writings/taoup/html/ch01s06.html#id2878450 Das Buch ist online frei erhältlich und sollte gelesen werden, wenn man sich für Codequalität interessiert. Bearbeitet von noisefloor: Syntax korrigiert, jetzt funktioniert der Link auch ☺ Externe Links werden nur in eckige Klammern eingeschlossen.[/mod]
|
|
rklm
Projektleitung
Anmeldungsdatum: 16. Oktober 2011
Beiträge: 13242
|
user_unknown schrieb:
Da ist aber schon der -e-Schalter gesetzt, der ein einen Zeilenumbruch erzeugt:
Ich habe auch nicht gesagt, dass das sinnvoll ist. ☺ Und auch, wenn man dieselbe Ausgabe erzielen kann, ist es trotzdem nicht das gleiche, weil $'..\n..' zu einer anderen Zeichenkette in der Variablen führt als '..\n..'. Das macht einen Unterschied, wenn es z.B. um einen Dateinamen geht, der tatsächlich einen Newline oder aber die Sequenz "\n" (zwei Zeichen / Bytes) enthält. user_unknown schrieb:
Das Buch ist online frei erhältlich und sollte gelesen werden, wenn man sich für Codequalität interessiert.
Aber nur, wenn es etwas Überraschendes zu sagen hat. 😉
|
|
kB
Supporter, Wikiteam
Anmeldungsdatum: 4. Oktober 2007
Beiträge: 9837
|
|
|
user_unknown
Anmeldungsdatum: 10. August 2005
Beiträge: 17630
|
kB schrieb: user_unknown schrieb: […]
Quelle: //www.catb.org/~esr/writings/taoup/html/ch01s06.html#id2878450:
Bitte den Link korrigieren!
Oh, selbstverständlich. (Rule of silence) http://www.catb.org/~esr/writings/taoup/html/ch01s06.html#id2878450 rklm schrieb: user_unknown schrieb:
Da ist aber schon der -e-Schalter gesetzt, der ein einen Zeilenumbruch erzeugt:
Ich habe auch nicht gesagt, dass das sinnvoll ist. ☺
Richtig, Du hast gesagt, besser geschrieben, es würde hier zu einem Newline, wobei das hier doppeldeutig ist, bezieht es sich auf hier den Code unter Review, oder hier, dass Beispiel hier, das Du dann bringst, und das eben mit meiner Frage nichts mehr zu tun hat, weil das -e unterschlagen wird, um einen Unterschied zu machen, der gar nicht zur Debatte stand.
Und auch, wenn man dieselbe Ausgabe erzielen kann, ist es trotzdem nicht das gleiche, weil $'..\n..' zu einer anderen Zeichenkette in der Variablen führt
Es gibt nur keine Variable.
als '..\n..'. Das macht einen Unterschied, wenn es z.B. um einen Dateinamen geht, der tatsächlich einen Newline oder aber die Sequenz "\n" (zwei Zeichen / Bytes) enthält.
Es gibt da auch keinen Dateinamen. Also so recht weiß ich nicht, was Du mit Deinem Posting sagen wolltest - wozu das Dollarzeichen gut wäre, wenn der Code ein ganz anderer wäre, offenbar.
|