ENG-12292: Add: SQL Server settings (version field) #438

gengdahlCyral · 2023-08-14T14:50:15Z

Note! Crud v5.9.0 has the API additions for which this TF PR is targeted.

Description of the change

The background to the changes made in the Cyral Terraform Provider is the updated listener API {{baseUrl}}/sidecars/:sidecarID/listeners.

This API has been extended with a complex attribute (similar to the mysql settings attribute):

Sql server settings (optional)
- version (string in format x.y.z) Required
  The SQL server settings is applicable for listeners associated with SQL server repo type only, and is only relevant for listeners that are multiplexed.
  Version is the version the sidecar will advertise in PRELOGIN MESSAGE to the client connecting.

Short description of changes in this PR:
This PR introduces changes to the Makefile, cyral/data_source_cyral_sidecar_listener.go,

In the Makefile, an up-deps target has been added to update module and test dependencies to the latest minor and patch levels.

In cyral/data_source_cyral_sidecar_listener.go, SQL Server settings have been added to the SidecarListener struct. This includes a new SQLServerSettings struct, methods to convert to and from interface representations, and relevant constants.

In cyral/resource_cyral_sidecar_listener.go, the SidecarListener struct has been expanded to include SQL Server settings similar to other data repository settings. Additionally, SQL Server settings methods for interface conversion have been added. The schema definition for SQL Server settings has also been included in the getSidecarListenerSchema function.

In cyral/resource_cyral_sidecar_listener_test.go, a test step has been added to cover SQL Server settings.

Documentation files (docs/data-sources/sidecar_listener.md and docs/resources/sidecar_listener.md) have been generated from updated code.

Type of change

Bug fix (non-breaking change that fixes an issue)
New feature (non-breaking change that adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklists

Development

Lint rules pass locally
The code changed/added as part of this pull request has been covered with tests
All tests related to the changed code pass in development

Code review

This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached
Jira issue referenced in commit message and/or PR title

Testing

Using Helm CP running updated crud

Created a listener without Sql server settings using TF.
Updated listener with sql server settings, but no version -> error from TF (expected)
Updated listener with valid server version (no error)
Listed listeners, noticed presence of sql server version
tried incorrect version format -> error from API (expected)

…s has one attribute: version. This is mandatory for listeners that are multiplexed.

…ngs and tls mode

…cific version (parent element is optional though). Added computed to the two optional elements "override repo client tls settings" and "tls mode" since these are populated by API if not provided.

…mode in listener data source

yoursnerdly · 2023-08-21T19:09:49Z

cyral/data_source_cyral_sidecar_listener.go

@@ -8,8 +8,9 @@ import (
 	"github.com/google/uuid"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"



nit: could you please delete this blank line so that all 3rd party imports are in the same block?

yoursnerdly · 2023-08-21T19:11:10Z

cyral/resource_cyral_sidecar_listener.go

+	ProxyModeKey         = "proxy_mode"
+	DynamoDbSettingsKey  = "dynamodb_settings"
+	SQLServerSettingsKey = "sqlserver_settings"
+	VersionKey           = "version"


How about (for clarity)?

Suggested change

VersionKey = "version"

SQLServerVersionKey = "version"

yes, I took your suggestion. Thanks

yoursnerdly · 2023-08-21T19:11:38Z

cyral/resource_cyral_sidecar_listener.go

 )

+func tlsModes() []string {


bring this change in later (since it's not being planned for implementation right now)?

thank you, leftover from refactor. Missed this one.

…tant

wcmjunior

Please add some tests to cover the ConflictsWith controls.

…alidation. Updated setupSidecarListenerConfig method to support constructing faulty configurations (switch -> multiple if statements)

gengdahlCyral · 2023-08-23T09:59:21Z

Please add some tests to cover the ConflictsWith controls.

Addressed by d0576c6

juniocezar · 2023-08-23T21:06:09Z

cyral/resource_cyral_sidecar_listener_test.go

 		dbVersion, charSet := "null", "null"
 		if listener.MySQLSettings.CharacterSet != "" {
 			charSet = fmt.Sprintf(`"%s"`, listener.MySQLSettings.CharacterSet)
 		}
 		if listener.MySQLSettings.DbVersion != "" {
 			dbVersion = fmt.Sprintf(`"%s"`, listener.MySQLSettings.DbVersion)
 		}
-		settings = fmt.Sprintf(
+		settings += fmt.Sprintf(


+ doesn't actually look required here, but shouldn't hurt to keep either.

This is actually intentional in order to create an invalid configuration (conflicting settings)

juniocezar

changes looks good to me, regarding what the API expects.

VictorGFM

Changes look good @gengdahlCyral, please consider my comment below regarding the ConflictsWith validation before we merge the PR, thanks

VictorGFM · 2023-09-11T11:15:40Z

cyral/resource_cyral_sidecar_listener.go

@@ -352,7 +378,7 @@ func getSidecarListenerSchema() map[string]*schema.Schema {
 			Optional:    true,
 			// Notice the MaxItems: 1 here. This ensures that the user can only specify one this block.
 			MaxItems:      1,
-			ConflictsWith: []string{MySQLSettingsKey, DynamoDbSettingsKey},
+			ConflictsWith: []string{MySQLSettingsKey, DynamoDbSettingsKey, SQLServerSettingsKey},


Should we also update the ConflictsWith in DynamoDbSettingsKey?

ConflictsWith: []string{S3SettingsKey, MySQLSettingsKey, SQLServerSettingsKey}

yes, sorry. Fixed

VictorGFM · 2023-09-11T11:16:47Z

cyral/resource_cyral_sidecar_listener.go

+							"Note: If the query returns a four part version number, only the first three parts " +
+							"should be used. Example: 16.0.1000.6 -> 16.0.1000",
+						Type:     schema.TypeString,
+						Optional: false,


Suggested change

Optional: false,

In API Sqlserver settings is optional, if provided though, it has to carry the version so I believe we want to align with that here.

Sure, my intention here is just to point out that there's no need to specify Optional: false - it can be omitted from the schema declaration, especially because we are already defining Required: true.

ah, sorry I didn't get that at first :) Want me to remove and push or leave it be?

@gengdahlCyral thats just a nit, so up to you! Let me know once we can merge the PR and I can help with that, changes look good to me.

Cool, I removed this line, it's a bit confusing to have optional and required mixed (and not really valid when I read the docs).
Feel free to merge. @VictorGFM

…alse)

sonarqubecloud · 2023-09-12T06:35:16Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

gengdahlCyral added 5 commits August 14, 2023 16:37

ENG-12292 - Added SQL Server settings to listener. SQL Server setting…

02c9725

…s has one attribute: version. This is mandatory for listeners that are multiplexed.

ENG-12292 - added listener attributes; override repo client tls setti…

beee09c

…ngs and tls mode

ENG-12292 - minor doc updates

08689a0

ENG-12292 - Setting slq settings to required since API mandates a spe…

eecb689

…cific version (parent element is optional though). Added computed to the two optional elements "override repo client tls settings" and "tls mode" since these are populated by API if not provided.

ENG-12292 - Added population of override client tls settings and tls …

1ec2032

…mode in listener data source

gengdahlCyral marked this pull request as ready for review August 15, 2023 12:21

gengdahlCyral requested review from wcmjunior, juniocezar and yoursnerdly August 15, 2023 12:21

wcmjunior changed the title ~~ENG-12292 - Add: SQL Server settings, Override client tls flag and TLS mode for listeners~~ ENG-12292: Add: SQL Server settings, Override client tls flag and TLS mode for listeners Aug 17, 2023

gengdahlCyral added 2 commits August 21, 2023 09:47

ENG-12292 - merge with main

05899b3

ENG-12292 -Remove tls settings; these are handled in separate PRs.

453304a

gengdahlCyral changed the title ~~ENG-12292: Add: SQL Server settings, Override client tls flag and TLS mode for listeners~~ ENG-12292: Add: SQL Server settings (version field) Aug 21, 2023

yoursnerdly reviewed Aug 21, 2023

View reviewed changes

ENG-12292 - Review comments; re-arrange imports, renamed version cons…

5d8aa7b

…tant

wcmjunior reviewed Aug 22, 2023

View reviewed changes

gengdahlCyral added 2 commits August 23, 2023 11:56

ENG-12292 - Review comments; added listener settings conflicts with v…

d0576c6

…alidation. Updated setupSidecarListenerConfig method to support constructing faulty configurations (switch -> multiple if statements)

ENG-12292 - comment update

40776c9

juniocezar reviewed Aug 23, 2023

View reviewed changes

juniocezar approved these changes Aug 23, 2023

View reviewed changes

VictorGFM reviewed Sep 11, 2023

View reviewed changes

gengdahlCyral added 2 commits September 11, 2023 13:33

ENG-12292 - Review comment: Fixed incorrect conflicts with clause

1519e70

ENG-12292 - Review comment: removed redundant schema flag (optional=f…

6643e84

…alse)

VictorGFM merged commit 79b4d46 into main Sep 12, 2023
3 checks passed

VictorGFM deleted the feature/ENG-12292 branch September 12, 2023 15:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENG-12292: Add: SQL Server settings (version field) #438

ENG-12292: Add: SQL Server settings (version field) #438

gengdahlCyral commented Aug 14, 2023 •

edited

Loading

yoursnerdly Aug 21, 2023

gengdahlCyral Aug 22, 2023

yoursnerdly Aug 21, 2023

gengdahlCyral Aug 22, 2023

yoursnerdly Aug 21, 2023

gengdahlCyral Aug 22, 2023

wcmjunior left a comment

gengdahlCyral commented Aug 23, 2023

juniocezar Aug 23, 2023

gengdahlCyral Sep 11, 2023

juniocezar left a comment

VictorGFM left a comment

VictorGFM Sep 11, 2023

gengdahlCyral Sep 11, 2023

VictorGFM Sep 11, 2023

gengdahlCyral Sep 11, 2023

VictorGFM Sep 11, 2023

gengdahlCyral Sep 11, 2023

VictorGFM Sep 11, 2023

gengdahlCyral Sep 12, 2023

sonarqubecloud bot commented Sep 12, 2023

		@@ -8,8 +8,9 @@ import (
		"github.com/google/uuid"
		"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"

ENG-12292: Add: SQL Server settings (version field) #438

ENG-12292: Add: SQL Server settings (version field) #438

Conversation

gengdahlCyral commented Aug 14, 2023 • edited Loading

Description of the change

Type of change

Checklists

Development

Code review

Testing

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

wcmjunior left a comment

Choose a reason for hiding this comment

gengdahlCyral commented Aug 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juniocezar left a comment

Choose a reason for hiding this comment

VictorGFM 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

sonarqubecloud bot commented Sep 12, 2023

gengdahlCyral commented Aug 14, 2023 •

edited

Loading