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

feat: Add Resource for External Volumes #3106

Merged
merged 19 commits into from
Oct 25, 2024
Merged

feat: Add Resource for External Volumes #3106

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
1704a50
feat: Add External Volume Resource
jdoldis Sep 24, 2024
afbd4a0
Clean up and call out existing issue
jdoldis Sep 29, 2024
fd4062e
Review feedback part 1
jdoldis Oct 14, 2024
f143a26
Generate show output assertions and update acceptance tests to use these
jdoldis Oct 14, 2024
afe0235
Add V1 candidate note
jdoldis Oct 14, 2024
2304ae4
Add custom external volumes assert for storage locations at index, and
jdoldis Oct 15, 2024
1f190e6
Update helpers file with changes suggested in review comments
jdoldis Oct 20, 2024
68c104d
Reduce number of test files and move list of valid storage providers
jdoldis Oct 20, 2024
337f545
Remove schema version, add defer for temp location cleanup and clean …
jdoldis Oct 20, 2024
4f89a9d
Rename longestCommonPrefix and use DeepEqual for StorageLocationsEqual
jdoldis Oct 20, 2024
2c09c7d
Order schema gen list alphabetically
jdoldis Oct 20, 2024
15b8738
Use pre defined temp storage location name
jdoldis Oct 21, 2024
90f2c88
Remove defer as it's not currently working
jdoldis Oct 21, 2024
6029e87
Finish revert of defer change
jdoldis Oct 21, 2024
ff43f8a
Bring back SNOW-999142 TODO, rename function and fix typo
jdoldis Oct 22, 2024
8ac4af1
Set FullyQualifiedNameAttributeName and move functions to external_vo…
jdoldis Oct 22, 2024
184b06c
Merge remote-tracking branch 'upstream/main'
sfc-gh-jmichalak Oct 25, 2024
b2836c7
Resolve conflicts
sfc-gh-jmichalak Oct 25, 2024
c4af02d
Merge remote-tracking branch 'upstream/main'
sfc-gh-jmichalak Oct 25, 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
2 changes: 1 addition & 1 deletion docs/resources/external_volume.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ Resource used to manage external volume objects. For more information, check [ex
Required:

- `storage_base_url` (String) Specifies the base URL for your cloud storage location.
- `storage_location_name` (String) Name of the storage location. Must be unique for the external volume. Due to technical limitations (read more [here](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/docs/technical-documentation/identifiers_rework_design_decisions.md#known-limitations-and-identifier-recommendations)), avoid using the following characters: `|`, `.`, `(`, `)`, `"`
- `storage_location_name` (String) Name of the storage location. Must be unique for the external volume. Do not use the name `terraform_provider_sentinel_storage_location` - this is reserved for the provider for performing update operations. Due to technical limitations (read more [here](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/docs/technical-documentation/identifiers_rework_design_decisions.md#known-limitations-and-identifier-recommendations)), avoid using the following characters: `|`, `.`, `(`, `)`, `"`
- `storage_provider` (String) Specifies the cloud storage provider that stores your data files. Valid values are (case-insensitive): `GCS` | `AZURE` | `S3` | `S3GOV`.

Optional:
Expand Down
21 changes: 21 additions & 0 deletions pkg/acceptance/bettertestspoc/assert/resourceassert/external_volume_resource_ext.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package resourceassert

import (
"fmt"
"strconv"

"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/bettertestspoc/assert"
Expand All @@ -10,3 +11,23 @@ func (e *ExternalVolumeResourceAssert) HasStorageLocationLength(len int) *Extern
e.AddAssertion(assert.ValueSet("storage_location.#", strconv.FormatInt(int64(len), 10)))
return e
}

func (e *ExternalVolumeResourceAssert) HasStorageLocationAtIndex(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: we can pass here a slice of a custom struct with these fields. But it's okay like this.

index int,
expectedName string,
expectedStorageProvider string,
expectedStorageBaseUrl string,
expectedStorageAwsRoleArn string,
expectedEncryptionType string,
expectedEncryptionKmsKeyId string,
expectedAzureTenantId string,
) *ExternalVolumeResourceAssert {
e.AddAssertion(assert.ValueSet(fmt.Sprintf("storage_location.%s.storage_location_name", strconv.Itoa(index)), expectedName))
e.AddAssertion(assert.ValueSet(fmt.Sprintf("storage_location.%s.storage_provider", strconv.Itoa(index)), expectedStorageProvider))
e.AddAssertion(assert.ValueSet(fmt.Sprintf("storage_location.%s.storage_base_url", strconv.Itoa(index)), expectedStorageBaseUrl))
e.AddAssertion(assert.ValueSet(fmt.Sprintf("storage_location.%s.storage_aws_role_arn", strconv.Itoa(index)), expectedStorageAwsRoleArn))
e.AddAssertion(assert.ValueSet(fmt.Sprintf("storage_location.%s.encryption_type", strconv.Itoa(index)), expectedEncryptionType))
e.AddAssertion(assert.ValueSet(fmt.Sprintf("storage_location.%s.encryption_kms_key_id", strconv.Itoa(index)), expectedEncryptionKmsKeyId))
e.AddAssertion(assert.ValueSet(fmt.Sprintf("storage_location.%s.azure_tenant_id", strconv.Itoa(index)), expectedAzureTenantId))
return e
}
186 changes: 19 additions & 167 deletions pkg/helpers/helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"path"
"reflect"
"regexp"
"slices"
"strconv"
"strings"

Expand Down Expand Up @@ -164,61 +165,15 @@ func ConcatSlices[T any](slices ...[]T) []T {
return tmp
}

// Structs for parsing external volume desribe output
type S3StorageLocation struct {
Name string `json:"NAME"`
StorageProvider string `json:"STORAGE_PROVIDER"`
StorageBaseUrl string `json:"STORAGE_BASE_URL"`
StorageAllowedLocations []string `json:"-"`
StorageAwsRoleArn string `json:"STORAGE_AWS_ROLE_ARN"`
StroageAwsIamUserArn string `json:"-"`
StorageAwsExternalId string `json:"STORAGE_AWS_EXTERNAL_ID"`
EncryptionType string `json:"ENCRYPTION_TYPE,omitempty"`
EncryptionKmsId string `json:"ENCRYPTION_KMS_KEY_ID,omitempty"`
}

type GCSStorageLocation struct {
Name string `json:"NAME"`
StorageProvider string `json:"STORAGE_PROVIDER"`
StorageBaseUrl string `json:"STORAGE_BASE_URL"`
StorageAllowedLocations []string `json:"-"`
StorageGcpServiceAccount string `json:"-"`
EncryptionType string `json:"ENCRYPTION_TYPE,omitempty"`
EncryptionKmsId string `json:"ENCRYPTION_KMS_KEY_ID,omitempty"`
}

type AzureStorageLocation struct {
Name string `json:"NAME"`
StorageProvider string `json:"STORAGE_PROVIDER"`
StorageBaseUrl string `json:"STORAGE_BASE_URL"`
StorageAllowedLocations []string `json:"-"`
AzureTenantId string `json:"AZURE_TENANT_ID"`
AzureMultiTenantAppName string `json:"-"`
AzureConsentUrl string `json:"-"`
EncryptionType string `json:"-"`
EncryptionKmsId string `json:"-"`
}

type StorageLocation struct {
Name string `json:"NAME"`
StorageProvider string `json:"STORAGE_PROVIDER"`
StorageBaseUrl string `json:"STORAGE_BASE_URL"`
StorageAwsRoleArn string `json:"STORAGE_AWS_ROLE_ARN,omitempty"`
StorageAwsExternalId string `json:"STORAGE_AWS_EXTERNAL_ID,omitempty"`
EncryptionType string `json:"ENCRYPTION_TYPE,omitempty"`
EncryptionKmsKeyId string `json:"ENCRYPTION_KMS_KEY_ID,omitempty"`
AzureTenantId string `json:"AZURE_TENANT_ID,omitempty"`
}

func storageLocationsEqual(s1 StorageLocation, s2 StorageLocation) bool {
return s1.Name == s2.Name &&
s1.StorageProvider == s2.StorageProvider &&
s1.StorageBaseUrl == s2.StorageBaseUrl &&
s1.StorageAwsRoleArn == s2.StorageAwsRoleArn &&
s1.StorageAwsExternalId == s2.StorageAwsExternalId &&
s1.EncryptionType == s2.EncryptionType &&
s1.EncryptionKmsKeyId == s2.EncryptionKmsKeyId &&
s1.AzureTenantId == s2.AzureTenantId
StorageAwsRoleArn string `json:"STORAGE_AWS_ROLE_ARN"`
StorageAwsExternalId string `json:"STORAGE_AWS_EXTERNAL_ID"`
EncryptionType string `json:"ENCRYPTION_TYPE"`
EncryptionKmsKeyId string `json:"ENCRYPTION_KMS_KEY_ID"`
AzureTenantId string `json:"AZURE_TENANT_ID"`
}

func validateParsedExternalVolumeDescribed(p ParsedExternalVolumeDescribed) error {
Expand All @@ -233,8 +188,8 @@ func validateParsedExternalVolumeDescribed(p ParsedExternalVolumeDescribed) erro
if len(s.Name) == 0 {
return fmt.Errorf("A storage location's Name in this volume could not be parsed.")
}
if len(s.StorageProvider) == 0 {
return fmt.Errorf("A storage location's StorageProvider in this volume could not be parsed.")
if !slices.Contains(sdk.AsStringList(sdk.AllStorageProviderValues), s.StorageProvider) {
return fmt.Errorf("invalid storage provider parsed: %s", s)
}
if len(s.StorageBaseUrl) == 0 {
return fmt.Errorf("A storage location's StorageBaseUrl in this volume could not be parsed.")
sfc-gh-jmichalak marked this conversation as resolved.
Show resolved Hide resolved
Expand Down Expand Up @@ -267,20 +222,6 @@ type ParsedExternalVolumeDescribed struct {
AllowWrites string
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is boolean value, you can change it and check in the parser (strings.ToLower(value) == "true" to get boolean value, but it depends what ALLOW_WRITES returns because sometimes Snowflake represents boolean values with Y/N YES/NO ON/OFF so the comparison would depend on what's the value of true in this case)

Copy link
Contributor Author

@jdoldis jdoldis Oct 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Agree that would be cleaner, and it does return "true" so we could change it.

Only consideration is the string type is used to validate that it was set right now -

terraform-provider-snowflake/pkg/helpers/helpers.go

Line 183 in 90f2c88

if len(p.AllowWrites) == 0 {
. I wouldn't want to change the type to *bool, as not having it set is not a valid state. Options I see -

  1. Change the type of AllowWrites to bool, remove the current validation and instead run a validation that checks the ALLOW_WRITES property was given in the props list at

    terraform-provider-snowflake/pkg/helpers/helpers.go

    Line 225 in 90f2c88

    func ParseExternalVolumeDescribed(props []sdk.ExternalVolumeProperty) (ParsedExternalVolumeDescribed, error) {
    . Only thing I don't like about this is that the validation isn't happening in the validation function.
  2. Make a copy of the ParsedExternalVolumeDescribed type, changing the type of AllowWrites to bool. Before returning, once validation is done, translate the string to a boolean and leave all other fields the same.

Any preferences from your side?

Copy link
Collaborator

@sfc-gh-jcieslak sfc-gh-jcieslak Oct 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

As I checked, the field seems to be pretty stable to the outputs it gives. It's either true or false (no empty values, since there is a default for this field). Personally, I would go with the first approach, and in the switch for properties in ParseExternalVolumeDescribed, I would assign strings.ToLower(value) == "true" to the boolean value (and remove the validation).
The only case I was thinking about leaving it as if this field could be empty or something, then we would string to represent three states (instead of two with boolean), but I don't think that's the case here. cc: @sfc-gh-asawicki

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That’s an interesting point on just deleting the validation. You’re right, it’s not possible on Snowflake that it’s not provided. That’s the case for everything I’m validating in the validation function. So if we’re happy to assume that the SDK is returning the properties as expected we can actually just remove that whole validation function. What do you think?

Copy link
Collaborator

@sfc-gh-jcieslak sfc-gh-jcieslak Oct 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If something changes in Snowflake and it's a breaking change (e.g. change of default behavior), then it should be included in the BCR changes bundle (e.g. this one) that we try to follow and adjust the code accordingly. Because of that we can assume some of the values and be sure they won't change randomly.
Also, previously, we had a few discussions on validations, and we decided to let some of the validation happen on the Snowflake side so it won't be duplicated on our side. We decided to still leave schema validation, some of the SDK validation, enum validation, and probably some other validations, but in some cases, we try to do best-effort mappings, so map as much as we can and leave the rest blank.
Let's leave it as it is right now, but also leave the thread open (so the person going through the code and open threads will decide what to do).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good 🙂

}

func ParsedExternalVolumesDescribedEqual(p1 ParsedExternalVolumeDescribed, p2 ParsedExternalVolumeDescribed) bool {
attributesEqual := p1.Active == p2.Active && p1.Comment == p2.Comment && p1.AllowWrites == p2.AllowWrites
if attributesEqual && (len(p1.StorageLocations) == len(p2.StorageLocations)) {
for i := range p1.StorageLocations {
if !storageLocationsEqual(p1.StorageLocations[i], p2.StorageLocations[i]) {
return false
}
}

return true
}
return false
}

func ParseExternalVolumeDescribed(props []sdk.ExternalVolumeProperty) (ParsedExternalVolumeDescribed, error) {
parsedExternalVolumeDescribed := ParsedExternalVolumeDescribed{}
var storageLocations []StorageLocation
Expand All @@ -293,118 +234,29 @@ func ParseExternalVolumeDescribed(props []sdk.ExternalVolumeProperty) (ParsedExt
case p.Name == "ALLOW_WRITES":
parsedExternalVolumeDescribed.AllowWrites = p.Value
case strings.Contains(p.Name, "STORAGE_LOCATION_"):
Copy link
Contributor Author

@jdoldis jdoldis Oct 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

One thing to check here - for azure storage locations the encryption_type will be set, although it's not set in the config. It's a special case where it's not a valid parameter, but is set on Snowflake side. This shouldn't cause any issues, as otherwise I think the tests would fail with unstable plans. But worth checking before merging.

switch {
case strings.Contains(p.Value, `"STORAGE_PROVIDER":"S3"`):
s3StorageLocation := S3StorageLocation{}
err := json.Unmarshal([]byte(p.Value), &s3StorageLocation)
if err != nil {
return ParsedExternalVolumeDescribed{}, err
}
storageLocation := StorageLocation{
Name: s3StorageLocation.Name,
StorageProvider: s3StorageLocation.StorageProvider,
StorageBaseUrl: s3StorageLocation.StorageBaseUrl,
StorageAwsRoleArn: s3StorageLocation.StorageAwsRoleArn,
StorageAwsExternalId: s3StorageLocation.StorageAwsExternalId,
EncryptionType: s3StorageLocation.EncryptionType,
EncryptionKmsKeyId: s3StorageLocation.EncryptionKmsId,
}
storageLocations = append(
storageLocations,
storageLocation,
)
case strings.Contains(p.Value, `"STORAGE_PROVIDER":"GCS"`):
gcsStorageLocation := GCSStorageLocation{}
err := json.Unmarshal([]byte(p.Value), &gcsStorageLocation)
if err != nil {
return ParsedExternalVolumeDescribed{}, err
}

storageLocation := StorageLocation{
Name: gcsStorageLocation.Name,
StorageProvider: gcsStorageLocation.StorageProvider,
StorageBaseUrl: gcsStorageLocation.StorageBaseUrl,
EncryptionType: gcsStorageLocation.EncryptionType,
EncryptionKmsKeyId: gcsStorageLocation.EncryptionKmsId,
}
storageLocations = append(
storageLocations,
storageLocation,
)
case strings.Contains(p.Value, `"STORAGE_PROVIDER":"AZURE"`):
azureStorageLocation := AzureStorageLocation{}
err := json.Unmarshal([]byte(p.Value), &azureStorageLocation)
if err != nil {
return ParsedExternalVolumeDescribed{}, err
}

storageLocation := StorageLocation{
Name: azureStorageLocation.Name,
StorageProvider: azureStorageLocation.StorageProvider,
StorageBaseUrl: azureStorageLocation.StorageBaseUrl,
AzureTenantId: azureStorageLocation.AzureTenantId,
}
storageLocations = append(
storageLocations,
storageLocation,
)
default:
return ParsedExternalVolumeDescribed{}, fmt.Errorf("Unrecognized storage provider in storage location property: %s", p.Value)
storageLocation := StorageLocation{}
err := json.Unmarshal([]byte(p.Value), &storageLocation)
if err != nil {
return ParsedExternalVolumeDescribed{}, err
}
storageLocations = append(
storageLocations,
storageLocation,
)
default:
return ParsedExternalVolumeDescribed{}, fmt.Errorf("Unrecognized external volume property: %s", p.Name)
}
}

parsedExternalVolumeDescribed.StorageLocations = storageLocations
validated := validateParsedExternalVolumeDescribed(parsedExternalVolumeDescribed)
if validated != nil {
return ParsedExternalVolumeDescribed{}, validated
err := validateParsedExternalVolumeDescribed(parsedExternalVolumeDescribed)
if err != nil {
return ParsedExternalVolumeDescribed{}, err
}

return parsedExternalVolumeDescribed, nil
}

// Generate input to the ParseExternalVolumeDescribedInput, useful for testing purposes
func GenerateParseExternalVolumeDescribedInput(comment string, allowWrites string, storageLocations []string, active string) []sdk.ExternalVolumeProperty {
storageLocationProperties := make([]sdk.ExternalVolumeProperty, len(storageLocations))
allowWritesProperty := sdk.ExternalVolumeProperty{
Parent: "",
Name: "ALLOW_WRITES",
Type: "Boolean",
Value: allowWrites,
Default: "true",
}

commentProperty := sdk.ExternalVolumeProperty{
Parent: "",
Name: "COMMENT",
Type: "String",
Value: comment,
Default: "",
}

activeProperty := sdk.ExternalVolumeProperty{
Parent: "STORAGE_LOCATIONS",
Name: "ACTIVE",
Type: "String",
Value: active,
Default: "",
}

for i, property := range storageLocations {
storageLocationProperties[i] = sdk.ExternalVolumeProperty{
Parent: "STORAGE_LOCATIONS",
Name: fmt.Sprintf("STORAGE_LOCATION_%s", strconv.Itoa(i+1)),
Type: "String",
Value: property,
Default: "",
}
}

return append(append([]sdk.ExternalVolumeProperty{allowWritesProperty, commentProperty}, storageLocationProperties...), activeProperty)
}

// TODO(SNOW-1569530): address during identifiers rework follow-up
func ParseRootLocation(location string) (sdk.SchemaObjectIdentifier, string, error) {
location = strings.TrimPrefix(location, "@")
Expand Down
Loading