Add Spire trustsource#14
Conversation
pawalt
force-pushed
the
pawalt/add-spire-trustsource
branch
4 times, most recently
from
dc80a6a to
daa224c
Compare
pawalt
force-pushed
the
pawalt/add-spire-trustsource
branch
2 times, most recently
from
c14eb3e to
38c8248
Compare
pawalt
force-pushed
the
pawalt/add-spire-trustsource
branch
3 times, most recently
from
08d783a to
1759f4e
Compare
Signed-off-by: Peyton Walters <pwalters18@bloomberg.net>
Signed-off-by: Peyton Walters <pwalters18@bloomberg.net>
Signed-off-by: Peyton Walters <pwalters18@bloomberg.net>
Signed-off-by: Peyton Walters <pwalters18@bloomberg.net>
Signed-off-by: Peyton Walters <pwalters18@bloomberg.net>
Signed-off-by: Peyton Walters <pwalters18@bloomberg.net>
Signed-off-by: Peyton Walters <pwalters18@bloomberg.net>
Signed-off-by: Peyton Walters <pwalters18@bloomberg.net>
Signed-off-by: Peyton Walters <pwalters18@bloomberg.net>
Signed-off-by: Peyton Walters <pwalters18@bloomberg.net>
Signed-off-by: Peyton Walters <pwalters18@bloomberg.net>
Signed-off-by: Peyton Walters <pwalters18@bloomberg.net>
pawalt
force-pushed
the
pawalt/add-spire-trustsource
branch
from
7d546c6 to
105f661
Compare
| select { | ||
| case w.source.updateChan <- struct{}{}: | ||
| case <-time.After(w.source.updateTimeout): | ||
| } |
There was a problem hiding this comment.
Is it necessary to write the backup to disk before making the certs available for use? I think it might be better to make them available for other threads, then write to disk. This makes the disk write a non-blocking call for cert usage.
There was a problem hiding this comment.
I need this blocking so that I can make sure the write completed before reading a file in my tests. I'm not too worried about extra blocking in tests. However, I only need it in tests, so I've added a testing flag. Let me know what you think.
Signed-off-by: Peyton Walters <pwalters18@bloomberg.net>
| // NewSpireTestSource creates a new spire trust source with the test flag on. | ||
| func NewSpireTestSource(spireEndpointURLs map[string]string, localBackupDir string) (*SpireTrustSource, error) { | ||
| ts, err := NewSpireTrustSource(spireEndpointURLs, localBackupDir) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| ts.testing = true | ||
| return ts, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
I don't love this. I'm going to give some thought to a better way to accomplish what's needed.
There was a problem hiding this comment.
OK. I would love to use method overriding to add an optional testing flag, but Go doesn't have overriding.
There was a problem hiding this comment.
Another common pattern is to pass in a config struct that can take on default values instead of using individual parameters.
There was a problem hiding this comment.
You could also pass an optional configuration function ala https://godoc.org/github.com/pkg/term#CBreakMode
There was a problem hiding this comment.
@dennisgove seemed to have the main problem of "we don't want to have test flags in prod code", so I don't think extra configuration functions would fix that problem. I'm really not sure how to fix that problem other than making a whole new type of watcher for testing purposes, but I feel like that defeats the purpose of the tests.
Signed-off-by: Peyton Walters <pwalters18@bloomberg.net>
pawalt
force-pushed
the
pawalt/add-spire-trustsource
branch
from
b758d1e to
2ed1e29
Compare
|
I've added a second struct. Let me know what you think. |
3 participants
This resolves #2
Describe your changes
This PR adds support for using Spire servers as a trust source.
Testing performed
Unit tests have been written in
spiretrustsource_test.goAdditional context
I moved the file trust source and this one to using afero to ease testing.
I'm not particularly happy with how much code is repeated between tests, so I'm looking to refactor them with the suite package.
EDIT: I refactored some code, and I think this is fine without using suite.