feat: expose new s2s iam #301

alex-reiff · 2025-03-06T04:47:24Z

Description

Expose built-in IAM engine to fscloud submodule (and 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.

alex-reiff · 2025-03-06T04:47:39Z

/run pipeline

ocofaigh

Why are we not consistent with the variable description in the module?

ocofaigh · 2025-03-06T08:40:59Z

solutions/standard/variables.tf

-  type        = string
-  description = "The name of the IAM engine used to configure a Secrets Manager IAM credentials engine. If the prefix input variable is passed it is attached before the value in the format of '<prefix>-value'."
-  default     = "iam-engine"
+  description = "Set this to false to enable a Secrets Manager IAM credentials engine. If set to true, no IAM engine will be configured for your instance."


Why are we not consistent with the variable description in the module?

ocofaigh · 2025-03-06T08:41:11Z

modules/fscloud/variables.tf

@@ -35,6 +35,12 @@ variable "existing_sm_instance_crn" {
  default     = null
 }

+variable "skip_iam_authorization_policy" {
+  type        = bool
+  description = "Set this to true to skip the creation of a Secrets Manager IAM credentials engine. If set to false, an IAM engine will be configured for your instance."


Why are we not consistent with the variable description in the module?

ocofaigh

I was wondering how the upgrade test is not failing as it should be destroying the old IAM engine resources (like the service ID and apikey) and after checking it seems we have no upgrade test for the DA! This is big gap. Can you please add an upgrade test for the DA (using RunSchematicUpgradeTest) in a separate PR first please (created tracking issue)?

ocofaigh · 2025-03-12T16:52:13Z

@alex-reiff can we address this now that upgrade test pr is merged?

alex-reiff · 2025-03-12T17:35:49Z

/run pipeline

alex-reiff · 2025-03-12T18:59:57Z

/run pipeline

alex-reiff · 2025-03-12T20:58:35Z

@ocofaigh DA was not creating an IAM engine before. Do we want to make that the new default?

(upgrade test failing as expected)

ocofaigh · 2025-03-15T14:21:35Z

@alex-reiff The DA is creating IAM engine ->

terraform-ibm-secrets-manager/solutions/standard/main.tf

Lines 123 to 131 in f12d4e9

    
           module "iam_secrets_engine" { 
        
             count                = var.iam_engine_enabled ? 1 : 0 
        
             source               = "terraform-ibm-modules/secrets-manager-iam-engine/ibm" 
        
             version              = "1.2.8" 
        
             region               = local.secrets_manager_region 
        
             iam_engine_name      = try("${local.prefix}-${var.iam_engine_name}", var.iam_engine_name) 
        
             secrets_manager_guid = local.secrets_manager_guid 
        
             endpoint_type        = "private" 
        
           }

Since this variation is actually getting replace by new variations in #300 is suggest to just leave the current DA as is, and we will only expose the new s2s auth policy in the new variation

alex-reiff · 2025-03-19T03:41:45Z

@ocofaigh So is there anything to do in this PR?

alex-reiff · 2025-03-19T15:08:13Z

/run pipeline

alex-reiff · 2025-03-19T15:55:32Z

/run pipeline

ocofaigh · 2025-03-19T15:59:22Z

@alex-reiff Remember to paste the reason for the previously failed run so we can track them. Looks like an issue with schematics talking to COS:
2025/03/19 15:44:36 Failed to upload file to object storage, %!(EXTRA string=File upload error: rpc error: code = Unavailable desc = keepalive ping failed to receive ACK within timeout)

I'll report to schematics team.

Also I see several warnings in the logs - Can you address these too (maybe create a new tracking issue and handle in new PR). I think we are passing inputs in the test that are not valid:

 2025/03/19 15:42:37 Terraform refresh | Warning: Value for undeclared variable
         2025/03/19 15:42:37 Terraform refresh | 
         2025/03/19 15:42:37 Terraform refresh | The root module does not declare a variable named "ca_name" but a value was
         2025/03/19 15:42:37 Terraform refresh | found in file "schematics.tfvars". If you meant to use this value, add a
         2025/03/19 15:42:37 Terraform refresh | "variable" block to the configuration.
         2025/03/19 15:42:37 Terraform refresh | 
         2025/03/19 15:42:37 Terraform refresh | To silence these warnings, use TF_VAR_... environment variables to provide
         2025/03/19 15:42:37 Terraform refresh | certain "global" settings to all configurations in your organization. To
         2025/03/19 15:42:37 Terraform refresh | reduce the verbosity of these warnings, use the -compact-warnings option.
         2025/03/19 15:42:37 Terraform refresh | 
         2025/03/19 15:42:37 Terraform refresh | Warning: Value for undeclared variable
         2025/03/19 15:42:37 Terraform refresh | 
         2025/03/19 15:42:37 Terraform refresh | The root module does not declare a variable named "private_engine_enabled"
         2025/03/19 15:42:37 Terraform refresh | but a value was found in file "schematics.tfvars". If you meant to use this
         2025/03/19 15:42:37 Terraform refresh | value, add a "variable" block to the configuration.
         2025/03/19 15:42:37 Terraform refresh | 
         2025/03/19 15:42:37 Terraform refresh | To silence these warnings, use TF_VAR_... environment variables to provide
         2025/03/19 15:42:37 Terraform refresh | certain "global" settings to all configurations in your organization. To
         2025/03/19 15:42:37 Terraform refresh | reduce the verbosity of these warnings, use the -compact-warnings option.
         2025/03/19 15:42:37 Terraform refresh | 
         2025/03/19 15:42:37 Terraform refresh | Warning: Values for undeclared variables
         2025/03/19 15:42:37 Terraform refresh | 
         2025/03/19 15:42:37 Terraform refresh | In addition to the other similar warnings shown, 3 other variable(s) defined
         2025/03/19 15:42:37 Terraform refresh | without being declared.

ocofaigh · 2025-03-20T11:38:11Z

@alex-reiff do we have a misconfigured test? Are you able to locally run this? (make run-tests-local RUN=TestRunExistingResourcesInstances)

        	Error Trace:	/__w/terraform-ibm-secrets-manager/terraform-ibm-secrets-manager/tests/pr_test.go:213
        	Error:      	Received unexpected error:
        	            	error setting workspace tar file: tar upload has failed with status FAILED - [ -ds9rp1 (eu-de.workspace.-ds9rp1.378d4092) ]
        	Test:       	TestRunExistingResourcesInstances
        	Messages:   	Schematic Test had unexpected error

Meanwhile I try trigger pipeline again..

ocofaigh · 2025-03-20T11:38:15Z

/run pipeline

terraform-ibm-modules-ops · 2025-03-20T12:58:22Z

🎉 This PR is included in version 1.25.3 🎉

The release is available on:

GitHub release
v1.25.3

Your semantic-release bot 📦🚀

feat: expose new s2s iam

9d0f458

alex-reiff marked this pull request as ready for review March 6, 2025 04:47

alex-reiff requested a review from shemau as a code owner March 6, 2025 04:47

alex-reiff requested a review from ocofaigh March 6, 2025 05:18

ocofaigh requested changes Mar 6, 2025

View reviewed changes

alex-reiff and others added 2 commits March 12, 2025 13:19

Merge branch 'main' into add-s2s

cf1917d

update variable desc

dda6b39

test: keep iam engine disabled

b1534f1

alex-reiff and others added 2 commits March 19, 2025 11:04

Merge branch 'main' into add-s2s

0e42457

only expose in fscloud module

6718ed3

ocofaigh approved these changes Mar 19, 2025

View reviewed changes

Merge branch 'main' into add-s2s

ef2d486

ocofaigh merged commit 94db9b1 into main Mar 20, 2025
2 checks passed

ocofaigh deleted the add-s2s branch March 20, 2025 12:57

terraform-ibm-modules-ops added the released label Mar 20, 2025

feat: expose new s2s iam #301

feat: expose new s2s iam #301

Uh oh!

Conversation

alex-reiff commented Mar 6, 2025

Description

Release required?

Release notes content

Run the pipeline

Checklist for reviewers

For mergers

Uh oh!

alex-reiff commented Mar 6, 2025

Uh oh!

ocofaigh left a comment

Choose a reason for hiding this comment

Uh oh!

ocofaigh Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

ocofaigh Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

ocofaigh left a comment • 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!

ocofaigh commented Mar 12, 2025

Uh oh!

alex-reiff commented Mar 12, 2025

Uh oh!

alex-reiff commented Mar 12, 2025

Uh oh!

alex-reiff commented Mar 12, 2025

Uh oh!

ocofaigh commented Mar 15, 2025

Uh oh!

alex-reiff commented Mar 19, 2025

Uh oh!

alex-reiff commented Mar 19, 2025

Uh oh!

alex-reiff commented Mar 19, 2025

Uh oh!

ocofaigh commented Mar 19, 2025

Uh oh!

ocofaigh commented Mar 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ocofaigh commented Mar 20, 2025

Uh oh!

Uh oh!

terraform-ibm-modules-ops commented Mar 20, 2025

Uh oh!

Uh oh!

ocofaigh left a comment •

edited

Loading

ocofaigh commented Mar 20, 2025 •

edited

Loading