Skip to content

Commit d5cbe46

Browse files
authored
[GCU] Add data acl table and rule check (sonic-net#3668)
ADO: 30051071 What I did Add a json patch check to avoid race condition issue when sonic consume ACL_TABLE and ACL_RULE together How I did it Add a special dataacl check for specific change where ACL_TABLE type change with ACL_RULE update How to verify it unit test
1 parent 03d8335 commit d5cbe46

File tree

2 files changed

+130
-0
lines changed

2 files changed

+130
-0
lines changed

generic_config_updater/gu_common.py

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,8 @@ def validate_field_operation(self, old_config, target_config):
181181
if any(op['op'] == operation and field == op['path'] for op in patch):
182182
raise IllegalPatchOperationError("Given patch operation is invalid. Operation: {} is illegal on field: {}".format(operation, field))
183183

184+
self.illegal_dataacl_check(old_config, target_config)
185+
184186
def _invoke_validating_function(cmd, jsonpatch_element):
185187
# cmd is in the format as <package/module name>.<method name>
186188
method_name = cmd.split(".")[-1]
@@ -212,6 +214,48 @@ def _invoke_validating_function(cmd, jsonpatch_element):
212214
if not _invoke_validating_function(function, element):
213215
raise IllegalPatchOperationError("Modification of {} table is illegal- validating function {} returned False".format(table, function))
214216

217+
def illegal_dataacl_check(self, old_config, upd_config):
218+
'''
219+
Block data ACL changes when patch includes:
220+
1. table "type" being replaced
221+
2. rule update on tables with table "type" replaced
222+
This will cause race condition when swss consume the change of
223+
acl table and acl rule and make the changed acl rule inactive
224+
'''
225+
old_acl_table = old_config.get("ACL_TABLE", {})
226+
upd_acl_table = upd_config.get("ACL_TABLE", {})
227+
228+
# Pick data acl table with "type" field
229+
old_dacl_table = [table for table, fields in old_acl_table.items()
230+
if fields.get("type") and fields["type"] != "CTRLPLANE"]
231+
upd_dacl_table = [table for table, fields in upd_acl_table.items()
232+
if fields.get("type") and fields["type"] != "CTRLPLANE"]
233+
234+
# Pick intersect common tables that "type" being replaced
235+
common_dacl_table = set(old_dacl_table).intersection(set(upd_dacl_table))
236+
# Identify tables from the intersection where the "type" field differs
237+
modified_common_dacl_table = [
238+
table for table in common_dacl_table
239+
if old_acl_table[table]["type"] != upd_acl_table[table]["type"]
240+
]
241+
242+
old_acl_rule = old_config.get("ACL_RULE", {})
243+
upd_acl_rule = upd_config.get("ACL_RULE", {})
244+
245+
# Pick rules with its dependent table which has "type" replaced
246+
old_dacl_rule = [rule for rule in old_acl_rule
247+
if rule.split("|")[0] in modified_common_dacl_table]
248+
upd_dacl_rule = [rule for rule in upd_acl_rule
249+
if rule.split("|")[0] in modified_common_dacl_table]
250+
251+
# Block changes if acl rule change on tables with "type" replaced
252+
for key in set(old_dacl_rule).union(set(upd_dacl_rule)):
253+
if (old_acl_rule.get(key, {}) != upd_acl_rule.get(key, {})):
254+
raise IllegalPatchOperationError(
255+
"Modification of dataacl rule {} is illegal: \
256+
acl table type changed in {}".format(
257+
key, modified_common_dacl_table
258+
))
215259

216260
def validate_lanes(self, config_db):
217261
if "PORT" not in config_db:

tests/generic_config_updater/field_operation_validator_test.py

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -188,6 +188,92 @@ def test_validate_field_operation_illegal__rm_loopback0(self):
188188
config_wrapper = gu_common.ConfigWrapper()
189189
self.assertRaises(gu_common.IllegalPatchOperationError, config_wrapper.validate_field_operation, old_config, target_config)
190190

191+
def test_validate_field_operation_illegal__dataacl_table_type_update_and_rule_change(self):
192+
old_config = {
193+
"ACL_TABLE": {
194+
"TEST_INGRESS": {
195+
"type": "L2"
196+
}
197+
},
198+
"ACL_RULE": {
199+
"TEST_INGRESS|RULE_1": {
200+
"ETHER_TYPE": "2048"
201+
}
202+
}
203+
}
204+
target_config = {
205+
"ACL_TABLE": {
206+
"TEST_INGRESS": {
207+
"type": "L3V6"
208+
}
209+
},
210+
"ACL_RULE": {
211+
"TEST_INGRESS|RULE_1": {
212+
"IP_TYPE": "IPV6ANY"
213+
}
214+
}
215+
}
216+
config_wrapper = gu_common.ConfigWrapper()
217+
self.assertRaises(gu_common.IllegalPatchOperationError,
218+
config_wrapper.validate_field_operation, old_config, target_config)
219+
220+
def test_validate_field_operation_legal__only_dataacl_table_type_update(self):
221+
old_config = {
222+
"ACL_TABLE": {
223+
"TEST_INGRESS": {
224+
"type": "L3"
225+
}
226+
},
227+
"ACL_RULE": {
228+
"TEST_INGRESS|RULE_1": {
229+
"IP_TYPE": "IPV6ANY"
230+
}
231+
}
232+
}
233+
target_config = {
234+
"ACL_TABLE": {
235+
"TEST_INGRESS": {
236+
"type": "L3V6"
237+
}
238+
},
239+
"ACL_RULE": {
240+
"TEST_INGRESS|RULE_1": {
241+
"IP_TYPE": "IPV6ANY"
242+
}
243+
}
244+
}
245+
config_wrapper = gu_common.ConfigWrapper()
246+
config_wrapper.validate_field_operation(old_config, target_config)
247+
248+
def test_validate_field_operation_legal__only_dataacl_rule_update(self):
249+
old_config = {
250+
"ACL_TABLE": {
251+
"TEST_INGRESS": {
252+
"type": "L3"
253+
}
254+
},
255+
"ACL_RULE": {
256+
"TEST_INGRESS|RULE_1": {
257+
"IP_TYPE": "IPV6ANY"
258+
}
259+
}
260+
}
261+
target_config = {
262+
"ACL_TABLE": {
263+
"TEST_INGRESS": {
264+
"type": "L3"
265+
}
266+
},
267+
"ACL_RULE": {
268+
"TEST_INGRESS|RULE_1": {
269+
"IP_TYPE": "IPV4ANY"
270+
}
271+
}
272+
}
273+
config_wrapper = gu_common.ConfigWrapper()
274+
config_wrapper.validate_field_operation(old_config, target_config)
275+
276+
191277
class TestGetAsicName(unittest.TestCase):
192278

193279
@patch('sonic_py_common.device_info.get_sonic_version_info')

0 commit comments

Comments
 (0)