-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Initial release of open source module #1
Initial release of open source module #1
Conversation
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.
@ryanckoch Some notes:
- Let's add support and an example with shared VPC.
- Remove credentials file everywhere
Received this error on running "simple" example:
Error: module.gke.google_container_node_pool.pools: node_config.0: invalid or unknown key: disk_type
README.md
Outdated
```hcl | ||
module "gke" { | ||
source = "github.com/terraform-google-modules/terraform-google-kubernetes-engine" | ||
credentials_path = "${local.credentials_file_path}" |
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.
Let's remove credentials_path everywhere!
subnetwork = "us-central1-01" | ||
ip_range_pods = "us-central1-01-gke-01-pods" | ||
ip_range_services = "us-central1-01-gke-01-services" | ||
node_service_account = "project-service-account@<PROJECT ID>.iam.gserviceaccount.com" |
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.
Service account should be configurable only on a node_pool level so I can have two node pools with different service accounts. Use the service account of the first node pool for the empty "default" node pool.
- Create GKE Node Pool(s) with provided configuration and attach to cluster | ||
- Replace the default kube-dns configmap if `stub_domains` are provided | ||
- Activate network policy if `network_policy` is true | ||
- Add `ip-masq-agent` configmap with provided `masq_non_masquerade_cidrs` if `network_policy` is true or `masq_config_enabled` is true |
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.
Change masq_non_masquerade_cidrs
to non_masquerade_cidrs
.
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.
Done.
@@ -0,0 +1,59 @@ | |||
/** |
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.
Don't need all outputs for all examples, just actually useful things (ex. endpoint for service).
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.
Done.
examples/deploy_service/main.tf
Outdated
target_port = 80 | ||
} | ||
|
||
type = "ClusterIP" |
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.
Let's deploy as a load balancer, easier to see results.
variables.tf
Outdated
|
||
variable "http_load_balancing" { | ||
description = "Enable httpload balancer addon" | ||
default = false |
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.
Should default to enabled.
variables.tf
Outdated
|
||
variable "horizontal_pod_autoscaling" { | ||
description = "Enable horizontal pod autoscaling addon" | ||
default = true |
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.
Should default to disabled.
variables.tf
Outdated
|
||
variable "kubernetes_dashboard" { | ||
description = "Enable kubernetes dashboard addon" | ||
default = true |
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.
Should be disabled by default.
variable "ip_masq_non_masquerade_cidrs" { | ||
type = "list" | ||
description = "List of strings in CIDR notation that specify the IP address ranges that do not use IP masquerading." | ||
default = ["10.0.0.0/8"] |
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.
Would this override the defualt ip-masq-agent behavior of all RFC1918 address space?
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.
From recent experience it may be prudent to include ip_range_pods
and ip_range_services
and the node network CIDR as a local var type if this were empty. Happy to discuss conditions I have in mind if you want to chat
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. @ryanckoch We should update the default to include all of RFC1918 space, as documented here: https://cloud.google.com/kubernetes-engine/docs/how-to/ip-masquerade-agent#how_ipmasq_works
ip_range_pods
and ip_range_services
should be part of RFC1918 space so I think that would be fine. Or do we also want to automatically append those when a custom ip_masq CIDR is specified?
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 would recommend against appending new rules to RFC1918. We had just hit a use case for supplying subsets of networks. Just for clarity to the user on declarative nature of the config.
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.
Sounds good, let's make sure to include a note in the docs about this.
@ryanckoch I've moved this code to a branch on the shared repo so we can collaborate, could you open a new PR from it: https://github.com/terraform-google-modules/terraform-google-kubernetes-engine |
…drienthebo/maint/update-changelog-v0.5.0 Update CHANGELOG with changes made from thefirstofthe300's repo
Removed kubernetes dashboard add config.
…odules/master Pull from upstream
…odules/master sync
…odules/master adding commits from master
…ndran-patch-1 Removed kubernetes dashboard add config.
…OPS-846 fix: expose var.datapath_provider to enable network_policy
No description provided.