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

Implement TCP checks #17

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/health-check.yml
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ jobs:
SOURCES: |
GitHub->https://github.com
Facebook->https://facebook.com
Never SSL->http://oldsplendidfresheclipse.neverssl.com/online/
TCPTest->tcp://tcpbin.com:4242
Comment on lines +56 to +57
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

⚠️ Unreliable health check endpoints could lead to false alerts

The current endpoints raise reliability concerns:

  • The HTTP endpoint (neverssl.com) is a static page last modified in 2022, designed for captive portal testing
  • The TCP endpoint (tcpbin.com) is a public service without guaranteed availability

Recommendations:

  • Use internal endpoints or dedicated health check endpoints with SLAs
  • Consider implementing a simple internal HTTP endpoint for basic connectivity tests
  • Document the endpoint selection criteria in the health check configuration
🔗 Analysis chain

Review reliability of test endpoints.

The current test endpoints raise some concerns:

  1. neverssl.com is primarily designed for captive portal testing, not for production health checks
  2. tcpbin.com is a third-party service that might have:
    • Reliability issues
    • Rate limiting
    • No SLA guarantees

Consider using:

  • Internal endpoints for TCP checks
  • More reliable HTTP endpoints for testing

Run this script to check the reliability of the current endpoints:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check the historical uptime of the test endpoints

# Test: Check if the domains are resolvable
echo "Checking DNS resolution..."
host oldsplendidfresheclipse.neverssl.com
host tcpbin.com

# Test: Check TCP connection to tcpbin
echo "Testing TCP connection..."
nc -zv tcpbin.com 4242

Length of output: 494


Script:

#!/bin/bash
# Search for endpoint usage and documentation
echo "Searching for endpoint references..."
rg -i "neverssl|tcpbin" --type yaml --type md

echo "Checking HTTP endpoint..."
curl -I http://oldsplendidfresheclipse.neverssl.com/online/

echo "Looking for health check documentation..."
fd -e md -e txt -e yaml . | xargs rg -i "health.?check|monitoring"

Length of output: 1399

GITHUB_TOKEN: ${{ github.token }}
JOB_NAME: ${{ github.workflow }}
ARTIFACT_NAME: ${{ env.ARTIFACT_NAME }}
Expand Down
4 changes: 2 additions & 2 deletions log/src/status.ts → log/src/checks/http.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { ActionLogger } from "./github/types";
import { ActionLogger } from "../github/types";
import axios, { AxiosError } from "axios";

export class StatusChecker {
export class HTTPChecker {
constructor(private readonly siteName: string, private readonly healthEndpoint: string,
private readonly logger: ActionLogger) {
logger.info(`Created Status Checker for ${siteName}`);
Expand Down
95 changes: 95 additions & 0 deletions log/src/checks/tcp.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
import * as net from 'net';
import { ActionLogger } from "../github/types";

function attemptConnection(host: string, port: number, logger: ActionLogger, retryCount: number = 5): Promise<boolean> {
return new Promise((resolve) => {
let attempts = 0;

function attemptConnection() {
const socket = new net.Socket();

// Set a timeout for the connection attempt
socket.setTimeout(1000);

// Attempt to connect to the specified host and port
socket.on('connect', () => {
// If connected, destroy the socket and resolve true
socket.removeAllListeners(); // Clean up listeners
socket.destroy();
resolve(true);

});

// Handle errors that occur during the connection attempt
socket.on('error', (err) => {
logger.error(`Connection error: ${err.message}`);
socket.removeAllListeners();
socket.destroy();
// If it's anything other then a connection refused, retry
if (!err.message.includes("ECONNREFUSED")) {
handleRetry();
} else {
resolve(false); // Resolve the promise on ECONNREFUSED
}
});

// Handle timeout scenario
socket.on('timeout', () => {
logger.error('Connection timeout');
socket.destroy();
resolve(false);
});

function handleRetry() {
if (attempts < retryCount) {
attempts += 1;
logger.info(`Retrying... (${attempts}/${retryCount})`);
setTimeout(attemptConnection, 500); // Retry after .5 seconds
} else {
resolve(false); // Failed after retrying
}
}

socket.connect(port, host);
}

attemptConnection();

});
}

export class TCPChecker {
constructor(private readonly siteName: string, private readonly healthEndpoint: string,
private readonly logger: ActionLogger) {
this.logger.info(`Created Status Checker for ${siteName}`);
}

async verifyEndpoint(): Promise<boolean> {
const [host, portString] = this.healthEndpoint.replace('tcp://', '').split(':');
if (!host) {
this.logger.error('Invalid host: Host cannot be empty');
return false;
}

// Convert the port part to a number
const port = parseInt(portString, 10);
if (Number.isNaN(port) || port < 1 || port > 65535) {
this.logger.error(`Invalid port number: ${portString}`);
return false;
}

try {
const isConnected = await attemptConnection(host, port, this.logger);
if (isConnected) {
this.logger.info('Connection successful');
return true;
} else {
this.logger.info('Connection failed');
return false;
}
} catch (error) {
this.logger.error('Error during connection attempt: ' + error);
return false;
}
}
}
40 changes: 36 additions & 4 deletions log/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
import { setFailed, setOutput } from "@actions/core";
import { setFailed, setOutput, warning } from "@actions/core";
import { context, getOctokit } from "@actions/github";
import { Context } from "@actions/github/lib/context";

import { envsafe, num, str } from "envsafe";
import moment from "moment";
import { ArtifactManager } from "./github/artifact";
import { Repo } from "./github/types";
import { StatusChecker } from "./status";
import { HTTPChecker } from "./checks/http";
import { TCPChecker } from "./checks/tcp";
import { ReportFile } from "./types";
import { generateCoreLogger } from "./util";
import { IncidentManager } from "./github/incidents";
Expand Down Expand Up @@ -38,6 +39,25 @@ const repo = getRepo(context);

setOutput("repo", `${repo.owner}/${repo.repo}`);

const SUPPORTED_PROTOCOLS = {
HTTP: 'http://',
HTTPS: 'https://',
TCP: 'tcp://'
} as const;

function isValidUrl(url: string): boolean {
try {
if (url.startsWith(SUPPORTED_PROTOCOLS.TCP)) {
const [host, port] = url.replace(SUPPORTED_PROTOCOLS.TCP, '').split(':');
return Boolean(host && port && !Number.isNaN(parseInt(port, 10)));
}
new URL(url);
return true;
} catch {
return false;
}
}

/**
* Inputs must be in style of
* ```log
Expand Down Expand Up @@ -73,8 +93,20 @@ const run = async () => {

// Run tests on each required source
for (const [name, url] of sources) {
const statusChecker = new StatusChecker(name, url, logger);
const result = await statusChecker.verifyEndpoint();
let result: boolean;
if (!isValidUrl(url)) {
result = false;
warning(`Invalid URL format for check: ${name}`);
} else if (url.startsWith(SUPPORTED_PROTOCOLS.HTTP) || url.startsWith(SUPPORTED_PROTOCOLS.HTTPS)) {
const statusChecker = new HTTPChecker(name, url, logger);
result = await statusChecker.verifyEndpoint();
} else if (url.startsWith(SUPPORTED_PROTOCOLS.TCP)) {
const statusChecker = new TCPChecker(name, url, logger);
result = await statusChecker.verifyEndpoint();
} else {
result = false;
warning(`Unsupported protocol for check: ${name} (URL: ${url})`);
}

let report: ReportFile["site"][number]["status"] | undefined = siteResult.get(name);

Expand Down
6 changes: 3 additions & 3 deletions log/src/test/index.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { ArtifactManager } from "../github/artifact";
import { GitHubClient } from "../github/types";
import { StatusChecker } from "../status";
import { HTTPChecker } from "../checks/http";
import { ReportFile } from "../types";

const sites: Array<[string, string]> = [
Expand All @@ -11,7 +11,7 @@ const sites: Array<[string, string]> = [

for (const [site, url] of sites) {
test(`Test obtaining metrics from ${site}`, async () => {
const statusChecker = new StatusChecker(site, url, console);
const statusChecker = new HTTPChecker(site, url, console);
await expect(statusChecker.verifyEndpoint()).resolves.toBe(true);
})
}
Expand All @@ -20,7 +20,7 @@ test("Write to doc", async () => {
const siteResult: ReportFile["site"] = [];

for (const [site, url] of sites) {
const statusChecker = new StatusChecker(site, url, console);
const statusChecker = new HTTPChecker(site, url, console);
siteResult.push({ name: site, status: [{ timestamp: new Date().getTime(), result: (await statusChecker.verifyEndpoint()) }] });
}
const am = new ArtifactManager(null as unknown as GitHubClient, console, "test");
Expand Down