atlanticwave-sdx / sdx-controller

Central Controller for AtlanticWave SDX.
https://www.atlanticwave-sdx.net
MIT License
1 stars 3 forks source link

Require connection_id from client to be uuid #263

Closed congwang09 closed 5 months ago

congwang09 commented 5 months ago

Resolves: https://github.com/atlanticwave-sdx/sdx-controller/issues/216

This will require the "connection_id" from client "POST connection" to be UUID. We don't want the server to generate connection_id for client, because if the HTTP request from server to client is lost, client will not know the connection_id anymore.

sajith commented 5 months ago

From the discussions after writing up #216, my understanding is that clients are responsible for generating connection IDs. They simply have to be strings. (Although we have format: uuid in the API definition, it doesn't seem to be enforced by connexion or pydantic.) Also, SDX Controller will reject connection requests with duplicate IDs.

I think #216 should be closed without a PR. I am not too sure about requiring connection IDs to be UUIDs. Shouldn't any free-form string do the job, as long as it is unique?

congwang09 commented 5 months ago

Resolves: #216 This will require the "connection_id" from client "POST connection" to be UUID. We don't want the server to generate connection_id for client, because if the HTTP request from server to client is lost, client will not know the connection_id anymore.

Hi Cong, thanks for providing the changes on this pull request to validate if the user has provided a valid uuid as the connection identified. Very nice done!

However, I would like to check with you if this is really the best approach. I mean: usually when you have something that is used as an identifier for your objects, the identifier is created by the application itself, not received from the user. I mean, for me it makes more sense that sdx-controller application generate its own connection id (using uuid) and then when receiving a delete, we can validate yes.. but ultimately, as long as the ID exists on the database, it is fine (just to be clear: validation at this point is also okay!).

I'm asking this because the Issue #216 is more related to "generating the connection ID", while this PR seems to be focusing on validating the connection ID for deletion. Can you please see if this makes any sense for you?

Yes that's def a valid concern. The major issue having SDX server generating connection uuid is that, we cannot ensure idempotency (no duplicate connection is placed). See here: https://aws.amazon.com/builders-library/making-retries-safe-with-idempotent-APIs/

We can also add mechanisms in SDX as Sajith mentioned, to allow client to send any string, and SDX service checks if the connection id has been taken or not, like creating a new email address. But that way client may need to try many times for a unique string.

congwang09 commented 5 months ago

From the discussions after writing up #216, my understanding is that clients are responsible for generating connection IDs. They simply have to be strings. (Although we have format: uuid in the API definition, it doesn't seem to be enforced by connexion or pydantic.) Also, SDX Controller will reject connection requests with duplicate IDs.

Yes it's not enforced.

I think #216 should be closed without a PR. I am not too sure about requiring connection IDs to be UUIDs. Shouldn't any free-form string do the job, as long as it is unique?

We can allow free-form string, but it client may need to try many times for a unique string, if we don't set some minimal string length or complexity for the id.

I feel it's sort of related to #216, we are just taking another approach to let client pass in an ID. I'm ok with either way!

sajith commented 5 months ago

I think letting clients deal with failure if/when they happen to use non-unique connection IDs should be okay. SDX Controller simply needs to reject connection requests if they attempt to reuse existing IDs. Simply requiring a connection ID to be in the form of an UUID is not a guarantee for uniqueness, since they can be reused.

congwang09 commented 5 months ago

No longer force uuid from client.