-
Notifications
You must be signed in to change notification settings - Fork 4
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
Create basic CDK support and minimalist demo stack #6
Conversation
…covery after deployment
ed4005b
to
8831b6d
Compare
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.
love it. lets get this in and iterate from there!
|
||
const controller = new AbortController(); | ||
const healthCheckTimeout = setTimeout(() => controller.abort(), 3000); | ||
const healthCheckUrl = `${props.ingressEndpoint}/grpc.health.v1.Health/Check`; |
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 think we should be using the health endpoint on the meta here, right?
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.
You're quite right, for pre-registration the meta health check is probably the one that matters! I'm going to revisit this entire handler to make it more robust but switching over to meta health check for now. (Separately, the ALB is continuously monitoring the ingress health check.)
console.log(`Got registration response back: ${await discoveryResponse.text()} (${discoveryResponse.status})`); | ||
|
||
if (!(healthResponse.status >= 200 && healthResponse.status < 300)) { | ||
// TODO: retry until successful, or some overall timeout is reached |
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.
if only we had some kind of orchestrator that did this for us ;)
* The name of the Secrets Store secret that contains the GitHub token to use for Docker login. | ||
* This is `/restate/docker/github-token` by default. | ||
*/ | ||
githubTokenPath?: string; |
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.
can be a followup, but i think we just made dist publicaly accessible :)
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.
Yeah, bit of a waste! But it was useful to remember how to do this in CDK – it may yet come up again. Something else I want to investigate: if we can push the Restate image into an ECR registry, perhaps we can eliminate the internet connectivity for the Restate VPC altogether. That would be a big security and cost win.
githubTokenPath?: string; | ||
|
||
/** | ||
* Temporary! This will disappear once we move to direct Lambda calls. |
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.
can be a followup - but latest dist now does support direct calls
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.
Ack! I'll iterate on that as a follow up; I still have something weird going on with the discovery resource that I want to get to the bottom of, I'll update to direct Lambda integration asap after I figure it out.
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.
Ended up doing this now, the latest branch changes switch to direct integration.
// These rules allow the service registration component to trigger service discovery as needed; the requests | ||
// originate from a VPC-bound Lambda function that backs the custom resource. | ||
this.vpc.privateSubnets.forEach((subnet) => { | ||
restateInstanceSecurityGroup.addIngressRule(ec2.Peer.ipv4(subnet.ipv4CidrBlock), ec2.Port.tcp(RESTATE_META_PORT), "Allow traffic from the VPC to Restate meta"); |
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.
probably will want also 9071 and 9072 (worker http and postgres endpoints)
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.
open question how introspection should work - vpn vs punch through load balancer. if punch through, then need some sort of auth story for the postgres endopint, cant just use aws stuff there
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.
One quick and secure way might be to allow customers to start an interactive psql
session using Session Manager? Not ideal for all users but better than opening an unauthenticated endpoint to the internet.
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.
lgtm!
@@ -43,7 +42,8 @@ export const handler: Handler<CloudFormationCustomResourceEvent, Partial<CloudFo | |||
|
|||
const registerCallTimeout = setTimeout(() => controller.abort(), 3000); | |||
const discoveryEndpointUrl = `${props.metaEndpoint}/endpoints`; | |||
const registrationRequest = JSON.stringify({ uri: props.serviceEndpoint }); | |||
// const registrationRequest = JSON.stringify({ uri: props.serviceEndpoint }); |
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; can clean this up
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.
Doh, manage to miss this! Will clean up in the next PR.
This change introduces a CDK project with several reusable constructs, and a demonstration stack that allows customers to easily deploy a self-hosted CDK stack and Lambda-based service handlers to AWS account.
As this is still a private repository (see linked GH issue for planned work), at the moment comment is primarily sought along these dimensions:
RestateSelfHostedStack
(restate-self-hosted-stack.ts
) simple and intuitive to you? (Imagine that in the future, a customer would write this code + their handler code only, while the Restate constructs will be provided as a library)Testing
"Works on my machine"
Issue: #1
Closes: #3