Support for disabling basic auth & client certificate

[miles-]

There isn't currently a way to disable basic authentication and the creation of a client certificate at cluster creation time. It looks like both of these can be accomplished by setting a master_auth config.

https://www.terraform.io/docs/providers/google/r/container_cluster.html#master_auth

[morgante]

Yes, I think it's a good idea to add this. Do you think we would want it as a default?

[miles-]

Yes I think it makes sense to disable both by default. When creating a cluster via the GCP console, both options have recommendations to disable them. Also, they will be disabled by default in 1.12.

[coryodaniel]

So I looked into this a few notes/questions:

Our best practices guide says to name variables in the positive. Defaulting to disabled basic auth (enable_basic_auth = false) will be a breaking change for deployed clusters. Enabling it by default requires a username and password. The default pw for GKE is 'admin', but the password is random.

So:

• Do we want to enable basic auth by default but, change their password using tf resource "random"
• Do we want to name this in the negative? Default: disable_basic_auth = false to prevent breakage?

Disabling client certificates is a destructive change. My assumption is we should name this in the positive and enable it by default. This is a catch-22 since its destructive to change. If we enable client certs by default, its not destructive to current users, but opts new users into a bad practice with the chance of being destructive if they did not notice the variable. If we disable it by default, its destructive to current users, but sets the module up for best practices.

It may also be worth noting, to set the certificate explicitly, its a nested setting that also requires the username and password be set. So its technically not possible to make this non-breaking for current users, because as soon as the certificate setting is set, the username and password has to be as well.

For setting basic auth, which do you prefer?

enable_basic_auth = true
basic_auth_username = "xxx"
basic_auth_password = "xxx"

basic_auth {
  username = "xxx"
  password = "yyy"
}

The later would assume if the map is empty, it is disabled.

@morgante

[morgante]

1. For basic auth, I think we should go with enable_basic_auth = false as the default going forward. This will generate a diff for existing module users, but that is acceptable as they can choose to either explicitly encode the desire to enable basic auth or require the new version.

2. Client certificates sound slightly more destructive and we use them internally, so let's start by adding an enable_client_certificate variable with a default of true.

Also, as per the docs it does seem to be possible to enable cluster cert but disable basic auth so we need to support that scenario: https://www.terraform.io/docs/providers/google/r/container_cluster.html#client_certificate_config

[coryodaniel]

Yep, I got that support in my first pass! Thanks for the input, sounds like a good approach to me.

[aaron-lane]

Fixed by #40.