address comments

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
envoyproxy · May 23, 2024 · 471a4d3 · 471a4d3
1 parent ba7ddbe
commit 471a4d3
Show file tree

Hide file tree

Showing 8 changed files with 48 additions and 20 deletions.
diff --git a/internal/gatewayapi/securitypolicy.go b/internal/gatewayapi/securitypolicy.go
@@ -373,7 +373,7 @@ func (t *Translator) translateSecurityPolicyForRoute(
 	}
 
 	if policy.Spec.Authorization != nil {
-		if authorization, err = t.buildAuthorization(policy.Spec.Authorization); err != nil {
+		if authorization, err = t.buildAuthorization(policy); err != nil {
 			errs = errors.Join(errs, err)
 		}
 	}
@@ -453,7 +453,7 @@ func (t *Translator) translateSecurityPolicyForGateway(
 	}
 
 	if policy.Spec.Authorization != nil {
-		if authorization, err = t.buildAuthorization(policy.Spec.Authorization); err != nil {
+		if authorization, err = t.buildAuthorization(policy); err != nil {
 			errs = errors.Join(errs, err)
 		}
 	}
@@ -862,9 +862,10 @@ func irConfigName(policy *egv1a1.SecurityPolicy) string {
 		utils.NamespacedName(policy).String())
 }
 
-func (t *Translator) buildAuthorization(authorization *egv1a1.Authorization) (*ir.Authorization, error) {
+func (t *Translator) buildAuthorization(policy *egv1a1.SecurityPolicy) (*ir.Authorization, error) {
 	var (
-		irAuth = &ir.Authorization{}
+		authorization = policy.Spec.Authorization
+		irAuth        = &ir.Authorization{}
 		// The default action is Deny if not specified
 		defaultAction = egv1a1.AuthorizationActionDeny
 	)
@@ -874,7 +875,7 @@ func (t *Translator) buildAuthorization(authorization *egv1a1.Authorization) (*i
 	}
 	irAuth.DefaultAction = defaultAction
 
-	for _, rule := range authorization.Rules {
+	for i, rule := range authorization.Rules {
 		principal := ir.Principal{}
 
 		for _, cidr := range rule.Principal.ClientCIDRs {
@@ -885,11 +886,26 @@ func (t *Translator) buildAuthorization(authorization *egv1a1.Authorization) (*i
 
 			principal.ClientCIDRs = append(principal.ClientCIDRs, cidrMatch)
 		}
+
+		var name string
+		if rule.Name != nil && *rule.Name != "" {
+			name = *rule.Name
+		} else {
+			name = defaultAuthorizationRuleName(policy, i)
+		}
 		irAuth.Rules = append(irAuth.Rules, &ir.AuthorizationRule{
+			Name:      name,
 			Action:    rule.Action,
 			Principal: principal,
 		})
 	}
 
 	return irAuth, nil
 }
+
+func defaultAuthorizationRuleName(policy *egv1a1.SecurityPolicy, index int) string {
+	return fmt.Sprintf(
+		"%s/authorization/rule/%s",
+		irConfigName(policy),
+		strconv.Itoa(index))
+}
diff --git a/internal/gatewayapi/testdata/securitypolicy-with-authoriztion.out.yaml b/internal/gatewayapi/testdata/securitypolicy-with-authoriztion.out.yaml
@@ -254,7 +254,6 @@ securityPolicies:
       defaultAction: Deny
       rules:
       - action: Allow
-        name: null
         principal:
           clientCIDRs:
           - 10.0.1.0/24
@@ -339,6 +338,7 @@ xdsIR:
             defaultAction: Allow
             rules:
             - action: Deny
+              name: deny-location-1
               principal:
                 clientCIDRs:
                 - cidr: 192.168.1.0/24
@@ -352,6 +352,7 @@ xdsIR:
                   isIPv6: false
                   maskLen: 24
             - action: Deny
+              name: deny-location-2
               principal:
                 clientCIDRs:
                 - cidr: 10.75.1.0/24
@@ -385,6 +386,7 @@ xdsIR:
             defaultAction: Deny
             rules:
             - action: Allow
+              name: securitypolicy/envoy-gateway/policy-for-gateway/authorization/rule/0
               principal:
                 clientCIDRs:
                 - cidr: 10.0.1.0/24

diff --git a/internal/ir/xds.go b/internal/ir/xds.go
@@ -775,6 +775,10 @@ type Authorization struct {
 //
 // +k8s:deepcopy-gen=true
 type AuthorizationRule struct {
+	// Name is a user-defined name for the rule.
+	// If not specified, a name will be generated by EG.
+	Name string `json:"name"`
+
 	// Action defines the action to be taken if the rule matches.
 	Action egv1a1.AuthorizationAction `json:"action"`
 

diff --git a/internal/xds/translator/authorization.go b/internal/xds/translator/authorization.go
@@ -154,7 +154,7 @@ func (*rbac) patchRoute(route *routev3.Route, irRoute *ir.HTTPRoute) error {
 	for _, rule := range authorization.Rules {
 		// Build the IPMatcher based on the client CIDRs.
 		ipRangeMatcher := &ipmatcherv3.Ip{
-			StatPrefix: "source_ip",
+			StatPrefix: "client_ip",
 		}
 
 		for _, cidr := range rule.Principal.ClientCIDRs {
@@ -186,7 +186,7 @@ func (*rbac) patchRoute(route *routev3.Route, irRoute *ir.HTTPRoute) error {
 				MatchType: &matcherv3.Matcher_MatcherList_Predicate_SinglePredicate_{
 					SinglePredicate: &matcherv3.Matcher_MatcherList_Predicate_SinglePredicate{
 						Input: &cncfv3.TypedExtensionConfig{
-							Name:        "source_ip",
+							Name:        "client_ip",
 							TypedConfig: sourceIPInput,
 						},
 						Matcher: &matcherv3.Matcher_MatcherList_Predicate_SinglePredicate_CustomMatch{
@@ -201,7 +201,7 @@ func (*rbac) patchRoute(route *routev3.Route, irRoute *ir.HTTPRoute) error {
 			OnMatch: &matcherv3.Matcher_OnMatch{
 				OnMatch: &matcherv3.Matcher_OnMatch_Action{
 					Action: &cncfv3.TypedExtensionConfig{
-						Name:        "action",
+						Name:        rule.Name,
 						TypedConfig: ruleAction,
 					},
 				},
@@ -223,6 +223,7 @@ func (*rbac) patchRoute(route *routev3.Route, irRoute *ir.HTTPRoute) error {
 						Matchers: matcherList,
 					},
 				},
+				// If no matcher matches, the default action will be used.
 				OnNoMatch: &matcherv3.Matcher_OnMatch{
 					OnMatch: &matcherv3.Matcher_OnMatch_Action{
 						Action: &cncfv3.TypedExtensionConfig{
@@ -242,6 +243,7 @@ func (*rbac) patchRoute(route *routev3.Route, irRoute *ir.HTTPRoute) error {
 		routeCfgProto.Rbac.Matcher.MatcherType = nil
 	}
 
+	// We need to validate the RBACPerRoute message before converting it to an Any.
 	if err = routeCfgProto.ValidateAll(); err != nil {
 		return err
 	}

diff --git a/internal/xds/translator/testdata/in/xds-ir/authorization.yaml b/internal/xds/translator/testdata/in/xds-ir/authorization.yaml
@@ -49,6 +49,7 @@ http:
         defaultAction: Allow
         rules:
         - action: Deny
+          name: deny-location-1
           principal:
             clientCIDRs:
             - cidr: 192.168.1.0/24
@@ -62,6 +63,7 @@ http:
               isIPv6: false
               maskLen: 24
         - action: Deny
+          name: deny-location-2
           principal:
             clientCIDRs:
             - cidr: 10.75.1.0/24
@@ -95,6 +97,7 @@ http:
         defaultAction: Deny
         rules:
         - action: Allow
+          name: securitypolicy/envoy-gateway/policy-for-gateway/authorization/rule/0
           principal:
             clientCIDRs:
             - cidr: 10.0.1.0/24

diff --git a/internal/xds/translator/testdata/out/xds-ir/authorization.routes.yaml b/internal/xds/translator/testdata/out/xds-ir/authorization.routes.yaml
@@ -40,7 +40,7 @@
                 matchers:
                 - onMatch:
                     action:
-                      name: action
+                      name: deny-location-1
                       typedConfig:
                         '@type': type.googleapis.com/envoy.config.rbac.v3.Action
                         action: DENY
@@ -56,14 +56,14 @@
                             prefixLen: 24
                           - addressPrefix: 192.168.2.0
                             prefixLen: 24
-                          statPrefix: source_ip
+                          statPrefix: client_ip
                       input:
-                        name: source_ip
+                        name: client_ip
                         typedConfig:
                           '@type': type.googleapis.com/envoy.extensions.matching.common_inputs.network.v3.SourceIPInput
                 - onMatch:
                     action:
-                      name: action
+                      name: deny-location-2
                       typedConfig:
                         '@type': type.googleapis.com/envoy.config.rbac.v3.Action
                         action: DENY
@@ -79,9 +79,9 @@
                             prefixLen: 24
                           - addressPrefix: 10.75.2.0
                             prefixLen: 24
-                          statPrefix: source_ip
+                          statPrefix: client_ip
                       input:
-                        name: source_ip
+                        name: client_ip
                         typedConfig:
                           '@type': type.googleapis.com/envoy.extensions.matching.common_inputs.network.v3.SourceIPInput
               onNoMatch:
@@ -106,7 +106,7 @@
                 matchers:
                 - onMatch:
                     action:
-                      name: action
+                      name: securitypolicy/envoy-gateway/policy-for-gateway/authorization/rule/0
                       typedConfig:
                         '@type': type.googleapis.com/envoy.config.rbac.v3.Action
                         name: ALLOW
@@ -121,9 +121,9 @@
                             prefixLen: 24
                           - addressPrefix: 10.0.2.0
                             prefixLen: 24
-                          statPrefix: source_ip
+                          statPrefix: client_ip
                       input:
-                        name: source_ip
+                        name: client_ip
                         typedConfig:
                           '@type': type.googleapis.com/envoy.extensions.matching.common_inputs.network.v3.SourceIPInput
               onNoMatch:

diff --git a/test/e2e/testdata/authorization-client-ip.yaml b/test/e2e/testdata/authorization-client-ip.yaml
@@ -81,8 +81,8 @@ spec:
         - 10.0.1.0/24
         - 10.0.2.0/24
 ---
-# This is a client traffic policy that enables client IP detection using a custom header.
-# So, the client IP can be detected from the custom header `client-ip` and used for authorization.
+# This is a client traffic policy that enables client IP detection using the XFF header.
+# So, the client IP can be detected from the XFF header and used for authorization.
 apiVersion: gateway.envoyproxy.io/v1alpha1
 kind: ClientTrafficPolicy
 metadata:

diff --git a/test/e2e/testdata/basic-auth.yaml b/test/e2e/testdata/basic-auth.yaml
@@ -59,6 +59,7 @@ spec:
     group: gateway.networking.k8s.io
     kind: HTTPRoute
     name: http-with-basic-auth-1
+    namespace: gateway-conformance-infra
   basicAuth:
     users:
       name: "basic-auth-users-secret-1"