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

Add a module to update the FreeBSD base system #54924

Open
wants to merge 24 commits into
base: devel
from

Conversation

Projects
None yet
4 participants
@part1zano
Copy link

commented Apr 5, 2019

SUMMARY

Adds support for the freebsd-update utility.
Fixes PR #41976

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

freebsd_update

ADDITIONAL INFORMATION

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

The test ansible-test sanity --test pylint [explain] failed with 1 error:

lib/ansible/modules/system/freebsd_update.py:94:14: ansible-format-automatic-specification Format string contains automatic field numbering specification

The test ansible-test sanity --test ansible-doc --python 2.6 [explain] failed with 1 error:

lib/ansible/modules/system/freebsd_update.py:0:0: missing documentation (or could not parse documentation): expected string or buffer

The test ansible-test sanity --test ansible-doc --python 2.7 [explain] failed with 1 error:

lib/ansible/modules/system/freebsd_update.py:0:0: missing documentation (or could not parse documentation): expected string or buffer

The test ansible-test sanity --test ansible-doc --python 3.5 [explain] failed with 1 error:

lib/ansible/modules/system/freebsd_update.py:0:0: missing documentation (or could not parse documentation): expected string or bytes-like object

The test ansible-test sanity --test ansible-doc --python 3.6 [explain] failed with 1 error:

lib/ansible/modules/system/freebsd_update.py:0:0: missing documentation (or could not parse documentation): expected string or bytes-like object

The test ansible-test sanity --test ansible-doc --python 3.7 [explain] failed with 1 error:

lib/ansible/modules/system/freebsd_update.py:0:0: missing documentation (or could not parse documentation): expected string or bytes-like object

The test ansible-test sanity --test ansible-doc --python 3.8 [explain] failed with 1 error:

lib/ansible/modules/system/freebsd_update.py:0:0: missing documentation (or could not parse documentation): expected string or bytes-like object

The test ansible-test sanity --test validate-modules [explain] failed with 4 errors:

lib/ansible/modules/system/freebsd_update.py:0:0: E305 DOCUMENTATION.options.action.description.0: expected str @ data['options']['action']['description'][0]. Got {'freebsd-update command to execute': 'fetch, install, rollback, IDS, cron.'}
lib/ansible/modules/system/freebsd_update.py:0:0: E307 version_added should be '2.8'. Currently '2.7'
lib/ansible/modules/system/freebsd_update.py:0:0: E312 No RETURN provided
lib/ansible/modules/system/freebsd_update.py:0:0: E324 Argument 'basedir' in argument_spec defines default as (None) but documentation defines default as ('/')

click here for bot help

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

The test ansible-test sanity --test ansible-doc --python 2.6 [explain] failed with 1 error:

lib/ansible/modules/system/freebsd_update.py:0:0: missing documentation (or could not parse documentation): expected string or buffer

The test ansible-test sanity --test ansible-doc --python 2.7 [explain] failed with 1 error:

lib/ansible/modules/system/freebsd_update.py:0:0: missing documentation (or could not parse documentation): expected string or buffer

The test ansible-test sanity --test ansible-doc --python 3.5 [explain] failed with 1 error:

lib/ansible/modules/system/freebsd_update.py:0:0: missing documentation (or could not parse documentation): expected string or bytes-like object

The test ansible-test sanity --test ansible-doc --python 3.6 [explain] failed with 1 error:

lib/ansible/modules/system/freebsd_update.py:0:0: missing documentation (or could not parse documentation): expected string or bytes-like object

The test ansible-test sanity --test ansible-doc --python 3.7 [explain] failed with 1 error:

lib/ansible/modules/system/freebsd_update.py:0:0: missing documentation (or could not parse documentation): expected string or bytes-like object

The test ansible-test sanity --test ansible-doc --python 3.8 [explain] failed with 1 error:

lib/ansible/modules/system/freebsd_update.py:0:0: missing documentation (or could not parse documentation): expected string or bytes-like object

The test ansible-test sanity --test validate-modules [explain] failed with 7 errors:

lib/ansible/modules/system/freebsd_update.py:0:0: E305 DOCUMENTATION.options.action.description.0: expected str @ data['options']['action']['description'][0]. Got {'freebsd-update command to execute': 'fetch, install, rollback, IDS, cron.'}
lib/ansible/modules/system/freebsd_update.py:0:0: E312 No RETURN provided
lib/ansible/modules/system/freebsd_update.py:0:0: E324 Argument 'basedir' in argument_spec defines default as (None) but documentation defines default as ('None')
lib/ansible/modules/system/freebsd_update.py:0:0: E324 Argument 'conffile' in argument_spec defines default as (None) but documentation defines default as ('None')
lib/ansible/modules/system/freebsd_update.py:0:0: E324 Argument 'key' in argument_spec defines default as (None) but documentation defines default as ('None')
lib/ansible/modules/system/freebsd_update.py:0:0: E324 Argument 'server' in argument_spec defines default as (None) but documentation defines default as ('None')
lib/ansible/modules/system/freebsd_update.py:0:0: E324 Argument 'workdir' in argument_spec defines default as (None) but documentation defines default as ('None')

click here for bot help

@ansibot ansibot removed the ci_verified label Apr 5, 2019

@mattclay

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

@part1zano What environment variables are needed that are missing?

@part1zano

This comment has been minimized.

Copy link
Author

commented Apr 6, 2019

@mattclay I haven't figured that out yet

@part1zano

This comment has been minimized.

Copy link
Author

commented Apr 6, 2019

Now it only fails with FreeBSD 11.1. I don't see why.

@part1zano

This comment has been minimized.

Copy link
Author

commented Apr 6, 2019

My assumption is, if the system is unsupported, the update "fails", i.e. has a non-zero status.
Here's what's inside that util, btw:

	# If the EoL time is past, warn.
	if [ ${EOLTIME} -lt ${NOWTIME} ]; then
		echo
		cat <<-EOF
		WARNING: `uname -sr` HAS PASSED ITS END-OF-LIFE DATE.
		Any security issues discovered after `date -r ${EOLTIME}`
		will not have been corrected.
		EOF
		return 1
	fi

As one can observe, it does return 1 if the system reaches EoL. Let me fix that quickly.

@part1zano

This comment has been minimized.

Copy link
Author

commented Apr 6, 2019

Now it fetches the update again even though it has just been fetched.

part1zano added some commits Apr 6, 2019

Revert "skip FreeBSD 11.1"
This reverts commit 4d47a61.

part1zano added some commits Apr 6, 2019

@part1zano

This comment has been minimized.

Copy link
Author

commented Apr 6, 2019

I skipped FreeBSD 11.1 correctly, the tests are about to go green.

@@ -0,0 +1,2 @@
- include_tasks: tests.yml
when: (ansible_distribution == 'FreeBSD') and (ansible_distribution_version != '11.1')

This comment has been minimized.

Copy link
@mattclay

mattclay Apr 6, 2019

Member

Why skip FreeBSD 11.1? It's EOL, but that's a scenario the module handles and should be tested for.

The test should be able to detect if the version being tested is EOL and act accordingly when verifying results.

This comment has been minimized.

Copy link
@part1zano

part1zano Apr 6, 2019

Author

Yes, and no. Updating the base system to a newer release hasn't been implemented. Currently, the module only updates a release to its latest patch level.

This comment has been minimized.

Copy link
@part1zano

part1zano Apr 6, 2019

Author

You might have noticed I skipped the upgrade command. To be fair, it's not an easy process, and I'm afraid it's not always automatable.

This comment has been minimized.

Copy link
@mattclay

mattclay Apr 9, 2019

Member

No need to test upgrade, just test update even if the current version is EOL.

@gundalow gundalow added the bsd label Apr 7, 2019

def main():
module = AnsibleModule(
argument_spec=dict(
action=dict(type='str', required=True, choices=['fetch', 'install', 'fetch_install', 'rollback', 'IDS', 'cron']),

This comment has been minimized.

Copy link
@mattclay

mattclay Apr 9, 2019

Member

I don't think the IDS command makes sense for this module. It doesn't make changes, so there's no work for the module to do to provide idempotency. The user would get the same benefit by simply using the following in their playbook:

  - command: freebsd-update IDS --not-running-from-cron

This comment has been minimized.

Copy link
@mattclay

mattclay Apr 9, 2019

Member

I also wouldn't include the cron command. The delay and email behavior can be handled as ansible tasks. Due to the large random delay it's not practical to include in integration tests either.

results[dbtree]['stdout_lines'] = out.split('\n')
results[dbtree]['command'] = cmd
results['changed'] = True
if 'No updates' in out:

This comment has been minimized.

Copy link
@mattclay

mattclay Apr 9, 2019

Member

This doesn't handle rollbacks with no changes. The output I get is No rollback directory found.

Be sure to include tests for rollback (and idempotency).

This comment has been minimized.

Copy link
@mattclay

mattclay Apr 9, 2019

Member

The fetch command will always report changed as well. You could detect Applying patches to determine if new files were downloaded for reporting changes.

Tests should check for idempotency for this as well.

key = module.params.get('key')

freebsd_update_bin = module.get_bin_path('freebsd-update', True)
cmd = [freebsd_update_bin]

This comment has been minimized.

Copy link
@mattclay

mattclay Apr 9, 2019

Member

Include '--not-running-from-cron' here:

Suggested change
cmd = [freebsd_update_bin]
cmd = [freebsd_update_bin, '--not-running-from-cron']
results = {dbtree: {}}

if (rc != 0) and ('HAS PASSED ITS END-OF-LIFE' in out):
rc = 0 # once FreeBSD reaches EoL, `freebsd-update` exits with status 1 on update.

This comment has been minimized.

Copy link
@mattclay

mattclay Apr 9, 2019

Member

It might be useful to include an EOL indicator in the module results when fetch or fetch_install is used, since we have that information here.


if rc == 0:
results[dbtree]['stdout'] = out
results[dbtree]['stdout_lines'] = out.split('\n')

This comment has been minimized.

Copy link
@mattclay

mattclay Apr 9, 2019

Member

Ansible will include stdout_lines so the module doesn't need to.

else:
msg = ' '.join(cmd) + ' failed'

module.fail_json(msg=msg, stderr=err, stdout=out)

This comment has been minimized.

Copy link
@mattclay

mattclay Apr 9, 2019

Member

Include rc in the failure and separate the message from the command:

Suggested change
module.fail_json(msg=msg, stderr=err, stdout=out)
result = dict(
cmd=cmd,
rc=rc,
stdout=out,
stderr=err,
)
module.fail_json(msg='non-zero return code', **result)
except Exception as e:
module.fail_json(msg=to_native(e), exception=traceback.format_exc())

dbtree = 'freebsd-update'

This comment has been minimized.

Copy link
@mattclay

mattclay Apr 9, 2019

Member

Use freebsd_update instead, so it follows Python naming conventions.

results[dbtree]['command'] = cmd
results['changed'] = True
if 'No updates' in out:
results['changed'] = False

This comment has been minimized.

Copy link
@mattclay

mattclay Apr 9, 2019

Member

The setting of results can be cleaned up and consolidated. Something like:

changed = True

if 'No updates' in out:
  changed = False

results = dict(
    freebsd_update=dict(
        cmd=cmd,
        stdout=out,
        changed=changed,
    ),
)

The above example also uses cmd instead of command for consistency with the failure scenario.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.