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

cc_ubuntu_advantage: Implement custom auto-attach behaviors #1583

Merged
merged 24 commits into from Sep 16, 2022

Conversation

aciba90
Copy link
Contributor

@aciba90 aciba90 commented Jul 12, 2022

Proposed Commit Message

cc_ubuntu_advantage: Implement custom auto-attach behaviors

Previously, ua auto-attach was performed in Ubuntu Pro images
transparently for cloud-init. Offering no configurability options
for cloud-init users.

Extend the `ubuntu_advantage` cloud-cfg section to include
`features.disable_auto_attach` and `enable_beta`.

Integrate new UA apis to detect pro instances and perform 
auto-attach if ua config is given.

The client can configure what non-beta and beta services will be enabled.

This config can be provided via:
- user-data allowing customers to configure auto-attach.
- A conf file under /etc/cloud/cloud.cfg.d allowing resellers to create 
ua-preconfigured golden images.

In the case that the ubuntu-advantage-client version is older than
the one containing the required apis, cc_ubuntu_advantage will fallback
to normal attach.

Additional Context

SC-1145
SC-1149
canonical/ubuntu-pro-client#2166
canonical/ubuntu-pro-client#2171

Test Steps

# Run it in gce, azure or ec2 (only clouds providing Pro images)
tox -e integration-test -- tests/integration_tests/modules/test_ubuntu_advantage.py::TestUbuntuAdvantagePro

Checklist:

  • My code follows the process laid out in the documentation
  • I have updated or added any unit tests accordingly
  • I have updated or added any documentation accordingly

@blackboxsw blackboxsw self-assigned this Jul 12, 2022
@blackboxsw blackboxsw requested a review from CalvoM July 12, 2022 15:36
Copy link
Collaborator

@blackboxsw blackboxsw left a comment

Choose a reason for hiding this comment

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

Thanks for pulling a sample PR together for discussion @aciba90 I think we need a bit of UA-dev team input on these new API functions/signature and behavior to make progress on this.

cloudinit/config/cc_ubuntu_advantage.py Outdated Show resolved Hide resolved
cloudinit/config/cc_ubuntu_advantage.py Outdated Show resolved Hide resolved
cloudinit/config/cc_ubuntu_advantage.py Outdated Show resolved Hide resolved
cloudinit/config/cc_ubuntu_advantage.py Outdated Show resolved Hide resolved
@aciba90
Copy link
Contributor Author

aciba90 commented Jul 29, 2022

This PR would benefit from the inclusion of #1604 in order to easily allocate Ubuntu Pro instances for integration-testing.

@aciba90 aciba90 force-pushed the custom_auto_attach_tests branch 5 times, most recently from 8d082aa to c7d411b Compare August 3, 2022 15:40
@github-actions
Copy link

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.)

@github-actions github-actions bot added the stale-pr Pull request is stale; will be auto-closed soon label Aug 26, 2022
@aciba90 aciba90 changed the title Custom auto attach tests cc_ubuntu_advantage: Implement custom auto-attach behaviors Aug 26, 2022
@aciba90 aciba90 force-pushed the custom_auto_attach_tests branch 2 times, most recently from 0b521e2 to 63b1b93 Compare August 29, 2022 16:39
},
"config": {
"type": "object",
"description": "Configuration settings or override Ubuntu Advantage config",
"description": "Configuration settings or override Ubuntu Advantage config. Only used in non Ubuntu Pro instances.",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

During the implementation of canonical/ubuntu-pro-client#2171, it was possible to pass an UAConfig instance (containing this config) to full_auto_attach. But this did not get to the merged commit. Is there an alternative way to pass the config? Or is it not necessary to do so in Pro instances? @CalvoM

Copy link
Collaborator

Choose a reason for hiding this comment

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

We have a way of passing the UAConfig to the endpoint via _full_auto_attach. However, we recommend that you use the endpoint not requiring the UAConfig parameter, since we create the object internally. You can use the other endpoint, when you have some custom configuration for UAConfig default object.

@aciba90 aciba90 force-pushed the custom_auto_attach_tests branch 2 times, most recently from d39c5d7 to da62b7c Compare August 29, 2022 16:57
@aciba90 aciba90 marked this pull request as ready for review August 29, 2022 16:58
@aciba90
Copy link
Contributor Author

aciba90 commented Aug 29, 2022

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.

@aciba90 aciba90 requested review from blackboxsw and CalvoM and removed request for blackboxsw and CalvoM August 29, 2022 17:09
configure_ua(
token=token,
enable=ua_section.get("enable"),
config=ua_section.get("config"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

@aciba90 I just realized one big gap in this implementation is that we are only invoking ua config *http_proxy* values on non-PRO instances. I think we actually need this feature to override proxy config for both PRO and non-PRO instances, so this may involve a bit of a refactor to ensure both PRO instances and can still manipulate proxy values prior to auto-attach of plain attach.

Copy link
Collaborator

@blackboxsw blackboxsw left a comment

Choose a reason for hiding this comment

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

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 _attach can separately call before performing their attach logic? Without this refactor, this represents a regression in current behavior of PRO images which can currently specify ubuntu_advantage:config settings 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

@aciba90
Copy link
Contributor Author

aciba90 commented Sep 16, 2022

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 _attach can separately call before performing their attach logic? Without this refactor, this represents a regression in current behavior of PRO images which can currently specify ubuntu_advantage:config settings 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 #1583 (comment) 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.

Copy link
Collaborator

@blackboxsw blackboxsw left a comment

Choose a reason for hiding this comment

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

Thanks @aciba90 let's then land this and iterate on the config handling before pushing a release into kinetic.

@blackboxsw blackboxsw merged commit 4b12fe2 into canonical:main Sep 16, 2022
@aciba90 aciba90 deleted the custom_auto_attach_tests branch September 19, 2022 09:03
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.

None yet

3 participants