Bug 887815

Summary: a comprehensive keystone.conf file should be included in the RPMS(s)
Product: Red Hat OpenStack Reporter: Nikola Dipanov <ndipanov>
Component: openstack-keystoneAssignee: Alan Pevec <apevec>
Status: CLOSED ERRATA QA Contact: Pavel Sedlák <psedlak>
Severity: medium Docs Contact:
Priority: high    
Version: 1.0 (Essex)CC: apevec, ayoung, dyocum, eglynn, markmc, pbrady
Target Milestone: snapshot5Keywords: Reopened, Triaged
Target Release: 2.1   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: openstack-keystone-2012.2.3-6.el6ost Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: 887334 Environment:
Last Closed: 2013-04-04 20:22:41 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 892420, 909013    
Attachments:
Description Flags
dist-git patch
none
dist-git patch
ayoung: review+
checks if keystone.conf has precedence against keystone-dist.conf none

Description Nikola Dipanov 2012-12-17 12:01:48 UTC
+++ This bug was initially created as a clone of Bug #887334 +++

Description of problem:

A .conf file with a comprehensive set of configuration options should be shipped various nova services.  Here is a good example to work from:

https://github.com/yocum137/nova.conf/blob/master/nova.conf

--- Additional comment from Dan Yocum on 2012-12-14 12:22:34 EST ---

This one looks like it's more up-to-date:

https://github.com/openstack/nova/blob/master/etc/nova/nova.conf.sample

Comment 1 Alan Pevec 2012-12-18 12:39:17 UTC
Unlike Nova, Keystone RPM is not shipping own copy of keystone.conf, it edits https://github.com/openstack/keystone/blob/master/etc/keystone.conf.sample
so doc comments are preserved.

Comment 3 Mark McLoughlin 2013-01-16 16:05:48 UTC
See bug #887804 for the conclusions we came to about Cinder

For keystone, things are a bit different

  1) The defaults which we change using openstack-config e.g.:

       openstack-config --set etc/keystone.conf DEFAULT log_file %{_localstatedir}/log/keystone/keystone.log

     and the PasteDeploy config (starting with [filter:debug]) should go
     into a /usr/share/keystone/keystone-dist.conf file and the 
     systemd units and init scripts will need to be updated to do:

       --config-file=/usr/share/keystone/keystone-dist.conf --config-file=/etc/keystone/keystone.conf

     This means that users can start with an empty /etc/keystone/keystone.conf
     file and still be using the distribution defaults

     This file should not be marked as %config(noreplace) since it is not
     intended to be user-editable

     One thing to watch out for here - keystone-all uses pastedeploy to load
     the wsgi apps from config_file[0] so a user won't be able to change the
     wsgi config by editing /etc/keystone/keystone.conf

     I see keystone uses /usr/share/openstack-keystone rather than
     /usr/share/keystone. I guess I'd prefer the latter for consistency with
     e.g. /etc/keystone but it's a question of whether it's safe to change
     now. Not a big deal, either is fine)

2-4) Modulo the PasteDeploy stuff, upstream's keystone.conf.sample contains 
     nothing but comments explaining what options are available. That's exactly
     what we want for /etc/keystone/keystone.conf

  5) In the productized packages, we should strip the sample keystone.conf to
     only include options we recommend using and perhaps improve the 
     descriptions.

  6) N/A

Comment 4 Mark McLoughlin 2013-01-17 22:26:23 UTC
From apevec - "One thing to verify is Nova selinux policy: /etc/nova/nova.conf is etc_t while files under /usr/share/nova/ get usr_t"

Comment 5 Alan Pevec 2013-02-18 11:21:46 UTC
While at it, paste-deploy config should be moved to a separate ini file like in other OpenStack services.

Backport of this recent upstream change https://review.openstack.org/#/c/20505
should help, along with introducing paste_deploy.config_file parameter like in Glance (instead of assuming first .conf or hardcoded 'keystone.conf' is a paste-deploy ini file)

Comment 6 Alan Pevec 2013-03-12 22:04:06 UTC
(In reply to comment #3)
>      and the PasteDeploy config (starting with [filter:debug]) should go
>      into a /usr/share/keystone/keystone-dist.conf file and the 

With the backport of https://review.openstack.org/24126 we can move PasteDeploy config part to a separate /usr/share/keystone/keystone-paste.ini

>      One thing to watch out for here - keystone-all uses pastedeploy to load
>      the wsgi apps from config_file[0] so a user won't be able to change the
>      wsgi config by editing /etc/keystone/keystone.conf

Above patch introduces paste_deploy.config_file parameter, so user would be able to change wsgi config but then they're on their own.

Comment 7 Alan Pevec 2013-03-13 02:48:51 UTC
Created attachment 709299 [details]
dist-git patch

Comment 8 Alan Pevec 2013-03-13 03:23:35 UTC
Created attachment 709310 [details]
dist-git patch

Comment 9 Pádraig Brady 2013-03-13 09:46:32 UTC
I had a quick look at the corresponding -patches change for this:
https://code.engineering.redhat.com/gerrit/gitweb?p=keystone.git;a=commit;h=069a57e
I notice that the [sql].connection var is commentd but still set to the
upstream default. Ideally that value would be left commented and
updated automatically at rpm build time from the corresponding -dist.conf

On a related note, cli tools may not be aware of the -dist.conf
and so maybe fail to sync the DB etc? Is there a requirement to
auto add -dist.conf to the config set? as was done in this nova patch for example:
https://code.engineering.redhat.com/gerrit/gitweb?p=nova.git;a=commit;h=634259d

Comment 10 Alan Pevec 2013-03-13 11:58:41 UTC
(In reply to comment #9)
> I notice that the [sql].connection var is commentd but still set to the
> upstream default. Ideally that value would be left commented and
> updated automatically at rpm build time from the corresponding -dist.conf

Indeed, I missed point 5) in comment 3. I'll look how is that handled in nova/cinder.

> On a related note, cli tools may not be aware of the -dist.conf
> and so maybe fail to sync the DB etc? Is there a requirement to
> auto add -dist.conf to the config set?

yes, I need to fix keystone-manage

Comment 11 Alan Pevec 2013-03-13 18:21:49 UTC
Follow up spec patch:
-%{_datadir}/keystone
+%dir %{_datadir}/keystone
+%attr(0640, root, keystone) %{_datadir}/keystone/keystone-dist.conf
+%attr(0640, root, keystone) %{_datadir}/keystone/keystone-dist-paste.ini
+%attr(0755, root, root) %{_datadir}/keystone/sample_data.sh
+%attr(0644, root, root) %{_datadir}/keystone/%{name}.upstart

Comment 13 Pavel Sedlák 2013-03-19 15:59:07 UTC
Maybe I do not understand it right, so please correct me if I'm mistaken, but following parts looks wrong.
Snippets from /etc/keystone/keystone.conf.


------
# Name of log file to output to. If not set, logging will go to stdout.
# log_file = /var/log/keystone/keystone.log
------
But default (at least for me) in /usr/.../keystone-dist.conf is set to /var/log/keystone/keystone.log.
So for user "If not set" actually means that log file, not stdout.
I think better could be "if set to empty" etc.


------
[sql]
# The SQLAlchemy connection string used to connect to the database
# connection = mysql://keystone:keystone@localhost/keystone
connection = mysql://keystone_admin:ks123456.33.11/keystone
------
Connection is set here, and also in -dist.conf:
  connection = mysql://keystone:keystone@localhost/keystone
Why is the commented-out line left in place and why the correct value (the one in etc) is not placed in -dist.conf?


Same goes for [catalog]-driver:
 - set in both places to same value
 - not commented-out in /etc
 - original original comment/example left in /etc (so there are two driver= lines)


------
[paste_deploy]
# Name of the paste configuration file that defines the available pipelines
------ (EOF)
Shouldn't be there an example like "# config_file = ..."?

Comment 14 Pádraig Brady 2013-03-20 11:42:57 UTC
Valid point about "If not set, logging will go to stdout."
These comments are generated upstream, automatically from source,
so the best fix would be to adjust comments so they don't assume
a single point of config. There are various issues with those
logfile comments actually, which I've addressed upstream with:
  https://review.openstack.org/24889
So the fix will bubble up at some stage, and if we think
important enough to expediate we can propose that to rh-folsom-rhel-6-patches.

*-dist.conf are the distribution defaults. I.E. not editable by users.
These defaults are reflected by the commented values in the user editable files.
config options are cumulative, with /etc overriding /usr.
Installation specific config changes should only be done in /etc.
Whether the commented line is left in place, is debatable.
I think it's OK though as it does document the dist default
in a single place.

I agree that the # config_file should have a comment in place,
Maybe that's also needed in the corresponding upstream change:
https://review.openstack.org/#/c/24126

Comment 15 Pavel Sedlák 2013-03-20 17:17:47 UTC
Created attachment 713375 [details]
checks if keystone.conf has precedence against keystone-dist.conf

Comment 16 Pavel Sedlák 2013-03-20 17:20:22 UTC
Verified that /etc/keystone/keystone.conf contains options with descriptions as keystone.conf.example 
provided by upstream also default distribution and paste/pipeline configs are in separate files.

Old version used: openstack-keystone-2012.2.1-3.el6ost
Verified in version: openstack-keystone-2012.2.3-6.el6ost

In old version there was /usr/share/openstack-keystone now it is /usr/share/keystone as mentioned.

In new version:
  - /usr/share/keystone/openstack-keystone.upstart was changed and it contains two --config paths (/etc and -dist ones)
  - /etc/init.d/openstack-keystone also specifies two --config paths 
  - /usr/share/keystone/ contains new keystone-dist.conf and keystone-dist-paste.ini
  - - in keystone-dist.conf there is [paste-deploy]/config_file path pointing to keystone-dist-paste.ini
  - - keystone-dist-paste.ini contains pipeline settings (unlike keystone.conf or keystone-dist.conf)

Tested precedence of keystone.conf and keystone-dist.conf with attached bash script:
 - adds admin_token to dist.conf and uses original one to authenticate
 - disables original token and uses the newly added (from dist.conf) to authenticate
 - script also verifies that dist.conf is replaced (new token cleaned) when package is reinstalled

Also when only working SQL connection is specified only in one of keystone.conf or keystone.dist.conf keystone-manage db_sync works - so is able to use both of them (compared to when there no valid connection specified).

Comment 18 errata-xmlrpc 2013-04-04 20:22:41 UTC
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

http://rhn.redhat.com/errata/RHSA-2013-0708.html