New 'etcupdate' sub command #660
Conversation
There was a problem hiding this comment.
While this change is somewhat opinionated, to be opinionated is necessary for this feature. Perhaps options could be added later for edge cases, but these choices seem sane and like they will cover the majority of use cases.
I made some small suggestions so the logs read properly, and changing a variable name to make it more clear/accurate.
Overall, this looks like a great and welcome contribution to me.
| TARGET="${1}" | ||
| COMPAREVERSION="${2}" | ||
| UPGRADEVERSION="${3}" | ||
| bastille_jail_base="${bastille_jailsdir}/${TARGET}" |
There was a problem hiding this comment.
Suggest changing bastille_jail_base to something like bastille_thin_jail because the former makes it sound like a "base" jail, which is what the release jails are in this context. The thin jail is the jail to be operated on.
| error_exit "See 'bastille stop ${TARGET}'." | ||
| fi | ||
|
|
||
| if [ ! -d "${bastille_jail_base}" ]; then |
There was a problem hiding this comment.
Suggest changing bastille_jail_base to something like bastille_thin_jail because the former makes it sound like a "base" jail, which is what the release jails are in this context. The thin jail is the jail to be operated on.
| for sourcefile in ${filelistrelease} | ||
| do | ||
| dirpathnf=$(echo "${sourcefile}" | awk -F '/etc' '{print $NF}') | ||
| jailfile="${bastille_jail_base}/root/etc${dirpathnf}" |
There was a problem hiding this comment.
Suggest changing bastille_jail_base to something like bastille_thin_jail because the former makes it sound like a "base" jail, which is what the release jails are in this context. The thin jail is the jail to be operated on.
There was a problem hiding this comment.
Although I agree with you, I named this variable following the same convention found in the codebase, for consistency. Perhaps best left as is until it (if) is reviewed as a whole? I can see pros and cons of both approaches. So.. umm.. I'm not sure.
bastille/usr/local/share/bastille/create.sh
Line 274 in 3a4ebc6
That's how I see it too. Can be useful in many cases and doesn't hurt edge cases if it's run anyway (but not entirely sure on this). A generic option. |
|
This PR might be related to: #704 Any idea when this PR can be merged? It'd be nice to have this feature implemented/documented as it allows to upgrade thin jails without manual interventions. |
| executeconditional "$cmd" | ||
| # Copy keeping permissions | ||
| fi | ||
| else |
There was a problem hiding this comment.
At this point, there have been changes to the jail's /etc files compared to the currentbasefile, so there will be a diff for C5 or C6. Rather than have a list of diffs to revisit later (the conservative approach is to leave them alone), it would be great to have the option enter an interactive mode to approve/modify the diff here.
|
Another issue came to mind while trying to test this. In short, this process would be better if it could detect the version of For example, I created a jail with 13.0-RELEASE. It currently runs on 13.2-RELEASE, but I haven't done any But that's actually not the major issue I'm finding. Looking at the diff's comparing 13.0 to the files it said are different in my jail, I didn't change most of the files. Unfortunately, this method of performing an For example, I didn't make changes to If this It would be a hassle to interactively approve patch after patch 45 times, but I'm inclined to say interactively approving patches would be preferable to the current state where it simply skips all these files because it thinks I have modified them. |
|
Yeah I agree that having to indicate the from version can be problematic. I was trying to think some approach to this fact by the time I wrote script. My conclusion was I'm not able to determine version automatically and reliably. /etc/os-release is obsolete ( https://forums.freebsd.org/threads/determining-the-version-of-freebsd.31324/ ) Or (rethoric question): If you inspect C4 condition, if most files are listed there this would be a good measure that you're comparing the right version? How reliable/unreliable is this manual comparison against trying to detect it automatically? Ideas? As for the interactive approach for me it sounds like a nice idea if added as user option (e.g. can opt for just doing it without asking) |
|
Are you sure /etc/os-release is obsolete? I understood that the port (Registering interest, as I think this solves my old issue, #438, which was closed but had no real resolution...) |
| if [ ! -f "${newbasefile}" ]; then | ||
| C3=$((C3+1)) | ||
| C3ct="$C3ct${jailfile}\n" | ||
| cmd="rm -rf ${jailfile}" |
There was a problem hiding this comment.
Recommend -- between switches and file list; if someone happens to place a file called -x into /etc, rm would see this as an argument, not a file. e.g. rm -f -- ${jailfile}
If $jailfile is always a file (constrained by find -type f in L56), does it need to be recursive -r?
Possible issue with whitespace in filename, if filename is folded into eval'd command string? Since you are using eval "$@" in executeconditional, couldn't this simply be executeconditional rm -f -- "${jailfile}"
| else | ||
| C4=$((C4+1)) | ||
| C4ct="$C4ct${jailfile}\n" | ||
| cmd="cp -p ${newbasefile} ${jailfile}" |
There was a problem hiding this comment.
As with above, could this be executeconditional cp -p -- "${newbasefile}" "${jailfile}"
|
|
||
| C1_C6_conditions() { | ||
| filelistjail=$(find "${jail_etc}" -mindepth 1 -type f) | ||
| for jailfile in ${filelistjail} |
There was a problem hiding this comment.
The pattern of for X in $(find...) is risky if filenames contain whitespace or control chars, but the "usual" mechanisms to sanely deal with arbitrary filenames are typically bash-specific (e.g. find ... -print0 | while IFS= read -r -d''; do ... done).
Generally, in POSIX shells, the only supported way (I know of) is to find -exec sh -c '...' + or find -print0 | xargs -0 sh -c '...', which would introduce a subshell and affect availability/scope of your variables.
Perhaps, this may be better written as...
search_jail_etc_dir () {
[ -n "$1" ] || return 1
[ -d "$1" ] || return 1
for jailfile in "${1}"/*; do
if [ -d "${jailfile}" ]; then
search_jail_etc_dir "${jailfile}"
else
process_jail_etc_file "${jailfile}"
fi
done
}
process_jail_etc_file () {
...
}
search_jail_etc_dir "${jail_etc}"
This should ensure that filenames are properly quoted, so even if they contain garbage, they will still be handled correctly.
| txtvar="" | ||
| txtname="" | ||
| for x in 1 2 3 4 5 6 7 8; do | ||
| eval txtvar="\$"C$x |
There was a problem hiding this comment.
I think you are trying to loop over the passed function arguments here, i.e. $1, $2, etc...
Consider refactoring into a while [ -n "$1" ]; do ... shift; done -- then you can always use $1 and have no limitation on the number of arguments passed:
format_output_summary () {
printf 'SUMMARY:\n'
while [ -n "$1" ]; do
printf '%s = %s\n' "$1" "$2"
shift; shift
done
}
format_output_details () { : ... ; } #...todo
format_output () {
format_output_summary "$C1txt" "$C1" "$C2txt" "$C2" "$C3txt" "$C3" "$C4txt" "$C4" #...etc
format_output_details "$C1ct" "$C1txt" "$C2ct" "$C2txt" "$C3ct" "$C3txt" "$C4ct" "$C4txt" #...etc
}
| jailpath="${bastille_jail_base}/root/etc${dirpathnf}" | ||
| if [ ! -d "${jailpath}" ]; then | ||
| C7=$((C7+1)) | ||
| cmd="mkdir ${jailpath}" |
There was a problem hiding this comment.
As with above, could this be executeconditional mkdir -p -- "${jailpath}"
(the -p would make any missing intermediate parent dirs; omit it if you don't want this)
| cmd="mkdir ${jailpath}" | ||
| executeconditional "$cmd" | ||
| dirperm=$(stat -f "%Mp%Lp" "${dirpath}") | ||
| cmd="chmod ${dirperm} ${jailpath}" |
There was a problem hiding this comment.
As with above, could this be executeconditional chmod "${dirperm}" -- "${jailpath}"
| jailfile="${bastille_jail_base}/root/etc${dirpathnf}" | ||
| if [ ! -f "${jailfile}" ]; then | ||
| C8=$((C8+1)) | ||
| cmd="cp -p ${sourcefile} ${jailfile}" |
There was a problem hiding this comment.
As with above, could this be executeconditional cp -p -- "${sourcefile}" "${jailfile}"
| fi | ||
| } | ||
|
|
||
| C1_C6_conditions() { |
There was a problem hiding this comment.
In general, the C1-C8 naming convention makes it difficult to read what is going on, which might make for maintenance difficulty. Perhaps prefer functions and variables that actually describe the operation, rather than the opaque, e.g. user_added (I think that's C1), upgrade_added (C2?), for readability.
| if [ ! -f "${currentbasefile}" ]; then | ||
| if [ ! -f "${newbasefile}" ]; then | ||
| C1=$((C1+1)) | ||
| C1ct="$C1ct${jailfile}\n" |
There was a problem hiding this comment.
Even considering the readability of C1 (as above), the suffix ct does not make sense. I believe this is a file list, built up for display purposes, but ct could easily be taken to mean count.
Perhaps $user_added_list and $user_added_count would make it more explicit?
| fi | ||
|
|
||
| if [ -f "${currentbasefile}" ]; then | ||
| diffr=$(diff -u "${jailfile}" "${currentbasefile}") |
There was a problem hiding this comment.
If the output of diff -u is important, consider using temp files to capture it rather than variables. Variables can have whitespace folding and length issues, which might lead to unexpected corruption.
Also, if diff3 is available (i.e. if command -v diff3 >/dev/null 1>&2; then ... fi), perhaps use a threeway diff to generate auto-updated files, e.g. diff3 -A "${newbasefile}" "${currentbasefile}" "${jailfile}" (I think that's the right way around for those file args; it may also/alternatively need -m). This should be able to better differentiate between changes that were made by the end-user and changes that were introduced by the update. I think this is what freebsd-update does for the "normal" install/upgrade procedure, and it might be good to do something similar here.
Perhaps use diffdir="$(mktemp -d -t "${TMPDIR:-/tmp}/etcupdate.XXXXXX")" to make a temporary directory, then write your diffs into the temp dir. Let the end-user decide whether to use your version or the existing version, then move the desired version out of the directory into the correct place. Use trap "[ -d '${diffdir}' ] && rm -rf -- '${diffdir}'" EXIT INT TERM to clean up all unused temp files at the end of the run.
Also, perhaps allow end-user to invoke vi (or "${VISUAL:-${EDITOR:-vi}}" for style points) on the updated file, so they can make manual edits to each file prior to acceptance. Especially, if doing in-place diffs with diff3, you can use the exit code to determine if a diff conflict has happened ([ "$?" -eq 1 ]), and prompt them to manually resolve.
Hi.
Started testing bastille recently and faced problems when upgrading 13.2-RELEASE 'test' jail to 14.0-RELEASE. The jail could start but wasn't able to 'bastille console' into the jail. This helped me solving this problem: https://forums.freebsd.org/threads/bastille-upgrading-jail-from-13-2-to-14-0-fails.91234/
I understand that this is not bastille fault, intrinsically. By the other hand it's concerning. It's non trivial having ./etc in sync considering different use cases, security changes, features or any other mix of situations like the issue I had.
The idea of 'etcupdate' command is to achieve a minimal level of sanity of ./etc folder, while being 'conservative' in regard to user modifications/additions made there. Includes a dry-run option and logs a semi-detailed log output. It's possible to see what it does checking the attached logfile produced by:
"bastille etcupdate -D testjail 13.2-RELEASE 14.0-RELEASE" from untouched 13.2-RELEASE jail upgraded to 14.0-RELEASE.
The options of what FreeBSD versions to compare are explicit. That's, script will compare actual jail's ./etc files against the two any reference versions, distributed along 8 conditions executed serially.
I don't know if this type of approach would be considered bastille scope by project owners. It's something I had to do anyway (do something about) because I'm worried about actual conditions of my prod jails (accumulating updates since... can't even remember). I tried to test it as thoroughly as I could ATM and it looks OK. Tested with bastille with ZFS and without with same results.
Thanks!
etcupdate.log