[GitHub] [pulsar] eolivelli commented on a diff in pull request #15576: [PIP][Authorization] Make Implicit Subscription Permission Configurable

[GitBox <gi...@apache.org>]

eolivelli commented on code in PR #15576:
URL: https://github.com/apache/pulsar/pull/15576#discussion_r872045946


##########
pulsar-common/src/main/java/org/apache/pulsar/client/admin/internal/data/AuthPoliciesImpl.java:
##########
@@ -42,6 +42,10 @@ public final class AuthPoliciesImpl implements AuthPolicies {
     @JsonProperty("subscription_auth_roles")
     private Map<String, Set<String>> subscriptionAuthentication = new TreeMap<>();
 
+    // Default value is set in the builder
+    @JsonProperty(value = "implicit_subscription_auth")
+    private boolean implicitSubscriptionAuth;

Review Comment:
   adding a field with default value "true" is problematic, thinking to the upgrade path.
   This is because on storage (ZK) all the old Policies do not have this field, but we are considering it "true" if it is not present.
   This may make us fall into some subtle problems with clusters upgraded to the latest version.
   
   I suggest to go down these ways:
   1) use Boolean (nullable boolean) and deal explicitly with the case "unknown"
   2) negate the flag, this way the default will be "false" and this value will also be applied in case of null/non existing field (so old cluster upgraded, or updates happening during rolling upgrades of the cluster
   
   I prefer 2) as it is easier to deal with both code changes and also with the migration



-- 
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