Anshu, Henrik, Clint, First: Anshu: I like to use people's names correctly, so I noticed you sign your emails "Ansh" but we've been writing "Anshu". Is one or the other, or both, correct?
Now on to business. First, thanks for the code. This is a good start and I'm happy to see that you've been able to jump into the code with apparent ease. Second, I disagree that the auto-reloading approach is the ideal solution. I think we all need to debate the merits of this approach further. So let's do that... In my humble opinion, this approach has the following drawbacks: 1) It's "gold plating" because in the real world the policy file won't/shouldn't change so frequently as to make autp-reloading a valuable feature. It's not too much to ask the user to execute one simple command, and chances are they will expect to do this. Thinking of Drizzle and MySQL, no auto-(re)loading features come immediately to my mind, so we won't be depriving the user of functionality they're used to. Of course, I'm all for new types of functionality when it's clear that the functionality will be a "big win" for users, but given the nature of this issue, I don't think auto-reloading is a big win and certainly not worth the extra engineering effort. 2) It's a bit of magic and magic usually leads to problems down the road. Sure, it's just one little thread that sleeps, checks, and maybe does something, but one immediate "problem" comes to mind: testing. Anything dependent on time or time-based is inherently more difficult to test. It's possible, of course, but (related to #1) is it worth the effort? Another problem comes to mind: what if the admin is editing the policy file and his editor auto-saves every N minutes, and auto-reload is on? Drizzle may read the policy file before the admin intends. Magic can be very difficult to wield safely. 3) It doesn't address the original security concern. Henrik raised the problem that anyone could execute the command to reload the policy file, but anyone could enable the auto-reload, too. The real solution lies in the authorization modules. Right now Drizzle has only very basic authorization: schema and table access (via this plugin nonetheless). What Drizzle needs is authorization for setting/changing global variables. I think we can make still make this dynamic and leave the authorization solution for another time/person/project. 4) It introduces a concept that will either become a norm or become an exception. Given the aforementioned points, I wouldn't like to see auto-reloading before a norm, and we certainly don't want more exceptions because one of our high-level goals is to make the "look and feel" of all plugins consistent. So if we do this here, then why not --auth-file.auto-reload too? But then we're neck deep in gold plating and magic. 5) It seems contrary to "the Drizzle way". Brian, Stewart, and the original core developers have more of a sense of what the Drizzle way is, but I'm pretty certain is centers on removing the extraneous and the gotchas. Drizzle prides itself it what it doesn't have: no views, no triggers, no timezones, etc. I think auto-reloading is extraneous (gold plating) and prone to becoming a gotcha. To modify my previous example: what if the senior admin enables auto-reloading but forgets to tell the junior admin who changes the policy file on Friday and plans to review the changes with the senior admin on Monday only for both of them to get a call during the weekend by the CTO because Sally in HR can't access the employees database and she's trying to do payroll so now everyone's paychecks will be late? Gotcha. So, in conclusion: your code is a good start, but I think we need another solution. Other opinions? -Daniel Le 3 avr. 2012 à 18:03, Anshu Kumar a écrit : > Hi Daniel, > For making all the plugins dynamic, as the earlier discussion for auth_file > could not come to a conclusion, I started off with making the regex_policy > plugin dynamic. Here is the complete process which am proposing for this > plugin > > 1. There would be a autoreload variable, default set to false. This variable > will determine if policy file needs to be reloaded. > > 2. Upon setting this variable to true (SET GLOBAL > regex_policy_autoreload=ON), a new thread will be created which will handle > the reload issues of policy file. > > 3. In the thread created, on every one minute, using the stat command to find > the last modified time and taking the difference from current time, it will > be checked that if the file was modified in last one minute. And if it was, > reload the file. If it wasn't modified, continue to the next poll. > > 4. This thing will go on till you again set the variable value to false (SET > GLOBAL regex_policy_autoreload=OFF) > > Scenario would be, if you want to change the policy file and want those > changes to be reflected, just change the variable to true, do your > modifications, and the changes will be reflected the next minute in the > system. > > I have implemented this whole thing. Here is the link of the branch. It would > be great if you can go through it and give your comments. > https://code.launchpad.net/~ansharyan015/drizzle/dynamic_regex_policy > > P.S. Thanks to Henrik for being the continuous help. :) > Comments and suggestions are welcome. > > > On Mon, Apr 2, 2012 at 11:01 PM, Henrik Ingo <henrik.i...@avoinelama.fi> > wrote: > Anshu: To create a background thread, you need to use class > drizzled::plugin::Daemon > > Look at plugin/json_server/json_server.cc (class JsonServer at the > end) for an example. > > henrik > > On Mon, Apr 2, 2012 at 4:16 PM, Henrik Ingo <henrik.i...@avoinelama.fi> wrote: > > Anshu > > > > You are right. Creating a thread is more correct. I was just trying to > > avoid it since you originally asked for "low-hanging-fruit" bug. > > > > Functionally, ignoring the performance hit, you could do this also > > from the restrict* methods and then you don't need to create your own > > thread. > > > > henrik > > > > On Mon, Apr 2, 2012 at 3:45 PM, Anshu Kumar <ansharyan...@gmail.com> wrote: > >> Hey Henrik, > >> What I was thinking is we can have a system like this. When autoreload > >> variable is set to true, we can create a thread which will check either by > >> inotify or stat() that the corresponding regex policy file is changed or > >> not. And it of does then only reload the file. This is just like creating a > >> handler for change in policy file. The performance will be better than > >> polling for a min and reloading the policy file. > >> The implementation is actually the same in this, and the way you suggested > >> by Policy::restrict methods. So did you want to say that when checking for > >> regex policy rights (if access is allowed or denied), we can check if file > >> needs to be refreshed? > >> > >> On Mon, Apr 2, 2012 at 6:02 PM, Henrik Ingo <henrik.i...@avoinelama.fi> > >> wrote: > >>> > >>> Anshu > >>> > >>> Actually, now that I re-read this, I don't know if it was a smart idea > >>> and maybe there needs to be a thread that does the polling, but my > >>> original idea was that you could reload the file during an > >>> authorization request. So basically when any of the > >>> Polixy::restrict... methods are called, you would first check if the > >>> file needs to be refreshed and then re-read it (or not). This way you > >>> don't need to have a loop or do any polling or such. > >>> > >>> This approach is not good because it would introduce a performance hit > >>> into random requests every N seconds. But you could still do it this > >>> way as a proof of concept, then we could work on making it a > >>> background thread later. > >>> > >>> henrik > >>> > >>> On Mon, Apr 2, 2012 at 2:51 PM, Anshu Kumar <ansharyan...@gmail.com> > >>> wrote: > >>> > Hey Guys, > >>> > I have tried to implement this dynamic thing to regex_policy plugin. > >>> > In reference to my talk with Henrik yesterday, we discussed that there > >>> > could > >>> > be a autoreload variable, and when made true it will continuously poll > >>> > for > >>> > changes in default regex policy file. And it it is made false, it will > >>> > stop > >>> > polling, checking for modifications. Sticking to the discussion, the > >>> > code I > >>> > wrote is > >>> > > >>> > void autoReload_Regex_Policy(Session *, sql_var_t) > >>> > { > >>> > if(policy->sysvar_autoreload) > >>> > { > >>> > while(1) > >>> > { > >>> > if (not policy->loadFile()) > >>> > { > >>> > errmsg_printf(error::ERROR, _("Could not > >>> > load > >>> > regex policy file: %s\n"), > >>> > (policy ? > >>> > policy->getError().str().c_str() : _("Unknown"))); > >>> > return; > >>> > } > >>> > sleep(60); > >>> > if(!policy->sysvar_autoreload) > >>> > break; > >>> > } > >>> > } > >>> > else > >>> > { > >>> > } > >>> > } > >>> > This function handles the case when you change the value of autoreload > >>> > from > >>> > drizzle client. The problem is as the function is using sleep > >>> > recursively, > >>> > when trying to change autoreload value by "SET GLOBAL", the client > >>> > hangs. > >>> > This is obviously due to function recursive structure. Initially, I > >>> > thought > >>> > that it would change the variable and then polls. > >>> > > >>> > Now, coming to the discussion earlier in this mail, even if I create a > >>> > pthread from this function which will check for modification using > >>> > stat() or > >>> > inotify(), this function won't exit untill its thread stop working. And > >>> > as > >>> > thread will continuously polls for changes, it wont exit. > >>> > > >>> > Is there any solution for this scenario, except adding the refresh > >>> > command > >>> > for refreshing the policy file? > >>> > > >>> > > >>> > On Fri, Mar 30, 2012 at 10:34 PM, Clint Byrum <cl...@fewbar.com> wrote: > >>> >> > >>> >> Excerpts from Henrik Ingo's message of Thu Mar 29 21:16:26 -0700 2012: > >>> >> > Daniel: > >>> >> > > >>> >> > Have you thought about authorization for this? I mean we wouldn't > >>> >> > want > >>> >> > any old logged in user to be able to > >>> >> > > >>> >> > SET GLOBAL auth_file.users=/home/hingo/igivemyselfrootpowers.users > >>> >> > > >>> >> > (Making the plugin reload the existing file will be helpful. But it > >>> >> > might not be a good idea to allow to change that value.) > >>> >> > > >>> >> > >>> >> Agreed. I'd like to see plugins like auth_file and regex_policy given > >>> >> a generic way to "watch" their files. There are a number of ways to do > >>> >> this, but I don't think each plugin should implement its own method. > >>> >> > >>> >> Thoughts I've had on this: > >>> >> > >>> >> * A thread which uses either inotify or falls back to polling > >>> >> with stat(), and whenever there is a change, calls any registered code > >>> >> to update that file's effect. > >>> >> > >>> >> * An admin command like REFRESH '/etc/drizzle/regex.policy' which does > >>> >> the same thing as the thread without the inotify/polling. > >>> >> > >>> >> * Cache the stat() call on the file and periodically expire the cache > >>> >> and refresh the contents if stat() indicates that it has changed.
_______________________________________________ Mailing list: https://launchpad.net/~drizzle-discuss Post to : drizzle-discuss@lists.launchpad.net Unsubscribe : https://launchpad.net/~drizzle-discuss More help : https://help.launchpad.net/ListHelp
