Hi Thorsten et all,
see the comments inline.
Detlef
Thorsten Frueauf wrote:
> Hi Detlef et al,
>
> since I am a bit swamped with work I will send my first round of review.
> It is incomplete, since I have still some file to look at. But here are
> my comments do far:
>
> - usr/src/cmd/ha-services/gds-agents/PostgreSQL/control_pgs.ksh
> * line 230, typo, substitute "file ,to get" with "file, to get".
>
>
Agreed
> * generic, you always have "rm ${LOGFILE} 2>/dev/null" for
> start_ssh_agent) and check_stdby), but do you really call
> log_message afterwards? Same is true for validate).
>
The start_ssh_agent is the first part in the start sequence. The
log_message will be called at the end of the start sequence. I double
check if I can remove it for the validate.
> * generic, you should substitute "rm" by "${RM}" to be explicit.
>
agreed.
>
> - usr/src/cmd/ha-services/gds-agents/PostgreSQL/functions.ksh
> * line 274, you say "Nodename of the standby host or the
> standby zone",
> but in the later code you assume it is a resolvable address.
> So I assume you do not want the value from /etc/nodename here?
> It could be any other valid IP that is staying with that node
> or zone
> (think about multi homed nodes/zones where the user has several
> networks dedicated for specific purposes)? Or do you assume this
> is a logical host?
> Maybe be more specific of what the STDBY_HOST really is?
>
I will find a better explanation
> * line 883, getent does not only check if the entry exists within
> /etc/inet/hosts - it does query any name service configured
> within
> /etc/nsswitch.conf. So the @user_action should not just mention
> /etc/hosts. Would recommend something like
> "Please add the host to one of your configured name services, so
> it can get listed with getent." or similar.
>
agreed
> * line 1484, typo, replace "because the an" with "because then an"
>
>
> * line 1570, reword @explanation text like
> "it is now killed by PMF using the configured stop signal."
>
>
> * line 1648, typo, "runningtogether" -> "running together"
>
> * line 1651, missing word, "trigger if" -> "trigger a restart if"
>
> * line 1671, typo, "running. if" -> "running. If"
>
> * line 1672, sentence hard to read. At least need comma after
> "without it, ...", but would recommend to be explicit and
> replace "it" with "ssh-agent".
>
> * line 1697, substitute "/tmp/<resourcename>" with
> "/tmp/<resourcename>-ssh"
>
> * line 1697, typo "contianing" -> "containing"
>
> * line 1880, double "are": "are are" -> "are"
>
> * line 1896, typo "passowrd" -> "password"
>
> * line 1903, missing parentheses $PGHOST -> ${PGHOST}
>
> * line 1955, typo "create an..." -> "Create an..."
>
>
The wording feedback will get resolved
> - usr/src/cmd/ha-services/gds-agents/PostgreSQL/functions_static.ksh
>
> * generic, start_ssh_agent() relies on a passphrase setup
> unencrypted within pgs_config? Is there no better way to
> treat this situation? The default permission for the
> pgs_config is 644. This would be a potential security issue.
>
> * it also seems that the parameter file permission is not
> guaranteed to be 600 for root, which again might expose the
> passphrase to anyone being able to access the pfile directory.
>
> * why not instruct the user to setup passwordless ssh by exchanging
> trusted public keys and having the necessary setup for
> authorized_keys?
> That would not expose a passphrase.
>
The passphrase is optional, and the code works perfect with the
authorized keys alone. so it is up to the user to configure the
passphrase. The passphrase was added to the code upon feedback as an
otional configuration. The administrator who prefers to go with
authorized keys and passwordless login between the PostgreSQL users is
free to do so. if he wants to add a passphrase to his keys he can do so
as well. so it is all about choice
I see the problem here, that the passphrase is readable at least for
root or the PostgreSQL user. But every simple encryption is easy
breakable, and does not add very much as well.
I will restrict the permissions of the parameter file to the PostgreSQL.
It has to be the PostgreSQL user sorry, otherwise I then would require a
passwordless login for the root user which I really want to avoid.
>
> Will send my comments for the missing files as soon as I can.
>
> Greets
> Thorsten
>
> Detlef Ulherr wrote:
>
>> Hi,
>>
>> I recently posted the webrev for the WAL file shipping integration. In
>> addition to this task I fixed the following bugs:
>>
>> * 6637843 Registration of the postgres agent fails if the nodelist
>> has two or more zone on the registering node
>> * 6608184 smf manifest should not depend on network/physical
>>
>>
>>
>> The webrev is available under:
>> http://cr.opensolaris.org/~ulherr/pgshotstandby/
>>
>> Any comments are highly appreciated.
>>
>> Kind Regards
>> Detlef
>>
>
>
--
*****************************************************************************
Detlef Ulherr
Staff Engineer Tel: (++49 6103) 752-248
Availability Engineering Fax: (++49 6103) 752-167
Sun Microsystems GmbH
Amperestra?e 6 mailto:detlef.ulherr at sun.com
63225 Langen http://www.sun.de/
*****************************************************************************
Sitz der Gesellschaft: Sun Microsystems GmbH, Sonnenallee 1, D-85551
Kirchheim-Heimstetten
Amtsgericht Muenchen: HRB 161028
Geschaeftsfuehrer: Thomas Schroeder, Wolfgang Engels, Dr. Roland Boemer
Vorsitzender des Aufsichtsrates: Martin Haering
*****************************************************************************
