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

make canary ingresses independent of the order they were applied #3525

Merged
merged 1 commit into from
Dec 10, 2018
Merged

make canary ingresses independent of the order they were applied #3525

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 20 additions & 3 deletions internal/ingress/controller/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,6 +389,8 @@ func (n *NGINXController) getBackendServers(ingresses []*ingress.Ingress) ([]*in
upstreams := n.createUpstreams(ingresses, du)
servers := n.createServers(ingresses, upstreams, du)

var canaryIngresses []*ingress.Ingress

for _, ing := range ingresses {
ingKey := k8s.MetaNamespaceKey(ing)
anns := ing.ParsedAnnotations
Expand Down Expand Up @@ -561,9 +563,15 @@ func (n *NGINXController) getBackendServers(ingresses []*ingress.Ingress) ([]*in
}
}

// set aside canary ingresses to merge later
if anns.Canary.Enabled {
klog.Infof("Canary ingress %v detected. Finding eligible backends to merge into.", ing.Name)
mergeAlternativeBackends(ing, upstreams, servers)
canaryIngresses = append(canaryIngresses, ing)
}
}

if nonCanaryIngressExists(ingresses, canaryIngresses) {
for _, canaryIng := range canaryIngresses {
mergeAlternativeBackends(canaryIng, upstreams, servers)
Copy link
Member

@ElvinEfendi ElvinEfendi Dec 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Since this is done before we set https://kubernetes.github.io/ingress-nginx/user-guide/nginx-configuration/annotations/#default-backend, canary feature won't work when the main backend has no endpoints and Nginx is proxying to custom default backend. IMO this is an expected behaviour. Not the concern of this PR but it would be nice to have an e2e test case where main ingress's backend has no endpoints (replicas: 0) and assert that all requests return 503 regardless of canary cookie/header being set to always.

Copy link
Author

Choose a reason for hiding this comment

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

That's a good point. I'll add an e2e for this

}
}

Expand Down Expand Up @@ -1127,8 +1135,17 @@ func (n *NGINXController) createServers(data []*ingress.Ingress,
return servers
}

// OK to merge canary ingresses iff there exists one or more ingresses to potentially merge into
func nonCanaryIngressExists(ingresses []*ingress.Ingress, canaryIngresses []*ingress.Ingress) bool {
return len(ingresses)-len(canaryIngresses) > 0
Copy link
Member

Choose a reason for hiding this comment

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

IMO this is a bit confusing because main safety is being done in mergeAlternativeBackends function. I'd suggest to call this something like nonCanaryIngressExists

}

// ensure that the following conditions are met
// 1) names of backends do not match and canary doesn't merge into itself
// 2) primary name is not the default upstream
// 3) the primary has a server
func canMergeBackend(primary *ingress.Backend, alternative *ingress.Backend) bool {
return primary.Name != alternative.Name && !primary.NoServer
return primary.Name != alternative.Name && primary.Name != defUpstreamName && !primary.NoServer
}

// Performs the merge action and checks to ensure that one two alternative backends do not merge into each other
Expand Down
Loading