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

[datadog_synthetics_test] Fix multistep client certificate #2683

Conversation

AntoineDona
Copy link
Contributor

Description

This PR was created in response to this issue where applying a multistep test with a client certificate multiple times would result in the certificate being deleted in the backend.
The tricky thing here is that the certificate is never stored in the tf state for security reasons, and we pass an empty value to the backend if its value does not change. Because we don't have the step ids in terraform, the backend can't partially update the step and just overrides everything, thus the certificate is destroyed.

As a fix, we decided to pass the certificate on each tf update.

@AntoineDona AntoineDona marked this pull request as ready for review November 19, 2024 16:34
@AntoineDona AntoineDona requested review from a team as code owners November 19, 2024 16:34
@AntoineDona AntoineDona changed the title [SYNTH-17025] Fix multistep client certificate [datadog_synthetics_test] [SYNTH-17025] Fix multistep client certificate Nov 19, 2024
@AntoineDona AntoineDona changed the title [datadog_synthetics_test] [SYNTH-17025] Fix multistep client certificate [datadog_synthetics_test] Fix multistep client certificate Nov 19, 2024
Copy link
Contributor

@Drarig29 Drarig29 left a comment

Choose a reason for hiding this comment

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

Great work! Do you have an idea of how to capture this in unit tests?

datadog/resource_datadog_synthetics_test_.go Show resolved Hide resolved
datadog/resource_datadog_synthetics_test_.go Outdated Show resolved Hide resolved
datadog/resource_datadog_synthetics_test_.go Outdated Show resolved Hide resolved
return &certString, &keyString
}

func overrideStateCertificate(requestClientCertificates []interface{}, configCert, configKey string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't do anything with the returned error - is it expected?

etnbrd
etnbrd previously approved these changes Nov 20, 2024
Copy link
Contributor

@etnbrd etnbrd left a comment

Choose a reason for hiding this comment

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

Overall LGTM, great work!
Left a few comments on structure to keep the provider implementation moving into the right direction.

datadog/resource_datadog_synthetics_test_.go Outdated Show resolved Hide resolved
datadog/resource_datadog_synthetics_test_.go Outdated Show resolved Hide resolved
datadog/resource_datadog_synthetics_test_.go Outdated Show resolved Hide resolved
datadog/resource_datadog_synthetics_test_.go Outdated Show resolved Hide resolved
etnbrd
etnbrd previously approved these changes Dec 12, 2024
Copy link
Contributor

@etnbrd etnbrd left a comment

Choose a reason for hiding this comment

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

I understand it's not the cleanest, but it's good enough like that. With buildDatadogRequestCertificates aligned, we'll try to align further the retrieval later.
Great job! Thanks a lot for taking into account our reviews 🙇

Copy link
Contributor

@etnbrd etnbrd left a comment

Choose a reason for hiding this comment

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

🚀

@AntoineDona
Copy link
Contributor Author

/merge

@dd-devflow
Copy link

dd-devflow bot commented Dec 13, 2024

Devflow running: /merge

View all feedbacks in Devflow UI.


2024-12-13 12:56:34 UTC ℹ️ MergeQueue: pull request added to the queue

The median merge time in master is 0s.


2024-12-13 12:57:52 UTC ℹ️ MergeQueue: This merge request was merged

@dd-mergequeue dd-mergequeue bot merged commit 146c518 into master Dec 13, 2024
18 of 19 checks passed
@dd-mergequeue dd-mergequeue bot deleted the antoine.donascimento/SYNTH-17025/fix-multistep-client-certificate branch December 13, 2024 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants