feat(bunny_storagezone): add a storagezone resource #64

jspizziri · 2022-06-01T18:10:27Z

jspizziri · 2022-06-01T18:15:02Z

@fho this is obviously a WIP still, but it does do some things. Feel free to do an early review if you have time and adjust my course. One question I have is about using a nested package/module structure (again, I'm a go noob, so I'm not sure what the language is for it). But if it were up to me I'd probably try to reduce the flatness of the internal/provider/* folder, but perhaps that's just the Go way?

I'll also add that the relationship between PZ & SZ seems to be taken care of by Terraform "magic". You cannot (AFAIK) currently update the SZ associated with a PZ via the API (although you can do it through the GUI) which means that any change that causes a recreate of the SZ cascades that recreate to the PZ. To add to that, most of the SZ properties are immutable, although it's unlikely that anyone would want to mutate them anyways.

fho · 2022-06-03T09:09:54Z

One question I have is about using a nested package/module structure (again, I'm a go noob, so I'm not sure what the language is for it). But if it were up to me I'd probably try to reduce the flatness of the internal/provider/* folder, but perhaps that's just the Go way?

In other Go projects I end up with much more packages. :-)
So far I did not see an advantage of introducing more packages.
Other provider follow the same one-package layout, see e.g.

I think having all resources in the same package is fine they belong together and I don't see an advantage of splitting them to multiple packages.
Some of the helper functions could be grouped in different packages (structure.go, types_*.go) though, not sure if would really improve it.

I'm open for improving the structure. If you want to do that, please create a separate PR for it. :-)

jspizziri · 2022-06-03T16:36:36Z

I'm fine with it based on your comments.

jspizziri · 2022-06-06T16:54:08Z

@fho I just added tests. I think this is ready for your review. All of it of course is dependent upon the update to the bunny-go client. So there would need to be another commit updating the vendor folder for that package before merging at the very least.

jspizziri · 2022-06-08T15:16:02Z

@fho rebased on main.

jspizziri · 2022-06-08T15:24:55Z

@fho just cleaned up the linter errors.

jspizziri · 2022-06-08T18:14:40Z

I just pushed a very small update: add the ability to import resources by id.

jspizziri · 2022-06-09T14:36:07Z

@fho any sense of when this might get merged/released? I can remove the import and put it in the other PR if that makes it easier.

fho · 2022-06-09T14:45:02Z

@jspizziri Sorry, I haven't had time to review it yet.
I hope I can test + review it latest on Tuesday.
As soon as it is in master, I can create a new release.

I can remove the import and put it in the other PR if that makes it easier.

The smaller the PR the better. If there would be an issue with the import functionality we could still merge the rest, if it is in a separate PR.
Btw, we need a testcase for the import functionality :-)

jspizziri · 2022-06-09T15:12:10Z

@fho no worries. I can use my local provider build for the time being and just disable my CD pipeline.

Btw, we need a testcase for the import functionality :-)

I figured you'd say that which is why I mentioned it 😁 . I've imported all my resources locally so it's not a blocker for me anymore. I'll put all the imports on a single PR and add testcases for them.

jspizziri · 2022-06-09T19:51:22Z

Import removed from this PR. I'll add it to another.

fho

@jspizziri looks great! I added some comments.

I noticed that forceRecreate does not work well with the storage zones because their deletion is postponed.
When an forceRecreate attribute is changed, "apply" will always fail because the old storage zone was not deleted yet and a new one with the same name can not be created.
I don't what a good way is to solve that.
Deletion seems to take a long time, the deleted storage zones I created still exist after ~>1h, so using a WaitForState func would take too long.
We could always append automatically a uuid to the storage-zone name on creation.
What do you think?

Feel free to also regenerate + commit the updated docs make docs" + to add an entry to the changelog, otherwise I can also do it later.

internal/provider/resource_storagezone.go

internal/provider/resource_storagezone_test.go

internal/provider/resource_storagezone.go

jspizziri · 2022-06-13T18:00:16Z

@fho

I've pushed a commit (which I'll squash later) that addresses your feedback with the following exceptions:

I couldn't find a good way to test the ForceNew in the CustomizeDiff. LMK if you have a clear path to this.
The storage zone recreation issue (which I'll address in long-form below)

Storage Zone Deletion/Recreation

I'm not in favor of appending a UUID to the end of the name because it's not what is actually specified as the name. As an example, we rely on a specific naming convention for my storage zone names, and appending something would throw that off. IMHO, this is just an eccentricity of Bunny which users should be aware of if they're using Bunny. I think if the provider is responsible for anything it would be a custom error message when failing due to an attempt to recreate a Zone if it already exists and is in the Deleting state (which is a property returned on the response of a Storage Zone).

I think the bigger concern with Deletes in general on a Storage Zone is that they're highly destructive actions. As, they're not simply deleting a zone, but also potentially GB's of data that is stored in that zone. All of this seems to fall in the category of "know what you're doing when you're using bunny API's".

Or perhaps we should just not allow for recreations and treat all such properties as immutable?

fho · 2022-06-14T07:49:09Z

I'm not in favor of appending a UUID to the end of the name because it's not what is actually specified as the name.

ok, I'm also not convinced about altering the name.

I think if the provider is responsible for anything it would be a custom error message when failing due to an attempt to recreate a Zone if it already exists and is in the Deleting state (which is a property returned on the response of a Storage Zone).

Ok, also fine with me that the user has to handle it.

I wonder if it is possible to represent the deletion behavior as it is on bunny.
When a Storage-Zone is deleted, the deleted attribute is set, the storage zone (it's ID) is kept.
When ReadContext is called and storage zone does not exist we unset the ID and the storage zone vanishes.

I think the bigger concern with Deletes in general on a Storage Zone is that they're highly destructive actions. As, they're not simply deleting a zone, but also potentially GB's of data that is stored in that zone. All of this seems to fall in the category of "know what you're doing when you're using bunny API's".
Or perhaps we should just not allow for recreations and treat all such properties as immutable?

Do you have a specific idea how the resource would be made immutable? By returning an error from DeleteContext()? Or is there are a better way?

I googled a bit and see 2 options:

Without requiring any support by the provider, users can set the lifecycle.prevent_destroy attribute manually on each of the storage zone, to prevent accidental deletions. (see also Make it harder to destroy resources hashicorp/terraform#24658). Therefore we could make this responsibility of the user.
The google provider prevents accidental bucket deletions, which contain objects, by having a force_destroy attribute on the resource. Only if it is set to true a bucket can be deleted.
We could have a similar attribute, called allow_deletion or so and return an error from DeleteContext() if it is not set.

Both options are reasonable for me. I slightly prefer 2 because of the additional safety.
What do you think?

jspizziri · 2022-06-14T13:46:47Z

@fho ,

I haven't tested this, based on what I learned when adding that CustomizeDiff enhancement, it might be possible there. Effectively, we would do the following:

Remove all ForceNew from all keys in the schema.
Add some HasChanged checks in theCustomizeDiff function that return an error if one of those "immutable" keys have been updated.

This way, the only way to actually "delete" a zone would be to remove the resource from your TF entirely, which seems logical to me. If you wanted to reuse the name, you would need to be aware that it just takes Bunny some time to delete the zone before you can recreate a new one with the same name. If we could do it this way then these error messages (I think) would show up during a plan.

I also think both of the options you suggested would work too, but I also prefer the 2nd of the two to make things as obvious as possible. I might slightly prefer the recommendation I just made (assuming we could even do it like that) as it makes it painfully obvious whats going on. Moreover, we're almost halfway there already given the modifications I made for replication_regions already in CustomizeDiff, we'd just need to change those to return errors instead of throwing warnings and modifying the diff with ForceNew.

Update: I just tested the method I proposed and can confirm that it works the way I anticipated. Example during a plan:

│ Error: 'replication_regions' can be added to but not be removed once the zone has been created. The Storage Zone must be deleted and recreated.
│
│   with bunny_storagezone.web,
│   on main.tf line 13, in resource "bunny_storagezone" "web":
│   13: resource "bunny_storagezone" "web" {
│
╵

We can make those error messages as explicit as necessary (including the actual diff in the values if you want).

internal/provider/resource_storagezone.go

fho

@jspizziri looks great

I fixed all the comments that I added already locally on my own.
You can ignore them. :-)

I don't have permissions to push to you repository.
I'll create a PR for your PR with some small changes instead. If you are fine with them, we can merge it to master.

Update: PR with my changes: 5-stones#1

internal/provider/resource_storagezone.go

jspizziri · 2022-06-15T11:20:50Z

Thanks for doing that!

jspizziri · 2022-06-15T12:34:47Z

Based on your earlier comment it sounds like you've already fixed the issues you mentioned. But if you need me to fix anything just let me know and I'll hop on it.

Creating a storagezone failed with the immutable attribute errors: * 'name' is immutable and cannot be changed from '' to 'tf-test-20220615075051824800000019'.[..] * 'region' is immutable and cannot be changed from '' to 'DE'. [..] Fix it by always allowing changes in validateImmutableStringProperty if the old value was an empty string.

- add newlines to make the message easier to read, - make the message vars constants,

It is not needed. d.Get() only returns the proposed state, if updating fails it is not stored as current state.

storageZoneFromResource was only setting fields in *bunny.StorageZoneUpdateOptions that changed in the resource. Change it to set always all fields to the current values. This is consistent with how other *FromResource functions are behaving. The update message sent to the bunny API will contain all settings also the current ones instead of only the changed ones. Depending on how the bunny.net endpoint handles missing fields in JSON payloads for update messages this could be better or worse. :-) If it always interprets missing fields as no change is done, it causes only unnecessary network traffic. If it interprets missing fields as the setting should be unset/disabled, it's better to include the whole configuration. The bunny.net API distinguishes between missing and empty string values for the OriginURL and Custom404FilePath fields. When submitting empty strings for those it resulted in an error, that those fields set to invalid values. To handle it the helper function getOkStrPtr() is introduced. It returns nil if the value is unset instead of an empty string. Replacing all get<TYPE>Ptr() calls with getOk<TYPE>Ptr() might make sense and should be investigated in a follow-up.

Add a testcase that ensures changing an immutable storagezone field fails

replace the randPullZoneName() and randStorageZoneName() functions that were doing the same with a function called randResourceName()

Feat/storage zone improvements

jspizziri · 2022-06-15T16:32:03Z

@fho alright, take a look at 3c1335a and let me know what you think. It addresses all the feedback I had on 5-stones#1

jspizziri · 2022-06-15T16:36:42Z

I've reached out to Bunny Support about "OriginUrl". I'm hoping we can just remove it.

internal/provider/resource_storagezone.go

fho · 2022-06-16T09:40:26Z

@jspizziri feel free to revert my storagezone: remove date_modified key commit if you prefer.
I did not realize that this is actually a field returned by the bunny api. I thought it's a field that the provider computates.

I don't know if there is a usecase for the field in the provider 🤷.

jspizziri · 2022-06-16T10:13:55Z

@jspizziri feel free to revert my storagezone: remove date_modified key commit if you prefer.

I did not realize that this is actually a field returned by the bunny api. I thought it's a field that the provider computates.

I don't know if there is a usecase for the field in the provider 🤷.

Yeah I don't really think it's useful. Especially since it will be constantly out of date unless you're refreshing your state. I'm fine leaving it out.

…t state

That after a failed update changes to the resource are still applied is a know bug (hashicorp/terraform-plugin-sdk#476). It affects attributes that are not retrieved via the Get function and set only by the provider. custom_404_file_path is such a field. As workaround d.Partial() can be called. This is more simple then the revertUpdateValues() implementation.

jspizziri · 2022-06-16T16:14:02Z

@fho alright, I think this is ready for another round of review:

Your commit has been merged.
I removed the unnecessary read/write to tf state on update
I added the default for the 404 URL that we discussed.

fho · 2022-06-19T16:04:16Z

@jspizziri

I've reached out to Bunny Support about "OriginUrl". I'm hoping we can just remove it.

Do you got a response from the bunny support?
I created #77 to track the issue.

The OriginURL issue is only a minor thing and everything else looks fine so I finally merged the PR 🥳.
Thanks a lot for your contribution and the great team work. :-)

I'll create a new release presumably end of the week.

jspizziri · 2022-06-19T16:28:37Z

I haven't seen a response yet but I'll let you know if I do.

Awesome, thanks a ton for all your help and work on getting this across the finish line!

Looking forward to using it 😎

jspizziri marked this pull request as draft June 1, 2022 18:10

jspizziri force-pushed the feat/storage-zone branch from 80ebbbc to e9791ad Compare June 6, 2022 16:52

jspizziri marked this pull request as ready for review June 6, 2022 16:53

jspizziri force-pushed the feat/storage-zone branch from e9791ad to 7347bac Compare June 8, 2022 15:15

jspizziri force-pushed the feat/storage-zone branch from 7347bac to e7e1084 Compare June 8, 2022 15:24

fho self-assigned this Jun 8, 2022

fho self-requested a review June 8, 2022 15:56

fho assigned jspizziri and unassigned fho Jun 8, 2022

fho linked an issue Jun 8, 2022 that may be closed by this pull request

[Feature Request] Storage Zones #62

Closed

jspizziri force-pushed the feat/storage-zone branch from e7e1084 to f2204f9 Compare June 8, 2022 18:14

jspizziri force-pushed the feat/storage-zone branch from f2204f9 to df036ca Compare June 9, 2022 19:50

jspizziri mentioned this pull request Jun 9, 2022

Feat/import #73

Closed

fho reviewed Jun 13, 2022

View reviewed changes

jspizziri force-pushed the feat/storage-zone branch 2 times, most recently from e7fed34 to 1f96bf3 Compare June 13, 2022 17:53

fho reviewed Jun 15, 2022

View reviewed changes

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

fho reviewed Jun 15, 2022

View reviewed changes

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

internal/provider/resource_storagezone.go Show resolved Hide resolved

fho and others added 10 commits June 15, 2022 15:50

storagezone: improve error message for immutable fields

0ebb31b

- add newlines to make the message easier to read, - make the message vars constants,

storagezone: remove date_modified key

103e042

storagezone: remove setting state to current values on failed update

932b9e2

It is not needed. d.Get() only returns the proposed state, if updating fails it is not stored as current state.

examples: add a simple storagezone resource example

cc62818

tests: add testcase for immutable storagezone fields

0764bd1

Add a testcase that ensures changing an immutable storagezone field fails

readme: mention support for storage zones

a4ca606

tests: replace randPullZoneName(), randStorageZoneName() with 1 function

ab02902

replace the randPullZoneName() and randStorageZoneName() functions that were doing the same with a function called randResourceName()

Merge pull request #1 from simplesurance/feat/storage-zone-improvements

e36ebef

Feat/storage zone improvements

fho reviewed Jun 16, 2022

View reviewed changes

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

fho reviewed Jun 16, 2022

View reviewed changes

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

jspizziri and others added 4 commits June 16, 2022 11:04

feat(storagezone): add appropriate default values for 404 properties

69eb1c6

tests: add testcase that ensures failed update does not change curren…

445fff8

…t state

fix(storagezone): fix typo in error message for immutable properties

ea76f21

jspizziri force-pushed the feat/storage-zone branch from 6677ca3 to ea76f21 Compare June 16, 2022 16:04

feat(storagezone): remove unnecessary state read/write on update

bc126db

fho merged commit ddb042c into simplesurance:main Jun 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(bunny_storagezone): add a storagezone resource #64

feat(bunny_storagezone): add a storagezone resource #64

jspizziri commented Jun 1, 2022

jspizziri commented Jun 1, 2022 •

edited

Loading

fho commented Jun 3, 2022

jspizziri commented Jun 3, 2022

jspizziri commented Jun 6, 2022

jspizziri commented Jun 8, 2022

jspizziri commented Jun 8, 2022

jspizziri commented Jun 8, 2022

jspizziri commented Jun 9, 2022

fho commented Jun 9, 2022

jspizziri commented Jun 9, 2022 •

edited

Loading

jspizziri commented Jun 9, 2022

fho left a comment

jspizziri commented Jun 13, 2022 •

edited

Loading

fho commented Jun 14, 2022

jspizziri commented Jun 14, 2022 •

edited

Loading

fho left a comment •

edited

Loading

jspizziri commented Jun 15, 2022

jspizziri commented Jun 15, 2022

jspizziri commented Jun 15, 2022

jspizziri commented Jun 15, 2022

fho commented Jun 16, 2022

jspizziri commented Jun 16, 2022

jspizziri commented Jun 16, 2022

fho commented Jun 19, 2022

jspizziri commented Jun 19, 2022

feat(bunny_storagezone): add a storagezone resource #64

feat(bunny_storagezone): add a storagezone resource #64

Conversation

jspizziri commented Jun 1, 2022

jspizziri commented Jun 1, 2022 • edited Loading

fho commented Jun 3, 2022

jspizziri commented Jun 3, 2022

jspizziri commented Jun 6, 2022

jspizziri commented Jun 8, 2022

jspizziri commented Jun 8, 2022

jspizziri commented Jun 8, 2022

jspizziri commented Jun 9, 2022

fho commented Jun 9, 2022

jspizziri commented Jun 9, 2022 • edited Loading

jspizziri commented Jun 9, 2022

fho left a comment

Choose a reason for hiding this comment

jspizziri commented Jun 13, 2022 • edited Loading

Storage Zone Deletion/Recreation

fho commented Jun 14, 2022

jspizziri commented Jun 14, 2022 • edited Loading

fho left a comment • edited Loading

Choose a reason for hiding this comment

jspizziri commented Jun 15, 2022

jspizziri commented Jun 15, 2022

jspizziri commented Jun 15, 2022

jspizziri commented Jun 15, 2022

fho commented Jun 16, 2022

jspizziri commented Jun 16, 2022

jspizziri commented Jun 16, 2022

fho commented Jun 19, 2022

jspizziri commented Jun 19, 2022

jspizziri commented Jun 1, 2022 •

edited

Loading

jspizziri commented Jun 9, 2022 •

edited

Loading

jspizziri commented Jun 13, 2022 •

edited

Loading

jspizziri commented Jun 14, 2022 •

edited

Loading

fho left a comment •

edited

Loading