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

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

Merged
merged 2 commits into from
Mar 9, 2022

Conversation

ghooo
Copy link
Contributor

@ghooo ghooo commented Mar 4, 2022

What I did

Fixes #2029 #2062

Changes to fields under BGP_PEER_RANGE, BGP_MONITORS using GCU are not reflected unless each key is deleted and added back. This means the fields are create-only.

How I did it

Marked the fields under BGP_PEER_RANGE, BGP_MONITORS as create-only

How to verify it

unit-test

Previous command output (if the output of a command-line utility has changed)

New command output (if the output of a command-line utility has changed)

@@ -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

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

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

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.

qiluo-msft
qiluo-msft previously approved these changes Mar 5, 2022
Copy link
Contributor

@qiluo-msft qiluo-msft left a comment

Choose a reason for hiding this comment

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

LGTM. Please wait for other active reviewers.

@ghooo ghooo changed the title [GCU] Marking fields under ACL_RULE, BGP_PEER_RANGE, BGP_MONITORS as create-only [GCU] Marking fields under BGP_PEER_RANGE, BGP_MONITORS as create-only Mar 8, 2022
@qiluo-msft qiluo-msft merged commit 2c56e92 into sonic-net:master Mar 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants