feat: modifed kafka to use config received from config-be. #3205

saurav-malani · 2023-04-13T07:51:57Z

Description

Modified kafka to get ssh config from config-BE in addition to from env.
NOTE: getting config via env has to be deprecated in future.

Notion Ticket

https://www.notion.so/rudderstacks/Kafka-with-SSH-control-plane-config-8a25b92171464903a2a5db9e40645290?pvs=4

Security

The code changed/added as part of this pull request won't create any security issues with how the software is being used.

codecov · 2023-04-13T08:16:46Z

Codecov Report

Patch coverage: 79.72% and project coverage change: -0.06 ⚠️

Comparison is base (a4f3b56) 51.95% compared to head (fe30de0) 51.90%.

❗ Current head fe30de0 differs from pull request most recent head a8a3d5a. Consider uploading reports for the commit a8a3d5a to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3205      +/-   ##
==========================================
- Coverage   51.95%   51.90%   -0.06%     
==========================================
  Files         321      322       +1     
  Lines       53194    53425     +231     
==========================================
+ Hits        27639    27732      +93     
- Misses      23910    24036     +126     
- Partials     1645     1657      +12

Impacted Files	Coverage Δ
services/streammanager/kafka/client/config.go	`60.27% <ø> (ø)`
services/controlplane/client.go	`82.09% <62.50%> (-3.41%)`	⬇️
services/streammanager/kafka/kafkamanager.go	`76.67% <86.95%> (+0.90%)`	⬆️
services/streammanager/kafka/client/client.go	`83.33% <100.00%> (-0.20%)`	⬇️

... and 27 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

services/controlplane/client.go

services/streammanager/kafka/kafkamanager.go

services/streammanager/kafka/kafkamanager_test.go

services/controlplane/client.go

fracasula · 2023-04-24T13:47:00Z

services/controlplane/client_test.go

+	require.NoError(t, err)
+	require.Equal(t, controlplane.SSHKeyPair{
+		PrivateKey: "test-private-key",
+		PublicKey:  "test-public-key",


@saurav-malani the public key was not being tested, I added it now.

services/controlplane/client_test.go

fracasula · 2023-04-24T13:48:07Z

services/controlplane/client_test.go

@@ -342,3 +343,61 @@ func TestRetriesTimeout(t *testing.T) {
 		})
 	}
 }
+
+func TestGetDestinationSSHKeyPair(t *testing.T) {


@saurav-malani I converted some test checks to make use of require for a more succint test.

fracasula · 2023-04-25T07:52:12Z

services/streammanager/kafka/kafkamanager.go

@@ -357,18 +366,6 @@ func NewProducer(destination *backendconfig.DestinationT, o common.Opts) (*Produ
 				return nil, fmt.Errorf("[Kafka] invalid SASL type: %w", err)
 			}
 		}
-
-		if destConfig.UseSSH {


@saurav-malani I moved the SSH block out of the if destConfig.SslEnabled { condition like it was before.

fracasula · 2023-04-25T13:02:09Z

services/streammanager/kafka/client/client.go

+			User:            conf.SSHConfig.User,
+			Auth:            []ssh.AuthMethod{ssh.PublicKeys(signer)},
+			Timeout:         conf.DialTimeout,
+			HostKeyCallback: ssh.InsecureIgnoreHostKey(), // skipcq: GSC-G106


@saurav-malani as discussed over standup this is an important change I introduced (cc @lvrach) and it's going to apply to all SSH connections for kafka destinations.

fracasula · 2023-05-02T11:55:48Z

Let's not merge this one until we have the chance to do some proper manual end to end testing on dev.

fracasula

@saurav-malani can you please revert the changes you made after the approvals? It looks like the SSH configuration is again read only if SSL is enabled which is a bug I had already fixed. As discussed yesterday the embedding of the Avro schema ID should be done in a separate PR. I suggest you just drop the recent commits and leave this PR as it was right after the 2 approvals and open another one with just the embedding.

NOTE: See this comment from last week.

fracasula

@saurav-malani I forced push a local copy that I had of the branch before you forced pushed the commits I did. Please leave these changes as they are, we just need to test them. If you need to test other things I suggest you start from this branch and push a copy of it e.g. feat.kafkaOverSshViaCP-test.

fracasula · 2023-05-05T10:55:52Z

@saurav-malani I'll resolve the conflicts, don't worry 👍

saurav-malani changed the title ~~feat: modifed kafka to use config received from config-be in place of env~~ feat: modifed kafka to use config received from config-be. Apr 13, 2023

github-actions bot added the server-team label Apr 13, 2023

saurav-malani force-pushed the feat.kafkaOverSshViaCP branch from 5592f18 to f47d873 Compare April 13, 2023 07:58

saurav-malani marked this pull request as draft April 13, 2023 07:58

github-actions bot added the with tests label Apr 19, 2023

saurav-malani force-pushed the feat.kafkaOverSshViaCP branch from 0e6ebcc to 2d09715 Compare April 19, 2023 12:54

saurav-malani marked this pull request as ready for review April 19, 2023 12:54

$fracasula$

fracasula reviewed Apr 20, 2023

View reviewed changes

services/controlplane/client.go Outdated Show resolved Hide resolved

$fracasula$

fracasula reviewed Apr 20, 2023

View reviewed changes

services/streammanager/kafka/kafkamanager.go Outdated Show resolved Hide resolved

$fracasula$

fracasula reviewed Apr 20, 2023

View reviewed changes

services/streammanager/kafka/kafkamanager_test.go Show resolved Hide resolved

saurav-malani force-pushed the feat.kafkaOverSshViaCP branch from 2d09715 to f47010d Compare April 21, 2023 14:50

saurav-malani requested a review from fracasula April 21, 2023 14:55

$fracasula$

fracasula reviewed Apr 24, 2023

View reviewed changes

services/controlplane/client.go Show resolved Hide resolved

$fracasula$

fracasula reviewed Apr 24, 2023

View reviewed changes

services/controlplane/client.go Show resolved Hide resolved

$fracasula$

fracasula reviewed Apr 24, 2023

View reviewed changes

services/controlplane/client_test.go Show resolved Hide resolved

$fracasula$

fracasula reviewed Apr 24, 2023

View reviewed changes

saurav-malani requested a review from fracasula April 25, 2023 06:50

$fracasula$

fracasula reviewed Apr 25, 2023

View reviewed changes

$fracasula$

fracasula approved these changes May 2, 2023

View reviewed changes

lvrach approved these changes May 4, 2023

View reviewed changes

saurav-malani force-pushed the feat.kafkaOverSshViaCP branch from e11f1e8 to d63ba4b Compare May 5, 2023 07:06

saurav-malani added the Do not merge label May 5, 2023

$fracasula$

fracasula requested changes May 5, 2023

View reviewed changes

saurav-malani force-pushed the feat.kafkaOverSshViaCP branch from d63ba4b to 25b87e7 Compare May 5, 2023 08:33

$@fracasula$ fracasula force-pushed the feat.kafkaOverSshViaCP branch from 25b87e7 to 6c673ef Compare May 5, 2023 10:45

$fracasula$

fracasula approved these changes May 5, 2023

View reviewed changes

$@fracasula$ fracasula removed the Do not merge label May 5, 2023

saurav-malani and others added 10 commits May 5, 2023 13:07

$@fracasula$

modifed kafka to use config received from config-be in place of env

9cdc507

$@fracasula$

minor test modification

6a20678

$@fracasula$

minor modification

42a0026

$@fracasula$

chore: moving request out of retry

f47d738

$@fracasula$

chore: moving SSH out of SASL + minor improvements

49ea4fd

$@fracasula$

chore: always accept any host key + test

3e718ad

$@fracasula$

chore: updating invalid port test

553eced

$@fracasula$

chore: removing duplicated code

5a91601

$@fracasula$

chore: removing duplicated code

384f65d

$@fracasula$

chore: removing duplicated code

a8a3d5a

$@fracasula$ fracasula force-pushed the feat.kafkaOverSshViaCP branch from d4782c5 to a8a3d5a Compare May 5, 2023 11:08

$@fracasula$ fracasula merged commit 0d1c75f into master May 5, 2023
16 checks passed

$@fracasula$ fracasula deleted the feat.kafkaOverSshViaCP branch May 5, 2023 14:33

devops-github-rudderstack mentioned this pull request May 5, 2023

chore: release 1.8.0 #3287

Merged

This was referenced May 5, 2023

chore: prerelease 1.8.0-rc.2 #3288

Merged

chore: prerelease 1.8.0-rc.3 #3292

Merged

chore: prerelease 1.8.0-rc.4 #3299

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: modifed kafka to use config received from config-be. #3205

feat: modifed kafka to use config received from config-be. #3205

saurav-malani commented Apr 13, 2023

codecov bot commented Apr 13, 2023 •

edited

$@fracasula$ fracasula Apr 24, 2023

$@fracasula$ fracasula Apr 24, 2023

$@fracasula$ fracasula Apr 25, 2023

$@fracasula$ fracasula Apr 25, 2023

fracasula commented May 2, 2023

$@fracasula$ fracasula left a comment

$@fracasula$ fracasula left a comment •

edited

fracasula commented May 5, 2023

feat: modifed kafka to use config received from config-be. #3205

feat: modifed kafka to use config received from config-be. #3205

Conversation

saurav-malani commented Apr 13, 2023

Description

Notion Ticket

Security

codecov bot commented Apr 13, 2023 • edited

Codecov Report

fracasula Apr 24, 2023

Choose a reason for hiding this comment

fracasula Apr 24, 2023

Choose a reason for hiding this comment

fracasula Apr 25, 2023

Choose a reason for hiding this comment

fracasula Apr 25, 2023

Choose a reason for hiding this comment

fracasula commented May 2, 2023

fracasula left a comment

Choose a reason for hiding this comment

fracasula left a comment • edited

Choose a reason for hiding this comment

fracasula commented May 5, 2023

codecov bot commented Apr 13, 2023 •

edited

$@fracasula$ fracasula Apr 24, 2023

$@fracasula$ fracasula Apr 24, 2023

$@fracasula$ fracasula Apr 25, 2023

$@fracasula$ fracasula Apr 25, 2023

$@fracasula$ fracasula left a comment

$@fracasula$ fracasula left a comment •

edited