Swift backend support by constantinius · Pull Request #207 · MapServer/mapcache

constantinius · 2019-10-02T09:27:00Z

Adding OpenStack Swift cache backend using modified keystone-client and swift-client libraries.

…nd swift-client libraries

constantinius · 2019-10-02T09:32:00Z

I used these libraries as a baseline to access swift/keystone:

I'd love to have some feedback on how I integrated it into mapcache. Currently this functionality is not really tested in production, so this PR stays a draft.

keystone-client uses json-c (added as cmake dependency), I'm not sure if this is done correctly. I'm aware, that mapcache ships with its own JSON implementation (cJSON), but I did not yet make an effort to replace either.

tbonfort

Not going into the actual swift logic, here are a few comments. The error checking and connection pool releasing applies to the whole cache_swift.c file, not just the reviewed lines.

tbonfort · 2019-10-02T10:00:40Z

include/keystone-client.h

@@ -0,0 +1,200 @@
+#ifndef KEYSTONE_CLIENT_H_


Missing licence on imported files

Right!

Besides: Do you think that it is alright to include a source file like that? The repo says it is GPL v3.

tbonfort · 2019-10-02T10:02:45Z

lib/cache_swift.c

+
+    keystone_err = keystone_authenticate(conn->keystone_context, cache->auth_url, cache->tenant, cache->username, cache->password);
+    if (keystone_err != KSERR_SUCCESS) {
+        ctx->set_error(ctx, 500, "failed to keystone_authenticate()");


error conditions should halt execution. add a return here and after all ctx->set_error calls

I will check all possible paths and fix these issues.

tbonfort · 2019-10-02T10:04:59Z

lib/cache_swift.c

+        }
+    }
+
+    mapcache_connection_pool_release_connection(ctx, pc);


This should be called before all exit paths (i.e. it is currently not called on error condition exits)

Right, I'll need to check those as-well.

tbonfort · 2019-10-02T10:08:25Z

lib/cache_swift.c

+ *
+ * Project:  MapServer
+ * Purpose:  MapCache tile caching support file: swift cache backend.
+ * Author:   Michael Downey and the MapServer team.


Is this the correct author?

…atus codes >= 400.

constantinius · 2020-03-31T15:42:34Z

@tbonfort

I updated the source code and now support keystone v3 API.
In principle, this works, we already use it in a staging environment. Unfortuately, we run into realloc(): invalid next size when using with mapcache_seed (and sometimes in the Apache module too).

With gdb, we got this stack trace:

#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1  0x00007fde75254801 in __GI_abort () at abort.c:79
#2  0x00007fde7529d897 in __libc_message (action=action@entry=do_abort, fmt=fmt@entry=0x7fde753cab9a "%s\n") at ../sysdeps/posix/libc_fatal.c:181
#3  0x00007fde752a490a in malloc_printerr (str=str@entry=0x7fde753c8ed3 "realloc(): invalid next size") at malloc.c:5350
#4  0x00007fde752a99b4 in _int_realloc (av=av@entry=0x7fde5c000020, oldp=oldp@entry=0x7fde5c024080, oldsize=oldsize@entry=64, nb=nb@entry=64)
    at malloc.c:4534
#5  0x00007fde752acf9b in __GI___libc_realloc (oldmem=0x7fde5c024090, bytes=56) at malloc.c:3230
#6  0x00007fde7719376e in swift_set_object () from /usr/lib/x86_64-linux-gnu/libmapcache.so.1
#7  0x00007fde7716fb62 in ?? () from /usr/lib/x86_64-linux-gnu/libmapcache.so.1
#8  0x00007fde77165f4e in mapcache_cache_tile_exists () from /usr/lib/x86_64-linux-gnu/libmapcache.so.1
#9  0x00005602271e7d7d in examine_tile ()
#10 0x00005602271e807b in cmd_recurse ()
#11 0x00005602271e866c in feed_worker ()
#12 0x00005602271e872b in ?? ()
#13 0x00007fde6dbb96db in start_thread (arg=0x7fde6257a700) at pthread_create.c:463
#14 0x00007fde7533588f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95

This points to an issue in swift_set_object, where I basically duplicate a string (the re-allocation). From my perspective it looks clean but I'm suspecting, that there is some issue with connection pooling (construction, destruction), but I can't figure it out. Could you maybe have a look at it and tell me if I use the interfaces correctly?

constantinius · 2020-04-02T12:24:19Z

I think I found the issue, a classic buffer overflow (re-)allocating strlen bytes, but strcpy writes a final '\0' character out of the allocated area.

This PR is good to go from my perspective. @tbonfort please let me know if need anything in addition.

jbo-ads · 2021-03-10T12:19:30Z

Have you figured out the causes of failed checks?

constantinius · 2021-04-01T15:26:46Z

@jbo-ads

Yes, I think I solved the issues

jbo-ads · 2021-04-06T11:45:01Z

I've checked your updates following @tbonfort's comments and it looks OK in my opinion.

About licenses, I think there is nothing wrong with using LGPLv3 source code in a bigger MIT licensed project.

I don't know the prefered way of mentioning it: copy license text at the beginning of related code (only swift-client.h and swift-client.c, since no license is mentioned in keystone-client repo), or include it in MapCache LICENSE file with a note indicating that only swift-client is concerned with LGPLv3. @jmckenna any advice on this?

jmckenna · 2021-04-06T13:25:35Z

@jbo-ads I prefer the second option, but I think it should be noted in several places:

source LICENSE file
source README.rst (so it displays nicely on Github)
add a 'copyright.txt' file in the MapCache documentation, similar to the one for MapServer at https://mapserver.org/copyright.html (so web users can see the full license for MapCache)

jbo-ads · 2021-04-06T19:46:40Z

@constantinius - Please don't forget to update the documentation with this new feature: https://mapserver.org/mapcache/caches.html

Schpidi · 2021-11-19T10:22:16Z

Unfortunately the latest commit breaks building for me:

[ 95%] Linking C executable mapcache.fcgi
cd /build/mapcache-1.10.0/obj-x86_64-linux-gnu/cgi && /usr/bin/cmake -E cmake_link_script CMakeFiles/mapcache.fcgi.dir/link.txt --verbose=1
/usr/bin/cc -g -O2 -fdebug-prefix-map=/build/mapcache-1.10.0=. -fstack-protector-strong -Wformat -Werror=format-security -DNDEBUG -Wdate-time -D_FORTIFY_SOURCE=2 -Wl,-Bsymbolic-functions -Wl,-z,relro -Wl,-z,now -Wdate-time -D_FORTIFY_SOURCE=2 -Wall -Werror=declaration-after-statement  -Wl,-Bsymbolic-functions -Wl,-z,relro -Wl,-z,now -rdynamic CMakeFiles/mapcache.fcgi.dir/mapcache.c.o  -o mapcache.fcgi -Wl,-rpath,/build/mapcache-1.10.0/obj-x86_64-linux-gnu: ../libmapcache.so.1.10.0 -lfcgi -lpng -lz -ljpeg -lcurl -lapr-1 -laprutil-1 -lpixman-1 -lgdal -lpcre -lsqlite3 -lpq -ltiff -ljson-c -ldl -lm 
../libmapcache.so.1.10.0: undefined reference to `keystone_get_service_url'
../libmapcache.so.1.10.0: undefined reference to `keystone_set_auth_version'
../libmapcache.so.1.10.0: undefined reference to `keystone_start'
../libmapcache.so.1.10.0: undefined reference to `keystone_end'
../libmapcache.so.1.10.0: undefined reference to `keystone_authenticate'
../libmapcache.so.1.10.0: undefined reference to `keystone_get_auth_token'
../libmapcache.so.1.10.0: undefined reference to `keystone_set_debug'
collect2: error: ld returned 1 exit status

Adding OpenStack Swift cache backend using modified keystone-client a…

bb0db52

…nd swift-client libraries

tbonfort reviewed Oct 2, 2019

View reviewed changes

constantinius added 10 commits October 2, 2019 22:59

Improved error handling. Cleanup.

4ca952c

Adding bindings to enable debugging

44c4e91

keystone v3 authentication

530afe6

Adding support for keystone v3 authorization

daa7276

Fixes to be able to use json-c 0.12

9bac975

Fixing typo: allocating container instead of object

a54efe7

Fixes for object existence check. Additional return codes for HTTP st…

c0be54c

…atus codes >= 400.

Special handling of 404 responses in the backend

813c6ec

Trying to fix segfaults in cache retrieval

e1a74fa

Adding source reference in comments

f3597e1

Fixing buffer overflow in swift_set_object and swift_set_container

a1c3931

constantinius marked this pull request as ready for review April 2, 2020 12:24

Base automatically changed from master to main January 15, 2021 22:57

Making comment to state why a byte was added in allocation

6173b17

jmckenna added this to the 1.12.0 milestone Mar 10, 2021

Fixing build error when Swift is disabled

34d1c64

jmckenna removed this from the 1.12.0 milestone Jan 29, 2023

jmckenna added the feedback requested label Jan 29, 2023

Uh oh!

Conversation

constantinius commented Oct 2, 2019

Uh oh!

constantinius commented Oct 2, 2019

Uh oh!

tbonfort left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

constantinius commented Mar 31, 2020

Uh oh!

constantinius commented Apr 2, 2020

Uh oh!

jbo-ads commented Mar 10, 2021

Uh oh!

constantinius commented Apr 1, 2021

Uh oh!

jbo-ads commented Apr 6, 2021

Uh oh!

jmckenna commented Apr 6, 2021

Uh oh!

jbo-ads commented Apr 6, 2021

Uh oh!

Schpidi commented Nov 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants