Enable connectivity to the host network for egress firewall matching pods #2506
Conversation
|
/lgtm |
|
Actually hold this, it seems I included a bunch of changes to |
alexanderConstantinescu
force-pushed
the
egress-firewall-host-network
branch
from
b5c77bd
to
edfd0ac
Compare
|
We can remove the hold, I've rebased now. |
|
@alexanderConstantinescu should we wait on this one til we land the libovsdb bits for routers? |
No, because we'll need to backport this to earlier versions. But you reminded me that this PR is not good enough, I will need to add a second commit which will not be backported and which will contain the libovsdb implementation - as I did on: #2580 |
alexanderConstantinescu
force-pushed
the
egress-firewall-host-network
branch
from
edfd0ac
to
be1340c
Compare
|
@dcbw : this patch is now updated and ready. It holds one "legacy" commit using |
alexanderConstantinescu
force-pushed
the
egress-firewall-host-network
branch
2 times, most recently
from
d42b742
to
2821020
Compare
go-controller/pkg/types/const.go
Outdated
| @@ -60,6 +60,7 @@ const ( | |||
| DefaultDenyPriority = 1000 | |||
|
|
|||
| // priority of logical router policies on the OVNClusterRouter | |||
| DefaultEgressFirewallPriority = "10001" | |||
There was a problem hiding this comment.
I'm not sure this name is suitable. Shouldn't it be like MaxEgressFirewallPriority? Also the EgressFirewallStartPriority should have a comment above it maybe describing that egress firewall priority's are decremented (I forget exactly how this works).
There was a problem hiding this comment.
OK, I will add some comments.
alexanderConstantinescu
force-pushed
the
egress-firewall-host-network
branch
from
2821020
to
debd794
Compare
alexanderConstantinescu
force-pushed
the
egress-firewall-host-network
branch
from
debd794
to
f47d5fa
Compare
|
@alexanderConstantinescu can you rebase/revive this PR please? |
…atching pods This is a legacy commit using ovn-nbctl for the implementation. This is done as to be able to have the commit backported Egress firewall matching pods didn't currently have any connectivity to the host network when a "deny 0.0.0.0" rule is created. That type of a rule blocked access to all cluster nodes. This was not in-line with the expectation of how the feature was working for people coming from openshift-sdn. They thus needed to explicitly allow all node IPs if they required pods to access the Kube API service, for example. That is not optimal though as if nodes leave / join the cluster, the rules need to be updated by the admin. We now have documentation specifically mentioning that we don't support this, so I am not sure we want this: openshift/openshift-docs#35311 but this patch was pretty straightforward to code, so we can discuss it on the PR. Note: anyone using ovn-kubernetes before this patch (and setting up specific allow rules for the nodes) should not be impacted negatively by this patch...but they will have a duplicate ACL for every node they've explicitly allowed Signed-off-by: Alexander Constantinescu <aconstan@redhat.com>
…pods Egress firewall matching pods didn't currently have any connectivity to the host network when a "deny 0.0.0.0" rule is created. That type of a rule blocked access to all cluster nodes. This was not in-line with the expectation of how the feature was working for people coming from openshift-sdn. They thus needed to explicitly allow all node IPs if they required pods to access the Kube API service, for example. That is not optimal though as if nodes leave / join the cluster, the rules need to be updated by the admin. We now have documentation specifically mentioning that we don't support this, so I am not sure we want this: openshift/openshift-docs#35311 but this patch was pretty straightforward to code, so we can discuss it on the PR. Note: anyone using ovn-kubernetes before this patch (and setting up specific allow rules for the nodes) should not be impacted negatively by this patch...but they will have a duplicate ACL for every node they've explicitly allowed Signed-off-by: Alexander Constantinescu <aconstan@redhat.com>
alexanderConstantinescu
force-pushed
the
egress-firewall-host-network
branch
from
f47d5fa
to
58aaf64
Compare
| @@ -80,6 +82,66 @@ func newEgressFirewallRule(rawEgressFirewallRule egressfirewallapi.EgressFirewal | |||
| return efr, nil | |||
| } | |||
|
|
|||
| func (oc *Controller) addNodeForEgressFirewall(node *v1.Node) error { | |||
| v4Addr, v6Addr := getNodeInternalAddrs(node) | |||
There was a problem hiding this comment.
// getNodeInternalAddrs returns the first IPv4 and/or IPv6 InternalIP defined
// for the node. On certain cloud providers (AWS) the egress IP will be added to
// the list of node IPs as an InternalIP address, we don't want to create the
// default allow logical router policies for that IP. Node IPs are ordered,
// meaning the egress IP will never be first in this list.
func getNodeInternalAddrs(node *v1.Node) (net.IP, net.IP) {
I wonder if we should get all ip addresses on the node, using the the k8s.ovn.org/host-addresses annotation? That would allow access to any IP that lives on a node.
|
Closing in favor of #2786 |
Egress firewall matching pods didn't currently have any connectivity to
the host network when a "deny 0.0.0.0" rule is created. That type of a
rule blocked access to all cluster nodes. This was not in-line with the
expectation of how the feature was working for people coming from
openshift-sdn. They thus needed to explicitly allow all node IPs if they
required pods to access the Kube API service, for example. That is not
optimal though as if nodes leave / join the cluster, the rules need to
be updated by the admin. We now have documentation specifically
mentioning that we don't support this, so I am not sure we want this:
openshift/openshift-docs#35311 but this patch
was pretty straightforward to code, so we can discuss it on the PR.
Note: anyone using ovn-kubernetes before this patch (and setting up
specific allow rules for the nodes) should not be impacted negatively by
this patch...but they will have a duplicate ACL for every node they've
explicitly allowed
/assign @JacobTanenbaum @trozet
- What this PR does and why is it needed
- Special notes for reviewers
- How to verify it
- Description for the changelog