feat: updated KMS auth policy scope to scope to the KMS key + cleanup #313

ocofaigh · 2024-10-14T18:26:36Z

Description

updated KMS auth policy scope to scope to the KMS key (expect this to fail upgrade test however Im using create before destroy to prevent disruption to service)
Started to use the CRN parser instead of messy regex
During cleanup I found a few bugs that I fixed, mainly around incorrect validation
Added code comments to make the code easier to read and maintain

Release required?

No release
Patch release (x.x.X)
Minor release (x.X.x)
Major release (X.x.x)

Release notes content

Run the pipeline

If the CI pipeline doesn't run when you create the PR, the PR requires a user with GitHub collaborators access to run the pipeline.

Run the CI pipeline when the PR is ready for review and you expect tests to pass. Add a comment to the PR with the following text:

/run pipeline

Checklist for reviewers

If relevant, a test for the change is included or updated with this PR.
If relevant, documentation for the change is included or updated with this PR.

For mergers

Use a conventional commit message to set the release level. Follow the guidelines.
Include information that users need to know about the PR in the commit message. The commit message becomes part of the GitHub release notes.
Use the Squash and merge option.

ocofaigh · 2024-10-14T18:35:10Z

/run pipeline

ocofaigh · 2024-10-14T19:16:40Z

As expected the upgrade test failed as it wants to recreate the auth policy. But it also failed on this update and no idea why...

        	Messages:   	Resource(s) identified to be updated 
        	            	Name: en_cos_integration
        	            	Address: module.event_notifications[0].ibm_en_integration_cos.en_cos_integration[0]
        	            	Actions: [update]
        	            	DIFF:
        	            	  Before: 
        	            		{"metadata":"SECURE_VALUE_HIDDEN_HASH:-cfa39bbf1424dbe87cb424f2a794fb25c65c2a34d1694dc0b6f5cb53"}
        	            	  After: 
        	            		{"metadata":"SECURE_VALUE_HIDDEN_HASH:-1eb7d7592816a31b59566ff9a7fd7863426a53a7dafd3a993b2110ff"}

I'm going to have to do a manual upgrade test to figure this out

ocofaigh · 2024-10-14T20:12:05Z

OK after doing manual check, I see why the update is happening. Its because I updated the default COS endpoint to be the direct endpoint instead of public one:

2024/10/14 20:07:29 Terraform plan |   # module.event_notifications[0].ibm_en_integration_cos.en_cos_integration[0] will be updated in-place
 2024/10/14 20:07:29 Terraform plan |   ~ resource "ibm_en_integration_cos" "en_cos_integration" {
 2024/10/14 20:07:29 Terraform plan |         id             = "77ce8909-eca2-450d-8875-8e84ea56eaf4/7a7ac1cb-049a-4017-bc35-4c235f8f29c5"
 2024/10/14 20:07:29 Terraform plan |         # (4 unchanged attributes hidden)
 2024/10/14 20:07:29 Terraform plan | 
 2024/10/14 20:07:29 Terraform plan |       ~ metadata {
 2024/10/14 20:07:29 Terraform plan |           ~ endpoint    = "https://s3.us-south.cloud-object-storage.appdomain.cloud" -> "https://s3.direct.us-south.cloud-object-storage.appdomain.cloud"
 2024/10/14 20:07:29 Terraform plan |             # (2 unchanged attributes hidden)
 2024/10/14 20:07:29 Terraform plan |         }
 2024/10/14 20:07:29 Terraform plan |     }

This is also expected, and non disruptive, so skipping upgrade test.
@Ak-sky can you please review?

ocofaigh · 2024-10-14T20:13:41Z

/run pipeline

Ak-sky · 2024-10-15T05:43:24Z

TestRunExistingResourcesInstances Test failed in Schematics with below error during TF plan-

024/10/14 20:25:15 Terraform plan |   # time_sleep.wait_for_en_authorization_policy will be created
 2024/10/14 20:25:15 Terraform plan |   + resource "time_sleep" "wait_for_en_authorization_policy" {
 2024/10/14 20:25:15 Terraform plan |       + create_duration = "30s"
 2024/10/14 20:25:15 Terraform plan |       + id              = (known after apply)
 2024/10/14 20:25:15 Terraform plan |     }
 2024/10/14 20:25:15 Terraform plan | 
 2024/10/14 20:25:15 Terraform plan | Plan: 1 to add, 0 to change, 0 to destroy.
 2024/10/14 20:25:15 Terraform plan | 
 2024/10/14 20:25:15 Terraform plan | Changes to Outputs:
 2024/10/14 20:25:15 Terraform plan |   + crn                              = "crn:v1:bluemix:public:event-notifications:au-syd:a/abac0df06b644a9cabc6e44f55b3880e:aab948d3-fff1-496f-9967-44ceb6135554::"
 2024/10/14 20:25:15 Terraform plan |   + event_notification_instance_name = "en-existing-t6wbsk-en"
 2024/10/14 20:25:15 Terraform plan |   + guid                             = "aab948d3-fff1-496f-9967-44ceb6135554"
 2024/10/14 20:25:15 Terraform plan | 
 2024/10/14 20:25:15 Terraform plan | Error: Invalid index
 2024/10/14 20:25:15 Terraform plan | 
 2024/10/14 20:25:15 Terraform plan |   on main.tf line 235, in locals:
 2024/10/14 20:25:15 Terraform plan |  235:   cos_instance_guid = var.existing_cos_instance_crn == null ? module.cos[0].cos_instance_guid : module.cos_instance_crn_parser[0].service_instance
 2024/10/14 20:25:15 Terraform plan |     ├────────────────
 2024/10/14 20:25:15 Terraform plan |     │ module.cos is empty tuple
 2024/10/14 20:25:15 Terraform plan | 
 2024/10/14 20:25:15 Terraform plan | The given key does not identify an element in this collection value: the
 2024/10/14 20:25:15 Terraform plan | collection has no elements.
 2024/10/14 20:25:16 Terraform plan | 
 2024/10/14 20:25:16 Terraform plan | Error: Invalid index
 2024/10/14 20:25:16 Terraform plan | 
 2024/10/14 20:25:16 Terraform plan |   on main.tf line 238, in locals:
 2024/10/14 20:25:16 Terraform plan |  238:   cos_account_id = var.existing_cos_instance_crn != null ? split("/", module.cos_instance_crn_parser[0].scope)[1] : module.cos[0].cos_account_id
 2024/10/14 20:25:16 Terraform plan |     ├────────────────
 2024/10/14 20:25:16 Terraform plan |     │ module.cos is empty tuple
 2024/10/14 20:25:16 Terraform plan | 
 2024/10/14 20:25:16 Terraform plan | The given key does not identify an element in this collection value: the
 2024/10/14 20:25:16 Terraform plan | collection has no elements.
 2024/10/14 20:25:16 Terraform plan | 
 2024/10/14 20:25:16 Terraform plan | Error: Invalid function argument
 2024/10/14 20:25:16 Terraform plan | 
 2024/10/14 20:25:16 Terraform plan |   on .terraform/modules/cos_kms_key_crn_parser/modules/crn-parser/main.tf line 6, in locals:
 2024/10/14 20:25:16 Terraform plan |    6:   split_crn        = coalescelist(split(":", var.crn))
 2024/10/14 20:25:16 Terraform plan |     ├────────────────
 2024/10/14 20:25:16 Terraform plan |     │ while calling split(separator, str)
 2024/10/14 20:25:16 Terraform plan |     │ var.crn is null
 2024/10/14 20:25:16 Terraform plan | 
 2024/10/14 20:25:16 Terraform plan | Invalid value for "str" parameter: argument must not be null.
 2024/10/14 20:25:16 �[1m�[31mTerraform PLAN error: Terraform PLAN errorexit status 1�[39m�[0m

ocofaigh · 2024-10-15T10:01:48Z

/run pipeline

ocofaigh · 2024-10-15T19:03:34Z

/run pipeline

ocofaigh · 2024-10-15T21:16:05Z

@Ak-sky I addressed the issues, and also added more tests. Can you please re-review?

…-event-notifications into auth

ocofaigh · 2024-10-16T08:23:33Z

/run pipeline

ocofaigh · 2024-10-16T08:48:26Z

/run pipeline

Aashiq-J

Just some minor changes related to the code comments.
Rest everything looks good to me.

Aashiq-J · 2024-10-16T09:00:40Z

solutions/standard/main.tf

+  # If a prefix value is passed, add it to the COS key ring name
+  cos_key_ring_name = var.prefix != null ? "${var.prefix}-${var.cos_key_ring_name}" : var.cos_key_ring_name
+  # Determine the COS KMS key CRN (new key or existing key). It will only have a value if not using an existing bucket or existing EN instance
+  # cos_kms_key_crn = var.existing_cos_bucket_name != null ? null : var.existing_kms_root_key_crn != null ? var.existing_kms_root_key_crn : var.existing_en_instance_crn == null ? module.kms[0].keys[format("%s.%s", local.cos_key_ring_name, local.cos_key_name)].crn : null


We can remove this commented out code, right?

Aashiq-J · 2024-10-16T09:01:26Z

solutions/standard/main.tf

+  # cos_kms_key_crn = var.existing_cos_bucket_name != null ? null : var.existing_kms_root_key_crn != null ? var.existing_kms_root_key_crn : var.existing_en_instance_crn == null ? module.kms[0].keys[format("%s.%s", local.cos_key_ring_name, local.cos_key_name)].crn : null
+  cos_kms_key_crn = var.existing_en_instance_crn != null || var.existing_cos_bucket_name != null ? null : var.existing_kms_root_key_crn != null ? var.existing_kms_root_key_crn : module.kms[0].keys[format("%s.%s", local.cos_key_ring_name, local.cos_key_name)].crn
+  # If existing KMS instance CRN passed, parse the account ID from it
+  cos_kms_key_id = local.cos_kms_key_crn != null ? module.cos_kms_key_crn_parser[0].resource : null


The above comment mentions about parsing the account ID, but we are getting the key_id.

Aashiq-J · 2024-10-16T09:06:45Z

solutions/standard/main.tf

+  validate_sm_crn = length(var.service_credential_secrets) > 0 && var.existing_secrets_manager_instance_crn == null ? tobool("'existing_secrets_manager_instance_crn' is required when adding service credentials with the 'service_credential_secrets' input.") : false
+  # parse SM GUID from CRN
+  existing_secrets_manager_instance_guid = var.existing_secrets_manager_instance_crn != null ? module.existing_sm_crn_parser[0].service_instance : null
+  # parse SM regiob from CRN


SM regiob -> SM region

ocofaigh · 2024-10-16T10:14:41Z

/run pipeline

terraform-ibm-modules-ops · 2024-10-16T12:10:37Z

🎉 This PR is included in version 1.13.0 🎉

The release is available on:

GitHub release
v1.13.0

Your semantic-release bot 📦🚀

cleanup

de23a02

ocofaigh requested review from Aashiq-J and Ak-sky as code owners October 14, 2024 18:26

update desc

60ee5c7

SKIP UPGRADE TEST - see pr comments

46c9d9b

update tests

48d7cf0

ocofaigh added 3 commits October 15, 2024 19:06

add extra tests

b69600c

fix test

3c18ed6

update validation

6dc6e2b

Merge branch 'main' of github.com:terraform-ibm-modules/terraform-ibm…

0f791ce

…-event-notifications into auth

Aashiq-J reviewed Oct 16, 2024

View reviewed changes

pr comments

0f7a719

Aashiq-J approved these changes Oct 16, 2024

View reviewed changes

ocofaigh merged commit 80b244f into main Oct 16, 2024
2 checks passed

ocofaigh deleted the auth branch October 16, 2024 12:10

terraform-ibm-modules-ops added the released label Oct 16, 2024

ocofaigh mentioned this pull request Oct 23, 2024

Add second type of existing resources test to event notifications repo #314

Closed

Uh oh!

feat: updated KMS auth policy scope to scope to the KMS key + cleanup #313

feat: updated KMS auth policy scope to scope to the KMS key + cleanup #313

Uh oh!

Conversation

ocofaigh commented Oct 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Release required?

Release notes content

Run the pipeline

Checklist for reviewers

For mergers

Uh oh!

ocofaigh commented Oct 14, 2024

Uh oh!

ocofaigh commented Oct 14, 2024

Uh oh!

ocofaigh commented Oct 14, 2024

Uh oh!

ocofaigh commented Oct 14, 2024

Uh oh!

Ak-sky commented Oct 15, 2024

Uh oh!

ocofaigh commented Oct 15, 2024

Uh oh!

ocofaigh commented Oct 15, 2024

Uh oh!

ocofaigh commented Oct 15, 2024

Uh oh!

ocofaigh commented Oct 16, 2024

Uh oh!

ocofaigh commented Oct 16, 2024

Uh oh!

Aashiq-J left a comment

Choose a reason for hiding this comment

Uh oh!

Aashiq-J Oct 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Aashiq-J Oct 16, 2024

Choose a reason for hiding this comment

Uh oh!

Aashiq-J Oct 16, 2024

Choose a reason for hiding this comment

Uh oh!

ocofaigh commented Oct 16, 2024

Uh oh!

Uh oh!

terraform-ibm-modules-ops commented Oct 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ocofaigh commented Oct 14, 2024 •

edited

Loading

Aashiq-J Oct 16, 2024 •

edited

Loading