Mailing-List: contact reviews-help@aurora.apache.org; run by ezmlm
Precedence: bulk
Reply-To: reviews@aurora.apache.org
Content-Type: multipart/alternative;
 boundary="===============4743642886445597688=="
MIME-Version: 1.0
Subject: Re: Review Request 60173: Allow custom Thrift method interceptors to
 be injected via Guice modules
From: Stephan Erb <serb@apache.org>
To: Santhosh Kumar Shanmugham <santhoshkumar.s@gmail.com>,
 David McLaughlin <david@dmclaughlin.com>, Zameer Manji <zmanji@apache.org>
Cc: Aurora <reviews@aurora.apache.org>, Jordan Ly <jordan.ly8@gmail.com>
Date: Tue, 20 Jun 2017 19:30:56 -0000
Message-ID: <20170620193056.1927.30072@reviews-vm2.apache.org>
Auto-Submitted: auto-generated
Sender: Stephan Erb <noreply@reviews.apache.org>
References: <20170619180354.24478.38495@reviews-vm2.apache.org>
In-Reply-To: <20170619180354.24478.38495@reviews-vm2.apache.org>
Reply-To: Stephan Erb <serb@apache.org>
archived-at: Tue, 20 Jun 2017 19:31:05 -0000

--===============4743642886445597688==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60173/#review178416
-----------------------------------------------------------


Ship it!


Ship It!

- Stephan Erb


On June 19, 2017, 8:03 p.m., Jordan Ly wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60173/
> -----------------------------------------------------------
> 
> (Updated June 19, 2017, 8:03 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, Stephan Erb, and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Allow for custom Thrift method interceptors to be injected via Guice modules. Cluster operators might use this feature to inject interceptors that only allow certain roles to call certain endpoints, or to dynamically check if a job should be able to use a constraint.
> 
> 
> Diffs
> -----
> 
>   RELEASE-NOTES.md e032f7927a68b00401ea8f073ff52b5def74f3ce 
>   docs/reference/scheduler-configuration.md 3d53c5a552e06f62a7572591fb0c92ccae42c54b 
>   src/main/java/org/apache/aurora/scheduler/thrift/aop/AopModule.java f59ee1a0514a6dc52573c0b932cba755e0a10e18 
> 
> 
> Diff: https://reviews.apache.org/r/60173/diff/2/
> 
> 
> Testing
> -------
> 
> Unit + integration tests pass.
> 
> Injected a custom module on a Vagrant box -- added a simple class and included the module when starting up the scheduler:
> ```
> diff --git a/examples/vagrant/upstart/aurora-scheduler.conf b/examples/vagrant/upstart/aurora-scheduler.conf
> index 63fcc87..18521af 100644
> --- a/examples/vagrant/upstart/aurora-scheduler.conf
> +++ b/examples/vagrant/upstart/aurora-scheduler.conf
> @@ -56,4 +56,5 @@ exec bin/aurora-scheduler \
>    -allow_container_volumes=true \
>    -offer_filter_duration=0secs \
>    -mesos_driver=V1_DRIVER \
> -  -unavailability_threshold=1mins
> +  -unavailability_threshold=1mins \
> +  -thrift_method_interceptor_modules=org.apache.aurora.scheduler.thrift.aop.ThriftWhitelistInterceptorModule
> diff --git a/src/main/java/org/apache/aurora/scheduler/thrift/aop/ThriftWhitelistInterceptorModule.java b/src/main/java/org/apache/aurora/scheduler/thrift/aop/ThriftWhitelistInterceptorModule.java
> new file mode 100644
> index 0000000..4296f81
> --- /dev/null
> +++ b/src/main/java/org/apache/aurora/scheduler/thrift/aop/ThriftWhitelistInterceptorModule.java
> @@ -0,0 +1,46 @@
> +package org.apache.aurora.scheduler.thrift.aop;
> +
> +import java.util.ArrayList;
> +import java.util.List;
> +
> +import com.google.inject.AbstractModule;
> +
> +import org.aopalliance.intercept.MethodInterceptor;
> +import org.aopalliance.intercept.MethodInvocation;
> +import org.apache.aurora.gen.JobConfiguration;
> +import org.apache.aurora.gen.Response;
> +import org.apache.aurora.gen.ResponseCode;
> +import org.apache.aurora.gen.ResponseDetail;
> +import org.apache.aurora.gen.TaskConfig;
> +
> +/** Module that checks if a role is allowed to do a specific action */
> +public class ThriftWhitelistInterceptorModule extends AbstractModule {
> +
> +  @Override
> +  protected void configure() {
> +    AopModule.bindThriftDecorator(binder(), AopModule.THRIFT_IFACE_MATCHER,
> +        new ThriftWhitelistInterceptor());
> +  }
> +
> +  private class ThriftWhitelistInterceptor implements MethodInterceptor {
> +
> +    @Override
> +    public Object invoke(MethodInvocation invocation) throws Throwable {
> +      Object[] args = invocation.getArguments();
> +      switch(invocation.getMethod().getName()) {
> +        case "createJob":
> +          JobConfiguration config = (JobConfiguration) args[0];
> +          TaskConfig task = config.getTaskConfig();
> +          String role = task.getJob().getRole();
> +          if (role.equals("vagrant")) {
> +            ResponseDetail detail = new ResponseDetail("Test response.");
> +            List<ResponseDetail> details = new ArrayList<>();
> +            details.add(detail);
> +            return new Response(ResponseCode.ERROR, null, details);
> +          }
> +      }
> +
> +      return (Response) invocation.proceed();
> +    }
> +  }
> +}
> ```
> 
> Tried to create a job with two different roles:
> ```
> vagrant@aurora:~$ aurora job create devcluster/vagrant/test/http_example /vagrant/src/test/sh/org/apache/aurora/e2e/http/http_example.aurora
>  INFO] Creating job http_example
> Job creation failed due to error:
> 	Test response.
>     
> vagrant@aurora:~$ aurora job create devcluster/www-data/test/http_example /vagrant/src/test/sh/org/apache/aurora/e2e/http/http_example.aurora
>  INFO] Creating job http_example
>  INFO] Checking status of devcluster/www-data/test/http_example
> Job create succeeded: job url=http://aurora.local:8081/scheduler/www-data/test/http_example
> ```
> 
> 
> Thanks,
> 
> Jordan Ly
> 
>


--===============4743642886445597688==--