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

cloudflare_origin_ca_certificate diff lead to new certificate creation #1031

Closed
azhurbilo opened this issue Apr 15, 2021 · 6 comments · Fixed by #1078
Closed

cloudflare_origin_ca_certificate diff lead to new certificate creation #1031

azhurbilo opened this issue Apr 15, 2021 · 6 comments · Fixed by #1078
Labels
triage/accepted Indicates an issue or PR is ready to be actively worked on. workflow/pr-attached Indicates the issue has PR(s) attached.

Comments

@azhurbilo
Copy link

azhurbilo commented Apr 15, 2021

Confirmation

My issue isn't already found on the issue tracker.
I have replicated my issue using the latest version of the provider and it is still present.

Terraform version

Terraform v0.12.24

Affected resource(s)

cloudflare_origin_ca_certificate

Terraform configuration files

resource "cloudflare_origin_ca_certificate" "ca" {
  csr                = tls_cert_request.cloudflare_origin_ca.cert_request_pem
  hostnames          = ["XXXX"]
  request_type       = "origin-rsa"
  requested_validity = 5475
}

resource "aws_iam_server_certificate" "cloudflare_origin_ca" {
  name_prefix      = "cloudflare_origin_ca"
  certificate_body = cloudflare_origin_ca_certificate.ca.certificate
  private_key      = tls_private_key.cloudflare_origin_ca.private_key_pem

  lifecycle {
    create_before_destroy = true
  }
}

Debug output

No response

Panic output

No response

Expected output

Maybe it's possible to show that new "cloudflare_origin_ca_certificate" resource will be created

Actual output

3 step diff, but as result new certificate was generated

# cloudflare_origin_ca_certificate.ca will be updated in-place
  ~ resource "cloudflare_origin_ca_certificate" "ca" {
        ....
        id                 = "XXXXXX"
        request_type       = "origin-rsa"
      ~ requested_validity = 5059 -> 5475
    }
Plan: 0 to add, 2 to change, 0 to destroy.

and next terraform run we got new diff related to other resources
4 step diff >>

 # aws_iam_server_certificate.cloudflare_origin_ca must be replaced
+/- resource "aws_iam_server_certificate" "cloudflare_origin_ca" {
      ~ arn              = "xxxxx" -> (known after apply)
      ~ certificate_body = <<~EOT # forces replacement
      ...
      ~ expiration       = "xxxx" -> (known after apply)
      ~ id               = "xxxxx" -> (known after apply)
      ~ name             = "xxxx" -> (known after apply)
        name_prefix      = "xxxx"
        path             = "/"
      ~ private_key      = (sensitive value)
      - tags             = {} -> null
      ~ upload_date      = "xxxx" -> (known after apply)
}

Steps to reproduce

  1. use cloudflare provider 2.7.0 with "requested_validity" bug (in TF state requested_validity value = 5059 but valid value is 5475)
  2. upgrade cloudflare provider to 2.20.0
  3. run TF plan/apply one time -> shows "updated in-place" but in reality create new "cloudflare_origin_ca_certificate"
  4. run TF plan/apply second time -> change all other TF resources who depedns on "cloudflare_origin_ca_certificate"

Additional factoids

As I understand (3 step) diff appears because of @jacobbednarz fix #955

as workaround we add

lifecycle {
    ignore_changes = [requested_validity]
  }

but in theory the same situation could happen with other fields too as it's not possible to modify cloudflare certificate (only revoke or create new one)

References

No response

@azhurbilo azhurbilo added kind/bug Categorizes issue or PR as related to a bug. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Apr 15, 2021
@jacobbednarz
Copy link
Member

@azhurbilo what version of the Cloudflare provider are you using? 20.7.0 isn't a version we have released.

@jacobbednarz jacobbednarz added triage/needs-information Indicates an issue needs more information in order to work on it. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. kind/bug Categorizes issue or PR as related to a bug. labels Apr 18, 2021
@azhurbilo
Copy link
Author

@kawikao
Copy link

kawikao commented May 6, 2021

Confirmed on provider v2.20.0.

@DavidOliver
Copy link

Is the issue here that it's not possible to simply keep (not "update in-place") the already-generated origin certificate, due to the requested validity being different from the remaining validity?

I'm currently using this to avoid updating:

resource "cloudflare_origin_ca_certificate" "all" {
  for_each           = local.sites
  csr                = tls_cert_request.all[each.key].cert_request_pem
  hostnames          = [lookup(each.value, "hostname")]
  request_type       = "origin-rsa"
  requested_validity = 5475

  # Avoid certificates being unnecessarily regenerated due to requested validity, which would necessitate redeployment
  lifecycle {
    ignore_changes = [
      requested_validity,
    ]
  }
}

Possibly relevant: https://www.reddit.com/r/Terraform/comments/gi7yw7/recreate_resource_based_on_days_remaining/

jacobbednarz added a commit that referenced this issue May 26, 2021
This is an optional field and not required for anything other than new
resources so it's fine to suppress all diffs for the field.

Closes #1031
@jacobbednarz jacobbednarz added triage/accepted Indicates an issue or PR is ready to be actively worked on. workflow/pr-attached Indicates the issue has PR(s) attached. and removed triage/needs-information Indicates an issue needs more information in order to work on it. labels May 26, 2021
@jacobbednarz
Copy link
Member

a PR is up for essentially ignoring this field after creation at # #1078 but still allowing it to be computed. i'd recommend you check it, build the provider locally and confirm it meets your expectations before it is merged.

@kawikao
Copy link

kawikao commented Jun 1, 2021

I can confirm that the update in-place message does not appear with the #1078 branch. Thanks.

boekkooi-lengoo added a commit to boekkooi-lengoo/terraform-provider-cloudflare that referenced this issue Mar 10, 2022
Instead of trying and failing to calculate the requested validity of the certificate we can use the returned validity.
It looks like `requested_validity` was being used in 2 ways.
The first one was to request the certificate for a time period and the second to indicate the amount of day the certificate is still valid.

The calculation is no longer done and could be seen as a BC break but since the current setup is causing issue like cloudflare#1148, cloudflare#1276 and cloudflare#1031. I expect that this is acceptable.

Related API docs: https://api.cloudflare.com/#origin-ca-get-certificate
boekkooi-lengoo added a commit to boekkooi-lengoo/terraform-provider-cloudflare that referenced this issue Mar 10, 2022
Instead of trying and failing to calculate the requested validity of the certificate we can use the returned validity.
It looks like `requested_validity` was being used in 2 ways.
The first one was to request the certificate for a time period and the second to indicate the amount of day the certificate is still valid.

The calculation is no longer done and could be seen as a BC break but since the current setup is causing issue like cloudflare#1148, cloudflare#1276 and cloudflare#1031. I expect that this is acceptable.

Related API docs: https://api.cloudflare.com/#origin-ca-get-certificate
boekkooi-lengoo added a commit to boekkooi-lengoo/terraform-provider-cloudflare that referenced this issue Mar 10, 2022
Instead of trying and failing to calculate the requested validity of the certificate we can use the returned validity.
It looks like `requested_validity` was being used in 2 ways.
The first one was to request the certificate for a time period and the second to indicate the amount of day the certificate is still valid.

The calculation is no longer done and could be seen as a BC break but since the current setup is causing issue like cloudflare#1148, cloudflare#1276 and cloudflare#1031. I expect that this is acceptable.

Related API docs: https://api.cloudflare.com/#origin-ca-get-certificate
boekkooi-lengoo added a commit to boekkooi-lengoo/terraform-provider-cloudflare that referenced this issue Mar 10, 2022
Instead of trying and failing to calculate the requested validity of the certificate we can use the returned validity.
It looks like `requested_validity` was being used in 2 ways.
The first one was to request the certificate for a time period and the second to indicate the amount of day the certificate is still valid.

The calculation is no longer done and could be seen as a BC break but since the current setup is causing issue like cloudflare#1448, cloudflare#1276 and cloudflare#1031. I expect that this is acceptable.

Related API docs: https://api.cloudflare.com/#origin-ca-get-certificate
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage/accepted Indicates an issue or PR is ready to be actively worked on. workflow/pr-attached Indicates the issue has PR(s) attached.
Projects
None yet
4 participants