Thank you for submitting this and a BIG apology on our part for letting it languish for the last couple of weeks! I'm going to split this review between me, @louisruch and @s-christoff since it's a large PR. Love seeing the inclusion of data sources and autogen from our API spec.

Thank you for submitting this and a BIG apology on our part for letting it languish for the last couple of weeks! I'm going to split this review between me, @louisruch and @s-christoff since it's a large PR. Love seeing the inclusion of data sources and autogen from our API spec.

Hi @malnick, thanks for taking the time to review this. One thing that I did not add in this PR is setting the Sensitive: true field where appropriate, I wanted to have your feedback for this. I think there is basically two options, either add a parameter to list the sensitive fields in scripts/generate_datasource.go or add this information in the OpenAPI spec as a vendor extension (I don't think there is a way to have this information without custom properties). I would prefer the second one, it would require cooperation from the Boundary team but it would make it much more easier as all the Terraform schema would come from one source only.

I like the idea of adding it to the API spec as well, there might be other wins we can get from that in tooling that leverages that spec too.

I'm roping in @jefferai on this PR since it's fairly cross functional and hits on a few key areas that are under his prevue. More on this very soon as we figure out the best way to break down the review. Thank you again!

I'm actually going to tag @jimlambrt on this since marking things sensitive is a part of the next step of eventing and probably we can have some fields automatically marked this way in openapi.

That said, I also don't think it necessarily matters, as Boundary will never round trip a secret value set from Terraform back in a read. I will admit I don't have a great idea of how Sensitive is handled in TF so maybe that's not relevant?

HI @jefferai,

That said, I also don't think it necessarily matters, as Boundary will never round trip a secret value set from Terraform back in a read.

This is good to know, we don't have to set anything Sensitive for the datasources then. I do plan to extend this to generate the resources to later on as it would make it very easy to have the complete provider auto-generated. Once we get there we will need to know which field are Sensitive as they would be leaked in the plan otherwise. This will be for a later PR thought.

Curious, any reason you based this on OpenAPI?

The OpenAPI output itself is generated based on our source of truth, our proto struct and service definitions. Because the OpenAPI generation is subject to various constraints of the OpenAPI v2 spec, and may one day become a v3 spec, we use the proto definitions to build our Go API libraries and our CLI. That way we're not generating based on something that is itself generated.

Edit: one of the reasons this has me concerned is that we produce OpenAPI because people ask us for it and because it might be useful for dynamic generation of web UI elements in the future but we don't really curate it. It's pretty much whatever grpc-gateway spits out. So I worry that something in there is currently or will be a bit funky because we're not paying close attention to it in the way that we are the underlying protos/services and the Go SDK output.

Hi @jefferai, I initially tried to base the generator on the protobufs but I found it more difficult to get it working than basing the generator on the OpenAPI, I think it was mostly because everything regarding the protobufs is in internal/ so it was difficult to get Go to read them properly without vendoring everything (which would have required to keep them in sync) or having a lot of boilerplate. I also wanted to avoid moving the Terraform provider generator in github.com/hashicorp/boundary both in fear that it may be cumbersome for you to have to check that the updates to the provider are correct every time you make a change to Boundary, as opposed to launching the script once per release, and because this is not what any of the users would expect and it would be an additional thing to learn to contribute to the provider.

I shared your fears regarding relying on the OpenAPI document, and I don't like OpenAPI in general, but since most of the work here is done by grpc-gateway which is the JSON proxy anyway I expect to be mostly correct as some other users of the grpc-gateway project are probably relying on it.

Regarding the transition from OpenAPI 2 to OpenAPI 3, I don't think it will be very difficult to update the generator which is currently quite short.

Also regarding Boundary and Terraform, but this time to make it possible to use Boundary proxies from Terraform I opened hashicorp/terraform#29402 that may interest you.

Everything is in internal by default until we need to move it out. It may be a good time to consider moving the generated files into the sdk module given that this would let them be used for purposes like this and we keep those APIs stable(ish) at this point anyways given that they are now part of the product. That would let this be based on the generated protos.

We also generate the Go structs in api/ from the protos. Depending on whether you need things like the comments from the OpenAPI, this might actually be the easiest thing to use, and we do curate the Go types that are created. Either of those options seem better than using the OpenAPI though.

Talked with the team internally -- we're going to move some of the files out of internal and into sdk. We're still figuring out exactly how we want to do to this based on some other needs we have. So it may take the form of the generated protobufs (and the .proto files) being put in sdk, it may take the form of those types being wrapped by SDK types, or it could be type aliases. Stay tuned...

Hi there -- we have two paths that are being considered, and curious which is easier for you:

1. You could base on the Go SDK (under github.com/hashicorp/boundary/api) which contains Go structs converted via protoreflect from the generated proto structs. This is slightly lossy compared to the original proto messages, but it's just type conversions to our API which you'd likely be doing anyways. So this may save you some work if you reflect against it.
2. We could alias or embed the generated proto types into the sdk module. From there you could use protoreflect against the message types.

Thoughts?

Hi @jefferai, sorry to answer this late, I've had a lot of work this week. I thought about both options and I don't have a strong preference for either, it seems to me that making the protobufs part of the external API would be more committing regarding backward compatibility. Since the types are already in github.com/hashicorp/boundary/api it seems to me like using those in the generator would be better as it would add less constraints.

You could use either! The PB files are now in the sdk module in the main branch.

Hi everyone, sorry for the delay for getting back I've had a lot of work last weeks. I've been looking into converting the current generator from using the swagger definition to either using reflect or protoreflect but there is some details I'm not sure about. For example we extract the Description from the swagger but it is not in the Go types, and only as a comment in the protobufs so we can't extract it (unless using ast but I suppose we want to avoid that as it would made maintenance harder).

Do you think we should add this in the Go types as a a struct tag?

Sorry for the lull in comms over here @remilapeyre ! We've been a little caught up in the day-to-day, but @jefferai will be reaching out again soon.

Sorry for the delay!

I think encoding the description in struct tags would likely then break whatever grpc-gateway is doing to use that description for openapi.

Probably we should either:

1. Not include description in the data source schema (asking people to reference the non-data-source resources for the values)
2. Copy whatever grpc-gateway's openapi generator is doing to populate the description for openapi

There's an option I think is better than either but is more work:
3. separate the descriptions from the protobufs. Put the descriptions in some kind of external structure in the sdk package and have templating code insert it into the proto files before generation, or something similar. There are a couple of real benefits to this approach:

• Easier translation, if we ever translate the descriptions, because the strings can be separated by lang/locale
• Exact same descriptions can be used in openapi, TF, and anywhere else that we want to use them, without them getting out-of-sync

So while I can live with (1) or (2) right now and either will work, I think long term we (being, the Boundary team, and you if interested) should push towards (3).

All committers have signed the CLA.

Any plans to give this traction in 2024?

@macmiranda The original author seems to have abandoned it but I think someone on the Boundary team might be picking it up soon.