Support specifying mac address without quotation mark #364

liangwen12year · 2021-03-17T15:50:49Z

Signed-off-by: Wen Liang [email protected]

thom311 · 2021-03-18T07:08:20Z

can you show an example how this is used?

What prompted this merge-request, is there an issue that you can link to?

   validator._validate_impl(41135085296, "mac"), "52:54:00:12:34:56"

Why does 41135085296 correspond to "52:54:00:12:34:56", or better, where did you get this (to me unknown) algorithm with divmod(num, 60)? That doesn't make sense to me.

liangwen12year · 2021-03-18T12:48:36Z

can you show an example how this is used?

What prompted this merge-request, is there an issue that you can link to?
   validator._validate_impl(41135085296, "mac"), "52:54:00:12:34:56"
Why does 41135085296 correspond to "52:54:00:12:34:56", or better, where did you get this (to me unknown) algorithm with divmod(num, 60)? That doesn't make sense to me.

The issue:
#365

module_utils/network_lsr/argument_validator.py

thom311 · 2021-03-18T13:15:22Z

The issue:
#365

Btw, would have been good to link the background information upfront.

That doesn't seem a good idea. It allows nonsense like:

   mac: 1

which IMO should be rejected as error.

Also, if you read https://stackoverflow.com/a/52732369/354393 you'll see that number larger or equal than 60 are treated as carrier to the next digit. So, I would guess that "00:00:00:00:60" is the same as "00:00:00:01:00", which seems very wrong.

thom311 · 2021-03-18T13:55:45Z

Also, if you read https://stackoverflow.com/a/52732369/354393 you'll see that number larger or equal than 60 are treated as carrier to the next digit. So, I would guess that "00:00:00:00:60" is the same as "00:00:00:01:00", which seems very wrong.

Ah wrong, it doesn't carry over. The link says "Since your other MAC-address, 84:19:14:15:86:58, has a number after the first number that is over 59 (86 that is), it is not considered as a sexagesimal number and is therefore treated literally as a string.".

But that seems not something that the role should try to work around. The user has to set the MAC address as string and not the role interpreting integers as hours:minutes:seconds...

liangwen12year · 2021-03-18T17:16:03Z

The issue:
#365

Btw, would have been good to link the background information upfront.

That doesn't seem a good idea. It allows nonsense like:
   mac: 1
which IMO should be rejected as error.

Also, if you read https://stackoverflow.com/a/52732369/354393 you'll see that number larger or equal than 60 are treated as carrier to the next digit. So, I would guess that "00:00:00:00:60" is the same as "00:00:00:01:00", which seems very wrong.

mac: 1 was indeed rejected as error by the function num_to_mac()

richm · 2021-03-18T17:30:48Z

Are you absolutely guaranteed that you can always convert a mac address specified like mac: 52:54:00:12:34:56 into a unique number, and that number can always be converted back into the same exact mac address specified in the original YAML?
If you are not absolutely guaranteed, then you will just have to require users to specify the mac address as a string in any of the various ways to specify a string in YAML.

liangwen12year · 2021-03-18T17:52:49Z

Also, if you read https://stackoverflow.com/a/52732369/354393 you'll see that number larger or equal than 60 are treated as carrier to the next digit. So, I would guess that "00:00:00:00:60" is the same as "00:00:00:01:00", which seems very wrong.

Ah wrong, it doesn't carry over. The link says "Since your other MAC-address, 84:19:14:15:86:58, has a number after the first number that is over 59 (86 that is), it is not considered as a sexagesimal number and is therefore treated literally as a string.".

But that seems not something that the role should try to work around. The user has to set the MAC address as string and not the role interpreting integers as hours:minutes:seconds...

Are you absolutely guaranteed that you can always convert a mac address specified like mac: 52:54:00:12:34:56 into a unique number, and that number can always be converted back into the same exact mac address specified in the original YAML?
If you are not absolutely guaranteed, then you will just have to require users to specify the mac address as a string in any of the various ways to specify a string in YAML.

the function num_to_mac() literally is to remediate yaml's peculiarity that "string literal consists of a series of one or two-digit numbers delimited by colons and all numbers but the first are between 0 and 59, it would be interpreted as a sexagesimal number, and would be converted automatically to the equivalent number of seconds". I am guaranteed that the function num_to_mac() will always convert it back into mac address specified in the original YAML. Also, I understand the concern from @thom311, like specifying arbitrary integer( e.g. mac: 1), but we also have force_len to raise error for invalid interger value

liangwen12year · 2021-03-18T18:01:56Z

Are you absolutely guaranteed that you can always convert a mac address specified like mac: 52:54:00:12:34:56 into a unique number, and that number can always be converted back into the same exact mac address specified in the original YAML?
If you are not absolutely guaranteed, then you will just have to require users to specify the mac address as a string in any of the various ways to specify a string in YAML.

And it is very good suggestion that always remind users to specify the mac address as a string in any of the various ways to specify a string in YAML.

README.md

module_utils/network_lsr/argument_validator.py

tests/unit/test_network_connections.py

cathay4t

Besides the magic number, this PR is breaking the API!

Previously, the unquoted mac address works well when mac address contains [a-f], but your patch change it to raise exception.

Your title says you are supporting unquoted mac address, but the code looks like you are raise exception for it.

module_utils/network_lsr/argument_validator.py

liangwen12year · 2021-04-02T21:57:55Z

Besides the magic number, this PR is breaking the API!

Previously, the unquoted mac address works well when mac address contains [a-f], but your patch change it to raise exception.

Your title says you are supporting unquoted mac address, but the code looks like you are raise exception for it.

For the unquoted mac address containing [a-f], it will not raise an exception, it will work as before. And the code will only support valid unquoted mac address, will raise an exception for invalid unquoted mac address.

cathay4t · 2021-04-30T04:30:37Z

module_utils/network_lsr/argument_validator.py

@@ -11,6 +11,9 @@
 from ansible.module_utils.network_lsr.utils import Util  # noqa:E501

 UINT32_MAX = 0xFFFFFFFF
+# a MAC address for type ethernet requires 6 octets
+# a MAC address for type infiniband requires 20 octets
+MAC_ADDR_OCTETS = [6, 20]


This is wrong.

The kernel has no limitation minimum MAC address length.
And the maximum is not 20, but in /usr/include/linux/netdevice.h:

#define MAX_ADDR_LEN 32 /* Largest hardware address length */

That is 32 octets and 64 hex string characters.

The reason that I define MAC_ADDR_OCTETS = [6, 20] is because I saw the validation check here,
https://github.com/linux-system-roles/network/blob/main/module_utils/network_lsr/argument_validator.py#L1483

And in NM,it was also clear indicated,

* ---ifcfg-rh--- * property: mac-address * variable: HWADDR * description: IBoIP 20-byte hardware address of the device (in traditional * hex-digits-and-colons notation). * Note that for initscripts this is the current MAC address of the device as found * during ifup. For NetworkManager this is the permanent MAC address. Or in case no * permanent MAC address exists, the MAC address initially configured on the device. * example: HWADDR=01:02:03:04:05:06:07:08:09:0A:01:02:03:04:05:06:07:08:09:11 * ---end--- /* ---keyfile--- * property: mac-address * format: usual hex-digits-and-colons notation * description: MAC address in traditional hex-digits-and-colons notation * (e.g. 00:22:68:12:79:A2), or semicolon separated list of 6 bytes (obsolete) * (e.g. 0;34;104;18;121;162) * ---end---

liangwen12year · 2021-05-02T01:18:50Z

@liangwen12year This PR is still in DRAFT state. And the git commit has no commit comment explaining why patch is required and how this patch is fixing the issue.

Added commit message to explain why patch is required and how this patch is fixing the issue.

jharuda · 2021-05-03T16:37:05Z

[citest pending]

- In yaml, all the string value consists of a series of one-digit or two-digit numbers delimited by colons and all numbers but the first are between 0 and 59, it would be interpreted as a sexagesimal number, and would be converted automatically to the equivalent number of seconds. - The committed patch will guarantee to remediate the yaml's peculiarity and convert back into the mac address specified in the original yaml file. And in case that user specifies arbitrary integer(e.g. `mac:1234`), we introduce `force_len`(e.g. `MAC_ADDR_OCTETS = [6, 20]`) to raise error for invalid integer value. Signed-off-by: Wen Liang <[email protected]>

coveralls · 2021-05-04T11:34:57Z

Pull Request Test Coverage Report for Build 809885115

0 of 0 changed or added relevant lines in 0 files are covered.
103 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.7%) to 40.22%

Files with Coverage Reduction	New Missed Lines	%
/home/runner/work/network/network/module_utils/network_lsr/utils.py	14	54.48%
/home/runner/work/network/network/module_utils/network_lsr/argument_validator.py	89	83.71%

Totals
Change from base Build 809105682:	0.7%
Covered Lines:	1024
Relevant Lines:	2546

💛 - Coveralls

richm · 2021-05-04T19:08:34Z

[citest pending]

richm · 2021-05-04T19:34:46Z

[citest bad]

richm · 2021-05-04T23:14:46Z

[citest pending]

cathay4t · 2021-05-07T07:04:52Z

@liangwen12year This PR is still in DRAFT mode. Is it ready for review?

liangwen12year · 2021-05-07T12:23:08Z

@liangwen12year This PR is still in DRAFT mode. Is it ready for review?

It is still in DRAFT mode, because we need to discuss first if this patch is actually needed.

stale · 2021-06-06T13:30:06Z

Thank you for your contribution! There was no activity in this pull request recently. To avoid pull requests to pile up, an automated process marked this pull request as stale. It will close this pull request if no further activity occurs. The current policy is available at: https://github.com//linux-system-roles/network/blob/main/.github/stale.yml

richm · 2021-06-14T21:28:58Z

[citest pending]

richm · 2021-07-06T18:01:29Z

any update on this?

tyll · 2021-07-06T18:42:08Z

This is currently not a priority, therefore it is just a draft PR. We can revisit this after #363 are handled and the current CI problems are handled. Would you prefer to have the PR closed for now?

richm · 2021-07-06T19:00:42Z

This is currently not a priority, therefore it is just a draft PR. We can revisit this after #363 are handled and the current CI problems are handled. Would you prefer to have the PR closed for now?

No, this is fine - it is marked as WIP/Draft

stale · 2021-08-05T19:41:39Z

Thank you for your contribution! There was no activity in this pull request recently. To avoid pull requests to pile up, an automated process marked this pull request as stale. It will close this pull request if no further activity occurs. The current policy is available at: https://github.com//linux-system-roles/network/blob/main/.github/stale.yml

stale · 2021-09-04T22:29:20Z

Thank you for your contribution! There was no activity in this pull request recently. To avoid pull requests to pile up, an automated process marked this pull request as stale. It will close this pull request if no further activity occurs. The current policy is available at: https://github.com//linux-system-roles/network/blob/main/.github/stale.yml

liangwen12year force-pushed the mac branch 2 times, most recently from a5dfe08 to 11e8424 Compare March 18, 2021 02:16

liangwen12year commented Mar 18, 2021

View reviewed changes

module_utils/network_lsr/argument_validator.py Outdated Show resolved Hide resolved

liangwen12year requested a review from tyll March 18, 2021 13:15

tyll linked an issue Mar 18, 2021 that may be closed by this pull request

Need to convert number back into mac address because of yaml's peculiarity #365

Open

liangwen12year force-pushed the mac branch 2 times, most recently from f06095e to 1f423c0 Compare March 18, 2021 20:59

tyll reviewed Mar 22, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

tyll reviewed Mar 22, 2021

View reviewed changes

module_utils/network_lsr/argument_validator.py Outdated Show resolved Hide resolved

tyll reviewed Mar 22, 2021

View reviewed changes

tests/unit/test_network_connections.py Outdated Show resolved Hide resolved

tyll reviewed Mar 22, 2021

View reviewed changes

tests/unit/test_network_connections.py Show resolved Hide resolved

liangwen12year force-pushed the mac branch 5 times, most recently from 1fca175 to 8859ad1 Compare March 24, 2021 01:09

cathay4t added the Wait_Review label Apr 1, 2021

liangwen12year changed the title ~~[WIP]Support specifying mac address without quotation mark~~ Support specifying mac address without quotation mark Apr 1, 2021

cathay4t requested changes Apr 2, 2021

View reviewed changes

module_utils/network_lsr/argument_validator.py Outdated Show resolved Hide resolved

cathay4t added Wait_Submitter The submitter needs to do something before this can move on and removed Wait_Review labels Apr 2, 2021

cathay4t added Wait_Submitter The submitter needs to do something before this can move on and removed Wait_Review labels Apr 30, 2021

cathay4t requested changes Apr 30, 2021

View reviewed changes

liangwen12year force-pushed the mac branch 2 times, most recently from 223ca16 to 48706ab Compare May 2, 2021 01:17

liangwen12year removed the Wait_Submitter The submitter needs to do something before this can move on label May 2, 2021

liangwen12year force-pushed the mac branch from 48706ab to e8734c3 Compare May 4, 2021 11:32

cathay4t added the Work_Ongoing label May 7, 2021

stale bot added the stale label Jun 6, 2021

stale bot removed the stale label Jun 14, 2021

stale bot added the stale label Aug 5, 2021

cathay4t removed the stale label Aug 5, 2021

stale bot added the stale label Sep 4, 2021

stale bot closed this Sep 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support specifying mac address without quotation mark #364

Support specifying mac address without quotation mark #364

liangwen12year commented Mar 17, 2021

thom311 commented Mar 18, 2021

liangwen12year commented Mar 18, 2021

thom311 commented Mar 18, 2021

thom311 commented Mar 18, 2021

liangwen12year commented Mar 18, 2021

richm commented Mar 18, 2021

liangwen12year commented Mar 18, 2021 •

edited

Loading

liangwen12year commented Mar 18, 2021

cathay4t left a comment

liangwen12year commented Apr 2, 2021 •

edited

Loading

cathay4t Apr 30, 2021

liangwen12year May 1, 2021

liangwen12year commented May 2, 2021

jharuda commented May 3, 2021

coveralls commented May 4, 2021 •

edited

Loading

richm commented May 4, 2021

richm commented May 4, 2021

richm commented May 4, 2021

cathay4t commented May 7, 2021

liangwen12year commented May 7, 2021 •

edited

Loading

stale bot commented Jun 6, 2021

richm commented Jun 14, 2021

richm commented Jul 6, 2021

tyll commented Jul 6, 2021

richm commented Jul 6, 2021

stale bot commented Aug 5, 2021

stale bot commented Sep 4, 2021

Support specifying mac address without quotation mark #364

Support specifying mac address without quotation mark #364

Conversation

liangwen12year commented Mar 17, 2021

thom311 commented Mar 18, 2021

liangwen12year commented Mar 18, 2021

thom311 commented Mar 18, 2021

thom311 commented Mar 18, 2021

liangwen12year commented Mar 18, 2021

richm commented Mar 18, 2021

liangwen12year commented Mar 18, 2021 • edited Loading

liangwen12year commented Mar 18, 2021

cathay4t left a comment

Choose a reason for hiding this comment

liangwen12year commented Apr 2, 2021 • edited Loading

cathay4t Apr 30, 2021

Choose a reason for hiding this comment

liangwen12year May 1, 2021

Choose a reason for hiding this comment

liangwen12year commented May 2, 2021

jharuda commented May 3, 2021

coveralls commented May 4, 2021 • edited Loading

Pull Request Test Coverage Report for Build 809885115

💛 - Coveralls

richm commented May 4, 2021

richm commented May 4, 2021

richm commented May 4, 2021

cathay4t commented May 7, 2021

liangwen12year commented May 7, 2021 • edited Loading

stale bot commented Jun 6, 2021

richm commented Jun 14, 2021

richm commented Jul 6, 2021

tyll commented Jul 6, 2021

richm commented Jul 6, 2021

stale bot commented Aug 5, 2021

stale bot commented Sep 4, 2021

liangwen12year commented Mar 18, 2021 •

edited

Loading

liangwen12year commented Apr 2, 2021 •

edited

Loading

coveralls commented May 4, 2021 •

edited

Loading

liangwen12year commented May 7, 2021 •

edited

Loading