-
Notifications
You must be signed in to change notification settings - Fork 282
Upgrade Examples and tests to use Helm 3 #88
Conversation
Looks like the tests failed. I'll try and see tomorrow what the problem was. |
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 looking pretty good to me. My only question is should the default example in the root of the repo be a private GKE cluster? I believe that's what we had originally? Is it too hard to remotely install charts this way?
.circleci/config.yml
Outdated
@@ -3,13 +3,12 @@ defaults: &defaults | |||
environment: | |||
GRUNTWORK_INSTALLER_VERSION: v0.0.21 | |||
TERRATEST_LOG_PARSER_VERSION: v0.13.13 | |||
KUBERGRUNT_VERSION: v0.3.9 | |||
HELM_VERSION: v2.11.0 | |||
HELM_VERSION: v3.1.0 |
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.
I might update this to v3.1.2
to match terraform-aws-eks
.
The original root example was the private Tiller example. That's why I opted for the Helm example as the new root. |
@autero1 sure but is it possible to do a Helm example in the new root with a private cluster? |
Ah, you're right... 😄 It was indeed a private cluster. I'll change that and fix the tests. |
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.
Looking solid 👍 Left some nits.
examples/gke-basic-helm/README.md
Outdated
Forwarding from [::1]:8080 -> 8080 | ||
``` | ||
|
||
You can now access the deployed service by opening your web browser to `http://localhost:8088`. |
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.
Did you mean for this to be http:localhost:8080
?
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.
Good catch!
@@ -0,0 +1,88 @@ | |||
# GKE Basic Helm Example |
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.
Awesome docs 👍
main.tf
Outdated
tiller_tls_key_file_name = "tls.pem" | ||
|
||
dependencies = [null_resource.tiller_tls_certs.id, kubernetes_cluster_role_binding.user.id] | ||
data "helm_repository" "bitnami" { |
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.
NIT: maybe worth a comment about what is happening here - would probably be helpful for folks new to Helm concepts.
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.
Specifically that its going to look up a chart from a chart repository.
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.
NIT: There is actually a critical issue with this data source (hashicorp/terraform-provider-helm#438 and hashicorp/terraform-provider-helm#416), and they are considering removing it entirely. I've found that it is much more robust and stable to directly use the repo URL in the repository
attribute of the helm_release
resource instead.
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.
Thanks @yorinasub17 for pointing this out. I'll fix this!
Co-Authored-By: Zack Proser <[email protected]>
…ke into helm3_upgrade
I bumped a lot of tool versions, but don't know if it fixed the test issue. A more confusing thing was an issue with the network module. Since v0.3.0 the After some experimentation, the example is successfully deployed to the Decided not to change the network module version for the other examples to not bloat the PR even further. |
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.
A more confusing thing was an issue with the network module. Since v0.3.0 the terraform-google-network changed NAT routing from public to private. I used v0.4.0 in the example (other examples untouched) and I initially failed to install any charts, because the nodes did not have internet connectivity and could not pull the images.
Actually it appears we'd always had a bug in our vpc-network
module. So I shipped gruntwork-io/terraform-google-network#53 in v0.3.0
, which enabled NAT routing for private subnetworks instead of the public one (I assumed the public subnetwork doesn't need NAT routing anyhow).
Sorry if that was confusing, but I think we have the right behaviour now. Please correct me if I'm wrong:
- Private Subnetworks with NAT routing enabled so the Nodes have internet connectivity.
- A Private GKE Cluster with a Helm chart deployed.
# This is an example of how to use the gke-cluster module to deploy a private Kubernetes cluster in GCP | ||
# DEPLOY A GKE PUBLIC CLUSTER IN GOOGLE CLOUD PLATFORM | ||
# This is an example of how to use the gke-cluster module to deploy a public Kubernetes cluster in GCP with a | ||
# Load Balancer in front of it. |
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.
# Load Balancer in front of it. |
# See the network access tier table for full details: | ||
# https://github.com/gruntwork-io/terraform-google-network/tree/master/modules/vpc-network#access-tier | ||
subnetwork = module.vpc_network.public_subnetwork | ||
subnetwork = module.vpc_network.private_subnetwork |
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.
nice 👍
Co-Authored-By: Rob Morgan <[email protected]>
Co-Authored-By: Rob Morgan <[email protected]>
Co-Authored-By: Rob Morgan <[email protected]>
That is correct. And I agree that with the fix in the network module, we now have correct behaviour. I applied the fixes you suggested. |
@autero1 cool man, I'll kick off the tests again |
@autero1 It looks like they passed in https://circleci.com/gh/gruntwork-io/terraform-google-gke/457. Ok to merge? |
Ok to merge! Though the tests seemed to fail until you made final tweaks to the PR branch. Will you merge those after you merge this PR? |
@autero1 yep, I'll merge my branch. It will take all of your commits, apply mine on top and close this PR! |
This PR updates the tests and examples to use Helm 3 instead of Helm 2 with Tiller. As a result, one of the examples was removed as redundant and the new
gke-basic-helm
was moved to the root of the repo.The
TestGKEBasicHelm
test case has some redundancy, as both the Terraform example and the test deploy a chart, but I decided to leave it that way - primarily because I felt it made the example itself much better, because the user actually gets to interact with the cluster.Tests pass locally, the only question mark for me is how they behave in CircleCI.
Fixes #82