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.

Sign up for GitHub

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

Jump to bottom

[GCU] Marking fields under BGP_PEER_RANGE, BGP_MONITORS as create-only #2092

Merged
merged 2 commits into from
Mar 9, 2022
Merged

[GCU] Marking fields under BGP_PEER_RANGE, BGP_MONITORS as create-only #2092

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
74f146f
[GCU] Marking fields under ACL_RULE, BGP_PEER_RANGE, BGP_MONITORS as …
ghooo Mar 4, 2022
5c3e94d
reverting ACL_RULE
ghooo Mar 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions generic_config_updater/patch_sorter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -581,6 +581,15 @@ def __init__(self, path_addressing):
["BGP_NEIGHBOR", "*", "local_addr"],
["BGP_NEIGHBOR", "*", "nhopself"],
["BGP_NEIGHBOR", "*", "rrclient"],
["ACL_RULE", "*", "*"],
Copy link
Contributor Author

@ghooo ghooo Mar 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@wen587 Can you please check if all the fields under ACL_RULE and BGP_PEER_RANGE need to be marked as create-only?

This can be verified by modifying each field one by one and see if the change is reflected correctly.

What is a field?
We can think of sonic configs as such

{
  "TABLE": {
    "KEY": {
      "FIELD": { ... }
    }
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wen587 Can you please also check if the this is enough to solve the CACL problem, we can try to 2 rules. See if they will get added correctly.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ghooo Checked BGP_PEER_RANGE, all fields are created only.
The ACL_RULE is not create only. We want to make it create only, because the CACL test will check the ip tables immediately after the change. But too many ops in ACL_RULE will cause 0.5 update delay. That's why the cacl test has random failure issue.

Copy link
Contributor Author

@ghooo ghooo Mar 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@wen587 but what if we made it create-only then the operation itself is adding multiple ACL rules. Each ACL rule might get added in a single call. Will that break CACL?

Copy link
Contributor Author

@ghooo ghooo Mar 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

ACL_RULE fields are not create-only

admin@vlab-01:~$ show acl rule
Table    Rule    Priority    Action    Match
-------  ------  ----------  --------  -------
admin@vlab-01:~$ sudo config apply-patch add-acl-rule.json -i /FEATURE -i /QUEUE -i /SCHEDULER -i ''
...
Patch Applier: Applying 1 change in order:
Patch Applier:   * [{"op": "add", "path": "/ACL_RULE", "value": {"SSH_ONLY|TEST_DROP": {"L4_DST_PORT": "22", "IP_PROTOCOL": "6", "IP_TYPE": "IP", "PACKET_ACTION": "DROP", "PRIORITY": "9998", "SRC_IP": "9.9.9.9/32"}}}]
Patch Applier: Verifying patch updates are reflected on ConfigDB.
Patch Applier: Patch application completed.
Patch applied successfully.
admin@vlab-01:~$ show acl rule
Table     Rule       Priority    Action    Match
--------  ---------  ----------  --------  ------------------
SSH_ONLY  TEST_DROP  9998        DROP      IP_PROTOCOL: 6
                                           IP_TYPE: IP
                                           L4_DST_PORT: 22
                                           SRC_IP: 9.9.9.9/32
admin@vlab-01:~$ sudo config apply-patch add-acl-rule.json -i /FEATURE -i /QUEUE -i /SCHEDULER -i ''
...
Patch Applier: Applying 1 change in order:
Patch Applier:   * [{"op": "replace", "path": "/ACL_RULE/SSH_ONLY|TEST_DROP/PRIORITY", "value": "9997"}]
Patch Applier: Verifying patch updates are reflected on ConfigDB.
Patch Applier: Patch application completed.
Patch applied successfully.
admin@vlab-01:~$ show acl rule
Table     Rule       Priority    Action    Match
--------  ---------  ----------  --------  ------------------
SSH_ONLY  TEST_DROP  9997        DROP      IP_PROTOCOL: 6
                                           IP_TYPE: IP
                                           L4_DST_PORT: 22
                                           SRC_IP: 9.9.9.9/32
admin@vlab-01:~$ sudo config apply-patch add-acl-rule.json -i /FEATURE -i /QUEUE -i /SCHEDULER -i ''
...
Patch Applier: Applying 1 change in order:
Patch Applier:   * [{"op": "replace", "path": "/ACL_RULE/SSH_ONLY|TEST_DROP/SRC_IP", "value": "9.9.9.10/32"}]
Patch Applier: Verifying patch updates are reflected on ConfigDB.
Patch Applier: Patch application completed.
Patch applied successfully.
admin@vlab-01:~$ show acl rule
Table     Rule       Priority    Action    Match
--------  ---------  ----------  --------  -------------------
SSH_ONLY  TEST_DROP  9997        DROP      IP_PROTOCOL: 6
                                           IP_TYPE: IP
                                           L4_DST_PORT: 22
                                           SRC_IP: 9.9.9.10/32
admin@vlab-01:~$ sudo config apply-patch add-acl-rule.json -i /FEATURE -i /QUEUE -i /SCHEDULER -i ''
...
Patch Applier: Applying 1 change in order:
Patch Applier:   * [{"op": "replace", "path": "/ACL_RULE/SSH_ONLY|TEST_DROP/IP_PROTOCOL", "value": "7"}]
Patch Applier: Verifying patch updates are reflected on ConfigDB.
Patch Applier: Patch application completed.
Patch applied successfully.
admin@vlab-01:~$ show acl rule
Table     Rule       Priority    Action    Match
--------  ---------  ----------  --------  -------------------
SSH_ONLY  TEST_DROP  9997        DROP      IP_PROTOCOL: 7
                                           IP_TYPE: IP
                                           L4_DST_PORT: 22
                                           SRC_IP: 9.9.9.10/32
admin@vlab-01:~$ sudo config apply-patch add-acl-rule.json -i /FEATURE -i /QUEUE -i /SCHEDULER -i ''
...
Patch Applier: Applying 1 change in order:
Patch Applier:   * [{"op": "replace", "path": "/ACL_RULE/SSH_ONLY|TEST_DROP/L4_DST_PORT", "value": "88"}]
Patch Applier: Verifying patch updates are reflected on ConfigDB.
Patch Applier: Patch application completed.
Patch applied successfully.
admin@vlab-01:~$ show acl rule
Table     Rule       Priority    Action    Match
--------  ---------  ----------  --------  -------------------
SSH_ONLY  TEST_DROP  9997        DROP      IP_PROTOCOL: 7
                                           IP_TYPE: IP
                                           L4_DST_PORT: 88
                                           SRC_IP: 9.9.9.10/32
admin@vlab-01:~$ sudo config apply-patch add-acl-rule.json -i /FEATURE -i /QUEUE -i /SCHEDULER 
...
Patch Applier: Applying 1 change in order:
Patch Applier:   * [{"op": "replace", "path": "/ACL_RULE/SSH_ONLY|TEST_DROP/IP_TYPE", "value": "ANY"}]
Patch Applier: Verifying patch updates are reflected on ConfigDB.
Patch Applier: Patch application completed.
Patch applied successfully.
admin@vlab-01:~$ show acl rule
Table     Rule       Priority    Action    Match
--------  ---------  ----------  --------  -------------------
SSH_ONLY  TEST_DROP  9997        DROP      IP_PROTOCOL: 7
                                           IP_TYPE: ANY
                                           L4_DST_PORT: 88
                                           SRC_IP: 9.9.9.10/32
admin@vlab-01:~$ sudo config apply-patch add-acl-rule.json -i /FEATURE -i /QUEUE -i /SCHEDULER 
...
Patch Applier: Applying 1 change in order:
Patch Applier:   * [{"op": "replace", "path": "/ACL_RULE/SSH_ONLY|TEST_DROP/PACKET_ACTION", "value": "FORWARD"}]
Patch Applier: Verifying patch updates are reflected on ConfigDB.
Patch Applier: Patch application completed.
Patch applied successfully.
admin@vlab-01:~$ show acl rule
Table     Rule       Priority    Action    Match
--------  ---------  ----------  --------  -------------------
SSH_ONLY  TEST_DROP  9997        FORWARD   IP_PROTOCOL: 7
                                           IP_TYPE: ANY
                                           L4_DST_PORT: 88
                                           SRC_IP: 9.9.9.10/32
admin@vlab-01:~$

Will revert the change to ACL_RULE

... wait... show command might be getting the data from ConfigDB

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using iptables -S I was able to verify that the src_ip and the action are not create-only. For the rest of the fields I was not able to change them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the the logs in #

Jan 29 03:58:06.226985 vlab-01 INFO caclmgrd[2235692]:     if self.config_db_map[namespace].get_table(self.ACL_TABLE)[acl_table]["type"] == self.ACL_TABLE_TYPE_CTRLPLANE:  <=========================
Jan 29 03:58:06.227040 vlab-01 INFO caclmgrd[2235692]: KeyError: 'SSH_ONLY'

It seems the ACL_TABLE SSH_ONLY was not retrieved correctly from configdb. It does not seem related to create-only and requires further investigation.

["BGP_PEER_RANGE", "*", "*"],
["BGP_MONITORS", "*", "holdtime"],
["BGP_MONITORS", "*", "keepalive"],
["BGP_MONITORS", "*", "name"],
["BGP_MONITORS", "*", "asn"],
["BGP_MONITORS", "*", "local_addr"],
["BGP_MONITORS", "*", "nhopself"],
["BGP_MONITORS", "*", "rrclient"],
],
path_addressing)

Expand Down
47 changes: 47 additions & 0 deletions tests/generic_config_updater/patch_sorter_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1011,6 +1011,37 @@ def test_hard_coded_create_only_paths(self):
"nhopself": "0",
"rrclient": "0"
}
},
"ACL_RULE": {
"V4-ACL-TABLE|Rule_20": {
"PACKET_ACTION": "FORWARD",
"DST_IP": "10.222.72.0/26",
"SRC_IP": "10.222.0.0/15",
"PRIORITY": "777780",
"IP_TYPE": "IPv4ANY"
}
},
"BGP_PEER_RANGE": {
"BGPSLBPassive": {
"ip_range": [
"10.255.0.0/25"
],
"name": "BGPSLBPassive",
"peer_asn": "65543",
"src_address": "10.1.0.32"
}
},
"BGP_MONITORS": {
"5.6.7.8": {
"admin_status": "up",
"asn": "65000",
"holdtime": "180",
"keepalive": "60",
"local_addr": "10.0.0.11",
"name": "BGPMonitor",
"nhopself": "0",
"rrclient": "0"
}
}
}
expected = [
Expand All @@ -1025,6 +1056,22 @@ def test_hard_coded_create_only_paths(self):
"/BGP_NEIGHBOR/10.0.0.57/name",
"/BGP_NEIGHBOR/10.0.0.57/nhopself",
"/BGP_NEIGHBOR/10.0.0.57/rrclient",
"/ACL_RULE/V4-ACL-TABLE|Rule_20/PACKET_ACTION",
"/ACL_RULE/V4-ACL-TABLE|Rule_20/DST_IP",
"/ACL_RULE/V4-ACL-TABLE|Rule_20/SRC_IP",
"/ACL_RULE/V4-ACL-TABLE|Rule_20/PRIORITY",
"/ACL_RULE/V4-ACL-TABLE|Rule_20/IP_TYPE",
"/BGP_PEER_RANGE/BGPSLBPassive/ip_range",
"/BGP_PEER_RANGE/BGPSLBPassive/name",
"/BGP_PEER_RANGE/BGPSLBPassive/peer_asn",
"/BGP_PEER_RANGE/BGPSLBPassive/src_address",
"/BGP_MONITORS/5.6.7.8/asn",
"/BGP_MONITORS/5.6.7.8/holdtime",
"/BGP_MONITORS/5.6.7.8/keepalive",
"/BGP_MONITORS/5.6.7.8/local_addr",
"/BGP_MONITORS/5.6.7.8/name",
"/BGP_MONITORS/5.6.7.8/nhopself",
"/BGP_MONITORS/5.6.7.8/rrclient",
]

actual = self.validator._get_create_only_paths(config)
Expand Down