Repository navigation
Conversation
|
Welcome @knoblichd! |
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @knoblichd. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
k8s-ci-robot
added
cncf-cla: no
|
CLA no signed |
|
Not too shure about implementation. Why only for single id, but not for all ids, otherwise the solution is very specific for a single use case. Project may have multiple zones, while we only would like to provide access only to specific zones to external-dns, not specifically one |
Good point — updated to use Get for all configured zone IDs, not just when a single ID is set. When --zone-id-filter is configured with any number of IDs, we now iterate through them calling Get on each and aggregate the results. When no zone ID filter is configured, we fall back to List as before — in that case the operator has intentionally chosen to let external-dns discover and manage all zones in the project, so the project-level permission is unavoidable and expected. This way the permission footprint is proportional to what the operator configured: scoped deployments no longer require project-level list visibility, while unfiltered deployments continue to work unchanged. |
|
Working on CLA. Running down who at our Org will handle that aspect. |
|
I looked at the proposed solution. Looks like there is a problem Example. The added concern - split-horizon zones: If you have a public and private zone with the same DNS name (e.g., "example.com."), they have distinct resource names in GCP (e.g., "my-zone-public", "my-zone-private"). In the old code, List() returns both, then the visibility filter keeps only the private one. In the new code, you'd need to have the exact private zone name in your filter - a suffix like "my-zone" that previously matched both and let the visibility filter do the disambiguation now just 404s on Get. Basically the problem, there is no way to find out where we passed a zone suffix or full zone id. |
Good catch on split-horizon. Updated to handle it: we attempt Get for each zone ID, but if we get a 404 (indicating the ID may be a suffix pattern rather than an exact name), we fall back to List so the existing suffix-match + visibility-filter behavior is fully preserved. Exact zone IDs still benefit from the reduced IAM permissions — suffix patterns degrade gracefully to the previous behavior with no change for existing deployments. EDIT: Why To add context on the motivation: in multi-tenant environments where multiple external-dns instances share a single GCP project, each instance's service account currently requires dns.managedZones.list — a project-level permission. This means any compromised SA can enumerate all zone names across every tenant in the project, enabling cross-environment reconnaissance. By using Get when exact zone IDs are configured, each SA only needs access to its own zone with zero project-level visibility into other tenants. |
fb3a4b9 to
ff778b8
Compare
k8s-ci-robot
added
cncf-cla: yes
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
Coverage Report for CI Build 43293585328Coverage decreased (-0.003%) to 83.468%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions41 previously-covered lines in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
k8s-ci-robot
added
size/L
f74b90f to
1453d0d
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
1453d0d to
a0b844c
Compare
|
Rebased onto current master (23 commits). All prior reviewer feedback from @ivankatliarchuk has been addressed — the implementation now tries Get per configured zone ID and falls back to List on 404 to preserve backward compatibility with suffix-pattern zone IDs. @szuecs @vflaux — would appreciate a review when you get a chance. Happy to make any further changes. |
|
/assign @vflaux /cc @mloiseleur If anyone has any concerns or improvements please let me know. |
mloiseleur
changed the title
| return nil, m.zonesErr | ||
| } | ||
| // Try lookup by zone name first (GCP API accepts both name and numeric ID) | ||
| if zone, ok := testZones[m.project+"/"+m.managedZone]; ok { |
There was a problem hiding this comment.
| if zone, ok := testZones[m.project+"/"+m.managedZone]; ok { | |
| zoneKey := zoneKey(m.project, m.managedZone.Name) | |
| if zone, ok := testZones[zoneKey]; ok { |
|
Would you please also add documentation on this feature? And also about the change on zone match? Note: I updated PR title to reflect that it's a breaking change. The suffix semantic was not really documented nor supported, but still, we may have users relying on it, so we need to document clearly this change. |
mloiseleur
changed the title
kubernetes-prow
Bot
added
docs
cncf-cla: no
|
@mloiseleur Thank you most kindly for the review! I believe I have addressed what you have noted. Please let me know if I have not. Thank you again! |
6f77723 to
009d300
Compare
kubernetes-prow
Bot
added
cncf-cla: yes
|
@knoblichd Did you accidentally remove your changes in Also, I don't think provider-specific behavior should be documented directly in generic flags. This belongs in the provider tutorial. |
|
@vflaux You're right, that last commit reverted the fix by mistake. I've restored the Get-with-List-fallback and applied @mloiseleur's suggestions. The docs are now in the GKE tutorial instead of the generic flag description, including the change to zone ID matching. |
|
I don't see any new commits in the PR. Could you push the changes? |
kubernetes-prow
Bot
added
cncf-cla: no
I do very much apologize. The push failed and I didn't realize it until today. Updates should be there now. |
|
@knoblichd You need to fix commits as described in easycla comment (#6408 (comment)) before we can continue on this PR. |
|
/easycla |
bb3830b to
0bd84aa
Compare
kubernetes-prow
Bot
added
cncf-cla: yes
|
I am very sorry for the delay. I thought I had addressed the commits, but apparently it had failed. Should be good to go now. |
0bd84aa to
ee645bf
Compare
…ingle zone Resolve each configured zone ID with managedZones.get, falling back to managedZones.list on 404, so external-dns can run with permissions scoped to a single zone. An exact zone name now resolves via Get alone and no longer matches suffix siblings. Document zone-scoped IAM in the GKE tutorial.
ee645bf to
fb2b884
Compare
mloiseleur
left a comment
mloiseleur
left a comment
There was a problem hiding this comment.
Thanks for restoring the change.
Design is fine to me: Get per ID, List fallback on 404, breaking change documented.
Remaining asks are about size: extract the Get path into a helper, shorten comments and docs, and assert listCallCount in the two other tests.
Could you also update the PR description? It still says "exactly one zone ID", does not mention the breaking change, and the docs checkbox is unchecked.
| // When zone ID filters are configured, attempt Get for each ID to avoid requiring | ||
| // dns.managedZones.list — a project-level permission that exposes all zone names in | ||
| // the project, enabling cross-environment enumeration in multi-tenant deployments. | ||
| // | ||
| // If Get returns 404 for a zone ID, it may be a suffix pattern (e.g. "my-zone" matching | ||
| // both "public-my-zone" and "private-my-zone" in split-horizon setups). In that case we | ||
| // fall back to List so the existing suffix-match + visibility-filter behavior is preserved. | ||
| // A zone ID that is an exact zone name or numeric ID resolves via Get alone, so it no | ||
| // longer also matches other zones whose names end with it. | ||
| if p.zoneIDFilter.IsConfigured() { | ||
| log.Debugf("Zone ID filters configured %v, attempting Get instead of List", p.zoneIDFilter.ZoneIDs) | ||
|
|
||
| needsList := false | ||
| for _, zoneID := range p.zoneIDFilter.ZoneIDs { | ||
| if zoneID == "" { | ||
| continue | ||
| } | ||
|
|
||
| zone, err := p.managedZonesClient.Get(p.project, zoneID).Do() | ||
| if err != nil { | ||
| var apiErr *googleapi.Error | ||
| if errors.As(err, &apiErr) && apiErr.Code == http.StatusNotFound { | ||
| log.Warnf("Zone %s not found via Get (may be a suffix pattern), falling back to List", zoneID) | ||
| needsList = true | ||
| break | ||
| } | ||
| return nil, provider.NewSoftErrorf("failed to get zone %s: %w", zoneID, err) | ||
| } | ||
|
|
||
| if zone.PeeringConfig == nil && p.domainFilter.Match(zone.DnsName) && p.zoneTypeFilter.Match(zone.Visibility) { | ||
| zones[zone.Name] = zone | ||
| log.Debugf("Matched %s (zone: %s) (visibility: %s)", zone.DnsName, zone.Name, zone.Visibility) | ||
| } else { | ||
| log.Debugf("Filtered %s (zone: %s) (visibility: %s)", zone.DnsName, zone.Name, zone.Visibility) | ||
| } | ||
| } | ||
|
|
||
| if !needsList { | ||
| if len(zones) == 0 { | ||
| log.Warnf("No zones in project %s matched zone ID filters %v and domain filters %v", p.project, p.zoneIDFilter.ZoneIDs, p.domainFilter) | ||
| } | ||
| for _, zone := range zones { | ||
| log.Debugf("Considering zone: %s (domain: %s)", zone.Name, zone.DnsName) | ||
| } | ||
| return zones, nil | ||
| } | ||
|
|
||
| zones = make(map[string]*dns.ManagedZone) |
There was a problem hiding this comment.
Could you move this block into a helper?
The needsList flag, the break, the map reset and the copied "No zones" / "Considering" logs all go away:
| // When zone ID filters are configured, attempt Get for each ID to avoid requiring | |
| // dns.managedZones.list — a project-level permission that exposes all zone names in | |
| // the project, enabling cross-environment enumeration in multi-tenant deployments. | |
| // | |
| // If Get returns 404 for a zone ID, it may be a suffix pattern (e.g. "my-zone" matching | |
| // both "public-my-zone" and "private-my-zone" in split-horizon setups). In that case we | |
| // fall back to List so the existing suffix-match + visibility-filter behavior is preserved. | |
| // A zone ID that is an exact zone name or numeric ID resolves via Get alone, so it no | |
| // longer also matches other zones whose names end with it. | |
| if p.zoneIDFilter.IsConfigured() { | |
| log.Debugf("Zone ID filters configured %v, attempting Get instead of List", p.zoneIDFilter.ZoneIDs) | |
| needsList := false | |
| for _, zoneID := range p.zoneIDFilter.ZoneIDs { | |
| if zoneID == "" { | |
| continue | |
| } | |
| zone, err := p.managedZonesClient.Get(p.project, zoneID).Do() | |
| if err != nil { | |
| var apiErr *googleapi.Error | |
| if errors.As(err, &apiErr) && apiErr.Code == http.StatusNotFound { | |
| log.Warnf("Zone %s not found via Get (may be a suffix pattern), falling back to List", zoneID) | |
| needsList = true | |
| break | |
| } | |
| return nil, provider.NewSoftErrorf("failed to get zone %s: %w", zoneID, err) | |
| } | |
| if zone.PeeringConfig == nil && p.domainFilter.Match(zone.DnsName) && p.zoneTypeFilter.Match(zone.Visibility) { | |
| zones[zone.Name] = zone | |
| log.Debugf("Matched %s (zone: %s) (visibility: %s)", zone.DnsName, zone.Name, zone.Visibility) | |
| } else { | |
| log.Debugf("Filtered %s (zone: %s) (visibility: %s)", zone.DnsName, zone.Name, zone.Visibility) | |
| } | |
| } | |
| if !needsList { | |
| if len(zones) == 0 { | |
| log.Warnf("No zones in project %s matched zone ID filters %v and domain filters %v", p.project, p.zoneIDFilter.ZoneIDs, p.domainFilter) | |
| } | |
| for _, zone := range zones { | |
| log.Debugf("Considering zone: %s (domain: %s)", zone.Name, zone.DnsName) | |
| } | |
| return zones, nil | |
| } | |
| zones = make(map[string]*dns.ManagedZone) | |
| if p.zoneIDFilter.IsConfigured() { | |
| if byID, ok, err := p.zonesByID(); err != nil || ok { | |
| return byID, err | |
| } | |
| } |
// zonesByID resolves zone IDs with Get, so only dns.managedZones.get is needed.
// ok is false when an ID is not an exact zone name or ID; the caller then lists.
func (p *GoogleProvider) zonesByID() (map[string]*dns.ManagedZone, bool, error) {
zones := make(map[string]*dns.ManagedZone)
for _, zoneID := range p.zoneIDFilter.ZoneIDs {
if zoneID == "" {
continue
}
zone, err := p.managedZonesClient.Get(p.project, zoneID).Do()
var apiErr *googleapi.Error
if errors.As(err, &apiErr) && apiErr.Code == http.StatusNotFound {
log.Warnf("Zone %s not found via Get (may be a suffix pattern), falling back to List", zoneID)
return nil, false, nil
}
if err != nil {
return nil, false, provider.NewSoftErrorf("failed to get zone %s: %w", zoneID, err)
}
if zone.PeeringConfig == nil && p.domainFilter.Match(zone.DnsName) && p.zoneTypeFilter.Match(zone.Visibility) {
zones[zone.Name] = zone
log.Debugf("Matched %s (zone: %s) (visibility: %s)", zone.DnsName, zone.Name, zone.Visibility)
} else {
log.Debugf("Filtered %s (zone: %s) (visibility: %s)", zone.DnsName, zone.Name, zone.Visibility)
}
}
if len(zones) == 0 {
log.Warnf("No zones in project %s matched zone ID filters %v and domain filters %v", p.project, p.zoneIDFilter.ZoneIDs, p.domainFilter)
}
return zones, true, nil
}The two-line doc comment replaces the 9-line block comment; the split-horizon and matching details are already in the GKE tutorial.
| // Try lookup by zone name first (GCP API accepts both name and numeric ID) | ||
| if zone, ok := testZones[zoneKey(m.project, m.managedZone)]; ok { | ||
| return zone, nil | ||
| } | ||
| // Fall back to lookup by numeric ID string (GCP API also accepts numeric IDs) | ||
| for key, zone := range testZones { | ||
| if strings.HasPrefix(key, m.project+"/") && fmt.Sprintf("%v", zone.Id) == m.managedZone { | ||
| return zone, nil | ||
| } |
There was a problem hiding this comment.
nit: these comments restate the code.
| // Try lookup by zone name first (GCP API accepts both name and numeric ID) | |
| if zone, ok := testZones[zoneKey(m.project, m.managedZone)]; ok { | |
| return zone, nil | |
| } | |
| // Fall back to lookup by numeric ID string (GCP API also accepts numeric IDs) | |
| for key, zone := range testZones { | |
| if strings.HasPrefix(key, m.project+"/") && fmt.Sprintf("%v", zone.Id) == m.managedZone { | |
| return zone, nil | |
| } | |
| if zone, ok := testZones[zoneKey(m.project, m.managedZone)]; ok { | |
| return zone, nil | |
| } | |
| for key, zone := range testZones { | |
| if strings.HasPrefix(key, m.project+"/") && fmt.Sprintf("%v", zone.Id) == m.managedZone { | |
| return zone, nil | |
| } | |
| } |
| validateZones(t, zones, map[string]*dns.ManagedZone{ | ||
| "internal-2": {Name: "internal-2", DnsName: "cluster.local.", Id: 10002, Visibility: "private"}, | ||
| "internal-3": {Name: "internal-3", DnsName: "cluster.local.", Id: 10003, Visibility: "private"}, | ||
| }) |
There was a problem hiding this comment.
This test is the main goal of the PR, so it should also assert that List is not called:
| }) | |
| }) | |
| assert.Equal(t, 0, provider.managedZonesClient.(*mockManagedZonesClient).listCallCount) |
|
|
||
| validateZones(t, zones, map[string]*dns.ManagedZone{ | ||
| "internal-1": {Name: "internal-1", DnsName: "cluster.local.", Id: 10001, Visibility: "private"}, | ||
| }) |
There was a problem hiding this comment.
| }) | |
| }) | |
| assert.Equal(t, 1, provider.managedZonesClient.(*mockManagedZonesClient).listCallCount) |
| // TestGoogleZonesIDFilterGetFallbackToList verifies that a zone ID suffix pattern that | ||
| // does not resolve via Get (404) falls back to List for backward compatibility. | ||
| // "ernal-1" is a suffix of "internal-1" but not an exact zone name, so Get 404s | ||
| // and we fall back to List which applies the suffix filter as before. |
There was a problem hiding this comment.
nit: the test name already says most of it; one line is enough here (same for L293-294 and L332-334).
| // TestGoogleZonesIDFilterGetFallbackToList verifies that a zone ID suffix pattern that | |
| // does not resolve via Get (404) falls back to List for backward compatibility. | |
| // "ernal-1" is a suffix of "internal-1" but not an exact zone name, so Get 404s | |
| // and we fall back to List which applies the suffix filter as before. | |
| // "ernal-1" is not an exact zone name, so Get returns 404 and List applies the suffix match. |
| ### Restricting access to specific zones | ||
|
|
||
| By default, ExternalDNS lists every managed zone in the project. That requires `dns.managedZones.list`, which can only be granted at project level (for example through `roles/dns.reader` above) and exposes the names of all zones in the project. | ||
|
|
||
| To limit ExternalDNS to specific zones, set `--zone-id-filter` to each zone's exact name or numeric ID, and grant only a zone-level role such as `roles/dns.admin` on those zones (`google_dns_managed_zone_iam_member` above), without the project-level `roles/dns.reader`. | ||
| ExternalDNS then looks up each zone with `dns.managedZones.get` instead of listing. | ||
|
|
||
| !!! warning "Change in zone ID matching" | ||
| `--zone-id-filter` values are matched as a suffix of the zone name or numeric ID, so `public` used to match both `public` and `prod-public`. | ||
| A value that is an exact zone name or ID now matches only that zone. | ||
| A value that matches no zone exactly (a suffix pattern such as `ernal-1`) still falls back to listing, keeps the old suffix behavior, and still needs `dns.managedZones.list`. |
There was a problem hiding this comment.
nit: paragraph 1 and the warning explain List/Get twice. Shorter version:
| ### Restricting access to specific zones | |
| By default, ExternalDNS lists every managed zone in the project. That requires `dns.managedZones.list`, which can only be granted at project level (for example through `roles/dns.reader` above) and exposes the names of all zones in the project. | |
| To limit ExternalDNS to specific zones, set `--zone-id-filter` to each zone's exact name or numeric ID, and grant only a zone-level role such as `roles/dns.admin` on those zones (`google_dns_managed_zone_iam_member` above), without the project-level `roles/dns.reader`. | |
| ExternalDNS then looks up each zone with `dns.managedZones.get` instead of listing. | |
| !!! warning "Change in zone ID matching" | |
| `--zone-id-filter` values are matched as a suffix of the zone name or numeric ID, so `public` used to match both `public` and `prod-public`. | |
| A value that is an exact zone name or ID now matches only that zone. | |
| A value that matches no zone exactly (a suffix pattern such as `ernal-1`) still falls back to listing, keeps the old suffix behavior, and still needs `dns.managedZones.list`. | |
| ### Restricting access to specific zones | |
| Set `--zone-id-filter` to each zone's exact name or numeric ID and grant a zone-level role (`google_dns_managed_zone_iam_member` above) instead of project-level `roles/dns.reader`. | |
| ExternalDNS then uses `dns.managedZones.get` and does not need `dns.managedZones.list`, which exposes every zone name in the project. | |
| !!! warning "Change in zone ID matching" | |
| An exact zone name or ID now matches only that zone (`public` no longer also matches `prod-public`). | |
| A suffix pattern that matches no zone exactly still lists all zones and needs `dns.managedZones.list`. | |
| With only zone-level roles, Cloud DNS returns `403 Forbidden` (not `404`) for a value that is not an exact zone name or ID, so a typo or a suffix pattern shows up as a permission error. |
The last line matters for support: without project-level permissions, Cloud DNS answers 403 for a zone that does not exist, so the 404 fallback never runs and the error looks like a missing IAM binding.
When exactly one zone ID filter is configured,
Zones()unconditionally callsdns.ManagedZones.Listeven though the zone is already known. This requiresdns.managedZones.list— a project-level IAM permission that cannot be scoped to a single zone — exposing all zone names in the GCP project to any entity with that binding.What does it do?
Adds a fast-path in
Zones(): whenzoneIDFiltercontains exactly one ID, calldns.ManagedZones.Getdirectly instead ofdns.ManagedZones.List. This eliminates the need for thedns.managedZones.listpermission entirely, fully scoping the service account to a single zone.Motivation
In multi-tenant GCP environments where multiple external-dns instances share a project, the
listpermission allows a compromised SA to enumerate all zone names project-wide, enabling cross-environment reconnaissance. With this change, each SA only needsdns.managedZones.getscoped to its own zone — it has no visibility into any other zone in the project.More