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 <jokse...@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 <a...@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.kopti...@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 <a...@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.kopti...@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....@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.kopti...@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....@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.kopti...@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 < > a...@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....@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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
