Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add profile attachments to DA stack #121

Closed
wants to merge 37 commits into from
Closed

feat: add profile attachments to DA stack #121

wants to merge 37 commits into from

Conversation

jor2
Copy link
Member

@jor2 jor2 commented Jun 4, 2024

Description

Configure SCC to create default attachment aligned with compliance claims for this stack

#13

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.

@jor2 jor2 self-assigned this Jun 4, 2024
@jor2 jor2 changed the title feat: add profile attachments to DA feat: add profile attachments to DA stack Jun 4, 2024
Copy link
Member

@vburckhardt vburckhardt left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I put some comments. Also we should scope the scan to the resource group created by the infra DA (first in the list). This will likely require an update on the SCC DA side as I do not see that option there.

https://github.com/terraform-ibm-modules/terraform-ibm-scc-da/blob/32ff8621bbd486d0f7ec367e38e1d23f7885f779/solutions/instances/main.tf#L157

@@ -284,6 +284,13 @@
"default_value": "__NULL__",
"description": "The CRN of an existing KMS instance to use in this solution. If not set, a new KP instance is provisioned.",
"required": false
},
{
Copy link
Member

Choose a reason for hiding this comment

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

I would not expose this input directly at the top level of the stack as it is unlikely to be common to change it.

@@ -212,6 +219,10 @@
"name": "existing_kms_instance_crn",
"value": "ref:../2a - Security Service - Key Management/outputs/kms_instance_crn"
},
{
"name": "profile_attachments",
"value": "ref:../../inputs/profile_attachments"
Copy link
Member

Choose a reason for hiding this comment

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

Set the default here - point to the AI ICT Guardrails (as opposed to fs cloud) as this is what we're claiming in the stack.

@jor2
Copy link
Member Author

jor2 commented Jun 13, 2024

/run pipeline

@jor2
Copy link
Member Author

jor2 commented Jun 13, 2024

/run pipeline

@jor2
Copy link
Member Author

jor2 commented Jun 13, 2024

/run pipeline

@ocofaigh
Copy link
Member

@vburckhardt As per the discussions here, we went with a basic list input variable and a predefined attachment configuration scoped to full account in order to keep the input variable simple to use via Projects.

As per that discussion, we also created an internal issue to expose a more advanced complex object type input, with supporting documentation on how to pass values to it from Projects UI, where users could basically customise exactly how they want there attachment to be set up.

Even if we had that advanced object type input variable, I don't think we would be able to reference the resource group outputs from the stack member DAs? I don't think it supports a kind of syntax like this?

{
          "name": "profile_attachments",
          "value": [{
                            profile_name    = "name"
                            profile_version = "1.0.0"
                            description     = "desc"
                            schedule        = "daily"
                            scope = [
                              {
                                environment = "ibm-cloud"
                                properties = [
                                  {
                                    name  = "scope_type"
                                    value = "account"
                                  },
                                  {
                                    name  = "scope_id"
                                    value = "ref:../1 - Account Infrastructure Base/outputs/audit_resource_group_name"
                                  },
                                  {
                                    name  = "scope_id"
                                    value = "ref:../1 - Account Infrastructure Base/outputs/observability_resource_group_name"
                                  }
                                ]
                              }
                            ]
                      }]
}

@jor2 jor2 requested a review from vburckhardt June 19, 2024 18:19
@jor2
Copy link
Member Author

jor2 commented Jun 19, 2024

/run pipeline

@jor2
Copy link
Member Author

jor2 commented Jun 24, 2024

/run pipeline

1 similar comment
@jor2
Copy link
Member Author

jor2 commented Jun 25, 2024

/run pipeline

@jor2
Copy link
Member Author

jor2 commented Jul 1, 2024

/run pipeline

@in-1911
Copy link
Contributor

in-1911 commented Jul 9, 2024

A couple of comments:

  • If you are changing the SCC module parameters anyway, could you please add the prefix parameter and pass the value from the stack? Otherwise SCC creates a bunch of resources with default names.
  • The current AI Security Guardrails profile name is actually AI Security Guardrails 2.0 (although the semver version on it is 1.0.0). The name "AI Security Guardrails" was used for v.1 which has some issues.
  • The problem with the AI Guardrails profile is that it requires much more setup (needs to have a target with credentials in SCC, trusted profile, etc) for it to work properly. So you can create an attachment, but the scans will come up with a bunch of "Unable to perform" warnings. So the user will have to do a lot more manual steps in order to make it work. It's not the DA problem per se, but we will create a bad impression.

ibm_catalog.json Outdated Show resolved Hide resolved
stack_definition.json Outdated Show resolved Hide resolved
@ocofaigh
Copy link
Member

@jor2 agree, lets do monthly by default in the stack

Jordan-Williams2 added 2 commits July 25, 2024 22:03
@jor2
Copy link
Member Author

jor2 commented Jul 25, 2024

/run pipeline

@jor2
Copy link
Member Author

jor2 commented Jul 25, 2024

/run pipeline

@jor2
Copy link
Member Author

jor2 commented Jul 25, 2024

@ocofaigh is this a know error?

    tests.go:272: [PROJECTS] Apply Error: {map[error_msg: failed to find an object with the 'metadata/guid' key = 'de5cc4e8-9116-44ed-84ea-c9ca77521a17' at //api.dataplatform.cloud.ibm.com/v2/projects resource_name:get_project resource_type:restapi_object]}
    tests.go:369: Error deploying configuration 4 - WatsonX SaaS services: deploy failed for configuration 4 - WatsonX SaaS services last state: deploying_failed
    tests.go:384: [PROJECTS] Apply Error: {map[error_msg: failed to find an object with the 'metadata/guid' key = 'de5cc4e8-9116-44ed-84ea-c9ca77521a17' at //api.dataplatform.cloud.ibm.com/v2/projects resource_name:get_project resource_type:restapi_object]}
    tests.go:414: [PROJECTS] Error deploying configurations, terminating deployment.

@in-1911
Copy link
Contributor

in-1911 commented Jul 25, 2024

I think we should set watsonx_project_name parameter to __NULL__ in the stack definition itself.

This should skip project creation and the bug that is related to that.

If any test code uses Watsonx SaaS DA, it should set that to __NULL__ as well. In the stack the project is created by the Sample App Config DA, and there I managed to go around that issue as well.

Now I am leaning towards the theory that this might be the timing issue with provisioning a new WML instance and immediately creating a new project in it, because in my stack deployments there is about 10 minutes gap between those two actions (because I suppress creating a project in Watson SaaS DA by the parameter above). And I did not get the same error even when trying to use some different combinations of credentials.

@jor2
Copy link
Member Author

jor2 commented Jul 29, 2024

/run pipeline

@jor2
Copy link
Member Author

jor2 commented Sep 2, 2024

/run pipeline

@jor2
Copy link
Member Author

jor2 commented Sep 2, 2024

/run pipeline

@jor2
Copy link
Member Author

jor2 commented Sep 2, 2024

two blockers:

  1. AI Guardrails profile fails creating attachment
  2. feat: merge dev tile into main #180

@jor2
Copy link
Member Author

jor2 commented Sep 5, 2024

/run pipeline

@ocofaigh
Copy link
Member

ocofaigh commented Sep 9, 2024

The current code now created an SCC attachment by default, however its scoped to the full account and the schedule is also hard coded. Since Jordan is off for a few weeks, I created #188 to track adding these updates, and going to close this PR.

@ocofaigh ocofaigh closed this Sep 9, 2024
@ocofaigh ocofaigh deleted the scc-feat branch September 9, 2024 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants