vatesfr / terraform-provider-xenorchestra

Xen Orchestra provider for Terraform
MIT License
150 stars 32 forks source link

Improve jsonrpc error handling #42

Closed ddelnano closed 4 years ago

ddelnano commented 4 years ago

The jsonrpc library that this project uses handles errors from the jsonrpc payload out of the box. Unfortunately this swallows a useful information that would make the user experience better. Here is an example of where the extra information would be useful.

While testing out #41 I intentionally supplied an invalid value for the high_availability attribute by running the following code.

data "xenorchestra_template" "template" {
    name_label = "Puppet agent - Bionic 18.04 - 1"
}

data "xenorchestra_pif" "pif" {
    device = "eth1"
    vlan = -1
}

resource "xenorchestra_vm" "bar" {
    memory_max = 256000000
    cpus  = 1
    cloud_config = "${xenorchestra_cloud_config.bar.template}"
    name_label = "Terraform test"
    template = "${data.xenorchestra_template.template.id}"
    high_availability = "does not exist"

    network {
        network_id = "${data.xenorchestra_pif.pif.network}"
    }

    disk {
      sr_id = "7f469400-4a2b-5624-cf62-61e522e50ea1"
      name_label = "Ubuntu Bionic Beaver 18.04_imavo"
      size = 10000000000
    }
}

This failed to apply for the following reason.

    testing.go:569: Step 1 error: errors during apply:

        Error: jsonrpc2: code 10 message: invalid parameters

          on /tmp/tf-test348309895/main.tf line 16:
          (source code not available)

Unfortunately this doesn't tell the user what parameter was invalid. In this case I only knew it was the high_availability field because I intentionally caused the problem.

I tcpdump'ed the traffic while causing this error so that I could see the response data. We see in the output below that the response data does include which field was problematic.

$ sudo tcpdump -Xnnvvs0  port 80 and host xoa.internal.ddelnano.com
tcpdump: listening on enp3s0, link-type EN10MB (Ethernet), capture size 262144 bytes
....
....
....
23:08:47.751344 IP (tos 0x0, ttl 64, id 8500, offset 0, flags [DF], proto TCP (6), length 311)
    192.168.88.86.80 > 192.168.88.254.41720: Flags [P.], cksum 0xbca8 (correct), seq 351:610, ack 526, win 252, options [nop,nop,TS val 683289132 ecr 827540563], length 259: HTTP
        0x0000:  4500 0137 2134 4000 4006 e5e7 c0a8 5856  E..7!4@.@.....XV
        0x0010:  c0a8 58fe 0050 a2f8 d82b 0c75 27e9 d55b  ..X..P...+.u'..[
        0x0020:  8018 00fc bca8 0000 0101 080a 28ba 2a2c  ............(.*,
        0x0030:  3153 4453 817e 00ff 7b22 6a73 6f6e 7270  1SDS.~..{"jsonrp
        0x0040:  6322 3a22 322e 3022 2c22 6964 223a 2d39  c":"2.0","id":-9
        0x0050:  3030 3731 3939 3235 3437 3430 3939 302c  007199254740990,
        0x0060:  2265 7272 6f72 223a 7b22 6d65 7373 6167  "error":{"messag
        0x0070:  6522 3a22 696e 7661 6c69 6420 7061 7261  e":"invalid.para
        0x0080:  6d65 7465 7273 222c 2263 6f64 6522 3a31  meters","code":1
        0x0090:  302c 2264 6174 6122 3a7b 2265 7272 6f72  0,"data":{"error
        0x00a0:  7322 3a5b 7b22 636f 6465 223a 6e75 6c6c  s":[{"code":null
        0x00b0:  2c22 7265 6173 6f6e 223a 2270 6174 7465  ,"reason":"patte
        0x00c0:  726e 222c 226d 6573 7361 6765 223a 226d  rn","message":"m
        0x00d0:  7573 7420 6d61 7463 6820 5b2f 5e28 6265  ust.match.[/^(be
        0x00e0:  7374 2d65 6666 6f72 747c 7265 7374 6172  st-effort|restar
        0x00f0:  747c 2924 2f5d 2c20 6275 7420 6973 2065  t|)$/],.but.is.e
        0x0100:  7175 616c 2074 6f20 5c22 7465 7374 5c22  qual.to.\"test\"
        0x0110:  222c 2270 726f 7065 7274 7922 3a22 402e  ","property":"@.
        0x0120:  6869 6768 5f61 7661 696c 6162 696c 6974  high_availabilit
        0x0130:  7922 7d5d 7d7d 7d                        y"}]}}}

Here is the data cleaned up to show what the json response was

"data":{
  "errors":[
    {
      "code":null,
      "reason":"pattern",
      "message":"must.match.[/^(best-effort|restart|)$/],.but.is.equal.to.\"test\"",
      "property":"@.high_availability"
    }
  ]
}

We need to make sure this information is reported in the terraform error output so simple fixes like this are obvious to the end user.

ddelnano commented 4 years ago

This was addressed in #43