Support passing HTTP Auth credentials to event stream

[usophia]

Support passing HTTP Auth credentials to event stream。

[timoreimann]

I'm not completely sure if HTTP Basic Auth supports empty usernames with non-empty passwords. When in doubt, we should probably extend the if condition by

|| r.config.HTTPBasicPassword != ""

[timoreimann]

Would you mind also writing some tests for the new functionality (and the various branches it contains)?

Let me know if you need help.

[timoreimann]

It also seems like the Debug part in your commit message isn't perfectly fitting. How about changing it to Support passing HTTP Auth credentials to event stream or something like that?

[usophia]

@timoreimann Thanks for your advice。I will write some tests for my commit。

[dmajere]

@anxueyan
Thanks for your commit, I'm very interested in this PR, do you need any help with unit tests?

[usophia]

@timoreimann Sorry for long time to reply. Yes ,you are right, we should add if condition
"|| r.config.HTTPBasicPassword != ". Even if HTTP credentials of marathon not support non-empty passwords or usernames It should allow pass empty passwords or usernames to marathon , the result may be authentication failed. but I have two questions：

1.The file client.go, It's also use the if condition "if r.config.HTTPBasicAuthUser != "" " in the function "func (r *marathonClient) apiCall(method, uri string, body, result interface{}) error " (line 247). why this place use this condition？
2. I run marathon in container with environment "MARATHON_HTTP_CREDENTIALS: "username:" " or "MARATHON_HTTP_CREDENTIALS: ":password" " , and do request like this
"# curl -I -u kdkdjdksls:lkkd http://10.17.102.16:8089/v2/apps (10.17.102.16:8089 is host:port of my marathon)"
the response is "HTTP/1.1 200 OK" . It's right? do you have met? It didn't seem to do certification .

[usophia]

@dmajere yes, It's need some help with unit tests. I met the problem mentioned above， and I don't know how to write a good unit tests. Here is an example, is what I write：

  func TestEventStreamConnectionHTTPNullAuthUser(t *testing.T) {
    config := NewDefaultConfig()
    config.EventsTransport = EventsTransportSSE
    config.URL = "http://non-existing-marathon-host.local:5555"
    config.HTTPBasicAuthUser = ""
    config.HTTPBasicPassword = "password"

    endpoint := newFakeMarathonEndpoint(t, &config)
    defer endpoint.Close()

    events := make(EventsChannel)
    err := endpoint.Client.AddEventsListener(events, EVENTS_APPLICATIONS)

    assert.NoError(t, err)
    endpoint.Client.RemoveEventsListener(events)
}

It's ok? if not ok ,how should I write?

[dmajere]

@anxueyan
In your case, as i see it, we need to verify that req object method BasicAuth returns user, passwd when we set them.
To do this we'll need to substitute eventsource.SubscribeWithRequest method, so we'll be able to record parameters passed to it and validate them.
To do this we'll have to change registerSSESubscription() method to registerSSESubscription(dialer) where dialer is a function or an interface, this way, we'll be able to pass eventsource.SubscribeWithRequest in our library and mock object in our tests.
This change can also help to write tests for this logic

` go func() {

    for {
        select {
        case ev := <-stream.Events:
            if err := r.handleEvent(ev.Data()); err != nil {
                // TODO let the user handle this error instead of logging it here
                r.debugLog.Printf("registerSSESubscription(): failed to handle event: %v\n", err)
            }
        case err := <-stream.Errors:
            // TODO let the user handle this error instead of logging it here
            r.debugLog.Printf("registerSSESubscription(): failed to receive event: %v\n", err)
        }
    }
}()`

cause we'll be able to return stream object that we'll be able to control in tests

[timoreimann]

@anxueyan Regarding your questions:

1. The section in question passes the credentials defined during the creation of the go-marathon client configuration to Marathon on every HTTP request. If you're wondering why we have to do this again in this PR: It's because event streams are implemented through an external library which doesn't run through go-marathon's low-level apiCall() method. It's good that you spotted this though as it seems to me that we should extract the existing if-credentials-enrich-request logic into a utility function in order to be applied from both call sites.
2. I'm not entirely sure, but maybe this indicates that password-less credentials are in fact not possible with Marathon? Then again, I'd be surprised if Marathon doesn't return an error in this case. Maybe you can find something in the logs?

[usophia]

@timoreimann The first question， I am confused why there is not add if condition
|| r.config.HTTPBasicPassword != ""
2. I have checked marathon log and find

{"log":"* Actor[akka://marathon/user/rateLimiter#186716615]\n","stream":"stdout","time":"2016-05-20T11:19:25.968477035Z"}
{"log":"* Actor[akka://marathon/user/AbdicateOnConnectionLoss#-373755716]\n","stream":"stdout","time":"2016-05-20T11:19:25.968500736Z"}
{"log":"* Actor[akka://marathon/user/launchQueue#1235565712]\n","stream":"stdout","time":"2016-05-20T11:19:25.968511885Z"}
{"log":"* Actor[akka://marathon/user/offerMatcherLaunchTokens#2069215227] (mesosphere.marathon.core.leadership.impl.LeadershipCoordinatorActor:marathon-akka.actor.default-dispatcher-4)\n","stream":"stdout","time":"2016-05-20T11:19:25.968521332Z"}
{"log":"[2016-05-20 11:19:26,132] INFO Now standing by. Closing existing handles and rejecting new. (mesosphere.marathon.event.http.HttpEventStreamActor:marathon-akka.actor.default-dispatcher-4)\n","stream":"stdout","time":"2016-05-20T11:19:26.133360897Z"}
{"log":"[2016-05-20 11:19:26,148] ERROR The HTTP credentials must be specified in the form of 'user:password'. (mesosphere.chaos.http.HttpModule:main)\n","stream":"stdout","time":"2016-05-20T11:19:26.148816253Z"}

Yes, marathon not support password-less credentials or username-less credentials, More powerful evidence can be find in file HttpServer.scala . If you set empty password or empty username for HTTP credentials of marathon, it don't do certification.

[timoreimann]

1. I suppose the condition is missing because nobody thought about the use case of an empty username prior to this PR. :-). That's why I suggested to extract the logic into a central function so that we avoid making the same mistakes at multiple call sites across the code base. Although in this case...
2. It turned out now that you must always pass both username and password anyway. Consequently, we don't need to extend the logical operation. However, I still think it'd be useful to centralize the credentials logic on the request instance for future changes on that part to come.

[usophia]

@timoreimann It's better to centralize the credentials logic on the request instance . 😊

[timoreimann]

@anxueyan The request instance is from the stdlib (http.Request) which cannot be amended. A simple wrapper function like setCredentials(req *http.Request, c *config) should hopefully do the trick. :-)

[timoreimann]

@anxueyan did you have a chance to try out @dmajere's suggested testing approach? Let us know if there's need for clarification, and feel free to push any intermediate results you may have at any point in time. Thanks!

[gambol99]

@timoreimann @anxueyan ... Since its very much related to issue #148 (auth+tls) .. and the PR donovanhide/eventsource#22 has been merged, perhaps we can clean both up with the below ... or similar ..

url := fmt.Sprintf("%s/%s", marathon, marathonAPIEventStream)
// step: create a new request for the stream
request, err := http.NewRequest("GET", url, nil)
if err != nil {
    return err
}
// step: add any basic authentication (perhaps wrap this in a method)
if r.config.HTTPBasicAuthUser != "" && r.config.HTTPBasicPassword != "" {
    request.SetBasicAuth(r.config.HTTPBasicAuthUser, r.config.HTTPBasicPassword)
}
// Try to connect to stream, reusing the http client settings
stream, err := eventsource.SubscribeWith("", r.httpClient, request)
if err != nil {
    return err
}

[gambol99]

was fixed in PR #156