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.

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

Bump to coredns/coredns v1.3.1 #6

Merged

Conversation

bbrowning
Copy link

This bumps our fork to coredns/coredns v1.3.1 while maintaining our patches as well.

Specifically:

  • Our custom Dockerfiles are unchanged
  • Kubernetes client version is unchanged
  • All deps are vendored, even the ones upstream marks as ignored - in the same spirit as Make ready to build for OpenShift - add a Dockerfile #1
    • this required overrides in Gopkg.toml to lock to the exact versions as upstream - the resolved versions without overrides differed only by patch release from what upstream coredns uses but that was enough to cause compilation errors

This also adds a new patch to our that adds a make test target back to the Makefile. Upstream got rid of the test target. We could choose to drop this patch, but there's not another target that easily exposes running tests that don't depend on a running etcd server.

nitisht and others added 30 commits July 1, 2018 07:32
Signed-off-by: Miek Gieben <miek@miek.nl>
Cache IP's and ports as well.

Signed-off-by: Miek Gieben <miek@miek.nl>
This revert 17d807f and re-adds the metadata plugin as a plugin that
just sets a label to a value function.

Add package documentation on how to use the metadata package. Make it
clear that any caching is up to the Func implemented.

There are now - no in tree users. We could add the request metadata by
default under names that copy request.Request, i.e

request/ip - remote IP
request/port - remote port

Variables.go has been deleted.

Signed-off-by: Miek Gieben <miek@miek.nl>
k8s' client-go has been updated to v8.0.0 (1.11). This fix
updates client-go dependency so that it is in sync.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
This was done anyway, but only deep in the functions, just do this
everywhere; allows for shorter code and request.Request allows for
caching as well.

Cleanups, make it more Go like.
* remove unneeded switches
* remove testdir (why was this there??)
* simplify the logic
* remove unneeded variables
* put short functions on a single line
* fix documentation.
* spin off wire funcs in wire.go, make them functions.

Signed-off-by: Miek Gieben <miek@miek.nl>
* request: add LocalIP

Fix TODO that was added: Add LocalIP and test the Clear() method.

Signed-off-by: Miek Gieben <miek@miek.nl>

* Move to Errorf

PR feedback and move to Errorf instead Fatalf.

Signed-off-by: Miek Gieben <miek@miek.nl>
…tion (coredns#1901) (coredns#1910)

Exporting Zone.File to avoid getters and setters

Updating getter and setter for Zone.File to be less racy

Renaming GetFile to File in zone plugin
* plugin/cache: add extra test for FORMERR

Add extra test that test for not caching a formerr.

Signed-off-by: Miek Gieben <miek@miek.nl>

* govet

Signed-off-by: Miek Gieben <miek@miek.nl>
Allow plugins to dump messages in text pcap to the log. The forward
plugin does this when a reply does not much the query.

If the debug plugin isn't loaded Hexdump and Hexdumpf are noop.

Signed-off-by: Miek Gieben <miek@miek.nl>
Automatically submitted.
* plugin/metadata: adjust doc to latest code.

Signed-off-by: Miek Gieben <miek@miek.nl>
* Doc updates

Make the name section fit on one line.

Signed-off-by: Miek Gieben <miek@miek.nl>

* Regen docs

Signed-off-by: Miek Gieben <miek@miek.nl>
* plugin/forward: add prefer_udp option

* updated according to code review

 - fixed linter warning
 - removed metric parameter in Proxy.Connect()
* DoH: put in pkg/doh

Factor out the DoH stuff into its own package, add function to request
a DoH response. This can be used by forward (and maybe proxy) to
implement DoH client support.

Signed-off-by: Miek Gieben <miek@miek.nl>

* lint

Signed-off-by: Miek Gieben <miek@miek.nl>

* ... and make it compile

Signed-off-by: Miek Gieben <miek@miek.nl>
Fix documentation and touch up plugin/forward/README.md

Signed-off-by: Miek Gieben <miek@miek.nl>
Fix documentation and remove the unused From method.

Signed-off-by: Miek Gieben <miek@miek.nl>
…oredns#1928)

* - add support of metadata values for edns0 local variables

* - comments from review.

* - simplify label check. Add UT

* - enhance check for Labels, add UT
- remove IsMetadataSet

* - edns0 variable - if variable is not found just ignore the rewrite.
* plugin/metadata: finish documentation

Finish the README.md, add corner case in the IsLabel test and reword
some code comments slightly.

Generate the man-pages and add man/coredns-metadata.7 as well.

Signed-off-by: Miek Gieben <miek@miek.nl>

* Fix test

Signed-off-by: Miek Gieben <miek@miek.nl>
Prevent future; "remove trailing whitespace" PR, but adding a simple
presubmit that checks for this.

This presubmit flagged quite some offenders, remove all trailing
whitespace from. Apart from that there aren't any other changes.

Signed-off-by: Miek Gieben <miek@miek.nl>
* plugin/forward: add HealthChecker interface

Make the HealthChecker interface and morph the current DNS health
checker into that interface.

Remove all whole bunch of method on Forward that didn't make sense.

This is done in preparation of adding a DoH client to forward - which
requires a completely different healthcheck implementation (and more,
but lets start here)

Signed-off-by: Miek Gieben <miek@miek.nl>

* Use protocol

Signed-off-by: Miek Gieben <miek@miek.nl>

* Dial doesnt need to be method an Forward either

Signed-off-by: Miek Gieben <miek@miek.nl>

* Address comments

Address various comments on the PR.

Signed-off-by: Miek Gieben <miek@miek.nl>
* release: automate the release

This PR aims to various pieces into place so we can automate the coredns
release. It needs the script from coredns/release to be installed. Dreck
is to be setup as described in coredns/release/README.md

The release-coredns script can be tested and allows for other branches
than master to be test.

This PR also features some cleanup in the Makefile.release so we don't
call the godep target for each linux release - this speeds it up for
some bit.

Manually running ./release-coredns -t auto-release builds the artifects
for this release, but (of course) doesn't upload anything yet.

Add /release to the OWNERS and allow command to be executed (this still
needs to be tested).

Signed-off-by: Miek Gieben <miek@miek.nl>

* that makefile target doesnt exist anymore

Signed-off-by: Miek Gieben <miek@miek.nl>

* test release for now

Signed-off-by: Miek Gieben <miek@miek.nl>

* Slightly better output

Signed-off-by: Miek Gieben <miek@miek.nl>

* remove again

Signed-off-by: Miek Gieben <miek@miek.nl>
Automatically submitted.
Releasing 1.2.0 uncovered some rough edges that need to be documented.
Also fix github-push target and docker login.

Signed-off-by: Miek Gieben <miek@miek.nl>
Automatically submitted.
Log and returns an error when the name rewrite creates a name that is
illegal. Add test in name_test.go to see if an error is returned.

Possible followup could be the only check this if a name-rewrite is
done.

Fixes: coredns#1638

Signed-off-by: Miek Gieben <miek@miek.nl>
chrisohaver and others added 13 commits January 10, 2019 07:34
Signed-off-by: Miek Gieben <miek@miek.nl>
* Remove version pinning of thrift, ugoriji/go, and etcd

For incompatibility reasons at one point, we were forced
to pining the version of thrift, ugoriji/go, and etcd
to very specific versions (some are not versioned commits)
to get around the build issues.

It looks like those incompatibility issues are gone so
we could remove the pinning of thrift, ugoriji/go, and etcd.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

* Update Gopkg.lock

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

* Update vendor library

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
Tag the 1.3.1 version.

Signed-off-by: Miek Gieben <miek@miek.nl>
Previous coredns versions had a `make test` target and our ci-operator
currently uses this target to kick off the tests. So, at least to get
tests running against this PR without requiring changes to the
openshift/release ci-operator setup, I'm adding that target back.

This just runs the full litany of tests that Travis would run except
for the coverage targets and the tests that require a running etcd
server.
At least in the case of mholt/caddy, compilation fails if not pinned
to the exact same version that upstream uses (ie v0.11.3 fails,
v0.11.1 does not).
This pins the Kubernetes client dependencies to the same version used
in previous releases of our fork. And updates the mismatched comment
vs reality in Gopkg.toml that we had previously to avoid future
confusion.
@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Feb 6, 2019
@bbrowning
Copy link
Author

I did not squash any of my commits that dealt with restoring our Gopkg.* and vendor/ changes in case the logic ordering of how I did things was useful. If it's not, I can squash those as desired.

The coredns tests pass for me locally. I swapped out the openshift-dns DaemonSet in my OpenShift 4 cluster to use the updated CoreDNS image:

$ oc rsh -n openshift-dns -c dns dns-default-pvf79 coredns -version
CoreDNS-1.3.1
linux/amd64, go1.10.3,

So far everything seems to be working as expected with some basic manual testing of DNS in the cluster.

@openshift-merge-robot
Copy link

/retest

1 similar comment
@openshift-merge-robot
Copy link

/retest

@ironcladlou
Copy link

Awesome work, thanks. One last thought: what about introducing a bespoke Makefile containing the test target we want rather than patching the upstream file? The CI job could be configured to include an execution of make against the file.

Worth it?

/cc @openshift/sig-network-edge

@bbrowning
Copy link
Author

A new Makefile to avoid future merge conflicts may not be a bad idea. But merges would still need to pull in any relevant changes as far as test suites, where tests live, how they're run, etc into that new Makefile. Would it be better to get alerted to that fact via a merge conflict or to manually keep an eye on it?

And, a new Makefile will require adjustment to ci-operator in the openshift/release repo for this component, right? That's the main reason I didn't do anything in this PR as that would have prevented Prow from being able to run the tests here.

@ironcladlou
Copy link

@bbrowning

A new Makefile to avoid future merge conflicts may not be a bad idea. But merges would still need to pull in any relevant changes as far as test suites, where tests live, how they're run, etc into that new Makefile. Would it be better to get alerted to that fact via a merge conflict or to manually keep an eye on it?
And, a new Makefile will require adjustment to ci-operator in the openshift/release repo for this component, right? That's the main reason I didn't do anything in this PR as that would have prevented Prow from being able to run the tests here.

Good points. Let's keep it simple with what you already have.

/lgtm

@openshift-ci-robot
Copy link

@ironcladlou: changing LGTM is restricted to assignees, and only openshift/coredns repo collaborators may be assigned issues.

In response to this:

@bbrowning

A new Makefile to avoid future merge conflicts may not be a bad idea. But merges would still need to pull in any relevant changes as far as test suites, where tests live, how they're run, etc into that new Makefile. Would it be better to get alerted to that fact via a merge conflict or to manually keep an eye on it?
And, a new Makefile will require adjustment to ci-operator in the openshift/release repo for this component, right? That's the main reason I didn't do anything in this PR as that would have prevented Prow from being able to run the tests here.

Good points. Let's keep it simple with what you already have.

/lgtm

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.

@ironcladlou
Copy link

@smarterclayton oh yeah, we don't patch OWNERS 😁

How do you want to manage merging?

@smarterclayton
Copy link

smarterclayton commented Feb 8, 2019 via email

@smarterclayton
Copy link

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 8, 2019
@bbrowning
Copy link
Author

Several e2e tests failed on that run, but from initial digging it looks like a couple of known flaking tests and some known manifest digest issues that sometimes crop up. I'll kick off the e2e tests again and see if the results repeat.

/test e2e-aws

@smarterclayton
Copy link

/retest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet