Skip to content

Commit e178588

Browse files
authored
SRVCOM-5404: create allow NP early for Serving with istio ingress (#4227)
* SRVCOM-5404: create allow NP early for Serving with istio ingress * use existing minio helper image
1 parent 4407c5c commit e178588

4 files changed

Lines changed: 105 additions & 11 deletions

File tree

‎hack/lib/tracing.bash‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -175,7 +175,7 @@ spec:
175175
value: tempo
176176
- name: MINIO_SECRET_KEY
177177
value: supersecret
178-
image: quay.io/minio/minio
178+
image: ghcr.io/grafana/tempo-operator/test-utils:sha-0fc5ef0
179179
name: minio
180180
ports:
181181
- containerPort: 9000

‎openshift-knative-operator/pkg/serving/extension.go‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -194,6 +194,16 @@ func (e *extension) Reconcile(ctx context.Context, comp base.KComponent) error {
194194
common.ConfigureIfUnset(&ks.Spec.CommonSpec, monitoring.ObservabilityCMName, monitoring.ObservabilityBackendKey, "none")
195195
}
196196

197+
// Create the policies before the install stage applies resources that trigger
198+
// the webhooks: otherwise the Service Mesh deny-all policy blocks the apiserver
199+
// from reaching the webhook and the install deadlocks. Missing policies only;
200+
// the manifest reconciliation owns their ongoing lifecycle.
201+
for _, np := range istioWebhookNetworkPolicies(ks) {
202+
if _, err := e.kubeclient.NetworkingV1().NetworkPolicies(np.Namespace).Create(ctx, np, metav1.CreateOptions{}); err != nil && !apierrors.IsAlreadyExists(err) {
203+
return fmt.Errorf("failed to create NetworkPolicy %s/%s: %w", np.Namespace, np.Name, err)
204+
}
205+
}
206+
197207
if !oldResourceRemoved.Load() {
198208
if err := e.cleanupOldResources(ctx, ks.GetNamespace()); err != nil {
199209
return err

‎openshift-knative-operator/pkg/serving/ingress.go‎

Lines changed: 26 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -63,21 +63,24 @@ func defaultIngressClass(ks *operatorv1beta1.KnativeServing) string {
6363
return kourierIngressClassName
6464
}
6565

66-
func generateDefaultIstioNetworkPoliciesIfRequired(ks base.KComponent) ([]mf.Manifest, error) {
67-
if !ks.(*operatorv1beta1.KnativeServing).Spec.Ingress.Istio.Enabled {
68-
return nil, nil
66+
// istioWebhookNetworkPolicies builds the NetworkPolicies that keep the Knative
67+
// webhook and metrics endpoints reachable in a Service Mesh member namespace,
68+
// where the deny-all policy would otherwise block ingress (including apiserver
69+
// admission calls). Returns nil when not required (Istio ingress disabled or
70+
// opted out via annotation).
71+
func istioWebhookNetworkPolicies(ks *operatorv1beta1.KnativeServing) []*networkingv1.NetworkPolicy {
72+
if !ks.Spec.Ingress.Istio.Enabled {
73+
return nil
6974
}
70-
7175
if v, ok := ks.GetAnnotations()[disableGeneratingIstioNetPoliciesAnnotation]; ok {
7276
if disableGeneration, _ := strconv.ParseBool(v); disableGeneration {
73-
return nil, nil
77+
return nil
7478
}
7579
}
7680

77-
unObjs := []unstructured.Unstructured{{}, {}, {}}
78-
79-
for i, name := range []string{"webhook", "net-istio-webhook", "allow-from-openshift-monitoring-ns"} {
80-
nwp := networkingv1.NetworkPolicy{
81+
policies := make([]*networkingv1.NetworkPolicy, 0, 3)
82+
for _, name := range []string{"webhook", "net-istio-webhook", "allow-from-openshift-monitoring-ns"} {
83+
nwp := &networkingv1.NetworkPolicy{
8184
ObjectMeta: metav1.ObjectMeta{
8285
Name: name,
8386
Namespace: ks.GetNamespace(),
@@ -108,7 +111,20 @@ func generateDefaultIstioNetworkPoliciesIfRequired(ks base.KComponent) ([]mf.Man
108111
}}
109112
}
110113

111-
if err := scheme.Scheme.Convert(&nwp, &unObjs[i], nil); err != nil {
114+
policies = append(policies, nwp)
115+
}
116+
return policies
117+
}
118+
119+
func generateDefaultIstioNetworkPoliciesIfRequired(ks base.KComponent) ([]mf.Manifest, error) {
120+
policies := istioWebhookNetworkPolicies(ks.(*operatorv1beta1.KnativeServing))
121+
if len(policies) == 0 {
122+
return nil, nil
123+
}
124+
125+
unObjs := make([]unstructured.Unstructured, len(policies))
126+
for i, p := range policies {
127+
if err := scheme.Scheme.Convert(p, &unObjs[i], nil); err != nil {
112128
return nil, err
113129
}
114130
}

‎openshift-knative-operator/pkg/serving/ingress_test.go‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,17 @@
11
package serving
22

33
import (
4+
"context"
45
"testing"
56

67
"github.com/google/go-cmp/cmp"
8+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
79
"knative.dev/operator/pkg/apis/operator/base"
810
operatorv1beta1 "knative.dev/operator/pkg/apis/operator/v1beta1"
11+
12+
configfake "github.com/openshift-knative/serverless-operator/pkg/client/config/injection/client/fake"
13+
routefake "github.com/openshift-knative/serverless-operator/pkg/client/route/injection/client/fake"
14+
kubefake "knative.dev/pkg/client/injection/kube/client/fake"
915
)
1016

1117
func TestDefaultIngressClass(t *testing.T) {
@@ -63,3 +69,65 @@ func TestDefaultIngressClass(t *testing.T) {
6369
})
6470
}
6571
}
72+
73+
// TestReconcileEnsuresWebhookNetworkPolicy verifies that, with Istio ingress
74+
// enabled, Reconcile eagerly creates the "webhook" NetworkPolicy so the apiserver
75+
// can reach the validation webhook before the install stage triggers it -
76+
// without it, the Service Mesh deny-all policy deadlocks the install.
77+
func TestReconcileEnsuresWebhookNetworkPolicy(t *testing.T) {
78+
ks := &operatorv1beta1.KnativeServing{
79+
ObjectMeta: metav1.ObjectMeta{Namespace: servingNamespace.Name},
80+
Spec: operatorv1beta1.KnativeServingSpec{
81+
Ingress: &operatorv1beta1.IngressConfigs{
82+
Istio: base.IstioIngressConfiguration{Enabled: true},
83+
},
84+
},
85+
}
86+
87+
ctx, _ := routefake.With(context.Background())
88+
ctx, _ = configfake.With(ctx, defaultIngress)
89+
ctx, kube := kubefake.With(ctx, &servingNamespace)
90+
ext := newFakeExtension(ctx, t)
91+
92+
if err := ext.Reconcile(ctx, ks); err != nil {
93+
t.Fatalf("Reconcile() returned error: %v", err)
94+
}
95+
96+
np, err := kube.NetworkingV1().NetworkPolicies(servingNamespace.Name).Get(context.Background(), "webhook", metav1.GetOptions{})
97+
if err != nil {
98+
t.Fatalf("expected 'webhook' NetworkPolicy to exist after Reconcile: %v", err)
99+
}
100+
if np.Spec.PodSelector.MatchLabels["app"] != "webhook" {
101+
t.Errorf("expected podSelector app=webhook, got: %+v", np.Spec.PodSelector)
102+
}
103+
// Allow-all ingress is a single rule with no "from" peers.
104+
if len(np.Spec.Ingress) != 1 || len(np.Spec.Ingress[0].From) != 0 {
105+
t.Errorf("expected a single allow-all ingress rule, got: %+v", np.Spec.Ingress)
106+
}
107+
}
108+
109+
// TestReconcileSkipsWebhookNetworkPolicyWithoutIstio verifies that with Istio
110+
// ingress disabled (e.g. Kourier), Reconcile creates no istio NetworkPolicies.
111+
func TestReconcileSkipsWebhookNetworkPolicyWithoutIstio(t *testing.T) {
112+
ks := &operatorv1beta1.KnativeServing{
113+
ObjectMeta: metav1.ObjectMeta{Namespace: servingNamespace.Name},
114+
Spec: operatorv1beta1.KnativeServingSpec{
115+
Ingress: &operatorv1beta1.IngressConfigs{
116+
Kourier: base.KourierIngressConfiguration{Enabled: true},
117+
},
118+
},
119+
}
120+
121+
ctx, _ := routefake.With(context.Background())
122+
ctx, _ = configfake.With(ctx, defaultIngress)
123+
ctx, kube := kubefake.With(ctx, &servingNamespace)
124+
ext := newFakeExtension(ctx, t)
125+
126+
if err := ext.Reconcile(ctx, ks); err != nil {
127+
t.Fatalf("Reconcile() returned error: %v", err)
128+
}
129+
130+
if _, err := kube.NetworkingV1().NetworkPolicies(servingNamespace.Name).Get(context.Background(), "webhook", metav1.GetOptions{}); err == nil {
131+
t.Errorf("did not expect 'webhook' NetworkPolicy to be created when Istio ingress is disabled")
132+
}
133+
}

0 commit comments

Comments
 (0)