Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactors ClientHelper to combine header logic #30620

Merged
merged 2 commits into from
May 16, 2018
Merged

Refactors ClientHelper to combine header logic #30620

merged 2 commits into from
May 16, 2018

Conversation

colings86
Copy link
Contributor

This change removes all the *ClientHelper classes which were
repeating logic between plugins and instead adds
ClientHelper.executeWithHeaders() and
ClientHelper.executeWithHeadersAsync() methods to centralise the
logic for executing requests with stored security headers.

I have request reviews from everyone I think this refractor affects and also @jaymode since this is really a security change in essence.

@colings86 colings86 self-assigned this May 15, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security

droberts195
droberts195 approved these changes May 15, 2018
View reviewed changes
Copy link
Contributor

@droberts195 droberts195 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

jaymode
jaymode approved these changes May 15, 2018
View reviewed changes
Copy link
Member

@jaymode jaymode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@polyfractal
Copy link
Contributor

LGTMToo :)

spinscale
spinscale approved these changes May 15, 2018
View reviewed changes
Copy link
Contributor

@spinscale spinscale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

You could remove Watcher.HEADER_FILTERS and replace with the ClientHelper ones, or I can do that as a follow up.

colings86 added 2 commits May 16, 2018 08:13
@colings86
Refactors ClientHelper to combine header logic
e19360c 
This change removes all the `*ClientHelper` classes which were
repeating logic between plugins and instead adds
`ClientHelper.executeWithHeaders()` and
`ClientHelper.executeWithHeadersAsync()` methods to centralise the
logic for executing requests with stored security headers.
@colings86
Removes Watcher headers constant
ef105f7
@colings86
Copy link
Contributor Author

@elasticmachine retest this please

@colings86 colings86 merged commit a75b8ad into elastic:master May 16, 2018
@colings86 colings86 deleted the refactor/clientHelpers branch May 16, 2018 10:42
colings86 added a commit that referenced this pull request May 16, 2018
@colings86
Refactors ClientHelper to combine header logic (#30620)
24948aa 
* Refactors ClientHelper to combine header logic

This change removes all the `*ClientHelper` classes which were
repeating logic between plugins and instead adds
`ClientHelper.executeWithHeaders()` and
`ClientHelper.executeWithHeadersAsync()` methods to centralise the
logic for executing requests with stored security headers.

* Removes Watcher headers constant

x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/tran
sport/actions/put/TransportPutWatchActionTests.java
/Users/colings86/dev/work/git/elasticsearch/.git/worktrees/elasticsearch
-6.x/CHERRY_PICK_HEAD

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ClientHelp
er.java
x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/MlClien
tHelper.java
x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/MlMetad
ata.java
x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/datafee
d/DatafeedUpdate.java
x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ClientHelp
erTests.java
x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/Transpo
rtPreviewDatafeedAction.java
x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/datafeed/extra
ctor/aggregation/AggregationDataExtractor.java
x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/datafeed/extra
ctor/chunked/ChunkedDataExtractor.java
x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/datafeed/extra
ctor/scroll/ScrollDataExtractor.java
x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/datafeed/extra
ctor/scroll/ScrollDataExtractorFactory.java
x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/MlClientHelper
Tests.java
x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/job/Ro
llupClientHelper.java
x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/job/Ro
llupJobTask.java
x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/job/Ro
llupClientHelperTests.java
x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/Watc
her.java
x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/Watc
herClientHelper.java
x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/acti
ons/index/ExecutableIndexAction.java
x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/inpu
t/search/ExecutableSearchInput.java
x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/tran
sform/search/ExecutableSearchTransform.java
x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/tran
sport/actions/put/TransportPutWatchAction.java
x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/Watc
herClientHelperTests.java
x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/tran
sport/actions/put/TransportPutWatchActionTests.java
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request May 16, 2018
@jasontedor
Merge remote-tracking branch 'elastic/master' into default-copy-setti…
168a56c 
…ngs-to-true

* elastic/master: (34 commits)
  Test: increase search logging for LicensingTests
  Adjust serialization version in IndicesOptions
  [TEST] Fix compilation
  Remove version argument in RangeFieldType (elastic#30411)
  Remove unused DirectoryUtils class. (elastic#30582)
  Mitigate date histogram slowdowns with non-fixed timezones. (elastic#30534)
  Add a MovingFunction pipeline aggregation, deprecate MovingAvg agg (elastic#29594)
  Removes AwaitsFix on IndicesOptionsTests
  Template upgrades should happen in a system context (elastic#30621)
  Fix bug in BucketMetrics path traversal (elastic#30632)
  Fixes IndiceOptionsTests to serialise correctly (elastic#30644)
  S3 repo plugin populate SettingsFilter (elastic#30652)
  mute IndicesOptionsTests.testSerialization
  Rest High Level client: Add List Tasks (elastic#29546)
  Refactors ClientHelper to combine header logic (elastic#30620)
  [ML] Wait for ML indices in rolling upgrade tests (elastic#30615)
  Watcher: Ensure secrets integration tests also run triggered watch (elastic#30478)
  Move allocation awareness attributes to list setting (elastic#30626)
  [Docs] Update code snippet in has-child-query.asciidoc (elastic#30510)
  Replace custom reloadable Key/TrustManager (elastic#30509)
  ...
ywelsch pushed a commit to ywelsch/elasticsearch that referenced this pull request May 23, 2018
@colings86 @ywelsch
Refactors ClientHelper to combine header logic (elastic#30620)
579deef 
* Refactors ClientHelper to combine header logic

This change removes all the `*ClientHelper` classes which were
repeating logic between plugins and instead adds
`ClientHelper.executeWithHeaders()` and
`ClientHelper.executeWithHeadersAsync()` methods to centralise the
logic for executing requests with stored security headers.

* Removes Watcher headers constant
@jpountz
Copy link
Contributor

jpountz commented Jan 30, 2019

@colings86 Can the "backport pending" label be removed now?

@colings86
Copy link
Contributor Author

Yes, sorry it got forgotten but thanks very much for catching this @jpountz

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@droberts195 droberts195 droberts195 approved these changes

@jaymode jaymode jaymode approved these changes

@polyfractal polyfractal polyfractal approved these changes

@spinscale spinscale spinscale approved these changes

Assignees

@colings86 colings86

Labels
>refactoring :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC v6.4.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@colings86 @elasticmachine @polyfractal @jpountz @spinscale @jaymode @droberts195