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

Fix bug where webhookconfig would not be updated #837

Merged
merged 1 commit into from
Nov 4, 2021

Conversation

lkysow
Copy link
Member

@lkysow lkysow commented Nov 3, 2021

Fixes an issue where if the webhookconfiguration is updated after the
webhook cert manager starts, then it is never reset to the correct
configuration until the manager is restarted or the certificate expires.

The issue was occurring because in the loop where we check the
webhookconfiguration we are only checking the webhookconfiguration of
the last updated certificate bundle. If there are multiple webhook
configurations then we are only watching a single one at a time since
the loop always operates on one bundle.

The fix is for each webhook to run its own loop.

In addition, the reason this was causing issues is because during a helm
upgrade we were resetting the webhookconfiguration's caBundle fields.
I've removed the caBundle setting (which already didn't exist in the
connect injector's webhook config) so that during a helm upgrade it
doesn't overwrite that field.

Fixes #808

How I've tested this PR:

  • unit tests (the test I added used to fail)
  • manual test: install Consul and then edit both the connect inject webhook config and the controller webhook config and set caBundle to "". Wait a couple seconds. Now get the configs. Only one will have been reset properly. Now do the same with this image (ghcr.io/lkysow/consul-k8s-dev:nov03-3)

How I expect reviewers to test this PR:

  • code/manual testing if interested

Checklist:

  • Tests added
  • CHANGELOG entry added

    HashiCorp engineers only, community PRs should not add a changelog entry.
    Entries should use present tense (e.g. Add support for...)

certNotify := &cert.Notify{Source: certSource, Ch: certCh, WebhookConfigName: config.Name, SecretName: config.SecretName, SecretNamespace: config.SecretNamespace}
notifiers = append(notifiers, certNotify)
go certNotify.Start(ctx)
go c.certWatcher(ctx, certCh, c.clientset, c.logger)
}

Copy link
Member Author

Choose a reason for hiding this comment

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

The issue is on line 203. bundle is always set to the last bundle that came out of ch. Then we wake up every defaultRetryDuration to check things but we're only checking one bundle.

@lkysow lkysow force-pushed the lkysow/cert-manager-fixes branch from bf96c50 to 577e6a3 Compare November 3, 2021 23:21
@lkysow lkysow marked this pull request as ready for review November 3, 2021 23:21
@lkysow lkysow requested review from a team, kschoche and thisisnotashwin and removed request for a team November 3, 2021 23:21
Copy link
Contributor

@thisisnotashwin thisisnotashwin left a comment

Choose a reason for hiding this comment

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

This is excellent! Thanks for the fix @lkysow 🚀

CHANGELOG.md Outdated Show resolved Hide resolved
Fixes an issue where if the webhookconfiguration is updated after the
webhook cert manager starts, then it is never reset to the correct
configuration until the manager is restarted or the certificate expires.

The issue was occurring because in the loop where we check the
webhookconfiguration we are only checking the webhookconfiguration of
the last updated certificate bundle. If there are multiple webhook
configurations then we are only watching a single one at a time since
the loop always operates on one bundle.

The fix is for each webhook to run its own loop.

In addition, the reason this was causing issues is because during a helm
upgrade we were resetting the webhookconfiguration's caBundle fields.
I've removed the caBundle setting (which already didn't exist in the
connect injector's webhook config) so that during a helm upgrade it
doesn't overwrite that field.
@lkysow lkysow force-pushed the lkysow/cert-manager-fixes branch from 577e6a3 to 1de9bca Compare November 4, 2021 17:10
Copy link
Contributor

@kschoche kschoche left a comment

Choose a reason for hiding this comment

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

Great work!!

@lkysow lkysow merged commit a08ddb0 into main Nov 4, 2021
@lkysow lkysow deleted the lkysow/cert-manager-fixes branch November 4, 2021 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants