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

Jump to bottom

Add annotations for enabling TLS on Envoy Prometheus endpoint #1303

Merged
merged 5 commits into from
Jun 29, 2022
Merged

Add annotations for enabling TLS on Envoy Prometheus endpoint #1303

merged 5 commits into from
Jun 29, 2022

Conversation

kyhavlov
Copy link
Contributor

@kyhavlov kyhavlov commented Jun 23, 2022 •
edited
Loading

Changes proposed in this PR:

How I've tested this PR:

  • Updated a container_init test to make sure the CLI flags get passed through correctly to the Envoy bootstrap command.
  • Testing this out manually with a CSI volume to make sure Envoy watches the cert paths correctly when they're updated on disk.

How I expect reviewers to test this PR:

  • The surface area of the change is pretty small; it should be covered by the updated unit test.

Checklist:

  • Tests added
  • CHANGELOG entry added

    HashiCorp engineers only, community PRs should not add a changelog entry.
    Entries should use present tense (e.g. Add support for...)

kyhavlov
kyhavlov commented Jun 23, 2022
View reviewed changes
charts/consul/templates/client-daemonset.yaml Outdated
@@ -1,6 +1,6 @@
{{- if .Values.global.imageK8s }}{{ fail "global.imageK8s is not a valid key, use global.imageK8S (note the capital 'S')" }}{{ end -}}
{{- if (or (and (ne (.Values.client.enabled | toString) "-") .Values.client.enabled) (and (eq (.Values.client.enabled | toString) "-") .Values.global.enabled)) }}
{{- 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 -}}
# {{- 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 -}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this check (and the one in server-statefulset.yml) need to be modified at all, or can it just be removed? Further down in this file we set the http port to 8501 if tls.enabled is true, so it seems like this can probably be deleted because now we'll support scraping both agent and Envoy prometheus metrics over TLS.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can just delete this check now and the corresponding BATS tests (if there are any)

@jmurret jmurret requested review from a team, ndhanushkodi and thisisnotashwin and removed request for a team June 23, 2022 23:00
kschoche
kschoche reviewed Jun 24, 2022
View reviewed changes
control-plane/connect-inject/container_init.go
@@ -214,6 +219,19 @@ func (w *MeshWebhook) containerInit(namespace corev1.Namespace, pod corev1.Pod,
data.PrometheusScrapePath = prometheusScrapePath
data.PrometheusBackendPort = mergedMetricsPort
}
// Pull the TLS config from the relevant annotations.
Copy link
Contributor

Choose a reason for hiding this comment

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

Are all of these annotations required or just one?
If they are all required, or required in sets we should enforce that with some validation and errors, in which case we should also update the unit tests accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cert, key, and one of CA file or path is required - would that validation go in the client/server daemonset templates, or somewhere else?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since it can be overridden by annotations I think we need it on both ends:

  • in the template logic for the defaults
  • here for the overrides

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the check/tests here - these won't be settable in the template/defaults though so this should be the only place the validation is needed.

@david-yu
Copy link
Contributor

Could we get some reviews on this from @thisisnotashwin and @ndhanushkodi. It looks like the feature is now merged on the Core side in main.

thisisnotashwin
thisisnotashwin approved these changes Jun 27, 2022
View reviewed changes
Copy link
Contributor

@thisisnotashwin thisisnotashwin left a comment

Choose a reason for hiding this comment

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

Thanks so much for doing this Kyle!

charts/consul/templates/client-daemonset.yaml Outdated
@@ -1,6 +1,6 @@
{{- if .Values.global.imageK8s }}{{ fail "global.imageK8s is not a valid key, use global.imageK8S (note the capital 'S')" }}{{ end -}}
{{- if (or (and (ne (.Values.client.enabled | toString) "-") .Values.client.enabled) (and (eq (.Values.client.enabled | toString) "-") .Values.global.enabled)) }}
{{- 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 -}}
# {{- 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 -}}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can just delete this check now and the corresponding BATS tests (if there are any)

kschoche
kschoche reviewed Jun 29, 2022
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
kschoche
kschoche approved these changes Jun 29, 2022
View reviewed changes
Copy link
Contributor

@kschoche kschoche left a comment

Choose a reason for hiding this comment

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

This looks great, thanks for doing this! I left just a couple comments which should help clear up the changelog so users can pick it up and run with it!

@kyhavlov @kschoche
Update CHANGELOG.md
bb14dc3 
Co-authored-by: Kyle Schochenmaier <kschoche@gmail.com>
@kyhavlov kyhavlov merged commit 53cb40d into main Jun 29, 2022
@kyhavlov kyhavlov deleted the prometheus-tls branch June 29, 2022 02:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kschoche kschoche kschoche approved these changes

@thisisnotashwin thisisnotashwin thisisnotashwin approved these changes

@ndhanushkodi ndhanushkodi Awaiting requested review from ndhanushkodi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@kyhavlov @david-yu @kschoche @thisisnotashwin