Repository: knative-sandbox/net-kourier
Pull request #522 (2021-05-20 08:32:44)

Make access logging configurable in the types
<!-- Thanks for sending a pull request! -->

# Changes

Ref #518 

Coming from the Golang types, this threads the possibility to configure access logging in the types. This will eventually connect with the ConfigMap to close the circle.

/assign @nak3 

knative-prow-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by:
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please ask for approval from **markusthoemmes** after the PR has been reviewed.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative-sandbox%2Fnet-kourier).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/knative-sandbox/net-kourier/blob/main/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["markusthoemmes"]} -->
knative-metrics-robot: The following is the coverage report on the affected files.
Say `/test pull-knative-sandbox-net-kourier-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[pkg/envoy/api/http_connection_manager.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative-sandbox_net-kourier/522/pull-knative-sandbox-net-kourier-go-coverage/1395296429227905024/artifacts/line-cov.html#file0) | 85.7% | 90.0% | 4.3

codecov[bot]: # [Codecov](https://codecov.io/gh/knative-sandbox/net-kourier/pull/522?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox) Report
> Merging [#522](https://codecov.io/gh/knative-sandbox/net-kourier/pull/522?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox) (275ebc0) into [main](https://codecov.io/gh/knative-sandbox/net-kourier/commit/118f612d785e7ca508f92d0f6b30b04fcb84c493?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox) (118f612) will **increase** coverage by `0.07%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/knative-sandbox/net-kourier/pull/522/graphs/tree.svg?width=650&height=150&src=pr&token=KmlRo23zX4&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox)](https://codecov.io/gh/knative-sandbox/net-kourier/pull/522?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox)

```diff
@@            Coverage Diff             @@
##             main     #522      +/-   ##
==========================================
+ Coverage   81.70%   81.78%   +0.07%     
==========================================
  Files          14       14              
  Lines         503      505       +2     
==========================================
+ Hits          411      413       +2     
  Misses         66       66              
  Partials       26       26              
```


| [Impacted Files](https://codecov.io/gh/knative-sandbox/net-kourier/pull/522?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox) | Coverage Δ | |
|---|---|---|
| [pkg/envoy/api/http\_connection\_manager.go](https://codecov.io/gh/knative-sandbox/net-kourier/pull/522/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox#diff-cGtnL2Vudm95L2FwaS9odHRwX2Nvbm5lY3Rpb25fbWFuYWdlci5nbw==) | `94.11% <100.00%> (+0.36%)` | :arrow_up: |
| [pkg/generator/caches.go](https://codecov.io/gh/knative-sandbox/net-kourier/pull/522/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox#diff-cGtnL2dlbmVyYXRvci9jYWNoZXMuZ28=) | `59.22% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative-sandbox/net-kourier/pull/522?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/knative-sandbox/net-kourier/pull/522?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox). Last update [118f612...275ebc0](https://codecov.io/gh/knative-sandbox/net-kourier/pull/522?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox).

knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative-sandbox/net-kourier/pull/522#pullrequestreview-664131076" title="LGTM">nak3</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative-sandbox%2Fnet-kourier).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/knative-sandbox/net-kourier/blob/main/OWNERS)~~ [nak3]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/pkg/envoy/api/http_connection_manager.go b/pkg/envoy/api/http_connection_manager.go
index 66bbe821..1183281c 100644
--- a/pkg/envoy/api/http_connection_manager.go
+++ b/pkg/envoy/api/http_connection_manager.go
@@ -34,7 +34,7 @@ import (
 
 // NewHTTPConnectionManager creates a new HttpConnectionManager that points to the given
 // RouteConfig for further configuration.
-func NewHTTPConnectionManager(routeConfigName string) *httpconnectionmanagerv2.HttpConnectionManager {
+func NewHTTPConnectionManager(routeConfigName string, enableAccessLog bool) *httpconnectionmanagerv2.HttpConnectionManager {
 	filters := make([]*httpconnectionmanagerv2.HttpFilter, 0, 1)
 
 	if config.ExternalAuthz.Enabled {
@@ -46,21 +46,10 @@ func NewHTTPConnectionManager(routeConfigName string) *httpconnectionmanagerv2.H
 		Name: wellknown.Router,
 	})
 
-	// Write access logs to stdout by default.
-	accessLog, _ := anypb.New(&accesslog_v2.FileAccessLog{
-		Path: "/dev/stdout",
-	})
-
-	return &httpconnectionmanagerv2.HttpConnectionManager{
+	mgr := &httpconnectionmanagerv2.HttpConnectionManager{
 		CodecType:   httpconnectionmanagerv2.HttpConnectionManager_AUTO,
 		StatPrefix:  "ingress_http",
 		HttpFilters: filters,
-		AccessLog: []*envoy_accesslog_v2.AccessLog{{
-			Name: "envoy.file_access_log",
-			ConfigType: &envoy_accesslog_v2.AccessLog_TypedConfig{
-				TypedConfig: accessLog,
-			},
-		}},
 		RouteSpecifier: &httpconnectionmanagerv2.HttpConnectionManager_Rds{
 			Rds: &httpconnectionmanagerv2.Rds{
 				ConfigSource: &envoy_api_v2_core.ConfigSource{
@@ -73,6 +62,22 @@ func NewHTTPConnectionManager(routeConfigName string) *httpconnectionmanagerv2.H
 			},
 		},
 	}
+
+	if enableAccessLog {
+		// Write access logs to stdout by default.
+		accessLog, _ := anypb.New(&accesslog_v2.FileAccessLog{
+			Path: "/dev/stdout",
+		})
+
+		mgr.AccessLog = []*envoy_accesslog_v2.AccessLog{{
+			Name: "envoy.file_access_log",
+			ConfigType: &envoy_accesslog_v2.AccessLog_TypedConfig{
+				TypedConfig: accessLog,
+			},
+		}}
+	}
+
+	return mgr
 }
 
 // NewRouteConfig create a new RouteConfiguration with the given name and hosts.
diff --git a/pkg/generator/caches.go b/pkg/generator/caches.go
index 84f59263..456bde16 100644
--- a/pkg/generator/caches.go
+++ b/pkg/generator/caches.go
@@ -213,8 +213,8 @@ func generateListenersAndRouteConfigs(
 	internalRouteConfig.ValidateClusters = wrapperspb.Bool(true)
 
 	// Now we setup connection managers, that reference the routeconfigs via RDS.
-	externalManager := envoy.NewHTTPConnectionManager(externalRouteConfig.Name)
-	internalManager := envoy.NewHTTPConnectionManager(internalRouteConfig.Name)
+	externalManager := envoy.NewHTTPConnectionManager(externalRouteConfig.Name, true /*enableAccessLog*/)
+	internalManager := envoy.NewHTTPConnectionManager(internalRouteConfig.Name, true /*enableAccessLog*/)
 	externalHTTPEnvoyListener, err := envoy.NewHTTPListener(externalManager, config.HTTPPortExternal)
 	if err != nil {
 		return nil, nil, err


Test patch:
diff --git a/pkg/envoy/api/http_connection_manager_test.go b/pkg/envoy/api/http_connection_manager_test.go
index cb0ec0f8..0c39670a 100644
--- a/pkg/envoy/api/http_connection_manager_test.go
+++ b/pkg/envoy/api/http_connection_manager_test.go
@@ -29,8 +29,13 @@ import (
 	"gotest.tools/v3/assert"
 )
 
-func TestNewHTTPConnectionManager(t *testing.T) {
-	connManager := NewHTTPConnectionManager("test")
+func TestNewHTTPConnectionManagerWithoutAccessLog(t *testing.T) {
+	connManager := NewHTTPConnectionManager("test", false /*enableAccessLog*/)
+	assert.Check(t, len(connManager.AccessLog) == 0)
+}
+
+func TestNewHTTPConnectionManagerWithAccessLog(t *testing.T) {
+	connManager := NewHTTPConnectionManager("test", true /*enableAccessLog*/)
 	accessLog := connManager.AccessLog[0]
 	accessLogPathAny := accessLog.ConfigType.(*envoy_config_filter_accesslog_v2.AccessLog_TypedConfig).TypedConfig
 	fileAccesLog := &accesslog_v2.FileAccessLog{}
diff --git a/pkg/envoy/api/listener_test.go b/pkg/envoy/api/listener_test.go
index f25cb52d..20bfb891 100644
--- a/pkg/envoy/api/listener_test.go
+++ b/pkg/envoy/api/listener_test.go
@@ -33,7 +33,7 @@ import (
 )
 
 func TestNewHTTPListener(t *testing.T) {
-	manager := NewHTTPConnectionManager("test")
+	manager := NewHTTPConnectionManager("test", true /*enableAccessLog*/)
 
 	l, err := NewHTTPListener(manager, 8080)
 	assert.NilError(t, err)
@@ -45,7 +45,7 @@ func TestNewHTTPListener(t *testing.T) {
 }
 
 func TestNewHTTPSListener(t *testing.T) {
-	manager := NewHTTPConnectionManager("test")
+	manager := NewHTTPConnectionManager("test", true /*enableAccessLog*/)
 
 	certChain := []byte("some_certificate_chain")
 	privateKey := []byte("some_private_key")
@@ -76,7 +76,7 @@ func TestNewHTTPSListenerWithSNI(t *testing.T) {
 		PrivateKey:       []byte("key2"),
 	}}
 
-	manager := NewHTTPConnectionManager("test")
+	manager := NewHTTPConnectionManager("test", true /*enableAccessLog*/)
 	listener, err := NewHTTPSListenerWithSNI(manager, 8443, sniMatches)
 	assert.NilError(t, err)