Enable envoy tls termination #41

dobrerazvan · 2022-01-27T13:49:40Z

Q	A
Bug fix?	no/yes
New feature?	no/yes
API breaks?	no/yes
Deprecations?	no/yes
Related tickets	fixes #X, partially #Y, mentioned in #Z
License	Apache 2.0

What's in this PR?

Why?

Additional context

Checklist

Implementation tested
Error handling code meets the guideline
Logging code meets the guideline
User guide and development docs updated (if needed)

To Do

If the PR is not complete but you want to discuss the approach, list what remains to be done here

api/v1beta1/kafkacluster_types.go

amuraru · 2022-01-31T14:05:52Z

pkg/resources/kafka/kafka.go

@@ -1035,6 +1035,9 @@ func (r *Reconciler) createExternalListenerStatuses(log logr.Logger) (map[string
 				if err != nil {
 					return nil, err
 				}
+				if eListener.ExternalStartingPort == -1 {


same comment, eListener offers a HostnameOverride field that can be used here?

api/v1beta1/kafkacluster_types.go

amuraru · 2022-01-31T14:07:00Z

we also need unit-tests added

api/v1beta1/kafkacluster_types.go

charts/kafka-operator/templates/crds.yaml

pkg/resources/envoy/configmap.go

alungu · 2022-02-03T18:12:00Z

pkg/resources/envoy/configmap.go

+	// Create an any cast broker access point
+	if elistener.ExternalStartingPort == -1 {
+		// append l[AnyCastPort] with another filter for the same listener
+		filterChain = &envoylistener.FilterChain{


Could we avoid somehow code duplication here? I see that only the hostname is different from the broker specific filter.

@dobrerazvan please don't forget about this code duplicate, too

dobrerazvan · 2022-02-15T13:52:05Z

@alungu @amuraru I implemented the requested review. I think it works. Next, unit tests.

ecojan · 2022-02-18T09:16:49Z

api/v1beta1/kafkacluster_types.go

@@ -373,6 +385,29 @@ func (c IngressServiceSettings) GetServiceType() corev1.ServiceType {
 	return c.ServiceType
 }

+// Replace {{.Id}} in brokerHostnameTemplate with actual broker id


Suggestion to do string replace here instead of template engine from Go.
Reasons are that we are doing the same process here (not calling any extra functions to require template engine), current deployment spec is done via Helm (need to escape double templating).
wdyt?

alungu

Mainly looks good 👍
There are only some minor changes that I think it would make the code a bit easier to read.

alungu · 2022-02-21T14:56:54Z

pkg/resources/envoy/configmap.go

+				{
+					CertificateChain: &envoycore.DataSource{
+						Specifier: &envoycore.DataSource_Filename{
+							Filename: "/certs/certificate.crt",


If these paths are hard coded, it means that we always have to mount the secrets to these paths, right?
Should we add this information the CRD?

Since this is generated and used un the envoy deployment to mount the ssl cert, i think it could be hardcoded, doesn't really matter where is mounted as long as it's mounted somewhere.

alungu · 2022-02-21T16:54:02Z

pkg/resources/envoy/configmap.go

+			}
+
+			if elistener.TLSEnabled() {
+				// tls


minor: should we drop this comment (and the one below non tls? The condition is very clear.

alungu · 2022-02-21T16:57:49Z

pkg/resources/envoy/configmap.go

+
+	for _, p := range ports {
+
+		if elistener.ExternalStartingPort == -1 {


could we replace this with the method helper TLSEnabled?

alungu · 2022-02-21T16:57:59Z

pkg/resources/envoy/deployment.go

@@ -66,6 +65,26 @@ func (r *Reconciler) deployment(log logr.Logger, extListener v1beta1.ExternalLis
 		},
 	}

+	if extListener.ExternalStartingPort == -1 && extListener.TLSSecretName != "" {


could we replace this with the method helper TLSEnabled?

alungu · 2022-02-21T16:59:08Z

pkg/resources/envoy/service.go

-				TargetPort: intstr.FromInt(int(extListener.ExternalStartingPort) + brokerId),
-				Protocol:   corev1.ProtocolTCP,
-			})
+	if extListener.ExternalStartingPort != -1 {


could we replace this with the method helper TLSEnabled?

alungu · 2022-02-22T09:29:29Z

pkg/resources/envoy/configmap.go

+			},
+		},
+	}
+	pbTlsContext, _ := anypb.New(tlsContext)


Should we check the returned error, too?

alungu · 2022-02-22T09:33:47Z

pkg/resources/envoy/configmap.go

+	// Create an any cast broker access point
+	if elistener.ExternalStartingPort == -1 {
+		// append l[AnyCastPort] with another filter for the same listener
+		filterChain = &envoylistener.FilterChain{


@dobrerazvan please don't forget about this code duplicate, too

alungu · 2022-02-22T09:56:06Z

pkg/resources/envoy/configmap.go

+		ports = append(ports, int(p))
+	}
+	sort.Ints(ports)
+


minor: extra newline

alungu · 2022-02-22T09:57:13Z

pkg/resources/envoy/configmap.go

+	for _, p := range ports {
+
+		if elistener.ExternalStartingPort == -1 {
+			listeners = append(listeners, &envoylistener.Listener{


The Listener is the same object for both TLS and non-TLS. could we reuse the code?

alungu · 2022-02-22T09:58:50Z

pkg/resources/envoy/configmap.go

@@ -226,37 +259,48 @@ func GenerateEnvoyConfig(kc *v1beta1.KafkaCluster, elistener v1beta1.ExternalLis
 					Cluster: fmt.Sprintf("broker-%d", brokerId),
 				},
 			}
+


This method (GenerateEnvoyConfig) has almost 400 lines. Could we split it in multiple functions, please?

I added dedicated functions that generate tls specifics

amuraru · 2022-03-07T17:58:00Z

api/v1beta1/kafkacluster_types.go

@@ -407,6 +428,8 @@ type IngressServiceSettings struct {
 	// In case of external listeners using NodePort access method the broker instead of node public IP (see "brokerConfig.nodePortExternalIP")
 	// is advertised on the address having the following format: <kafka-cluster-name>-<broker-id>.<namespace><value-specified-in-hostnameOverride-field>
 	HostnameOverride string `json:"hostnameOverride,omitempty"`
+	// Template used to generate broker hostnames for tls enabled envoy. %id will be replaced with brokerId value
+	BrokerHostnameTemplate string `json:"brokerHostnameTemplate,omitempty"`


@dobrerazvan can this be moved to KafkaClusterSpec.EnvoyConfig ?

The reason for this is that the comment says it applies to "envoy" ingress while this IngressServiceSettings is generic for all kafka ingresses (istio, envoy)

@amuraru Implemented the review.

amuraru · 2022-03-14T09:57:07Z

@dobrerazvan please rebase this on top of master. I just force rebased the master branch to sync with upstream banzaicloud/master branch

…adobe/kafka docker hub repos - build the koperator docker image - build Apache kafka docker image When `kafka-*` tags are created a github action is triggered to build and push a new adobe/kafka docker image version

dobrerazvan · 2022-03-15T16:25:48Z

@dobrerazvan please rebase this on top of master. I just force rebased the master branch to sync with upstream banzaicloud/master branch

Did that.

amuraru · 2022-03-15T16:27:46Z

api/v1beta1/kafkacluster_types.go

@@ -377,6 +390,16 @@ func (c IngressServiceSettings) GetServiceType() corev1.ServiceType {
 	return c.ServiceType
 }

+// Replace %id in brokerHostnameTemplate with actual broker id
+func (c EnvoyConfig) GetBrokerHostname(brokerId int32) string {


Suggested change

func (c EnvoyConfig) GetBrokerHostname(brokerId int32) string {

func (c EnvoyConfig) Get BrokerHostnameTemplate(brokerId int32) string {

* [INTERNAL] [BUILD] Publish docker images to adobe/kafka-operator and adobe/kafka docker hub repos - build the koperator docker image - build Apache kafka docker image When `kafka-*` tags are created a github action is triggered to build and push a new adobe/kafka docker image version * [INTERNAL] make manifests should be called manually, if needed (#25) We made some chnages for spinnaker annotations and `preserveUnknownFields` that would be overriden by `make manifests` * [INTERNAL] Allow Kafka to use External DNS for inter-broker protocol (#17) (#22) * [INTERNAL] Allow external listeners to be used for inner communication (#26) * [INTERNAL] Ensure external listerners are always the first the advertised.listeners configuration This is needed for old clients connecting to kafka through Zookeeper that does not have a way to infer the right listener. In this case, the first listener in the advertised.listener config is used to connect to brokers. This patch ensures the external listeners (those reachable from outside) are listed before internal ones * [INTERNAL] Generate CRDs resources * [INTERNAL] Upgrade to Kafka 2.8.1 (#36) * Enable envoy idleTimeout and TCP keep-alive for connections to kafka and clients 1/ Kafka broker defines connections.max.idle.ms=600s To ensure envoy as a client for kafka broker is terminating the connection first to avoid network disconnects this patch is setting the idleTimeout to value slightly less than that 2/ Enable tcp-keep alive for all TCP connections established by envoy to kafka and to client (or fronting Load Balancer) * Enable envoy tls termination (#41) * [INTERNAL] Build kafka 3.1.1 using Oracle OpenJDK * Envoy config generated by the operator is invalid in envoy 1.22 Added explicit typeconfig for envoy.filters.http.router ``` [2022-06-16 13:27:58.425][1][info][main] [source/server/server.cc:939] exiting Didn't find a registered implementation for 'envoy.filters.http.router' with type URL: '' ``` * Added TaintedBrokersSelector to kafkaClusterSpec (#48) Co-authored-by: Adrian Muraru <adi.muraru@gmail.com> * Build kafka 3.2.2 * Upgrade kafka to 3.2.3 * Upgrade kafka to 3.3.1 * [Internal] Update helm for adobe builds (#52) * [INTERNAL] Use local replacement for sub-modules (#54) As part of banzaicloud#929, the local replacement for sub-modules was removed in favor of using valid tags. Adobe Koperator fork has some internal changes in the api sub-module, so we need to use the local version of the sub-module instead of the upstream version, so we are forced to revert the changes from banzaicloud#929. * Upgrade kafka to 3.4.0 --------- Co-authored-by: Adi Muraru <amuraru@adobe.com> Co-authored-by: Adrian Lungu <adrian.lungu89@gmail.com> Co-authored-by: Razvan Dobre <dobre.razvan@gmail.com> Co-authored-by: Adrian Muraru <adi.muraru@gmail.com> Co-authored-by: Adrian Coman <acoman@adobe.com> Co-authored-by: aguzovatii <guzovatii.anatolii@gmail.com>

dobrerazvan added enhancement New feature or request WIP Work in progress - do not merge labels Jan 27, 2022

amuraru reviewed Jan 31, 2022

View reviewed changes

api/v1beta1/kafkacluster_types.go Outdated Show resolved Hide resolved

amuraru reviewed Jan 31, 2022

View reviewed changes

api/v1beta1/kafkacluster_types.go Outdated Show resolved Hide resolved

alungu reviewed Feb 3, 2022

View reviewed changes

dobrerazvan force-pushed the dobre/work-2 branch from d57de35 to c62bdc7 Compare February 17, 2022 08:04

ecojan reviewed Feb 18, 2022

View reviewed changes

alungu reviewed Feb 22, 2022

View reviewed changes

alungu approved these changes Feb 22, 2022

View reviewed changes

dobrerazvan force-pushed the dobre/work-2 branch from 07776c5 to 642a67f Compare February 23, 2022 10:32

ecojan approved these changes Feb 23, 2022

View reviewed changes

amuraru reviewed Mar 7, 2022

View reviewed changes

amuraru force-pushed the master branch from acd09d7 to e7cc1ab Compare March 14, 2022 09:56

amuraru force-pushed the master branch from e7cc1ab to 3782b99 Compare March 14, 2022 10:00

amuraru and others added 3 commits March 15, 2022 18:00

Update Kafka PDB to use live Pods instead of the CR Spec (adobe#42)

63ce9cf

Run goimports -local "github.com/banzaicloud" (adobe#44)

5ae048b

dobrerazvan force-pushed the dobre/work-2 branch from e4466c2 to f5ea9e1 Compare March 15, 2022 16:09

Enable TLS on the Envoy ingress

810b7a5

dobrerazvan force-pushed the dobre/work-2 branch from c50cab2 to 810b7a5 Compare March 15, 2022 16:26

amuraru reviewed Mar 15, 2022

View reviewed changes

amuraru approved these changes Mar 15, 2022

View reviewed changes

amuraru removed the WIP Work in progress - do not merge label Mar 15, 2022

dobrerazvan merged commit c7d6614 into adobe:master Mar 17, 2022

amuraru pushed a commit that referenced this pull request Mar 21, 2022

Enable envoy tls termination (#41)

7b67bcb

amuraru pushed a commit that referenced this pull request Mar 30, 2022

Enable envoy tls termination (#41)

6108081

amuraru pushed a commit that referenced this pull request Apr 22, 2022

Enable envoy tls termination (#41)

f9249e4

amuraru pushed a commit that referenced this pull request May 3, 2022

Enable envoy tls termination (#41)

ca7b709

amuraru pushed a commit that referenced this pull request May 3, 2022

Enable envoy tls termination (#41)

150e105

amuraru pushed a commit that referenced this pull request May 8, 2022

Enable envoy tls termination (#41)

f24d3b4

amuraru pushed a commit that referenced this pull request May 11, 2022

Enable envoy tls termination (#41)

98c440e

amuraru pushed a commit that referenced this pull request May 11, 2022

Enable envoy tls termination (#41)

489b8bf

amuraru pushed a commit that referenced this pull request May 13, 2022

Enable envoy tls termination (#41)

9541053

amuraru pushed a commit that referenced this pull request May 23, 2022

Enable envoy tls termination (#41)

982baa3

amuraru pushed a commit that referenced this pull request May 27, 2022

Enable envoy tls termination (#41)

0a8378d

amuraru pushed a commit that referenced this pull request Jun 17, 2022

Enable envoy tls termination (#41)

7fc78fc

amuraru pushed a commit that referenced this pull request Jun 29, 2022

Enable envoy tls termination (#41)

826ef22

azun pushed a commit that referenced this pull request Jul 28, 2022

Enable envoy tls termination (#41)

4981be4

azun pushed a commit that referenced this pull request Jul 29, 2022

Enable envoy tls termination (#41)

992e297

amuraru pushed a commit that referenced this pull request Jul 29, 2022

Enable envoy tls termination (#41)

bea1e67

amuraru pushed a commit that referenced this pull request Aug 22, 2022

Enable envoy tls termination (#41)

ca16844

amuraru pushed a commit that referenced this pull request Sep 10, 2022

Enable envoy tls termination (#41)

535e9a9

alungu pushed a commit that referenced this pull request Jan 5, 2023

Enable envoy tls termination (#41)

55140ed

aguzovatii pushed a commit to aguzovatii/koperator that referenced this pull request Mar 22, 2023

Enable envoy tls termination (adobe#41)

2713356

aguzovatii pushed a commit to aguzovatii/koperator that referenced this pull request Mar 22, 2023

Enable envoy tls termination (adobe#41)

7df57fe

aguzovatii pushed a commit to aguzovatii/koperator that referenced this pull request Mar 23, 2023

Enable envoy tls termination (adobe#41)

346114c

amuraru pushed a commit that referenced this pull request May 16, 2023

Enable envoy tls termination (#41)

6321dfd

ctrlaltluc pushed a commit that referenced this pull request Jun 8, 2023

Enable envoy tls termination (#41)

11e9ed3

ctrlaltluc pushed a commit that referenced this pull request Jun 8, 2023

Enable envoy tls termination (#41)

4f395db

amuraru pushed a commit that referenced this pull request Jun 10, 2023

Enable envoy tls termination (#41)

e1e2946

amuraru pushed a commit that referenced this pull request Dec 12, 2023

Enable envoy tls termination (#41)

450ebe7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable envoy tls termination #41

Enable envoy tls termination #41

dobrerazvan commented Jan 27, 2022

amuraru Jan 31, 2022

amuraru commented Jan 31, 2022

alungu Feb 3, 2022

alungu Feb 22, 2022

dobrerazvan commented Feb 15, 2022

ecojan Feb 18, 2022

alungu left a comment

alungu Feb 21, 2022

dobrerazvan Feb 22, 2022

alungu Feb 21, 2022

alungu Feb 21, 2022

alungu Feb 21, 2022

alungu Feb 21, 2022

alungu Feb 22, 2022

alungu Feb 22, 2022

alungu Feb 22, 2022

alungu Feb 22, 2022

alungu Feb 22, 2022

dobrerazvan Feb 22, 2022

amuraru Mar 7, 2022

dobrerazvan Mar 15, 2022

amuraru commented Mar 14, 2022

dobrerazvan commented Mar 15, 2022

amuraru Mar 15, 2022


		for _, p := range ports {

		if elistener.ExternalStartingPort == -1 {

	func (c EnvoyConfig) GetBrokerHostname(brokerId int32) string {
	func (c EnvoyConfig) Get BrokerHostnameTemplate(brokerId int32) string {

Enable envoy tls termination #41

Enable envoy tls termination #41

Conversation

dobrerazvan commented Jan 27, 2022

What's in this PR?

Why?

Additional context

Checklist

To Do

Choose a reason for hiding this comment

amuraru commented Jan 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dobrerazvan commented Feb 15, 2022

Choose a reason for hiding this comment

alungu 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

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

Choose a reason for hiding this comment

amuraru commented Mar 14, 2022

dobrerazvan commented Mar 15, 2022

Choose a reason for hiding this comment