tailscale: add support for S3 logstreaming #468
Conversation
zehauser
force-pushed
the
zach/s3-logstreaming
branch
2 times, most recently
from
9354aac to
a0e1650
Compare
There was a problem hiding this comment.
Successful acceptance test run: https://github.com/tailscale/corp/actions/runs/12906014118
| github.com/tailscale/tailscale-client-go/v2 v2.0.0-20250122081626-c84613baf18d h1:I3Ka+YodSpslWRaHHOtKQiHFcfuTr8o23619oP1P11I= | ||
| github.com/tailscale/tailscale-client-go/v2 v2.0.0-20250122081626-c84613baf18d/go.mod h1:i/MSgQ71kdyh1Wdp50XxrIgtsyO4uZ2SZSPd83lGKHM= |
There was a problem hiding this comment.
We probably need to modify this after tailscale/tailscale-client-go#131 is actually merged.
| if err != nil && tsclient.IsNotFound(err) { | ||
| d.SetId("") | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Oh right! This is an unrelated bugfix I snuck in. Or at least I think it's a bugfix.
If you delete your Terraform-created logstream configuration out-of-band (e.g. in the admin console), without this fix, we fail with a 404 error on plan. I think probably we should instead detect that the config has been deleted and report that, right?
There was a problem hiding this comment.
I'll defer to @mpminardi on this, as I don't know what the usual convention is with Terraform.
We recently added support for S3 logstreaming endpoints to our API. This involved adding several new fields to the LogstreamConfiguration resource (available when the new "s3" destinationType is used), plus a new AWSExternalID resource needed when AWS role-based authentication is used for an S3 logstreaming endpoint. This commit updates the Terraform provider to reflect these changes. We add support for the new fields to the tailscale_logstream_configuration resource, and create a new tailscale_aws_external_id resource. Unfortunately, we have to add some special handling for the "user" field of tailscale_logstream_configuration. Previously, we specified a default dummy value of "user" for the "user" field. Now, the PUT LogstreamConfiguration endpoint does not allow any value to be specified for a s3 destinationType. We could loosen that restriction at the API layer, but I would prefer not to do that. However, we don't want to break any existing users who are relying on this default and have it in their Terraform state. So the approach I've chosen is to add special handling for the "user" field: - When creating or updating a configuration, if destinationType == "s3" and user == "user", we assume the "user" value came from the default and we do not actually send it to the API. - When reading or importing a configuration, if destinationType == "s3" and there is no user field (which should always be true), we set user = "user" in the Terraform state. Fixes #458 Fixes tailscale/corp#24533 Signed-off-by: Zach Hauser <[email protected]>
zehauser
force-pushed
the
zach/s3-logstreaming
branch
from
a0e1650 to
21d8c90
Compare
| func resourceAWSExternalIDCreate(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Diagnostics { | ||
| client := m.(*tsclient.Client) | ||
|
|
||
| aid, err := client.Logging().CreateOrGetAwsExternalId(ctx, tsclient.CreateOrGetAwsExternalIdRequest{Reusable: false}) |
There was a problem hiding this comment.
It might be worth a comment on why "reusable" is false, and why it must be.
| // corresponding scenario for resourceLogstreamConfigurationRead(), we'll add | ||
| // user = 'user' to the state.) | ||
| user := d.Get("user").(string) | ||
| if destinationType == tsclient.LogstreamS3Endpoint && user == "user" { |
There was a problem hiding this comment.
Do we need the user == "user" check? For S3, we never want to send a user, right?
There was a problem hiding this comment.
And we if change this, can we get rid of the below?
| if err != nil && tsclient.IsNotFound(err) { | ||
| d.SetId("") | ||
| return nil | ||
| } |
There was a problem hiding this comment.
I'll defer to @mpminardi on this, as I don't know what the usual convention is with Terraform.
| log_type = "network" | ||
| destination_type = "s3" | ||
| s3_bucket = "example-bucket" | ||
| s3_region = "us-west-2" |
We recently added support for S3 logstreaming endpoints to our API. This involved adding several new fields to the LogstreamConfiguration resource (available when the new "s3" destinationType is used), plus a new AWSExternalID resource needed when AWS role-based authentication is used for an S3 logstreaming endpoint.
This commit updates the Terraform provider to reflect these changes. We add support for the new fields to the tailscale_logstream_configuration resource, and create a new tailscale_aws_external_id resource.
Unfortunately, we have to add some special handling for the "user" field of tailscale_logstream_configuration. Previously, we specified a default dummy value of "user" for the "user" field. Now, the PUT LogstreamConfiguration endpoint does not allow any value to be specified for a s3 destinationType. We could loosen that restriction at the API layer, but I would prefer not to do that. However, we don't want to break any existing users who are relying on this default and have it in their Terraform state. So the approach I've chosen is to add special handling for the "user" field:
Fixes #458
Fixes tailscale/corp#24533