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.

Sign up for GitHub

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

Jump to bottom

Fix race on closing event channel. #1798

Merged
merged 3 commits into from Jun 29, 2017
Merged

Fix race on closing event channel. #1798

merged 3 commits into from Jun 29, 2017

Conversation

timoreimann
Copy link
Contributor

  • Wait for secret controller to finish synchronizing.
  • Improve basic auth handling.
  • Continue Ingress processing on auth retrieval failure.

The last point is noteworthy: We previously returned an error when we couldn't retrieve basic auth credentials, blocking following Ingresses from being processed. Since retrieving the auth credentials is highly dependent on an external component (the Kubernetes Secret object), we should opt for dropping such failing Ingresses but move forward with the remaining ones.

Fixes #1794.

@dtomcej @errm @emilevauge @ldez and others: PTAL.

@timoreimann timoreimann requested review from errm and dtomcej June 27, 2017 23:54
@ldez ldez added this to the 1.3 milestone Jun 28, 2017
@ldez
Copy link
Member

ldez commented Jun 28, 2017 

+ go test -cover -coverprofile=cover.out ./provider/kubernetes
# github.com/containous/traefik/provider/kubernetes
/tmp/go-build583504459/github.com/containous/traefik/provider/kubernetes/_test/_obj_test/kubernetes.go:380: undefined: annotationKubernetesAuthType
/tmp/go-build583504459/github.com/containous/traefik/provider/kubernetes/_test/_obj_test/kubernetes.go:386: undefined: annotationKubernetesAuthSecret
FAIL	github.com/containous/traefik/provider/kubernetes [build failed]
make: *** [test-unit] Error 2

@timoreimann timoreimann force-pushed the k8s-fix-events-channel-closing-race branch from b2304e0 to af53fa7 Compare June 28, 2017 01:14
@timoreimann
Copy link
Contributor Author

timoreimann commented Jun 28, 2017 •
edited

Thanks @ldez, should be fixed now.

Tests are still failing though. Marked WIP.

@timoreimann timoreimann removed the WIP label Jun 28, 2017
@timoreimann
Copy link
Contributor Author

Seems like we're good.

Ready for review!

ldez
ldez approved these changes Jun 28, 2017
View reviewed changes
Copy link
Member

@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

nmengin
nmengin approved these changes Jun 29, 2017
View reviewed changes
Copy link
Contributor

@nmengin nmengin left a comment •
edited

Choose a reason for hiding this comment

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

LGTM

juliens
juliens approved these changes Jun 29, 2017
View reviewed changes
Copy link
Member

@juliens juliens left a comment

Choose a reason for hiding this comment

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

LGTM

@ldez ldez changed the title [k8s] Fix race on closing event channel. Fix race on closing event channel. Jun 29, 2017
@timoreimann @ldez
Wait for secret controller to finish synchronizing.
d9c88cf 
Prevents a race on closing the events channel, possibly leading to a
double-close.
@timoreimann @ldez
Improve basic auth handling.
d278701 
- Enrich logging.
- Move error closer to producer.
@timoreimann @ldez
Continue Ingress processing on auth retrieval failure.
036babf
@ldez ldez force-pushed the k8s-fix-events-channel-closing-race branch from af53fa7 to 036babf Compare June 29, 2017 08:26
emilevauge
emilevauge approved these changes Jun 29, 2017
View reviewed changes
Copy link
Member

@emilevauge emilevauge left a comment

Choose a reason for hiding this comment

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

Thanks @timoreimann !
LGTM

@ldez ldez merged commit 6473002 into traefik:v1.3 Jun 29, 2017
@nmengin nmengin mentioned this pull request Jun 29, 2017
Closed
@jeroenjacobs79 jeroenjacobs79 mentioned this pull request Jul 4, 2017
Closed
@timoreimann timoreimann deleted the k8s-fix-events-channel-closing-race branch July 5, 2017 15:47
@timoreimann timoreimann mentioned this pull request Jul 5, 2017
Merged
@timoreimann timoreimann mentioned this pull request Jul 24, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juliens juliens juliens approved these changes

@ldez ldez ldez approved these changes

@emilevauge emilevauge emilevauge approved these changes

@nmengin nmengin nmengin approved these changes

@errm errm Awaiting requested review from errm

@dtomcej dtomcej Awaiting requested review from dtomcej

Assignees
No one assigned
Labels
area/provider/k8s/ingress kind/bug/fix a bug fix
Projects
None yet
Milestone
1.3
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@timoreimann @ldez @juliens @emilevauge @nmengin