`azurerm_servicebus_topic` - add premium namespace partition validation #26680

xiaxyi · 2024-07-18T09:10:29Z

Community Note

Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
Please do not leave comments along the lines of "+1", "me too" or "any updates", they generate extra noise for PR followers and do not help prioritize for review

Description

For premium service bus namespace, the setting of the partitioning feature for its entity such as servicebus namespace queue/ topic is only available during the namespace creation. Given the fact that the property partitioning_enabled is forcenew, there is a chance of unexpected deletion if user ignores the terraform plan message. Add a validation to warn the user.

PR Checklist

I have followed the guidelines in our Contributing Documentation.
I have checked to ensure there aren't other open Pull Requests for the same update/change.
I have checked if my changes close any open issues. If so please include appropriate closing keywords below.
I have updated/added Documentation as required written in a helpful and kind way to assist users that may be unfamiliar with the resource / data source.
I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.
For example: “resource_name_here - description of change e.g. adding property new_property_name_here”

Changes to existing Resource / Data Source

I have added an explanation of what my changes do and why I'd like you to include them (This may be covered by linking to an issue above, but may benefit from additional explanation).
I have written new tests for my resource or datasource changes & updated any relevent documentation.
I have successfully run tests with my changes locally. If not, please provide details on testing challenges that prevented you running the tests.
(For changes that include a state migration only). I have manually tested the migration path between relevant versions of the provider.

Testing

My submission includes Test coverage as described in the Contribution Guide and the tests pass. (if this is not possible for any reason, please include details of why you did or could not add test coverage)

--- PASS: TestAccServiceBusTopic_nonPartitionedPremiumNamespaceError (280.30s)
PASS
ok github.com/hashicorp/terraform-provider-azurerm/internal/services/servicebus (cached)

--- PASS: TestAccServiceBusTopic_partitionedPremiumNamespace (338.72s)
PASS
ok github.com/hashicorp/terraform-provider-azurerm/internal/services/servicebus 355.206s

Change Log

Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.

azurerm_resource - support for the thing1 property [GH-00000]

This is a (please select all that apply):

Bug Fix
New Feature (ie adding a service, resource, or data source)
Enhancement
Breaking Change

Related Issue(s)

Fixes #26642

Note

If this PR changes meaningfully during the course of review please update the title and description as required.

stephybun · 2024-07-31T09:01:50Z

internal/services/servicebus/servicebus_topic_resource.go

+	isPremiumNamespacePartitioned := true
+	var sku namespaces.SkuName
+	if nsModel := resp.Model; nsModel != nil {
+		sku = nsModel.Sku.Name
+	}
+
+	if sbNamespaceModel := resp.Model; sbNamespaceModel != nil {
+		if sbNamespaceModel.Properties != nil &&
+			sbNamespaceModel.Properties.PremiumMessagingPartitions != nil && *sbNamespaceModel.Properties.PremiumMessagingPartitions == 1 {
+			isPremiumNamespacePartitioned = false
+		}
+	}


A few things here:

We can condense this so we only nil check the model once

Sku is a pointer so this should be nil checked to prevent a potential panic

Premium messaging partitions can be 0, 1, 2 or 4, I'm assuming any value other than 0 means that the namespace is partitioned so the condition should be updated to check whether premium messaging partitions is greater than 0 and set the bool to true if it is.

Suggested change

isPremiumNamespacePartitioned := true

var sku namespaces.SkuName

if nsModel := resp.Model; nsModel != nil {

sku = nsModel.Sku.Name

}

if sbNamespaceModel := resp.Model; sbNamespaceModel != nil {

if sbNamespaceModel.Properties != nil &&

sbNamespaceModel.Properties.PremiumMessagingPartitions != nil && *sbNamespaceModel.Properties.PremiumMessagingPartitions == 1 {

isPremiumNamespacePartitioned = false

}

}

isPremiumNamespacePartitioned := false

var sku namespaces.SkuName

if model := resp.Model; model != nil {

if model.Sku != nil {

sku = model.Sku.Name

}

if props := model.Properties; props != nil && props.PremiumMessagingPartitions != nil && props.PremiumMessagingPartitions > 0 {

isPremiumNamespacePartitioned = true

}

}

Thanks @stephybun for the comment. The message partition cannot be set to 0 for servicebus premium namespace. premium_messaging_partitions = 1 means the servicebus namespace is not partitioned, while value greater than 1 means the servicebus namespace is partitioned.

If the parent namespace is partitioned, the child entities such as queue and topic can't be set to the non-partitioned.

The provider accepts 0 as a value for partitioning. If the API doesn't allow this then we should also update the premium_messaging_partitions property in the azurerm_servicebus_namespace resource.

Furthermore, please still make the following changes since there are redundant nil checks and variable assignments in the original.

Suggested change

isPremiumNamespacePartitioned := true

var sku namespaces.SkuName

if nsModel := resp.Model; nsModel != nil {

sku = nsModel.Sku.Name

}

if sbNamespaceModel := resp.Model; sbNamespaceModel != nil {

if sbNamespaceModel.Properties != nil &&

sbNamespaceModel.Properties.PremiumMessagingPartitions != nil && *sbNamespaceModel.Properties.PremiumMessagingPartitions == 1 {

isPremiumNamespacePartitioned = false

}

}

isPremiumNamespacePartitioned := true

var sku namespaces.SkuName

if model := resp.Model; model != nil {

if model.Sku != nil {

sku = model.Sku.Name

}

if props := model.Properties; props != nil && props.PremiumMessagingPartitions != nil && props.PremiumMessagingPartitions == 1 {

isPremiumNamespacePartitioned = false

}

}

We can't set the default value to 1 as the premiumMessagingPartitions also applies to standard namespace which has 0 as the default value.

Updated the nil check

stephybun · 2024-07-31T09:03:11Z

internal/services/servicebus/servicebus_topic_resource.go

+
+	if sku == namespaces.SkuNamePremium {
+		if isPremiumNamespacePartitioned && !enablePartitioning {
+			return fmt.Errorf("non-partitioned entities are not allowed in partitioned namespace")


It would be helpful to rephrase the error message into something actionable for the user

Suggested change

return fmt.Errorf("non-partitioned entities are not allowed in partitioned namespace")

return fmt.Errorf("topic must have `partitioning_enabled` set to `true` when the parent namespace is partitioned")

This hasn't been updated and there is no response as to why?

internal/services/servicebus/servicebus_topic_resource.go

xiaxyi · 2024-08-08T08:28:05Z

Thanks @stephybun for the review, I left my comments in the thread.

stephybun

@xiaxyi please see responses

…idation

xiaxyi · 2024-08-27T09:10:35Z

Sorry for the late response as I was out of office for the past two weeks. Please see my response and let me know if there is anything needed.

stephybun

@xiaxyi once the last round of comments have been resolved this should be good to go. Thanks!

stephybun · 2025-01-28T13:54:42Z

internal/services/servicebus/servicebus_topic_resource.go

+		if isPremiumNamespacePartitioned && !enablePartitioning {
+			return fmt.Errorf("topic must have `partitioning_enabled` set to `true` when the parent namespace is partitioned")
+		} else if !isPremiumNamespacePartitioned && enablePartitioning {
+			return fmt.Errorf("the parent premium namespace is not partitioned and the partitioning for premium namespace is only available at the namepsace creation")


Can we please re-write this

Suggested change

return fmt.Errorf("the parent premium namespace is not partitioned and the partitioning for premium namespace is only available at the namepsace creation")

return fmt.Errorf("topic partitioning is only available if the parent namespace is partitioned")

stephybun · 2025-01-28T13:55:53Z

internal/services/servicebus/servicebus_topic_resource_test.go

+	data.ResourceTest(t, r, []acceptance.TestStep{
+		{
+			Config:      r.nonPartitionedPremiumNamespaceError(data),
+			ExpectError: regexp.MustCompile("the parent premium namespace is not partitioned and the partitioning for premium namespace is only available at the namepsace creation"),


This error message needs to be updated

stephybun · 2025-01-28T13:56:08Z

internal/services/servicebus/servicebus_topic_resource_test.go

+	data.ResourceTest(t, r, []acceptance.TestStep{
+		{
+			Config:      r.PremiumNamespacePartitioned(data, false),
+			ExpectError: regexp.MustCompile("non-partitioned entities are not allowed in partitioned namespace"),


This error message doesn't match what's been written in the resource

stephybun · 2025-01-28T13:58:57Z

website/docs/r/servicebus_topic.html.markdown

@@ -61,7 +61,7 @@ The following arguments are supported:

 * `partitioning_enabled` - (Optional) Boolean flag which controls whether to enable the topic to be partitioned across multiple message brokers. Changing this forces a new resource to be created.

-> **NOTE:** Partitioning is available at entity creation for all queues and topics in Basic or Standard SKUs. It is not available for the Premium messaging SKU, but any previously existing partitioned entities in Premium namespaces continue to work as expected. Please [see the documentation](https://docs.microsoft.com/azure/service-bus-messaging/service-bus-partitioning) for more information.
+-> **NOTE:** Partitioning is available at entity creation for all queues and topics in Basic or Standard SKUs. It is not available for the Premium messaging SKU, but any previously existing partitioned entities in Premium namespaces continue to work as expected. For premium namespace, partitioning is available at namespace creation, and all queues and topics in the partitioned namespace will be partitioned, for the premium namespace that has `premium_messaging_partitions` sets to `1`, the namespace is not partitioned. Please [see the documentation](https://docs.microsoft.com/azure/service-bus-messaging/service-bus-partitioning) for more information.


Suggested change

-> **NOTE:** Partitioning is available at entity creation for all queues and topics in Basic or Standard SKUs. It is not available for the Premium messaging SKU, but any previously existing partitioned entities in Premium namespaces continue to work as expected. For premium namespace, partitioning is available at namespace creation, and all queues and topics in the partitioned namespace will be partitioned, for the premium namespace that has `premium_messaging_partitions` sets to `1`, the namespace is not partitioned. Please [see the documentation](https://docs.microsoft.com/azure/service-bus-messaging/service-bus-partitioning) for more information.

-> **NOTE:** Partitioning is available at entity creation for all queues and topics in Basic or Standard SKUs. It is not available for the Premium messaging SKU, but any previously existing partitioned entities in Premium namespaces continue to work as expected. For premium namespaces, partitioning is available at namespace creation and all queues and topics in the partitioned namespace will be partitioned. Premium namespaces that have `premium_messaging_partitions` set to `1` are not partitioned. Please [see the documentation](https://docs.microsoft.com/azure/service-bus-messaging/service-bus-partitioning) for more information.

add premium namespace partition validation

1ea2aa6

github-actions bot added documentation service/service-bus size/M labels Jul 18, 2024

xiaxyi added 2 commits July 18, 2024 17:42

update test case to fit the 4.0 flag in teamCity

a8aa1e4

update test case - remove import step from the error validation test

34b9699

xiaxyi marked this pull request as draft July 18, 2024 10:01

xiaxyi marked this pull request as ready for review July 19, 2024 02:43

simplify the test checks

f1fd24c

stephybun requested changes Jul 31, 2024

View reviewed changes

github-actions bot added the waiting-response label Jul 31, 2024

github-actions bot added waiting-response and removed waiting-response labels Aug 8, 2024

stephybun requested changes Aug 8, 2024

View reviewed changes

xiaxyi added 2 commits August 27, 2024 16:24

Merge remote-tracking branch 'upstream/main' into servicebus/topicVal…

7f2390e

…idation

update code per reviewer's comment

849feb8

github-actions bot removed the waiting-response label Aug 27, 2024

rcskosir added the enhancement label Oct 14, 2024

katbyte requested review from katbyte and a team as code owners November 14, 2024 00:08

stephybun requested changes Jan 28, 2025

View reviewed changes

github-actions bot assigned stephybun Jan 28, 2025

github-actions bot added the waiting-response label Jan 28, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`azurerm_servicebus_topic` - add premium namespace partition validation #26680

`azurerm_servicebus_topic` - add premium namespace partition validation #26680

xiaxyi commented Jul 18, 2024 •

edited

Loading

stephybun Jul 31, 2024

xiaxyi Aug 8, 2024

stephybun Aug 8, 2024

xiaxyi Aug 27, 2024

stephybun Jul 31, 2024

stephybun Aug 8, 2024

xiaxyi Aug 27, 2024

xiaxyi commented Aug 8, 2024

stephybun left a comment

xiaxyi commented Aug 27, 2024

stephybun left a comment

stephybun Jan 28, 2025

stephybun Jan 28, 2025

stephybun Jan 28, 2025

stephybun Jan 28, 2025

	return fmt.Errorf("non-partitioned entities are not allowed in partitioned namespace")
	return fmt.Errorf("topic must have `partitioning_enabled` set to `true` when the parent namespace is partitioned")

	return fmt.Errorf("the parent premium namespace is not partitioned and the partitioning for premium namespace is only available at the namepsace creation")
	return fmt.Errorf("topic partitioning is only available if the parent namespace is partitioned")

azurerm_servicebus_topic - add premium namespace partition validation #26680

Are you sure you want to change the base?

azurerm_servicebus_topic - add premium namespace partition validation #26680

Conversation

xiaxyi commented Jul 18, 2024 • edited Loading

Community Note

Description

PR Checklist

Changes to existing Resource / Data Source

Testing

Change Log

Related Issue(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiaxyi commented Aug 8, 2024

stephybun left a comment

Choose a reason for hiding this comment

xiaxyi commented Aug 27, 2024

stephybun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

`azurerm_servicebus_topic` - add premium namespace partition validation #26680

`azurerm_servicebus_topic` - add premium namespace partition validation #26680

xiaxyi commented Jul 18, 2024 •

edited

Loading