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

Adding support for the old leader-elector #607

Merged
merged 3 commits into from
Sep 16, 2021
Merged

Conversation

tvoran
Copy link
Member

@tvoran tvoran commented Sep 3, 2021

Adds an option to deploy the old leader-elector container that was removed in PR #568. (The new vault-k8s uses an internal mechanism for leader determination, so this is just for backwards compatibility, and can be removed in the near future.)

Note that this defaults to not deploying the old leader-elector container and Endpoint, unless injector.leaderElector.useContainer is set to true.

Adds the leader-elector container support that was removed in
PR #568. The new vault-k8s uses an internal mechanism for leader
determination, so this is just for backwards compatibility, and can
be removed in the near future.
@tvoran tvoran requested review from tomhjp and benashz September 3, 2021 21:52
labels:
app.kubernetes.io/name: {{ include "vault.name" . }}-agent-injector
app.kubernetes.io/instance: {{ .Release.Name }}
app.kubernetes.io/managed-by: {{ .Release.Service }}
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if adding an extra annotation denoting the deprecation of this container would be a good idea. Not sure if that would satisfy what @calvn requested w.r.t. facilitating debugging.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, something like deprecated: true?

I also added a change to the logging in hashicorp/vault-k8s#297 to make it easier to tell which leader elector scheme is in use.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, that sounds good.

Default to not deploying the old leader-elector container, unless
injector.leaderElector.useContainer is `true`.
@tvoran tvoran changed the title Adding back support for the old leader-elector Adding support for the old leader-elector Sep 15, 2021
@tvoran tvoran requested a review from benashz September 15, 2021 00:54
Copy link
Contributor

@tomhjp tomhjp left a comment

Choose a reason for hiding this comment

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

Shall we pick a version now that we'll remove it again, so we can give advanced notice in the next changelog?

@tvoran tvoran merged commit 5a864f7 into master Sep 16, 2021
@tvoran tvoran deleted the old-leader-compatibility branch September 16, 2021 01:43
@tvoran
Copy link
Member Author

tvoran commented Sep 16, 2021

@tomhjp That sounds like a good idea; how about v0.18.0? https://github.com/hashicorp/vault-helm/blob/master/CHANGELOG.md

@tvoran tvoran mentioned this pull request Sep 16, 2021
@tomhjp
Copy link
Contributor

tomhjp commented Sep 22, 2021

@tvoran v0.18.0 sounds good to me, gives a nice amount of overlap and warning.

dreamblack86 pushed a commit to dreamblack86/vault-helm that referenced this pull request Oct 27, 2021
The commit:
Added webhook-certs volume mount to sidecar injector (hashicorp#545)
had it fixed correctly in the past. But by the short-term decision to take the leader elector container out then back in has "probably" led to this circumstance during copy&paste that the volumemount has moved down again.

see: Adding support for the old leader-elector (hashicorp#607)
dreamblack86 added a commit to dreamblack86/vault-helm that referenced this pull request Oct 27, 2021
The commit:
d27121 Added webhook-certs volume mount to sidecar injector (hashicorp#545)
had it fixed correctly in the past.

But by the short-term decision to take the leader elector container out then back in has "probably" led to this circumstance during copy&paste that the volumemount has moved down again.

see changes how it came to this small error.
remove leader-elector: d31f94 Support vault-k8s internal leader election (hashicorp#568)
bring leader-elector back: 5a864f Adding support for the old leader-elector (hashicorp#607)
illegalnumbers pushed a commit to streamnative/vault-helm that referenced this pull request Mar 17, 2022
Adds the leader-elector container support that was removed in
PR hashicorp#568. The new vault-k8s uses an internal mechanism for leader
determination, so this is just for backwards compatibility, and can
be removed in the near future.

* mark the endpoint as deprecated

* add a new useContainer option for leaderElector

Default to not deploying the old leader-elector container, unless
injector.leaderElector.useContainer is `true`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants