feat(bunny_pullzone): add a datasource for pull zones #98

jspizziri · 2022-11-11T18:11:58Z

No description provided.

jspizziri · 2022-11-11T18:18:25Z

This PR is independent from, but ultimately serves the work I'm doing on bunny_videolibrary here #97

Video Libraries each have a unique underlying Pull Zone that is manged by Bunny Stream. However, there is a lot of important data on these pull zones that is needed to properly use a Video Library for anything (such as signing keys, pull zone name, etc). By providing a data source you can do something like:

resource "bunny_videolibrary" "vl" {
  name = "foobar"
}

data "bunny_pullzone" "vlpz" {
    pull_zone_id = bunny_videolibrary.vl.pull_zone_id
}

You can then subsequently pass around important bits of information: data.bunny_pullzone.vlpz.zone_security_key, ...

fho · 2022-11-14T14:55:49Z

@jspizziri duplicating all those fields with the descriptions in the data source will be a pain to maintain.
I guess it's not possible to reuse the same map[string]*schema.Schema definition from the resource, is it?
If not, what do you think about only adding a limited set of attributes for the datasource that are likely to be used or that you currently need?
I imagine that it is unlikely that somebody ever needs keyEnableLogging from the data source.

Could you please also add a testcase?

jspizziri · 2022-11-14T15:50:04Z

@fho that was my thought as well! Unfortunately, it appears this is how it has to be done with TF. I looked at how the providers for AWS and K8s are working and they do it like this as well (the example does it this way too). Lastly, there are schema properties that can be put on the Resource schema that simply cause a run-time error if they're present on the data import schema.

If you could do some double-checking on this I think that would be wise, but this is what I found in my research last week.

I could certainly pair it down a good deal at least for my use case, and probably a sensible amount for most use cases. If you confirm that this is the best path forward I'll go ahead and do that and add some tests.

fho · 2022-11-14T16:18:05Z

@jspizziri I was also checking other provides and could not find one that was reusing the schema definition.
Often data sources also have a different structure then the related resource.

Lastly, there are schema properties that can be put on the Resource schema that simply cause a run-time error if they're present on the data import schema.

great that you tried that 👍

Lets go then with a minimal data source definition.

jspizziri · 2022-11-14T20:57:12Z

@fho I trimmed the payload and added a test. The test isn't working at the moment and I could use some guidance. When I run it I get the following error, but I don't know what the issue is:

F_ACC=1 go test -trimpath -ldflags="-X github.com/simplesurance/terraform-provider-bunny/internal/provider.Version=dev -X github.com/simplesurance/terraform-provider-bunny/internal/provider.Commit=v0.9.0-17-gf36123e-dirty" -race -v  -run TestAccDataSourcePullZone -timeout 120m ./...
?   	github.com/simplesurance/terraform-provider-bunny	[no test files]
=== RUN   TestAccDataSourcePullZone
panic: Invalid address to set: []string{"aws_signing_enabled"}

goroutine 262 [running]:
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.(*ResourceData).Set(0xc000307900, {0x1e8b878, 0x13}, {0x1d110c0, 0xc000352f8f})
	github.com/hashicorp/terraform-plugin-sdk/v2@v2.24.0/helper/schema/resource_data.go:230 +0x43f
github.com/simplesurance/terraform-provider-bunny/internal/provider.pullZoneToResource(0xc000601200, 0x2052768?)
	github.com/simplesurance/terraform-provider-bunny/internal/provider/resource_pullzone.go:578 +0xf1
github.com/simplesurance/terraform-provider-bunny/internal/provider.resourcePullZoneRead({0x2052768, 0xc000380960}, 0x7?, {0x1e2d060?, 0xc000487710})
	github.com/simplesurance/terraform-provider-bunny/internal/provider/resource_pullzone.go:547 +0x22d
github.com/simplesurance/terraform-provider-bunny/internal/provider.dataSourcePullZoneRead({0x2052768, 0xc000380960}, 0x4?, {0x1e2d060, 0xc000487710})
	github.com/simplesurance/terraform-provider-bunny/internal/provider/data_source_pullzone.go:75 +0xee
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.(*Resource).read(0xc00016a8c0, {0x20527a0, 0xc000206d80}, 0xd?, {0x1e2d060, 0xc000487710})
	github.com/hashicorp/terraform-plugin-sdk/v2@v2.24.0/helper/schema/resource.go:724 +0x2e9
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.(*Resource).ReadDataApply(0xc00016a8c0, {0x20527a0, 0xc000206d80}, 0xc000307800, {0x1e2d060, 0xc000487710})
	github.com/hashicorp/terraform-plugin-sdk/v2@v2.24.0/helper/schema/resource.go:943 +0x245
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.(*GRPCProviderServer).ReadDataSource(0xc0001332a8, {0x20527a0, 0xc000206c90}, 0xc00007c0e0)
	github.com/hashicorp/terraform-plugin-sdk/v2@v2.24.0/helper/schema/grpc_provider.go:1179 +0x79b
github.com/hashicorp/terraform-plugin-go/tfprotov5/tf5server.(*server).ReadDataSource(0xc000222500, {0x20527a0, 0xc000206420}, 0xc0006280a0)
	github.com/hashicorp/terraform-plugin-go@v0.14.0/tfprotov5/tf5server/server.go:658 +0x7f2
github.com/hashicorp/terraform-plugin-go/tfprotov5/internal/tfplugin5._Provider_ReadDataSource_Handler({0x1e44260?, 0xc000222500}, {0x20527a0, 0xc000206420}, 0xc000158150, 0x0)
	github.com/hashicorp/terraform-plugin-go@v0.14.0/tfprotov5/internal/tfplugin5/tfplugin5_grpc.pb.go:421 +0x25b
google.golang.org/grpc.(*Server).processUnaryRPC(0xc0005f23c0, {0x20566a0, 0xc0001056c0}, 0xc000490480, 0xc000679c50, 0x26037f0, 0x0)
	google.golang.org/grpc@v1.48.0/server.go:1295 +0x11ca
google.golang.org/grpc.(*Server).handleStream(0xc0005f23c0, {0x20566a0, 0xc0001056c0}, 0xc000490480, 0x0)
	google.golang.org/grpc@v1.48.0/server.go:1636 +0xff9
google.golang.org/grpc.(*Server).serveStreams.func1.2()
	google.golang.org/grpc@v1.48.0/server.go:932 +0xed
created by google.golang.org/grpc.(*Server).serveStreams.func1
	google.golang.org/grpc@v1.48.0/server.go:930 +0x4de
FAIL	github.com/simplesurance/terraform-provider-bunny/internal/provider	8.976s
FAIL
make: *** [testacc] Error 1

fho · 2022-11-15T11:24:03Z

@jspizziri

pullZoneToResource() calls d.Set() for all keys in the resource schema.
You are calling pullZoneToResource() and are passing the datasource schema, which only has a subset of the keys.
d.Set fails because the key does not exist in the datasource schema.

jspizziri · 2022-11-15T11:32:46Z

@fho ah, thanks for that 🤦 .

So to reduce the schema we'll have to duplicate the mapping. Is that still your preference?

fho · 2022-11-15T11:39:22Z

@fho ah, thanks for that facepalm .

So to reduce the schema we'll have to duplicate the mapping. Is that still your preference?

@jspizziri
Yes, I think it is still preferable to have a small data source schema.

To reduce the duplicate code, we can also move the d.Set calls that work for both schemas to another function and call them when creating the resource and the datasource schema.

jspizziri · 2022-11-15T13:56:14Z

@fho updated, please take a look.

examples/data-sources/bunny_pullzone/data-source.tf

fho · 2022-11-16T14:58:50Z

internal/provider/data_source_pullzone.go

+			keyZoneSecurityEnabled: {
+				Type:        schema.TypeBool,
+				Computed:    true,
+				Description: "True if the URL secure token authentication security is enabled.",
+			},
+			keyZoneSecurityIncludeHashRemoteIP: {
+				Type:        schema.TypeBool,
+				Computed:    true,
+				Description: "True if the zone security hash should include the remote IP.",


What is the usecase for keyZoneSecurityEnabled and keyZoneSecurityIncludeHashRemoteIP, when managing a videolib?

Those are properties that, while exposed via the Video Library update endpoint, are not fetchable as part of the Video Library, as they reside exclusively on the Pull Zone. I put them in here as I think they'll be helpful in certain cases to determine the full state of a video library.

Do you mean that the EnableTokenIPVerification and PlayerTokenAuthenticationEnabled fields in the videolib update call change the keyZoneSecurityEnabled and keyZoneSecurityIncludeHashRemoteIP settings in the pull-zone? 👀

Correct, those are pass throughs that bunny internally sets on the managed pull-zone. The unfortunate part is that they don't also return them in the video library API. I asked the Bunny team about this in a support request and they said you have to query the Pull Zone directly to get them. The other alternative to this is to do a special PullZone.Get as part of the VideoLibrary update process, which doesn't feel great to me. Exposing them in this way seemed fairly simple, although it has the downside of your bunny_videolibrary resource state potentially being incorrect if something weird happens.

I do like bunny a lot, but when they do stuff like this I get a bit frustrated with them.

hum, I see, that makes it complicated.

Having to use fields in a pullzone datasource as workaround for the issue makes the Terraform provider difficult to use.
Users would need to know these API internals, that a videolib has an underlying pullzone, that some videolib attributes are stored in the pullzone and also the different attribute names.

A simple solution in other cases were a field was not retrievable via the Get endpoint was to mark the resource attribute as ForceNew: true.
I don't know if that is feasible for those videolib attributes.
Would be having to recreated the videolib when these attributes are changed an option?

Otherwise I think it is better to do the additional pullzone get call in the ReadContext function of the videolib.

ForceNew isn't really an option. It would delete all of your video content. As of the current state of the videolibrary resource I did something similar to what we did on the custom_404 property for storage zones if you recall that. Effectively, if the API call succeeds we just update the state to whatever value was sent to bunny. Which seems finish... until I found a bug in the bunny API where they were silently failing to set those properties in certain circumstances (I filed a bug report with them and they're going to fix it).

But still, stuff like that can happen unless you do a hard read on them.

Also, if we're going to read the pullzone in the bunny videolibrary resource, then it seems like we might as well return the entire data set that we care about from it instead of doing a datasource. The data source seemed like a cleaner way at first, but maybe that should be considered instead.

Also, if we're going to read the pullzone in the bunny videolibrary resource, then it seems like we might as well return the entire data set that we care about from it instead of doing a datasource.

Let's go for that solution.
Hiding these ugly internals to make the provider easier to use will be worth the additional api requests and code.

internal/provider/data_source_pullzone.go

internal/provider/data_source_pullzone_test.go

internal/provider/data_source_pullzone.go

fho self-requested a review November 14, 2022 12:35

fho reviewed Nov 16, 2022

View reviewed changes

internal/provider/data_source_pullzone_test.go Outdated Show resolved Hide resolved

fho reviewed Nov 16, 2022

View reviewed changes

feat(bunny_pullzone): add a datasource for pull zones

1f0bb76

jspizziri force-pushed the feat/datasource_pullzone branch from c6a8ce6 to 1f0bb76 Compare November 16, 2022 16:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(bunny_pullzone): add a datasource for pull zones #98

feat(bunny_pullzone): add a datasource for pull zones #98

jspizziri commented Nov 11, 2022

jspizziri commented Nov 11, 2022

fho commented Nov 14, 2022

jspizziri commented Nov 14, 2022

fho commented Nov 14, 2022

jspizziri commented Nov 14, 2022

fho commented Nov 15, 2022

jspizziri commented Nov 15, 2022

fho commented Nov 15, 2022 •

edited

Loading

jspizziri commented Nov 15, 2022

fho Nov 16, 2022

jspizziri Nov 16, 2022

fho Nov 16, 2022

jspizziri Nov 16, 2022

fho Nov 16, 2022

jspizziri Nov 16, 2022

jspizziri Nov 16, 2022

fho Nov 16, 2022

feat(bunny_pullzone): add a datasource for pull zones #98

Are you sure you want to change the base?

feat(bunny_pullzone): add a datasource for pull zones #98

Conversation

jspizziri commented Nov 11, 2022

jspizziri commented Nov 11, 2022

fho commented Nov 14, 2022

jspizziri commented Nov 14, 2022

fho commented Nov 14, 2022

jspizziri commented Nov 14, 2022

fho commented Nov 15, 2022

jspizziri commented Nov 15, 2022

fho commented Nov 15, 2022 • edited Loading

jspizziri commented Nov 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fho commented Nov 15, 2022 •

edited

Loading