-
Notifications
You must be signed in to change notification settings - Fork 3
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
Support App diagnostics endpoint features #76
Changes from all commits
6465be2
00a4adf
60e0248
feec655
3d1dd29
ee19cda
ad202cc
c2804ef
7da679e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,6 +26,12 @@ dependencies { | |
implementation("com.squareup.okhttp3:okhttp:4.12.0") | ||
|
||
testImplementation("org.springframework.boot:spring-boot-starter-test") | ||
|
||
if (JavaVersion.current().isJava11Compatible) { | ||
testImplementation("uk.org.webcompere:system-stubs-jupiter:2.1.6") | ||
} else { | ||
testImplementation("uk.org.webcompere:system-stubs-jupiter:1.2.1") | ||
} | ||
Comment on lines
+30
to
+34
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't like this at all but it works and it's only a test dependency. https://github.com/webcompere/system-stubs?tab=readme-ov-file#system-stubs
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, that sucks we can't just use one version for everything but this seems like an OK solution given they've moved their LTS to a newer JDK than inngest-kt's minimum |
||
} | ||
|
||
dependencyManagement { | ||
|
@@ -39,6 +45,11 @@ dependencyManagement { | |
tasks.withType<Test> { | ||
useJUnitPlatform() | ||
systemProperty("junit.jupiter.execution.parallel.enabled", true) | ||
systemProperty("test-group", "unit-test") | ||
|
||
// Required by `system-stubs-jupiter` for JDK 21+ compatibility | ||
// https://github.com/raphw/byte-buddy/issues/1396 | ||
jvmArgs = listOf("-Dnet.bytebuddy.experimental=true") | ||
KiKoS0 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
testLogging { | ||
events = | ||
setOf( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,139 @@ | ||
package com.inngest.springbootdemo; | ||
|
||
import com.inngest.*; | ||
import com.inngest.signingkey.BearerTokenKt; | ||
import com.inngest.signingkey.SignatureVerificationKt; | ||
import com.inngest.springboot.InngestConfiguration; | ||
import org.junit.jupiter.api.BeforeAll; | ||
import org.junit.jupiter.api.Nested; | ||
import org.junit.jupiter.api.Test; | ||
import org.junit.jupiter.api.condition.EnabledIfSystemProperty; | ||
import org.junit.jupiter.api.extension.ExtendWith; | ||
import org.springframework.beans.factory.annotation.Autowired; | ||
import org.springframework.boot.test.autoconfigure.web.servlet.WebMvcTest; | ||
import org.springframework.context.annotation.Bean; | ||
import org.springframework.context.annotation.Import; | ||
import org.springframework.test.web.servlet.MockMvc; | ||
import uk.org.webcompere.systemstubs.environment.EnvironmentVariables; | ||
import uk.org.webcompere.systemstubs.jupiter.SystemStub; | ||
import uk.org.webcompere.systemstubs.jupiter.SystemStubsExtension; | ||
|
||
import java.util.HashMap; | ||
|
||
import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; | ||
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.*; | ||
|
||
class ProductionConfiguration extends InngestConfiguration { | ||
|
||
public static final String INNGEST_APP_ID = "spring_test_prod_demo"; | ||
|
||
@Override | ||
protected HashMap<String, InngestFunction> functions() { | ||
return new HashMap<>(); | ||
} | ||
|
||
@Override | ||
protected Inngest inngestClient() { | ||
return new Inngest(INNGEST_APP_ID); | ||
} | ||
|
||
@Override | ||
protected ServeConfig serve(Inngest client) { | ||
return new ServeConfig(client); | ||
} | ||
|
||
@Bean | ||
protected CommHandler commHandler(@Autowired Inngest inngestClient) { | ||
ServeConfig serveConfig = new ServeConfig(inngestClient); | ||
return new CommHandler(functions(), inngestClient, serveConfig, SupportedFrameworkName.SpringBoot); | ||
} | ||
} | ||
|
||
@ExtendWith(SystemStubsExtension.class) | ||
public class CloudModeIntrospectionTest { | ||
|
||
private static final String productionSigningKey = "signkey-prod-b2ed992186a5cb19f6668aade821f502c1d00970dfd0e35128d51bac4649916c"; | ||
private static final String productionEventKey = "test"; | ||
@SystemStub | ||
private static EnvironmentVariables environmentVariables; | ||
|
||
@BeforeAll | ||
static void beforeAll() { | ||
environmentVariables.set("INNGEST_DEV", "0"); | ||
environmentVariables.set("INNGEST_SIGNING_KEY", productionSigningKey); | ||
environmentVariables.set("INNGEST_EVENT_KEY", productionEventKey); | ||
} | ||
|
||
// The nested class is useful for setting the environment variables before the configuration class (Beans) runs. | ||
// https://www.baeldung.com/java-system-stubs#environment-and-property-overrides-for-junit-5-spring-tests | ||
KiKoS0 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
@Import(ProductionConfiguration.class) | ||
@WebMvcTest(DemoController.class) | ||
@Nested | ||
@EnabledIfSystemProperty(named = "test-group", matches = "unit-test") | ||
class InnerSpringTest { | ||
@Autowired | ||
private MockMvc mockMvc; | ||
|
||
@Test | ||
public void shouldReturnInsecureIntrospectionWhenSignatureIsMissing() throws Exception { | ||
mockMvc.perform(get("/api/inngest").header("Host", "localhost:8080")) | ||
.andExpect(status().isOk()) | ||
.andExpect(content().contentType("application/json")) | ||
.andExpect(header().string(InngestHeaderKey.Framework.getValue(), "springboot")) | ||
.andExpect(jsonPath("$.authentication_succeeded").value(false)) | ||
.andExpect(jsonPath("$.function_count").isNumber()) | ||
.andExpect(jsonPath("$.has_event_key").value(true)) | ||
.andExpect(jsonPath("$.has_signing_key").value(true)) | ||
.andExpect(jsonPath("$.mode").value("cloud")) | ||
.andExpect(jsonPath("$.schema_version").value("2024-05-24")); | ||
} | ||
|
||
@Test | ||
public void shouldReturnInsecureIntrospectionWhenSignatureIsInvalid() throws Exception { | ||
mockMvc.perform(get("/api/inngest") | ||
.header("Host", "localhost:8080") | ||
.header(InngestHeaderKey.Signature.getValue(), "invalid-signature")) | ||
.andExpect(status().isOk()) | ||
.andExpect(content().contentType("application/json")) | ||
.andExpect(header().string(InngestHeaderKey.Framework.getValue(), "springboot")) | ||
.andExpect(jsonPath("$.authentication_succeeded").value(false)) | ||
.andExpect(jsonPath("$.function_count").isNumber()) | ||
.andExpect(jsonPath("$.has_event_key").value(true)) | ||
.andExpect(jsonPath("$.has_signing_key").value(true)) | ||
.andExpect(jsonPath("$.mode").value("cloud")) | ||
.andExpect(jsonPath("$.schema_version").value("2024-05-24")); | ||
} | ||
|
||
@Test | ||
public void shouldReturnSecureIntrospectionWhenSignatureIsValid() throws Exception { | ||
long currentTimestamp = System.currentTimeMillis() / 1000; | ||
|
||
String signature = SignatureVerificationKt.signRequest("", currentTimestamp, productionSigningKey); | ||
String formattedSignature = String.format("s=%s&t=%d", signature, currentTimestamp); | ||
|
||
String expectedSigningKeyHash = BearerTokenKt.hashedSigningKey(productionSigningKey); | ||
|
||
mockMvc.perform(get("/api/inngest") | ||
.header("Host", "localhost:8080") | ||
.header(InngestHeaderKey.Signature.getValue(), formattedSignature)) | ||
.andExpect(status().isOk()) | ||
.andExpect(content().contentType("application/json")) | ||
.andExpect(header().string(InngestHeaderKey.Framework.getValue(), "springboot")) | ||
.andExpect(jsonPath("$.authentication_succeeded").value(true)) | ||
.andExpect(jsonPath("$.function_count").isNumber()) | ||
.andExpect(jsonPath("$.has_event_key").value(true)) | ||
.andExpect(jsonPath("$.has_signing_key").value(true)) | ||
.andExpect(jsonPath("$.mode").value("cloud")) | ||
.andExpect(jsonPath("$.schema_version").value("2024-05-24")) | ||
.andExpect(jsonPath("$.api_origin").value("https://api.inngest.com/")) | ||
.andExpect(jsonPath("$.app_id").value(ProductionConfiguration.INNGEST_APP_ID)) | ||
.andExpect(jsonPath("$.env").value("prod")) | ||
.andExpect(jsonPath("$.event_api_origin").value("https://inn.gs/")) | ||
.andExpect(jsonPath("$.framework").value("springboot")) | ||
.andExpect(jsonPath("$.sdk_language").value("java")) | ||
.andExpect(jsonPath("$.event_key_hash").value("9f86d081884c7d659a2feaa0c55ad015a3bf4f1b2b0b822cd15d6c15b0f00a08")) | ||
.andExpect(jsonPath("$.sdk_version").value(Version.Companion.getVersion())) | ||
.andExpect(jsonPath("$.signing_key_hash").value(expectedSigningKeyHash)); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,8 +4,11 @@ import com.beust.klaxon.Json | |
import com.beust.klaxon.Klaxon | ||
import com.fasterxml.jackson.databind.ObjectMapper | ||
import com.fasterxml.jackson.databind.SerializationFeature | ||
import com.inngest.signingkey.checkHeadersAndValidateSignature | ||
import com.inngest.signingkey.getAuthorizationHeader | ||
import com.inngest.signingkey.hashedSigningKey | ||
import java.io.IOException | ||
import java.security.MessageDigest | ||
|
||
data class ExecutionRequestPayload( | ||
val ctx: ExecutionContext, | ||
|
@@ -177,8 +180,50 @@ class CommHandler( | |
// TODO | ||
// fun sync(): Result<InngestSyncResult> = Result.success(InngestSyncResult.None) | ||
|
||
fun introspect(origin: String): String { | ||
val requestPayload = getRegistrationRequestPayload(origin) | ||
fun introspect( | ||
signature: String?, | ||
requestBody: String, | ||
serverKind: String?, | ||
): String { | ||
val insecureIntrospection = | ||
InsecureIntrospection( | ||
functionCount = functions.size, | ||
hasEventKey = Environment.isInngestEventKeySet(client.eventKey), | ||
hasSigningKey = config.hasSigningKey(), | ||
mode = if (client.env == InngestEnv.Dev) "dev" else "cloud", | ||
) | ||
|
||
val requestPayload = | ||
when (client.env) { | ||
InngestEnv.Dev -> insecureIntrospection | ||
|
||
else -> | ||
runCatching { | ||
KiKoS0 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
checkHeadersAndValidateSignature(signature, requestBody, serverKind, config) | ||
|
||
SecureIntrospection( | ||
functionCount = functions.size, | ||
hasEventKey = Environment.isInngestEventKeySet(client.eventKey), | ||
hasSigningKey = config.hasSigningKey(), | ||
authenticationSucceeded = true, | ||
mode = "cloud", | ||
env = client.env.value, | ||
appId = config.appId(), | ||
apiOrigin = "${config.baseUrl()}/", | ||
framework = framework.value, | ||
sdkVersion = Version.getVersion(), | ||
sdkLanguage = "java", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I went with java as the sdk language following this: https://linear.app/inngest/project/javakotlin-sdk-0e841a80efe2#projectUpdate-dd0137ce&comment-1288aae4 |
||
servePath = config.servePath(), | ||
serveOrigin = config.serveOrigin(), | ||
signingKeyHash = hashedSigningKey(config.signingKey()), | ||
eventApiOrigin = "${Environment.inngestEventApiBaseUrl(client.env)}/", | ||
eventKeyHash = if (config.hasSigningKey()) hashedEventKey(client.eventKey) else null, | ||
) | ||
}.getOrElse { | ||
insecureIntrospection.apply { authenticationSucceeded = false } | ||
} | ||
} | ||
|
||
return serializePayload(requestPayload) | ||
} | ||
|
||
|
@@ -198,4 +243,14 @@ class CommHandler( | |
val servePath = config.servePath() ?: "/api/inngest" | ||
return "$serveOrigin$servePath" | ||
} | ||
|
||
private fun hashedEventKey(eventKey: String): String? = | ||
eventKey | ||
.takeIf { Environment.isInngestEventKeySet(it) } | ||
?.let { | ||
MessageDigest | ||
.getInstance("SHA-256") | ||
.digest(it.toByteArray()) | ||
.joinToString("") { byte -> "%02x".format(byte) } | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,11 +11,20 @@ object Environment { | |
).filterValues { (it is String) }.entries.associate { (k, v) -> k to v!! } | ||
} | ||
|
||
private const val DUMMY_KEY_EVENT = "NO_EVENT_KEY_SET" | ||
|
||
fun inngestEventKey(key: String? = null): String { | ||
if (key != null) return key | ||
return System.getenv(InngestSystem.EventKey.value) ?: "NO_EVENT_KEY_SET" | ||
return System.getenv(InngestSystem.EventKey.value) ?: DUMMY_KEY_EVENT | ||
} | ||
|
||
fun isInngestEventKeySet(value: String?) = | ||
when { | ||
value.isNullOrEmpty() -> false | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this because the empty case isn't being handled properly by the elvis operator on line 18? nulls would get replaced by the DUMMY_KEY_EVENT right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. exactly yeah i should probably check if other SDKs are using a null value or not. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Python sdk sets the same dummy value as well: https://github.com/inngest/inngest-py/blob/2bc0fb51d9b05a15c19a0a92c61602823beded49/inngest/_internal/client_lib/client.py#L29 but it doesn't seem to populate the the Instead it conditionally uses it when |
||
value == DUMMY_KEY_EVENT -> false | ||
else -> true | ||
} | ||
|
||
fun inngestEventApiBaseUrl( | ||
env: InngestEnv, | ||
url: String? = null, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
package com.inngest | ||
|
||
import com.beust.klaxon.Json | ||
|
||
abstract class Introspection( | ||
@Json("authentication_succeeded") open val authenticationSucceeded: Boolean?, | ||
open val functionCount: Int, | ||
open val hasEventKey: Boolean, | ||
open val hasSigningKey: Boolean, | ||
open val mode: String, | ||
@Json("schema_version") val schemaVersion: String = "2024-05-24", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is it possible to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. great suggestion i was really hoping for it to work but it doesn't seem that it works with Klaxon and i don't think it has an equivalent for it either. I wonder if a custom converter would work though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh right I missed the SO thread was about Jackson specifically. This could be nice as a follow up but non blocking for this PR |
||
) | ||
|
||
internal data class InsecureIntrospection( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. any reason for using insecure/secure as the terminology when the spec and JS SDK are using unauthenticated/authenticated? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i started implementing closely with the Go sdk, i can rename them since the spec and both JS and Python has it as unauthenticated/authenticated. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah ok, fine to leave as is if there's precedent in the go sdk |
||
@Json("authentication_succeeded") override var authenticationSucceeded: Boolean? = null, | ||
@Json("function_count") override val functionCount: Int, | ||
@Json("has_event_key") override val hasEventKey: Boolean, | ||
@Json("has_signing_key") override val hasSigningKey: Boolean, | ||
override val mode: String, | ||
) : Introspection(authenticationSucceeded, functionCount, hasEventKey, hasSigningKey, mode) | ||
|
||
internal data class SecureIntrospection( | ||
@Json("api_origin") val apiOrigin: String, | ||
@Json("app_id") val appId: String, | ||
@Json("authentication_succeeded") override val authenticationSucceeded: Boolean?, | ||
// TODO: Add capabilities when adding the trust probe | ||
// @Json("capabilities") val capabilities: Capabilities, | ||
@Json("event_api_origin") val eventApiOrigin: String, | ||
@Json("event_key_hash") val eventKeyHash: String?, | ||
val env: String?, | ||
val framework: String, | ||
@Json("function_count") override val functionCount: Int, | ||
@Json("has_event_key") override val hasEventKey: Boolean, | ||
@Json("has_signing_key") override val hasSigningKey: Boolean, | ||
@Json("has_signing_key_fallback") val hasSigningKeyFallback: Boolean = false, | ||
override val mode: String, | ||
@Json("sdk_language") val sdkLanguage: String, | ||
@Json("sdk_version") val sdkVersion: String, | ||
@Json("serve_origin") val serveOrigin: String?, | ||
@Json("serve_path") val servePath: String?, | ||
@Json("signing_key_fallback_hash") val signingKeyFallbackHash: String? = null, | ||
@Json("signing_key_hash") val signingKeyHash: String?, | ||
) : Introspection(authenticationSucceeded, functionCount, hasEventKey, hasSigningKey, mode) |
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.
don't we need to get the actual request body somehow?
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.
hmm i believe it gave me an error trying to get the body of a GET request and later on trying it in cloud mode, it didn't seem to be sending a body. I'll try again to get it.
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.
Oh I forgot this is a GET, so yeah you're right there probably won't be a request body. Should we make the argument optional then?
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.
Also I made sure that the JS sdk returns an empty string for
GET
as well:https://github.com/inngest/inngest-js/blob/56ed5c11081517db2a72ae27c83cbf4263d9b6ed/packages/inngest/src/components/InngestCommHandler.ts#L729-L735