Add routing rules for Session Prefix

assigned to @godfat-gitlab

added to epic gitlab-org&12776 (closed)

changed the description

mentioned in issue gitlab-org/gitlab#439322 (closed)

changed milestone to %Backlog

marked this issue as related to gitlab-com/gl-infra/production-engineering#25621 (closed)

mentioned in issue gitlab-com/gl-infra/production-engineering#25621 (closed)

@lohrc @sxuereb Shall we schedule this for the next milestone ?

@tkuah I would agree, since this is the next milestone

changed milestone to %17.4

For more info refer to: https://handbook.gitlab.com/handbook/engineering/architecture/design-documents/cells/http_routing_service/#routing-rules

Implementation steps

Create a new rule under src/rules/, e.g. session_prefix.json
Update src/rules/index.ts to include the new rule
Update applyRule method to support regex matching
Add tests
Update documentation in GitLab.org / cells / HTTP Router

I have a few questions:

What does classify mean in this context? It looks like it'll POST to Topology service, and the service will simply say proxy it. My assumption is that classification is the way for the http router to ask the topology service which cell it should fetch from. Do we support proxying more than one cells yet?
It looks like we want this new rule to be self-contained so that if nothing is matched, it's proxying. This makes sense. However it also looks like we're applying all rules via this? https://gitlab.com/gitlab-org/cells/http-router/-/blob/99792920a4d8e2ea9b9f6fbf7face3fad0f27a29/src/index.ts#L54-56
```
for (const rule of rules) {
  return applyRule(modifiedRequest, env, ctx, rule);
}
```
If so does that mean having src/rules/passthrough.json can already proxy all the requests? Or did I misunderstand how TypeScript works here, that it'll not iterate through all the rules?
What is src/rules/firstcell.json rule used for?

However it also looks like we're applying all rules via this? https://gitlab.com/gitlab-org/cells/http-router/-/blob/99792920a4d8e2ea9b9f6fbf7face3fad0f27a29/src/index.ts#L54-56

I didn't read the type carefully. Now I see this is configured via GITLAB_RULES_CONFIG, so we'll only pick one rules set in a JSON file. We set to passthrough so only src/rules/passthrough.json is used.

Now I think that the above loop should be updated because otherwise we can't have more than one rules. match is where conditional rules are introduced. It looks like it's partially implemented in gitlab-org/cells/http-router@92e89766

@godfat-gitlab there is some context here: gitlab-org/cells/http-router!122 (comment 2063151385). I think this will be fixed when we introduce matching logic for the rules.

Have you seen this https://handbook.gitlab.com/handbook/engineering/architecture/design-documents/cells/http_routing_service/ ?

Hey @godfat-gitlab sorry for the delay

What does classify mean in this context? It looks like it'll POST to Topology service, and the service will simply say proxy it. My assumption is that classification is the way for the http router to ask the topology service which cell it should fetch from. Do we support proxying more than one cells yet?

Routing rules and classification should give you better picture.

It looks like we want this new rule to be self-contained so that if nothing is matched, it's proxying. This makes sense. However it also looks like we're applying all rules via this? https://gitlab.com/gitlab-org/cells/http-router/-/blob/99792920a4d8e2ea9b9f6fbf7face3fad0f27a29/src/index.ts#L54-56

We're not applying all rules, currently we'll apply the first and only matching rule.

What is src/rules/firstcell.json rule used for?

firstcell is a routing rule performing dynamic classification to route to the FirstCell defined by Topology Service. Requires GITLAB_TOPOLOGY_SERVICE_URL to be configured.

There are also some docs here that might be helpful.

Let me know if you need anything else.

@OmarQunsulGitlab Thanks a lot! The context helped a bunch. I did read the doc but didn't read into my mind. Now with better context in mind I understand much more while re-reading it.

Routing rules and classification should give you better picture.

@bmarjanovic Yes, thank you for the specific sections! Re-reading them closer now, I am a bit surprised to see this line:

Sends the request to /api/v1/classify (type=project_id_or_path, value=1000) if no Cells was found in cache.

Below are not relevant to this issue, and I think it must have been discussed before:

I was only thinking about project paths before, and since project paths need to be unique globally (across cells), it makes sense to have topology service to oversee this. I completely forgot the API part.

This sounds like it means we need to make project id globally unique across cells as well, and essentially any resources that we use a global id to look up via API, which doesn't feel like a good design to me, especially if we try to make this id serial and incremental.

I understand this is how GitLab is working right now, given that having a single database, a serial incremental id isn't really an issue. But do we have any plans to change this somehow? I am asking this because if we can scope everything under a group or a project, then it shouldn't be an issue if the id collides between cells.

That said, maybe if we can make sure the ids are all still unique across cells, we can more easily move organization between cells without rewriting the ids. That said, a serial is still not needed, maybe we can utilize UUID or another kind of random id maybe? This way we wouldn't need to lock on incrementing the id either.

firstcell is a routing rule performing dynamic classification to route to the FirstCell defined by Topology Service. Requires GITLAB_TOPOLOGY_SERVICE_URL to be configured.

Thanks! I think my main question is why do we need to know the first cell, and have a way to talk to it? Is it a kind of leader in the cells?

I checked my local GDK closer, and launched cell-2 to verify my understanding. It looks like:

We're actually treating the top level GitLab instance as cell-1 (listens on 3333), and ignore gitlab-cells/cell-1 (listens on 12001).
It doesn't look like we support more than one cell yet (which is my assumption as well)

So I guess maybe firstcell is just a test or experiment?

sorry for the delay

Not at all. It's blazing fast I am also happy that I am piecing more and more in my mind now.

I tried to talk to topology service directly and it works:

curl -vd '{"type":"FIRST_CELL"}' http://localhost:9096/v1/classify

The last thing I am not very sure yet is how we use regex_match. It looks like my answers can be found at gitlab-com/gl-infra/production-engineering#25621 (closed)

And I also found where we talked about the unique id I was mentioning above: https://handbook.gitlab.com/handbook/engineering/architecture/design-documents/cells/unique_sequences/

Edited: Ok I think I understand now:

> curl -d '{"type":"SESSION_PREFIX","value":"cell-1"}' http://localhost:9096/v1/classify
{"action":"PROXY", "proxy":{"address":"127.0.0.1:3333"}}

regex_match will be used to match the cookie value. Is it ok to rename at this stage? I think the name is a bit confusing. I'll probably call it value_match or value_regex_match, or match_value.

@godfat-gitlab yes, the FirstCell is a starting point, and AFAIK GDK should support multiple cells.

regex_match will be used to match the cookie value. Is it ok to rename at this stage? I think the name is a bit confusing. I'll probably call it value_match or value_regex_match, or match_value.

I think we can rename it at this stage.

/cc @ayufan @tkuah

@bmarjanovic GDK supports managing multiple cells. I am now running 3 GitLab instances, one from top-level, and 2 from cells. However running gdk reconfigure doesn't make http router nor topology service aware of the 2 GitLab instances from cells, so they're effectively idling.

My understanding is that of course that is the case, since we're now implementing session prefix, and that's one of the prerequisites to utilize more than one cells after all.

There are going to have more to be implemented in GDK as we implement more in http router and topology service. For example gitlab-org/gitlab-development-kit#2156 (closed) and I actually think gitlab-org/gitlab#474405 (closed) is a bit differently?

@godfat-gitlab

This sounds like it means we need to make project id globally unique across cells as well, and essentially any resources that we use a global id to look up via API, which doesn't feel like a good design to me, especially if we try to make this id serial and incremental.

In initial iteration we do very simple design. We assign a big range of sequences for a Cell, that is guaranteed to be monotonic within this Cell.

Yes, we want all IDs to be unique across cluster so we can easily move them around and to not rewrite the IDs. The problem with IDs is that they are often exposed to user, over API, so if we were meant to rewrite them, we are likely to brake some access patterns that users do.

@godfat-gitlab

Correct, we should update the config.toml inside the topology-service to be aware of the multiple Cells. You can take a look into this, to get better understanding.

https://handbook.gitlab.com/handbook/engineering/architecture/design-documents/cells/topology_service/#workflow

@ayufan This is so small that it's not readable by itself It's ok, if I need I can find the source of the diagram.

At the time of writing, the largest ID is 11,098,430,930 (primary key of security_findings table), so it’s 200 times the current largest ID, which should be (more than) sufficient.

Very interesting. How about jobs though? I would guess it doesn't really fit. That said, for API or web UI, it's scoped to the project: https://docs.gitlab.com/ee/api/jobs.html#get-a-single-job so maybe it doesn't matter to collide globally after all, unless, maybe we won't be able to move organizations into the same cell if jobs ids are not globally unique? Or could we make jobs use composite primary key, with the old primary id and the project id? That should be unique globally.

Edited: Oh haha, a lot of great references to read! Solution 3: Using composite primary key [(existing PKs), original cell ID]

Correct, we should update the config.toml inside the topology-service to be aware of the multiple Cells. You can take a look into this, to get better understanding.

@bmarjanovic Thanks, so I think that part is currently missing for gdk reconfigure, and can be maybe part of gitlab-org/gitlab-development-kit#2156 (closed)

@godfat-gitlab

I would guess it doesn't really fit. That said, for API or web UI, it's scoped to the project: https://docs.gitlab.com/ee/api/jobs.html#get-a-single-job so maybe it doesn't matter to collide globally after all,

That design is protective in a sense that don't have full understanding if the composite IDs, or keys collisions can introduce-unsolvable-problem/or-significant-breakage-to-application to us. We do not know that. The reason for this is that, we can rather guarantee cluster-wide uniqueness, and if this becomes limiting factor we can relax this case-by-case.

I'm simply worried about a system where we deliberately introduce key collisions, and cannot authoritatively make decision which record is valid.

Second reason is that moving organizations between Cells becomes significantly harder if we allow collisions and potentially a big security risk:

all IDs needs to be remapped
this brings a lot complexity on Org Mover to rewrite all places (including ones that do not have FKs/LFKs)
us missing some rewrite can introduce nasty cross-linking of data
makes it significantly harder to perform data validation, or make Org Mover process retryable/interruptable

Also: adding composite Primary Key in all places has been a nightmare when looking at the CI partitioning effort Edit: hard and long effort.

@ayufan Thanks! That's a lot of good points. Exhausting jobs id is probably less of a concern comparing to all that, and that can be looked at later.

Also: adding composite Primary Key in all places has been a nightmare when looking at the CI partitioning effort

I am not familiar with partitioning and our efforts in that I would guess only jobs id could be exhausted though.

@godfat-gitlab

I would guess only jobs id could be exhausted though.

I'm not afraid of it, since:

Cells are meant to be 1%-10% of the current Cell 1.
We can always change and allocate higher bits to give more headroom. We intentionally do not allocate all range to give us a way to change to ULID or other format if this is required later.

@godfat-gitlab Until gitlab-org/gitlab-development-kit#2156 (closed) is implemented you need to update each GDK's gdk.yml for now.

It should be possible to configure session_cookie_token_prefix, and gitlab.topology_service for each cell today.

Hey @ayufan should we align on the structure here?

Suggested one:

{
    "match": {
      "type": "cookie",
      "name": "_gitlab_session",
      "regex_match": "^(?<cell_name>cell-\\d+)-.*"
    },
    "action": "classify",
    "classify": {
      "type": "SessionPrefix",
      "value": "${cell_name}"
    }
  },
  {
    "action": "proxy"
  }

Documented one:

{
    "cookies": {
      "_gitlab_session": {
        "match_regex": "^(?<cell_name>cell.*:)" // accept `_gitlab_session` that are prefixed with `cell1:`
      }
    },
    "action": "classify",
    "classify": {
      "type": "session_prefix",
      "value": "${cell_name}"
    }
}

@OmarQunsulGitlab @sxuereb @bmarjanovic

I believe we had a conversation in here: gitlab-com/gl-infra/production-engineering#25621 (comment 2017436862). How we should proceed?

@ayufan @OmarQunsulGitlab @bmarjanovic A summary of gitlab-com/gl-infra/production-engineering#25621 (comment 2017436862) is that we agreed on:

cell-$CELL_ID-$RANDOM_HASH_ALREADY_GENERATED or cell$CELL_ID$RANDOM_HASH_ALREADY_GENERATED or cell$CELL_ID-$RANDOM_HASH_ALREADY_GENERATED.

Keeping regex in mind, I think a match for cell$CELL_ID-$RANDOM_HASH_ALREADY_GENERATED would be easiest and clearer from a code perspective.

$CELL_ID = will be a random number we can start with just 1.
$RANDOM_HASH_ALREADY_GENERATED = Is the hash we already currently create.

Some other things we discussed during a call:

c$CELL_ID but this conflicts with dedicated since they use c1, c2 to refer to customers internally.

@sxuereb whatever we agree on here, it should be reflected on some other issue like this GDK issue, to configure the additional Cells Session Prefixes

to configure the additional Cells Session Prefixes

@OmarQunsulGitlab are you talking about production or some other environment? If we are talking about the environment this is what we have in gitlab-com/gl-infra/production-engineering#25621 (closed)

@sxuereb in this case, only about dev environment (GDK)

I am mentioning this, because the matching rules will be the same in all environments, so just to make sure that also on GDK we generate the correct prefixes as well. With the same format.

@OmarQunsulGitlab makes sense! Maybe it should be done in this issue since it's introducing the rule?

@sxuereb Could be. Whoever works on this issue can, and probably should, do that to test that the new routing rule works.

In the other GDK issue which is still in the Backlog for some reason, we still have to configure other stuff as well.

I will add that as an optional sub-task to this issue

@OmarQunsulGitlab @sxuereb It will be easier to manually generate for now, and statically set it in GDK.

Mainly, the automation for multiple cells in GDK is not fully settled, so I do not want to complicate that automation.

We can later add automation, and also add enforcement in Topology that the session prefix reported by the cell is conformant.

set weight to 2

mentioned in issue gitlab-org/gitlab-development-kit#2156 (closed)

changed the description

mentioned in issue gitlab-org/quality/triage-reports#19089 (closed)

changed the description

mentioned in epic gitlab-org&12491

changed the description

@ayufan @manojmj @OmarQunsulGitlab

I have added this new rule (to the issue description) so that we can force requests to route to the special cell given a special cookie

  {
    "match": {
      "type": "cookie",
      "name": "force-cell",
      "regex_match": "^(?<cell_name>cell-\\d+)"
    },
    "action": "classify",
    "classify": {
      "type": "SessionPrefix",
      "value": "${cell_name}"
    }
  },

Do we need a new Classify type, or is it OK to re-use SessionPrefix ?

Do we need a new Classify type, or is it OK to re-use SessionPrefix ?

SessionPrefix would work since the logic for checking value of the cookie with a specified name is the same. (Provided none of the rules above this rule are matched)

I have added this new rule so that we can force requests to route to the special cell given a special cookie

@tkuah I didn't see this from the latest code. Was it reverted? Do I need to consider this? Should I add it back?

@godfat-gitlab I believe @tkuah meant that he updated the description with the new rule.

Oh haha, thanks, I see. I did remember I saw it somewhere but I forgot it's from the description!

Sorry for the confusion ! Updated my comment now.

mentioned in epic gitlab-org&14509

mentioned in issue gitlab-org/quality/triage-reports#19201 (closed)

mentioned in issue gitlab-org/quality/triage-reports#19287 (closed)

mentioned in issue gitlab-org/quality/triage-reports#19371 (closed)

@godfat-gitlab Potentially, this is something you can look at.

If you need help, or want to ask questions, @bmarjanovic and @manojmj can help. I think there previously was a draft MR, but we also need to validate the routing actually does happen to the 2nd (local GDK) cell.

Thanks a lot! I'll go through the context and make something.

I didn't see any merge requests linked in this issue though. If there's one I can continue on that.

@manojmj Can you please link to the related draft MR ?

It's not really a draft MR, we introduced Session prefixed based routing in the same MR that introduced multiple types of routing rules (gitlab-org/cells/http-router!122 (merged)), but it was removed within the same MR as we didn't want to ship Session prefix based routing at that point in time. You can see the commit that removed that code associated to Session prefix based routing here: gitlab-org/cells/http-router@92e89766

And, for transparency, the topology-service counterpart that introduced support for Session prefix based routing is: gitlab-org/cells/topology-service!27 (merged)

To get me started in TypeScript, here's my ever first contribution in TypeScript to add getCookieValue: gitlab-org/cells/http-router!214 (merged)

I introduced a new abstraction Action so that we can follow object-oriented programming patterns, instead of switch case all over the places: gitlab-org/cells/http-router!218 (merged)

This should make adding features much more easier.

Next I would want to change src/rules/index.ts so that we instantiate the right instance from the very beginning, either ClassifyRule or ProxyRule, so that we can move the only switch case there.

If we don't do this, we might be forced to use another switch case in findMatchingRule which is not ideal.

FYI @OmarQunsulGitlab this might introduce conflicts with your (1st MR) Refactorings to prepare to add a Logger (gitlab-org/cells/http-router!217 - merged)

Whoops! Thanks for bringing that up. It looks like it would introduce quite some conflicts. Conceptually it could be resolved, but it'll require some changes.

Since gitlab-org/cells/http-router!217 (merged) is probably closer to be merged? I'll wait then.

Status update:

First prerequisite is merged: gitlab-org/cells/http-router!218 (merged)
Follow up: gitlab-org/cells/http-router!231 (merged)
And another follow up to construct the concrete object for Rule (it was named Action but renamed to Rule in this merge request): gitlab-org/cells/http-router!230 (merged)

After above are all merged, we can look into adding match method into the Rule object. All the prerequisites are used to make sure we only need a single switch case to construct the concrete objects, and the rest can just follow object-oriented programming that the rule object itself can decide what it should do.

Lastly, it can help with gitlab-org/cells/http-router!223 (comment 2089584740) so that we can easily add a new rule called version:

{
  "match": {
    "type": "path",
    "regex_value": "/-/http_router/version"
  },
  "action": "version"
}

When this rule is matched, the router will return its version number. We also need to make sure that Rails developers are aware that this route is claimed by http-router so it will never reach to Rails when http-router is on the front.

Status update: All above are merged.

I expect this is the last refactoring I want to do for this feature: gitlab-org/cells/http-router!232 (merged)
And I started actually implementing this feature at: gitlab-org/cells/http-router!233 (merged)
- It's on top of gitlab-org/cells/http-router!232 (merged) and still a draft that I just made it compile without errors. I should be able to complete it tomorrow and ask for review.
- After gitlab-org/cells/http-router!232 (merged) adding a new version action/rule should be very simple. We just need to create a new VersionRule class and override match and fetch

Status update: Not familiar with vitest is slowing me down

Base refactoring is under review: gitlab-org/cells/http-router!232 (merged)
The feature is implemented in gitlab-org/cells/http-router!233 (merged), but I am not fully done with it yet
- Need to add more tests, notably src/rules/session_prefix.json is correctly behaving as GITLAB_RULES_CONFIG
- Add individual tests for src/rules/classify.ts and src/rules/proxy.ts
- Manual testing with GDK

I should be able to ask for full review on Monday.

Status update: I think we have some issues in the tests, and I made a quick fix: gitlab-org/cells/http-router!240 (merged)

I believe there are more state leaks we need to fix, and I would like to get gitlab-org/cells/http-router!233 (merged) merged first before fixing other tests and adding other tests, as I don't want to block gitlab-org/cells/http-router!223 (merged) any longer and would like to iterate with shipping this new feature first before I get OOO (out of office) starting this Friday.

My plan is (in this order):

Get gitlab-org/cells/http-router!240 (merged) merged
Manually test gitlab-org/cells/http-router!233 (merged) with local GDK (go through code review at the same time)
Get gitlab-org/cells/http-router!233 (merged) merged

Show some example codes for gitlab-org/cells/http-router!223 (merged) to implement:

{
  "match": {
    "type": "path",
    "regex_value": "/-/http_router/version"
  },
  "action": "version"
}

Fix tests so that we can use vi.resetAllMocks() and so that we don't need to call mockRestore on individual mocks.
- Edited: I created a new issue for this: https://gitlab.com/gitlab-org/cells/http-router/-/issues/59
Add src/rules/classify.ts and src/rules/proxy.ts.
Go on vacation for 2 weeks

Not confident that I can complete all of above before going on vacation (I also have other things to do ), but I hope I could at least reach to getting gitlab-org/cells/http-router!233 (merged) merged.

Status update:

gitlab-org/cells/http-router!233 (merged) is tested locally and I think it's working fine. Waiting for final review.

I think it's clear that I can't get all of above done before I go on vacation, but I would hope I could at least get gitlab-org/cells/http-router!233 (merged) merged.

I made gitlab-org/cells/http-router!248 (closed) to demonstrate how we can add a new version rule on top of the current architecture, helping: gitlab-org/cells/http-router!223 (comment 2100384366)

Status update: We couldn't merge today due to gitlab-com/gl-infra/production-engineering#25799 (closed) and this project will deploy to staging if we merge merge requests, and staging is part of the lock.

I asked @bmarjanovic to take over while I am away for 2 weeks. I am changing workflow to workflowin review in GitLab.org / cells / HTTP Router because I don't expect more changes unless there are other feedback.

changed the description

assigned to @godfat-gitlab

mentioned in epic gitlab-org#14509

mentioned in merge request gitlab-org/cells/http-router!214 (merged)

mentioned in epic gitlab-org&12383

mentioned in issue gitlab-org/quality/triage-reports#19542 (closed)

mentioned in merge request gitlab-org/cells/http-router!223 (merged)

mentioned in merge request gitlab-org/cells/http-router!230 (merged)

mentioned in merge request gitlab-org/cells/http-router!232 (merged)

mentioned in merge request gitlab-org/cells/http-router!233 (merged)

changed the description

mentioned in issue gitlab-org/quality/triage-reports#19657 (closed)

@tkuah I am now trying to write tests for gitlab-org/cells/http-router!233 (merged) and thinking that if force-cell should take priority over _gitlab_session. That is, when force-cell is presented, we classify for that, and ignore _gitlab_session.

Does that make sense? If so, we should probably reverse the order in JSON so that we always match from top to bottom.

@godfat-gitlab Interesting question. I was thinking force-cell is lower - it's only a means to allow someone to get to the login page on the cell.

There may be situations where we want it to have the highest priority, for debugging ? I think that's more debug-cell, or we just use the internal DNS for that cell instead.

@tkuah If it's lower, how do people switch to the other cell once they previously landed on another cell? Do we clear _gitlab_session when logging out, and don't write to it if the user is not logged in?

I am imagining that we'll use force-cell where appropriate, and once landed on a particular cell, the application will remove it, so next it'll follow with _gitlab_session, logging in or not.

That said, it should be simple to swap around when needed. For now I put force-cell higher in gitlab-org/cells/http-router!233 (merged) Happy to swap whenever.

I am imagining that we'll use force-cell where appropriate, and once landed on a particular cell, the application will remove it, so next it'll follow with _gitlab_session, logging in or not.

Sounds good @godfat-gitlab , happy to do this too.

Cool, I updated the description to swap the order.

This test has state leak: https://gitlab.com/gitlab-org/cells/http-router/-/blob/91baacca92462a31823daf98dcc14594034f8c8a/test/index.spec.ts#L288-319

it('should return cached response if available', async () => {
  const cachesMock = {
    default: {
      match: vi.fn(),
    },
  };

  caches.default.match = cachesMock.default.match;

  const cachedResponse = new Response(JSON.stringify({ action: 'PROXY', proxy: { address: 'cached.example.com' } }), {
    headers: { 'Content-Type': 'application/json' },
  });

  cachesMock.default.match.mockResolvedValue(cachedResponse);

  fetchMock
    .get('http://cached.example.com')
    .intercept({ path: '/', headers: { 'x-forwarded-host': 'example.com' } })
    .reply(200, 'response from cell1');

  const cachesDefaultMatchSpy = vi.spyOn(caches.default, 'match');
  const cachesDefaultPutSpy = vi.spyOn(caches.default, 'put');
  const response = await workerFetchURL('http://example.com')

  expect(await response.text()).toBe('response from cell1');
  expect(cachesDefaultMatchSpy).toHaveBeenCalledTimes(1);
  expect(cachesDefaultPutSpy).not.toBeCalled();
});

If I run it with my other tests, it'll show this error which I have a hard time understanding what it means:

 FAIL  test/index.spec.ts > when rules are configured > with `session_prefix` rule > when force-cell cookie is matched > when value matched the format > sets correct headers
TypeError: Body has already been used. It can only be used once. Use tee() first if you need to read it twice.
 ❯ Module.classifyFetch src/topology_service/classify.ts:25:19
     23|   }
     24|
     25|   return response.json() as Promise<ClassifyResponse>;
       |                   ^
     26| }
     27|
 ❯ ClassifyRule.fetch src/rules/classify.ts:21:22
 ❯ workerFetchRequest test/index.spec.ts:62:20
 ❯ test/index.spec.ts:389:28

I identified the issue. The problematic line is: caches.default.match = cachesMock.default.match; and I am fixing it at gitlab-org/cells/http-router!240 (merged)

I created a new issue for this: https://gitlab.com/gitlab-org/cells/http-router/-/issues/59

mentioned in merge request gitlab-org/cells/http-router!240 (merged)

mentioned in issue gitlab-org/quality/triage-reports#19669 (closed)

changed the description

changed milestone to %17.5

mentioned in epic gitlab-org#12383

closed with merge request gitlab-org/cells/http-router!233 (merged)

mentioned in commit gitlab-org/cells/http-router@aaf51ed4

@bmarjanovic @godfat-gitlab Can you please make a demo video about this ? Being able to route to a 2nd cell on GDK is amazing, and it is worth sharing this exciting achievement.

Sure, sounds like a plan.

@godfat-gitlab do you have some availability for tomorrow?

@bmarjanovic Yeah, I have no calls tomorrow. How would you like to proceed? I was thinking something like we did in the call, running here and there, and record it.

Amazing @godfat-gitlab, I'll schedule a call for tomorrow then.

The idea is to enable multiple Cells on the GDK and apply session_prefix rules. Based on the cookie values, we can then demonstrate how the new rule functions. WDYT?

I tried to get this running locally now. I configured 2 cells via GDK, starting them via:

gdk cells start

I am observing the logs via 2 console tabs:

gdk cells cell-1 tail workhorse
gdk cells cell-2 tail workhorse

I configured topology service as:

[[cells]]
id = 100
name = "cell-1"
address = "127.0.0.1:12001"
session_prefix = "cell-1"

[[cells]]
id = 200
name = "cell-2"
address = "127.0.0.1:12151"
session_prefix = "cell-2"

The IP I filled was observed when I ran gdk cells start. The following links can visit the respective cells via the port:

Next I started http router and the topology service at the top-level GDK:

gdk start gitlab-http-router gitlab-topology-service

For the http router, GITLAB_TOPOLOGY_SERVICE_URL is set to http://localhost:9096 which is the address of the topology service, and GITLAB_RULES_CONFIG is set to session_prefix.

I didn't set GITLAB_PROXY_HOST and I changed the last rule in src/rules/session_prefix.json from proxy to classify to FIRST_CELL so that I don't need to set a proxy

With all the above setting, when I visit http://localhost:3000 which is the address of the http router, I can see that the first cell is visited by looking at gdk cells cell-1 tail workhorse.

When I add a cookie force-cell=cell-2, I can see that the other cell is visited instead by looking at gdk cells cell-2 tail workhorse. When I remove it, I can see it goes back to cell-1.

Next I probably need to configure actual session prefix in cell-1 and cell-2 to verify looking at session prefix does work.

Cross-posting gitlab-org/gitlab-development-kit#2229 (closed) issue description, since it can be valuable here.

Set unique_cookie_key_postfix to false

I successfully configured session prefix with the following commands:

gdk cells cell-1 config set gitlab.rails.session_store.session_cookie_token_prefix cell-1-
gdk cells cell-1 reconfigure
gdk cells cell-1 restart

gdk cells cell-2 config set gitlab.rails.session_store.session_cookie_token_prefix cell-2-
gdk cells cell-2 reconfigure
gdk cells cell-2 restart

The Demo has been recorded

Slack Link (internal-only).

Thanks for handling the recording!

Set unique_cookie_key_postfix to false

This will go along side with session_cookie_token_prefix, so in the end the gdk.yml for the cell should look like this:

gitlab:
  rails:
    session_store:
      session_cookie_token_prefix: cell-1-
      unique_cookie_key_postfix: false

Really cool, I could reproduce this locally! If this can help, here is a more complete list of config to set:

gdk cells cell-1 config set gitlab.rails.session_store.session_cookie_token_prefix cell-1-
gdk cells cell-1 config set unique_cookie_key_postfix false
gdk cells cell-1 reconfigure
gdk cells cell-1 restart

gdk cells cell-2 config set gitlab.rails.session_store.session_cookie_token_prefix cell-2-
gdk cells cell-2 config set unique_cookie_key_postfix false
gdk cells cell-2 reconfigure
gdk cells cell-2 restart

# allow to use the default '3000' port for the HTTP router (change 3001 to something else if you're already using 3001)
gdk config set port 3001
gdk config set gitlab_http_router.port 3000
gdk reconfigure
gdk restart

# allow to use the default '3000' port for the HTTP router (change 3001 to something else if you're already using 3001)
gdk config set port 3001
gdk config set gitlab_http_router.port 3000

Actually, based on https://gitlab.com/gitlab-org/gitlab-development-kit/-/blob/main/doc/howto/cells.md#using-the-http-router, HTTP Router should listen on port 3000 by default.