Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make AzureProperties w/ shared-key creds serializable #10045

Merged
merged 1 commit into from
Jun 26, 2024

Conversation

snazy
Copy link
Member

@snazy snazy commented Mar 26, 2024

As StorageSharedKeyCredential is not serializable, shared key auth doesn't work with Spark.

@nastra
Copy link
Contributor

nastra commented Apr 29, 2024

@snazy could you add a test please?

@simonykq
Copy link

any eta on when this would be merged?

})
.doesNotThrowAnyException();

byte[] serialized = out.toByteArray();
Copy link
Contributor

@nastra nastra Jun 25, 2024

Choose a reason for hiding this comment

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

it would be good to actually create an instance of AzureProperties and verify that this matches props. Additionally, you can use

public static <T> T roundTripSerialize(T type) throws IOException, ClassNotFoundException {
to do the roundtrip serialization

Copy link
Contributor

Choose a reason for hiding this comment

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

something like

AzureProperties props =
        new AzureProperties(
            ImmutableMap.<String, String>builder()
                .put(ADLS_SAS_TOKEN_PREFIX + "foo", "bar")
                .put(ADLS_CONNECTION_STRING_PREFIX + "foo", "bar")
                .put(ADLS_READ_BLOCK_SIZE, "42")
                .put(ADLS_WRITE_BLOCK_SIZE, "42")
                .put(ADLS_SHARED_KEY_ACCOUNT_NAME, "me")
                .put(ADLS_SHARED_KEY_ACCOUNT_KEY, "secret")
                .build());

    AzureProperties serdedProps = TestHelpers.roundTripSerialize(props);
    assertThat(serdedProps.adlsReadBlockSize()).isEqualTo(props.adlsReadBlockSize());
    assertThat(serdedProps.adlsWriteBlockSize()).isEqualTo(props.adlsWriteBlockSize());
    assertThat(serdedProps.adlsSasTokens()).isEqualTo(props.adlsSasTokens());
    assertThat(serdedProps.namedKeyCreds()).isEqualTo(props.namedKeyCreds());
    assertThat(serdedProps.adlsConnectionStrings()).isEqualTo(props.adlsConnectionStrings());

@snazy snazy force-pushed the azure-props-serializable branch 2 times, most recently from 22629aa to c069f11 Compare June 26, 2024 11:32
As `StorageSharedKeyCredential` is not serializable, shared key auth doesn't work with Spark.
@snazy snazy force-pushed the azure-props-serializable branch from c069f11 to 99c1d2f Compare June 26, 2024 11:43
@nastra nastra merged commit c88e942 into apache:main Jun 26, 2024
45 checks passed
@snazy snazy deleted the azure-props-serializable branch June 26, 2024 14:03
jasonf20 pushed a commit to jasonf20/iceberg that referenced this pull request Aug 4, 2024
…10045)

As `StorageSharedKeyCredential` is not serializable, shared key auth doesn't work with Spark.
zachdisc pushed a commit to zachdisc/iceberg that referenced this pull request Dec 23, 2024
…10045)

As `StorageSharedKeyCredential` is not serializable, shared key auth doesn't work with Spark.
Sign up for free to 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.

3 participants