NE-1476: Added network policies for the router #1263
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@knobunc: This pull request references NE-1476 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
knobunc
added
the
do-not-merge/work-in-progress
knobunc
force-pushed
the
NE-1476-Network-Policies-For-Router
branch
from
9a40cd0 to
b8fc7f7
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knobunc
force-pushed
the
NE-1476-Network-Policies-For-Router
branch
2 times, most recently
from
b392321 to
777addf
Compare
|
/retest-required |
knobunc
force-pushed
the
NE-1476-Network-Policies-For-Router
branch
5 times, most recently
from
71e5e0a to
abb8394
Compare
Added the framework for network policies for the router. The operator has a deny all network policy that for the openshift-ingress-operator namespace and an allow policy for egress to the apiserver and dns ports at any IP. The operator installs a deny all network policy for the openshift-ingress namespace. Then for each ingresscontroller that it manages it installs an allow policy for ingress for http and https traffic and metrics. It has to allow ingress from the router pods to any IP because the route endpoints can be at any ip or pod. It also needs access to the api server, but that is covered by the wildcard allow policy. https://issues.redhat.com/browse/NE-1476
knobunc
force-pushed
the
NE-1476-Network-Policies-For-Router
branch
from
abb8394 to
cf93ea5
Compare
|
@knobunc the e2e-gcp-operator CI tests are failing with:
|
|
@candita thanks. I've been focusing on the e2e-aws-ovn tests and those are consistently failing on: I am pretty sure the issue is that the test is creating a pod and it is unreachable without policy, but I haven't had time to chase it all the way down yet. |
|
@knobunc: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/assign |
| - ingressclasses | ||
| - networkpolicies | ||
| verbs: | ||
| - '*' |
There was a problem hiding this comment.
If possible, avoid adding wildcard rules. We have complaints about existing wildcards in RBAC rules, and we should avoid adding more.
| - Ingress | ||
| - Egress | ||
| --- | ||
| ### Allow the operator needs to talk to the apiserver and dns, |
There was a problem hiding this comment.
Do you mean the cluster DNS service (that is, CoreDNS) here?
| ### Allow the operator needs to talk to the apiserver and dns, | |
| ### The operator needs to talk to the apiserver and cluster DNS service, |
| ### Allow the operator needs to talk to the apiserver and dns, | ||
| ### but it also needs to be able to configure external DNS and the endpoints | ||
| ### are only known at run time. So it needs wildcard egress. | ||
| ### Allow access to the metrics ports on the operators |
There was a problem hiding this comment.
| ### Allow access to the metrics ports on the operators | |
| ### Allow access to the secure metrics port on the operator. |
| resources: | ||
| - networkpolicies | ||
| verbs: | ||
| - "*" |
There was a problem hiding this comment.
Avoid using a wildcard if possible.
| # Ingress to http on port 80 and https on port 443 (TCP) and to metrics (1936) | ||
| apiVersion: networking.k8s.io/v1 | ||
| kind: NetworkPolicy | ||
| # name, namespace,labels and annotations are set at runtime |
There was a problem hiding this comment.
It looks a bit odd not to set the namespace here when you set it in pkg/manifests/assets/gateway-api/gateway-networkpolicy-allow.yaml and pkg/manifests/assets/router/networkpolicy-deny-all.yaml.
Suppose we did support managed router deployments in other namespaces. Then you would need a deny-all policy created in the same namespace as the allow policy.
I would be all right with hard-coding the "openshift-ingress" namespace here and leaving it to either the cluster-admin or future us to handle networkpolicies for routers in other namespaces if that ever becomes something we support.
| - protocol: TCP | ||
| port: 1936 |
There was a problem hiding this comment.
It's unfortunate we need to expose the metrics port, but the same port is also the healthcheck endpoint, and so we must expose it to be used with the HostNetwork and NodePortService endpoint publishing strategy types.
| if _, _, err := r.ensureCanaryNamespaceNetworkPolicy(); err != nil { | ||
| return result, fmt.Errorf("failed to ensure canary namespace network policy: %v", err) | ||
| } | ||
|
|
||
| if _, _, err := r.ensureCanaryNetworkPolicy(); err != nil { | ||
| return result, fmt.Errorf("failed to ensure canary network policy: %v", err) | ||
| } |
There was a problem hiding this comment.
Is the ordering important? That is, is it important that we not create the allow policy if we fail to create the default-deny policy?
Actually, maybe it should be the other way around: Don't create the default-deny policy unless you can create the allow policy. Would that be safer?
In any case, please use %w for wrapped errors.
| if _, _, err := r.ensureCanaryNamespaceNetworkPolicy(); err != nil { | |
| return result, fmt.Errorf("failed to ensure canary namespace network policy: %v", err) | |
| } | |
| if _, _, err := r.ensureCanaryNetworkPolicy(); err != nil { | |
| return result, fmt.Errorf("failed to ensure canary network policy: %v", err) | |
| } | |
| if _, _, err := r.ensureCanaryNamespaceNetworkPolicy(); err != nil { | |
| return result, fmt.Errorf("failed to ensure canary namespace network policy: %w", err) | |
| } | |
| // Only create the allow policy if the deny-all policy was created successfully. | |
| if _, _, err := r.ensureCanaryNetworkPolicy(); err != nil { | |
| return result, fmt.Errorf("failed to ensure canary network policy: %w", err) | |
| } |
There was a problem hiding this comment.
+1 (I did a similar question on cluster-dns-operator without realizing Miciah did it here).
One thing to consider is that Ingress Network policies are usually a fail close behavior, so once creating a netpol that selects a pod works, it should automatically deny any traffic not covered by any other rule. It is better to create the specific rule before everything, and fast fail in case the creation fails
| if np.Name != "ingress-canary" || np.Namespace != "openshift-ingress-canary" { | ||
| t.Fatalf("unexpected name/namespace: %s/%s", np.Namespace, np.Name) | ||
| } |
There was a problem hiding this comment.
Consider using testify for these assertions.
| want := map[int32]struct{}{8888: {}, 8443: {}} | ||
| got := map[int32]struct{}{} | ||
| for _, r := range np.Spec.Ingress { | ||
| for _, p := range r.Ports { | ||
| if p.Protocol != nil && *p.Protocol == corev1.ProtocolTCP && p.Port != nil && p.Port.IntVal != 0 { | ||
| got[p.Port.IntVal] = struct{}{} |
There was a problem hiding this comment.
struct{} is passé. Consider using "k8s.io/apimachinery/pkg/util/sets".Set[int32].
| // Change in owner label should be detected but preserve other labels. | ||
| mutated = original.DeepCopy() | ||
| mutated.Labels["ingress.openshift.io/canary"] = "other" | ||
| if changed, updated := canaryNetworkPolicyChanged(original, mutated); !changed { | ||
| t.Fatalf("expected change detected for label diff") | ||
| } else if updated.Labels["ingress.openshift.io/canary"] != "other" { | ||
| t.Fatalf("expected owner label updated") | ||
| } |
There was a problem hiding this comment.
Despite the comment, the test logic doesn't actually check that other labels are preserved.
| // Ensure the allow NetworkPolicy for Gateway API components. | ||
| if _, _, err := r.ensureGatewayAPINetworkPolicy(ctx); err != nil { | ||
| return reconcile.Result{}, fmt.Errorf("failed to ensure gateway-api network policy: %w", err) | ||
| } |
There was a problem hiding this comment.
This logic to create the NetworkPolicy for gateway proxies seems like it belongs in the operator's gatewayclass controller with the logic that installs the Istio control plane.
There was a problem hiding this comment.
why? I think that the idea here is correct. You are watching for a Gateway and react creating its NetworkPolicy.
There was a problem hiding this comment.
The gatewayapi controller watches featuregates and customresourcedefinitions, and it reconciles the Gateway API CRDs.
The gatewayclass controller watches gatewayclasses, subscriptions, installplans, istios and customresourcedefinitions, and this controller also creates the Istio CR.
The Istio CR tells the OSSM Operator to install an Istiod control-plane, which is the thing that watches and reconciles gateways.
Come to think of it, it would make some sense to have Istiod create the networkpolicies. However, we are trying to fix the problem in cluster-ingress-operator, so it makes sense to have the controller that installs Istiod (which in turn creates the proxy pods) also create the networkpolicies for Istio's proxy pods.
The controllers are confusingly named, and we should refactor them at some point. Part of the history here is that we treat CRD life-cycle management on the one hand and gateway controller on the other hand as separate features. From that perspective, it makes sense to manage networkpolicies as part of the gateway-controller logic, not the CRD-life-cycle logic.
There was a problem hiding this comment.
ah ok, makes sense. That said, wouldn't make more sense to have a separate controller just for Network Policies based on Gateway, as we have for DNS as well?
There was a problem hiding this comment.
This PR is adding logic to create a generic NetworkPolicy that applies to all gateways that are created in the namespace, which are expected to belong to the Istio CR that the gatewayapi controller creates in the namespace. This NetworkPolicy doesn't depend on the definition of any particular Gateway. It therefore makes some sense for the same controller that creates the Istio CR to create the generic NetworkPolicy as well.
However, in the future, it would make sense to have a separate controller, which could create a NetworkPolicy for each Gateway, based off that Gateway's listeners. So I anticipate that this logic will be refactored in the future, but I think it seems reasonable for now.
| if np.Labels == nil { | ||
| np.Labels = map[string]string{} | ||
| } | ||
| np.Labels[operatorcontroller.ControllerDeploymentLabel] = operatorcontroller.OpenShiftDefaultGatewayClassName |
There was a problem hiding this comment.
This is potentially confusing as there is not necessarily any GatewayClass with the default GatewayClass name. See
cluster-ingress-operator/pkg/operator/controller/gatewayclass/istio.go
Lines 114 to 124 in cf93ea5
If this logic were in the gatewayclass controller, it could use the actual GatewayClass's name. Of course, it could still be confusing because you could create a GatewayClass with the openshift.io/gateway-controller/v1 controller name, and then create a second GatewayClass with the same controller name; presumably the NetworkPolicy would have only the first GatewayClass as the owner, which isn't too confusing. But then what if you delete the first GatewayClass? Do we need to update the label?
Note that we actually have a similar problem with the Istio CR.
| if cmp.Equal(current.Spec, expected.Spec, cmpopts.EquateEmpty()) && cmp.Equal(current.Labels, expected.Labels, cmpopts.EquateEmpty()) { | ||
| return false, nil | ||
| } | ||
| updated := current.DeepCopy() | ||
| updated.Spec = expected.Spec | ||
| // Merge/overwrite labels from expected; preserve any unrelated labels. | ||
| if updated.Labels == nil { | ||
| updated.Labels = map[string]string{} | ||
| } | ||
| for k, v := range expected.Labels { | ||
| updated.Labels[k] = v | ||
| } |
There was a problem hiding this comment.
This function is not fixed-point. That is, you can get in an update loop where current has some unexpected label that isn't in expected. Then the function merges updated into expected, but it keeps the unexpected label, and then the cmp.Equal(current.Labels, expected.Labels, ...) detects this as a change, and you get a loop.
| } | ||
|
|
||
| if _, _, err := r.ensureRouterNamespaceNetworkPolicy(ci); err != nil { | ||
| return fmt.Errorf("failed to ensure default namespacenetwork policy: %v", err) |
There was a problem hiding this comment.
| return fmt.Errorf("failed to ensure default namespacenetwork policy: %v", err) | |
| return fmt.Errorf("failed to ensure default namespace network policy: %w", err) |
| if !errors.IsNotFound(err) { | ||
| return false, nil, fmt.Errorf("failed to get router network policy %s/%s: %v", np.Namespace, np.Name, err) | ||
| } | ||
| if err := r.client.Create(context.TODO(), np); err != nil { | ||
| return false, nil, fmt.Errorf("failed to create router network policy %s/%s: %v", np.Namespace, np.Name, err) |
There was a problem hiding this comment.
This is the "default namespace network policy", not the "router network policy". Anyway, ensureIngressController wraps these errors, so the wrapping here seems redundant.
|
|
||
| // ensureRouterNetworkPolicy ensures the per-ingresscontroller NetworkPolicy that | ||
| // allows ingress to router pods and egress to the network exists and is up to date. | ||
| func (r *reconciler) ensureRouterNetworkPolicy(ic *operatorv1.IngressController) error { |
There was a problem hiding this comment.
Any reason this ensure function doesn't return the object or Boolean value? It doesn't strictly need to do so, so I don't object if you prefer to keep it simple, but it deviates from the established pattern for ensure methods.
| // Change in owner label should be detected but preserve other labels. | ||
| mutated = original.DeepCopy() | ||
| mutated.Labels["ingresscontroller.operator.openshift.io/owning-ingresscontroller"] = "other" | ||
| if changed, updated := routerNetworkPolicyChanged(original, mutated); !changed { | ||
| t.Fatalf("expected change detected for label diff") | ||
| } else if updated.Labels["ingresscontroller.operator.openshift.io/owning-ingresscontroller"] != "other" { | ||
| t.Fatalf("expected owner label updated") | ||
| } |
There was a problem hiding this comment.
This isn't checking that other labels are preserved.
| return true, nil | ||
| } | ||
|
|
||
| func gatewayAPINetworkPolicyChanged(current, expected *networkingv1.NetworkPolicy) (bool, *networkingv1.NetworkPolicy) { |
There was a problem hiding this comment.
this function name doesn't reflect much on the function behavior. I understand that it checks if 2 policies have changed, but why then it returns a new policy?
Is this the whole case as of "if it is updated, return a version with current + changes so once applying it won't conflict"?
I am wondering if long term we want to start using client.Apply with patch helper more, or state that we will always use Update on these managed objects and users cannot change it (including not adding arbitrary labels).
There was a problem hiding this comment.
this function name doesn't reflect much on the function behavior. I understand that it checks if 2 policies have changed, but why then it returns a new policy?
Right, current is the actual NetworkPolicy from the API server, desired is the NetworkPolicy that we want to persist, and updated is the result of merging the two.
The operator owns the object, so we use Update. That said, we do sometimes carve out exceptions (for example, for custom labels or annotations) and deliberately preserve those things.
We have a bunch of updateFoo and fooChanged functions that follow this pattern; Ben is just following the established pattern.
There was a problem hiding this comment.
cool, thanks! I was a bit confused at first.
| return fmt.Errorf("failed to ensure cluster role binding: %v", err) | ||
| } | ||
|
|
||
| if err := r.ensureRouterNetworkPolicy(ci); err != nil { |
There was a problem hiding this comment.
same comment on network policy made above may apply here, we probably want this policy before the default deny all
6 participants
Added the framework for network policies for the router.
The operator has a deny all network policy that for the openshift-ingress-operator namespace and an allow policy for egress to the apiserver and dns ports at any IP.
The operator installs a deny all network policy for the openshift-ingress namespace.
Then for each ingresscontroller that it manages it installs an allow policy for ingress for http and https traffic and metrics.
It has to allow ingress from the router pods to any IP because the route endpoints can be at any ip or pod.
It also needs access to the api server, but that is covered by the wildcard allow policy.
https://issues.redhat.com/browse/NE-1476