-
Notifications
You must be signed in to change notification settings - Fork 897
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
Changes from 6 commits
a7fdbc8
e9906e8
975ffbe
e292034
5e626d7
51a4b73
51c9ea0
ced6cc5
7c1a3a5
7c95527
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -329,8 +329,16 @@ def _get_data(self) -> bool: | |||||||||||||||||||||||||||||
# That's the reason for not using util.mergemanydict(). | ||||||||||||||||||||||||||||||
merged: dict = {} | ||||||||||||||||||||||||||||||
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") | ||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great thanks! |
||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||
if agent_data: | ||||||||||||||||||||||||||||||
if user_data: | ||||||||||||||||||||||||||||||
LOG.debug("Merging both user_data and agent.yaml configs.") | ||||||||||||||||||||||||||||||
|
@@ -345,6 +353,14 @@ def _get_data(self) -> bool: | |||||||||||||||||||||||||||||
", ".join(overridden_keys) | ||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||
if user_tags: | ||||||||||||||||||||||||||||||
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 | ||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
|
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
self.userdata_raw = "#cloud-config\n%s" % yaml.dump(merged) | ||||||||||||||||||||||||||||||
return True | ||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -355,6 +355,8 @@ def test_get_data_sh(self, m_lsb_release, tmpdir, paths): | |||||||||||||||
|
||||||||||||||||
@mock.patch("cloudinit.util.get_linux_distro") | ||||||||||||||||
def test_data_precedence(self, m_get_linux_dist, tmpdir, paths): | ||||||||||||||||
"""Validates the precedence of user-data files.""" | ||||||||||||||||
|
||||||||||||||||
m_get_linux_dist.return_value = SAMPLE_LINUX_DISTRO | ||||||||||||||||
|
||||||||||||||||
# Set up basic user data: | ||||||||||||||||
|
@@ -400,9 +402,17 @@ def test_data_precedence(self, m_get_linux_dist, tmpdir, paths): | |||||||||||||||
|
||||||||||||||||
assert "" == shell_script | ||||||||||||||||
|
||||||||||||||||
# Additionally set up some UP4W agent data: | ||||||||||||||||
@mock.patch("cloudinit.util.get_linux_distro") | ||||||||||||||||
def test_interaction_with_pro(self, m_get_linux_dist, tmpdir, paths): | ||||||||||||||||
"""Validates the interaction of user-data and Pro For WSL agent data""" | ||||||||||||||||
|
||||||||||||||||
m_get_linux_dist.return_value = SAMPLE_LINUX_DISTRO | ||||||||||||||||
|
||||||||||||||||
user_file = tmpdir.join(".cloud-init", "ubuntu-24.04.user-data") | ||||||||||||||||
user_file.dirpath().mkdir() | ||||||||||||||||
user_file.write("#cloud-config\nwrite_files:\n- path: /etc/wsl.conf") | ||||||||||||||||
|
||||||||||||||||
# Now the winner should be the merge of the agent and Landscape data. | ||||||||||||||||
# The winner should be the merge of the agent and user provided data. | ||||||||||||||||
ubuntu_pro_tmp = tmpdir.join(".ubuntupro", ".cloud-init") | ||||||||||||||||
os.makedirs(ubuntu_pro_tmp, exist_ok=True) | ||||||||||||||||
|
||||||||||||||||
|
@@ -412,7 +422,8 @@ def test_data_precedence(self, m_get_linux_dist, tmpdir, paths): | |||||||||||||||
landscape: | ||||||||||||||||
client: | ||||||||||||||||
account_name: agenttest | ||||||||||||||||
ubuntu_advantage: | ||||||||||||||||
tags: wsl | ||||||||||||||||
ubuntu_pro: | ||||||||||||||||
token: testtoken""" | ||||||||||||||||
) | ||||||||||||||||
|
||||||||||||||||
|
@@ -436,17 +447,48 @@ def test_data_precedence(self, m_get_linux_dist, tmpdir, paths): | |||||||||||||||
) | ||||||||||||||||
assert "wsl.conf" in userdata | ||||||||||||||||
assert "packages" not in userdata | ||||||||||||||||
assert "ubuntu_advantage" in userdata | ||||||||||||||||
assert "ubuntu_pro" in userdata | ||||||||||||||||
assert "landscape" in userdata | ||||||||||||||||
assert "agenttest" in userdata | ||||||||||||||||
|
||||||||||||||||
# Additionally set up some Landscape provided user data | ||||||||||||||||
@mock.patch("cloudinit.util.get_linux_distro") | ||||||||||||||||
def test_landscape_provided_data(self, m_get_linux_dist, tmpdir, paths): | ||||||||||||||||
"""Validates the interaction of Pro For WSL agent and Landscape data""" | ||||||||||||||||
|
||||||||||||||||
m_get_linux_dist.return_value = SAMPLE_LINUX_DISTRO | ||||||||||||||||
|
||||||||||||||||
user_file = tmpdir.join(".cloud-init", "ubuntu-24.04.user-data") | ||||||||||||||||
user_file.dirpath().mkdir() | ||||||||||||||||
user_file.write( | ||||||||||||||||
"""#cloud-config | ||||||||||||||||
landscape: | ||||||||||||||||
client: | ||||||||||||||||
account_name: user | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||||||||||||||||
tags: usertags | ||||||||||||||||
package_update: true""" | ||||||||||||||||
) | ||||||||||||||||
|
||||||||||||||||
ubuntu_pro_tmp = tmpdir.join(".ubuntupro", ".cloud-init") | ||||||||||||||||
os.makedirs(ubuntu_pro_tmp, exist_ok=True) | ||||||||||||||||
|
||||||||||||||||
agent_file = ubuntu_pro_tmp.join("agent.yaml") | ||||||||||||||||
agent_file.write( | ||||||||||||||||
"""#cloud-config | ||||||||||||||||
landscape: | ||||||||||||||||
client: | ||||||||||||||||
account_name: agenttest | ||||||||||||||||
tags: wsl | ||||||||||||||||
ubuntu_pro: | ||||||||||||||||
token: testtoken""" | ||||||||||||||||
) | ||||||||||||||||
|
||||||||||||||||
landscape_file = ubuntu_pro_tmp.join("%s.user-data" % INSTANCE_NAME) | ||||||||||||||||
landscape_file.write( | ||||||||||||||||
"""#cloud-config | ||||||||||||||||
landscape: | ||||||||||||||||
client: | ||||||||||||||||
account_name: landscapetest | ||||||||||||||||
tags: tag_aiml, tag_dev | ||||||||||||||||
package_update: true""" | ||||||||||||||||
) | ||||||||||||||||
|
||||||||||||||||
|
@@ -471,7 +513,7 @@ def test_data_precedence(self, m_get_linux_dist, tmpdir, paths): | |||||||||||||||
|
||||||||||||||||
assert "wsl.conf" not in userdata | ||||||||||||||||
assert "packages" not in userdata | ||||||||||||||||
assert "ubuntu_advantage" in userdata | ||||||||||||||||
assert "ubuntu_pro" in userdata | ||||||||||||||||
assert "package_update" in userdata, ( | ||||||||||||||||
"package_update entry should not be overriden by agent data" | ||||||||||||||||
" nor ignored" | ||||||||||||||||
|
@@ -480,3 +522,152 @@ def test_data_precedence(self, m_get_linux_dist, tmpdir, paths): | |||||||||||||||
assert ( | ||||||||||||||||
"landscapetest" not in userdata and "agenttest" in userdata | ||||||||||||||||
), "Landscape account name should have been overriden by agent data" | ||||||||||||||||
# Make sure we have tags from Landscape data, not agent's | ||||||||||||||||
assert ( | ||||||||||||||||
"tag_aiml" in userdata and "tag_dev" in userdata | ||||||||||||||||
), "User-data should override agent data's Landscape computer tags" | ||||||||||||||||
Comment on lines
+572
to
+574
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's strictly assert we don't see
Suggested change
|
||||||||||||||||
# and also not from the user provided data. | ||||||||||||||||
assert ( | ||||||||||||||||
"usertags" not in userdata | ||||||||||||||||
), "User provided data should have been overriden" | ||||||||||||||||
|
||||||||||||||||
@mock.patch("cloudinit.util.get_linux_distro") | ||||||||||||||||
def test_with_landscape_no_tags(self, m_get_linux_dist, tmpdir, paths): | ||||||||||||||||
"""Validates the Pro For WSL default Landscape tags are applied""" | ||||||||||||||||
|
||||||||||||||||
m_get_linux_dist.return_value = SAMPLE_LINUX_DISTRO | ||||||||||||||||
|
||||||||||||||||
ubuntu_pro_tmp = tmpdir.join(".ubuntupro", ".cloud-init") | ||||||||||||||||
os.makedirs(ubuntu_pro_tmp, exist_ok=True) | ||||||||||||||||
|
||||||||||||||||
agent_file = ubuntu_pro_tmp.join("agent.yaml") | ||||||||||||||||
agent_file.write( | ||||||||||||||||
"""#cloud-config | ||||||||||||||||
landscape: | ||||||||||||||||
client: | ||||||||||||||||
account_name: agenttest | ||||||||||||||||
tags: wsl | ||||||||||||||||
ubuntu_pro: | ||||||||||||||||
token: testtoken""" | ||||||||||||||||
) | ||||||||||||||||
# Set up some Landscape provided user data without tags | ||||||||||||||||
blackboxsw marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||
landscape_file = ubuntu_pro_tmp.join("%s.user-data" % INSTANCE_NAME) | ||||||||||||||||
landscape_file.write( | ||||||||||||||||
"""#cloud-config | ||||||||||||||||
landscape: | ||||||||||||||||
client: | ||||||||||||||||
account_name: landscapetest | ||||||||||||||||
package_update: true""" | ||||||||||||||||
) | ||||||||||||||||
|
||||||||||||||||
# Run the datasource | ||||||||||||||||
ds = wsl.DataSourceWSL( | ||||||||||||||||
sys_cfg=SAMPLE_CFG, | ||||||||||||||||
distro=_get_distro("ubuntu"), | ||||||||||||||||
paths=paths, | ||||||||||||||||
) | ||||||||||||||||
|
||||||||||||||||
assert ds.get_data() is True | ||||||||||||||||
ud = ds.get_userdata() | ||||||||||||||||
|
||||||||||||||||
assert ud is not None | ||||||||||||||||
userdata = cast( | ||||||||||||||||
str, | ||||||||||||||||
join_payloads_from_content_type( | ||||||||||||||||
cast(MIMEMultipart, ud), "text/cloud-config" | ||||||||||||||||
), | ||||||||||||||||
) | ||||||||||||||||
|
||||||||||||||||
assert "landscape" in userdata | ||||||||||||||||
assert ( | ||||||||||||||||
"landscapetest" not in userdata and "agenttest" in userdata | ||||||||||||||||
), "Landscape account name should have been overriden by agent data" | ||||||||||||||||
assert ( | ||||||||||||||||
"tags: wsl" in userdata | ||||||||||||||||
), "Landscape computer tags should match UP4W agent's data defaults" | ||||||||||||||||
|
||||||||||||||||
@mock.patch("cloudinit.util.get_linux_distro") | ||||||||||||||||
def test_with_no_tags_at_all(self, m_get_linux_dist, tmpdir, paths): | ||||||||||||||||
"""Asserts the DS still works if there are no Landscape tags at all""" | ||||||||||||||||
|
||||||||||||||||
m_get_linux_dist.return_value = SAMPLE_LINUX_DISTRO | ||||||||||||||||
|
||||||||||||||||
user_file = tmpdir.join(".cloud-init", "ubuntu-24.04.user-data") | ||||||||||||||||
user_file.dirpath().mkdir() | ||||||||||||||||
user_file.write("#cloud-config\nwrite_files:\n- path: /etc/wsl.conf") | ||||||||||||||||
|
||||||||||||||||
ubuntu_pro_tmp = tmpdir.join(".ubuntupro", ".cloud-init") | ||||||||||||||||
os.makedirs(ubuntu_pro_tmp, exist_ok=True) | ||||||||||||||||
|
||||||||||||||||
agent_file = ubuntu_pro_tmp.join("agent.yaml") | ||||||||||||||||
# Make sure we don't crash if there are no tags anywhere. | ||||||||||||||||
agent_file.write( | ||||||||||||||||
"""#cloud-config | ||||||||||||||||
ubuntu_pro: | ||||||||||||||||
token: up4w_token""" | ||||||||||||||||
) | ||||||||||||||||
# Set up some Landscape provided user data without tags | ||||||||||||||||
landscape_file = ubuntu_pro_tmp.join("%s.user-data" % INSTANCE_NAME) | ||||||||||||||||
landscape_file.write( | ||||||||||||||||
"""#cloud-config | ||||||||||||||||
landscape: | ||||||||||||||||
client: | ||||||||||||||||
account_name: landscapetest | ||||||||||||||||
package_update: true""" | ||||||||||||||||
) | ||||||||||||||||
|
||||||||||||||||
# Run the datasource | ||||||||||||||||
ds = wsl.DataSourceWSL( | ||||||||||||||||
sys_cfg=SAMPLE_CFG, | ||||||||||||||||
distro=_get_distro("ubuntu"), | ||||||||||||||||
paths=paths, | ||||||||||||||||
) | ||||||||||||||||
|
||||||||||||||||
assert ds.get_data() is True | ||||||||||||||||
ud = ds.get_userdata() | ||||||||||||||||
|
||||||||||||||||
assert ud is not None | ||||||||||||||||
userdata = cast( | ||||||||||||||||
str, | ||||||||||||||||
join_payloads_from_content_type( | ||||||||||||||||
cast(MIMEMultipart, ud), "text/cloud-config" | ||||||||||||||||
), | ||||||||||||||||
) | ||||||||||||||||
assert "landscapetest" in userdata | ||||||||||||||||
assert "up4w_token" in userdata | ||||||||||||||||
|
||||||||||||||||
# Make sure we don't crash if there is no client subkey. | ||||||||||||||||
# (That would be a bug in the agent as there is no other config | ||||||||||||||||
# value for landscape outside of landscape.client, so I'm making up | ||||||||||||||||
# some non-sense keys just to make sure we won't crash) | ||||||||||||||||
agent_file.write( | ||||||||||||||||
"""#cloud-config | ||||||||||||||||
landscape: | ||||||||||||||||
server: | ||||||||||||||||
port: 6554 | ||||||||||||||||
ubuntu_pro: | ||||||||||||||||
token: up4w_token""" | ||||||||||||||||
) | ||||||||||||||||
# Run the datasource | ||||||||||||||||
ds = wsl.DataSourceWSL( | ||||||||||||||||
sys_cfg=SAMPLE_CFG, | ||||||||||||||||
distro=_get_distro("ubuntu"), | ||||||||||||||||
paths=paths, | ||||||||||||||||
) | ||||||||||||||||
|
||||||||||||||||
assert ds.get_data() is True | ||||||||||||||||
ud = ds.get_userdata() | ||||||||||||||||
|
||||||||||||||||
assert ud is not None | ||||||||||||||||
userdata = cast( | ||||||||||||||||
str, | ||||||||||||||||
join_payloads_from_content_type( | ||||||||||||||||
cast(MIMEMultipart, ud), "text/cloud-config" | ||||||||||||||||
), | ||||||||||||||||
) | ||||||||||||||||
assert "landscapetest" not in userdata | ||||||||||||||||
assert ( | ||||||||||||||||
"port: 6554" in userdata | ||||||||||||||||
), "agent data should override the entire landscape config." | ||||||||||||||||
|
||||||||||||||||
assert "up4w_token" in userdata |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.