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

Add tests for TLS dynamic configuration in ETCD3 #2606

Merged
merged 2 commits into from
Dec 21, 2017

Conversation

dahefanteng
Copy link
Contributor

@dahefanteng dahefanteng commented Dec 21, 2017

What does this PR do?

When I delete all the tls cert/key pairs in my etcd(v3),the debug log and api(/api/providers) show traefik has read configurations which has an empty tlsconfigure.But Actually it does't work.
After reading the code,I think traefik skip delete when the all the tlsconfigurations in etcd be deleted in a very short time.

And I wrote some test for this.

Motivation

when i write test case in my own project,I delete the tls cert/key pairs,expecting recevie the "TRAEFIK DEFAULT CERT" from traefik,but what i receive is still the deleted one after a very long wait.

More

  • Added/updated tests
  • Added/updated documentation

Additional Notes

Copy link
Contributor

@nmengin nmengin left a comment

Choose a reason for hiding this comment

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

Hello @dahefanteng .

Many thanks for your PR. But the bug seems to be fixed in PR #2603.

However yout Etcd test is very interesting.

WDYT about :

  • Rebasing your PR on the branch v1.5,
  • Deleting the code in server.go and keeping the test.

@dahefanteng
Copy link
Contributor Author

@nmengin I have rebase the PR and delete code in server.go

@nmengin nmengin changed the title Fix "delete all certificates doesn't work" bug Add tests for TLS dynamic configuration in ETCD3 Dec 21, 2017
Copy link
Contributor

@nmengin nmengin left a comment

Choose a reason for hiding this comment

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

Many thanks @dahefanteng .

SGTM, only few suggestions 😉

defer display(c)

// prepare to config
whoami1IP := s.composeProject.Container(c, "whoami1").NetworkSettings.IPAddress
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the constant ipWhoami01 instead of the variable whoami1IP


// prepare to config
whoami1IP := s.composeProject.Container(c, "whoami1").NetworkSettings.IPAddress
whoami2IP := s.composeProject.Container(c, "whoami2").NetworkSettings.IPAddress
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the constant ipWhoami02 instead of the variable whoami2IP

defer cmd.Process.Kill()

// wait for Træfik
err = try.GetRequest("http://127.0.0.1:8081/api/providers", 60*time.Second, try.BodyContains(string("MIIEpQIBAAKCAQEA1RducBK6EiFDv3TYB8ZcrfKWRVaSfHzWicO3J5WdST9oS7h")))
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use constant traefikWebEtcdURL instead of http://127.0.0.1:8081

}

// waiting for Træfik to pull configuration
err = try.GetRequest("http://127.0.0.1:8081/api/providers", 30*time.Second, try.BodyNotContains("MIIEpQIBAAKCAQEA1RducBK6EiFDv3TYB8ZcrfKWRVaSfHzWicO3J5WdST9oS7h"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use constant traefikWebEtcdURL instead of http://127.0.0.1:8081

Copy link
Contributor

@nmengin nmengin left a comment

Choose a reason for hiding this comment

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

Sorry, I wouldn't approve but request changes...

@dahefanteng
Copy link
Contributor Author

@nmengin Really appreciate your patience,Thank you!

Copy link
Contributor

@nmengin nmengin left a comment

Choose a reason for hiding this comment

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

LGTM 👏 👏

Copy link
Member

@mmatur mmatur left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@ldez ldez left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@ldez ldez left a comment

Choose a reason for hiding this comment

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

LGTM

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.

None yet

5 participants