Skip to content

Conversation

@akalcosmin
Copy link

@akalcosmin akalcosmin commented Oct 30, 2025

📝 Description

Summary
Default volume encryption to enabled on create when omitted.
Show encryption="enabled" in the plan for new resources when omitted.
Keep existing volumes unchanged when encryption is omitted in config.
Changing encryption forces replacement.

Behavior Details
Create: if encryption is not set, we set it to enabled (no region capability checks).
Update: encryption is Optional+Computed with UseStateForUnknown, so omission preserves the current state (no diff).
Change: explicit encryption changes continue to RequireReplace.

Implementation Details
Added DefaultEnabledOnCreate plan modifier to set planned value to enabled on create when omitted.
Simplified Create logic to always default encryption to enabled when omitted.
Schema retains UseStateForUnknown and RequiresReplace for encryption.
Unit tests updated to assert presence of DefaultEnabledOnCreate.
Acceptance tests updated to validate default/explicit/replace behavior.

✔️ How to Test

Running against Linode API:

export TF_CLI_CONFIG_FILE=/tmp/tf-linode-encryption-verify/cli.tfrc

VOL1=$(terraform -chdir=/tmp/tf-linode-encryption-verify show -json | jq -r '.values.root_module.resources[] | select(.address=="linode_volume.test") | .values.id')
VOL2=$(terraform -chdir=/tmp/tf-linode-encryption-verify show -json | jq -r '.values.root_module.resources[] | select(.address=="linode_volume.test_disabled") | .values.id')
VOL3=$(terraform -chdir=/tmp/tf-linode-encryption-verify show -json | jq -r '.values.root_module.resources[] | select(.address=="linode_volume.test_enabled") | .values.id')
for V in "$VOL1" "$VOL2" "$VOL3"; do
  curl -s -H "Authorization: Bearer $LINODE_TOKEN" "https://api.linode.com/v4/volumes/$V" \
    | jq -r '[.id, .label, .region, .encryption] | @tsv'
done
11941762	tf-encrypt-test-1	us-east	enabled
11942696	tf-encrypt-test-2	us-east	disabled
11957402	tf-encrypt-test-3	us-east	enabled

Acceptance tests added

# Default enabled
go test  -tags=integration ./linode/volume -run TestAccDataSourceVolume_defaultEncryptionEnabled

# Explicit enabled
go test  -tags=integration ./linode/volume -run TestAccDataSourceVolume_withBlockStorageEncryption

# Explicit disabled
go test  -tags=integration ./linode/volume -run TestAccDataSourceVolume_withBlockStorageEncryptionDisabled

ok  	github.com/linode/terraform-provider-linode/v3/linode/volume	2.018s
ok  	github.com/linode/terraform-provider-linode/v3/linode/volume	1.409s
ok  	github.com/linode/terraform-provider-linode/v3/linode/volume	1.460s

@akalcosmin akalcosmin marked this pull request as ready for review November 19, 2025 16:50
@akalcosmin akalcosmin requested a review from a team as a code owner November 19, 2025 16:50
@akalcosmin akalcosmin requested review from vshanthe and zliang-akamai and removed request for a team November 19, 2025 16:50
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR changes the default encryption setting for Linode volumes from disabled to enabled, improving security by default for users who don't explicitly specify the encryption parameter.

Key Changes:

  • Updated the default value for volume encryption from "disabled" to "enabled" in the resource schema
  • Added test coverage for the new default behavior and explicit encryption disabled scenario
  • Added unit tests to verify the encryption attribute schema configuration

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
linode/volume/framework_schema_resource.go Changed the default encryption value from "disabled" to "enabled"
linode/volume/datasource_test.go Added integration tests for default encryption enabled and explicit encryption disabled scenarios
linode/volume/tmpl/template.go Added helper function for encryption disabled test template
linode/volume/tmpl/data_with_block_storage_encryption_disabled.gotf New template file for testing explicitly disabled encryption
linode/volume/framework_schema_resource_unit_test.go New unit test to guard encryption attribute schema properties

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@akalcosmin akalcosmin force-pushed the volume_encryption_enabled branch from b7ccef8 to 1b0a4b5 Compare November 20, 2025 23:01
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@zliang-akamai zliang-akamai added the breaking-change for breaking changes in the changelog. label Nov 26, 2025
@lgarber-akamai lgarber-akamai self-requested a review November 26, 2025 18:38
@lgarber-akamai
Copy link
Contributor

@zliang-akamai Thanks for adding the breaking change tag! It's also worth noting that this change corresponds to a change in the API.

I do wonder what the behavior would be for a volume created without a user-specified encryption field (defaulting to false), and then the user upgrades provider versions and plans again. I'm thinking it might propose a recreation that could be overlooked.

Would we want to suppress the diff unless the user has explicitly specified the field?

…Storage Encryption” are created with encryption="enabled". New volumes in other regions follow the API default. Existing volumes are unaffected unless the user explicitly changes encryption
@akalcosmin akalcosmin force-pushed the volume_encryption_enabled branch from 6a430ed to d57a022 Compare December 2, 2025 19:57
@tyakin-akamai
Copy link

tyakin-akamai commented Dec 2, 2025

@zliang-akamai Thanks for adding the breaking change tag! It's also worth noting that this change corresponds to a change in the API.

I do wonder what the behavior would be for a volume created without a user-specified encryption field (defaulting to false), and then the user upgrades provider versions and plans again. I'm thinking it might propose a recreation that could be overlooked.

Would we want to suppress the diff unless the user has explicitly specified the field?

I made the following commit (d57a022) to avoid unexpected destroy-and-recreate on provider upgrade caused by changing the default for a RequiresReplace field (volume.encryption). Users who omitted encryption would otherwise see a replacement proposed after upgrade.

I removed the schema default for encryption; added UseStateForUnknown so omitted config preserves existing state; kept RequiresReplace and validators. When encryption is omitted: New volumes in regions supporting “Block Storage Encryption” are created with encryption="enabled". New volumes in other regions follow the API default. Existing volumes are unaffected unless the user explicitly changes encryption.
I updated tests and expectations to reflect observed API behavior (default reads back as "disabled").

User impact
• Existing volumes (encryption omitted): no replacement on upgrade; state is preserved.
• New volumes:
◦ In encryption-capable regions: default to encryption="enabled" when omitted.
◦ In other regions: follow the API’s default (observed as "disabled").
• Explicitly changing encryption still replaces the volume (as intended). Users can opt out of encryption on new volumes by setting encryption = "disabled".

@akalcosmin akalcosmin requested a review from Copilot December 3, 2025 20:51
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- Default encryption to "enabled" when omitted on create; remove region capability checks

- Add DefaultEnabledOnCreate plan modifier so plans show encryption="enabled" for new resources

- Keep Optional+Computed + UseStateForUnknown so omission on existing resources preserves state (no diff)

- Keep RequiresReplace so explicit encryption changes force replacement

- Unit: assert presence of DefaultEnabledOnCreate in volume schema
@zliang-akamai zliang-akamai removed the breaking-change for breaking changes in the changelog. label Jan 6, 2026
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