Re: [SSSD] [PATCH] Make offline status backend global
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 09/13/2009 10:31 AM, Simo Sorce wrote: With this patch all the backend providers can share the same offline status. This means composite backends like what AD or IPA will be that use a mix of ldap and krb can share the offline status for both protocols. A further extension might allow to have per protocol online/offline status, that will be done eventually when we integrate also the DNS discovery options. Tested with ldap_id+ldap_auth and ldap_id+krb5_auth Simo. ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel Just a nitpick, but why did you replace sbus_conn_send_reply() in be_check_online with sbus_get_connection and dbus_connection_send()? They are functionally identical. (except that sbus_conn_send_reply() can get the connection in one fewer stack frame, since it can access the sbus_connection object directly) Assuming I'm reading this correctly, we're talking about considering a single backend process as being online or offline as a whole. Why is this dependent only on the ID provider for the backend? Shouldn't we consider that if the authentication module or password change modules are offline that we are offline? Furthermore, even if the ID provider is offline, if we have cached user information that allows us to initiate a connection to a still-live authentication provider, isn't that perfectly reasonable? I'd argue that if any ONE of the modules was the ultimate determinator of online/offline status, it should be authentication rather than identification. Code itself is sensible, so this is a Nack until you can convince me that the approach itself is right. - -- Stephen Gallagher RHCE 804006346421761 Looking to carve out IT costs? www.redhat.com/carveoutcosts/ -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.9 (GNU/Linux) Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/ iEYEARECAAYFAkquLYgACgkQeiVVYja6o6Nm+gCdErNOsdFbMcZQKF5RlUhOMxhj gscAmgJnBx4a8s6XMD1QPeezFuiKVv72 =Oh2l -END PGP SIGNATURE- ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] Make offline status backend global
On Mon, 2009-09-14 at 07:48 -0400, Stephen Gallagher wrote:
Just a nitpick, but why did you replace sbus_conn_send_reply() in
be_check_online with sbus_get_connection and dbus_connection_send()?
They are functionally identical. (except that sbus_conn_send_reply()
can
get the connection in one fewer stack frame, since it can access the
sbus_connection object directly)
I merged together what previously was an async request.
It was only asking the id provider about offline status.
Since now the offline status is directly accessible by the
dp_provider_be.c there is no need to make a request to any provider, we
directly return the answer to dp. (this call is not used by anything
anyway so far but we were planning to use it to force a backend to go
offline so it will come handy later on).
Assuming I'm reading this correctly, we're talking about considering a
single backend process as being online or offline as a whole. Why is
this dependent only on the ID provider for the backend?
It isn't, attached new patch that add be_mark_offline() calls also to
the auth backends.
Shouldn't we
consider that if the authentication module or password change modules
are offline that we are offline?
yes, should be fixed in the new patch, now.
Furthermore, even if the ID provider is offline, if we have cached
user
information that allows us to initiate a connection to a still-live
authentication provider, isn't that perfectly reasonable?
No, also because this is just an initial patch to start building
infrastructure. If you remember we discussed the idea or allowing the
monitor to put a backend forcibly offline, in that case all providers
must respect this. Further more during auth we want to always refresh
users, so if the id part is not available we can as well auth from the
cached password (if caching passwords is allowed).
This infrastructure is also need for DNS discovery later, where we want
a central place to tell a specific server is unreachable.
In any case I would rather put the infrastructure in place now, and
tweak specific behaviors later.
I'd argue that if any ONE of the modules was the ultimate determinator
of online/offline status, it should be authentication rather than
identification.
Nope the auth module contacts the servers more rarely than the id
backend (auths are rare compared to requests to get id information), so
normally the ID backend is more qualified. In any case I am not going to
make any provider king, all of them should be able to signal that
servers are unreachable.
Code itself is sensible, so this is a Nack until you can convince me
that the approach itself is right.
Let's see if the new patch and explanations are enough :)
Simo.
--
Simo Sorce * Red Hat, Inc * New York
From a0b02295b0e0c55174b37ad3b231d28e0e1d1f50 Mon Sep 17 00:00:00 2001
From: Simo Sorce sso...@redhat.com
Date: Sat, 12 Sep 2009 00:05:55 -0400
Subject: [PATCH] Make the offline status backend-global
Add helpers functions to query/set the offline status per backend.
Now all providers share the same offline status.
---
server/providers/data_provider_be.c | 117 +-
server/providers/dp_backend.h | 11 +++-
server/providers/krb5/krb5_auth.c | 17 -
server/providers/ldap/ldap_auth.c | 29 +++--
server/providers/ldap/ldap_id.c | 55 ++--
server/providers/proxy.c| 80
6 files changed, 94 insertions(+), 215 deletions(-)
diff --git a/server/providers/data_provider_be.c b/server/providers/data_provider_be.c
index 55fc278..2e83ab6 100644
--- a/server/providers/data_provider_be.c
+++ b/server/providers/data_provider_be.c
@@ -133,66 +133,39 @@ static int be_file_request(struct be_ctx *ctx,
return EOK;
}
-static void online_chk_callback(struct be_req *req, int status,
-const char *errstr)
-{
-struct be_online_req *oreq;
-DBusMessage *reply;
-DBusConnection *dbus_conn;
-dbus_bool_t dbret;
-dbus_uint16_t online;
-dbus_uint16_t err_maj = 0;
-dbus_uint32_t err_min = 0;
-const char *err_msg = Success;
-
-oreq = talloc_get_type(req-req_data, struct be_online_req);
-if (status != EOK) {
-online = MOD_OFFLINE;
-err_maj = DP_ERR_FATAL;
-err_min = status;
-err_msg = errstr;
-}
-
-online = oreq-online;
-reply = (DBusMessage *)req-pvt;
+bool be_is_offline(struct be_ctx *ctx)
+{
+time_t now = time(NULL);
-dbret = dbus_message_append_args(reply,
- DBUS_TYPE_UINT16, online,
- DBUS_TYPE_UINT16, err_maj,
- DBUS_TYPE_UINT32, err_min,
- DBUS_TYPE_STRING, err_msg,
- DBUS_TYPE_INVALID);
-if (!dbret) {
-DEBUG(1, (Failed to generate dbus reply\n));
-return;
+/* check if we are past
Re: [SSSD] [PATCH] Make offline status backend global
On Mon, Sep 14, 2009 at 11:30:44AM -0400, Simo Sorce wrote: On Mon, 2009-09-14 at 07:48 -0400, Stephen Gallagher wrote: Just a nitpick, but why did you replace sbus_conn_send_reply() in be_check_online with sbus_get_connection and dbus_connection_send()? They are functionally identical. (except that sbus_conn_send_reply() can get the connection in one fewer stack frame, since it can access the sbus_connection object directly) I merged together what previously was an async request. It was only asking the id provider about offline status. Since now the offline status is directly accessible by the dp_provider_be.c there is no need to make a request to any provider, we directly return the answer to dp. (this call is not used by anything anyway so far but we were planning to use it to force a backend to go offline so it will come handy later on). Assuming I'm reading this correctly, we're talking about considering a single backend process as being online or offline as a whole. Why is this dependent only on the ID provider for the backend? It isn't, attached new patch that add be_mark_offline() calls also to the auth backends. Shouldn't we consider that if the authentication module or password change modules are offline that we are offline? yes, should be fixed in the new patch, now. Furthermore, even if the ID provider is offline, if we have cached user information that allows us to initiate a connection to a still-live authentication provider, isn't that perfectly reasonable? No, also because this is just an initial patch to start building infrastructure. If you remember we discussed the idea or allowing the monitor to put a backend forcibly offline, in that case all providers must respect this. Further more during auth we want to always refresh users, so if the id part is not available we can as well auth from the cached password (if caching passwords is allowed). This infrastructure is also need for DNS discovery later, where we want a central place to tell a specific server is unreachable. In any case I would rather put the infrastructure in place now, and tweak specific behaviors later. I'd argue that if any ONE of the modules was the ultimate determinator of online/offline status, it should be authentication rather than identification. Nope the auth module contacts the servers more rarely than the id backend (auths are rare compared to requests to get id information), so normally the ID backend is more qualified. In any case I am not going to make any provider king, all of them should be able to signal that servers are unreachable. Code itself is sensible, so this is a Nack until you can convince me that the approach itself is right. Let's see if the new patch and explanations are enough :) Simo. I agree, this patch is a good starting point and we can add fine tuning later. ACK. bye, Sumit ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] Make offline status backend global
On Mon, 2009-09-14 at 18:10 +0200, Sumit Bose wrote: I agree, this patch is a good starting point and we can add fine tuning later. ACK. pushed Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
[SSSD] [PATCH] Make offline status backend global
With this patch all the backend providers can share the same offline
status. This means composite backends like what AD or IPA will be that
use a mix of ldap and krb can share the offline status for both
protocols.
A further extension might allow to have per protocol online/offline
status, that will be done eventually when we integrate also the DNS
discovery options.
Tested with ldap_id+ldap_auth and ldap_id+krb5_auth
Simo.
--
Simo Sorce * Red Hat, Inc * New York
From 0cdf03e956838ae727760f8c22255958199f8e89 Mon Sep 17 00:00:00 2001
From: Simo Sorce sso...@redhat.com
Date: Sat, 12 Sep 2009 00:05:55 -0400
Subject: [PATCH] Make the offline status backend-global
Add helpers functions to query/set the offline status per backend.
Now all providers share the same offline status.
---
server/providers/data_provider_be.c | 117 +-
server/providers/dp_backend.h | 11 +++-
server/providers/krb5/krb5_auth.c | 12 +++-
server/providers/ldap/ldap_auth.c | 22 +--
server/providers/ldap/ldap_id.c | 55 ++--
server/providers/proxy.c| 76 +++
6 files changed, 79 insertions(+), 214 deletions(-)
diff --git a/server/providers/data_provider_be.c b/server/providers/data_provider_be.c
index 55fc278..2e83ab6 100644
--- a/server/providers/data_provider_be.c
+++ b/server/providers/data_provider_be.c
@@ -133,66 +133,39 @@ static int be_file_request(struct be_ctx *ctx,
return EOK;
}
-static void online_chk_callback(struct be_req *req, int status,
-const char *errstr)
-{
-struct be_online_req *oreq;
-DBusMessage *reply;
-DBusConnection *dbus_conn;
-dbus_bool_t dbret;
-dbus_uint16_t online;
-dbus_uint16_t err_maj = 0;
-dbus_uint32_t err_min = 0;
-const char *err_msg = Success;
-
-oreq = talloc_get_type(req-req_data, struct be_online_req);
-if (status != EOK) {
-online = MOD_OFFLINE;
-err_maj = DP_ERR_FATAL;
-err_min = status;
-err_msg = errstr;
-}
-
-online = oreq-online;
-reply = (DBusMessage *)req-pvt;
+bool be_is_offline(struct be_ctx *ctx)
+{
+time_t now = time(NULL);
-dbret = dbus_message_append_args(reply,
- DBUS_TYPE_UINT16, online,
- DBUS_TYPE_UINT16, err_maj,
- DBUS_TYPE_UINT32, err_min,
- DBUS_TYPE_STRING, err_msg,
- DBUS_TYPE_INVALID);
-if (!dbret) {
-DEBUG(1, (Failed to generate dbus reply\n));
-return;
+/* check if we are past the offline blackout timeout */
+/* FIXME: get offline_timeout from configuration */
+if (ctx-offstat.went_offline + 60 now) {
+ctx-offstat.offline = false;
}
-dbus_conn = sbus_get_connection(req-be_ctx-dp_conn);
-dbus_connection_send(dbus_conn, reply, NULL);
-dbus_message_unref(reply);
+return ctx-offstat.offline;
+}
-DEBUG(4, (Request processed. Returned %d,%d,%s\n,
- err_maj, err_min, err_msg));
+void be_mark_offline(struct be_ctx *ctx)
+{
+DEBUG(8, (Going offline!\n));
-/* finally free the request */
-talloc_free(req);
+ctx-offstat.went_offline = time(NULL);
+ctx-offstat.offline = true;
}
-
static int be_check_online(DBusMessage *message, struct sbus_connection *conn)
{
-struct be_online_req *req;
-struct be_req *be_req;
struct be_ctx *ctx;
DBusMessage *reply;
+DBusConnection *dbus_conn;
dbus_bool_t dbret;
void *user_data;
-int ret;
dbus_uint16_t online;
-dbus_uint16_t err_maj;
-dbus_uint32_t err_min;
-const char *err_msg;
+dbus_uint16_t err_maj = 0;
+dbus_uint32_t err_min = 0;
+static const char *err_msg = Success;
user_data = sbus_conn_get_private_data(conn);
if (!user_data) return EINVAL;
@@ -202,45 +175,10 @@ static int be_check_online(DBusMessage *message, struct sbus_connection *conn)
reply = dbus_message_new_method_return(message);
if (!reply) return ENOMEM;
-/* process request */
-be_req = talloc(ctx, struct be_req);
-if (!be_req) {
-online = MOD_OFFLINE;
-err_maj = DP_ERR_FATAL;
-err_min = ENOMEM;
-err_msg = Out of memory;
-goto done;
-}
-be_req-be_ctx = ctx;
-be_req-fn = online_chk_callback;
-be_req-pvt = reply;
-
-req = talloc(be_req, struct be_online_req);
-if (!req) {
+if (be_is_offline(ctx)) {
online = MOD_OFFLINE;
-err_maj = DP_ERR_FATAL;
-err_min = ENOMEM;
-err_msg = Out of memory;
-goto done;
-}
-req-online = 0;
-
-be_req-req_data = req;
-
-ret = be_file_request(ctx, ctx-bet_info[BET_ID].bet_ops-check_online, be_req);
-if (ret != EOK) {
-online = MOD_OFFLINE;
-err_maj = DP_ERR_FATAL;
-err_min = ret;
-
