docker-archive / deploykit

A toolkit for creating and managing declarative, self-healing infrastructure.
Apache License 2.0
2.25k stars 262 forks source link

Check for changes to SSL cert and health monitor path. #824

Closed craigyam closed 6 years ago

craigyam commented 6 years ago

A docker service update (or a docker service rm followed by a docker service create) which changes the certificate or health check path values may result in the load balancer configuration for the service having the configuration of the older service.

The ingress controller does not check if the certificate or health check path has changed between the Service Load Balancer configuration and the Docker Swarm Listener configuration. This results in the old configuration not getting updated to the new values.

craigyam commented 6 years ago

One thought I'd like to add... Would there be any concerns with moving the removes before the creates? I'm thinking of the case where the front end port was changed - the create would fail with dupe port (for IBM Cloud anyway) so the changed route wouldn't be added. The remove would then remove that route and then another sync cycle would be needed before the route would be added. Not sure how other providers handle that so wasn't sure if that would be a valid change.

codecov[bot] commented 6 years ago

Codecov Report

Merging #824 into master will decrease coverage by 0.02%. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #824      +/-   ##
==========================================
- Coverage   47.16%   47.14%   -0.03%     
==========================================
  Files          89       89              
  Lines        7944     7944              
==========================================
- Hits         3747     3745       -2     
- Misses       3836     3837       +1     
- Partials      361      362       +1
Impacted Files Coverage Δ
pkg/types/spec.go 58.4% <0%> (-1.77%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update cb2e08f...7e4bdc1. Read the comment docs.

chungers commented 6 years ago

One thought I'd like to add... Would there be any concerns with moving the removes before the creates? I'm thinking of the case where the front end port was changed - the create would fail with dupe port (for IBM Cloud anyway) so the changed route wouldn't be added. The remove would then remove that route and then another sync cycle would be needed before the route would be added. Not sure how other providers handle that so wasn't sure if that would be a valid change.

This is a valid concern and the behavior you described would be typical. Do you want to incorporate that in this PR or make a separate one?

craigyam commented 6 years ago

Thanks David. I will look at creating a new PR with the change to move the removesahead of the creates.