Persist proxies from config files #4407

mkeeler · 2018-07-17T20:49:08Z

Also change how loadProxies works. Now it will load all persisted proxies into a map, then when loading config file proxies will look up the previous proxy token in that map.

The new test is actually testing that proxy tokens are saved/restored between unloading/reloading of the proxy configuration (happens during a restart or during a online config reload). Additionally its also testing that removing a proxy from the config will remove it from what gets persisted to disk and wont get reload the next time the proxies are read from disk.

Additionally I tested manually with running a config file managed proxy, performing online reloads and restarting the process. It no longer kills the existing proxy and restarts it.

Some behavior that is currently still in there is that proxy configurations can be overwritten for config-file based proxies via the API. During the next reload the configuration will be reloaded from the config and the API registration will be effectively gone.

There is a edge case where you have a proxy in the config, do an online register via the API, remove it from the config and when you reload/restart the API proxy is still valid and gets loaded properly. I don't think this is incorrect, just behavior that might not be immediately apparent.

banks · 2018-07-18T16:15:37Z

Some behavior that is currently still in there

To be clear for other readers, that is the same issue that occurs now - if you update a config-file registration via API it's new config will survive until agent reloads. After reload, agent will see both states and will treat the config file is authoritative so effectively "undoes" the changes made via the API.

I think that edge case is arguably the desired behaviour but might confuse someone. In practice you should choose one of files or API for registering a particular instance so this is moot now and after this PR.

banks

This looks great!

Mostly nit picks, one thing that seems worth changing to not end up in a worse state than now in case snapshot files are corrupt.

banks · 2018-07-18T16:17:21Z

agent/agent.go

@@ -208,6 +208,9 @@ type Agent struct {

 	// proxyManager is the proxy process manager for managed Connect proxies.
 	proxyManager *proxy.Manager
+
+	// Protects proxy information in the local state from concurrent modification


nit: idiomatic go comments always start with the name of the variable or function being documented. Even though this is private and so will never show up in Godoc, it seems better to stick to the convention unless it's really awkward for some reason.

In this case it's trivial to add // proxyLock protects proxy...

I never knew that was a thing. I will update it.

banks · 2018-07-18T16:18:54Z

agent/agent.go

@@ -1616,16 +1619,18 @@ func (a *Agent) purgeService(serviceID string) error {
 type persistedProxy struct {
 	ProxyToken string
 	Proxy      *structs.ConnectManagedProxy
+	FromConfig bool


bikeshed: is it clearer to say FromFile? Technically "config" is also provided via the API, the distinction here is really the source (file vs API).

banks · 2018-07-18T16:19:22Z

agent/agent.go

@@ -1616,16 +1619,18 @@ func (a *Agent) purgeService(serviceID string) error {
 type persistedProxy struct {
 	ProxyToken string
 	Proxy      *structs.ConnectManagedProxy
+	FromConfig bool
 }

 // persistProxy saves a proxy definition to a JSON file in the data dir


nit: document the new param semantics.

banks · 2018-07-18T16:20:27Z

agent/agent.go

 	}
 	return nil
 }

+func (a *Agent) AddProxy(proxy *structs.ConnectManagedProxy, persist, fromConfig bool,


nit: go vet will complain a public function has no doc comment. Maybe just copy paste the old one here?

banks · 2018-07-18T16:20:49Z

agent/agent.go

@@ -2316,6 +2332,12 @@ func (a *Agent) RemoveProxy(proxyID string, persist bool) error {
 	return nil
 }

+func (a *Agent) RemoveProxy(proxyID string, persist bool) error {


banks · 2018-07-18T16:23:09Z

agent/agent.go

+	if err != nil {
+		if !os.IsNotExist(err) {
+			return fmt.Errorf("Failed reading proxies dir %q: %s", proxyDir, err)
+		}


nit: it's more idiomatic in go to prefer to return in the branch rather than indent the rest of the method in an else. We could just return nil here and then remove the else below.

I forgot for a moment you can do a range on a nil slice and it will do nothing. Otherwise the else would be necessary to prevent that as we wouldn't return for an IsNotExist error.

Ah true, I thought all of the rest of the method was guarded by else so return nil here was fine but yeah all good 😄

banks · 2018-07-18T16:26:44Z

agent/agent.go

+			buf, err := ioutil.ReadAll(fh)
+			fh.Close()
+			if err != nil {
+				return fmt.Errorf("failed reading proxy file %q: %s", file, err)


Interesting thought: these failures have changed with this refactor. Before if a snapshot file was corrupt or broken in some way, the proxies from config would still be loaded and just the API persisted ones after the failing one in the dir list would fail to load.

I don't think that was strongly principled but it's now the case that a corrupt snapshot file prevents us from loading any proxies at all even from config...

Maybe we should move this whole loop out into another method and then still continue if it errors but log the error. That way we can still load proxies from config file as before even if the state files are jank. (and then we'd blow away the state which "fixes" the problem with the caveat that API registered stuff might dissappear. That at least is the same as now though not worse.

Moving it out isn't really necessary. Instead I can just change the return fmt.Errorf into a.logger.Printf and all will be well.

Right but you need to stop the current loop iteration too. Return and separate method seems clean to me just like you did 👍

Also change how loadProxies works. Now it will load all persisted proxies into a map, then when loading config file proxies will look up the previous proxy token in that map.

banks

Great 🎉

banks

Hmm the test failures actually look legit:

--- FAIL: TestAgent_PurgeProxyOnDuplicate (0.03s)
    agent_test.go:1527: 
        	Error Trace:	agent_test.go:1527
        	Error:      	An error is expected but got nil.
        	Test:       	TestAgent_PurgeProxyOnDuplicate
        	Messages:   	should have removed remote state
--- FAIL: TestAgent_ReLoadProxiesFromConfig (0.19s)
    agent_test.go:2890: 
        	Error Trace:	agent_test.go:2890
        	Error:      	"map[web-proxy:%!s(*local.ManagedProxy=&{0xc009de59c0 9336b181-3b34-a340-b656-a90f3ae3a186 0xc009dc8300})]" should have 0 item(s), but has 1
        	Test:       	TestAgent_ReLoadProxiesFromConfig

Can you check those out?

mkeeler · 2018-07-19T13:16:30Z

So they do. Apparently my GOTEST_FLAGS="-run TestAgent_*Prox*" wasn't running everything I thought it was.

mkeeler · 2018-07-19T13:21:43Z

@banks Fortunately it was the tests that needed updating. Serves me right for trying to use two wildcards in a test name match.

mkeeler · 2018-07-19T13:23:08Z

Locally ran make test and everything passed without needing a retry.

banks · 2018-07-19T14:07:34Z

FWIW I think -run ~~can be~~ is a regex:

Yep:

$ go help testflag
...
	-run regexp
	    Run only those tests and examples matching the regular expression.
	    For tests, the regular expression is split by unbracketed slash (/)
	    characters into a sequence of regular expressions, and each part
	    of a test's identifier must match the corresponding element in
	    the sequence, if any. Note that possible parents of matches are
	    run too, so that -run=X/Y matches and runs and reports the result
	    of all tests matching X, even those without sub-tests matching Y,
	    because it must run them to look for those sub-tests.

mkeeler requested a review from banks July 18, 2018 14:15

mkeeler force-pushed the proxy-persist branch 2 times, most recently from f19baaa to 1ff43ba Compare July 18, 2018 14:18

banks requested changes Jul 18, 2018

View reviewed changes

mkeeler force-pushed the proxy-persist branch from 1ff43ba to af7725f Compare July 18, 2018 20:37

Persist proxies from config files

3fe5f56

Also change how loadProxies works. Now it will load all persisted proxies into a map, then when loading config file proxies will look up the previous proxy token in that map.

mkeeler force-pushed the proxy-persist branch from af7725f to 3fe5f56 Compare July 18, 2018 21:04

banks reviewed Jul 19, 2018

View reviewed changes

banks approved these changes Jul 19, 2018

View reviewed changes

banks requested changes Jul 19, 2018

View reviewed changes

Update a couple erroneous tests.

ca58513

banks approved these changes Jul 19, 2018

View reviewed changes

Use the agent logger instead of log module

560c9c2

pearkes added this to the 1.2.2 milestone Jul 19, 2018

mkeeler merged commit 9b44d08 into master Jul 20, 2018

banks deleted the proxy-persist branch July 20, 2018 13:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Persist proxies from config files #4407

Persist proxies from config files #4407

mkeeler commented Jul 17, 2018

banks commented Jul 18, 2018

banks left a comment

banks Jul 18, 2018

mkeeler Jul 18, 2018

banks Jul 18, 2018

banks Jul 18, 2018

banks Jul 18, 2018

banks Jul 18, 2018

banks Jul 18, 2018

mkeeler Jul 18, 2018

banks Jul 19, 2018

banks Jul 18, 2018

mkeeler Jul 18, 2018

banks Jul 19, 2018

banks left a comment

banks left a comment

mkeeler commented Jul 19, 2018

mkeeler commented Jul 19, 2018

mkeeler commented Jul 19, 2018

banks commented Jul 19, 2018 •

edited

Loading

Persist proxies from config files #4407

Persist proxies from config files #4407

Conversation

mkeeler commented Jul 17, 2018

banks commented Jul 18, 2018

banks left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

banks left a comment

Choose a reason for hiding this comment

banks left a comment

Choose a reason for hiding this comment

mkeeler commented Jul 19, 2018

mkeeler commented Jul 19, 2018

mkeeler commented Jul 19, 2018

banks commented Jul 19, 2018 • edited Loading

banks commented Jul 19, 2018 •

edited

Loading