Daniel, I don't like the end user semantics of what you are proposing. It looks like you are abusing a side effect of something else.
"something_reload=1" is quite clear what it means. Note that for an average mysql/drizzle user even the meaning of the @@var_name syntax might be unclear. henrik On Sat, Apr 28, 2012 at 8:56 AM, Daniel Nichter <dan...@percona.com> wrote: > Anshu, > > My reply is quite late, but the conference and Drizzle Day were both > enlightening. I look forward to next year. > > As for updating files, I think there's a simpler, more direct approach: > > SET GLOBAL regex_policy_policy=@@regex_policy_policy; > > That sets the var to its current value which triggers the plugin's update > hook and since the new value == the old value, the result is simply to > reload the file. > > This avoid having two code paths: one for SET > GLOBAL regex_policy_policy="new-file" and another for SET > GLOBAL regex_policy_reload=1 (which would eventually call the same code as > the first statement). > > Thoughts on this (you and anyone else)? > > -Daniel > > Le 17 avr. 2012 à 15:09, Anshu Kumar a écrit : > > Hi Daniel, > How was the conference and drizzle day? > Coming to the point, after brainstorming the possibilities that I have in > the issue of making regex policy plugin dynamic, I think that the solution I > proposed in my last mail is best for now. And using that we wont need any > issue handling with if the policy file was not correct or change in the look > and feel of some specific plugins. From coding point of view, we can do this > by adding an option of reload variable which will default be set to false. > Upon making it true, it will trigger a function which will create a separate > instance of policy and will call loadfile on it. If the call is a success, > we will remove all old policies and reload the system with new ones. And if > the call gives some error message then do nothing. After both the cases, the > function will change the reload variable to false. This way we wont need any > separate threads also. > Waiting for your review comments to start working on this. > > On Wed, Apr 4, 2012 at 2:55 PM, Anshu Kumar <ansharyan...@gmail.com> wrote: >> >> Hey Daniel, >> After reading your views, I too think now that auto reloading is not the >> ideal solution to this scenario. In this solution I have used the idea of >> thread for reloading the policy files. You put a point that what if admin is >> using a ext editor which auto saves the changes after some fixed time. In >> that case incorrect policies may be read. >> >> For this issue, instead of using a thread, we can come up with a solution >> in which the reloading won't be dynamic. So it would be like, if you are an >> admin and you want to change the policy file, do it. After all changes are >> done, just change the value of regex_policy_reload to true and policies will >> then be reloaded. After policies are reloaded, the value of >> regex_policy_reload will again be set to false from code itself. In this >> way, it can be ensured that only the correct policies are loaded there. It >> is true that here too anyone can change the status of regex_policy_reload to >> true, so either don't let him access the policy file or we will be needing >> an authorization system for changing global variables too. >> >> Brainstorming further for the most optimal solution. Comment and >> suggestions are welcome. >> >> About the name concern, I use Ansh as my nick. Both are correct. You can >> call me anything you like. However changing my sign to 'Anshu', so that it >> don't cause any further confusion to anyone. :) >> >> On Wed, Apr 4, 2012 at 1:40 PM, Henrik Ingo <henrik.i...@avoinelama.fi> >> wrote: >>> >>> Daniel >>> >>> FWIW, I think you might be correct. I originally thought that >>> auto-reloading would be an easy way to implement a way to re-read the policy >>> file. For instance, it could be implemented crudely without any new >>> configuration options and no new threads needing to launch. However, as >>> we've gone deeper into the subject it became clear that a good >>> implementation will need those things, hence the "easy" attribute is no >>> longer there. I support going back to explore your original idea. >>> >>> henrik >>> >>> >>> On Wed, Apr 4, 2012 at 8:12 AM, Daniel Nichter <dan...@percona.com> >>> wrote: >>>> >>>> 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. >>> >>> >>> >>> >>> -- >>> henrik.i...@avoinelama.fi >>> +358-40-8211286 skype: henrik.ingo irc: hingo >>> www.openlife.cc >>> >>> My LinkedIn profile: http://www.linkedin.com/profile/view?id=9522559 >> >> >> >> >> -- >> Regards, >> Anshu >> > > > > -- > Regards, > Anshu > > -- henrik.i...@avoinelama.fi +358-40-8211286 skype: henrik.ingo irc: hingo www.openlife.cc My LinkedIn profile: http://www.linkedin.com/profile/view?id=9522559 _______________________________________________ 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
