Repository navigation
Conversation
jtmcdole
left a comment
There was a problem hiding this comment.
This is mostly LGTM from me with some nits / suggestions / questions.
| "aud": "https://dashboard.flutter.dev/api/partner/callback", | ||
| "iat": 1774963500, | ||
| "exp": 1774963800, | ||
| "data": { |
There was a problem hiding this comment.
Should we encode data in a json schema? If we're using json serializable, it has the option to export the schema.
There was a problem hiding this comment.
Payload is already base64 encoded. I would prefer to avoid double-encoded objects.
There was a problem hiding this comment.
I was asking about making a json schema (https://json-schema.org/) for partners when they are coding.
| * [RFC 7517 Section 4.5](https://www.rfc-editor.org/info/rfc7517/#section-4.5) & [Section 5](https://www.rfc-editor.org/info/rfc7517/#section-5) for `kid` lookup via cached JWKS; | ||
| * [RFC 8725 Section 3](https://www.rfc-editor.org/info/rfc8725/#section-3) for algorithm pinning and `iss`/`aud`/`exp` claim checks. | ||
|
|
||
| In addition, Cocoon's callback endpoint validates that `data.uid` exists and has not been processed yet. |
There was a problem hiding this comment.
Above we check the aud, which tells us the request did indeed come from a partner. However, should we also check that this uid was assigned to this partner? One partner should not be able to update another partner's task by accident.
There was a problem hiding this comment.
Sure we can but iss not aud. The sample mentioned in 4.1.3.6. JWT Payload is for cocoon to call partner, when partner calls cocoon iss should be 3p-partner-name and aud something like https://flutter-dashboard.appspot.com/api/webhook
|
|
||
| #### 4.1.2. Flow Breakdown | ||
|
|
||
| * **1. Trigger:** GitHub notifies Cocoon via `/api/github/webhook-subscription` when a commit lands on `main`. |
There was a problem hiding this comment.
In the future, we will likely want to use distributed CI to vet an upcoming beta / stable release. These won't be on the main branch. Would that affect this design at all?
| * **pr**: The Pull Request number associated with the event. | ||
| * **sha**: The target git commit SHA hash. | ||
| * **task_name**: Name of task to be displayed on dashboard, to potentially support several CI types or platforms per partner. For only one task per partner can be ignored. | ||
| * **tags**: Optional key-value string pairs, for future usage. Should be returned as is in the status response. |
There was a problem hiding this comment.
Could you expand why we need this? What kind of information would we store here? Do we need to be worried about a partner accidentally dropping/corrupting these tags? Could we instead store this information in Firestore and look it up by uid?
| }, | ||
| { | ||
| "name": "test_2", | ||
| "status": "success", |
There was a problem hiding this comment.
FYI this is success but below it's succeeded. One of these is incorrect :)
| { | ||
| "alg": "EdDSA", | ||
| "typ": "JWT", | ||
| "kid": 12345 |
| * **3. Outbound Queueing:** For every configured partner, Cocoon creates a record in Firestore with a `uid` (unique ID) to map partner notifications to Cocoon tasks, and publishes a message containing the `uid` and commit context (`commit_sha`, `pr_number`, `repo`, etc.) to an internal GCP Pub/Sub topic for each partner. | ||
| * **4. Partner Dispatch:** A Pub/Sub subscriber fetches partner configurations, generates a short-lived JWT signed with an [Ed25519](https://www.rfc-editor.org/info/rfc8410/) private key, and issues an HTTP POST webhook request to the partner's configured URL. Cocoon remains stateless and retries the dispatch via Pub/Sub if it timed out waiting for status response, receives 5xx (Server error), or 408 (Request Timeout) response. | ||
| * **5. Terminal unsuccessful response:** If a partner webhook responds with client error code 4xx (except 408 Request Timeout), Cocoon will log that as an error. Status code 409 (Conflict) will be considered an already processed notification and will be logged as a warning. | ||
| * **6. Execution & Callback:** The partner CI executes tests on proprietary hardware and makes an API callback to Cocoon with the execution status (`in_progress`, `cancelled`, `infra_failure`, `failed`, `succeeded`, `neutral`, `skipped`), message, and log URLs. Cocoon reads the Key ID from the JWT header and does a lookup for the corresponding cached public key; if the public key is not found it attempts to re-read partner's JWKS and update cache; if the key is not found it responds with 401. If Cocoon successfully validates the payload and sends the partner status to Pub/Sub, it will respond with status code 200. Failure responses indicating that the partner CI system should retry sending the CI execution status are: 5xx and 408. In case of a JWT signature validation failure, Cocoon will respond with 401. If there is a validation error, it responds with 400; if the `uid` is not found, it responds with 404; if already processed, it responds with 409; if body size exceeds 8MB it responds with 413. |
There was a problem hiding this comment.
The in_progress status is disallowed in the callback right? The partner can only use that status during the on-demand status check?
| * **6. Execution & Callback:** The partner CI executes tests on proprietary hardware and makes an API callback to Cocoon with the execution status (`in_progress`, `cancelled`, `infra_failure`, `failed`, `succeeded`, `neutral`, `skipped`), message, and log URLs. Cocoon reads the Key ID from the JWT header and does a lookup for the corresponding cached public key; if the public key is not found it attempts to re-read partner's JWKS and update cache; if the key is not found it responds with 401. If Cocoon successfully validates the payload and sends the partner status to Pub/Sub, it will respond with status code 200. Failure responses indicating that the partner CI system should retry sending the CI execution status are: 5xx and 408. In case of a JWT signature validation failure, Cocoon will respond with 401. If there is a validation error, it responds with 400; if the `uid` is not found, it responds with 404; if already processed, it responds with 409; if body size exceeds 8MB it responds with 413. | |
| * **6. Execution & Callback:** The partner CI executes tests on proprietary hardware and makes an API callback to Cocoon with the execution status (`cancelled`, `infra_failure`, `failed`, `succeeded`, `neutral`, `skipped`), message, and log URLs. Cocoon reads the Key ID from the JWT header and does a lookup for the corresponding cached public key; if the public key is not found it attempts to re-read partner's JWKS and update cache; if the key is not found it responds with 401. If Cocoon successfully validates the payload and sends the partner status to Pub/Sub, it will respond with status code 200. Failure responses indicating that the partner CI system should retry sending the CI execution status are: 5xx and 408. In case of a JWT signature validation failure, Cocoon will respond with 401. If there is a validation error, it responds with 400; if the `uid` is not found, it responds with 404; if already processed, it responds with 409; if body size exceeds 8MB it responds with 413. |
| * **1. Trigger:** GitHub notifies Cocoon via `/api/github/webhook-subscription` when a commit lands on `main`. | ||
| * **2. Scheduling:** Cocoon schedules tasks on LUCI and creates dummy tasks in Firestore/Redis for each configured partner. | ||
| * **3. Outbound Queueing:** For every configured partner, Cocoon creates a record in Firestore with a `uid` (unique ID) to map partner notifications to Cocoon tasks, and publishes a message containing the `uid` and commit context (`commit_sha`, `pr_number`, `repo`, etc.) to an internal GCP Pub/Sub topic for each partner. | ||
| * **4. Partner Dispatch:** A Pub/Sub subscriber fetches partner configurations, generates a short-lived JWT signed with an [Ed25519](https://www.rfc-editor.org/info/rfc8410/) private key, and issues an HTTP POST webhook request to the partner's configured URL. Cocoon remains stateless and retries the dispatch via Pub/Sub if it timed out waiting for status response, receives 5xx (Server error), or 408 (Request Timeout) response. |
There was a problem hiding this comment.
Can dispatch happen before engine artifacts are uploaded? It would be helpful to clarify this timing here, as it determines whether partners are expected to compile the engine themselves.
If dispatch can occur before artifacts are ready, should we support an optional mode where notifications are delayed until engine artifacts are available? I'm fine with punting on this feature for now, but I want to make sure the current design could support this feature later if needed.
|
This also LGTM me with just minor nits and questions! |
| Note over Partner: Response timeout | ||
|
|
||
| Partner->>+Cocoon: Retry HTTP POST (signed with partner key,<br/>JWT body with status for sha=wxyz with uid=abcd) | ||
| Cocoon->>Partner: HTTP GET of Partner JWKS url |
There was a problem hiding this comment.
Question: why would we call their CI for the JWKS url? Shouldn't we know this through an agreement first?
There was a problem hiding this comment.
I think this means "Hit the partner's JWKS endpoint", not "Ask the partner what their JWKS URL is". (Let me know if I misunderstood your question)
converting flutter.dev/go/flutter-pluggable-ci-design into rfc
Pre-launch Checklist
design doc.dart run bin/assign_rfc_number.dartlocally when instructed).If you need help, consider asking for advice on the #hackers channel on Discord.