PUT operation disrupt objects in case re-authentication is done

[westenb]

Hello,

there seems to be severe issue with the re-authentication in combination with modifying requests (e.g. PUT) with InputStream payloads.

Scenario:
OSClient is created from a previously stored Token, and PUT operation is done (e.g. SWIFT file upload). If the Token is expired, the HttpExecutorServiceImpl (e.g. from the Jersey connector) will trigger a re-authentication and retry the request

Response response = command.execute();
        if (command.getRetries() == 0 && response.getStatus() == 401 && !command.getRequest().getHeaders().containsKey(ClientConstants.HEADER_OS4J_AUTH))
        {
            OSAuthenticator.reAuthenticate();
            command.getRequest().getHeaders().put(ClientConstants.HEADER_X_AUTH_TOKEN, OSClientSession.getCurrent().getTokenId());
            return invokeRequest(command.incrementRetriesAndReturn());
        }

However, the same Payload object is re-used as far as I can tell, which in case of an InputStream will be a stream which has been processed already during the first call. This leads in the case of uploading a file to SWIFT to the severe effect that the retry will succeed but create a file with content length 0 (in simple words: destroy the file). I believe the InputStream / Entity should be resetted before the Retry.

Is my analysis correct?

Regards,
Eric

PS: Maybe it would also make sense to remove this retry logic from the specific connectors, and have that at a central place.

[vinodborole]

@westenb Is there any with this issue #676

[westenb]

Yes, I believe this is the root cause for the problem described in #676. I opened a separate ticket because #676 was about another topic initially.

[westenb]

I analysed the problem a bit deeper and tried some strategies to fix the problem. However, based on the current implementation structure, I don't see an easy path without touching many parts. In particular for file and url payloads, the request entities would have to be reconstructed (to create a new InputStream).

Currently, my feeling is that the quickest fix which prevents at least the current flawed behavior and has a simple, well-defined behaviour, is to add a check to all connector implementations of HttpExecutorServiceImpl.java which prevent a retry in case an entity is present and instead fail regularly due to invalid credentials, e.g.

[...]
Response response = command.execute();
if (command.getRetries() == 0 
 && response.getStatus() == 401
 && !command.hasEntity() // THIS IS NEW
 && !command.getRequest().getHeaders().containsKey(ClientConstants.HEADER_OS4J_AUTH))
        {
            // re-authentication and retry request
           [...]
        } 

// normal response handling even in case of 401           
[...]

I didn't yet create a PR because maybe someone with deeper os4j knowhow has a better proposal.

Regards,
Eric

[gschukin]

I assume that this can be fixed using Expect: 100-continue.

[gschukin]

00:45:56.932 [main] DEBUG org.apache.http.headers - http-outgoing-6 >> POST /v2/8410bfa9c33d4535b32e34a7e77f0760/servers HTTP/1.1
00:45:56.932 [main] DEBUG org.apache.http.headers - http-outgoing-6 >> Accept: application/json; charset=utf-8
00:45:56.932 [main] DEBUG org.apache.http.headers - http-outgoing-6 >> X-Auth-Token: e08e3386558b4060ab4635caed587b8f
00:45:56.932 [main] DEBUG org.apache.http.headers - http-outgoing-6 >> User-Agent: OpenStack4j / OpenStack Client
00:45:56.932 [main] DEBUG org.apache.http.headers - http-outgoing-6 >> Content-Length: 1708
00:45:56.932 [main] DEBUG org.apache.http.headers - http-outgoing-6 >> Content-Type: application/json; charset=UTF-8
00:45:56.932 [main] DEBUG org.apache.http.headers - http-outgoing-6 >> Host: selena.clouddc.ru:8774
00:45:56.932 [main] DEBUG org.apache.http.headers - http-outgoing-6 >> Connection: Keep-Alive
00:45:56.932 [main] DEBUG org.apache.http.headers - http-outgoing-6 >> Expect: 100-continue
00:45:56.932 [main] DEBUG org.apache.http.headers - http-outgoing-6 >> Accept-Encoding: gzip,deflate
00:45:56.950 [main] DEBUG org.apache.http.headers - http-outgoing-6 << HTTP/1.1 401 Unauthorized
00:45:56.951 [main] DEBUG org.apache.http.headers - http-outgoing-6 << Www-Authenticate: Keystone uri='http://10.20.32.21:5000/'
00:45:56.951 [main] DEBUG org.apache.http.headers - http-outgoing-6 << Content-Type: text/plain
00:45:56.951 [main] DEBUG org.apache.http.headers - http-outgoing-6 << X-Compute-Request-Id: req-6f2e70c7-3502-4cbf-a769-0c0ce91823cf
00:45:56.951 [main] DEBUG org.apache.http.headers - http-outgoing-6 << Content-Length: 23
00:45:56.951 [main] DEBUG org.apache.http.headers - http-outgoing-6 << Date: Mon, 06 Jun 2016 21:45:56 GMT
00:45:56.951 [main] DEBUG org.apache.http.headers - http-outgoing-6 << Connection: close

Can be enabled in httpclient via

       HttpClientFactory.registerInterceptor( new HttpClientConfigInterceptor() {
            @Override
            void onClientCreate(HttpClientBuilder client, RequestConfig.Builder requestConfig, Config config) {
                requestConfig.expectContinueEnabled = true
            }
        })

[westenb]

Hi,

using the 100-continue approach looks very interesting; but the coding above is connector specific (here HTTP connector), and cannot be easily enabled from the outside.

Following this approach, I guess the clean way would be add a kind of "expectContinueEnabled" setting to org.openstack4j.core.transport.Config and handle it in the respective connectors accordingly. What do you think?

Regards,
Eric

[gschukin]

@westenb afaik not all connectors support this. Okhttp has broken support since last year.

[juliccr]

Hi @westenb , this issue is present for OSClientV2 also? Or only for OSClientV3? Thanks.

[westenb]

Hi @juliccr ,

as far as I understand, this issue is independent of the client version / openstack service, as this is part of the generic HTTP request/handling. The issue occurs whenever a request including a payload (InputStream) is done, and during the handling a re-authentication is occurring (e.g. due to token expiration).

I believe this was already introduced when solving #45 long time ago, but apparently no one ran into the issue so far?!

Regards, Eric

[juliccr]

Ok, thank you so much, @westenb , I'll try it out then ;) .

Regards, Julián.

[vinodborole]

@westenb closing this for now, please reopen if this issue is related to o4j