Skip to content

OCPBUGS-6829: Add support for protocolStrategy API field to enable force_tcp configuration#359

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
gcs278:force_tcp
Jun 5, 2023
Merged

OCPBUGS-6829: Add support for protocolStrategy API field to enable force_tcp configuration#359
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
gcs278:force_tcp

Conversation

@gcs278
Copy link
Copy Markdown
Contributor

@gcs278 gcs278 commented Mar 23, 2023

Support for protocolStrategy API field to CoreDNS Corefile that allows force_tcp to be enabled on any upstream server configuration.

API: openshift/api#1429

Adds support for the following fields:

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

@openshift-ci-robot openshift-ci-robot added the jira/severity-important Referenced Jira bug's severity is important for the branch this PR is targeting. label Mar 23, 2023
@openshift-ci 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 23, 2023
@openshift-ci-robot openshift-ci-robot added jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Mar 23, 2023
@openshift-ci-robot
Copy link
Copy Markdown
Contributor

@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:

WIP for adding force_tcp API field to CoreDNS Corefile

API: openshift/api#1429

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 openshift-ci Bot requested review from frobware and knobunc March 23, 2023 21:06
@gcs278
Copy link
Copy Markdown
Contributor Author

gcs278 commented Mar 24, 2023

/jira refresh

@openshift-ci-robot openshift-ci-robot added jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. and removed jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Mar 24, 2023
@openshift-ci-robot
Copy link
Copy Markdown
Contributor

@gcs278: This pull request references Jira Issue OCPBUGS-6829, which is valid. The bug has been moved to the POST state.

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 ASSIGNED, 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.

@gcs278 gcs278 changed the title [WIP] OCPBUGS-6829: Add support for force_tcp API field [WIP] OCPBUGS-6829: Add support for protocolPolicy API field to enable force_tcp configuration Mar 24, 2023
@openshift-ci-robot
Copy link
Copy Markdown
Contributor

@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:

WIP for adding force_tcp API field to CoreDNS Corefile

API: openshift/api#1429

Adds support for 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
Copy link
Copy Markdown
Contributor

@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:

Support for protocolPolicy API field to CoreDNS Corefile that allows force_tcp to be enabled on any upstream server configuration.

API: openshift/api#1429

Adds support for 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.

@gcs278 gcs278 changed the title [WIP] OCPBUGS-6829: Add support for protocolPolicy API field to enable force_tcp configuration OCPBUGS-6829: Add support for protocolPolicy API field to enable force_tcp configuration Mar 28, 2023
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 28, 2023
@gcs278
Copy link
Copy Markdown
Contributor Author

gcs278 commented Mar 28, 2023

/hold
requires API bump openshift/api#1429

@openshift-ci openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 28, 2023
@candita
Copy link
Copy Markdown
Contributor

candita commented Mar 29, 2023

/assign @alebedev87
/assign @Miciah

Copy link
Copy Markdown
Contributor

@alebedev87 alebedev87 left a comment

Choose a reason for hiding this comment

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

LGTM, will wait for the API bump to revisit. Just a small question.

ForwardPlugin: operatorv1.ForwardPlugin{
Upstreams: []string{"1.1.1.1", "2.2.2.2:5353"},
Policy: operatorv1.RoundRobinForwardingPolicy,
ProtocolPolicy: operatorv1.ProtocolPolicyNone,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm wondering whether None wouldn't be the default value in which case it would be tested by the existing forwardplugin and upstreamresolvers test cases.

Copy link
Copy Markdown
Contributor

@alebedev87 alebedev87 Apr 5, 2023

Choose a reason for hiding this comment

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

Update: I think that makes some sense. Sort of double check on the defaulting or omitempty option. This way we have all the three possibilities covered: nothing is specified by the user, none and forcetcp. Don't mind my previous comment.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

With recent updates to this API, which have just merged, we now have "" and TCP. omitempty was removed due to new API conventions (or previously un-followed conventions).
https://github.com/openshift/api/blob/333bc194ef7a13338a4343c25f81ff856ab0d0fd/operator/v1/types_dns.go#L425

@gcs278 gcs278 force-pushed the force_tcp branch 2 times, most recently from dbe5cc4 to 2d80926 Compare May 8, 2023 18:03
@gcs278 gcs278 changed the title OCPBUGS-6829: Add support for protocolPolicy API field to enable force_tcp configuration OCPBUGS-6829: Add support for protocolStrategy API field to enable force_tcp configuration May 8, 2023
@melvinjoseph86
Copy link
Copy Markdown

/test e2e-aws-ovn-upgrade

@melvinjoseph86
Copy link
Copy Markdown

Pre verify the changes with cluster bot and steps are below:
https://polarion.engineering.redhat.com/polarion/#/project/OSE/workitem?id=OCP-63512
/label qe-approved

@openshift-ci openshift-ci Bot added the qe-approved Signifies that QE has signed off on this PR label May 11, 2023
@openshift-ci-robot
Copy link
Copy Markdown
Contributor

@gcs278: An error was encountered querying GitHub for users with public email (mjoseph@redhat.com) for bug OCPBUGS-6829 on the Jira server at https://issues.redhat.com/. No known errors were detected, please see the full error message for details.

Full error message. Post "http://ghproxy/graphql": dial tcp 172.30.229.2:80: i/o timeout

Please contact an administrator to resolve this issue, then request a bug refresh with /jira refresh.

Details

In response to this:

Support for protocolStrategy API field to CoreDNS Corefile that allows force_tcp to be enabled on any upstream server configuration.

API: openshift/api#1429

Adds support for 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.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 17, 2023
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 17, 2023
@gcs278
Copy link
Copy Markdown
Contributor Author

gcs278 commented May 17, 2023

We need to bump k8s to 0.27, but we are currently broke on #368 (comment).

Using #368 to bump k8s.

@melvinjoseph86
Copy link
Copy Markdown

/retest-required

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 26, 2023
…rce_tcp configuration

`go.mod`: Vendor API support for protocolStrategy
`pkg/operator/controller/controller_dns_configmap.go`: Logic to add
force_tcp in corefile if protocolStrategy is TCP
`pkg/operator/controller/controller_dns_configmap_test.go`: Unit tests
`pkg/operator/controller/testdata/*`: New corefiles for unit testing
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 26, 2023
@gcs278
Copy link
Copy Markdown
Contributor Author

gcs278 commented May 26, 2023

/unhold
1.27 rebase has merged. Rebased and ready to go.

@openshift-ci 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 26, 2023
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented May 26, 2023

@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.

@Miciah
Copy link
Copy Markdown
Contributor

Miciah commented May 30, 2023

/approve
/lgtm
/hold
for @alebedev87.

@openshift-ci openshift-ci Bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels May 30, 2023
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented May 30, 2023

[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

Details 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 30, 2023
@alebedev87
Copy link
Copy Markdown
Contributor

/unhold

@openshift-ci openshift-ci Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 5, 2023
@openshift-merge-robot openshift-merge-robot merged commit 5c85a31 into openshift:master Jun 5, 2023
@openshift-ci-robot
Copy link
Copy Markdown
Contributor

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

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

Details

In response to this:

Support for protocolStrategy API field to CoreDNS Corefile that allows force_tcp to be enabled on any upstream server configuration.

API: openshift/api#1429

Adds support for 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. jira/severity-important Referenced Jira bug's severity is important for the branch this PR is targeting. jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. qe-approved Signifies that QE has signed off on this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants