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

Fix checkUserExists functions #79

Closed
wants to merge 2 commits into from
Closed

Conversation

biofa98
Copy link

@biofa98 biofa98 commented Aug 17, 2022

This PR fixes an issue that occurs when removing an existing user from a group.
The error occurs during the terraform apply phase, the error that gets thrown is the following:

│ Error: error reading info about user: %!s(<nil>)
│
│   with module.groups["group_name"].redshift_group.group,
│   on ../../modules/group/main.tf line 1, in resource "redshift_group" "group":
│    1: resource "redshift_group" "group" {

Currently, the query used to check for the existence of a user is: SELECT 1 from pg_user_info WHERE usename=$1

The issue is that the usename is not enclosed in single quotes '', when manually executed this query causes redshift to return the following error:

db=# SELECT 1 from pg_user_info WHERE usename=existinguser;
ERROR:  column "existinguser" does not exist in pg_user_info

The same query executed with the usename enclosed in quotes returns the correct result:

db=# SELECT 1 from pg_user_info WHERE usename='existinguser';
?column?
----------
        1
(1 row)

@winglot
Copy link
Member

winglot commented Sep 1, 2022

Hi, thanks for your report.

Currently, the query used to check for the existence of a user is: SELECT 1 from pg_user_info WHERE usename=$1

The issue is that the usename is not enclosed in single quotes '', when manually executed this query causes redshift to return the following error:

db=# SELECT 1 from pg_user_info WHERE usename=existinguser;
ERROR:  column "existinguser" does not exist in pg_user_info

The same query executed with the usename enclosed in quotes returns the correct result:

db=# SELECT 1 from pg_user_info WHERE usename='existinguser';
?column?
----------
        1
(1 row)

This is all true about the quoting. Except this is not how the query is executed in the Go code. This is a prepared statement with one text parameter ($1). If you wish to simulate the same behavior, you would have to write it like this:

terraform_provider_tester=# PREPARE userExists(text) AS
terraform_provider_tester-# SELECT 1 from pg_user_info WHERE usename=$1;
PREPARE
terraform_provider_tester=# EXECUTE userExists('root');
 ?column?
----------
        1
(1 row)

terraform_provider_tester=# EXECUTE userExists('nonexisting');
 ?column?
----------
(0 rows)

which as you can see, works fine.

But not everything is lost. Thanks to your report I skimmed through the code and noticed the issue you are having is hiding just a few lines below your change in the checkIfUserExists function. There is this switch statement that makes absolutely no sense:

	switch err {
	case sql.ErrNoRows:
		return false, nil
	case nil:
		return false, fmt.Errorf("error reading info about user: %s", err)
	}

what the author (most likely me) meant to do here is:

	switch {
	case err == sql.ErrNoRows:
		return false, nil
	case err != nil:
		return false, fmt.Errorf("error reading info about user: %s", err)
	}

which, when fixed, makes removing existing users from groups possible.

I will close your PR and prepare a new one with a fixed switch statement and additional tests.

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

Successfully merging this pull request may close these issues.

3 participants