[8.0] Pilot submission with tokens #6750
Conversation
DIRACGridBot
added
the
alsoTargeting:integration
atsareg
force-pushed
the
dev-ce-tokens-2
branch
from
d13fdf6 to
7757f55
Compare
atsareg
force-pushed
the
dev-ce-tokens-2
branch
from
7757f55 to
f5260b2
Compare
| self.log.verbose("Pilot Statuses: %s " % resultDict) | ||
| return S_OK(resultDict) | ||
|
|
||
| def _treatCondorHistory(self, condorHistCall, qList): |
There was a problem hiding this comment.
Did you move this from DIRAC.Resources.Computing.BatchSystems.Condor.treatCondorHistory? Why?
Anyway, you can remove the import now but mostly remove the other code to avoid duplications?
| :param qList: list of jobID and status from condor_q output, will be modified in this function | ||
| :type qList: python:list |
There was a problem hiding this comment.
| :param qList: list of jobID and status from condor_q output, will be modified in this function | |
| :type qList: python:list | |
| :param list qList: list of jobID and status from condor_q output, will be modified in this function |
|
I hope to have addressed all the comments. Please, rereview |
| @@ -177,7 +177,7 @@ def __checkProperties(self, requestedUserDN: str, requestedUserGroup: str): | |||
| @gTokensSync | |||
| def export_getToken( | |||
| self, | |||
| username: str, | |||
| userName: str, | |||
There was a problem hiding this comment.
Does this need to be changed? It becomes inconsistent with other usages like getDNForUsername.
There was a problem hiding this comment.
We do have conventions on variables naming that we should follow. I will correct for that also the Registry helper
|
|
||
| # Set the token in the environment if needed | ||
| if self.token: | ||
| os.environ["BEARER_TOKEN"] = self.token["access_token"] |
There was a problem hiding this comment.
Should this be cleaned from the environment after the function returns?
There was a problem hiding this comment.
I do not think it is necessary. This is only the environment of a single SiteDirector for which there is a single possible value.
|
|
||
| def setParameters(self, parameters): | ||
| def setParameters(self, parameters: dict): |
There was a problem hiding this comment.
dict isn't a valid type hint. The types of the keys should be included in square brackets.
There was a problem hiding this comment.
Never used it before. Should we probably discuss how to use type hints in the DIRAC code ? Use it systematically, or just when updating codes, or... For example, the syntax without square brackets is apparently also valid as the tests pass.
There was a problem hiding this comment.
There is an issue for this: #6315
I think that as for other cases of syntax updates we should rely on existing tools rather than try to do it ourselves.
There was a problem hiding this comment.
For example, the syntax without square brackets is apparently also valid as the tests pass.
cpython doesn't use the type hints at runtime and we don't have any CI jobs to check them so you can put almost anything without breaking the tests.
| @@ -560,3 +560,27 @@ def getUserProfile(self): | |||
| except Exception as e: | |||
| self.log.exception(e) | |||
| return S_ERROR("Cannot get user profile: %s" % repr(e)) | |||
|
|
|||
| def getTokenFromAgent(self, userName: str, requiredScope=None, requiredTimeLeft=3500): | |||
There was a problem hiding this comment.
I thought we decided not to have any code for oidc-agent?
There was a problem hiding this comment.
I think this does not harm and can be useful without any extra charge
| else: | ||
| err.append(result["Message"]) | ||
|
|
||
| # No luck so far, let's refresh the token stored in the database |
There was a problem hiding this comment.
I'm struggling to understand where this is using the REST interface of IAM/CheckIn to get the tokens. Am I missing somthing?
There was a problem hiding this comment.
If idpObj is an instance of OAuth2IdProvider, then it calls authlib.OAuthClient.refreshToken() which makes the REST request.
(OAuth2IdProvider inherits from OAuth2Session, which inherits from authlib.OAuth2Session, which inherits from authlib.OAuth2Client)
| @@ -279,6 +279,8 @@ Agents | |||
| CETypes = any | |||
| # List of CEs that will be treated by this SiteDirector ("any" can refer to any type of CE defined in the CS) | |||
| CEs = any | |||
| # List of Tags that are required to be present in the CE/Queue definition | |||
| Tags = | |||
There was a problem hiding this comment.
Should it be more like "RequiredTags"?
There was a problem hiding this comment.
No, these are not the Tags required by the site to be present in the Jobs. This is the semantics of the RequiredTags option. This is to say that the SiteDirector only serves sites with certain properties.
|
I do not see the problematic commit message in the interactive rebase, so I can not fix it. Any idea how this can be overcome ? |
|
Squash all your commits in one or two (it's much nicer in general) |
atsareg
force-pushed
the
dev-ce-tokens-2
branch
from
ef7b285 to
a37926d
Compare
|
Something went wrong with squashing commits. Redoing the PR |
This PR adds possibility to submit pilots with tokens. It replaces #6580 PR which was corrupted and restarted anew here.
See review comments and answers there.
The tokens are obtained by SiteDirectors via a standard mechanism from the TokenManager. Using OAuth2 IdProviders with pilot tokens generated from the long tokens in the TokenDB are supported.
In order to use the oidc-agent instead of the OAuth2 flow, a new OidcAgentIdProvider is added. This can be useful in some environments, also for debugging. The oidc-agent is supposed to have a client created with the name of the user submitting pilots and having necessary compute scopes.
SiteDirector is enabled to select queues by Tag values. Queues can define Token tag to be used with tokens, otherwise with certificates. Later on, the default will be switched to Token I hope :).
The following ComputingElements should be enabled with the tokens:
HTCondorCE
ARC/ARC6
AREX
The case of CloudComputingElement is not addressed specifically as it can be solved by using Application Credentials authentication mechanism where Application Credentials can be created after authentication with OAuth2 tokens.
BEGINRELEASENOTES
*Framework
NEW: OidcAgentIdProvider identity provider class
FIX: TokenManager - use refreshToken flow to generate access tokens from the stored refresh tokens
*WorkloadManagement
NEW: SiteDirector enabled to select queues by tags
NEW: SiteDirector sets up tokens for ComputingElements configured with the Token tag
*Resources
NEW: HTCondorComputingElement, ARC(6)ComputingElement and AREXComputingElement enabled to for job operations with tokens
ENDRELEASENOTES