MAISTRA-1980: Fix private destination rules in root namespace #210
Conversation
Cherry-pick of: istio/istio@831ee9b
Cherry-pick of: istio/istio@c247c88
Cherry-pick of: istio/istio@a497e86 Includes part of: istio/istio@b1ab306
|
@bison: The following test failed, say
Instructions 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/test-infra repository. I understand the commands that are listed here. |
There was a problem hiding this comment.
I don't think this would be very problematic to merge, as its impact is pretty well-defined. it shouldn't impact anything but DestinationRule processing.
Then again though, I'm not sure in which support phase we are with 1.1 - it's likely that we're only fixing security and critical issues at this point, which I wouldn't think this is, as there's a quite simple workaround
| @@ -606,14 +600,6 @@ func (ps *PushContext) GetAllSidecarScopes() map[string][]*SidecarScope { | |||
|
|
|||
| // DestinationRule returns a destination rule for a service name in a given domain. | |||
| func (ps *PushContext) DestinationRule(proxy *Proxy, service *Service) *Config { | |||
| // FIXME: this code should be removed once the EDS issue is fixed | |||
| if proxy == nil { | |||
There was a problem hiding this comment.
I think we can't remove this check in this older version; you can see some unit tests stack tracing. I suppose the mentioned "EDS issue" isn't fixed on this branch yet 👎
There was a problem hiding this comment.
Ah, yeah you're right. I couldn't easily bring over the refactor of services that I think make this unnecessary. Hmm.. not actually sure at first glance what the right thing to do here is.
|
After reviewing this, and some offline discussions. I'm not sure we can fix this without pulling in more changes. Given there's a workaround and the 1.1 release is going into maintenance mode, I'm going to close this. We can always revisit if it's determined to be more critical. |
This is cherry-picks or partially cherry-picks a few upstream commits to try to fix private destination rules in the root namespace for the maistra-1.1 branch.
This seems to fix the issue for me, but I'm actually not convinced we should merge this. It's a fairly big change, and it wasn't possible to fully bring over the changes to this stuff from 1.6 and above without making the diff huge. So, it's hard to say whether this has other side effects. There's also a workaround, which is to simply run the gateway in a namespace that is not the root config namespace. In any case, I wanted to put this PR up so we can decide one way or the other.