Cleanup unused applicationPorts and inbound port meta#17986
Merged
istio-testing merged 2 commits intoistio:masterfrom Oct 25, 2019
Merged
Cleanup unused applicationPorts and inbound port meta#17986istio-testing merged 2 commits intoistio:masterfrom
istio-testing merged 2 commits intoistio:masterfrom
Conversation
Contributor
|
@diemtvu I am pretty sure you used applicationPorts in the past. And I do see TestAuthenticationZ test failure. Could you confirm? |
Contributor
|
@howardjohn Thank you for the clean up! ISTIO_META_INCLUDE_INBOUND_PORTS can be removed as we chatted. But applicationPort is slightly out of control. It's a little more than it was designed as readiness only. I will let @diemtvu to confirm |
Member
Author
|
I think the failure is just a test flake |
hzxuzhonghu
approved these changes
Oct 18, 2019
Member
Author
|
/retest |
rshriram
approved these changes
Oct 21, 2019
Contributor
|
/lgtm FYI + @yangminzhu |
Member
Author
|
@istio/wg-environments-maintainers please take a look |
ayj
approved these changes
Oct 23, 2019
707f845 to
ca3e867
Compare
Member
Author
|
/retest |
Member
Author
|
/retest |
1 similar comment
Member
Author
|
/retest |
Application ports is no longer used for sidecar, we now capture all. For gateway, we added this as a short-sighted hack to work around proxy start up issues. These issues have been fixed in multiple ways (initial_fetch_timeout and checking server_info) so it is no longer required. The INCLUDE_INBOUND_PORTS metadata is never used either, so cleaned that up. Note: this does not remove the annotation, which applies to the iptables, just no longer passes to pilot.
ca3e867 to
508650b
Compare
Member
Author
|
/retest |
ericvn
approved these changes
Oct 25, 2019
This was referenced Oct 26, 2019
howardjohn
added a commit
to howardjohn/istio
that referenced
this pull request
Oct 26, 2019
This was removed in istio#17986. The removal was not neccesarily wrong, but it means that the injector deployment and injection template need to move in lock step, which has some problems. Specifically, the operator did not have charts updated, so it fails now, and anyone using :latest tags will also fail without updating the template. There is a near-zero cost of keeping the function around, so we might as well add it back to make things a bit simpler.
istio-testing
pushed a commit
that referenced
this pull request
Oct 28, 2019
This was removed in #17986. The removal was not neccesarily wrong, but it means that the injector deployment and injection template need to move in lock step, which has some problems. Specifically, the operator did not have charts updated, so it fails now, and anyone using :latest tags will also fail without updating the template. There is a near-zero cost of keeping the function around, so we might as well add it back to make things a bit simpler.
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
12 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.
Application ports is no longer used for sidecar, we now capture all. For
gateway, we added this as a short-sighted hack to work around proxy
start up issues. These issues have been fixed in multiple ways
(initial_fetch_timeout and checking server_info) so it is no longer
required.
The INCLUDE_INBOUND_PORTS metadata is never used either, so cleaned that
up. Note: this does not remove the annotation, which applies to the
iptables, just no longer passes to pilot.
Please provide a description for what this PR is for.
And to help us figure out who should review this PR, please
put an X in all the areas that this PR affects.
[ ] Configuration Infrastructure
[ ] Docs
[ ] Installation
[ ] Networking
[ ] Performance and Scalability
[ ] Policies and Telemetry
[ ] Security
[ ] Test and Release
[ ] User Experience
[ ] Developer Infrastructure