Skip to content

secret rotate should execute sequentially(fix TestRefreshSecret )#9055

Merged
rshriram merged 1 commit intoistio:masterfrom
hzxuzhonghu:secret-rotate
Oct 15, 2018
Merged

secret rotate should execute sequentially(fix TestRefreshSecret )#9055
rshriram merged 1 commit intoistio:masterfrom
hzxuzhonghu:secret-rotate

Conversation

@hzxuzhonghu
Copy link
Copy Markdown
Member

@hzxuzhonghu hzxuzhonghu commented Sep 30, 2018 •
edited
Loading

  1. As we startup goroutines during rotate, so wait for all routines exit.

  2. sync.Map.Range(func()...) should not call sync.Map.Store in callback func.

The racetest TestRefreshSecret failure maybe related to this.

@googlebot
Copy link
Copy Markdown
Collaborator

googlebot commented Sep 30, 2018

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

@googlebot googlebot added the cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. label Sep 30, 2018
@googlebot
Copy link
Copy Markdown
Collaborator

googlebot commented Sep 30, 2018

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. labels Sep 30, 2018
@codecov
Copy link
Copy Markdown

codecov bot commented Sep 30, 2018 •
edited
Loading

Codecov Report

Merging #9055 into master will decrease coverage by 1%.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #9055    +/-   ##
=======================================
- Coverage      70%     69%   -<1%     
=======================================
  Files         412     413     +1     
  Lines       37774   38153   +379     
=======================================
+ Hits        26196   26244    +48     
- Misses      10319   10654   +335     
+ Partials     1259    1255     -4
Impacted Files Coverage Δ
security/pkg/nodeagent/cache/secretcache.go 0% <0%> (-69%) ⬇️
mixer/pkg/protobuf/yaml/resolver.go 95% <0%> (-5%) ⬇️
pilot/pkg/model/authentication.go 84% <0%> (-1%) ⬇️
mixer/adapter/servicecontrol/testhelper.go 70% <0%> (-1%) ⬇️
mixer/adapter/servicecontrol/quotaprocessor.go 83% <0%> (ø) ⬇️
mixer/adapter/kubernetesenv/cache.go 89% <0%> (ø) ⬇️
mixer/adapter/signalfx/signalfx.go 83% <0%> (ø) ⬇️
...olarwinds/internal/papertrail/papertrail_logger.go 81% <0%> (ø) ⬇️
mixer/adapter/fluentd/fluentd.go 76% <0%> (ø) ⬇️
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d2abca4...7e24358. Read the comment docs.

@hzxuzhonghu
Copy link
Copy Markdown
Member Author

hzxuzhonghu commented Sep 30, 2018

/assign @diemtvu

@hzxuzhonghu hzxuzhonghu changed the title secret rotate should keep insync secret rotate should execute one by one Oct 1, 2018
@hzxuzhonghu hzxuzhonghu changed the title secret rotate should execute one by one secret rotate should execute sequentially Oct 1, 2018
@myidpt myidpt requested a review from quanjielin October 3, 2018 06:01
@hzxuzhonghu
Copy link
Copy Markdown
Member Author

hzxuzhonghu commented Oct 8, 2018

/test e2e-dashboard

quanjielin
quanjielin approved these changes Oct 8, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@quanjielin quanjielin left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@istio-testing
Copy link
Copy Markdown
Collaborator

istio-testing commented Oct 8, 2018

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: quanjielin
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: diemtvu

If they are not already assigned, you can assign the PR to them by writing /assign @diemtvu in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@hzxuzhonghu
Copy link
Copy Markdown
Member Author

hzxuzhonghu commented Oct 9, 2018

@rshriram for approval

@hzxuzhonghu
Copy link
Copy Markdown
Member Author

hzxuzhonghu commented Oct 10, 2018

@diemtvu @myidpt can you approve?

hzxuzhonghu
hzxuzhonghu commented Oct 10, 2018
View reviewed changes
security/pkg/nodeagent/cache/secretcache.go
log.Debug("Refresh job running")

secretMap := map[ConnKey]*model.SecretItem{}
wg := sync.WaitGroup{}
Copy link
Copy Markdown
Member Author

@hzxuzhonghu hzxuzhonghu Oct 10, 2018

Choose a reason for hiding this comment

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

Original logic indeed has a potential issue: old rotate goroutine may not exit when new rotate takes place

@hzxuzhonghu hzxuzhonghu changed the title secret rotate should execute sequentially secret rotate should execute sequentially(fix TestRefreshSecret ) Oct 15, 2018
@hzxuzhonghu
Copy link
Copy Markdown
Member Author

hzxuzhonghu commented Oct 15, 2018

/assign @rshriram

@rshriram rshriram merged commit c7b3cce into istio:master Oct 15, 2018
@hzxuzhonghu hzxuzhonghu deleted the secret-rotate branch October 16, 2018 01:17
This was referenced Apr 15, 2019
Merged
Closed
brian-avery pushed a commit to brian-avery/istio that referenced this pull request Apr 15, 2021
@istio-testing @howardjohn
[release-1.9] Add blog for zero configuration Istio (istio#9055)
dc607bd 
* Add blog for zero configuration Istio

The intent here is to show off what Istio provides out of the box, to
attempt to counteract some of the reputation Istio has gotten for being
over complicated/requiring too many CRDs.

* fix links

* Address comments

* Apply suggestions from code review

Co-authored-by: craigbox <craigbox@google.com>

Co-authored-by: John Howard <howardjohn@google.com>
Co-authored-by: craigbox <craigbox@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@geeknoid geeknoid Awaiting requested review from geeknoid

@linsun linsun Awaiting requested review from linsun

@myidpt myidpt Awaiting requested review from myidpt

2 more reviewers

@rshriram rshriram rshriram approved these changes

@quanjielin quanjielin quanjielin approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@rshriram rshriram

@diemtvu diemtvu

@quanjielin quanjielin

Labels

kind/test failure

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@hzxuzhonghu @googlebot @istio-testing @rshriram @quanjielin @diemtvu