ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Pavel Kovalenko <jokse...@gmail.com>
Subject Re: Review needed for IGNITE-11410 Sandbox for user-defined code
Date Mon, 14 Oct 2019 13:16:35 GMT
Denis,

The idea of having a sandbox for running a user-defined code is useful, but
I don't fully understand the implementation approach.
There is no detailed description of the ticket about what public API
methods or configuration parameters should be covered.
There is no description of what have done in initial PR and how.
First of all, there should be an umbrella ticket that should contain all
public API points and configuration parameters where used defined code may
be run.
Without a full list of all possible user-defined code injections, we can't
track what was covered and where are a possible security lacks.
I've checked PR and I have the following questions:
1) Why do you run resource injection through security and how it tested?
2) Why do you check security at *dumpThreads* and *wrapThreadLoader *methods?
These methods are needed only for internal node processes.
3) Have you tested security if a compute job is canceled?
4) There are suspicious security checks at:
CacheOperationContext:37
GridCacheDefaultAffinityKeyMapper:86
PageMemoryImpl:874
I'm not following why they are needed.




пн, 14 окт. 2019 г. в 12:19, Anton Vinogradov <av@apache.org>:

> Fully agree with the benchmark's importance.
> Currently, we're not able to perform proper benchmarking.
> Slava, Is it possible to ask you to check the solution using GridGain's
> benchmarking environment?
>
> On Mon, Oct 14, 2019 at 12:07 PM Вячеслав Коптилин <
> slava.koptilin@gmail.com>
> wrote:
>
> > Hello Anton,
> >
> > > We should avoid heavy merges if possible.
> > Why it should be avoided? To be honest, I don't see any reason for that.
> > Every pull request can be and should be reviewed when it is done and
> ready
> > to be merged into the epic branch (IEP branch).
> > So, the final review of the entire IEP is just a technical/trivial task,
> in
> > my opinion.
> >
> > If I am not mistaken, we are at the stage of preparing a new release
> (2.8),
> > right?
> > And we are trying to add a new feature that may impact the performance.
> > For example, affinity function, which can be overridden by the end-user,
> > and therefore should be covered by `sandbox`.
> > On the other hand, affinity function is a crucial component that is used
> > very often.
> > Are we really sure that the proposed change does not affect the
> > performance? Do we have a benchmark?
> >
> > Please don't get me wrong, guys. I am not against the feature itself.
> > Moreover, it is a great feature and improvement of security.
> > I just want to say that we need to be sure that we are on the right way
> of
> > implementing this without affecting other developers.
> >
> > PS: This is just my opinion, which may be wrong.
> >
> > Thanks,
> > S.
> >
> > пн, 14 окт. 2019 г. в 09:09, Anton Vinogradov <av@apache.org>:
> >
> > > Folks,
> > >
> > > We should avoid heavy merges if possible.
> > > I'm ok with IEP to keep tasks properly, but strictly against all-in-one
> > > "+27000,-18200" merges.
> > > This task implements Sandbox (API + core) which covered by tests and by
> > > some integrations with existing components, which is enough to merge.
> > > The most important thing here is that we will be able to speed-up
> Sandbox
> > > coverage development once its core menged to the master.
> > >
> > > On Fri, Oct 11, 2019 at 5:41 PM Вячеслав Коптилин <
> > > slava.koptilin@gmail.com>
> > > wrote:
> > >
> > > > Hi Denis,
> > > >
> > > > In my humble opinion, the security (the sandbox feature is about
> > > security,
> > > > right?) either covers all APIs/subsystems or not.
> > > > Security should work always and everywhere otherwise it is not
> security
> > > :)
> > > >
> > > > > From my point, we should divide the sandbox and features that use
> it.
> > > > > Also, I added in the main features of Ignite (cache and compute)
> the
> > > > sandbox calls.
> > > > And at this point, you mixed both in the same pull-request.
> > > >
> > > > > I don't see any problem to have the sandbox in the master branch
> and
> > > > implement covering for existing and new features if needed.
> > > > On the other hand, this approach leads to ...
> > > > ignite-123 [IEP-X] introduces new cool API
> > > > ignite-124 [IEP-X] improved cool API
> > > > ignite-125 [IEP-X] fixed a bug
> > > > ignite-126 [IEP-X] fixed performance drop
> > > > ignite-127 [IEP-X] Cache API uses new API
> > > > ignite-127 [IEP-X] Compute grid uses new API
> > > > ...
> > > >
> > > > Why should it be a part of the master branch history? All these
> things
> > > can
> > > > be done on the feature branch, I think. Anyway, it is up to you.
> > > >
> > > > Thanks,
> > > > S.
> > > >
> > > > пт, 11 окт. 2019 г. в 16:31, Denis Garus <garus.d.g@gmail.com>:
> > > >
> > > > > From my point, we should divide the sandbox and features that use
> it.
> > > > > The sandbox is fully implemented and has needed tests.
> > > > >
> > > > > Also, I added in the main features of Ignite (cache and compute)
> the
> > > > > sandbox calls.
> > > > >
> > > > > I don't see any problem to have the sandbox in the master branch
> > > > > and implement covering for existing and new features if needed.
> > > > >
> > > > > пт, 11 окт. 2019 г. в 15:21, Вячеслав Коптилин
<
> > > slava.koptilin@gmail.com
> > > > >:
> > > > >
> > > > > > Hi Denis,
> > > > > >
> > > > > > Yep, I understand the scope of the ticket, but... I think it
is
> > not a
> > > > > good
> > > > > > idea to merge partly implemented feature(s) into the master
> branch.
> > > > > > Especially, at this moment. We are at the stage of preparing
a
> new
> > > > > release
> > > > > > and I doubt that all improvements, tests (unit tests, integration
> > > > tests,
> > > > > > and performance tests) can be implemented before the release
> branch
> > > is
> > > > > cut
> > > > > > off.
> > > > > > Personally, I would prefer to create an epic/feature branch
for
> > these
> > > > > > activities. In that case, we can implement a feature step by
step
> > and
> > > > > merge
> > > > > > it into the master branch once all components are covered.
> > > > > >
> > > > > > > But, sure, we should execute any user-defined code in the
> sandbox
> > > on
> > > > a
> > > > > > remote node. Feel free to create issues.
> > > > > > will do.
> > > > > >
> > > > > > Thanks,
> > > > > > S.
> > > > > >
> > > > > > пт, 11 окт. 2019 г. в 14:52, Denis Garus <garus.d.g@gmail.com>:
> > > > > >
> > > > > > > Hello, Slava!
> > > > > > >
> > > > > > > The scope of the issue is limited by the following features:
> > > > > > >
> > > > > > >    - StreamReceiver for DataStreamer;
> > > > > > >    - EntryProcessor;
> > > > > > >    - ComputeJob;
> > > > > > >    - filter and transformer for ScanQuery.
> > > > > > >
> > > > > > > But, sure, we should execute any user-defined code in the
> sandbox
> > > on
> > > > a
> > > > > > > remote node.
> > > > > > > Feel free to create issues.
> > > > > > >
> > > > > > > Thanks for the feedback!
> > > > > > >
> > > > > > > пт, 11 окт. 2019 г. в 13:26, Вячеслав Коптилин
<
> > > > > slava.koptilin@gmail.com
> > > > > > >:
> > > > > > >
> > > > > > > > Hello Denis, Anton,
> > > > > > > >
> > > > > > > > Could you please clarify the following aspect? Do
we need the
> > > same
> > > > > > > > changes/capabilities related to Continuous Queries,
Disco
> > > > listeners,
> > > > > > > > CacheStore Factories etc?
> > > > > > > >
> > > > > > > > Thanks,
> > > > > > > > S.
> > > > > > > >
> > > > > > > > пт, 11 окт. 2019 г. в 12:24, Anton Vinogradov
<av@apache.org
> >:
> > > > > > > >
> > > > > > > > > Folks,
> > > > > > > > >
> > > > > > > > > As a prereviewer, I'd like to say that the solution
looks
> > good
> > > to
> > > > > me,
> > > > > > > but
> > > > > > > > > fresh eyes would be good.
> > > > > > > > >
> > > > > > > > > On Fri, Oct 11, 2019 at 9:40 AM Denis Garus <
> > > garus.d.g@gmail.com
> > > > >
> > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > > Hello, Igniters!
> > > > > > > > > >
> > > > > > > > > > I've raised the PR [1] with the sandbox
for AI [2].
> > > > > > > > > > Could somebody review it?
> > > > > > > > > >
> > > > > > > > > > If you have questions and prefer the Slack,
I've created
> > the
> > > > > > channel
> > > > > > > > [3].
> > > > > > > > > >
> > > > > > > > > > 1. https://github.com/apache/ignite/pull/6707
> > > > > > > > > > 2. https://issues.apache.org/jira/browse/IGNITE-11410
> > > > > > > > > > 3. https://app.slack.com/client/T4S1WH2J3/CP8JER880
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

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