Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support OIDC in eventshub sender #595

Merged

Conversation

creydr
Copy link
Contributor

@creydr creydr commented Oct 6, 2023

Fixes knative/eventing#7333

Changes

  • 🎁 Add OIDC support in eventshub sender. Adds the following options:
    • OIDCValidToken() adds a valid OIDC token to the request of the sender
    • OIDCExpiredToken() adds an OIDC token, which will expire in 10 minutes (allowed minimum) and adds a delay of 11 Minutes to sending to the request (via InitialSenderDelay())
    • OIDCInvalidAudience() adds an OIDC token to the request but with an audience which differs from the sinks audience
    • OIDCCorruptedSignature() adds an OIDC token to the request with a corrupted tokens signature
    • OIDCToken(token) adds the given token to the request (allows to fully customize the used token)

@knative-prow 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 Oct 6, 2023
@creydr
Copy link
Contributor Author

creydr commented Oct 6, 2023

/cc @pierDipi

@creydr creydr force-pushed the support-oidc-in-eventshub-sender branch from a59e8bd to 233843f Compare October 6, 2023 10:04
@creydr
Copy link
Contributor Author

creydr commented Oct 6, 2023

Rebased to get changes from #596 to get tests green

Copy link
Member

@pierDipi pierDipi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By having the sender creating the OIDC token, it will be hard to test what happens when sending an invalid token because it will be always valid, how do you plan to test failure cases?

IMHO, better to pass the token to the sender and have eventshub options that encapsulate the provisioning in order to:

  • send invalid tokens for various reasons
    • expired
    • issued by a different issuer
    • for a different audience
    • etc
  • send valid token

All of the failure or success cases could be options on eventshub sender and encapsulate the logic to provision the expected token valid or invalid token

@pierDipi
Copy link
Member

pierDipi commented Oct 6, 2023

The API for the raw sender could be similar to the SinkBinding contract, a directory from where to get the token, and then the eventshub options could configure what value goes into that directory but on the test side

@creydr
Copy link
Contributor Author

creydr commented Oct 6, 2023

By having the sender creating the OIDC token, it will be hard to test what happens when sending an invalid token because it will be always valid, how do you plan to test failure cases?

IMHO, better to pass the token to the sender and have eventshub options that encapsulate the provisioning in order to:

  • send invalid tokens for various reasons

    • expired
    • issued by a different issuer
    • for a different audience
    • etc
  • send valid token

All of the failure or success cases could be options on eventshub sender and encapsulate the logic to provision the expected token valid or invalid token

I didn't want to do the token generation in the test/feature. For that I added for example the SinkAudience eventshub.Option to let the user set a custom audience (e.g. in case they want to test for a different audience - see for example knative/eventing@e247119). So we can add more of those options to "customize" the token (even to specify a raw token).

Alternatively we could also add some kind of "TokenBuilder", which then gets passed to the eventshub sender as an option, which could be easier to extend 🤷

@pierDipi
Copy link
Member

pierDipi commented Oct 6, 2023

How do we test the other cases like expired tokens?

@creydr
Copy link
Contributor Author

creydr commented Oct 6, 2023

How do we test the other cases like expired tokens?

We could add another option e.g. to specify the expiry of a token, which then gets taken into account in getOIDCToken()

@pierDipi
Copy link
Member

pierDipi commented Oct 6, 2023

That has the problem that every option needs to be built into the sender, I think sender should offer low level APIs, higher level logic can be offered as functions/options.

That also helps with test debugging, errors in the sender are not visible in the test output/logs

@creydr
Copy link
Contributor Author

creydr commented Oct 6, 2023

I updated it to let the user specify the token via the OIDCToken() option.

Can be used like in the following:

// Send event with correct audience
	f.Requirement("install source", func(ctx context.Context, t feature.T) {
		address, err := k8s.Address(ctx, broker.GVR(), brokerName)
		if err != nil {
			t.Fatal(err)
		}

		token, err := auth.NewOIDCTokenProvider(ctx).GetJWT(types.NamespacedName{Namespace: namespace, Name: source1}, *address.Audience)
		if err != nil {
			t.Fatalf("could not get token for source 1 service account: %w", err)
		}

		eventshub.Install(
			source2,
			eventshub.StartSenderToResource(broker.GVR(), brokerName),
			eventshub.OIDCToken(token),
			eventshub.InputEvent(eventForCorrectAudience),
		)(ctx, t)
	})

IMO it would be better, if we have an additional eventhub option, which allows to pass a token generator callback, which gets the context, the Audience of the Sink and the namespace of the source passed, so that we could do something like following:

        f.Requirement("install source", eventshub.Install(
		source2,
		eventshub.StartSenderToResource(broker.GVR(), brokerName),
		eventshub.EnableOIDCAuth,
		eventshub.OIDCTokenGenerator(func(ctx context.Context, sinkAudience, sourceNamespace string) (string, error) {
			return auth.NewOIDCTokenProvider(ctx).GetJWT(types.NamespacedName{
				Namespace: sourceNamespace, 
				Name: source1ServiceAccountName,
			}, sinkAudience)
		}),
		eventshub.InputEvent(eventForCorrectAudience),
	))

But not sure, how this can be done with the current EventhubOption signature 🤔

@creydr creydr changed the title Support OIDC in eventshub sender [WIP] Support OIDC in eventshub sender Oct 6, 2023
@knative-prow knative-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 6, 2023
@creydr creydr force-pushed the support-oidc-in-eventshub-sender branch 2 times, most recently from b99de41 to 8280857 Compare October 9, 2023 10:16
@creydr creydr force-pushed the support-oidc-in-eventshub-sender branch from 8280857 to c56fceb Compare October 9, 2023 10:28
@creydr creydr requested a review from pierDipi October 9, 2023 18:06
@pierDipi
Copy link
Member

Are you planning to have e2e tests for this feature in https://github.com/knative-extensions/reconciler-test/tree/main/test/e2e/eventshub ?

@creydr
Copy link
Contributor Author

creydr commented Oct 10, 2023

Are you planning to have e2e tests for this feature in https://github.com/knative-extensions/reconciler-test/tree/main/test/e2e/eventshub ?

Good point. We should add it, as soon as we have OIDC support in the receiver too (I'll create an issue for tracking it)

pkg/resources/pod/pod.go Outdated Show resolved Hide resolved
pkg/eventshub/options.go Outdated Show resolved Hide resolved
@creydr creydr force-pushed the support-oidc-in-eventshub-sender branch from a4682db to d64579f Compare October 10, 2023 18:35
@creydr creydr requested a review from pierDipi October 10, 2023 19:00
@creydr creydr requested a review from pierDipi October 11, 2023 10:46
Copy link
Member

@pierDipi pierDipi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

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

knative-prow bot commented Oct 11, 2023

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

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

@knative-prow knative-prow bot merged commit 5622ca3 into knative-extensions:main Oct 11, 2023
25 of 28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for OIDC in eventshub sender
2 participants