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.

Sign up for GitHub

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

Jump to bottom

Add the DNS-over-TLS enhancement initial draft #987

Merged
merged 1 commit into from
May 19, 2022
Merged

Add the DNS-over-TLS enhancement initial draft #987

merged 1 commit into from
May 19, 2022

Conversation

brandisher
Copy link
Contributor

No description provided.

@brandisher brandisher force-pushed the NE-750-forward-dns-over-tls-proposal branch from 641459a to a3cd6b2 Compare December 14, 2021 20:46
@brandisher
Copy link
Contributor Author

/cc @Miciah @frobware @candita @knobunc @rfredette

@brandisher
Copy link
Contributor Author

/uncc @russellb @JoelSpeed

@brandisher brandisher changed the title Add the DNS-over-TLS enhancement intial draft Add the DNS-over-TLS enhancement initial draft Jan 11, 2022
Miciah
Miciah reviewed Jan 13, 2022
View reviewed changes
Copy link
Contributor

@Miciah Miciah left a comment

Choose a reason for hiding this comment

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

This is detailed and well written. Looks good overall. I've pointed out a few places where we need to tweak the proposed API or fill in some details.

enhancements/dns/forward-dns-over-tls.md Outdated Show resolved Hide resolved
enhancements/dns/forward-dns-over-tls.md Outdated Show resolved Hide resolved
enhancements/dns/forward-dns-over-tls.md Outdated Show resolved Hide resolved
enhancements/dns/forward-dns-over-tls.md Outdated Show resolved Hide resolved
enhancements/dns/forward-dns-over-tls.md Outdated Show resolved Hide resolved
enhancements/dns/forward-dns-over-tls.md Outdated Show resolved Hide resolved
enhancements/dns/forward-dns-over-tls.md Show resolved Hide resolved
enhancements/dns/forward-dns-over-tls.md Outdated Show resolved Hide resolved
enhancements/dns/forward-dns-over-tls.md Outdated Show resolved Hide resolved
enhancements/dns/forward-dns-over-tls.md Outdated Show resolved Hide resolved
@brandisher brandisher force-pushed the NE-750-forward-dns-over-tls-proposal branch from af75e85 to f6db4ef Compare January 25, 2022 22:08
@brandisher brandisher force-pushed the NE-750-forward-dns-over-tls-proposal branch 5 times, most recently from b7f875d to 923373f Compare February 2, 2022 15:55
@brandisher brandisher mentioned this pull request Feb 3, 2022
Merged
@brandisher brandisher force-pushed the NE-750-forward-dns-over-tls-proposal branch 3 times, most recently from 2aae734 to 8986a06 Compare February 22, 2022 15:39
@brandisher brandisher force-pushed the NE-750-forward-dns-over-tls-proposal branch 3 times, most recently from 72ce808 to 3c28aad Compare February 22, 2022 22:22
@deads2k
Copy link
Contributor

deads2k commented Feb 23, 2022

https://github.com/openshift/enhancements/pull/987/files#r813350736

looks like a blocker. If I want TLS connections, I'm not going to be happy if my secure configuration actually uses an insecure connection.

@brandisher
Copy link
Contributor Author

https://github.com/openshift/enhancements/pull/987/files#r813350736

looks like a blocker. If I want TLS connections, I'm not going to be happy if my secure configuration actually uses an insecure connection.

Totally agree. I'll adjust this as it'd require a workaround on our part to cater to the failed TLS connection so this makes the work simpler and the end result more secure.

@brandisher brandisher force-pushed the NE-750-forward-dns-over-tls-proposal branch from 3c28aad to febad75 Compare February 28, 2022 21:24
@openshift-bot
Copy link

Inactive enhancement proposals go stale after 28d of inactivity.

See https://github.com/openshift/enhancements#life-cycle for details.

Mark the proposal as fresh by commenting /remove-lifecycle stale.
Stale proposals rot after an additional 7d of inactivity and eventually close.
Exclude this proposal from closing by commenting /lifecycle frozen.

If this proposal is safe to close now please do so with /close.

/lifecycle stale

@openshift-ci openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 29, 2022
@brandisher
Copy link
Contributor Author

/remove-lifecycle stale

@openshift-ci openshift-ci bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 29, 2022
@brandisher brandisher force-pushed the NE-750-forward-dns-over-tls-proposal branch from febad75 to cffc110 Compare April 5, 2022 17:04
Miciah
Miciah reviewed Apr 13, 2022
View reviewed changes
enhancements/dns/forward-dns-over-tls.md Outdated Show resolved Hide resolved
enhancements/dns/forward-dns-over-tls.md Outdated Show resolved Hide resolved
enhancements/dns/forward-dns-over-tls.md Outdated Show resolved Hide resolved
enhancements/dns/forward-dns-over-tls.md Outdated Show resolved Hide resolved
enhancements/dns/forward-dns-over-tls.md Outdated Show resolved Hide resolved
@brandisher brandisher force-pushed the NE-750-forward-dns-over-tls-proposal branch from 862e650 to daa4fea Compare May 5, 2022 18:44
@brandisher
Add the DNS-over-TLS enhancement intial draft
7ef1256 
- Address PR feedback for initial proposal
- Add the json annotation for the ServerName member
- Update the non-goals section to be more clear
- Add note about TLS impacting support procedures since packet captures will be encrypted
- Update the servername validation to use a pattern
- Expand on the ServerName use case
- Remove TLSecurityProfile for CoreDNS compatibility
- Switch EnableTLS to Transport
- Narrow Transport validation and add examples
- Note that DNS-over-TLS will stop working post-downgrade
- Fix casing of serverName and caBundle
- Expand the support statement with packet capture details
- Add the answer to the open question about cert storage
- Drop Transport for Upstream type as it can't manifest in a Corefile
- Update the API and YAML references
@brandisher brandisher force-pushed the NE-750-forward-dns-over-tls-proposal branch from daa4fea to 7ef1256 Compare May 5, 2022 19:02
@Miciah
Copy link
Contributor

Miciah commented May 19, 2022

Excellent! Thanks!
/approve
/lgtm

openshift/kubernetes#1247 might warrant a follow-up update to the enhancement.

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 19, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 19, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Miciah

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

The pull request process is described 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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 19, 2022
@Miciah
Copy link
Contributor

Miciah commented May 19, 2022

The linter has a new requirement:

 enhancements/dns/forward-dns-over-tls.md missing "### Workflow Description"

The requirement wasn't there when the PR was posted, and you do provide a user story and plenty of detail, so I'll override the linter CI job.

/override ci/prow/markdownlint

@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 19, 2022

@Miciah: Overrode contexts on behalf of Miciah: ci/prow/markdownlint

In response to this:

The linter has a new requirement:

enhancements/dns/forward-dns-over-tls.md missing "### Workflow Description"

The requirement wasn't there when the PR was posted, and you do provide a user story and plenty of detail, so I'll override the linter CI job.

/override ci/prow/markdownlint

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.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 19, 2022

@brandisher: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@openshift-merge-robot openshift-merge-robot merged commit d09e1dd into openshift:master May 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Miciah Miciah Miciah left review comments

@candita candita candita left review comments

@deads2k deads2k deads2k left review comments

@frobware frobware Awaiting requested review from frobware

@knobunc knobunc Awaiting requested review from knobunc

@rfredette rfredette Awaiting requested review from rfredette

Assignees

@Miciah Miciah

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@brandisher @deads2k @openshift-bot @Miciah @candita @openshift-merge-robot