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 <[email protected]>: > 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 <[email protected]>: > >> 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 <[email protected]>: >> >> > 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 Вячеслав Коптилин < >> > [email protected]> >> > 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 <[email protected]>: >> > > >> > > > 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 Вячеслав Коптилин < >> > > > [email protected]> >> > > > 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 <[email protected]>: >> > > > > >> > > > > > 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, Вячеслав Коптилин < >> > > > [email protected] >> > > > > >: >> > > > > > >> > > > > > > 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 <[email protected] >> >: >> > > > > > > >> > > > > > > > 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, Вячеслав Коптилин < >> > > > > > [email protected] >> > > > > > > >: >> > > > > > > > >> > > > > > > > > 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 < >> [email protected] >> > >: >> > > > > > > > > >> > > > > > > > > > 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 < >> > > > [email protected] >> > > > > > >> > > > > > > > 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 >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > >> >
