Skip to content
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

[DO NOT MERGE] Gen1 Request Forwarding to Gen2 #10535

Closed
wants to merge 13 commits into from

Conversation

soloseng
Copy link
Contributor

Description

Turn gen1 cloud function into a proxy between client and gen2, when enabled by config.

@soloseng soloseng changed the title add request forwarding to Gen2 Gen1 Request Forwarding to Gen2 Aug 31, 2023
@soloseng soloseng marked this pull request as ready for review August 31, 2023 18:14
@soloseng soloseng requested a review from a team August 31, 2023 18:14
@soloseng soloseng requested a review from a team August 31, 2023 21:37
@socket-security
Copy link

socket-security bot commented Aug 31, 2023

New dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
j6 1.0.2 eval, filesystem +0 4.27 MB ccckmit
@celo/wallet-hsm-azure 5.0.4 None +2 1.44 MB aaron-clabs
@types/http-proxy 1.17.12 None +0 14.9 kB types
http-proxy 1.18.1 network +0 232 kB jcrugzz
pretty-quick 2.0.2 filesystem +0 67.6 kB azz
@truffle/artifactor 4.0.180 filesystem +0 38.9 kB kevinbluer
@azure/identity 1.5.2 network, filesystem, shell, environment +0 697 kB azure-sdk

Copy link
Contributor

@gastonponti gastonponti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add the timeout to avoid issues. Check the error response, but other than that, it looks good. Nice work

packages/phone-number-privacy/combiner/src/server.ts Outdated Show resolved Hide resolved
packages/phone-number-privacy/combiner/src/server.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@alecps alecps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Just a few comments

proxyTimeout: config.phoneNumberPrivacy.odisServices.timeoutMilliSeconds,
})

const originalPath = req.path
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you just trying to get the endpoint? You should be able to just use req.url for that

case 'mainnet':
// XXX (soloseng):URL may need to be updated after gen2 function is created on mainnet
destinationUrl =
'https://us-central1-celo-pgpnp-mainnet.cloudfunctions.net/combinerGen2' +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like you forgot to add strippedPath here

}
proxy.on('error', (_) => {
logger.error('Error in Proxying request to Combiner.')
res.status(500).json({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this override the actual error returned by the gen2 combiner? I'd rather just pass that along to the end user if possible

@soloseng soloseng changed the title Gen1 Request Forwarding to Gen2 [DO NOT MERGE] Gen1 Request Forwarding to Gen2 Sep 12, 2023
@soloseng soloseng mentioned this pull request Sep 12, 2023
@soloseng soloseng self-assigned this Sep 12, 2023
@aaronmgdr
Copy link
Member

moved to celo-org/social-connect#46

@aaronmgdr aaronmgdr closed this Sep 29, 2023
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.

4 participants