Daan, Please see my comments in-line below.
Thanks, -John On Jun 17, 2013, at 9:40 AM, Daan Hoogland <daan.hoogl...@gmail.com> wrote: > John, > > If I understand it correctly, you are stating that my take on the solution > is 'not done/not the way to go'? > > For the record the case I solved was an instance of A, but I would not call > it adding technical debt. A arose from existing code in combination of a > requirement to work with a non-posix-path compliant (but unc) nfs server. From my perspective, it is technical debt because the solution, as implemented, is masking/compensating for underlying defects. I think we should fix the underlying defects, input validation and value persistence, rather than trying to compensate for it in the storage layer. We also likely need some type of utility/functionality to upgrade tools to identify invalid path data in existing installations for correction. > > regards, > > > On Mon, Jun 17, 2013 at 2:01 PM, John Burwell <jburw...@basho.com> wrote: > >> All, >> >> Please see my comments in-line below. >> >> Thanks, >> -John >> >> On Jun 15, 2013, at 6:11 AM, Hiroaki KAWAI <ka...@stratosphere.co.jp> >> wrote: >> >>> Probably we've agreed on that double slash should not >>> generated by cloudstack. >>> >>> If something went wrong and double slash was passed to >>> Winfows based NFS, the reason may A) there was another >>> code that generates double slash B) cloudstack configuration >>> or something user input was bad C) some path components became >>> empty string because of database error or something unexpeceted >>> D) cloudstack is really being attacked etc., >> >> A indicates that we adding technical debt and later defects to the system. >> We need to fix upstream for correctness before it rots further. B sound >> like a case for stronger input validation rather than a "fix up" on the >> backend. C seems like we need to be more careful in how we persist and >> retrieve the information from the database. The more we discuss this >> solution, the more this feels like a front-end input validation and >> database persistence issue. Treating it this way would obviate any >> security issues or logging needs. >> >>> >>> Anyway, double slash should not happen and the admins should be >>> able to know when the NFS layer got that sequence. >>> I'd prefer WARN for this reason, but INFO may do as well. >>> I don't have strong opinion on log level. >> >> >> If it shouldn't happen then we should be rejecting the data as part of >> input validation and no allowing it to be persisted. >> >>> >>> In addition to that, "auto-fix" may not be a "fix" for example in >>> case "C". I don't want to see autofix code in many places, >>> "auto-fix" might be a "fix" where the path is really passed to >>> NFS layer. >>> >>> Another approach to double-slash is just reject the input and raise >>> a CloudstackRuntimeException. >>> But I'd prefer auto-fix because of case "A" at this moment… >> >> Originally, I thought this fix was the equivalent of escaping a URL or >> HTML string. Now that I understand it more fully, I believe we need to >> throw a CloudRuntimeException to ferret out code generating incorrectly >> formatted input. >> >>> >>> >>> (2013/06/15 18:01), Daan Hoogland wrote: >>>> H John, >>>> >>>> Yes, actually I was going to make it info level but you swapped me of my >>>> feet with your remark. >>>> >>>> The point is that a mixed posix-paths/UNC system triggered this fix. A >>>> double slash has double meaning in such an environment. However the >> error, >>>> be it human or system generated, does not destabalize cloudstack in any >>>> way, so I will stick with the info. It is certainly not debug in my >>>> opinion. It is not a bug that needs debugging. >>>> >>>> Of course a deeper understanding of cloudstack might change my position >> on >>>> the issue. >>>> >>>> regards, >>>> Daan >>>> >>>> >>>> On Fri, Jun 14, 2013 at 5:58 PM, John Burwell <jburw...@basho.com> >> wrote: >>>> >>>>> Daan, >>>>> >>>>> Since a WARN indicates a condition that could lead to system >> instability, >>>>> many folks configure their log analysis to trigger notifications on >> WARN >>>>> and INFO. Does escaping a character in a path warrant meet that >> criteria? >>>>> >>>>> Thanks, >>>>> -John >>>>> >>>>> On Jun 14, 2013, at 11:52 AM, Daan Hoogland <daan.hoogl...@gmail.com> >>>>> wrote: >>>>> >>>>>> H John, >>>>>> >>>>>> I browsed through your comments and most I will apply. There is one >> where >>>>>> you contradict Hiroaki. This is about the logging level for reporting >> a >>>>>> changed path. I am going to follow my heart at this unless there is a >>>>>> project directive on it. >>>>>> >>>>>> regards, >>>>>> Daan >>>>>> >>>>>> >>>>>> On Fri, Jun 14, 2013 at 5:25 PM, John Burwell <jburw...@basho.com> >>>>> wrote: >>>>>> >>>>>>> Daan, >>>>>>> >>>>>>> I just looked through the review request, and published my comments. >>>>>>> >>>>>>> Thanks, >>>>>>> -John >>>>>>> >>>>>>> On Jun 14, 2013, at 10:27 AM, Daan Hoogland <daan.hoogl...@gmail.com >>> >>>>>>> wrote: >>>>>>> >>>>>>>> Hiroaki, >>>>>>>> >>>>>>>> - auto-fix may happen where it is really required >>>>>>>>> >>>>>>>> I do not have a clear view on this, so I took the approach of better >>>>> safe >>>>>>>> then sorry. The submitted is what works. I don't see how the >> auto-fix >>>>>>>> should ever be needed if the source is fixed. Hope you can live with >>>>>>> this. >>>>>>>> >>>>>>>>> - and if auto-fix happens, it should log it with >>>>>>>>> WARN level. >>>>>>>> >>>>>>>> Applied >>>>>>>> >>>>>>>> >>>>>>>> regards, >>>>>>>> >>>>>>>> >>>>>>>> On Fri, Jun 14, 2013 at 10:35 AM, Daan Hoogland < >>>>> daan.hoogl...@gmail.com >>>>>>>> wrote: >>>>>>>> >>>>>>>>> Thanks Hiroaki, >>>>>>>>> >>>>>>>>> On Fri, Jun 14, 2013 at 3:41 AM, Hiroaki KAWAI < >>>>>>> ka...@stratosphere.co.jp>wrote: >>>>>>>>> >>>>>>>>>> I'd suggest: >>>>>>>>>> - fix the generation of double slash itself >>>>>>>>>> >>>>>>>>> Is in the patch >>>>>>>>> >>>>>>>>>> - auto-fix may happen where it is really required >>>>>>>>>> - and if auto-fix happens, it should log it with >>>>>>>>>> WARN level. >>>>>>>>> >>>>>>>>> Good point, I will up the level in an update. >>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> (2013/06/13 21:15), Daan Hoogland wrote: >>>>>>>>>> >>>>>>>>>>> H, >>>>>>>>>>> >>>>>>>>>>> Can someone look at Review Request #11861< >> https://reviews.apache.** >>>>>>>>>>> org/r/11861/ <https://reviews.apache.org/r/11861/>> for me >> please? >>>>>>>>>>> >>>>>>>>>>> Thanks, >>>>>>>>>>> Daan Hoogland >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>> >>>>>>> >>>>> >>>>> >>>> >>> >> >>
