Create a dedicated script for removing TinyPilot auto-generated changes

[mtlynch]

When TinyPilot modifies config files owned by other services, we sometimes add custom markers that allow us to back out our changes later:

• tinypilot/debian-pkg/opt/tinypilot-privileged/scripts/change-hostname

  Lines 59 to 84 in d74d358

  	# Retrieve original `/etc/hosts`.
  	# - Read file line by line, make sure to preserve all whitespace.
  	# - Remove all marker sections.
  	etc_hosts_original=()
  	is_in_marker_section='false'
  	while IFS= read -r line; do
  	if "${is_in_marker_section}" && [[ "${line}" == "${MARKER_END}" ]]; then
  	is_in_marker_section='false'
  	continue
  	fi
  	if "${is_in_marker_section}" || [[ "${line}" == "${MARKER_START}" ]]; then
  	is_in_marker_section='true'
  	continue
  	fi
  	etc_hosts_original+=("${line}")
  	done < /etc/hosts
  	if "${is_in_marker_section}"; then
  	echo 'Unclosed marker section' >&2
  	exit 1
  	fi
  	# Populate new hostname to `/etc/hostname`.
  	# Note that the value must be newline-terminated, see:
  	# https://manpages.debian.org/stretch/systemd/hostname.5.en.html
  	printf "%s\n" "${NEW_HOSTNAME}" > /etc/hostname

• tinypilot/debian-pkg/opt/tinypilot-privileged/scripts/unset-static-ip

  Lines 81 to 118 in d74d358

  	# Remove any existing auto-generated configuration for the interface.
  	dhcpcd_original=()
  	section=()
  	interface_matched="false"
  	while IFS= read -r line; do
  	if [[ "${#section[@]}" -ne 0 ]] && [[ "${line}" == "${MARKER_END}" ]]; then
  	section+=("${line}")
  	if [[ "${interface_matched}" == "false" ]] ; then
  	dhcpcd_original=("${dhcpcd_original[@]}" "${section[@]}")
  	fi
  	section=()
  	interface_matched="false"
  	continue
  	fi
  	if [[ "${#section[@]}" -ne 0 ]] || [[ "${line}" == "${MARKER_START}" ]]; then
  	if [[ "${line}" == "interface ${INTERFACE}" ]] ; then
  	interface_matched="true"
  	fi
  	section+=("${line}")
  	continue
  	fi
  	dhcpcd_original+=("${line}")
  	done < "${CONFIG_FILE}"
  	if [[ "${#section[@]}" -ne 0 ]] ; then
  	echo "The contents of ${CONFIG_FILE} are not in the expected format." >&2
  	echo "Auto-generated changes may have been manually edited." >&2
  	echo "" >&2
  	echo "Remove the relevant configuration manually to proceed." >&2
  	exit 1
  	fi
  	# Convert array of lines to a single string.
  	OLD_CONFIG_FILE=$(printf "%s\n" "${dhcpcd_original[@]}")
  	readonly OLD_CONFIG_FILE
  	# Write original file contents back to the file.
  	echo "${OLD_CONFIG_FILE}" | sudo tee "${CONFIG_FILE}" > /dev/null

We're about to add a third incarnation of this code on the website repo:

• https://github.com/tiny-pilot/tinypilotkvm.com/blob/86507d824d6a431b6983efeadede40cc57322671/content/faq/wifi-ap/index.md?plain=1#L116-L135

We should refactor this logic into a script whose only job is to remove the TinyPilot-generated section. We can use this reusable script for removing lines and markers rather than duplicating the logic in multiple places.

Example

$ cat example.conf
foo=bar

# --- AUTOGENERATED BY TINYPILOT - START ---
tinypilot_timing=7
hdmi_output=3
# --- AUTOGENERATED BY TINYPILOT - END ---

baz=baa

$ /opt/tinypilot/scripts/remove-tinypilot-lines < example.conf
foo=bar

baz=baa

[jotaen4tinypilot]

I’m not sure we can easily consolidate the instance in unset-static-ip: the code contains additional logic to account for the --interface flag, so it only removes marker sections that match the given network interface, and disregards any other marker section. For example, if multiple static IP configurations for different network interfaces had been added to dhcpcd.conf, then they are each wrapped into their own markers. You can then do e.g. unset-static-ip --interface eth0, to only remove the one particular marker section for the eth0 interface, while leaving all other marker sections untouched.

A potential solution that would come to my mind is to provide some sort of filter functionality in an otherwise generic remove-tinypilot-lines script, to check whether a marker section is eligible for dropping. However, I’m worried that this would complicate things instead of simplifying them, partly because it might be cumbersome to express such filter clauses with bash APIs.

We originally introduced the static IP CLI scripts a year ago. I don’t know whether we actually make use of the multi-interface functionality anywhere in practice right now, and whether it’s therefore safe or not to drop it:

• We don’t need it for the static IP UI right now, though I don’t know whether we have plans for that. In the static IP UI, it’s currently not possible to specify the network interface, and it implicitly defaults to eth0. (I’m also actually not sure the UI can correctly deal with multiple static IP configs for different interfaces at all, or what would happen if they are present.)
• We might have advertised CLI usage of the static IP scripts publicly, so some users might rely on it?

I think we could generally consider to establish an internal convention around these marker sections. My intuitive take is (was) that there should only be one marker section per file, and we must always be able to re-generate the section in its entirety. That at least would be the main benefit that I’d see in using the marker section approach over something more sophisticated. Otherwise, we have to do (conditional) parsing work to inspect the contents of the file, and that is actually what the marker section approach tries to avoid.

[jotaen4tinypilot]

That being said, my question is how we’d proceed from here best? E.g.,

• We could still create a remove-tinypilot-lines script, but only use it in change-hostname and the FAQ, and not in unset-static-ip.
• We could look into refactoring the static IP logic, and trying to unify the multiple markers sections into a single one.
• We could try to find a remove-tinypilot-lines abstractions that somehow works for both use cases.
• We do re-consider the issue altogether.

[jotaen4tinypilot]

(WIP branch, including bats tests; unfortunately, I didn’t notice this problem earlier / right away.)

[mtlynch]

I think we can just drop the --interface flag from unset-static-ip. In other words, unset-static-ip removes all TinyPilot-assigned static IPs from all interfaces.

We should make it error out if the user passes an --interface flag so that we don't silently break old behavior if anyone has scripts relying on this (unlikely, but possible).

For this to break a use-case, we'd need to have a user go through a flow like this, correct?

1. Assign a static IP to eth0
2. Assign a static IP to wlan0
3. Remove a static IP from eth0 only

I think the number of users who go through a flow like that is vanishingly small, probably zero.

I don't recall anyone ever requesting support for this. I think we just added it because "why not?" and we didn't think through the maintenance burden.

[jotaen4tinypilot]

I think we can just drop the --interface flag from unset-static-ip. In other words, unset-static-ip removes all TinyPilot-assigned static IPs from all interfaces.

Okay cool, that simplifies things a lot.

For this to break a use-case, we'd need to have a user go through a flow like this, correct?

Correct. In this case two things could happen when they call the unset-static-ip script:

• They don’t specify an --interface flag: the script would previously have defaulted to deleting the eth0 section. Then, it would delete all sections/interfaces.
• They specify an --interface flag, e.g. --interface wlan0: the script would previously have deleted only the wlan0 section. Then, it would fail with our about-to-be-added error message.

I’d do this now:

1. Remove --interface flag from unset-static-ip API as discussed, to make unset-static-ip on par with change-hostname.
2. Introduce unified remove-tinypilot-lines script
3. Introduce bats tests
4. Switch unset-static-ip and change-hostname to delegate to remove-tinypilot-lines.

[jotaen4tinypilot]

One addendum, just for the records: set-static-ip invokes unset-static-ip. So users will effectively only be able to configure one interface at a time, because trying to set a second one would clear any previously set configuration.

So regarding the scenario above:

1. Assign a static IP to eth0
2. Assign a static IP to wlan0 ← This clears the assignment from step 1, only leaving wlan0

[mtlynch]

So users will effectively only be able to configure one interface at a time, because trying to set a second one would clear any previously set configuration.

Ah, I hadn't considered that. Still, I think it's probably a rare scenario to need static IP for both WiFi and Ethernet. If we get requests for that, it shouldn't be too hard for us to revisit the feature and figure out a way to support it later.