-
Notifications
You must be signed in to change notification settings - Fork 880
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
add configuration for active/standy services for server #546
add configuration for active/standy services for server #546
Conversation
e406943
to
1a36d8f
Compare
1a36d8f
to
c7d9077
Compare
This would solve #588 |
I like this change and I think it should be merged, but someone more qualified than me needs to review it. |
Related #674 |
This would be really helpful - we are facing this same issue meaning multiple load balancers are provisioned without any mechanism to control which services they are linked with. |
Any update on this? I would also find this change helpful to be able to set annotations for a particular service. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like where this is going, especially with the annotations, but we'll want to make sure we're backwards compatible (unless there is a dire reason we can't).
I can see where the annotations are useful, but Is the clusterIP / nodePort override behavior also useful for people?
@@ -13,20 +13,26 @@ metadata: | |||
app.kubernetes.io/instance: {{ .Release.Name }} | |||
app.kubernetes.io/managed-by: {{ .Release.Service }} | |||
annotations: | |||
{{ template "vault.service.annotations" .}} | |||
{{ template "vault.service.active.annotations" .}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'll need to include the previous annotations as well in both the active and standby. (Both because of backwards compatibility and because we might want to have common annotations in the vault.service.annotations
template.)
"active": { | ||
"type": "object", | ||
"properties": { | ||
"annotations": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These appear to be missing entries for type
, nodePort
, clusterIP`.
@@ -459,6 +459,52 @@ server: | |||
# to the service. | |||
annotations: {} | |||
|
|||
# Enables a headless service that contains the active pod only |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These sections don't enable this behavior, so I would remove this comment.
@@ -459,6 +459,52 @@ server: | |||
# to the service. | |||
annotations: {} | |||
|
|||
# Enables a headless service that contains the active pod only | |||
active: | |||
# clusterIP controls whether a Cluster IP address is attached to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a comment about these values defaulting to the values.server.service
values if missing, except annotations
(which will be appended)?
Revived this PR in #896 |
Superseded by #896 |
Currently there is no ability to configure services for active and standby services, as they inherit their settings from the main server service configuration. This is a problem, for example, when you need to attach different annotations to the different services. Instead of just allowing for setting of annotations, I opted to allow configuring the services for more flexibility.