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

[yang-models] Validating 'services' exist if ACL type is 'CTRLPLANE' #9295

Merged
merged 2 commits into from
Dec 10, 2021

Conversation

ghooo
Copy link
Contributor

@ghooo ghooo commented Nov 17, 2021

Why I did it

Fixing issue #9294

How I did it

Updating ACL yang model

How to verify it

Validating issue with config patch-apply is fixed.

  • Start a KVM
  • Add file add-ctrl-plane-tbl.json-patch with content:
[
    {
     "op": "add",
     "path": "/ACL_TABLE/ACTRLPLANETABLE",
     "value": {
      "policy_desc": "ACTRLPLANETABLE",
      "services": [
       "SSH"
      ],
      "stage": "ingress",
      "type": "CTRLPLANE"
     }
    }
]
  • Run sudo config apply-patch add-ctrl-plane-tbl.json-patch

Before:

Patch Applier: The patch was sorted into 4 changes:
Patch Applier:   * [{"op": "add", "path": "/ACL_TABLE/ACTRLPLANETABLE", "value": {"type": "CTRLPLANE"}}]
Patch Applier:   * [{"op": "add", "path": "/ACL_TABLE/ACTRLPLANETABLE/policy_desc", "value": "ACTRLPLANETABLE"}]
Patch Applier:   * [{"op": "add", "path": "/ACL_TABLE/ACTRLPLANETABLE/services", "value": ["SSH"]}]
Patch Applier:   * [{"op": "add", "path": "/ACL_TABLE/ACTRLPLANETABLE/stage", "value": "ingress"}]

After:

Patch Applier: The patch was sorted into 1 change:
Patch Applier:   * [{"op": "add", "path": "/ACL_TABLE/ACTRLPLANETABLE", "value": {"policy_desc": "ACTRLPLANETABLE", "services": ["SSH"], "stage": "ingress", "type": "CTRLPLANE"}}]

Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106

Description for the changelog

A picture of a cute animal (not mandatory but encouraged)

@ghooo ghooo requested a review from lguohan as a code owner November 17, 2021 23:45
@ghooo
Copy link
Contributor Author

ghooo commented Nov 18, 2021

there is a build error, i think i will fix it and a unit-test

@dgsudharsan
Copy link
Collaborator

Can you please add UT to cover this case?

@wen587
Copy link
Contributor

wen587 commented Nov 18, 2021

Issue #9294 is fixed from my mannually test.

Copy link
Collaborator

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

As comment

@@ -259,6 +259,8 @@ module sonic-acl {
type string;
}

must "(type != 'CTRLPLANE') or (boolean(services))";
Copy link
Collaborator

@qiluo-msft qiluo-msft Nov 18, 2021

Choose a reason for hiding this comment

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

Add some comment to explain the constrain? #Closed

Copy link
Collaborator

Choose a reason for hiding this comment

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

@bingwang-ms Do we require service field in data plane ACL?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Add some unit test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@dgsudharsan dgsudharsan added the YANG YANG model related changes label Nov 18, 2021
qiluo-msft
qiluo-msft previously approved these changes Dec 9, 2021
@ghooo ghooo merged commit 02d2732 into sonic-net:master Dec 10, 2021
@qiluo-msft qiluo-msft added the Request for 202111 Branch For PRs being requested for 202111 branch label Jan 5, 2022
judyjoseph pushed a commit that referenced this pull request Jan 9, 2022
…9295)

#### Why I did it
Fixing issue #9294

#### How I did it
Updating ACL yang model

#### How to verify it

Validating issue with `config patch-apply` is fixed.

- Start a KVM
- Add file `add-ctrl-plane-tbl.json-patch ` with content:
```json
[
    {
     "op": "add",
     "path": "/ACL_TABLE/ACTRLPLANETABLE",
     "value": {
      "policy_desc": "ACTRLPLANETABLE",
      "services": [
       "SSH"
      ],
      "stage": "ingress",
      "type": "CTRLPLANE"
     }
    }
]
```
- Run `sudo config apply-patch add-ctrl-plane-tbl.json-patch`


Before:
```
Patch Applier: The patch was sorted into 4 changes:
Patch Applier:   * [{"op": "add", "path": "/ACL_TABLE/ACTRLPLANETABLE", "value": {"type": "CTRLPLANE"}}]
Patch Applier:   * [{"op": "add", "path": "/ACL_TABLE/ACTRLPLANETABLE/policy_desc", "value": "ACTRLPLANETABLE"}]
Patch Applier:   * [{"op": "add", "path": "/ACL_TABLE/ACTRLPLANETABLE/services", "value": ["SSH"]}]
Patch Applier:   * [{"op": "add", "path": "/ACL_TABLE/ACTRLPLANETABLE/stage", "value": "ingress"}]
```

After:
```
Patch Applier: The patch was sorted into 1 change:
Patch Applier:   * [{"op": "add", "path": "/ACL_TABLE/ACTRLPLANETABLE", "value": {"policy_desc": "ACTRLPLANETABLE", "services": ["SSH"], "stage": "ingress", "type": "CTRLPLANE"}}]
```

#### Which release branch to backport (provide reason below if selected)

<!--
- Note we only backport fixes to a release branch, *not* features!
- Please also provide a reason for the backporting below.
- e.g.
- [x] 202006
-->

- [ ] 201811
- [ ] 201911
- [ ] 202006
- [ ] 202012
- [ ] 202106

#### Description for the changelog
<!--
Write a short (one line) summary that describes the changes in this
pull request for inclusion in the changelog:
-->


#### A picture of a cute animal (not mandatory but encouraged)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Included in 202111 Branch Request for 202106 Branch Request for 202111 Branch For PRs being requested for 202111 branch YANG YANG model related changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants