Create s2s authorisation DA

[dishankkalra23]

Description

Create s2s authorisation with existing options provided in module and add a flag to enable/disable the creation of CBR

New or affected modules

https://github.com/terraform-ibm-modules/terraform-ibm-s2s-auth

---

Tracking Issue

https://github.ibm.com/dreadnought/kanban/issues/3722

By submitting this issue, you agree to follow our Code of Conduct

[dishankkalra23]

Draft PR raised: #282

[dishankkalra23]

1. Updated the usage of count to for_each for creating multiple authorization polices in line-6 as it is causing re-creation of auth policy and deletion. For more info, check the output where two auth policies were created and later deleting the first auth policy at index 0 deleted the second auth policy and re-created it.

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
  + create

Terraform will perform the following actions:

  # module.service_auth_cbr_rules.ibm_iam_authorization_policy.auth_policies[0] will be created
  + resource "ibm_iam_authorization_policy" "auth_policies" {
      + description                 = "Auth policy-1"
      + id                          = (known after apply)
      + roles                       = [
          + "Operator",
          + "Service ID creator",
        ]
      + source_resource_group_id    = (known after apply)
      + source_resource_instance_id = "be19bb4e1d12482c9c7a7e483ea90c7d"
      + source_resource_type        = (known after apply)
      + source_service_account      = (known after apply)
      + source_service_name         = "secrets-manager"
      + target_resource_group_id    = "be19bb4e1d12482c9c7a7e483ea90c7d"
      + target_resource_instance_id = (known after apply)
      + target_resource_type        = (known after apply)
      + target_service_name         = "iam-identity"
      + transaction_id              = (known after apply)
      + version                     = (known after apply)

      + resource_attributes (known after apply)

      + subject_attributes (known after apply)
    }

  # module.service_auth_cbr_rules.ibm_iam_authorization_policy.auth_policies[1] will be created
  + resource "ibm_iam_authorization_policy" "auth_policies" {
      + description                 = "Auth policy-2"
      + id                          = (known after apply)
      + roles                       = [
          + "Groups Service Member Manage",
        ]
      + source_resource_group_id    = (known after apply)
      + source_resource_instance_id = "be19bb4e1d12482c9c7a7e483ea90c7d"
      + source_resource_type        = (known after apply)
      + source_service_account      = (known after apply)
      + source_service_name         = "secrets-manager"
      + target_resource_group_id    = "be19bb4e1d12482c9c7a7e483ea90c7d"
      + target_resource_instance_id = (known after apply)
      + target_resource_type        = (known after apply)
      + target_service_name         = "iam-groups"
      + transaction_id              = (known after apply)
      + version                     = (known after apply)

      + resource_attributes (known after apply)

      + subject_attributes (known after apply)
    }

Later

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
  - destroy
-/+ destroy and then create replacement

Terraform will perform the following actions:

  # module.service_auth_cbr_rules.ibm_iam_authorization_policy.auth_policies[0] must be replaced
-/+ resource "ibm_iam_authorization_policy" "auth_policies" {
      ~ description                 = "Auth policy-1" -> "Auth policy-2"
      ~ id                          = "eae676a9-a2a1-46a3-9522-d04b3b520010" -> (known after apply)
      ~ roles                       = [
          - "Operator",
          - "Service ID creator",
          + "Groups Service Member Manage",
        ]
      + source_resource_group_id    = (known after apply)
      + source_resource_type        = (known after apply)
      ~ source_service_account      = "d5ab7fb21c384aec8f2daa12e32542d2" -> (known after apply)
      + target_resource_instance_id = (known after apply)
      + target_resource_type        = (known after apply)
      ~ target_service_name         = "iam-identity" -> "iam-groups" # forces replacement
      ~ transaction_id              = "b2cdc81a3ff74033b055e415d2d2c435" -> (known after apply)
      + version                     = (known after apply)
        # (3 unchanged attributes hidden)

      ~ resource_attributes (known after apply)
      - resource_attributes {
          - name     = "accountId" -> null
          - operator = "stringEquals" -> null
          - value    = "d5ab7fb21c384aec8f2daa12e32542d2" -> null
        }
      - resource_attributes {
          - name     = "resourceGroupId" -> null
          - operator = "stringEquals" -> null
          - value    = "be19bb4e1d12482c9c7a7e483ea90c7d" -> null
        }
      - resource_attributes {
          - name     = "serviceName" -> null
          - operator = "stringEquals" -> null
          - value    = "iam-identity" -> null
        }

      ~ subject_attributes (known after apply)
    }

  # module.service_auth_cbr_rules.ibm_iam_authorization_policy.auth_policies[1] will be destroyed
  # (because index [1] is out of range for count)
  - resource "ibm_iam_authorization_policy" "auth_policies" {
      - description                 = "Auth policy-2" -> null
      - id                          = "5420623d-51c5-49d0-91b3-518824eb73f1" -> null
      - roles                       = [
          - "Groups Service Member Manage",
        ] -> null
      - source_resource_instance_id = "be19bb4e1d12482c9c7a7e483ea90c7d" -> null
      - source_service_account      = "d5ab7fb21c384aec8f2daa12e32542d2" -> null
      - source_service_name         = "secrets-manager" -> null
      - target_resource_group_id    = "be19bb4e1d12482c9c7a7e483ea90c7d" -> null
      - target_service_name         = "iam-groups" -> null
      - transaction_id              = "9e7356868c6d4e14a130f3ef293bcabb" -> null
        # (4 unchanged attributes hidden)

      - resource_attributes {
          - name     = "accountId" -> null
          - operator = "stringEquals" -> null
          - value    = "d5ab7fb21c384aec8f2daa12e32542d2" -> null
        }
      - resource_attributes {
          - name     = "resourceGroupId" -> null
          - operator = "stringEquals" -> null
          - value    = "be19bb4e1d12482c9c7a7e483ea90c7d" -> null
        }
      - resource_attributes {
          - name     = "serviceName" -> null
          - operator = "stringEquals" -> null
          - value    = "iam-groups" -> null
        }
    }

[dishankkalra23]

2. Added a flag to enable or disable creation of CBRs: enable_cbr

3. Added support to create cross account service to service authorization by addition of an attribute: source_service_account_id in service_map variable

[dishankkalra23]

Following points will be discussed in today's deep dive:

1. Regarding how CBRs should be created and enforced: Currently, there is a bug in the module when default values are set for CBR input variables, the module fails. Need to check how CBRs should be enforced along with s2s auth policies. (Link)
2. The use of count in the current module implementation is problematic and could result in unexpected changes. To resolve this, we should switch to for_each. This would require updating the input variable service_map to the type map(objects). (Link)

Slack thread: https://ibm-cloudplatform.slack.com/archives/C042TNQU4MN/p1760430857108039?thread_ts=1753775067.229439&cid=C042TNQU4MN

[toddgiguere]

After deep dive we are suggesting the following path forward, which will probably result in a major version (breaking change):

1. We do need a boolean for CBR rules, the module is expecting CBR to be supplied. The boolean you created at the root level is correctly implemented, but its default should be true instead of false, so users can "opt-out" if needed. I would also add a nullable = false to make sure it is always defaulting to true as well. You do not need to expose at the DA level if you don't want, you could hard-code inside DA as false?
2. We agree that the current service_map list(object) is problematic and should be changed in the root module. Suggest to change it to object(object(.... current properties....)) at the top level and let the user supply unique names for the entries. Then you can use a for_each instead of count for the s2s block and order will not matter. This will probably be a breaking change, we would like to take a look at it ourselves to decide how we release.
3. We noticed something else that needs to be changed: the validation for source and target service names in the variables.tf are not current. Even now there are some missing services in that list. We suggest removing that validation block completely and let the provider error on invalid service name, as keeping this list up-to-date will be very difficult

[dishankkalra23]

After deep dive we are suggesting the following path forward, which will probably result in a major version (breaking change):

1. We do need a boolean for CBR rules, the module is expecting CBR to be supplied. The boolean you created at the root level is correctly implemented, but its default should be true instead of false, so users can "opt-out" if needed. I would also add a nullable = false to make sure it is always defaulting to true as well. You do not need to expose at the DA level if you don't want, you could hard-code inside DA as false?

2. We agree that the current service_map list(object) is problematic and should be changed in the root module. Suggest to change it to object(object(.... current properties....)) at the top level and let the user supply unique names for the entries. Then you can use a for_each instead of count for the s2s block and order will not matter. This will probably be a breaking change, we would like to take a look at it ourselves to decide how we release.

3. We noticed something else that needs to be changed: the [validation](https://github.com/terraform-ibm-modules/terraform-ibm-s2s-auth/blob/main/variables.tf#L25) for source and target service names in the variables.tf are not current. Even now there are some missing services in that list. We suggest removing that validation block completely and let the provider error on invalid service name, as keeping this list up-to-date will be very difficult

Above three points are taken care.

[dishankkalra23]

1. Minor issues are fixed - renaming module, updating output after removal of CBR enforcement in DA
2. Update variable service_map schema in local of both the existing examples - basic and complete
3. Added test for DA and verified the test is passing in schematics.