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

Provide (optional?) health-check on root endpoint even if TLSRedirect is enabled #1357

Closed
Legogris opened this issue Jun 13, 2019 · 3 comments

Comments

@Legogris
Copy link
Contributor

Given the following deployment scenario:

  • Deploying Chainlink node(s) on Google Cloud's GKE Kubernetes platform
  • Exposing node admin GUI via Google's managed L7 Load Balancer
  • Disabling TLS on node, using Ingress to terminate TLS
  • Enabling TLS Redirect (Togglable TLS Redirect #1322 + setting SSLProxyHeaders: map[string]string{"X-Forwarded-Proto": "https"}, on secure middleware config)

This fails today because Google's Load Balancer will enforce a health-check that requires HTTP requests to / to respond with HTTP 200.

It would make things a lot easier if we could get an empty or otherwise 200 for requests on / for requests coming from the load balancer. One could look at the User-Agent header for this and it should pose no security issues doing so (we are currently seeing GoogleHC 1.1).

Some description of the general issue: kubernetes/ingress-gce#596
Discussion on resolving this: kubernetes/ingress-gce#42

There is already an implementation for a middleware for this: https://github.com/tumelohq/gke-ingress-healthcheck-middleware (context: kubernetes/ingress-gce#674)

There are possible workarounds, like loading Nginx as a sidecar container receiving all HTTP requests and proxying through all but the LB health-checks to the node - but I imagine that the deployment scenario I describe is not going to be uncommon, so it could make sense to implement an option for handling this in the node regardless (seeing as it's already doing things like TLS).

Maintainers: If you would be open to a solution like this, I could take a shot at a PR.

@jleeh
Copy link
Contributor

jleeh commented Jun 17, 2019

Hey @Legogris. The Chainlink core node itself doesn't horizontally scale currently, only one node can be active at any one time. So even if you trick the healthchecks to get it to pass, you'd be routed to the wrong nodes that don't have the UI up.

For this to work properly, multiple nodes need to be active at once, routing any incoming job runs between them.

@jleeh
Copy link
Contributor

jleeh commented Jun 17, 2019

Horizontal scaling of nodes has been discussed and may be included in the future, so I'll close this issue for now.

@jleeh jleeh closed this as completed Jun 17, 2019
@se3000
Copy link
Contributor

se3000 commented Jun 17, 2019

@Legogris to give a little more detail, the way we currently do a healthcheck is by looking for chainlink to open the port for its API, defaults to 6688. This is implicit and doesn't offer very much information, so we've created a story for a more explicit health check here. It starts with latest blockcheight, as read through the DB, so it will provide info on both Ethereum client liveness and DB connectivity.

We can add other metrics in subsequent stories, so if there is anything else that would be helpful for you in a health check please let us know.

asoliman92 pushed a commit that referenced this issue Aug 27, 2024
## Motivation
fix: CCIP-3095 fix flaky test assertions in offramp tests

Few unit tests were still asserting on `gasUsed` in
`ExecutionStateChanged` event emitted by offramp contract while
executing messages in the report.

## Solution
use assertion helper function to assert the ExecutionStateChanged Event
indexed topics and chosen fields of event data
asoliman92 pushed a commit that referenced this issue Aug 28, 2024
## Motivation
fix: CCIP-3095 fix flaky test assertions in offramp tests

Few unit tests were still asserting on `gasUsed` in
`ExecutionStateChanged` event emitted by offramp contract while
executing messages in the report.

## Solution
use assertion helper function to assert the ExecutionStateChanged Event
indexed topics and chosen fields of event data
asoliman92 pushed a commit that referenced this issue Aug 28, 2024
## Motivation
fix: CCIP-3095 fix flaky test assertions in offramp tests

Few unit tests were still asserting on `gasUsed` in
`ExecutionStateChanged` event emitted by offramp contract while
executing messages in the report.

## Solution
use assertion helper function to assert the ExecutionStateChanged Event
indexed topics and chosen fields of event data
asoliman92 pushed a commit that referenced this issue Aug 28, 2024
## Motivation
fix: CCIP-3095 fix flaky test assertions in offramp tests

Few unit tests were still asserting on `gasUsed` in
`ExecutionStateChanged` event emitted by offramp contract while
executing messages in the report.

## Solution
use assertion helper function to assert the ExecutionStateChanged Event
indexed topics and chosen fields of event data
asoliman92 pushed a commit that referenced this issue Aug 28, 2024
## Motivation
fix: CCIP-3095 fix flaky test assertions in offramp tests

Few unit tests were still asserting on `gasUsed` in
`ExecutionStateChanged` event emitted by offramp contract while
executing messages in the report.

## Solution
use assertion helper function to assert the ExecutionStateChanged Event
indexed topics and chosen fields of event data
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

No branches or pull requests

3 participants