-
Notifications
You must be signed in to change notification settings - Fork 321
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 consul-telemetry-collector deployment when enableConsulNamespaces #3192
Conversation
{{- if .Values.global.enableConsulNamespaces }} | ||
- name: CONSUL_NAMESPACE | ||
value: {{ .Values.syncCatalog.consulNamespaces.consulDestinationNamespace }} |
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.
this removes all ref to .Values.syncCatalog.consulNamespaces.consulDestinationNamespace
. AFAICT, .Values.syncCatalog.enabled
might be false
but we'll still insert it here.
My guess is that this is a bug that was from copying this setting from the Sync Catalog Deployment:
-consul-destination-namespace={{ .Values.syncCatalog.consulNamespaces.consulDestinationNamespace }} \ |
CONSUL_LOGIN_NAMESPACE
26240ad
to
e2fc3e9
Compare
e2fc3e9
to
b2f7f00
Compare
CONSUL_LOGIN_NAMESPACE
- -login-namespace={{ .Values.syncCatalog.consulNamespaces.consulDestinationNamespace }} | ||
- -login-namespace={{ .Values.connectInject.consulNamespaces.consulDestinationNamespace }} |
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.
this is one of the fixes: login-name is from connectInject destination namespace not syncCatalog
@jjti When you say you failed to test with non-default partitions is this because non-default partition support is not available in the telemetry collector? Or is it because you would need to help with partitions on K8s? |
At this point it's a me problem. If you have a ref values.yaml for that that'd be appreciated. It think it'll work, but the changes here are copied from adjacent deployments like ingress-gateways |
{{- if .Values.global.enableConsulNamespaces }} | ||
{{- if .Values.syncCatalog.consulNamespaces.mirroringK8S }} | ||
{{- if .Values.connectInject.consulNamespaces.mirroringK8S }} | ||
- -service-namespace={{ .Values.connectInject.consulNamespaces.mirroringK8SPrefix }}{{ .Release.Namespace }} |
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.
change here to use k8s namespace prefixing like we do in the webhook
@jjti Does this help? https://developer.hashicorp.com/consul/docs/enterprise/admin-partitions#install-on-the-non-default-partition-clusters-running-workloads or do you need a writeup specifically written for HCP? |
It does and thanks. I've been trying to hack helm templates together but lack the background knowledge to be efficient. Thanks! I'll draft this until that's done |
Add 1.3.0 notes to changelog
…the latest Consul submodules (#3194) * Add replace directive in Go mod for Control Plane so that we pull in the latest Consul submodules * Add a comment to explain why we need a replace directive
Add MeshGateway to `mesh.consul.hashicorp.com/v2beta1`
add validation to account for type change
* checkpoint * checkpoint, passing test * kitchen sink, NET-4992 * lint issue * clean up unneeded calls --------- Co-authored-by: Sarah Alsmiller <sarah.alsmiller@sarah.alsmiller-RQQ26PQ2L5>
…eway-reso… (#3200) * Adds GatewayClassConfig and MeshGateway resources to the gateway-resources-configmap.yaml in the Helm chart * Updates the configmap to include more fields for the gatewayClassConfig for mesh gateways
* chore: skaffold build experiment * feedback: add experiment comments
* NET-6401 Stub MeshGateway controller * Add MeshGateways resource to connect-inject-clusterrole * Setup v2controller for MeshGateway * Add bats test assertion for connect-inject-clusterrole * Regenerate control-plane/config/rbac/role.yaml
42e19e4
to
83ca25c
Compare
Issues
@david-yu reported multiple issues with the Consul Telemetry Collector deployment. A different PR fixed a couple of consul-dataplane args: #3184
This attempts to fix some of the connect-inject issues with namespaces:
My guess at what happened when this was written was, we were trying to replicate the namespace selection logic of the Consul Dataplane Sidecar webhook:
consul-k8s/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go
Line 237 in b843390
We run into issues tho, doing that. Partly because that's using
syncCatalog
. But also because, if CTC is the first service in a namespace, we fail to inject it because the namespace does not exist.Changes
This PR is an attempt at fixing some of those namespace/partition issues.
-service-partition
arg to the consul-dataplane containerHow I've tested this PR:
Non-default namespaces
connectInject.consulNamespaces.mirroringK8s=true
and deployed the consul-telemetry-collector to anamespace: test-namespace
after creating a deployment from this template change: https://github.com/jjti/consul-experiments/blob/ec80caaeded716a1dfbc34343d0da5db6142ad19/telemetry-collector/resources/consul-telemetry-collector.yaml#L33Non-default partitions
How I expect reviewers to test this PR:
Checklist: