[server] use owner and repo name for workspace id #7391
Conversation
roboquat
added
do-not-merge/work-in-progress
do-not-merge/release-note-label-needed
team: webapp
svenefftinge
force-pushed
the
se/workspaceid
branch
from
1a220a0 to
b05a8ea
Compare
|
/werft run 👍 started the job as gitpod-build-se-workspaceid.2 |
|
/werft run 👍 started the job as gitpod-build-se-workspaceid.3 |
|
/werft run 👍 started the job as gitpod-build-se-workspaceid.4 |
|
/werft run 👍 started the job as gitpod-build-se-workspaceid.5 |
|
The change makes sense - it also considerably increases the chances for workspace ID collisions. What happens when such a collision occurs? I.e. how does Gitpod behave in that case? |
|
I agree with the collision argument. If we increase the likelihood of collisions in the first two segments, we should increase the overall variances. Maybe by making the 3rd argument longer? back-of-the-envelope calculation:
@csweichel Does this sound about right? 🤔 Another thought: Did we re-visit the "id vs. title" debate again? Changing the workspaceId is always a tradeoff (see above). The questions is: Why do people look at the URL in the first place? If we made the |
|
I think we only need to handle a collision gracefully. I.e. catch an internal error and generate a new ID. |
| } | ||
|
|
||
| function clean(segment: string | undefined, maxChars: number = 16) { | ||
| if (segment) { |
There was a problem hiding this comment.
Do I read this correct as:
if (!segment) return undefined;
const result = Array.from(segment)
.filter(c => characters.indexOf(c) !== -1)
.join('');
if (result.length >= 2) {
return result.substring(0, maxChars);
}
return result;
🤔
There was a problem hiding this comment.
Ok, I rewrote it as suggested.
I feel we got stuck in discussion for way too long on this. It's definitely not a solution to all related problems but an improvement over generic color-animal ids. There are other things that can be done, but I don't see why this should stop this small improvement. |
Totally agree. Unfortunately the cross-cluster/db-sync setup makes that difficult. Especially for "high-traffic" repos it's more likely that they're used in different regions where we cannot ensure that an ID isn't used twice (except if we wait for the db-sync interval during workspace ID generation - we definitely don't want that). Gero's observation is on-point: with just a tad more entropy we can achieve a collision likelihood similar to what we have now. We don't even need to extend the last segment, but we could just pad/replace three characters in the second segment (if we fret modifying the regexp yet again). Example workspace IDs would be: Looking at the collision probabilities:
Implementation is simple: export async function generateWorkspaceID(firstSegment?: string, secondSegment?: string): Promise<string> {
const firstSeg = clean(firstSegment) || await random(colors);
let secondSeq: string;
if (!!secondSegment) {
secondSeq = clean(secondSegment, Math.min(16, 24-firstSeg.length)).substring(0, 13) + (await random(characters, 8));
} else {
secondSeq = await random(animals);
}
return firstSeg+'-'+secondSeq+'-'+(await random(characters, 8));
}If we're concerned with mixing in random characters in the second segment, just using numbers would. also work and collide with a chance of |
Isn't it super unlikely that an id conflict happens within 10 mins between US and EU? Or am I missing something? |
|
I'd prefer adding additional characters to the last segment instead of changing the human readable first two. |
svenefftinge
force-pushed
the
se/workspaceid
branch
from
b05a8ea to
79e9593
Compare
|
I have added the three characters to the third segment as suggested. I decided to not add a collision check, because it would need to be done in the generateWorkspaceId function (we don't have an explicit insert call that could fail, well maybe that is not too hard to do with TypeORM?) and I think adding another DB request for every workspace start given how unlikely it is to have a collision isn't worth it. |
|
/werft run with-clean-slate 👍 started the job as gitpod-build-se-workspaceid.7 |
|
/lgtm |
|
LGTM label has been added. Git tree hash: 479779e9058424fa4b1bb413af60675b6a676647
|
|
/werft run with-clean-slate 👍 started the job as gitpod-build-se-workspaceid.8 |
svenefftinge
force-pushed
the
se/workspaceid
branch
from
79e9593 to
e6c234c
Compare
Codecov Report
@@ Coverage Diff @@
## main #7391 +/- ##
===========================================
+ Coverage 19.04% 35.31% +16.26%
===========================================
Files 2 23 +21
Lines 168 2455 +2287
===========================================
+ Hits 32 867 +835
- Misses 134 1535 +1401
- Partials 2 53 +51
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
This change introduces optional arguments in generateWorkspaceId for the first two segments. And makes use of it in workspace factory using the repos org/group and name. fixes #4129
JanKoehnlein
force-pushed
the
se/workspaceid
branch
from
e6c234c to
51b9866
Compare
|
Corrected another instance of the regex pattern. |
Thank you! Did you also do testing and review? |
|
@csweichel the force pushing removed your LGTM |
| @@ -162,7 +162,7 @@ func run(origin, sshConfig string, apiPort int, allowCORSFromPort bool, autoTunn | |||
| return err | |||
| } | |||
| wsHostRegex := "(\\.[^.]+)\\." + strings.ReplaceAll(originURL.Host, ".", "\\.") | |||
| wsHostRegex = "([0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}|[0-9a-z]{2,16}-[0-9a-z]{2,16}-[0-9a-z]{8})" + wsHostRegex | |||
| wsHostRegex = "([0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}|[0-9a-z]{2,16}-[0-9a-z]{2,16}-[0-9a-z]{8,11})" + wsHostRegex | |||
There was a problem hiding this comment.
Does it mean that existing users of VS Code desktop and local companion shuold upgrade now? Or it is backard compatible?
There was a problem hiding this comment.
It is not compatible. If you start a new workspace and still have an old local companion app running this logic would not match.
How is the upgrade process for local companion app going for other incompatible changes or do we never have them?
There was a problem hiding this comment.
We did not do incompatible changes so far. Giving that it is in Beta state, maybe it is fine to break. For VS Code Desktop we can switch easily from the local companion as soon as SSH gateway is deployed.
|
Tested and LGTM, but @geropl had a comment on the |
He was asking me to rewrite the function to do an early exit. I did that already. |
|
/lgtm |
|
LGTM label has been added. Git tree hash: f183a59c825a94d23420f693d764f10876c5c67b
|
|
/lgtm @gitpod-io/engineering-ide if someone asks that local port forwarding does not work anymore ask them to upgrade to latest local app |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: akosyakov, csweichel, JanKoehnlein Associated issue: #4129 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
roboquat
added
deployed: webapp
This change introduces optional arguments in
generateWorkspaceIdfor the first two segments. And makes use of it in workspace factory
using the repos org/group and name.
fixes #4129
Testing
Start a workspace on GitHub and GitLab repos (try with nested groups on GitLab) and find the repo names and orgs reflected in the workspace id.
Release Notes