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

Don't write service-defaults during connect-inject unless protocol set explicitly #168

Closed
lkysow opened this issue Nov 29, 2019 · 0 comments · Fixed by #169
Closed

Don't write service-defaults during connect-inject unless protocol set explicitly #168

lkysow opened this issue Nov 29, 2019 · 0 comments · Fixed by #169
Labels
area/connect Related to Connect service mesh, e.g. injection

Comments

@lkysow
Copy link
Member

lkysow commented Nov 29, 2019

Background

The init container we inject for Consul connect writes a service-defaults config for the service if central config is enabled (the default in the helm chart):

{{- if .CentralConfig }}
# Create the central config's service registration
cat <<EOF >/consul/connect-inject/central-config.hcl
kind = "service-defaults"
name = "{{ .ServiceName }}"
protocol = "{{ .ServiceProtocol }}"
EOF
{{- end }}

The protocol can be set passing the -default-protocol flag which is controlled by the Helm chart's connectInject.centralConfig.defaultProtocol value. This protocol can be overridden by an annotation on the pod (consul.hashicorp.com/connect-service-protocol). If there is no annotation or Helm value, it will be set to an empty string which Consul will treat as TCP.

Problem

By default we write TCP - When the Pod starts, the service-defaults config will be written. By default, this will set the protocol to TCP. If the protocol is TCP, none of the L7 routing rules will work. A user isn't expecting this config to be written because they haven't configured it anywhere. If they had written a global proxy-defaults config and set the protocol to http so their L7 rules work, the service-defaults config will override the protocol to tcp. This is unexpected. If a user hasn't explicitly configured a protocol, we should not write a service-defaults config.

Proposal

Only write service-defaults if the protocol is explicitly configured, i.e. in connectInject.centralConfig.defaultProtocol or the annotation.

@lkysow lkysow closed this as completed Nov 29, 2019
@lkysow lkysow reopened this Nov 30, 2019
@lkysow lkysow changed the title Don't write service-defaults during connect-inject unless pod is annotated Don't write service-defaults during connect-inject unless protocol set explicitly Nov 30, 2019
lkysow added a commit that referenced this issue Nov 30, 2019
Previously, we would write a service-defaults config regardless of the
value of the -default-protocol flag or the Pod's protocol annotation. If
neither of these were set, we would write a config with protocol set to
the empty string. This is the same as setting it to tcp. So for every
service, we were by default setting its protocol to tcp.

If a user explicitly created a global proxy-defaults config and set the
protocol to, say, http. Then our service-defaults config would override
that protocol. This behaviour was unexpected because users had never
explicitly told us to write a service-defaults config and it wasn't
helping them.

This change will cause us to only write a service-defaults config if the
protocol is explicitly set–either via the -default-protocol flag or via
the Pod annotation.

I've also renamed the CentralConfig field to WriteServiceDefaults
because there are many types of central config now so it's best to be
explicit.

Fixes #168
lkysow added a commit that referenced this issue Nov 30, 2019
Previously, we would write a service-defaults config regardless of the
value of the -default-protocol flag or the Pod's protocol annotation. If
neither of these were set, we would write a config with protocol set to
the empty string. This is the same as setting it to tcp. So for every
service, we were by default setting its protocol to tcp.

If a user explicitly created a global proxy-defaults config and set the
protocol to, say, http. Then our service-defaults config would override
that protocol. This behaviour was unexpected because users had never
explicitly told us to write a service-defaults config and it wasn't
helping them.

This change will cause us to only write a service-defaults config if the
protocol is explicitly set–either via the -default-protocol flag or via
the Pod annotation.

I've also renamed the CentralConfig field to WriteServiceDefaults
because there are many types of central config now so it's best to be
explicit.

Fixes #168
@lkysow lkysow added the area/connect Related to Connect service mesh, e.g. injection label Nov 30, 2019
lkysow added a commit that referenced this issue Dec 4, 2019
Previously, we would write a service-defaults config regardless of the
value of the -default-protocol flag or the Pod's protocol annotation. If
neither of these were set, we would write a config with protocol set to
the empty string. This is the same as setting it to tcp. So for every
service, we were by default setting its protocol to tcp.

If a user explicitly created a global proxy-defaults config and set the
protocol to, say, http. Then our service-defaults config would override
that protocol. This behaviour was unexpected because users had never
explicitly told us to write a service-defaults config and it wasn't
helping them.

This change will cause us to only write a service-defaults config if the
protocol is explicitly set–either via the -default-protocol flag or via
the Pod annotation.

I've also renamed the CentralConfig field to WriteServiceDefaults
because there are many types of central config now so it's best to be
explicit.

Fixes #168
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connect Related to Connect service mesh, e.g. injection
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant