Skip to content
This repository has been archived by the owner on Feb 22, 2022. It is now read-only.

[incubator/vault] make High Availability and TLS work #4968

Closed
wants to merge 1 commit into from

Conversation

nazarewk
Copy link
Contributor

@nazarewk nazarewk commented Apr 12, 2018

Builds on top of #4709

What this PR does / why we need it:
We need this PR to make it possible to run High Availability Vault with TLS enabled and request forwarding working (behind LoadBalancer/Ingress).

This PR adds following:

  • changes livenessProbe to httpGet always returning 200,
  • makes readinessProbe customization map more directly to GET parameters,
  • allows setting environment variables through .Values.vault.env (sets VAULT_ADDR to http:// by default),
  • allows using Pod cluster DNS name as cluster_address (.Values.vault.podDNSAsClusterAddr) enabling creation of wildcard TLS certificate (*.<namespaces>.pod.cluster.local) therefore enabling request forwarding instead of redirection in TLS setup,
  • moved podAnnotations to vault.annotations,
  • adds HA & TLS examples to README.md,
  • adds maxSurge and maxUnavailable customization,

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 12, 2018
@k8s-ci-robot
Copy link
Contributor

Hi @nazarewk. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 12, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: nazarewk
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: scottrigby

Assign the PR to them by writing /assign @scottrigby in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@nazarewk
Copy link
Contributor Author

/assign @scottrigby

@ghost
Copy link

ghost commented Apr 14, 2018

This looks pretty good. Until things like this are solved at helm, this is likely the best we can do for Vault itself. helm/helm#3276 (Maybe Vault's secrets engines would help with subsequently installed charts).

Edit: There might be an opportunity to use Let's Encrypt here, but that's a polarizing component -- so I get the need for custom cert.

@sfrique
Copy link
Contributor

sfrique commented Apr 16, 2018

Part of this is implemented at: PR #4709

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 17, 2018
@nazarewk nazarewk changed the title [incubator/vault] fix readinessProbe, enable TLS and allow setting env vars [incubator/vault] make High Availability and TLS work Apr 17, 2018
@nazarewk
Copy link
Contributor Author

nazarewk commented Apr 17, 2018

@sfrique
Right I did not see the alternative PR.

@coreypobrien
Should we join our efforts and make a single PR?
I have just managed to set up full HA (with requests forwarding) with proper TLS certificate issued on my cluster using most recent version (0.7.11)

@coreypobrien
Copy link
Contributor

It looks like this is substantially similar to #4709. It looks like the primary additions here are the option to use pod dns instead of pod IP for the cluster_addr and adding arbitrary env vars. Do you think you could add that after #4709 merges?

Out of curiousity, what is the benefit of using the pod dns for cluster_addr? It seems like a lot of hoops to go through (e.g. random jq work in an init container) but I'm not sure why.

@nazarewk
Copy link
Contributor Author

nazarewk commented Apr 18, 2018

@coreypobrien it allows us to issue a wildcard certificate (*.<namespace>.pod.cluster.local) for the Vault server. We can hardly issue certificates on per-pod basis without external tools.

I am not sure what is your setup, but in my setup behind Ingress it is necessary for request forwarding. Vault redirects to the common api_addr and HTTP/2 does not drop the connection and we get trapped in a single pod.

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 18, 2018
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 18, 2018
@nazarewk
Copy link
Contributor Author

@coreypobrien how about i just rebase onto your branch and then we discuss changes?

@nazarewk nazarewk force-pushed the vault-tls branch 3 times, most recently from 193e6c2 to 35208d1 Compare April 18, 2018 07:06
@nazarewk nazarewk closed this Apr 18, 2018
@nazarewk nazarewk deleted the vault-tls branch April 18, 2018 07:06
@nazarewk nazarewk restored the vault-tls branch April 18, 2018 07:08
@nazarewk
Copy link
Contributor Author

closing for a rework

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants