-
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
Private Cluster Configuration #51
Conversation
c494ed9
to
ee437e3
Compare
/****************************************** | ||
Create regional cluster | ||
*****************************************/ | ||
resource "google_container_cluster" "primary_private" { |
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.
What is the reason for needing a separate cluster config for private clusters? Ideally we should minimize how many different google_container_cluster
resources we have?
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.
Apologies, a commit was missing from this. 719ee8b shows the actual configuration of the private cluster.
The issue here is that Terraform 0.11.x doesn't support conditionally adding this block. Hopefully Terraform 0.12 will make this less painful, but for now I'm not sure there's a better option than duplicating the resource.
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.
As far as I can tell, the private_cluster_config
block itself doesn't force anything. If you set all its arguments to false, you would have a standard public cluster.
it's the enable_private_nodes
piece which is what is primarily meant by a private cluster usually.
In terms of arguments, I'd actually like to add 3 new ones:
enable_private_endpoint
enable_private_nodes
master_ipv4_cidr_block
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.
@morgante Unfortunately, the provider requires a master_ipv4_cidr_block
to be provided as a valid CIDR whenever the private_cluster_config
block is specified, and overriding it from the default (unspecified) value can have hard-to-reason-about results. If we simply don't provide that attribute, things work, but we fall into the Terraform 0.11.x trap where an empty string is not considered the same as null, and we have no way to represent null when it should be so.
I would suggest that we merge this as-is, understanding that there is additional complexity as a result of this, and file an upstream issue to make the validation on master_ipv4_cidr_block
less stringent.
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 think we should hold off as this is a change which will cause breakage later on if we walk it back and instead file an upstream provider issue.
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 approach this by making a submodule as @aaron-lane suggested.
0e38f96
to
f50d9ba
Compare
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.
Rather than defining a multitude of resources which may or may not be created based on variable values, can we redesign this as a submodule which can be included if a private cluster is desired? The complexity may be significantly reduced.
Other than that, there are just a few small corrections to be made in the tests.
end | ||
|
||
describe "default node pool" do | ||
let(:default_node_pool) { data['nodePools'].select { |p| p['name'] == "default-pool" }.first } |
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 appears to be obsolete in this context.
|
||
it "is private" do | ||
expect(data['privateClusterConfig']['enablePrivateEndpoint']).to eq true | ||
expect(data['privateClusterConfig']['enablePrivateNodes']).to eq 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.
Ditto about single assertions.
|
||
it "is private" do | ||
expect(data['privateClusterConfig']['enablePrivateEndpoint']).to eq true | ||
expect(data['privateClusterConfig']['enablePrivateNodes']).to eq 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.
We should try to limit tests to single assertions in order to provide clarity when tests fail.
31434df
to
cc77bfb
Compare
Closing in favor of #69 |
Supersedes #21. Includes #48 to allow testing on Concourse.