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

GEP-2627 DNS Policy #2712

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

GEP-2627 DNS Policy #2712

wants to merge 7 commits into from

Conversation

maleck13
Copy link
Contributor

/kind gep

What this PR does / why we need it:
Adds a draft GEP outlining more on the what and why for supporting DNS configuration as part of Gateway API

Fixes #2627

@k8s-ci-robot
Copy link
Contributor

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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.

@k8s-ci-robot k8s-ci-robot added kind/gep PRs related to Gateway Enhancement Proposal(GEP) do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 11, 2024
@k8s-ci-robot
Copy link
Contributor

Hi @maleck13. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 11, 2024
@maleck13 maleck13 changed the title initial wip Draft for GEP-2627 initial Draft for GEP-2627 Jan 12, 2024
@maleck13 maleck13 changed the title initial Draft for GEP-2627 Initial draft for GEP-2627 Jan 12, 2024
Copy link
Contributor

@candita candita left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some grammar fixes needed but generally looks good, could use some clarifications on the goals and use cases.

geps/gep-2627/metadata.yaml Outdated Show resolved Hide resolved
geps/gep-2627/index.md Outdated Show resolved Hide resolved
geps/gep-2627/index.md Outdated Show resolved Hide resolved
geps/gep-2627/index.md Outdated Show resolved Hide resolved
geps/gep-2627/index.md Outdated Show resolved Hide resolved
geps/gep-2627/index.md Outdated Show resolved Hide resolved
geps/gep-2627/index.md Outdated Show resolved Hide resolved
geps/gep-2627/index.md Outdated Show resolved Hide resolved
geps/gep-2627/index.md Outdated Show resolved Hide resolved
geps/gep-2627/index.md Outdated Show resolved Hide resolved
@maleck13 maleck13 changed the title Initial draft for GEP-2627 GEP-2627 DNS Policy Mar 8, 2024
maleck13 and others added 3 commits March 8, 2024 12:42
minor tweaks

Update geps/gep-2627/index.md

Co-authored-by: Candace Holman <[email protected]>

Update geps/gep-2627/index.md

Co-authored-by: Candace Holman <[email protected]>

Update geps/gep-2627/index.md

Co-authored-by: Candace Holman <[email protected]>
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: maleck13
Once this PR has been reviewed and has the lgtm label, please assign shaneutt for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@costinm
Copy link

costinm commented Apr 16, 2024

This needs a very serious security review. As a DNS admin - either public or private - I expect very strict controls on who can create DNS entries affecting the entire domain or VPC. It is certainly not something to enable in all clusters or namespaces - or trust that users have the security expertise to use this safely.

I think external-dns (alpha) CRD is the right approach for users who want to use KRM to manage DNS - the other options allowing random namespace owners to create arbitrary DNS (A and TXT) entries are a huge security risk.

I would be very strongly against this feature - DNS management is a very different thing from Gateway management, the user controlling DNS decides if a specific name (including ability to get ACME certs, etc) can be delegated to a particular Gateway in a particular cluster or not - which in turn allows specific routes (like .well-known/ for getting certs) to be sent to specific namespaces.

The argument that 'user just want an easy way to shoot themselves in the foot and break their own security - because they know what they're doing' is not valid for this particular case, since the implications are far broader than a single cluster.

@costinm
Copy link

costinm commented Apr 16, 2024

I should mention Istio - and any mesh based on interception, or L4-based security mechanisms - is completely dependent on DNS security - both the DNS resolver ( bob.com resolving to evil.com IP ) but also control over creation of entries in private or public DNS.

At the very least this option should not be allowed if any GAMMA implementation is used in the same VPC ( not only cluster).

@maleck13
Copy link
Contributor Author

@costinm Thanks for your response.

This needs a very serious security review. As a DNS admin - either public or private - I expect very strict controls on who can create DNS entries affecting the entire domain or VPC. It is certainly not something to enable in all clusters or namespaces - or trust that users have the security expertise to use this safely. 

I think external-dns (alpha) CRD is the right approach for users who want to use KRM to manage DNS - the other options allowing random namespace owners to create arbitrary DNS (A and TXT) entries are a huge security risk.

Not sure if it is clear, but the proposal and discussion was focused around a new API and as such would be controlled by regular RBAC controls just as creating a Gateway would be or creating an external dns resource would be. So there should be no issue controlling who is allowed to create such an object and in turn control the DNS for a given Gateway's listeners.

@costinm
Copy link

costinm commented May 14, 2024

RBAC is per cluster. Are you saying one zone per cluster ? It also doesn't look inside the resource - how do we limit what names a namespace owner can set ?

A separate CRD for DNS with RBAC is the minimum - but is it in scope for Gateway ( instead of core networking and dns for k8s) ?

@costinm
Copy link

costinm commented May 14, 2024

Btw - the CRD defined by external-dns project seems like a good start and has support for most DNS servers. Any reason users can't just use that ?

@youngnick
Copy link
Contributor

@costinm, this is an initial GEP that lays down the scope and terms to start the discussion of if, and if so, how, we can represent that Gateways should have external DNS configured in a standard, portable way. I agree that there are significant security considerations, and that those security considerations should be included in the GEP.

If you have specific feedback about extra use cases or other concerns that you would like addressed, the correct way to indicate them is via comments on the file, rather than broad generalizations discouraging further work on this.

Once this PR merges and we have a basis for discussion, I'd love for you to shepherd some more changes to it to ensure that your concerns are captured.

@costinm
Copy link

costinm commented May 23, 2024 via email

@costinm
Copy link

costinm commented May 23, 2024 via email

@youngnick
Copy link
Contributor

This work is to provide a standardized way to request DNS records are created for Gateway instances - as you said earlier, basically the same as what external-dns does today. The idea is to see if we can find some common ground and provide a relatively standardized way to communicate this config, that can cover common use cases across multiple providers or implementations.

Some implementations (external-dns for one) are already looking at the Gateway object to provision DNS config, the intent here is to ensure that relevant information has a structured, extensible, portable format that it can be stored in.

The process for GEPs is documented at https://gateway-api.sigs.k8s.io/geps/overview/, and as this is the first stage (moving from discussion into Provisional), the bar is intended to be pretty low. It's intended that we use this chance to agree:

  • that the problem is one we should consider
  • what terms we will use to describe the problem
  • what use cases we are considering

There can be multiple PRs while a GEP is in Provisional, but the intent is to provide a place that is more durable than Github comments to raise concerns, answer questions, and so on.

The best type of feedback here is feedback on the specific file, yes, but I think that questions about if the problem is in scope for Gateway API should be addressed here, so that we can all be on the same page.

I believe the problem of ensuring that there is a reliable way for Gateway owners to request provisioning of DNS records to cover access to the "outside" of the Gateway is in scope for Gateway API. If we agree on that, then the discussion becomes about:

  • what config is needed for the given use cases
  • where that config should live

It could be that minimal extra configuration is needed, but from the fact that external-dns and other projects already have Policy objects to cover this, it seems likely that we will need additional structured configuration of some kind. Additionally, if there are at least three implementations of the same feature, then that is the level at which we will consider doing something not provider-specific, to leave room for other implementations to do the same task in a compatible, portable way.

@costinm
Copy link

costinm commented May 24, 2024 via email

@youngnick
Copy link
Contributor

@costinm, my responses below.

As I mentioned in my previous comment - I believe this is out of scope for the Gateway WG - and perfectly in scope for the 'external DNS WG' - which is focused on representing multiple K8S resources in DNS - as well as the 'Core API WG', which define the core DNS. It looks like the 'external DNS' WG also believes (implicitly) it is in their scope - given that they already implemented support for representing gateway resources among many others.

I disagree. People using this API need to have an answer for how they ask some DNS management system to consider their Gateway in scope for management. Right now, in external-dns, the support only looks at Route resources, which misses the whole model we have for having hostnames match between Gateway and Route. I can see you logged kubernetes-sigs/external-dns#4402 to request adding support for Gateways to the API, which I agree should definitely be supported.

The reason here is that we (Gateway API) have not documented anywhere how consumers of the DNS information already present in the API should use that information. We don't have anything that says "Implementations wanting to do automatic configuration must consider both Gateway and Routes in generating information". It's implied by the spec (since you can't get an address without looking at the Gateway), but we haven't actually said that anywhere.

@maleck13 and the other folks who are working on Kuadrant have also already built a solution for managing the same config, in a different way to the external-dns team. So, some discussion here is required, even if the outcome is that we end up picking one of those implementations as the canonical one. (I think that's unlikely.)

Nobody is suggesting that Gateway API own end-to-end DNS provisioning here. But, as a traffic routing specification, we interact with DNS, and unless we define how that happens, and explicitly rule things out of scope, then we will end up owning it by default. I don't want that, and it seems that you don't either. But that requries us defining what is in scope.

The Gateway WG does have a larger security problem around ownership for hostnames - it does solve some of the 'confused deputy' issues and has some basic delegation/grant model, however the 'persona' who owns the DNS zone and domain ( and indirectly all associated security - ability to get certs for subdomains, etc) is different from the persona configuring a Gateway in a namespace.

Yes, and this is one of the many reasons why we haven't addressed this at all up until now.

Again, this provisional GEP is not suggesting that we solve this problem. In fact, I believe that this should be the responsibility of the implementation that is actuating the creation of DNS records. If a Gateway is created somewhere, and that person shouldn't be creating hostnames there, that's up to the thing that handles DNS creation, not the Gateway API implementation.

Again, this GEP is about defining how other controllers that maybe don't implement routing configuration can manage DNS config sourced from Gateway API resources. Doing this does not mean that north-south Gateway implementations need to support it. Sure, they can if they want to, but the idea here is to ensure that everyone doing it (which, again, multiple implementations already are) does it in the same way.

DNS may span multiple clusters and non-k8s environments - and operates on a top-down model, and needs a consistent security and delegation model across all record types and uses. How to configure DNS (securely) is clearly a very important problem - but best handled in a WG focused on DNS, not in 'leaf' APIs that interact with a single record and small subset - and without a security/IAM model matching DNS needs.

This is not yet a proposal, to be clear. It's an attempt to start talking about what a proposal will involve. The fact that certain questions have been asked and others haven't is exactly what this process is supposed to achieve.

Also, part of the job of a Gateway is to map addresses to routing config. Part of the routing config is the hostname, so it's reasonable natural to assume that people may want to mark hostnames on a Gateway as "please configure these automatically".

I agree that it shouldn't be the job of Gateway API to decide how the requests to map an address to a hostname should be actuated. But it's entirely within our scope to define something that says "use these mappings and not these ones." You're asking us to boil the ocean of solving all DNS management problems, of course that's out of scope. But marking particular mappings is well inside the scope of Gateway API.

The proposal doesn't mention any 'prior art' or existing LB implementation that programs DNS - Istio does handle DNS interception but only for egress use cases. The permissions needed for an in-cluster load balancer to program a customer DNS zone - and the complexity on integrating with the many DNS APIs ( see the list of plugins in external DNS and ACME programs ) makes it unlikely that even 2-3 gateway implementations will be able to do that - while external-dns has a working implementation that can be used with any gateway ( and vendors can provide specialized ones as well - most of the code is related to the 100 DNS APIs).

Yes, because it is at the stage of gathering questions, not proposing solutions.

In general: any GEP that is not based on existing art and features that a significant number of gateways supports ( I would go for 'majority'...) will lead to fragmentation and user pain with the proliferation of optional features - as well as poor APIs that are not based on well established and broadly implemented designs (and I would say in Istio we have a bit of experience with that...)

I don't understand what saying this is intended to achieve. Yes, the point of the GEP process is to ensure that the features we include can be well supported by multiple implementations. Specifications that we define don't have to be implemented by every implementation, and I think External DNS is a great example of functionality that will probably be mostly implemented by a different set of controllers than the ones that provide core routing functions.

These functions may be optional in the spec, but if we don't define them, each implementation will define their own, probably using annotations, and we will be back where we were with Ingress all over again.

I'm about to go and add some suggested changes to capture some of what we've been discussing here. If you wish to discuss this further, I think that we should move it to the community meeting or a separate dedicated meeting to get some higher-bandwidth communication.

Copy link
Contributor

@youngnick youngnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some suggestions based on my discussion with @costinm in the main comments of the PR. Hopefully these capture the flavor of our discussion.

geps/gep-2627/index.md Outdated Show resolved Hide resolved
geps/gep-2627/index.md Outdated Show resolved Hide resolved
geps/gep-2627/index.md Outdated Show resolved Hide resolved
geps/gep-2627/index.md Outdated Show resolved Hide resolved
@shaneutt shaneutt added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 21, 2024
@abursavich
Copy link
Contributor

Right now, in external-dns, the support only looks at Route resources, which misses the whole model we have for having hostnames match between Gateway and Route. I can see you logged kubernetes-sigs/external-dns#4402 to request adding support for Gateways to the API, which I agree should definitely be supported.

Hey... I'm just now seeing this issue/PR for the first time.

I implemented Gateway API support in external-dns and this claim isn't true. It has always respected Listener AllowedRoutes and Hostnames. I'd be happy to hear about and fix any cases where this isn't working properly.

@abursavich
Copy link
Contributor

abursavich commented Jul 14, 2024

To better support DNS use cases, I'd be happy to see a change in Gateway API to allow hostnames in Routes and Listeners that don't need hostnames (i.e. move Hostnames to CommonRouteSpec). It would be mostly backwards compatible because the empty hostname matches everything. The only wrinkle is that the Hostname field in the Listener is documented as "ignored for protocols that don't require hostname based matching." It wouldn't be backwards compatible from a Go package API standpoint, but I don't think that's been a problem before :)

HTTPRoutes, TLSRoutes, and GRPCRoutes and their associated Gateway Listeners can mutually specify hostnames which lets Cluster Operators and Application Developers collaborate to control which hostnames are used. I understand that hostnames would be unavailable in the actual routing path for TCPRoutes and UDPRoutes. However, it could be used in Listener attachment and/or DNS management (the part I care about).

External-DNS does all the things you would hope it would do to determine the correct hostnames for HTTP, TLS, and GRPC Routes. For TCP and UDP Routes it reuses the existing external-dns.alpha.kubernetes.io/hostname annotation on the Routes without any control or validation from the Gateway. The caveat that makes this somewhat more tenable is that external-dns provides a lot of ways to filter (e.g. explicit lists or regular expressions) which domains are allowed and/or blocked.

As far as which WG owns or has claimed this as in their scope... I'm not on any WGs. I'm just some guy who wanted DNS automation for Gateway API to exist, so I implemented it in external-dns three years ago, and I've tried to keep it up to date since then. There was an issue to add support for it, which I think was created by a member of GWAPI. I wrote the code and opened a pull request. I presented my design with a detailed doc at a GWAPI meeting, which went very smoothly, but it took a long time to get the PR merged.

@robscott
Copy link
Member

To better support DNS use cases, I'd be happy to see a change in Gateway API to allow hostnames in Routes and Listeners that don't need hostnames (i.e. move Hostnames to CommonRouteSpec)

@abursavich Listener Hostname is already optional:

// +optional
Hostname *Hostname `json:"hostname,omitempty"`

We do have a requirement that the combination of hostname, protocol, and port is unique among Listeners, is that what you'd like to remove?

@abursavich
Copy link
Contributor

We do have a requirement that the combination of hostname, protocol, and port is unique among Listeners, is that what you'd like to remove?

@robscott, my suggestion is to add hostnames to Routes that don't directly use them: TCPRoute and UDPRoute. I can understand why you might not want to do that. It can't be enforced in the data path and would probably confuse some people, but it would be helpful for DNS.

@youngnick
Copy link
Contributor

One thing that we probably should make more clear is that, for TCPRoute and UDPRoute, you can't have more than one of the type attached to any specific Listener. So adding a hostname to the Routes probably wouldn't do much - for both of those Kinds you need to look at the Listener hostname anyway.

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle rotten
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Sep 21, 2024
@maleck13
Copy link
Contributor Author

/remove-lifecycle rotten

@k8s-ci-robot k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. kind/gep PRs related to Gateway Enhancement Proposal(GEP) needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GEP: DNS configuration as part of Gateway API
9 participants