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

jdiff should support multiple ref key anchoring #92

Closed
lvrfrc87 opened this issue Nov 17, 2022 · 1 comment · Fixed by #98
Closed

jdiff should support multiple ref key anchoring #92

lvrfrc87 opened this issue Nov 17, 2022 · 1 comment · Fixed by #98
Assignees

Comments

@lvrfrc87
Copy link
Collaborator

As per json below, we might have multiple vrf and for each vrf multiple peer IPs - which are unique within each vrf. In the example below, we want to anchor global and 192.168.0.0. So regex would look like: $*$.peers.$*$.*.*.[accepted_prefixes,received_prefixes,sent_prefixes]

{
  "global": {
    "router_id": "10.255.255.1",
    "peers": {
      "192.168.0.0": {
        "description": "",
        "remote_id": "10.255.255.240",
        "address_family": {
          "ipv6": {
            "received_prefixes": -1,
            "sent_prefixes": -1,
            "accepted_prefixes": -1
          },
          "ipv4": {
            "received_prefixes": -1,
            "sent_prefixes": -1,
            "accepted_prefixes": -1
          }
        },
        "remote_as": 30000,
        "is_up": true,
        "local_as": 30001,
        "is_enabled": true,
        "uptime": 699262
@pszulczewski
Copy link
Collaborator

After analyzing the current code-base such pinning of the VRF key won't be possible without changing the current implementation of ref_key processing.

The proposal in this issue is to allow to use multi ref_keys like that:
"$*$.peers.$*$.*.*.[accepted_prefixes,received_prefixes,sent_prefixes]"
|-------------^
One for vrf one for peer IPs, but look where the peers key is pinned ... and yes this is a very intuitive way IMO.
However when you look at our unittest, for the same data structure when we want to assign the ref_key for peer IPs it's currently done like that:
"global.$peers$.*.*.ipv4.[accepted_prefixes,received_prefixes,sent_prefixes]"
|-----------^
Which returns this data:

[{'10.1.0.0': {'accepted_prefixes': 1000,
               'received_prefixes': 1000,
               'sent_prefixes': 1000}},
 {'10.2.0.0': {'accepted_prefixes': 1000,
               'received_prefixes': 1000,
               'sent_prefixes': 1000}},
 {'10.64.207.255': {'accepted_prefixes': 1000,
                    'received_prefixes': 1000,
                    'sent_prefixes': 1000}},
 {'7.7.7.7': {'accepted_prefixes': 1000,
              'received_prefixes': 1000,
              'sent_prefixes': 1000}}]

Conclusion:
Current implementation to pin the ref_key for peer IPs expects that the $$ markers are used on the preceding key/path element "peers" and not on the exact key location, like proposed in this issue.
If you look at vrf in this case global, that's the first key in the data structure and there is no preceding key to pin it.
To implement multi-ref_key support we need to change the current behavior.

The process would look like this:

  • re-implement ref_key processing to stick to the element which in fact needs to be pinned.
  • update ref_keys in unittests, output data should not change, only the ref_key markers will be moved right.

This was referenced Apr 18, 2023
pszulczewski added a commit that referenced this issue Apr 19, 2023
pszulczewski added a commit that referenced this issue Apr 19, 2023
pszulczewski added a commit that referenced this issue Apr 19, 2023
pszulczewski added a commit that referenced this issue Apr 19, 2023
pszulczewski added a commit that referenced this issue Apr 19, 2023
pszulczewski added a commit that referenced this issue Apr 19, 2023
@pszulczewski pszulczewski linked a pull request Apr 19, 2023 that will close this issue
pszulczewski added a commit that referenced this issue Apr 19, 2023
lvrfrc87 pushed a commit that referenced this issue Apr 20, 2023
* Base-line tests for future refactor.

* Refactor to simplify code.

* Implement issue #92
lvrfrc87 added a commit that referenced this issue Apr 20, 2023
* Update readme to start with use cases (#84)

* Update readme to start with use cases

* Apply suggestions from code review

Co-authored-by: Patryk Szulczewski <[email protected]>
Co-authored-by: Stephen Corry <[email protected]>

* Update README.md

Co-authored-by: Patryk Szulczewski <[email protected]>
Co-authored-by: Stephen Corry <[email protected]>

* Doc update (#87)

* Fix operator checks to follow other check_type logic. (#85)

* Fix operator checks to follow other check_type logic.

* Add new release 0.0.2

Co-authored-by: Patryk Szulczewski <[email protected]>

* Implement `ge` and `le` operator type (#89)

* Release 0.0.2  (#88)

* Update readme to start with use cases (#84)

* Update readme to start with use cases

* Apply suggestions from code review

Co-authored-by: Patryk Szulczewski <[email protected]>
Co-authored-by: Stephen Corry <[email protected]>

* Update README.md

Co-authored-by: Patryk Szulczewski <[email protected]>
Co-authored-by: Stephen Corry <[email protected]>

* Doc update (#87)

* Fix operator checks to follow other check_type logic. (#85)

* Fix operator checks to follow other check_type logic.

* Add new release 0.0.2

Co-authored-by: Patryk Szulczewski <[email protected]>

Co-authored-by: Christian Adell <[email protected]>
Co-authored-by: Ken Celenza <[email protected]>
Co-authored-by: Patryk Szulczewski <[email protected]>
Co-authored-by: Stephen Corry <[email protected]>
Co-authored-by: Patryk Szulczewski <[email protected]>

* add operator ge and le

* fix validate tests

Co-authored-by: Christian Adell <[email protected]>
Co-authored-by: Ken Celenza <[email protected]>
Co-authored-by: Patryk Szulczewski <[email protected]>
Co-authored-by: Stephen Corry <[email protected]>
Co-authored-by: Patryk Szulczewski <[email protected]>

* Fix parameter_match check, to support non-normalized data (#90)

* Fix parameter_match check, to support non-normalized data

* Add test

* Update tests.

* remove test print

Co-authored-by: Patryk Szulczewski <[email protected]>

* Update mypy

* Bugfix to data normalization

* Fix ref_key parsing

* Update docs

* Issue 92 (#98)

* Base-line tests for future refactor.

* Refactor to simplify code.

* Implement issue #92

* Bump certifi from 2022.9.24 to 2022.12.7 (#95)

Bumps [certifi](https://github.com/certifi/python-certifi) from 2022.9.24 to 2022.12.7.
- [Release notes](https://github.com/certifi/python-certifi/releases)
- [Commits](certifi/python-certifi@2022.09.24...2022.12.07)

---
updated-dependencies:
- dependency-name: certifi
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: Christian Adell <[email protected]>
Co-authored-by: Ken Celenza <[email protected]>
Co-authored-by: Patryk Szulczewski <[email protected]>
Co-authored-by: Stephen Corry <[email protected]>
Co-authored-by: Patryk Szulczewski <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
lvrfrc87 added a commit that referenced this issue May 12, 2023
* Update readme to start with use cases (#84)

* Update readme to start with use cases

* Apply suggestions from code review

Co-authored-by: Patryk Szulczewski <[email protected]>
Co-authored-by: Stephen Corry <[email protected]>

* Update README.md

Co-authored-by: Patryk Szulczewski <[email protected]>
Co-authored-by: Stephen Corry <[email protected]>

* Doc update (#87)

* Fix operator checks to follow other check_type logic. (#85)

* Fix operator checks to follow other check_type logic.

* Add new release 0.0.2

Co-authored-by: Patryk Szulczewski <[email protected]>

* Implement `ge` and `le` operator type (#89)

* Release 0.0.2  (#88)

* Update readme to start with use cases (#84)

* Update readme to start with use cases

* Apply suggestions from code review

Co-authored-by: Patryk Szulczewski <[email protected]>
Co-authored-by: Stephen Corry <[email protected]>

* Update README.md

Co-authored-by: Patryk Szulczewski <[email protected]>
Co-authored-by: Stephen Corry <[email protected]>

* Doc update (#87)

* Fix operator checks to follow other check_type logic. (#85)

* Fix operator checks to follow other check_type logic.

* Add new release 0.0.2

Co-authored-by: Patryk Szulczewski <[email protected]>

Co-authored-by: Christian Adell <[email protected]>
Co-authored-by: Ken Celenza <[email protected]>
Co-authored-by: Patryk Szulczewski <[email protected]>
Co-authored-by: Stephen Corry <[email protected]>
Co-authored-by: Patryk Szulczewski <[email protected]>

* add operator ge and le

* fix validate tests

Co-authored-by: Christian Adell <[email protected]>
Co-authored-by: Ken Celenza <[email protected]>
Co-authored-by: Patryk Szulczewski <[email protected]>
Co-authored-by: Stephen Corry <[email protected]>
Co-authored-by: Patryk Szulczewski <[email protected]>

* Fix parameter_match check, to support non-normalized data (#90)

* Fix parameter_match check, to support non-normalized data

* Add test

* Update tests.

* remove test print

Co-authored-by: Patryk Szulczewski <[email protected]>

* Update mypy

* Bugfix to data normalization

* Fix ref_key parsing

* Update docs

* Issue 92 (#98)

* Base-line tests for future refactor.

* Refactor to simplify code.

* Implement issue #92

* Bump certifi from 2022.9.24 to 2022.12.7 (#95)

Bumps [certifi](https://github.com/certifi/python-certifi) from 2022.9.24 to 2022.12.7.
- [Release notes](https://github.com/certifi/python-certifi/releases)
- [Commits](certifi/python-certifi@2022.09.24...2022.12.07)

---
updated-dependencies:
- dependency-name: certifi
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Update version and changelog

* update poetry lock

* Update changelog

* Issue 94 & 91 (#100)

* Baseline unittests

* Fix issue #94

* Remove test print

* Fix issue #91

* Relax deepdiff dependency.

* Update pyproject.toml

Co-authored-by: Patryk Szulczewski <[email protected]>

* Update poetry.lock

* Relase v0.0.4

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: Christian Adell <[email protected]>
Co-authored-by: Ken Celenza <[email protected]>
Co-authored-by: Stephen Corry <[email protected]>
Co-authored-by: Patryk Szulczewski <[email protected]>
Co-authored-by: Federico87 <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Network to Code <[email protected]>
Co-authored-by: Leo Kirchner <[email protected]>
Co-authored-by: Leo Kirchner <[email protected]>
lvrfrc87 added a commit that referenced this issue May 12, 2023
* Update readme to start with use cases (#84)

* Update readme to start with use cases

* Apply suggestions from code review

Co-authored-by: Patryk Szulczewski <[email protected]>
Co-authored-by: Stephen Corry <[email protected]>

* Update README.md

Co-authored-by: Patryk Szulczewski <[email protected]>
Co-authored-by: Stephen Corry <[email protected]>

* Doc update (#87)

* Fix operator checks to follow other check_type logic. (#85)

* Fix operator checks to follow other check_type logic.

* Add new release 0.0.2

Co-authored-by: Patryk Szulczewski <[email protected]>

* Implement `ge` and `le` operator type (#89)

* Release 0.0.2  (#88)

* Update readme to start with use cases (#84)

* Update readme to start with use cases

* Apply suggestions from code review

Co-authored-by: Patryk Szulczewski <[email protected]>
Co-authored-by: Stephen Corry <[email protected]>

* Update README.md

Co-authored-by: Patryk Szulczewski <[email protected]>
Co-authored-by: Stephen Corry <[email protected]>

* Doc update (#87)

* Fix operator checks to follow other check_type logic. (#85)

* Fix operator checks to follow other check_type logic.

* Add new release 0.0.2

Co-authored-by: Patryk Szulczewski <[email protected]>

Co-authored-by: Christian Adell <[email protected]>
Co-authored-by: Ken Celenza <[email protected]>
Co-authored-by: Patryk Szulczewski <[email protected]>
Co-authored-by: Stephen Corry <[email protected]>
Co-authored-by: Patryk Szulczewski <[email protected]>

* add operator ge and le

* fix validate tests

Co-authored-by: Christian Adell <[email protected]>
Co-authored-by: Ken Celenza <[email protected]>
Co-authored-by: Patryk Szulczewski <[email protected]>
Co-authored-by: Stephen Corry <[email protected]>
Co-authored-by: Patryk Szulczewski <[email protected]>

* Fix parameter_match check, to support non-normalized data (#90)

* Fix parameter_match check, to support non-normalized data

* Add test

* Update tests.

* remove test print

Co-authored-by: Patryk Szulczewski <[email protected]>

* Update mypy

* Bugfix to data normalization

* Fix ref_key parsing

* Update docs

* Issue 92 (#98)

* Base-line tests for future refactor.

* Refactor to simplify code.

* Implement issue #92

* Bump certifi from 2022.9.24 to 2022.12.7 (#95)

Bumps [certifi](https://github.com/certifi/python-certifi) from 2022.9.24 to 2022.12.7.
- [Release notes](https://github.com/certifi/python-certifi/releases)
- [Commits](certifi/python-certifi@2022.09.24...2022.12.07)

---
updated-dependencies:
- dependency-name: certifi
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Update version and changelog

* update poetry lock

* Update changelog

* Issue 94 & 91 (#100)

* Baseline unittests

* Fix issue #94

* Remove test print

* Fix issue #91

* Relax deepdiff dependency.

* Update pyproject.toml

Co-authored-by: Patryk Szulczewski <[email protected]>

* Update poetry.lock

* Relase v0.0.4

* Release v0.0.4 (#105)

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: Christian Adell <[email protected]>
Co-authored-by: Ken Celenza <[email protected]>
Co-authored-by: Stephen Corry <[email protected]>
Co-authored-by: Patryk Szulczewski <[email protected]>
Co-authored-by: Federico87 <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Network to Code <[email protected]>
Co-authored-by: Leo Kirchner <[email protected]>
Co-authored-by: Leo Kirchner <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants