agroal / pgagroal

High-performance connection pool for PostgreSQL
https://agroal.github.io/pgagroal/
BSD 3-Clause "New" or "Revised" License
684 stars 61 forks source link

[#459] Unknown tls value in server configuration fix #460

Closed ashu3103 closed 2 months ago

ashu3103 commented 3 months ago

WIP: Issue #459

Changed the value of unknown pointer in key_in_section to NULL where global=true (section is pgagroal/pgagroal-vault).

@jesperpedersen PTAL

jesperpedersen commented 3 months ago

struct server has a tls value...

ashu3103 commented 3 months ago

struct server has a tls value...

Yeah, but when trying to use tls = on in [primary] section, I am getting this warning.

jesperpedersen commented 3 months ago

I'll leave this to @fluca1978 -- we have .tls value for struct server so we need to parse it

ashu3103 commented 3 months ago

I'll leave this to @fluca1978 -- we have .tls value for struct server so we need to parse it

Okay, but why can't we parse it just like we parse "host" and "port" for server. Like -

else if (key_in_section("port", section, key, true, NULL))
   {
      if (as_int(value, &config->common.port))
      {
         unknown = true;
      }
   }
   else if (key_in_section("port", section, key, false, &unknown))
   {
      memcpy(&srv->name, section, strlen(section));
      if (as_int(value, &srv->port))
      {
         unknown = true;
      }
      atomic_store(&srv->state, SERVER_NOTINIT);
   }

Here, for global=true, we are passing NULL and for global=false we are passing &unknown

fluca1978 commented 2 months ago

Since the change is scoped within pgagroal_apply_vault_configuration (https://github.com/agroal/pgagroal/blob/master/src/libpgagroal/configuration.c#L4736), I think we can accept the proposed solution.

ashu3103 commented 2 months ago

Since the change is scoped within pgagroal_apply_vault_configuration (https://github.com/agroal/pgagroal/blob/master/src/libpgagroal/configuration.c#L4736), I think we can accept the proposed solution.

I have proposed the changes in pgagroal_apply_main_configuration, since I was facing the issue while trying to use tls for pgagroal--postgres server.

Kindly refer to this issue : https://github.com/agroal/pgagroal/issues/459

fluca1978 commented 2 months ago

Since the change is scoped within pgagroal_apply_vault_configuration (https://github.com/agroal/pgagroal/blob/master/src/libpgagroal/configuration.c#L4736), I think we can accept the proposed solution.

I have proposed the changes in pgagroal_apply_main_configuration, since I was facing the issue while trying to use tls for pgagroal--postgres server.

Kindly refer to this issue : #459

Sorry, I was mislead by another comment. I've checked out the pull request, and it works fine. Even setting tls in main section works fine, this can be easily tested with pgagroal-cli conf get tls or pgagroal-cli conf get server.primary.tls.

@jesperpedersen I think it is fine to merge.

jesperpedersen commented 2 months ago

Merged.

Thanks for your contribution !