internal encryption e2e tests #14092

KauzClay · 2023-06-12T16:25:38Z

Proposed Changes

Introduces a test that will watch logs and check for TLS connections
I imagine these tests can be expanded as the internal-encryption/trust config feature work continues. For example, I think logging the TLS config on a request will let us see the SAN?

Release Note

N/A

codecov · 2023-06-12T18:04:14Z

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (3eb979a) 86.04% compared to head (3f255b2) 86.10%.

❗ Current head 3f255b2 differs from pull request most recent head bf69494. Consider uploading reports for the commit bf69494 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #14092      +/-   ##
==========================================
+ Coverage   86.04%   86.10%   +0.06%     
==========================================
  Files         196      196              
  Lines       14880    14887       +7     
==========================================
+ Hits        12803    12818      +15     
+ Misses       1764     1759       -5     
+ Partials      313      310       -3

Files	Coverage Δ
pkg/activator/config/store.go	`100.00% <100.00%> (ø)`

... and 8 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

KauzClay · 2023-06-12T20:48:55Z

@evankanderson this is an attempt at the idea you shared during a past Security WG meeting.

evankanderson

Some initial thoughts; overall, this looks pretty good, but I'm going to push to simplify a few parts and complexify a few others. 😁

evankanderson · 2023-06-13T16:05:50Z

pkg/activator/handler/metric_handler.go

+			if isTLS {
+				pkgmetrics.Record(reporterCtx, tlsConnectionCountM.M(1))
+			}


Rather than reporting a second metric, how about adding a tag to the existing metric, ala:

securityMode := // get security mode from context reporterCtx, _ := tag.New(reporterCtx, tag.Upsert(SecurityMode, securityMode))

That will also provide a tag which you can use to check the mutual-TLS mode when it's implemented, and doesn't need to expose an extra metric.

ah cool, I didn't know about tags. That makes sense, this will be easier to expand when the rest of the trust levels are implemented.

I'll make a first attempt at this where I pass the trustmode into the MetricHandler creation, instead of pulling it from Context.

As far as I understand, the activator doesn't have a watcher for the config-network configmap, so I don't think you can pull it from the context. And I'm less confident about changing that right now.

~~ah, now it does have that ability, so I can probably pull from context now: #13694~~

nevermind, that is about certs, not the configuration itself

evankanderson · 2023-06-13T16:07:37Z

pkg/activator/handler/metrics.go

+			Description: "The number of requests that the Activator handles with TLS",
+			Measure:     tlsConnectionCountM,
+			Aggregation: view.Count(),
+			TagKeys:     []tag.Key{metrics.PodKey, metrics.ContainerKey, metrics.ResponseCodeKey, metrics.ResponseCodeClassKey},


If you take my tagging suggestion, You'd need to add the SecurityMode to requestCountM. I'd add it to responseTimeInMsecM as well (and maybe requestConcurrencyM), to allow comparing request latency for TLS and non-TLS if we decide to do that some day.

evankanderson · 2023-06-13T16:13:05Z

test/e2e-internal-encryption-tests.sh

+source $(dirname $0)/e2e-common.sh
+
+function setup_internal_encryption_env_variables() {
+  export TLS_TEST_NAMESPACE="tls"
+
+  local INGRESS_NAMESPACE=${GATEWAY_NAMESPACE_OVERRIDE}
+  if [[ -z "${GATEWAY_NAMESPACE_OVERRIDE}" ]]; then
+    INGRESS_NAMESPACE="istio-system"
+  fi
+  local INGRESS_SERVICE=${GATEWAY_OVERRIDE}
+  if [[ -z "${GATEWAY_OVERRIDE}" ]]; then
+    INGRESS_SERVICE="istio-ingressgateway"
+  fi
+  local IP=$(kubectl get svc -n ${INGRESS_NAMESPACE} ${INGRESS_SERVICE} -o jsonpath="{.status.loadBalancer.ingress[0].ip}")
+  export INTERNAL_ENCRYPTION_TEST_INGRESS_IP=${IP}
+}
+
+function setup_internal_encryption() {
+  toggle_feature dataplane-trust enabled config-network
+
+  sleep 5
+
+  # with the current implementation, Activator is always in the request path, and needs to be restarted after configuring dataplane-trust
+  kubectl -n ${SYSTEM_NAMESPACE} delete pod -l app=activator
+}
+
+function cleanup_internal_encryption() {
+  toggle_feature dataplane-trust disabled config-network
+
+  sleep 5
+
+  # with the current implementation, Activator is always in the request path, and needs to be restarted after configuring dataplane-trust
+  kubectl -n ${SYSTEM_NAMESPACE} delete pod -l app=activator
+}
+
+# Script entry point.
+initialize "$@" --skip-istio-addon --min-nodes=4 --max-nodes=4 --enable-ha --cluster-version=1.25
+
+# Run the tests
+header "Running tests"
+
+failed=0
+
+# Currently only Contour and Kourier implement the alpha features.
+alpha=""
+if [[ "${INGRESS_CLASS}" == "contour.ingress.networking.knative.dev" || "${INGRESS_CLASS}" == "kourier.ingress.networking.knative.dev" ]]; then
+  alpha="--enable-alpha"
+fi
+
+INTERNAL_ENCRYPTION_TEST_OPTIONS="${INTERNAL_ENCRYPTION_TEST_OPTIONS:-${alpha} --enable-beta}"
+
+# Auto TLS E2E tests mutate the cluster and must be ran separately
+# because they need auto-tls and cert-manager specific configurations
+subheader "Setup internal encryption"
+setup_internal_encryption
+add_trap "cleanup_internal_encryption" EXIT SIGKILL SIGTERM SIGQUIT


Eeep... any way this could be less shell, or have a little block explaining why it's shell?

(Maybe "see the README" is sufficient...)

ah yeah, this part was one I wasn't so sure on. I was under the impression you needed a script like this if you needed to change a config on the test cluster (like setting dataplane-trust), so I just tried to base things on the auto-tls tests.

But I'll be honest, I don't completely understand how that script ties into the rest of the tooling so that the tests actually run.

okay, I got a little clarification, and I think I can move the core behavior here into the e2e-tests.sh file, and get rid of the file itself.

test/e2e/internalencryption/README.md

evankanderson · 2023-06-13T16:16:07Z

test/e2e/internalencryption/README.md

+
+In order to test Internal Encryption, this test looks at `tls_connection_count` metrics from the Activator and QueueProxy.
+
+The `metricsreader` test image [here](../../test_images/metricsreader/README.md) was created for this purpose. Given the PodIPs of the Activator and the Knative Service pod (i.e. the QueueProxy), it will make requests to each respective `/metrics` endpoint, pull out the `*_tls_connection_count` metric, and respond with the value.


Can metricsreader find the pods using label queries and/or DNS queries (if the services are headless services -- I don't know if / how that would perturb the rest of Knative, though.)

Hmm I'm not sure I'm following. Are you suggesting that the metricsreader image itself gets the IPs, rather than gathering that info as part of the test code?

Right now, I have the test getting that info, since it seemed easier to test and make sure everything was up and ready. My initial intention was to make the metricsreader image do as little as possible.

* also only run for Contour and Kourier right now

* was having some bash issues with properly interpretting the patch

* we have switched to system-internal-tls

KauzClay · 2023-10-05T15:12:40Z

/test kourier-stable

nak3 · 2023-10-06T03:31:02Z

/retest

nak3 · 2023-10-06T03:31:34Z

/test kourier-stable

nak3 · 2023-10-06T04:53:50Z

/lgtm
/approve

Thank you 🎉

knative-prow · 2023-10-06T04:53:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: KauzClay, nak3

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [nak3]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

nak3 · 2023-10-06T05:28:27Z

/retest

knative-prow bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 12, 2023

KauzClay mentioned this pull request Jun 12, 2023

Implement e2e test for internal encryption #13536

Closed

knative-prow bot added the area/API API objects and controllers label Jun 12, 2023

knative-prow bot requested review from evankanderson and skonto June 12, 2023 16:26

knative-prow bot added area/autoscale area/networking area/test-and-release It flags unit/e2e/conformance/perf test issues for product features labels Jun 12, 2023

KauzClay force-pushed the ck-internal-encryption-e2e-tests branch from a6ca6fa to ed90501 Compare June 12, 2023 16:40

knative-prow bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 12, 2023

KauzClay force-pushed the ck-internal-encryption-e2e-tests branch 5 times, most recently from c0c3824 to a99da3c Compare June 12, 2023 18:02

knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 12, 2023

KauzClay force-pushed the ck-internal-encryption-e2e-tests branch from a99da3c to f83b492 Compare June 12, 2023 18:03

KauzClay force-pushed the ck-internal-encryption-e2e-tests branch from f83b492 to 380eb72 Compare June 12, 2023 18:04

knative-prow bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 13, 2023

KauzClay force-pushed the ck-internal-encryption-e2e-tests branch 2 times, most recently from 73c1f11 to db3ca6f Compare June 13, 2023 14:34

evankanderson reviewed Jun 13, 2023

View reviewed changes

knative-prow bot added the area/monitoring label Jun 13, 2023

KauzClay force-pushed the ck-internal-encryption-e2e-tests branch from 432196e to b443b09 Compare June 15, 2023 14:12

KauzClay added 20 commits October 5, 2023 10:53

test: add test image for reading request metrics

b078718

test: add internal encryption e2e tests

c0e4365

run update-deps

6983066

first pass: change internal encryption test to read logs

d0a75ba

remove metricreader image

2387840

update internal encryption test readme

08c57c4

run update-deps

d531732

remove securitymode metric

b351b70

address linter suggestions

e71fcba

more lint suggestions

e5b35d2

clean up test

c7b730c

make internal encryption test alpha only

bcb33c9

* also only run for Contour and Kourier right now

cleanup leftovers from metrics stuff

4e842b0

address nits

b1059c2

fix typo

a3aad96

address PR feedback

0ec9af3

fix failing test

b431397

use correct symbol when looking for TLS access log

192a578

refactor toggle_feature

58256a7

* was having some bash issues with properly interpretting the patch

use new flag name

bf69494

* we have switched to system-internal-tls

KauzClay force-pushed the ck-internal-encryption-e2e-tests branch from 3f255b2 to bf69494 Compare October 5, 2023 15:15

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Oct 6, 2023

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 6, 2023

knative-prow bot merged commit c183543 into knative:main Oct 6, 2023
62 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

internal encryption e2e tests #14092

internal encryption e2e tests #14092

KauzClay commented Jun 12, 2023 •

edited

Loading

codecov bot commented Jun 12, 2023 •

edited

Loading

KauzClay commented Jun 12, 2023

evankanderson left a comment

evankanderson Jun 13, 2023

KauzClay Jun 13, 2023

KauzClay Aug 14, 2023 •

edited

Loading

evankanderson Jun 13, 2023

evankanderson Jun 13, 2023

evankanderson Jun 13, 2023

KauzClay Jun 13, 2023

KauzClay Jun 15, 2023

evankanderson Jun 13, 2023

KauzClay Jun 14, 2023

KauzClay commented Oct 5, 2023

nak3 commented Oct 6, 2023

nak3 commented Oct 6, 2023

nak3 commented Oct 6, 2023

knative-prow bot commented Oct 6, 2023

nak3 commented Oct 6, 2023


		In order to test Internal Encryption, this test looks at `tls_connection_count` metrics from the Activator and QueueProxy.

		The `metricsreader` test image [here](../../test_images/metricsreader/README.md) was created for this purpose. Given the PodIPs of the Activator and the Knative Service pod (i.e. the QueueProxy), it will make requests to each respective `/metrics` endpoint, pull out the `*_tls_connection_count` metric, and respond with the value.

internal encryption e2e tests #14092

internal encryption e2e tests #14092

Conversation

KauzClay commented Jun 12, 2023 • edited Loading

Proposed Changes

codecov bot commented Jun 12, 2023 • edited Loading

Codecov Report

KauzClay commented Jun 12, 2023

evankanderson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KauzClay Aug 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KauzClay commented Oct 5, 2023

nak3 commented Oct 6, 2023

nak3 commented Oct 6, 2023

nak3 commented Oct 6, 2023

knative-prow bot commented Oct 6, 2023

nak3 commented Oct 6, 2023

KauzClay commented Jun 12, 2023 •

edited

Loading

codecov bot commented Jun 12, 2023 •

edited

Loading

KauzClay Aug 14, 2023 •

edited

Loading