Skip to content

Conversation

@iamar7
Copy link
Member

@iamar7 iamar7 commented Sep 25, 2024

Description

Resolves: #115

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.

@iamar7
Copy link
Member Author

iamar7 commented Sep 25, 2024

/run pipeline

@iamar7 iamar7 self-assigned this Sep 25, 2024
@iamar7
Copy link
Member Author

iamar7 commented Sep 25, 2024

/run pipeline

@iamar7
Copy link
Member Author

iamar7 commented Sep 25, 2024

Failed due to hitting the maximum no of target allowed for the account -

Screenshot 2024-09-25 at 9 24 16 PM

@iamar7 iamar7 marked this pull request as ready for review September 25, 2024 16:20
Copy link
Contributor

@ocofaigh ocofaigh left a comment

Choose a reason for hiding this comment

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

  • You need to rename enable_platform_logs_metrics to enable_platform_metrics since we will no longer provision Log Analysis. To enable platform logs with cloud logs, users will not use the new logs_routing_tenant_regions input.
  • There is now a circular dependency you need to resolve - removed existing_monitoring_crn from Event Notifications input (line 239). The dependency is now the other way around - Observability depends on EN.

@iamar7
Copy link
Member Author

iamar7 commented Sep 27, 2024

/run pipeline

@iamar7
Copy link
Member Author

iamar7 commented Sep 27, 2024

Getting this in the pipeline.

Error: ServiceNotAuthorized: The specified COS Service Instance does not have sufficient permissions to access the resource associated with the KMS key CRN.
2024/09/27 11:26:44 Terraform apply | status code: 401, request id: 2741b9da-xxxd-4xx2-bxxa-b5xxxxxXXxxxxdbdf, host id:
2024/09/27 11:26:44 Terraform apply |
2024/09/27 11:26:44 Terraform apply | with module.cos[0].ibm_cos_bucket.cos_bucket[0],
2024/09/27 11:26:44 Terraform apply | on .terraform/modules/cos/main.tf line 133, in resource "ibm_cos_bucket" "cos_bucket":
2024/09/27 11:26:44 Terraform apply | 133: resource "ibm_cos_bucket" "cos_bucket" {

@ocofaigh
Copy link
Contributor

@iamar7 I'm actually in the process of adding support to the Observability DA to support an enable_platform_logs variable for cloud logs, so when that is in, we can use that instead of exposing the logs_routing_tenant_regions input. Thats the approach we are going with for RAG stack. Ill let you know when its ready

@iamar7
Copy link
Member Author

iamar7 commented Sep 30, 2024

/run pipeline

@ocofaigh
Copy link
Contributor

@iamar7 you need to wait for terraform-ibm-modules/terraform-ibm-observability-da#171 to be merged and a new Observability DA version released so you can use the enable_platform_metrics feature with Cloud Logs. It means you change the variable name back, but update the description of it

@iamar7
Copy link
Member Author

iamar7 commented Oct 1, 2024

/run pipeline

Copy link
Contributor

@ocofaigh ocofaigh left a comment

Choose a reason for hiding this comment

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

You can keep enable_platform_logs_metrics instead of splitting into 2 separate ones

@iamar7
Copy link
Member Author

iamar7 commented Oct 1, 2024

/run pipeline

@iamar7
Copy link
Member Author

iamar7 commented Oct 2, 2024

/run pipeline

@iamar7
Copy link
Member Author

iamar7 commented Oct 3, 2024

Getting this in the pipeline run -

Screenshot 2024-10-03 at 6 16 42 AM

@ocofaigh
Copy link
Contributor

ocofaigh commented Oct 3, 2024

@iamar7 Can we update all of the version locators to match what we have in the RAG stack: https://github.com/terraform-ibm-modules/stack-retrieval-augmented-generation/blob/main/solutions/basic/stack_definition.json

At least we know that exact combination works. Keep on eye on some of the member inputs and refs, as they may need updates too.

@iamar7
Copy link
Member Author

iamar7 commented Oct 3, 2024

/run pipeline

@ocofaigh
Copy link
Contributor

ocofaigh commented Oct 4, 2024

/run pipeline

@iamar7
Copy link
Member Author

iamar7 commented Oct 4, 2024

@ocofaigh The pipeline is failing is due to the changes in order of observability and event notification. Earlier, the cos_kms_auth_policy was being created in the observability-da and skipped in the event-notification DA. So now since the event notification DA is being deployed first so need to create the cos_kms_auth_policy in the event-notification DA and skip it in the observability-da.

@iamar7
Copy link
Member Author

iamar7 commented Oct 4, 2024

/run pipeline

@ocofaigh
Copy link
Contributor

ocofaigh commented Oct 4, 2024

I think we will want to do a manual upgrade test against this branch before we proceed - but lets get pipeline passing here fist and ill get prepped for a manual upgrade

@iamar7
Copy link
Member Author

iamar7 commented Oct 4, 2024

TestProjectsFullTest passed in the above run. The TestProjectsExistingResourcesTest while undeploying failed with timeout error. So re-running the pipeline.

Screenshot 2024-10-04 at 8 54 15 PM

@ocofaigh
Copy link
Contributor

ocofaigh commented Oct 7, 2024

As discussed internally, there is a breaking change here due to the dependency tree order changing and the requirement to change which DA is creating the COS to KMS auth policy. We are discussing options currently.
image

Copy link
Contributor

@ocofaigh ocofaigh left a comment

Choose a reason for hiding this comment

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

@iamar7 While we are making a decision on how to handle the breaking change, can you please update the diagram to replace Log Analysis with Cloud Logs, the cloud logs bucket, and the AT route to cloud logs

@iamar7
Copy link
Member Author

iamar7 commented Oct 7, 2024

@iamar7 While we are making a decision on how to handle the breaking change, can you please update the diagram to replace Log Analysis with Cloud Logs, the cloud logs bucket, and the AT route to cloud logs

Sure @ocofaigh

@ocofaigh
Copy link
Contributor

ocofaigh commented Oct 8, 2024

There is another issue we are seeing now when upgrading Observability to remove logdna:

 2024/10/08 07:47:24 Terraform plan | Error: Cannot read the remote archive resource
 2024/10/08 07:47:24 Terraform plan | 
 2024/10/08 07:47:24 Terraform plan |   with module.observability_instance.module.log_analysis[0].logdna_archive.archive_config[0],
 2024/10/08 07:47:24 Terraform plan |   on .terraform/modules/observability_instance/modules/log_analysis/main.tf line 42, in resource "logdna_archive" "archive_config":
 2024/10/08 07:47:24 Terraform plan |   42: resource "logdna_archive" "archive_config" {
 2024/10/08 07:47:24 Terraform plan | 
 2024/10/08 07:47:24 Terraform plan | expected either servicekey or iamtoken to be set

My guess is since we have identified the logdna resources (including the resource key used to initialise the logdna provider) for deletion, the logdna provider can no longer be initialised, and we get this error.
We need to find a way to handle this.

@ocofaigh
Copy link
Contributor

Going to have to do the upgrade in a 2 step process to fully removed Log Analysis. In this PR we will disable the log archiving, but keep the LogDNA instance up, as well as deploy Cloud Logs. We will deploy Cloud Logs without Event Notifications integration for now, and in the next release, we will add it, since it will require manual steps to upgrade, so hence will make the next change a major version update.

@ocofaigh
Copy link
Contributor

/run pipeline

@ocofaigh
Copy link
Contributor

Some issue with key protect service:

 2024/10/11 10:05:51 Terraform apply | ---
 2024/10/11 10:05:51 Terraform apply | id: terraform-46882f4c
Error: 0/11 10:05:51 Terraform apply | summary: '[ERROR] Error while creating key ring : kp.Error:
 2024/10/11 10:05:51 Terraform apply | correlation_id=''7c640068-1d71-4343-859f-99bbe462ece7'',
 2024/10/11 10:05:51 Terraform apply |   msg=''This operation cannot be completed. Please try your request at a later time.
 2024/10/11 10:05:51 Terraform apply |   If the issue persists, please contact IBM KeyProtect.'''
 2024/10/11 10:05:51 Terraform apply | severity: error
 2024/10/11 10:05:51 Terraform apply | resource: ibm_kms_key_rings
 2024/10/11 10:05:51 Terraform apply | operation: create
 2024/10/11 10:05:51 Terraform apply | component:
 2024/10/11 10:05:51 Terraform apply |   name: github.com/IBM-Cloud/terraform-provider-ibm
 2024/10/11 10:05:51 Terraform apply |   version: 1.69.2
 2024/10/11 10:05:51 Terraform apply | ---

Retrying..

@ocofaigh
Copy link
Contributor

/run pipeline

@ocofaigh ocofaigh merged commit 79d8e7e into main Oct 11, 2024
2 checks passed
@ocofaigh ocofaigh deleted the 10536-icl branch October 11, 2024 12:31
@terraform-ibm-modules-ops
Copy link
Contributor

🎉 This PR is included in version 1.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@terraform-ibm-modules-ops
Copy link
Contributor

🎉 This PR is included in version 1.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

1 similar comment
@terraform-ibm-modules-ops
Copy link
Contributor

🎉 This PR is included in version 1.5.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.

[stack-ibm-core-security-services] Add support for cloud logs

4 participants