Merge ~ack/maas:1928098-workload-annotation-keys-format into maas:master

Proposed by Alberto Donato
Status: Merged
Approved by: Alberto Donato
Approved revision: ac3957568dc13b334b1879fe5a4ea3e7ed4e2114
Merge reported by: MAAS Lander
Merged at revision: not available
Proposed branch: ~ack/maas:1928098-workload-annotation-keys-format
Merge into: maas:master
Diff against target: 228 lines (+90/-24)
5 files modified
src/maasserver/migrations/maasserver/0240_ownerdata_key_fix.py (+42/-0)
src/maasserver/models/ownerdata.py (+15/-12)
src/maasserver/models/tests/test_ownerdata.py (+7/-0)
src/maasserver/websockets/handlers/machine.py (+4/-1)
src/maasserver/websockets/handlers/tests/test_machine.py (+22/-11)
Reviewer Review Type Date Requested Status
Björn Tillenius Approve
MAAS Lander Approve
Review via email: mp+402632@code.launchpad.net

Commit message

LP: #1928098 - only allow alphanumeric and some special chars (-, _, .) in
workload annotation names

To post a comment you must log in.
Revision history for this message
MAAS Lander (maas-lander) wrote :

UNIT TESTS
-b 1928098-workload-annotation-keys-format lp:~ack/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/10049/console
COMMIT: 6c0370c1f2819ccb2b77980267143bad6741665c

review: Needs Fixing
ac39575... by Alberto Donato

add/fix websocket tests

Revision history for this message
MAAS Lander (maas-lander) wrote :

UNIT TESTS
-b 1928098-workload-annotation-keys-format lp:~ack/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: ac3957568dc13b334b1879fe5a4ea3e7ed4e2114

review: Approve
Revision history for this message
Björn Tillenius (bjornt) wrote :

+1

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/src/maasserver/migrations/maasserver/0240_ownerdata_key_fix.py b/src/maasserver/migrations/maasserver/0240_ownerdata_key_fix.py
2new file mode 100644
3index 0000000..b2d4949
4--- /dev/null
5+++ b/src/maasserver/migrations/maasserver/0240_ownerdata_key_fix.py
6@@ -0,0 +1,42 @@
7+# Generated by Django 2.2.12 on 2021-05-11 16:22
8+
9+import re
10+
11+from django.contrib.postgres.aggregates import ArrayAgg
12+from django.db import migrations
13+
14+REPLACE_RE = re.compile(r"[^\w.-]")
15+
16+
17+def fix_ownerdata_keys(apps, schema_editor):
18+ OwnerData = apps.get_model("maasserver", "OwnerData")
19+
20+ existing_node_keys = {
21+ node_id: set(values)
22+ for node_id, values in OwnerData.objects.values_list(
23+ "node_id"
24+ ).annotate(keys=ArrayAgg("key"))
25+ }
26+
27+ for entry in OwnerData.objects.exclude(key__regex=r"^[\w.-]+$"):
28+ orig_key = entry.key
29+
30+ node_keys = existing_node_keys[entry.node_id]
31+ node_keys.remove(orig_key)
32+
33+ new_key = REPLACE_RE.sub("_", entry.key)
34+ while new_key in node_keys:
35+ new_key += "_"
36+ node_keys.add(new_key)
37+
38+ entry.key = new_key
39+ entry.save()
40+
41+
42+class Migration(migrations.Migration):
43+
44+ dependencies = [
45+ ("maasserver", "0239_add_iprange_specific_dhcp_snippets"),
46+ ]
47+
48+ operations = [migrations.RunPython(fix_ownerdata_keys)]
49diff --git a/src/maasserver/models/ownerdata.py b/src/maasserver/models/ownerdata.py
50index f81a585..5d26041 100644
51--- a/src/maasserver/models/ownerdata.py
52+++ b/src/maasserver/models/ownerdata.py
53@@ -4,6 +4,8 @@
54 """Owner key/value data placed on a machine while it is owned."""
55
56
57+import re
58+
59 from django.db.models import (
60 CASCADE,
61 CharField,
62@@ -16,6 +18,8 @@ from django.db.models import (
63 from maasserver import DefaultMeta
64 from maasserver.models.cleansave import CleanSave
65
66+DATA_KEY_RE = re.compile(r"[\w.-]+$")
67+
68
69 class OwnerDataManager(Manager):
70 def set_owner_data(self, node, owner_data):
71@@ -24,20 +28,19 @@ class OwnerDataManager(Manager):
72 This will update any keys for `node` in `owner_data`. If the key has
73 the value of None then that key will be removed from the `node`.
74 """
75- # Remove the keys set to None value.
76- keys_to_remove = {
77- key for key, value in owner_data.items() if value is None
78- }
79- if len(keys_to_remove) > 0:
80- self.delete_owner_data(node, keys_to_remove)
81-
82- # Create/update the owner data.
83+ to_remove = set()
84 for key, value in owner_data.items():
85 if value is None:
86- continue
87- self.update_or_create(
88- node=node, key=key, defaults={"value": value}
89- )
90+ to_remove.add(key)
91+ else:
92+ if not DATA_KEY_RE.match(key):
93+ raise ValueError("Invalid character in key name")
94+
95+ self.update_or_create(
96+ node=node, key=key, defaults={"value": value}
97+ )
98+ if to_remove:
99+ self.delete_owner_data(node, to_remove)
100
101 def get_owner_data(self, node):
102 # Note: ownerdata_set is prefetched, so this is more efficient than it
103diff --git a/src/maasserver/models/tests/test_ownerdata.py b/src/maasserver/models/tests/test_ownerdata.py
104index 0877e8a..d079d63 100644
105--- a/src/maasserver/models/tests/test_ownerdata.py
106+++ b/src/maasserver/models/tests/test_ownerdata.py
107@@ -25,6 +25,13 @@ class TestOwnerData(MAASServerTestCase):
108 OwnerData.objects.set_owner_data(node, owner_data)
109 self.assertEqual(owner_data, self.get_owner_data(node))
110
111+ def test_set_owner_data_invalid_name(self):
112+ node = factory.make_Node()
113+ owner_data = {"this is invalid": factory.make_name("value")}
114+ self.assertRaises(
115+ ValueError, OwnerData.objects.set_owner_data, node, owner_data
116+ )
117+
118 def test_set_owner_data_updates_data(self):
119 node = factory.make_Node()
120 owner_data = {
121diff --git a/src/maasserver/websockets/handlers/machine.py b/src/maasserver/websockets/handlers/machine.py
122index e426740..3236647 100644
123--- a/src/maasserver/websockets/handlers/machine.py
124+++ b/src/maasserver/websockets/handlers/machine.py
125@@ -1133,5 +1133,8 @@ class MachineHandler(NodeHandler):
126 key: None if value == "" else value
127 for key, value in params["workload_annotations"].items()
128 }
129- OwnerData.objects.set_owner_data(machine, owner_data)
130+ try:
131+ OwnerData.objects.set_owner_data(machine, owner_data)
132+ except ValueError as e:
133+ raise HandlerValidationError(str(e))
134 return OwnerData.objects.get_owner_data(machine)
135diff --git a/src/maasserver/websockets/handlers/tests/test_machine.py b/src/maasserver/websockets/handlers/tests/test_machine.py
136index 966dfc8..8c7493b 100644
137--- a/src/maasserver/websockets/handlers/tests/test_machine.py
138+++ b/src/maasserver/websockets/handlers/tests/test_machine.py
139@@ -1,8 +1,6 @@
140 # Copyright 2016-2020 Canonical Ltd. This software is licensed under the
141 # GNU Affero General Public License version 3 (see the file LICENSE).
142
143-"""Tests for `maasserver.websockets.handlers.node`"""
144-
145
146 from functools import partial
147 import logging
148@@ -5313,14 +5311,12 @@ class TestMachineHandlerUpdateFilesystem(MAASServerTestCase):
149 )
150
151
152-class TestMachineHandlerOwnerData(MAASServerTestCase):
153- """Tests for MachineHandle methods set_workload_annotations and get_workload_annotations."""
154-
155+class TestMachineHandlerWorkloadAnnotations(MAASServerTestCase):
156 def test_set_workload_annotations(self):
157 user = factory.make_User()
158 node = factory.make_Node(owner=user)
159 handler = MachineHandler(user, {}, None)
160- workload_annotations = {"data 1": "value 1"}
161+ workload_annotations = {"data1": "value 1"}
162 self.assertEqual(
163 workload_annotations,
164 handler.set_workload_annotations(
165@@ -5331,11 +5327,26 @@ class TestMachineHandlerOwnerData(MAASServerTestCase):
166 ),
167 )
168
169- def test_set_workload_annotations_overwrite(self):
170+ def test_set_workload_annotations_invalid_char(self):
171 user = factory.make_User()
172 node = factory.make_Node(owner=user)
173 handler = MachineHandler(user, {}, None)
174 workload_annotations = {"data 1": "value 1"}
175+ error = self.assertRaises(
176+ HandlerValidationError,
177+ handler.set_workload_annotations,
178+ {
179+ "system_id": node.system_id,
180+ "workload_annotations": workload_annotations,
181+ },
182+ )
183+ self.assertEqual(error.message, "Invalid character in key name")
184+
185+ def test_set_workload_annotations_overwrite(self):
186+ user = factory.make_User()
187+ node = factory.make_Node(owner=user)
188+ handler = MachineHandler(user, {}, None)
189+ workload_annotations = {"data1": "value 1"}
190 self.assertEqual(
191 workload_annotations,
192 handler.set_workload_annotations(
193@@ -5345,7 +5356,7 @@ class TestMachineHandlerOwnerData(MAASServerTestCase):
194 }
195 ),
196 )
197- workload_annotations = {"data 1": "value 2"}
198+ workload_annotations = {"data1": "value 2"}
199 self.assertEqual(
200 workload_annotations,
201 handler.set_workload_annotations(
202@@ -5360,7 +5371,7 @@ class TestMachineHandlerOwnerData(MAASServerTestCase):
203 user = factory.make_User()
204 node = factory.make_Node(owner=user)
205 handler = MachineHandler(user, {}, None)
206- workload_annotations = {"data 1": "value 1"}
207+ workload_annotations = {"data1": "value 1"}
208 self.assertEqual(
209 workload_annotations,
210 handler.set_workload_annotations(
211@@ -5370,7 +5381,7 @@ class TestMachineHandlerOwnerData(MAASServerTestCase):
212 }
213 ),
214 )
215- workload_annotations = {"data 1": ""}
216+ workload_annotations = {"data1": ""}
217 self.assertEqual(
218 {},
219 handler.set_workload_annotations(
220@@ -5385,7 +5396,7 @@ class TestMachineHandlerOwnerData(MAASServerTestCase):
221 user = factory.make_User()
222 node = factory.make_Node(owner=user)
223 handler = MachineHandler(user, {}, None)
224- workload_annotations = {"data 1": "value 1"}
225+ workload_annotations = {"data1": "value 1"}
226 self.assertEqual(
227 workload_annotations,
228 handler.set_workload_annotations(

Subscribers

People subscribed via source and target branches