push headless endpoint change when headless enabled or dns is enabled #42165
Conversation
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
istio-testing
added
the
size/M
ramaraochavali
added
the
release-notes-none
|
/test release-notes |
pilot/pkg/xds/proxy_dependencies.go
Outdated
| // When endpoints for headless service change, we should push if EnableHeadlessService is true | ||
| // so that we create updated listeners or when DNSCapture is enabled for proxy so that the NDS | ||
| // table is updated. | ||
| if req.Full && len(req.Reason) == 1 && req.Reason[0] == model.HeadlessEndpointUpdate && |
There was a problem hiding this comment.
This is not very right, we could merge two push requests. If so the reasons index can be random
There was a problem hiding this comment.
Ah Right. I missed that part.
There was a problem hiding this comment.
@hzxuzhonghu & @ramaraochavali One more question, I noticed that only one request reason can be added in L#77, so where should more than one requests can be merged? Thanks!
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
|
🤔 🐛 You appear to be fixing a bug in Go code, yet your PR doesn't include updates to any test files. Did you forget to add a test? Courtesy of your friendly test nag. |
pilot/pkg/xds/proxy_dependencies.go
Outdated
| // When endpoints for headless service change, we should push if EnableHeadlessService is true | ||
| // so that we create updated listeners or when DNSCapture is enabled for proxy so that the NDS | ||
| // table is updated. | ||
| if req.Full && headlessEndpointsUpdated(req) && (features.EnableHeadlessService || bool(proxy.Metadata.DNSCapture)) { |
There was a problem hiding this comment.
This may introduce additional full push when EnableHeadlessService = false, actually in this case we only need NDS and EDS?
There was a problem hiding this comment.
But we push NDS only on full push - so this is needed.
There was a problem hiding this comment.
How about updating ndsNeedsPush, push nds on headless service endpoint updated
There was a problem hiding this comment.
But is n't it the same if headlessendpoints trigger full push whether we do it here or in ndsNeedsPush?
There was a problem hiding this comment.
The diff is make full = false and you can set reason = HeadlessEndpoint.
There was a problem hiding this comment.
I think we can make full = false because it is needed when EnableHeadlessService is enabled. But changed a bit - hopefully this is better. PTAL
pilot/pkg/xds/proxy_dependencies.go
Outdated
| @@ -35,6 +35,13 @@ func ConfigAffectsProxy(req *model.PushRequest, proxy *model.Proxy) bool { | |||
| return true | |||
| } | |||
|
|
|||
| // When endpoints for headless service change, we should push if EnableHeadlessService is true | |||
There was a problem hiding this comment.
I am not sure I am following.. since this checks Full && ..., wouldn't we push it anyways? The only way we would filter is if it was filtered out by a Sidecar, but in that case we really don't want to push?
There was a problem hiding this comment.
Are you saying full pushes are always pushed? But when we pushConnection, do not we call this for every proxy?
There was a problem hiding this comment.
No, it isn't always, but ConfigAffectsProxy scopes based on Sidecar which seems fine...?
There was a problem hiding this comment.
I have changed it a bit. Hopefully that should address this part. PTAL and let me know.
There was a problem hiding this comment.
This is what i thought, let @howardjohn give a chance to have a look
|
I will add tests. |
Continuing on #42089, when headless service endpoints change, but features. EnableHeadlessService is off, DNS is enabled, NDS will have stale endpoints.
This PR fixes it.
Please check any characteristics that apply to this pull request.