Replace the --lets-encrypt flag with --insecure-skip-tls-verify flag #1093
Conversation
metlos
requested review from
MatousJobanek,
xcoulon,
alexeykazakov,
rajivnathan,
ranakan19 and
mfrancisc
as code owners
make/test.mk
Outdated
| else | ||
| TLS_VERIFY_PARAM := --insecure-skip-tls-verify=true |
There was a problem hiding this comment.
should we disable tls verification by default when running the tests "locally" ? Or should we document this and let the user decide ?
There was a problem hiding this comment.
correct me if I'm wrong, if the flag is not specified, then it would use whatever is in the client's kubeconfig. The flag should be used only if you want to override (for some specific reason) the information in kubeconfig.
This means that the default behavior shouldn't use the flag at all, only if requested explicitly via a variable.
There was a problem hiding this comment.
+1 I thought the same, but maybe I've missed something.
There was a problem hiding this comment.
I wanted to keep the same behavior as there was with the --lets-encrypt flag. The main reason being that I'm not fully sure about the consequences of relying on the contents of the kubeconfig.
If the intended behavior should change, I'm fine with it though. As far as I looked we ALWAYS set the --lets-encrypt=false in all environments (e.g. CI or default when running locally) - I haven't found a place where we'd set IS_OSD=true anywhere.
To keep this behavior, we'd need to set --insecure-skip-tls-verify=false only when IS_OSD=true and we should be explicit about it.
I don't know what the kubeconfig looks like in the CI (whether it explicitly sets the insecure-skip-tls-verify or whether the connection to the clusters in the CI doesn't require it) so I didn't want to risk wreaking havoc in all our CI jobs.
There was a problem hiding this comment.
I haven't found a place where we'd set IS_OSD=true anywhere.
that was used for local dev use-cases. Some teams/developers had OSD cluster for the local dev (instead of a pure OCP)
To keep this behavior, we'd need to set --insecure-skip-tls-verify=false only when IS_OSD=true and we should be explicit about it.
if it's OSD and you (as an admin) execute the register-member command, then you don't have this field defined in your kubeconfig, nor the cert as the OSD cluster uses let's encrypt cert which is a well-know one.
I don't know what the kubeconfig looks like in the CI (whether it explicitly sets the insecure-skip-tls-verify or whether the connection to the clusters in the CI doesn't require it)
does it matter? if the kubeconfig has insecure-skip-tls-verify defined, then the command copies it, if not, then it should have the self-sign certificate, which should be copied as well.
Let's do what we wanted to do and what was the reason for these changes - rely on the content of the kubeconfig if not explicitly defined otherwise.
There was a problem hiding this comment.
I discovered we already have DISABLE_KUBE_CLIENT_TLS_VERIFY to have this behavior for the kubeclient used in the tests. I think we can use this also to specify whether to use --insecure-skip-tls-verify=true for the ksctl. I've updated the make files to do this.
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fbm3307, metlos, mfrancisc The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
This is to update toolchain-e2e to the new flag in ksctl.
See kubesaw/ksctl#95.