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

Fail if partitions enabled with federation #892

Merged
merged 2 commits into from
Dec 6, 2021
Merged

Fail if partitions enabled with federation #892

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0e4d3a0
Fail if both adminPartitions and wan federation are enabled.
Dec 2, 2021
8b35b0b
Update CHANGELOG and fix indentation.
Dec 2, 2021
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
13 changes: 7 additions & 6 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,21 @@
## UNRELEASED

BREAKING CHANGES:
* Update minimum go version for project to 1.17 [[GH-878](https://github.com/hashicorp/consul-k8s/pull/878)]
* Control Plane
* Update minimum go version for project to 1.17 [[GH-878](https://github.com/hashicorp/consul-k8s/pull/878)]

IMPROVEMENTS:
* CLI
* Pre-check in the `install` command to verify the correct license secret exists when using an enterprise Consul image. [[GH-875](https://github.com/hashicorp/consul-k8s/pull/875)]
* Pre-check in the `install` command to verify the correct license secret exists when using an enterprise Consul image. [[GH-875](https://github.com/hashicorp/consul-k8s/pull/875)]
* Control Plane
* Add a label "managed-by" to every secret the control-plane creates. Only delete said secrets on an uninstall. [[GH-835](https://github.com/hashicorp/consul-k8s/pull/835)]
* Add a label "managed-by" to every secret the control-plane creates. Only delete said secrets on an uninstall. [[GH-835](https://github.com/hashicorp/consul-k8s/pull/835)]
* Add support for labeling a Kubernetes service with `consul.hashicorp.com/service-ignore` to prevent services from being registered in Consul. [[GH-858](https://github.com/hashicorp/consul-k8s/pull/858)]
* Helm Chart
* Fail an installation/upgrade if WAN federation and Admin Partitions are both enabled. [[GH-892](https://github.com/hashicorp/consul-k8s/issues/892)]

BUG FIXES:
* Control Plane:
* Add a workaround to check that the ACL token is replicated to other Consul servers. [[GH-862](https://github.com/hashicorp/consul-k8s/issues/862)]

BUG FIXES:
* Add a workaround to check that the ACL token is replicated to other Consul servers. [[GH-862](https://github.com/hashicorp/consul-k8s/issues/862)]
* Helm Chart
* Admin Partitions **(Consul Enterprise only)**: Do not mount Consul CA certs to partition-init job if `externalServers.useSystemRoots` is `true`. [[GH-885](https://github.com/hashicorp/consul-k8s/pull/885)]

Expand Down
1 change: 1 addition & 0 deletions charts/consul/templates/client-daemonset.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
{{- if (and (and .Values.global.tls.enabled .Values.global.tls.httpsOnly) (and .Values.global.metrics.enabled .Values.global.metrics.enableAgentMetrics))}}{{ fail "global.metrics.enableAgentMetrics cannot be enabled if TLS (HTTPS only) is enabled" }}{{ end -}}
{{- $serverEnabled := (or (and (ne (.Values.server.enabled | toString) "-") .Values.server.enabled) (and (eq (.Values.server.enabled | toString) "-") .Values.global.enabled)) -}}
{{- if (and .Values.global.adminPartitions.enabled $serverEnabled (ne .Values.global.adminPartitions.name "default"))}}{{ fail "global.adminPartitions.name has to be \"default\" in the server cluster" }}{{ end -}}
{{- if and .Values.global.federation.enabled .Values.global.adminPartitions.enabled }}{{ fail "If global.federation.enabled is true, global.adminPartitions.enabled must be false because they are mutually exclusive" }}{{ end }}
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need this check on the clients? I think federation is only possible if servers are also enabled

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is true. I put a check on clients in case someone was using a values file with federation set to enabled on a non-default admin partition.

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 federation is only possible if servers are also enabled

I think actually we require federation: true if doing HCP federation. I can double check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we have a situation where we might have federation enabled and partitions enabled? Consul Core intends on disabling this. https://github.com/hashicorp/consul-enterprise/pull/1391
Am I confusing specifics?

Copy link
Member

Choose a reason for hiding this comment

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

I'm saying that I think we need to keep this check here rather than on the server statefulset since potentially someone has:

server:
  enabled: false
global:
  federation:
    enabled: true

Although that being said I haven't had time to find out if we actually do use this for HCP fed.

# DaemonSet to run the Consul clients on every node.
apiVersion: apps/v1
kind: DaemonSet
Expand Down
1 change: 1 addition & 0 deletions charts/consul/templates/server-statefulset.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
{{- if (or (and (ne (.Values.server.enabled | toString) "-") .Values.server.enabled) (and (eq (.Values.server.enabled | toString) "-") .Values.global.enabled)) }}
{{- if and .Values.global.federation.enabled .Values.global.adminPartitions.enabled }}{{ fail "If global.federation.enabled is true, global.adminPartitions.enabled must be false because they are mutually exclusive" }}{{ end }}
{{- if and .Values.global.federation.enabled (not .Values.global.tls.enabled) }}{{ fail "If global.federation.enabled is true, global.tls.enabled must be true because federation is only supported with TLS enabled" }}{{ end }}
{{- if and .Values.global.federation.enabled (not .Values.meshGateway.enabled) }}{{ fail "If global.federation.enabled is true, meshGateway.enabled must be true because mesh gateways are required for federation" }}{{ end }}
{{- if and .Values.server.serverCert.secretName (not .Values.global.tls.caCert.secretName) }}{{ fail "If server.serverCert.secretName is provided, global.tls.caCert must also be provided" }}{{ end }}
Expand Down
12 changes: 12 additions & 0 deletions charts/consul/test/unit/client-daemonset.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1465,6 +1465,18 @@ rollingUpdate:
[[ "$output" =~ "global.adminPartitions.name has to be \"default\" in the server cluster" ]]
}

@test "client/DaemonSet: federation and admin partitions cannot be enabled together" {
cd `chart_dir`
run helm template \
-s templates/client-daemonset.yaml \
--set 'global.adminPartitions.enabled=true' \
--set 'global.federation.enabled=true' \
.

[ "$status" -eq 1 ]
[[ "$output" =~ "If global.federation.enabled is true, global.adminPartitions.enabled must be false because they are mutually exclusive" ]]
}

#--------------------------------------------------------------------
# extraContainers

Expand Down
15 changes: 15 additions & 0 deletions charts/consul/test/unit/server-statefulset.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,21 @@ load _helpers
[ "${actual}" = "true" ]
}

#--------------------------------------------------------------------
# admin-partitions

@test "server/StatefulSet: federation and admin partitions cannot be enabled together" {
cd `chart_dir`
run helm template \
-s templates/server-statefulset.yaml \
--set 'global.adminPartitions.enabled=true' \
--set 'global.federation.enabled=true' \
.

[ "$status" -eq 1 ]
[[ "$output" =~ "If global.federation.enabled is true, global.adminPartitions.enabled must be false because they are mutually exclusive" ]]
}

#--------------------------------------------------------------------
# image

Expand Down