Re: Allow file inclusion in pg_hba and pg_ident files

On Wed, Nov 16, 2022 at 10:53:02AM +0800, Julien Rouhaud wrote:
> While being the same inclusion infrastructure, it's likely that people will
> have different usage. I'm assuming that for GUCs the main usage is to have
> your automation tool put one of your template conf for instance (small_vm.conf,
> big_vm.conf or something like that) in an included directory, so you don't
> really need a lot of them. You can also rely on ALTER SYSTEM to avoid manually
> handling configuration files entirely.
>
> For authentication there are probably very different pattern. The use case I
> had when writing this patch is some complex application that relies on many
> services, each service having dedicated role and authentication, and services
> can be enabled or disabled dynamically pretty much anytime. I had to write
> code to merge possibly new entries with existing pg_hba/pg_ident configuration
> files. With this feature it would be much easier (and robust) to simply have a
> main pg_hba/pg_ident that includes some directory, and have the service
> enable/disable simply creates/removes a dedicated file for each service, and we
> then would usually have at least a dozen files there.
>
> I'm assuming people doing multi-tenant can also have similar usage.

So you main application for HBA/ident is include_dir/.. For GUCs, we
would just skip the directory if it has no files, but the directory
has to exist. Your patch is behaving the same.

>> but that does not
>> mean, it seems to me, that there should be an inconsistency in the way
>> we process those commands because one has implemented a feature but
>> not the other. On the contrary, I'd rather try to make them
>> consistent.
>
> You mean stopping at the first error, even if it's only for the view reporting?
> That will make the reload consistent, but the view will be a bit useless then.

Yep, I meant to stop the generation of the TokenizedAuthLines at the
first inclusion error by letting tokenize_auth_file() return a status
to be able to stop the recursions. But after some second-thoughts
pondering about this, I see why I am wrong and why you are right. As
you say, stopping the generation of the TokenizedAuthLines would just
limit the amount of data reported at once in the view, the golden rule
for HBA/ident being that we would reload nothing as long as there is
at least one error reported when parsing things. So giving more
control to tokenize_auth_file() to stop a recursion, say by making it
return a boolean status, makes little sense.

> It would be nice to have the information that an "include_if_exists" file
> didn't exist, but having a log-level message in the "error" column is a clear
> POLA violation. People will probably just do something like
>
> SELECT file_name, line_number, error FROM pg_hba_file_rules WHERE error IS NOT
> NULL;
>
> and report an error if any row is found. Having to parse the error field to
> know if that's really an error or not is going to be a huge foot-gun. Maybe we
> could indeed report the problem in err_msg but for include_if_exists display it
> in some other column of the view?

Hmm. One possibility would be to add a direct mention to the
"include", "include_dir" and "include_if_exists" clauses through
pg_hba_file_rules.type? I don't see how to do that without making the
patch much more invasive as per the existing separation between
tokenization and Hba/IdentLine filling, though, and perhaps the error
provided with the full path to the file would provide enough context
for one to know if the failure is happening on an included file for
database/user lists or a full file from an "include" clause. It also
means that include_if_exists remains transparent all the time.
Simpler may be for the best here, at the end.

By the way, I am wondering whether process_included_authfile() is
the most intuitive interface here. The only thing that prevents a
common routine to process the include commands is the failure on
GetConfFilesInDir(), where we need to build a TokenizedAuthLine when
the others have already done so tokenize_file_with_context(). Could
it be cleaner to have a small routine like makeTokenizedAuthLine()
that gets reused when we fail scanning a directory to build a
TokenizedAuthLine, in combination with a small-ish routine working on
a directory like ParseConfigDirectory() but for HBA/ident? Or we
could just drop process_included_authfile() entirely? On failure,
this would make the code do a next_line all the time for all the
include clauses.
--
Michael