feat: support for oidc credential /test endpoint#16370
feat: support for oidc credential /test endpoint#16370fincamd wants to merge 1 commit intoansible:develfrom
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
| ) | ||
| backend_kwargs['workload_identity_token'] = workload_identity_token | ||
| from jwt import decode as _jwt_decode | ||
| response_body['sent_jwt_payload'] = _jwt_decode(workload_identity_token, algorithms=["RS256"], options={"verify_signature": False}) |
There was a problem hiding this comment.
In the conversation we had around the PoC we were thinking about nesting sent_jwt_payload under something more generic:
{
"details": {
"sent_jwt_payload": "..."
}
}This pattern is followed some lines above, but not here. Was this intentional?
| return Response({'detail': _('Job template ID is required.')}, status=status.HTTP_400_BAD_REQUEST) | ||
| job_template = models.JobTemplate.objects.get(id=int()) | ||
| if not request.user.can_access(models.JobTemplate, 'read', job_template): | ||
| raise PermissionDenied(_(f'You do not have access to job template with id: {job_template.id}.')) |
There was a problem hiding this comment.
What happens in this case? Do we get a 500 HTTP error? Or something else? Shouldn't we be returning another HTTP_400_BAD_REQUEST here, instead of raising an exception?
There was a problem hiding this comment.
| # If workload identity token field exists, add job_template to metadata | ||
| if found_wit_field: | ||
| metadata = value['inputs'].get('metadata', []) | ||
| metadata.append({ | ||
| "id": "job_template_id", | ||
| "label": "ID of a Job Template", | ||
| "type": "string", | ||
| "help_text": "Job template ID to use when generating a token." | ||
| }) | ||
| value['inputs']['metadata'] = metadata | ||
|
|
There was a problem hiding this comment.
We talked about making this a dynamic input in the UI instead of making it a dynamic field in the backend.
Both solutions are ugly, but my personal take is that the UI workaround is less ugly. Please take a look that the drafted PR linked in AAP-64624 (the corresponding UI change). Thoughts?
There was a problem hiding this comment.
Agreed that it's less ugly for the UI to special-case the inclusion of job_template_id when performing a test. This just came up on Slack and I said the same thing 😄 . So I think we can delete 2945-2955
There was a problem hiding this comment.
Kudos to @matoval for testing this with the UI. Some early feedback:
- Both
CredentialExternalTestandCredentialTypeExternalTestviews need to implement the new behavior. Right now it's just happening inCredentialExternalTest - Calling the API with a
job_template_idfails because the plugin's backend function doesn't expect this argument. Matthew suggested to.pop()the job template id instead of.get(). I don't remember running into this problem in my PoC 🤔, but we obviously don't want it to fail. - The test operation re-uses
retrieve_workload_identity_jwtfunction from jobs.py to get a JWT, but that's intended for jobs not templates and isn't working. We'll need to do something different there - we can't use that function as-is with job templates, it's meant for jobs.
| raise PermissionDenied(_(f'You do not have access to job template with id: {job_template.id}.')) | ||
|
|
||
| # Get a Workload Identity Token | ||
| workload_identity_token = retrieve_workload_identity_jwt( |
There was a problem hiding this comment.
Credit to @matoval but this function expects a job, not a job template.
3 participants
SUMMARY
ISSUE TYPE
COMPONENT NAME
STEPS TO REPRODUCE AND EXTRA INFO