Skip to content

Summary: Add 'network-config' support in ovf-env.xml through VMware g…#947

Merged
TheRealFalcon merged 2 commits into
canonical:mainfrom
PengpengSun:PengpengSun-OVF-read-network-config-from-vmware-guestinfo
Jul 20, 2021
Merged

Summary: Add 'network-config' support in ovf-env.xml through VMware g…#947
TheRealFalcon merged 2 commits into
canonical:mainfrom
PengpengSun:PengpengSun-OVF-read-network-config-from-vmware-guestinfo

Conversation

@PengpengSun
Copy link
Copy Markdown
Contributor

…uestinfo.ovfEnv

Details:

  1. Support guest set network config through guestinfo.ovfEnv using OVF
  2. 'network-config' Property is optional
  3. 'network-config' Property's value has to be base64 encoded

Added unittests and updated ovf-env.xml example

Test Steps

  1. Create a ovf-env.xml which includes 'network-config' Property
  2. Set 'network-config' Property's value to base64 encoded network config
  3. Load ovf-env.xml to VMware guestinfo.ovfEnv
  4. Verify cloud-init can get network config from guestinfo.ovfEnv in OVF

Checklist:

  • My code follows the process laid out in the documentation
  • I have updated or added any unit tests accordingly
  • I have updated or added any documentation accordingly

…uestinfo.ovfEnv

Details:
1. Support guest set network config through guestinfo.ovfEnv using OVF
2. 'network-config' Property is optional
3. 'network-config' Property's value has to be base64 encoded

Added unittests and updated ovf-env.xml example
Copy link
Copy Markdown
Contributor

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@TheRealFalcon TheRealFalcon merged commit f0ab1e6 into canonical:main Jul 20, 2021
break
if contents:
(md, ud, cfg) = read_ovf_environment(contents)
read_network = ('com.vmware.guestinfo' == name)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this not have been com.vmware.guestInfo (different capitalization) the same as in line 353 above?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also why is this not supposed to work with the ISO transport?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@t-8ch Thanks for catching this. I should add more tests on this change. Will file a PR to fix it and add tests.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@t-8ch For ISO transport, It can be supported, but user need create ISO for each instance with specified network configuration, I don't see it's a common user case when I opened this PR.
And com.vmware.guestInfo is VMware platform specified, I get this request from inside of VMware, the guestinfo.ovfEnv can be updated easily from outside of instance(vm).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would not VMware itself be able to generate the ISO on demand, at least when specifying the ISO transport in VMware?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, AFAIK, ISO can be generated on demand on VMware platform, till now, network config is never included in such ISO. The ISO transport is not VMware specified, if there is indeed such requirement, I can make the change and let cloud-init team review it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@PengpengSun Thanks for your patch!

Personally I would like to see the network-config also be supported via the ISO.
It should not necessarily change more oftern than the user-data .

Also the ISO transport is defined directly by the upstream DMTF standard.
(See https://www.dmtf.org/sites/default/files/standards/documents/DSP0243_2.1.1.pdf , 11.2 Transport media type)

If you prefer I can also send a patch.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@t-8ch Please fell free to send the patch, I was hesitating to add ISO transport because it's not specified to VMware platform. Let's see if cloud-init team is ok with it too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants