Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update common.sh with functions for all jails+first migration "console.sh" #792

Open
wants to merge 34 commits into
base: master
Choose a base branch
from

Conversation

tschettervictor
Copy link
Collaborator

@tschettervictor tschettervictor commented Jan 6, 2025

This PR does 3 things.

  1. Rename two functions inside the main executable to have the suffix _old
  • this is because the new functions inside commons.sh are named the same, and since the old ones are only ever called inside the main executable, it is best to rename them for now, for future removal
  1. Update common.sh with the new functions which include "set_target_single" "set_target" and others that will be used in the future.

  2. Update console command to allow use with JID and jailname autocomplete

And as a treat, adds the "debug mode" function.

@JRGTH can you go over this and see if it will fit current code structure?
@bmac2

To test

  • use the console command with JID or (next for nextcloud) etc...
  • if you have more than one jail with next as the prefix, you should get an error, if not, it should simply let you into the jail

Just a quick note. This PR should affect nothing except the console command. Everything else will still use the standard methods of validating jail names, check if running/stopped etc... so shouldn't be too hard to test.

Rename "check_target_is_running" and "target_all_jails" to have the suffix "_old".

These two function are only ever called from within the main bastille executable. 

I am integrating these functions in "common.sh" so renaming them here for removal in the future is the best path forward.
These are all the new function I am beginning to move to common.sh for use with all jails.
This makes console act with the new common.sh file.
Enable debug mode
Allow console by JID
Allow jail autocomplete
@tschettervictor tschettervictor mentioned this pull request Jan 6, 2025
This is because bastille0 is always used ad the default loopback when using "bastille setup" and interferes when trying to create the first VNET jail on the list. This ensures that VNET jails will have their epairs (non-bridge) start with bastille1 then 2 then 3 etc...
Copy link
Collaborator

@JRGTH JRGTH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello, I would like more insight about this PR as I have no "bastille0" loopback set and I have both VNET and shared IP jails, all of them works perfectly fine and both jails types gets internet as expected.

Here part of my bastille network config example for further reference:

## Networking
bastille_network_loopback=""                                          ## default: "bastille0"
bastille_network_pf_ext_if="ext_if"                                   ## default: "ext_if"
bastille_network_pf_table="jails"                                     ## default: "jails"
bastille_network_shared="em0"                                         ## default: ""
bastille_network_gateway=""                                           ## default: ""
bastille_network_gateway6=""                                          ## default: ""

Notice that ${bastille_network_loopback}/${bastille_network_gateway} are empty/non-set but only the shared em0 physical interface.

However I have to note that I don't use neither PF nor IPFW for jails since my Firewall stuff is managed by post-ISP external OPNsense server/appliance, so I keep my shared-IP/VNET jails networking as simple as possible on my homelab, so excuse my ignorance here.

So just thinking, can this additional IF can be created if the user is explicitly is using "bastille0" loopback?, so it keep system/configuration simplicity and avoid unnecessary interfaces?

PR in this regards: 02396f5

Regards!

@tschettervictor
Copy link
Collaborator Author

tschettervictor commented Jan 6, 2025

So, when you run "bastille setup" an interface gets created for use with non-VNET jails called "bastille0" and populates the "bastille_loopback" variable with that. This interface is used when creating jails and not specifying an interface. If you don not use pf/firewall, then of course this won't matter, except if you run "bastille setup", then your first VNET jail won't ever work because it also tries to use "bastille0" as the interface. This PR starts the count at bastille1 (which to me is better anyway)

I ran into this problem just yesterday when trying to create a VNET jails after having run "bastille setup". The VNET jails tried to create e0a_bastille0 and e0b_bastille0. The jail wouldn't start because it couldn't create the interface pairs. Switching it to start at e0[ab]_bastille1 solved it.

This commit only changes that one fact and the one below. Its starts looking for "epair[1-9]+" for bridges and "bastille[1-9]+" for VNET.

I've chosen to also change the naming schema of the bridge epairs to stay simply "epair1[ab]" and add a description as -V does because of a naming limit I ran into. See #786

@tschettervictor
Copy link
Collaborator Author

After further studying, we should find a better way to handle bastille setup

Doing bastille setup vnet will also throw a wrench into this, because it creates a bridge, and names it bastille1.

Perhaps we should change the default names to "bastille_bridge" and "bastille_loopback" to avoid these issues.

Thoughts?

@tschettervictor
Copy link
Collaborator Author

Below is the relevant code inside "setup.sh"

# Configure bastille loopback network interface
configure_network() {
    info "Configuring ${bastille_network_loopback} loopback interface"
    sysrc cloned_interfaces+=lo1
    sysrc ifconfig_lo1_name="${bastille_network_loopback}"

    info "Bringing up new interface: ${bastille_network_loopback}"
    service netif cloneup
}

configure_vnet() {
    info "Configuring bridge interface"
    sysrc cloned_interfaces+=bridge1
    sysrc ifconfig_bridge1_name=bastille1

    info "Bringing up new interface: bastille1"
    service netif cloneup

    if [ ! -f /etc/devfs.rules ]; then
        info "Creating bastille_vnet devfs.rules"
        cat << EOF > /etc/devfs.rules
[bastille_vnet=13]
add include \$devfsrules_hide_all
add include \$devfsrules_unhide_basic
add include \$devfsrules_unhide_login
add include \$devfsrules_jail
add include \$devfsrules_jail_vnet
add path 'bpf*' unhide
EOF
    fi
}

@JRGTH
Copy link
Collaborator

JRGTH commented Jan 6, 2025

So, when you run "bastille setup" an interface gets created for use with non-VNET jails called "bastille0" and populates the "bastille_loopback" variable with that. This interface is used when creating jails and not specifying an interface. If you don not use pf/firewall, then of course this won't matter.

I ran into this problem just yesterday when trying to create a VNET jails after having run "bastille setup". The VNET jails tried to create e0a_bastille0 and e0b_bastille0. The jail wouldn't start because it couldn't create the interface pairs. Switching it to start at e0[ab]_bastille1 solved it.

This commit only changes that one fact and the one below. Its starts looking for "epair[1-9]+" for bridges and "bastille[1-9]+" for VNET.

I've chosen to also change the naming schema of the bridge epairs to stay simply "epair1[ab]" and add a description as -V does because of a naming limit I ran into. See #786

Hello, I understand that this takes place only when using the loopback and/or PF/Firewall, more on this below.

I've just executed bastille setup just to see what happens, then it does not asked me to perform something(or any agreement/options to perform for) and my configuration was wiped, I will post some screenshots from Webmin about this critical bug:

Part of my bastille.conf config before bastille setup execution:
Screenshot from 2025-01-06 14-06-16

Part of my bastille.conf config after bastille setup execution:
Screenshot from 2025-01-06 14-05-09

I'm a bit busy ATM to perform a full test on all the PR's but I will try to look this more in deep on the weekend if possible, so hopefully you and other developers/collaborators/contributors can test this PR's in every possible case user scenario, from simple bastille network setups setups to more complex ones and sames applies for ZFS and UFS as the underlying storage filesystem.

Between keep this nice contributions going so we can enhance this little jail manager further.

Regards!

@tschettervictor
Copy link
Collaborator Author

Your "bastille_network_loopback" is not filled in (as you said above) with the default value of "bastille0"
On a new install, this will be filled with "bastille0" by default.

@JRGTH
Copy link
Collaborator

JRGTH commented Jan 6, 2025

After further studying, we should find a better way to handle bastille setup

Doing bastille setup vnet will also throw a wrench into this, because it creates a bridge, and names it bastille1.

Perhaps we should change the default names to "bastille_bridge" and "bastille_loopback" to avoid these issues.

Thoughts?

Hello, the major problem/issue is that changing variable names already used for long(specially bastille config) may break current configurations for most user unaware of it, and may broke third party jails managers such as XigmaNAS Bastille Ext and Webmin Bastille manager among others DevOps related operations, if we are talking about bastille core code we can always find a solution indeed.

Hopefully this can ve solved however.

Regards!

@tschettervictor
Copy link
Collaborator Author

Good point.
Perhaps starting a new naming schema for VNET jails would then be best. This would leave the old ones still in place and working, and going forward they would just have new names.

Right now they are called "e0b_bastille[0-9]+" so perhaps "bastille_vnet[0-9]+" would be a good starting place. This not only tells you what the interface is being used as, but also tells you it is a VNEt interface, just like "epair" tells you it is a bridge interface.

That way, bastille0 can stay the loopback, and bastille1 can stay the bridge loopback.

@JRGTH
Copy link
Collaborator

JRGTH commented Jan 6, 2025

Good point. Perhaps starting a new naming schema for VNET jails would then be best. This would leave the old ones still in place and working, and going forward they would just have new names.

Right now they are called "e0b_bastille[0-9]+" so perhaps "bastille_vnet[0-9]+" would be a good starting place. This not only tells you what the interface is being used as, but also tells you it is a VNEt interface, just like "epair" tells you it is a bridge interface.

That way, bastille0 can stay the loopback, and bastille1 can stay the bridge loopback.

Hi, yes name schemas for basic/vnet/linux jails is a nice addition and can also increase readability for further code addition/updates, between be aware that the part of eXb_ denotes the (b)ridge prefix and we need to keep compatibility with the jib command as well as it uses this schemas already.

# jib
Usage: jib action [arguments]
Actions:
	addm [-b BRIDGE_NAME] NAME [!]iface0 [[!]iface1 ...]
		Creates e0b_NAME [e1b_NAME ...]
	show
		List possible NAME values for `show NAME'
	show NAME
		Lists e0b_NAME [e1b_NAME ...]
	destroy NAME
		Destroy e0b_NAME [e1b_NAME ...]

Regards!

@tschettervictor
Copy link
Collaborator Author

Yes I am aware. Just throwing out ideas.

bastille_vnet1a
bastille_vnet1b

bastille_vnet2a
bastille_vnet2b

Would work.

I can't really think of a better schema right now.

@JRGTH
Copy link
Collaborator

JRGTH commented Jan 6, 2025

Yes I am aware. Just throwing out ideas.

bastille_vnet1a bastille_vnet1b

bastille_vnet2a bastille_vnet2b

Would work.

I can't really think of a better schema right now.

Yeah that should work indeed, between this may be discussed further with developers and @cedwards in regards of this naming changes and adoption/ideas.

Lets see for further comments/opinions/ideas about this from other users running in production as well.

Regards!

@tschettervictor
Copy link
Collaborator Author

On even further consideration, I will adjust this PR.

I will be focusing on just the other functions people are asking for.

This way we can discuss what the path forward is for networking.

Thanks for your interaction @JRGTH

@tschettervictor
Copy link
Collaborator Author

@JRGTH I've reverted so this PR now only adds the new functions, and updates console.sh to use them.
We will discuss networking in a future PR.

See it you can test if more easily now.

@tschettervictor tschettervictor changed the title Update common.sh with functions for all jails+first migration "console.sh Update common.sh with functions for all jails+first migration "console.sh" Jan 6, 2025
@bmac2
Copy link
Collaborator

bmac2 commented Jan 9, 2025

per @cedwards the setup was a hack he threw together. I think since it tends to clobber everything that we may want to disable that for now until we can discuss further what a "setup" command should actually do. @tschettervictor and I talked about adding a make a .bak file of the config file, but in reading all of this discussion and thinking through all the possibilities, I am not sure having the setup is even a good idea. What do you guys think?

@yaazkal @cedwards @JRGTH @tschettervictor

@tschettervictor
Copy link
Collaborator Author

tschettervictor commented Jan 9, 2025

I think a check for each setup sub command would work.

If the ZFS vars are filled, exit.

If the interface it tries to create exists, exit.

If the pf file exists, exit. (This last one it already does)

Or have an interactive script that will ask the user to confirm every step.

@JRGTH
Copy link
Collaborator

JRGTH commented Jan 9, 2025

per @cedwards the setup was a hack he threw together. I think since it tends to clobber everything that we may want to disable that for now until we can discuss further what a "setup" command should actually do. @tschettervictor and I talked about adding a make a .bak file of the config file, but in reading all of this discussion and thinking through all the possibilities, I am not sure having the setup is even a good idea. What do you guys think?

@yaazkal @cedwards @JRGTH @tschettervictor

Hi Barry, yes this command is more so like a wrapper/helper for a quick ZFS/Network/Firewall etc setup, and may be useful for newcomers and for third-party managers using bastille under the hood, however several system checks has to be made for example in the ZFS setup we need to perform some deep system checks as not every user uses ZFS, and some users using ZFS don't put bastille in a dataset to keep configuration simplicity, on top of this there are the diehard UFS only users, the simple "shared-IP"(no Firewall, VNET etc.) users with a standard/simple network requirements and so on.

I will provide some help here in the ZFS/UFS department for a proper ZFS activation in bastille and for the standard/simple networking setup, however there is no magical/guess for every system configuration solely based on "what just we think personally" to just trow an unattended auto-setup, hence a options/text based user interactive/detailed menu for some options is still required for a proper configuration solely based on what the user wants in regards his hardware/config.

I will wait for the currents PR's as they need deep testing, so I can provide some help in this setup.sh command to make it compliant with options and user interactive friendly, yet with fallback, safe defaults, current config backup/revert options etc to avoid nuking bastille configuration.

Regards!

@bmac2
Copy link
Collaborator

bmac2 commented Jan 9, 2025

@JRGTH @tschettervictor should we have a separate PR for fixing / updating setup subcommand? I don't want this PR here to become too hard to test with too many moving pieces. I like smaller PRs that are cleaner to test. Thoughts?

@yaazkal

@tschettervictor
Copy link
Collaborator Author

Separate I think.

@JRGTH
Copy link
Collaborator

JRGTH commented Jan 9, 2025

@JRGTH @tschettervictor should we have a separate PR for fixing / updating setup subcommand? I don't want this PR here to become too hard to test with too many moving pieces. I like smaller PRs that are cleaner to test. Thoughts?

@yaazkal

+1, This command should be on a separate PR and needs special attention and careful testing, as this command does write to bastille configuration.

I will start to re-write this command specially on the ZFS/UFS filesystem setups, the detailed/interactive user options and menu, then after deep testing on a several system configurations including ZFS and UFS configs I can upload so other with experience in the complex networking part can follow the code layout/consistency and enhance the rest of the bits.

Regards

@JRGTH
Copy link
Collaborator

JRGTH commented Jan 9, 2025

Separate I think.

+1 on this as well.

Regards!

@bmac2
Copy link
Collaborator

bmac2 commented Jan 9, 2025

I will start to re-write this command specially on the ZFS/UFS filesystem setups, the detailed/interactive user options and menu, then after deep testing on a several system configurations including ZFS and UFS configs I can upload so other with experience in the complex networking part can follow the code layout/consistency and enhance the rest of the bits.

so @JRGTH you start a PR for the setup command rewrite please. Lets fix Christer's setup hack to the next level with testing and some warnings.

@yaazkal @tschettervictor @cedwards

@JRGTH
Copy link
Collaborator

JRGTH commented Jan 9, 2025

I will start to re-write this command specially on the ZFS/UFS filesystem setups, the detailed/interactive user options and menu, then after deep testing on a several system configurations including ZFS and UFS configs I can upload so other with experience in the complex networking part can follow the code layout/consistency and enhance the rest of the bits.

so @JRGTH you start a PR for the setup command rewrite please. Lets fix Christer's setup hack to the next level with testing and some warnings.

@yaazkal @tschettervictor @cedwards

No worry man, I will start the rewrite of setup.sh this Friday internally(local repo), after deep testing the ZFS related code on FreeBSD 13/14 32/64Bit ZFS/UFS etc, will submit the enhanced PR to minimize lots of smaller PR's for other users readability ease.

Regards!

@JRGTH
Copy link
Collaborator

JRGTH commented Jan 13, 2025

The setup.sh is already under rewrite and the default ZFS activation is under heavy test against several config parameters scenarios for common bastille installations(ports/pkg), also ZFS activation for customized install is already being implemented as well , then rest of the code accordingly.

EDIT: Initial interactive ZFS activation helper is tested/completed for systems booting from ZFS as the default, now I'm testing alternate configurations for systems booting from UFS systems but with jails stored on dedicated ZFS storage pool to keep both worlds happy, after a deep re-test will submit an initial PR to be open for additional enhancements.

Regards!

@bmac2
Copy link
Collaborator

bmac2 commented Jan 20, 2025

Tested using JID to console into a jail.
autocomplete upon hitting ENTER, not tab works if the name you started typing is unique.

Tested as advertised.

@yaazkal @tschettervictor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants