Skip to content

File change monitoring on s3_auth #8905

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
Merged
randall merged 4 commits into from
Jul 8, 2022
Merged

File change monitoring on s3_auth #8905

randall merged 4 commits into from
Jul 8, 2022

Conversation

@moonchen
Copy link
Contributor

@moonchen moonchen commented Jun 10, 2022

  1. Add a file change monitoring API in iocore.
  2. Use it to monitor config changes for s3_auth via a --watch-config flag.

@moonchen moonchen marked this pull request as draft June 10, 2022 22:17
@bneradt
Copy link
Contributor

bneradt commented Jun 14, 2022

Hi @moonchen. I know it's just a draft, but it might be good to add the license header to the top of the following files so that CI can proceed beyond the RAT (license) check:

https://ci.trafficserver.apache.org/job/Github_Builds/job/rat/1163/console

 Printing headers for text files without a valid license header...
 
=====================================================
== File: ./tests/gold_tests/pluginTest/s3_auth/rules/region_map.conf
=====================================================
127.0.0.1 : us-east-1

=====================================================
== File: ./tests/gold_tests/pluginTest/s3_auth/rules/v4-modified.conf
=====================================================
access_key=1111111
secret_key=5555555
version=4
ttl=0

=====================================================
== File: ./tests/gold_tests/pluginTest/s3_auth/rules/v4.conf
=====================================================
access_key=1234567
secret_key=9999999
version=4
ttl=0
+ grep '0 Unknown Licenses' RAT.txt

@moonchen moonchen marked this pull request as ready for review June 21, 2022 20:08
@randall randall added this to the 10.0.0 milestone Jun 21, 2022
@randall randall assigned randall and moonchen and unassigned randall Jun 21, 2022
@duke8253
Copy link
Contributor

duke8253 commented Jun 27, 2022

[approve ci autest]

@bryancall bryancall requested a review from duke8253 June 27, 2022 23:10
duke8253
duke8253 requested changes Jul 1, 2022
View reviewed changes
Copy link
Contributor

@duke8253 duke8253 left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me, just a few things needs fixing.

moonchen added 4 commits July 7, 2022 15:15
@moonchen
File change monitoring on s3_auth
937c286 
-Add a file change monitoring API
-Use it to monitor config changes for s3_auth
@moonchen
Add license headers to conf files, rename test
a661b25
@moonchen
Remove port from gold test
1c90892
@moonchen
Code Review Changes
2806fd5 
-Remove unused constant
-Newlines
@moonchen moonchen force-pushed the fs-notify-squash branch from e57e862 to 2806fd5 Compare July 7, 2022 20:30
@moonchen
Copy link
Contributor Author

moonchen commented Jul 7, 2022

Addressed comments and rebased.

@moonchen moonchen requested a review from duke8253 July 7, 2022 20:30
duke8253
duke8253 approved these changes Jul 8, 2022
View reviewed changes
Copy link
Contributor

@duke8253 duke8253 left a comment

Choose a reason for hiding this comment

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

looks good.

@randall randall merged commit e93063e into apache:9.2.x Jul 8, 2022
bneradt
bneradt reviewed Jul 11, 2022
View reviewed changes
Copy link
Contributor

@bneradt bneradt left a comment

Choose a reason for hiding this comment

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

Adding a simple observation I noticed when merging this into our internal branch.

tests/gold_tests/pluginTest/s3_auth/gold/s3_auth_basic.gold
< Age: 0
< Transfer-Encoding: chunked
< Connection: keep-alive
< Server: ATS/9.2.0
Copy link
Contributor

@bneradt bneradt Jul 11, 2022

Choose a reason for hiding this comment

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

We'll want to update this so it accommodates other releases, otherwise this test will fail when run on different branches.:

< Server: ``

Copy link
Contributor Author

@moonchen moonchen Jul 11, 2022

Choose a reason for hiding this comment

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

Nice catch! I'll make this change.

tests/gold_tests/pluginTest/s3_auth/gold/s3_auth_basic.gold
< Age: 0
< Transfer-Encoding: chunked
< Connection: keep-alive
< Server: ATS/9.2.0
Copy link
Contributor

@bneradt bneradt Jul 11, 2022

Choose a reason for hiding this comment

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

Same comment as above.

@bneradt
Copy link
Contributor

bneradt commented Jul 12, 2022

We also need to make sure this goes into master. That might require a separate PR since this was filed against 9.2.x?

moonchen added a commit to moonchen/trafficserver that referenced this pull request Jul 13, 2022
@moonchen
File change monitoring on s3_auth (apache#8905)
62e2632
moonchen added a commit to moonchen/trafficserver that referenced this pull request Jul 13, 2022
@moonchen
File change monitoring on s3_auth (apache#8905)
a463153
@moonchen moonchen mentioned this pull request Jul 13, 2022
Closed
@bneradt bneradt modified the milestones: 10.0.0, 9.2.0 Jul 13, 2022
moonchen added a commit to moonchen/trafficserver that referenced this pull request Jul 13, 2022
@moonchen
File change monitoring on s3_auth (apache#8905)
c2a91e9
bneradt added a commit that referenced this pull request Jul 13, 2022
@bneradt
Revert "File change monitoring on s3_auth (#8905)"
cbff2f4 
This reverts commit e93063e.
bneradt pushed a commit to bneradt/trafficserver that referenced this pull request Jul 13, 2022
Revert "File change monitoring on s3_auth (apache#8905)"
63eece7 
This reverts commit e93063e.

PR apache#8905 was accidentally filed against 9.2.x instead of master.
@moonchen filed a new PR for master via apache#8960, so the work will continue
with that PR. Since this patch is not intended for 9.2.x, this PR
reverts the commit.
@bneradt bneradt mentioned this pull request Jul 13, 2022
Closed
@zwoop zwoop removed this from the 9.2.0 milestone Jul 13, 2022
zwoop added a commit that referenced this pull request Jul 13, 2022
@zwoop
Revert "File change monitoring on s3_auth (#8905)"
ccbe928 
This reverts commit e93063e.

This went into the wrong branch, and we'll redo for 10.0.0.
moonchen added a commit to moonchen/trafficserver that referenced this pull request Aug 17, 2022
@moonchen
File change monitoring on s3_auth (apache#8905)
6d6dd02
@zwoop zwoop added this to the 10.0.0 milestone Jan 17, 2023
masaori335 pushed a commit to masaori335/trafficserver that referenced this pull request Feb 21, 2023
@moonchen @zwoop
File change monitoring on s3_auth (apache#8905)
0e429ac
masaori335 pushed a commit to masaori335/trafficserver that referenced this pull request Feb 21, 2023
@zwoop
Revert "File change monitoring on s3_auth (apache#8905)"
d378587 
This reverts commit e93063e.

This went into the wrong branch, and we'll redo for 10.0.0.
masaori335 pushed a commit to masaori335/trafficserver that referenced this pull request Feb 21, 2023
@zwoop
Merge remote-tracking branch 'asf/9.2.x' into 9.2.x
a754dca 
* asf/9.2.x: (22 commits)
  Updated ChangeLog
  Add 5xx's to be allowed to be used for simple retries (apache#8518)
  Extend milestone api time tracking to remap. (apache#8520)
  Destroy ssl context after use. (apache#8531)
  Proxy Verifier: Update to version 2.4.1 (apache#8965)
  Fixes issue with file size calculation for existing logs (apache#8971)
  Fix reverting PR#7302 (apache#8975)
  add a metric to track how often the range seek bug is detected (apache#8970)
  Updated ChangeLog
  Revert "File change monitoring on s3_auth (apache#8905)"
  .fit/fmt/.clang-format-installed prerequisite (apache#8950)
  Make the autopep8 clang-format targets quieter (apache#8944)
  Add option to disable JIT in lua plugin (apache#8618)
  Fix doc formatting for plugin remap_stats (apache#8942)
  Fix doc formatting for rate_limit plugin (apache#8943)
  Clear lua plugin http context after each hook handler (apache#8607)
  LGTM: Remove function declaration in block (HdrHeap.cc) (apache#8588)
  ESI processing when origin returns 304 response (apache#8563)
  call je_dallocx with flags when needed (apache#8547)
  Updated ChangeLog
  ...

 Conflicts:
	CHANGELOG-9.2.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bneradt bneradt bneradt left review comments

@duke8253 duke8253 duke8253 approved these changes

@SolidWallOfCode SolidWallOfCode Awaiting requested review from SolidWallOfCode

@zwoop zwoop Awaiting requested review from zwoop zwoop is a code owner

@bryancall bryancall Awaiting requested review from bryancall bryancall is a code owner

Assignees

@moonchen moonchen

Labels

Plugins

Projects

None yet

Milestone

10.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@moonchen @bneradt @duke8253 @zwoop @randall