Add support for SASL and SSL #534

pierDipi · 2021-01-13T10:27:07Z

Fixes #368

This PR adds support for SASL and SSL.

A KafkaSink or a Broker enables SASL and SSL by adding a reference to a secret, like:

# broker config ConfigMap
auth.secret.ref.name: my-secret

# KafkaSink spec
spec:
  auth:
    secret:
      ref:
        name: my-secret

# Or
spec:
  auth.secret.ref.name: my-secret

The referenced secret format is documented in our proto definition and E2E tests contain all possible formats, see test/e2e/broker_sasl_ssl_test.go

Proposed Changes

Add support for SASL and SSL

Release Note

Add support for SASL and SSL.

Docs

TODO User documentation

/kind enhancement

codecov · 2021-01-13T10:29:35Z

Codecov Report

Merging #534 (03e7068) into master (0a76189) will decrease coverage by 5.07%.
The diff coverage is 80.21%.

@@             Coverage Diff              @@
##             master     #534      +/-   ##
============================================
- Coverage     76.96%   71.88%   -5.08%     
- Complexity      269      362      +93     
============================================
  Files            59       71      +12     
  Lines          1984     2586     +602     
  Branches         82      121      +39     
============================================
+ Hits           1527     1859     +332     
- Misses          342      583     +241     
- Partials        115      144      +29

Flag	Coverage Δ	Complexity Δ
java-unittests	`79.11% <85.77%> (+1.16%)`	`0.00 <95.00> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ	Complexity Δ
...nting/kafka/broker/core/security/AuthProvider.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
...knative/eventing/kafka/broker/dispatcher/Main.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
...v/knative/eventing/kafka/broker/receiver/Main.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
...fka/broker/core/security/PlaintextCredentials.java	`25.00% <25.00%> (ø)`	`2.00 <2.00> (?)`
control-plane/pkg/reconciler/base/reconciler.go	`5.45% <28.57%> (ø)`	`0.00 <0.00> (?)`
.../eventing/kafka/broker/receiver/RequestMapper.java	`92.62% <42.85%> (-3.07%)`	`17.00 <0.00> (+1.00)`	⬇️
control-plane/pkg/reconciler/kafka/admin.go	`63.63% <50.00%> (-11.37%)`	`0.00 <0.00> (ø)`
.../core/reconciler/impl/ResourcesReconcilerImpl.java	`86.20% <50.00%> (-0.64%)`	`41.00 <0.00> (+1.00)`	⬇️
control-plane/pkg/security/config.go	`53.33% <53.33%> (ø)`	`0.00 <0.00> (?)`
control-plane/pkg/security/scram.go	`63.63% <63.63%> (ø)`	`0.00 <0.00> (?)`
... and 31 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a76189...9836bf6. Read the comment docs.

pierDipi · 2021-01-13T11:15:54Z

The linter complains about the generated code.

pierDipi · 2021-01-13T11:20:12Z

/kind api-change

pierDipi · 2021-01-13T12:17:06Z

/retest

pierDipi · 2021-01-13T13:31:24Z

Unrelated
/test pull-knative-sandbox-eventing-kafka-broker-integration-tests

pierDipi · 2021-01-13T14:18:01Z

/test pull-knative-sandbox-eventing-kafka-broker-integration-tests

pierDipi · 2021-01-13T15:20:49Z

/test pull-knative-sandbox-eventing-kafka-broker-integration-tests

pierDipi · 2021-01-13T16:48:10Z

/test pull-knative-sandbox-eventing-kafka-broker-integration-tests
#537

matzew · 2021-01-14T17:34:26Z

/assign

I will do a review on Friday

slinkydeveloper

This is going in the right direction. I left some style comments and a bunch of questions, but I look forward to the @matzew review, he has better knowledge about sasl/tls than me

proto/def/contract.proto

...main/java/dev/knative/eventing/kafka/broker/dispatcher/http/HttpConsumerVerticleFactory.java

data-plane/pom.xml

...ain/java/dev/knative/eventing/kafka/broker/core/reconciler/impl/ResourcesReconcilerImpl.java

data-plane/core/src/main/java/dev/knative/eventing/kafka/broker/core/security/Credentials.java

control-plane/pkg/reconciler/broker/testdata/ca.crt

slinkydeveloper · 2021-01-14T18:00:07Z

control-plane/pkg/reconciler/sink/controller.go

+	secretinformer.Get(ctx).Informer().AddEventHandler(controller.HandleAll(reconciler.SecretTracker.OnChanged))
+
+	sinkInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
+		DeleteFunc: reconciler.OnDeleteObserver,


Why that? Can't you just invoke OnDeleteObserver in the FinalizeKind of the broker reconciler?

I started with your proposed approach and then migrated to this one since this is the way used in other places in eventing.
ATM, they should be equivalent.
However, given this issue: knative/pkg#1956, I think it's safer to call it regardless of being the leader or not.

Can you add a TODO, in order to remember in future that we need to port this code to FinalizeKind after pkg#1956 is solved? The FinalizeKind approach is still more idiomatic

For example:
time 1: p is the leader for x -> reconcile x
time 2: p is not the leader for x anymore
time 3: delete x
time 4: no FinilizeKind called on x in p (this is a leak)

Calling it outside FinalizeKind makes sure that the reconciler doesn't leak.

Example usage in serving: https://github.com/knative/serving/blob/b7a7c18bb5f6c14ae73c9dda1a817ded1ae879b0/pkg/reconciler/route/controller.go#L108-L110

ATM, they should be equivalent.

This is not true.

slinkydeveloper · 2021-01-14T18:02:51Z

control-plane/pkg/reconciler/broker/controller.go

+	brokerInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{
+		FilterFunc: kafka.BrokerClassFilter(),
+		Handler: cache.ResourceEventHandlerFuncs{
+			DeleteFunc: reconciler.OnDeleteObserver,


Why that? Can't you just invoke OnDeleteObserver in the FinalizeKind of the broker reconciler?

Same as the other comment.

Signed-off-by: Pierangelo Di Pilato <[email protected]>

knative-metrics-robot · 2021-01-25T10:12:40Z

The following is the coverage report on the affected files.
Say /test pull-knative-sandbox-eventing-kafka-broker-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
control-plane/pkg/apis/eventing/v1alpha1/kafka_sink_types.go	66.7%	75.0%	8.3
control-plane/pkg/reconciler/base/reconciler.go	Do not exist	3.7%
control-plane/pkg/reconciler/broker/broker.go	89.4%	87.9%	-1.5
control-plane/pkg/reconciler/broker/controller.go	78.6%	81.8%	3.2
control-plane/pkg/reconciler/kafka/admin.go	87.5%	81.8%	-5.7
control-plane/pkg/reconciler/sink/auth_provider.go	Do not exist	100.0%
control-plane/pkg/reconciler/sink/controller.go	75.0%	78.9%	3.9
control-plane/pkg/reconciler/sink/kafka_sink.go	85.3%	83.5%	-1.8
control-plane/pkg/security/config.go	Do not exist	70.0%
control-plane/pkg/security/scram.go	Do not exist	72.7%
control-plane/pkg/security/secret.go	Do not exist	92.4%

knative-prow-robot · 2021-01-25T11:12:23Z

@pierDipi: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-knative-sandbox-eventing-kafka-broker-go-coverage	`9836bf6`	link	`/test pull-knative-sandbox-eventing-kafka-broker-go-coverage`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

pierDipi · 2021-01-25T11:14:10Z

/test pull-knative-sandbox-eventing-kafka-broker-integration-tests

slinkydeveloper · 2021-01-27T15:18:15Z

/unhold

/lgtm
/approve

knative-prow-robot · 2021-01-27T15:18:25Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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~~ [pierDipi,slinkydeveloper]

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

* Add Data plane security module and Kubernetes provider Signed-off-by: Pierangelo Di Pilato <[email protected]> * Add Auth configurations to contract Signed-off-by: Pierangelo Di Pilato <[email protected]> * Add contol plane security module Signed-off-by: Pierangelo Di Pilato <[email protected]> * Integrate security module in Broker and Sink reconciler Signed-off-by: Pierangelo Di Pilato <[email protected]> * Add control plane E2E tests Signed-off-by: Pierangelo Di Pilato <[email protected]> * Update proto schema and use PEM format Signed-off-by: Pierangelo Di Pilato <[email protected]> * Move data plane to PEM certificates format Signed-off-by: Pierangelo Di Pilato <[email protected]> * Extend E2E test by sending events Signed-off-by: Pierangelo Di Pilato <[email protected]> * Run E2E test multiple times to reduce flakiness Signed-off-by: Pierangelo Di Pilato <[email protected]> * Improve comment Signed-off-by: Pierangelo Di Pilato <[email protected]> * Refresh third party license list Signed-off-by: Pierangelo Di Pilato <[email protected]> * Update docs in proto definition Signed-off-by: Pierangelo Di Pilato <[email protected]> * Rename E2E test functions Signed-off-by: Pierangelo Di Pilato <[email protected]> * auth.secret.name -> auth.secret.ref.name Make reference explicit. Signed-off-by: Pierangelo Di Pilato <[email protected]> * Add boilerplate to reconciler_test.go Signed-off-by: Pierangelo Di Pilato <[email protected]> * Lint and update codegen Signed-off-by: Pierangelo Di Pilato <[email protected]> * Change comment to Kubernetes resource reference Signed-off-by: Pierangelo Di Pilato <[email protected]> * Remove Nullable annotations Signed-off-by: Pierangelo Di Pilato <[email protected]> * Ensure TypeMeta when Tracker OnChanged is called Signed-off-by: Pierangelo Di Pilato <[email protected]> * Remove unused Sarama logger adapter function Signed-off-by: Pierangelo Di Pilato <[email protected]> * Use symlinks to testdata certs Signed-off-by: Pierangelo Di Pilato <[email protected]> * KafkaSink supports SASL / SSL Signed-off-by: Pierangelo Di Pilato <[email protected]> * Move bootstrap servers config in one place Signed-off-by: Pierangelo Di Pilato <[email protected]> * Add KafkaSink E2E tests Signed-off-by: Pierangelo Di Pilato <[email protected]> * Refresh third party file Signed-off-by: Pierangelo Di Pilato <[email protected]> * Add Validation test for secret reference Signed-off-by: Pierangelo Di Pilato <[email protected]> * Update codegen Signed-off-by: Pierangelo Di Pilato <[email protected]> * Update KafkaSink CRD schema Signed-off-by: Pierangelo Di Pilato <[email protected]> * Test security config functions Signed-off-by: Pierangelo Di Pilato <[email protected]> * Test security config and scram modules Signed-off-by: Pierangelo Di Pilato <[email protected]> * Rename data plane roles Signed-off-by: Pierangelo Di Pilato <[email protected]> * Maven exclusion directly in parent pom Signed-off-by: Pierangelo Di Pilato <[email protected]> * Refactor credentials fetching Signed-off-by: Pierangelo Di Pilato <[email protected]>

Neustradamus · 2022-01-11T00:32:54Z

@pierDipi: Thanks a lot for all about SCRAM.

Linked to:

State of Play scram-sasl/info#1

knative-prow-robot added kind/enhancement area/control-plane labels Jan 13, 2021

knative-prow-robot requested review from matzew and slinkydeveloper January 13, 2021 10:27

knative-prow-robot added area/data-plane area/test 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. labels Jan 13, 2021

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Jan 13, 2021

pierDipi force-pushed the KNATIVE-368 branch from 49227ad to d27ee8f Compare January 13, 2021 11:14

pierDipi mentioned this pull request Jan 13, 2021

[Sink] Add support for SASL #368

Closed

knative-prow-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Jan 13, 2021

knative-prow-robot assigned matzew Jan 14, 2021

slinkydeveloper reviewed Jan 14, 2021

View reviewed changes

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 18, 2021

pierDipi added 6 commits January 18, 2021 14:12

Add Data plane security module and Kubernetes provider

217e4b3

Signed-off-by: Pierangelo Di Pilato <[email protected]>

Add Auth configurations to contract

885bae0

Signed-off-by: Pierangelo Di Pilato <[email protected]>

Add contol plane security module

4b6c831

Signed-off-by: Pierangelo Di Pilato <[email protected]>

Integrate security module in Broker and Sink reconciler

f93d129

Signed-off-by: Pierangelo Di Pilato <[email protected]>

Add control plane E2E tests

28802a7

Signed-off-by: Pierangelo Di Pilato <[email protected]>

Update proto schema and use PEM format

8e1e702

Signed-off-by: Pierangelo Di Pilato <[email protected]>

pierDipi added 8 commits January 18, 2021 14:12

Remove Nullable annotations

3b312d2

Signed-off-by: Pierangelo Di Pilato <[email protected]>

Ensure TypeMeta when Tracker OnChanged is called

ba47c98

Signed-off-by: Pierangelo Di Pilato <[email protected]>

Remove unused Sarama logger adapter function

d8fbee0

Signed-off-by: Pierangelo Di Pilato <[email protected]>

Use symlinks to testdata certs

c8079c4

Signed-off-by: Pierangelo Di Pilato <[email protected]>

KafkaSink supports SASL / SSL

4eb7b89

Signed-off-by: Pierangelo Di Pilato <[email protected]>

Move bootstrap servers config in one place

d3cbc35

Signed-off-by: Pierangelo Di Pilato <[email protected]>

Add KafkaSink E2E tests

3dd445d

Signed-off-by: Pierangelo Di Pilato <[email protected]>

Refresh third party file

e9882f0

Signed-off-by: Pierangelo Di Pilato <[email protected]>

pierDipi force-pushed the KNATIVE-368 branch from 7361b5c to e9882f0 Compare January 18, 2021 13:17

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 18, 2021

pierDipi added 8 commits January 18, 2021 14:28

Add Validation test for secret reference

44b7086

Signed-off-by: Pierangelo Di Pilato <[email protected]>

Update codegen

f98026f

Signed-off-by: Pierangelo Di Pilato <[email protected]>

Update KafkaSink CRD schema

b680429

Signed-off-by: Pierangelo Di Pilato <[email protected]>

Test security config functions

0f00ee3

Signed-off-by: Pierangelo Di Pilato <[email protected]>

Test security config and scram modules

efdd565

Signed-off-by: Pierangelo Di Pilato <[email protected]>

Rename data plane roles

417c5f7

Signed-off-by: Pierangelo Di Pilato <[email protected]>

Maven exclusion directly in parent pom

0ff238d

Signed-off-by: Pierangelo Di Pilato <[email protected]>

Refactor credentials fetching

9836bf6

Signed-off-by: Pierangelo Di Pilato <[email protected]>

knative-prow-robot assigned slinkydeveloper Jan 27, 2021

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 27, 2021

knative-prow-robot merged commit 9fd3feb into knative-extensions:master Jan 27, 2021

pierDipi mentioned this pull request Feb 1, 2021

Document SASL and SSL support for Kafka Broker and KafkaSink knative/docs#3202

Closed

pierDipi deleted the KNATIVE-368 branch April 14, 2021 21:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for SASL and SSL #534

Add support for SASL and SSL #534

pierDipi commented Jan 13, 2021 •

edited

Loading

codecov bot commented Jan 13, 2021 •

edited

Loading

pierDipi commented Jan 13, 2021

pierDipi commented Jan 13, 2021

pierDipi commented Jan 13, 2021

pierDipi commented Jan 13, 2021

pierDipi commented Jan 13, 2021

pierDipi commented Jan 13, 2021

pierDipi commented Jan 13, 2021

matzew commented Jan 14, 2021

slinkydeveloper left a comment

slinkydeveloper Jan 14, 2021

pierDipi Jan 15, 2021

slinkydeveloper Jan 18, 2021

pierDipi Jan 18, 2021 •

edited

Loading

pierDipi Jan 18, 2021

slinkydeveloper Jan 14, 2021

pierDipi Jan 15, 2021

knative-metrics-robot commented Jan 25, 2021

knative-prow-robot commented Jan 25, 2021 •

edited

Loading

pierDipi commented Jan 25, 2021

slinkydeveloper commented Jan 27, 2021

knative-prow-robot commented Jan 27, 2021

Neustradamus commented Jan 11, 2022

Add support for SASL and SSL #534

Add support for SASL and SSL #534

Conversation

pierDipi commented Jan 13, 2021 • edited Loading

Proposed Changes

codecov bot commented Jan 13, 2021 • edited Loading

Codecov Report

pierDipi commented Jan 13, 2021

pierDipi commented Jan 13, 2021

pierDipi commented Jan 13, 2021

pierDipi commented Jan 13, 2021

pierDipi commented Jan 13, 2021

pierDipi commented Jan 13, 2021

pierDipi commented Jan 13, 2021

matzew commented Jan 14, 2021

slinkydeveloper left a comment

Choose a reason for hiding this comment

slinkydeveloper Jan 14, 2021

Choose a reason for hiding this comment

pierDipi Jan 15, 2021

Choose a reason for hiding this comment

slinkydeveloper Jan 18, 2021

Choose a reason for hiding this comment

pierDipi Jan 18, 2021 • edited Loading

Choose a reason for hiding this comment

pierDipi Jan 18, 2021

Choose a reason for hiding this comment

slinkydeveloper Jan 14, 2021

Choose a reason for hiding this comment

pierDipi Jan 15, 2021

Choose a reason for hiding this comment

knative-metrics-robot commented Jan 25, 2021

knative-prow-robot commented Jan 25, 2021 • edited Loading

pierDipi commented Jan 25, 2021

slinkydeveloper commented Jan 27, 2021

knative-prow-robot commented Jan 27, 2021

Neustradamus commented Jan 11, 2022

pierDipi commented Jan 13, 2021 •

edited

Loading

codecov bot commented Jan 13, 2021 •

edited

Loading

pierDipi Jan 18, 2021 •

edited

Loading

knative-prow-robot commented Jan 25, 2021 •

edited

Loading