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

set default for kpk in storage delegation #223

Merged
merged 22 commits into from
Dec 18, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
1fc5cac
set default for kpk in storage delegation
Aayush-Abhyarthi Nov 28, 2024
9029960
Merge branch 'main' into use-dft-sd-kpk-name
Aayush-Abhyarthi Dec 4, 2024
838cdca
Merge branch 'main' into use-dft-sd-kpk-name
Aayush-Abhyarthi Dec 9, 2024
362e2d5
add: upgrade test with kms encryption enabled
Aayush-Abhyarthi Dec 10, 2024
dc5b6fc
Merge remote-tracking branch 'origin/use-dft-sd-kpk-name' into use-df…
Aayush-Abhyarthi Dec 10, 2024
4f14d53
fix: provider visibility
Aayush-Abhyarthi Dec 11, 2024
a67cefc
add: validation and enable encrytion in tests
Aayush-Abhyarthi Dec 12, 2024
cb7d23e
do not pass cos_kms_key_crn
Aayush-Abhyarthi Dec 17, 2024
1531c69
fix: pre-commit
Aayush-Abhyarthi Dec 17, 2024
7c2022f
use new project name
Aayush-Abhyarthi Dec 17, 2024
3ba06ee
revert prefix
Aayush-Abhyarthi Dec 17, 2024
66f8459
Merge branch 'main' into use-dft-sd-kpk-name
Aayush-Abhyarthi Dec 17, 2024
f0fd1e3
wait 20s to configure project
Aayush-Abhyarthi Dec 17, 2024
a870099
Merge remote-tracking branch 'origin/use-dft-sd-kpk-name' into use-df…
Aayush-Abhyarthi Dec 17, 2024
c386f71
fix: cra-scan
Aayush-Abhyarthi Dec 17, 2024
ec44fc1
revert configure project time
Aayush-Abhyarthi Dec 17, 2024
4ea7c5e
revert configure project time
Aayush-Abhyarthi Dec 17, 2024
b768045
trigger test
Aayush-Abhyarthi Dec 18, 2024
01fdf53
SKIP UPGRADE TEST
Aayush-Abhyarthi Dec 18, 2024
84e7419
Merge branch 'main' into use-dft-sd-kpk-name
Aayush-Abhyarthi Dec 18, 2024
47a6ba8
resolve conflicts
Aayush-Abhyarthi Dec 18, 2024
a7c6bfb
UNSKIP UPGRADE TEST
Aayush-Abhyarthi Dec 18, 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 README.md
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ statement instead the previous block.
|------|-------------|------|---------|:--------:|
| <a name="input_cos_kms_crn"></a> [cos\_kms\_crn](#input\_cos\_kms\_crn) | Key Protect service instance CRN used to encrypt the COS buckets used by the watsonx projects. Required if `enable_cos_kms_encryption` is true. | `string` | `null` | no |
| <a name="input_cos_kms_key_crn"></a> [cos\_kms\_key\_crn](#input\_cos\_kms\_key\_crn) | Key Protect key CRN used to encrypt the COS buckets used by the watsonx projects. If not set, then the cos\_kms\_new\_key\_name must be specified. | `string` | `null` | no |
| <a name="input_cos_kms_new_key_name"></a> [cos\_kms\_new\_key\_name](#input\_cos\_kms\_new\_key\_name) | Name of the Key Protect key to create for encrypting the COS buckets used by the watsonx projects. | `string` | `""` | no |
| <a name="input_cos_kms_new_key_name"></a> [cos\_kms\_new\_key\_name](#input\_cos\_kms\_new\_key\_name) | Name of the Key Protect key to create for encrypting the COS buckets used by the watsonx projects. | `string` | `"storage-delegation-key"` | no |
| <a name="input_cos_kms_ring_id"></a> [cos\_kms\_ring\_id](#input\_cos\_kms\_ring\_id) | The identifier of the Key Protect ring to create the cos\_kms\_new\_key\_name into. If it is not set, then the new key will be created in the default ring. | `string` | `null` | no |
| <a name="input_cos_plan"></a> [cos\_plan](#input\_cos\_plan) | The plan that's used to provision the Cloud Object Storage instance. | `string` | `"standard"` | no |
| <a name="input_enable_cos_kms_encryption"></a> [enable\_cos\_kms\_encryption](#input\_enable\_cos\_kms\_encryption) | Flag to enable COS KMS encryption. If set to true, a value must be passed for `cos_kms_crn`. | `bool` | `true` | no |
Expand Down
2 changes: 1 addition & 1 deletion common-dev-assets
2 changes: 1 addition & 1 deletion storage_delegation/variables.tf
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ variable "cos_kms_key_crn" {
variable "cos_kms_new_key_name" {
description = "Name of the Key Protect key to create for encrypting the COS buckets used by the watsonx projects."
type = string
default = ""
default = "storage-delegation-key"
}

variable "cos_kms_ring_id" {
Expand Down
84 changes: 84 additions & 0 deletions tests/pr_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -178,3 +178,87 @@ func TestWithExistingKP(t *testing.T) {
}

}

func TestRunUpgradeExistingKP(t *testing.T) {
t.Parallel()

// ------------------------------------------------------------------------------------
// Provision KP first
// ------------------------------------------------------------------------------------

prefix := fmt.Sprintf("kp-ut-%s", strings.ToLower(random.UniqueId()))
realTerraformDir := "./resources/kp-instance"
tempTerraformDir, _ := files.CopyTerraformFolderToTemp(realTerraformDir, fmt.Sprintf(prefix+"-%s", strings.ToLower(random.UniqueId())))
region := "us-south"

// Verify ibmcloud_api_key variable is set
checkVariable := "TF_VAR_ibmcloud_api_key"
val, present := os.LookupEnv(checkVariable)
require.True(t, present, checkVariable+" environment variable not set")
require.NotEqual(t, "", val, checkVariable+" environment variable is empty")

logger.Log(t, "Tempdir: ", tempTerraformDir)
existingTerraformOptions := terraform.WithDefaultRetryableErrors(t, &terraform.Options{
TerraformDir: tempTerraformDir,
Vars: map[string]interface{}{
"prefix": prefix,
"region": region,
},
// Set Upgrade to true to ensure latest version of providers and modules are used by terratest.
// This is the same as setting the -upgrade=true flag with terraform.
Upgrade: true,
})

terraform.WorkspaceSelectOrNew(t, existingTerraformOptions, prefix)
_, existErr := terraform.InitAndApplyE(t, existingTerraformOptions)
if existErr != nil {
assert.True(t, existErr == nil, "Init and Apply of temp existing resource failed")
} else {

// ------------------------------------------------------------------------------------
// Upgrade test for watsonx DA passing in existing KP details
// ------------------------------------------------------------------------------------

options := testhelper.TestOptionsDefault(&testhelper.TestOptions{
Testing: t,
TerraformDir: rootDaDir,
Prefix: "existing-kp-upg",
IgnoreDestroys: testhelper.Exemptions{ // Ignore for consistency check
List: []string{
"module.configure_user.null_resource.configure_user",
"module.configure_user.null_resource.restrict_access",
},
},
IgnoreUpdates: testhelper.Exemptions{ // Ignore for consistency check
List: []string{
"module.configure_user.null_resource.configure_user",
"module.configure_user.null_resource.restrict_access",
},
},
TerraformVars: map[string]interface{}{
"location": validRegions[rand.Intn(len(validRegions))],
"resource_group_name": prefix,
"provider_visibility": "public",
"cos_kms_crn": terraform.Output(t, existingTerraformOptions, "key_protect_crn"),
"cos_kms_key_crn": terraform.Output(t, existingTerraformOptions, "kms_key_crn"),
Copy link
Member

Choose a reason for hiding this comment

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

To enable encryption, you need to set enable_cos_kms_encryption = true. The fact that the test passed even when passing values for cos_kms_crn and cos_kms_key_crn makes me think we should add variable validation to the DA code to ensure that if user does pass values for these, that enable_cos_kms_encryption has to be set to true

Copy link
Member

Choose a reason for hiding this comment

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

It also looks like TestWithExistingKP also needs this update

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ocofaigh Added enable_cos_kms_encryption = true , still the test passed.

},
})

output, err := options.RunTestUpgrade()
assert.Nil(t, err, "This should not have errored")
assert.NotNil(t, output, "Expected some output")
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be updated to:

        output, err := options.RunTestUpgrade()
	if !options.UpgradeTestSkipped {
		assert.Nil(t, err, "This should not have errored")
		assert.NotNil(t, output, "Expected some output")
	}

}

// Check if "DO_NOT_DESTROY_ON_FAILURE" is set
envVal, _ := os.LookupEnv("DO_NOT_DESTROY_ON_FAILURE")
// Destroy the temporary existing resources if required
if t.Failed() && strings.ToLower(envVal) == "true" {
fmt.Println("Terratest failed. Debug the test and delete resources manually.")
} else {
logger.Log(t, "START: Destroy (existing resources)")
terraform.Destroy(t, existingTerraformOptions)
terraform.WorkspaceDelete(t, existingTerraformOptions, prefix)
logger.Log(t, "END: Destroy (existing resources)")
}

}
2 changes: 1 addition & 1 deletion variables.tf
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@ variable "cos_kms_key_crn" {
variable "cos_kms_new_key_name" {
description = "Name of the Key Protect key to create for encrypting the COS buckets used by the watsonx projects."
type = string
default = ""
default = "storage-delegation-key"
}

variable "cos_kms_ring_id" {
Expand Down