[WFLY-12190] EJB: EJB over HTTP discovery configuration #12887

tadamski · 2020-01-08T00:35:18Z

https://issues.redhat.com/browse/EAP7-1236
https://issues.redhat.com/browse/WFLY-12190

Depends on:
#13305

Thanks for submitting your Pull Request!

Please make sure your PR meets the following requirements:

Pull Request title is properly formatted: [WFLY-XYZ] Subject or WFLY-XYZ Subject
Pull Request contains link to the JIRA issue(s)
Pull Request contains description of the issue(s)
Pull Request does not include fixes for issues other than the main ticket
Attached commits represent units of work and are properly formatted

For bigger changes, major and minor component upgrades make sure your PR also meets following requirements:

Pull Request requires a change to the documentation
Documentation have been updated accordingly
Tests were added to cover changes

For new features ensure as well:

Analysis was done
Test Plan has been done
Tests were verified in advance

If you are not an active contributor of the WildFly project you can request sponsorship by one of the members to help guide you through the process.

ejb3/src/main/resources/org/jboss/as/ejb3/subsystem/LocalDescriptions.properties

darranl

The model description constants need defining.

However as on the analysis before we add a new server to server protocol we need to review the security implications.

tadamski · 2020-01-13T18:59:33Z

@darranl I have updated the description. I have also added the possibility of HTTP discovery from standalone client (in attached PRs). Regarding security, EJB over HTTP protocol is integrated with Elytron and the invocations are secured. Specifically, I had to provide Elytron configuration for the tests to pass (discovery invocations to succeed). I agree that it has to be double-checked though.

darranl · 2020-01-14T09:07:10Z

@tadamski I know the server side is already covered by Elytron security, the bit I am not sure about is the client side of these server to server connection. In this case there are a few scenarios that may need some integration: -

SSLContext
Authentication Configuration
Identity Propagation using Delegated Credential

tadamski · 2020-01-15T11:18:30Z

@darranl OK, I will try to investigate those but I would probably need some help. OTOH server->server invocations are already working in WildFly (with static discovery) so we may need to double-check that those are fine as well.

bstansberry · 2020-01-15T20:58:13Z

@tadamski #12891 no longers includes the http-client part of this; it's just a bug fix update. The http-client part of this is now in a 1.1 release so this PR should include the upgrade to that.

pferraro

This change requires a model version bump, as well as transformers and transformation tests.

darranl · 2020-04-30T10:51:32Z

@tadamski we haven't really followed up on this together, it looks like it has a merge conflict anyway but do we want to set something up so we can look at the security implications in more detail?

tadamski · 2020-04-30T11:24:33Z

@darranl I have to update this PR but this is mostly config stuff. The key PR is wildfly/wildfly-http-client#29 on which this PR relies. I would be happy to discuss it.

luck3y · 2020-05-04T20:35:19Z

test, please ignore

jamezp · 2020-05-19T18:18:43Z

I've removed the missing-reqs label as there was an agreement the test development can be late on this feature.

fl4via · 2020-05-19T22:14:00Z

This PR depends on #13305

tadamski · 2020-07-13T08:53:47Z

@jamezp merged; failing test seem unrelated; this should be ready to merge

zhantemirov · 2020-08-20T07:50:49Z

I see all checks passing, do anything else prevent this PR from merging? Thanks

bstansberry

I've made some comments.

For this one to move forward we should get an approval from @darranl.

...-pack/common/src/main/resources/modules/system/layers/base/org/jboss/as/ejb3/main/module.xml

ee/src/main/java/org/jboss/as/ee/metadata/EJBClientDescriptorMetaData.java

docs/src/main/asciidoc/_admin-guide/subsystem-configuration/EJB3.adoc

bstansberry · 2020-09-05T13:44:24Z

docs/src/main/asciidoc/_admin-guide/subsystem-configuration/EJB3.adoc

+==== <remote-http-connection>
+
+`remote-http-connection` tag is used to define dynamic discovery based on
+HTTP procol:


s/procol/protocol

I think the main question I would like to ask is - is there sufficient test coverage of clients using authentication client configuration for a dynamically discovered destination?

As mentioned earlier we have aspects such as SSL and the credentials for authentication which should already be covered by the EJB client to verification that the behaviour is correct. In this case I believe it will be preferable for the resolved configuration to be based on the discovered destination.

The test coverage for the dynamically discovered destinations would require additional analysis. OTOH, dynamic discovery is main route of invocation also for remoting and this commit just adds this feature for HTTP. Currently EAP/WildFly already relies on dynamic invocation and if we decide that the testsuite have to be extended it should be done for a number of scenarios, possibly independently of this PR.

Ok, lets go with what we have here - if you are happy HTTP is just using the same pattern as the existing approaches we should be good.

Hmm, but just to clarify cause I'm not sure if I understand correctly. Discovery provider and HttpEjbReceiver (which is used to perform invocation to discovered destination) (also other receivers) resolve their AuthenticationContexts independently:
https://github.com/wildfly/wildfly-http-client/blob/master/ejb/src/main/java/org/wildfly/httpclient/ejb/HttpEJBReceiver.java#L132

Should this be modified?

bstansberry · 2020-09-10T00:59:11Z

/retest

bstansberry · 2020-09-10T01:01:13Z

The issues I've raised have all been addressed, but the discussion with Darran remains open.

darranl · 2020-09-10T08:55:20Z

.../src/test/java/org/jboss/as/test/multinode/ejb/http/EjbOverHttpWrongCredentialsTestCase.java

@@ -0,0 +1,85 @@
+package org.jboss.as.test.multinode.ejb.http;


Missing header

darranl · 2020-09-10T09:05:28Z

I have one comment for testing that I don't think should hold up this PR so is something more general. At the moment our testsuite is becoming very slow to execute so the pattern of Configure -> Deploy -> Single Test -> Undeploy -> Revert Configuration adds a lot of steps for one test. Ideally if we can find ways a common deployment can be used for multiple scenarios we can execute multiple tests for a single pair or configure and pair of deploy tasks. Ideally finding ways to test the success and failure scenarios under the same test case.

But in the case of this specific change I see we have some simple authentication client configuration where a single configuration is available but I would still like to see something where we test a configuration where the correct configuration is only resolved / resolvable after discovery. I don't necessarily have an issue with that being in a follow up PR if we can make sure it happens.

The reason I am seeing this piece as important is because it will be an area of code we must maintain for backwards compatibility so once we release a Final version we will not be able to change the resolution logic without the risk of breaking something developed against the original behaviour.

…pDescriptorTestCase

…Case

bstansberry

@tadamski This needs an update to the transformers to the previous management API version:

https://github.com/wildfly/wildfly/blob/master/ejb3/src/main/java/org/jboss/as/ejb3/subsystem/EJBTransformers.java#L289

Ejb3SubsystemUnitTestCase and Ejb3TransformersTestCase should cover it as well.

bstansberry · 2020-09-16T15:39:13Z

@tadamski https://issues.redhat.com/browse/WFLY-13878 is to track my last request.

chengfang · 2020-09-16T23:59:52Z

@bstansberry I'll update the transformer to handle the new element remote-http-connection

darranl added Feature PR provides a new feature missing-reqs Features missing one or more of the pre-merge requirements labels Jan 8, 2020

darranl reviewed Jan 8, 2020

View reviewed changes

ejb3/src/main/resources/org/jboss/as/ejb3/subsystem/LocalDescriptions.properties Outdated Show resolved Hide resolved

darranl requested changes Jan 10, 2020

View reviewed changes

tadamski force-pushed the WFLY-12190 branch from 348863d to 780f982 Compare January 13, 2020 18:51

pferraro requested changes Jan 28, 2020

View reviewed changes

tadamski force-pushed the WFLY-12190 branch from 780f982 to 4624281 Compare March 12, 2020 10:40

tadamski force-pushed the WFLY-12190 branch 2 times, most recently from a45fab7 to a79e3f9 Compare May 7, 2020 09:58

jamezp added rebase-this PR has a merge conflict. and removed missing-reqs Features missing one or more of the pre-merge requirements labels May 19, 2020

bstansberry mentioned this pull request Jun 9, 2020

[WFLY-13492] Upgrade WildFly HTTP Client to 1.1.1.Final #13305

Closed

zhantemirov mentioned this pull request Jun 11, 2020

[WFLY-12190]: Add EjbOverHttpWrongCredentialsTestCase #13365

Closed

tadamski force-pushed the WFLY-12190 branch from a79e3f9 to 9af08db Compare July 11, 2020 09:46

wildfly-ci added the deps-ok Dependencies have been checked, and there are no significant changes label Jul 11, 2020

tadamski force-pushed the WFLY-12190 branch 2 times, most recently from d57f27d to dfeb23f Compare July 12, 2020 20:37

jamezp removed the rebase-this PR has a merge conflict. label Jul 13, 2020

tadamski force-pushed the WFLY-12190 branch from dfeb23f to 9b65bdb Compare July 21, 2020 06:49

tadamski force-pushed the WFLY-12190 branch from 9b65bdb to 0e609e1 Compare August 10, 2020 09:53

bstansberry requested changes Sep 5, 2020

View reviewed changes

tadamski force-pushed the WFLY-12190 branch 3 times, most recently from d9af418 to a758a31 Compare September 9, 2020 09:21

bstansberry assigned darranl Sep 10, 2020

darranl reviewed Sep 10, 2020

View reviewed changes

tadamski and others added 5 commits September 10, 2020 12:00

[WFLY-13492] Upgrade Wildfly HTTP Client to 1.1.2.Final

9012c2a

[WFLY-12190] EJB over HTTP discovery: configuration

425c73d

[WFLY-12190] EJB over HTTP discovery: EjbOverHttpTestCase, EjbOverHtt…

986eb9c

…pDescriptorTestCase

[WFLY-12190] EJB over HTTP discovery: community docs

7e91f23

[WFLY-12190] EJB over HTTP discovery: EjbOverHttpWrongCredentialsTest…

8ea2b20

…Case

tadamski force-pushed the WFLY-12190 branch from a758a31 to 8ea2b20 Compare September 10, 2020 10:07

bstansberry added the 21.x label Sep 15, 2020

bstansberry requested changes Sep 15, 2020

View reviewed changes

bstansberry mentioned this pull request Sep 17, 2020

Pulls #13579

Closed

bstansberry merged commit 850852d into wildfly:master Sep 17, 2020

chengfang mentioned this pull request Sep 17, 2020

WFLY-13878 Add transformer and subsystem test coverage for EJB over H… #13582

Merged

tadamski deleted the WFLY-12190 branch July 6, 2022 12:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WFLY-12190] EJB: EJB over HTTP discovery configuration #12887

[WFLY-12190] EJB: EJB over HTTP discovery configuration #12887

tadamski commented Jan 8, 2020 •

edited

darranl left a comment

tadamski commented Jan 13, 2020

darranl commented Jan 14, 2020

tadamski commented Jan 15, 2020

bstansberry commented Jan 15, 2020

pferraro left a comment •

edited

darranl commented Apr 30, 2020

tadamski commented Apr 30, 2020

luck3y commented May 4, 2020

jamezp commented May 19, 2020

fl4via commented May 19, 2020

tadamski commented Jul 13, 2020

zhantemirov commented Aug 20, 2020

bstansberry left a comment

bstansberry Sep 5, 2020

tadamski Sep 8, 2020

darranl Sep 8, 2020

tadamski Sep 10, 2020 •

edited

darranl Sep 10, 2020

tadamski Sep 10, 2020

bstansberry commented Sep 10, 2020

bstansberry commented Sep 10, 2020

darranl Sep 10, 2020

tadamski Sep 10, 2020

darranl commented Sep 10, 2020

bstansberry left a comment

bstansberry commented Sep 16, 2020

chengfang commented Sep 16, 2020

		@@ -0,0 +1,85 @@
		package org.jboss.as.test.multinode.ejb.http;

[WFLY-12190] EJB: EJB over HTTP discovery configuration #12887

[WFLY-12190] EJB: EJB over HTTP discovery configuration #12887

Conversation

tadamski commented Jan 8, 2020 • edited

darranl left a comment

Choose a reason for hiding this comment

tadamski commented Jan 13, 2020

darranl commented Jan 14, 2020

tadamski commented Jan 15, 2020

bstansberry commented Jan 15, 2020

pferraro left a comment • edited

Choose a reason for hiding this comment

darranl commented Apr 30, 2020

tadamski commented Apr 30, 2020

luck3y commented May 4, 2020

jamezp commented May 19, 2020

fl4via commented May 19, 2020

tadamski commented Jul 13, 2020

zhantemirov commented Aug 20, 2020

bstansberry left a comment

Choose a reason for hiding this comment

bstansberry Sep 5, 2020

Choose a reason for hiding this comment

tadamski Sep 8, 2020

Choose a reason for hiding this comment

darranl Sep 8, 2020

Choose a reason for hiding this comment

tadamski Sep 10, 2020 • edited

Choose a reason for hiding this comment

darranl Sep 10, 2020

Choose a reason for hiding this comment

tadamski Sep 10, 2020

Choose a reason for hiding this comment

bstansberry commented Sep 10, 2020

bstansberry commented Sep 10, 2020

darranl Sep 10, 2020

Choose a reason for hiding this comment

tadamski Sep 10, 2020

Choose a reason for hiding this comment

darranl commented Sep 10, 2020

bstansberry left a comment

Choose a reason for hiding this comment

bstansberry commented Sep 16, 2020

chengfang commented Sep 16, 2020

tadamski commented Jan 8, 2020 •

edited

pferraro left a comment •

edited

tadamski Sep 10, 2020 •

edited