Fix use :@ characters and add sub-section to configure username and password for sync_replica

[XaBbl4]

There is a problem with using :@ characters in the connection string to a sync_replica

For example, if the username contains the @ symbol: jonh@it-corp:qwe@rty@localhost:employee - the parser split it as:

username = jonh                                 // wrong, should be jonh@it-corp
password = <null>                               // wrong, should be qwe@rty
database = it-corp:qwe@rty@localhost:employee   // wrong, should be localhost:employee

or only the password contains the @ symbol: jonh:qwe@rty@db_alias, then:

username = jonh                     // is correct
password = qwe                      // wrong, should be qwe@rty
database = rty@localhost:employee   // wrong, should be localhost:employee

In addition to fix it, this PR add a subsection to the sync_replica configuration. You can specify a username and password without any character restrictions, or specify where they should be read: from the first non-empty line of the file or from an environment variable.

Now in the replication.conf can:

database = <master_path>
{
  # 1) old format with restrictions:
  #    @ can't be used in database
  #    : can't be used in username
  #    all characters can be used in password
  sync_replica = jonh@it-corp:qwe@rty@localhost:employee
    #  username = jonh@it-corp        // is correct
    #  password = qwe@rty             // is correct
    #  database = localhost:employee  // is correct

  # 2) new sub-stream config with plain username and password without restrictions (username can be : character)
  sync_replica = localhost/3051:employee
  {
    username = jonh:smith@it-corp
    password = qwe@rty
  }

  # 3) new sub-stream when read values from environment and file
  sync_replica = localhost/3052:employee
  {
    username_env = replication_user
    password_file = replication_pass
  }
}

[aafemt]

User names are SQL identifiers and cannot include @ without being delimited. You path just move problem from user part to database part (which also can include legal @). Instead you must properly parse delimiters.

[XaBbl4]

Therefore, a subsection for configuration has been added that completely eliminates this problem.

In the database part, the @ character is used less frequently (I can't imagine those who use it in the database name), but for the username we have already encountered this problem when specifying a department with @ in the username.

[dyemanov]

I'd suggest that if the new (sub-section) syntax is used for sync_replica, the database name is not parsed at all (and thus it may not contain username and/or password). In this case a database path containing @ will work perfectly. Is it already the case in your patch?

[XaBbl4]

I'd suggest that if the new (sub-section) syntax is used for sync_replica, the database name is not parsed at all (and thus it may not contain username and/or password). In this case a database path containing @ will work perfectly. Is it already the case in your patch?

Yes, if a sub-section is used, the database name is used as is, including any characters.

[dyemanov]

Any opinion whether it should be backported into v5?

[hvlad]

Where it should be documented ? I see no changes in replication.conf (at least).

Also, it is not completely clear how environment or file could be used - is it allowed to use username_file and\or password_env ?

I support backporting it into v5.

[hvlad]

Is it correct to read the file in advance, before check of key name ?

[XaBbl4]

I think so. Since the configuration is set only by the administrator, it doesn't matter when to read from the file, before or after the key check, but in this case there is no need for double checking the key.

But can be refactor this into a separate function if need?

[hvlad]

I think so. Since the configuration is set only by the administrator, it doesn't matter when to read from the file, before or after the key check, but in this case there is no need for double checking the key.

There will be confusion with error diags when wrong key name points to the non-existing file, for example.
Consider case: pasword_file = deleted_file_name

But can be refactor this into a separate function if need?

Sure.

[XaBbl4]

There will be confusion with error diags when wrong key name points to the non-existing file, for example.
Consider case: pasword_file = deleted_file_name

In this case, the administrator will get an error:
repl_database specifies missing or inaccessible file: deleted_file_name
which will force him to create this file anyway, and after that he will get an error:
repl_database specifies unknown parameter: pasword_file

After refactor the errors will be same, only in reverse order, first he will fix the parameter name, and then create the file.

But I agree, the option to check the parameter name before reading the file looks better, I will prepare a fix.

[XaBbl4]

Fixed in the commit ebfc854

[dyemanov]

I will add a description to replication.conf, thanks for the reminder.

[dyemanov]

Also, it is not completely clear how environment or file could be used - is it allowed to use username_file and\or password_env ?

Username / password (independently of each other) may be specified as either a string (key without suffix) or via envvar (key suffixed by _env) or via filename (key suffixed by _file). So both options in your example above are valid.

[dyemanov]

Please review: 3575d91

[XaBbl4]

Please review: 3575d91

That's all right. But now I've found out that there is uncertainty when several parameters with the same key are specified in a subsection, for example:

{
  username = jonh
  username_file = repl_user.txt
  password_env = FB_REPL_PWD
}

In this case, username will first be equal to jonh, and then read from the file and replaced in the order of the processing queue, either add this point to the description, or output an error when keys are duplicated.