Initial prototype of a XML transformation hook based on XSLT

[dmacvicar]

Sometimes the provider does not allow to set some libvirt attributes. Good examples are #350 (the provider does not allow to change the NIC model) #419 (one needs to add a touch device).

This PR explores the idea of allowing for a transformation hook before creating the domain. The hook is in the form of a XSLT stylesheet. This allow for almost unlimited capabilities when "editing" the generated XML.

provider "libvirt" {
  uri = "qemu:///system"
}

resource "libvirt_domain" "xslt-demo-domain" {
  name = "xslt-demo-domain"
  memory = "512"

  network_interface {
    network_name = "default"
  }

  xml {
    xslt = "${file("nicmodel.xsl")}"
  }
}

nicmodel.xsl:

<?xml version="1.0" ?>
<xsl:stylesheet version="1.0"
                xmlns:xsl="http://www.w3.org/1999/XSL/Transform">
  <xsl:output omit-xml-declaration="yes" indent="yes"/>
  <xsl:template match="node()|@*">
     <xsl:copy>
       <xsl:apply-templates select="node()|@*"/>
     </xsl:copy>
  </xsl:template>

  <xsl:template match="/domain/devices/interface[@type='network']/model/@type">
    <xsl:attribute name="type">
      <xsl:value-of select="'e1000'"/>
    </xsl:attribute>
  </xsl:template>

</xsl:stylesheet>

Some notes about the implementation:

• The current implementation depends on xsltproc to do the processing
• It works well for properties that are not supported by the provider. Otherwise the provider will insist in changing them back. I have an acceptance test for both cases: changing the nic model and changing the network name.
• I am not sure if it is worth to have the xsltproperty nested into an xml element.
• Only implemented for domain resources, but 95% of the code is generic. A few lines are needed in every resource creation and schema to add support for it.

The idea is of this PR is to gather some feedback and foresee problems I may not be seeing yet.

//cc @ncsurfus @unsubtleguy

TODO

• Gather feedback
• Decide on the element name
• Write some docs
• Add missing resources
• Hook the new diff function
• Do a no-op if xslt is empty

[MalloZup]

i will have a look more in details on this during the week.

Generally i think is a good idea to have this kind of custom parser for modify the resource ( for corner cases etc)
This will give User the possibility to implement their own Custom Resource if not supported, and also can give us the possibility to improve and focus on others core zone of the provider, instead of implementing/fixing issues on corner cases. 👍 ❤️ 💘

A part of this, i think as reminder we should keep in mind to take care also the following mechanism:

• updating resource.
  In the cloudinit (in the past) and also currently in networking resource, the update of resource is not rockstable.

In this case we should keep in mind that an user can modify the created initial xlst with other parameters in the file and this imho should force the recreation of the domain or other resource.

Basically:

• domain + xsltCustomA ==> terraform apply --> new resource
• user change xslt with xsltCustoB
• terraform apply -> new resource

Also as corner case , we should not have a Diff if the xlst has spaces/tab etc, so we don't force recreation of resource for nothing ( see past cloudinit bug).

I will have look more and think about it more with calm 😃

[Bischoff]

That is a great idea, and I think I might need that feature very soon! ❤️ ❤️ ❤️

[MalloZup]

sofar looks good but as corner case we might consider the update of resources

[Bischoff]

@dmacvicar What about skipping the transformation if xslt.(string) is empty (for example)?

Rationale: make transformation optional based on some other boolean. For example, in my use case, I need to leave the XML unchanged if there is no need to change the declared "hardware type".

Alternatively (more powerful, but also more complicated), we could pass parameters (xslt:param) to the XSLT stylesheet...

Note: I'm currently able to work around with code like:

    xslt = "${file(var.hardware_type ? "hwtype.xsl" : "noop.xsl")}" 

[dmacvicar]

We can make it a no-op if xslt.(string) is empty. But then you would end with:

xslt = "${var.hardware_type ? file(var.hardware_type) : ""}" 

Is that ok?

[Bischoff]

@dmacvicar yes, that was exactly the idea.

That would spare writing the noop.xslboilerplate file.

An alternate idea could be to make xslt a list. With no element, it would do a no-op. With two elements, it would run the two stylesheets in sequence. That would be useful to run different stylesheets based on different settings.

[Bischoff]

@MalloZup I am using this feature for the test suite, so I would need it packaged.

For me it is good enough in this current state, and it just works smoothly.

[dmacvicar]

I still need to do a last pass before merging. Hook the diff function and add the transformation to the other resources.

[MalloZup]

https://github.com/dmacvicar/terraform-provider-libvirt/blob/master/libvirt/resource_libvirt_cloud_init_test.go#L72
You can test in testacc that we do not have diffs see the example above

[dmacvicar]

Blocked by #459 (can't run acceptance tests locally)

[MalloZup]

i will revert the PR today ( for having like before the testacc)

#466

[dmacvicar]

I think I have addressed all points. If you are fine with the nested xml.xslt attribute, this should be ready for review or merge.

This is a very advanced feature that should not be for day to day use, but to address shortcomings of the schema and corner cases, like our teams running testsuites with it.

//cc @Bischoff

[moio]

I like the ability of having temporary fixes while an official fix is not ready yet. In general, it is a much saner approach not to have a functionality rather to have it wrong - while still providing a way out to users (C#'s initially minimalistic approach, with very slow additions only when problems and solutions are very well understood comes to mind).

I had a look at the code and could not find surprises, although I do not feel authoritative at all yet!

[Bischoff]

Thanks @dmacvicar !!!