Skip to content
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.

Sign up for GitHub

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

Replace the --lets-encrypt flag with --insecure-skip-tls-verify flag #1093

Merged
merged 5 commits into from
Jan 17, 2025
Merged

Replace the --lets-encrypt flag with --insecure-skip-tls-verify flag #1093

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ba5d466
Replace the --lets-encrypt flag with --insecure-skip-tls-verify flag
metlos Jan 13, 2025
cd19006
reuse the already used DISABLE_KUBE_CLIENT_TLS_VERIFY to already defi…
metlos Jan 16, 2025
bae6563
remove the now unused IS_OSD variable
metlos Jan 16, 2025
e9c149a
Merge branch 'master' into remove-lets-encrypt-flag
mfrancisc Jan 16, 2025
1e68117
Merge branch 'master' into remove-lets-encrypt-flag
fbm3307 Jan 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions make/test.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,12 @@ endif

E2E_TEST_EXECUTION ?= true

ifneq ($(IS_OSD),true)
LETS_ENCRYPT_PARAM := --lets-encrypt=false
# let's always be explicit about TLS verify param so that we don't have to rely on the values
# set in the kubeconfig (if any)
ifeq ($(IS_OSD),true)
TLS_VERIFY_PARAM := --insecure-skip-tls-verify=false
else
TLS_VERIFY_PARAM := --insecure-skip-tls-verify=true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we disable tls verification by default when running the tests "locally" ? Or should we document this and let the user decide ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 I thought the same, but maybe I've missed something.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

endif

TESTS_RUN_FILTER_REGEXP ?= ""
Expand Down Expand Up @@ -78,7 +82,7 @@ e2e-migration-verify:

.PHONY: e2e-deploy-latest
e2e-deploy-latest:
$(MAKE) get-publish-install-and-register-operators MEMBER_NS=${MEMBER_NS} MEMBER_NS_2=${MEMBER_NS_2} HOST_NS=${HOST_NS} REGISTRATION_SERVICE_NS=${REGISTRATION_SERVICE_NS} ENVIRONMENT=${ENVIRONMENT} INSTALL_OPERATOR=${INSTALL_OPERATOR} DEPLOY_LATEST=true LETS_ENCRYPT_PARAM=${LETS_ENCRYPT_PARAM}
$(MAKE) get-publish-install-and-register-operators MEMBER_NS=${MEMBER_NS} MEMBER_NS_2=${MEMBER_NS_2} HOST_NS=${HOST_NS} REGISTRATION_SERVICE_NS=${REGISTRATION_SERVICE_NS} ENVIRONMENT=${ENVIRONMENT} INSTALL_OPERATOR=${INSTALL_OPERATOR} DEPLOY_LATEST=true TLS_VERIFY_PARAM=${TLS_VERIFY_PARAM}

.PHONY: prepare-e2e
prepare-e2e: build clean-e2e-files
Expand Down Expand Up @@ -263,8 +267,8 @@ print-operator-logs:

.PHONY: setup-toolchainclusters
setup-toolchainclusters: ksctl
${KSCTL_BIN_DIR}ksctl adm register-member --host-ns="$(HOST_NS)" --member-ns="$(MEMBER_NS)" --host-kubeconfig="$(or ${KUBECONFIG}, ${HOME}/.kube/config)" --member-kubeconfig="$(or ${KUBECONFIG}, ${HOME}/.kube/config)" ${LETS_ENCRYPT_PARAM} -y
if [[ ${SECOND_MEMBER_MODE} == true ]]; then ${KSCTL_BIN_DIR}ksctl adm register-member --host-ns="$(HOST_NS)" --member-ns="$(MEMBER_NS_2)" --host-kubeconfig="$(or ${KUBECONFIG}, ${HOME}/.kube/config)" --member-kubeconfig="$(or ${KUBECONFIG}, ${HOME}/.kube/config)" ${LETS_ENCRYPT_PARAM} --name-suffix="2" -y ; fi
${KSCTL_BIN_DIR}ksctl adm register-member --host-ns="$(HOST_NS)" --member-ns="$(MEMBER_NS)" --host-kubeconfig="$(or ${KUBECONFIG}, ${HOME}/.kube/config)" --member-kubeconfig="$(or ${KUBECONFIG}, ${HOME}/.kube/config)" ${TLS_VERIFY_PARAM} -y
if [[ ${SECOND_MEMBER_MODE} == true ]]; then ${KSCTL_BIN_DIR}ksctl adm register-member --host-ns="$(HOST_NS)" --member-ns="$(MEMBER_NS_2)" --host-kubeconfig="$(or ${KUBECONFIG}, ${HOME}/.kube/config)" --member-kubeconfig="$(or ${KUBECONFIG}, ${HOME}/.kube/config)" ${TLS_VERIFY_PARAM} --name-suffix="2" -y ; fi


.PHONY: deploy-single-member-e2e
Expand Down Expand Up @@ -468,4 +472,4 @@ test-support:

.PHONY: test-setup
test-setup:
@go test github.com/codeready-toolchain/toolchain-e2e/setup/... -failfast
@go test github.com/codeready-toolchain/toolchain-e2e/setup/... -failfast
Loading