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

[WIP] Update stacks when needed #123

Closed
wants to merge 1 commit into from
Closed

[WIP] Update stacks when needed #123

wants to merge 1 commit into from

Conversation

mikkeloscar
Copy link
Collaborator

This PR aims to address two issues.

  1. Stacks were scheduled to update by sending them to a channel and having a separate go routine triggering the updates every hour (by default). Instead of waiting one hour to do the update we can just check if an update is needed and schedule it right away. This is important for the SNI support (Add sni support #111) as we want to add new certificates to existing ALBs and not wait an hour for it to propagate.

  2. FindManagedStacks was only returning stacks in a completed state, this resulted in more stacks being created in cases where a stack update was triggered and some stacks were not returned from the FindManagedStacks call because they were in an update state. Instead it just returns all stacks and checks the state when deciding if they should be updated or not. I.e. stacks already being updated will not be updated again until they reach a complete state first.

I marked this WIP because I need to validate that this works correctly, but initial review is welcome :)

@coveralls
Copy link

coveralls commented Feb 7, 2018

Coverage Status

Coverage increased (+0.4%) to 60.274% when pulling 90a336b on fix-update into 289884b on master.

if s == nil {
return false
}
return isComplete(s.status)
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to move the whole isComplete(string) body to this function, because it's only used if you have a Stack and this will reduce number of functions spread.

model[certificateARN+"/"+ingress.Scheme()] = &managedItem{
ingresses: []*kubernetes.Ingress{ingress},
Update: true,
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Update is always true here. This is wrong..

@@ -175,8 +157,12 @@ func buildManagedModel(certsProvider certs.CertificatesProvider, ingresses []*ku
}
if item, ok := model[certificateARN+"/"+ingress.Scheme()]; ok {
item.ingresses = append(item.ingresses, ingress)
item.Update = true
Copy link
Member

Choose a reason for hiding this comment

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

This Update = true is in both cases set. We have to lookup it somewhere to decide if there was a change that we now want to react on. Maybe we have a currentKnownState and check the new data to this one, but this would mean we are becoming stateful and are not stateless anymore. I don't see another way of doing it or trigger updates by time.Duration and additionally in the initial run.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The state is in the cloudformation stack as it has the certificate arn and scheme defined. We just need to check against that an decide if there was a change.

@mikkeloscar
Copy link
Collaborator Author

Closing this in favor of #111 where these changes are now included and make more sense in that context.

@mikkeloscar mikkeloscar deleted the fix-update branch February 18, 2018 19:43
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