First draft of a simple auth-aware HttpClient service #40
Conversation
Codecov Report
@@ Coverage Diff @@
## master #40 +/- ##
=============================================
+ Coverage 71.18% 84.52% +13.33%
- Complexity 3 35 +32
=============================================
Files 2 7 +5
Lines 59 252 +193
Branches 0 1 +1
=============================================
+ Hits 42 213 +171
- Misses 17 38 +21
- Partials 0 1 +1
Continue to review full report at Codecov.
|
islandora-http-client/build.gradle
Outdated
|
|
||
| instruction 'Import-Package', 'org.apache.http.client,' + | ||
| defaultOsgiImports | ||
| instruction 'Export-Package', 'ca.islandora.indexing.http.client' |
|
API-X PR is here: Once it's merged, adding to |
| </cm:default-properties> | ||
| </cm:property-placeholder> | ||
|
|
||
| <service id="httpClient" interface="org.apache.http.client.HttpClient" filter="(osgi.jndi.service.name=claw/httpClient)"> |
There was a problem hiding this comment.
Does this work? I've never tried the filter property when exporting a service. My only experience is with the form:
<service-properties>
<entry key="osgi.jndi.service.name" value="claw/httpClient" />
</service-properties>
There was a problem hiding this comment.
Probably I was just being absent minded. This is why we need more i-tests... I will fix this to the style you are suggesting, which I'm sure is correct.
|
@birkland, I've got your PR and my PR together in a but I'm getting a jillion 401s off of API-X after startup of the form I record here. Eventually, the logs settle down and just repeat a pattern like this: apparently, forever. Tentatively, I'm thinking that this is due to actual Camel pipelines (looks like in Do you buy that theory? Can I give you further diagnostic info? This is far enough into the guts of API-X that I am not very confident in my diagnostic abilities. |
|
I think I found the issue, it's not with |
|
@ajs6f OK, just pushed an update. There's a little configuration change. Instead of putting the property Clearly, we need to incorporate auth into the ITs on the API-X side. I've created a new issue to do that separately, so we can just get the fix merged right away for CLAW |
|
@birkland, unfortunately, I think something is still missing. I pulled your last commit and rebuilt API-X. Then I altered the config as you specified above and did the usual It seems that to get to that point, the authenticating client must be available to API-X (for a successful startup), so is it possible that there is some other point at which the client may need to be injected...? Seems unlikely, but I'm grasping at straws. :) |
|
Darn it, that's why we need ITs for this. I can never get the OSGi wiring right on the first try; was injecting the wrong client. Now it should be using the right one. I'm presently working on getting the existing ITs set up to cope with auth. |
|
@ajs6f I just pushed an update |
|
@birkland Thanks-- having applied the update (to the blueprint.xml) I am now getting full bundle activation, lots of JMS connection errors (which don't bother me and seem totally unrelated), and then a log with endless repeating: |
|
Interestingly (or maybe not) when I go to |
|
@ajs6f Hm, you do not see messages like "Container ___ does not exist, adding"? |
|
@birkland No, exactly. That's odd, no? Here is a full log. The sequence was:
|
|
@ajs6f What must be happening is that a HEAD request fails, and the underlying cause isn't being logged (or is being logged at a lower level). In any case, it's not being user friendly, which bugs me. I don't want to waste too much of your time doing this blind. Let me see if I can finish getting the auth-enabled ITs working in order to observe firsthand... |
|
Oh, I finally see the root problem for why which is bizarre, because: And why did it start up when I restarted it? |
|
@birkland ++++ I will wait to hear from you. Thanks very much for all your work on this. (My only viable proffer of comfort is that a lot of Fedora repos are gong to have authN in place, so this should be useful to other people as well.) |
|
@ajs6f Also, good observation re: 0 HttpClient services. It seems impossible to tell blueprint to look for service while filtering by a property whose value is provided by configuration. So the end-around is to resolve the service manually. The downside is that the blueprint lifecycle won't delay activating the bundle if the service isn't resolved. So I think that's the issue here |
|
@birkland Have you tried just using a direct |
|
Here's a completely useless suggestion: https://issues.apache.org/jira/browse/ARIES-1665 says that we could switch the whole deal to CDI. :) |
|
@ajs6f Yes, the problem is that you can't do |
|
Ah, I see. That's some stinky cheese, indeed. Actually, CDI isn't going to help as much with that particular problem, because I think you are actually using a difficult pattern by allowing the filter expression to vary at runtime. (Although I totally get why you are doing it.) In CDI-OSGi, that might look like discarding all the XML and a more typesafe kind of injection: with custom client annotation like: to be used in bundles that supply a custom client. CDI-OSGi takes care of a lossless CDI qualifiers <-> OSGi filter expressions translation. There are certainly other ways to do it, too. There are events to which to listen when services get registered and so forth, so that if a custom client shows up after the registry starts, the registry can start using it. |
|
@birkland There's also the CDI |
|
@ajs6f Well, I spent the afternoon OSGi fighting, and am still doing so. I haven't had luck with using service priorities with blueprint in the past. Are service priorities in CDI-OSGI any better? Here's the issue: What needs to happen is that if a bundle comes along that publishes an What actually happens is that blueprint makes a choice between all services available at the time once, injects once, and moves on. If a higher-ranked service pops into existence later on, it's ignored. So if CLAW is providing a third-party HttpClient, it would need to assure that the bundle is loaded and started before everything else. I worry that this is a burdensome thing to ask As a result of all this, I think our hand is forced to service listeners (or equivalent). Will try and see what happens... |
|
Well, re-injected is a little strong, I forgot that blueprint uses proxies underneath. In any case, here's the specific issue we're up against: |
|
I completely agree about the "burden" of bundle ordering. In the OSGi field, demanding a particular load order for bundles is six kinds of wrong. I do not know whether a given CDI-OSGi implementation (there are several) suffers from the same problem-- I will find out. In the meantime, I think you are right that we need to be dropping back to programmatic work, directly with the service registry. And frankly, that's not too awful, in this case. We have only one service at stake and one client, right? (Or is there more than one client?) Here's a possible design. Select a constant service location in JNDI. Then look for a service under that name. If there is more than one, an error. If there is one, use it. If there is not one, use a default client (not a service, just internal to the bundle that needs to do the work, the registry bundle, right?). Attach a service listener to the registry and if a service with that name shows up, use it. If more than one service with that name ever exists at a time, an error. So the semantics for that service change from "the client to use" to "the custom client to use, if there is one." Does that sound plausible? I could probably get that into a PR fairly quickly. (He wrote, while packing up his office and becoming increasingly desperate about the potential to fit all of the plants into the back of the Prius in one load.) |
|
@ajs6f Yes, that sounds totally plausible. I think that would need to happen within a proxy, which is the approach I've started exploring this morning Right now, there's an So I think the least invasive way do to this listening is within a proxy. That is to say, I've just started to implement this in my local branch now. I already set up the ITs to use a Fedora protected by basic auth, and created a service in |
|
@ajs6f OK, I think the OSGi bits are working. I'm now diagnosing the inexplicable |
|
@birkland ++ are you using the CLAW vagrant? Or just a stock Karaf with a Fedora on the side? |
|
@ajs6f I'm running all in the API-X ITs. A fedora instance protected by basic auth launched via the cargo maven plugin, an API-X instance in Karaf launched by pax exam, and an httpclient provided by a test bundle. I wasn't creating the test HttpClient right. Now it looks like things are starting to work. |
|
Awesome! Let me know when you want me to fire it against |
|
OK, will do. Need to pick up the kids, but will hopefully have all ITs passing later tonight or tomorrow morning. |
|
No hurry-- I'm doing the same (family time) and trying to get through the last few tasks I have at UVa. |
|
@ajs6f I just updated the PR, all ITs except for the service indexing IT are passing. The service indexing IT is camel-based, and uses the fcrepo-camel component to interact with Fedora. I'm looking onto that next, but It uses your suggestion to simply use OSGi service priority ranking, rather than looking for a service name in config. In particular:
The underlying implementation listens for OSGi binding events. So for the CLAW case, it should immediately start using the provided |
|
I will test this out today in a cage match against |
|
It worked! @dannylamb and I are arranging the CLAW PRs, but you will have to do a release of API-X with your PR merged to let us pick up the changes... |
|
Yay! Excellent!. I think we can merge once the Camel-based ITs are resolved, and I add some text to the markdown documenting all of this. There shouldn't be a problem cutting a release quickly thereafter, I'll mention that prospect on today's API-X call. |
|
I believe that this can now be safely merged to support islandora-deprecated/claw_vagrant#40. |
|
@ajs6f @birkland Circling back around to the one now that Islandoracon is over. I'll test both this and islandora-deprecated/claw_vagrant#40, and will let y'all know how it goes. |
|
Tested alongside islandora-deprecated/claw_vagrant#40
|
NOT FOR IMMEDIATE MERGE
Add API-X to vagrant install: (Islandora/documentation#504)
fcrepo4-labs/fcrepo-api-x#112
What does this Pull Request do?
This PR adds an
HttpClientto the OSGi service registry that uses a configurable but static token to try to authenticate to its server. The intended "user" of this service is API-X, which needs to be able to act with authentication against a Syn-protected Fedora repository to install its own configuration.What's new?
There is now an authenticating client as described above available in the OSGi service registry.
How should this be tested?
This PR will only be useful once API-X is modified to accept an authenticating client for use setting up its configuration. @birkland is willing to work on that problem.
Interested parties
@dhlamb @birkland @jonathangreen @whikloj