Skip to content

[LATER] Integration of HTTPS with Tornado#3815

Closed
louisjdmartin wants to merge 26 commits into
DIRACGrid:integrationfrom
louisjdmartin:stage_toDIRAC_clean
Closed

[LATER] Integration of HTTPS with Tornado#3815
louisjdmartin wants to merge 26 commits into
DIRACGrid:integrationfrom
louisjdmartin:stage_toDIRAC_clean

Conversation

@louisjdmartin

Copy link
Copy Markdown
Contributor

This PR is the result of my internship at CERN, it includes a new client and a new server which are capable to speak in HTTPS
It also includes some modifications in DIRAC core to integrate HTTPS
See also:

BEGINRELEASENOTES
*Tornado
NEW: HTTPS Client
NEW: HTTPS Server
NEW: Documentation...
NEW: Integration tests

*CS
NEW: Configuration/Server handler for HTTPS
NEW: Configuration/Server patch for JSON serialization (HTTPS)
CHANGE: Removing calls to RPCClient and use a child of DIRAC.Core.Base.Client instead in CSAPI, Refresher, ServiceInterface
CHANGE: Code factorisation & using IOLoop in the refresher
CHANGE: Code factorisation & using IOLoop in ServiceInterface

*Core
NEW: DIRAC.Core.Base.Client can choose between DISET and HTTPS

ENDRELEASENOTES

@coveralls

coveralls commented Aug 30, 2018

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.09%) to 22.155% when pulling 492f4a9 on louisjdmartin:stage_toDIRAC_clean into 379b48e on DIRACGrid:integration.

@atsareg

atsareg commented Aug 31, 2018

Copy link
Copy Markdown
Contributor

Very impressive indeed !
Some general comments. We are introducing a new client-server protocol. So, to keep the same code organization, the Tornado code should rather go to DIRAC.Core.Tornado (or DIRAC.Core.HTTPS) rather than to DIRAC.TornadoServices . So, this will be in parallel with the DISET package which it is mostly an homologue to.
Please, rename ConfigurationServerClient -> ConfigurationClient to keep a convention Client for all the clients

@louisjdmartin

Copy link
Copy Markdown
Contributor Author

Thanks,

I've changed the name to ConfigurationClient and moved Tornado into DIRAC.Core.Tornado. I push and test again on Jenkins just to be sure.

@fstagni

fstagni commented Sep 17, 2018

Copy link
Copy Markdown
Contributor

I scrolled through the code. My only purpose was to check about backward compatibility, to just answer the question "is this merge-able?" IIUC the logic for this is all in RPCClientSelector, and this looks correct to me, so it seems that this PR could be merged. I have anyway a few comments:

  • there are some code (almost) re-writes, namely in the tornado CS handler and in the tornado refresher. I understand the need and the purpose, but we should try to minimize them as much as possible. And we should remember them when applying changes.
  • there is a tests/Performance directory in the code where the performance integration tests sit (e.g. those done with multimechanize). Here they are in a different location.
  • there are some added scripts that starts with "tornado-.py". DIRAC scripts all start with "dirac-.py"
  • I would love to NOT merge the requested changes in the requirements.txt file, and yes I know we are waiting for Tornado maintainers to merge Chris's PR

@chaen

chaen commented Sep 25, 2018

Copy link
Copy Markdown
Contributor
  • there are some code (almost) re-writes, namely in the tornado CS handler and in the tornado refresher. I understand the need and the purpose, but we should try to minimize them as much as possible. And we should remember them when applying changes.

Yep, there is a lot of copy paste, and it is normally marked in the comment. The idea was to be very flexible, and once it stabilizes, factorize the code

  • there is a tests/Performance directory in the code where the performance integration tests sit (e.g. those done with multimechanize). Here they are in a different location.

OK, I will move them.

  • there are some added scripts that starts with "tornado-.py". DIRAC scripts all start with "dirac-.py"

Fair enough dirac-tornado-* suits you better ?

  • I would love to NOT merge the requested changes in the requirements.txt file, and yes I know we are waiting for Tornado maintainers to merge Chris's PR

Well, totally agree on that, I did not notice.

Anyway, as I said, I would like to wait to merge this one finally, first the M2Crypto one (that I write properly, and hope to have done by end of October after my vacations)

@fstagni

fstagni commented Sep 25, 2018

Copy link
Copy Markdown
Contributor

OK, are you going to make PR against @louisjdmartin repo?
Please add (more) comments in those "duplicate" codes to remember to update both. The risk is that, BEFORE this PR is merged, some other PR (from someone else) will modify the Refresher.py or the CS handler, and these may need to be ported here.

dirac-tornado-* looks OK to me.

Please also consider that there are some conflicts that should be resolved.
And, given your last comment, I would mark this PR as [WIP]

@fstagni fstagni changed the title [v7r0] Integration of HTTPS with Tornado [LATER] Integration of HTTPS with Tornado Oct 18, 2018
@fstagni

fstagni commented Oct 6, 2020

Copy link
Copy Markdown
Contributor

Superseded by #4677

@fstagni fstagni closed this Oct 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants