Bug 1795035: Give reason for not using cloud provider CA cert

[mandre]

This makes it easier to debug in case CAPO can't read the CA cert from
the openshift-config/cloud-provider-config configmap.

[mandre]

/bugzilla refresh

[openshift-ci-robot]

@mandre: This pull request references Bugzilla bug 1795035, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

/bugzilla refresh

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.

[pierreprinetti]

watch out: the cacert == "" condition now leads to calling NewInstanceServiceFromCloud with a non-nil argument ([]byte(""), which is a non-nil slice of length zero). In NewInstanceServiceFromCloud, that will trigger the code for adding a new CA

cluster-api-provider-openstack/pkg/cloud/openstack/clients/machineservice.go

Lines 223 to 237 in 86e93b0

	if cert != nil {
	certPool, err := x509.SystemCertPool()
	if err != nil {
	return nil, fmt.Errorf("Create system cert pool failed: %v", err)
	}
	certPool.AppendCertsFromPEM(cert)
	client := http.Client{
	Transport: &http.Transport{
	TLSClientConfig: &tls.Config{
	RootCAs: certPool,
	},
	},
	}
	provider.HTTPClient = client
	}

[iamemilio]

I think the way @mandre sets the warning is correct. Validation should be done here logically:

cluster-api-provider-openstack/pkg/cloud/openstack/clients/machineservice.go

Line 224 in 393f770

if cert != nil {

I also think a separate bz+ pr should handle this though. I'd be ok to lgtm this

[mandre]

Yes, that was intentional. I think we shouldn't do validation on the cert and instead defer to the certPool.AppendCertsFromPEM(cert) call below to know what to do with an empty string.

It should be pretty obvious to debug if the user sets the configmap entry to an empty string by mistake.

[iamemilio]

Pierre made a good point in our conversation. The block of code wrapped in the conditional he linked above should only run if there is a cert. By default, we set the cert to "", so this means that this code block always runs by default, which is confusing.

[adduarte]

would it make sense to move the cacert == "" to inside NewInstanceServiceFromCloud?

like so if (cert != nil && cert != ""),
It would catch any calls to the function that pass in a cert = "" condition.

[mandre]

I'd rather not make any assumption about the value of cert other than it's not nil if certPool.AppendCertsFromPEM(cert) is capable of dealing with it.

[pierreprinetti]

/lgtm
/honk

[openshift-ci-robot]

@pierreprinetti:

In response to this:

/lgtm
/honk

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.

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mandre, pierreprinetti

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 [mandre,pierreprinetti]

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

[openshift-ci-robot]

@mandre: All pull requests linked via external trackers have merged. Bugzilla bug 1795035 has been moved to the MODIFIED state.

In response to this:

Bug 1795035: Give reason for not using cloud provider CA cert

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.

[openshift-ci-robot]

@mandre: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-openstack	3641ea3	link	/test e2e-openstack

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.