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

Properly detect CoreDNS version with suffixes and bump support to CoreDNS < 1.9.0 #790

Merged
merged 6 commits into from
Aug 8, 2021
Merged

Conversation

0rax
Copy link
Contributor

@0rax 0rax commented May 28, 2021

What does this PR do?

This PR:

The issue was exposed in #788 and has some checks showed that github.com/hashicorp/go-version considers suffixed version as pre-release meaning that some checks here will not be satisfied.

Example:

versionCoreDNS17 := goversion.Must(goversion.NewVersion("1.7"))
coreDNSVersion := goversion.Must(goversion.NewVersion("v1.7.0-eksbuild.1"))
fmt.Println(coreDNSVersion.LessThan(versionCoreDNS17)) // Prints true

This is an issue in two contexts:

  • While checking if the upstream keyword needs to be used when patching the CoreDNS configmap as v1.7.0-eksbuild.1 would be considered a version inferior to 1.7
  • While checking for if the current used version is in the range of supported version:
    • While checking for the minimum version support, current checks wouldn't allow a suffixed version to be used (i.e: v1.3.0-eksbuild.0 is not consider GreaterThanOrEqual to 1.3)
    • While checking for the maximum version support, a suffixed build of the next major version would be accepted (i.e: v1.8.0-eksbuild.1 is considered LessThan 1.8)

I've fixed this issue by updating go-version to 1.3.0 which adds a new method Core which allows us to retrieve the non suffixed / pre-release version linked to the currently parsed version. One thing to keep in mind here is that the new checks would allow an alpha / beta version of CoreDNS 1.3 to be used with Mesh though there is no real solution here as go-version is unable to detect the difference between an alpha, beta, rc suffix from an eksbuild.X suffix.

  • Extends support to CoreDNS 1.8

After checking all changelogs since the last support version (CoreDNS-1.7.1), it doesn't look like any breaking changes were introduced preventing mesh from supporting it. This is currently being tested in a EKS cluster running Kubernetes v1.20 (Platform version eks.1) and CoreDNS v1.8.3-eksbuild.1.

This would extend support of Traefik Mesh to EKS running Kubernetes v1.20, EKS is using CoreDNS 1.8 as its default version since v1.19 (which is the current default) and has removed support for choosing CoreDNS 1.7 at deployment in v1.20.

Fixes #787
Fixes #788

How to test it

  • Tests are added to make sure the current changes in version detection are doing what is expected (those tests would previously fail).
  • Support for CoreDNS 1.8 can be tested by deploying a new cluster with CoreDNS 1.8 (i.e EKS v1.19 or up) and deploy Traefik Mesh over it. Not sure exactly if there is a suit of functional tests that could be run to make sure all functionality are there though.

Additional Notes

I've made the choice to upgrade go-version to v1.3.0 but another solution was discussed in #788 (comment).

@0rax 0rax marked this pull request as ready for review May 28, 2021 13:51
@0rax
Copy link
Contributor Author

0rax commented May 28, 2021

Looks like html-proofer has some issue installing. It seems that it now requires a version of ruby-ffi that is not available in Alpine Linux packages and tries to build it. I was able to fix that by adding build-base and ruby-dev to apk add so that it compiles it itself. Not sure about that fix though as it adds quite a bit of dependencies to install in docs/check.Dockerfile.

@0rax
Copy link
Contributor Author

0rax commented Jun 9, 2021

This fix has been deployed in our staging environment for 2 weeks now and has been stable. If someone is in need of it for their environment. A custom image can be built using:

make build
docker tag traefik/mesh:latest myimage:mytag

@kevinpollet
Copy link
Member

/sem-approve

Copy link
Member

@kevinpollet kevinpollet left a comment

Choose a reason for hiding this comment

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

Thanks 👍

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

Successfully merging this pull request may close these issues.

3 participants