KAFKA-20921: Make ScramImage mechanism maps unmodifiable by Gimini-3 · Pull Request #23128 · apache/kafka

Gimini-3 · 2026-08-11T06:11:26Z

This change makes the mechanism maps exposed by ScramImage
unmodifiable.

Previously, only the outer mechanisms map was wrapped, while the nested
per-mechanism credential maps remained mutable. A caller could clear a
nested map returned by mechanisms() and change an existing image.
ScramDelta.apply() supplies mutable HashMap instances.

The change:

copies the outer mechanism map and wraps it as unmodifiable;
wraps each nested credential map with Collections.unmodifiableMap
without copying its contents, following the existing metadata ownership
convention;
adds regression coverage that both map levels exposed by
mechanisms() are unmodifiable;
corrects the ScramCredentialData Javadoc description.

During review, defensive copies of the nested maps and
ScramCredentialData byte arrays were withdrawn to keep the change
consistent with the metadata ownership convention. No credential array
behavior is changed.

Related work: KAFKA-19305 mentioned ScramImage immutability, but PR
#19847 only updated ClientQuotaImage and TopicImage.

Validation:

./gradlew :metadata:test :metadata:spotlessCheck (passed)

This contribution is my original work and I license it to the project
under the project's open source license.

Reviewers: Kevin Wu kwu@confluent.io, José Armando García Sancio
jsancio@apache.org

Generated-by: OpenAI Codex

github-actions · 2026-09-10T03:21:32Z

A label of 'needs-attention' was automatically added to this PR in order to raise the
attention of the committers. Once this issue has been triaged, the triage label
should be removed to prevent this automation from happening again.

kevin-wu24

Thanks for the PR @Gimini-3. I left a few comments on metadata/src.

kevin-wu24 · 2026-09-14T18:35:44Z

-        mechanisms = Collections.unmodifiableMap(mechanisms);
+        Map<ScramMechanism, Map<String, ScramCredentialData>> copiedMechanisms = new HashMap<>();
+        mechanisms.forEach((mechanism, credentials) ->
+            copiedMechanisms.put(mechanism, Collections.unmodifiableMap(new HashMap<>(credentials))));


It looks like there is a convention of wrapping maps in unmodifiableMap in image record compact constructors. I suspect the motivation behind this is unmodifiableMap is O(1), and can prevent certain "bad code" from modifying the image at test/runtime. However, it is probably not performant to actually deep copy all of the kafka objects contained in the various ...Image classes. The reason why I believe this is "okay" is below:

The call-sites of all of the metadata sub-images (SCRAM, topics, cluster, etc.) compact constructors are all on the single-threaded MetadataLoader's event queue, which is the only thread the writing/construction of any given MetadataImage can occur. There is no risk of concurrent access there. There may be things that read a MetadataImage's data which live on other threads (e.g. request handling), but each ...Image is a record to make its internal data immutable. Code that reads the image and modifies any underlying image data is obviously wrong, because the image represents on-disk metadata, that can only be changed via KRaft replication.

This code is not doing a true deep copy of each ScramCredentialData object. That is fine, because this method is O(n) where n is the size of the larger map. This is much cheaper than linear with the number of ScramCredentialData byte arrays probably. However, it makes the included changes in ScramCredentialData redundant IMO.

Thanks for the review. I narrowed this PR to mechanism maps and reverted the credential-array copies. I kept the snapshot copies of the outer and nested maps, with a regression test covering changes to constructor inputs as well as writes through mechanisms(). The PR title and description now reflect that narrower scope.

kevin-wu24 · 2026-09-14T18:43:04Z

    public UserScramCredentialRecord toRecord(
        String userName,
        ScramMechanism mechanism
    ) {
        return new UserScramCredentialRecord().
            setName(userName).
            setMechanism(mechanism.type()).
-            setSalt(salt).
-            setStoredKey(storedKey).
-            setServerKey(serverKey).
+            setSalt(salt()).
+            setStoredKey(storedKey()).
+            setServerKey(serverKey()).
            setIterations(iterations);
    }

    public ScramCredential toCredential() {
-        return new ScramCredential(salt, storedKey, serverKey, iterations);
+        return new ScramCredential(salt(), storedKey(), serverKey(), iterations);


Both of these methods, which are the only callers of the newly introduced deep-copy getters, are only called on the single-threaded image publishing pipeline. IMO, they can be removed.

This object is a in-memory representation of a binary metadata record on disk. Any modification of this object by the code downstream of toCredential() does not make sense.

Thanks, @kevin-wu24. I’ve reverted the defensive array copies in ScramCredentialData, including the accessor and conversion changes, and removed their regression tests. This PR is now scoped to ScramImage mechanism maps; credential array behavior is unchanged. I updated the PR title and description to reflect that.

Generated-by: OpenAI Codex

kevin-wu24

Thanks for the update @Gimini-3. Left another review:

kevin-wu24 · 2026-09-16T15:28:29Z

+        Map<ScramMechanism, Map<String, ScramCredentialData>> copiedMechanisms = new HashMap<>();
+        mechanisms.forEach((mechanism, credentials) ->
+            copiedMechanisms.put(mechanism, Collections.unmodifiableMap(new HashMap<>(credentials))));


Your current implementation is copying over each inner-map's contents into a new HashMap. For the same reasons as previously discussed, I don't think this is necessary.

The metadata image publishing pipeline only has one image at a time. When deltas are applied to the current image, which changes the contents, a new image is constructed. Code that tries to modify MetadataImage contents outside of MetadataLoader/BatchLoader/Delta is incorrect.

Suggested change

Map<ScramMechanism, Map<String, ScramCredentialData>> copiedMechanisms = new HashMap<>();

mechanisms.forEach((mechanism, credentials) ->

copiedMechanisms.put(mechanism, Collections.unmodifiableMap(new HashMap<>(credentials))));

Map<ScramMechanism, Map<String, ScramCredentialData>> copiedMechanisms = new HashMap<>(mechanisms.size());

mechanisms.forEach((mechanism, credentials) ->

copiedMechanisms.put(mechanism, Collections.unmodifiableMap(credentials)));

Thanks, @kevin-wu24. I applied the suggested implementation in fbe387f5c2: the outer map is pre-sized and copied, while each nested map is wrapped directly with Collections.unmodifiableMap without copying its contents. I also updated the PR title and description to describe unmodifiable maps rather than immutable snapshots.

kevin-wu24 · 2026-09-16T15:43:49Z

+        mechanisms.clear();
+        credentials.clear();
+        assertEquals(
+            Map.of(ScramMechanism.SCRAM_SHA_256, Map.of("alice", credential)),
+            image.mechanisms());


This passes because we copy the nested map's contents.

Agreed. I removed the credentials.clear() assertion and renamed the test to testMechanismMapsAreUnmodifiable. It now verifies the copied outer map and that both map levels exposed by mechanisms() reject mutation. :metadata:test and :metadata:spotlessCheck pass.

Generated-by: OpenAI Codex

jsancio

Thanks for the fixes @Gimini-3

jsancio · 2026-09-22T17:58:10Z

+        Map<ScramMechanism, Map<String, ScramCredentialData>> copiedMechanisms = new HashMap<>(mechanisms.size());
+        mechanisms.forEach((mechanism, credentials) ->
+            copiedMechanisms.put(mechanism, Collections.unmodifiableMap(credentials)));
+        mechanisms = Collections.unmodifiableMap(copiedMechanisms);


Please use Java's built in for loop:

public ScramImage { Map<ScramMechanism, Map<String, ScramCredentialData>> wrapped = new HashMap<>(mechanisms.size()); for (var entry : mechanisms.entrySet()) { wrapped.put(entry.getKey(), Collections.unmodifiableMap(entry.getValue())); } mechanisms = Collections.unmodifiableMap(wrapped); }

Thanks, José. I replaced the forEach lambda with the suggested for (var entry : mechanisms.entrySet()) loop in c4efa35.

jsancio · 2026-09-22T18:08:50Z

+
+        ScramImage image = new ScramImage(mechanisms);
+
+        mechanisms.clear();


With this call to clear() the test passes because it copies the root node. This is not required because the expectation is that ScramDelta is passing the ownership of the maps to the ScramImage.

The important thing to check is that the users of ScramImage cannot update the internal maps like you are doing in the rest of the test.

In short, you can remove this call to clear() and the related checks.

Agreed. I removed mechanisms.clear() and its related assertion in c4efa35. The test now checks only that callers cannot mutate either map level through image.mechanisms(). ScramImageTest and :metadata:spotlessCheck pass.

Generated-by: OpenAI Codex

KAFKA-20921: Make SCRAM metadata deeply immutable

cf094f3

Generated-by: OpenAI Codex

github-actions Bot added triage PRs from the community kraft labels Aug 11, 2026

Gimini-3 marked this pull request as ready for review August 11, 2026 06:16

github-actions Bot added the needs-attention label Sep 10, 2026

kevin-wu24 reviewed Sep 14, 2026

View reviewed changes

github-actions Bot removed needs-attention triage PRs from the community labels Sep 15, 2026

KAFKA-20921: Scope SCRAM immutability fix to mechanism maps

79fde90

Generated-by: OpenAI Codex

github-actions Bot added the small Small PRs label Sep 15, 2026

Gimini-3 changed the title ~~KAFKA-20921: Make SCRAM metadata deeply immutable~~ KAFKA-20921: Make ScramImage mechanism maps immutable snapshots Sep 15, 2026

kevin-wu24 reviewed Sep 16, 2026

View reviewed changes

KAFKA-20921: Wrap SCRAM credential maps without copying

fbe387f

Generated-by: OpenAI Codex

Gimini-3 changed the title ~~KAFKA-20921: Make ScramImage mechanism maps immutable snapshots~~ KAFKA-20921: Make ScramImage mechanism maps unmodifiable Sep 16, 2026

kevin-wu24 approved these changes Sep 17, 2026

View reviewed changes

jsancio reviewed Sep 22, 2026

View reviewed changes

KAFKA-20921: Address ScramImage review feedback

c4efa35

Generated-by: OpenAI Codex


		ScramImage image = new ScramImage(mechanisms);

		mechanisms.clear();

Conversation

Gimini-3 commented Aug 11, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Sep 10, 2026

Uh oh!

kevin-wu24 left a comment

Choose a reason for hiding this comment

Uh oh!

kevin-wu24 Sep 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevin-wu24 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsancio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Gimini-3 commented Aug 11, 2026 •

edited by github-actions Bot

Loading

kevin-wu24 Sep 14, 2026 •

edited

Loading