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

WIP Create SANless certificate when removing all SANs from certificate #8756

Conversation

tomelliff
Copy link
Contributor

This should recreate the certificate without any SANs (other than the CN on the actual certificate rather than the API representation).

Right now if you change the SANs or add some then it will recreate the certificate but completely removing the SANs doesn't work.

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Fixes #8755

Release note for CHANGELOG:


Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccXXX'

...

This should recreate the certificate without any SANs (other than the CN on the actual certificate rather than the API representation).
Right now if you change the SANs or add some then it will recreate the certificate but completely removing the SANs doesn't work.
@ghost ghost added size/XS Managed by automation to categorize the size of a PR. service/acm Issues and PRs that pertain to the acm service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels May 23, 2019
@tomelliff
Copy link
Contributor Author

Just raised with the failing test right now as I haven't had a chance to work out what's broken here:

$ make testacc TEST=./aws TESTARGS="-run=TestAccAWSAcmCertificate_san_single"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -parallel 20 -run=TestAccAWSAcmCertificate_san_single -timeout 120m
=== RUN   TestAccAWSAcmCertificate_san_single
=== PAUSE TestAccAWSAcmCertificate_san_single
=== CONT  TestAccAWSAcmCertificate_san_single
--- FAIL: TestAccAWSAcmCertificate_san_single (21.65s)
    testing.go:568: Step 0 error: Check failed: Check 4/15 error: aws_acm_certificate.cert: Attribute 'domain_validation_options.0.domain_name' expected "tf-acc-8200070770777500028.infrastructure-dev.cloudnc.com", got "tf-acc-8200070770777500028-san.infrastructure-dev.cloudnc.com"
FAIL
FAIL	github.com/terraform-providers/terraform-provider-aws/aws	21.662s
GNUmakefile:20: recipe for target 'testacc' failed
make: *** [testacc] Error 1

@tomelliff
Copy link
Contributor Author

Hmmm, is this actually a core issue? Having not seen anything obviously wrong here I checked another Optional and ForceNew attribute and if I remove the attribute there (in this case associate_public_ip_address from aws_instance) then a plan shows nothing new to change when it should go back to the default of False or an empty list in this case.

@aeschright aeschright requested a review from a team June 28, 2019 18:15
@bflad
Copy link
Contributor

bflad commented May 27, 2020

Hi @tomelliff 👋

The behavior you are noticing with the subject_alternative_names argument is due to it being Computed: true in the schema. When schema attributes are marked with this field, Terraform will ignore changes if the argument is not present in the Terraform configuration. The schema attribute is marked like this since we added ImportCertificate API support in the resource, which only requires the private key and certificate body to be configured, while SANs are determined by the API response in that case and we do not want to force operators to extraneously need to include that configuration.

To remove all elements a TypeList/TypeSet attribute when it was previously configured with multiple elements, it can either be set to an empty list, e.g.

resource "aws_acm_certificate" "example" {
  # ... other configuration ...

  subject_alternative_names = []
}

Or the terraform taint command can be used to force Terraform to recreate the resource.

Since this pull request only contains a testing change and since we likely do not want to adjust the attribute's behavior with relation to the Computed: true field, I'm going to close this for now. We plan on updating the resource documentation to explicitly include information about the above actions very shortly as part of #13053. Thank you for submitting this though and we look forward to other contributions.

@bflad bflad closed this May 27, 2020
@ghost
Copy link

ghost commented Jun 26, 2020

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Jun 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
service/acm Issues and PRs that pertain to the acm service. size/XS Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Removing subject_alternative_names parameter doesn't force a new certificate
2 participants