CFE-851: Add OCP DNSNameResolver external plugin functionality #1

arkadeepsen · 2023-11-06T15:51:46Z

This PR adds the new external plugin ocp_dnsnameresolver as per the enhancement proposal openshift/enhancements#1335

openshift-ci-robot · 2023-11-06T15:51:50Z

@arkadeepsen: This pull request references CFE-851 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set.

In response to this:

This PR adds the new external plugin ocp_dnsnameresolver as per the enhancement proposal openshift/enhancements#1335

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.

arkadeepsen · 2023-11-06T15:54:55Z

/jira refresh

openshift-ci-robot · 2023-11-06T15:54:58Z

@arkadeepsen: This pull request references CFE-851 which is a valid jira issue.

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.

…n go.mod file.

candita · 2023-11-22T18:58:48Z

/assign @Miciah
/assign @alebedev87

alebedev87

Part 1: dockerfile, makefile, hack scripts and readme. To be continued...

OWNERS

hack/add-plugin.sh

README.md

Makefile

…mt. Updated Dockerfile to use make build-coredns. Updated Makefile to add defaults for some env vars and to add verify rule. Updated isWildcard function to use HasPrefix rather than TrimPrefix and use "*." as the prefix.

Makefile

hack/build-coredns.sh

hack/verify-gofmt.sh

…then it should match with the current one.

arkadeepsen · 2024-02-01T16:59:50Z

/retest

…. Update ocp version from 4.15 to 4.16 in Dockerfile.

npinaeva · 2024-02-05T10:27:38Z

OWNERS

@@ -0,0 +1,34 @@
+reviewers:


I would say from the SDN team side, you could add myself to reviewers and danwinship to reviewers and approvers, since no one else from the team is involved in this effort for now. And we can add more people later if needed.

alebedev87

Part 2. First look at the go code. The handler and the unit tests are not reviewed yet.

hack/build-coredns.sh

setup.go

dnsnameresolver.go

setup.go

…temp dir. Removed the '-mod=vendor' flag from the coredns go build command as coredns already contains the vendor directory.

… in all files. Added description about maps in dnsnameresolve.go.

…dency on the send channel in the initInformer function by using "k8s.io/apimachinery/pkg/util/wait" package functions in the TestServeDNS test function. Refactored updateResolvedNamesSuccess and updateResolvedNamesFailure functions and simplified the code by breaking them down into multple functions.

namespace.go

…ng to kuberenetes plugin for starting server with unsynced informer.

alebedev87

Reviewed the handler's unit test. The coverage looks good to me. Some remarks and questions though.

alebedev87 · 2024-03-14T12:53:39Z

handler_test.go

+				// Create the DNSNameResolver object.
+				_, err := fakeNetworkClient.NetworkV1alpha1().DNSNameResolvers("dns").Create(context.TODO(), &dnsNameResolver, metav1.CreateOptions{})
+				if err != nil {
+					t.Fatalf("error injecting dns name resolver: %v", err)


Suggested change

t.Fatalf("error injecting dns name resolver: %v", err)

t.Fatalf("error creating dns name resolver: %v", err)

handler_test.go

alebedev87 · 2024-03-14T17:59:32Z

handler_test.go

+		},
+	},
+	{
+		name: "Update wildcard dns name resolver object status with wildcard dns name, then don't add regular dns name",


What's the difference from Update wildcard dns name resolver object status with only wildcard dns name case?

Good catch. It is a duplication of the same test. I'll remove this one.

handler_test.go

alebedev87 · 2024-03-14T18:05:43Z

handler_test.go

+				Answer: []dns.RR{
+					test.A("*.example.com. 30 IN A 1.1.1.1"),
+					test.A("*.example.com. 30 IN A 1.1.1.2"),
+				},
+			},
+			{
+				Qname: "www.example.com.",
+				Qtype: dns.TypeA,
+				Rcode: dns.RcodeSuccess,
+				Answer: []dns.RR{
+					test.A("www.example.com. 30 IN A 1.1.1.1"),
+					test.A("www.example.com. 30 IN A 1.1.1.2"),


I think it's better to make the IP addresses different here to make sure they were not mixed up by the implementation.

The IPs are made same to demonstrate that though the details of the regular DNS name is not added to the status of the DNSNameResolver corresponding to the wildcard DNS name, but added to the status of the DNSNameResolver corresponding to the regular DNS name.

This is in line with the previous comment (#1 (comment)) and tests the behavior for a DNSNameResolver object for a regular DNS name.

For the sake of completeness of the behavior: can we have a test case where wildcard and regular name matching the wildcard would have different addresses?

This test case is already added here:https://github.com/openshift/coredns-ocp-dnsnameresolver/pull/1/files#diff-39de3493ae7887b6c07ddf65fc59c005b3f2fccc9b06da022954597de5b12d7cR1526-R1609

alebedev87 · 2024-03-18T13:16:18Z

To sum up what's expected before we can approve and merge the PR:

New test case for the sane dns name from multiple namespaces
Refactor the DNSNameResolver comparison logic (polling and comparison)
Fake plugin maker function
New field for the last lookup time tweaks
Doc update about the plugin's allowed verbs (add/update/delete)
New test case for the record of a type different than A/AAAA (CNAME, NS)
Removal of a duplicate test case
Distance the resolution failure from TTL expiration
New test case for the wildcard and regular name having different target IP addresses
Some comments to be added:
- 1
- 2
- 3

…ddress and DNS name functionality of the plugin. Update handler.go file to take care of edge cases of status update. Update dnsnameresolver.go file to take care of the case of multiple namespaces. Update handle_test.go to use diff on the entire status rather than iterating over each resolved name. Add new test cases for multiple namespaces, and NS and CNAME record types. Add more comments wherever possible to better explain the flow.

…racting the time since last lookup from the current time for the unit tests.

openshift-ci · 2024-03-19T15:45:06Z

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

arkadeepsen · 2024-03-19T18:13:35Z

@alebedev87 I have updated the code to take care of the review comments. The response is inline.

To sum up what's expected before we can approve and merge the PR:

New test case for the sane dns name from multiple namespaces

Added tests for multiple namespaces.

Refactor the DNSNameResolver comparison logic (polling and comparison)

Refactored the comparison logic. Removed the polling as the informer is already synced via the sleep during the execution of the ServeDNS function. Refactor the logic to use diff directly on the status rather than iterating over the resolved name and using diff on each resolved name.

Fake plugin maker function

Updated the name to fakeNextPluginHandler.

New field for the last lookup time tweaks

Added the new field for getting the time since last lookup and that is subtracted from the current time when creating the objects.

Doc update about the plugin's allowed verbs (add/update/delete)

Updated the description section of the README file for adding more details about how the plugin takes care of a DNS lookup.

New test case for the record of a type different than A/AAAA (CNAME, NS)

Test cases for NS and CNAME are added,

Removal of a duplicate test case

The duplicate test case is removed.

Distance the resolution failure from TTL expiration

The TTL of all IP address should have expired so that the plugin can remove the resolved name if the resolution failure becomes greater than or equal to 5 (#1 (comment)).

New test case for the wildcard and regular name having different target IP addresses

The test already exists (#1 (comment)).

Some comments to be added:

1

Added the comment to explain the sleep.

2

Updated the comment stating the reason the why the polling is required.

3

Added the comment to explain why the TTL of the IP addresses was changed to 5 seconds.

alebedev87 · 2024-03-22T15:42:58Z

Thanks @arkadeepsen for the updates! Let's move on with the implementation.

/lgtm
/apptrove

alebedev87 · 2024-03-22T15:43:38Z

/hold

In case @Miciah wants to review too.

alebedev87 · 2024-03-22T18:59:38Z

/unhold

As discussed with Micah.

alebedev87 · 2024-03-22T19:03:39Z

/approve

openshift-ci · 2024-03-22T19:03:58Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: alebedev87

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

Needs approval from an approver in each of these files:

OWNERS

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

snarayan-redhat · 2024-03-25T14:17:29Z

/label docs-approved

CFields651 · 2024-03-25T14:26:06Z

/label px-approved

melvinjoseph86 · 2024-03-26T06:55:42Z

/label qe-approved

openshift-ci-robot · 2024-03-26T06:55:48Z

@arkadeepsen: This pull request references CFE-851 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target either version "4.16." or "openshift-4.16.", but it targets "openshift-4.15" instead.

In response to this:

This PR adds the new external plugin ocp_dnsnameresolver as per the enhancement proposal openshift/enhancements#1335

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 openshift-eng/jira-lifecycle-plugin repository.

arkadeepsen · 2024-03-26T06:59:44Z

/jira refresh

openshift-ci-robot · 2024-03-26T06:59:46Z

@arkadeepsen: This pull request references CFE-851 which is a valid jira issue.

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 openshift-eng/jira-lifecycle-plugin repository.

Add OCP DNSNameResolver external plugin functionality

c366a95

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Nov 6, 2023

arkadeepsen mentioned this pull request Nov 8, 2023

CFE-852: Enable ocp_dnsnameresolver CoreDNS plugin openshift/cluster-dns-operator#393

Merged

Add OWNERS file with NE and SDN team members as reviewers and approvers

b16cf42

arkadeepsen force-pushed the ocp_dnsnameresolver branch from 1c91e30 to b1f95c0 Compare November 22, 2023 15:28

Add build scripts, Dockerfile, Makefile and .gitignore. Use go 1.20 i…

db3ae91

…n go.mod file.

arkadeepsen force-pushed the ocp_dnsnameresolver branch from b1f95c0 to db3ae91 Compare November 22, 2023 15:59

openshift-ci bot assigned alebedev87 and Miciah Nov 22, 2023

alebedev87 reviewed Jan 5, 2024

View reviewed changes

alebedev87 reviewed Jan 18, 2024

View reviewed changes

Makefile Outdated Show resolved Hide resolved

arkadeepsen added 2 commits January 19, 2024 14:50

Remove unused struct. Updated error messages to start with lower case.

1e3416a

alebedev87 reviewed Jan 25, 2024

View reviewed changes

Makefile Outdated Show resolved Hide resolved

alebedev87 reviewed Jan 25, 2024

View reviewed changes

hack/build-coredns.sh Outdated Show resolved Hide resolved

alebedev87 reviewed Jan 25, 2024

View reviewed changes

hack/verify-gofmt.sh Outdated Show resolved Hide resolved

arkadeepsen added 2 commits January 25, 2024 21:30

Updated shell scripts and Makefile

dcfd146

Added code to check if information of DNSNameResolver already exists …

9801219

…then it should match with the current one.

Update golang version from 1.20 to 1.21 in go.mod file and Dockerfile…

63722e1

…. Update ocp version from 4.15 to 4.16 in Dockerfile.

npinaeva reviewed Feb 5, 2024

View reviewed changes

Update SDN team reviewers and approvers in OWNERS file

906f195

arkadeepsen force-pushed the ocp_dnsnameresolver branch from 22982d0 to 906f195 Compare February 6, 2024 05:36

alebedev87 reviewed Feb 15, 2024

View reviewed changes

arkadeepsen added 2 commits February 20, 2024 12:58

Removed the check for non-empty coredns dir inside the newly created …

c87174d

…temp dir. Removed the '-mod=vendor' flag from the coredns go build command as coredns already contains the vendor directory.

Updated error messages in setup.go. Renamed "network" to "ocpnetwork"…

b3f0817

… in all files. Added description about maps in dnsnameresolve.go.

alebedev87 reviewed Mar 13, 2024

View reviewed changes

namespace.go Outdated Show resolved Hide resolved

arkadeepsen mentioned this pull request Mar 13, 2024

CFE-1037: Add DNSNameResolver controller package #2

Merged

Update godoc of "configuredNamespace" function. Add comment referenci…

a2b55f5

…ng to kuberenetes plugin for starting server with unsynced informer.

alebedev87 reviewed Mar 14, 2024

View reviewed changes

arkadeepsen added 3 commits March 19, 2024 18:03

Minor change: remove whitespace.

e7bde2e

Update comment describing the how the last lookup time is set by subt…

371c2a5

…racting the time since last lookup from the current time for the unit tests.

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 Mar 22, 2024

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 22, 2024

arkadeepsen mentioned this pull request Mar 25, 2024

CFE-1007: Temporarily remove approved label for coredns-ocp-dnsnameresolver repo openshift/release#50186

Merged

openshift-ci bot added the docs-approved Signifies that Docs has signed off on this PR label Mar 25, 2024

openshift-ci bot added the px-approved Signifies that Product Support has signed off on this PR label Mar 25, 2024

openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Mar 26, 2024

openshift-merge-bot bot merged commit fc0f617 into openshift:main Mar 26, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CFE-851: Add OCP DNSNameResolver external plugin functionality #1

CFE-851: Add OCP DNSNameResolver external plugin functionality #1

arkadeepsen commented Nov 6, 2023

openshift-ci-robot commented Nov 6, 2023 •

edited by openshift-ci bot

arkadeepsen commented Nov 6, 2023

openshift-ci-robot commented Nov 6, 2023 •

edited by openshift-ci bot

candita commented Nov 22, 2023

alebedev87 left a comment

arkadeepsen commented Feb 1, 2024

npinaeva Feb 5, 2024

alebedev87 left a comment •

edited

alebedev87 left a comment

alebedev87 Mar 14, 2024

alebedev87 Mar 14, 2024

arkadeepsen Mar 15, 2024

alebedev87 Mar 14, 2024

arkadeepsen Mar 15, 2024

alebedev87 Mar 18, 2024

arkadeepsen Mar 18, 2024 •

edited

alebedev87 commented Mar 18, 2024

openshift-ci bot commented Mar 19, 2024

arkadeepsen commented Mar 19, 2024

alebedev87 commented Mar 22, 2024

alebedev87 commented Mar 22, 2024

alebedev87 commented Mar 22, 2024

alebedev87 commented Mar 22, 2024

openshift-ci bot commented Mar 22, 2024

snarayan-redhat commented Mar 25, 2024

CFields651 commented Mar 25, 2024

melvinjoseph86 commented Mar 26, 2024

openshift-ci-robot commented Mar 26, 2024 •

edited by openshift-ci bot

arkadeepsen commented Mar 26, 2024

openshift-ci-robot commented Mar 26, 2024 •

edited by openshift-ci bot

	t.Fatalf("error injecting dns name resolver: %v", err)
	t.Fatalf("error creating dns name resolver: %v", err)

CFE-851: Add OCP DNSNameResolver external plugin functionality #1

CFE-851: Add OCP DNSNameResolver external plugin functionality #1

Conversation

arkadeepsen commented Nov 6, 2023

openshift-ci-robot commented Nov 6, 2023 • edited by openshift-ci bot

arkadeepsen commented Nov 6, 2023

openshift-ci-robot commented Nov 6, 2023 • edited by openshift-ci bot

candita commented Nov 22, 2023

alebedev87 left a comment

Choose a reason for hiding this comment

arkadeepsen commented Feb 1, 2024

npinaeva Feb 5, 2024

Choose a reason for hiding this comment

alebedev87 left a comment • edited

Choose a reason for hiding this comment

alebedev87 left a comment

Choose a reason for hiding this comment

alebedev87 Mar 14, 2024

Choose a reason for hiding this comment

alebedev87 Mar 14, 2024

Choose a reason for hiding this comment

arkadeepsen Mar 15, 2024

Choose a reason for hiding this comment

alebedev87 Mar 14, 2024

Choose a reason for hiding this comment

arkadeepsen Mar 15, 2024

Choose a reason for hiding this comment

alebedev87 Mar 18, 2024

Choose a reason for hiding this comment

arkadeepsen Mar 18, 2024 • edited

Choose a reason for hiding this comment

alebedev87 commented Mar 18, 2024

openshift-ci bot commented Mar 19, 2024

arkadeepsen commented Mar 19, 2024

alebedev87 commented Mar 22, 2024

alebedev87 commented Mar 22, 2024

alebedev87 commented Mar 22, 2024

alebedev87 commented Mar 22, 2024

openshift-ci bot commented Mar 22, 2024

snarayan-redhat commented Mar 25, 2024

CFields651 commented Mar 25, 2024

melvinjoseph86 commented Mar 26, 2024

openshift-ci-robot commented Mar 26, 2024 • edited by openshift-ci bot

arkadeepsen commented Mar 26, 2024

openshift-ci-robot commented Mar 26, 2024 • edited by openshift-ci bot

openshift-ci-robot commented Nov 6, 2023 •

edited by openshift-ci bot

openshift-ci-robot commented Nov 6, 2023 •

edited by openshift-ci bot

alebedev87 left a comment •

edited

arkadeepsen Mar 18, 2024 •

edited

openshift-ci-robot commented Mar 26, 2024 •

edited by openshift-ci bot

openshift-ci-robot commented Mar 26, 2024 •

edited by openshift-ci bot