Custom auto attach tests
This PR would benefit from the inclusion of https://github.com/canonical/cloud-init/pull/1604 in order to easily allocate Ubuntu Pro instances for integration-testing.
Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close.
If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging TheRealFalcon, and he will ensure that someone takes a look soon.
(If the pull request is closed and you would like to continue working on it, please do tag TheRealFalcon to reopen it.)
I have integrated the future UA API to execute auto-attach programmatically as we agreed. The implementation is backwards compatible if ubuntu-advantage-client's version is < v28.0, that means we could merge this PR even without the UA release.
I have executed the following test:
tox -e integration-test -- tests/integration_tests/modules/test_ubuntu_advantage.py::TestUbuntuAdvantagePro
and verified that full_auto_attach is called correctly from cloud-init.
This PR is ready to be reviewed. @blackboxsw, @CalvoM.
Thanks for the review, @blackboxsw. I have addressed your comments and refactored _should_auto_attach to except UserFacingError exceptions instead of the generic Exception.
This PR is ready to be reviewed after the build passes.
Force-pushing to retrigger CI.
Thanks for the review, @blackboxsw!
minor comment on your existing PR related to how we redact tokens in our integration logs.
- some comments on how to better log unexpected types or values for ubuntu_advantage.features values to avoid tracebacks and provide more context on cloud-init's behavior when facing unexpected values
I think I have addressed all the comments and left open the decision about the UA features' example.
While thinking about redact behavior, I think we have 2 more things I'd like us to address: 3. Make sure we redact the token value from the logged message
LOG.debug("Attaching to Ubuntu Advantage. %s", " ".join(attach_cmd))4. Make sure our invocation ofsubp.subp also redacts what command it logs by using logstringsubp.subp(attach_cmd, logstring=redacted_cmd)`
Nice idea. I'm going to create a separated PR addressing this, as it is kind of unrelated to this PR.
- please set back
additionalProperties: falseonfeaturesschema as it doesn't make sense to allow properties that we just log that we will ignore.
Done.
- Please make sure
make debworks. I think we may be hitting a pytest exception format error in trying to match handling of "Unable to importuaclient:
Thanks for pointing out this one. The problem was that my way to simulate an import error was not correct. I was removing uaclient from sys.modules and this was not enough because _should_auto_attach and _auto_attach perform the imports locally. So when those imports were reached, python tried to import again as it did not find them within sys.modules.
The reason only make deb detected the issue was that ./packages/bddeb and debuild run with the system python3 interpreter, and uaclient is available there.
I have decorated builtins.__import__ to simulate an import error, avoiding previous difficulties.
This is ready to be reviewed after CI passes, @blackboxsw.
I do think the one final review blocking this PR is that auto_attach (PRO) images still need to be able to specify 'config' settings. Can you please factor that out to a function that both auto_attach and
_attachcan separately call before performing their attach logic? Without this refactor, this represents a regression in current behavior of PRO images which can currently specifyubuntu_advantage:configsettings to configure proxy vars.If you wish to take that refactor as a separate PR that follows this one I'm fine with that. We just need to make sure we don't release until PRO can support
ubuntu_advantage.config
Per https://github.com/canonical/cloud-init/pull/1583#discussion_r957565379 I did wrongly understand there was not an alternative way to set the UA config up for full_auto_attach. Thanks, @blackboxsw, for pointing out that it is possible to set it up via cli.
I am working on it and I have found some bugs related to the config validation and processing, therefore I would suggest including it as a follow-up PR, as this one is getting considerable big.