Re: Review Request 69330: OOZIE-3379 Auth token cache file name should include Oozie URL

[András Piros via Review Board]

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


Ship it!




Ship It!

- András Piros


On Dec. 4, 2018, 5:29 p.m., zhang junfan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69330/
> ---
> 
> (Updated Dec. 4, 2018, 5:29 p.m.)
> 
> 
> Review request for oozie, András Piros and Peter Bacsko.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> Auth token cache file name should include Oozie URL, link to oozie-3379.
> 
> 
> Diffs
> -
> 
>   client/src/main/java/org/apache/oozie/client/AuthOozieClient.java 3a8b5ab6 
>   core/src/main/java/org/apache/oozie/test/EmbeddedServletContainer.java 
> fc9d840b 
>   
> core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java
>  04fde730 
> 
> 
> Diff: https://reviews.apache.org/r/69330/diff/4/
> 
> 
> Testing
> ---
> 
> I added a test for authOozieClients with the different oozieUrl.
> I changed the code for EmbeddedServletContainer. Because this patch needs to 
> bind the cache file to oozieUrl, you need to test whether multiple different 
> clients can use the cache. In the original EmbeddedServletContainer code, the 
> port was random and could not get a given container context path. So the port 
> binding needs to be added.
> 
> 
> File Attachments
> 
> 
> oozie-3379-6.patch
>   
> https://reviews.apache.org/media/uploaded/files/2018/11/27/ec465408-bcbc-44bc-a816-7ff73e3c0e68__oozie-3379-6.patch
> 
> 
> Thanks,
> 
> zhang junfan
> 
>

Re: Review Request 69330: OOZIE-3379 Auth token cache file name should include Oozie URL

[zhang junfan]

> On 十一月 28, 2018, 2:23 p.m., András Piros wrote:
> >

Hello , I have submit the oozie-3379-7.patch. Please review it. 
Thanks


- zhang


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


On 十二月 4, 2018, 5:29 p.m., zhang junfan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69330/
> ---
> 
> (Updated 十二月 4, 2018, 5:29 p.m.)
> 
> 
> Review request for oozie, András Piros and Peter Bacsko.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> Auth token cache file name should include Oozie URL, link to oozie-3379.
> 
> 
> Diffs
> -
> 
>   client/src/main/java/org/apache/oozie/client/AuthOozieClient.java 3a8b5ab6 
>   core/src/main/java/org/apache/oozie/test/EmbeddedServletContainer.java 
> fc9d840b 
>   
> core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java
>  04fde730 
> 
> 
> Diff: https://reviews.apache.org/r/69330/diff/4/
> 
> 
> Testing
> ---
> 
> I added a test for authOozieClients with the different oozieUrl.
> I changed the code for EmbeddedServletContainer. Because this patch needs to 
> bind the cache file to oozieUrl, you need to test whether multiple different 
> clients can use the cache. In the original EmbeddedServletContainer code, the 
> port was random and could not get a given container context path. So the port 
> binding needs to be added.
> 
> 
> File Attachments
> 
> 
> oozie-3379-6.patch
>   
> https://reviews.apache.org/media/uploaded/files/2018/11/27/ec465408-bcbc-44bc-a816-7ff73e3c0e68__oozie-3379-6.patch
> 
> 
> Thanks,
> 
> zhang junfan
> 
>

Re: Review Request 69330: OOZIE-3379 Auth token cache file name should include Oozie URL

[zhang junfan]

> On 十一月 28, 2018, 2:23 p.m., András Piros wrote:
> > core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java
> > Lines 199-201 (original), 243 (patched)
> > 
> >
> > Deleting and assertion for non-existence missing. Is it by chance?

I don't think so. The deletion operation only guarantees the security of the 
test. However, in my changes, the file name was variable, which made it 
impossible for me to delete at the beginning. But I also think that the 
pre-delete file operation is not necessary. Whether it exists or not will not 
affect the test results


> On 十一月 28, 2018, 2:23 p.m., András Piros wrote:
> > core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java
> > Lines 339 (patched)
> > 
> >
> > Do we have to set this also when 
> > `oozie.authentication.signature.secret.file` has been set in beforehand?

This is due to the difference in the version of hadoop, see the comment in the 
source code.


- zhang


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


On 十二月 4, 2018, 5:29 p.m., zhang junfan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69330/
> ---
> 
> (Updated 十二月 4, 2018, 5:29 p.m.)
> 
> 
> Review request for oozie, András Piros and Peter Bacsko.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> Auth token cache file name should include Oozie URL, link to oozie-3379.
> 
> 
> Diffs
> -
> 
>   client/src/main/java/org/apache/oozie/client/AuthOozieClient.java 3a8b5ab6 
>   core/src/main/java/org/apache/oozie/test/EmbeddedServletContainer.java 
> fc9d840b 
>   
> core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java
>  04fde730 
> 
> 
> Diff: https://reviews.apache.org/r/69330/diff/4/
> 
> 
> Testing
> ---
> 
> I added a test for authOozieClients with the different oozieUrl.
> I changed the code for EmbeddedServletContainer. Because this patch needs to 
> bind the cache file to oozieUrl, you need to test whether multiple different 
> clients can use the cache. In the original EmbeddedServletContainer code, the 
> port was random and could not get a given container context path. So the port 
> binding needs to be added.
> 
> 
> File Attachments
> 
> 
> oozie-3379-6.patch
>   
> https://reviews.apache.org/media/uploaded/files/2018/11/27/ec465408-bcbc-44bc-a816-7ff73e3c0e68__oozie-3379-6.patch
> 
> 
> Thanks,
> 
> zhang junfan
> 
>

Re: Review Request 69330: OOZIE-3379 Auth token cache file name should include Oozie URL

[zhang junfan]

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

(Updated 十二月 4, 2018, 5:29 p.m.)


Review request for oozie, András Piros and Peter Bacsko.


Repository: oozie-git


Description
---

Auth token cache file name should include Oozie URL, link to oozie-3379.


Diffs (updated)
-

  client/src/main/java/org/apache/oozie/client/AuthOozieClient.java 3a8b5ab6 
  core/src/main/java/org/apache/oozie/test/EmbeddedServletContainer.java 
fc9d840b 
  
core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java 
04fde730 


Diff: https://reviews.apache.org/r/69330/diff/4/

Changes: https://reviews.apache.org/r/69330/diff/3-4/


Testing
---

I added a test for authOozieClients with the different oozieUrl.
I changed the code for EmbeddedServletContainer. Because this patch needs to 
bind the cache file to oozieUrl, you need to test whether multiple different 
clients can use the cache. In the original EmbeddedServletContainer code, the 
port was random and could not get a given container context path. So the port 
binding needs to be added.


File Attachments


oozie-3379-6.patch
  
https://reviews.apache.org/media/uploaded/files/2018/11/27/ec465408-bcbc-44bc-a816-7ff73e3c0e68__oozie-3379-6.patch


Thanks,

zhang junfan

Re: Review Request 69330: OOZIE-3379 Auth token cache file name should include Oozie URL

[András Piros via Review Board]

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




client/src/main/java/org/apache/oozie/client/AuthOozieClient.java
Lines 105 (patched)


Are you sure you only get characters that fit into a file name on Linux 
OSes? I think special behavior is needed to mask such incorrect characters.



core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java
Lines 199-201 (original), 243 (patched)


Deleting and assertion for non-existence missing. Is it by chance?



core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java
Lines 320 (patched)


A couple of other test scenarios would be also of importance:

* multiple `OozieCLI` calls to the same Oozie host w/ same context roots
* multiple `OozieCLI` calls to the same Oozie host w/ different context 
roots
* multiple `OozieCLI` calls to different Oozie host w/ and different 
context roots



core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java
Lines 322-338 (patched)


Please extract to a well-named method for clarity.



core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java
Lines 328 (patched)


What about `try-with-resources` and omitting the `close()` call?



core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java
Lines 339 (patched)


Do we have to set this also when 
`oozie.authentication.signature.secret.file` has been set in beforehand?



core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java
Lines 343-350 (patched)


Extract method.



core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java
Lines 347 (patched)


Please put appropriate assertion message.



core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java
Lines 351-358 (patched)


Extract method.



core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java
Lines 355 (patched)


Please put appropriate assertion message.



core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java
Lines 366 (patched)


Assertion message.



core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java
Lines 368 (patched)


Assertion message.


- András Piros


On Nov. 28, 2018, 12:15 p.m., zhang junfan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69330/
> ---
> 
> (Updated Nov. 28, 2018, 12:15 p.m.)
> 
> 
> Review request for oozie, András Piros and Peter Bacsko.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> Auth token cache file name should include Oozie URL, link to oozie-3379.
> 
> 
> Diffs
> -
> 
>   client/src/main/java/org/apache/oozie/client/AuthOozieClient.java 3a8b5ab6 
>   core/src/main/java/org/apache/oozie/test/EmbeddedServletContainer.java 
> fc9d840b 
>   
> core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java
>  04fde730 
> 
> 
> Diff: https://reviews.apache.org/r/69330/diff/3/
> 
> 
> Testing
> ---
> 
> I added a test for authOozieClients with the different oozieUrl.
> I changed the code for EmbeddedServletContainer. Because this patch needs to 
> bind the cache file to oozieUrl, you need to test whether multiple different 
> clients can use the cache. In the original EmbeddedServletContainer code, the 
> port was random and could not get a given container context path. So the port 
> binding needs to be added.
> 
> 
> File Attachments
> 
> 
> oozie-3379-6.patch
>   
> https://reviews.apache.org/media/uploaded/files/2018/11/27/ec465408-bcbc-44bc-a816-7ff73e3c0e68__oozie-3379-6.patch
> 
> 
> Thanks,
> 
> zhang junfan
> 
>

Re: Review Request 69330: OOZIE-3379 Auth token cache file name should include Oozie URL

[zhang junfan]

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

(Updated 十一月 28, 2018, 12:15 p.m.)


Review request for oozie, András Piros and Peter Bacsko.


Repository: oozie-git


Description
---

Auth token cache file name should include Oozie URL, link to oozie-3379.


Diffs (updated)
-

  client/src/main/java/org/apache/oozie/client/AuthOozieClient.java 3a8b5ab6 
  core/src/main/java/org/apache/oozie/test/EmbeddedServletContainer.java 
fc9d840b 
  
core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java 
04fde730 


Diff: https://reviews.apache.org/r/69330/diff/3/

Changes: https://reviews.apache.org/r/69330/diff/2-3/


Testing
---

I added a test for authOozieClients with the different oozieUrl.
I changed the code for EmbeddedServletContainer. Because this patch needs to 
bind the cache file to oozieUrl, you need to test whether multiple different 
clients can use the cache. In the original EmbeddedServletContainer code, the 
port was random and could not get a given container context path. So the port 
binding needs to be added.


File Attachments


oozie-3379-6.patch
  
https://reviews.apache.org/media/uploaded/files/2018/11/27/ec465408-bcbc-44bc-a816-7ff73e3c0e68__oozie-3379-6.patch


Thanks,

zhang junfan

Re: Review Request 69330: OOZIE-3379 Auth token cache file name should include Oozie URL

[zhang junfan]

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

(Updated 十一月 28, 2018, 12:14 p.m.)


Review request for oozie, András Piros and Peter Bacsko.


Repository: oozie-git


Description
---

Auth token cache file name should include Oozie URL, link to oozie-3379.


Diffs (updated)
-

  
core/src/main/java/org/apache/oozie/service/CoordMaterializeTriggerService.java 
1cbd4749 


Diff: https://reviews.apache.org/r/69330/diff/2/

Changes: https://reviews.apache.org/r/69330/diff/1-2/


Testing
---

I added a test for authOozieClients with the different oozieUrl.
I changed the code for EmbeddedServletContainer. Because this patch needs to 
bind the cache file to oozieUrl, you need to test whether multiple different 
clients can use the cache. In the original EmbeddedServletContainer code, the 
port was random and could not get a given container context path. So the port 
binding needs to be added.


File Attachments


oozie-3379-6.patch
  
https://reviews.apache.org/media/uploaded/files/2018/11/27/ec465408-bcbc-44bc-a816-7ff73e3c0e68__oozie-3379-6.patch


Thanks,

zhang junfan

Re: Review Request 69330: OOZIE-3379 Auth token cache file name should include Oozie URL

[zhang junfan]

> On 十一月 20, 2018, 11:45 a.m., András Piros wrote:
> >

Thanks to András Piros for your comment. I re-uploaded the oozie-3379-6.patch. 
Please review it. Look forward to your reply.


- zhang


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


On 十一月 27, 2018, 3:55 p.m., zhang junfan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69330/
> ---
> 
> (Updated 十一月 27, 2018, 3:55 p.m.)
> 
> 
> Review request for oozie, András Piros and Peter Bacsko.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> Auth token cache file name should include Oozie URL, link to oozie-3379.
> 
> 
> Diffs
> -
> 
>   client/src/main/java/org/apache/oozie/client/AuthOozieClient.java 3a8b5ab6 
>   core/src/main/java/org/apache/oozie/test/EmbeddedServletContainer.java 
> fc9d840b 
>   
> core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java
>  04fde730 
> 
> 
> Diff: https://reviews.apache.org/r/69330/diff/1/
> 
> 
> Testing
> ---
> 
> I added a test for authOozieClients with the different oozieUrl.
> I changed the code for EmbeddedServletContainer. Because this patch needs to 
> bind the cache file to oozieUrl, you need to test whether multiple different 
> clients can use the cache. In the original EmbeddedServletContainer code, the 
> port was random and could not get a given container context path. So the port 
> binding needs to be added.
> 
> 
> File Attachments
> 
> 
> oozie-3379-6.patch
>   
> https://reviews.apache.org/media/uploaded/files/2018/11/27/ec465408-bcbc-44bc-a816-7ff73e3c0e68__oozie-3379-6.patch
> 
> 
> Thanks,
> 
> zhang junfan
> 
>

Re: Review Request 69330: OOZIE-3379 Auth token cache file name should include Oozie URL

[zhang junfan]

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

(Updated 十一月 27, 2018, 3:55 p.m.)


Review request for oozie, András Piros and Peter Bacsko.


Repository: oozie-git


Description
---

Auth token cache file name should include Oozie URL, link to oozie-3379.


Diffs
-

  client/src/main/java/org/apache/oozie/client/AuthOozieClient.java 3a8b5ab6 
  core/src/main/java/org/apache/oozie/test/EmbeddedServletContainer.java 
fc9d840b 
  
core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java 
04fde730 


Diff: https://reviews.apache.org/r/69330/diff/1/


Testing
---

I added a test for authOozieClients with the different oozieUrl.
I changed the code for EmbeddedServletContainer. Because this patch needs to 
bind the cache file to oozieUrl, you need to test whether multiple different 
clients can use the cache. In the original EmbeddedServletContainer code, the 
port was random and could not get a given container context path. So the port 
binding needs to be added.


File Attachments (updated)


oozie-3379-6.patch
  
https://reviews.apache.org/media/uploaded/files/2018/11/27/ec465408-bcbc-44bc-a816-7ff73e3c0e68__oozie-3379-6.patch


Thanks,

zhang junfan

Re: Review Request 69330: OOZIE-3379 Auth token cache file name should include Oozie URL

[András Piros via Review Board]

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




client/src/main/java/org/apache/oozie/client/AuthOozieClient.java
Lines 73 (patched)


Can be `final`.



client/src/main/java/org/apache/oozie/client/AuthOozieClient.java
Lines 100 (patched)


Use `@VisibleForTesting`, and can be package-private.



client/src/main/java/org/apache/oozie/client/AuthOozieClient.java
Lines 101 (patched)


Why not use Guava's `BaseEncoding` like this?

```
BaseEncoding
.base64Url()
.omitPadding()
.encode("https://localhost:11443/oozie".getBytes(Charsets.UTF_8))
```



core/src/main/java/org/apache/oozie/test/EmbeddedServletContainer.java
Lines 131-133 (patched)


Can you please extract to a well-named method, or leave a code comment for 
future maintainers, or both?



core/src/main/java/org/apache/oozie/test/EmbeddedServletContainer.java
Line 128 (original), 136 (patched)


Can you please extract to a well-named method, or leave a code comment for 
future maintainers, or both?



core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java
Lines 312-336 (patched)


Can you please:

* extract to separate test method
* provide assertion messages


- András Piros


On Nov. 14, 2018, 2:02 a.m., zhang junfan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69330/
> ---
> 
> (Updated Nov. 14, 2018, 2:02 a.m.)
> 
> 
> Review request for oozie, András Piros and Peter Bacsko.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> Auth token cache file name should include Oozie URL, link to oozie-3379.
> 
> 
> Diffs
> -
> 
>   client/src/main/java/org/apache/oozie/client/AuthOozieClient.java 3a8b5ab6 
>   core/src/main/java/org/apache/oozie/test/EmbeddedServletContainer.java 
> fc9d840b 
>   
> core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java
>  04fde730 
> 
> 
> Diff: https://reviews.apache.org/r/69330/diff/1/
> 
> 
> Testing
> ---
> 
> I added a test for authOozieClients with the different oozieUrl.
> I changed the code for EmbeddedServletContainer. Because this patch needs to 
> bind the cache file to oozieUrl, you need to test whether multiple different 
> clients can use the cache. In the original EmbeddedServletContainer code, the 
> port was random and could not get a given container context path. So the port 
> binding needs to be added.
> 
> 
> Thanks,
> 
> zhang junfan
> 
>

Review Request 69330: OOZIE-3379 Auth token cache file name should include Oozie URL

[zhang junfan]

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

Review request for oozie, András Piros and Peter Bacsko.


Repository: oozie-git


Description
---

Auth token cache file name should include Oozie URL, link to oozie-3379.


Diffs
-

  client/src/main/java/org/apache/oozie/client/AuthOozieClient.java 3a8b5ab6 
  core/src/main/java/org/apache/oozie/test/EmbeddedServletContainer.java 
fc9d840b 
  
core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java 
04fde730 


Diff: https://reviews.apache.org/r/69330/diff/1/


Testing
---

I added a test for authOozieClients with the different oozieUrl.
I changed the code for EmbeddedServletContainer. Because this patch needs to 
bind the cache file to oozieUrl, you need to test whether multiple different 
clients can use the cache. In the original EmbeddedServletContainer code, the 
port was random and could not get a given container context path. So the port 
binding needs to be added.


Thanks,

zhang junfan