Skip to content

Conversation

@barankyle
Copy link
Contributor

@barankyle barankyle commented Dec 6, 2024

Summary

As part of IR-5570, which is mostly addressed here, the address and port of the instanceserver that a websocket connection is being made to are no longer sourced from query params on the WS address. Instead, the nginx server now looks up the instance record of the already-provided instanceID and proxies to the address in the record, if the record exists.

As the query params are no longer used, this removes them from the connection. In developments settings, they are still used as the direct IP:port of the connection since there normally is no nginx server to act as an intermediary.

Subtasks Checklist

Breaking Changes

References

closes #insert number here

QA Steps

As part of IR-5570, the address and port of the instanceserver that a websocket
connection is being made to are no longer sourced from query params on the WS
address. Instead, the nginx server now looks up the instance record of the
already-provided instanceID and proxies to the address in the record, if the
record exists.

As the query params are no longer used, this removes them from the connection.
In developments settings, they are still used as the direct IP:port of the
connection since there normally is no nginx server to act as an intermediary.
@barankyle barankyle marked this pull request as ready for review December 6, 2024 00:23
@barankyle
Copy link
Contributor Author

Please do not merge this in after approving, I will do so once I update the helm deployments on (mt-)dev once the linked PR is approved and merged. Just merging this will break deployments without the helm deployment updates.

@HexaField
Copy link
Contributor

@barankyle this seems to have been lost to time but still relevant?

@barankyle
Copy link
Contributor Author

It's sort of not relevant any more. This isn't used with P2P I believe. On GCP, we're no longer using nginx, so the additional nginx code I'd written in the ir-engine-ops repo to get the IP/port from the database instead of whatever it was passed on these query params isn't really relevant any more, either.

@HexaField
Copy link
Contributor

but if we still don't need the search params in the primus request then we should just remove them right?

@barankyle
Copy link
Contributor Author

It would technically break any AWS deployments, which we don't care about any more, but someone else might. If we merge in the linked PR as well then I think it's fine to merge this too.

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