Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optionally revert iptables changes on exit #126

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Optionally revert iptables changes on exit #126

wants to merge 3 commits into from

Conversation

dangorst1066
Copy link
Contributor

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 !

@dangorst1066 dangorst1066 changed the title Revert iptables optionally on exit, add timeout to health check call Optionally revert iptables changes on exit Jan 31, 2018
@coveralls
Copy link

coveralls commented Jan 31, 2018

Coverage Status

Coverage increased (+0.9%) to 19.355% when pulling 2349511 on HotelsDotCom:master into d0518d9 on jtblin:master.

@jay-rob
Copy link

jay-rob commented Mar 5, 2018

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)

Copy link
Owner

@jtblin jtblin left a comment

Choose a reason for hiding this comment

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

Sorry for the lag, need some minor changes.

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"
Copy link
Owner

Choose a reason for hiding this comment

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

Can you move the ones below with the rest of the standard library i.e. "strings"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@@ -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.
Copy link
Owner

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Owner

Choose a reason for hiding this comment

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

SIGABRT is not passed to signal.Notify below, is that going to work properly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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...

@noqcks
Copy link

noqcks commented Feb 12, 2019

Any updates on this @jtblin ?

I have built this and tested on my own Kubernetes cluster and it works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants