policy: Cache redirect status on PerSelectorPolicy

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
cilium · Dec 13, 2022 · 27861ae · 27861ae
1 parent 38589e7
commit 27861ae
Show file tree

Hide file tree

Showing 8 changed files with 79 additions and 32 deletions.
diff --git a/pkg/envoy/server.go b/pkg/envoy/server.go
@@ -1428,11 +1428,11 @@ func getWildcardNetworkPolicyRule(selectors policy.L7DataMap) *cilium.PortNetwor
 			remoteMap[uint64(id)] = struct{}{}
 		}
 
-		if !l7.IsEmpty() {
-			// If it is not empty then we issue the warning.
+		if l7.IsRedirect() {
+			// Issue a warning if this port-0 rule is a redirect.
 			// Deny rules don't support L7 therefore for the deny case
-			// l7.IsEmpty() will always return true.
-			log.Warningf("L3-only rule for selector %v surprisingly has L7 rules (%v)!", sel, *l7)
+			// l7.IsRedirect() will always return false.
+			log.Warningf("L3-only rule for selector %v surprisingly requires proxy redirection (%v)!", sel, *l7)
 		}
 	}
 

diff --git a/pkg/policy/l4.go b/pkg/policy/l4.go
@@ -183,6 +183,9 @@ type PerSelectorPolicy struct {
 	// TLS handshake.
 	ServerNames StringSet `json:"serverNames,omitempty"`
 
+	// isRedirect is 'true' when traffic must be redirected
+	isRedirect bool `json:"-"`
+
 	// Pre-computed HTTP rules, computed after rule merging is complete
 	EnvoyHTTPRules *cilium.HttpNetworkPolicyRules `json:"-"`
 
@@ -202,22 +205,14 @@ func (a *PerSelectorPolicy) Equal(b *PerSelectorPolicy) bool {
 		a.TerminatingTLS.Equal(b.TerminatingTLS) &&
 		a.OriginatingTLS.Equal(b.OriginatingTLS) &&
 		a.ServerNames.Equal(b.ServerNames) &&
+		a.isRedirect == b.isRedirect &&
 		a.IsDeny == b.IsDeny &&
 		a.L7Rules.DeepEqual(&b.L7Rules)
 }
 
 // IsRedirect returns true if the L7Rules are a redirect.
 func (a *PerSelectorPolicy) IsRedirect() bool {
-	// policy is for an proxy redirect if it:
-	// - is not empty, i.e., has L7 rules or TLS config, and
-	// - it is not a deny policy, as deny policies do not support L7 rules
-	return !a.IsEmpty() && !a.IsDeny
-}
-
-// IsEmpty returns whether the `L7Rules` is nil or has no L7 rules and no TLS config.
-func (a *PerSelectorPolicy) IsEmpty() bool {
-	return a == nil ||
-		(a.L7Rules.IsEmpty() && a.TerminatingTLS == nil && a.OriginatingTLS == nil && len(a.ServerNames) > 0)
+	return a != nil && a.isRedirect
 }
 
 // HasL7Rules returns whether the `L7Rules` contains any L7 rules.
@@ -594,12 +589,13 @@ func (l4 *L4Filter) cacheFQDNSelector(sel api.FQDNSelector, selectorCache *Selec
 
 // add L7 rules for all endpoints in the L7DataMap
 
-func (l7 L7DataMap) addRulesForEndpoints(rules *api.L7Rules, terminatingTLS, originatingTLS *TLSContext, deny bool, sni []string) {
+func (l7 L7DataMap) addRulesForEndpoints(rules *api.L7Rules, terminatingTLS, originatingTLS *TLSContext, deny bool, sni []string, forceRedirect bool) {
 	l7policy := &PerSelectorPolicy{
 		TerminatingTLS: terminatingTLS,
 		OriginatingTLS: originatingTLS,
 		IsDeny:         deny,
 		ServerNames:    NewStringSet(sni),
+		isRedirect:     !deny && (forceRedirect || terminatingTLS != nil || originatingTLS != nil || len(sni) > 0 || !rules.IsEmpty()),
 	}
 	if rules != nil {
 		l7policy.L7Rules = *rules
@@ -724,7 +720,7 @@ func createL4Filter(policyCtx PolicyContext, peerEndpoints api.EndpointSelectorS
 		}
 
 		if l4.L7Parser != ParserTypeNone {
-			l4.L7RulesPerSelector.addRulesForEndpoints(pr.Rules, terminatingTLS, originatingTLS, policyCtx.IsDeny(), pr.ServerNames)
+			l4.L7RulesPerSelector.addRulesForEndpoints(pr.Rules, terminatingTLS, originatingTLS, policyCtx.IsDeny(), pr.ServerNames, false)
 		}
 	}
 
@@ -782,15 +778,11 @@ func createL4IngressFilter(policyCtx PolicyContext, fromEndpoints api.EndpointSe
 		return nil, err
 	}
 
-	pr := rule.GetPortRule()
-	if pr == nil {
-		return filter, nil
-	}
-	// If the filter would apply L7 rules for the Host, when we should accept everything from host,
-	// then wildcard Host at L7.
-	if !pr.Rules.IsEmpty() && len(hostWildcardL7) > 0 {
-		for cs := range filter.L7RulesPerSelector {
-			if cs.Selects(identity.ReservedIdentityHost) {
+	// If the filter would apply proxy redirection for the Host, when we should accept
+	// everything from host, then wildcard Host at L7.
+	if len(hostWildcardL7) > 0 {
+		for cs, l7 := range filter.L7RulesPerSelector {
+			if l7.IsRedirect() && cs.Selects(identity.ReservedIdentityHost) {
 				for _, name := range hostWildcardL7 {
 					selector := api.ReservedEndpointSelectors[name]
 					filter.cacheIdentitySelector(selector, policyCtx.GetSelectorCache(), policyCtx.IsDeny())

diff --git a/pkg/policy/l4_filter_deny_test.go b/pkg/policy/l4_filter_deny_test.go
@@ -561,6 +561,7 @@ func (ds *PolicyTestSuite) TestL3L4AllowRuleWithByL3DenyAll(c *C) {
 				L7Rules: api.L7Rules{
 					HTTP: []api.PortRuleHTTP{{Path: "/", Method: "GET"}},
 				},
+				isRedirect: true,
 			},
 		},
 		Ingress:          true,
@@ -638,6 +639,7 @@ func (ds *PolicyTestSuite) TestL3L4AllowRuleWithByL3DenyAll(c *C) {
 				L7Rules: api.L7Rules{
 					HTTP: []api.PortRuleHTTP{{Path: "/", Method: "GET"}},
 				},
+				isRedirect: true,
 			},
 		},
 		Ingress:          true,

diff --git a/pkg/policy/l4_filter_test.go b/pkg/policy/l4_filter_test.go
@@ -286,6 +286,7 @@ func (ds *PolicyTestSuite) TestMergeAllowAllL3AndShadowedL7(c *C) {
 				L7Rules: api.L7Rules{
 					HTTP: []api.PortRuleHTTP{{Path: "/", Method: "GET"}, {}},
 				},
+				isRedirect: true,
 			},
 		},
 		Ingress:          true,
@@ -403,6 +404,7 @@ func (ds *PolicyTestSuite) TestMergeIdenticalAllowAllL3AndRestrictedL7HTTP(c *C)
 				L7Rules: api.L7Rules{
 					HTTP: []api.PortRuleHTTP{{Path: "/", Method: "GET"}},
 				},
+				isRedirect: true,
 			},
 		},
 		Ingress:          true,
@@ -488,6 +490,7 @@ func (ds *PolicyTestSuite) TestMergeIdenticalAllowAllL3AndRestrictedL7Kafka(c *C
 				L7Rules: api.L7Rules{
 					Kafka: []kafka.PortRule{{Topic: "foo"}},
 				},
+				isRedirect: true,
 			},
 		},
 		Ingress:          true,
@@ -790,6 +793,7 @@ func (ds *PolicyTestSuite) TestMergeTLSTCPPolicy(c *C) {
 				EnvoyHTTPRules:  nil,
 				CanShortCircuit: false,
 				L7Rules:         api.L7Rules{},
+				isRedirect:      true,
 			},
 		},
 		Ingress:          false,
@@ -881,6 +885,7 @@ func (ds *PolicyTestSuite) TestMergeTLSHTTPPolicy(c *C) {
 				L7Rules: api.L7Rules{
 					HTTP: []api.PortRuleHTTP{{}},
 				},
+				isRedirect: true,
 			},
 		},
 		Ingress:          false,
@@ -989,6 +994,7 @@ func (ds *PolicyTestSuite) TestMergeTLSSNIPolicy(c *C) {
 				L7Rules: api.L7Rules{
 					HTTP: []api.PortRuleHTTP{{}},
 				},
+				isRedirect: true,
 			},
 		},
 		Ingress:          false,
@@ -1193,6 +1199,7 @@ func (ds *PolicyTestSuite) TestL3RuleWithL7RulePartiallyShadowedByL3AllowAll(c *
 				L7Rules: api.L7Rules{
 					HTTP: []api.PortRuleHTTP{{Path: "/", Method: "GET"}},
 				},
+				isRedirect: true,
 			},
 		},
 		Ingress:          true,
@@ -1268,6 +1275,7 @@ func (ds *PolicyTestSuite) TestL3RuleWithL7RulePartiallyShadowedByL3AllowAll(c *
 				L7Rules: api.L7Rules{
 					HTTP: []api.PortRuleHTTP{{Path: "/", Method: "GET"}},
 				},
+				isRedirect: true,
 			},
 		},
 		Ingress:          true,
@@ -1355,11 +1363,13 @@ func (ds *PolicyTestSuite) TestL3RuleWithL7RuleShadowedByL3AllowAll(c *C) {
 				L7Rules: api.L7Rules{
 					HTTP: []api.PortRuleHTTP{{Path: "/", Method: "GET"}},
 				},
+				isRedirect: true,
 			},
 			cachedSelectorA: &PerSelectorPolicy{
 				L7Rules: api.L7Rules{
 					HTTP: []api.PortRuleHTTP{{Path: "/", Method: "GET"}},
 				},
+				isRedirect: true,
 			},
 		},
 		Ingress:          true,
@@ -1439,11 +1449,13 @@ func (ds *PolicyTestSuite) TestL3RuleWithL7RuleShadowedByL3AllowAll(c *C) {
 				L7Rules: api.L7Rules{
 					HTTP: []api.PortRuleHTTP{{Path: "/", Method: "GET"}},
 				},
+				isRedirect: true,
 			},
 			cachedSelectorA: &PerSelectorPolicy{
 				L7Rules: api.L7Rules{
 					HTTP: []api.PortRuleHTTP{{Path: "/", Method: "GET"}},
 				},
+				isRedirect: true,
 			},
 		},
 		Ingress:          true,
@@ -1642,11 +1654,13 @@ func (ds *PolicyTestSuite) TestMergingWithDifferentEndpointsSelectedAllowSameL7(
 				L7Rules: api.L7Rules{
 					HTTP: []api.PortRuleHTTP{{Path: "/", Method: "GET"}},
 				},
+				isRedirect: true,
 			},
 			cachedSelectorA: &PerSelectorPolicy{
 				L7Rules: api.L7Rules{
 					HTTP: []api.PortRuleHTTP{{Path: "/", Method: "GET"}},
 				},
+				isRedirect: true,
 			},
 		},
 		Ingress:          true,
@@ -1798,6 +1812,7 @@ func (ds *PolicyTestSuite) TestAllowingLocalhostShadowsL7(c *C) {
 				L7Rules: api.L7Rules{
 					HTTP: []api.PortRuleHTTP{{Path: "/", Method: "GET"}},
 				},
+				isRedirect: true,
 			},
 			cachedSelectorHost: nil, // no proxy redirect
 		},

diff --git a/pkg/policy/repository_test.go b/pkg/policy/repository_test.go
@@ -757,6 +757,7 @@ func (ds *PolicyTestSuite) TestWildcardL3RulesIngress(c *C) {
 					L7Rules: api.L7Rules{
 						Kafka: []kafka.PortRule{kafkaRule.Ingress[0].ToPorts[0].Rules.Kafka[0]},
 					},
+					isRedirect: true,
 				},
 			},
 			DerivedFromRules: labels.LabelArrayList{labelsKafka},
@@ -772,6 +773,7 @@ func (ds *PolicyTestSuite) TestWildcardL3RulesIngress(c *C) {
 					L7Rules: api.L7Rules{
 						HTTP: []api.PortRuleHTTP{httpRule.Ingress[0].ToPorts[0].Rules.HTTP[0]},
 					},
+					isRedirect: true,
 				},
 			},
 			DerivedFromRules: labels.LabelArrayList{labelsHTTP},
@@ -788,6 +790,7 @@ func (ds *PolicyTestSuite) TestWildcardL3RulesIngress(c *C) {
 						L7Proto: "tester",
 						L7:      []api.PortRuleL7{l7Rule.Ingress[0].ToPorts[0].Rules.L7[0]},
 					},
+					isRedirect: true,
 				},
 			},
 			DerivedFromRules: labels.LabelArrayList{labelsL7},
@@ -922,6 +925,7 @@ func (ds *PolicyTestSuite) TestWildcardL4RulesIngress(c *C) {
 					L7Rules: api.L7Rules{
 						HTTP: []api.PortRuleHTTP{httpRule.Ingress[0].ToPorts[0].Rules.HTTP[0]},
 					},
+					isRedirect: true,
 				},
 			},
 			DerivedFromRules: labels.LabelArrayList{labelsL4HTTP, labelsL7HTTP},
@@ -938,6 +942,7 @@ func (ds *PolicyTestSuite) TestWildcardL4RulesIngress(c *C) {
 					L7Rules: api.L7Rules{
 						Kafka: []kafka.PortRule{kafkaRule.Ingress[0].ToPorts[0].Rules.Kafka[0]},
 					},
+					isRedirect: true,
 				},
 			},
 			DerivedFromRules: labels.LabelArrayList{labelsL4Kafka, labelsL7Kafka},
@@ -1250,6 +1255,7 @@ func (ds *PolicyTestSuite) TestWildcardL3RulesEgress(c *C) {
 					L7Rules: api.L7Rules{
 						DNS: []api.PortRuleDNS{dnsRule.Egress[0].ToPorts[0].Rules.DNS[0]},
 					},
+					isRedirect: true,
 				},
 			},
 			DerivedFromRules: labels.LabelArrayList{labelsDNS},
@@ -1265,6 +1271,7 @@ func (ds *PolicyTestSuite) TestWildcardL3RulesEgress(c *C) {
 					L7Rules: api.L7Rules{
 						HTTP: []api.PortRuleHTTP{httpRule.Egress[0].ToPorts[0].Rules.HTTP[0]},
 					},
+					isRedirect: true,
 				},
 			},
 			DerivedFromRules: labels.LabelArrayList{labelsHTTP},
@@ -1436,6 +1443,7 @@ func (ds *PolicyTestSuite) TestWildcardL4RulesEgress(c *C) {
 					L7Rules: api.L7Rules{
 						HTTP: []api.PortRuleHTTP{httpRule.Egress[0].ToPorts[0].Rules.HTTP[0]},
 					},
+					isRedirect: true,
 				},
 			},
 			DerivedFromRules: labels.LabelArrayList{labelsL3HTTP, labelsL7HTTP},
@@ -1452,6 +1460,7 @@ func (ds *PolicyTestSuite) TestWildcardL4RulesEgress(c *C) {
 					L7Rules: api.L7Rules{
 						DNS: []api.PortRuleDNS{dnsRule.Egress[0].ToPorts[0].Rules.DNS[0]},
 					},
+					isRedirect: true,
 				},
 			},
 			DerivedFromRules: labels.LabelArrayList{labelsL3DNS, labelsL7DNS},
@@ -1557,6 +1566,7 @@ func (ds *PolicyTestSuite) TestWildcardCIDRRulesEgress(c *C) {
 							Path:    "/",
 						}},
 					},
+					isRedirect: true,
 				},
 			},
 			DerivedFromRules: labels.LabelArrayList{labelsHTTP},
@@ -1693,6 +1703,7 @@ func (ds *PolicyTestSuite) TestWildcardL3RulesIngressFromEntities(c *C) {
 					L7Rules: api.L7Rules{
 						Kafka: []kafka.PortRule{kafkaRule.Ingress[0].ToPorts[0].Rules.Kafka[0]},
 					},
+					isRedirect: true,
 				},
 			},
 			DerivedFromRules: labels.LabelArrayList{labelsKafka},
@@ -1708,6 +1719,7 @@ func (ds *PolicyTestSuite) TestWildcardL3RulesIngressFromEntities(c *C) {
 					L7Rules: api.L7Rules{
 						HTTP: []api.PortRuleHTTP{httpRule.Ingress[0].ToPorts[0].Rules.HTTP[0]},
 					},
+					isRedirect: true,
 				},
 			},
 			DerivedFromRules: labels.LabelArrayList{labelsHTTP},
@@ -1831,6 +1843,7 @@ func (ds *PolicyTestSuite) TestWildcardL3RulesEgressToEntities(c *C) {
 					L7Rules: api.L7Rules{
 						DNS: []api.PortRuleDNS{dnsRule.Egress[0].ToPorts[0].Rules.DNS[0]},
 					},
+					isRedirect: true,
 				},
 			},
 			DerivedFromRules: labels.LabelArrayList{labelsDNS},
@@ -1846,6 +1859,7 @@ func (ds *PolicyTestSuite) TestWildcardL3RulesEgressToEntities(c *C) {
 					L7Rules: api.L7Rules{
 						HTTP: []api.PortRuleHTTP{httpRule.Egress[0].ToPorts[0].Rules.HTTP[0]},
 					},
+					isRedirect: true,
 				},
 			},
 			DerivedFromRules: labels.LabelArrayList{labelsHTTP},
@@ -1968,6 +1982,7 @@ func (ds *PolicyTestSuite) TestMinikubeGettingStarted(c *C) {
 				L7Rules: api.L7Rules{
 					HTTP: []api.PortRuleHTTP{{Method: "GET", Path: "/"}, {}},
 				},
+				isRedirect: true,
 			},
 		},
 		Ingress:          true,

diff --git a/pkg/policy/resolve_test.go b/pkg/policy/resolve_test.go
@@ -311,6 +311,7 @@ func (ds *PolicyTestSuite) TestL7WithIngressWildcard(c *C) {
 									HTTP: []api.PortRuleHTTP{{Method: "GET", Path: "/good"}},
 								},
 								CanShortCircuit: true,
+								isRedirect:      true,
 							},
 						},
 						DerivedFromRules: labels.LabelArrayList{nil},
@@ -406,6 +407,7 @@ func (ds *PolicyTestSuite) TestL7WithLocalHostWildcardd(c *C) {
 									HTTP: []api.PortRuleHTTP{{Method: "GET", Path: "/good"}},
 								},
 								CanShortCircuit: true,
+								isRedirect:      true,
 							},
 							cachedSelectorHost: nil,
 						},

diff --git a/pkg/policy/rule.go b/pkg/policy/rule.go
@@ -148,6 +148,9 @@ func mergePortProto(ctx *SearchContext, existingFilter, filterToMerge *L4Filter,
 				newL7Rules = &PerSelectorPolicy{}
 			}
 
+			// Merge isRedirect flag
+			l7Rules.isRedirect = l7Rules.isRedirect || newL7Rules.isRedirect
+
 			if l7Rules.TerminatingTLS == nil || newL7Rules.TerminatingTLS == nil {
 				if newL7Rules.TerminatingTLS != nil {
 					l7Rules.TerminatingTLS = newL7Rules.TerminatingTLS