Skip to content

[SPARK-45541][CORE] Add SSLFactory - #43386

Closed
hasnain-db wants to merge 5 commits into
apache:masterfrom
hasnain-db:spark-tls-factory
Closed

[SPARK-45541][CORE] Add SSLFactory#43386
hasnain-db wants to merge 5 commits into
apache:masterfrom
hasnain-db:spark-tls-factory

Conversation

@hasnain-db

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

As titled - add a factory which supports creating SSL engines, and a corresponding builder for it. This will be used in a follow up PR by the TransportContext and related files to add SSL support.

Why are the changes needed?

We need a mechanism to initialize the appropriate SSL implementation with the configured settings (such as protocol, ciphers, etc) for RPC SSL support.

Does this PR introduce any user-facing change?

No

How was this patch tested?

existing tests. This will be more thoroughly tested in a follow up PR which adds callsites to it. It has been integration tested as part of #42685

Was this patch authored or co-authored using generative AI tooling?

No

@hasnain-db

Copy link
Copy Markdown
ContributorAuthor

test failures looked unrelated, retrying now though.

@hasnain-db

Copy link
Copy Markdown
ContributorAuthor

cc @mridulm@JoshRosen now that tests are green

@mridulmmridulm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took a quick pass through the change, will go over the PR again in more detail later this week.

@mridulmmridulm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of nits.
Can you update to master and retrigger the tests please ? Let us see if we can get a clean build (though the failures are not related)

@hasnain-db

Copy link
Copy Markdown
ContributorAuthor

will re-request review once CI comes back

@mridulmmridulm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, will wait for CI to succeed.

@hasnain-db

Copy link
Copy Markdown
ContributorAuthor

thanks! some jobs failed, rerunning them so hopefully there is a green CI in the morning

@mridulm

Copy link
Copy Markdown
Contributor

The test failure is unrelated to this PR.
Merging to master.

Thanks for fixing this @hasnain-db !

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@hasnain-db@mridulm