Skip to content
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

azurerm_servicebus_topic - add premium namespace partition validation #26680

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions internal/services/servicebus/servicebus_topic_resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,27 @@ func resourceServiceBusTopicCreateUpdate(d *pluginsdk.ResourceData, meta interfa
return fmt.Errorf("retrieving ServiceBus Namespace %q (Resource Group %q): %+v", id.NamespaceName, id.ResourceGroupName, err)
}

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
}
}
Copy link
Member

Choose a reason for hiding this comment

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

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
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

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")

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

} 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")
Copy link
Member

Choose a reason for hiding this comment

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

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 marked this conversation as resolved.
Show resolved Hide resolved
}

// output of `max_message_size_in_kilobytes` is also set in non-Premium namespaces, with a value of 256
if v, ok := d.GetOk("max_message_size_in_kilobytes"); ok && v.(int) != 256 {
if model := resp.Model; model != nil {
Expand Down
90 changes: 90 additions & 0 deletions internal/services/servicebus/servicebus_topic_resource_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package servicebus_test
import (
"context"
"fmt"
"regexp"
"testing"

"github.com/hashicorp/go-azure-sdk/resource-manager/servicebus/2021-06-01-preview/topics"
Expand Down Expand Up @@ -236,6 +237,37 @@ func TestAccServiceBusTopic_isoTimeSpanAttributes(t *testing.T) {
})
}

func TestAccServiceBusTopic_nonPartitionedPremiumNamespaceError(t *testing.T) {
data := acceptance.BuildTestData(t, "azurerm_servicebus_topic", "test")
r := ServiceBusTopicResource{}

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"),
Copy link
Member

Choose a reason for hiding this comment

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

This error message needs to be updated

},
})
}

func TestAccServiceBusTopic_partitionedPremiumNamespace(t *testing.T) {
data := acceptance.BuildTestData(t, "azurerm_servicebus_topic", "test")
r := ServiceBusTopicResource{}

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

Choose a reason for hiding this comment

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

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

},
{
Config: r.PremiumNamespacePartitioned(data, true),
Check: acceptance.ComposeTestCheckFunc(
check.That(data.ResourceName).ExistsInAzure(r),
),
},
data.ImportStep(),
})
}

func (t ServiceBusTopicResource) Exists(ctx context.Context, clients *clients.Client, state *pluginsdk.InstanceState) (*bool, error) {
id, err := topics.ParseTopicID(state.ID)
if err != nil {
Expand Down Expand Up @@ -505,3 +537,61 @@ resource "azurerm_servicebus_topic" "test" {
}
`, data.RandomInteger, data.Locations.Primary, data.RandomInteger, data.RandomInteger)
}

func (ServiceBusTopicResource) PremiumNamespacePartitioned(data acceptance.TestData, enabled bool) string {
return fmt.Sprintf(`
provider "azurerm" {
features {}
}

resource "azurerm_resource_group" "test" {
name = "acctestRG-%d"
location = "%s"
}

resource "azurerm_servicebus_namespace" "test" {
name = "acctestservicebusnamespace-%d"
resource_group_name = azurerm_resource_group.test.name
location = azurerm_resource_group.test.location
sku = "Premium"
premium_messaging_partitions = 2
capacity = 2
}

resource "azurerm_servicebus_topic" "test" {
name = "acctestservicebustopic-%d"
namespace_id = azurerm_servicebus_namespace.test.id

partitioning_enabled = %t
}
`, data.RandomInteger, data.Locations.Primary, data.RandomInteger, data.RandomInteger, enabled)
}

func (ServiceBusTopicResource) nonPartitionedPremiumNamespaceError(data acceptance.TestData) string {
return fmt.Sprintf(`
provider "azurerm" {
features {}
}

resource "azurerm_resource_group" "test" {
name = "acctestRG-%d"
location = "%s"
}

resource "azurerm_servicebus_namespace" "test" {
name = "acctestservicebusnamespace-%d"
resource_group_name = azurerm_resource_group.test.name
location = azurerm_resource_group.test.location
sku = "Premium"
premium_messaging_partitions = 1
capacity = 1
}

resource "azurerm_servicebus_topic" "test" {
name = "acctestservicebustopic-%d"
namespace_id = azurerm_servicebus_namespace.test.id

partitioning_enabled = true
}
`, data.RandomInteger, data.Locations.Primary, data.RandomInteger, data.RandomInteger)
}
2 changes: 1 addition & 1 deletion website/docs/r/servicebus_topic.html.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ The following arguments are supported:

* `enable_partitioning` - (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.
Copy link
Member

Choose a reason for hiding this comment

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

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.


* `max_message_size_in_kilobytes` - (Optional) Integer value which controls the maximum size of a message allowed on the topic for Premium SKU. For supported values see the "Large messages support" section of [this document](https://docs.microsoft.com/azure/service-bus-messaging/service-bus-premium-messaging#large-messages-support-preview).

Expand Down
Loading