Re: Review needed for IGNITE-11410 Sandbox for user-defined code

[Denis Garus <ga...@gmail.com>]

Hello, Igniters!


Alex Plehanov did the review, and I've reworked the implementation of the
issue [1] that is part of the IEP-38 [2].

Could somebody else do a review of the PR [3]?


Alex, thank you for the review!



   1. https://issues.apache.org/jira/browse/IGNITE-11410
   2. https://cwiki.apache.org/confluence/display/IGNITE/IEP-38%3A+Sandbox
   3. https://github.com/apache/ignite/pull/6707


чт, 17 окт. 2019 г. в 16:21, Denis Garus <ga...@gmail.com>:

> Hello, Pavel!
>
> Thank you for the feedback!
>
> I've created IEP-38 that describes the Ignite Sandbox [1].
>
> Yes, the issue requires documentation (there is the flag "Docs equired"),
> but common practice is to write documentation in the end.
>
> >> 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.
> >> 4) There are suspicious security checks at:
> >> CacheOperationContext:37
> >> GridCacheDefaultAffinityKeyMapper:86
> >> PageMemoryImpl:874
> >> I'm not following why they are needed.
>
> These questions have a common answer.
> A user-defined code can call any operation through the public API of
> Ignite. But he may don't have permissions to execute this operation
> successfully.
> For example, to put a value into a cache, it requires permissions for
> accessing to reflection API and reading system property
> IGNITE_ALLOW_ATOMIC_OPS_IN_TX.
> In that case, we have to use AccessController#doPrirvelged call to exclude
> a user-defined code from checking of permissions.
> SecurityUtils#doPriveleged does calling AccessController#doPrirvelged a
> more convenient way.
>
> >> 3) Have you tested security if a compute job is canceled?
> You are right; we should add a test for the cancel case.
> But, for now, we have the issue [2] with the current SecurityContext for
> the canceling of ComputeJob.
>
> 1. https://cwiki.apache.org/confluence/display/IGNITE/IEP-38%3A+Sandbox
> 2. https://issues.apache.org/jira/browse/IGNITE-12300
>
> пн, 14 окт. 2019 г. в 16:16, Pavel Kovalenko <jo...@gmail.com>:
>
>> 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 <ga...@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
>> > > > > > > > > > >
>> > > > > > > > > >
>> > > > > > > > >
>> > > > > > > >
>> > > > > > >
>> > > > > >
>> > > > >
>> > > >
>> > >
>> >
>>
>