refactor: use MEMORY type cache #36
Conversation
zmstone
force-pushed
the
0823-per-node-ccache
branch
from
e22ca01
to
defb243
Compare
c_src/sasl_auth.c
Outdated
| * | ||
| * krb5 doc says krb5_cc_default is essentially krb5_cc_resolve with default ccname, but it does not work. | ||
| * So we set environment variable KRB5CCNAME and call krb5_cc_default instead */ | ||
| setenv("KRB5CCNAME", (const char*)ccname_char, 1); |
There was a problem hiding this comment.
This in fact might be the simple reason why this didn't work for me when I experimented with it previously. Great find!
There was a problem hiding this comment.
Ok, better news, this does work, but you have to set default name first, there might be a better way, but :
if ((error = krb5_cc_set_default_name(context, (const char*)ccname_char)) != 0) {
tag = "krb5_cc_set_default_name";
goto kinit_finish;
}
if ((error = krb5_cc_default(context, &ccache)) != 0) {
tag = "krb5_cc_default";
goto kinit_finish;
}
And voila, we don't have to rely on os envs now 😄 We'll need to look at caveats though around threads and such, though krb5 seems pretty thread safe.
There was a problem hiding this comment.
Better solution indeed, so you were half way there, but we need to make this call first : https://github.com/krb5/krb5/blob/b9b654e5b469140d5603f27af5bf83ee9a826349/src/lib/krb5/ccache/t_cc.c#L621C40-L621C51
Looks like there's a default memory cc struct (krb5_cc_ops) in an include somewhere.
There was a problem hiding this comment.
^ ignore, krb5_cc_resolve works fine, only need to register if it's a truly new cc type.
if ((error = krb5_cc_resolve(context, (const char*)ccname_char, &ccache)) != 0) {
tag = "krb5_cc_set_default_name";
goto kinit_finish;
}
printf("CC TYPE : %s\r\n", krb5_cc_get_type(context, ccache));
then
1> example:main(client1).
CC TYPE : MEMORY
ok
2> There was a problem hiding this comment.
krb5_cc_get_type probably will return what you want to see,
but what matters is the return value of sasl_client_new.
|
I'll give this a whirl in anger shortly. |
zmstone
changed the title
zmstone
force-pushed
the
0823-per-node-ccache
branch
from
a175b7f
to
d94938e
Compare
|
Alas, I'm running to the same problem I had with cyrus-sasl when testing this (no creds supplied error). That said, I have a reproducer for you. This can be done with just sasl_auth app, but for the sake of time, I did the following :
-module(example).
-export([main/1, race_test/0]).
race_test() ->
ClientNames = [erlang:list_to_atom("client_" ++ erlang:integer_to_list(I)) || I <- lists:seq(1, 100)],
[spawn(fun() -> example:main(Client) end) || Client <- ClientNames].
main(ClientName) ->
try
application:ensure_all_started(brod),
application:load(brod_gssapi),
application:set_env(brod_gssapi,
default_handshake_vsn,
1),
KafkaBootstrapEndpoints = [{"kafka.kerberos-demo.local", 9093}],
Topic = <<"mytest">>,
%Partition = 0,
KeyTab = <<"/var/lib/secret/rig.key">>,
Principal = <<"[email protected]">>,
Config = [{sasl, {callback, brod_gssapi, {gssapi, KeyTab, Principal}}}],
brod:start_client(KafkaBootstrapEndpoints, ClientName, Config),
brod:start_producer(ClientName, Topic, _ProducerConfig = [])
catch
ErrorClass:Reason:Stack ->
io:format("~n~n~n~n~n\033[0;31mFAIL\033[0m (Failed to send and receive messages with brod)~n~n ~p:~p:~p~n~n~n~n~n", [ErrorClass, Reason, Stack])
%erlang:halt(1)
end.
If you do that on main, you'll end up with file missing errors from krb5, if you do that on this branch you'll end up with no credentials error. As said, I started to experiment with this and other solutions (such as using unique files, though this can go poorly if you do not clean up afterwards). The most optimal solution is indeed using memory cache, even if your on a sane platform and using SSSD, that's still a bottle neck. iirc, when I looked to this previously, cyrus-sasl is doing something that prevents memory cache from effectively being used, though I do like to be wrong, and hope I and am wrong. |
|
Thank you @starbelly
It would be great if you can give some pointers about what is preventing memory cache. |
@zmstone I think actually this is working, there's just a potential problem perhaps with going across schedulers (hypothesis, not theory). If I reduce the number of schedulers to 1, such that all memory access happens within a single thread, I have no issues, and in fact can change the number of clients starting up to 1000 without issue (do not it was just starting the client and the producer as above, though that should be good enough for these purposes). We could also look at the latest cyrus-sasl and see if there is improvement there, but I think the intention by memory cache is that it's to be used by a single process (which I believe really means thread) : https://github.com/krb5/krb5/blob/b9b654e5b469140d5603f27af5bf83ee9a826349/doc/basic/ccache_def.rst?plain=1#L101 That said, maybe we can make a unique name per scheduler? Edit: This works, in fact, I think the solution right now was working, but path deps (as part of the brod_gssapi example client) was not working as expected (i.e., it didn't pull in the checkout). This is good news 😄 That said, I still wonder if should have a cache per scheduler per what I alluded to above. Extra edit : Sorry for the noise, but trying to get my comments in before I head out for an hour or so. I think we also want to ensure we only set this once, if possible. |
|
found some discussions in memory cache thread-safe topic: https://bugzilla.redhat.com/show_bug.cgi?id=1605756 |
|
Changed to set |
No need for the env var 😄 See #36 (comment) |
|
I tride to test |
What os? I tried ubuntu, but I can try redhat today. |
Ubuntu 20. I noticed, but not tried to fix yet: |
|
this is how I run my tests ( |
|
I tried that, I didn't get an error, but not sure the tests went well, but exited with error none the less. Can you try the test I described utilizing brod_gssapi? |
|
I can confirm that krb5_cc_resolve works for this test: #36 (comment) |
|
right. it works in Ubuntu 22 (libgssapi-krb5-2 1.19.2-2), but not 20 (libgssapi-krb5-2 1.17-6). |
|
Beautiful! I suppose we should enhance tests here ala what's going on there. |
|
I'll keep setting env in the load call. |
|
Sorry, Maybe I tested something wrong on Ubuntu 22, it's actually failing after I tried to repeat the tests with int-test.sh |
Sorry, it did not work after all. With below diff.
The same error from |
but also allow kinit/3 to override when the name is not empty string
zmstone
force-pushed
the
0823-per-node-ccache
branch
from
9b2fe4a
to
e8f7001
Compare
|
Ok, let me take a further look this evening. |
Wasn't able to get to this last night, will plan on looking today. |
|
Hi @starbelly by any chance you can help to have a look soon? In the next iteration we can focus on:
|
|
@zmstone I conquer with your findings, it can only work this way in fact, so not sure how I ended up with success previously, but that's another story. Sadly, cyrus-sasl's sasl_client_new provides no way to provide a ccname explicitly, which is what I think is ultimately needed to avoid env vars. Using env vars is unfortunate, but will work for now. I will give this one more look over code wise. |
|
thanks! @starbelly |
Hopefully fixes #26, but not 100% sure because I cannot reproduce it.
The idea is to:
MEMORYtype cache by default (because the reported error is seemingly related to file system/hard drive)kinit(KeyTab, Principal, "FILE:/tmp/my_cc_name"). This should provide flexibility at application level.