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.

Sign up for GitHub

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

Jump to bottom

azurerm_redis_cache : fix incorrect ssl values for redis_primary_connection_string and secondary_connection_string #23575

Merged
merged 4 commits into from
Oct 18, 2023
Merged

azurerm_redis_cache : fix incorrect ssl values for redis_primary_connection_string and secondary_connection_string #23575

merged 4 commits into from
Oct 18, 2023

Conversation

sinbai
Copy link
Contributor

@sinbai sinbai commented Oct 16, 2023

Since the ports of primary_connection_string and secondary_connection_string are set to props.SslPort, the ssl value should always be true, rather than relying on the value of enable_non_ssl_port .

Fix #23490

@sinbai sinbai mentioned this pull request Oct 16, 2023
Closed
1 task
@sinbai sinbai changed the title azurerm_redis_cache : fix the wrong ssl value of redis_primary_connection_string and secondary_connection_string azurerm_redis_cache : fix incorrect ssl values for redis_primary_connection_string and secondary_connection_string Oct 16, 2023
catriona-m
catriona-m requested changes Oct 16, 2023
View reviewed changes
Copy link
Member

@catriona-m catriona-m left a comment

Choose a reason for hiding this comment

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

Thanks for opening this @sinbai. It looks like there is a test failure relating to this that needs to be addressed:

------- Stdout: -------
=== RUN   TestAccRedisCache_withoutSSL
=== PAUSE TestAccRedisCache_withoutSSL
=== CONT  TestAccRedisCache_withoutSSL
    testcase.go:113: Step 1/2 error: Check failed: Check 2/3 error: Bad: wrong SSL setting in connection string: primary_connection_string
--- FAIL: TestAccRedisCache_withoutSSL (2048.00s)
FAIL

@Poil
Copy link

Poil commented Oct 16, 2023

Hi,

Thanks,

Perhaps we need two outputs nonssl_primary_connection_string & primary_connection_string to cover every usage ?

of course, nonssl_primary_connection_string should use nonssl port with ssl=false and primary_connection_string should use ssl port with ssl=true ?

@sinbai
Copy link
Contributor Author

sinbai commented Oct 17, 2023

Thanks for opening this @sinbai. It looks like there is a test failure relating to this that needs to be addressed:

------- Stdout: -------
=== RUN   TestAccRedisCache_withoutSSL
=== PAUSE TestAccRedisCache_withoutSSL
=== CONT  TestAccRedisCache_withoutSSL
    testcase.go:113: Step 1/2 error: Check failed: Check 2/3 error: Bad: wrong SSL setting in connection string: primary_connection_string
--- FAIL: TestAccRedisCache_withoutSSL (2048.00s)
FAIL

Hi @catriona-m thanks for your feedback. The code has been updated. Could you please take another look?

catriona-m
catriona-m reviewed Oct 17, 2023
View reviewed changes
internal/services/redis/redis_cache_resource_test.go Outdated
@@ -1492,7 +1492,7 @@ resource "azurerm_redis_cache" "test" {
`, data.RandomInteger, data.Locations.Primary, data.RandomInteger)
}

func testCheckSSLInConnectionString(resourceName string, propertyName string, requireSSL bool) acceptance.TestCheckFunc {
func testCheckSSLInConnectionString(resourceName string, propertyName string) acceptance.TestCheckFunc {
Copy link
Member

Choose a reason for hiding this comment

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

given that the connection string is now specifically set to be true now, we probably don't need to have this check in the tests at all anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion, fixed. Thanks.

catriona-m
catriona-m approved these changes Oct 18, 2023
View reviewed changes
Copy link
Member

@catriona-m catriona-m left a comment

Choose a reason for hiding this comment

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

Thanks @sinbai LGTM!

@catriona-m catriona-m merged commit f8a3e41 into hashicorp:main Oct 18, 2023
22 checks passed
@github-actions github-actions bot added this to the v3.77.0 milestone Oct 18, 2023
catriona-m added a commit that referenced this pull request Oct 18, 2023
@catriona-m
Update CHANGELOG.md #23575
566c75b
dduportal pushed a commit to jenkins-infra/azure that referenced this pull request Oct 20, 2023
@jenkins-infra-updatecli @jenkins-infra-bot
Bump Terraform azurerm provider version to 3.77.0 (#500)
106e45d 
<Actions>
<action
id="4a39167e811ac038e4a588362092472c27cfbe9e4929ae61d035f708a093a669">
        <h3>Bump Terraform `azurerm` provider version</h3>
<details
id="1d9343c012f5434ac9fe8a98135bae3667b399259be16d9b14302ea3bd424a24">
            <summary>Update Terraform lock file</summary>
<p>&#34;hashicorp/azurerm&#34; updated from &#34;3.76.0&#34; to
&#34;3.77.0&#34; in file &#34;.terraform.lock.hcl&#34;</p>
            <details>
                <summary>3.77.0</summary>
<pre>Changelog retrieved
from:&#xA;&#x9;https://github.com/hashicorp/terraform-provider-azurerm/releases/tag/v3.77.0&#xA;FEATURES:&#xA;&#xA;*
New Resources: `azurerm_application_load_balancer_frontend`
([#23411](hashicorp/terraform-provider-azurerm#23411
New Resources: `azurerm_dev_center`
([#23538](hashicorp/terraform-provider-azurerm#23538
New Resources: `azurerm_dev_center_project`
([#23538](https://github.com/hashicorp/terraform-provider-azurerm/issues/23538))&#xA;&#xA;ENHANCEMENTS:&#xA;&#xA;*
dependencies: updating to `v0.62.0` of
`github.com/hashicorp/go-azure-helpers`
([#23581](hashicorp/terraform-provider-azurerm#23581
dependencies: updating Kusto SDK from `2023-05-02` to `2023-08-15`
([#23598](hashicorp/terraform-provider-azurerm#23598
dependencies: updating nginx from `2022-08-01` to `2023-04-01`
([#23583](hashicorp/terraform-provider-azurerm#23583
`netapp`: updating to use API Version `2023-05-01`
([#23576](hashicorp/terraform-provider-azurerm#23576
`springcloud`: updating to use API Version `2023-09-01-preview`
([#23544](hashicorp/terraform-provider-azurerm#23544
`storage`: updating to use API Version `2023-01-01`
([#23543](hashicorp/terraform-provider-azurerm#23543
`internal/sdk`: fixing an issue where struct fields containing
`removedInNextMajorVersion` wouldn&#39;t be decoded correctly
([#23564](hashicorp/terraform-provider-azurerm#23564
`internal/sdk`: struct tag parsing is now handled consistently during
both encoding and decoding
([#23568](hashicorp/terraform-provider-azurerm#23568
provider: the `roll_instances_when_required` provider feature in the
`virtual_machine_scale_set` block is now optional
([#22976](hashicorp/terraform-provider-azurerm#22976
Data Source: `azurerm_automation_account`: refactoring the remaining
usage of `Azure/azure-sdk-for-go` to use `hashicorp/go-azure-sdk`
([#23555](hashicorp/terraform-provider-azurerm#23555
`azurerm_automation_account`: refactoring the remaining usage of
`Azure/azure-sdk-for-go` to use `hashicorp/go-azure-sdk`
([#23555](hashicorp/terraform-provider-azurerm#23555
`azurerm_resource_deployment_script_azure_cli` - improve validation for
the `version` property to support newer versions
([#23370](hashicorp/terraform-provider-azurerm#23370
`azurerm_resource_deployment_script_azure_power_shell` - improve
validation for the `version` property to support newer versions
([#23370](hashicorp/terraform-provider-azurerm#23370
`azurerm_nginx_deployment` - support for the `capacity` and `email`
properties
([#23596](https://github.com/hashicorp/terraform-provider-azurerm/issues/23596))&#xA;&#xA;BUG
FIXES:&#xA;&#xA;* Data Source: `azurerm_virtual_hub_connection` - export
the `inbound_route_map_id`, `outbound_route_map_id`, and
`static_vnet_local_route_override_criteria` attributes in the `routing`
block, and fix a bug where these attributes could not be set
([#23491](hashicorp/terraform-provider-azurerm#23491
`azurerm_cdn_frontdoor_rule` - the `url_filename_condition` properties
`match_values` is now optional if `operator` is set to `Any`
([#23541](hashicorp/terraform-provider-azurerm#23541
`azurerm_shared_image_gallery` - added the `Private` and `Groups`
options for the `sharing.permission` property
([#23570](hashicorp/terraform-provider-azurerm#23570
`azurerm_redis_cache` - fixed incorrect ssl values for
`redis_primary_connection_string` and `secondary_connection_string`
([#23575](hashicorp/terraform-provider-azurerm#23575
`azurerm_monitor_activity_log_alert` - the `recommend_category` property
now can be set to `HighAvailability`
([#23605](hashicorp/terraform-provider-azurerm#23605
`azurerm_recovery_services_vault` - the `encryption` property can now be
used with the `cross_region_restore_enabled` property
([#23618](hashicorp/terraform-provider-azurerm#23618
`azurerm_storage_account_customer_managed_key` - prevent a panic when
the keyvault id is empty
([#23599](https://github.com/hashicorp/terraform-provider-azurerm/issues/23599))&#xA;&#xA;&#xA;</pre>
            </details>
        </details>
    </action>
</Actions>

---

<table>
  <tr>
    <td width="77">
<img src="https://www.updatecli.io/images/updatecli.png" alt="Updatecli
logo" width="50" height="50">
    </td>
    <td>
      <p>
Created automatically by <a
href="https://www.updatecli.io/">Updatecli</a>
      </p>
      <details><summary>Options:</summary>
        <br />
<p>Most of Updatecli configuration is done via <a
href="https://www.updatecli.io/docs/prologue/quick-start/">its
manifest(s)</a>.</p>
        <ul>
<li>If you close this pull request, Updatecli will automatically reopen
it, the next time it runs.</li>
<li>If you close this pull request and delete the base branch, Updatecli
will automatically recreate it, erasing all previous commits made.</li>
        </ul>
        <p>
Feel free to report any issues at <a
href="https://github.com/updatecli/updatecli/issues">github.com/updatecli/updatecli</a>.<br
/>
If you find this tool useful, do not hesitate to star <a
href="https://github.com/updatecli/updatecli/stargazers">our GitHub
repository</a> as a sign of appreciation, and/or to tell us directly on
our <a
href="https://matrix.to/#/#Updatecli_community:gitter.im">chat</a>!
        </p>
      </details>
    </td>
  </tr>
</table>

Co-authored-by: Jenkins Infra Bot (updatecli) <60776566+jenkins-infra-bot@users.noreply.github.com>
@sinbai sinbai deleted the redis/fix_issue_23490 branch March 28, 2024 02:45
@github-actions GitHub Actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@catriona-m catriona-m catriona-m approved these changes

Assignees
No one assigned
Labels
service/redis size/S
Projects
None yet
Milestone
v3.77.0
Development

Successfully merging this pull request may close these issues.

azurerm_redis_cache - output redis_primary_connection_string ssl=false but port is 6380
3 participants
@sinbai @Poil @catriona-m