Re: [Freeipa-devel] [PATCH 506] cert renewal: make renewal of ipaCert atomic

[Jan Cholasta]

On 19.11.2015 13:01, David Kupka wrote:

On 18/11/15 14:10, Jan Cholasta wrote:

On 10.11.2015 19:19, Rob Crittenden wrote:

Jan Cholasta wrote:

On 9.11.2015 16:51, Rob Crittenden wrote:

Jan Cholasta wrote:

Hi,

the attached patch fixes
.

Honza





There be a note in renew_ra_cert that the lock is obtained in
advance by
renew_ra_cert_pre.


Added comment.



It looks like it will silently fail if the lock cannot be acquired. Is
that desired?


All unhandled exceptions are logged to syslog in both renew_ra_cert_pre
and renew_ra_cert:

 try:
 main()
 except Exception:
 syslog.syslog(syslog.LOG_ERR, traceback.format_exc())

Updated patch attached.



My confusion was with the auto-expiration. I guess this is ok. When
debugging this sort of thing via logs the more the merrier, so I guess
I'd have added a syslog to say "obtaining lock" or "locked" and then
something when the renewal actually starts, so one can try to piece
together what happened after the fact if something goes wrong.

I guess certmonger already logs when a pre/post command is executed so
that may already be available.


Yes. The ticket is not related to logging anyway.

Is the last patch OK, then?



Thanks for the patch. Works for me, ACK.


Pushed to:
master: f3076c6ab37e081ba9b0ec9f0502379f60dfbd10
ipa-4-2: f831cb6a3da0c5f2a3e71004ae327273b25723fa

--
Jan Cholasta

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 506] cert renewal: make renewal of ipaCert atomic

[David Kupka]

On 18/11/15 14:10, Jan Cholasta wrote:

On 10.11.2015 19:19, Rob Crittenden wrote:

Jan Cholasta wrote:

On 9.11.2015 16:51, Rob Crittenden wrote:

Jan Cholasta wrote:

Hi,

the attached patch fixes
.

Honza





There be a note in renew_ra_cert that the lock is obtained in
advance by
renew_ra_cert_pre.


Added comment.



It looks like it will silently fail if the lock cannot be acquired. Is
that desired?


All unhandled exceptions are logged to syslog in both renew_ra_cert_pre
and renew_ra_cert:

 try:
 main()
 except Exception:
 syslog.syslog(syslog.LOG_ERR, traceback.format_exc())

Updated patch attached.



My confusion was with the auto-expiration. I guess this is ok. When
debugging this sort of thing via logs the more the merrier, so I guess
I'd have added a syslog to say "obtaining lock" or "locked" and then
something when the renewal actually starts, so one can try to piece
together what happened after the fact if something goes wrong.

I guess certmonger already logs when a pre/post command is executed so
that may already be available.


Yes. The ticket is not related to logging anyway.

Is the last patch OK, then?



Thanks for the patch. Works for me, ACK.

--
David Kupka

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 506] cert renewal: make renewal of ipaCert atomic

[Jan Cholasta]

On 10.11.2015 19:19, Rob Crittenden wrote:

Jan Cholasta wrote:

On 9.11.2015 16:51, Rob Crittenden wrote:

Jan Cholasta wrote:

Hi,

the attached patch fixes .

Honza





There be a note in renew_ra_cert that the lock is obtained in advance by
renew_ra_cert_pre.


Added comment.



It looks like it will silently fail if the lock cannot be acquired. Is
that desired?


All unhandled exceptions are logged to syslog in both renew_ra_cert_pre
and renew_ra_cert:

 try:
 main()
 except Exception:
 syslog.syslog(syslog.LOG_ERR, traceback.format_exc())

Updated patch attached.



My confusion was with the auto-expiration. I guess this is ok. When
debugging this sort of thing via logs the more the merrier, so I guess
I'd have added a syslog to say "obtaining lock" or "locked" and then
something when the renewal actually starts, so one can try to piece
together what happened after the fact if something goes wrong.

I guess certmonger already logs when a pre/post command is executed so
that may already be available.


Yes. The ticket is not related to logging anyway.

Is the last patch OK, then?

--
Jan Cholasta

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 506] cert renewal: make renewal of ipaCert atomic

[Rob Crittenden]

Jan Cholasta wrote:
> On 9.11.2015 16:51, Rob Crittenden wrote:
>> Jan Cholasta wrote:
>>> Hi,
>>>
>>> the attached patch fixes .
>>>
>>> Honza
>>>
>>>
>>>
>>
>> There be a note in renew_ra_cert that the lock is obtained in advance by
>> renew_ra_cert_pre.
> 
> Added comment.
> 
>>
>> It looks like it will silently fail if the lock cannot be acquired. Is
>> that desired?
> 
> All unhandled exceptions are logged to syslog in both renew_ra_cert_pre
> and renew_ra_cert:
> 
> try:
> main()
> except Exception:
> syslog.syslog(syslog.LOG_ERR, traceback.format_exc())
> 
> Updated patch attached.
> 

My confusion was with the auto-expiration. I guess this is ok. When
debugging this sort of thing via logs the more the merrier, so I guess
I'd have added a syslog to say "obtaining lock" or "locked" and then
something when the renewal actually starts, so one can try to piece
together what happened after the fact if something goes wrong.

I guess certmonger already logs when a pre/post command is executed so
that may already be available.

rob

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 506] cert renewal: make renewal of ipaCert atomic

[Jan Cholasta]

On 9.11.2015 16:51, Rob Crittenden wrote:

Jan Cholasta wrote:

Hi,

the attached patch fixes .

Honza





There be a note in renew_ra_cert that the lock is obtained in advance by
renew_ra_cert_pre.


Added comment.



It looks like it will silently fail if the lock cannot be acquired. Is
that desired?


All unhandled exceptions are logged to syslog in both renew_ra_cert_pre 
and renew_ra_cert:


try:
main()
except Exception:
syslog.syslog(syslog.LOG_ERR, traceback.format_exc())

Updated patch attached.

--
Jan Cholasta
From a1beeccedf53678e1522002b366d2d7e7a4f447f Mon Sep 17 00:00:00 2001
From: Jan Cholasta 
Date: Mon, 9 Nov 2015 10:53:02 +0100
Subject: [PATCH] cert renewal: make renewal of ipaCert atomic

This prevents errors when renewing other certificates during the renewal of
ipaCert.

https://fedorahosted.org/freeipa/ticket/5436
---
 install/restart_scripts/Makefile.am   |  1 +
 install/restart_scripts/renew_ra_cert |  5 -
 install/restart_scripts/renew_ra_cert_pre | 18 ++
 ipaserver/install/cainstance.py   |  2 +-
 ipaserver/install/server/upgrade.py   |  4 ++--
 5 files changed, 26 insertions(+), 4 deletions(-)
 create mode 100755 install/restart_scripts/renew_ra_cert_pre

diff --git a/install/restart_scripts/Makefile.am b/install/restart_scripts/Makefile.am
index 58057aa..c4bf819 100644
--- a/install/restart_scripts/Makefile.am
+++ b/install/restart_scripts/Makefile.am
@@ -7,6 +7,7 @@ app_DATA =  \
 	renew_ca_cert			\
 	renew_ra_cert			\
 	stop_pkicad			\
+	renew_ra_cert_pre		\
 	$(NULL)
 
 EXTRA_DIST =\
diff --git a/install/restart_scripts/renew_ra_cert b/install/restart_scripts/renew_ra_cert
index cf770a9..9b5e231 100644
--- a/install/restart_scripts/renew_ra_cert
+++ b/install/restart_scripts/renew_ra_cert
@@ -77,8 +77,11 @@ def _main():
 
 
 def main():
-with certs.renewal_lock:
+try:
 _main()
+finally:
+# lock acquired in renew_ra_cert_pre
+certs.renewal_lock.release('renew_ra_cert')
 
 
 try:
diff --git a/install/restart_scripts/renew_ra_cert_pre b/install/restart_scripts/renew_ra_cert_pre
new file mode 100755
index 000..d0f743c
--- /dev/null
+++ b/install/restart_scripts/renew_ra_cert_pre
@@ -0,0 +1,18 @@
+#!/usr/bin/python2 -E
+#
+# Copyright (C) 2015  FreeIPA Contributors see COPYING for license
+#
+
+import syslog
+import traceback
+
+from ipaserver.install import certs
+
+
+def main():
+certs.renewal_lock.acquire('renew_ra_cert')
+
+try:
+main()
+except Exception:
+syslog.syslog(syslog.LOG_ERR, traceback.format_exc())
diff --git a/ipaserver/install/cainstance.py b/ipaserver/install/cainstance.py
index 23fdf30..1cbc0d0 100644
--- a/ipaserver/install/cainstance.py
+++ b/ipaserver/install/cainstance.py
@@ -1339,7 +1339,7 @@ class CAInstance(DogtagInstance):
 pin=None,
 pinfile=paths.ALIAS_PWDFILE_TXT,
 secdir=paths.HTTPD_ALIAS_DIR,
-pre_command=None,
+pre_command='renew_ra_cert_pre',
 post_command='renew_ra_cert')
 except RuntimeError as e:
 self.log.error(
diff --git a/ipaserver/install/server/upgrade.py b/ipaserver/install/server/upgrade.py
index 4337995..b9621a3 100644
--- a/ipaserver/install/server/upgrade.py
+++ b/ipaserver/install/server/upgrade.py
@@ -806,7 +806,7 @@ def certificate_renewal_update(ca):
 dogtag_constants = dogtag.configured_constants()
 
 # bump version when requests is changed
-version = 3
+version = 4
 requests = (
 (
 dogtag_constants.ALIAS_DIR,
@@ -844,7 +844,7 @@ def certificate_renewal_update(ca):
 paths.HTTPD_ALIAS_DIR,
 'ipaCert',
 'dogtag-ipa-ca-renew-agent',
-None,
+'renew_ra_cert_pre',
 'renew_ra_cert',
 None,
 ),
-- 
2.4.3

From d19a34271a779eee62f30c479b20adf502c751e5 Mon Sep 17 00:00:00 2001
From: Jan Cholasta 
Date: Mon, 9 Nov 2015 10:53:02 +0100
Subject: [PATCH] cert renewal: make renewal of ipaCert atomic

This prevents errors when renewing other certificates during the renewal of
ipaCert.

https://fedorahosted.org/freeipa/ticket/5436
---
 install/restart_scripts/Makefile.am   |  1 +
 install/restart_scripts/renew_ra_cert |  5 -
 install/restart_scripts/renew_ra_cert_pre | 18 ++
 ipaserver/install/cainstance.py   |  2 +-
 ipaserver/install/server/upgrade.py   |  4 ++--
 5 files changed, 26 insertions(+), 4 deletions(-)
 create mode 100755 install/restart_scripts/renew_ra_cert_pre

diff --git a/install/restart_scripts/Makefile.am b/install/restart_scripts/Makefile.am
index 58057aa..c4bf819 100644
--- a/install/restart_scripts/Makefile.am
+++ b/install/restart_scripts/Makefile.am
@@ -7,6 +7,7 @@ app_DATA =  \
 	renew_ca_cert	

Re: [Freeipa-devel] [PATCH 506] cert renewal: make renewal of ipaCert atomic

[Rob Crittenden]

Jan Cholasta wrote:
> Hi,
> 
> the attached patch fixes .
> 
> Honza
> 
> 
> 

There be a note in renew_ra_cert that the lock is obtained in advance by
renew_ra_cert_pre.

It looks like it will silently fail if the lock cannot be acquired. Is
that desired?

rob

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code