chore: improve kafka startup logging #696

lzap · 2023-10-04T12:32:04Z

It looks like we use TLS on stage but not on prod and ephemeral.

This is small logging improvement that should confirm it as we do not dump configuration on prod.

lzap · 2023-10-04T12:32:24Z

internal/kafka/kafka.go

@@ -98,7 +106,6 @@ func NewKafkaBroker(ctx context.Context) (Broker, error) {

 		//nolint:gosec
 		tlsConfig = &tls.Config{
-			MinVersion:         tls.VersionTLS12,


Unrelated, TLS 1.2 is the default value when this field is nil so useless.

ezr-ondrej · 2023-10-04T13:46:48Z

internal/kafka/kafka.go

@@ -81,7 +81,15 @@ func NewKafkaBroker(ctx context.Context) (Broker, error) {
 	var saslMechanism sasl.Mechanism

 	logger := zerolog.Ctx(ctx)
-	logger.Debug().Msgf("Setting up Kafka transport: %v", config.Kafka.Brokers)
+	logger.Info().Msgf("Setting up Kafka transport: %v TLS:%b Skip:%b CA:%b Auth:%s SASLMech:%s SASLProto:%s",


I guess %b should have been %t?

Suggested change

logger.Info().Msgf("Setting up Kafka transport: %v TLS:%b Skip:%b CA:%b Auth:%s SASLMech:%s SASLProto:%s",

logger.Info().Msgf("Setting up Kafka transport: %v TLS:%t Skip:%t CA:%t Auth:%s SASLMech:%s SASLProto:%s",

lzap · 2023-10-04T13:48:54Z

Rebased.

ezr-ondrej · 2023-10-04T14:25:27Z

internal/kafka/kafka.go

+		config.Kafka.Brokers,
+		config.Kafka.TlsEnabled,
+		config.Kafka.TlsSkipVerify,
+		config.Kafka.CACert != "",
+		config.Kafka.AuthType,
+		config.Kafka.SASL.SaslMechanism,
+		config.Kafka.SASL.SecurityProtocol,


aren't we more interested in what's in the clowder config directly?

ezr-ondrej

sure, let's get it in :)

ezr-ondrej · 2023-10-04T14:42:42Z

/retest

lzap · 2023-10-04T15:04:53Z

Do not merge, I have some new info about clowder.

lzap · 2023-10-04T15:49:15Z

After chat with Derek Horton, he suggest to provide an empty TLS config value as the library is supposed to automatically detect if TLS is present on a cluster or not and use the safest transport. He mentioned that he uses this on their project and it works even after Kafka migration from this weekend.

Therefore this patch drops the newly introduced setting completely, I don’t have a TLS Kafka setup in my home lab so we need to test this. Ephemeral is plaintext and stage is TLS so if both works prod should be fine. Then I will remove these flags.

lzap · 2023-10-04T16:12:33Z

DO NOT MERGE.

Looks like Derek was not right, might not work as he described.

lzap · 2023-10-05T07:34:13Z

I am reverting the commit back to the initial one - only improves logging and drops a line that has no effect. Looks like our solution is the best for now. I asked via email if we can use one of clowder's fields to detect TLS, let’s wait until I get some reply.

lzap · 2023-10-05T07:37:58Z

_Regarding how apps should know whether SSL is on or off moving forward... I think a good example we have is the one Dan Kuc just pushed for malware-detection[1]. You are correct that the only values Clowder sets on this field is either "SSL"[2] or "SASL_SSL"[3,4]. In short, I think you can use this logic to determine if your Kafka client should use SSL or not:

if kafka.brokers[0].securityProtocol is defined and its value is "SSL" or "SASL_SSL", then SSL should be ON

Your client should be able to use SSL with or without the 'cacert' field present. If the field is present, then use that as your client's trust store. If not, then ideally your client falls back to using the "system default trust store" -- which should already trust well-known root CA's like Digicert, Verisign, LetsEncrypt, and so on._

So I will go ahead and drop our own field and rely on this field instead.

lzap · 2023-10-05T07:52:06Z

So the latest version drops the KAFKA_TLS_ENABLED flag and uses SecurityProtocol flag to check if it contains "SSL" string. If it does, it configures TLS.

The same way it also checks for SASL in the AuthType which should contain this string if SASL should be used.

I am keeping the verification skip flag tho, can be useful for local TLS testing. Once this is confirmed to work on both stage and ephemeral we can remove flags from app-interface. I believe this is the final version.

ezr-ondrej

Just two nits and only one of them addressable xD
Apart of it great effort here to find out how to do it properly! 👍

ezr-ondrej · 2023-10-05T09:12:38Z

internal/config/config.go

-			Password         string `env:"PASSWORD" env-default:"" env-description:"kafka SASL password"`
-			SaslMechanism    string `env:"MECHANISM" env-default:"" env-description:"kafka SASL mechanism (scram-sha-512, scram-sha-256 or plain)"`
-			SecurityProtocol string `env:"PROTOCOL" env-default:"" env-description:"kafka SASL security protocol"`
+		Enabled          bool     `env:"ENABLED" env-default:"false" env-description:"kafka service enabled"`


should we delete it right away, given we are now quite sure it's probably not gonna be used? :)

Delete what, enabled flag? How are you going to test Kafka in dev environment (without clowder)?

Still not sure what you mean here, rebasing anyeway.

ezr-ondrej · 2023-10-05T09:14:11Z

internal/kafka/kafka.go

@@ -93,18 +100,17 @@ func NewKafkaBroker(ctx context.Context) (Broker, error) {
 		}
 	}

-	if config.Kafka.TlsEnabled && !config.InEphemeralClowder() {
+	if strings.Contains(strings.ToUpper(config.Kafka.SecurityProtocol), "SSL") {


I'd love to have the values in constants, but I guess we can contribute that to https://github.com/RedHatInsights/app-common-go and then use it.
So good for now 👍

Ehm "middleware" ehm.

right xD :)

lzap · 2023-10-06T07:22:32Z

So we got the following info for the field: PLAINTEXT, SSL, SASL_PLAINTEXT, or SASL_SSL I can incorporate this in the help text if necessary.

Signed-off-by: Lukáš Zapletal <[email protected]>

lzap · 2023-10-10T11:03:17Z

Rebased the docs-only change.

ezr-ondrej · 2023-10-10T11:31:02Z

/retest

lzap · 2023-10-10T16:53:25Z

Fails on unexpected backend data that is the error we talked about on scrum. Something is deleting source prior this request. @akhil-jha any ideas?

akhil-jha · 2023-10-10T17:04:54Z

The error here is - image builder couldn't find the image. Becuase it expired. Will have to update the images for the stub. This is not related to the error sources we were talking about :)

lzap · 2023-10-11T10:34:18Z

/retest

ezr-ondrej · 2023-10-11T12:30:11Z

Thanks! 🧡

lzap commented Oct 4, 2023

View reviewed changes

ezr-ondrej reviewed Oct 4, 2023

View reviewed changes

lzap force-pushed the kafka-better-logging branch from 4190439 to ebd8275 Compare October 4, 2023 13:48

ezr-ondrej reviewed Oct 4, 2023

View reviewed changes

ezr-ondrej approved these changes Oct 4, 2023

View reviewed changes

lzap force-pushed the kafka-better-logging branch 3 times, most recently from ae9df73 to bd2617c Compare October 4, 2023 15:48

lzap force-pushed the kafka-better-logging branch from bd2617c to 883efe0 Compare October 4, 2023 16:08

lzap force-pushed the kafka-better-logging branch 2 times, most recently from 7dbb463 to e2aca9c Compare October 5, 2023 07:33

lzap force-pushed the kafka-better-logging branch 2 times, most recently from 8a61f31 to 4d7316b Compare October 5, 2023 07:50

lzap force-pushed the kafka-better-logging branch from 4d7316b to 095e981 Compare October 5, 2023 07:58

ezr-ondrej reviewed Oct 5, 2023

View reviewed changes

chore: improve kafka startup logging

4cccdb5

Signed-off-by: Lukáš Zapletal <[email protected]>

lzap force-pushed the kafka-better-logging branch from 095e981 to 4cccdb5 Compare October 10, 2023 11:03

ezr-ondrej merged commit ac740ed into RHEnVision:main Oct 11, 2023
6 checks passed

lzap deleted the kafka-better-logging branch October 11, 2023 14:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: improve kafka startup logging #696

chore: improve kafka startup logging #696

lzap commented Oct 4, 2023

lzap Oct 4, 2023

ezr-ondrej Oct 4, 2023

lzap commented Oct 4, 2023

ezr-ondrej Oct 4, 2023

ezr-ondrej left a comment

ezr-ondrej commented Oct 4, 2023

lzap commented Oct 4, 2023

lzap commented Oct 4, 2023

lzap commented Oct 4, 2023

lzap commented Oct 5, 2023

lzap commented Oct 5, 2023

lzap commented Oct 5, 2023

ezr-ondrej left a comment

ezr-ondrej Oct 5, 2023

lzap Oct 6, 2023

lzap Oct 10, 2023

ezr-ondrej Oct 5, 2023

lzap Oct 6, 2023

ezr-ondrej Oct 10, 2023

lzap commented Oct 6, 2023

lzap commented Oct 10, 2023

ezr-ondrej commented Oct 10, 2023

lzap commented Oct 10, 2023

akhil-jha commented Oct 10, 2023

lzap commented Oct 11, 2023

ezr-ondrej commented Oct 11, 2023

	logger.Info().Msgf("Setting up Kafka transport: %v TLS:%b Skip:%b CA:%b Auth:%s SASLMech:%s SASLProto:%s",
	logger.Info().Msgf("Setting up Kafka transport: %v TLS:%t Skip:%t CA:%t Auth:%s SASLMech:%s SASLProto:%s",

chore: improve kafka startup logging #696

chore: improve kafka startup logging #696

Conversation

lzap commented Oct 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzap commented Oct 4, 2023

Choose a reason for hiding this comment

ezr-ondrej left a comment

Choose a reason for hiding this comment

ezr-ondrej commented Oct 4, 2023

lzap commented Oct 4, 2023

lzap commented Oct 4, 2023

lzap commented Oct 4, 2023

lzap commented Oct 5, 2023

lzap commented Oct 5, 2023

lzap commented Oct 5, 2023

ezr-ondrej 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

lzap commented Oct 6, 2023

lzap commented Oct 10, 2023

ezr-ondrej commented Oct 10, 2023

lzap commented Oct 10, 2023

akhil-jha commented Oct 10, 2023

lzap commented Oct 11, 2023

ezr-ondrej commented Oct 11, 2023