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

Request to accept the username and password as two different arguments for db.open function. #33

Closed
vamsidarbhamulla opened this issue Feb 17, 2023 · 4 comments

Comments

@vamsidarbhamulla
Copy link

vamsidarbhamulla commented Feb 17, 2023

Hi,

I'm trying to connect to aws rds based postgres instance with a password string containing a good number of special characters.

I did observe an error as net/url: invalid userinfo while building the xk6-sql extension and providing the connection string.

After a bit of research I'm able to fix my issue with a local build of xk6-sql by following the below two suggestions:

I'm pretty new to golang ecosystem so requesting to get the same code changes on the actual repo if it makes sense.

// need to add two more arguments for username and password
func (*SQL) Open(database string, connectionString string, username string, password string) (*dbsql.DB, error) {
	supportedDatabases := []string{"mysql", "postgres", "sqlite3", "sqlserver"}
	if !contains(supportedDatabases, database) {
		return nil, fmt.Errorf("database %s is not supported", database)
	}

   // modified code suggestion start
    connectionUrl, connErr := url.Parse(connectionString)
    if connErr != nil {
         fmt.Errorf("ERROR: %v\n", connErr)
         return nil, connErr
    }
    connectionUrl.User = url.UserPassword(username, password)

	db, err := dbsql.Open(database, connectionUrl.String())
	// modified code suggestion end 
	if err != nil {
		return nil, err
	}

	return db, nil
}
@imiric
Copy link
Contributor

imiric commented Feb 17, 2023

Hi there!

If you're using special characters in your connection string, you need to URL encode them first.

You can use the builtin encodeURIComponent() function for this. For example:

const pass = encodeURIComponent('pass[@#$%^&]');
const db = sql.open('postgres', `postgres://user:${pass}@127.0.0.1:5432/postgres?sslmode=disable`);

I confirmed this works on a local Postgres instance, so it should work with AWS RDS as well.

So we don't need to add separate arguments for this, even though it wouldn't be a breaking change.

@imiric imiric closed this as not planned Won't fix, can't repro, duplicate, stale Feb 17, 2023
@vamsidarbhamulla
Copy link
Author

Hi,

Thanks for the quick response. Unfortunately, I could not make it work either with encodeURIComponent() or even by providing an already encoded password directly.

The only observed difference so far I can see is that encodeURIComponent function did not encode these two characters !) but even after replacing them with the actual encoded values it did not work for me.

I would prefer to use my local build with username and password as different arguments on the open function in the golang sql.go script as that is the only approach I can make it work.

Thanks again for your feedback. I really appreciate it.

@imiric
Copy link
Contributor

imiric commented Feb 21, 2023

@vamsidarbhamulla Sorry to read that, but I think there's another reason you're getting the invalid userinfo. You're right that the !) characters don't get encoded, but from my tests with a local Postgres instance, they don't need to be. I can connect fine by either encoding them or not.

Can you test with plain Postgres and not RDS, to confirm if it's an RDS issue? I run a Docker container with:

docker run --rm --name postgres -p 127.0.0.1:5432:5432 -e POSTGRES_USER=user -e POSTGRES_PASSWORD='pass[@#$%^&!)]' postgres:14.4

The reason I'm reluctant to do this change is because db.open is not just used for Postgres. While technically JavaScript allows calling functions with less arguments than defined, and it likely wouldn't be breaking change for k6 scripts, we would need to ensure it works well for other RDBMSs, and it's not such a trivial change.

You could just keep using your fork, but I don't see a reason why encodeURIComponent() shouldn't work in this case.

@vamsidarbhamulla
Copy link
Author

Hi @imiric - I did test it with plain postgres docker setup and I'm able to interact with the local docker DB. Not sure what exactly is the issue with RDS setup and the inavlid userinfo error.

I totally agree with the reasoning behind not modifying the current code changes. I will keep using the fork. Thanks for the response.

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

No branches or pull requests

2 participants