Add auth-proxy and enable on IntegrationSink #8708

creydr · 2025-09-09T13:20:22Z

Most/all of the IntegrationSink components are not written in Golang and thus can use our libs for AuthN and AuthZ checks. Therefor the IntegrationSink does not support AuthN and AuthZ currently.
This PR adds an auth-proxy, which can be used as a sidecar container and does the Auth checks. Therefor we can get AuthN/AuthZ support on the IntegrationSink.

This PR contains the following main parts:

Adds the auth-proxy (788cc90)
Updates the integration sink controller to include the auth-proxy as a sidecar to the integrationsink deployments and takes care about the needed RBAC (auth-proxy needs permissions to read the features & logging CM from eventing namespace) (c5f6679)
Adding AuthN & AuthZ e2e tests for the IntegrationSink (9f0ff40)

Hint:
The auth-proxy requires permissions to read the features and logging configmaps from the knative-eventing namespace. Therefor each pod running an IntegrationSink needs to have a RoleBinding for the knative-eventing-auth-proxy role in the knative-eventing namespace. Instead of adding a RoleBinding for each IntegrationSink, we aggregate it together in the eventing-auth-proxy RoleBinding to not have x similar RoleBindings.

codecov · 2025-09-09T13:27:48Z

Codecov Report

❌ Patch coverage is 6.08229% with 525 lines in your changes missing coverage. Please review.
✅ Project coverage is 50.73%. Comparing base (941dafa) to head (4409cbc).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
cmd/auth_proxy/main.go	0.00%	195 Missing ⚠️
...iler/integration/sink/resources/container_image.go	0.00%	167 Missing ⚠️
pkg/reconciler/integration/sink/integrationsink.go	35.22%	51 Missing and 6 partials ⚠️
pkg/reconciler/integration/sink/controller.go	0.00%	42 Missing ⚠️
pkg/auth/verifier.go	0.00%	39 Missing ⚠️
.../apis/sinks/v1alpha1/integration_sink_lifecycle.go	0.00%	12 Missing ⚠️
pkg/eventingtls/eventingtls.go	0.00%	6 Missing ⚠️
pkg/reconciler/testing/v1alpha1/integrationsink.go	0.00%	6 Missing ⚠️
pkg/reconciler/integration/sink/resources/names.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8708      +/-   ##
==========================================
- Coverage   51.62%   50.73%   -0.90%     
==========================================
  Files         401      402       +1     
  Lines       25459    25962     +503     
==========================================
+ Hits        13144    13172      +28     
- Misses      11509    11979     +470     
- Partials      806      811       +5

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

cmd/auth_proxy/main.go

Cali0707 · 2025-09-09T13:54:42Z

cmd/auth_proxy/main.go

+	}
+
+	logger.Info("Starting auth proxy servers...")
+	if err = serverManager.StartServers(ctx); err != nil {


maybe we add otel instrumentation to these servers for a better observability picture into this sidecar?

IMO, this should be a follow up PR on top of this

yes, let's do OTel in a separate one

pkg/reconciler/integration/sink/integrationsink.go

Cali0707

@creydr could you TAL at the linter errors?

Error: cmd/auth_proxy/main.go:155:2: ineffectual assignment to ctx (ineffassign)
  	ctx = logging.WithLogger(ctx, logger)
  	^
  Error: pkg/auth/verifier.go:400:2: Consider pre-allocating `subjectsWithFiltersFromApplyingPolicies` (prealloc)
  	var subjectsWithFiltersFromApplyingPolicies []SubjectsWithFilters

creydr · 2025-09-09T15:09:07Z

@creydr could you TAL at the linter errors?

Error: cmd/auth_proxy/main.go:155:2: ineffectual assignment to ctx (ineffassign)
  	ctx = logging.WithLogger(ctx, logger)
  	^
  Error: pkg/auth/verifier.go:400:2: Consider pre-allocating `subjectsWithFiltersFromApplyingPolicies` (prealloc)
  	var subjectsWithFiltersFromApplyingPolicies []SubjectsWithFilters

fixed

Cali0707

/lgtm
/hold in case @creydr wants to wait for other reviews

Cali0707 · 2025-09-10T12:24:32Z

cmd/auth_proxy/main.go

+
+	logger.Debugf("Handling request to %s", r.RequestURI)
+
+	err := h.authVerifier.VerifyRequestFromSubjectsWithFilters(ctx, features, h.config.SinkAudience, h.authSubjects, h.config.SinkNamespace, r, w)


@creydr I may be missing something but since we aren't copying the request here would this have the same problem for structured events that #8710 is trying to avoid?

The authVerifier.Verify...() method, internally copies the request to avoid the problem.
In #8710 we had the issue, that the request was accessed before it was passed to the authVerifier already. (At least I think so)

But I'll rebase this PR after #8710 is in, to get the authz tests updated, which check for this issue now

Co-authored-by: Calum Murray <cmurray@redhat.com>

creydr · 2025-09-10T13:44:48Z

Needed to resolve some merge conflicts

matzew · 2025-09-10T14:37:50Z

pkg/reconciler/integration/sink/controller.go

+		trustBundleConfigMapLister: trustBundleConfigMapInformer.Lister(),
+		integrationSinkLister:      integrationSinkInformer.Lister(),
+		rolebindingLister:          rolebindingInformer.Lister(),
+		authProxyImage:             env.AuthProxyImage,


I htink we need like this for event transformer as well

but lets do on separate pr

Yes. I'd do this as a separate PR. Added #8715 to have an issue for it

matzew

/lgtm
/approve

knative-prow · 2025-09-10T14:39:32Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [creydr,matzew]

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

creydr · 2025-09-10T14:57:27Z

Pod scheduling timeout.
/retest-required
/unhold

Cali0707 · 2025-09-10T17:41:49Z

@creydr @matzew should we backport this one at all?

creydr · 2025-09-16T07:30:11Z

@creydr @matzew should we backport this one at all?

I wouldn't at this point and give it a bit time to settle (and also integrate it into EventTransform). But when users ask for it, we can do it of course.

creydr requested review from Cali0707 and matzew September 9, 2025 13:20

knative-prow bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. area/test-and-release Test infrastructure, tests or release labels Sep 9, 2025

knative-prow bot requested review from mgencur and pierDipi September 9, 2025 13:20

Cali0707 reviewed Sep 9, 2025

View reviewed changes

creydr requested a review from Cali0707 September 9, 2025 20:05

Cali0707 reviewed Sep 9, 2025

View reviewed changes

knative-prow bot assigned Cali0707 Sep 9, 2025

knative-prow bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Sep 9, 2025

Cali0707 reviewed Sep 10, 2025

View reviewed changes

creydr and others added 9 commits September 10, 2025 15:43

Add auth-proxy

7f358d6

Add auth-proxy to IntegrationSink

fa061e3

Add OIDC and AuthZ test for IntegrationSink

0e549d6

Fix linting issue

d0b5fad

Fix unit tests

ebfa542

Mark required fields in envConfig

65c9cec

Reorder envConfig

236d9c7

Update cmd/auth_proxy/main.go

0d63a21

Co-authored-by: Calum Murray <cmurray@redhat.com>

Address linter issues

4409cbc

creydr force-pushed the auth-proxy branch from 6f3fbf4 to 4409cbc Compare September 10, 2025 13:44

knative-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 10, 2025

matzew reviewed Sep 10, 2025

View reviewed changes

matzew approved these changes Sep 10, 2025

View reviewed changes

knative-prow bot assigned matzew Sep 10, 2025

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Sep 10, 2025

creydr mentioned this pull request Sep 10, 2025

Add auth checks to EventTransformer #8715

Open

knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 10, 2025

knative-prow bot merged commit e8e8035 into knative:main Sep 10, 2025
34 of 36 checks passed

creydr mentioned this pull request Sep 12, 2025

IntegrationSink: OIDC test #8524

Closed

5 tasks


		logger.Debugf("Handling request to %s", r.RequestURI)

		err := h.authVerifier.VerifyRequestFromSubjectsWithFilters(ctx, features, h.config.SinkAudience, h.authSubjects, h.config.SinkNamespace, r, w)

Add auth-proxy and enable on IntegrationSink #8708

Add auth-proxy and enable on IntegrationSink #8708

Uh oh!

Conversation

creydr commented Sep 9, 2025

Uh oh!

codecov bot commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Cali0707 Sep 9, 2025

Choose a reason for hiding this comment

Uh oh!

matzew Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Cali0707 left a comment

Choose a reason for hiding this comment

Uh oh!

creydr commented Sep 9, 2025

Uh oh!

Cali0707 left a comment

Choose a reason for hiding this comment

Uh oh!

Cali0707 Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

creydr Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

creydr commented Sep 10, 2025

Uh oh!

matzew Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

creydr Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

matzew left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow bot commented Sep 10, 2025

Uh oh!

creydr commented Sep 10, 2025

Uh oh!

Uh oh!

Cali0707 commented Sep 10, 2025

Uh oh!

creydr commented Sep 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Sep 9, 2025 •

edited

Loading