Event Notification DA Fully configurable #395

whoffler · 2025-03-04T10:23:42Z

Description

for issue - https://github.ibm.com/GoldenEye/issues/issues/12573

Event Notification DA Fully configurable flavor

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.

solutions/fully-configurable/main.tf

ocofaigh · 2025-03-11T09:55:52Z

@whoffler Vipin has a PR open to update the current standard variation with some best practises. Since that variation is going away, can I suggest we just make the changes directly in this PR in the new variation you are adding?

ocofaigh

Left an initial first round of comments to address - there will be more. Also we need to claify if we really should be using cloud_object_storage and key_management_service in our input names, as opposed to just cos and kms. Discussion started on slack

README.md

ocofaigh · 2025-03-13T15:19:05Z

solutions/security-enforced/version.tf

@@ -0,0 +1,14 @@
+terraform {
+  required_version = ">= 1.3.0"


I suggest you update to >= 1.9.0 so we can use the terraform 1.9 cross variable validation feature.
We will need to use it in many input variables in variables.tf. For an idea of how to use it, see my branch (WIP): https://github.com/terraform-ibm-modules/terraform-ibm-scc/blob/DA/solutions/fully-configurable/variables.tf

solutions/security-enforced/version.tf

solutions/security-enforced/DA-cbr_rules.md

solutions/fully-configurable/variables.tf

solutions/fully-configurable/main.tf

solutions/fully-configurable/catalogValidationValues.json.template

solutions/fully-configurable/README.md

ocofaigh

@whoffler still a lot of things to address here. Its not consistent at all with the SCC DA. Please review your variable descriptions, variable validation and other logic to ensure consistency. Some other things that stand out:

security-enforced variation needs to be a striaght wrapper around fully-configurable. See https://github.com/terraform-ibm-modules/terraform-ibm-scc/tree/main/solutions/security-enforced as a reference
It seems we are supporting creation of cross regional buckets. We are not doing that anywhere else right now, so I think remove it so its less confusing for consumers

solutions/fully-configurable/variables.tf

ocofaigh · 2025-03-24T16:54:45Z

solutions/fully-configurable/variables.tf

+  }
+}
+
+variable "existing_kms_key_name" {


We will never need a existing KMS key name?? The user will either pass the key using existing_kms_root_key_crn, or we will create the key using existing_kms_instance_crn so I'm confused as to what this is for?

We might need 2 existing KMS key inputs - one for the COS bucket, and one for the Event Notifications instance

Yeah if a KMS key is provided to the fscloud version it uses it for everything. Adding another KMS key input makes sense.. more configurable

solutions/fully-configurable/variables.tf

solutions/fully-configurable/main.tf

ibm_catalog.json

ocofaigh

@whoffler Also take a look at terraform-ibm-modules/terraform-ibm-secrets-manager#309 - we will need something similar in this PR

whoffler · 2025-03-25T10:43:19Z

@ocofaigh

security-enforced variation needs to be a striaght wrapper around fully-configurable. See https://github.com/terraform-ibm-modules/terraform-ibm-scc/tree/main/solutions/security-enforced as a reference

was going to do that in a separate PR but will add here instead

ocofaigh · 2025-03-25T11:09:42Z

@whoffler A separate Pr is fine if you want - but just remove the security-enforced code from this PR then

rajatagarwal-ibm · 2025-04-11T16:21:01Z

/run pipeline

rajatagarwal-ibm · 2025-04-11T20:31:24Z

/run pipeline

rajatagarwal-ibm · 2025-04-15T13:54:01Z

/run pipeline

rajatagarwal-ibm · 2025-04-16T10:56:56Z

/run pipeline

rajatagarwal-ibm · 2025-04-16T12:19:29Z

/run pipeline

rajatagarwal-ibm · 2025-04-25T09:21:36Z

@ocofaigh working with @whoffler on this, and will try to get that done as soo as we can. Thanks!

whoffler · 2025-04-26T09:16:29Z

@rajatagarwal-ibm @ocofaigh think I've figured out why that unit test is failing.

For COS bucket when we set kms_encryption_enabled to true then the cos bucket module creates a policy:

# Create IAM Authorization Policy to allow COS to access KMS for the encryption key
resource "ibm_iam_authorization_policy" "policy" {

The existing standard solution does not pass kms_encryption_enabled variable to cos module

In Event Notifications we create policy:

# Create cross account IAM Authorization Policy to allow COS to read the KMS encryption key
resource "ibm_iam_authorization_policy" "cos_kms_policy" {

Which creates the access policy conflict we are seeing.

Removing kms_encryption_enabled and the test passes for me. Maybe we remove the access policy altogether from Event Notifications and let COS module do its thing?

SKIP UPGRADE TEST: breaking changes

whoffler · 2025-04-28T15:38:52Z

/run pipeline

whoffler · 2025-04-28T16:17:16Z

/run pipeline

whoffler · 2025-04-28T17:32:27Z

/run pipeline

whoffler · 2025-04-28T18:19:53Z

/run pipeline

whoffler · 2025-04-28T19:32:35Z

/run pipeline

whoffler · 2025-04-28T21:18:41Z

/run pipeline

rajatagarwal-ibm · 2025-04-28T21:54:27Z

/run pipeline

akocbek · 2025-04-29T11:10:01Z

cra-config.yaml

@@ -1,12 +1,14 @@
 # More info about this file at https://github.com/terraform-ibm-modules/common-pipeline-assets/blob/main/.github/workflows/terraform-test-pipeline.md#cra-config-yaml
 version: "v1"
 CRA_TARGETS:
-  - CRA_TARGET: "solutions/standard" # Target directory for CRA scan. If not provided, the CRA Scan will not be run.
+  - CRA_TARGET: "solutions/fully-configurable" # Target directory for CRA scan. If not provided, the CRA Scan will not be run.
    CRA_IGNORE_RULES_FILE: "cra-tf-validate-ignore-rules.json" # CRA Ignore file to use. If not provided, it checks the repo root directory for `cra-tf-validate-ignore-rules.json`
    PROFILE_ID: "fe96bd4d-9b37-40f2-b39f-a62760e326a3"         # SCC profile ID (currently set to 'IBM Cloud Framework for Financial Services' '1.7.0' profile).
    CRA_ENVIRONMENT_VARIABLES:


should TF_VAR_kms_encryption_enabled: true be added as well?

No, it is not required as the fully-configurable does not enforce KMS encryption.

akocbek · 2025-04-29T11:29:03Z

ibm_catalog.json

-            },
-            {
-              "key": "resource_group_name"
+              "key": "existing_resource_group_name"


picker should be used

{ "key": "existing_resource_group_name", "required": true, "custom_config": { "type": "resource_group", "grouping": "deployment", "original_grouping": "deployment", "config_constraints": { "identifier": "rg_name" } } },

akocbek · 2025-04-29T11:30:35Z

ibm_catalog.json

@@ -169,10 +357,27 @@
              ]
            },
            {
-              "key": "en_key_ring_name"
+              "key": "provider_visibility",


should be hidden.

"hidden": true

Not sure yet if we are going to hide this - it will be discussed in todays deep dive

ocofaigh

Please do a full re-review of the code for consistency between the 2 variations, and between other DAs (for example SCC DA)

ocofaigh · 2025-04-29T11:14:34Z

solutions/fully-configurable/variables.tf

+variable "kms_encryption_enabled" {
+  type        = bool
+  description = "Set to true to enable KMS encryption on Event Notifications instance and Cloud Object Storage bucket. When set to true 'kms_endpoint_url' and one of 'existing_kms_instance_crn' or 'existing_kms_root_key_crn' must be set."
+  default     = true


This should not default to true in fully configurable because the user is then required to pass either a KMS instance CRN, or a KMS key. We want fully configurable to be like a one click deploy. This is consistent with all our other fully configurable DAs

ocofaigh · 2025-04-29T11:16:03Z

solutions/fully-configurable/variables.tf

+variable "enable_collecting_failed_events" {
+  type        = bool
+  description = "Set to true to enable Cloud Object Storage integration."
+  default     = true


Same as above, I don't think this should be true by default in fully configurable, since it will have a dependency on COS

ocofaigh · 2025-04-29T11:18:27Z

solutions/fully-configurable/variables.tf

  type        = bool
-  description = "Whether to add a randomly generated 4-character suffix to the newly provisioned Object Storage bucket name. Used only if not using an existing bucket. Set to `false` if you want full control over bucket naming by using the `cos_bucket_name` variable."
+  description = "Set to true to enable key management service encryption on Cloud Object Storage bucket."
  default     = true


same as above, this should not default to true for fully configurable

ocofaigh · 2025-04-29T11:19:57Z

solutions/fully-configurable/variables.tf

+variable "cos_bucket_region" {
+  type        = string
+  description = "The COS bucket region. If `cos_bucket_region` is set to null, then `region` will be used."
+  default     = "us-south"


Can we take same approach as SCC DA where default value will be null (meaning it will be provisioned in same region as the EN instance). See https://github.com/terraform-ibm-modules/terraform-ibm-scc/blob/6fbd4973c72855920c157848ab9eee0b7cce5439/solutions/fully-configurable/variables.tf#L301-L305

ocofaigh · 2025-04-29T11:21:08Z

solutions/security-enforced/README.md

+When `existing_en_instance_crn` is not passed, this solution configures the following infrastructure:
+
+- optionally a KMS key ring and key for IBM Event Notifications encryption
+- optionally a KMS key ring and key for IBM Cloud Object Storage encryption


KMS is mandated in security enforced. So I would not use the word "optionally" here. The options are the DA either creates the key and key ring, or the DA takes in existing key

ocofaigh · 2025-04-29T11:24:12Z

solutions/security-enforced/variables.tf

+variable "existing_cos_instance_crn" {
+  type        = string
+  nullable    = false
+  description = "The CRN of an IBM Cloud Object Storage instance."


Description should say what its used for

ocofaigh · 2025-04-29T11:25:25Z

solutions/security-enforced/variables.tf

+
+variable "kms_endpoint_url" {
+  type        = string
+  description = "The KMS endpoint URL to use when you configure KMS encryption. The Hyper Protect Crypto Services endpoint URL format is `https://api.private.<REGION>.hs-crypto.cloud.ibm.com:<port>` and the Key Protect endpoint URL format is `https://<REGION>.kms.cloud.ibm.com`. Not required if passing an existing instance using the `existing_en_instance_crn` input."


existing_en_instance_crn is not a valid input. Do we have variable validation checking the condition mentioned?

ocofaigh · 2025-04-29T11:26:05Z

solutions/security-enforced/variables.tf

+  type        = string
+  description = "The key CRN of a root key, existing in the KMS instance passed in the `existing_kms_instance_crn` input, which will be used to encrypt the data. If no value passed, a new key will be created in the instance provided in the `existing_kms_instance_crn` input."
+  default     = null
+}


You seem to be missing variable validation for the KMS inputs. For example: https://github.com/terraform-ibm-modules/terraform-ibm-scc/blob/6fbd4973c72855920c157848ab9eee0b7cce5439/solutions/security-enforced/variables.tf#L159-L181

ocofaigh · 2025-04-29T11:26:50Z

solutions/security-enforced/variables.tf

+variable "event_notifications_name" {
+  type        = string
+  description = "The name of the Event Notifications instance that is created by this solution. If a `prefix` input variable is specified, it is added to this name in the `<prefix>-value` format."
+  default     = "base-event-notifications"


This does not match the value in fully configurable. Can you please do a full review to ensure consistency

ocofaigh · 2025-04-29T11:28:15Z

solutions/security-enforced/variables.tf

+
+variable "prefix" {
+  type        = string
+  description = "(Optional) Prefix to add to all resources created by this solution. To not use any prefix value, you can set this value to `null` or an empty string."


why is this description different to the fully configurable one? Please ensure consistency

akocbek · 2025-04-29T12:10:19Z

solutions/security-enforced/README.md

@@ -0,0 +1,82 @@
+# Event Notifications solution


security-enforced should be added to the title

akocbek · 2025-04-29T12:11:31Z

solutions/fully-configurable/README.md

@@ -0,0 +1,109 @@
+# Cloud automation for Event Notifications (Fully configurable)


to be in consistent with other DAs, let's follow the structure by defining Prerequisites as well . Same for security-enforced DA

https://github.com/terraform-ibm-modules/terraform-ibm-scc/blob/main/solutions/fully-configurable/README.md#prerequisites

whoffler force-pushed the flexibleVariation branch 4 times, most recently from fd9cef1 to 5c55830 Compare March 5, 2025 11:57

whoffler commented Mar 6, 2025

View reviewed changes

solutions/fully-configurable/main.tf Outdated Show resolved Hide resolved

whoffler commented Mar 6, 2025

View reviewed changes

solutions/fully-configurable/main.tf Outdated Show resolved Hide resolved

whoffler commented Mar 6, 2025

View reviewed changes

solutions/fully-configurable/main.tf Outdated Show resolved Hide resolved

ocofaigh mentioned this pull request Mar 11, 2025

refactor: da best input practices #393

Open

6 tasks

whoffler changed the title ~~Event Notification DA Fully configurable - Work In Progress~~ Event Notification DA Fully configurable Mar 13, 2025

ocofaigh requested changes Mar 13, 2025

View reviewed changes

whoffler force-pushed the flexibleVariation branch 2 times, most recently from abeb33c to dcf9464 Compare March 20, 2025 09:08

whoffler requested a review from ocofaigh March 20, 2025 09:25

whoffler force-pushed the flexibleVariation branch from 29995b7 to 8d7f1f9 Compare March 20, 2025 11:04

ocofaigh requested changes Mar 24, 2025

View reviewed changes

ocofaigh reviewed Mar 24, 2025

View reviewed changes

ibm_catalog.json Outdated Show resolved Hide resolved

ocofaigh reviewed Mar 24, 2025

View reviewed changes

whoffler force-pushed the flexibleVariation branch 2 times, most recently from 71ea249 to b3f69cf Compare March 27, 2025 10:42

rajatagarwal-ibm marked this pull request as ready for review April 11, 2025 16:19

rajatagarwal-ibm requested review from Ak-sky and Aashiq-J as code owners April 11, 2025 16:19

whoffler and others added 11 commits April 28, 2025 16:24

WIP

6ea3f04

address ful-con feedback, add sec-enf wrapper

a849703

rajat-updates

21a0e72

SKIP UPGRADE TEST: breaking changes

rajat-updates

6ce7ed7

SKIP UPGRADE TEST: breaking changes

rajat-updates

2ce9669

SKIP UPGRADE TEST: breaking changes

rajat-updates

39c4097

SKIP UPGRADE TEST: breaking changes

rajat-updates

3152afb

SKIP UPGRADE TEST: breaking changes

rajat-updates

0780cf3

SKIP UPGRADE TEST: breaking changes

rajat-updates

6142612

SKIP UPGRADE TEST: breaking changes

rajat-updates

1fc962d

SKIP UPGRADE TEST: breaking changes

fix unit tests

60653b0

rebase on main

4898435

whoffler force-pushed the flexibleVariation branch from 782ce09 to 4898435 Compare April 28, 2025 15:47

rebase on main

5703e02

fix

e44774b

akocbek reviewed Apr 29, 2025

View reviewed changes

ocofaigh requested changes Apr 29, 2025

View reviewed changes

akocbek reviewed Apr 29, 2025

View reviewed changes

		@@ -0,0 +1,109 @@
		# Cloud automation for Event Notifications (Fully configurable)

Event Notification DA Fully configurable #395

Are you sure you want to change the base?

Event Notification DA Fully configurable #395

Conversation

whoffler commented Mar 4, 2025 • edited Loading

Description

Release required?

Release notes content

Run the pipeline

Checklist for reviewers

For mergers

ocofaigh commented Mar 11, 2025 • edited Loading

ocofaigh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ocofaigh left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ocofaigh left a comment

Choose a reason for hiding this comment

whoffler commented Mar 25, 2025

ocofaigh commented Mar 25, 2025

rajatagarwal-ibm commented Apr 11, 2025

rajatagarwal-ibm commented Apr 11, 2025

rajatagarwal-ibm commented Apr 15, 2025

rajatagarwal-ibm commented Apr 16, 2025

rajatagarwal-ibm commented Apr 16, 2025

rajatagarwal-ibm commented Apr 25, 2025

whoffler commented Apr 26, 2025 • edited Loading

whoffler commented Apr 28, 2025

whoffler commented Apr 28, 2025

whoffler commented Apr 28, 2025

whoffler commented Apr 28, 2025

whoffler commented Apr 28, 2025

whoffler commented Apr 28, 2025

rajatagarwal-ibm commented Apr 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ocofaigh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whoffler commented Mar 4, 2025 •

edited

Loading

ocofaigh commented Mar 11, 2025 •

edited

Loading

ocofaigh left a comment •

edited

Loading

whoffler commented Apr 26, 2025 •

edited

Loading