Skip to content

OCPBUGS-6829: Add protocolStrategy for upstreams in dnses.operator.openshift.io #1429

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

Merged
openshift-merge-robot merged 1 commit into from
May 16, 2023
Merged

OCPBUGS-6829: Add protocolStrategy for upstreams in dnses.operator.openshift.io #1429

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions openapi/generated_openapi/zz_generated.openapi.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 10 additions & 0 deletions openapi/openapi.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23748,6 +23748,11 @@
"description": "policy is used to determine the order in which upstream servers are selected for querying. Any one of the following values may be specified:\n\n* \"Random\" picks a random upstream server for each query. * \"RoundRobin\" picks upstream servers in a round-robin order, moving to the next server for each new query. * \"Sequential\" tries querying upstream servers in a sequential order until one responds, starting with the first server for each new query.\n\nThe default value is \"Random\"",
"type": "string"
},
"protocolStrategy": {
"description": "protocolStrategy specifies the protocol to use for upstream DNS requests. Valid values for protocolStrategy are \"TCP\" and omitted. When omitted, this means no opinion and the platform is left to choose a reasonable default, which is subject to change over time. The current default is to use the protocol of the original client request. \"TCP\" specifies that the platform should use TCP for all upstream DNS requests, even if the client request uses UDP. \"TCP\" is useful for UDP-specific issues such as those created by non-compliant upstream resolvers, but may consume more bandwidth or increase DNS response time. Note that protocolStrategy only affects the protocol of DNS requests that CoreDNS makes to upstream resolvers. It does not affect the protocol of DNS requests between clients and CoreDNS.",
"type": "string",
"default": ""
},
"transportConfig": {
"description": "transportConfig is used to configure the transport type, server name, and optional custom CA or CA bundle to use when forwarding DNS requests to an upstream resolver.\n\nThe default value is \"\" (empty) which results in a standard cleartext connection being used when forwarding DNS requests to an upstream resolver.",
"default": {},
Expand Down Expand Up @@ -27378,6 +27383,11 @@
"description": "Policy is used to determine the order in which upstream servers are selected for querying. Any one of the following values may be specified:\n\n* \"Random\" picks a random upstream server for each query. * \"RoundRobin\" picks upstream servers in a round-robin order, moving to the next server for each new query. * \"Sequential\" tries querying upstream servers in a sequential order until one responds, starting with the first server for each new query.\n\nThe default value is \"Sequential\"",
"type": "string"
},
"protocolStrategy": {
"description": "protocolStrategy specifies the protocol to use for upstream DNS requests. Valid values for protocolStrategy are \"TCP\" and omitted. When omitted, this means no opinion and the platform is left to choose a reasonable default, which is subject to change over time. The current default is to use the protocol of the original client request. \"TCP\" specifies that the platform should use TCP for all upstream DNS requests, even if the client request uses UDP. \"TCP\" is useful for UDP-specific issues such as those created by non-compliant upstream resolvers, but may consume more bandwidth or increase DNS response time. Note that protocolStrategy only affects the protocol of DNS requests that CoreDNS makes to upstream resolvers. It does not affect the protocol of DNS requests between clients and CoreDNS.",
"type": "string",
"default": ""
},
"transportConfig": {
"description": "transportConfig is used to configure the transport type, server name, and optional custom CA or CA bundle to use when forwarding DNS requests to an upstream resolver.\n\nThe default value is \"\" (empty) which results in a standard cleartext connection being used when forwarding DNS requests to an upstream resolver.",
"default": {},
Expand Down
12 changes: 12 additions & 0 deletions operator/v1/0000_70_dns-operator_00.crd.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,12 @@ spec:
- RoundRobin
- Sequential
type: string
protocolStrategy:
description: protocolStrategy specifies the protocol to use for upstream DNS requests. Valid values for protocolStrategy are "TCP" and omitted. When omitted, this means no opinion and the platform is left to choose a reasonable default, which is subject to change over time. The current default is to use the protocol of the original client request. "TCP" specifies that the platform should use TCP for all upstream DNS requests, even if the client request uses UDP. "TCP" is useful for UDP-specific issues such as those created by non-compliant upstream resolvers, but may consume more bandwidth or increase DNS response time. Note that protocolStrategy only affects the protocol of DNS requests that CoreDNS makes to upstream resolvers. It does not affect the protocol of DNS requests between clients and CoreDNS.
enum:
- TCP
- ""
Copy link
Copy Markdown
Contributor

@candita candita Mar 30, 2023

Choose a reason for hiding this comment

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

I think this "" needs to be fixed in the kubebuilder statement, see below.

Copy link
Copy Markdown
Contributor Author

@gcs278 gcs278 Mar 31, 2023

Choose a reason for hiding this comment

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

Addressed below.

type: string
transportConfig:
description: "transportConfig is used to configure the transport type, server name, and optional custom CA or CA bundle to use when forwarding DNS requests to an upstream resolver. \n The default value is \"\" (empty) which results in a standard cleartext connection being used when forwarding DNS requests to an upstream resolver."
properties:
Expand Down Expand Up @@ -172,6 +178,12 @@ spec:
- RoundRobin
- Sequential
type: string
protocolStrategy:
description: protocolStrategy specifies the protocol to use for upstream DNS requests. Valid values for protocolStrategy are "TCP" and omitted. When omitted, this means no opinion and the platform is left to choose a reasonable default, which is subject to change over time. The current default is to use the protocol of the original client request. "TCP" specifies that the platform should use TCP for all upstream DNS requests, even if the client request uses UDP. "TCP" is useful for UDP-specific issues such as those created by non-compliant upstream resolvers, but may consume more bandwidth or increase DNS response time. Note that protocolStrategy only affects the protocol of DNS requests that CoreDNS makes to upstream resolvers. It does not affect the protocol of DNS requests between clients and CoreDNS.
enum:
- TCP
- ""
type: string
transportConfig:
description: "transportConfig is used to configure the transport type, server name, and optional custom CA or CA bundle to use when forwarding DNS requests to an upstream resolver. \n The default value is \"\" (empty) which results in a standard cleartext connection being used when forwarding DNS requests to an upstream resolver."
properties:
Expand Down
54 changes: 54 additions & 0 deletions operator/v1/types_dns.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,25 @@ type ForwardPlugin struct {
//
// +optional
TransportConfig DNSTransportConfig `json:"transportConfig,omitempty"`


// protocolStrategy specifies the protocol to use for upstream DNS
// requests.
// Valid values for protocolStrategy are "TCP" and omitted.
// When omitted, this means no opinion and the platform is left to choose
// a reasonable default, which is subject to change over time.
// The current default is to use the protocol of the original client request.
// "TCP" specifies that the platform should use TCP for all upstream DNS requests,
// even if the client request uses UDP.
// "TCP" is useful for UDP-specific issues such as those created by
// non-compliant upstream resolvers, but may consume more bandwidth or
// increase DNS response time. Note that protocolStrategy only affects
// the protocol of DNS requests that CoreDNS makes to upstream resolvers.
// It does not affect the protocol of DNS requests between clients and
// CoreDNS.
//
// +optional
ProtocolStrategy ProtocolStrategy `json:"protocolStrategy"`
}

// UpstreamResolvers defines a schema for configuring the CoreDNS forward plugin in the
Expand Down Expand Up @@ -329,6 +348,24 @@ type UpstreamResolvers struct {
//
// +optional
TransportConfig DNSTransportConfig `json:"transportConfig,omitempty"`

// protocolStrategy specifies the protocol to use for upstream DNS
// requests.
// Valid values for protocolStrategy are "TCP" and omitted.
// When omitted, this means no opinion and the platform is left to choose
// a reasonable default, which is subject to change over time.
// The current default is to use the protocol of the original client request.
// "TCP" specifies that the platform should use TCP for all upstream DNS requests,
// even if the client request uses UDP.
// "TCP" is useful for UDP-specific issues such as those created by
// non-compliant upstream resolvers, but may consume more bandwidth or
// increase DNS response time. Note that protocolStrategy only affects
// the protocol of DNS requests that CoreDNS makes to upstream resolvers.
// It does not affect the protocol of DNS requests between clients and
// CoreDNS.
//
// +optional
ProtocolStrategy ProtocolStrategy `json:"protocolStrategy"`
}

// Upstream can either be of type SystemResolvConf, or of type Network.
Expand Down Expand Up @@ -376,6 +413,23 @@ const (
NetworkResolverType UpstreamType = "Network"
)

// ProtocolStrategy is a preference for the protocol to use for DNS queries.
Copy link
Copy Markdown
Contributor

@JoelSpeed JoelSpeed May 2, 2023

Choose a reason for hiding this comment

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

What should a consumer of this do if there's a value that they do not recognise? Think about what you'd expect the operator to do if a new value was added to this enum?

Copy link
Copy Markdown
Contributor Author

@gcs278 gcs278 May 3, 2023

Choose a reason for hiding this comment

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

Hm. I figured the consumer (cluster-dns-operator) would just do nothing if it received a value it doesn't recognize. That's how I have it coded right now (it's out-of-date with API, but openshift/cluster-dns-operator#359). By nothing, meaning, it will use the default mode of operation (same as "").

Copy link
Copy Markdown
Contributor

@JoelSpeed JoelSpeed May 4, 2023

Choose a reason for hiding this comment

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

That's fine, I would just add a comment for developers, this is our get out of jail free card for adding a future strategy, as we've already told the consumers what they should be doing if they see a value they don't recognise (which would happen for some consumers on upgrade for example)

Suggested change
// ProtocolStrategy is a preference for the protocol to use for DNS queries.
// ProtocolStrategy is a preference for the protocol to use for DNS queries.
// ---
// + When consumers observe an unknown value, they should use the default strategy.

Copy link
Copy Markdown
Contributor Author

@gcs278 gcs278 May 4, 2023

Choose a reason for hiding this comment

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

Got it, done. Not sure if you literally wanted --- and a +, but let me know if you did.

Copy link
Copy Markdown
Contributor

@JoelSpeed JoelSpeed May 4, 2023

Choose a reason for hiding this comment

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

Yeah I did 😅 Sorry, should have explained, the format I put there means that the doc comment will only be visible here, it prevents the doc being picked up in any swagger or openapi copies of the docs. --- prevents swagger taking anything after it, + stops openapi picking it up

I was wrong though, the --- line needs to be // + ---

Copy link
Copy Markdown
Contributor Author

@gcs278 gcs278 May 4, 2023

Choose a reason for hiding this comment

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

Interesting okay. I pushed up again, let me know if I interpreted you correctly.

Copy link
Copy Markdown
Contributor

@JoelSpeed JoelSpeed May 4, 2023

Choose a reason for hiding this comment

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

Yes, perfect, thanks

// + ---
// + When consumers observe an unknown value, they should use the default strategy.
// +kubebuilder:validation:Enum:=TCP;""
Copy link
Copy Markdown
Member

@ingvagabund ingvagabund Apr 24, 2023

Choose a reason for hiding this comment

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

Sweet. I did not know one can apply the validation directly to a type and have it distributed into both ForwardPlugin and UpstreamResolvers structs.

type ProtocolStrategy string

var (
// ProtocolStrategyDefault specifies no opinion for DNS protocol.
// If empty, the default behavior of CoreDNS is used. Currently, this means that CoreDNS uses the protocol of the
// originating client request as the upstream protocol.
// Note that the default behavior of CoreDNS is subject to change.
ProtocolStrategyDefault ProtocolStrategy = ""

// ProtocolStrategyTCP instructs CoreDNS to always use TCP, regardless of the originating client's request protocol.
ProtocolStrategyTCP ProtocolStrategy = "TCP"
)

// DNSNodePlacement describes the node scheduling configuration for DNS pods.
type DNSNodePlacement struct {
// nodeSelector is the node selector applied to DNS pods.
Expand Down
Loading