Allow accepting additional StatusCodes with custom fallback response

[jethron]

Allow specifying HTTP error codes that should be accepted instead of producing an error (e.g. 404, 500, etc).
You can configure a default/fallback response body to use if the errors are encountered, or just use the real response.
If you specify 200 as an allowed error, you can also use this to mock out the response; potential footgun, but could also be useful in some testing scenarios.

See #107, also applicable to #41.

[hashicorp-cla]

All committers have signed the CLA.

[theipster]

Just my opinion, but the definition of an "error" for this http data resource should not be focused on HTTP status codes at all. An "error" for a HTTP request-response should cover lower-level things like DNS resolution errors, SSL handshake errors... basically, the same kind of error you'd receive from http.NewRequestWithContext().

A HTTP status code of 404 doesn't make the request-response any less valid or legitimate than if it contained a status code of 200. For example, maybe I'm expecting to see a 403, because I want to verify that the access controls on the target URL are functioning properly (as if it's a negative test). Status codes are all about semantics, which can be context-dependent and are not the concern of this http data resource.

How might the above look in terms of code? I think the longer-term goal should be to discourage users from thinking about HTTP status codes in their requests, which we can do by:

• Exposing resp.StatusCode as a new attribute status_code, e.g. data.http.foo_request.status_code; and encourage the user to deal with the status code however they wish, but outside of the http data resource; and,
• Removing the if resp.StatusCode != 200 { ... } condition completely...
  • ...or, for backwards compatibility, feature-flag it behind a new, but deprecated, argument expect_status_code_200 with a default value of true that users can then override.

[jethron]

Rebased onto main.

---

Just my opinion, but the definition of an "error" for this http data resource should not be focused on HTTP status codes at all. An "error" for a HTTP request-response should cover lower-level things like DNS resolution errors, SSL handshake errors... basically, the same kind of error you'd receive from http.NewRequestWithContext().

Good point @theipster . I guess I was more of the perspective that those lower level issues could theoretically occur for any resource/data resource that's provider calls an API, and such an error is a provider error, rather than a (data) resource error, which is what I was trying to allow handling here. I've updated the terminology to reference 'fallbacks', rather than explicitly naming 'errors'.

A HTTP status code of 404 doesn't make the request-response any less valid or legitimate than if it contained a status code of 200. For example, maybe I'm expecting to see a 403, because I want to verify that the access controls on the target URL are functioning properly (as if it's a negative test). Status codes are all about semantics, which can be context-dependent and are not the concern of this http data resource.

Agreed, that's what this was kind of trying to address.

How might the above look in terms of code? I think the longer-term goal should be to discourage users from thinking about HTTP status codes in their requests, which we can do by:

* Exposing `resp.StatusCode` as a new attribute `status_code`, e.g. `data.http.foo_request.status_code`; and encourage the user to deal with the status code however they wish, but _outside_ of the `http` data resource; and,

* Removing the `if resp.StatusCode != 200 { ... }` condition completely...
  
  * ...or, for backwards compatibility, feature-flag it behind a new, but deprecated, argument `expect_status_code_200` with a default value of `true` that users can then override.

Yes, the feature flag approach seems OK to me. I think the only issue with it would be that you would be opting out of all errors, rather than just opting into specific ones with this approach. You would then just end up with an unexpected status/body without a good way to reason about them. But I think once TF 1.2.0 is out, pre/post conditions would probably allow you to fix that and turn such status codes into a proper error again.

Happy to switch approaches if it would make it more likely to merge, but given the lack of maintainer feedback I suspect this is just not a desired change for the project, which is ok too. :)

[bendbennett]

@jethron thank you for submitting this PR and apologies that it has taken a little while for us to respond. Having read through the proposed code changes and the comments from @theipster we're inclined to suggest going with the approach that @theipster describes and then leveraging precondition and postcondition that have been made available in Terraform v1.2.0 and later.

@jethron would you be happy to update the PR along the lines that @theipster suggests:

• expose status_code
• remove the check on the response status code completely

As these are breaking changes we can then have a major release and indicate that practitioners should check the status code by either using a postcondition on http data source or a precondition on a dependent resource. For instance:

Postcondition on http data source

data "http" "example" {
  url = "https://checkpoint-api.hashicorp.com/v1/check/terraform"

  # Optional request headers
  request_headers = {
    Accept = "application/json"
  }

  lifecycle {
    postcondition {
      condition     = contains([201, 204], self.status_code)
      error_message = "Status code invalid"
    }
  }
}

Precondition on dependent resource

data "http" "example" {
  url = "https://checkpoint-api.hashicorp.com/v1/check/terraform"

  # Optional request headers
  request_headers = {
    Accept = "application/json"
  }
}

resource "random_uuid" "example" {
  lifecycle {
    precondition {
      condition     = contains([201, 204], data.http.example.status_code)
      error_message = "Status code invalid"
    }
  }
}

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.