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

[ZOOKEEPER-1525] Plumb ZooKeeperServer object into auth plugins #84

Closed
wants to merge 24 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@Randgalt
Member

Randgalt commented Oct 4, 2016

Based on patch work from https://issues.apache.org/jira/browse/ZOOKEEPER-1525

Created ServerAuthenticationProvider which has a method to accept the ZooKeeper
server so that auth can be done using values in the ZK database. As this is a new
interface, existing implementations aren't affected helping backward compatibility

Based on patch work from https://issues.apache.org/jira/browse/ZOOKEE…
…PER-1525

Created ServerAuthenticationProvider which has a method to accept the ZooKeeper
server so that auth can be done using values in the ZK database. As this is a new
interface, existing implementations aren't affected helping backward compatibility
@@ -51,7 +47,7 @@
final public static Object me = new Object();
private static final Logger LOG = LoggerFactory.getLogger(ServerCnxn.class);
protected ArrayList<Id> authInfo = new ArrayList<Id>();
private Set<Id> authInfo = Collections.newSetFromMap(new ConcurrentHashMap<Id, Boolean>());

This comment has been minimized.

@Randgalt

Randgalt Oct 19, 2016

Member

@fpj this is a more aggressive change but one for the better. 1) there's no good reason to have this field be protected; 2) it was defined as a List but used as a Set - so correct that; 3) make it concurrent/thread safe

@Randgalt

Randgalt Oct 19, 2016

Member

@fpj this is a more aggressive change but one for the better. 1) there's no good reason to have this field be protected; 2) it was defined as a List but used as a Set - so correct that; 3) make it concurrent/thread safe

This comment has been minimized.

@fpj

fpj Oct 29, 2016

Contributor

Ok.

@fpj

fpj Oct 29, 2016

Contributor

Ok.

@fpj

This comment has been minimized.

Show comment
Hide comment
@fpj

fpj Oct 19, 2016

Contributor

@Randgalt Here are my current thoughts on the abstraction discussion. The main issue is that AuthenticationProvider implementations take no constructor parameter, and we are trying to introduce one that requires an input parameter. My two main goals are:

1- To have getProvider to return a base class rather than wrapping
2- To have a more general mechanism to pass parameters in the case future changes require other parameters

I suggest we create a base class that we call BaseAuthenticationProvider. BaseAuthenticationProvider is the current AuthenticationProvider without the matches call. Both ServerAuthenticationProvider and AuthenticationProvider extend BaseAuthenticationProvider. The getProvider call should also get a set of generic parameters that it passes to the provider implementations. Implementations may choose to ignore or use any of the parameters.

You suggest passing a set of generic parameters with a metadata map. As I understand it, it would be a map from a BaseAuthenticationProvider class to a set of objects or some form of container that is provider specific.

Let me know if this makes sense to you.

Contributor

fpj commented Oct 19, 2016

@Randgalt Here are my current thoughts on the abstraction discussion. The main issue is that AuthenticationProvider implementations take no constructor parameter, and we are trying to introduce one that requires an input parameter. My two main goals are:

1- To have getProvider to return a base class rather than wrapping
2- To have a more general mechanism to pass parameters in the case future changes require other parameters

I suggest we create a base class that we call BaseAuthenticationProvider. BaseAuthenticationProvider is the current AuthenticationProvider without the matches call. Both ServerAuthenticationProvider and AuthenticationProvider extend BaseAuthenticationProvider. The getProvider call should also get a set of generic parameters that it passes to the provider implementations. Implementations may choose to ignore or use any of the parameters.

You suggest passing a set of generic parameters with a metadata map. As I understand it, it would be a map from a BaseAuthenticationProvider class to a set of objects or some form of container that is provider specific.

Let me know if this makes sense to you.

@Randgalt

This comment has been minimized.

Show comment
Hide comment
@Randgalt

Randgalt Oct 19, 2016

Member

@fpj I don't see how this can be done without breaking existing code. If we introduce a new base class then all the current AuthenticationProviders have to change. Or, we'd have to do runtime instanceof checks or something to differentiate which is a mistake and error prone. How do you propose to mitigate this?

Member

Randgalt commented Oct 19, 2016

@fpj I don't see how this can be done without breaking existing code. If we introduce a new base class then all the current AuthenticationProviders have to change. Or, we'd have to do runtime instanceof checks or something to differentiate which is a mistake and error prone. How do you propose to mitigate this?

@fpj

This comment has been minimized.

Show comment
Hide comment
@fpj

fpj Oct 19, 2016

Contributor

@Randgalt actually, I'm thinking that if we get to pass a set of parameters generically to an AuthenticationProvider, then we don't even need new provider interfaces. If I'm not mistaken, all we need to do is to pass the metadata we have discussed as a constructor parameter and let the AuthenticationProvider implementation deal with the parameters internally. Does it work?

Contributor

fpj commented Oct 19, 2016

@Randgalt actually, I'm thinking that if we get to pass a set of parameters generically to an AuthenticationProvider, then we don't even need new provider interfaces. If I'm not mistaken, all we need to do is to pass the metadata we have discussed as a constructor parameter and let the AuthenticationProvider implementation deal with the parameters internally. Does it work?

@Randgalt

This comment has been minimized.

Show comment
Hide comment
@Randgalt

Randgalt Oct 19, 2016

Member

@fpj Unfortunately not. All current versions directly implement AuthenticationProvider. We're not at Java 8 so we can't add default methods. Any change at all to AuthenticationProvider will break existing code.

Member

Randgalt commented Oct 19, 2016

@fpj Unfortunately not. All current versions directly implement AuthenticationProvider. We're not at Java 8 so we can't add default methods. Any change at all to AuthenticationProvider will break existing code.

@fpj

This comment has been minimized.

Show comment
Hide comment
@fpj

fpj Oct 19, 2016

Contributor

yeah, I realized that after sending the comment, thinking some more...

Contributor

fpj commented Oct 19, 2016

yeah, I realized that after sending the comment, thinking some more...

@Randgalt

This comment has been minimized.

Show comment
Hide comment
@Randgalt

Randgalt Oct 20, 2016

Member

@fpj that's why I added the Wrapper class. I don't see any way around it. It solves the problem nicely.

Member

Randgalt commented Oct 20, 2016

@fpj that's why I added the Wrapper class. I don't see any way around it. It solves the problem nicely.

@fpj

This comment has been minimized.

Show comment
Hide comment
@fpj

fpj Oct 21, 2016

Contributor

My main concern here is with the generality of the approach. I think that when we did the AuthenticationProvider stuff originally, we were thinking that we wouldn't need parameters for the constructor of provider implementations or new parameters to the matches method.

I'm with you that your wrapper proposal fixes the issue for ServerAuthenticationProvider, but I'm concerned that in the future we might realize that we need some other parameters for a different class of AuthenticationProvider implementations, and we shove them into the getProvider signature again. We will also need some more wrapping because we can only return one type in getProvider, we make a single call to matches in PrepRequestProcessor, and we don't want to do instanceof all the time.

At a high level, we need two things:

  1. A signature for matches that serves both implementations of AuthenticationProvider and implementations of ServerAuthenticationProvider.
  2. A generic way to pass parameters to build AuthenticationProvider implementations.

Assuming that the above makes sense, one possibility is that we change the signature of matches to take a generic set of parameters. That's not great either because once you generalize, you're opening up for errors. So we are left with pushing the problem for later, if we have ever the need of adding another provider interface, or generalizing and opening up for errors.

Contributor

fpj commented Oct 21, 2016

My main concern here is with the generality of the approach. I think that when we did the AuthenticationProvider stuff originally, we were thinking that we wouldn't need parameters for the constructor of provider implementations or new parameters to the matches method.

I'm with you that your wrapper proposal fixes the issue for ServerAuthenticationProvider, but I'm concerned that in the future we might realize that we need some other parameters for a different class of AuthenticationProvider implementations, and we shove them into the getProvider signature again. We will also need some more wrapping because we can only return one type in getProvider, we make a single call to matches in PrepRequestProcessor, and we don't want to do instanceof all the time.

At a high level, we need two things:

  1. A signature for matches that serves both implementations of AuthenticationProvider and implementations of ServerAuthenticationProvider.
  2. A generic way to pass parameters to build AuthenticationProvider implementations.

Assuming that the above makes sense, one possibility is that we change the signature of matches to take a generic set of parameters. That's not great either because once you generalize, you're opening up for errors. So we are left with pushing the problem for later, if we have ever the need of adding another provider interface, or generalizing and opening up for errors.

@Randgalt

This comment has been minimized.

Show comment
Hide comment
@Randgalt

Randgalt Oct 21, 2016

Member

@fpj perfection is the enemy of the good right? The current implementation has worked well for a long time. I'd rather see us pick a reasonable set of arguments now that try to have something that will work for a possible future. I think removing the ZookeeperServer from the getProvider() is actually a better solution that what we have here now. I'll work on and push that. Let's discuss after I've refined this a bit.

Member

Randgalt commented Oct 21, 2016

@fpj perfection is the enemy of the good right? The current implementation has worked well for a long time. I'd rather see us pick a reasonable set of arguments now that try to have something that will work for a possible future. I think removing the ZookeeperServer from the getProvider() is actually a better solution that what we have here now. I'll work on and push that. Let's discuss after I've refined this a bit.

Reworked ServerAuthenticationProvider so that it's an abstract class.…
… This provides a clearer distinction between the two

AuthenticationProviders and allows subclasses of ServerAuthenticationProvider to not have to implement the older matches() and
handleAuthentication(). Additionally, removed the method to set the ZooKeeperServer in favor of passing it as a parameter as
needed.
@Randgalt

This comment has been minimized.

Show comment
Hide comment
@Randgalt

Randgalt Oct 23, 2016

Member

@fpj Let me know what you think of this: I reworked ServerAuthenticationProvider so that it's an abstract class. This provides a clearer distinction between the two AuthenticationProviders and allows subclasses of ServerAuthenticationProvider to not have to implement the older matches() and handleAuthentication(). Additionally, removed the method to set the ZooKeeperServer in favor of passing it as a parameter as needed.

Member

Randgalt commented Oct 23, 2016

@fpj Let me know what you think of this: I reworked ServerAuthenticationProvider so that it's an abstract class. This provides a clearer distinction between the two AuthenticationProviders and allows subclasses of ServerAuthenticationProvider to not have to implement the older matches() and handleAuthentication(). Additionally, removed the method to set the ZooKeeperServer in favor of passing it as a parameter as needed.

@fpj

This comment has been minimized.

Show comment
Hide comment
@fpj

fpj Oct 24, 2016

Contributor

@Randgalt It does look better, thanks for reworking it. Let me make another pass pro detail, but I think the general approach looks good.

Contributor

fpj commented Oct 24, 2016

@Randgalt It does look better, thanks for reworking it. Let me make another pass pro detail, but I think the general approach looks good.

@Randgalt

This comment has been minimized.

Show comment
Hide comment
@Randgalt

Randgalt Oct 27, 2016

Member

@fpj Any more thoughts on this Flavio?

Member

Randgalt commented Oct 27, 2016

@fpj Any more thoughts on this Flavio?

@hanm

This comment has been minimized.

Show comment
Hide comment
@hanm

hanm Oct 27, 2016

Contributor

(Not particularly related to the logic of this PR) Might be better to do a git squash to consolidate 16 commits into a single commit to keep the commit history clean - I've seen most projects operate that way.

Contributor

hanm commented Oct 27, 2016

(Not particularly related to the logic of this PR) Might be better to do a git squash to consolidate 16 commits into a single commit to keep the commit history clean - I've seen most projects operate that way.

</para>
<programlisting>
public abstract class ServerAuthenticationProvider implements AuthenticationProvider {

This comment has been minimized.

@fpj

fpj Oct 29, 2016

Contributor

Indentation? Not sure if it matters within the listing block.

@fpj

fpj Oct 29, 2016

Contributor

Indentation? Not sure if it matters within the listing block.

This comment has been minimized.

@Randgalt

Randgalt Oct 29, 2016

Member

I'm copying the other parts of the zookeeperProgrammers.xml here. I built the docs and it looks good.

@Randgalt

Randgalt Oct 29, 2016

Member

I'm copying the other parts of the zookeeperProgrammers.xml here. I built the docs and it looks good.

This comment has been minimized.

@fpj

fpj Nov 15, 2016

Contributor

Ok.

@fpj

fpj Nov 15, 2016

Contributor

Ok.

Show outdated Hide outdated src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java Outdated
@@ -51,7 +47,7 @@
final public static Object me = new Object();
private static final Logger LOG = LoggerFactory.getLogger(ServerCnxn.class);
protected ArrayList<Id> authInfo = new ArrayList<Id>();
private Set<Id> authInfo = Collections.newSetFromMap(new ConcurrentHashMap<Id, Boolean>());

This comment has been minimized.

@fpj

fpj Oct 29, 2016

Contributor

Ok.

@fpj

fpj Oct 29, 2016

Contributor

Ok.

Show outdated Hide outdated src/java/test/org/apache/zookeeper/test/KeyAuthClientTest.java Outdated
Show outdated Hide outdated src/java/test/org/apache/zookeeper/test/KeyAuthClientTest.java Outdated
@Test
public void testAuth() throws Exception {
// NOTE: the tests need to run in-order, and older versions of

This comment has been minimized.

@fpj

fpj Oct 29, 2016

Contributor

I think it is fine if you have a sequence of steps in your test case, but I really see this as a single test case with multiple steps. I suggest we remove the test prefix from the methods to make clear that they aren't independent test cases.

@fpj

fpj Oct 29, 2016

Contributor

I think it is fine if you have a sequence of steps in your test case, but I really see this as a single test case with multiple steps. I suggest we remove the test prefix from the methods to make clear that they aren't independent test cases.

This comment has been minimized.

@Randgalt

Randgalt Nov 7, 2016

Member

done

@Randgalt

Randgalt Nov 7, 2016

Member

done

zk.setData("/abc", "testData1".getBytes(), -1);
zk.create("/key", null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT);
zk.setData("/key", "5".getBytes(), -1);
Thread.sleep(1000);

This comment has been minimized.

@fpj

fpj Oct 29, 2016

Contributor

I'm not sure why we need this sleep here.

@fpj

fpj Oct 29, 2016

Contributor

I'm not sure why we need this sleep here.

This comment has been minimized.

@Randgalt

Randgalt Nov 7, 2016

Member

Me neither - I didn't write this code.

@Randgalt

Randgalt Nov 7, 2016

Member

Me neither - I didn't write this code.

This comment has been minimized.

@eribeiro

eribeiro Nov 8, 2016

Contributor

Only thing I can suppose (and please correct me if I am saying something utterly stupid) is that this sleep was inserted in the hopes of giving some time to ZK commit the changes. Naive but yet (shrug)...

Well, I guess we can either remove it or replace it with a zk.getData("/key");, right?

@eribeiro

eribeiro Nov 8, 2016

Contributor

Only thing I can suppose (and please correct me if I am saying something utterly stupid) is that this sleep was inserted in the hopes of giving some time to ZK commit the changes. Naive but yet (shrug)...

Well, I guess we can either remove it or replace it with a zk.getData("/key");, right?

This comment has been minimized.

@Randgalt

Randgalt Nov 8, 2016

Member

These types of sleeps are all over the ZK tests (and Curator for that matter).

@Randgalt

Randgalt Nov 8, 2016

Member

These types of sleeps are all over the ZK tests (and Curator for that matter).

This comment has been minimized.

@eribeiro

eribeiro Nov 8, 2016

Contributor

So, a question for a future ticket (IMHO). 🌵

@eribeiro

eribeiro Nov 8, 2016

Contributor

So, a question for a future ticket (IMHO). 🌵

new HashMap<>();
//VisibleForTesting
public static void reset() {

This comment has been minimized.

@fpj

fpj Oct 29, 2016

Contributor

Is this used anywhere?

@fpj

fpj Oct 29, 2016

Contributor

Is this used anywhere?

This comment has been minimized.

@Randgalt

Randgalt Oct 29, 2016

Member

I use it in the external code that I'm writing that uses this feature. I can see it being useful to others as well. Without it it's very difficult to write unit tests using custom auth plugins.

@Randgalt

Randgalt Oct 29, 2016

Member

I use it in the external code that I'm writing that uses this feature. I can see it being useful to others as well. Without it it's very difficult to write unit tests using custom auth plugins.

Show outdated Hide outdated .../main/org/apache/zookeeper/server/auth/ServerAuthenticationProvider.java Outdated
Show outdated Hide outdated ...main/org/apache/zookeeper/server/auth/WrappedAuthenticationProvider.java Outdated
@fpj

This comment has been minimized.

Show comment
Hide comment
@fpj

fpj Oct 29, 2016

Contributor

@Randgalt My comments are pretty minor, but it will give it a chance to QA to test this PR.

Contributor

fpj commented Oct 29, 2016

@Randgalt My comments are pretty minor, but it will give it a chance to QA to test this PR.

@Randgalt

This comment has been minimized.

Show comment
Hide comment
@Randgalt

Randgalt Oct 29, 2016

Member

@fpj I've addressed and pushed changes for all your comments.

Member

Randgalt commented Oct 29, 2016

@fpj I've addressed and pushed changes for all your comments.

@Randgalt

This comment has been minimized.

Show comment
Hide comment
@Randgalt

Randgalt Nov 1, 2016

Member

@fpj any updates on this?

Member

Randgalt commented Nov 1, 2016

@fpj any updates on this?

@fpj

This comment has been minimized.

Show comment
Hide comment
@fpj

fpj Nov 2, 2016

Contributor

@Randgalt I'm sorry that QA isn't working for pull requests yet, could please create a diff patch and upload to the jira so that we can get QA to run on it?

Contributor

fpj commented Nov 2, 2016

@Randgalt I'm sorry that QA isn't working for pull requests yet, could please create a diff patch and upload to the jira so that we can get QA to run on it?

@Randgalt

This comment has been minimized.

Show comment
Hide comment
@Randgalt

Randgalt Nov 2, 2016

Member

latest patch added

Member

Randgalt commented Nov 2, 2016

latest patch added

@lvfangmin

This comment has been minimized.

Show comment
Hide comment
@lvfangmin

lvfangmin Nov 9, 2016

Generally looks good to me, but I have the same concern as @fpj, maybe in the future the newly added auth needs additional parameters, then we have to add another wrapper with new interface to keep the old ones backward compatible. I also agree a general metadata map is error-prone, but I think we can have a compromise solution between explicit parameters and general metadata map:

Create a class (e.g. AuthData) with the explicit parameters(zks, cnxn, path, perm, setAcls) in it, which has the get and set method, and use this class in the newly added ServerAuthenticationProvider interface. In the future if we need additional parameter, we only need to change this class, no interface and backward compatible issues will be involved. What do you think @Randgalt @fpj?

lvfangmin commented Nov 9, 2016

Generally looks good to me, but I have the same concern as @fpj, maybe in the future the newly added auth needs additional parameters, then we have to add another wrapper with new interface to keep the old ones backward compatible. I also agree a general metadata map is error-prone, but I think we can have a compromise solution between explicit parameters and general metadata map:

Create a class (e.g. AuthData) with the explicit parameters(zks, cnxn, path, perm, setAcls) in it, which has the get and set method, and use this class in the newly added ServerAuthenticationProvider interface. In the future if we need additional parameter, we only need to change this class, no interface and backward compatible issues will be involved. What do you think @Randgalt @fpj?

Move ServerAuthenticationProvider args into container classes so that…
… this can be upgraded more easily in the future without resorting more wrappers
@Randgalt

This comment has been minimized.

Show comment
Hide comment
@Randgalt

Randgalt Nov 10, 2016

Member

OK - I've put the args into container classes. Let me know what you think.

Member

Randgalt commented Nov 10, 2016

OK - I've put the args into container classes. Let me know what you think.

@fpj

This comment has been minimized.

Show comment
Hide comment
@fpj

fpj Nov 15, 2016

Contributor

@Randgalt thanks for the recent changes, they look fine for me, except that we didn't get QA to run on it.

@lvfangmin there are a few levels of indirection here, which doesn't make it look very pretty, but I don't see a much better way of doing it. how do you see the recent changes, do they look goo to you?

Contributor

fpj commented Nov 15, 2016

@Randgalt thanks for the recent changes, they look fine for me, except that we didn't get QA to run on it.

@lvfangmin there are a few levels of indirection here, which doesn't make it look very pretty, but I don't see a much better way of doing it. how do you see the recent changes, do they look goo to you?

@asfgit asfgit closed this in 179c8db Nov 16, 2016

lvfangmin pushed a commit to lvfangmin/zookeeper that referenced this pull request Jun 17, 2018

ZOOKEEPER-1525: Plumb ZooKeeperServer object into auth plugins
Based on patch work from https://issues.apache.org/jira/browse/ZOOKEEPER-1525

Created ServerAuthenticationProvider which has a method to accept the ZooKeeper
server so that auth can be done using values in the ZK database. As this is a new
interface, existing implementations aren't affected helping backward compatibility

Author: randgalt <jordan@jordanzimmerman.com>

Reviewers: fpj <fpj@apache.org>, Allan Lyu <lvfangmin@gmail.com>

Closes apache#84 from Randgalt/ZOOKEEPER-1525
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment