`azurerm_search_service` - add support for `local_authentication_enabled`, `authentication_failure_mode`, `authentication_options` and `hosting_mode` #21323

WodansSon · 2023-04-06T19:12:26Z

WodansSon · 2023-04-09T12:44:28Z

Progress! 🙂

WodansSon · 2023-04-13T09:35:34Z

tombuildsstuff

Thanks for this PR - I've taken a look through and left some design comments inline, but if we can fix those up then we should be able to take another look here.

Thanks!

internal/services/search/search_service_resource.go

tombuildsstuff · 2023-04-13T09:56:47Z

internal/services/search/search_service_resource.go

+	// fix for issue #10151, if the identity type is TypeNone do not include it
+	// in the create call, only in the update call when 'identity' is removed from the
+	// configuration file...
+	if expandedIdentity.Type != identity.TypeNone {


there's a Swagger issue for this if memory serves?

I don't know... I looked but was not able to locate it... 🤷‍♂️

tombuildsstuff · 2023-04-13T09:57:12Z

internal/services/search/search_service_resource.go

+				return fmt.Errorf("expanding `identity`: %+v", err)
+			}
+
+			// NOTE: Passing type 'None' on update will remove all identities from the service.


that's the standard behaviour in ARM, so I think we can remove this comment?

Suggested change

// NOTE: Passing type 'None' on update will remove all identities from the service.

tombuildsstuff · 2023-04-13T11:42:23Z

internal/services/search/search_service_resource.go

+			var cmkCompliance string
+			if props.EncryptionWithCmk != nil {
+				cmkEnforcement = strings.EqualFold(string(pointer.From(props.EncryptionWithCmk.Enforcement)), string(services.SearchEncryptionWithCmkEnabled))
+				cmkCompliance = string(pointer.From(props.EncryptionWithCmk.EncryptionComplianceStatus))
+			}


Given the compliance status is checking data within the search service, it's a reasonable expectation that this would be as async value, therefore the value returned from the API is likely to be outdated on the initial run, so I'm curious as to the value of exposing these field(s)? Whilst the enabled toggle may make sense, the status isn't going to be something users are likely to action upon in Terraform (and would instead be triggered via a report/interactively via the CLI/Portal etc) - so I have a feeling we shouldn't expose status here?

I am not sure, but I would lean toward exposing it rather than hide it as we don't know how the end-users would use this field. On a side note, what is the harm in having it exposed even if no one uses it?

On a side note, what is the harm in having it exposed even if no one uses it?

The challenge in doing so is where doing so can have unintended consequences - for example in this case since the status field is likely to be updated asynchronously, the value being returned by Terraform here likely doesn't make sense for us to expose. Whilst we could expose the enabled field - since the status field is both:

a) possible out of date since it's likely this is updated OOB
b) likely run interactively (e.g. via the Azure CLI, or alerted on via a monitor etc)

I think we should remove the status field for now until we have a valid use-case for that

tombuildsstuff · 2023-04-13T11:43:07Z

internal/services/search/search_service_resource_test.go

+
+func TestAccSearchService_replicaCountInvalid(t *testing.T) {
+	data := acceptance.BuildTestData(t, "azurerm_search_service", "test")
+	// NOTE: The default qouta for the 'free' sku is 1


Suggested change

// NOTE: The default qouta for the 'free' sku is 1

// NOTE: The default quota for the 'free' sku is 1

tombuildsstuff · 2023-04-13T11:44:31Z

internal/services/search/search_service_resource_test.go

+	sku                                       string
+	count                                     int
+	enforceCmk                                bool
+	apiAccessControlType                      string
+	apiAccessControlAuthenticationFailureMode string


nit: elsewhere in the provider we pass these arguments in where required - but typically use pre-templated Terraform configurations as tests - could we update this to match the convention used in the rest of the provider?

Suggested change

sku string

count int

enforceCmk bool

apiAccessControlType string

apiAccessControlAuthenticationFailureMode string

tombuildsstuff · 2023-04-13T11:46:07Z

internal/services/search/search_service_resource_test.go

+	rbac := SearchServiceResource{sku: "standard", apiAccessControlType: "role_based_access_control"}
+	rbacApiFourZeroOne := SearchServiceResource{sku: "standard", apiAccessControlType: "role_based_access_control_and_api_keys", apiAccessControlAuthenticationFailureMode: "http401WithBearerChallenge"}
+	rbacApiFourZeroThree := SearchServiceResource{sku: "standard", apiAccessControlType: "role_based_access_control_and_api_keys", apiAccessControlAuthenticationFailureMode: "http403"}


whilst there's some readability benefits on a per-test basis here - it's more likely we'll introduce bugs with this approach since the configs are harder to reason with, can we update this to use distinct Terraform configurations (as above)?

tombuildsstuff · 2023-04-13T12:23:05Z

internal/services/search/validate/search_service_partition_count.go

+	partitionCount := v.(int)
+
+	switch partitionCount {
+	case 5, 7, 8, 9, 10, 11:
+		errors = append(errors, fmt.Errorf("%q must be 1, 2, 3, 4, 6, or 12, got %d", k, partitionCount))
+	}
+
+	if partitionCount > 12 {
+		errors = append(errors, fmt.Errorf("%q must be 1, 2, 3, 4, 6, or 12, got %d", k, partitionCount))
+	}
+
+	return warnings, errors


we can reuse an existing validation function here:

Suggested change

partitionCount := v.(int)

switch partitionCount {

case 5, 7, 8, 9, 10, 11:

errors = append(errors, fmt.Errorf("%q must be 1, 2, 3, 4, 6, or 12, got %d", k, partitionCount))

}

if partitionCount > 12 {

errors = append(errors, fmt.Errorf("%q must be 1, 2, 3, 4, 6, or 12, got %d", k, partitionCount))

}

return warnings, errors

return validation.IntInSlice([]int{

1,

2,

3,

4,

6,

12,

})

WodansSon · 2023-04-14T01:33:41Z

tombuildsstuff

hey @WodansSon

Thanks for pushing those changes, I've taken a look through and left some comments inline - so that we can get this PR merged I hope you don't mind but I'm going to push a commit to fix those, so that we can get this shipped.

Thanks!

internal/services/search/search_service_resource.go

tombuildsstuff · 2023-04-26T09:08:39Z

internal/services/search/search_service_resource.go

+
+	// NOTE: hosting mode is only valid if the SKU is 'standard3'
+	if skuName != services.SkuNameStandardThree && hostingMode == services.HostingModeHighDensity {
+		return fmt.Errorf("'hosting_mode' can only be defined if the 'sku' field is set to the 'standard3' SKU, got %q", skuName)


Can you link the other PR? Where a constant value is used in an error message, we should be referencing the constant value, since this ensures we avoid typos etc

tombuildsstuff · 2023-04-26T09:22:06Z

internal/services/search/search_service_resource.go

+			var cmkCompliance string
+			if props.EncryptionWithCmk != nil {
+				cmkEnforcement = strings.EqualFold(string(pointer.From(props.EncryptionWithCmk.Enforcement)), string(services.SearchEncryptionWithCmkEnabled))
+				cmkCompliance = string(pointer.From(props.EncryptionWithCmk.EncryptionComplianceStatus))
+			}


On a side note, what is the harm in having it exposed even if no one uses it?

The challenge in doing so is where doing so can have unintended consequences - for example in this case since the status field is likely to be updated asynchronously, the value being returned by Terraform here likely doesn't make sense for us to expose. Whilst we could expose the enabled field - since the status field is both:

a) possible out of date since it's likely this is updated OOB
b) likely run interactively (e.g. via the Azure CLI, or alerted on via a monitor etc)

I think we should remove the status field for now until we have a valid use-case for that

internal/services/search/search_service_resource.go

jackofallops

Thanks @WodansSon & @tombuildsstuff - This LGTM 👍

tombuildsstuff · 2023-04-26T14:27:16Z

Tests pass:

github-actions · 2023-04-28T13:29:51Z

This functionality has been released in v3.54.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

njglenuk · 2023-05-01T05:49:53Z

There seems to be an issue with the new argument: "hosting_mode" with azurerm 3.54. It is forcing a replacement of the search service due to the API returning 'Default' with a capital and azurerm trying change this to 'default' without a capital:

hosting_mode = "Default" -> "default" # forces replacement

I've checked the terraform state: terraform state show "azurerm_search_service.example"
And it returns:

resource "azurerm_search_service" "example" {
...
hosting_mode = "Default"
...

Removing the resource from state and re-importing does not help to resolve the issue.

Setting the argument in the Terraform code to: hosting_mode = "default" also forces the replacement.

Trying to use hosting_mode = "Default" errors out too.

I've had to lock the azurerm provider to 3.53 to allow the run to complete without forcing the replacement.

Here is the original Terrafrom code:

resource "azurerm_search_service" "example" {
name = local.cognitive_search_name
resource_group_name = local.resource_group_name
location = local.location
sku = "standard"
public_network_access_enabled = false
identity {
type = "SystemAssigned"
}
tags = local.common_tags
}

github-actions · 2024-06-01T02:09:19Z

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.

Initial Check-in...

94343e7

WodansSon added the service/search label Apr 6, 2023

WodansSon added this to the Future milestone Apr 6, 2023

github-actions bot added the size/XL label Apr 6, 2023

WodansSon marked this pull request as draft April 6, 2023 19:21

Update documentation...

2570c85

github-actions bot added the documentation label Apr 6, 2023

Checking-in progress...

04679c5

WodansSon changed the title ~~azurerm_search_service - add support for authentication_options, cmk_encryption and hosting_mode~~ azurerm_search_service - add support for api_access_control, cmk_encryption and hosting_mode Apr 7, 2023

Checking-in progress...

11574a2

WodansSon changed the title ~~azurerm_search_service - add support for api_access_control, cmk_encryption and hosting_mode~~ azurerm_search_service - add support for api_access_control, cmk_enforcement_enabled and hosting_mode Apr 9, 2023

WodansSon added 2 commits April 9, 2023 04:25

Update docs...

79ee3e8

Update Docs...

ef6c1c1

WodansSon added 3 commits April 12, 2023 04:55

Check-in progress...

6563bc1

Update functions names...

a201b59

Check-in progress...

ef25696

WodansSon marked this pull request as ready for review April 13, 2023 08:51

WodansSon modified the milestones: Future, v3.53.0 Apr 13, 2023

tombuildsstuff suggested changes Apr 13, 2023

View reviewed changes

github-actions bot added waiting-response and removed waiting-response labels Apr 13, 2023

WodansSon added 3 commits April 13, 2023 17:26

Address PR comments...

0d09a5d

Fix some test cases...

7f4030f

More test tweaks...

cfb6f75

Update to docs...

a3f6b7e

WodansSon changed the title ~~azurerm_search_service - add support for api_access_control, cmk_enforcement_enabled and hosting_mode~~ azurerm_search_service - add support for local_authentication_disabled, authentication_failure_mode and hosting_mode Apr 14, 2023

WodansSon added 3 commits April 15, 2023 01:54

Added Setting API Access Control section to docs

a1ab2a3

Fix spacing in said doc table...

5b761d5

Center last col in docs...

f4fa384

katbyte modified the milestones: v3.53.0, v3.54.0 Apr 21, 2023

tombuildsstuff approved these changes Apr 26, 2023

View reviewed changes

github-actions bot added size/XS and removed size/XL labels Apr 26, 2023

r/search_service: refactoring comments from PR review

859433a

tombuildsstuff force-pushed the e_search_fix branch from 6e9e741 to 859433a Compare April 26, 2023 12:00

github-actions bot added size/XL and removed size/XS labels Apr 26, 2023

tombuildsstuff requested a review from jackofallops April 26, 2023 12:04

jackofallops approved these changes Apr 26, 2023

View reviewed changes

tombuildsstuff merged commit 7b14fce into main Apr 26, 2023

tombuildsstuff deleted the e_search_fix branch April 26, 2023 14:30

tombuildsstuff added a commit that referenced this pull request Apr 26, 2023

updating to include #21323

574aa9a

a111792 mentioned this pull request May 2, 2023

azurerm_search_service: Issue with "hosting_mode" in azurerm 3.54. Forces replacement of Resource due to the API returning 'Default' with a capital and azurerm trying change this to 'default' without a capital #21630

Closed

github-actions bot locked as resolved and limited conversation to collaborators Jun 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`azurerm_search_service` - add support for `local_authentication_enabled`, `authentication_failure_mode`, `authentication_options` and `hosting_mode` #21323

`azurerm_search_service` - add support for `local_authentication_enabled`, `authentication_failure_mode`, `authentication_options` and `hosting_mode` #21323

WodansSon commented Apr 6, 2023

WodansSon commented Apr 9, 2023

WodansSon commented Apr 13, 2023

tombuildsstuff left a comment

tombuildsstuff Apr 13, 2023

WodansSon Apr 13, 2023

tombuildsstuff Apr 13, 2023

WodansSon Apr 13, 2023

tombuildsstuff Apr 13, 2023

WodansSon Apr 14, 2023

tombuildsstuff Apr 26, 2023

tombuildsstuff Apr 13, 2023

WodansSon Apr 13, 2023

tombuildsstuff Apr 13, 2023

WodansSon Apr 13, 2023

tombuildsstuff Apr 13, 2023

WodansSon Apr 13, 2023

tombuildsstuff Apr 13, 2023

WodansSon Apr 13, 2023

WodansSon commented Apr 14, 2023

tombuildsstuff left a comment

tombuildsstuff Apr 26, 2023

tombuildsstuff Apr 26, 2023

jackofallops left a comment

tombuildsstuff commented Apr 26, 2023

github-actions bot commented Apr 28, 2023

njglenuk commented May 1, 2023 •

edited

Loading

github-actions bot commented Jun 1, 2024

	// NOTE: The default qouta for the 'free' sku is 1
	// NOTE: The default quota for the 'free' sku is 1

azurerm_search_service - add support for local_authentication_enabled, authentication_failure_mode, authentication_options and hosting_mode #21323

azurerm_search_service - add support for local_authentication_enabled, authentication_failure_mode, authentication_options and hosting_mode #21323

Conversation

WodansSon commented Apr 6, 2023

WodansSon commented Apr 9, 2023

WodansSon commented Apr 13, 2023

tombuildsstuff 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

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

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

WodansSon commented Apr 14, 2023

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackofallops left a comment

Choose a reason for hiding this comment

tombuildsstuff commented Apr 26, 2023

github-actions bot commented Apr 28, 2023

njglenuk commented May 1, 2023 • edited Loading

github-actions bot commented Jun 1, 2024

`azurerm_search_service` - add support for `local_authentication_enabled`, `authentication_failure_mode`, `authentication_options` and `hosting_mode` #21323

`azurerm_search_service` - add support for `local_authentication_enabled`, `authentication_failure_mode`, `authentication_options` and `hosting_mode` #21323

njglenuk commented May 1, 2023 •

edited

Loading