Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

function provider access to logs #1403

Merged
merged 3 commits into from
Jul 15, 2024
Merged

Conversation

akihikokuroda
Copy link
Collaborator

Summary

Part of #1400

Details and comments

This PR makes the function provider access to the logs for the job executing its function.

Comment on lines 442 to 446
if author != job.author:
if job.program and job.program.provider:
if job.program.provider.admin_group in author.groups.all():
return Response({"logs": logs})
return Response({"logs": "No available logs"})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to simplify the logic here to something like

if provider function run by end user:
    return "No logs"
return "Logs"

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh, I miss implemented it. The log is returned for the user only when the function is not used. I'll do rework. Thanks!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@psschwei Logs still must be returned for the user if the job is not executing the function from the provider.

@akihikokuroda akihikokuroda marked this pull request as draft July 10, 2024 16:52
@akihikokuroda akihikokuroda marked this pull request as ready for review July 10, 2024 17:14
Comment on lines +442 to +448
if job.program and job.program.provider:
if job.program.provider.admin_group in author.groups.all():
return Response({"logs": logs})
return Response({"logs": "No available logs"})
if author == job.author:
return Response({"logs": logs})
return Response({"logs": "No available logs"})
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm still wondering if we can simplify the logic a bit here (and if I'm over-complicating things let me know):

Assuming the only time we don't want to return logs is if the user is running a third-party / provider function (and correct me if I'm wrong here), could we do a simple check for that condition (is this an end user running a provider function) and if so then don't return logs, otherwise return the logs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@psschwei Here is what I suppose to implement.

There are 4 cases:

  1. job is executing the provider function
    a. request from the provider -- return log
    b. request from someone else (including the job author) -- no log
  2. Job is executing private function
    a. request from the job author -- return log
    b. request from someone else -- no log

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would the 2B scenario get caught higher up? I'm assuming we don't allow User B to access User A's jobs, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I changed it to allow the others access to the log. Otherwise the provider can not access to the job executed by the user.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ahh, ok - do we need to block anything else in the 2B case (results for example)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, it's not necessary. Only this function is handling the jobs of all users.

Copy link
Member

@Tansito Tansito left a comment

Choose a reason for hiding this comment

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

Just only comment about the logic, Aki 👍

@@ -436,9 +436,16 @@ def logs(self, request, pk=None): # pylint: disable=invalid-name,unused-argumen
tracer = trace.get_tracer("gateway.tracer")
ctx = TraceContextTextMapPropagator().extract(carrier=request.headers)
with tracer.start_as_current_span("gateway.job.logs", context=ctx):
job = self.get_object()
job = Job.objects.filter(pk=pk).first()
Copy link
Member

Choose a reason for hiding this comment

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

I would add a if job is None return Reponse(404) if not it will break if job doesn't exist.

@Tansito Tansito self-requested a review July 15, 2024 21:00
@akihikokuroda akihikokuroda merged commit acf1323 into Qiskit:main Jul 15, 2024
14 checks passed
@akihikokuroda akihikokuroda deleted the functionlog branch July 15, 2024 22:54
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.

3 participants