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

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

Merged
merged 1 commit into from
Sep 7, 2024

Conversation

ashu3103
Copy link
Collaborator

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
Copy link
Collaborator

struct server has a tls value...

@ashu3103
Copy link
Collaborator Author

struct server has a tls value...

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

@jesperpedersen
Copy link
Collaborator

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

@ashu3103
Copy link
Collaborator Author

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
Copy link
Collaborator

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.

Copy link
Collaborator

@fluca1978 fluca1978 left a comment

Choose a reason for hiding this comment

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

I think it is fine, since it is scoped within the vault configuration.

@ashu3103
Copy link
Collaborator Author

ashu3103 commented Sep 5, 2024

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

@fluca1978
Copy link
Collaborator

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 jesperpedersen merged commit 8b4b7ed into agroal:master Sep 7, 2024
2 checks passed
@jesperpedersen
Copy link
Collaborator

Merged.

Thanks for your contribution !

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