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.

Sign up for GitHub

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

Inconsistent handling of --dry-run in different configs. #54

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Inconsistent handling of --dry-run in different configs. #54

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 18 additions & 15 deletions nilrt_snac/_configs/_console_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,26 +14,29 @@ def __init__(self):
def configure(self, args: argparse.Namespace) -> None:
print("Deconfiguring console access...")

if args.dry_run:
return

subprocess.run(
["nirtcfg", "--set", "section=systemsettings,token=consoleout.enabled,value=False"],
check=True,
)
if not args.dry_run:
subprocess.run(
["nirtcfg", "--set", "section=systemsettings,token=consoleout.enabled,value=False"],
check=True,
)
else:
print("Dry run: would have run nirtcfg --set section=systemsettings,token=consoleout.enabled,value=False")
opkg.remove("sysconfig-settings-console", force_depends=True)

def verify(self, args: argparse.Namespace) -> bool:
print("Verifying console access configuration...")
valid = True
result = subprocess.run(
["nirtcfg", "--get", "section=systemsettings,token=consoleout.enabled"],
check=True,
stdout=subprocess.PIPE,
)
if result.stdout.decode().strip() != "False":
valid = False
logger.error("FOUND: console access not diabled")
if not args.dry_run:
result = subprocess.run(
["nirtcfg", "--get", "section=systemsettings,token=consoleout.enabled"],
check=True,
stdout=subprocess.PIPE,
)
if result.stdout.decode().strip() != "False":
valid = False
logger.error("FOUND: console access not diabled")
else:
print("Dry run: would have run nirtcfg --get section=systemsettings,token=consoleout.enabled")
if opkg.is_installed("sysconfig-settings-console"):
valid = False
logger.error("FOUND: sysconfig-settings-console still installed.")
Expand Down
153 changes: 84 additions & 69 deletions nilrt_snac/_configs/_firewall_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,49 +6,65 @@
from nilrt_snac import logger
from nilrt_snac.opkg import opkg_helper

def _cmd(*args: str):
def _cmd(dry_run: bool, *args: str):
"Syntactic sugar for firewall-cmd -q."
subprocess.run(["firewall-cmd", "-q"] + list(args), check=True)
if not dry_run:
subprocess.run(["firewall-cmd", "-q"] + list(args), check=True)
else:
print("Dry run: would have run firewall-cmd -q " + " ".join(args))

def _offlinecmd(*args: str):
def _offlinecmd(dry_run: bool, *args: str):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor. Could these dry_run parameters be keywords at the end of the method prototype that default to False, so that devs don't have to specify _offlinecmd(dry_run,... every time they want to use the method?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When would you ever not need to pass dry_run in? The whole purpose of the parameter is to not run the command when the --dry-run parameter is passed in on the command line.

Copy link
Collaborator

Choose a reason for hiding this comment

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

When you're... not doing a dry-run? I don't understand the question.

"Syntactic sugar for firewall-offline-cmd -q."
subprocess.run(["firewall-offline-cmd", "-q"] + list(args), check=True)
if not dry_run:
subprocess.run(["firewall-offline-cmd", "-q"] + list(args), check=True)
else:
print("Dry run: would have run firewall-offline-cmd -q " + " ".join(args))

def _check_target(policy: str, expected: str = "REJECT") -> bool:
def _check_target(dry_run: bool, policy: str, expected: str = "REJECT") -> bool:
"Verifies firewall-cmd --policy=POLICY --get-target matches what is expected."

actual: str = subprocess.getoutput(
f"firewall-cmd --permanent --policy={policy} --get-target")
if expected == actual:
if not dry_run:
actual: str = subprocess.getoutput(
f"firewall-cmd --permanent --policy={policy} --get-target")
if expected == actual:
return True
logger.error(f"ERROR: policy {policy} target: expected {expected}, observed {actual}")
return False
else:
print("Dry run: would have run firewall-cmd --policy=" + policy + " --get-target")
return True
logger.error(f"ERROR: policy {policy} target: expected {expected}, observed {actual}")
return False

def _check_service(Q: str, service: str, expected: str = "yes") -> bool:
def _check_service(dry_run: bool, Q: str, service: str, expected: str = "yes") -> bool:
"""Verifies firewall-cmd (--policy=POLICY/--zone=ZONE/etc.) --query-service=SERVICE
matches what is expected.
"""

actual: str = subprocess.getoutput(
f"firewall-cmd --permanent {Q} --query-service={service}")
if expected == actual:
if not dry_run:
actual: str = subprocess.getoutput(
f"firewall-cmd --permanent {Q} --query-service={service}")
if expected == actual:
return True
logger.error(f"ERROR: {Q} service {service}: expected {expected}, observed {actual}")
return False
else:
print("Dry run: would have run firewall-cmd " + Q + " --query-service=" + service)
return True
logger.error(f"ERROR: {Q} service {service}: expected {expected}, observed {actual}")
return False


def _check_service_info(service: str, Q: str, expected: str) -> bool:
def _check_service_info(dry_run: bool, service: str, Q: str, expected: str) -> bool:
"""Verifies firewall-cmd --service=SERVICE (--get-ports/--get-description/etc.)
matches what is expected.
"""

actual: str = subprocess.getoutput(
f"firewall-cmd --permanent --service={service} {Q}")
if expected == actual:
if not dry_run:
actual: str = subprocess.getoutput(
f"firewall-cmd --permanent --service={service} {Q}")
if expected == actual:
return True
logger.error(f"ERROR: service {service} {Q}: expected {expected}, observed {actual}")
return False
else:
print("Dry run: would have run firewall-cmd --service=" + service + " " + Q)
return True
logger.error(f"ERROR: service {service} {Q}: expected {expected}, observed {actual}")
return False


class _FirewallConfig(_BaseConfig):
def __init__(self):
Expand All @@ -57,59 +73,57 @@ def __init__(self):
def configure(self, args: argparse.Namespace) -> None:
print("Configuring firewall...")
dry_run: bool = args.dry_run
if dry_run:
return

# nftables installed via deps
self._opkg_helper.install("firewalld")
self._opkg_helper.install("firewalld-offline-cmd")
self._opkg_helper.install("firewalld-log-rotate")
self._opkg_helper.install("ni-firewalld-servicedefs")

_offlinecmd("--reset-to-defaults")
_offlinecmd(dry_run, "--reset-to-defaults")

_offlinecmd("--zone=work", "--add-interface=wglv0")
_offlinecmd("--zone=work", "--remove-forward")
_offlinecmd("--zone=public", "--remove-forward")
_offlinecmd(dry_run, "--zone=work", "--add-interface=wglv0")
_offlinecmd(dry_run, "--zone=work", "--remove-forward")
_offlinecmd(dry_run, "--zone=public", "--remove-forward")

_offlinecmd("--new-policy=work-in")
_offlinecmd("--policy=work-in", "--add-ingress-zone=work")
_offlinecmd("--policy=work-in", "--add-egress-zone=HOST")
_offlinecmd("--policy=work-in", "--add-protocol=icmp")
_offlinecmd("--policy=work-in", "--add-protocol=ipv6-icmp")
_offlinecmd("--policy=work-in",
_offlinecmd(dry_run, "--new-policy=work-in")
_offlinecmd(dry_run, "--policy=work-in", "--add-ingress-zone=work")
_offlinecmd(dry_run, "--policy=work-in", "--add-egress-zone=HOST")
_offlinecmd(dry_run, "--policy=work-in", "--add-protocol=icmp")
_offlinecmd(dry_run, "--policy=work-in", "--add-protocol=ipv6-icmp")
_offlinecmd(dry_run, "--policy=work-in",
"--add-service=ssh",
"--add-service=mdns",
)

_offlinecmd("--new-policy=work-out")
_offlinecmd("--policy=work-out", "--add-ingress-zone=HOST")
_offlinecmd("--policy=work-out", "--add-egress-zone=work")
_offlinecmd("--policy=work-out", "--add-protocol=icmp")
_offlinecmd("--policy=work-out", "--add-protocol=ipv6-icmp")
_offlinecmd("--policy=work-out",
_offlinecmd(dry_run, "--new-policy=work-out")
_offlinecmd(dry_run, "--policy=work-out", "--add-ingress-zone=HOST")
_offlinecmd(dry_run, "--policy=work-out", "--add-egress-zone=work")
_offlinecmd(dry_run, "--policy=work-out", "--add-protocol=icmp")
_offlinecmd(dry_run, "--policy=work-out", "--add-protocol=ipv6-icmp")
_offlinecmd(dry_run, "--policy=work-out",
"--add-service=ssh",
"--add-service=http",
"--add-service=https",
)
_offlinecmd("--policy=work-out", "--set-target=REJECT")

_offlinecmd("--new-policy=public-in")
_offlinecmd("--policy=public-in", "--add-ingress-zone=public")
_offlinecmd("--policy=public-in", "--add-egress-zone=HOST")
_offlinecmd("--policy=public-in", "--add-protocol=icmp")
_offlinecmd("--policy=public-in", "--add-protocol=ipv6-icmp")
_offlinecmd("--policy=public-in",
_offlinecmd(dry_run, "--policy=work-out", "--set-target=REJECT")

_offlinecmd(dry_run, "--new-policy=public-in")
_offlinecmd(dry_run, "--policy=public-in", "--add-ingress-zone=public")
_offlinecmd(dry_run, "--policy=public-in", "--add-egress-zone=HOST")
_offlinecmd(dry_run, "--policy=public-in", "--add-protocol=icmp")
_offlinecmd(dry_run, "--policy=public-in", "--add-protocol=ipv6-icmp")
_offlinecmd(dry_run, "--policy=public-in",
"--add-service=ssh",
"--add-service=wireguard",
)

_offlinecmd("--new-policy=public-out")
_offlinecmd("--policy=public-out", "--add-ingress-zone=HOST")
_offlinecmd("--policy=public-out", "--add-egress-zone=public")
_offlinecmd("--policy=public-out", "--add-protocol=icmp")
_offlinecmd("--policy=public-out", "--add-protocol=ipv6-icmp")
_offlinecmd("--policy=public-out",
_offlinecmd(dry_run, "--new-policy=public-out")
_offlinecmd(dry_run, "--policy=public-out", "--add-ingress-zone=HOST")
_offlinecmd(dry_run, "--policy=public-out", "--add-egress-zone=public")
_offlinecmd(dry_run, "--policy=public-out", "--add-protocol=icmp")
_offlinecmd(dry_run, "--policy=public-out", "--add-protocol=ipv6-icmp")
_offlinecmd(dry_run, "--policy=public-out",
"--add-service=dhcp",
"--add-service=dhcpv6",
"--add-service=http",
Expand All @@ -118,30 +132,31 @@ def configure(self, args: argparse.Namespace) -> None:
"--add-service=dns",
"--add-service=ntp",
)
_offlinecmd("--policy=public-out", "--set-target=REJECT")
_offlinecmd(dry_run, "--policy=public-out", "--set-target=REJECT")

_offlinecmd("--policy=work-in",
_offlinecmd(dry_run, "--policy=work-in",
"--add-service=ni-labview-realtime",
"--add-service=ni-labview-viserver",
"--add-service=ni-logos-xt",
"--add-service=ni-mxs",
"--add-service=ni-rpc-server",
"--add-service=ni-service-locator",
)
_offlinecmd("--policy=work-in",
_offlinecmd(dry_run, "--policy=work-in",
# Temporary port add; see x-niroco-static-port.ini
"--add-port=55184/tcp",
)
_offlinecmd("--policy=work-out",
_offlinecmd(dry_run, "--policy=work-out",
"--add-service=amqp",
"--add-service=salt-master",
)

_cmd("--reload")
_cmd(dry_run, "--reload")

def verify(self, args: argparse.Namespace) -> bool:
print("Verifying firewall configuration...")
valid: bool = True
dry_run: bool = args.dry_run

try:
pid: int = int(subprocess.getoutput("pidof -x /usr/sbin/firewalld"))
Expand All @@ -150,19 +165,19 @@ def verify(self, args: argparse.Namespace) -> bool:
valid = False

try:
_cmd("--check-config")
_cmd(dry_run, "--check-config")
except FileNotFoundError:
logger.error(f"MISSING: firewall-cmd")
valid = False

valid = all([
_check_target("work-in", "CONTINUE"),
_check_target("work-out"),
_check_target("public-in", "CONTINUE"),
_check_target("public-out"),
_check_service("--policy=work-in", "ni-labview-realtime"),
_check_service("--policy=public-in", "ni-labview-realtime", "no"),
_check_service("--zone=public", "ni-labview-realtime", "no"),
_check_target(dry_run, "work-in", "CONTINUE"),
_check_target(dry_run, "work-out"),
_check_target(dry_run, "public-in", "CONTINUE"),
_check_target(dry_run, "public-out"),
_check_service(dry_run, "--policy=work-in", "ni-labview-realtime"),
_check_service(dry_run, "--policy=public-in", "ni-labview-realtime", "no"),
_check_service(dry_run, "--zone=public", "ni-labview-realtime", "no"),
_check_service_info("ni-labview-realtime", "--get-ports", "3079/tcp"),
])

Expand Down
7 changes: 4 additions & 3 deletions nilrt_snac/_configs/_graphical_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,11 @@ class _GraphicalConfig(_BaseConfig):

def configure(self, args: Namespace) -> None:
print("Deconfiguring the graphical UI...")
if args.dry_run:
return

run(["nirtcfg", "--set", "section=systemsettings,token=ui.enabled,value=False"], check=True)
if not args.dry_run:
run(["nirtcfg", "--set", "section=systemsettings,token=ui.enabled,value=False"], check=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have a concern that the nirtcfg binary will not be in the PATH for non-interactive users. Current, the binary is under /usr/local/natinst/..., which is a nonstandard path which gets sourced by interactive logins, but not by non-interactive users (iirc). Could you test this by issuing a call to the configuration tool via a non-interactive ssh shell? ssh -c "nilrt-snac configure --dry-run".

I have an AZDO PR to move nirtcfg into the PATH, but I've had trouble getting linuxpkg to work, so it isn't in yet.

else:
print("Dry run: would have run nirtcfg --set section=systemsettings,token=ui.enabled,value=False")
opkg.remove("packagegroup-ni-graphical", autoremove=True)
opkg.remove("packagegroup-core-x11", autoremove=True)

Expand Down
4 changes: 3 additions & 1 deletion nilrt_snac/_configs/_niauth_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,11 @@ def configure(self, args: argparse.Namespace) -> None:
if not self._opkg_helper.is_installed("nilrt-snac-conflicts"):
self._opkg_helper.install(str(SNAC_DATA_DIR / "nilrt-snac-conflicts.ipk"))

logger.debug("Removing root password")
if not dry_run:
logger.debug("Removing root password")
subprocess.run(["passwd", "-d", "root"], check=True)
else:
print("Dry run: would have run passwd -d root")

def verify(self, args: argparse.Namespace) -> bool:
print("Verifying NIAuth...")
Expand Down
4 changes: 3 additions & 1 deletion nilrt_snac/_configs/_ntp_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,11 @@ def configure(self, args: argparse.Namespace) -> None:
config_file.add("server 0.us.pool.ntp.mil iburst maxpoll 16")

config_file.save(dry_run)
logger.debug("Restarting ntp service")
if not dry_run:
logger.debug("Restarting ntp service")
subprocess.run(["/etc/init.d/ntpd", "restart"])
else:
print("Dry run: would have run /etc/init.d/ntpd restart")

def verify(self, args: argparse.Namespace) -> bool:
print("Verifying NTP configuration...")
Expand Down
2 changes: 2 additions & 0 deletions nilrt_snac/_configs/_opkg_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ def configure(self, args: argparse.Namespace) -> None:
logger.debug("Removing unsupported package feeds...")
if not dry_run:
subprocess.run(["rm", "-fv", "/etc/opkg/NI-dist.conf"], check=True)
else:
print("Dry run: would have run rm -fv /etc/opkg/NI-dist.conf")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be more useful to describe the operation in concept, rather than repeating the exact command.

  1. Users who are issuing the dry run don't necessarily understand what the command string is supposed to do.
  2. Devs who are trying to debug using the dry-run are going to grep for the output sting and see the real command in the source anyway.
  3. It would help keep the dry-run output string from falling out of sync w/ the real code.


if base_feeds_config_file.contains("src.*/extra/.*"):
base_feeds_config_file.update("^src.*/extra/.*", "")
Expand Down
4 changes: 3 additions & 1 deletion nilrt_snac/_configs/_wifi_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,12 @@ def configure(self, args: argparse.Namespace) -> None:
)

config_file.save(dry_run)
logger.debug("Removing any WiFi modules in memory")
if not dry_run:
logger.debug("Removing any WiFi modules in memory")
# We do not check for success. If the modules are not loaded, this will return an error
subprocess.run(["rmmod", "cfg80211", "mac80211"], check=False)
else:
print("Dry run: would have run rmmod cfg80211 mac80211")

def verify(self, args: argparse.Namespace) -> bool:
print("Verifying WiFi configuration...")
Expand Down
5 changes: 4 additions & 1 deletion nilrt_snac/_configs/_wireguard_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@ def configure(self, args: argparse.Namespace) -> None:
private_key.save(dry_run)
public_key.save(dry_run)
ifplug_conf.save(dry_run)
logger.debug("Restating wireguard service")
if not dry_run:
logger.debug("Restating wireguard service")
subprocess.run(
[
"update-rc.d",
Expand All @@ -75,6 +75,9 @@ def configure(self, args: argparse.Namespace) -> None:
check=True,
)
subprocess.run(["/etc/init.d/ni-wireguard-labview", "restart"], check=True)
else:
print("Dry run: would have run update-rc.d ni-wireguard-labview start 03 3 4 5 . stop 05 0 6 .")
print("Dry run: would have run /etc/init.d/ni-wireguard-labview restart")

def verify(self, args: argparse.Namespace) -> bool:
print("Verifying wireguard configuration...")
Expand Down
Loading