Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Feat/storage zone improvements #1

Merged
merged 9 commits into from
Jun 15, 2022
Merged

Feat/storage zone improvements #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
89bda18
storagezone: fix: creation fails
fho Jun 15, 2022
0ebb31b
storagezone: improve error message for immutable fields
fho Jun 15, 2022
103e042
storagezone: remove date_modified key
fho Jun 15, 2022
932b9e2
storagezone: remove setting state to current values on failed update
fho Jun 15, 2022
773b9be
simplify storageZoneFromResource
fho Jun 15, 2022
cc62818
examples: add a simple storagezone resource example
fho Jun 15, 2022
0764bd1
tests: add testcase for immutable storagezone fields
fho Jun 15, 2022
a4ca606
readme: mention support for storage zones
fho Jun 15, 2022
ab02902
tests: replace randPullZoneName(), randStorageZoneName() with 1 function
fho Jun 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

This repository provides a [Terraform](https://terraform.io) provider for the
[Bunny.net CDN platform](https://bunny.net/). \
It currently only supports to manage Pull Zones.
It supports to manage Pull and Storage Zones.

## Development

Expand Down
1 change: 0 additions & 1 deletion docs/resources/storagezone.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ description: |-

### Read-Only

- `date_modified` (String) The last modified date of the storage zone.
- `deleted` (Boolean)
- `files_stored` (Number) The number of files stored in the storage zone.
- `id` (String) The ID of this resource.
Expand Down
4 changes: 4 additions & 0 deletions examples/resources/storagezone_resource/basic.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
resource "bunny_storagezone" "mysz" {
name = "testsz"
region = "DE"
}
12 changes: 6 additions & 6 deletions internal/provider/resource_edgerule_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ func defPullZoneHostname(pullzoneName string) string {
}

func TestAccEdgeRule_full(t *testing.T) {
pzName := randPullZoneName()
pzName := randResourceName()

tfPz := fmt.Sprintf(`
resource "bunny_pullzone" "mypz" {
Expand Down Expand Up @@ -311,7 +311,7 @@ resource "bunny_edgerule" "er3" {
}

func TestAccEdgeRule_basic(t *testing.T) {
pzName := randPullZoneName()
pzName := randResourceName()
tf := fmt.Sprintf(`
resource "bunny_pullzone" "mypz" {
name = "%s"
Expand Down Expand Up @@ -363,7 +363,7 @@ resource "bunny_edgerule" "myer" {
}

func TestAccEdgeRule_delete(t *testing.T) {
pzName := randPullZoneName()
pzName := randResourceName()

tfPz := fmt.Sprintf(`
resource "bunny_pullzone" "mypz" {
Expand Down Expand Up @@ -421,7 +421,7 @@ resource "bunny_pullzone" "mypz" {
}

func TestAccEdgeRule_enable_disable(t *testing.T) {
pzName := randPullZoneName()
pzName := randResourceName()

tfPz := fmt.Sprintf(`
resource "bunny_pullzone" "mypz" {
Expand Down Expand Up @@ -558,8 +558,8 @@ resource "bunny_edgerule" "er2" {
}

func TestAccEdgeRule_changePullZoneID(t *testing.T) {
pzName1 := randPullZoneName()
pzName2 := randPullZoneName()
pzName1 := randResourceName()
pzName2 := randResourceName()

tfPz := fmt.Sprintf(`
resource "bunny_pullzone" "pz1" {
Expand Down
16 changes: 8 additions & 8 deletions internal/provider/resource_hostname_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ func sortHostnames(hostnames []*bunny.Hostname) {
}

func TestAccHostname_basic(t *testing.T) {
pzName := randPullZoneName()
pzName := randResourceName()
tf := fmt.Sprintf(`
resource "bunny_pullzone" "pz" {
name = "%s"
Expand Down Expand Up @@ -121,7 +121,7 @@ resource "bunny_hostname" "h1" {
}

func TestAccHostname_addRemove(t *testing.T) {
pzName := randPullZoneName()
pzName := randResourceName()
hostname1 := randHostname()
hostname2 := randHostname()
hostname3 := randHostname()
Expand Down Expand Up @@ -244,7 +244,7 @@ resource "bunny_hostname" "h2" {
}

func TestAccHostname_DefiningDuplicateHostnamesFails(t *testing.T) {
pzName := randPullZoneName()
pzName := randResourceName()
tf := fmt.Sprintf(`
resource "bunny_pullzone" "pz" {
name = "%s"
Expand Down Expand Up @@ -275,7 +275,7 @@ resource "bunny_hostname" "h2" {
}

func TestAccHostname_DefiningDefPullZoneHostnameFails(t *testing.T) {
pzName := randPullZoneName()
pzName := randResourceName()
tf := fmt.Sprintf(`
resource "bunny_pullzone" "pz" {
name = "%s"
Expand All @@ -300,7 +300,7 @@ resource "bunny_hostname" "h1" {
}

func TestAccCertificateOneof(t *testing.T) {
pzName := randPullZoneName()
pzName := randResourceName()
tf := fmt.Sprintf(`
resource "bunny_pullzone" "pz" {
name = "%s"
Expand Down Expand Up @@ -333,7 +333,7 @@ resource "bunny_hostname" "h1" {
}

func TestAccCertificateCanBeSetWhenLoadFreeCertIsDisabled(t *testing.T) {
pzName := randPullZoneName()
pzName := randResourceName()
tf := fmt.Sprintf(`
resource "bunny_pullzone" "pz" {
name = "%s"
Expand Down Expand Up @@ -366,7 +366,7 @@ resource "bunny_hostname" "h1" {
}

func TestAccCertificates(t *testing.T) {
pzName := randPullZoneName()
pzName := randResourceName()
hostname := randHostname()

resource.Test(t, resource.TestCase{
Expand Down Expand Up @@ -486,7 +486,7 @@ resource "bunny_hostname" "h1" {
func TestAccHostname_StateIsValidWhenCertUploadFails(t *testing.T) {
t.Skip("disabled, because test sends 800kiB of bogus data to bunny api, which is not kind")

pzName := randPullZoneName()
pzName := randResourceName()
hostname := randHostname()

// The bunny API does not return an error if the posted data is not a
Expand Down
12 changes: 4 additions & 8 deletions internal/provider/resource_pullzone_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,6 @@ import (
bunny "github.com/simplesurance/bunny-go"
)

func randPullZoneName() string {
return resource.PrefixedUniqueId(resourcePrefix)
}

func randHostname() string {
return resource.PrefixedUniqueId(resourcePrefix) + ".test"
}
Expand Down Expand Up @@ -173,7 +169,7 @@ func TestAccPullZone_basic(t *testing.T) {
*/
attrs := pullZoneWanted{
TerraformResourceName: "bunny_pullzone.mytest1",
Name: randPullZoneName(),
Name: randResourceName(),
OriginURL: "https://tabletennismap.de",
EnableGeoZoneAsia: true,
EnableGeoZoneEU: true,
Expand Down Expand Up @@ -285,7 +281,7 @@ func TestAccPullZone_full(t *testing.T) {
VerifyOriginSSL: ptr.ToBool(true),
ZoneSecurityEnabled: ptr.ToBool(true),
ZoneSecurityIncludeHashRemoteIP: ptr.ToBool(false),
Name: ptr.ToString(randPullZoneName()),
Name: ptr.ToString(randResourceName()),
// TODO: Test StorageZoneID
ZoneSecurityKey: ptr.ToString("xyz"),

Expand Down Expand Up @@ -523,7 +519,7 @@ resource "bunny_pullzone" "%s" {
}

func TestAccPullZone_CaseInsensitiveOrderIndependentFields(t *testing.T) {
pzName := randPullZoneName()
pzName := randResourceName()

resource.Test(t, resource.TestCase{
Providers: testProviders,
Expand Down Expand Up @@ -772,7 +768,7 @@ func pzDiff(t *testing.T, a, b interface{}) []string {
}

func TestAccPullZone_OriginURLAndStorageZoneIDAreExclusive(t *testing.T) {
pzName := randPullZoneName()
pzName := randResourceName()

resource.Test(t, resource.TestCase{
Providers: testProviders,
Expand Down
76 changes: 16 additions & 60 deletions internal/provider/resource_storagezone.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import (
const (
keyUserID = "user_id"
keyPassword = "password"
keyDateModified = "date_modified"
keyDeleted = "deleted"
keyStorageUsed = "storage_used"
keyFilesStored = "files_stored"
Expand Down Expand Up @@ -94,11 +93,6 @@ func resourceStorageZone() *schema.Resource {
Computed: true,
Sensitive: true,
},
keyDateModified: {
Type: schema.TypeString,
Description: "The last modified date of the storage zone.",
Computed: true,
},
keyDeleted: {
Type: schema.TypeBool,
Computed: true,
Expand Down Expand Up @@ -166,6 +160,10 @@ func validateImmutableStringProperty(key string, old interface{}, new interface{
o := old.(string)
n, nok := new.(string)

if o == "" {
return nil
}

if new == nil || !nok {
return immutableStringPropertyError(key, o, "")
}
Expand All @@ -178,16 +176,16 @@ func validateImmutableStringProperty(key string, old interface{}, new interface{
}

func immutableStringPropertyError(key string, old string, new string) error {
message := "'%s' is immutable and cannot be changed from '%s' to '%s'. " +
"If you must change the '%s' of our region you must first delete your resource and then redefine it. " +
const message = "'%s' is immutable and cannot be changed from '%s' to '%s'.\n" +
"If you must change the '%s' of our region, first delete your resource and then redefine it.\n" +
"WARNING: deleting a 'bunny_storagezone' will also delete all the data it contains!"
return fmt.Errorf(message, key, old, new, key)
}

func immutableReplicationRegionError(key string, removed []interface{}) error {
message := "'%s' can be added to but not be removed once the zone has been created. " +
"This error occurred when attempting to remove values %+q from '%s'. " +
"To remove an existing '%s' the 'bunny_storagezone' must be deleted and recreated. " +
const message = "'%s' can be added but not removed once the zone has been created.\n" +
"This error occurred when attempting to remove values %+q from '%s'.\n" +
"To remove an existing '%s' the 'bunny_storagezone' must be deleted and recreated.\n" +
"WARNING: deleting a 'bunny_storagezone' will also delete all the data it contains!"
return fmt.Errorf(
message,
Expand Down Expand Up @@ -244,10 +242,7 @@ func resourceStorageZoneCreate(ctx context.Context, d *schema.ResourceData, meta
func resourceStorageZoneUpdate(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics {
clt := meta.(*bunny.Client)

storageZone, err := storageZoneFromResource(d)
if err != nil {
return diagsErrFromErr("converting resource to API type failed", err)
}
storageZone := storageZoneFromResource(d)

id, err := getIDAsInt64(d)
if err != nil {
Expand All @@ -256,14 +251,6 @@ func resourceStorageZoneUpdate(ctx context.Context, d *schema.ResourceData, meta

updateErr := clt.StorageZone.Update(ctx, id, storageZone)
if updateErr != nil {
// if our update failed then revert our values to their original
// state so that we can run an apply again.
revertErr := revertUpdateValues(d)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fho this is actually currently an important bit, the reason is that there are certain values that never get read out of bunny state on a get (specifically the ones referenced in revertUpdateValues). Additionally, there is validation of these properties that happens on Bunny's side which can fail. If that validation fails on an apply we need to manually revert those values back to what they were before otherwise TF gets confused.

Thats not super clear, so here's a concrete example:

  1. Add custom_404_file_path = "/foo.html" to your resource
  2. Run an apply (this will succeed and correctly modify that value)
  3. Update custom_404_file_path = "/bar" on your resource
  4. Run an apply this apply will fail due to a validation error in Bunny
  5. Run a plan immediately after the failed apply and see that TF now thinks that the state is up-to-date even though it's incorrect as the path is actually /foo.html and not /bar.

To my knowledge, this is an artifact of how those properties can't be read out of Bunny state so we need to infer their state based on successful or errored responses on update.

Hopefully that makes sense. If there's a better way to handle this I'm all ears.


if revertErr != nil {
return diagsErrFromErr("updating storage zone via API failed", revertErr)
}

return diagsErrFromErr("updating storage zone via API failed", updateErr)
}

Expand Down Expand Up @@ -332,9 +319,6 @@ func storageZoneToResource(sz *bunny.StorageZone, d *schema.ResourceData) error
if err := d.Set(keyPassword, sz.Password); err != nil {
return err
}
if err := d.Set(keyDateModified, sz.DateModified); err != nil {
return err
}
if err := d.Set(keyDeleted, sz.Deleted); err != nil {
return err
}
Expand All @@ -357,41 +341,13 @@ func storageZoneToResource(sz *bunny.StorageZone, d *schema.ResourceData) error
return nil
}

func revertUpdateValues(d *schema.ResourceData) error {
o, _ := d.GetChange(keyOriginURL)
if err := d.Set(keyOriginURL, o); err != nil {
return err
}
o, _ = d.GetChange(keyCustom404FilePath)
if err := d.Set(keyCustom404FilePath, o); err != nil {
return err
}
o, _ = d.GetChange(keyRewrite404To200)
if err := d.Set(keyRewrite404To200, o); err != nil {
return err
}

return nil
}

// storageZoneFromResource returns a StorageZoneUpdateOptions API type that
// has fields set to the values in d.
func storageZoneFromResource(d *schema.ResourceData) (*bunny.StorageZoneUpdateOptions, error) {
var res bunny.StorageZoneUpdateOptions

res.ReplicationRegions = getStrSetAsSlice(d, keyReplicationRegions)

if d.HasChange(keyOriginURL) {
res.OriginURL = getStrPtr(d, keyOriginURL)
}

if d.HasChange(keyCustom404FilePath) {
res.Custom404FilePath = getStrPtr(d, keyCustom404FilePath)
}

if d.HasChange(keyRewrite404To200) {
res.Rewrite404To200 = getBoolPtr(d, keyRewrite404To200)
func storageZoneFromResource(d *schema.ResourceData) *bunny.StorageZoneUpdateOptions {
return &bunny.StorageZoneUpdateOptions{
ReplicationRegions: getStrSetAsSlice(d, keyReplicationRegions),
jspizziri marked this conversation as resolved.
Show resolved Hide resolved
OriginURL: getOkStrPtr(d, keyOriginURL),
Custom404FilePath: getOkStrPtr(d, keyCustom404FilePath),
Rewrite404To200: getBoolPtr(d, keyRewrite404To200),
}

return &res, nil
}
Loading