-
Notifications
You must be signed in to change notification settings - Fork 123
feat: allow multiple CR connections of the same type #1078
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
Open
tyler-catlin
wants to merge
17
commits into
master
Choose a base branch
from
allow-multiple-crs-of-same-type
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+526
−11
Open
Changes from all commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
12f9506
feat: allow multiple CR connections of the same type
tyler-catlin ab28408
feat: BrokerServerPostResponseHandler logging
gclapperton 0fa9239
fix: destroying null socket on timeout
gclapperton c589635
feat: add failing test for missing broker rule
lewispclark c957a16
fix: allow route for deleting refs in ghe
lewispclark 2e50bcd
fix: update snapshot to include delete branch rule
lewispclark 84fbf13
chore: ACC-3107 remove response socket timeout logging
gclapperton d66739a
feat: show failing tests for branch deletetions
lewispclark f84dbd5
fix: show failing tests for branch deletetions
lewispclark 9957e25
fix: add rules to accept deletion requests in gitlab and github
lewispclark f0007eb
fix: acc-2456 bump axios to 1.13.4
gclapperton 9f90035
fix: linting
tyler-catlin ee6ed32
fix: update testing values, use correct header access
tyler-catlin 54fcd76
Merge branch 'master' into allow-multiple-crs-of-same-type
tyler-catlin f029ae5
fix: comment clarity, removal of debug logs
tyler-catlin 4a9f210
Merge branch 'master' into allow-multiple-crs-of-same-type
tyler-catlin 7124a31
Merge branch 'master' into allow-multiple-crs-of-same-type
tyler-catlin File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| { | ||
| "BROKER_CLIENT_CONFIGURATION": { | ||
| "common": { | ||
| "default": { | ||
| "BROKER_SERVER_URL": "https://broker.dev.snyk.io", | ||
| "BROKER_HA_MODE_ENABLED": "false", | ||
| "BROKER_DISPATCHER_BASE_URL": "https://api.dev.snyk.io" | ||
| }, | ||
| "oauth": { | ||
| "clientId": "${CLIENT_ID}", | ||
| "clientSecret": "${CLIENT_SECRET}" | ||
| } | ||
| } | ||
| }, | ||
| "CONNECTIONS": { | ||
| "ecr1": { | ||
| "type": "ecr", | ||
| "identifier": "${BROKER_TOKEN_1}", | ||
| "CR_AGENT_URL": "http://localhost:17500", | ||
| "BROKER_CLIENT_URL": "http://localhost:7341" | ||
| }, | ||
| "ecr2": { | ||
| "type": "ecr", | ||
| "identifier": "${BROKER_TOKEN_2}", | ||
| "CR_AGENT_URL": "http://localhost:17500", | ||
| "BROKER_CLIENT_URL": "http://localhost:7341" | ||
| } | ||
| } | ||
| } | ||
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
117 changes: 117 additions & 0 deletions
117
test/functional/multiple-container-registries-same-type.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,117 @@ | ||
| const PORT = 9001; | ||
| import path from 'path'; | ||
| import { axiosClient } from '../setup/axios-client'; | ||
| import { | ||
| BrokerClient, | ||
| closeBrokerClient, | ||
| waitForBrokerServerConnections, | ||
| } from '../setup/broker-client'; | ||
| import { | ||
| BrokerServer, | ||
| closeBrokerServer, | ||
| createBrokerServer, | ||
| waitForUniversalBrokerClientsConnection, | ||
| } from '../setup/broker-server'; | ||
| import { TestWebServer, createTestWebServer } from '../setup/test-web-server'; | ||
| import { DEFAULT_TEST_WEB_SERVER_PORT } from '../setup/constants'; | ||
| import { createUniversalBrokerClient } from '../setup/broker-universal-client'; | ||
|
|
||
| const fixtures = path.resolve(__dirname, '..', 'fixtures'); | ||
| const serverAccept = path.join(fixtures, 'server', 'filters-cra.json'); | ||
|
|
||
| /** | ||
| * Integration test for multiple container registries of the same type. | ||
| * | ||
| * Note: This test verifies that multiple ECR registries can be configured | ||
| * and that identifier-based routing works. The identifier header is set by | ||
| * the BrokerWorkload when it receives requests from the server over websocket, | ||
| * so this test verifies the end-to-end flow where: | ||
| * 1. Server sends request over websocket to client | ||
| * 2. BrokerWorkload adds identifier header | ||
| * 3. Middleware routes based on identifier | ||
| * 4. Request is forwarded to correct downstream registry | ||
| */ | ||
| describe('Multiple container registries of the same type - identifier-based routing', () => { | ||
| let tws: TestWebServer; | ||
| let bs: BrokerServer; | ||
| let bc: BrokerClient; | ||
| process.env.API_BASE_URL = `http://localhost:${DEFAULT_TEST_WEB_SERVER_PORT}`; | ||
|
|
||
| beforeAll(async () => { | ||
| tws = await createTestWebServer(); | ||
|
|
||
| bs = await createBrokerServer({ port: PORT, filters: serverAccept }); | ||
|
|
||
| process.env.SKIP_REMOTE_CONFIG = 'true'; | ||
| process.env.SNYK_BROKER_SERVER_UNIVERSAL_CONFIG_ENABLED = 'true'; | ||
| process.env.UNIVERSAL_BROKER_ENABLED = 'true'; | ||
| process.env.SERVICE_ENV = 'universaltest-ecr'; | ||
| process.env.CLIENT_ID = 'clientid'; | ||
| process.env.CLIENT_SECRET = 'clientsecret'; | ||
| // Create two ECR registries with different tokens | ||
| process.env.BROKER_TOKEN_1 = 'ecr-registry-1-token'; | ||
| process.env.BROKER_TOKEN_2 = 'ecr-registry-2-token'; | ||
| process.env.SNYK_BROKER_CLIENT_CONFIGURATION__common__default__BROKER_SERVER_URL = `http://localhost:${bs.port}`; | ||
|
|
||
| bc = await createUniversalBrokerClient(); | ||
| await waitForUniversalBrokerClientsConnection(bs, 2); | ||
| }); | ||
|
|
||
| afterAll(async () => { | ||
| await tws.server.close(); | ||
| if (bc) { | ||
| await closeBrokerClient(bc); | ||
| } | ||
| await closeBrokerServer(bs); | ||
| delete process.env.BROKER_SERVER_URL; | ||
| delete process.env.BROKER_TOKEN_1; | ||
| delete process.env.BROKER_TOKEN_2; | ||
| delete process.env.CLIENT_ID; | ||
| delete process.env.CLIENT_SECRET; | ||
| }); | ||
|
|
||
| it('should have two ECR connections established', async () => { | ||
| const serverMetadata = await waitForBrokerServerConnections(bc); | ||
| expect(serverMetadata.length).toBeGreaterThanOrEqual(2); | ||
| expect(serverMetadata.map((x) => x.brokertoken)).toEqual( | ||
| expect.arrayContaining(['ecr-registry-1-token', 'ecr-registry-2-token']), | ||
| ); | ||
| }); | ||
|
|
||
| it('should successfully broker container registry requests with identifier-based routing', async () => { | ||
| const serverMetadata = await waitForBrokerServerConnections(bc); | ||
| expect(serverMetadata.length).toBeGreaterThanOrEqual(2); | ||
|
|
||
| // Verify both connections are established | ||
| const ecr1Metadata = serverMetadata.find( | ||
| (x) => x.brokertoken === 'ecr-registry-1-token', | ||
| ); | ||
| const ecr2Metadata = serverMetadata.find( | ||
| (x) => x.brokertoken === 'ecr-registry-2-token', | ||
| ); | ||
|
|
||
| expect(ecr1Metadata).toBeDefined(); | ||
| expect(ecr2Metadata).toBeDefined(); | ||
| expect(ecr1Metadata!.identifier).toBeDefined(); | ||
| expect(ecr2Metadata!.identifier).toBeDefined(); | ||
| // Identifiers should be different | ||
| expect(ecr1Metadata!.identifier).not.toEqual(ecr2Metadata!.identifier); | ||
|
|
||
| // When server sends requests over websocket, BrokerWorkload will add | ||
| // the identifier header, and the middleware will route correctly. | ||
| // This test verifies identifier-based routing by simulating a server request | ||
| // with the connection identifier header. | ||
| const ecr1Identifier = ecr1Metadata!.identifier; | ||
| const response = await axiosClient.post( | ||
| `http://localhost:${bc.port}/api/v2/import/done`, | ||
| { some: { example: 'json' } }, | ||
| { | ||
| headers: { | ||
| 'snyk-broker-connection-identifier': ecr1Identifier, | ||
| }, | ||
| }, | ||
| ); | ||
|
|
||
| expect(response.status).toEqual(200); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: is this supposed to be a 505? Maybe a 400 would be better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree this is an odd return value, but the reason it's here is because that was the previous behavior.
505 is kind of related to this (
version not supported), but I agree that a 4xx is probably the better return code. If we're okay changing the status codes that we're returning here then I can change it, but I'm not sure if that falls under the purview of this change.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sensei @pavel-snyk what do you think?