Documentation: add a document on storage options #632

ericchiang · 2016-10-24T19:04:59Z

fyi @dghubble there's some third party resource docs here.

pop

Overall this looks good.

I have a lot of feedback, but most of it is related to following the style guide. As far as the word choice and sentence flow goes you can take my feedback with a grain of salt.

As far as enforcing the coreos/docs style guide goes:

Links should follow the [link text][ref], with [ref]: url at the bottom of the file.
Headings ought to be in sentence heading.

I'll make a PR with a reference to the coreos docs style guide to the CONTRIBUTING.md doc ASAP.

pop · 2016-10-24T23:47:36Z

Documentation/storage.md

+
+## Caveat: Running replicated instances
+
+Validation is still being done to ensure correctness when multiple instances of dex are using the same backing storage. While everything "should just work", edge cases require time to investigate and write tests for. This work can be followed on the issue tracker [here](https://github.com/coreos/dex/issues/600).


Reference style hyperlinks ([hyperlinked text][label])

I would reword this to be:

Dex performs correctness validation when multiple instances use the same backing storage. This should "Just Work" but when it doesn't you should check out the [dex issue tracker][dex-issue-tracker-url] and add or comment on a relevant issue.

Dex performs correctness validation when multiple instances use the same backing storage.

That's not the same thing. This is stating that we still need to write the tests to prove correctness and you probably shouldn't be running these kind of setups in the first place. How does this sound?

Tests still need to be written to validate that multiple instances of dex behave correctly when using the same storage.
While there aren't any technical limitations, edge cases have been observed and progress on these kind of bugs can be found on the [dex issue tracker].

pop · 2016-10-24T23:48:29Z

Documentation/storage.md

+
+Regardless of the implementation, dex saves sensitive data in its backing storage: signing keys, bcrypt'd passwords, etc. Storage breaches are serious because attacks may propagate to any application which relies on dex. Whatever storage option used, transport security and database ACLs are both musts.
+
+## Caveat: Running replicated instances


Sentence style headings (## Only the first character and proper nouns are capitalized)

pop · 2016-10-24T23:49:16Z

Documentation/storage.md

+description: "An OAuth2 client."
+```
+
+Once the `ThirdPartyResource` is created, custom resources can be created at a namespace level (though there will be a gap between the `ThirdPartyResource` being created and the API server accepting the custom resource). While most fields are user defined, the API server still respects the common `ObjectMeta` and `TypeMeta` values. For example names are still restricted to a small set of characters, and the `resourceVersion` field can be used for an [atomic compare and swap](https://github.com/kubernetes/kubernetes/blob/release-1.4/docs/devel/api-conventions.md#concurrency-control-and-consistency).


Reference style hyperlinks ([hyperlinked text][label])

pop · 2016-10-24T23:50:59Z

Documentation/storage.md

+      caFile: /etc/dex/postgres.ca
+```
+
+The SSL "mode" corresponds to the `github.com/lib/pq` package [connection options](https://godoc.org/github.com/lib/pq#hdr-Connection_String_Parameters). If unspecified, dex defaults to the strictest mode "verify-full".


Reference style hyperlinks ([hyperlinked text][label])

pop · 2016-10-24T23:52:26Z

Documentation/storage.md

+
+Each storage implementation bears a large ongoing maintenance cost and needs to be updated every time a feature requires storing a new type. Bugs often require in depth knowledge of the backing software, and much of this work will be done by developers who are not the original authors. Changes to dex which add new storage implementations are not merged lightly.
+
+Those who still want to construct a proposal for a new storage should review the following packages:


Put this under a third-tier heading ### New storage option references

Wouldn't that imply that it's part of the SQL section (as it's not)?

The above second-tier heading is ## adding a new storage option, so putting a third-tier heading should make it obvious that this is a sub-topic of ## adding a new storage option.
This fine-grain organization makes the table of contents on coreos.com more useful.

pop · 2016-10-25T00:23:27Z

Documentation/storage.md

+
+Migrations are performed automatically on the first connection to the SQL server (it does not support rolling back). Because of this, dex requires being able to add and alter the tables for its database.
+
+NOTE: Previous versions of dex required symmetric keys to encrypt certain values before sending them to the database. This feature has not been ported to V2 yet, though it may be added later (but possibly without a migration path for users of the current implementation).


I would make Note bold.

I would reword the last second sentence to be

This feature has not yet been ported to dex v2. If it is added later there may not be a migration path for current v2 users.

pop · 2016-10-25T00:33:59Z

Documentation/storage.md

+
+### Postgres
+
+Dex authenticates to Postgres with a username and password. As stated above, because dex performs migrations, it requires privileged access to its own database. Database table names are not configurable, and to prevent table name clashes with other applications admins should expect to dedicated a database for dex's sole use. 


I would reword this paragraph to be:

Admins may want to dedicate a database to dex for the following reasons:

Dex requires privileged access to its database because it performs migrations.

Dex's database table names are not configurable; when shared with other applications there may be table name clashes.

pop · 2016-10-25T00:34:59Z

Documentation/storage.md

+
+Those who still want to construct a proposal for a new storage should review the following packages:
+
+* `github.com/coreos/dex/storage`: Interface definitions which the storage must implement. NOTE: This package is not stable.


This should be a clickable link.

We can't link to a godoc.org until we've made this branch the default branch. Part of that getting to that point is writing docs like this :)

Fair enough. I wanted to click on it and figured readers would too, but if it's technically not an option that's 👍

pop · 2016-10-25T00:35:00Z

Documentation/storage.md

+Those who still want to construct a proposal for a new storage should review the following packages:
+
+* `github.com/coreos/dex/storage`: Interface definitions which the storage must implement. NOTE: This package is not stable.
+* `github.com/coreos/dex/storage/conformance`: Conformance tests which storage implementations must pass.


This should be a clickable link.

pop · 2016-10-25T00:36:01Z

Documentation/storage.md

+
+* Integration testing setups (Travis and developer workstations).
+* Transactional requirements: atomic deletes, updates, etc.
+* Is there an established, reasonable Go client?


I would reword this last bullet to be:

Is there an established and reasonable Go client?

ericchiang · 2016-10-25T17:44:26Z

@ElijahCaine thanks. Have a few counter comments, but other than that will rework this PR later today.

ericchiang · 2016-10-26T17:15:39Z

@ElijahCaine updated with a second commit. Going to leave this here if you want to add any last second notes. Will squash and merge in a bit.

pop

Hey Eric, this all looks really good.

I misspoke earlier when I said sentences were on their own lines, the style guide actually says that it's one paragraph per line. If you could revert that formatting to what you had earlier (sorry again!) and the line numbering change I commented on, I'll give this the thumbs up.

pop · 2016-10-26T17:52:17Z

Documentation/storage.md

+When using Postgres, admins may want to dedicate a database to dex for the following reasons:
+
+1. Dex requires privileged access ot its database because it performs migrations.
+1. Dex's database table names are not configurable; when shared with other applications there may be table name clashes.


This should probably be 2.

updated, thanks

ericchiang · 2016-10-26T19:25:55Z

Haha, thanks I'll update again.

ericchiang added kind/documentation v2 labels Oct 24, 2016

ericchiang assigned pop Oct 24, 2016

pop suggested changes Oct 25, 2016

View reviewed changes

ericchiang force-pushed the dev-docs-storage-options branch from 4475ce7 to 7928ea0 Compare October 26, 2016 17:14

pop suggested changes Oct 26, 2016

View reviewed changes

Documentation: add a document on storage options

6c48398

ericchiang force-pushed the dev-docs-storage-options branch from 7928ea0 to 6c48398 Compare October 26, 2016 19:33

ericchiang merged commit 99e312e into dexidp:dev Oct 26, 2016

ericchiang deleted the dev-docs-storage-options branch October 26, 2016 19:34

rithujohn191 unassigned pop Aug 31, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Documentation: add a document on storage options #632

Documentation: add a document on storage options #632

ericchiang commented Oct 24, 2016

pop left a comment •

edited

Loading

pop Oct 24, 2016 •

edited

Loading

pop Oct 25, 2016

ericchiang Oct 25, 2016

pop Oct 24, 2016

pop Oct 24, 2016 •

edited

Loading

pop Oct 24, 2016 •

edited

Loading

pop Oct 24, 2016

ericchiang Oct 25, 2016

pop Oct 25, 2016

pop Oct 25, 2016

pop Oct 25, 2016

pop Oct 25, 2016

pop Oct 25, 2016

ericchiang Oct 25, 2016

pop Oct 25, 2016

pop Oct 25, 2016

pop Oct 25, 2016

ericchiang commented Oct 25, 2016

ericchiang commented Oct 26, 2016

pop left a comment

pop Oct 26, 2016

ericchiang Oct 26, 2016

ericchiang commented Oct 26, 2016


		## Caveat: Running replicated instances

		Validation is still being done to ensure correctness when multiple instances of dex are using the same backing storage. While everything "should just work", edge cases require time to investigate and write tests for. This work can be followed on the issue tracker [here](https://github.com/coreos/dex/issues/600).


		Regardless of the implementation, dex saves sensitive data in its backing storage: signing keys, bcrypt'd passwords, etc. Storage breaches are serious because attacks may propagate to any application which relies on dex. Whatever storage option used, transport security and database ACLs are both musts.

		## Caveat: Running replicated instances


		Each storage implementation bears a large ongoing maintenance cost and needs to be updated every time a feature requires storing a new type. Bugs often require in depth knowledge of the backing software, and much of this work will be done by developers who are not the original authors. Changes to dex which add new storage implementations are not merged lightly.

		Those who still want to construct a proposal for a new storage should review the following packages:


		Migrations are performed automatically on the first connection to the SQL server (it does not support rolling back). Because of this, dex requires being able to add and alter the tables for its database.

		NOTE: Previous versions of dex required symmetric keys to encrypt certain values before sending them to the database. This feature has not been ported to V2 yet, though it may be added later (but possibly without a migration path for users of the current implementation).


		### Postgres

		Dex authenticates to Postgres with a username and password. As stated above, because dex performs migrations, it requires privileged access to its own database. Database table names are not configurable, and to prevent table name clashes with other applications admins should expect to dedicated a database for dex's sole use.


		Those who still want to construct a proposal for a new storage should review the following packages:

		* `github.com/coreos/dex/storage`: Interface definitions which the storage must implement. NOTE: This package is not stable.

Documentation: add a document on storage options #632

Documentation: add a document on storage options #632

Conversation

ericchiang commented Oct 24, 2016

pop left a comment • edited Loading

Choose a reason for hiding this comment

pop Oct 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pop Oct 24, 2016 • edited Loading

Choose a reason for hiding this comment

pop Oct 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericchiang commented Oct 25, 2016

ericchiang commented Oct 26, 2016

pop left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericchiang commented Oct 26, 2016

pop left a comment •

edited

Loading

pop Oct 24, 2016 •

edited

Loading

pop Oct 24, 2016 •

edited

Loading

pop Oct 24, 2016 •

edited

Loading