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

[datadog_synthetics_test] Fix multistep client certificate #2683

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

[datadog_synthetics_test] Fix multistep client certificate #2683

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1f5abbc
Fix multistep client certificate
AntoineDona Nov 19, 2024
799a190
clear old comments
AntoineDona Nov 19, 2024
39eb57b
remove debug log
AntoineDona Nov 20, 2024
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
78 changes: 77 additions & 1 deletion datadog/resource_datadog_synthetics_test_.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ import (
"strconv"
"strings"

"github.com/hashicorp/go-cty/cty"

"github.com/terraform-providers/terraform-provider-datadog/datadog/internal/utils"
"github.com/terraform-providers/terraform-provider-datadog/datadog/internal/validators"

Expand Down Expand Up @@ -2057,7 +2059,7 @@ func buildDatadogSyntheticsAPITest(d *schema.ResourceData) *datadogV1.Synthetics
if attr, ok := d.GetOk("api_step"); ok && syntheticsTest.GetSubtype() == "multi" {
steps := []datadogV1.SyntheticsAPIStep{}

for _, s := range attr.([]interface{}) {
for i, s := range attr.([]interface{}) {
step := datadogV1.SyntheticsAPIStep{}
stepMap := s.(map[string]interface{})

Expand Down Expand Up @@ -2123,6 +2125,12 @@ func buildDatadogSyntheticsAPITest(d *schema.ResourceData) *datadogV1.Synthetics
}
}
}
// Override the request client certificate with the one from the config
rawConfig := d.GetRawConfig()
AntoineDona marked this conversation as resolved.
Show resolved Hide resolved
configCert, configKey := getConfigCertificate(rawConfig, i)
if configCert != nil && configKey != nil {
overrideStateCertificate(stepMap["request_client_certificate"].([]interface{}), *configCert, *configKey)
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep this function pure, just returning the right structure, and overriding here.

Copy link
Contributor Author

@AntoineDona AntoineDona Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The overriding is quite complexe due to the nested structure of stepMap["request_client_certificate"] -> arrays of 1 object containing arrays of 1 object etc. completeSyntheticsTestRequest expects the weird structure as a param, and then calls buildDatadogRequestCertificates to create the right structure for the certificate, so we can't create it beforehand.
If we want to override without any mutation, it will be easier to create a deepCopy of stepMap["request_client_certificate"], then override it and return the finale clientCertificate.

}

request = *completeSyntheticsTestRequest(request, stepMap["request_headers"].(map[string]interface{}), stepMap["request_query"].(map[string]interface{}), stepMap["request_basicauth"].([]interface{}), stepMap["request_client_certificate"].([]interface{}), stepMap["request_proxy"].([]interface{}), stepMap["request_metadata"].(map[string]interface{}))

Expand Down Expand Up @@ -3783,3 +3791,71 @@ func validateSyntheticsAssertionOperator(val interface{}, key string) (warns []s
}
return
}

func getConfigCertificate(rawConfig cty.Value, stepIndex int) (*string, *string) {

AntoineDona marked this conversation as resolved.
Show resolved Hide resolved
basePath := cty.GetAttrPath("api_step").
Index(cty.NumberIntVal(int64(stepIndex))).
GetAttr("request_client_certificate").
Index(cty.NumberIntVal(0))

// Construct paths to the cert and key content
certPath := basePath.
GetAttr("cert").
Index(cty.NumberIntVal(0)).
GetAttr("content")

keyPath := basePath.
GetAttr("key").
Index(cty.NumberIntVal(0)).
GetAttr("content")

// Apply paths to retrieve the cert and key
certValue, err := certPath.Apply(rawConfig)
if err != nil || !certValue.IsKnown() || certValue.IsNull() {
return nil, nil
}

keyValue, err := keyPath.Apply(rawConfig)
if err != nil || !keyValue.IsKnown() || keyValue.IsNull() {
return nil, nil
}

// Convert certValue and keyValue to strings
certString := certValue.AsString()
keyString := keyValue.AsString()

return &certString, &keyString
}
AntoineDona marked this conversation as resolved.
Show resolved Hide resolved

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?


AntoineDona marked this conversation as resolved.
Show resolved Hide resolved
if len(requestClientCertificates) == 0 {
return fmt.Errorf("requestClientCertificates is empty")
}
requestClientCertificate, ok := requestClientCertificates[0].(map[string]interface{})
if !ok {
Drarig29 marked this conversation as resolved.
Show resolved Hide resolved
return fmt.Errorf("requestClientCertificates[0] is not a map")
}
certList, ok := requestClientCertificate["cert"].([]interface{})
if !ok || len(certList) == 0 {
return fmt.Errorf("cert is not a valid list or is empty")
}
cert, ok := certList[0].(map[string]interface{})
if !ok {
return fmt.Errorf("cert[0] is not a map")
}
cert["content"] = configCert

keyList, ok := requestClientCertificate["key"].([]interface{})
if !ok || len(keyList) == 0 {
return fmt.Errorf("key is not a valid list or is empty")
}
key, ok := keyList[0].(map[string]interface{})
if !ok {
return fmt.Errorf("key[0] is not a map")
}
key["content"] = configKey

return nil
}
Comment on lines +3831 to +3861
Copy link
Contributor

Choose a reason for hiding this comment

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

Mutation is to be avoided as much as possible. Instead, let's have this function return the requestClientCertificates interface, and override it in the caller.

Suggested change
func overrideStateCertificate(requestClientCertificates []interface{}, configCert, configKey string) error {
if len(requestClientCertificates) == 0 {
return fmt.Errorf("requestClientCertificates is empty")
}
requestClientCertificate, ok := requestClientCertificates[0].(map[string]interface{})
if !ok {
return fmt.Errorf("requestClientCertificates[0] is not a map")
}
certList, ok := requestClientCertificate["cert"].([]interface{})
if !ok || len(certList) == 0 {
return fmt.Errorf("cert is not a valid list or is empty")
}
cert, ok := certList[0].(map[string]interface{})
if !ok {
return fmt.Errorf("cert[0] is not a map")
}
cert["content"] = configCert
keyList, ok := requestClientCertificate["key"].([]interface{})
if !ok || len(keyList) == 0 {
return fmt.Errorf("key is not a valid list or is empty")
}
key, ok := keyList[0].(map[string]interface{})
if !ok {
return fmt.Errorf("key[0] is not a map")
}
key["content"] = configKey
return nil
}
func buildDatadogRequestClientCertificate(configCert, configKey string) {
requestClientCertificates := datadogV1. ... TODO
certList, ok := requestClientCertificate["cert"].([]interface{})
cert, ok := certList[0].(map[string]interface{})
if !ok {
return fmt.Errorf("cert[0] is not a map")
}
cert["content"] = configCert
keyList, ok := requestClientCertificate["key"].([]interface{})
if !ok || len(keyList) == 0 {
return fmt.Errorf("key is not a valid list or is empty")
}
key, ok := keyList[0].(map[string]interface{})
if !ok {
return fmt.Errorf("key[0] is not a map")
}
key["content"] = configKey
return requestClientCertificates
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed that we should avoid the mutation. However, completeSyntheticsTestRequest is not expecting a nicely formatted requestClientCertificates, but rather a raw stepMap["request_client_certificate"] with a more complexe structure. So the buildDatadogRequestClientCertificate does not really make sense, even more that we already have the buildDatadogRequestCertificates function which kinda does the same thing, inside of completeSyntheticsTestRequest.

Loading