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

KSQL-12955 | Introduce KsqlResourceExtensions for plugins configured externally. #10665

Open
wants to merge 5 commits into
base: 7.6.x
Choose a base branch
from

Conversation

pbadani
Copy link
Member

@pbadani pbadani commented Jan 27, 2025

Description

Introduce KsqlResourceExtensions for plugins configured externally.

Testing done

Describe the testing strategy. Unit and integration tests are expected for any behavior changes.

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")
  • Do these changes have compatibility implications for rollback? If so, ensure that the ksql command version is bumped.

@pbadani pbadani requested a review from a team as a code owner January 27, 2025 10:16
@confluent-cla-assistant
Copy link

🎉 All Contributor License Agreements have been signed. Ready to merge.
Please push an empty commit if you would like to re-run the checks to verify CLA status for all contributors.

@pbadani pbadani requested a review from hrishabhg January 27, 2025 10:17
@airlock-confluentinc airlock-confluentinc bot force-pushed the pbadani/KSQL-12955 branch 3 times, most recently from 778c81b to fb2753f Compare January 27, 2025 13:04
Copy link
Member

@hrishabhg hrishabhg left a comment

Choose a reason for hiding this comment

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

Refactoring is looking good.

I would strongly recommend to move all FIPS logic out from this repo.

@@ -1792,6 +1792,10 @@ public Map<String, Object> getConsumerClientConfigProps() {
return Collections.unmodifiableMap(map);
}

public boolean enableFips() {
return getBoolean(ConfluentConfigs.ENABLE_FIPS_CONFIG);
Copy link
Member

Choose a reason for hiding this comment

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

If we can get rid of ConfluentConfigs, that would be better and will help removing internal depedency and ce-kafka dependency. We can have our own flag also.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we have to get rid of it. I have introduced a new config with the same name.
The only dependency left now is ConfluentConfigs.buildFipsValidator()

import io.confluent.ksql.rest.server.KsqlRestConfig;
import io.confluent.ksql.util.KsqlConfig;

public interface KsqlResourceContext {
Copy link
Member

Choose a reason for hiding this comment

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

any benefit of building an interface? Avoid if there is exactly one implementation and in future also, there is no rationale for multiple implementations.

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea behind this is to encapsulate a collection of objects that are useful to the ResourceExtension in future.


public interface KsqlResourceContext {

KsqlConfig ksqlConfig();
Copy link
Member

Choose a reason for hiding this comment

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

Since KsqlConfig and KsqlRestConfig are already available, is an additional Context class necessary? If not, it should be avoided.

if (getString(KSQL_RESOURCE_EXTENSION).isEmpty()) {
return Collections.emptyList();
}
return getConfiguredInstances(KSQL_RESOURCE_EXTENSION, KsqlResourceExtension.class);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return getConfiguredInstances(KSQL_RESOURCE_EXTENSION, KsqlResourceExtension.class);
return getConfiguredInstances(KSQL_RESOURCE_EXTENSIONS, KsqlResourceExtension.class);

we can take KSQL_RESOURCE_EXTENSIONS as comma separated or we can allow prefix based ksql.resource.extension.fipsValidator.class=<className> with ksql.resource.extension.fipsValidator.enable etc.

"ksql.resource.extension.class";
private static final String KSQL_RESOURCE_EXTENSION_DEFAULT = "";
private static final String KSQL_RESOURCE_EXTENSION_DOC =
"A list of KsqlResourceExtension implementations to register with ksqlDB server.";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"A list of KsqlResourceExtension implementations to register with ksqlDB server.";
"A list of KsqlResourceExtension implementations to be registered with the ksqlDB server."

@@ -444,6 +445,12 @@ public class KsqlRestConfig extends AbstractConfig {
KSQL_COMMAND_TOPIC_MIGRATION_MIGRATING
);

public static final String KSQL_RESOURCE_EXTENSION =
Copy link
Member

Choose a reason for hiding this comment

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

one extension or allowing multiple extensions?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is for multiple extensions.

@@ -379,6 +380,16 @@ public void shouldConfigureRocksDBConfigSetter() {
verify(rocksDBConfigSetterHandler).accept(ksqlConfig);
}

@Test(expected = KsqlException.class)
public void shouldFailIfFipsValidationEnabledButNotConfigured() {
Copy link
Member

Choose a reason for hiding this comment

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

We can make a generic test.

shouldFailIfExtensionIsEnabledButNotConfigured

if (KsqlConstants.FIPS_VALIDATOR
.equals(resourceExtension.getClass().getCanonicalName())) {
isFipsValidatorConfigured = true;
}
Copy link
Member

Choose a reason for hiding this comment

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

we can get rid of FIPS Specific checks. We can move tests related to that to confluent-security-plugins itself.

@pbadani pbadani requested a review from hrishabhg January 31, 2025 10:05
@airlock-confluentinc airlock-confluentinc bot force-pushed the pbadani/KSQL-12955 branch 3 times, most recently from 84e811b to 2b66ad6 Compare February 3, 2025 05:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants