Enhanced Rule#18 to Disallow YANG LISTs with Over-lapping Keys #1458

faraazbrcm · 2023-09-04T08:15:06Z

It is observed that guideline does not describe the rules for YANG list keys. Added rules to make YANG LIST unique.

linux-foundation-easycla · 2023-09-04T08:15:09Z

The committers listed above are authorized under a signed CLA.

✅ login: faraazbrcm (af3dea2, c1422e6, a343a39, adb11d5, 236c6a1)

adyeung · 2023-09-13T19:24:30Z

@dutta-partha @praveen-li pls help review

praveen-li · 2023-09-19T00:47:09Z

doc/mgmt/SONiC_YANG_Model_Guidelines.md

@@ -550,12 +562,166 @@ container sonic-interface {
 ......
 ```

+#### Example 2: Composite Keys with Same Number of Elements(NOT Allowed case)


This one looks right to me.

you mean Rule holds good.. And example given makes sense. correct?

Hi @praveen-li , here is the issue created by @faraazbrcm : sonic-net/sonic-buildimage#16356
The same key count cannot be parsed in GCU side. because two list share the same key count and same key type. When GCU is verifying the YANG model: telemetry_client.yang. GCU failed to parse TELEMETRY_CLIENT_SUB_LIST. It verify the list matches TELEMETRY_CLIENT_DS_LIST then return failure.

I would suggest to use different key count to protect GCU parse.

I can think of two fix on this:
Combine two list into one with key count=2. Example: https://github.com/sonic-net/sonic-buildimage/blob/master/src/sonic-yang-models/yang-models/sonic-bgp-allowed-prefix.yang#L70-L71
Use two different key count to represent these lists. Example: https://github.com/sonic-net/sonic-buildimage/blob/master/src/sonic-yang-models/yang-models/sonic-interface.yang#L45-L95

Composite Keys with Same Number of Elements.

[PC]: Same number of element is allowed by translation. 'Same number of element plus Same type' is not allowed. But if we can support patterns, we can allow same type in future. I feel it should be allowed and should be added in translation.

@praveen-li Elements with distinct types but the same number can also lead to overlaps, especially since Redis always treats keys as strings. For instance, the number 123 can also be interpreted as the string '123'. To prevent this, we should apply specific constraints, like patterns for strings and ranges for integer types. This requirement should be clearly stated in the guideline.

@praveen-li , in the telemetry client sample, the list defination(TELEMETRY_CLIENT_SUB_LIST and TELEMETRY_CLIENT_DS_LIST) belongs to the "same number of element plus Same type". It should not be allowed.

praveen-li · 2023-09-19T00:48:17Z

doc/mgmt/SONiC_YANG_Model_Guidelines.md

+
+#### Example 3: Pattern Constraints (Allowed case)
+
+In this example, `INTERFACE_LIST` uses a key `ifname` that must start with "Eth", while `INTERFACE_IPADDR_LIST` uses a key `ifname` that must start with "Vlan".


Does the translation support it. Also do we have use case for it? Kindly paste if we have.

Translation currently does not support it. Currently the Apps such as sonic-utility, mgmt-common's CVL does not support this translation. But this SONiC YANG contains this scenario. I have raised an issue for sonic-utilities. If this is not supported we need to revert the YANG or accept this guideline to enhance the Apps to support this translation.

I believe we should either develop a SONiC YANG validator that aligns with our guidelines and integrate it into the sonic-buildimage's Makefile to run with each build. This would prevent any unsupported translations. Alternatively, we could enhance the existing YANG test to accept data in CONFIG_DB format instead of YANG format. The test runner would then convert the CONFIG_DB format data to YANG format before executing the test, and any errors encountered should halt the build process.

validator is in place.

test for translation and reverse translation

Config db used for above test

Yes, seems for telemetry client, no config may be added in sample_config_db.json. That's why no build failures. This can be in Guideline to add config.

@praveen-li There exists a config for telemetry in sample_config_db.json. Any idea why this has NOT caused the build failure?

praveen-li · 2023-09-19T00:50:14Z

doc/mgmt/SONiC_YANG_Model_Guidelines.md

+
+#### Example 6: Length Constraints (NOT Allowed case)
+
+Here, both `INTERFACE_LIST` and `INTERFACE_ANOTHER_LIST` use a key ifname with overlapping length constraints, making them potentially ambiguous.


I have similar commands for examples, 4 to 6. Do we have valid use case right now, where these example or constraints can be used. Thanks.

There is no usecase as of today for 4 to 6, but they could become relevant at any time. As I've mentioned, we need to future-proof our applications. One way to do this is by not allowing such YANG lists, which we can enforce through a validator. Additionally, we need to ensure that all YANGs comply with the guidelines. To achieve this, the guidelines must be as clear and explicit as possible, and these points aim to accomplish that.

Looking at telemetry YANG, seems there is a genuine case. If yes then.

-- Update Guideline. [please provide real example of today's config].
-- above should be done after translation PR is raised, which supports pattern if primary key has 'same number of element and same type.

@praveen-li IMO guideline PR has to go first then the translations. The translations are based on guideline. correct me.

adyeung · 2023-09-22T17:58:44Z

@praveen-li @qiluo-msft @wen587 @faraazbrcm A webex meeting invite is sent to discuss this PR on 9/26 Tues 8:30pm PST, pls plan to join to get a closure.

@wen587 I don't have your email contact, I routed the invite thru Qi, pls reachout to Qi for webex info

praveen-li · 2023-09-27T05:02:35Z

@faraazbrcm See this Page https://github.com/sonic-net/sonic-utilities/tree/master

Below, Instead of sonic-utilities, build sonic-yang-mgmt PKG. Then you can run tests. Also Note, sonic-yang-mgmt PKG have dependency on sonic-yang-models PKG, so if you change yang models then build sonic-yang-models PKG too.

Build the sonic-utilities Python wheel package inside the Bullseye slave container, and tell the build system to keep the container alive when finished

make NOSTRETCH=1 NOBUSTER=1 KEEP_SLAVE_ON=yes target/python-wheels/bullseye/sonic_utilities-1.2-py3-none-any.whl

    When the build finishes, your prompt will change to indicate you are inside the slave container. Change into the src/sonic-utilities/ directory

    user@911799f161a0:/sonic$ cd src/sonic-utilities/

    You can now make changes to the sonic-utilities source and build the package or run unit tests with the commands below. When finished, you can exit the container by calling exit.

[To build](https://github.com/sonic-net/sonic-utilities/tree/master#to-build)

python3 setup.py bdist_wheel

[To run unit tests](https://github.com/sonic-net/sonic-utilities/tree/master#to-run-unit-tests)

python3 setup.py test

Enhanced PR as per the discussion with YANG subgroup

praveen-li · 2023-10-09T15:50:40Z

@faraazbrcm For {yang list with [same number of keys + string type] }

In current case, a yang list with [same number of keys + string type] can be allowed. A restriction to include 'pattern of the key' will be added to differentiate. Without pattern keyword for key, translation will not be supported from config_db.json to sonic_yang.json.

wen587 · 2023-10-10T03:05:43Z

Hi @praveen-li , I want to remind that GCU only supports different keys count for now. That's why the TELEMETRY_CLIENT Yang fails in GCU process.

wen587 · 2023-10-18T06:08:24Z

Create a PR for Path 2: Do not permit lists to have the same number of keys.
sonic-net/sonic-buildimage#16861

wen587 · 2023-10-23T08:02:34Z

Create a PR for Path 2: Do not permit lists to have the same number of keys. sonic-net/sonic-buildimage#16861

Hi, @faraazbrcm and @praveen-li , could you help review this PR about Path 2

faraazbrcm · 2023-10-25T06:21:28Z

@praveen-li, I've updated this PR in line with Path1, as discussed in subgroup email correspondence with the Alibaba team. I believe we agreed to proceed with Path1, which involves adding support for the pattern statement. Could you please review this PR? If everything looks satisfactory, kindly approve it. If any modifications are needed, please let me know.

wen587 · 2023-10-25T06:22:51Z

@qiluo-msft add Qi for vis

kwangsuk · 2023-11-15T19:37:02Z

As you see in the typical YANG modeling practice like ipv4/ip6, the union type of key node is commonly used in the YANG community like openconfig or ietf. So, in the similar line, the path2 approach would be better versed and structured in YANG modeling.

faraazbrcm · 2023-11-16T04:17:40Z

@praveen-li @adyeung @yaqiangz we should decide whether to go with path 1 or path 2 ASAP. This PR is aligned with path 1.

kwangsuk · 2023-11-18T00:01:07Z

@praveen-li @adyeung @yaqiangz we should decide whether to go with path 1 or path 2 ASAP. This PR is aligned with path 1.

Let's not compromise the effective modeling practices just because of internal implementation constraints in consuming YANG. As we move on, we should be able to encourage SONiC contributors to abstract services in well versed and structured YANG models with a rich set of YANG statements wherever applicable.

faraazbrcm · 2024-01-16T14:20:35Z

closing because alternate path sonic-net/sonic-buildimage#16861 is accepted

Enhanced Rule#18 to disallow LISTs with over-lapping List keys

af3dea2

It is observed that guideline does not describe the rules for YANG list keys. Added rules to make YANG LIST unique.

faraazbrcm changed the title ~~Enhanced Rule#18 to disallow LISTs with over-lapping List keys~~ Enhanced Rule#18 to disallow LISTs with over-lapping keys Sep 4, 2023

faraazbrcm changed the title ~~Enhanced Rule#18 to disallow LISTs with over-lapping keys~~ Enhanced Rule#18 to Disallow YANG LISTs with Over-lapping Keys Sep 4, 2023

faraazbrcm mentioned this pull request Sep 13, 2023

[yang] Unable to apply config from sonic-telemetry_client.yang sonic-net/sonic-buildimage#16356

Closed

adyeung requested a review from praveen-li September 13, 2023 19:24

qiluo-msft requested review from dgsudharsan and ganglyu September 14, 2023 20:36

qiluo-msft added the yang label Sep 14, 2023

praveen-li reviewed Sep 19, 2023

View reviewed changes

faraazbrcm requested a review from praveen-li September 19, 2023 07:10

faraazbrcm added 3 commits September 20, 2023 14:15

Update SONiC_YANG_Model_Guidelines.md

c1422e6

Update SONiC_YANG_Model_Guidelines.md

a343a39

Update SONiC_YANG_Model_Guidelines.md

adb11d5

faraazbrcm requested a review from wen587 September 21, 2023 16:41

Update SONiC_YANG_Model_Guidelines.md

236c6a1

Enhanced PR as per the discussion with YANG subgroup

faraazbrcm mentioned this pull request Oct 25, 2023

Update TELEMETRY_CLIENT YANG model sonic-net/sonic-buildimage#16861

Merged

11 tasks

faraazbrcm closed this Jan 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhanced Rule#18 to Disallow YANG LISTs with Over-lapping Keys #1458

Enhanced Rule#18 to Disallow YANG LISTs with Over-lapping Keys #1458

faraazbrcm commented Sep 4, 2023

linux-foundation-easycla bot commented Sep 4, 2023 •

edited

Loading

adyeung commented Sep 13, 2023

praveen-li Sep 19, 2023

faraazbrcm Sep 19, 2023

wen587 Sep 21, 2023

wen587 Sep 21, 2023

praveen-li Sep 21, 2023

faraazbrcm Sep 21, 2023

wen587 Sep 22, 2023

praveen-li Sep 19, 2023

faraazbrcm Sep 19, 2023 •

edited

Loading

praveen-li Sep 21, 2023 •

edited

Loading

faraazbrcm Sep 21, 2023

praveen-li Sep 19, 2023

faraazbrcm Sep 19, 2023

praveen-li Sep 21, 2023

faraazbrcm Sep 21, 2023

adyeung commented Sep 22, 2023

praveen-li commented Sep 27, 2023 •

edited

Loading

praveen-li commented Oct 9, 2023

wen587 commented Oct 10, 2023

wen587 commented Oct 18, 2023

wen587 commented Oct 23, 2023

faraazbrcm commented Oct 25, 2023

wen587 commented Oct 25, 2023

kwangsuk commented Nov 15, 2023 •

edited

Loading

faraazbrcm commented Nov 16, 2023

kwangsuk commented Nov 18, 2023 •

edited

Loading

faraazbrcm commented Jan 16, 2024


		#### Example 3: Pattern Constraints (Allowed case)

		In this example, `INTERFACE_LIST` uses a key `ifname` that must start with "Eth", while `INTERFACE_IPADDR_LIST` uses a key `ifname` that must start with "Vlan".


		#### Example 6: Length Constraints (NOT Allowed case)

		Here, both `INTERFACE_LIST` and `INTERFACE_ANOTHER_LIST` use a key ifname with overlapping length constraints, making them potentially ambiguous.

Enhanced Rule#18 to Disallow YANG LISTs with Over-lapping Keys #1458

Enhanced Rule#18 to Disallow YANG LISTs with Over-lapping Keys #1458

Conversation

faraazbrcm commented Sep 4, 2023

linux-foundation-easycla bot commented Sep 4, 2023 • edited Loading

adyeung commented Sep 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

faraazbrcm Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

praveen-li Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adyeung commented Sep 22, 2023

praveen-li commented Sep 27, 2023 • edited Loading

praveen-li commented Oct 9, 2023

wen587 commented Oct 10, 2023

wen587 commented Oct 18, 2023

wen587 commented Oct 23, 2023

faraazbrcm commented Oct 25, 2023

wen587 commented Oct 25, 2023

kwangsuk commented Nov 15, 2023 • edited Loading

faraazbrcm commented Nov 16, 2023

kwangsuk commented Nov 18, 2023 • edited Loading

faraazbrcm commented Jan 16, 2024

linux-foundation-easycla bot commented Sep 4, 2023 •

edited

Loading

faraazbrcm Sep 19, 2023 •

edited

Loading

praveen-li Sep 21, 2023 •

edited

Loading

praveen-li commented Sep 27, 2023 •

edited

Loading

kwangsuk commented Nov 15, 2023 •

edited

Loading

kwangsuk commented Nov 18, 2023 •

edited

Loading