OCPBUGS-6829: Add protocolStrategy for upstreams in dnses.operator.openshift.io by gcs278 · Pull Request #1429 · openshift/api

gcs278 · 2023-03-22T20:44:11Z

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

protocolStrategy adds the ability to configure the force_tcp CoreDNS Corefile configuration for upstreams, which forces TCP for all upstream DNS requests. This is to resolve issues regarding problematic DNS upstreams as well as a potential UDP reliability problems.

Adds the following fields:

spec.upstreamResolvers.protocolStrategy
- For default (".") usptream server
spec.servers[].forwardPlugin.protocolStrategy
- For custom list of DNS resolvers

openshift-ci-robot · 2023-03-22T20:44:17Z

@gcs278: This pull request references Jira Issue OCPBUGS-6829, which is invalid:

expected the bug to target the "4.14.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

OCPBUGS-6829: Add forceTCP API for dnses.operator.openshift.io

forceTCP turns on the force_tcp CoreDNS Corefile configuration, which forces TCP for all upstream DNS requests. This is to resolve issues regarding problematic DNS upstreams as well as a potential UDP reliability problems.

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 · 2023-03-22T20:44:22Z

Hello @gcs278! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

openshift-ci-robot · 2023-03-24T17:12:14Z

@gcs278: This pull request references Jira Issue OCPBUGS-6829, which is invalid:

expected the bug to target the "4.14.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

OCPBUGS-6829: Add upstreamProtocolPolicy for dnses.operator.openshift.io

forceTCP turns on the force_tcp CoreDNS Corefile configuration, which forces TCP for all upstream DNS requests. This is to resolve issues regarding problematic DNS upstreams as well as a potential UDP reliability problems.

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-robot · 2023-03-24T17:13:05Z

@gcs278: This pull request references Jira Issue OCPBUGS-6829, which is invalid:

expected the bug to target the "4.14.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

OCPBUGS-6829: Add upstreamProtocolPolicy for dnses.operator.openshift.io

upstreamProtocolPolicy adds the ability to configure the force_tcp CoreDNS Corefile configuration, which forces TCP for all upstream DNS requests. This is to resolve issues regarding problematic DNS upstreams as well as a potential UDP reliability problems.

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.

gcs278 · 2023-03-24T17:13:57Z

/jira refresh

openshift-ci-robot · 2023-03-24T17:14:03Z

@gcs278: This pull request references Jira Issue OCPBUGS-6829, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.14.0) matches configured target version for branch (4.14.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Details

In response to this:

/jira refresh

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-robot · 2023-03-24T18:18:14Z

@gcs278: This pull request references Jira Issue OCPBUGS-6829, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.14.0) matches configured target version for branch (4.14.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Details

In response to this:

OCPBUGS-6829: Add protocolPolicy for upstreams in dnses.operator.openshift.io

protocolPolicy adds the ability to configure the force_tcp CoreDNS Corefile configuration for upstreams, which forces TCP for all upstream DNS requests. This is to resolve issues regarding problematic DNS upstreams as well as a potential UDP reliability problems.

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-robot · 2023-03-24T19:03:46Z

@gcs278: This pull request references Jira Issue OCPBUGS-6829, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.14.0) matches configured target version for branch (4.14.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

OCPBUGS-6829: Add protocolPolicy for upstreams in dnses.operator.openshift.io

protocolPolicy adds the ability to configure the force_tcp CoreDNS Corefile configuration for upstreams, which forces TCP for all upstream DNS requests. This is to resolve issues regarding problematic DNS upstreams as well as a potential UDP reliability problems.

Adds the following fields:
spec.upstreamResolvers.protocolPolicy

For default (".") usptream server
spec.servers[].forwardPlugin.protocolPolicy

For custom list of DNS resolvers

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-robot · 2023-03-24T19:04:03Z

@gcs278: This pull request references Jira Issue OCPBUGS-6829, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.14.0) matches configured target version for branch (4.14.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Details

In response to this:

OCPBUGS-6829: Add protocolPolicy for upstreams in dnses.operator.openshift.io

protocolPolicy adds the ability to configure the force_tcp CoreDNS Corefile configuration for upstreams, which forces TCP for all upstream DNS requests. This is to resolve issues regarding problematic DNS upstreams as well as a potential UDP reliability problems.

Adds the following fields:

spec.upstreamResolvers.protocolPolicy

For default (".") usptream server

spec.servers[].forwardPlugin.protocolPolicy

For custom list of DNS resolvers

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.

JoelSpeed · 2023-05-02T16:50:27Z

+
+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


Nit can we be consistent with indentation

Sorry - I actually turned off auto format in GoLand because it kept changing one of the // * formats to remove the bullet points and missed this.

Done.

JoelSpeed · 2023-05-02T16:51:25Z

 	NetworkResolverType   UpstreamType = "Network"
 )

+// ProtocolStrategy is a preference for the protocol to use for DNS queries.


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?

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 "").

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.

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

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 // + ---

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

Yes, perfect, thanks

JoelSpeed · 2023-05-04T14:59:18Z

I don't think this file should be checked in, it should have been removed during the verify stage 🤔

Whoops, not sure how this got here, but removed.

JoelSpeed · 2023-05-04T14:59:29Z

As well, I don't think this should have been checked in

…enshift.io protocolStrategy adds the ability to configure the force_tcp CoreDNS Corefile configuration for upstreams, which forces TCP for all upstream DNS requests. This is to resolve issues regarding problematic DNS upstreams as well as a potential UDP reliability problems.

JoelSpeed · 2023-05-05T10:00:33Z

/approve

/hold

Have you considered getting QE to validate the API changes in tandem with an implementation PR before we merge? Can be useful sometimes to have QE validate the API change via implementation, would not be the first time QE feedback has caused changes to APIs.

If you're confident, feel free to remove the hold

gcs278 · 2023-05-05T13:10:59Z

Have you considered getting QE to validate the API changes in tandem with an implementation PR before we merge? Can be useful sometimes to have QE validate the API change via implementation, would not be the first time QE feedback has caused changes to APIs.

I'm asking QE right now to validate these changes to be safe.

Thanks for the approval

Miciah · 2023-05-08T18:59:13Z

/lgtm

openshift-ci · 2023-05-08T19:05:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: candita, gcs278, JoelSpeed, Miciah

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [JoelSpeed]

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

melvinjoseph86 · 2023-05-10T12:11:51Z

Hi Team,
I tried with the cluster bot and initial test on functionality are all working fine.

We can see the default version as none in the edit section
oc edit dns.operator default
<----snip--->
operatorLogLevel: Normal
upstreamResolvers:
policy: Sequential
protocolStrategy: ""
transportConfig: {}
<----snip--->

melvinjoseph@mjoseph-mac Downloads % oc get dns.operator default -oyaml
<----snip--->
upstreamResolvers:
policy: Sequential
protocolStrategy: ""
transportConfig: {}
<----snip--->

sh-4.4# cat /etc/coredns/Corefile
<----snip--->
forward . /etc/resolv.conf {
policy sequential
}
<----snip--->

Changing the protocolStrategy to `TCP
oc edit dns.operator default
<----snip--->
upstreamResolvers:
policy: Sequential
protocolStrategy: TCP
transportConfig: {}
<----snip--->

melvinjoseph@mjoseph-mac Downloads % oc get dns.operator default -oyaml
<----snip--->
upstreamResolvers:
policy: Sequential
protocolStrategy: TCP
<----snip--->

sh-4.4# cat /etc/coredns/Corefile
<----snip--->
forward . /etc/resolv.conf {
policy sequential
force_tcp
}
<----snip--->

So initial functionality looks good, need to test further on advance end

JoelSpeed · 2023-05-15T11:06:41Z

@Miciah @gcs278 if you're happy with that response from QE, feel free to remove the hold

gcs278 · 2023-05-16T19:51:24Z

@JoelSpeed yes we are, removing the hold, thanks for the reviews.
/unhold

openshift-ci · 2023-05-16T20:17:46Z

@gcs278: all tests passed!

Full PR test history. Your PR dashboard.

Details

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-ci-robot · 2023-05-16T20:18:53Z

@gcs278: Jira Issue OCPBUGS-6829: Some pull requests linked via external trackers have merged:

openshift/api#1429

The following pull requests linked via external trackers have not merged:

openshift/cluster-dns-operator#359 is open

These pull request must merge or be unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with /jira refresh.

Jira Issue OCPBUGS-6829 has not been moved to the MODIFIED state.

Details

In response to this:

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

protocolStrategy adds the ability to configure the force_tcp CoreDNS Corefile configuration for upstreams, which forces TCP for all upstream DNS requests. This is to resolve issues regarding problematic DNS upstreams as well as a potential UDP reliability problems.

Adds the following fields:

spec.upstreamResolvers.protocolStrategy

For default (".") usptream server

spec.servers[].forwardPlugin.protocolStrategy

For custom list of DNS resolvers

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.

gcs278 · 2023-06-20T21:20:43Z

/cherry-pick release-4.13

openshift-cherrypick-robot · 2023-06-20T21:21:32Z

@gcs278: new pull request created: #1500

Details

In response to this:

/cherry-pick release-4.13

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.

gcs278 · 2023-06-21T13:18:24Z

/cherry-pick release-4.12

openshift-cherrypick-robot · 2023-06-21T13:19:19Z

@gcs278: new pull request created: #1502

Details

In response to this:

/cherry-pick release-4.12

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-robot added jira/severity-important Referenced Jira bug's severity is important for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. labels Mar 22, 2023

openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 22, 2023

openshift-ci-robot added the jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. label Mar 22, 2023

openshift-ci Bot requested review from JoelSpeed and adambkaplan March 22, 2023 20:44

gcs278 mentioned this pull request Mar 23, 2023

OCPBUGS-6829: Add support for protocolStrategy API field to enable force_tcp configuration openshift/cluster-dns-operator#359

Merged

gcs278 commented Mar 24, 2023

View reviewed changes

Comment thread operator/v1/0000_70_dns-operator_00.crd.yaml Outdated

gcs278 commented Mar 24, 2023

View reviewed changes

Comment thread operator/v1/0000_70_dns-operator_00.crd.yaml Outdated

gcs278 force-pushed the dns-force_tcp branch 3 times, most recently from fa7abf7 to bdc74da Compare March 24, 2023 16:32

gcs278 changed the title ~~[WIP] OCPBUGS-6829: Add forceTCP API for dnses.operator.openshift.io~~ [WIP] OCPBUGS-6829: Add upstreamProtocolPolicy for dnses.operator.openshift.io Mar 24, 2023

gcs278 force-pushed the dns-force_tcp branch 3 times, most recently from a66cde7 to 1226985 Compare March 24, 2023 18:17

gcs278 changed the title ~~[WIP] OCPBUGS-6829: Add upstreamProtocolPolicy for dnses.operator.openshift.io~~ [WIP] OCPBUGS-6829: Add protocolPolicy for upstreams in dnses.operator.openshift.io Mar 24, 2023

gcs278 force-pushed the dns-force_tcp branch from 1226985 to be0097d Compare March 24, 2023 18:21

openshift-ci Bot assigned JoelSpeed Apr 25, 2023

gcs278 force-pushed the dns-force_tcp branch from 62d426d to 3c38729 Compare April 25, 2023 14:22

JoelSpeed reviewed May 2, 2023

View reviewed changes

gcs278 force-pushed the dns-force_tcp branch 3 times, most recently from 9247495 to 962ca3e Compare May 4, 2023 14:55

JoelSpeed reviewed May 4, 2023

View reviewed changes

gcs278 force-pushed the dns-force_tcp branch from 962ca3e to 0e0a453 Compare May 4, 2023 17:58

openshift-ci Bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 4, 2023

openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 5, 2023

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label May 8, 2023

openshift-ci Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 16, 2023

openshift-merge-robot merged commit 333bc19 into openshift:master May 16, 2023

openshift-cherrypick-robot mentioned this pull request Jun 20, 2023

[release-4.13] OCPBUGS-15225: Add protocolStrategy for upstreams in dnses.operator.openshift.io #1500

Merged

openshift-cherrypick-robot mentioned this pull request Jun 21, 2023

[release-4.12] OCPBUGS-15251: Add protocolStrategy for upstreams in dnses.operator.openshift.io #1502

Merged

Conversation

gcs278 commented Mar 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Mar 22, 2023

Uh oh!

openshift-ci Bot commented Mar 22, 2023

Uh oh!

Uh oh!

Uh oh!

openshift-ci-robot commented Mar 24, 2023

Uh oh!

openshift-ci-robot commented Mar 24, 2023

Uh oh!

gcs278 commented Mar 24, 2023

Uh oh!

openshift-ci-robot commented Mar 24, 2023

Uh oh!

openshift-ci-robot commented Mar 24, 2023

Uh oh!

openshift-ci-robot commented Mar 24, 2023

Uh oh!

openshift-ci-robot commented Mar 24, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoelSpeed commented May 5, 2023

Uh oh!

gcs278 commented May 5, 2023

Uh oh!

Miciah commented May 8, 2023

Uh oh!

openshift-ci Bot commented May 8, 2023

Uh oh!

melvinjoseph86 commented May 10, 2023

Uh oh!

JoelSpeed commented May 15, 2023

Uh oh!

gcs278 commented May 16, 2023

Uh oh!

openshift-ci Bot commented May 16, 2023

Uh oh!

openshift-ci-robot commented May 16, 2023

Uh oh!

gcs278 commented Jun 20, 2023

Uh oh!

openshift-cherrypick-robot commented Jun 20, 2023

Uh oh!

gcs278 commented Jun 21, 2023

Uh oh!

openshift-cherrypick-robot commented Jun 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

gcs278 commented Mar 22, 2023 •

edited

Loading