[HPR-1157] HCP Webhook resource

[sylviamoss]

🛠️ Description

🏗️ Acceptance tests

• Are there any feature flags that are required to use this functionality?
• Have you added an acceptance test for the functionality being added?
• Have you run the acceptance tests on this branch?

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccNotificationsWebhookResource -test.v'
==> Checking that code complies with gofmt requirements...
golangci-lint run --config ./golangci-config.yml 
TF_ACC=1 go test ./internal/... -v -run=TestAccNotificationsWebhookResource -test.v -timeout 360m -parallel=10
?       github.com/hashicorp/terraform-provider-hcp/internal/clients/iampolicy  [no test files]
testing: warning: no tests to run
PASS
ok      github.com/hashicorp/terraform-provider-hcp/internal/clients    (cached) [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/hashicorp/terraform-provider-hcp/internal/consul     (cached) [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/hashicorp/terraform-provider-hcp/internal/hcpvalidator       (cached) [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/hashicorp/terraform-provider-hcp/internal/input      (cached) [no tests to run]
?       github.com/hashicorp/terraform-provider-hcp/internal/provider   [no test files]
?       github.com/hashicorp/terraform-provider-hcp/internal/provider/acctest   [no test files]
?       github.com/hashicorp/terraform-provider-hcp/internal/provider/modifiers [no test files]
testing: warning: no tests to run
PASS
ok      github.com/hashicorp/terraform-provider-hcp/internal/provider/iam       (cached) [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/hashicorp/terraform-provider-hcp/internal/provider/logstreaming      (cached) [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/hashicorp/terraform-provider-hcp/internal/provider/resourcemanager   (cached) [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/hashicorp/terraform-provider-hcp/internal/provider/vaultsecrets      (cached) [no tests to run]
=== RUN   TestAccNotificationsWebhookResource
--- PASS: TestAccNotificationsWebhookResource (18.53s)
PASS
ok      github.com/hashicorp/terraform-provider-hcp/internal/provider/webhook   19.311s
testing: warning: no tests to run
PASS
ok      github.com/hashicorp/terraform-provider-hcp/internal/provider/webhook/validator (cached) [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/hashicorp/terraform-provider-hcp/internal/providersdkv2      (cached) [no tests to run]

[dadgar]

Is there any documentation we can link the user to that enumerates the options they can set?

[nywilken]

That's a great question. I believe these will be documented per-product on their respective webhook pages - based on https://developer.hashicorp.com/hcp/docs/packer/webhooks

Linking to the HCP Packer page to start would be helpful but I don't know how fast it will become outdated once other products start to offer webhooks.

Wondering if there should be a central webhooks page for HCP that gets updated as products enable webhooks, which can be linked to later on. What do you think @dadgar?

[sylviamoss]

Wilken is right, these will be documented per-product and I think the best solution would be to have a single page where we would link to those documentation. I think that makes total sense.

I can work on adding a section to https://developer.hashicorp.com/hcp/docs/packer/webhooks, but before, I would like to confirm if that would address the concerns of this thread.

[dadgar]

What about having a generic web hooks documentation page that describes the capabilities and have links to each product page that integrates with web hooks for more details on events. That way it is scalable.

Something like:

# Webhooks
How they work

## Setting them up examples
...

## Webhook Event Documentation

Packer Webhooks -> Link to existing page
HVS Webhooks -> Link to HVS
...

[nywilken]

This is a good option. Especially, if teams already update provider docs as they add platform support or other updates to resources this option would be ideal.

I considered it previously for a moment but thought that given the distance between the provider and the product docs themselves that teams not needing to change the provider could forget or not know now to update the docs. In my head the closer it is the the platform docs the more visibility it would get.

Thinking out loud, users will probably find their way to the docs for their product of choice after clinking into anyone of the web events pages and searching. So maybe the docs becoming stale is not a big issue. A generic web hooks documentation page that describes the capabilities and links to product pages seems like a good tradeoff at the moment.

[sylviamoss]

Would this generic page be a guide in the provider?

[dadgar]

I think it would make more sense to add here so it can be reference for TF, CLI, API.

https://github.com/hashicorp/hcp-docs

[sylviamoss]

Got it. I will add a new section to https://developer.hashicorp.com/hcp/docs/hcp/admin/projects/webhooks
I think it also makes sense to add a section about the terraform resource.

[brandonc]

Some smoke testing findings. I like the new schema!

[brandonc]

If you wanted to, you could add a schema validator to prevent the situation where its possible to plan multiple duplicate events or wildcard + events on the same source, but then get an API error when you try to apply those. No huge deal either way but it's nice when you can prevent apply errors after successful plans.

Example config:

resource "hcp_webhook" "foo" {
  name        = "example-webhook"
  description = "My new webhook!"
  enabled     = true
  config = {
    url = "https://en46gax89xugj.x.pipedream.net"
    hmac = "example"
  }
  subscriptions = [
    {
      events = [
        {
          actions = ["*"]
          source  = "hashicorp.packer.iteration"
        },
        {
          actions = ["create"]
          source  = "hashicorp.packer.iteration"
        }
      ]
    }
  ]
}

[sylviamoss]

Hey @brandonc it took me a bit of work but I was able to implement a quite complex validator. Let me know what you think.

[brandonc]

Works great! Regarding the complexity of the validator, I noticed you included a lot of type assertion error checks that would probably already be covered by the basic schema validator. For instance, this code checks to ensure events is indeed a list:

eventsValuable, ok := events.(basetypes.ListValue)
if !ok {
resp.Diagnostics.AddAttributeError(
	req.Path,
	"Invalid subscription events",
	"While performing schema-based validation, an unexpected error occurred. "+
		"The attribute declares a List value validator, however its values do not implement types.ListType interface for custom List types. "+
		"This is likely an issue with terraform-plugin-framework and should be reported to the provider developers.\n\n"+
		fmt.Sprintf("Path: %s\n", req.Path.String())+
		fmt.Sprintf("Element Type: %T\n", EventsType)+
		fmt.Sprintf("Element Value Type: %T\n", events),
)
return
}

but if I change events to be not a list, I get the following error from terraform validate or any other command that loads config:

│ Error: Incorrect attribute value type
│
│   on main.tf line 15, in resource "hcp_notifications_webhook" "foo":
│   15:   subscriptions = [
│   16:     {
│   17:       events = ""
│   18:     }
│   19:   ]
│
│ Inappropriate value for attribute "subscriptions": element 0: attribute "events": list of object required.

That being said, this code doesn't hurt your validator and does seem to protect against framework bugs, as described in the error message

[sylviamoss]

Thanks for having a lot. It seems that I overcomplicated with those checks, but as it doesn't hurt I will leave it. If it is bothering the readability too much I will remove it whenever that happens.

[brandonc]

As an aside, I noticed that this resource is not sensitive to changes to the default project ID when you change it at the provider level. In other words, if the resource uses the provider-configured project, then the project ID changes on the provider config, the next plan does not re-create the resource in the new project.

We have a similar setting in the tfe provider (organization) and I solved it with a plan modifier https://github.com/hashicorp/terraform-provider-tfe/blob/main/internal/provider/resource_tfe_registry_provider.go#L177-L179

[sylviamoss]

That's interesting and I think that other resources in this provider are also not sensitive to provider-level project id change. I did looked how they handle project_id and didn't see anything different from what I implemented here. Thanks for catching this one. I will have a look at the plan modifier you created.

[sylviamoss]

@dadgar I want to catch your attention to @brandonc's comment. I couldn't find any resource that is taking care of this, which means none of them are considering the provider's default changes and cascading updates to resources.

[dadgar]

@brandonc Good catch, could you file an issue so we eventually fix that. @jasonpilz may be interested in that

[brandonc]

I couldn't get the provider to detect that I had changed my hmac key

[sylviamoss]

I did test that but maybe I made a change in between that changed the behavior. I will have a look and an acceptance test for this too.

[sylviamoss]

I tested it again and I couldn't reproduce what you experienced. I added two new acceptance tests to test that it is possible to update from none to an hmac_key and from one hmac_key to another. Can you try one more time?

[brandonc]

Looks really good. For some reason I combed for whitespace issues in the string concatenations but everything smoke tested... except I still can't modify the hmac field.

To repro, I applied a simple config like this:

resource "hcp_notifications_webhook" "foo" {
  name        = "example-webhook"
  description = "My new webhook!"
  enabled     = false
  config = {
    url = "https://en46gax89xugj.x.pipedream.net"
    hmac = "example"
  }
  subscriptions = [
    {
      events = [
        {
          actions = ["create"]
          source  = "hashicorp.packer.iteration"
        },
      ]
    }
  ]
}

and then tried changing "example" to something else, like "example2". The second apply did not detect changes between the state and the config

[sylviamoss]

@brandonc
I have just noticed that you configured a hmac instead of hmac_key. Should the provider fail if you configure an attribute that is not present in the schema? 🤔 I would expect yes and would be detected by the framework, but apparently, the resource should implement some source of custom type for that. Is that correct?

[dadgar]

Seems like you could store a boolean here httpsOnly bool and then use the same validator for both url and HTTPSUrl.

With a separate constructor:

func HTTPSURL() validator.String {
   return urlValidator{
      httpsOnly: true,
    }
 }

[dadgar]

Should this say it defaults to true

[sylviamoss]

Yes. I forgot that, I will add!

[dadgar]

This is the default, remove?

[nywilken]

I have an open question about adding a validator for catching invalid event actions. But this otherwise looks good. Approving in advance in case the validator was intentionally skipped.