openshift / openshift-sdn

Apache License 2.0
69 stars 63 forks source link

Use firewalld D-Bus API to configure firewall rules #139

Closed danwinship closed 9 years ago

danwinship commented 9 years ago

Closes #32

(Note: I didn't bother changing the lbr code.)

danwinship commented 9 years ago

@rajatchopra @pravisankar PTAL. The travis-ci failure seems spurious. (It's claiming the "glog" package doesn't exist?)

marun commented 9 years ago

Is there a reason every single change is being duplicated across the kube and multitenant plugins? Why not factor out the common stuff to simplify ongoing maintenance?

danwinship commented 9 years ago

I copied it to kube just because it was trivial to do so in this case (basically just diff | patch). I assume that in general we're not extending the kube plugin with any new features. Maybe I should just drop that part of this patch.

rajatchopra commented 9 years ago

@danwinship This is dependent on the upstream PR, isn't it? Firewalld will be disabled in kubernetes without that, right?

danwinship commented 9 years ago

@rajatchopra hm... well, I guess until the upstream PR (https://github.com/kubernetes/kubernetes/pull/12396) is committed, kubernetes will require that firewalld is disabled, and so the firewalld-specific code in this PR would never get run. But right now openshift-sdn always adds iptables rules to work around firewalld even when firewalld isn't running (aka always), so this PR is an improvement over that in a way.

danwinship commented 9 years ago

updated to address @marun's "data-driven" comment, and to remove firewalld support from the kube code; if we merge kube+multitenant more later we can make the firewalld support generic then

danwinship commented 9 years ago

@rajatchopra poke

rajatchopra commented 9 years ago

Do we need to enhance the rpm spec? Any new dependencies that this will require? That list of files in Godeps is scary.

danwinship commented 9 years ago

godbus is a purely-golang implementation of D-Bus, so no, there are no new dependencies

rajatchopra commented 9 years ago

LGTM