Add insecure-skip-tls-verify option in cluster's secret

[isutton]

If the 'tls.insecure-skip-tls-verify' key is found in the cluster secret
data, configure the client to allow insecure connections. Having this
option makes it easier to develop Shipper against Docker for Desktop's
Kubernetes instead of Minikube.

[kanatohodets]

It seems a little funky to filter for IsDecodeError like this.

I'd hope that in 99% of cases we take the branch that involves insecureSkipTLSVerify being missing from the Secret: IMO the production secrets should not have this key present at all, since the consequences of it being interpreted as "true" are significant.

The way this is written, it isn't immediately obvious to me what will happen if that key is not present (will we assign "nil" to that field in the config? but the config field is a bool? does this even compile?) -- after reading the inner function I can see its OK, it just seems like it could be clearer.

[isutton]

insecureSkipTlsVerify is a bool, and util.GetBool returns false in case an error is returned. The check on IsDecodeError is meant to catch cases where the value exists in the secret but couldn't be decoded (base64.StdEncoding.DecodeString has failed) or parsed (strconv.ParseBool has failed).

[isutton]

From our conversation offline earlier, I'll change this implementation to fetch the insecure "flag" from the v1.Secret label instead, since the cluster secret was intended initially to contain only client identification information, which insecure-skip-tls-verify clearly isn't.

[kanatohodets]

Following up from the earlier comment, I think KeyNotFoundError isn't really an error, perhaps -- it seems fine to me to have a secret which does not specify insecureSkipTLSVerify at all.

It strikes me as a (minor) bug in Docker that it requires this setting to be present in order to work, so I'd like us to be soft about expecting it.

[isutton]

Another option would be return (*bool, err). This would make possible for util.GetBool to return the nil, nil tuple, but would still require an extra check on whether *bool is nil or has a value.

This would require the caller to use it like the following:

if v, err := util.GetBool(secret, "k"); err != nil { 
        // Decode or ParseBool error
} else if v != nil {
        // Do something with v here.
}

[ksurent]

I think it should be an annotation, not a label. Perhaps booking.com/clusterSecret.tls.insecure-skip-verify or something along those lines.

[isutton]

I think it should be an annotation, not a label. Perhaps booking.com/clusterSecret.tls.insecure-skip-verify or something along those lines.

I didn't explore this path since it can be easier to search for cluster secrets with this particular label than annotations (as in kubectl get secrets -n shipper-system -l tls.insecure-tls-skip-verify).

But yes, after revisiting labels (meant to be consumed by humans) and annotations (meant to be used as internal metadata) I'll use an annotation instead.