pilot: switch to incremental Service Account building#39133
Merged
istio-testing merged 11 commits intoJun 29, 2022
Conversation
howardjohn
added
the
release-notes-none
Collaborator
|
Skipping CI for Draft Pull Request. |
istio-testing
added
do-not-merge/work-in-progress
Member
Author
|
Attempt to fix #38709 /test all |
istio-policy-bot
added
the
lifecycle/stale
howardjohn
changed the title
howardjohn
marked this pull request as ready for review
June 28, 2022 22:40
istio-testing
removed
the
do-not-merge/work-in-progress
howardjohn
force-pushed
the
pilot/use-consistent-sa-view
branch
from
June 28, 2022 22:48
9c8df7e to
58ebf9b
Compare
Contributor
|
Changes LGTM. UT failure seems related. And do we have a test that validates this behaviour ? |
istio-policy-bot
removed
the
lifecycle/stale
Member
Author
|
Yeah we have a few existing tests that validate the SAN matching (I know since most of them failed previously when I missed some parts ) and I added a test to validate it is incremental |
ramaraochavali
approved these changes
Jun 29, 2022
lei-tang
approved these changes
Jun 29, 2022
istio-testing
removed
the
size/L
istio-testing
added
the
size/XL
howardjohn
added a commit
to howardjohn/istio
that referenced
this pull request
Jul 1, 2022
This impacts tests only. In istio#39133 we changed to notify on writes; this ended up triggering a ton of test flake. Instead, just add explicit functions to notify (new test needs them) and revert the other changes
howardjohn
added a commit
that referenced
this pull request
Jul 1, 2022
* Revert changes to always notify XdsUpdater in mem discovery This impacts tests only. In #39133 we changed to notify on writes; this ended up triggering a ton of test flake. Instead, just add explicit functions to notify (new test needs them) and revert the other changes * conflict
This was referenced Sep 7, 2022
Merged
howardjohn
added a commit
to howardjohn/istio
that referenced
this pull request
Sep 8, 2022
Fixes istio#39652 This reverts istio#36882. At the time, that PR was needed because EDS ServiceAccounts and CDS ServiceAccounts were decoupled; Since istio#39133, this is no longer true, and the fix in 36882 is not needed any longer. This PR *removes* the test added in 36882 (since it tests low level details that are not relevant anymore). It improves the existing TestEndpointFlipFlops test -- while that test *would* have caught the regression, it didn't actually set any service accounts so it was missed. The update changes it to correctly detect the behavior (it now fails without this PR, passes with it).
istio-testing
pushed a commit
that referenced
this pull request
Sep 12, 2022
* Disable full push on scale from 1->0->1 Fixes #39652 This reverts #36882. At the time, that PR was needed because EDS ServiceAccounts and CDS ServiceAccounts were decoupled; Since #39133, this is no longer true, and the fix in 36882 is not needed any longer. This PR *removes* the test added in 36882 (since it tests low level details that are not relevant anymore). It improves the existing TestEndpointFlipFlops test -- while that test *would* have caught the regression, it didn't actually set any service accounts so it was missed. The update changes it to correctly detect the behavior (it now fails without this PR, passes with it). * add note * fix note * Add new SA case
howardjohn
added a commit
to howardjohn/istio
that referenced
this pull request
Sep 12, 2022
* Disable full push on scale from 1->0->1 Fixes istio#39652 This reverts istio#36882. At the time, that PR was needed because EDS ServiceAccounts and CDS ServiceAccounts were decoupled; Since istio#39133, this is no longer true, and the fix in 36882 is not needed any longer. This PR *removes* the test added in 36882 (since it tests low level details that are not relevant anymore). It improves the existing TestEndpointFlipFlops test -- while that test *would* have caught the regression, it didn't actually set any service accounts so it was missed. The update changes it to correctly detect the behavior (it now fails without this PR, passes with it). * add note * fix note * Add new SA case (cherry picked from commit 9f1cbfb)
istio-testing
pushed a commit
that referenced
this pull request
Sep 13, 2022
* SAN improvements (#40863) * wip * wip * Add tests * drop logs * revert changes * Add one more case * Add namespace into SA key as well * Add back svc.ServiceAccounts * add note * lint (cherry picked from commit 0abe39d) * Disable full push on scale from 1->0->1 (#40866) * Disable full push on scale from 1->0->1 Fixes #39652 This reverts #36882. At the time, that PR was needed because EDS ServiceAccounts and CDS ServiceAccounts were decoupled; Since #39133, this is no longer true, and the fix in 36882 is not needed any longer. This PR *removes* the test added in 36882 (since it tests low level details that are not relevant anymore). It improves the existing TestEndpointFlipFlops test -- while that test *would* have caught the regression, it didn't actually set any service accounts so it was missed. The update changes it to correctly detect the behavior (it now fails without this PR, passes with it). * add note * fix note * Add new SA case (cherry picked from commit 9f1cbfb)
12 tasks
yxun
pushed a commit
to yxun/maistra-istio
that referenced
this pull request
Sep 28, 2023
* Disable full push on scale from 1->0->1 Fixes istio/istio#39652 This reverts istio/istio#36882. At the time, that PR was needed because EDS ServiceAccounts and CDS ServiceAccounts were decoupled; Since istio/istio#39133, this is no longer true, and the fix in 36882 is not needed any longer. This PR *removes* the test added in 36882 (since it tests low level details that are not relevant anymore). It improves the existing TestEndpointFlipFlops test -- while that test *would* have caught the regression, it didn't actually set any service accounts so it was missed. The update changes it to correctly detect the behavior (it now fails without this PR, passes with it). * add note * fix note * Add new SA case
12 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is both a bug fix and a long standing optimization.
Fixes #38709 - I recommend reading the last few notes their to understand why its broken today. But basically we don't update Service Accounts when they change, only when we happen to trigger a full push at a later time. In some circumstances this delta causes critical issues.