Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes around workload.hostname #1119

Open
howardjohn opened this issue Jun 6, 2024 · 0 comments
Open

Fixes around workload.hostname #1119

howardjohn opened this issue Jun 6, 2024 · 0 comments

Comments

@howardjohn
Copy link
Member

Currently, Istiod will only generate a hostname workload for SE/WE of DNS type. In ztunnel, we try to match DNS requests to it:

ztunnel/src/dns/server.rs

Lines 390 to 396 in 2e41538

} else if let Some(wl) = state.workloads.find_hostname(&search_name_str) {
// Didn't find a service, try a workload.
return Some(ServerMatch {
server: Address::Workload(wl),
name: search_name,
alias,
});
.

This is broken.

apiVersion: networking.istio.io/v1
kind: ServiceEntry
metadata:
  name: external-service
  namespace: echo
spec:
  addresses:
  - 240.240.240.239
  endpoints:
  - address: external.external.svc.cluster.local
  exportTo:
  - .
  hosts:
  - fake.external.com

This config should NOT trigger responses for 'external.external.svc.cluster.local'.

What we should be allowing is looking up pods in headless services. The logic is tricky, see https://github.com/istio/istio/blob/f69047b03842074eb2f56fef5d9ad7ba326de4cc/pkg/dns/server/name_table.go#L58. We don't do that today.

Finally, WorkloadStore has a by_hostname->Workload. This is not right; a hostname is not unique (definitely not across clusters!).

So a few changes needed

  • Turn off DNS lookups for SE/WE endpoints (ill do this)
  • Turn on DNS lookups for headless pods (not super high priority, we will forward them anyways)
  • Make by_hostname correct, assuming we still need it

Most of by_hostname's usage beyond DNS is in gateway addresses. I am not sure actually these should allow workload hostnames, only service ones. So maybe we can remove it entirely. However, we will probably need it to do headless pods... but we should make sure its not used in all those other cases anyways. So, regardless, we need to clean it up

howardjohn added a commit to howardjohn/ztunnel that referenced this issue Jun 6, 2024
Part 1 of istio#1119. This removes the
broken stuff. TODO is to add it back properly, but for now this PR
strictly removes broken code.
howardjohn added a commit to howardjohn/ztunnel that referenced this issue Jun 6, 2024
Part 1 of istio#1119. This removes the
broken stuff. TODO is to add it back properly, but for now this PR
strictly removes broken code.
howardjohn added a commit to howardjohn/ztunnel that referenced this issue Jun 6, 2024
Part 1 of istio#1119. This removes the
broken stuff. TODO is to add it back properly, but for now this PR
strictly removes broken code.
istio-testing pushed a commit that referenced this issue Jun 7, 2024
Part 1 of #1119. This removes the
broken stuff. TODO is to add it back properly, but for now this PR
strictly removes broken code.
howardjohn added a commit to howardjohn/ztunnel that referenced this issue Jun 7, 2024
Part 1 of istio#1119. This removes the
broken stuff. TODO is to add it back properly, but for now this PR
strictly removes broken code.

(cherry picked from commit fac3e11)
istio-testing pushed a commit that referenced this issue Jun 11, 2024
Part 1 of #1119. This removes the
broken stuff. TODO is to add it back properly, but for now this PR
strictly removes broken code.

(cherry picked from commit fac3e11)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant