Skip to content

fix: Enable broker log delivery for Express brokers - #69

Open
somaz94 wants to merge 1 commit into
terraform-aws-modules:masterfrom
somaz94:fix/express-broker-logging
Open

somaz94 wants to merge 1 commit into
terraform-aws-modules:masterfrom
somaz94:fix/express-broker-logging

Conversation

@somaz94

@somaz94 somaz94 commented Jun 25, 2026

Copy link
Copy Markdown

Description

Express brokers now support broker log delivery (CloudWatch Logs, Firehose, S3), but the module still restricts the logging_info block to Standard brokers, so cloudwatch_logs_enabled / firehose_logs_enabled / s3_logs_enabled are silently ignored on Express clusters.

The Express guard was introduced in #48, back when Express brokers did not accept a logging_info block. AWS has since added broker log delivery for Express brokers (https://docs.aws.amazon.com/msk/latest/developerguide/msk-logging.html), so the guard is no longer needed. This reverts the dynamic "logging_info" wrapper back to the original static logging_info block, applying the same logging configuration to both broker types.

Closes #68

Validation

  • terraform validate -> Success! The configuration is valid.
  • pre-commit run --files main.tf -> terraform_fmt, terraform_docs, terraform_validate, terraform_validate with tflint all pass.

Breaking Changes

None for Standard brokers (the rendered logging_info block is unchanged). Express brokers gain the logging configuration that was previously ignored.

@somaz94
somaz94 marked this pull request as ready for review June 25, 2026 06:52
@somaz94

somaz94 commented Jul 2, 2026

Copy link
Copy Markdown
Author

Thanks for the approval, @vjancich! CI is clean and this simply restores broker log delivery now that AWS has GA'd it for MSK Express brokers. @bryantbiggs / @antonbabenko — would you be able to merge when you have a moment? Thanks!

@somaz94

somaz94 commented Jul 28, 2026

Copy link
Copy Markdown
Author

Gentle ping — this has been green and community-approved (thanks @vjancich) for about a month now. It is a small fix: broker log delivery was silently skipped for Express brokers.

Happy to rebase or split it further if that helps it land. No rush if the queue is deep.

@bryantbiggs

Copy link
Copy Markdown
Member

what provider version enabled this - because it wasn't always possible. if we don't raise the min supported provider version for where it was enabled, we break workflows. small fixes are rarely simple

@somaz94

somaz94 commented Jul 29, 2026

Copy link
Copy Markdown
Author

Fair question — I dug into it, and the short answer is that no provider version enabled this, because the provider never gated it. The block was the Kafka API itself, so there is no min-version to raise to. Details:

What actually blocked it. The error that motivated the guard in #48 is quoted in #47:

BadRequestException: Broker logs are not supported for clusters with Express instance types.

That is an AWS API response, not a Terraform schema or validation error. AWS lifted it on 2026-02-11: Amazon MSK now supports broker logs on Express Brokers ("available for both new and existing Express brokers… in all AWS regions where express brokers are available"). The developer guide now states it directly: "Broker logs are available on both MSK Standard and Express brokers" (msk-logging).

Why the provider isn't a factor. aws_msk_cluster has always been a pass-through here — logging_info is a plain optional block, instance_type is a bare required string with no ValidateFunc, and there is no express handling anywhere in internal/service/kafka outside the test file. The supporting history:

  • hashicorp/terraform-provider-aws#40357 — the issue express brokers not supported #47 pointed at — was closed by a maintainer as "the resource is working as I would expect… the original issue was the result of a configuration issue", with the dynamic "logging_info" workaround prescribed. That workaround is exactly what fix: The logging info block should not be present if node types are "express*" #48 implemented.
  • #46680 asked for "support broker logs for MSK Express clusters" after the AWS announcement, and was answered with a working express.m7g.large + full cloudwatch_logs/firehose/s3 config on the then-current provider — i.e. nothing to implement.
  • The resulting PR, #46702, touches only cluster_test.go and one docs line. It ships no changelog entry, and its acceptance test TestAccKafkaCluster_loggingInfoForExpress passed against pre-v6.35.0 code. Grepping the provider CHANGELOG for MSK Express logging returns nothing at any version.

And even on the most conservative reading, this module already requires aws >= 6.40, which is later than v6.35.0 (the release that merely carries that acceptance test). So the existing floor already covers it either way — there is nothing to bump.

One upgrade note that is worth flagging, since it is the closest thing to a real behavior change here: an existing Express-broker user who upgrades will start seeing logging_info in the plan where it was previously absent. With the module defaults (cloudwatch_logs_enabled, firehose_logs_enabled, s3_logs_enabled all false) it is a no-op logging config — no logs before, no logs after — but it will surface as an in-place update rather than an empty plan. If you would rather that not happen implicitly, I am happy to gate the block on var.cloudwatch_logs_enabled || var.firehose_logs_enabled || var.s3_logs_enabled so it only appears once a sink is actually turned on. Just say the word and I will push it.

@danielOfir1

Copy link
Copy Markdown
Contributor

Any updates on this? It would really help to have those logs now that AWS supports it.

@somaz94

somaz94 commented Aug 18, 2026

Copy link
Copy Markdown
Author

@bryantbiggs following up on the provider-version question. The short version of my July reply: no provider version gated this. The restriction was on the AWS API side (BadRequestException: Broker logs are not supported for clusters with Express instance types) and AWS lifted it on 2026-02-11, so there is no minimum version to raise to. aws_msk_cluster has always passed logging_info straight through.

If you would still prefer a floor on the provider version, say which one and I will add it.

CI is green and @vjancich approved in July. @danielOfir1 is running into the same gap.

@bryantbiggs

Copy link
Copy Markdown
Member

There has to be a provider version bump for when this was enabled - the Go AWS SDK v2 would have to have changed to allow this and the AWS provider consumes this and is released at some version

@somaz94

somaz94 commented Aug 19, 2026

Copy link
Copy Markdown
Author

I went and checked the SDK, because that premise is testable. It turns out no SDK change was needed, and none happened — the wire model could always express this.

1. InstanceType is a free-form string, not an enum.

In service/kafka/types/types.go, BrokerNodeGroupInfo:

// The type of Amazon EC2 instances to use for Apache Kafka brokers...
//
// This member is required.
InstanceType *string

It's a *string. Adding express.m7g.large as a valid value required zero SDK change — there is no enum to extend, no validation to relax. (For contrast, the same file does use real enum types elsewhere: BrokerAZDistribution, EnhancedMonitoring, ClientBroker.)

2. LoggingInfo is unconditional on CreateClusterInput.

In api_op_CreateCluster.go:

LoggingInfo *types.LoggingInfo

A plain optional field. There is no relationship in the model between it and InstanceType — nothing to gate, nothing that had to be un-gated.

So the SDK could always serialize "Express instance type + logging_info". The rejection was server-side validation (BadRequestException: Broker logs are not supported for clusters with Express instance types), which AWS lifted on 2026-02-11. Grepping the whole service/kafka types for express returns only doc-comment prose (intelligent rebalancing) — no type, field, or enum.

3. The provider resource has no Express handling either.

internal/service/kafka/cluster.go in terraform-provider-aws contains zero occurrences of express. The only matches in the whole kafka service are in cluster_test.go — TestAccKafkaCluster_loggingInfoForExpress, gated behind SkipIfEnvVarNotSet(t, "MSK_EXPRESS_BROKER_ENABLED"). That's an acceptance test, not a feature. #46702, which added it, ships no changelog entry — because it implemented nothing.

So there is no provider release to raise the floor to, because no provider release changed behaviour here. The module already requires aws >= 6.40, which is past v6.35.0 (the release carrying that test) regardless.


That said, I take the underlying concern seriously: you don't want an upgrade to silently break someone. The real upgrade risk here isn't a provider version, it's the plan diff — an existing Express user will start seeing a logging_info block where there was none. With the module defaults all three sinks are false, so it's a no-op logging config, but it shows up as an in-place update rather than an empty plan.

If that bothers you, I'm happy to push the guard I offered in July:

dynamic "logging_info" {
  for_each = var.cloudwatch_logs_enabled || var.firehose_logs_enabled || var.s3_logs_enabled ? [true] : []
  ...
}

That keeps the block absent until a sink is actually enabled, so no existing user sees a diff at all — and it drops the instance-type coupling that caused #47 in the first place. Say the word and I'll push it.

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.

Logging not available for Express brokers

4 participants