Enable TLS for OIDC e2e tests #7551

Leo6Leo · 2024-01-08T22:17:53Z

Fixes #7496
Fixes #7544
Fixes #7545
Fixes #7546
Fixes #7547
Fixes #7548
Fixes #7549
Fixes #7558

Proposed Changes

EventsHub receiver/sender with enforced TLS and individual resources which deliver events with CACerts specified for Destination

Pre-review Checklist

At least 80% unit test coverage
E2E tests for any new behavior
Docs PR for any user-facing impact
Spec PR for any new API feature
Conformance test for any change to the spec

Release Note

Docs

Signed-off-by: Leo Li <leoli@redhat.com>

…kenToDLS Signed-off-by: Leo Li <leoli@redhat.com>

knative-prow · 2024-01-08T22:17:56Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

codecov · 2024-01-08T22:23:52Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (fb9be2b) 74.52% compared to head (0cf666c) 74.52%.
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #7551   +/-   ##
=======================================
  Coverage   74.52%   74.52%           
=======================================
  Files         262      262           
  Lines       14970    14970           
=======================================
  Hits        11157    11157           
  Misses       3223     3223           
  Partials      590      590

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

Signed-off-by: Leo Li <leoli@redhat.com>

Leo6Leo · 2024-01-09T19:23:24Z

test/auth/features/oidc/broker.go

+			delivery.WithDeadLetterSinkFromDestination(&duckv1.Destination{
+				Ref:      service.AsKReference(dls),
+				Audience: &dlsAudience,
+				//CACerts: eventshub.GetCaCerts(ctx),


@creydr Question: Do you have any idea on how should I pass the CACert here? With this line I wrote, it is causing the broker creation fail. And I can't find the error message.

When you check the created YAML for the broker with the cert, you see we have a wrong indent of the cert.
Simply add 2 more spaces in delivery.WithDeadLetterSinkFromDestination() when the newlines are replaced:

diff --git a/test/rekt/resources/delivery/delivery.go b/test/rekt/resources/delivery/delivery.go index 8348e5647..626b62c60 100644 --- a/test/rekt/resources/delivery/delivery.go +++ b/test/rekt/resources/delivery/delivery.go @@ -90,7 +90,7 @@ func WithDeadLetterSinkFromDestination(dest *duckv1.Destination) manifest.CfgFn if dest.CACerts != nil { // This is a multi-line string and should be indented accordingly. // Replace "new line" with "new line + spaces". - dls["CACerts"] = strings.ReplaceAll(*dest.CACerts, "\n", "\n ") + dls["CACerts"] = strings.ReplaceAll(*dest.CACerts, "\n", "\n ") } if dest.Audience != nil {

test/auth/features/oidc/broker.go

Signed-off-by: Leo Li <leoli@redhat.com>

Leo6Leo · 2024-01-09T23:01:45Z

test/auth/oidc_test.go

@@ -41,7 +43,7 @@ import (
 	"knative.dev/eventing/test/rekt/resources/sequence"
 )

-func TestBrokerSupportsOIDC(t *testing.T) {
+func TestBrokerSupportsOIDCUnderTLS(t *testing.T) {


Note: Change the test name to include the keyword TLS so that prow will enable TLS for this.

I think we can keep it as TestBrokerSupportsOIDC as we're migrating all of them and we can adjust the prow config for OIDC tests too. And OIDC without TLS is "useless" anyhow

creydr

Hey @Leo6Leo,
besides of the comments, I had to do the following to get the tests run/pass:

enable string transport encryption (via feature flag) - we need to do this for CI too (simply adjust test/auth/config/features.yaml)
install the TLS manifests (config/tls, config/brokers/mt-channel-broker-tls and config/channels/in-memory-channel-tls)

creydr · 2024-01-10T07:46:35Z

test/auth/oidc_test.go

@@ -41,7 +43,7 @@ import (
 	"knative.dev/eventing/test/rekt/resources/sequence"
 )

-func TestBrokerSupportsOIDC(t *testing.T) {
+func TestBrokerSupportsOIDCUnderTLS(t *testing.T) {


I think we can keep it as TestBrokerSupportsOIDC as we're migrating all of them and we can adjust the prow config for OIDC tests too. And OIDC without TLS is "useless" anyhow

test/auth/features/oidc/broker.go

creydr · 2024-01-10T08:26:54Z

test/auth/features/oidc/broker.go

+		// create an empty destination ref
+		d := service.AsDestinationRef("")
+		d.CACerts = eventshub.GetCaCerts(ctx)
+		// uri is an addressable, create a new one and put the bad	uri in it
+		d.URI, _ = apis.ParseURL("bad://uri")
+		trigger.Install(triggerName, brokerName, trigger.WithSubscriberFromDestination(d))(ctx, t)


I got a failed to create resource admission webhook "validation.webhook.eventing.knative.dev" denied the request: validation failed: Absolute URI is not allowed when Ref or [apiVersion, kind, name] is present: spec.subscriber.ref, spec.subscriber.uri, spec.subscriber[apiVersion, kind, name] when running this.
So there is no need to create a destination via service.AsDestinationRef(""). You can create it directly via duckv1.Destination{} and propagate the fields then.

creydr · 2024-01-10T08:29:15Z

test/auth/features/oidc/broker.go

+			delivery.WithDeadLetterSinkFromDestination(&duckv1.Destination{
+				Ref:      service.AsKReference(dls),
+				Audience: &dlsAudience,
+				//CACerts: eventshub.GetCaCerts(ctx),


When you check the created YAML for the broker with the cert, you see we have a wrong indent of the cert.
Simply add 2 more spaces in delivery.WithDeadLetterSinkFromDestination() when the newlines are replaced:

diff --git a/test/rekt/resources/delivery/delivery.go b/test/rekt/resources/delivery/delivery.go index 8348e5647..626b62c60 100644 --- a/test/rekt/resources/delivery/delivery.go +++ b/test/rekt/resources/delivery/delivery.go @@ -90,7 +90,7 @@ func WithDeadLetterSinkFromDestination(dest *duckv1.Destination) manifest.CfgFn if dest.CACerts != nil { // This is a multi-line string and should be indented accordingly. // Replace "new line" with "new line + spaces". - dls["CACerts"] = strings.ReplaceAll(*dest.CACerts, "\n", "\n ") + dls["CACerts"] = strings.ReplaceAll(*dest.CACerts, "\n", "\n ") } if dest.Audience != nil {

Signed-off-by: Leo Li <leoli@redhat.com>

Leo6Leo · 2024-01-12T13:11:12Z

install the TLS manifests (config/tls, config/brokers/mt-channel-broker-tls and config/channels/in-memory-channel-tls)

@creydr Regarding the way to install the TLS manifest, I saw the implementation here. Don't those config have been installed here?

eventing/hack/generate-yamls.sh

Lines 100 to 103 in 3cbddd6

    
           ko resolve ${KO_YAML_FLAGS} -Rf config/tls/ \ 
        
             -Rf config/channels/in-memory-channel-tls/ \ 
        
             -Rf config/brokers/mt-channel-broker-tls/ \ 
        
             | "${LABEL_YAML_CMD[@]}" > "${EVENTING_TLS_YAML}"

eventing/test/e2e-common.sh

Lines 152 to 162 in 3cbddd6

    
           local EVENTING_TLS_REPLACES=${TMP_DIR}/${EVENTING_TLS_YAML##*/} 
        
           sed "s/namespace: ${KNATIVE_DEFAULT_NAMESPACE}/namespace: ${SYSTEM_NAMESPACE}/g" "${EVENTING_TLS_YAML}" > "${EVENTING_TLS_REPLACES}" 
        
           if [[ ! -z "${CLUSTER_SUFFIX:-}" ]]; then 
        
             sed -i "s/cluster.local/${CLUSTER_SUFFIX}/g" "${EVENTING_TLS_REPLACES}" 
        
           fi 
        
           kubectl apply \ 
        
             -f "${EVENTING_TLS_REPLACES}" || return 1 
        
           UNINSTALL_LIST+=( "${EVENTING_TLS_REPLACES}" ) 
        
           kubectl patch horizontalpodautoscalers.autoscaling -n "${SYSTEM_NAMESPACE}" eventing-webhook -p '{"spec": {"minReplicas": '"${REPLICAS}"'}}' || return 1

creydr · 2024-01-12T13:14:34Z

install the TLS manifests (config/tls, config/brokers/mt-channel-broker-tls and config/channels/in-memory-channel-tls)

@creydr Regarding the way to install the TLS manifest, I saw the implementation here. Don't those config have been installed here?

eventing/hack/generate-yamls.sh

Lines 100 to 103 in 3cbddd6

ko resolve ${KO_YAML_FLAGS} -Rf config/tls/ \

-Rf config/channels/in-memory-channel-tls/ \

-Rf config/brokers/mt-channel-broker-tls/ \

| "${LABEL_YAML_CMD[@]}" > "${EVENTING_TLS_YAML}"

eventing/test/e2e-common.sh

Lines 152 to 162 in 3cbddd6

local EVENTING_TLS_REPLACES=${TMP_DIR}/${EVENTING_TLS_YAML##*/}

sed "s/namespace: ${KNATIVE_DEFAULT_NAMESPACE}/namespace: ${SYSTEM_NAMESPACE}/g" "${EVENTING_TLS_YAML}" > "${EVENTING_TLS_REPLACES}"

if [[ ! -z "${CLUSTER_SUFFIX:-}" ]]; then

sed -i "s/cluster.local/${CLUSTER_SUFFIX}/g" "${EVENTING_TLS_REPLACES}"

fi

kubectl apply \

-f "${EVENTING_TLS_REPLACES}" || return 1

UNINSTALL_LIST+=( "${EVENTING_TLS_REPLACES}" )

kubectl patch horizontalpodautoscalers.autoscaling -n "${SYSTEM_NAMESPACE}" eventing-webhook -p '{"spec": {"minReplicas": '"${REPLICAS}"'}}' || return 1

Hey @Leo6Leo,

yes. Sorry for not being clear, that this is only required for local tests as it is already in CI (in contrast to the feature flag, which was not enabled that time in CI).

Leo6Leo · 2024-01-12T13:16:20Z

@creydr Sounds good! thanks for the explanation. Then I think I have addressed all the review comments you gave me. PTAL again when you have time!

creydr · 2024-01-12T13:20:27Z

@creydr Sounds good! thanks for the explanation. Then I think I have addressed all the review comments you gave me. PTAL again when you have time!

Can you check why the auth e2e tests fail?

Signed-off-by: Leo Li <leoli@redhat.com>

creydr · 2024-01-18T07:42:23Z

/retest

creydr

Left some comments. Mostly for naming & style.

And as you included the changes for other resources besides of broker, can you rename the PR?

test/rekt/resources/sequence/sequence.go

creydr · 2024-01-18T07:49:51Z

test/auth/features/oidc/broker.go

 	))
+
+	f.Setup("install the trigger and specify the CA cert of the destination", func(ctx context.Context, t feature.T) {


I would simply name this step install the trigger, as you do some other configuration besides the CA too. Same in the other files.

creydr · 2024-01-18T07:50:57Z

test/auth/features/oidc/broker.go

 	f.Setup("Broker is ready", broker.IsReady(brokerName))

+	// FIXME: current progress left over here. Need to figure out why trigger cannot be initialized correctly.


test/auth/features/oidc/broker.go

creydr · 2024-01-18T07:53:32Z

test/auth/features/oidc/broker.go


 	// Install broker
 	f.Setup("install broker", broker.Install(brokerName, broker.WithEnvConfig()...))
 	f.Setup("Broker is ready", broker.IsReady(brokerName))
+	f.Setup("Broker has HTTPS address", broker.ValidateAddress(brokerName, addressable.AssertHTTPSAddress))


Not sure if we need this, as we have f.Prerequisite("transport encryption is strict", featureflags.TransportEncryptionStrict()). IMO this should be tested in a TLS related test 🤷

test/auth/features/oidc/parallel.go

Co-authored-by: Christoph Stäbler <cstabler@redhat.com>

…er-oidc-test

Co-authored-by: Christoph Stäbler <cstabler@redhat.com>

creydr · 2024-01-18T17:06:42Z

test/auth/features/oidc/sequence.go

@@ -54,7 +56,7 @@ func SequenceSendsEventWithOIDC() *feature.FeatureSet {
 		Name: "Sequence send events with OIDC support",
 		Features: []*feature.Feature{
 			SequenceSendsEventWithOIDCTokenToSteps(),
-			SequenceSendsEventWithOIDCTokenToReply(),
+			//SequenceSendsEventWithOIDCTokenToReply(),


Suggested change

//SequenceSendsEventWithOIDCTokenToReply(),

SequenceSendsEventWithOIDCTokenToReply(),

i was in testing it by commenting out the reply portion. I will uncomment it

Signed-off-by: Leo Li <leoli@redhat.com>

… into enable-tls-for-broker-oidc-test

Signed-off-by: Leo Li <leoli@redhat.com>

Leo6Leo · 2024-01-18T20:42:16Z

/cc @creydr

creydr

Awesome! Thanks @Leo6Leo for your work on this!

/lgtm
/retitle Enable TLS for OIDC tests

knative-prow · 2024-01-18T21:10:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: creydr, Leo6Leo

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:

~~test/OWNERS~~ [Leo6Leo,creydr]

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

Leo6Leo added 3 commits January 8, 2024 16:37

Enable the TLS feature for BrokerSendEventWithOIDCTokenToSubscriber

d46ad64

Signed-off-by: Leo Li <leoli@redhat.com>

Enable the TLS feature for BrokerSendEventWithOIDCTokenToReply

bc97023

Signed-off-by: Leo Li <leoli@redhat.com>

Save the progress on adding TLS support for BrokerSendEventWithOIDCTo…

7db5f0e

…kenToDLS Signed-off-by: Leo Li <leoli@redhat.com>

knative-prow bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. area/test-and-release Test infrastructure, tests or release labels Jan 8, 2024

knative-prow bot requested review from evankanderson and mgencur January 8, 2024 22:17

knative-prow bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 8, 2024

Leo6Leo added 2 commits January 9, 2024 13:09

Merge branch 'main' into enable-tls-for-broker-oidc-test

27678b1

Still couldn't figure out. Will leave a comment there

53e8c40

Signed-off-by: Leo Li <leoli@redhat.com>

Leo6Leo commented Jan 9, 2024

View reviewed changes

Leo6Leo requested a review from creydr January 9, 2024 19:27

Leo6Leo marked this pull request as ready for review January 9, 2024 19:27

Update more test to enable TLS

765ff4e

Signed-off-by: Leo Li <leoli@redhat.com>

Leo6Leo commented Jan 9, 2024

View reviewed changes

creydr reviewed Jan 10, 2024

View reviewed changes

This was referenced Jan 10, 2024

Enable TLS for Apiserversource OIDC test #7556

Closed

enable the TLS feature for ContainerSource's OIDC tests #7559

Closed

Leo6Leo added 5 commits January 10, 2024 15:42

Merge branch 'main' into enable-tls-for-broker-oidc-test

ba9e0da

Fix the review comments

b9ed6db

Signed-off-by: Leo Li <leoli@redhat.com>

Merge branch 'main' into enable-tls-for-broker-oidc-test

cb42428

Fix the review comments by using destination object

eecf885

Signed-off-by: Leo Li <leoli@redhat.com>

Enable TLS in the CI

3ce0a71

Signed-off-by: Leo Li <leoli@redhat.com>

Leo6Leo added 2 commits January 17, 2024 16:23

enable the TLS for TestApiserversourceSendEventWithJWT

162b239

Signed-off-by: Leo Li <leoli@redhat.com>

enable the TLS for TestContainerSourceSendsEventsWithOIDCSupport

fc055e8

Signed-off-by: Leo Li <leoli@redhat.com>

creydr requested changes Jan 18, 2024

View reviewed changes

knative-prow bot assigned creydr Jan 18, 2024

Leo6Leo and others added 6 commits January 18, 2024 11:59

Update test/rekt/resources/sequence/sequence.go

7a156b4

Co-authored-by: Christoph Stäbler <cstabler@redhat.com>

Update test/auth/features/oidc/broker.go

e567102

Co-authored-by: Christoph Stäbler <cstabler@redhat.com>

Update test/auth/features/oidc/broker.go

3e84423

Co-authored-by: Christoph Stäbler <cstabler@redhat.com>

Update test/auth/features/oidc/parallel.go

41c9372

Co-authored-by: Christoph Stäbler <cstabler@redhat.com>

Merge remote-tracking branch 'upstream/main' into enable-tls-for-brok…

f0a6f31

…er-oidc-test

Update test/auth/features/oidc/broker.go

60463ba

Co-authored-by: Christoph Stäbler <cstabler@redhat.com>

creydr reviewed Jan 18, 2024

View reviewed changes

Leo6Leo changed the title ~~[WIP] Enable tls for broker OIDC test~~ [WIP] Enable tls for all OIDC test Jan 18, 2024

creydr mentioned this pull request Jan 18, 2024

Add TLS test for sequence #7600

Merged

Leo6Leo added 5 commits January 18, 2024 12:29

enable the TLS for TestSequenceSendsEventsWithOIDCSupport

d82242a

Signed-off-by: Leo Li <leoli@redhat.com>

Merge remote-tracking branch 'origin/enable-tls-for-broker-oidc-test'…

3d42786

… into enable-tls-for-broker-oidc-test

enable the TLS for TestPingSourceSendsEventsWithOIDC

d7a180d

Signed-off-by: Leo Li <leoli@redhat.com>

enable the TLS for TestChannelDispatcherAuthenticatesWithOIDC

810b5e7

Signed-off-by: Leo Li <leoli@redhat.com>

add the audience field

400c9f1

Signed-off-by: Leo Li <leoli@redhat.com>

Leo6Leo changed the title ~~[WIP] Enable tls for all OIDC test~~ Enable tls for all OIDC test Jan 18, 2024

knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 18, 2024

Code clean up

0cf666c

Signed-off-by: Leo Li <leoli@redhat.com>

knative-prow bot requested a review from creydr January 18, 2024 20:42

creydr approved these changes Jan 18, 2024

View reviewed changes

knative-prow bot changed the title ~~Enable tls for all OIDC test~~ Enable TLS for OIDC tests Jan 18, 2024

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jan 18, 2024

creydr changed the title ~~Enable TLS for OIDC tests~~ Enable TLS for OIDC e2e tests Jan 18, 2024

knative-prow bot merged commit ad51fee into knative:main Jan 18, 2024
38 of 41 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable TLS for OIDC e2e tests #7551

Enable TLS for OIDC e2e tests #7551

Leo6Leo commented Jan 8, 2024 •

edited

Loading

knative-prow bot commented Jan 8, 2024

codecov bot commented Jan 8, 2024 •

edited

Loading

Leo6Leo Jan 9, 2024

creydr Jan 10, 2024

Leo6Leo Jan 9, 2024

creydr Jan 10, 2024

creydr left a comment

creydr Jan 10, 2024

creydr Jan 10, 2024

creydr Jan 10, 2024

Leo6Leo commented Jan 12, 2024

creydr commented Jan 12, 2024

Leo6Leo commented Jan 12, 2024

creydr commented Jan 12, 2024

creydr commented Jan 18, 2024

creydr left a comment

creydr Jan 18, 2024

creydr Jan 18, 2024

creydr Jan 18, 2024

creydr Jan 18, 2024

Leo6Leo Jan 18, 2024

Leo6Leo commented Jan 18, 2024

creydr left a comment

knative-prow bot commented Jan 18, 2024

		))

		f.Setup("install the trigger and specify the CA cert of the destination", func(ctx context.Context, t feature.T) {

		f.Setup("Broker is ready", broker.IsReady(brokerName))

		// FIXME: current progress left over here. Need to figure out why trigger cannot be initialized correctly.

	//SequenceSendsEventWithOIDCTokenToReply(),
	SequenceSendsEventWithOIDCTokenToReply(),

Enable TLS for OIDC e2e tests #7551

Enable TLS for OIDC e2e tests #7551

Conversation

Leo6Leo commented Jan 8, 2024 • edited Loading

Proposed Changes

Pre-review Checklist

knative-prow bot commented Jan 8, 2024

codecov bot commented Jan 8, 2024 • edited Loading

Codecov Report

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

creydr left a 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

Leo6Leo commented Jan 12, 2024

creydr commented Jan 12, 2024

Leo6Leo commented Jan 12, 2024

creydr commented Jan 12, 2024

creydr commented Jan 18, 2024

creydr left a 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

Leo6Leo commented Jan 18, 2024

creydr left a comment

Choose a reason for hiding this comment

knative-prow bot commented Jan 18, 2024

Leo6Leo commented Jan 8, 2024 •

edited

Loading

codecov bot commented Jan 8, 2024 •

edited

Loading