-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 definition of a Safer Cluster module. #272
Initial definition of a Safer Cluster module. #272
Conversation
I can't find details about the concourse-ci/lint-tests failure. "make -s" is successful. How can I get the logs? |
@mmontan You can ignore the (legacy) Concourse tests, but please do try to get the Cloud Build triggers working. You should have access to Cloud Build logs, but if not please ping me. |
The PR adds a new "safer" GKE module. The module includes hardening suggestions from multiple sources.
The Cloud Build triggers are working now. |
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.
In general, I think that instead of making a specific safer_cluster
by wrapping the beta_private_cluster
, we should be just making the beta_private_cluster
secure by default by updating necessary variable defaults. Could you please give some reasoning or justification as to why we need this "safer" wrapper module as opposed to making the exising module(s) secure by default?
subnetwork = var.subnetwork | ||
ip_range_pods = var.ip_range_pods | ||
ip_range_services = var.ip_range_services | ||
master_ipv4_cidr_block = "172.16.0.0/28" |
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 should be a variable instead of being hard-coded to allow users to pass in their own master CIDR block
http_load_balancing = var.http_load_balancing | ||
|
||
// Disable the dashboard. It creates risk by running as a very sensitive user. | ||
kubernetes_dashboard = 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.
This is unnecessary since kubernetes_dashboard
is false by default.
// policies should be under the control of a cental cluster management team, | ||
// rather than individual teams. | ||
network_policy = true | ||
network_policy_provider = "CALICO" |
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.
Unnecessary since network_policy_provider
is "CALICO"
by default
// destroying the cluster. | ||
remove_default_node_pool = true | ||
|
||
disable_legacy_metadata_endpoints = 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.
disable_legacy_metadata_endpoints
is already true by default
node_pools_tags = var.node_pools_tags | ||
|
||
// TODO(mmontan): we generally considered applying | ||
// just the cloud-platofrm scope and use Cloud IAM |
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.
typo: s/cloud-platofrm/cloud-platform/
upstream_nameservers = var.upstream_nameservers | ||
|
||
// We should use IP Alias. | ||
configure_ip_masq = 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.
configure_ip_masq
is already false by default
|
||
The module defines a safer configuration for a GKE cluster. | ||
|
||
TODO(mmontan): add documentation for the module. |
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.
Please add your README from #281 into this PR
// All applications shuold run with an identity defined via Workload Identity anyway. | ||
// - Use a service account passed as a parameter to the module, in case the user | ||
// wants to maintain control of their service accounts. | ||
create_service_account = length(var.compute_engine_service_account) > 0 ? false : 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.
Instead of using length
here, prefer comparing it with its default value. Such as:
var.compute_engine_service_account == "" ? false : true
database_encryption = var.database_encryption | ||
|
||
// We suggest to define policies about which images can run on a cluster. | ||
enable_binary_authorization = 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.
It's worth noting that since you are using the beta-private-cluster
, this field enable_binary_authorization
is actually not implemented yet, it's just a stub so it doesn't do anything
}] | ||
|
||
resource_usage_export_dataset_id = var.resource_usage_export_dataset_id | ||
node_metadata = "SECURE" |
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.
node_metadata
is already "SECURE"
by default
The PR adds a new "safer" GKE module. The module includes hardening suggestions from multiple sources.
…ubernetes-engine into safer-cluster
Hi @mmontan is this being worked on? |
@bharathkkb Go ahead and take this over. |
@bharathkkb Thank you for taking this over! |
Still TODO: