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

Feat(eos_cli_config_gen): DHCP Server data model #3269

Merged
merged 28 commits into from Jan 3, 2024

Conversation

emilarista
Copy link
Contributor

@emilarista emilarista commented Oct 19, 2023

Change Summary

Implements dhcp servers in eos_cli_config_gen

Component(s) name

arista.avd.eos_cli_config_gen

Proposed changes

```yaml
dhcp_servers:
  - disabled: <bool>
    vrf: <str>
    dns_domain_name_ipv4: <str>
    dns_domain_name_ipv6: <str>
    ipv4_vendor_options:
      - vendor_id: <str>
        sub_options:
          - number: <int>
            type: <str>
            data: <str>
    subnets:
      - subnet: <str>
        name: <str>
        default_gateway: <str>
        dns_servers:
          - <str>
        ranges:
          - start: <str>
            end: <str>
        lease_time:
          days: <int>
          hours: <int>
          minutes: <int>
```

How to test

Repository Checklist

  • My code has been rebased from devel before I start
  • I have read the CONTRIBUTING document.
  • My change requires a change to the documentation and documentation have been updated accordingly.
  • I have updated molecule CI testing accordingly. (check the box if not applicable)

@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions
Copy link

Conflicts have been resolved. A maintainer will review the pull request shortly.

@emilarista emilarista marked this pull request as ready for review October 23, 2023 17:56
@emilarista emilarista requested review from a team as code owners October 23, 2023 17:56
@RestlessWanderer
Copy link

Just tried rendering config with this data model and I was able to render a subnet with a prefix with no subnet mask and a plain-text value, which wouldn't be accepted at the CLI. See below:

With subnet with no subnet mask:

---
type: l3spine

dhcp_servers:
  - disabled: false
    vrf: default
    dns_domain_name_ipv4: arista.com
    dns_domain_name_ipv6: arista.com
    ipv4_vendor_options:
      - vendor_id: phone
        sub_options:
          - number: 66
            type: ipv4-address
            data: 1.1.1.1
    subnets:
      - subnet: 10.255.255.0
        name: Phones
        default_gateway: 10.255.255.1
        dns_servers:
          - 8.8.8.8
        ranges:
          - start: 10.255.255.50
            end: 10.255.255.254
        lease_time:
          days: 5
          hours: 24
          minutes: 15

Resulting configuration:

dhcp server vrf default
   dns domain name ipv4 arista.com
   dns domain name ipv6 arista.com
   !
   subnet 10.255.255.0
      !
      range 10.255.255.50 10.255.255.254
      name Phones
      dns server 8.8.8.8
      lease time 5 days 24 hours 15 minutes
      default-gateway 10.255.255.1
   !
   vendor-option ipv4 phone
      sub-option 66 type ipv4-address data 1.1.1.1

With plain text:

---
type: l3spine

dhcp_servers:
  - disabled: false
    vrf: default
    dns_domain_name_ipv4: arista.com
    dns_domain_name_ipv6: arista.com
    ipv4_vendor_options:
      - vendor_id: phone
        sub_options:
          - number: 66
            type: ipv4-address
            data: 1.1.1.1
    subnets:
      - subnet: phone-subnet
        name: Phones
        default_gateway: 10.255.255.1
        dns_servers:
          - 8.8.8.8
        ranges:
          - start: 10.255.255.50
            end: 10.255.255.254
        lease_time:
          days: 5
          hours: 24
          minutes: 15

Resulting configuration:

dhcp server vrf default
   dns domain name ipv4 arista.com
   dns domain name ipv6 arista.com
   !
   subnet phone-subnet
      !
      range 10.255.255.50 10.255.255.254
      name Phones
      dns server 8.8.8.8
      lease time 5 days 24 hours 15 minutes
      default-gateway 10.255.255.1
   !
   vendor-option ipv4 phone
      sub-option 66 type ipv4-address data 1.1.1.1

Everything else looks good.

@gmuloc
Copy link
Contributor

gmuloc commented Oct 26, 2023

Just tried rendering config with this data model and I was able to render a subnet with a prefix with no subnet mask and a plain-text value, which wouldn't be accepted at the CLI. See below:

With subnet with no subnet mask:

---
type: l3spine

dhcp_servers:
  - disabled: false
    vrf: default
    dns_domain_name_ipv4: arista.com
    dns_domain_name_ipv6: arista.com
    ipv4_vendor_options:
      - vendor_id: phone
        sub_options:
          - number: 66
            type: ipv4-address
            data: 1.1.1.1
    subnets:
      - subnet: 10.255.255.0
        name: Phones
        default_gateway: 10.255.255.1
        dns_servers:
          - 8.8.8.8
        ranges:
          - start: 10.255.255.50
            end: 10.255.255.254
        lease_time:
          days: 5
          hours: 24
          minutes: 15

Resulting configuration:

dhcp server vrf default
   dns domain name ipv4 arista.com
   dns domain name ipv6 arista.com
   !
   subnet 10.255.255.0
      !
      range 10.255.255.50 10.255.255.254
      name Phones
      dns server 8.8.8.8
      lease time 5 days 24 hours 15 minutes
      default-gateway 10.255.255.1
   !
   vendor-option ipv4 phone
      sub-option 66 type ipv4-address data 1.1.1.1

With plain text:

---
type: l3spine

dhcp_servers:
  - disabled: false
    vrf: default
    dns_domain_name_ipv4: arista.com
    dns_domain_name_ipv6: arista.com
    ipv4_vendor_options:
      - vendor_id: phone
        sub_options:
          - number: 66
            type: ipv4-address
            data: 1.1.1.1
    subnets:
      - subnet: phone-subnet
        name: Phones
        default_gateway: 10.255.255.1
        dns_servers:
          - 8.8.8.8
        ranges:
          - start: 10.255.255.50
            end: 10.255.255.254
        lease_time:
          days: 5
          hours: 24
          minutes: 15

Resulting configuration:

dhcp server vrf default
   dns domain name ipv4 arista.com
   dns domain name ipv6 arista.com
   !
   subnet phone-subnet
      !
      range 10.255.255.50 10.255.255.254
      name Phones
      dns server 8.8.8.8
      lease time 5 days 24 hours 15 minutes
      default-gateway 10.255.255.1
   !
   vendor-option ipv4 phone
      sub-option 66 type ipv4-address data 1.1.1.1

Everything else looks good.

These are both valid points though today we don't support yet format validation on these type of keys except through regex and we are not really implementing this today for instance you could generate this using AVD:

interface Vlan12
   description VRF10_VLAN12
   no shutdown
   vrf VRF10
   ip address virtual is-this-the-right-format <<<<<<<<<<<<<<

so let's not fix it in this PR yet

@carlbuchmann carlbuchmann requested review from gmuloc and a team November 2, 2023 01:32
@ClausHolbechArista
Copy link
Contributor

Moving to draft until review comments have been addressed.

@ClausHolbechArista ClausHolbechArista marked this pull request as draft November 8, 2023 07:50
@github-actions github-actions bot added the state: conflict PR with conflict label Nov 21, 2023
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Copy link

Conflicts have been resolved. A maintainer will review the pull request shortly.

@emilarista emilarista marked this pull request as ready for review November 27, 2023 10:37
@gmuloc gmuloc requested a review from a team December 1, 2023 10:07
@gmuloc gmuloc requested a review from a team December 14, 2023 09:39
Copy link
Contributor

@gmuloc gmuloc left a comment

Choose a reason for hiding this comment

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

Couple of final touches - my changes will require to re-run molecule so let me do this. And I will approve!

Thanks @emilarista

@gmuloc gmuloc requested review from a team December 19, 2023 10:12
@gmuloc gmuloc requested review from a team December 19, 2023 10:37
@gmuloc gmuloc added the one approval This PR has one approval and is only missing one more. label Dec 19, 2023
Copy link

@hubertsumarno hubertsumarno left a comment

Choose a reason for hiding this comment

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

Checked and works as intended!

@gmuloc
Copy link
Contributor

gmuloc commented Jan 3, 2024

Thanks for the review @hubertsumarno
Merging this once CI finishes - Thanks for the work @emilarista !!

@gmuloc gmuloc merged commit 82ad975 into aristanetworks:devel Jan 3, 2024
41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
EPIC - AVD AutoVPN/WAN one approval This PR has one approval and is only missing one more. rn: Feat(eos_cli_config_gen) role: eos_cli_config_gen issue related to eos_cli_config_gen role state: CI Updated CI scenario have been updated in the PR state: Documentation role Updated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants