add download method for updater by groob · Pull Request #20 · kolide/updater

groob · 2017-06-26T18:34:18Z

Closes #13

In the process of adding the method also discovered and updated a few other concerns:

Closes #16
Closes #18

still a WIP pr for now

groob · 2017-06-26T18:35:03Z

+	return &Client{manager: manager}, nil
+}
+
+func (c *Client) Update() (files map[targetNameType]FileIntegrityMeta, latest bool, err error) {


// update refreshes local state, returning all updated targets.

groob · 2017-06-26T18:35:38Z

+	return <-ff, latest, nil
+}
+
+func (c *Client) Download(targetName string, destination io.Writer) error {


Download securely copies a target to an io.Writer, doing all the checks on that file.

groob · 2017-06-26T18:35:55Z

+	}
+	currentMeta, ok := files[targetNameType(targetName)]
+	if !ok {
+		// return errors.Errorf("targetName %s not found", targetName)


groob · 2017-06-26T18:37:37Z

 	return nil
 }

+func (fim FileIntegrityMeta) verifyIO(r io.Reader, size int64) error {


This method works like fim.verify() but attempts to work on a stream in stead of []byte, addressing #18

groob · 2017-06-26T18:40:42Z

 }

+func (fim FileIntegrityMeta) verifyIO(r io.Reader, size int64) error {
+	if size != int64(fim.Length) {


@murphybytes I had to convert int to int64 a few times. Would it make sense to just set fim.Length to an int64 type, so it's decoded from JSON into int64? Would probably be easier to work with.

Yes. I don't think that will cause any issues?

groob · 2017-06-26T18:42:38Z

 	targets   *Targets
 	client    *http.Client
+
+	actionc chan func()


the Snapshot, Timestamp, Targets and Root fields on this structure need to be safe for concurrency. To accomplish that, I created a function type channel which I use in the new refresh method, avoiding race conditions and ensuring files are updated/read in order.

groob · 2017-06-26T18:42:54Z

+func (rs *repoMan) refresSafe() (bool, error) {
+	errc := make(chan error)
+	var isLatest bool
+	rs.actionc <- func() {


example action channel function.

groob · 2017-06-26T18:43:21Z

+	<-quit
+}
+
+func (rs *repoMan) refresSafe() (bool, error) {


need to remove the old refresh() method and rename this one to refresh.

groob · 2017-06-26T18:44:23Z

 	return stagePath, nil
 }

+func (rs *repoMan) downloatTargetToWriter() error {


TODO: delete

groob · 2017-06-26T18:50:51Z

 }

-func (rs *repoMan) refreshTargets(root *Root, snapshot *Snapshot) (*Targets, string, error) {
+func (rs *repoMan) refreshTargets(root *Root, snapshot *Snapshot) (*Targets, bool, error) {


In the previous method, refreshTargets downloaded the settings.Target.
Sometimes we want to refresh without downloading to the staging path. I let the caller decide if they need to update.

There's a problem here, that someone calling Download() will update the local repository, but that won't be seen by the looping update function... need to think/test better.

murphybytes · 2017-06-26T18:55:57Z

My general sense is that you're adding a lot of complexity here by adding concurrency and I don't see the need for it given what our needs are today. Concurrency will make it more probable that we'll miss a test case for a situation that might cause a panic in a production environment. If you have a really really good reason for going down this path I'd like to hear it.

groob · 2017-06-26T19:00:47Z

I'm not adding concurrency, just trying to limit the potential for race conditions in already existing situations.

Today there's a loop which checks for new updates every X minutes. That loop ends up calling refresh() which updates a bunch of pointers on a struct.

That same method is also needed in order to Download something. There's no way to keep update running safely while also calling Download somewhere without ensuring that the code is thread safe.

murphybytes · 2017-06-26T19:09:57Z

I see your point. Another way to do this would be to add a Download method to the updater, that uses a channel to send a message to the goroutine in the updater, this would perform the download, blocking refresh until you're done, at which point you send a message back to the Download method with the results. Or you could send a function that would get executed in the updater goroutine and get the results as arguments.

murphybytes · 2017-06-26T19:12:41Z

If you could perform the operations in the updater goroutine you eliminate the possibility of race conditions since that goroutine is the only thing that touches the tuf repository

groob · 2017-06-26T19:19:13Z

That doesn't fix a problem that exists right now:

the update method cares about a specific target, like latest/target. It downloads that target if the targets file was updated.

But the intention of the Download method is to download a specifc target, instead of the one in the settings. Say the updater is watching for latest/target, but I need to download previous/target. When that happens, the state of the repository will get updated, and the update loop will not realize that it needs to perform an update.

There needs to be a way to watch for updates to a specific file rather than to the entire targets file..

groob · 2017-06-26T19:20:39Z

what I'm saying is that the current updater method assumes that if the targets.json has been updated there was an update to a specific target, which is not true.

murphybytes · 2017-06-26T19:21:48Z

Ahh, got it. I see what you're up to.

Closes #13 In the process of adding the method also discovered and updated a few other issues: Closes #16 Closes #18

…cessary casting

murphybytes · 2017-06-27T17:17:19Z

+	actionc             chan func()
 	checkFrequency      time.Duration
 	notificationHandler NotificationHandler
 	client              *tuf.Client


Maybe the client would be better if it were instantiated inside the loop and was not a member of the Updater struct since your sharing it's state between the main process and the loop goroutine unless you have a reason to share that state.

If you instantiated the client in the goroutine you could pass it to an argument the func you send over the channel.

I need to call download on the client itself so it has to be available on the updater struct.

murphybytes · 2017-06-27T17:18:08Z

 		select {
 		case <-ticker:
+		case f := <-u.actionc:
+			f()


👍 I love this pattern

murphybytes · 2017-06-27T17:21:41Z

+	// goroutine.
+	errc := make(chan error)
+	u.actionc <- func() {
+		_, _, err := u.client.Update()


u.actionc <- func(client *tuf.Client) { _, _, err := client.Update() if err != nil { errc <- err return } errc <- client.Download(target, destination) return <-errc }

Not sure I follow

That would limit the action channel to only be usable with a func(client) signature. I don't see the benefit. We'll just have to change it next time we want to do something in the loop.

murphybytes · 2017-06-27T17:25:29Z

@@ -124,21 +128,72 @@ func (u *Updater) Stop() {
 func (u *Updater) loop() {
 	ticker := time.NewTicker(u.checkFrequency).C


So you'd instantiate the tuf.Client here.

groob · 2017-06-27T18:53:43Z

@murphybytes this branch is now ready to merge. I've updated the tests and verified manually that the example works with both continually monitoring a target and with using Download directly.

Once this is merged, I'll work on #22 where I'll only expose the current updater methods and make everything except the types private.

murphybytes

Looks great.

groob commented Jun 26, 2017

View reviewed changes

groob mentioned this pull request Jun 27, 2017

Add support to handle very large files. #21

Merged

groob added 8 commits June 27, 2017 09:24

add download method for updater

ae040de

Closes #13 In the process of adding the method also discovered and updated a few other issues: Closes #16 Closes #18

update error checking

203b45b

add download flag

6b03158

delete now unneeded method with file stream

b3171bd

move all download actions to updater loop

36fce67

consolidate methods

94e828b

remove targetNameType and switch Length field to int64 to reduce unne…

7e86c84

…cessary casting

use hash to check if new file exists

a9553ef

murphybytes reviewed Jun 27, 2017

View reviewed changes

Comment thread updater.go

select {

case <-ticker:

case f := <-u.actionc:

f()

Copy link
Copy Markdown

Contributor

murphybytes Jun 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I love this pattern

murphybytes reviewed Jun 27, 2017

View reviewed changes

fix ints in tests

5cdb39a

groob mentioned this pull request Jun 27, 2017

combine updater and tuf packages #22

Open

groob added 2 commits June 27, 2017 14:32

switch tests to use client

61f29df

use a mock server for the notary url

1368a25

murphybytes approved these changes Jun 27, 2017

View reviewed changes

groob merged commit 04e45b3 into kolide:master Jun 27, 2017

groob deleted the download_method branch June 27, 2017 20:23

		@@ -124,21 +128,72 @@ func (u *Updater) Stop() {
		func (u *Updater) loop() {
		ticker := time.NewTicker(u.checkFrequency).C

Conversation

groob commented Jun 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

murphybytes commented Jun 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

groob commented Jun 26, 2017

Uh oh!

murphybytes commented Jun 26, 2017

Uh oh!

murphybytes commented Jun 26, 2017

Uh oh!

groob commented Jun 26, 2017

Uh oh!

groob commented Jun 26, 2017

Uh oh!

murphybytes commented Jun 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

murphybytes Jun 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

groob commented Jun 27, 2017

Uh oh!

murphybytes left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

murphybytes commented Jun 26, 2017 •

edited

Loading

murphybytes Jun 27, 2017 •

edited

Loading