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

ctfe: Add readonly mode #927

Merged
merged 2 commits into from
May 11, 2022
Merged

ctfe: Add readonly mode #927

merged 2 commits into from
May 11, 2022

Conversation

pav-kv
Copy link
Contributor

@pav-kv pav-kv commented May 10, 2022

The new is_readonly flag in CTFE config allows setting up logs that accept
only read requests. This is particularly useful in the scenario when a CT log is
gradually phased out from the ecosystem.

Handling readonly transition at CTFE level is beneficial over (in some ways),
but is mostly complementary to the current approach of setting the Trillian log
to DRAINING / FROZEN state.

The Trillian state transition is:

  • more responsive (it happens immediately after the state update RPC), and
  • more defensive w.r.t. correctness (it prevents new writes to the Merkle tree
    on Trillian level).

The state transition at the CTFE level is:

  • as [not] responsive as the config rollout procedure, and
  • less strict in a sense that updates might still slip through to the Merkle
    tree (e.g. if some CTFE job is stale or rolled back), but is
  • more resource-efficient, as it rejects write requests early.

Checklist

@pav-kv pav-kv marked this pull request as ready for review May 10, 2022 14:16
@pav-kv
Copy link
Contributor Author

pav-kv commented May 10, 2022

PTAL

@mhutchinson
Copy link
Contributor

It looks like this will cause a 404 to be served as the endpoint definition is literally deleted. Is that analysis correct, and is that the actual behaviour we'd want? Seems like it would be surprising for a client that was writing to a given URL to be suddenly provided with a 404. My first instinct would be that the log is down, not that it has been made read-only.

@pav-kv
Copy link
Contributor Author

pav-kv commented May 10, 2022

Yes, it would return 404. Which code would you expect?

@phbnf
Copy link
Contributor

phbnf commented May 11, 2022

I like the idea of returning a 404, it's simple and not ambiguous: the endpoint was not found.

I see your point Martin, and we could also consider a different error code such as "405 Method Not Allowed". However:

  1. Would that actually address your concern? The endpoint would still be unavailable, just with a different error code.
  2. It opens the door to inferring the state of a log based on the error code it returns. I don't think it's a desirable thing.

Do you have any other suggestion?

@mhutchinson
Copy link
Contributor

403 Forbidden or 405 Method Not Allowed would feel more natural to me, but as this is largely subjective and I'm in the minority of 2:1, then YOLO.

@roger2hk
Copy link
Contributor

403 Forbidden or 405 Method Not Allowed sounds better than 404 Not Found when the write request is being refused due to a read-only resource.

We may want to consider 422 Unprocessable Entity as well.

   The 422 (Unprocessable Entity) status code means the server
   understands the content type of the request entity (hence a
   415(Unsupported Media Type) status code is inappropriate), and the
   syntax of the request entity is correct (thus a 400 (Bad Request)
   status code is inappropriate) but was unable to process the contained
   instructions.  For example, this error condition may occur if an XML
   request body contains well-formed (i.e., syntactically correct), but
   semantically erroneous, XML instructions.

@pav-kv
Copy link
Contributor Author

pav-kv commented May 11, 2022

Spec of the 405 error says it MUST provide an Allow header with the list of allowed methods: https://datatracker.ietf.org/doc/html/rfc7231#section-6.5.5. Also, "method" means a specific thing in HTTP: https://datatracker.ietf.org/doc/html/rfc7231#section-4, it doesn't look like a synonym to our (more narrowly defined) "endpoint". It still applies in our case though, but the way I interpret it is that "POST" method is not allowed on "/ct/v1/add-chain" resource. In this regard, no method is allowed on this resource, and one can also argue that "add-chain" resource is not found.

Other codes would probably also have peculiarities. 422 says "the syntax of the request entity is correct", which effectively binds us to have some request validation before returning this code.

403 says:

An origin server that wishes to "hide" the current existence of a
forbidden target resource MAY instead respond with a status code of
404 (Not Found).

I propose to go with 404, to avoid [over-]thinking these peculiarities, because it "just works", and is also a (tinily) simpler implementation.

@pav-kv pav-kv merged commit 810a97f into google:master May 11, 2022
@pav-kv pav-kv deleted the add_readonly_mode branch May 11, 2022 16:14
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

Successfully merging this pull request may close these issues.

None yet

4 participants