Skip to content

fix overwriting of name field for IAM Group user in resourceSqlUserRead method #11466

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

Merged
merged 1 commit into from
Aug 22, 2024

Conversation

pratikgarg10
Copy link
Member

@pratikgarg10 pratikgarg10 commented Aug 16, 2024

Release Note Template for Downstream PRs (will be copied)

sql: fixed overwriting of `name` field for IAM Group user for resource `google_sql_user`

@github-actions github-actions bot requested a review from BBBmau August 16, 2024 01:15
Copy link

Hello! I am a robot. Tests will require approval from a repository maintainer to run.

@BBBmau, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look.

You can help make sure that review is quick by doing a self-review and by running impacted tests locally.

@pratikgarg10 pratikgarg10 changed the title Main fix overwriting of name field for IAM Group user in resourceSqlUserRead method Aug 16, 2024
@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 1 file changed, 5 insertions(+), 2 deletions(-))
google-beta provider: Diff ( 1 file changed, 5 insertions(+), 2 deletions(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 96
Passed tests: 82
Skipped tests: 14
Affected tests: 0

Click here to see the affected service packages
  • sql

$\textcolor{green}{\textsf{All tests passed!}}$

View the build log

Copy link

@BBBmau This PR has been waiting for review for 3 weekdays. Please take a look! Use the label disable-review-reminders to disable these notifications.

@pratikgarg10
Copy link
Member Author

Hi @BBBmau, Could you please take a look at this pull request for review?

@rileykarson rileykarson requested review from ScottSuarez and removed request for BBBmau August 21, 2024 20:31
@ScottSuarez
Copy link
Contributor

Is there a ticket for this issue or could you provide an explanation of the change? I am not familiar with this resource.

@ScottSuarez
Copy link
Contributor

Also if a testcase could be added showing exactly what was broken and is now fixed that would be great !

@pratikgarg10
Copy link
Member Author

Is there a ticket for this issue or could you provide an explanation of the change? I am not familiar with this resource.
Here is the issue: hashicorp/terraform-provider-google#17040

Issue:
Creating IAM group user resource fails with error "Root resource was present, │ but now absent. "

Explanation:
The code snippet in Condition 1 below truncates the SQL user name field and overwrites it if the condition is satisfied for any user in the users list, but later the comparison in Condition 2 will fail for valid SQL user name due to overwritten truncation of previous user in the list.

Condition 1:

if !(strings.Contains(databaseInstance.DatabaseVersion, "POSTGRES") || currentUser.Type == "CLOUD_IAM_GROUP") {
            name = strings.Split(name, "@")[0]
        }

Condition 2:

if currentUser.Name == name {
	// Host can only be empty for postgres instances,
	// so don't compare the host if the API host is empty.
	if host == "" || currentUser.Host == host {
		user = currentUser
		break
	}
}

During IAM group creation on the instance, the condition that triggers the issue is the naming convention used for the built-in users & IAM user/groups and corresponding alphabetical order. If the IAM group user comes last in the order, we'll see the issue.

@pratikgarg10
Copy link
Member Author

Also if a testcase could be added showing exactly what was broken and is now fixed that would be great !

It was tested locally, but difficult to reproduce via unit test

@ScottSuarez
Copy link
Contributor

Got it!! This change seems reasonable. We are currently frozen for 6.0 release but I'll approve this once things move along.

I'm going to leave this PR as unreviewed simply to maintain it in my review queue and that I don't lose track of it.

Copy link
Contributor

@ScottSuarez ScottSuarez left a comment

Choose a reason for hiding this comment

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

unfroze early.. merging now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants