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

fix(wsl): Special handling Landscape client config tags #5460

Merged
merged 10 commits into from
Jul 19, 2024

Conversation

CarlosNihelton
Copy link
Contributor

@CarlosNihelton CarlosNihelton commented Jun 28, 2024

Proposed Commit Message

fix(wsl): Special handling Landscape client config tags

UP4W business logic is so that its data overrides user at a key (module) 
level.
That means the entire Landscape config is overriden if both agent data
and user data contains config for that module.
Yet, for better usability, computer tags must be assignable per instance.
That's not possible with agent.yaml, because it's meant to be global.
Its config data affects all Ubuntu WSL instances.

Thus this aims to make a special case for landscape.client.tags,
if present in user provided data (either Landscape or local user -
  whatever is picked up before merging with agent.yaml)
its value overwrites any tags set by agent.yaml.

Only landscape.client.tags are treated specially.
The pre-existing merge rules still apply for any other value present in
both agent.yaml and user provided data.

Fixes UDENG-2464

Additional Context

Test Steps

Fixes UDENG-2464

Merge type

  • Squash merge using "Proposed Commit Message"
  • Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

UP4W business logic is so that its data overrides user at a key (module)
level.
That means the entire Landscape config is overriden if both agent data
and user data contains config for that module.
Yet, for better usability, computer tags must be assignable per instance.
That's not possible with agent.yaml, because it's meant to be global.
Its config data affects all Ubuntu WSL instances.

Thus this aims to make a special case for landscape.client.tags,
if present in user provided data (either Landscape or local user -
  whatever is picked up before merging with agent.yaml)
its value overwrites any tags set by agent.yaml.

Only landscape.client.tags are treated specially.
The pre-existing merge rules still apply for any other value present in
both agent.yaml and user provided data.
Seems a cosmetic "fix", but consistency is good.
`ubuntu_advantage` is deprecated, so we'd better not risk
our test's future-proofing.
@CarlosNihelton CarlosNihelton marked this pull request as ready for review June 28, 2024 15:27
@blackboxsw blackboxsw self-assigned this Jul 2, 2024
tests/unittests/sources/test_wsl.py Show resolved Hide resolved
cloudinit/sources/DataSourceWSL.py Outdated Show resolved Hide resolved
cloudinit/sources/DataSourceWSL.py Outdated Show resolved Hide resolved
cloudinit/sources/DataSourceWSL.py Outdated Show resolved Hide resolved
if user_data:
merged = user_data
user_tags = (
merged.get("landscape", {}).get("client", {}).get("tags")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Our agent data overriding user-data policy in WSL leads us down this path where we may start defining multiple exceptions to the "rule". If we end up coming up with a second case were other user-data keys are preferred over agent data keys, we may want to define a separate userdata_override_paths dict or list or a separate function that extracts a dict of userdata_overrides from the original data so we have one place that defines these expected userdata paths.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Given that every computer needs a different landscape.client.computer_title would we expect to override this with user-data too?

Copy link
Contributor Author

@CarlosNihelton CarlosNihelton Jul 19, 2024

Choose a reason for hiding this comment

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

Nope, Ubuntu Pro for WSL handles controls the computer title assignment (via the wsl-pro-service component running inside the WSL instances, but that work is transparent to cloud-init).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Great thanks!

Comment on lines 357 to 363
LOG.debug(
" Landscape client conf updated with user-data"
" landscape.client.tags: %s",
user_tags,
)
if merged.get("landscape", {}).get("client"):
merged["landscape"]["client"]["tags"] = user_tags
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's trim the leading whitespace of the log and move this debug statement into the scope of where we are actually merging landscape.client.tags.

Suggested change
LOG.debug(
" Landscape client conf updated with user-data"
" landscape.client.tags: %s",
user_tags,
)
if merged.get("landscape", {}).get("client"):
merged["landscape"]["client"]["tags"] = user_tags
if merged.get("landscape", {}).get("client"):
LOG.debug(
"Landscape client conf updated with user-data"
" landscape.client.tags: %s",
user_tags,
)
merged["landscape"]["client"]["tags"] = user_tags

"""#cloud-config
landscape:
client:
account_name: user
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is invalid JSON schema for landscape as it also requires a computer_title. maybe good to have that represented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm pushing updates with more realistic Landscape configs. Yet, we don't need to represent the computer title in the UP4W-related cases, as that piece of data is handled internally.

if user_data:
merged = user_data
user_tags = (
merged.get("landscape", {}).get("client", {}).get("tags")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given that every computer needs a different landscape.client.computer_title would we expect to override this with user-data too?

tests/unittests/sources/test_wsl.py Show resolved Hide resolved
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.

Beyond the outstanding question on whether we need user-data overrides for landscape.client.computer_name as well I ran this test on Azure Windows Server and can verify correct behavior in various permutations of agent.yaml w/ and w/out landscape.client config and user-data w/ and w/out landscape config tags.

Just minor nits on when to log that merge data line and breaking up the last unit test into two separate unit tests

Adding common fields we expect to see in real world usage.
Computer title is certainly expected on local user setups,
but in the case of UP4W it's handled internally, transparently from
cloud-init.
Splitting the longer tests in more specific test cases.
Split "missing tags" from "missing landscape.client subkey" tests.
Remove some no-longer relevant assertions.
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.

minor test nits and mypy fix being pushed Will land it after CI passes.
Thanks @CarlosNihelton!

Comment on lines 332 to 336
# We've already checked, this is just to please mypy.
assert user_data is not None
user_tags = (
user_data.get("landscape", {}).get("client", {}).get("tags")
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

heh. I saw this and originally agreed, but this is a real issue mypy has for use. at this point agent_data OR user_data could be non-None, so there is still a possibility that user_data could be None here, which would raise an AssertionError and break cloud-init. Since we are actually setting this 3 lines below after our if user_data check, let's just initialize user_tags to None here and set it if present a couple lines below.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll apply this diff and approve after CI.

    mypy: init user_tags to empty string, only set it when present in userdata

diff --git a/cloudinit/sources/DataSourceWSL.py b/cloudinit/sources/DataSourceWSL.py
index bd71cd431..7a75ff4e6 100644
--- a/cloudinit/sources/DataSourceWSL.py
+++ b/cloudinit/sources/DataSourceWSL.py
@@ -328,16 +328,12 @@ class DataSourceWSL(sources.DataSource):
         # provides them instead.
         # That's the reason for not using util.mergemanydict().
         merged: dict = {}
+        user_tags: str = ""
         overridden_keys: typing.List[str] = []
-        # We've already checked, this is just to please mypy.
-        assert user_data is not None
-        user_tags = (
-            user_data.get("landscape", {}).get("client", {}).get("tags")
-        )
         if user_data:
             merged = user_data
             user_tags = (
-                merged.get("landscape", {}).get("client", {}).get("tags")
+                merged.get("landscape", {}).get("client", {}).get("tags", "")
             )
         if agent_data:
             if user_data:

tests/unittests/sources/test_wsl.py Show resolved Hide resolved
Comment on lines +572 to +574
assert (
"tag_aiml" in userdata and "tag_dev" in userdata
), "User-data should override agent data's Landscape computer tags"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's strictly assert we don't see wsl as well from agent.yaml. Also note landscape client config schema disallows spaces between tags. so our example should be tags: x,y

Suggested change
assert (
"tag_aiml" in userdata and "tag_dev" in userdata
), "User-data should override agent data's Landscape computer tags"
assert (
"tags: tag_aiml,tag_dev" in userdata
), "User-data should override agent data's Landscape computer tags"
assert "wsl" not in userdata

@CarlosNihelton
Copy link
Contributor Author

minor test nits and mypy fix being pushed Will land it after CI passes.
Thanks @CarlosNihelton!

Thanks for the collaboration as usual!

…rdata

- tests: lanscape.client.tags schema disallows spaces between tags.
  Update tests to assert comma-delimited values and the exclusion of
  the wsl tag from agent.yaml
@blackboxsw blackboxsw merged commit 5532b4a into canonical:main Jul 19, 2024
23 checks passed
holmanb pushed a commit to holmanb/cloud-init that referenced this pull request Aug 2, 2024
)

UP4W business logic is so that its data overrides user at a key (module) 
level.
That means the entire Landscape config is overriden if both agent data
and user data contains config for that module.
Yet, for better usability, computer tags must be assignable per instance.
That's not possible with agent.yaml, because it's meant to be global.
Its config data affects all Ubuntu WSL instances.

Thus this aims to make a special case for landscape.client.tags,
if present in user provided data (either Landscape or local user -
  whatever is picked up before merging with agent.yaml)
its value overwrites any tags set by agent.yaml.

Only landscape.client.tags are treated specially.
The pre-existing merge rules still apply for any other value present in
both agent.yaml and user provided data.

Fixes UDENG-2464
holmanb pushed a commit that referenced this pull request Aug 6, 2024
UP4W business logic is so that its data overrides user at a key (module) 
level.
That means the entire Landscape config is overriden if both agent data
and user data contains config for that module.
Yet, for better usability, computer tags must be assignable per instance.
That's not possible with agent.yaml, because it's meant to be global.
Its config data affects all Ubuntu WSL instances.

Thus this aims to make a special case for landscape.client.tags,
if present in user provided data (either Landscape or local user -
  whatever is picked up before merging with agent.yaml)
its value overwrites any tags set by agent.yaml.

Only landscape.client.tags are treated specially.
The pre-existing merge rules still apply for any other value present in
both agent.yaml and user provided data.

Fixes UDENG-2464
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.

2 participants