Fix duplicate check with different mac format #397

fossedihelm · 2023-10-24T09:45:44Z

A macAddress can be specified with different separators:

":" e.g. "02:00:00:00:ff:ff"
"-" e.g. "02-00-00-00-ff-ff"
"." e.g. "0200.0000.ffff"

Currently, the duplicate mac check works only if the two compared mac address use the same separator.
The duplicate check is made looking at the existence of key of the macMap. The keys of the mapMap are the macAddress string.

With this commit, a macKey struct is introduced. The latter stores the normalized string of the macAddress and can be used as unique key and identifier of a macAddress.

What this PR does / why we need it:

Special notes for your reviewer:
Fixes https://issues.redhat.com/browse/CNV-30974

Release note:

Fix duplicate check with different mac address separator

kubevirt-bot · 2023-10-24T09:45:55Z

Hi @fossedihelm. Thanks for your PR.

I'm waiting for a k8snetworkplumbingwg member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

fossedihelm · 2023-10-25T07:35:54Z

@RamLavi Can you have a look please? Thanks

maiqueb

This PR is required but IMO it doesn't do enough since there are other supported types of format for MAC addresses:

canonical (- separator, which your PR adds support for)
hexstring (: separator, the only thing currently accepted by kubemacpool)
dot notation (. separator, in 4 char segments - e.g. 0000.5e00.5301)
hexadecimal

Maybe the correct thing to do is to create a MAC address - net.HardwareAddr - using net.ParseMAC and then get the string from it (whatever that is) ?

kubevirt-bot · 2023-10-26T10:18:59Z

@maiqueb: changing LGTM is restricted to collaborators

In response to this:

This PR is required but IMO it doesn't do enough since there are other supported types of format for MAC addresses:

canonical (- separator, which your PR adds support for)

hexstring (: separator, the only thing currently accepted by kubemacpool)

dot notation (. separator, in 4 char segments - e.g. 0000.5e00.5301)

hexadecimal

Maybe the correct thing to do is to create a MAC address - net.HardwareAddr - using net.ParseMAC and then get the string from it (whatever that is) ?

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

fossedihelm · 2023-10-26T11:24:29Z

Maybe the correct thing to do is to create a MAC address - net.HardwareAddr - using net.ParseMAC and then get the string from it (whatever that is) ?

@maiqueb That's a great idea! IMHO we can also skip the err from ParseMAC function, since it has already been validated. WDYT?

maiqueb · 2023-10-26T11:26:53Z

Maybe the correct thing to do is to create a MAC address - net.HardwareAddr - using net.ParseMAC and then get the string from it (whatever that is) ?

@maiqueb That's a great idea! IMHO we can also skip the err from ParseMAC function, since it has already been validated. WDYT?

Fine with me. I mean, there's nothing wrong in being redundant - checking against nil won't take long - but if you're sure that was already validated, all good. I just find it more readable / comforting to see the (ugly) golang pattern of if err != nil ; return err . It's up to the maintainer.

maiqueb

Looks good !

kubevirt-bot · 2023-10-26T12:39:37Z

@maiqueb: changing LGTM is restricted to collaborators

In response to this:

Looks good !

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

fossedihelm · 2023-10-26T12:42:59Z

Fine with me. I mean, there's nothing wrong in being redundant - checking against nil won't take long - but if you're sure that was already validated, all good. I just find it more readable / comforting to see the (ugly) golang pattern of if err != nil ; return err . It's up to the maintainer.

Yes, I generally agree! In this specific case, handling the error means to create something like this:

func NewMacKey(macAddressDashes string) (macKey, error) {
	macAddress, err := net.ParseMAC(macAddressDashes)
        if err != nil{
            return macKey{}, err
        }
	return macKey{normalizedMacAddress: macAddress.String()}, nil
}

This is not a problem at all, but this will not allow us to create inline entry in the macMap.
And the latter is very useful in tests, in addition to the fact that we should clearly always handle the return error.

maiqueb · 2023-10-26T12:47:33Z

Fine with me. I mean, there's nothing wrong in being redundant - checking against nil won't take long - but if you're sure that was already validated, all good. I just find it more readable / comforting to see the (ugly) golang pattern of if err != nil ; return err . It's up to the maintainer.

Yes, I generally agree! In this specific case, handling the error means to create something like this:
func NewMacKey(macAddressDashes string) (macKey, error) {
	macAddress, err := net.ParseMAC(macAddressDashes)
        if err != nil{
            return macKey{}, err
        }
	return macKey{normalizedMacAddress: macAddress.String()}, nil
}
This is not a problem at all, but this will not allow us to create inline entry in the macMap. And the latter is very useful in tests, in addition to the fact that we should clearly always handle the return error.

Maybe it's best to add a comment to the NewMacKey func indicating it's inputs must be already validated MAC addresses ? And indicate all it does is parse from N mac address formats to "our" definition of canonical format.

This is just a nit. Consider adding it if someone else asks you to change the code.

fossedihelm · 2023-10-26T12:50:41Z

Maybe it's best to add a comment to the NewMacKey func indicating it's inputs must be already validated MAC addresses ? And indicate all it does is parse from N mac address formats to "our" definition of canonical format.

This is just a nit. Consider adding it if someone else asks you to change the code.

I put this just before the ParseMAC func:

// we can ignore the error here since the string value has already been validated

but yes, I can insert the description above the func! Thanks, will do!

fossedihelm · 2023-10-26T12:58:21Z

@maiqueb PTAL

kubevirt-bot · 2023-10-26T13:46:53Z

@maiqueb: changing LGTM is restricted to collaborators

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

RamLavi · 2023-10-29T08:52:54Z

/ok-to-test

RamLavi

Hey @fossedihelm, thanks for your contribution!
Please see comments added.

Also, note the e2e test failing is concerning - and suggests that we missed something.. If you need help ping me

pkg/pool-manager/macentry_test.go

pkg/pool-manager/pod_pool.go

pkg/pool-manager/pool.go

pkg/pool-manager/virtualmachine_pool.go

macPoolMap provides a save remove entry function that could always be preferred respect the directly `delete` function. Signed-off-by: fossedihelm <ffossemo@redhat.com>

RamLavi · 2023-10-30T10:15:50Z

/hold

let's merge only after stable branches are introduced to KMP

fossedihelm · 2023-10-30T10:19:42Z

Thanks @RamLavi for the review!
I am working on the relevant failing e2e tests. Thank you!

fossedihelm · 2023-10-30T11:36:16Z

Changes:

Addressed comments
Fixed failing e2e test:
-- there were a mixed comparison between iface.MacAddress and macKey.String()
-- the first one was upper case, the latter in lower case.
Added MarshalJSON() ([]byte, error) to macMap struct to allow logger to correctly print the map

RamLavi

thanks for the changes!
Big like for the Marshal function added.

Added one more comment.

pkg/pool-manager/macpoolmap.go

pkg/pool-manager/mackey.go

fossedihelm · 2023-10-30T12:56:45Z

thanks for the changes! Big like for the Marshal function added.

Added one more comment.

Thanks @RamLavi for the great suggestion! I addressed the commets. PTAL

RamLavi

last one :)

pkg/pool-manager/virtualmachine_pool.go

As per https://pkg.go.dev/net#ParseMAC, a macAddress can be specified as an IEEE 802 MAC-48, EUI-48, EUI-64, or a 20-octet IP using the following formats: - 00:00:5e:00:53:01 - 02:00:5e:10:00:00:00:01 - 00:00:00:00:fe:80:00:00:00:00:00:00:02:00:5e:10:00:00:00:01 - 00-00-5e-00-53-01 - 02-00-5e-10-00-00-00-01 - 00-00-00-00-fe-80-00-00-00-00-00-00-02-00-5e-10-00-00-00-01 - 0000.5e00.5301 - 0200.5e10.0000.0001 - 0000.0000.fe80.0000.0000.0000.0200.5e10.0000.0001 Currently, the duplicate mac check works only if the two compared mac address use the same separator. The duplicate check is made looking at the existence of key of the macMap. The keys of the mapMap are the macAddress string. With this commit, a `macKey` struct is introduced. The latter stores the normalized string of the macAddress and can be used as unique key and identifier of a macAddress. NB: Currently KubeVirt does not allow EUI-64 and 20-octet IP mac addresses. Signed-off-by: fossedihelm <ffossemo@redhat.com>

RamLavi

/lgtm
/approve

@fossedihelm does this change need to be backported?
If not, then I'll remove the old after the CVE storm passes and I add the appropriate stable branches...

kubevirt-bot · 2023-10-31T09:48:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RamLavi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [RamLavi]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

RamLavi · 2023-10-31T18:58:50Z

/hold cancel

RamLavi · 2023-11-01T08:23:48Z

/cherry-pick release-0.42

kubevirt-bot · 2023-11-01T08:24:42Z

@RamLavi: new pull request created: #407

In response to this:

/cherry-pick release-0.42

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

kubevirt-bot added release-note dco-signoff: yes needs-ok-to-test labels Oct 24, 2023

kubevirt-bot added the size/L label Oct 24, 2023

kubevirt-bot requested review from phoracek and qinqon October 24, 2023 09:46

maiqueb suggested changes Oct 26, 2023

View reviewed changes

fossedihelm force-pushed the fix_duplicate_check branch from aaa41c8 to c4f5bff Compare October 26, 2023 12:36

maiqueb approved these changes Oct 26, 2023

View reviewed changes

fossedihelm force-pushed the fix_duplicate_check branch from c4f5bff to 2479b07 Compare October 26, 2023 12:57

maiqueb approved these changes Oct 26, 2023

View reviewed changes

kubevirt-bot added ok-to-test and removed needs-ok-to-test labels Oct 29, 2023

RamLavi requested changes Oct 29, 2023

View reviewed changes

pkg/pool-manager/macentry_test.go Show resolved Hide resolved

pkg/pool-manager/pod_pool.go Show resolved Hide resolved

pkg/pool-manager/pool.go Outdated Show resolved Hide resolved

pkg/pool-manager/virtualmachine_pool.go Show resolved Hide resolved

kubevirt-bot assigned RamLavi Oct 29, 2023

Use removeMacEntry function instead of directly delete

085785c

macPoolMap provides a save remove entry function that could always be preferred respect the directly `delete` function. Signed-off-by: fossedihelm <ffossemo@redhat.com>

kubevirt-bot added the do-not-merge/hold label Oct 30, 2023

fossedihelm force-pushed the fix_duplicate_check branch from 2479b07 to 447fd66 Compare October 30, 2023 11:29

RamLavi requested changes Oct 30, 2023

View reviewed changes

pkg/pool-manager/macpoolmap.go Outdated Show resolved Hide resolved

pkg/pool-manager/mackey.go Outdated Show resolved Hide resolved

fossedihelm force-pushed the fix_duplicate_check branch from 447fd66 to 193b9a1 Compare October 30, 2023 12:55

RamLavi requested changes Oct 30, 2023

View reviewed changes

pkg/pool-manager/virtualmachine_pool.go Outdated Show resolved Hide resolved

fossedihelm force-pushed the fix_duplicate_check branch from 193b9a1 to 9e0c087 Compare October 30, 2023 13:20

RamLavi approved these changes Oct 31, 2023

View reviewed changes

kubevirt-bot added the lgtm label Oct 31, 2023

kubevirt-bot added the approved label Oct 31, 2023

kubevirt-bot removed the do-not-merge/hold label Oct 31, 2023

kubevirt-bot merged commit 69258d0 into k8snetworkplumbingwg:main Oct 31, 2023
3 of 4 checks passed

kubevirt-bot mentioned this pull request Nov 1, 2023

[release-0.42] Fix duplicate check with different mac format #407

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix duplicate check with different mac format #397

Fix duplicate check with different mac format #397

fossedihelm commented Oct 24, 2023 •

edited

kubevirt-bot commented Oct 24, 2023

fossedihelm commented Oct 25, 2023

maiqueb left a comment

kubevirt-bot commented Oct 26, 2023

fossedihelm commented Oct 26, 2023

maiqueb commented Oct 26, 2023

maiqueb left a comment

kubevirt-bot commented Oct 26, 2023

fossedihelm commented Oct 26, 2023 •

edited

maiqueb commented Oct 26, 2023

fossedihelm commented Oct 26, 2023

fossedihelm commented Oct 26, 2023

kubevirt-bot commented Oct 26, 2023

RamLavi commented Oct 29, 2023

RamLavi left a comment •

edited

RamLavi commented Oct 30, 2023

fossedihelm commented Oct 30, 2023

fossedihelm commented Oct 30, 2023

RamLavi left a comment

fossedihelm commented Oct 30, 2023

RamLavi left a comment

RamLavi left a comment

kubevirt-bot commented Oct 31, 2023

RamLavi commented Oct 31, 2023

RamLavi commented Nov 1, 2023

kubevirt-bot commented Nov 1, 2023

Fix duplicate check with different mac format #397

Fix duplicate check with different mac format #397

Conversation

fossedihelm commented Oct 24, 2023 • edited

kubevirt-bot commented Oct 24, 2023

fossedihelm commented Oct 25, 2023

maiqueb left a comment

Choose a reason for hiding this comment

kubevirt-bot commented Oct 26, 2023

fossedihelm commented Oct 26, 2023

maiqueb commented Oct 26, 2023

maiqueb left a comment

Choose a reason for hiding this comment

kubevirt-bot commented Oct 26, 2023

fossedihelm commented Oct 26, 2023 • edited

maiqueb commented Oct 26, 2023

fossedihelm commented Oct 26, 2023

fossedihelm commented Oct 26, 2023

kubevirt-bot commented Oct 26, 2023

RamLavi commented Oct 29, 2023

RamLavi left a comment • edited

Choose a reason for hiding this comment

RamLavi commented Oct 30, 2023

fossedihelm commented Oct 30, 2023

fossedihelm commented Oct 30, 2023

RamLavi left a comment

Choose a reason for hiding this comment

fossedihelm commented Oct 30, 2023

RamLavi left a comment

Choose a reason for hiding this comment

RamLavi left a comment

Choose a reason for hiding this comment

kubevirt-bot commented Oct 31, 2023

RamLavi commented Oct 31, 2023

RamLavi commented Nov 1, 2023

kubevirt-bot commented Nov 1, 2023

fossedihelm commented Oct 24, 2023 •

edited

fossedihelm commented Oct 26, 2023 •

edited

RamLavi left a comment •

edited