Skip to content

KAFKA-19220: Add tests to ensure the internal configs don't return by public APIs by default #19650

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

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

xijiu
Copy link
Contributor

@xijiu xijiu commented May 6, 2025

Add tests to check whether the results returned by the API createTopics and describeConfigs contain internal configurations.

@github-actions github-actions bot added triage PRs from the community tests Test fixes (including flaky tests) clients small Small PRs labels May 6, 2025
Copy link
Collaborator

@m1a2st m1a2st left a comment

Choose a reason for hiding this comment

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

Thanks @xijiu for this patch, left a little comment

}

private void assertNotContainsInternalConfig(Config config, Map<String, ConfigDef.ConfigKey> configKeyMap,
Set<String> whiteListSet) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you avoid using whiteList, and replace with more neutral terminology.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I have changed it, PTAL

@github-actions github-actions bot removed the triage PRs from the community label May 7, 2025
Copy link
Collaborator

@m1a2st m1a2st left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM

Copy link
Contributor

@frankvicky frankvicky left a comment

Choose a reason for hiding this comment

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

@xijiu: LGTM

Copy link
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

@xijiu thanks for your patch. I leave two minor comments.


assertNotNull(configKey);
if (!ignoreConfigNames.contains(configName)) {
assertFalse(configKey.internalConfig);
Copy link
Member

Choose a reason for hiding this comment

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

Could you please add error message for this assert?

// broker (see org.apache.kafka.common.test.KafkaClusterTestKit.Builder.createNodeConfig()),
// so the API `describeConfigs` will also return these three configurations. However, other internal
// configurations will not be returned
Config brokerConfig = configResourceMap.get(brokerResource);
Copy link
Member

Choose a reason for hiding this comment

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

Could you please verify the internal config for controller node too?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-approved clients small Small PRs tests Test fixes (including flaky tests)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants