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

Add config for webhook-cert-manager tolerations #712

Merged
merged 7 commits into from
Sep 9, 2021

Conversation

t-eckert
Copy link
Contributor

@t-eckert t-eckert commented Sep 2, 2021

Changes proposed in this PR:

  • Add configuration for the webhook-cert-manager deployment tolerations

This work closes issue #655 where the user was running Consul on a private cluster in GKE.

How I've tested this PR:
Added bats and manually produced the deployment YAML using helm.

How I expect reviewers to test this PR:
Run bats

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...)

@t-eckert t-eckert requested review from a team, lkysow and ishustava and removed request for a team September 2, 2021 19:15
CHANGELOG.md Outdated Show resolved Hide resolved
charts/consul/values.yaml Show resolved Hide resolved
charts/consul/values.yaml Outdated Show resolved Hide resolved
Co-authored-by: Kyle Schochenmaier <kschoche@gmail.com>
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.

Looks great!

@t-eckert t-eckert requested review from ndhanushkodi and removed request for ishustava September 8, 2021 15:51
Copy link
Contributor

@ndhanushkodi ndhanushkodi left a comment

Choose a reason for hiding this comment

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

Looks great Thomas!!

I think the style of tolerations has varied throughout our values.yaml, with some being set to "" and some being set to null, but I think since the inconsistency exists already, and this matches one of the patterns, it still works. For example see https://github.com/hashicorp/consul-k8s/blob/main/charts/consul/values.yaml#L885-L895

@t-eckert
Copy link
Contributor Author

t-eckert commented Sep 9, 2021

@ndhanushkodi, I noticed that too. I thought it might be a good onboarding task to pick a standard and I can go through and change all of the nullish instances to use that in a later PR.

@t-eckert t-eckert merged commit d12d988 into main Sep 9, 2021
@t-eckert t-eckert deleted the t-eckert/webhook-cert-manager-tolerations branch September 9, 2021 21:23
lawliet89 pushed a commit to lawliet89/consul-k8s that referenced this pull request Sep 13, 2021
* Add a new flag to the framework, -use-kind. The only thing it affects currently are the mesh gateway tests: it uses NodePort service instead of the LoadBalancer service to expose the mesh gateways. This is because load balancer services are not available on kind. For the NodePort services, we don't need to do anything to allow cross-cluster communication because kind uses the docker bridge network, and so the nodes are reachable by default.

* Change the main pipeline to use kind instead of gke, and move gke tests to the nightly pipeline.

* Use circleci's parallelism feature to run tests in parallel. Currently, tests are split by package, which is not the best efficient way to parallelize them, but we can address that at a later time. Currently, using 6 parallel nodes reduces the test runtime to about 30min.
t-eckert pushed a commit that referenced this pull request Sep 17, 2021
* Add webhookCertManager.tolerations setting

* Add tolerations to webhook cert template

* Add bats test for setting tolerations

* Update CHANGELOG

* Set PR number in CHANGELOG

* Add short desc to webhookCertManager

* Update charts/consul/values.yaml

Co-authored-by: Kyle Schochenmaier <kschoche@gmail.com>

Co-authored-by: Kyle Schochenmaier <kschoche@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants