Remove all port proxies upon shutdown #4503
Conversation
Signed-off-by: Nino Kodabande <nkodabande@suse.com>
Signed-off-by: Nino Kodabande <nkodabande@suse.com>
Signed-off-by: Nino Kodabande <nkodabande@suse.com>
Nino-K
force-pushed
the
clean-up-port-mappings
branch
from
3ec03ed
to
ac831b5
Compare
Signed-off-by: Nino Kodabande <nkodabande@suse.com>
src/go/privileged-service/go.mod
Outdated
| github.com/docker/go-connections v0.4.0 // indirect | ||
| github.com/docker/go-connections v0.4.0 |
There was a problem hiding this comment.
It would be good to clean up this file. (Merge all of the require() blocks into one, then run go mod tidy.)
| } | ||
|
|
||
| func (p *proxy) exec(portMapping types.PortMapping) error { | ||
| portProxy := portProxy{ |
There was a problem hiding this comment.
Can we call this port or newPort instead (so that we don't have portProxy referring to either the instance or the type depending on context)?
| p.mutex.Lock() | ||
| defer p.mutex.Unlock() | ||
| for _, proxy := range p.portMappings { | ||
| _ = p.delete(proxy) |
There was a problem hiding this comment.
Should we at least log the error?
| @@ -67,6 +100,7 @@ func deleteProxy(portMapping types.PortMapping) error { | |||
| } | |||
| } | |||
| } | |||
| delete(p.portMappings, getHash(portProxy)) | |||
There was a problem hiding this comment.
Hmm, shouldn't this be done under the mutex?
There was a problem hiding this comment.
Yes, thanks for catching it, I totally missed it.
| @@ -150,3 +184,8 @@ func portProxyAddArgs(listenPort, listenAddr, connectAddr string) ([]string, err | |||
| fmt.Sprintf("connectaddress=%s", connectAddr), | |||
| }, nil | |||
| } | |||
|
|
|||
| func getHash(portProxy portProxy) string { | |||
| data := fmt.Sprintf("%v", portProxy) | |||
There was a problem hiding this comment.
Is %v going to be consistent in the right way? (Would this include any pointers? Because if it does, then the hash would not match when we want it to.)
There was a problem hiding this comment.
The %v is consistent since the protProxy and its sub-structs do not contain any pointers, however, this is not really future-proof, but maybe that's ok since we are retiring the privileged service soon.
An alternative approach is to use gob or JSON, I will investigate an alternative.
| p.mutex.Lock() | ||
| p.portMappings[getHash(portProxy)] = portProxy | ||
| p.mutex.Unlock() |
There was a problem hiding this comment.
Is there a risk of a race here? Assume the same port definition below.
addgets called, the port is added, but blocks on acquiring the mutex.deletegets called, runs through completion. (The port is removed fromnetsh.)addcontinues, and inserts intop.portMappingseven though it's no longer innetsh.
There was a problem hiding this comment.
Yes, this could potentially happen but remember the purpose of portMappings map is to remove all the port mappings upon shutdown and it ignores any potential errors from netsh interface portproxy delete.... So, if there is an attempt to delete an entry that doesn't have a corresponding netsh it's ok because the error is ignored. The most important thing is to make sure the netsh is up to date and that's why that operation takes precedence.
Signed-off-by: Nino Kodabande <nkodabande@suse.com>
| } | ||
|
|
||
| func addProxy(portMapping types.PortMapping) error { | ||
| for _, v := range portMapping.Ports { | ||
| func (p *proxy) add(portProxy portProxy) error { |
There was a problem hiding this comment.
Consider calling the variable something like so we don't repeat the type name.
- Also, changes the byte creation for struct to json marshal - Manually sorts all the sub-slices to prevent hash mismatch Signed-off-by: Nino Kodabande <nkodabande@suse.com>
Nino-K
force-pushed
the
clean-up-port-mappings
branch
from
d9ef463
to
ea82dfd
Compare
| actual portProxy | ||
| expect portProxy | ||
| shouldMatch bool | ||
| expectedErr error |
There was a problem hiding this comment.
This can't be anything non-nil, because we expect both hashes to lead to the same error. (This means the coverage is not great.)
There was a problem hiding this comment.
Yes, there is a separate test case that needs to be added for when json.Marshal returns an error, which requires making json.Marshal fail. But, when I thought about it in detail, I realized we would just be testing the go's built-in packages which are already tested by the library. Also, when json.Marshal fails getHash always returns an empty string, so it wouldn't have been a great test, to begin with.
There was a problem hiding this comment.
Right, the comment was more "why do we have this field" than anything :)
Currently, privileged services leave behind all the port proxies that it creates, this change allows to clean up during shutdown. Privileged service manages this by keeping track of all the added/deleted port proxies in a map using the hash of the port mapping struct as a map key, this is so that we can avoid the recursive lookups.
Fixes: #3567