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

HVT-4279 add support for cloudwatch observability on vault_cluster resource #580

Merged
merged 7 commits into from
Aug 30, 2023

Conversation

bosouza
Copy link
Contributor

@bosouza bosouza commented Aug 21, 2023

🛠️ Description

Add CloudWatch option for observability providers on HCP Vault clusters. See HVT-4279. I manually tested updating the audit-logs and metrics settings using the TF provider on cluster creation and cluster updates. Didn't add an acceptance test because the increase in the test duration didn't seem justified by such a small change.

🏗️ Acceptance tests

  • Are there any feature flags that are required to use this functionality?
  • Have you added an acceptance test for the functionality being added?
  • Have you run the acceptance tests on this branch?

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccVaultClusterAWS'
==> Checking that code complies with gofmt requirements...
golangci-lint run --config ./golangci-config.yml 
TF_ACC=1 go test ./internal/... -v -run=TestAccVaultClusterAWS -timeout 360m -parallel=10
testing: warning: no tests to run
PASS
ok      github.com/hashicorp/terraform-provider-hcp/internal/clients    (cached) [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/hashicorp/terraform-provider-hcp/internal/consul     (cached) [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/hashicorp/terraform-provider-hcp/internal/input      (cached) [no tests to run]
=== RUN   TestAccVaultClusterAWS
=== PAUSE TestAccVaultClusterAWS
=== CONT  TestAccVaultClusterAWS
--- PASS: TestAccVaultClusterAWS (3339.18s)
PASS
ok      github.com/hashicorp/terraform-provider-hcp/internal/provider   3339.188s

...

@hashicorp-cla
Copy link

hashicorp-cla commented Aug 21, 2023

CLA assistant check
All committers have signed the CLA.

Description: "CloudWatch access key ID for streaming audit logs",
Type: schema.TypeString,
Computed: true,
},

Choose a reason for hiding this comment

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

Looks like we need cloudwatch_secret_access_key here as well?

Copy link
Contributor Author

@bosouza bosouza Aug 28, 2023

Choose a reason for hiding this comment

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

done, funny how the code for preventing the value from the state file from being updated with "redacted" prevented this issue from surfacing in my tests 😆

@mercedesbh
Copy link

Looks great! Just one small question

@bosouza bosouza requested a review from mercedesbh August 28, 2023 20:31
configMap["cloudwatch_access_key_id"] = cloudwatch.AccessKeyID
configMap["cloudwatch_region"] = cloudwatch.Region
// ensure we only set properties that are defined in metrics/audit-logs streaming
if propertyName == "metrics_config" {
Copy link
Contributor

Choose a reason for hiding this comment

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

How do you find out whether a field within a provider should only be set for metrics/only set for audit logs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, it's kinda messy. Have a look at applyObservabilityConfigMutations on cloud-vault-service to see what fields are automatically set by us when creating/updating the observability configs, not how they're different for metrics/logs. There's also elsasticsearch.go with the Vector definition for each of the providers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let me know if it's still unclear and I can walk you through the relevant code.

Also, you could open the UI and see what fields are shown for logs/ metrics when configuring elasticsearch

@bosouza bosouza requested a review from a team as a code owner August 30, 2023 18:06
@bosouza bosouza requested a review from AnnaDu21 August 30, 2023 18:06
@bosouza bosouza merged commit ec421b1 into main Aug 30, 2023
@bosouza bosouza deleted the bosouza-hcpv-cloudwatch branch August 30, 2023 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants