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

external-dns should not delete DNS record when HTTPProxy is not completely invalid #5337

Closed
ajayslele opened this issue May 5, 2023 · 9 comments
Labels
area/httpproxy Issues or PRs related to the HTTPProxy API. area/status Issues or PRs related to exposing status for APIs. blocked/needs-design Categorizes the issue or PR as blocked because it needs a design document. kind/feature Categorizes issue or PR as related to a new feature.
Milestone

Comments

@ajayslele
Copy link

If there is incorrect (pointing to non-existing child-proxy) or duplicate (multiple inclusions with same match condition) inclusion on a root-proxy instance, it returns status as invalid. However apart from incorrect/duplicate inclusion, the other inclusions continue to function. In such cases, contour should return status like partially-valid. Other status fields like reason can be used to indicate what is wrong. This will allow integrations like external-dns to distinguish between cases where proxy is completely broken and DNS record for virtualhost fqdn can be removed vs. when only some inclusions/routes are not working and it should continue to maintain DNS record for fqdn so that those inclusions/routes can continue to be used. Right now external-dns removes DNS records for all invalid proxies essentially rendering the entire endpoint useless (see https://github.com/kubernetes-sigs/external-dns/blob/master/source/contour_httpproxy.go#L144).

@sunjayBhatia sunjayBhatia added this to the 1.26.0 milestone May 5, 2023
@sunjayBhatia
Copy link
Member

Adding to 1.26 milestone for prioritization discussion

@sunjayBhatia
Copy link
Member

sunjayBhatia commented May 15, 2023

Some thoughts from triage:

  • in HTTPProxy processing, we need to figure out if a resource has produced some valid config or not (similar to the "Accepted" condition in Gateway API, "syntactically and semantically valid enough to produce some configuration...") to be able to differentiate between "full-valid" and "partially-valid"
  • if we overload the top-level CurrentStatus field/Valid DetailedCondition to mean that "partially-valid" HTTPProxies will be set to valid there are a couple implications
    • the terms are overloaded
    • right now Valid means no Errors, we may have to change that and unwind any internal logic that assumes that
    • reduces visibility for operators, HTTPProxies that needed to be changed because they were invalid might not be visible
  • if we add a new "Programmed" (name TBD) Condition to reflect partially valid
    • we can assume Programmed w/ no Errors is totally valid, Programmed w/ some Errors is partially valid
    • need to teach integrations/operators about this new API surface

@skriss
Copy link
Member

skriss commented May 15, 2023

xref. https://github.com/projectcontour/contour/blob/main/design/httpproxy-conflict.md for some previous thinking on this topic, that is partially implemented.

@skriss skriss added kind/feature Categorizes issue or PR as related to a new feature. area/httpproxy Issues or PRs related to the HTTPProxy API. area/status Issues or PRs related to exposing status for APIs. labels May 15, 2023
@skriss skriss added the blocked/needs-design Categorizes the issue or PR as blocked because it needs a design document. label May 23, 2023
@skriss
Copy link
Member

skriss commented Jun 27, 2023

ref. #2019
ref. #2141

@ajayslele
Copy link
Author

Another option to address the external-dns integration issue mentioned in description can be to provide an additional flag explicitly for create/delete dns record instruction in the proxy status, which external-dns can look at to make the dns record update decision.

@skriss skriss modified the milestones: 1.26.0, 1.27.0 Aug 9, 2023
@skriss skriss self-assigned this Aug 21, 2023
@skriss
Copy link
Member

skriss commented Aug 22, 2023

Another option to address the external-dns integration issue mentioned in description can be to provide an additional flag explicitly for create/delete dns record instruction in the proxy status, which external-dns can look at to make the dns record update decision.

This would likely be a simpler option in the short term. A possible approach would be to have external-dns look for an annotation on HTTPProxies that would define whether to create a DNS record even if the proxy is not marked "valid". I've been looking through external-dns and don't see any existing functionality that exactly matches this, but I'll open a discussion with that project to get their input.

@skriss skriss changed the title Return partially-valid status instead of invalid when proxy is not completely broken external-dns should not delete DNS record when HTTPProxy is not completely invalid Aug 22, 2023
@skriss skriss removed their assignment Aug 23, 2023
@skriss skriss modified the milestones: 1.27.0, 1.28.0 Oct 9, 2023
@lubronzhan
Copy link
Contributor

lubronzhan commented Jan 8, 2024

Might be fixed by kubernetes-sigs/external-dns@42aaa58 (kubernetes-sigs/external-dns#3978)

@skriss
Copy link
Member

skriss commented Jan 9, 2024

Closing this out as it is resolved by the above external-dns PR which will create/keep DNS records for root HTTPProxies regardless of whether they are valid or not.

@skriss skriss closed this as completed Jan 9, 2024
@christianang
Copy link
Contributor

Ended up trying this myself and it should work as expected. Do note though that it will probably be in the next release of external-dns after v0.14.0. The PR @lubronzhan mentioned above was merged a week or so after v0.14.0 was cut.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/httpproxy Issues or PRs related to the HTTPProxy API. area/status Issues or PRs related to exposing status for APIs. blocked/needs-design Categorizes the issue or PR as blocked because it needs a design document. kind/feature Categorizes issue or PR as related to a new feature.
Projects
Status: Done
Development

No branches or pull requests

5 participants