fix: allow insecure clusters #800

filariow · 2023-09-27T16:34:03Z

No description provided.

Signed-off-by: Francesco Ilario <[email protected]>

openshift-ci · 2023-09-27T16:34:14Z

Hi @filariow. Thanks for your PR.

I'm waiting for a codeready-toolchain member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

alexeykazakov · 2023-09-27T16:36:19Z

/ok-to-test

alexeykazakov · 2023-09-27T21:28:53Z

OK. It failed :(

    init.go:68: 
        	Error Trace:	/go/src/github.com/codeready-toolchain/toolchain-e2e/testsupport/init.go:68
        	            				/usr/local/go/src/sync/once.go:74
        	            				/usr/local/go/src/sync/once.go:65
        	            				/go/src/github.com/codeready-toolchain/toolchain-e2e/testsupport/init.go:45
        	            				/go/src/github.com/codeready-toolchain/toolchain-e2e/test/migration/setup/setup_migration_test.go:12
        	Error:      	Received unexpected error:
        	            	specifying a root certificates file with the insecure flag is not allowed
        	Test:       	TestSetupMigration
--- FAIL: TestSetupMigration (0.00s)

dperaza4dustbit

looks good but one sugestion

dperaza4dustbit · 2023-09-27T21:55:08Z

testsupport/init.go

-		kubeconfig, err := clientcmd.NewDefaultClientConfig(*apiConfig, &clientcmd.ConfigOverrides{}).ClientConfig()
+		configOverrides := clientcmd.ConfigOverrides{
+			ClusterDefaults: api.Cluster{
+				InsecureSkipTLSVerify: true,


Do we want to make this an option we can pass make instead of hardcoding to true? What if we want to test security?

+1

It seems that disabling the TLSVerify doesn't work on openshift CI at least. If this configuration works on your cluster then maybe we could introduce an env variable to optionally turn it off?

Something like:

make test-e2e DISABLE_KUBE_CLIENT_TLS_VERIFY=true

and keep the tls verification on by default, as it is now.

thanks for the review. I implemented this in 96535fb. Do you have any suggestion on a better place where to fetch the env variable?

Nice! Extracting the config logic builder into a function is what I was thinking as well! 👍

Should we add few lines to document this new variable, maybe adding a know issues/workarounds section here: https://github.com/codeready-toolchain/toolchain-e2e/blob/master/CRC.adoc
reporting the tls failure and the workaround of using this env variable ?

If it's not specific to CRC maybe we could add this new section in the main README.

WDYT?

I like the idea, let me work on this! 👍🏾
It's not related to CRC, so I'll go for the README change

@mfrancisc I've added a note in the README's Running End-to-End Tests. WDYT?

Signed-off-by: Francesco Ilario <[email protected]>

filariow · 2023-09-28T14:16:20Z

OK. It failed :(

    init.go:68: 
        	Error Trace:	/go/src/github.com/codeready-toolchain/toolchain-e2e/testsupport/init.go:68
        	            				/usr/local/go/src/sync/once.go:74
        	            				/usr/local/go/src/sync/once.go:65
        	            				/go/src/github.com/codeready-toolchain/toolchain-e2e/testsupport/init.go:45
        	            				/go/src/github.com/codeready-toolchain/toolchain-e2e/test/migration/setup/setup_migration_test.go:12
        	Error:      	Received unexpected error:
        	            	specifying a root certificates file with the insecure flag is not allowed
        	Test:       	TestSetupMigration
--- FAIL: TestSetupMigration (0.00s)

I was able to reproduce this error on CRC and pushed a new commit that worked on CRC so that we can see if it works also on Openshift CI

Signed-off-by: Francesco Ilario <[email protected]>

mfrancisc · 2023-09-28T15:44:45Z

/ok-to-test

mfrancisc · 2023-09-28T17:00:42Z

/retest

infra

alexeykazakov

Looks good! But please update the README as @mfrancisc suggested.

Signed-off-by: Francesco Ilario <[email protected]>

mfrancisc

Looks good 👍

Thanks for addressing my comments.

openshift-ci · 2023-09-29T10:59:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexeykazakov, filariow, mfrancisc, rajivnathan

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:

~~OWNERS~~ [alexeykazakov]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: Francesco Ilario <[email protected]>

filariow · 2023-09-29T13:00:14Z

Looks good 👍

Thanks for addressing my comments.

Thank you for your reviews. I pushed a new commit renaming the new function to a more coherent name.

README.adoc

Co-authored-by: Baiju Muthukadan <[email protected]>

sonarqubecloud · 2023-09-29T15:51:56Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

alexeykazakov · 2023-09-29T16:58:58Z

/lgtm

fix: allow insecure clusters

08975b5

Signed-off-by: Francesco Ilario <[email protected]>

openshift-ci bot requested review from MatousJobanek and xcoulon September 27, 2023 16:34

openshift-ci bot added the needs-ok-to-test label Sep 27, 2023

alexeykazakov approved these changes Sep 27, 2023

View reviewed changes

openshift-ci bot added the approved label Sep 27, 2023

openshift-ci bot added ok-to-test and removed needs-ok-to-test labels Sep 27, 2023

rajivnathan approved these changes Sep 27, 2023

View reviewed changes

dperaza4dustbit reviewed Sep 27, 2023

View reviewed changes

Override ca.crt in kubeconfigs

244724f

Signed-off-by: Francesco Ilario <[email protected]>

Add feature flag to disable TLS

96535fb

Signed-off-by: Francesco Ilario <[email protected]>

alexeykazakov approved these changes Sep 28, 2023

View reviewed changes

Update README

ee7f712

Signed-off-by: Francesco Ilario <[email protected]>

mfrancisc approved these changes Sep 29, 2023

View reviewed changes

Rename util.BuildKubernetesClient to BuildKubernetesRESTConfig

6efb788

Signed-off-by: Francesco Ilario <[email protected]>

baijum reviewed Sep 29, 2023

View reviewed changes

README.adoc Outdated Show resolved Hide resolved

Update README.adoc

77856fa

Co-authored-by: Baiju Muthukadan <[email protected]>

openshift-ci bot assigned alexeykazakov Sep 29, 2023

openshift-ci bot added the lgtm label Sep 29, 2023

openshift-merge-robot merged commit ff18cca into codeready-toolchain:master Sep 29, 2023
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: allow insecure clusters #800

fix: allow insecure clusters #800

filariow commented Sep 27, 2023

openshift-ci bot commented Sep 27, 2023

alexeykazakov commented Sep 27, 2023

alexeykazakov commented Sep 27, 2023

dperaza4dustbit left a comment

dperaza4dustbit Sep 27, 2023

mfrancisc Sep 28, 2023

filariow Sep 28, 2023

mfrancisc Sep 28, 2023

filariow Sep 29, 2023 •

edited

Loading

filariow Sep 29, 2023

filariow commented Sep 28, 2023 •

edited

Loading

mfrancisc commented Sep 28, 2023

mfrancisc commented Sep 28, 2023

alexeykazakov left a comment

mfrancisc left a comment

openshift-ci bot commented Sep 29, 2023

filariow commented Sep 29, 2023

sonarqubecloud bot commented Sep 29, 2023

alexeykazakov commented Sep 29, 2023

fix: allow insecure clusters #800

fix: allow insecure clusters #800

Conversation

filariow commented Sep 27, 2023

openshift-ci bot commented Sep 27, 2023

alexeykazakov commented Sep 27, 2023

alexeykazakov commented Sep 27, 2023

dperaza4dustbit left a comment

Choose a reason for hiding this comment

dperaza4dustbit Sep 27, 2023

Choose a reason for hiding this comment

mfrancisc Sep 28, 2023

Choose a reason for hiding this comment

filariow Sep 28, 2023

Choose a reason for hiding this comment

mfrancisc Sep 28, 2023

Choose a reason for hiding this comment

filariow Sep 29, 2023 • edited Loading

Choose a reason for hiding this comment

filariow Sep 29, 2023

Choose a reason for hiding this comment

filariow commented Sep 28, 2023 • edited Loading

mfrancisc commented Sep 28, 2023

mfrancisc commented Sep 28, 2023

alexeykazakov left a comment

Choose a reason for hiding this comment

mfrancisc left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Sep 29, 2023

filariow commented Sep 29, 2023

sonarqubecloud bot commented Sep 29, 2023

alexeykazakov commented Sep 29, 2023

filariow Sep 29, 2023 •

edited

Loading

filariow commented Sep 28, 2023 •

edited

Loading