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

Add a mac_address config validation helper #18571

Closed
wants to merge 2 commits into from

Conversation

ahayworth
Copy link
Contributor

@ahayworth ahayworth commented Nov 19, 2018

Description:

This adds a helper to verify that a given string is indeed a MAC
address. While this is possible with the regex config validation
helper, this purpose-specific helper has the following benefits:

  • You don't have to write a regex yourself!
  • Allows you to easily specify allowed MAC address separators,
    and also allows you to easily specify if lower/uppercase hex
    characters are allowed.

The latter may seem like a strange 'feature', but various platforms
specify (and require that MAC addresses be specified) in a variety
of formats. In my own personal experience, I've seen the following
be considered "valid":

  • 01:23:45:67:89:AB
  • 01:23:45:67:89:ab
  • 01.23.45.67.89.AB
  • 01.23.45.67.89.ab
  • 01-23-45-67-89-AB
  • 01-23-45-67-89-ab
  • 01 23 45 67 89 AB
  • 01 23 45 67 89 ab
  • 0123456789AB
  • 0123456789ab
  • 0123.4567.89AB
  • 0123.4567.89ab

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

This adds a helper to verify that a given string is indeed a MAC
address. While this is possible with the regex config validation
helper, this purpose-specific helper has the following benefits:

- You don't have to write a regex yourself!
- Allows you to easily specify allowed MAC address separators,
  and also allows you to easily specify if lower/uppercase hex
  characters are allowed.

The latter may seem like a strange 'feature', but various platforms
specify (and require that MAC addresses *be* specified) in a variety
of formats. In my own personal experience, I've seen the following
be considered "valid":

- 01:23:45:67:89:AB
- 01:23:45:67:89:ab
- 01.23.45.67.89.AB
- 01.23.45.67.89.ab
- 01-23-45-67-89-AB
- 01-23-45-67-89-ab
- 01 23 45 67 89 AB
- 01 23 45 67 89 ab
- 0123456789AB
- 0123456789ab
- 0123.4567.89AB
- 0123.4567.89ab
@ahayworth ahayworth requested a review from a team as a code owner November 19, 2018 04:20
@ghost ghost added the in progress label Nov 19, 2018
@ahayworth ahayworth mentioned this pull request Nov 19, 2018
7 tasks
@@ -474,6 +474,60 @@ def validator(config):
return validator


def is_mac_address(value=None, separators=None, allow_lowercase=True,
Copy link
Member

Choose a reason for hiding this comment

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

This method seems overkill and is not in line with our other config validation. What we do instead is that we pick a format and normalize everything to be that specific format. A method to format mac addresses is already available here and could be moved to config_validation.

Also this method does weird things by accepting value (unused) and raising Invalid (but that is raised outside of the scope of actual validating config)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@balloob I think you're right - I reflected on it this morning and it does indeed feel like overkill. I'll close this and reconfigure the PR that uses it to use other validators. I don't think normalizing MAC addresses in the whole codebase is something I could do soon, though. 😄

FWIW - The reason value was accepted was because it seems to be passed implicitly by voluptuous when you call one of these helpers. Without it (leaving separators=None as the first parameter), you'd end up with weird behavior. For example, calling cv.is_mac_address without arguments would end up calling cv.is_mac_address(separator="whatever you wanted to validate"), and your regular expression would be ^[0-9a-zA-Z]{2}[whatever you wanted to validate]{1}[0-9a-zA-Z{2}[whatever you wanted to validate]{1} .... It was odd.

In any case, thank you for the feedback! 😄

@ahayworth ahayworth closed this Nov 19, 2018
@ghost ghost removed the in progress label Nov 19, 2018
@ahayworth
Copy link
Contributor Author

Closing per @balloob's feedback!

@ahayworth ahayworth deleted the ahayworth-mac-config branch November 19, 2018 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants