`azurerm_servicebus_topic` - add premium namespace partition validation by xiaxyi · Pull Request #26680 · hashicorp/terraform-provider-azurerm

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

 * `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.

…idation

xiaxyi · 2025-02-10T09:05:27Z

@stephybun Thanks for the review, code is updated and tests are passed:

--- PASS: TestAccServiceBusTopic_enablePartitioningStandard (221.89s)
--- PASS: TestAccServiceBusTopic_complete (222.31s)
--- PASS: TestAccServiceBusTopicAuthorizationRule_listen (228.72s)
--- PASS: TestAccServiceBusTopicAuthorizationRule_rightsUpdate (254.37s)
--- PASS: TestAccServiceBusTopic_nonPartitionedPremiumNamespaceError (288.48s)
--- PASS: TestAccServiceBusTopicAuthorizationRule_requiresImport (187.39s)
--- PASS: TestAccServiceBusTopic_enablePartitioningPremium (411.51s)
--- PASS: TestAccServiceBusTopicAuthorizationRule_manage (209.02s)
--- PASS: TestAccServiceBusTopicAuthorizationRule_listensend (204.26s)
--- PASS: TestAccServiceBusTopic_update (237.50s)
--- PASS: TestAccServiceBusTopic_isoTimeSpanAttributes (184.77s)
--- PASS: TestAccServiceBusTopic_basicDisableEnable (284.71s)
--- PASS: TestAccServiceBusTopic_partitionedPremiumNamespace (327.35s)
--- PASS: TestAccServiceBusTopic_enableDuplicateDetection (185.33s)
--- PASS: TestAccServiceBusTopicAuthorizationRule_send (191.62s)
--- PASS: TestAccServiceBusTopic_requiresImport (166.00s)
--- PASS: TestAccServiceBusTopicAuthorizationRule_withAliasConnectionString (574.55s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/servicebus    1022.553s

stephybun

Thanks @xiaxyi LGTM 👍

* CHANGELOG.md for v4.19.0 * Update CHANGELOG.md #28523 * Update CHANGELOG.md #28691 * Updated to include #28717 * Update for #26680 * Update CHANGELOG.md #28633 * Update CHANGELOG.md for #28703 * Update CHANGELOG.md for #28391 * Update CHANGELOG.md #28725 * Update #28733 * Update CHANGELOG.md #28659 * Update for #28741 * Update CHANGELOG.md #28712 * Update CHANGELOG.md #28441 * Update CHANGELOG.md #28441 * Update CHANGELOG.md #28441 * Update CHANGELOG.md for #28602 * Update for #27424 * Update CHANGELOG.md for #28524 * Update CHANGELOG.md #28726 * Update for #28767 * Update for #28195 * prep for release v4.19.0 --------- Co-authored-by: sreallymatt <[email protected]> Co-authored-by: Wodans Son <[email protected]> Co-authored-by: stephybun <[email protected]> Co-authored-by: Wyatt Fry <[email protected]> Co-authored-by: Matthew Frahry <[email protected]> Co-authored-by: jackofallops <[email protected]>

…on (hashicorp#26680) * add premium namespace partition validation * update test case to fit the 4.0 flag in teamCity * update test case - remove import step from the error validation test * simplify the test checks * update code per reviewer's comment * update code based on review

* CHANGELOG.md for v4.19.0 * Update CHANGELOG.md hashicorp#28523 * Update CHANGELOG.md hashicorp#28691 * Updated to include hashicorp#28717 * Update for hashicorp#26680 * Update CHANGELOG.md hashicorp#28633 * Update CHANGELOG.md for hashicorp#28703 * Update CHANGELOG.md for hashicorp#28391 * Update CHANGELOG.md hashicorp#28725 * Update hashicorp#28733 * Update CHANGELOG.md hashicorp#28659 * Update for hashicorp#28741 * Update CHANGELOG.md hashicorp#28712 * Update CHANGELOG.md hashicorp#28441 * Update CHANGELOG.md hashicorp#28441 * Update CHANGELOG.md hashicorp#28441 * Update CHANGELOG.md for hashicorp#28602 * Update for hashicorp#27424 * Update CHANGELOG.md for hashicorp#28524 * Update CHANGELOG.md hashicorp#28726 * Update for hashicorp#28767 * Update for hashicorp#28195 * prep for release v4.19.0 --------- Co-authored-by: sreallymatt <[email protected]> Co-authored-by: Wodans Son <[email protected]> Co-authored-by: stephybun <[email protected]> Co-authored-by: Wyatt Fry <[email protected]> Co-authored-by: Matthew Frahry <[email protected]> Co-authored-by: jackofallops <[email protected]>

github-actions · 2025-03-13T02:15:41Z

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

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 suggested 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 suggested 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 a team and katbyte as code owners November 14, 2024 00:08

stephybun suggested 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

xiaxyi added 2 commits February 10, 2025 16:19

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

31f4674

…idation

update code based on review

6675c4c

github-actions bot removed the waiting-response label Feb 10, 2025

stephybun approved these changes Feb 10, 2025

View reviewed changes

stephybun merged commit 5c81ab3 into hashicorp:main Feb 10, 2025
33 checks passed

github-actions bot added this to the v4.19.0 milestone Feb 10, 2025

stephybun added a commit that referenced this pull request Feb 10, 2025

Update for #26680

7a21eb2

github-actions bot locked as resolved and limited conversation to collaborators Mar 13, 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")

Conversation

xiaxyi commented Jul 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Community Note

Description

PR Checklist

Changes to existing Resource / Data Source

Testing

Change Log

Related Issue(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xiaxyi commented Aug 8, 2024

Uh oh!

stephybun left a comment

Choose a reason for hiding this comment

Uh oh!

xiaxyi commented Aug 27, 2024

Uh oh!

stephybun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xiaxyi commented Feb 10, 2025

Uh oh!

stephybun left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Mar 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

xiaxyi commented Jul 18, 2024 •

edited

Loading