new z9 ccchh router #98
Labels
No labels
bug
duplicate
enhancement
help wanted
invalid
question
renovate
waiting on upstream
wontfix
No milestone
No project
No assignees
4 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
CCCHH/ansible-infra!98
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "new_ccchh_router"
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?
Linux router with systemd_networkd, nftables, unbound and kea_dhcp
new IP Ranges (10.89.208.0/20):
creates roles:
Potentially missing:
do we maybe want to split physical and virtual hosts?
Core configuration looks good, tho I left some comments. Left some more comments on the roles as well.
The
kea_dhcprole also is still missing a README, see the following READMEs for good references:7832978ff7/roles/secrets7832978ff7/roles/deploy_systemd_resolved_config@ -1,6 +1,7 @@skip_list:- "yaml[line-length]"- "name[casing]"- "yaml[brackets]"That's not what linting is for.
Either just fix the brackets (you can also run ansible-lint locally to check for the errors) or ignore a specific file, if it doesn't make sense to lint.
@ -14,6 +14,9 @@ all:yate:ansible_host: yate.ccchh.netansible_user: chaosrt1:It's fine, just feel like
rt1is one of those obscure names again, which gives more trouble than benefit.do you have an idea/a proposal for a better name?
maybe z9-router or something like that?
I would say it would not be a good idea to name it just router,
because of the indirect name collision with the chaosknoten router.
Oh, that totally makes sense. I'm fine with the name, if others really like it, but personally I would prefer something like
z9-routerindeed.@ -53,3 +69,4 @@dooris:rt1:ansible_pull_hosts:hosts:If we configure ansible-pull variables above, the host should also be added to the relevant host group. (However an ansible-pull age private key is still missing.)
@ -0,0 +49,4 @@ct state invalid dropct state established,related acceptip protocol icmp acceptThis seems to be using tabs for some reason, probably best to replace with spaces.
We might want to start using
.editorconfigfiles in the future.@ -0,0 +75,4 @@udp dport 51820 accept comment "allow WireGuard access"# Allow DHCP server access.iifname { $lan_ifs } udp dport 67 accept comment "allow dhcp server access"Same indentation problem here.
@ -0,0 +102,4 @@ct state established,related accept# Allow internet access.iifname { $lan_ifs, $if_wg55_management } oifname $wan_ifs accept comment "allow internet access"Same indentation problem here.
@ -0,0 +108,4 @@meta nfproto ipv4 oifname $v4_exposed_ifs accept comment "allow v4 exposed network access"meta nfproto ipv6 oifname $v6_exposed_ifs accept comment "allow v6 exposed network access"# Allow clients and managment to most"managment" -> "management"
Also "Allow clients and management to lan interfaces." might be a better comment for this rule.
@ -0,0 +5,4 @@[WireGuard]ListenPort=51820PrivateKeyFile=/etc/ansible_secrets/wireguard_wg55_privat_keywireguard_wg55_privat_key->wireguard_wg55_private_key@ -0,0 +75,4 @@[WireGuardPeer]# friendly_name = lilly-lillysLaptopAllowedIPs = 10.89.214.16/32 #,2a07:c481:1:37::/128IPv6 is missing the most significant bits. And is generally commented out weirdly?
#,2a07:c481:1:37::/128->,2a07:c481:1:37::16/128took that directly from the opnsense config and just converted it to networkd format
and to not produce unexpected behavior I commented the ipv6 out,
because it did not have the important part
Ah, ye, makes sense! Would just assign an IP then as that doesn't hurt and Lilly can choose to use it or not.
@ -0,0 +80,4 @@[WireGuardPeer]# friendly_name = bitwhiskerAllowedIPs = 10.89.214.17/32,2a07:c481:1:37::a/128If we want to count the v6 up in hex (which I think is reasonable as we do the same for the VLAN ID in the prefix), then it should also be done properly.
took that directly from the opnsense config and just converted it to networkd format
was considering it, but my reason for not doing that, was because I did not want to change the last part of the IPs that are already in use
Ah, I see, yeah, okay. We would have the option now, as we change things anyway, to clean up that configuration, but I'm also fine with keeping it as is for legacy reasons.
Just something to consider.
@ -0,0 +17,4 @@[IPv6SendRA]UplinkInterface=netwan.400EmitDomains=trueDomains=ccchh.netThe search domain would be
z9.ccchh.net. Unless it got decided to drop that now.yes, we said yesterday, that we want to get rid of z9.ccchh.net. and z9. (tld)
@ -0,0 +4,4 @@ansible.builtin.systemd_service:daemon_reload: true- name: Kea_dhcp4.reloadedCalled "reloaded" even tho the action is
restarted. One of them needs to be adjusted.@ -0,0 +10,4 @@state: restartedenabled: true- name: Kea_dhcp6.reloadedSame restarted/reloaded comment as above.
@ -0,0 +16,4 @@state: restartedenabled: true- name: Kea_ctrl.reloadedSame restarted/reloaded comment as above.
@ -0,0 +1,8 @@---- name: Install Kea on ArchlinuxWe don't have any Arch Linux infrastructure, so we don't need this.
@ -0,0 +5,4 @@when: ansible_facts['distribution'] == "Debian"ansible.builtin.deb822_repository:name: "isc-{{ kea_dhcp__version_repo }}"uris: "https://dl.cloudsmith.io/public/isc/{{ kea_dhcp__version_repo }}/deb/debian"Do we have a requirement for the more up-to-date version present in the upstream repository or is the one in the Debian repos sufficient?
chris wrote this, I don't know why they thought this would be necessary
@c6ristian
For Club this properly doesn't matter. This was because the compatibility between different version has history been often Subject to Change. As long as there are no plan to do HA its probably fine.
Alright, gotcha. Then just using the Debian-packaged version nicely saves us adding a repo.
@ -0,0 +16,4 @@ansible.builtin.apt:name:- isc-kea-dhcp4- isc-kea-dhcp6Since we got granular control over whether to use dhcpv4, v6 or the agent, would it make sense to then also granularly install the relevant packages?
I would think Debian would also automatically start the services, which wouldn't make sense, if no sensible v6 config is present.
@ -0,0 +1,51 @@---- name: Include config varstags: [ kea, include_vars ]I would rather just stick to only having tags at the playbook level. I don't think this kind granular control for the role is really needed as it the config files also shouldn't trigger a reload, if not changed.
this was just copied from the fux noc ansible, but I can remove it
@ -0,0 +3,4 @@tags: [kea, dhcp]block:- name: Install Kea on Archlinuxwhen: ansible_facts['distribution'] == "Archlinux"Again, getting rid of the Arch Linux logic also greatly simplifies this file.
this was just copied from the fux noc ansible, but I can remove it
@ -0,0 +18,4 @@- name: Install stork-agent with aur_pkg_installansible.builtin.include_role:name: aur_pkg_installWe don't even have this role present in our repo, so just getting rid of the Arch Linux logic probably makes sense.
@ -0,0 +12,4 @@{% for subnet in kea_dhcp__dhcp6.subnets %}{"id": {{ subnet.id }},"subnet": "{{ subnet.subnet }}",Just a nit-pick, but having the indentation be consistent across the dhcp4 and dhcp6 config would be nice.
@ -0,0 +1,44 @@### the IP or hostname to listen on for incoming Stork server connections# STORK_AGENT_HOST=Most of the variables here are commented out, so we can just massively simplify this file.
If they are included for documentation purposes, I would rather include a link in the README.
@ -0,0 +1 @@nameserver 127.0.0.1Same comment as below: We already have a role for managing the
resolv.conf, so rather include that.See:
7832978ff7/roles/deploy_systemd_resolved_config@ -0,0 +1,27 @@- name: unbound.restartedtags: [ unbound, dns, dns_resolver ]Again, same comment as in the kea role, I don't think it makes sense to have granular tags in the role itself.
@ -0,0 +10,4 @@- name: install extra dns toolingbecome: trueansible.builtin.package:name: [ bind ] # the bind package includes tools like dig in archlinuxThere is no
bindpackage on Debian and we already installdigin thebase_configrole:- dnsutils@ -0,0 +39,4 @@state: startedenabled: true- name: disable systemd-resolvedWe have a nice role for managing systemd-resolved already, so no need to duplicate functionality here.
See:
7832978ff7/roles/deploy_systemd_resolved_config@ -0,0 +1,17 @@---- name: install unbound prometheus exporterbecome: trueansible.builtin.package:There's no
unbound-prometheus-exporterpackage on Debian.@ -0,0 +3,4 @@become: trueansible.builtin.package:name: prometheus-unbound-exporternotify: prometheus-unbound-exporter.enabledI would rather move the enable and start logic into the file itself, since that's not really the job of a handler. See the main unbound setup tasks for reference.
@ -0,0 +3,4 @@server:{% if unbound_enable_dnssec -%}# disable chroot because unbound is the only thing running on the VM# and because it has issues with how archlinux configures the systemd units write protection regarding the anchor fileIs this still relevant on Debian? Again, we don't use Arch Linux in our infra.
@ -0,0 +12,4 @@{% endif -%}# use all CPUsnum-threads: 2Are those all the CPUs we use? Might it make sense to have this configurable or at least change the comment?
28150818a7960315d182remaning TODOs:
2c825d2b851fbc903125724d537cd58ba3d9145d8ba3d9145dd2f91f2f28Generally looks good and thanks for all the work!
See the attached comments.
@ -3,3 +3,3 @@Folgende Geräte und Server werden duch dieses Ansible Repository verwaltet:* Diverse VMs auf dem ThinkCCCluster* Diverse VMs auf dem z9 (PVE Cluster)This sentence is now grammatically incorrect. Maybe: "Diverse VMs im z9 (PVE Cluster)"?
@ -41,30 +43,102 @@ pve04 A 10.89.213.14pbs A 10.89.213.15AAAA 2a07:c481:1:36::15unifi A 10.89.213.21; AAAA 2a07:c481:1:36::21If this is commented out, can this be just removed or re-established?
@ -44,4 +47,4 @@;; Club-ServicesMaybe explicitly call this "Internal Club-Services" now that we have the category "Public Club-Services".
@ -68,0 +134,4 @@;SIP-T46S A 10.89.212.227;SIP-T46S A 10.89.212.230I assume these deprecated entries are for documentation purposes?
@ -14,3 +14,3 @@# replace with the IP address of your resolverresolver 10.31.208.1;#resolver 10.31.208.1; # is this needed?I'm not quite sure, if this is actually needed for resolving hostnames in the config. If everything works without the
resolverconfig option, then lets just remove it.@ -56,3 +39,3 @@# replace with the IP address of your resolverresolver 10.31.208.1;#resolver 10.31.208.1; # is this needed?Same here with the
resolver.@ -0,0 +6,4 @@# Interfaces#define if_netwan = "netwan"If this isn't in use, it can just be removed.
@ -0,0 +49,4 @@ct state invalid dropct state established,related acceptip protocol icmp acceptSome spacing seems to be off here.
@ -0,0 +103,4 @@ct state established,related accept# Allow internet access.iifname { $lan_ifs, $if_wg56_management } oifname $wan_ifs accept comment "allow internet access"Some spacing seems to be off here as well.
@ -0,0 +7,4 @@[Network]#LinkLocalAddressing=noDHCP=no#IPv6AcceptRA=noI guess those commented out options can just be removed, if they aren't needed.
@ -0,0 +12,4 @@IPv6SendRA=yes[Address]Address=10.89.213.1/24This file is commented out in the main
netlan.networkfile and uses the same IP addresses. Maybe just removing it would clear up confusion or are there plans to re-establish it down the line?@ -0,0 +7,4 @@- name: Kea_dhcp4.restarted_debianlisten: Kea_dhcp4.restartedbecome: truewhen: ansible_facts['distribution'] == 'Debian'Again, I think rather not having the checks makes more sense for our use case.
@ -0,0 +4,4 @@register: kea_dhcp__repoansible.builtin.deb822_repository:name: "isc-{{ kea_dhcp__version_repo }}"uris: "https://dl.cloudsmith.io/public/isc/{{ kea_dhcp__version_repo }}/deb/debian"To clarify: I assume the Debian package isn't sufficient then?
(in reference to: #98 (comment))
yes
@ -0,0 +13,4 @@become: trueansible.builtin.apt:name:- isc-kea-dhcp4Again, I think installing this conditionally, if we also deploy the configs conditionally, makes sense.
(in reference to: #98 (comment))
@ -0,0 +9,4 @@owner: rootgroup: "{{ kea_dhcp__service_group }}"mode: "u=rw,g=r,o="validate: "/usr/sbin/kea-dhcp4 -T %s" # does not work with debian apparmor policysIf this doesn't work on our target platform, than maybe just don't have this check. Does this fail?
this does not fail, because we deactivated app armor, this is just a comment, for if some one in the future has this problem
@ -0,0 +13,4 @@- name: Setup Kea DHCPblock:- name: Install Kea on Debianwhen: ansible_facts['distribution'] == "Debian"I would rather remove this check and have it just fail while running the Ansible playbook, rather than installing a broken Kea deployment.
@ -0,0 +2,4 @@- name: Install stork-agentblock:- name: Install stork-agent on Debianwhen: ansible_facts['distribution'] == "Debian"Same here with the distro check.
@ -0,0 +6,4 @@"control-sockets": {{ kea_dhcp__dhcp4['control-sockets'] | to_nice_json | indent(8) }},"lease-database": {{ kea_dhcp__dhcp4['lease-database'] | to_nice_json | indent(8) }},{% if kea_dhcp__dhcp4['hooks-libraries'] is defined and kea_dhcp__dhcp4['hooks-libraries'] %}"hooks-libraries": {{ kea_dhcp__dhcp4['hooks-libraries'] | to_nice_json | indent(8) }},This option isn't documented in the README.
@ -0,0 +11,4 @@{% if kea_dhcp__dhcp4['option-data'] is defined and kea_dhcp__dhcp4['option-data'] %}"option-data": {{ kea_dhcp__dhcp4['option-data'] | to_nice_json | indent(8) }},{% endif %}"loggers": {{ kea_dhcp__dhcp4['logging'] | to_nice_json | indent(8) }},This option isn't documented in the README.
@ -0,0 +6,4 @@"control-sockets": {{ kea_dhcp__dhcp6['control-sockets'] | to_nice_json | indent(8) }},"lease-database": {{ kea_dhcp__dhcp6['lease-database'] | to_nice_json | indent(8) }},{% if kea_dhcp__dhcp6['hooks-libraries'] is defined and kea_dhcp__dhcp6['hooks-libraries'] %}"hooks-libraries": {{ kea_dhcp__dhcp6['hooks-libraries'] | to_nice_json | indent(8) }},This option isn't documented in the README.
@ -0,0 +11,4 @@{% if kea_dhcp__dhcp6['option-data'] is defined and kea_dhcp__dhcp6['option-data'] %}"option-data": {{ kea_dhcp__dhcp6['option-data'] | to_nice_json | indent(8) }},{% endif %}"loggers": {{ kea_dhcp__dhcp6['logging'] | to_nice_json | indent(8) }}{% if kea_dhcp__dhcp6.subnets is defined and kea_dhcp__dhcp6.subnets %},{% endif %}This option isn't documented in the README.
@ -0,0 +1,44 @@### the IP or hostname to listen on for incoming Stork server connectionsI would just get rid of all the commented out options, if we don't use them anyway (and instead maybe link to a full-fledged config example from the docs or something).
(#98 (comment))
@ -0,0 +1,20 @@# Unbound DNS resolverRole fora a validating, recursive, caching DNS resolver based on [Unbound](https://nlnetlabs.nl/projects/unbound/about/).Spelling "fora" -> "for".
@ -0,0 +9,4 @@The following variables can be used to customize this role:| Variable | Type | Default | Description |I think it would be nice to stick to the list format used in all the other roles for the sake of consistency.
@ -0,0 +1 @@nameserver 127.0.0.1I would still be in favor of just depending on our
deploy_systemd_resolved_configrole for managing this.(#98 (comment))
@ -0,0 +4,4 @@name: unbound.servicestate: restarted- name: unbound.reloadedI think this handler isn't in use, is it?
@ -0,0 +1,55 @@- name: unbound role mainblock:If all tasks are in the block, then this doesn't need to be a block.
@ -0,0 +8,4 @@block:- name: request github apiansible.builtin.uri:url: "{{ prometheus_unbound_exporter_github_api_url }}"If we hardcode the version anyway, I think we should first check, if this version is already installed locally and only then hit the GitHub API. This avoids unnecessary steps and also avoids hitting the unreliable GitHub API for every run.
One could either use apt (via a command or facts maybe?) to check for the installed version and only get a new one, if they differ or just keep the latest dep as a sort of canary file or something and only get a new one, if it isn't as expected.
Something similar is done for the Forgejo Runner installation, see
5ecf70ade3/roles/forgejo_runner/tasks/main/01_install.yaml, tho this doesn't use apt, so can't really be adapted.@ -0,0 +39,4 @@access-control: {{ i }} allow{% endfor -%}{% for i in unbound_private_domain -%}This setting is missing from documentation in the README.
These things need to be added (already applied manually on the router):
nftables:
und
Anstatt der firewall freigabe für udp dport 53.
/etc/unbound/unbound.conf
Also the same for the other interfaces.
lgtm
69c36161be4fbac5cfe6New commits pushed, approval review dismissed automatically according to repository settings
lgtm
New commits pushed, approval review dismissed automatically according to repository settings
WIP: new z9 ccchh routerto new z9 ccchh routerlgtm
miau