docs/design: Initial Validation design document

[bflad]

Reference: #17

Covering background, prior implementations, and goals. Further updates to this document will outline proposals and ultimately recommendations.

[kmoe]

StringValueValidator has great UX - no type assertions needed in the provider code!

How would this work with complex types, e.g. types.List? Since there are as many list types as there are element types, it seems that the options are either:

1. Create a single ListValueValidator interface, and require type assertions on the list elements inside the Validate function,
2. Create multiple ListOfStringsValueValidator, etc, interfaces, accepting that not all cases can be covered but trying to provide helpers for the most common ones, or
3. Do not create strongly typed validator interfaces for complex types.

[bflad]

In my head I was thinking in the area of 2 and leaving an escape hatch interface (3). 1 could make 2 slightly easier to do, so might be beneficial.

I'm also curious though of what validation space exists here -- in the previous framework there is not much available validation for complex types (e.g. you can only validate the number of elements in a list/set/map). There have been very odd cases of needing a list type for ordering, but needing element uniqueness (like a set).

I think my focus would be covering the very common string/number cases then allowing those more odd cases to bubble up.

[kmoe]

Really well structured design doc. One option I'd like to see considered is the request/response pattern for the parameters in the validation functions. All other things being equal (which they likely aren't), it might be good to use that pattern consistently across CRUD, plan modification, and validation.

[paddycarver]

I'd love to see options outlined for:

• having a Validate method on the resource/provider/data source that allows for validation to be implemented inline at the resource level, instead of needing to return validator interfaces.
• Using the request and response pattern for these validation interfaces.

I have concerns about the strongly typed function signatures. I think that may hurt discoverability, and I think it's going to be an absolute mess when complex types get involved. One option that's raised in #64 is to have an attr.ValueAs helper that works like Get, meaning we could use the same plan/config/state access as the rest of the framework and be consistent with plan modification (if it goes that route). It may be worth exploring that here, too.

I have concerns with the typed error returns and using errors in general when the rest of the framework is diagnostics. It seems to prioritize sharing information with callers, but we're the only callers, and I think the primary audience for that error information is the practitioner. I'm worried this is either going to be too restrictive for provider developers to offer a good experience, or add cognitive load on provider developers that we can avoid. I'm interested in hearing how you feel about returning diagnostics like we do everywhere else, and iterating on the interface to and design of diagnostics to try and reap some of the benefits you're identifying.

I'm having some trouble keeping track of the distinctions you're introducing here. It may be easier to track if we collapse some of the sections--for example, we may not need to talk about how something is set on a Schema and what its type/type signature is separately. I think it's okay to pick one example when showing it on the schema, and then just show alternate types/type signatures. It may reduce the number of headings and the level of nesting in the headings, which gets hard to follow. But mostly, the first time you refer to something (say, "PathValidation") it may be helpful to just note what makes it distinct from the other, similar concepts you've introduced.

I think this is great, a lot of context, and you've covered the problem space really well. I'd love to see the other solutions I mentioned considered and their benefits/tradeoffs weighed. I'd also like to see some more information about how complex types would be handled in your proposed strongly-typed helpers.

[paddycarver]

Another potential benefit is that it is the most flexible implementation, allowing incredibly complex validations to have their most straightforward implementation.

[kmoe]

Is "straightforward" (in the comment) intended to contradict "verbose" (in the text)? Without seeing a ValueValidator I can't work out whether the "incredibly complex validations" will be verbose or not.

[paddycarver]

Sorry, missed this in the shuffle. I think what I'm trying to say is that, for example:

// ignore the type signature, it's a place holder
func (r computeInstanceResource) Validate(ctx context.Context, req ValidateResourceRequest, resp *ValidateResourceResponse) {
  // assume we want to ensure, e.g., that no disks combine to more than one terabyte of storage, or something
  totalStorage := 0
  for _, disk := range plan.Disks { // not how any of this works, but pseudo-code!
    totalStorage += disk.Size
  }
  if totalStorage >= 1024*1024*1024*1024 {
    // return an error!
  }
}

This is a weird thing to define a reusable helper for, and it's weirder if we throw more details on this: maybe it can only be 1tb of spinning rust disks, 512gb of SSDs, and no more than 1tb of disks total. Maybe the size of the instance comes into play. That's what I mean by "incredibly complex"--I guess a better way to phrase it would be "unlikely to be reused across resources".

To implement this declaratively, you'd need to do something like this:

// ignore the type signature, it's a place holder
func (r computeInstanceResource) Validators(ctx context.Context) []Validators {
  return []Validators{computeInstanceValidator{}}
}

type computeInstanceValidator struct{}

func (r computeInstanceValidator) Validate(ctx context.Context, req ValidateResourceRequest, resp *ValidateResourceResponse) {
  // assume we want to ensure, e.g., that no disks combine to more than one terabyte of storage, or something
  totalStorage := 0
  for _, disk := range plan.Disks { // not how any of this works, but pseudo-code!
    totalStorage += disk.Size
  }
  if totalStorage >= 1024*1024*1024*1024 {
    // return an error!
  }
}

I'm arguing this is less straightforward because you need to introduce the indirection of declaring the validation logic separately from the resource, in a reusable chunk, even though you're only using it for the resource. The code is a bit more verbose, and feels a little less intuitive, imho.

[paddycarver]

Couldn't we also solve this with a resource-level Validator method, if we're pushing to the resource-level?

[kmoe]

A more thorough review prior to the request/response pattern additions (edit: which I see you've just added. Will do another pass if I get time later today).

[kmoe]

See comment above regarding implementations of custom types using the attr.Type interface - or perhaps I'm misunderstanding the custom type design this refers to, in which case an example would be helpful.

[kmoe]

Is "straightforward" (in the comment) intended to contradict "verbose" (in the text)? Without seeing a ValueValidator I can't work out whether the "incredibly complex validations" will be verbose or not.

[kmoe]

Aha, are this and the previous section to be considered orthogonal concerns: firstly, what the ValueValidators are attached to, and secondly, what type of Go construct (func type, interface) they are?

[kmoe]

I like the flexibility of being able to add this later if needed.

[kmoe]

Using context for attribute paths is a really interesting idea in general that I hadn't thought of before, and I appreciate the opportunity to add nuance to my general principle "you probably shouldn't use context.WithValue()".

From thecontext.WithValue() docs: "Use context Values only for request-scoped data that transits processes and APIs, not for passing optional parameters to functions.".

If we consider the RPC and its handling in framework and provider code a request, then the attribute path is certainly request-scoped data that transits processes (Terraform and the plugin) and APIs (the gRPC call, the framework handler, the provider validation func).

I agree, however, that it is less discoverable, and having to make a type assertion on the attribute path isn't very nice. Interesting idea to keep in mind though.

[paddycarver]

I am incredibly in favor of "you probably shouldn't use context.WithValue", with the exception of request-scoped data. What is considered to be request-scoped data I think will continue to be the source of some debate.

[paddycarver]

This latest revision looks great, and the proposal looks reasonable to me. There are a couple of doc suggestions here that don't really touch the design, and some bikeshedding on some naming, but overall, I'm really happy with the doc and the direction. I'm super interested in hearing @kmoe's thoughts, though, and how she thinks it will intersect with plan modification, as I think we want those designs to be similar.

[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.