You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/10/17 13:47:41 UTC
[GitHub] [pulsar] congbobo184 opened a new pull request, #18074: [improve][schema] Change update schema auth from tenant to produce
congbobo184 opened a new pull request, #18074:
URL: https://github.com/apache/pulsar/pull/18074
discuss: https://lists.apache.org/thread/0ts6josxxq4gt3qw2toxf9jd77d7s6kj
### Motivation
Now, we have two authentications for updating the schema
1. producer and consumer can update the schema using TopicOperation
produce or consume when open autoUpdateSchema
2. pulsar admin uses Tenant authentication
This will produce problems when using different authentications to
update the schema.
### Modification
1. admin use produce permission to update the schema
2. enable autoUpdataSchema, the client can use produce permission to update the schema automatically. When no schema in the topic, consume permission also can first update the schema automatically first
### Does this pull request potentially affect one of the following parts:
*If `yes` was chosen, please highlight the changes*
- Dependencies (does it add or upgrade a dependency): (no)
- The public API: (no)
- The schema: (no)
- The default values of configurations: (no)
- The wire protocol: (no)
- The rest endpoints: (no)
- The admin cli options: (no)
- Anything that affects deployment: (no)
### Documentation
- Does this pull request introduce a new feature? (yes)
- If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
- If a feature is not applicable for documentation, explain why?
- If a feature is not documented yet in this PR, please create a followup issue for adding the documentation
- [x] `doc-not-needed`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Technoboy- closed pull request #18074: [improve][schema] Change update schema auth from tenant to produce
Posted by GitBox <gi...@apache.org>.
Technoboy- closed pull request #18074: [improve][schema] Change update schema auth from tenant to produce
URL: https://github.com/apache/pulsar/pull/18074
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] congbobo184 commented on a diff in pull request #18074: [improve][schema] Change update schema auth from tenant to produce
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on code in PR #18074:
URL: https://github.com/apache/pulsar/pull/18074#discussion_r997827435
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/AdminResource.java:
##########
@@ -742,40 +742,40 @@ protected CompletableFuture<SchemaCompatibilityStrategy> getSchemaCompatibilityS
return validateTopicPolicyOperationAsync(topicName,
PolicyName.SCHEMA_COMPATIBILITY_STRATEGY,
PolicyOperation.READ)
- .thenCompose((__) -> {
- CompletableFuture<SchemaCompatibilityStrategy> future;
- if (config().isTopicLevelPoliciesEnabled()) {
- future = getTopicPoliciesAsyncWithRetry(topicName)
- .thenApply(op -> op.map(TopicPolicies::getSchemaCompatibilityStrategy).orElse(null));
- } else {
- future = CompletableFuture.completedFuture(null);
- }
-
- return future.thenCompose((topicSchemaCompatibilityStrategy) -> {
- if (!SchemaCompatibilityStrategy.isUndefined(topicSchemaCompatibilityStrategy)) {
- return CompletableFuture.completedFuture(topicSchemaCompatibilityStrategy);
- }
- return getNamespacePoliciesAsync(namespaceName).thenApply(policies -> {
- SchemaCompatibilityStrategy schemaCompatibilityStrategy =
- policies.schema_compatibility_strategy;
- if (SchemaCompatibilityStrategy.isUndefined(schemaCompatibilityStrategy)) {
- schemaCompatibilityStrategy = SchemaCompatibilityStrategy.fromAutoUpdatePolicy(
- policies.schema_auto_update_compatibility_strategy);
- if (SchemaCompatibilityStrategy.isUndefined(schemaCompatibilityStrategy)) {
- schemaCompatibilityStrategy = pulsar().getConfig().getSchemaCompatibilityStrategy();
- }
- }
- return schemaCompatibilityStrategy;
- });
- });
- }).whenComplete((__, ex) -> {
+ .thenCompose((__) -> getSchemaCompatibilityStrategyAsyncWithoutAuth()).whenComplete((__, ex) -> {
if (ex != null) {
log.error("[{}] Failed to get schema compatibility strategy of topic {} {}",
clientAppId(), topicName, ex);
}
});
}
+ protected CompletableFuture<SchemaCompatibilityStrategy> getSchemaCompatibilityStrategyAsyncWithoutAuth() {
+ CompletableFuture<SchemaCompatibilityStrategy> future = CompletableFuture.completedFuture(null);
+ if (config().isTopicLevelPoliciesEnabled()) {
+ future = getTopicPoliciesAsyncWithRetry(topicName)
+ .thenApply(op -> op.map(TopicPolicies::getSchemaCompatibilityStrategy).orElse(null));
+ }
+
+ return future.thenCompose((topicSchemaCompatibilityStrategy) -> {
+ if (!SchemaCompatibilityStrategy.isUndefined(topicSchemaCompatibilityStrategy)) {
+ return CompletableFuture.completedFuture(topicSchemaCompatibilityStrategy);
+ }
+ return getNamespacePoliciesAsync(namespaceName).thenApply(policies -> {
+ SchemaCompatibilityStrategy schemaCompatibilityStrategy =
+ policies.schema_compatibility_strategy;
+ if (SchemaCompatibilityStrategy.isUndefined(schemaCompatibilityStrategy)) {
+ schemaCompatibilityStrategy = SchemaCompatibilityStrategy.fromAutoUpdatePolicy(
+ policies.schema_auto_update_compatibility_strategy);
+ if (SchemaCompatibilityStrategy.isUndefined(schemaCompatibilityStrategy)) {
+ schemaCompatibilityStrategy = pulsar().getConfig().getSchemaCompatibilityStrategy();
+ }
+ }
+ return schemaCompatibilityStrategy;
+ });
+ });
+ }
+
Review Comment:
yes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on pull request #18074: [improve][schema] Change update schema auth from tenant to produce
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on PR #18074:
URL: https://github.com/apache/pulsar/pull/18074#issuecomment-1324437629
@congbobo184 Could you please help cherry-pick this PR to branch-2.10 and branch-2.9? It will help users who want to create topics by Admin API, but the tenant admin is required before upgrading to 2.11.0.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codecov-commenter commented on pull request #18074: [improve][schema] Change update schema auth from tenant to produce
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #18074:
URL: https://github.com/apache/pulsar/pull/18074#issuecomment-1281850824
# [Codecov](https://codecov.io/gh/apache/pulsar/pull/18074?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#18074](https://codecov.io/gh/apache/pulsar/pull/18074?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4314b72) into [master](https://codecov.io/gh/apache/pulsar/commit/6c65ca0d8a80bfaaa4d5869e0cea485f5c94369b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6c65ca0) will **increase** coverage by `15.71%`.
> The diff coverage is `77.41%`.
[](https://codecov.io/gh/apache/pulsar/pull/18074?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #18074 +/- ##
=============================================
+ Coverage 34.91% 50.62% +15.71%
- Complexity 5707 8658 +2951
=============================================
Files 607 607
Lines 53396 53388 -8
Branches 5712 5714 +2
=============================================
+ Hits 18644 27029 +8385
+ Misses 32119 23355 -8764
- Partials 2633 3004 +371
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests | `50.62% <77.41%> (+15.71%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pulsar/pull/18074?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../service/SystemTopicBasedTopicPoliciesService.java](https://codecov.io/gh/apache/pulsar/pull/18074/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL1N5c3RlbVRvcGljQmFzZWRUb3BpY1BvbGljaWVzU2VydmljZS5qYXZh) | `62.97% <0.00%> (+11.38%)` | :arrow_up: |
| [.../pulsar/broker/stats/BrokerOperabilityMetrics.java](https://codecov.io/gh/apache/pulsar/pull/18074/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zdGF0cy9Ccm9rZXJPcGVyYWJpbGl0eU1ldHJpY3MuamF2YQ==) | `98.21% <ø> (+5.56%)` | :arrow_up: |
| [...g/apache/pulsar/compaction/CompactedTopicImpl.java](https://codecov.io/gh/apache/pulsar/pull/18074/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NvbXBhY3Rpb24vQ29tcGFjdGVkVG9waWNJbXBsLmphdmE=) | `69.28% <0.00%> (+58.57%)` | :arrow_up: |
| [.../org/apache/pulsar/broker/admin/v2/Namespaces.java](https://codecov.io/gh/apache/pulsar/pull/18074/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9hZG1pbi92Mi9OYW1lc3BhY2VzLmphdmE=) | `57.05% <50.00%> (+49.02%)` | :arrow_up: |
| [...apache/pulsar/proxy/server/DirectProxyHandler.java](https://codecov.io/gh/apache/pulsar/pull/18074/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLXByb3h5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9wdWxzYXIvcHJveHkvc2VydmVyL0RpcmVjdFByb3h5SGFuZGxlci5qYXZh) | `63.63% <50.00%> (ø)` | |
| [...broker/delayed/InMemoryDelayedDeliveryTracker.java](https://codecov.io/gh/apache/pulsar/pull/18074/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9kZWxheWVkL0luTWVtb3J5RGVsYXllZERlbGl2ZXJ5VHJhY2tlci5qYXZh) | `65.00% <75.00%> (+65.00%)` | :arrow_up: |
| [.../org/apache/pulsar/broker/admin/AdminResource.java](https://codecov.io/gh/apache/pulsar/pull/18074/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9hZG1pbi9BZG1pblJlc291cmNlLmphdmE=) | `66.51% <93.33%> (+34.03%)` | :arrow_up: |
| [...pulsar/broker/admin/impl/PersistentTopicsBase.java](https://codecov.io/gh/apache/pulsar/pull/18074/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9hZG1pbi9pbXBsL1BlcnNpc3RlbnRUb3BpY3NCYXNlLmphdmE=) | `52.18% <100.00%> (+40.78%)` | :arrow_up: |
| [.../pulsar/broker/admin/impl/SchemasResourceBase.java](https://codecov.io/gh/apache/pulsar/pull/18074/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9hZG1pbi9pbXBsL1NjaGVtYXNSZXNvdXJjZUJhc2UuamF2YQ==) | `89.89% <100.00%> (+44.44%)` | :arrow_up: |
| [...rg/apache/pulsar/broker/service/BrokerService.java](https://codecov.io/gh/apache/pulsar/pull/18074/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL0Jyb2tlclNlcnZpY2UuamF2YQ==) | `58.04% <100.00%> (+10.03%)` | :arrow_up: |
| ... and [150 more](https://codecov.io/gh/apache/pulsar/pull/18074/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on pull request #18074: [improve][schema] Change update schema auth from tenant to produce
Posted by GitBox <gi...@apache.org>.
nodece commented on PR #18074:
URL: https://github.com/apache/pulsar/pull/18074#issuecomment-1281759096
This PR also should sync to branch-2.9 and branch-2.10.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on a diff in pull request #18074: [improve][schema] Change update schema auth from tenant to produce
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #18074:
URL: https://github.com/apache/pulsar/pull/18074#discussion_r997206530
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiSchemaWithAuthTest.java:
##########
@@ -108,12 +110,17 @@ public void testGetCreateDeleteSchema() throws Exception {
.serviceHttpUrl(brokerUrl != null ? brokerUrl.toString() : brokerUrlTls.toString())
.authentication(AuthenticationToken.class.getName(), CONSUME_TOKEN)
.build();
+
+ PulsarAdmin adminWithProducePermission = PulsarAdmin.builder()
+ .serviceHttpUrl(brokerUrl != null ? brokerUrl.toString() : brokerUrlTls.toString())
+ .authentication(AuthenticationToken.class.getName(), PRODUCE_TOKEN)
+ .build();
admin.topics().grantPermission(topicName, "consumer", EnumSet.of(AuthAction.consume));
admin.topics().grantPermission(topicName, "producer", EnumSet.of(AuthAction.produce));
SchemaInfo si = Schema.BOOL.getSchemaInfo();
- assertThrows(PulsarAdminException.class, () -> adminWithoutPermission.schemas().createSchema(topicName, si));
- adminWithAdminPermission.schemas().createSchema(topicName, si);
+ assertThrows(PulsarAdminException.class, () -> adminWithConsumePermission.schemas().getSchemaInfo(topicName));
+ adminWithProducePermission.schemas().createSchema(topicName, si);
Review Comment:
Please keep the old test, and then append a new test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Technoboy- commented on a diff in pull request #18074: [improve][schema] Change update schema auth from tenant to produce
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on code in PR #18074:
URL: https://github.com/apache/pulsar/pull/18074#discussion_r997677232
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/AdminResource.java:
##########
@@ -742,40 +742,40 @@ protected CompletableFuture<SchemaCompatibilityStrategy> getSchemaCompatibilityS
return validateTopicPolicyOperationAsync(topicName,
PolicyName.SCHEMA_COMPATIBILITY_STRATEGY,
PolicyOperation.READ)
- .thenCompose((__) -> {
- CompletableFuture<SchemaCompatibilityStrategy> future;
- if (config().isTopicLevelPoliciesEnabled()) {
- future = getTopicPoliciesAsyncWithRetry(topicName)
- .thenApply(op -> op.map(TopicPolicies::getSchemaCompatibilityStrategy).orElse(null));
- } else {
- future = CompletableFuture.completedFuture(null);
- }
-
- return future.thenCompose((topicSchemaCompatibilityStrategy) -> {
- if (!SchemaCompatibilityStrategy.isUndefined(topicSchemaCompatibilityStrategy)) {
- return CompletableFuture.completedFuture(topicSchemaCompatibilityStrategy);
- }
- return getNamespacePoliciesAsync(namespaceName).thenApply(policies -> {
- SchemaCompatibilityStrategy schemaCompatibilityStrategy =
- policies.schema_compatibility_strategy;
- if (SchemaCompatibilityStrategy.isUndefined(schemaCompatibilityStrategy)) {
- schemaCompatibilityStrategy = SchemaCompatibilityStrategy.fromAutoUpdatePolicy(
- policies.schema_auto_update_compatibility_strategy);
- if (SchemaCompatibilityStrategy.isUndefined(schemaCompatibilityStrategy)) {
- schemaCompatibilityStrategy = pulsar().getConfig().getSchemaCompatibilityStrategy();
- }
- }
- return schemaCompatibilityStrategy;
- });
- });
- }).whenComplete((__, ex) -> {
+ .thenCompose((__) -> getSchemaCompatibilityStrategyAsyncWithoutAuth()).whenComplete((__, ex) -> {
if (ex != null) {
log.error("[{}] Failed to get schema compatibility strategy of topic {} {}",
clientAppId(), topicName, ex);
}
});
}
+ protected CompletableFuture<SchemaCompatibilityStrategy> getSchemaCompatibilityStrategyAsyncWithoutAuth() {
+ CompletableFuture<SchemaCompatibilityStrategy> future = CompletableFuture.completedFuture(null);
+ if (config().isTopicLevelPoliciesEnabled()) {
+ future = getTopicPoliciesAsyncWithRetry(topicName)
+ .thenApply(op -> op.map(TopicPolicies::getSchemaCompatibilityStrategy).orElse(null));
+ }
+
+ return future.thenCompose((topicSchemaCompatibilityStrategy) -> {
+ if (!SchemaCompatibilityStrategy.isUndefined(topicSchemaCompatibilityStrategy)) {
+ return CompletableFuture.completedFuture(topicSchemaCompatibilityStrategy);
+ }
+ return getNamespacePoliciesAsync(namespaceName).thenApply(policies -> {
+ SchemaCompatibilityStrategy schemaCompatibilityStrategy =
+ policies.schema_compatibility_strategy;
+ if (SchemaCompatibilityStrategy.isUndefined(schemaCompatibilityStrategy)) {
+ schemaCompatibilityStrategy = SchemaCompatibilityStrategy.fromAutoUpdatePolicy(
+ policies.schema_auto_update_compatibility_strategy);
+ if (SchemaCompatibilityStrategy.isUndefined(schemaCompatibilityStrategy)) {
+ schemaCompatibilityStrategy = pulsar().getConfig().getSchemaCompatibilityStrategy();
+ }
+ }
+ return schemaCompatibilityStrategy;
+ });
+ });
+ }
+
Review Comment:
Make this reusable, right？
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Technoboy- merged pull request #18074: [improve][schema] Change update schema auth from tenant to produce
Posted by GitBox <gi...@apache.org>.
Technoboy- merged PR #18074:
URL: https://github.com/apache/pulsar/pull/18074
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org