Skip to content

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

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

Merged
openshift-merge-robot merged 1 commit into from
Jan 30, 2020
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
27 changes: 10 additions & 17 deletions pkg/cloud/openstack/clients/machineservice.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,20 +152,6 @@ func GetCloudFromSecret(kubeClient kubernetes.Interface, namespace string, secre
return clouds.Clouds[cloudName], nil
}

func getCACertFromConfigmap(kubeClient kubernetes.Interface, namespace string, configmapName string, key string) (string, error) {
cloudConfig, err := kubeClient.CoreV1().ConfigMaps(namespace).Get(configmapName, metav1.GetOptions{})
if err != nil {
return "", fmt.Errorf("failed to get configmap %s/%s/%s from kubernetes api: %v", namespace, configmapName, key, err)
}

val, ok := cloudConfig.Data[key]
if !ok {
return "", fmt.Errorf("configmap does not contain key, %s", key)
}

return val, nil
}

// TODO: Eventually we'll have a NewInstanceServiceFromCluster too
func NewInstanceServiceFromMachine(kubeClient kubernetes.Interface, machine *machinev1.Machine) (*InstanceService, error) {
machineSpec, err := openstackconfigv1.MachineSpecFromProviderSpec(machine.Spec.ProviderSpec)
Expand All @@ -184,12 +170,17 @@ func NewInstanceServiceFromMachine(kubeClient kubernetes.Interface, machine *mac
}
}

cacert, err := getCACertFromConfigmap(kubeClient, "openshift-config", "cloud-provider-config", "ca-bundle.pem")
if err != nil || cacert == "" {
cloudConfig, err := kubeClient.CoreV1().ConfigMaps("openshift-config").Get("cloud-provider-config", metav1.GetOptions{})
if err != nil {
Copy link
Copy Markdown

@pierreprinetti pierreprinetti Jan 27, 2020

Choose a reason for hiding this comment

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

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
}

Copy link
Copy Markdown

@iamemilio iamemilio Jan 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

@mandre mandre Jan 29, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown

@iamemilio iamemilio Jan 29, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown

@adduarte adduarte Jan 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member Author

@mandre mandre Jan 29, 2020

Choose a reason for hiding this comment

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

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.

klog.Warningf("failed to get configmap openshift-config/cloud-provider-config from kubernetes api: %v", err)
return NewInstanceServiceFromCloud(cloud, nil)
}

return NewInstanceServiceFromCloud(cloud, []byte(cacert))
if cacert, ok := cloudConfig.Data["ca-bundle.pem"]; ok {
return NewInstanceServiceFromCloud(cloud, []byte(cacert))
}

return NewInstanceServiceFromCloud(cloud, nil)
}

func NewInstanceService() (*InstanceService, error) {
Expand Down Expand Up @@ -234,6 +225,8 @@ func NewInstanceServiceFromCloud(cloud clientconfig.Cloud, cert []byte) (*Instan
},
}
provider.HTTPClient = client
} else {
klog.Infof("Cloud provider CA cert not provided, using system trust bundle")
}

err = openstack.Authenticate(provider, *opts)
Expand Down