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

smartos.vm_present could not handle nics with vrrp_vrid property #55906

Merged
merged 2 commits into from
Mar 10, 2020
Merged

smartos.vm_present could not handle nics with vrrp_vrid property #55906

merged 2 commits into from
Mar 10, 2020

Conversation

sjorge
Copy link
Contributor

@sjorge sjorge commented Jan 17, 2020

What does this PR do?

SmartOS will automatically calculate (and override) the mac property of any nics that have vrrp_vrid property set.

This causes a lot of issues:

  • we keep trying to add a new nic if the mac that was provided is incorrect
  • we keep trying to delete the existing nic with the mac based on the vrid
  • we keep trying to incorrect update a nic if the vrid was change (cannot be update, remove and add new nic)

What issues does this PR fix or reference?

n/a

Previous Behavior

We would try to add/remove/update the incorrect nic when vrrp_vrid property was present making the state module fail.

New Behavior

We now account for this in _parse_vmconfig(), this function is used to map unique identifiers for various smartos resource types (nics, disks, ...)

We will use the provided mac address from the resource key *unless the vrrp_vrid property is present, then generate the mac based on the provided vrid.

This results in the nic being correctly added, when the vrid is changes this results in a remove of the old nic and adding of the new nic as is expected by vmadm.

Tests written?

Yes, I've added tests to test the mac handling of _parse_vmconfig()

Commits signed with GPG?

No

@sjorge sjorge requested a review from a team as a code owner January 17, 2020 20:15
@ghost ghost requested a review from Ch3LL January 17, 2020 20:15
@sjorge
Copy link
Contributor Author

sjorge commented Jan 18, 2020

Tests are working local, but that was with py3... will look into py2

@sjorge
Copy link
Contributor Author

sjorge commented Jan 18, 2020

Interesting, the order is different between versions!

python3:

00:00:5e:00:01:01
00:00:5e:00:01:f0
00:22:06:00:00:01

OrderedDict([('nics', [OrderedDict([('vrrp_vrid', 1), ('vrrp_primary_ip', '12.34.5.6'), ('mac', '00:00:5e:00:01:01')]), OrderedDict([('vrrp_vrid', 240), ('vrrp_primary_ip', '12.34.5.6'), ('mac', '00:00:5e:00:01:f0')]), OrderedDict([('ips', ['12.34.5.6/24']), ('mac', '00:22:06:00:00:01')])])])

python2

00:22:06:00:00:01
00:00:5e:00:01:01
00:00:5e:00:01:f0

OrderedDict([(u'nics', [OrderedDict([(u'ips', [u'12.34.5.6/24']), (u'mac', u'00:22:06:00:00:01')]), OrderedDict([(u'vrrp_vrid', 1), (u'vrrp_primary_ip', u'12.34.5.6'), (u'mac', u'00:00:5e:00:01:01')]), OrderedDict([(u'vrrp_vrid', 240), (u'vrrp_primary_ip', u'12.34.5.6'), (u'mac', u'00:00:5e:00:01:f0')])])])

This is very weird, because the input is also an ordereddict and the order should not change, as we see on python3... I am unsure why it is changing on python2...

@Ch3LL any ideas? Have you seen this before?

EDIT: This is very very strange, I did some minor debugging. I added this to the test and it prints different for python2 and python3!

print(salt.utils.odict.OrderedDict({"zzz": "hello", "aaa": "world"}))

py2: OrderedDict([(u'aaa', u'world'), (u'zzz', u'hello')])
py3: OrderedDict([('zzz', 'hello'), ('aaa', 'world')])

I believe the result of py3 is correct?

@sjorge
Copy link
Contributor Author

sjorge commented Jan 18, 2020

I can change the tests and call the function 3 times with a seperate ret object to test... as the order is not important for this test... but I do believe OrderedDict might be broken?

@sjorge
Copy link
Contributor Author

sjorge commented Jan 18, 2020

nvm, I am dumb as a rock and incorrectly create the OrderedDict.

When using vrrp_vrid configuration option the mac address gets automatically calculated based on the value. This allows the smartos state to handle this instead of trying to remove the nic and add a new vrrp nic. (which fails)
@sjorge
Copy link
Contributor Author

sjorge commented Jan 19, 2020

Current test failures look to be for different modules?

@sjorge
Copy link
Contributor Author

sjorge commented Mar 4, 2020

@Ch3LL bump

@Ch3LL Ch3LL added the v3000.1 vulnerable version label Mar 5, 2020
@dwoz dwoz merged commit 485a47c into saltstack:master Mar 10, 2020
@sjorge sjorge deleted the smartos_vrrp branch March 13, 2020 22:05
@sagetherage sagetherage added ZRelease-Sodium retired label and removed ZRelease-Sodium retired label labels May 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v3000.1 vulnerable version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants