Skip to content

Conversation

@kierramarie
Copy link
Member

@kierramarie kierramarie commented Jun 6, 2025

Description

Updated standard solution to fully-configurable and added security enforced da.

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.

@kierramarie kierramarie requested review from iamar7 and shemau as code owners June 6, 2025 15:51
@kierramarie
Copy link
Member Author

I am unsure if the values in the .catalog-onboard-pipeline.yaml are correct.

@kierramarie
Copy link
Member Author

/run pipeline

@kierramarie
Copy link
Member Author

/run pipeline

@kierramarie
Copy link
Member Author

/run pipeline

@kierramarie
Copy link
Member Author

Only test that is failing is the upgrade test, which is expected.

@kierramarie
Copy link
Member Author

/run pipeline

Copy link
Contributor

@akocbek akocbek left a comment

Choose a reason for hiding this comment

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

Make sure that all changes that are for fully-configurable DA must be applied to security-enforced as well.

cra-config.yaml Outdated
TF_VAR_resource_group_name: "geretain-test-redis"
TF_VAR_existing_resource_group_name: "geretain-test-rabbitmq"
TF_VAR_provider_visibility: "public"
TF_VAR_use_ibm_owned_encryption_key: false
Copy link
Contributor

Choose a reason for hiding this comment

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

does fullly-configurable DA have use_ibm_owned_encryption_key input?
sync with Redis, pay attention to the order as well

"solution",
"rabbitmq standard",
"cache",
"in memory"
Copy link
Contributor

Choose a reason for hiding this comment

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

remove rabbitmq standard" keywords

"offering_icon_url": "https://raw.githubusercontent.com/terraform-ibm-modules/terraform-ibm-icd-rabbitmq/main/images/rabbitmq_icon.svg",
"provider_name": "IBM",
"support_details": "This product is in the community registry, as such support is handled through the originated repo. If you experience issues please open an issue in the repository [https://github.com/terraform-ibm-modules/terraform-ibm-icd-rabbitmq/issues](https://github.com/terraform-ibm-modules/terraform-ibm-icd-rabbitmq/issues). Please note this product is not supported via the IBM Cloud Support Center.",
"features": [
Copy link
Contributor

Choose a reason for hiding this comment

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

sync features with Redis. Check the order, add links to the features, etc.

ibm_catalog.json Outdated
{
"label": "Standard",
"name": "standard",
"label": "Fully Configurable",
Copy link
Contributor

Choose a reason for hiding this comment

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

should be "label": "Fully configurable",


##############################################################
# Encryption
##############################################################
Copy link
Contributor

Choose a reason for hiding this comment

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

tags rename to resource_tags,sync with Redis

error_message = "When 'use_ibm_owned_encryption_key' is false, you must provide either 'existing_kms_instance_crn' (to create a new key) or 'existing_kms_key_crn' (to use an existing key)."
condition = (var.existing_kms_instance_crn == null && var.existing_kms_key_crn == null && var.existing_backup_kms_key_crn == null) || var.kms_encryption_enabled
error_message = "When either 'existing_kms_instance_crn', 'existing_kms_key_crn' or 'existing_backup_kms_key_crn' is set then 'kms_encryption_enabled' must be set to true."
}
Copy link
Contributor

Choose a reason for hiding this comment

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

existing_kms_instance_crn sync desc with Redis

var.existing_secrets_manager_instance_crn != null
)
error_message = "`existing_secrets_manager_instance_crn` is required when adding service credentials to a secrets manager secret."
}
Copy link
Contributor

Choose a reason for hiding this comment

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

existing_backup_kms_key_crn sync with Redis

error_message = "`existing_secrets_manager_instance_crn` is required when adding service credentials to a secrets manager secret."
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

use_default_backup_encryption_key sync with redis

error_message = "`existing_secrets_manager_instance_crn` is required when adding service credentials to a secrets manager secret."
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

add empty line before variable "provider_visibility". sync with redis

@akocbek
Copy link
Contributor

akocbek commented Jun 30, 2025

CRA scanning failed with

Error: Invalid value for variable
│ 
│   on variables.tf line 160:
│  160: variable "kms_encryption_enabled" {
│     ├────────────────
│     │ var.existing_backup_kms_key_crn is null
│     │ var.existing_kms_instance_crn is "crn:v1:bluemix:public:hs-crypto:us-south:a/abac0df06b644a9cabc6e44f55b3880e:e6dce284-e80f-46e1-a3c1-830f7adff7a9::"
│     │ var.existing_kms_key_crn is "crn:v1:bluemix:public:hs-crypto:us-south:a/abac0df06b644a9cabc6e44f55b3880e:e6dce284-e80f-46e1-a3c1-830f7adff7a9:key:1368d2eb-3ed0-4a8b-b09c-2155895f01ea"
│     │ var.kms_encryption_enabled is false
│ 
│ When either 'existing_kms_instance_crn', 'existing_kms_key_crn' or
│ 'existing_backup_kms_key_crn' is set then 'kms_encryption_enabled' must be
│ set to true.

kms_encryption_enabled must be added to CRA

shemau
shemau previously requested changes Jun 30, 2025
tests/pr_test.go Outdated
TerraformDir: fullyConfigurableSolutionTerraformDir,
BestRegionYAMLPath: regionSelectionPath,
Prefix: "rabbitmq-st-da-upg",
Prefix: "rmq-da-upg",
Copy link
Contributor

Choose a reason for hiding this comment

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

This is still too long. Limit is 16 and the test infrastructure adds 7, so limit is 9 characters.

"ibmcloud_api_key": $VALIDATION_APIKEY,
"region": "us-south",
"tags": $TAGS,
"name": $PREFIX,
Copy link
Contributor

Choose a reason for hiding this comment

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

existing_resource_group_name is a required property. It should be included here for the catalog pipeline.

@kierramarie kierramarie requested review from akocbek and shemau June 30, 2025 13:52
@kierramarie
Copy link
Member Author

/run pipeline

Copy link
Contributor

@akocbek akocbek left a comment

Choose a reason for hiding this comment

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

about the diagrams:

Please use the latest stencil version. For example, the latest stencil gives you a dotted box for resource group.

resource group has dots, but its not the right box, should be red colored with icon, also other icons are different

image

also check how the KMS encryption is added in other DAs. For an example the redis
image

],
"service_name": "Resource group only",
"notes": "Viewer access is required in the resource group you want to provision in."
},
Copy link
Contributor

Choose a reason for hiding this comment

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

for Resource group only iam permission, role_crn should have one indent less

image

Copy link
Contributor

Choose a reason for hiding this comment

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

check security-enforced as well

ibm_catalog.json Outdated
"title": "KMS encryption",
"description": "Provides KMS encryption for the data that you store in the database."
"description": "Provides [KMS encryption](https://cloud.ibm.com/docs/messages-for-rabbitmq?topic=messages-for-rabbitmq-key-protect&interface=ui) for the data that you store in the database, enhancing data security."

Copy link
Contributor

Choose a reason for hiding this comment

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

remove extra line

{
"key": "existing_rabbitmq_instance_crn"
}
]
Copy link
Contributor

Choose a reason for hiding this comment

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

move "key": "cbr_rules" after existing_rabbitmq_instance_crn. following the rule that inputs that are not in redis, should be placed at the end

image

Copy link
Contributor

Choose a reason for hiding this comment

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

check security-enforced as well

"service_name": "kms"
"service_name": "hs-crypto",
"notes": "[Optional] Editor access is required to create keys in HPCS. It is only required when using HPCS for encryption."
}
Copy link
Contributor

Choose a reason for hiding this comment

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

existing_resource_group_name -> delete required
check security-enforced as well

type = number
description = "The number of members that are allocated. [Learn more](https://cloud.ibm.com/docs/messages-for-rabbitmq?topic=messages-for-rabbitmq-resources-scaling)."
default = 3
default = 2
Copy link
Contributor

Choose a reason for hiding this comment

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

pipeline failed, it looks it has to be 3 after all. let's update it everywhere back to 3.

@kierramarie
Copy link
Member Author

/run pipeline

@kierramarie kierramarie requested a review from akocbek June 30, 2025 16:27
"key": "cbr_rules"
}
]
}
Copy link
Contributor

Choose a reason for hiding this comment

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

diagrams not the same in light and dark mode (do we need that extra arrow?). Also name the key, probably would be rabbitmq-key?

image image

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah that arrow was an accident. I've removed it.

@kierramarie
Copy link
Member Author

/run pipeline

@kierramarie kierramarie requested a review from akocbek June 30, 2025 16:54
@kierramarie
Copy link
Member Author

/run pipeline

@kierramarie
Copy link
Member Author

Only upgrade test is failing now.

@akocbek
Copy link
Contributor

akocbek commented Jun 30, 2025

@kierramarie upgrade test failed because

 TestRunFullyConfigurableUpgradeSolution 2025-06-30T17:37:52Z retry.go:99: Returning due to fatal error: FatalError{Underlying: error while running command: chdir /tmp/terraform-base-rmq-upg-3pt1095509553/solutions/fully-configurable

this is expected, since main branch doesn't have solutions/fully-configurable yet. It is fine to skip the upgrade test

@kierramarie
Copy link
Member Author

/run pipeline

@akocbek akocbek removed request for iamar7 and shemau June 30, 2025 19:10
@toddgiguere toddgiguere dismissed shemau’s stale review June 30, 2025 20:05

All review issues resolved

@toddgiguere toddgiguere merged commit a04664d into main Jun 30, 2025
2 checks passed
@toddgiguere toddgiguere deleted the ks-da branch June 30, 2025 20:07
@terraform-ibm-modules-ops
Copy link
Contributor

🎉 This PR is included in version 2.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants