remove finding of pods by IP#56502
Conversation
|
Skipping CI for Draft Pull Request. |
|
/test all |
|
/test all |
|
@howardjohn wdyt? all tests pass correctly 🤔 |
|
https://github.com/istio/istio/pull/28992/files#r535394193 - reason why PodbyIP is still used |
Seems like the concern mostly was backwards compatibility on 2020, which I guess should no longer be the a worry at this point? Don't know how many peope customize proxies to the point that the proxy ID is not in the form we would expect. Maybe I could change the logic to be exclusive, if we find a @howardjohn what's your take on this? |
It is quite possible. I have seen folks do that with a non injected proxy with a custom bootstrap pointing to Istiod for controlplane information. |
We could add an Upgrade Note for next version warning of this change and make them send the correct metadata |
|
Added a feature flag to restore the origina behaviour but it's turned off by default, also added an Upgrade Note to warn users, do you think this is enough? |
|
@sschepens: The following test failed, say
DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
ramaraochavali
left a comment
There was a problem hiding this comment.
Thanks. Curios, Did you run in to any issue and that is wanted to fix or as a general cleanup?
| ).Get() | ||
|
|
||
| EnableProxyFindPodByIP = env.Register("ENABLE_PROXY_FIND_POD_BY_IP", false, | ||
| "If enabled, the pod controller will allow findig pods matching proxies by IP if it fails to find them by name.").Get() |
There was a problem hiding this comment.
nit: /s/findig/finding
|
We found an issue in which apparently our informers were delayed and a new pod came up using the same IP address as a previous one, this caused this piece of code to not find the pod by name and search it by IP, returning an incorrect pod. The most basic fix is #56501 but it's technically not even correct to do that, even if it's in the same namespace it's probably not the pod we want. In our case this ended up making Istiod take the Labels from an incorrect pod and ended up pushing configurations that shouldn't have been pushed to that proxy. |
|
ok. Thanks. @hzxuzhonghu FYI |
* remove finding of pods by IP * fix test * add feature flag and release note
* remove finding of pods by IP * fix test * add feature flag and release note
* remove finding of pods by IP * fix test * add feature flag and release note
* remove finding of pods by IP * fix test * add feature flag and release note
* remove finding of pods by IP * fix test * add feature flag and release note
* remove finding of pods by IP * fix test * add feature flag and release note
* remove finding of pods by IP * fix test * add feature flag and release note
* update native sidecar default value * update webhook and injection * update golden files * release notes * fix test * update golden file * comments and updates * lint * update testcases * lint * lint * remove finding of pods by IP (#56502) * remove finding of pods by IP * fix test * add feature flag and release note * comments and updates * update testcases * update native sidecar default value and related comment updates * update feature file * update golden files * update golden files for openshift * update openshift golden files * update golden file * update default settings * lint --------- Co-authored-by: sschepens <sebastian.schepens@mercadolibre.com>
* remove finding of pods by IP * fix test * add feature flag and release note
* update native sidecar default value * update webhook and injection * update golden files * release notes * fix test * update golden file * comments and updates * lint * update testcases * lint * lint * remove finding of pods by IP (#56502) * remove finding of pods by IP * fix test * add feature flag and release note * comments and updates * update testcases * update native sidecar default value and related comment updates * update feature file * update golden files * update golden files for openshift * update openshift golden files * update golden file * update default settings * lint --------- Co-authored-by: irenezhong2861 <irenezhong@microsoft.com> Co-authored-by: sschepens <sebastian.schepens@mercadolibre.com>
* remove finding of pods by IP * fix test * add feature flag and release note (cherry picked from commit 56454b3)
* upstream/master: (28 commits) Automator: update common-files@master in istio/istio@master (istio#56545) Automator: update proxy@master in istio/istio@master (istio#56544) Automator: update go-control-plane in istio/istio@master (istio#56543) Automator: update proxy@master in istio/istio@master (istio#56540) Automator: update ztunnel@master in istio/istio@master (istio#56532) Ambient: In ambient index, filter configs by revision (istio#56477) Automator: update istio/client-go@master dependency in istio/istio@master (istio#56539) Automator: update proxy@master in istio/istio@master (istio#56538) Automator: update common-files@master in istio/istio@master (istio#56537) optimization: allow for lazy sidecar initialization (istio#47221) static collection eager indexes (istio#56530) fix typo in flag (istio#56534) feat: enable support for proxy protocol on status port (istio#55986) remove finding of pods by IP (istio#56502) Automator: update proxy@master in istio/istio@master (istio#56528) migrate file monitor to krt (istio#55970) Automator: update istio/client-go@master dependency in istio/istio@master (istio#56525) Automator: update ztunnel@master in istio/istio@master (istio#56518) Fix crash in merging http routes (istio#56499) krt: add assertions (istio#56510) ...
Please provide a description of this PR:
Creating this as a draft to check if tests pass, I'm not sure why we still maintain this logic, I cannot understand in which cases we would fail to find a Pod by it's name+namespace but find them by IP.