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

Use state for unknown for computed attributes which won't change during plan application #677

Merged
merged 21 commits into from
Aug 5, 2023
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
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
4 changes: 2 additions & 2 deletions build/Makefile.test
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ _report_path:
.PHONY: unit
unit: _report_path
@ echo "-> Running unit tests for $(BINARY)..."
@ TF_ACC_TERRAFORM_VERSION=1.3.9 go test $(TEST) $(TESTARGS) $(TESTUNITARGS)
@ go test $(TEST) $(TESTARGS) $(TESTUNITARGS)

## Alias to "unit".
tests: unit
Expand All @@ -42,7 +42,7 @@ endif

.PHONY: testacc-ci
testacc-ci:
@ TF_ACC_TERRAFORM_VERSION=1.3.9 EC_API_KEY=$(shell cat .ci/.apikey) $(MAKE) testacc
@ EC_API_KEY=$(shell cat .ci/.apikey) $(MAKE) testacc

.PHONY: sweep-ci
sweep-ci:
Expand Down
10 changes: 5 additions & 5 deletions docs/resources/deployment.md
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,7 @@ resource "ec_deployment" "ccs" {
- `request_id` (String) Request ID to set when you create the deployment. Use it only when previous attempts return an error and `request_id` is returned as part of the error.
- `reset_elasticsearch_password` (Boolean) Explicitly resets the elasticsearch_password when true
- `tags` (Map of String) Optional map of deployment tags
- `traffic_filter` (Set of String) List of traffic filters rule identifiers that will be applied to the deployment. Removing this attribute entirely *will not* remove managed traffic filters, instead first set it to an empty list (e.g `traffic_filter = []`) to remove the managed traffic filters.
- `traffic_filter` (Set of String) List of traffic filters rule identifiers that will be applied to the deployment.

### Read-Only

Expand Down Expand Up @@ -765,6 +765,7 @@ Optional:

- `config` (Attributes) Optionally define the Integrations Server configuration options for the IntegrationsServer Server (see [below for nested schema](#nestedatt--integrations_server--config))
- `elasticsearch_cluster_ref_id` (String)
- `endpoints` (Object) URLs for the accessing the Fleet and APM API's within this Integrations Server resource. (see [below for nested schema](#nestedatt--integrations_server--endpoints))
- `instance_configuration_id` (String)
- `ref_id` (String)
- `size` (String)
Expand All @@ -773,7 +774,6 @@ Optional:

Read-Only:

- `endpoints` (Attributes) URLs for the accessing the Fleet and APM API's within this Integrations Server resource. (see [below for nested schema](#nestedatt--integrations_server--endpoints))
- `http_endpoint` (String)
- `https_endpoint` (String)
- `region` (String)
Expand All @@ -795,10 +795,10 @@ Optional:
<a id="nestedatt--integrations_server--endpoints"></a>
### Nested Schema for `integrations_server.endpoints`

Read-Only:
Optional:

- `apm` (String) URL to access the APM server instance for this Integrations Server resource
- `fleet` (String) URL to access the Fleet server instance for this Integrations Server resource
- `apm` (String)
- `fleet` (String)



Expand Down
13 changes: 2 additions & 11 deletions ec/acc/deployment_basic_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,10 @@ func TestAccDeployment_basic_tf(t *testing.T) {
randomAlias := "alias" + acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum)
trafficFilterCfg := "testdata/deployment_basic_with_traffic_filter_2.tf"
trafficFilterUpdateCfg := "testdata/deployment_basic_with_traffic_filter_3.tf"
emptyTrafficFilterCfg := "testdata/deployment_basic_with_empty_traffic_filter.tf"
resetPasswordCfg := "testdata/deployment_basic_reset_password.tf"
cfg := fixtureAccDeploymentResourceBasicWithAppsAlias(t, startCfg, randomAlias, randomName, getRegion(), defaultTemplate)
cfgWithTrafficFilter := fixtureAccDeploymentResourceBasicWithTF(t, trafficFilterCfg, randomName, getRegion(), defaultTemplate)
cfgWithTrafficFilterUpdate := fixtureAccDeploymentResourceBasicWithTF(t, trafficFilterUpdateCfg, randomName, getRegion(), defaultTemplate)
cfgWithEmptyTrafficFilter := fixtureAccDeploymentResourceBasicWithAppsAlias(t, emptyTrafficFilterCfg, randomAlias, randomName, getRegion(), defaultTemplate)
cfgResetPassword := fixtureAccDeploymentResourceBasicWithAppsAlias(t, resetPasswordCfg, randomAlias, randomName, getRegion(), defaultTemplate)
deploymentVersion, err := latestStackVersion()
if err != nil {
Expand All @@ -59,7 +57,7 @@ func TestAccDeployment_basic_tf(t *testing.T) {
resource.TestCheckResourceAttr(resName, "alias", randomAlias),
resource.TestCheckNoResourceAttr(resName, "apm.config"),
resource.TestCheckNoResourceAttr(resName, "enterprise_search.config"),
resource.TestCheckNoResourceAttr(resName, "traffic_filter"),
resource.TestCheckResourceAttr(resName, "traffic_filter.#", "0"),
// Ensure at least 1 account is trusted (self).
resource.TestCheckResourceAttr(resName, "elasticsearch.trust_account.#", "1"),
),
Expand All @@ -78,16 +76,9 @@ func TestAccDeployment_basic_tf(t *testing.T) {
resource.TestCheckResourceAttr(resName, "traffic_filter.#", "1"),
),
},
// Unset the traffic filter (this should not remove the traffic filter)
// Unset the traffic filter to remove the traffic filter
{
Config: cfg,
Check: checkBasicDeploymentResource(resName, randomName, deploymentVersion,
resource.TestCheckResourceAttr(resName, "traffic_filter.#", "1"),
),
},
// Explicitly set the traffic filter to an empty list to remove the traffic filter
{
Config: cfgWithEmptyTrafficFilter,
Check: checkBasicDeploymentResource(resName, randomName, deploymentVersion,
resource.TestCheckResourceAttr(resName, "traffic_filter.#", "0"),
func(s *terraform.State) error {
Expand Down
9 changes: 8 additions & 1 deletion ec/acc/deployment_ccs_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,12 @@ func TestAccDeployment_ccs(t *testing.T) {
{
// Create a CCS deployment with the default settings.
Config: cfg,
// The legacy CCS DT does not support autoscaling, which leads to autoscaling being 'unknown'.
// Ideally we would set autoscaling to null if the deployment template does not support autoscaling,
// but that would require's refactoring our schema and this template is no longer part of the public offering.
//
// We can revisit this if there's demand for clean plans when the template does not support autoscaling.
ExpectNonEmptyPlan: true,
Check: resource.ComposeAggregateTestCheckFunc(

// CCS Checks
Expand Down Expand Up @@ -94,7 +100,8 @@ func TestAccDeployment_ccs(t *testing.T) {
},
{
// Change the Elasticsearch topology size and node count.
Config: secondConfigCfg,
Config: secondConfigCfg,
ExpectNonEmptyPlan: true,
Check: resource.ComposeAggregateTestCheckFunc(
// Changes.
resource.TestCheckResourceAttrSet(ccsResName, "elasticsearch.hot.instance_configuration_id"),
Expand Down
3 changes: 3 additions & 0 deletions ec/acc/deployment_pre_node_role_migration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,9 @@ func TestAccDeployment_pre_node_roles(t *testing.T) {
},
{
Config: cfgF(upgradeVersionCfg),
// Expect a non-empty plan here. We explicitly avoid migrating node_roles when the version changes
// however will the migrate the deployment to node_roles on the next TF application.
ExpectNonEmptyPlan: true,
Check: resource.ComposeAggregateTestCheckFunc(
resource.TestCheckResourceAttrSet(resName, "elasticsearch.hot.instance_configuration_id"),
resource.TestCheckResourceAttr(resName, "elasticsearch.hot.size", "1g"),
Expand Down
33 changes: 0 additions & 33 deletions ec/acc/testdata/deployment_basic_with_empty_traffic_filter.tf

This file was deleted.

12 changes: 12 additions & 0 deletions ec/ecresource/deploymentresource/apm/v2/schema.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,15 +83,27 @@ func ApmSchema() schema.Attribute {
},
"resource_id": schema.StringAttribute{
Computed: true,
PlanModifiers: []planmodifier.String{
stringplanmodifier.UseStateForUnknown(),
},
},
"region": schema.StringAttribute{
Computed: true,
PlanModifiers: []planmodifier.String{
stringplanmodifier.UseStateForUnknown(),
},
},
"http_endpoint": schema.StringAttribute{
Computed: true,
PlanModifiers: []planmodifier.String{
stringplanmodifier.UseStateForUnknown(),
},
},
"https_endpoint": schema.StringAttribute{
Computed: true,
PlanModifiers: []planmodifier.String{
stringplanmodifier.UseStateForUnknown(),
},
},
"instance_configuration_id": schema.StringAttribute{
Optional: true,
Expand Down
8 changes: 7 additions & 1 deletion ec/ecresource/deploymentresource/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,13 @@ func (r *Resource) Create(ctx context.Context, req resource.CreateRequest, resp

resp.Diagnostics.Append(v2.HandleRemoteClusters(ctx, r.client, *res.ID, plan.Elasticsearch)...)

deployment, diags := r.read(ctx, *res.ID, nil, &plan, res.Resources)
filters := []string{}
if request.Settings != nil && request.Settings.TrafficFilterSettings != nil && request.Settings.TrafficFilterSettings.Rulesets != nil {
filters = request.Settings.TrafficFilterSettings.Rulesets
}

deployment, diags := r.read(ctx, *res.ID, nil, &plan, res.Resources, filters)
updatePrivateStateTrafficFilters(ctx, resp.Private, filters)

resp.Diagnostics.Append(diags...)

Expand Down
34 changes: 22 additions & 12 deletions ec/ecresource/deploymentresource/deployment/v2/deployment_read.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"github.com/blang/semver"
"github.com/elastic/cloud-sdk-go/pkg/models"
"github.com/elastic/cloud-sdk-go/pkg/util/ec"
"golang.org/x/exp/slices"

apmv2 "github.com/elastic/terraform-provider-ec/ec/ecresource/deploymentresource/apm/v2"
elasticsearchv2 "github.com/elastic/terraform-provider-ec/ec/ecresource/deploymentresource/elasticsearch/v2"
Expand Down Expand Up @@ -229,23 +230,32 @@ func (dep *Deployment) ProcessSelfInObservability() {
}
}

func (dep *Deployment) HandleEmptyTrafficFilters(ctx context.Context, base DeploymentTF) diag.Diagnostics {
var diags diag.Diagnostics
// Ensure consistency between null, and empty configured traffic filter values.
// The Cloud API represents an empty set of traffic filters as a null/missing value. Terraform does distinguish between those two cases.
// If the Cloud response does not include traffic filters, then set the read value as the planned value, but only if the planned value is empty.
if dep.TrafficFilter == nil {
var baseFilters []string
diags := base.TrafficFilter.ElementsAs(ctx, &baseFilters, true)
if diags.HasError() {
return diags
func (dep *Deployment) IncludePrivateStateTrafficFilters(ctx context.Context, base DeploymentTF, privateFilters []string) diag.Diagnostics {
var baseFilters []string
diags := base.TrafficFilter.ElementsAs(ctx, &baseFilters, true)
if diags.HasError() {
return diags
}

for _, filter := range privateFilters {
if !slices.Contains[string](baseFilters, filter) {
baseFilters = append(baseFilters, filter)
}
}

if len(baseFilters) == 0 {
dep.TrafficFilter = baseFilters
if len(baseFilters) == 0 {
dep.TrafficFilter = baseFilters
}

intersectionFilters := []string{}
for _, filter := range dep.TrafficFilter {
if slices.Contains(baseFilters, filter) {
intersectionFilters = append(intersectionFilters, filter)
}
}

dep.TrafficFilter = intersectionFilters

return diags
}

Expand Down
Loading