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.

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

Update instead of replace google_storage_bucket_object #10488

Closed
joe-a-t opened this issue Nov 3, 2021 · 9 comments
Closed

Update instead of replace google_storage_bucket_object #10488

joe-a-t opened this issue Nov 3, 2021 · 9 comments

Comments

@joe-a-t
Copy link

joe-a-t commented Nov 3, 2021

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment. If the issue is assigned to the "modular-magician" user, it is either in the process of being autogenerated, or is planned to be autogenerated soon. If the issue is assigned to a user, that user is claiming responsibility for the issue. If the issue is assigned to "hashibot", a community member has claimed the issue already.

Description

Please add a new argument for google_storage_bucket_object that will allow people to tell Terraform that Terraform should upload a new version of the object once all of the contents are known without deleting the old object.

Right now, if the contents of google_storage_bucket_object include the output from another Terraform resource or module and those output are not fully determined until that other resource has been applied, Terraform starts the apply by removing the existing google_storage_bucket_object. It then tries to apply the other resources before it tries to upload the new google_storage_bucket_object. However, if the apply of those other resources fails, you end up in a state where it never got to trying to upload the new google_storage_bucket_object so the object no longer exists until you fix everything and as such anything downstream that consumes the google_storage_bucket_object will fail because the object doesn't exist.

In our case, we do not want Terraform to touch the google_storage_bucket_object until all of the contents have been determined and due to the way that the GCS API works, it should simply upload the new object to the same path instead of deleting the old object and uploading a completely new one. That way if something fails with the current Terraform apply, there is a chance that downstream uses of the object will still be able to work with the older version of the object (we know that's not foolproof, especially if we are updating existing values in the object instead of just adding new ones, but our use case is mainly adding new values to the object to it would be preferable for us if we simply didn't pick up the new values until they have succeeded instead of having everything downstream break). It actually looks like there might already be a code path in the provider for this, we just haven't been able to figure out how to hit it.

Example of issue

Start with the following.

locals {
  bucket_name = "FOO"
}

resource "google_storage_bucket" "bucket" {
  name                        = local.bucket_name
  location                    = "US"
  uniform_bucket_level_access = true
}

resource "google_storage_bucket_object" "object" {
  name    = "test-object"
  content = "hi"
  bucket  = local.bucket_name
}

After ^ has been successfully applied, change the file to be the following

locals {
  bucket_name = "FOO"
}

resource "google_storage_bucket" "bucket" {
  name                        = local.bucket_name
  location                    = "US"
  uniform_bucket_level_access = true
}

resource "google_storage_bucket_object" "object" {
  name    = "test-object"
  content = google_storage_bucket.bucket_two.self_link
  bucket  = local.bucket_name
}

# Intentionally using the same bucket name so that the apply will fail
resource "google_storage_bucket" "bucket_two" {
  name                        = local.bucket_name
  location                    = "US"
  uniform_bucket_level_access = true
}

That results in the following output and order of operations from the apply:

$ terraform apply
google_storage_bucket.bucket: Refreshing state... [id=FOO]
google_storage_bucket_object.object: Refreshing state... [id=FOO-test-object]

Terraform used the selected providers to generate the following execution plan.
Resource actions are indicated with the following symbols:
  + create
-/+ destroy and then create replacement

Terraform will perform the following actions:

  # google_storage_bucket.bucket_two will be created
  + resource "google_storage_bucket" "bucket_two" {
      + force_destroy               = false
      + id                          = (known after apply)
      + location                    = "US"
      + name                        = "FOO"
      + project                     = (known after apply)
      + self_link                   = (known after apply)
      + storage_class               = "STANDARD"
      + uniform_bucket_level_access = true
      + url                         = (known after apply)
    }

  # google_storage_bucket_object.object must be replaced
-/+ resource "google_storage_bucket_object" "object" {
      ~ content          = (sensitive value) # forces replacement
      ~ content_type     = "text/plain; charset=utf-8" -> (known after apply)
      ~ crc32c           = "BAR" -> (known after apply)
      ~ detect_md5hash   = "BAR" -> "different hash" # forces replacement
      - event_based_hold = false -> null
      ~ id               = "FOO-test-object" -> (known after apply)
      + kms_key_name     = (known after apply)
      ~ md5hash          = "BAR" -> (known after apply)
      ~ media_link       = "https://storage.googleapis.com/download/storage/v1/b/FOO/o/test-object?generation=BAR&alt=media" -> (known after apply)
      - metadata         = {} -> null
        name             = "test-object"
      ~ output_name      = "test-object" -> (known after apply)
      ~ self_link        = "https://www.googleapis.com/storage/v1/b/FOO/o/test-object" -> (known after apply)
      ~ storage_class    = "STANDARD" -> (known after apply)
      - temporary_hold   = false -> null
        # (1 unchanged attribute hidden)
    }

Plan: 2 to add, 0 to change, 1 to destroy.

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

google_storage_bucket_object.object: Destroying... [id=FOO-test-object]
google_storage_bucket_object.object: Destruction complete after 0s
google_storage_bucket.bucket_two: Creating...
╷
│ Error: googleapi: Error 409: You already own this bucket. Please select another name., conflict
│ 
│   with google_storage_bucket.bucket_two,
│   on main.tf line 22, in resource "google_storage_bucket" "bucket_two":
│   22: resource "google_storage_bucket" "bucket_two" {

At the end of the day, test-object does not exist in my bucket but my desired state would be that test-object still exists with the content "hi" until after I've fixed the configuration so that google_storage_bucket.bucket_two has successfully created and the new content of test-object is known and has been successfully uploaded to GCS.

New or Affected Resource(s)

  • google_storage_bucket_object

Potential Terraform Configuration

# Propose what you think the configuration to take advantage of this feature should look like.
# We may not use it verbatim, but it's helpful in understanding your intent.
resource "google_storage_bucket_object" "object" {
  name           = "test-object"
  content        = "hi"
  bucket         = local.bucket_name
  update_content = true
}

To handcraft some rough terraform apply output, here is what I am envisioning:

$ terraform apply
google_storage_bucket.bucket: Refreshing state... [id=FOO]
google_storage_bucket_object.object: Refreshing state... [id=FOO-test-object]

Terraform used the selected providers to generate the following execution plan.
Resource actions are indicated with the following symbols:
  + create
  ~ update in-place

Terraform will perform the following actions:

  # google_storage_bucket.bucket_two will be created
  + resource "google_storage_bucket" "bucket_two" {
      + force_destroy               = false
      + id                          = (known after apply)
      + location                    = "US"
      + name                        = "FOO"
      + project                     = (known after apply)
      + self_link                   = (known after apply)
      + storage_class               = "STANDARD"
      + uniform_bucket_level_access = true
      + url                         = (known after apply)
    }

  # google_storage_bucket_object.object will be updated in-place
  ~ resource "google_storage_bucket_object" "object" {
      ~ content          = (sensitive value)
      ~ content_type     = "text/plain; charset=utf-8" -> (known after apply)
      ~ crc32c           = "BAR" -> (known after apply)
      ~ detect_md5hash   = "BAR" -> "different hash"
      - event_based_hold = false -> null
      ~ id               = "FOO-test-object" -> (known after apply)
      + kms_key_name     = (known after apply)
      ~ md5hash          = "BAR" -> (known after apply)
      ~ media_link       = "https://storage.googleapis.com/download/storage/v1/b/FOO/o/test-object?generation=BAR&alt=media" -> (known after apply)
      - metadata         = {} -> null
        name             = "test-object"
      ~ output_name      = "test-object" -> (known after apply)
      ~ self_link        = "https://www.googleapis.com/storage/v1/b/FOO/o/test-object" -> (known after apply)
      ~ storage_class    = "STANDARD" -> (known after apply)
      - temporary_hold   = false -> null
        # (1 unchanged attribute hidden)
    }

Plan: 1 to add, 1 to change, 0 to destroy.

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

google_storage_bucket.bucket_two: Creating...
╷
│ Error: googleapi: Error 409: You already own this bucket. Please select another name., conflict
│ 
│   with google_storage_bucket.bucket_two,
│   on main.tf line 22, in resource "google_storage_bucket" "bucket_two":
│   22: resource "google_storage_bucket" "bucket_two" {

References

  • b/275620571
@rileykarson rileykarson self-assigned this Nov 8, 2021
@joe-a-t
Copy link
Author

joe-a-t commented Mar 29, 2022

@rileykarson any update on this? It does cause us fairly regular pain.

@rileykarson
Copy link
Collaborator

I assigned this to myself a while ago to write up a closure reason for, but it turns out to be a little more complicated than anticipated. The facts:

  • The update and patch methods in the API only support updating the metadata. Those are what we call from the update path in the resource.
  • The behaviour of insert is close to PUT/upsert "Stores a new object and metadata. The uploaded object replaces any existing object with the same name.", with a restriction against metadata-only updates ("Note: Metadata-only requests are not allowed. To change an object's metadata, use either the update or patch methods.") that means it isn't a true PUT method
  • Normally in these cases we'd recommend https://www.terraform.io/language/meta-arguments/lifecycle#create_before_destroy, although that has issues with GCP fairly often, because the name of the resource is reserved by the about-to-be-deleted copy. On this resource though, it would upsert the contents and then proceed to delete them.

I think a reasonable solution here would be to add a field to the resource that controls the delete behaviour, skip_destroy in a similar vein to https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/google_project_service#disable_on_destroy or https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/app_engine_flexible_app_version#noop_on_destroy. That way, the resource will behave as it currently does for most users, but users that want to do an in-place replacement will be able to specify the create_before_destroy lifecycle parameter along with skip_destroy = true to have Terraform upsert the resource. We'll automatically handle metadata-only updates that way too, since Terraform won't cause a recreate in those cases and we'll call update/patch.

I'll note that that approach doesn't handle permanent deletion well. We can't tell if we're doing a deletion due to removal from the config, terraform destroy, or due to a recreate in the provider (Core strips the info) so the skip_destroy behaviour will be applied indiscriminately. Despite that, I think it's still the best approach here.


I was assigned here to prepare this for triage, so I'll unassign myself now and it will go through that next week.

@rileykarson rileykarson removed their assignment Apr 21, 2022
@joe-a-t
Copy link
Author

joe-a-t commented Apr 21, 2022

That is... fun. Thanks a lot for all the info. So if I understand correctly the flow would be

  1. Add create_before_destroy lifecycle parameter
  2. Add skip_destroy = true to google_storage_bucket_object resource
  3. Terraform updates the contents of the object per the create part of create_before_destroy
  4. Terraform skips the destroy part of create_before_destroy because of skip_destroy
  5. If someone tried to remove the object permanently without changing skip_destroy, the object would just be orphaned.
  6. If someone wants Terraform to remove the object permanently, it would only happen if they changed skip_destroy to false (kind of like how storage_bucket#force_destroy or sql_database_instance#deletion_protection work except the new status of skip_destroy would not need to be applied separately and in the state file already).

If that understanding is correct, that flow makes sense to me and would be extremely useful despite the permanent deletion gotcha. It would be nice if there was a way to set it via a single flag on google_storage_bucket_object instead of needing to set both create_before_destroy lifecycle parameter and skip_destroy = true on the google_storage_bucket_object resource, but even with needing both arguments it will accomplish our objectives.

@rileykarson
Copy link
Collaborator

kind of like how storage_bucket#force_destroy or sql_database_instance#deletion_protection work except the new status of skip_destroy would not need to be applied separately and in the state file already

Ah, not quite- it'd have the same limitations those do, as does any destroy-time provider-level field that's similar. The value needs to be committed to state in an apply because Terraform short-circuits on deletes and does not update the state based on new changes to config (this makes sense for identify fields that get changed, for example, but doesn't work as well for these pseudo-lifecycle fields). In contrast, Terraform Core's lifecycle fields are config-only and never get recorded in state so new fields do work, but if the resource config is removed they no longer work.

Otherwise, you've got it!

It's possible such an approach is overcomplicating things. We could always do whole-resource update in the update method, which would mean the resource would no longer recreate, although we'd need to detect the metadata-only case in just provider code. That can be tricky- the information Core gives us is lossy compared to what it had available in plan.

@rileykarson rileykarson added this to the Goals milestone Apr 25, 2022
@gustavovalverde
Copy link

gustavovalverde commented Jun 30, 2022

I'm having this issue after applying 01-resman from Fast stages (https://github.com/GoogleCloudPlatform/cloud-foundation-fabric/tree/master/fast/stages/01-resman), destroying and trying to reapply with a different configuration:

module.branch-pf-dev-gcs[0].google_storage_bucket.bucket: Creating...
module.branch-pf-prod-gcs[0].google_storage_bucket.bucket: Creating...
module.branch-network-gcs.google_storage_bucket.bucket: Creating...
module.branch-security-gcs.google_storage_bucket.bucket: Creating...
╷
│ Error: googleapi: Error 409: Your previous request to create the named bucket succeeded and you already own it., conflict
│ 
│   with module.branch-network-gcs.google_storage_bucket.bucket,
│   on .terraform/modules/branch-network-gcs/modules/gcs/main.tf line 26, in resource "google_storage_bucket" "bucket":
│   26: resource "google_storage_bucket" "bucket" {
│ 
╵
╷
│ Error: googleapi: Error 409: Your previous request to create the named bucket succeeded and you already own it., conflict
│ 
│   with module.branch-pf-dev-gcs[0].google_storage_bucket.bucket,
│   on .terraform/modules/branch-pf-dev-gcs/modules/gcs/main.tf line 26, in resource "google_storage_bucket" "bucket":
│   26: resource "google_storage_bucket" "bucket" {
│ 
╵
╷
│ Error: googleapi: Error 409: Your previous request to create the named bucket succeeded and you already own it., conflict
│ 
│   with module.branch-pf-prod-gcs[0].google_storage_bucket.bucket,
│   on .terraform/modules/branch-pf-prod-gcs/modules/gcs/main.tf line 26, in resource "google_storage_bucket" "bucket":
│   26: resource "google_storage_bucket" "bucket" {
│ 
╵
╷
│ Error: googleapi: Error 409: Your previous request to create the named bucket succeeded and you already own it., conflict
│ 
│   with module.branch-security-gcs.google_storage_bucket.bucket,
│   on .terraform/modules/branch-security-gcs/modules/gcs/main.tf line 26, in resource "google_storage_bucket" "bucket":
│   26: resource "google_storage_bucket" "bucket" {
│ 
╵

I'm not sure how to proceed after we hit this scenario.

@joe-a-t
Copy link
Author

joe-a-t commented Jul 5, 2022

@gustavovalverde That is definitely a very different issue, I would recommend you create a separate issue since issues re-creating buckets is unrelated to issues updating objects within a bucket

@Maarc-D
Copy link

Maarc-D commented Feb 23, 2024

I did create a PR on the repo that generate the provider to fix this delete create issue, hope it will be approve and will help all of you as it will help me : GoogleCloudPlatform/magic-modules#10038

@joe-a-t
Copy link
Author

joe-a-t commented Jun 24, 2024

@rileykarson @BBBmau I think this can be closed now? Maybe something went wrong with the auto-close linking you normally do?

@BBBmau BBBmau closed this as completed Jun 24, 2024
Copy link

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants