Bug 833007

Created attachment 592615 [details]
[PATCH 1/4] added error checks and avoid access to the 'None' cds

This patch fixes the issue I reported.

Also, this patch try to detect cases if CDS (load balancers)
not found in the cds list file asap.


I'm not sure how to use conduit.error, so if that is wrong,
please replace that w/ yum.Errors.RepoError or sometihng
more suitable one.

Created attachment 592619 [details]
[PATCH 2/4] [] is evaluated as False in boolean context

This is a trivial patch to make it confirming to the style guide line:
http://code.google.com/p/soc/wiki/PythonStyleGuide#True_False_evaluations


That is, [] is evaluated as False in boolean context so len() is
not needed in actual.

Created attachment 592621 [details]
[PATCH 3/4] introduced _get_lb_from_file() to make it exceptional safe while loading CDS from list files

Currently, there is no try..catch clause while loading CDS from list file.
So there is a possibility of crash of yum if permission-denied or 
file-not-existent or something cause IO/OSError while loading it
due to operational mistake and so on.

This patch adds a utility function to make it exceptional safe and
just returns [] (empty cds list) if something goes wrong w/ list files.


And if [] is returned, administrators still can find such errors
as I added logging code to notify CDS list is empty in previous
patch (#1: 1/4), I guess.

Created attachment 592625 [details]
[PATCH 4/4] introduced a cached cds list file to save CDS found, and make it not trying to overwrite the cds list provided from RHUI and owned by client entitlement RPM package

This patch adds some experimental code to make CDS found but not 
in the CDS list provided by client entitlement RPMs originally
built w/ rhui-manager, into the other cached version of CDS list
(/var/cache/yum/rhui-load-balancers for example in this patch).

The file /etc/yum.repos.d/rhui-load-balancers is owned by
client entitlement RPMs and IMHO it is not intended to be 
overwritten.

Also, this patch makes use of newer 'with' statement can be 
run in python > 2.5 but it's not essential. So it should be
able to replaced w/ try..finally caluse if needed.

I think we could probably incorporate patches 1 and 2 in a future release.  

Patch 3 has the effect of hiding the error that caused the exception.  Since an error at this step is unlikely, I think we want to let the exception bubble up so that it's displayed to the user and we know what went wrong.  Although, we could most likely make that more user friendly. 

Patch 4, I'm not sure what the intent is.  Just to not modify the packaged file?  I actually think this is ok.  The rhui-load-balancers file should be considered like a config file.  We should make sure this is marked as %config in our spec file.  Also, we can't make use of the "with" keyword as we still have to support Python 2.4 which is what's available in RHEL 5.

Thanks a lot for reaviewing these!


Comments inline below.

> Patch 4, I'm not sure what the intent is.  Just to not modify the packaged file?

I intended that (not try to touch files owned by RPMs) in this patch.

> I actually think this is ok.  The rhui-load-balancers file should be 
> considered like a config file.  We should make sure this is marked as
> %config in our spec file.

IMHO, it does not look true, as far as my understanding and rpm in rhel 6.

Configuration files not marked as "noreplace" are owned by RPM packages
and user and program should not touch it as it is replaced anytime
the package updated in general (this behavior might change depends on
rpm version: rpm in rhel6 seems like so as far as I read its source code).

And if files may be modified, special flag '%verify(not md5 size mtime)'
should be needed for that files in RPM specs.

If user or program modified it, verification may be failed such like:

[root@rhel-6-client-0 ~]# rpm -qf /etc/yum.repos.d/rhui-load-balancers
rhui-client-entitlement-rhel-6-2.0-1.el6.noarch
[root@rhel-6-client-0 ~]# vim /etc/yum.repos.d/rhui-load-balancers
... (add CDS instance) ...
[root@rhel-6-client-0 ~]# rpm -V rhui-client-entitlement-rhel-6
S.5....T.  c /etc/yum.repos.d/rhui-load-balancers
[root@rhel-6-client-0 ~]#


Also, FHS says /etc is to keep *static* local files used to control
the operation of a program:
http://www.pathname.com/fhs/pub/fhs-2.3.html#ETCHOSTSPECIFICSYSTEMCONFIGURATION

(Unfortunatelly, there is some exceptional cases such like
/etc/blkid.tab, /etc/resolv.conf, /etc/mtab - but it's just a symlink to
/proc/mounts these days - because histrocal reasons or something, however.)


And I feel unconfortable program may change config files under /etc
anytime before we know it and it does not have special reason for that
such as historical limitation or something. IMHO, it should be placed
under /var if program may modify it as needed.

> Also, we can't make use of the "with" keyword as we still
> have to support Python 2.4 which is what's available in RHEL 5.

Sorry, I forgot it. Then, how about replace that with statements w/ 
try ... finally clause?

Confirmed patch #1 works..

Loaded plugins: product-id, rhui-lb, security, subscription-manager
Updating certificate-based repositories.
Unable to read consumer identity
Could not retrieve mirrorlist https://None/pulp/mirror/path/test error was
14: PYCURL ERROR 6 - "Couldn't resolve host 'None'"
repo id                                     repo name                                                          status
rhui-custom-10001                           Custom Repositories - 10001                                        0
rhui-rhui-2.0                               Red Hat Update Infrastructure 2.0 (RPMs)                           0
repolist: 0
[root@ip-10-190-159-157 ~]# patch   /usr/lib/yum-plugins/rhui-lb.py /root/yum.patch 
patching file /usr/lib/yum-plugins/rhui-lb.py
Hunk #1 succeeded at 49 (offset 2 lines).
Hunk #2 succeeded at 89 (offset 2 lines).
[root@ip-10-190-159-157 ~]# yum repolist
Loaded plugins: product-id, rhui-lb, security, subscription-manager
Updating certificate-based repositories.
Unable to read consumer identity
No CDS load balancers found in the lists.
rhui-custom-10001                                                                             | 2.6 kB     00:00     
rhui-custom-10001/primary_db                                                                  | 7.4 kB     00:00     
rhui-rhui-2.0                                                                                 | 2.6 kB     00:00     
rhui-rhui-2.0/primary_db                                                                      |  23 kB     00:00     
repo id                                     repo name                                                          status
rhui-custom-10001                           Custom Repositories - 10001                                         1
rhui-rhui-2.0                               Red Hat Update Infrastructure 2.0 (RPMs)                           37

sorry.. adding a snippet of yum working w/ an empty load balancer file 


[root@ip-10-190-159-157 ~]# cat /etc/yum.repos.d/rhui-load-balancers
[root@ip-10-190-159-157 ~]# yumdownloader gofer
Loaded plugins: product-id, rhui-lb
No CDS load balancers found in the lists.
gofer-0.64-1.el6.noarch.rpm

regarding patch #3

w/o patching...

[root@ip-10-190-159-157 yum.repos.d]# rm -Rf /etc/yum.repos.d/rhui-load-balancers
[root@ip-10-190-159-157 yum.repos.d]# yum repolist
Loaded plugins: product-id, rhui-lb, security, subscription-manager
Updating certificate-based repositories.
Unable to read consumer identity

[Errno 2] No such file or directory: '/etc/yum.repos.d/rhui-load-balancers'

** on a rhel6 client, this seems fine to me w/o a patch, also tested removing the contents of the file and patch#1 handled that.

+1 on the patch in https://bugzilla.redhat.com/show_bug.cgi?id=833007#c1

moving to version 2.1.1

Again, thanks for submitting these patches!

I've committed patches 1 and 2 to our cloude repo (bce25888274a6efe2b46c9a9b26d7de06cefa4ab).

Patch 3: I don't want to commit for the reason I mentioned above.  We want to fail in this case.  That file should never be missing, or unreadable.  If it is, we don't want the yum transaction to continue.  If the customer has some reason why this can't be, they can always disable the rhui-lb plugin to accomplish whatever they're trying to accomplish.

Patch 4: I understand your concerns about the violation of the FHS and modifying rpm owned files.  I'm not sure I know the full history as to why these choices were made initially for rhui-lb.  However, I don't want to introduce a lot of change, which would include migrations for existing customers unless their is a specific blocking issue we're  trying to solve.

[root@rhua ~]# rpm -q rh-rhui-tools
rh-rhui-tools-2.1.15-1.el6_3.noarch

[root@cds1 ~]# service pulp-cds stop
Stopping httpd:                                            [  OK  ]
Stopping goferd                                            [  OK  ]

[root@cli1 ~]# yum repolist
Loaded plugins: product-id, rhui-lb, security, subscription-manager
Updating certificate-based repositories.
Unable to read consumer identity


Could not contact any CDS load balancers: cds1.example.com.

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/RHBA-2013-0571.html