Optionally revert iptables changes on exit #126
Conversation
dangorst1066
changed the title
|
Can we get this merged? It does cause issues, especially when you perform a port change(which can happen since its using host network/port) |
cmd/main.go
Outdated
| @@ -10,6 +10,9 @@ import ( | |||
| "github.com/jtblin/kube2iam/iptables" | |||
| "github.com/jtblin/kube2iam/server" | |||
| "github.com/jtblin/kube2iam/version" | |||
| "os" | |||
There was a problem hiding this comment.
Can you move the ones below with the rest of the standard library i.e. "strings"?
| @@ -145,14 +146,19 @@ func (s *Server) getRoleMapping(IP string) (*mappings.RoleMappingResult, error) | |||
| return roleMapping, nil | |||
| } | |||
|
|
|||
| // HealthResponse represents a response for the health check. | |||
| type HealthResponse struct { | |||
| // healthResponse represents a response for the health check. | |||
There was a problem hiding this comment.
These changes seem to be unrelated to the iptable change on exit, it would have been better to keep that separated. Is the goal to add a 5s timeout on the metadata check?
There was a problem hiding this comment.
Yeah exactly - encountered some funky behaviour as a result of the lack of a timeout so bundled the change up, but happy to move if needed though!
cmd/main.go
Outdated
| go func() { | ||
| if err := s.Run(s.APIServer, s.APIToken, s.Insecure); err != nil { | ||
| log.Errorf("%s", err) | ||
| signalChan <- syscall.SIGABRT // On error, just quit now by faking a signal |
There was a problem hiding this comment.
SIGABRT is not passed to signal.Notify below, is that going to work properly?
There was a problem hiding this comment.
SIGABRT just fakes an o/s signal so we force an exit in the event the goroutine was unable to start the server. (i.e. so <-signalChan a little further down won't block) Kube can then get on with its job and restart this...
There are certainly better ways of handling this, but would be a somewhat more invasive PR - happy to revisit if needed...
|
Any updates on this @jtblin ? I have built this and tested on my own Kubernetes cluster and it works. |
Referencing #44
Hey - just raising this as a possible simple answer to the issue around kube2iam not cleaning up after itself and removing iptables rules on exit. Judging from the debate perhaps this is behaviour the users should configure depending on their security requirements?
I think this is something that should be addressed one way or another (even if not through this pr!) as there are real world scenarios where you might need to abort/roll back a deploy of kube2iam without leaving your cluster unable to access ec2 metadata, or perhaps reconfigure it to use another interface or port etc without wanting to manually remove iptables rules.
ps please ignore my other closed pr for this same issue, that was opened prematurely !