cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Daan Hoogland <daan.hoogl...@gmail.com>
Subject Re: committer wanted for review
Date Mon, 17 Jun 2013 13:40:51 GMT
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.

regards,


On Mon, Jun 17, 2013 at 2:01 PM, John Burwell <jburwell@basho.com> wrote:

> All,
>
> Please see my comments in-line below.
>
> Thanks,
> -John
>
> On Jun 15, 2013, at 6:11 AM, Hiroaki KAWAI <kawai@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 <jburwell@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.hoogland@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 <jburwell@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.hoogland@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.hoogland@gmail.com
> >>>>>> wrote:
> >>>>>>
> >>>>>>> Thanks Hiroaki,
> >>>>>>>
> >>>>>>> On Fri, Jun 14, 2013 at 3:41 AM, Hiroaki KAWAI <
> >>>>> kawai@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
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>
> >>>>>
> >>>>>
> >>>
> >>>
> >>
> >
>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message