dhcp: T6611: add vendor support for ShoreTel IP phones #3884

bk2zsto · 2024-07-26T21:14:18Z

Change Summary

Add ShoreTel support to dhcp

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Code style update (formatting, renaming)
Refactoring (no functional changes)
Migration from an old Vyatta component to vyos-1x, please link to related PR inside obsoleted component
Other (please describe):

Related Task(s)

Related PR(s)

Component(s) name

dhcp

Proposed changes

update KEA initial conf and interface def

How to test

Smoketest result

Checklist:

I have read the CONTRIBUTING document
I have linked this PR to one or more Phabricator Task(s)
I have run the components SMOKETESTS if applicable
My commit headlines contain a valid Task id
My change requires a change to the documentation
I have updated the documentation accordingly

github-actions · 2024-07-26T21:14:40Z

❌
Commit title 'match ShoreTel camel case in interface help' does not match the required format!. Valid title example: T99999: make IPsec secure

github-actions · 2024-07-26T21:14:59Z

✅ No issues found in unused-imports check.. Please refer the workflow run

sarthurdev

Missing logic to add the option in Kea config, see here: https://github.com/vyos/vyos-1x/blob/current/python/vyos/kea.py#L94

Also please include the option in a dhcp server smoketest to validate the functionality: https://github.com/vyos/vyos-1x/blob/current/smoketest/scripts/cli/test_service_dhcp-server.py#L159

interface-definitions/include/dhcp/option-v4.xml.i

bk2zsto · 2024-07-28T18:49:28Z

I flubbed the commit message when I merged current and the build seems to be failing in unrelated areas. The smokes succeeded so I think everything is there. Should I amend the message and push again or just leave it be?

github-actions · 2024-07-28T19:10:12Z

CI integration ❌ failed!

Details

CI logs

CLI Smoketests 👍 passed
Config tests ❌ failed
RAID1 tests 👍 passed

sarthurdev · 2024-07-28T20:06:03Z

Thanks for the changes, this is looking good. Have you tested a build to ensure the option works as intended?

bk2zsto · 2024-07-28T20:46:40Z

Thanks for the changes, this is looking good. Have you tested a build to ensure the option works as intended?

Haven't gotten there yet. I'll be offline for about a week so won't be able to revisit until after that.

dmbaturin

Just a minor comment about help strings.

dmbaturin · 2024-08-02T12:31:37Z

interface-definitions/include/dhcp/option-v4.xml.i

@@ -232,6 +232,25 @@
            </leafNode>
          </children>
        </node>
+        <node name="shoretel">
+          <properties>
+            <help>Shoretel-specfic parameters</help>


I suppose we should consistently capitalize "ShoreTel" like they do in their branding.

bk2zsto · 2024-08-07T15:39:31Z

Good point. I'll merge that in, do a build and then test.

…

On Fri, Aug 2, 2024 at 8:32 AM Daniil Baturin ***@***.***> wrote: ***@***.**** requested changes on this pull request. Just a minor comment about help strings. ------------------------------ In interface-definitions/include/dhcp/option-v4.xml.i <#3884 (comment)>: > @@ -232,6 +232,25 @@ </leafNode> </children> </node> + <node name="shoretel"> + <properties> + <help>Shoretel-specfic parameters</help> I suppose we should consistently capitalize "ShoreTel" like they do in their branding. — Reply to this email directly, view it on GitHub <#3884 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA7Y6RVHQXUJGIOLFJOFP6LZPN35TAVCNFSM6AAAAABLRGBIDKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDEMJVGQ4DEMJZHA> . You are receiving this because you were assigned.Message ID: ***@***.***>

c-po · 2024-08-20T05:38:52Z

@Mergifyio rebase

mergify · 2024-08-20T05:39:04Z

rebase

✅ Branch has been successfully rebased

bk2zsto · 2024-08-29T21:53:38Z

Have been busy with other projects but for simplicity, the plan was to unconditionally send this option to all clients but an ISC KB shows the issue. It looks like ShoreTel chose 156 and defined it as a string before it was registered (cf. 1.1.1.1) but never told IANA. After that RFC 6926 defined 156 as an integer so Kea barfs on the redefinition if it is in the "dhcp4" (default) space. In the "shoretel" space it will not be sent unless an option 60 triggers it. Oddly (or totally expectedly) Windows DHCP server cares not a whit about this.

The client class stuff is relatively easy in the Kea cfg file itself but how does the top of kea-dhcp4.conf.j2 check for a shared-network that needs the class? Seems really inefficient to kea_shared_network_json, then json.loads() the result and then check for the option. OTOH not looking to unconditionally load up client classes.

I probably have the Python-fu to do a generic/abstract client class checker/handler but not sure how to do it in a VyOS-ic way. This may even be reinventing the wheel, I dont know.

This is my itch to scratch so I can take on the coding but suspect I need some design guidance. Help!

interface-definitions/include/dhcp/option-v4.xml.i

Co-authored-by: Daniil Baturin <[email protected]>

https://kb.isc.org/docs/redefining-standard-options

sarthurdev

Thanks for your changes, sorry for the lack of comments. The client-class is a great solution for the addition of these options only when needed.

Added few requests to get this PR in a state ready for merge.

sarthurdev · 2024-09-23T11:54:52Z

interface-definitions/include/dhcp/option-v4.xml.i

+                  <description>Comma-separated parameters for ShoreTel phone configuration</description>
+                </valueHelp>
+                <constraint>
+                  <validator name="shoretel-server"/>


Please remove the validator and instead just put the regex in the XML instead.

Example here: https://github.com/vyos/vyos-1x/blob/current/interface-definitions/firewall.xml.in#L14

sarthurdev · 2024-09-23T11:57:03Z

python/vyos/kea.py

@@ -114,6 +115,13 @@ def kea_parse_subnet(subnet, config):
        if 'bootfile_server' in config['option']:
            out['next-server'] = config['option']['bootfile_server']

+        if kea_client_classes(config):
+            if kea_client_classes(config,'shoretel'):


This should be optimised so the kea_client_classes is not needing to be called twice.

sarthurdev · 2024-09-23T11:57:18Z

python/vyos/kea.py

@@ -138,6 +146,13 @@ def kea_parse_subnet(subnet, config):
                if 'bootfile_server' in range_config['option']:
                    pool['next-server'] = range_config['option']['bootfile_server']

+                if kea_client_classes(config):
+                    if kea_client_classes(config,'shoretel'):


This should be optimised so the kea_client_classes is not needing to be called twice.

sarthurdev · 2024-09-23T11:57:23Z

python/vyos/kea.py

@@ -169,6 +184,13 @@ def kea_parse_subnet(subnet, config):
                if 'bootfile_server' in host_config['option']:
                    reservation['next-server'] = host_config['option']['bootfile_server']

+                if kea_client_classes(config):
+                    if kea_client_classes(config,'shoretel'):


This should be optimised so the kea_client_classes is not needing to be called twice.

sarthurdev · 2024-09-23T11:57:58Z

python/vyos/template.py

@@ -48,9 +48,9 @@ def _get_environment(location=None):
        loc_loader=FileSystemLoader(location)
    env = Environment(
        # Don't check if template files were modified upon re-rendering
-        auto_reload=False,
+        auto_reload=True,


Aren't these changes irrelevant to this PR?

sarthurdev · 2024-09-23T11:58:25Z

python/vyos/template.py

@@ -871,6 +871,32 @@ def kea_high_availability_json(config):

    return dumps(data)

+@register_filter('kea_client_classes')


As not used in any Jinja templates, this doesn't need to be registered.

sarthurdev · 2024-09-23T11:59:25Z

python/vyos/template.py

@@ -897,12 +923,20 @@ def kea_shared_network_json(shared_networks):
            if 'bootfile_server' in config['option']:
                network['next-server'] = config['option']['bootfile_server']

+            if kea_client_classes(config):
+                if kea_client_classes(config, 'shoretel'):


This should be optimised so the kea_client_classes is not needing to be called twice.

sarthurdev · 2024-09-23T11:59:29Z

python/vyos/template.py

        if 'subnet' in config:
            for subnet, subnet_config in config['subnet'].items():
                if 'disable' in subnet_config:
                    continue
                network['subnet4'].append(kea_parse_subnet(subnet, subnet_config))

+                if kea_client_classes(subnet_config):
+                    if kea_client_classes(subnet_config, 'shoretel'):


This should be optimised so the kea_client_classes is not needing to be called twice.

bk2zsto · 2024-09-27T13:40:13Z

Hi Simon, Thanks for the feedback. I am not going to be able to revisit this soon and am thinking it needs to be refactored to handle client-class generally (defining classes, match criteria, etc.). So staying under the option > vendor-option interface would be client-class with options for name and test. The test would probably need to be a simple "startswith"-type match at first. Havent thought thru this much beyond that. So ... I want to keep this hanging over my head since we want to get to 1.5 once it is certified for DellEMC but I dont want to keep it as a nuisance for anyone maintaining. Let me know if its better to close this out and start again or if I can keep it queued for myself. Thanks, Bill

…

On Mon, Sep 23, 2024 at 8:02 AM Simon ***@***.***> wrote: ***@***.**** requested changes on this pull request. Thanks for your changes, sorry for the lack of comments. The client-class is a great solution for the addition of these options only when needed. Added few requests to get this PR in a state ready for merge. ------------------------------ In interface-definitions/include/dhcp/option-v4.xml.i <#3884 (comment)>: > @@ -232,6 +232,25 @@ </leafNode> </children> </node> + <node name="shoretel"> + <properties> + <help>ShoreTel-specfic parameters</help> + </properties> + <children> + <leafNode name="shoretel-server"> + <properties> + <help>ShoreTel phone configuration</help> + <valueHelp> + <format>text</format> + <description>Comma-separated parameters for ShoreTel phone configuration</description> + </valueHelp> + <constraint> + <validator name="shoretel-server"/> Please remove the validator and instead just put the regex in the XML instead. Example here: https://github.com/vyos/vyos-1x/blob/current/interface-definitions/firewall.xml.in#L14 ------------------------------ In python/vyos/kea.py <#3884 (comment)>: > @@ -114,6 +115,13 @@ def kea_parse_subnet(subnet, config): if 'bootfile_server' in config['option']: out['next-server'] = config['option']['bootfile_server'] + if kea_client_classes(config): + if kea_client_classes(config,'shoretel'): This should be optimised so the kea_client_classes is not needing to be called twice. ------------------------------ In python/vyos/kea.py <#3884 (comment)>: > @@ -138,6 +146,13 @@ def kea_parse_subnet(subnet, config): if 'bootfile_server' in range_config['option']: pool['next-server'] = range_config['option']['bootfile_server'] + if kea_client_classes(config): + if kea_client_classes(config,'shoretel'): This should be optimised so the kea_client_classes is not needing to be called twice. ------------------------------ In python/vyos/kea.py <#3884 (comment)>: > @@ -169,6 +184,13 @@ def kea_parse_subnet(subnet, config): if 'bootfile_server' in host_config['option']: reservation['next-server'] = host_config['option']['bootfile_server'] + if kea_client_classes(config): + if kea_client_classes(config,'shoretel'): This should be optimised so the kea_client_classes is not needing to be called twice. ------------------------------ In python/vyos/template.py <#3884 (comment)>: > @@ -48,9 +48,9 @@ def _get_environment(location=None): loc_loader=FileSystemLoader(location) env = Environment( # Don't check if template files were modified upon re-rendering - auto_reload=False, + auto_reload=True, Aren't these changes irrelevant to this PR? ------------------------------ In python/vyos/template.py <#3884 (comment)>: > @@ -871,6 +871,32 @@ def kea_high_availability_json(config): return dumps(data) ***@***.***_filter('kea_client_classes') As not used in any Jinja templates, this doesn't need to be registered. ------------------------------ In python/vyos/template.py <#3884 (comment)>: > @@ -897,12 +923,20 @@ def kea_shared_network_json(shared_networks): if 'bootfile_server' in config['option']: network['next-server'] = config['option']['bootfile_server'] + if kea_client_classes(config): + if kea_client_classes(config, 'shoretel'): This should be optimised so the kea_client_classes is not needing to be called twice. ------------------------------ In python/vyos/template.py <#3884 (comment)>: > if 'subnet' in config: for subnet, subnet_config in config['subnet'].items(): if 'disable' in subnet_config: continue network['subnet4'].append(kea_parse_subnet(subnet, subnet_config)) + if kea_client_classes(subnet_config): + if kea_client_classes(subnet_config, 'shoretel'): This should be optimised so the kea_client_classes is not needing to be called twice. — Reply to this email directly, view it on GitHub <#3884 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA7Y6RRSGMBM3R57GUNPMIDZX77MFAVCNFSM6AAAAABLRGBIDKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGMRRHE3DCNZXG4> . You are receiving this because you were assigned.Message ID: ***@***.***>

sarthurdev · 2024-10-09T08:40:55Z

Let me know if its better to close this out and start again or if I can keep it queued for myself.

Feel free to get to this when you can. Thanks

bk2zsto requested a review from a team as a code owner July 26, 2024 21:14

bk2zsto requested review from dmbaturin, sarthurdev, zdc, jestabro, sever-sever and nicolas-fort July 26, 2024 21:14

github-actions bot assigned bk2zsto Jul 26, 2024

github-actions bot added the current label Jul 26, 2024

sarthurdev requested changes Jul 26, 2024

View reviewed changes

interface-definitions/include/dhcp/option-v4.xml.i Show resolved Hide resolved

dmbaturin requested changes Aug 2, 2024

View reviewed changes

github-actions bot added the rebase label Aug 8, 2024

bk2zsto added 4 commits August 20, 2024 05:39

dhcp: T6611: add vendor support for ShoreTel IP phones

3462acc

dhcp: T6611: add smoke, validator and cfg insert

99cb4b8

match ShoreTel camel case in interface help

9668c05

broken space name, typos

23fd349

c-po force-pushed the circinus_shoretel branch from 512f0cc to 23fd349 Compare August 20, 2024 05:39

dmbaturin requested changes Sep 2, 2024

View reviewed changes

interface-definitions/include/dhcp/option-v4.xml.i Outdated Show resolved Hide resolved

bk2zsto and others added 2 commits September 3, 2024 09:36

Update interface-definitions/include/dhcp/option-v4.xml.i

13513a1

Co-authored-by: Daniil Baturin <[email protected]>

Merge branch 'vyos:current' into circinus_shoretel

1bc070c

bk2zsto added 3 commits September 6, 2024 08:51

dhcp: T6611: ShoreTel options with flex_option

5bdb186

https://kb.isc.org/docs/redefining-standard-options

dhcp: T6611: fix dict_search_args call

c018ab6

dhcp: T6611: fix etwork typo

68cb56b

sarthurdev requested changes Sep 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dhcp: T6611: add vendor support for ShoreTel IP phones #3884

dhcp: T6611: add vendor support for ShoreTel IP phones #3884

bk2zsto commented Jul 26, 2024

github-actions bot commented Jul 26, 2024 •

edited

Loading

github-actions bot commented Jul 26, 2024 •

edited

Loading

sarthurdev left a comment

bk2zsto commented Jul 28, 2024

github-actions bot commented Jul 28, 2024

sarthurdev commented Jul 28, 2024

bk2zsto commented Jul 28, 2024

dmbaturin left a comment

dmbaturin Aug 2, 2024

bk2zsto commented Aug 7, 2024 via email

c-po commented Aug 20, 2024

mergify bot commented Aug 20, 2024

bk2zsto commented Aug 29, 2024

sarthurdev left a comment

sarthurdev Sep 23, 2024

sarthurdev Sep 23, 2024

sarthurdev Sep 23, 2024

sarthurdev Sep 23, 2024

sarthurdev Sep 23, 2024

sarthurdev Sep 23, 2024

sarthurdev Sep 23, 2024

sarthurdev Sep 23, 2024

bk2zsto commented Sep 27, 2024 via email

sarthurdev commented Oct 9, 2024

		@@ -871,6 +871,32 @@ def kea_high_availability_json(config):

		return dumps(data)

		@register_filter('kea_client_classes')

dhcp: T6611: add vendor support for ShoreTel IP phones #3884

Are you sure you want to change the base?

dhcp: T6611: add vendor support for ShoreTel IP phones #3884

Conversation

bk2zsto commented Jul 26, 2024

Change Summary

Types of changes

Related Task(s)

Related PR(s)

Component(s) name

Proposed changes

How to test

Smoketest result

Checklist:

github-actions bot commented Jul 26, 2024 • edited Loading

github-actions bot commented Jul 26, 2024 • edited Loading

sarthurdev left a comment

Choose a reason for hiding this comment

bk2zsto commented Jul 28, 2024

github-actions bot commented Jul 28, 2024

Details

sarthurdev commented Jul 28, 2024

bk2zsto commented Jul 28, 2024

dmbaturin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bk2zsto commented Aug 7, 2024 via email

c-po commented Aug 20, 2024

mergify bot commented Aug 20, 2024

✅ Branch has been successfully rebased

bk2zsto commented Aug 29, 2024

sarthurdev left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bk2zsto commented Sep 27, 2024 via email

sarthurdev commented Oct 9, 2024

github-actions bot commented Jul 26, 2024 •

edited

Loading

github-actions bot commented Jul 26, 2024 •

edited

Loading