Repository: envoyproxy/gateway
Pull request #2338 (2023-12-21 13:26:43)

feat(helm): support resources configuration for certgen job
**What this PR does / why we need it**:
Add support for configuring resources requests and limits for `certgen` job.

**Which issue(s) this PR fixes**: N/A


codecov[bot]: ## [Codecov](https://app.codecov.io/gh/envoyproxy/gateway/pull/2338?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=envoyproxy) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Comparison is base [(`d7d411c`)](https://app.codecov.io/gh/envoyproxy/gateway/commit/d7d411c37ca07a289bee9ac9c80e2ffa748ce1aa?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=envoyproxy) 64.55% compared to head [(`df99496`)](https://app.codecov.io/gh/envoyproxy/gateway/pull/2338?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=envoyproxy) 64.59%.


<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main    #2338      +/-   ##
==========================================
+ Coverage   64.55%   64.59%   +0.03%     
==========================================
  Files         112      112              
  Lines       16072    16072              
==========================================
+ Hits        10376    10381       +5     
+ Misses       5043     5039       -4     
+ Partials      653      652       -1     
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/envoyproxy/gateway/pull/2338?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=envoyproxy).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=envoyproxy).

zhaohuabing: Is the default enough for certgen?
shahar-h: > The default isn't enough for certgen?

Which default?
shahar-h: `e2e-test` failed with:
```log
Error: Unable to download artifact(s): Unable to download and extract artifact: Request timeout: /actions-results/e317eb1c-05a8-4e46-b13a-4417a5c7835b/workflow-job-run-ca395085-040a-526b-2ce8-bdc85f692774/artifacts/envoy-gateway.zip?se=2023-12-22T16%3A56%3A31Z&sig=NyfXAH1AY5MjtpA3udLo7flsDsdt4hwmFPn5M%2B8i0B8%3D&sp=r&spr=https&sr=b&st=2023-12-22T16%3A46%3A31Z&sv=2021-12-02
```
How can I retrigger it?
shahar-h: > > The default isn't enough for certgen?
> 
> Which default?

@zhaohuabing can you please reply?
zhaohuabing: > > > The default isn't enough for certgen?
> > 
> > 
> > Which default?
> 
> @zhaohuabing can you please reply?

Sorry for the late response. I mean isn't the default resource enough for certgen?
zhaohuabing: > > > > > The default isn't enough for certgen?
> > > > 
> > > > 
> > > > Which default?
> > > 
> > > 
> > > @zhaohuabing can you please reply?
> > 
> > 
> > Sorry for the late response. I mean isn't the default resource enough for certgen?
> 
> In case you meant reusing [deployment.envoyGateway.resources](https://github.com/envoyproxy/gateway/blob/main/charts/gateway-helm/values.tmpl.yaml#L7-L13) also for certgrn job, I guess it's enough but since certgen probably requires much more lower requests than EG I think it's a good idea to add its own resources configuration.

Oops, My bad. I just noticed that Kubernetes doesn’t provide default resource limits out-of-the-box. I think we should define the limit for certgen in EG. But since it's a quick an lightweight task, we might not even expose it through the API.
shahar-h: @zhaohuabing you can provide default values for certgen requests and limits, so they can still be overridden, same as for EG pod.
zhaohuabing: > @zhaohuabing you can provide default values for certgen requests and limits, so they can still be overridden, same as for EG pod.

I think users probably won't need to override the requests and limits for certgen. Unlike EG or Envoy, whose resource consumption depends on the scale of the application,  certgen's resources are quite steady and predictable. 
shahar-h: @zhaohuabing maybe we can start with allowing users to configure `certgen` job resources via chart values and at later point provide internal defaults? Anyway it's optional so I don't see how it will cause any issue.
It's important to have resources for all workloads since there are policy engines that validate that, i.e. [Kyverno require-requests-limits policy](https://kyverno.io/policies/best-practices/require-pod-requests-limits/require-pod-requests-limits/).

zhaohuabing: > @zhaohuabing maybe we can start with allowing users to configure `certgen` job resources via chart values and at later point provide internal defaults? Anyway it's optional so I don't see how it will cause any issue.
> It's important to have resources for all workloads since there are policy engines that validate that, i.e. [Kyverno require-requests-limits policy](https://kyverno.io/policies/best-practices/require-pod-requests-limits/require-pod-requests-limits/).
> 

I have no objection with settings a reasonable default, just don't want to expose it to the API because certgen won't scale up or down. However, I may be wrong, wait other @envoyproxy/gateway-maintainers to chime in.
zirain: IMO, [advanced helm chart customization](https://istio.io/latest/docs/setup/additional-setup/customize-installation-helm/#advanced-helm-chart-customization) will help this.
shahar-h: @zhaohuabing I think that resources should be configurable from chart values because some users would prefer for example to omit CPU limits in order to avoid CPU throttling. On the other hand some users would prefer to define CPU limits in order to make sure other workloads get more CPU time. See https://home.robusta.dev/blog/stop-using-cpu-limits.

Patch:
diff --git a/api/v1alpha1/jwt_types.go b/api/v1alpha1/jwt_types.go
index 6700d28b8..43e222743 100644
--- a/api/v1alpha1/jwt_types.go
+++ b/api/v1alpha1/jwt_types.go
@@ -88,10 +88,36 @@ type ClaimToHeader struct {
 }
 
 // JWTExtractor defines a custom JWT token extraction from HTTP request.
+// If specified, Envoy will extract the JWT token from the listed extractors (headers, cookies, or params) and validate each of them.
+// If any value extracted is found to be an invalid JWT, a 401 error will be returned.
 type JWTExtractor struct {
+	// Headers represents a list of HTTP request headers to extract the JWT token from.
+	//
+	// +optional
+	Headers []JWTHeaderExtractor `json:"headers,omitempty"`
+
 	// Cookies represents a list of cookie names to extract the JWT token from.
-	// If specified, Envoy will extract the JWT token from the listed cookies and validate each of them.
-	// If any cookie is found to be an invalid JWT, a 401 error will be returned.
 	//
+	// +optional
 	Cookies []string `json:"cookies,omitempty"`
+
+	// Params represents a list of query parameters to extract the JWT token from.
+	//
+	// +optional
+	Params []string `json:"params,omitempty"`
+}
+
+// JWTHeaderExtractor defines an HTTP header location to extract JWT token
+type JWTHeaderExtractor struct {
+	// Name is the HTTP header name to retrieve the token
+	//
+	// +kubebuilder:validation:Required
+	Name string `json:"name"`
+
+	// ValuePrefix is the prefix that should be stripped before extracting the token.
+	// The format would be used by Envoy like "{ValuePrefix}<TOKEN>".
+	// For example, "Authorization: Bearer <TOKEN>", then the ValuePrefix="Bearer " with a space at the end.
+	//
+	// +optional
+	ValuePrefix *string `json:"valuePrefix,omitempty"`
 }
diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go
index e01c43df3..a86564545 100644
--- a/api/v1alpha1/zz_generated.deepcopy.go
+++ b/api/v1alpha1/zz_generated.deepcopy.go
@@ -1578,11 +1578,23 @@ func (in *JWT) DeepCopy() *JWT {
 // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
 func (in *JWTExtractor) DeepCopyInto(out *JWTExtractor) {
 	*out = *in
+	if in.Headers != nil {
+		in, out := &in.Headers, &out.Headers
+		*out = make([]JWTHeaderExtractor, len(*in))
+		for i := range *in {
+			(*in)[i].DeepCopyInto(&(*out)[i])
+		}
+	}
 	if in.Cookies != nil {
 		in, out := &in.Cookies, &out.Cookies
 		*out = make([]string, len(*in))
 		copy(*out, *in)
 	}
+	if in.Params != nil {
+		in, out := &in.Params, &out.Params
+		*out = make([]string, len(*in))
+		copy(*out, *in)
+	}
 }
 
 // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new JWTExtractor.
@@ -1595,6 +1607,26 @@ func (in *JWTExtractor) DeepCopy() *JWTExtractor {
 	return out
 }
 
+// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
+func (in *JWTHeaderExtractor) DeepCopyInto(out *JWTHeaderExtractor) {
+	*out = *in
+	if in.ValuePrefix != nil {
+		in, out := &in.ValuePrefix, &out.ValuePrefix
+		*out = new(string)
+		**out = **in
+	}
+}
+
+// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new JWTHeaderExtractor.
+func (in *JWTHeaderExtractor) DeepCopy() *JWTHeaderExtractor {
+	if in == nil {
+		return nil
+	}
+	out := new(JWTHeaderExtractor)
+	in.DeepCopyInto(out)
+	return out
+}
+
 // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
 func (in *JWTProvider) DeepCopyInto(out *JWTProvider) {
 	*out = *in
diff --git a/charts/gateway-helm/crds/generated/gateway.envoyproxy.io_securitypolicies.yaml b/charts/gateway-helm/crds/generated/gateway.envoyproxy.io_securitypolicies.yaml
index ae3a0489d..32a52cdb3 100644
--- a/charts/gateway-helm/crds/generated/gateway.envoyproxy.io_securitypolicies.yaml
+++ b/charts/gateway-helm/crds/generated/gateway.envoyproxy.io_securitypolicies.yaml
@@ -202,10 +202,36 @@ spec:
                           properties:
                             cookies:
                               description: Cookies represents a list of cookie names
-                                to extract the JWT token from. If specified, Envoy
-                                will extract the JWT token from the listed cookies
-                                and validate each of them. If any cookie is found
-                                to be an invalid JWT, a 401 error will be returned.
+                                to extract the JWT token from.
+                              items:
+                                type: string
+                              type: array
+                            headers:
+                              description: Headers represents a list of HTTP request
+                                headers to extract the JWT token from.
+                              items:
+                                description: JWTHeaderExtractor defines an HTTP header
+                                  location to extract JWT token
+                                properties:
+                                  name:
+                                    description: Name is the HTTP header name to retrieve
+                                      the token
+                                    type: string
+                                  valuePrefix:
+                                    description: 'ValuePrefix is the prefix that should
+                                      be stripped before extracting the token. The
+                                      format would be used by Envoy like "{ValuePrefix}<TOKEN>".
+                                      For example, "Authorization: Bearer <TOKEN>",
+                                      then the ValuePrefix="Bearer " with a space
+                                      at the end.'
+                                    type: string
+                                required:
+                                - name
+                                type: object
+                              type: array
+                            params:
+                              description: Params represents a list of query parameters
+                                to extract the JWT token from.
                               items:
                                 type: string
                               type: array
diff --git a/charts/gateway-helm/templates/certgen.yaml b/charts/gateway-helm/templates/certgen.yaml
index 3a228b795..bea4792f2 100644
--- a/charts/gateway-helm/templates/certgen.yaml
+++ b/charts/gateway-helm/templates/certgen.yaml
@@ -34,6 +34,10 @@ spec:
         image: {{ .Values.deployment.envoyGateway.image.repository }}:{{ .Values.deployment.envoyGateway.image.tag | default .Chart.AppVersion }}
         imagePullPolicy: {{ .Values.deployment.envoyGateway.imagePullPolicy }}
         name: envoy-gateway-certgen
+        {{- with .Values.certgen.job.resources }}
+        resources:
+          {{- toYaml . | nindent 10 }}
+        {{- end }}
       {{- with .Values.deployment.envoyGateway.imagePullSecrets }}
       imagePullSecrets:
         {{- toYaml . | nindent 8 }}
diff --git a/charts/gateway-helm/templates/proxy-rbac.yaml b/charts/gateway-helm/templates/proxy-rbac.yaml
deleted file mode 100644
index 0ed136a7c..000000000
--- a/charts/gateway-helm/templates/proxy-rbac.yaml
+++ /dev/null
@@ -1,34 +0,0 @@
-apiVersion: rbac.authorization.k8s.io/v1
-kind: ClusterRole
-metadata:
-  name: {{ include "eg.fullname" . }}-proxy-role
-  labels:
-  {{- include "eg.labels" . | nindent 4 }}
-rules:
-- apiGroups:
-  - authentication.k8s.io
-  resources:
-  - tokenreviews
-  verbs:
-  - create
-- apiGroups:
-  - authorization.k8s.io
-  resources:
-  - subjectaccessreviews
-  verbs:
-  - create
----
-apiVersion: rbac.authorization.k8s.io/v1
-kind: ClusterRoleBinding
-metadata:
-  name: {{ include "eg.fullname" . }}-proxy-rolebinding
-  labels:
-  {{- include "eg.labels" . | nindent 4 }}
-roleRef:
-  apiGroup: rbac.authorization.k8s.io
-  kind: ClusterRole
-  name: '{{ include "eg.fullname" . }}-proxy-role'
-subjects:
-- kind: ServiceAccount
-  name: 'envoy-gateway'
-  namespace: '{{ .Release.Namespace }}'
diff --git a/charts/gateway-helm/values.tmpl.yaml b/charts/gateway-helm/values.tmpl.yaml
index ea6e7125a..fb9c1ae4e 100644
--- a/charts/gateway-helm/values.tmpl.yaml
+++ b/charts/gateway-helm/values.tmpl.yaml
@@ -49,6 +49,7 @@ kubernetesClusterDomain: cluster.local
 certgen:
   job:
     annotations: {}
+    resources: {}
     ttlSecondsAfterFinished: 0
   rbac:
     annotations: {}
diff --git a/internal/xds/translator/jwt.go b/internal/xds/translator/jwt.go
index abb4f0b13..c590bcf04 100644
--- a/internal/xds/translator/jwt.go
+++ b/internal/xds/translator/jwt.go
@@ -20,6 +20,7 @@ import (
 	"google.golang.org/protobuf/types/known/durationpb"
 	"k8s.io/utils/ptr"
 
+	"github.com/envoyproxy/gateway/api/v1alpha1"
 	"github.com/envoyproxy/gateway/internal/ir"
 	"github.com/envoyproxy/gateway/internal/xds/types"
 )
@@ -146,7 +147,9 @@ func buildJWTAuthn(irListener *ir.HTTPListener) (*jwtauthnv3.JwtAuthentication,
 			}
 
 			if irProvider.ExtractFrom != nil {
+				jwtProvider.FromHeaders = buildJwtFromHeaders(irProvider.ExtractFrom.Headers)
 				jwtProvider.FromCookies = irProvider.ExtractFrom.Cookies
+				jwtProvider.FromParams = irProvider.ExtractFrom.Params
 			}
 
 			providerKey := fmt.Sprintf("%s/%s", route.Name, irProvider.Name)
@@ -332,3 +335,19 @@ func routeContainsJWTAuthn(irRoute *ir.HTTPRoute) bool {
 func (*jwt) patchRouteConfig(*routev3.RouteConfiguration, *ir.HTTPListener) error {
 	return nil
 }
+
+// buildJwtFromHeaders returns a list of JwtHeader transformed from JWTFromHeader struct
+func buildJwtFromHeaders(headers []v1alpha1.JWTHeaderExtractor) []*jwtauthnv3.JwtHeader {
+	jwtHeaders := make([]*jwtauthnv3.JwtHeader, 0, len(headers))
+
+	for _, header := range headers {
+		jwtHeader := &jwtauthnv3.JwtHeader{
+			Name:        header.Name,
+			ValuePrefix: ptr.Deref(header.ValuePrefix, ""),
+		}
+
+		jwtHeaders = append(jwtHeaders, jwtHeader)
+	}
+
+	return jwtHeaders
+}


Test patch:
diff --git a/internal/gatewayapi/testdata/securitypolicy-with-jwt-with-custom-extractor.in.yaml b/internal/gatewayapi/testdata/securitypolicy-with-jwt-with-custom-extractor.in.yaml
index 96580c849..4c43bc662 100644
--- a/internal/gatewayapi/testdata/securitypolicy-with-jwt-with-custom-extractor.in.yaml
+++ b/internal/gatewayapi/testdata/securitypolicy-with-jwt-with-custom-extractor.in.yaml
@@ -117,5 +117,10 @@ securityPolicies:
         - header: three-route-example-key
           claim: claim3
         extractFrom:
+          headers:
+          - name: Authorization
+            valuePrefix: 'Bearer '
           cookies:
           - session_access_token
+          params:
+          - token
diff --git a/internal/gatewayapi/testdata/securitypolicy-with-jwt-with-custom-extractor.out.yaml b/internal/gatewayapi/testdata/securitypolicy-with-jwt-with-custom-extractor.out.yaml
index 6cf96952e..10b9b7e2d 100644
--- a/internal/gatewayapi/testdata/securitypolicy-with-jwt-with-custom-extractor.out.yaml
+++ b/internal/gatewayapi/testdata/securitypolicy-with-jwt-with-custom-extractor.out.yaml
@@ -201,6 +201,11 @@ securityPolicies:
         extractFrom:
           cookies:
           - session_access_token
+          headers:
+          - name: Authorization
+            valuePrefix: 'Bearer '
+          params:
+          - token
         issuer: https://three.example.com
         name: example3
         remoteJWKS:
@@ -344,6 +349,11 @@ xdsIR:
             extractFrom:
               cookies:
               - session_access_token
+              headers:
+              - name: Authorization
+                valuePrefix: 'Bearer '
+              params:
+              - token
             issuer: https://three.example.com
             name: example3
             remoteJWKS:
diff --git a/internal/xds/translator/testdata/in/xds-ir/jwt-custom-extractor.yaml b/internal/xds/translator/testdata/in/xds-ir/jwt-custom-extractor.yaml
index 2104396d6..2a362a469 100644
--- a/internal/xds/translator/testdata/in/xds-ir/jwt-custom-extractor.yaml
+++ b/internal/xds/translator/testdata/in/xds-ir/jwt-custom-extractor.yaml
@@ -23,6 +23,11 @@ http:
         extractFrom:
           cookies:
           - session_access_token
+          headers:
+          - name: Authorization
+            valuePrefix: 'Bearer '
+          params:
+          - token
     destination:
       name: "first-route-dest"
       settings:
diff --git a/internal/xds/translator/testdata/out/xds-ir/jwt-custom-extractor.listeners.yaml b/internal/xds/translator/testdata/out/xds-ir/jwt-custom-extractor.listeners.yaml
index 3bd76da8a..a0d75b33c 100644
--- a/internal/xds/translator/testdata/out/xds-ir/jwt-custom-extractor.listeners.yaml
+++ b/internal/xds/translator/testdata/out/xds-ir/jwt-custom-extractor.listeners.yaml
@@ -24,6 +24,11 @@
                 forward: true
                 fromCookies:
                 - session_access_token
+                fromHeaders:
+                - name: Authorization
+                  valuePrefix: 'Bearer '
+                fromParams:
+                - token
                 issuer: https://www.example.com
                 payloadInMetadata: https://www.example.com
                 remoteJwks:
diff --git a/site/content/en/latest/api/extension_types.md b/site/content/en/latest/api/extension_types.md
index 0ef92184c..ab2f40bfc 100644
--- a/site/content/en/latest/api/extension_types.md
+++ b/site/content/en/latest/api/extension_types.md
@@ -1095,14 +1095,31 @@ _Appears in:_
 
 
 
-JWTExtractor defines a custom JWT token extraction from HTTP request.
+JWTExtractor defines a custom JWT token extraction from HTTP request. If specified, Envoy will extract the JWT token from the listed extractors (headers, cookies, or params) and validate each of them. If any value extracted is found to be an invalid JWT, a 401 error will be returned.
 
 _Appears in:_
 - [JWTProvider](#jwtprovider)
 
 | Field | Description |
 | --- | --- |
-| `cookies` _string array_ | Cookies represents a list of cookie names to extract the JWT token from. If specified, Envoy will extract the JWT token from the listed cookies and validate each of them. If any cookie is found to be an invalid JWT, a 401 error will be returned. |
+| `headers` _[JWTHeaderExtractor](#jwtheaderextractor) array_ | Headers represents a list of HTTP request headers to extract the JWT token from. |
+| `cookies` _string array_ | Cookies represents a list of cookie names to extract the JWT token from. |
+| `params` _string array_ | Params represents a list of query parameters to extract the JWT token from. |
+
+
+#### JWTHeaderExtractor
+
+
+
+JWTHeaderExtractor defines an HTTP header location to extract JWT token
+
+_Appears in:_
+- [JWTExtractor](#jwtextractor)
+
+| Field | Description |
+| --- | --- |
+| `name` _string_ | Name is the HTTP header name to retrieve the token |
+| `valuePrefix` _string_ | ValuePrefix is the prefix that should be stripped before extracting the token. The format would be used by Envoy like "{ValuePrefix}<TOKEN>". For example, "Authorization: Bearer <TOKEN>", then the ValuePrefix="Bearer " with a space at the end. |
 
 
 #### JWTProvider
diff --git a/site/content/en/latest/install/api.md b/site/content/en/latest/install/api.md
index c293a42ab..e007fdba6 100644
--- a/site/content/en/latest/install/api.md
+++ b/site/content/en/latest/install/api.md
@@ -25,6 +25,7 @@ The Helm chart for Envoy Gateway
 | Key | Type | Default | Description |
 |-----|------|---------|-------------|
 | certgen.job.annotations | object | `{}` |  |
+| certgen.job.resources | object | `{}` |  |
 | certgen.job.ttlSecondsAfterFinished | int | `0` |  |
 | certgen.rbac.annotations | object | `{}` |  |
 | certgen.rbac.labels | object | `{}` |  |