-
Notifications
You must be signed in to change notification settings - Fork 2
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 support to set ICL as targets in logs routing #559
Conversation
/run pipeline |
/run pipeline |
/run pipeline |
/run pipeline |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments
modules/cloud_logs/main.tf
Outdated
log_sink_crn = ibm_resource_instance.cloud_logs.crn | ||
name = local.instance_name | ||
parameters { | ||
host = "${ibm_resource_instance.cloud_logs.guid}.ingress.${var.region}.logs.cloud.ibm.com" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this not a public host? We probably want to use private by default, but I guess we should also optionally allow user to public incase its not a VRF enabled account.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this, I used service_endpoints as reference, if Cloud logs is setup with public endpoint then use public endpoint or else private.
Let me know if any concern.
/run pipeline |
1 similar comment
/run pipeline |
/run pipeline |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few changes we need to make here. Im going to push a commit wih some updates. But bare in mind, the current code is only creating 1 tenant in whatever region the provider is set to. And in order to get platform logs from services in all regions, you need to create a tenant in every region.
It seems this is not yet supported in the provider, but I see its coming in IBM-Cloud/terraform-provider-ibm#5634
I am pushing to see if we can get this merged and a hotfix generated, but in parallel I'll proceed with updating the code in this PR directly which would only support 1 tenant in 1 region.
/run pipeline |
/run pipeline |
/run pipeline |
/run pipeline |
/run pipeline |
The provider team might be creating a patch release for us when IBM-Cloud/terraform-provider-ibm#5634 is merged. If this is the case, I would like to wait for that instead of merging the code with the limitation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we discovered, only public ingress endpoints are supported when configuring a target to an IBM Cloud Logs instance, so we need to remove the use_private_endpoint_logs_routing
variable and always use public for now
modules/cloud_logs/main.tf
Outdated
@@ -165,15 +163,14 @@ resource "ibm_logs_router_tenant" "logs_router_tenant_instances" { | |||
# until provider supports passing region to this resource (coming in https://github.com/IBM-Cloud/terraform-provider-ibm/pull/5634), | |||
# the for_each will only ever include the provider region | |||
|
|||
# for_each = contains(var.logs_routing_tenant_regions, "*") ? toset(data.ibm_is_regions.regions.regions[*].name) : var.logs_routing_tenant_regions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh @Aashiq-J I left that that code comment here on purpose, so we know to swithc to it when the provider support comes. You might want to add it back
New provider version is coming today with the fix we need. I can update this PR once its out |
/run pipeline |
/run pipeline |
/run pipeline |
/run pipeline |
/run pipeline |
/run pipeline |
🎉 This PR is included in version 2.18.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Description
Release required?
x.x.X
)x.X.x
)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:
Checklist for reviewers
For mergers