This repository was archived by the owner on Nov 10, 2022. It is now read-only.
Open
Conversation
MikeCamel
added
documentation
steveej
reviewed
Feb 10, 2020
steveej
left a comment
steveej
left a comment
There was a problem hiding this comment.
Thanks for taking the time to write this up!
Minor wording change from Stefan. Co-Authored-By: Stefan Junker <steveeJ@users.noreply.github.com>
Minor typo fix. Co-Authored-By: Stefan Junker <steveeJ@users.noreply.github.com>
lkatalin
suggested changes
Feb 12, 2020
Contributor
lkatalin
left a comment
lkatalin
left a comment
There was a problem hiding this comment.
I suggested a number of somewhat nitpicky changes here. Feel free to disregard anything I may have misunderstood. Most of the changes requested center around:
- Eliminating the use of "it" as a pronoun. The description is very technical and I think it should be painfully clear which components are doing what. Actually, most of these changes are about establishing painful clarity in all sentences.
- Being more detailed about where the WPEK comes from, which components are privvy to it, etc. There are some details omitted that would raise questions to an outside reader, IMO. The reader should know exactly what this key is so they understand why the channel is secure.
remove "it" Co-Authored-By: Lily Sturmann <lkatalin@users.noreply.github.com>
Disambiguation. Co-Authored-By: Lily Sturmann <lkatalin@users.noreply.github.com>
Disambiguation. Co-Authored-By: Lily Sturmann <lkatalin@users.noreply.github.com>
Disambiguation. Co-Authored-By: Lily Sturmann <lkatalin@users.noreply.github.com>
Disambiguation. Co-Authored-By: Lily Sturmann <lkatalin@users.noreply.github.com>
Disambiguation. Co-Authored-By: Lily Sturmann <lkatalin@users.noreply.github.com>
Disambiguation. Co-Authored-By: Lily Sturmann <lkatalin@users.noreply.github.com>
Typo. Co-Authored-By: Lily Sturmann <lkatalin@users.noreply.github.com>
Disambiguation. Co-Authored-By: Lily Sturmann <lkatalin@users.noreply.github.com>
Disambiguation. Co-Authored-By: Lily Sturmann <lkatalin@users.noreply.github.com>
Closed
axelsimon
suggested changes
Feb 17, 2020
Contributor
Author
Done. |
MikeCamel
requested review from
axelsimon,
connorkuehl,
lkatalin,
npmccallum and
ueno
lkatalin
suggested changes
Jul 16, 2020
Contributor
lkatalin
left a comment
lkatalin
left a comment
There was a problem hiding this comment.
I actually think this is pretty close. There are a few formatting errors and I had a question about one of the rejected proposals.
|
|
||
| ## Motivation | ||
|
|
||
| Once a Keep is prepared with shim, Wasm runtime, etc., the |
Contributor
There was a problem hiding this comment.
Suggested change
| Once a Keep is prepared with shim, Wasm runtime, etc., the | |
| Once a Keep is prepared with shim and Wasm runtime, the |
The "etc." leaves me feeling confused about what else is in there.
Comment on lines
+23
to
+24
| A Keep is the fundamental runtime component of Enarx: a TEE instance | ||
| + Enarx runtime pieces, including WebAssembly and WASI layers. In |
Contributor
There was a problem hiding this comment.
This is rendered strangely in markdown. "Enarx runtime pieces" becomes a bullet point.
Comment on lines
+81
to
+83
| If a port has been pre-established, between the Enarx Client Agent and | ||
| the Keep, then the Keep MUST listen on this port, | ||
| and MAY also listen on the well-known port. |
Contributor
There was a problem hiding this comment.
Why would the Keep listen on both ports?
Comment on lines
+98
to
+99
| This RFC arose from this issue: [Workload (WASM binary) loading | ||
| - connection termination](https://github.com/enarx/enarx/issues/140) |
Contributor
There was a problem hiding this comment.
This also renders strangely in markdown.
| sends to Keep via a listening agent in the Keep. Keep decrypts, loads | ||
| and runs. This maintains the host agent's status as a proxy between | ||
| other components, and doesn't make further assumptions about routing | ||
| which is made by 2. |
Comment on lines
+122
to
+125
| - the host agent should act as an untrusted proxy wherever possible, | ||
| and not include process logic. | ||
| - maintaining multiple versions of host agents is complex from | ||
| a deployment point of view. |
Contributor
There was a problem hiding this comment.
I don't actually understand why process logic is required for the host agent to be a proxy in this way, or why multiple versions of the host agent would be necessary with this design.
enarxbot
assigned
MikeCamel and unassigned
lkatalin,
ueno,
npmccallum,
axelsimon and
connorkuehl
enarxbot
assigned
axelsimon,
connorkuehl,
npmccallum and
ueno and unassigned
ueno,
npmccallum,
axelsimon and
connorkuehl
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Initial draft of workload loading RFC.
Created two new issues based on the work: #227, #228.
Added line breaks: what are the kids using as default line length these days?
Addresses issue #140.