Skip to content

[v7r1] Fix proxyDB test#4469

Merged
atsareg merged 52 commits into
DIRACGrid:rel-v7r1from
TaykYoku:orig_v7r1_fixtest
Apr 8, 2020
Merged

[v7r1] Fix proxyDB test#4469
atsareg merged 52 commits into
DIRACGrid:rel-v7r1from
TaykYoku:orig_v7r1_fixtest

Conversation

@TaykYoku

@TaykYoku TaykYoku commented Feb 27, 2020

Copy link
Copy Markdown
Contributor

This PR add debug and test about problem with VOMS proxy creation with this tests utilits, add new method to DIRACCA that try to create a fake proxy for tests.

BEGINRELEASENOTES

*ProxyDB
FIX: add new test, debug, correct proxyProvider.getProxy argument

*Resources/DIRACCA
NEW: add method to do fake proxy, needed for tests, add tests for DIRACCA

*Core
FIX: add exception to ProxyInfo

ENDRELEASENOTES

@chaen

chaen commented Feb 27, 2020

Copy link
Copy Markdown
Contributor

But why does it work with pyGSI ?

@andresailer

Copy link
Copy Markdown
Contributor

There are also problems in the assert calls

self.assertTrue(result['OK'], '\n%s' % result.get('Message') or 'Error message is absent.')

The "%" operator has precedence over "or". This should be for example.

self.assertTrue(result['OK'], '\n' + result.get('Message', 'Error message is absent.'))

And also in the rest of the cases in this file.

@TaykYoku

Copy link
Copy Markdown
Contributor Author

@chaen I don't know, need to more tests.. because I tested voms-proxy-fake and it didn't add VOMS extention as expected, so ProxyDB successfully uploaded proxy(without VOMS extention that is normal).

@TaykYoku

TaykYoku commented Feb 27, 2020

Copy link
Copy Markdown
Contributor Author

@chaen the result depended from chain.isVOMS():

FrameworkSystem/DB/ProxyDB
...
_ result = chain.isVOMS()
_ if result['OK'] and result.get('Value'):
____ return S_ERROR("Proxies with VOMS extensions are not allowed to be uploaded")
...

if this method return different results with/without pyGSI that can be a reason.

@fstagni

fstagni commented Feb 27, 2020

Copy link
Copy Markdown
Contributor

There shouldn't be any difference at all between pyGSI and M2Crypto, not at this level for sure.

Comment thread tests/Integration/Framework/Test_ProxyDB.py Outdated
Comment thread tests/Integration/Framework/Test_ProxyDB.py Outdated
@fstagni

fstagni commented Feb 28, 2020

Copy link
Copy Markdown
Contributor

Can you merge https://github.com/DIRACGrid/DIRAC/pull/4461/files in this branch?

@chaen

chaen commented Feb 28, 2020

Copy link
Copy Markdown
Contributor

Yes, pelase do not comment out the VOMS tests, they must work

@TaykYoku TaykYoku requested a review from atsareg as a code owner February 28, 2020 15:53
@TaykYoku

TaykYoku commented Mar 3, 2020

Copy link
Copy Markdown
Contributor Author

@chaen I don't know, need to more tests.. because I tested voms-proxy-fake and it didn't add VOMS extention as expected, so ProxyDB successfully uploaded proxy(without VOMS extention that is normal).

I was wrong.. voms-proxy-fake working perfect.

@TaykYoku

TaykYoku commented Mar 3, 2020

Copy link
Copy Markdown
Contributor Author

At least, VOMS client tools can see extension.

$ voms-proxy-fake --hostcert /opt/dirac/pro/DIRAC/Core/Security/test/certs/host/hostcert.pem --hostkey /opt/dirac/pro/DIRAC/Core/Security/test/certs/host/hostkey.pem -uri fakeserver.cern.ch:15000 -voms "vo_1" -fqan "/vo_1/Role=None/Capability=NULL" -key /opt/dirac/pro/DIRAC/Core/Security/test/certs/user/userkey.pem -cert /opt/dirac/pro/DIRAC/Core/Security/test/certs/user/usercert.pem
Creating proxy ........................................ Done
Your proxy is valid until Wed Mar  4 00:48:03 2020
$ voms-proxy-info -all
subject   : /O=Dirac Computing/O=CERN/CN=MrUser/CN=proxy
issuer    : /O=Dirac Computing/O=CERN/CN=MrUser
identity  : /O=Dirac Computing/O=CERN/CN=MrUser
type      : full legacy globus proxy
strength  : 1024
path      : /tmp/x509up_u25133
timeleft  : 11:59:47
key usage : Digital Signature, Key Encipherment
=== VO vo_1 extension information ===
VO        : vo_1
subject   : /O=Dirac Computing/O=CERN/CN=MrUser
issuer    : /O=Dirac Computing/O=CERN/CN=VOBox
attribute : /vo_1/Role=None/Capability=NULL
timeleft  : 11:59:47
uri       : fakeserver.cern.ch:15000

Comment thread Core/Security/ProxyInfo.py
Comment thread Core/Security/ProxyInfo.py Outdated
@chrisburr

Copy link
Copy Markdown
Member

I guess this should be closed now that #4490 has been created?

(If not there is a bad merge in the server tests)

@TaykYoku

TaykYoku commented Mar 5, 2020

Copy link
Copy Markdown
Contributor Author

Question, we need the changes that I merged from #4461?

@TaykYoku

TaykYoku commented Mar 6, 2020

Copy link
Copy Markdown
Contributor Author

Can you merge https://github.com/DIRACGrid/DIRAC/pull/4461/files in this branch?

@fstagni could you, please, confirm that this merge still needed?

@fstagni

fstagni commented Mar 6, 2020

Copy link
Copy Markdown
Contributor

I don't know anymore, I am lost in all these changes right now.

@chrisburr

Copy link
Copy Markdown
Member

As #4461 is still open, I think it's probably better to merge that PR as this one only adds changes for debugging?

@TaykYoku TaykYoku force-pushed the orig_v7r1_fixtest branch from 91d33f1 to b3c9520 Compare March 6, 2020 16:06
@chaen

chaen commented Mar 10, 2020

Copy link
Copy Markdown
Contributor

From the tests I have done, (#4490 (comment)) the fact of having an underscore or a digit in the Subject does not seem to cause issue.
So by changing that, you are probably hiding the real problem, which would be nice to resolve.
If you send me the proxy that you generate and that is faulty, I can have a look.

Comment thread Resources/ProxyProvider/DIRACCAProxyProvider.py Outdated
@TaykYoku

Copy link
Copy Markdown
Contributor Author

if so, what other cases do you suggest adding to make this test complete enough?

@chaen

chaen commented Mar 10, 2020

Copy link
Copy Markdown
Contributor

So, after looking at the proxy you sent me, I noticed that the DN is using some T61String type, which are encoded with ISO 8859-1, i.e. Latin Characters. I have no idea where that comes from, but potentially from your local language settings (we've seen this in the past).
I will prepare a PR for you to test.
The only thing I am afraid is that these ISO 8859-1 makes it to the DB, and then is unusable by some storage/CE. That was an issue in the past one, and it is very very very difficult to debug

@TaykYoku

Copy link
Copy Markdown
Contributor Author

I leave both cases (/O=DN/O=DIRAC/CN=user_1 & /O=DN/O=DIRAC/CN=user), but I not agree that by modifying that, I trying to hiding the real problem : ) because the goal of modified block is to check specific case(Stored proxy already have different VOMS extension, not decoding process) and I reported about this exception. Just need to add all absent test cases.
IMHO, described exceptions comes from Core/Security/VOMS.py methods, so maybe better to use loop for all generated proxies types in separately test instead of doing it in the context of ProxyDB methods tests?

@atsareg

atsareg commented Apr 2, 2020

Copy link
Copy Markdown
Contributor

@chaen : we have to conclude with this one. If you have some really important requests for changes, please do. If there can be different ways to achieve the same thing, let's favor the one which is already coded. I would like to merge this one to resume the normal testing and proceed with further PRs

@chaen

chaen commented Apr 2, 2020

Copy link
Copy Markdown
Contributor

As I explained during the BILD, I have a hard time telling what is important and what is not. I also would have liked other people to look at the PR.
And yes, I think I've put enough effort into helping to get the tests green that you rest assured I want this to be done quickly :-)
Now, it's not about finding a better way to do, but I still believe that purely test functions, that deliver a fully functional proxy with no checks, has absolutely nothing to do in a production code. And I do not think that is an unreasonably picky argument.

1 similar comment
@chaen

chaen commented Apr 2, 2020

Copy link
Copy Markdown
Contributor

As I explained during the BILD, I have a hard time telling what is important and what is not. I also would have liked other people to look at the PR.
And yes, I think I've put enough effort into helping to get the tests green that you rest assured I want this to be done quickly :-)
Now, it's not about finding a better way to do, but I still believe that purely test functions, that deliver a fully functional proxy with no checks, has absolutely nothing to do in a production code. And I do not think that is an unreasonably picky argument.

@atsareg

atsareg commented Apr 2, 2020

Copy link
Copy Markdown
Contributor

You say it should not be like that. Then suggest a solution. This function relies on the internal functions of the class. Extracting it from the class is not evident. Making yet another class inheriting this one and making internal function protected instead of private (oh my) just because it seems to you that this should be "not in production" code does not seem too much justifying the change. Having this function does not harm the production code and helps testing. It can be useful later, although not sure, for resource monitoring. I do not think that this should be a blocking issue for this PR

@fstagni fstagni left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally don't see blocking issues here.

Comment thread Resources/ProxyProvider/PUSPProxyProvider.py
Comment thread Resources/ProxyProvider/PUSPProxyProvider.py Outdated
Comment thread Resources/ProxyProvider/PUSPProxyProvider.py Outdated
@chaen

chaen commented Apr 3, 2020

Copy link
Copy Markdown
Contributor

OK

@TaykYoku

TaykYoku commented Apr 3, 2020

Copy link
Copy Markdown
Contributor Author

IIUC, we have other PRs that cannot be in progress because this PR contains some test fixes. If it's true, IMHO, I can open a new separate PR about ProxyProvider resources where we can continue. And merge this PR that gives us the ability to pass tests in other PRs and continue browsing all in parallel.

@TaykYoku

TaykYoku commented Apr 3, 2020

Copy link
Copy Markdown
Contributor Author
dirac-install [ERROR] http://diracproject.web.cern.ch/diracproject/tars/release-DIRAC-v7r1-pre22.cfg does not exist

Comment thread FrameworkSystem/DB/ProxyDB.py
Comment thread Resources/ProxyProvider/ProxyProvider.py Outdated
Comment thread Resources/ProxyProvider/DIRACCAProxyProvider.py Outdated
Comment thread Resources/ProxyProvider/DIRACCAProxyProvider.py Outdated
@TaykYoku

TaykYoku commented Apr 4, 2020

Copy link
Copy Markdown
Contributor Author

Integration tests
##[error]The operation was canceled.

Comment thread Resources/ProxyProvider/DIRACCAProxyProvider.py Outdated
@TaykYoku

TaykYoku commented Apr 6, 2020

Copy link
Copy Markdown
Contributor Author

After the documents changed, the next test failed:
ClientInstallDIR/DIRAC/tests/Integration/AccountingSystem/Test_DataStoreClient.py:67: AssertionError
IMHO, unlikely this is linked with the last commit

@fstagni

fstagni commented Apr 6, 2020

Copy link
Copy Markdown
Contributor

That one appears for only one combination of the matrix of integration jobs, so most probably it is a random error depending from a bad execution, and has nothing to do with what you did here.

@fstagni fstagni left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dare to approve.

@atsareg atsareg merged commit c063eb4 into DIRACGrid:rel-v7r1 Apr 8, 2020
@TaykYoku TaykYoku deleted the orig_v7r1_fixtest branch September 6, 2021 13:37
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.

6 participants