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

Misconfigured f-API can create infinite request loop #89

Open
2 tasks
surchs opened this issue May 23, 2024 · 4 comments
Open
2 tasks

Misconfigured f-API can create infinite request loop #89

surchs opened this issue May 23, 2024 · 4 comments
Assignees
Labels
documentation Changes only affect the documentation someday Not a priority right now, but we want to keep this around to think or discuss more.

Comments

@surchs
Copy link
Contributor

surchs commented May 23, 2024

If I

Then: I can create an infinite loop because once I send a query to my f-API, it will then send a query of its own to any APIs listed in local_nb_nodes.json - including itself. Because the f-API endpoints are so similar to the n-API endpoints, this will actually be a valid query request, thus starting another federated query and so on.

We should:

  • Confirm that this happens every time I set my own f-API in the local_nb_nodes.json and then write a test to make sure we prevent a loop from happening in this case
  • Add some basic check that lets and f-API realize when it is talking to itself
Copy link

github-actions bot commented Aug 7, 2024

We want to keep our issues up to date and active. This issue hasn't seen any activity in the last 75 days.
We have applied the _flag:stale label to indicate that this issue should be reviewed again.
When you review, please reread the spec and then apply one of these three options:

  • prioritize: apply the flag:schedule label to suggest moving this issue into the backlog now
  • close: if the issue is no longer relevant, explain why (give others a chance to reply) and then close.
  • archive: sometimes an issue has important information or ideas but we won't work on it soon. In this case
    apply the someday label to show that this won't be prioritized. The stalebot will ignore issues with this
    label in the future. Use sparingly!

@github-actions github-actions bot added the _flag:stale [BOT ONLY] Flag issue that hasn't been updated in a while and needs to be triaged again label Aug 7, 2024
@alyssadai alyssadai added flag:schedule Flag issue that should go on the roadmap or backlog. and removed _flag:stale [BOT ONLY] Flag issue that hasn't been updated in a while and needs to be triaged again flag:schedule Flag issue that should go on the roadmap or backlog. labels Oct 23, 2024
@alyssadai alyssadai moved this to Backlog in Neurobagel Oct 23, 2024
@alyssadai alyssadai self-assigned this Oct 28, 2024
@alyssadai
Copy link
Contributor

After further investigation into this issue, I think this will actually require more effort to resolve than it's probably worth.

The problem is that the only port exposed to the f-API is NB_API_PORT, which is the port used by the service inside the container. To catch when local_nb_nodes.json has been misconfigured to point at the f-API, the service would need to know about NB_API_PORT_HOST, which is not an environment variable inside the container - it's only used by the docker-compose.yml file itself.

To catch misconfigurations, we would probably need to expose NB_API_PORT_HOST as an environment variable to the f-API container, and then also ensure that the f-API reads it in - all just for the purposes of emitting a warning message.

Instead, maybe we could try to prevent these issues by updating our docs with a warning about the ports.

@surchs
Copy link
Contributor Author

surchs commented Oct 29, 2024

We can also attach some unique identifier for the node making the request as part of the request. Then the solution would be more generic:

  • I received a request from a node with my own identifier -> I must be talking to myself. Better stop
  • I received a query request that includes a node identifier (that is also in my local_nodes list) -> there is a loop here. Better stop

But I agree with you @alyssadai, that's more involved and not for now. Maybe a good time to look at this again would be when we address auth - because each node will have a unique client_id as part of that, and we could reuse the client_id for this purpose.

@alyssadai
Copy link
Contributor

alyssadai commented Oct 29, 2024

Thanks @surchs.

To clarify, the ideal solution would be generic enough to detect a request coming from any f-API, regardless of whether or not the f-API is part of the same stack as the recipient f-API, to avoid unpredictable behaviours.

That said, I guess the infinite request loop would only happen if an f-API is actually pointing to itself.

@alyssadai alyssadai removed the status in Neurobagel Oct 29, 2024
@alyssadai alyssadai added the someday Not a priority right now, but we want to keep this around to think or discuss more. label Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Changes only affect the documentation someday Not a priority right now, but we want to keep this around to think or discuss more.
Projects
Status: No status
Development

No branches or pull requests

2 participants