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.
|
Wondering if one could simply replace those files with the files from the upgraded release...? |
You can if you haven't made any changes to them between the two releases, but configuration/personalisation of a system usually requires that you edit these a lot. The point of an ideal |
|
Good point. But isn't it best practice to stay out of there? And make any customizations inside /usr/local/etc instead? |
In a perfect world, yes. In practice, you are probably getting hung up on Or... |
|
And how is this PR working for you so far? Is it functional? |
Can't honestly say at this point. Lack of support for etcupdate (i.e. the FreeBSD function) was a blocker for me implementing Bastille, back in 2021. I remain interested, but am not in a position to test. Gut feel when I offered up my review comments was that the PR itself might not be ready for prime time, yet. |
|
I don't know if this PR has risen to the level of something that should be added. Perhaps with some polish, it would. But I like it a lot, in certain circumstances. Consider this workflow:
In Step 8, it identifies (in what is called Note: this was a smooth, ideal process because I did not upgrade the point releases. I knew that For now, I'm content to not have point release upgrades for my thin jails, because in exchange I am able to completely* upgrade when a new release is available. * Well, everything will be done automatically except the items in I'll continue to manually patch in this script. Hopefully someone comes up with an elegant way to handle point releases (or comes up with a way to upgrade from any given stale |
|
Are you aware that there is a command in freebsd called This is how Appjail does it. We could simply integrate this into a bastille command as Appjail does. |
|
Would it not be better to have the A better structure IMO would be to allow the |
|
Consider this script i just spun up. It has no error checking currently, so be careful. This will simply bootstrap and tar whichever RELEASE specified. This tarball can then be used with a simple etcupdate command to compare and merge any jail to. Use -d for a dry run. EDIT Ive now used this to update my jail to a 14.2-RELEASE etc and then back to a 14.0-RELEASE etc Would be nice to have people give it a go... |
|
The only down side to what I mentioned above is that the src.txz has to be downloaded. The upside is that it used the built in etcupdate command so any upstream changes will already be included when bastille tries to run it. And it looks like I was able jump back and forth from 14 abs 14.2, which won't remove the need to specify the current release. It with just update the jail etc to whichever release was specified. Would be nice if @bmac2 @yaazkal would share their thoughts on what the best way for ward would be in their minds. |
Yes, I'm sure that is why the subcommand was named as it was 😄
I think this is why I originally avoided using native If I remember correctly, Bastille initially copies In my old home-grown jail management (predating Bastille, but with some similarities; hence why I was looking to replace with Bastille), my approach was essentially to But the idea of effectively "bootstrapping" |
|
When using the etcupdate default command, it will use the working directory + /var/db/etcupdate. This puts each location inside each jail, so I think it should work out just fine. |
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