Skip to content

Append router CA to cluster CA in kubeconfig #1242

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
merged 1 commit into from
Feb 15, 2019
Merged

Append router CA to cluster CA in kubeconfig #1242

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
56 changes: 56 additions & 0 deletions cmd/openshift-install/create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package main

import (
"context"
"crypto/x509"
"fmt"
"io/ioutil"
"path/filepath"
Expand All @@ -12,6 +13,7 @@ import (
"github.com/sirupsen/logrus"
"github.com/spf13/cobra"
corev1 "k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/fields"
"k8s.io/apimachinery/pkg/util/wait"
Expand Down Expand Up @@ -116,6 +118,10 @@ var (
logrus.Fatal(err)
}

if err = addRouterCAToClusterCA(config, rootOpts.dir); err != nil {
logrus.Fatal(err)
Copy link

Choose a reason for hiding this comment

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

Should this be fatal or only a warning? I thinking since it doesn't block accessing the cluster, but maybe it's ok to fail since this is the last step?

Copy link
Member

Choose a reason for hiding this comment

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

Should this be fatal or only a warning?

I'm fine failing fast. We can relax later if necessary, but unless this is fatal by default we don't get CI coverage (or will e2e start failing without this step?).

}

err = logComplete(rootOpts.dir, consoleURL)
if err != nil {
logrus.Fatal(err)
Expand Down Expand Up @@ -186,6 +192,56 @@ func runTargetCmd(targets ...asset.WritableAsset) func(cmd *cobra.Command, args
}
}

// addRouterCAToClusterCA adds router CA to cluster CA in kubeconfig
func addRouterCAToClusterCA(config *rest.Config, directory string) (err error) {
client, err := kubernetes.NewForConfig(config)
if err != nil {
return errors.Wrap(err, "creating a Kubernetes client")
}

// Configmap may not exist. log and accept not-found errors with configmap.
caConfigMap, err := client.CoreV1().ConfigMaps("openshift-config-managed").Get("router-ca", metav1.GetOptions{})
if err != nil {
if apierrors.IsNotFound(err) {
logrus.Infof("router-ca resource not found in cluster, perhaps you are not using default router CA")
return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really want to silently ignore a missing router-ca secret?

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 will check, but I think there's a possible expectation that it won't be created.

Copy link
Contributor

Choose a reason for hiding this comment

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

The secret will always be created. However, I'd suggest using the router-ca configmap in the openshift-config-managed namespace, which will be created when some clusteringress is using the operator-generated default certificate generated using that CA certificate, and will be absent 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.

thanks @Miciah will do!

Copy link

Choose a reason for hiding this comment

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

but also, at the very least log the error. preferably don't swallow it at all.

Copy link
Contributor Author

@sallyom sallyom Feb 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I've added a log msg for that error

Copy link
Member

Choose a reason for hiding this comment

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

Won't we want to poll/wait until the config-map has a non-empty CA instead of giving up?

}
return errors.Wrap(err, "fetching router-ca configmap from openshift-config-managed namespace")
}

routerCrtBytes := []byte(caConfigMap.Data["ca-bundle.crt"])
kubeconfig := filepath.Join(directory, "auth", "kubeconfig")
Copy link
Member

Choose a reason for hiding this comment

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

Ideally we'd hold this in memory, but this is fine short-term.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes I think there's a FIXME already elsewhere in this file for that

kconfig, err := clientcmd.LoadFromFile(kubeconfig)
if err != nil {
return errors.Wrap(err, "loading kubeconfig")
}

if kconfig == nil || len(kconfig.Clusters) == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

I think we want:

if kconfig == nil || len(kconfig.Clusters) != 1 {
  return errors.New("invalid kubeconfig")
}

which will also allow us to drop the following loop over Clusters. We will be able to drop this guard once we switch to keeping the kubeconfig in memory.

Copy link
Contributor Author

@sallyom sallyom Feb 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I'll open a follow-up? I could not find how to get this string:

// Clusters is a map of referencable names to cluster configs
 Clusters map[string]*Cluster

here

referencable names to cluster configs? What is that reference-able name? I'm happy to do some testing (and open a PR to clarify for others) to figure that out (or maybe someone knows and can tell me). But if there_were_ more than 1 cluster I'd want to add to each, so the != 0 seems ok here. If not I can also include that in the follow-up PR.

return errors.New("kubeconfig is missing expected data")
}

for _, c := range kconfig.Clusters {
clusterCABytes := c.CertificateAuthorityData
if len(clusterCABytes) == 0 {
return errors.New("kubeconfig CertificateAuthorityData not found")
}
certPool := x509.NewCertPool()
if !certPool.AppendCertsFromPEM(clusterCABytes) {
return errors.New("cluster CA found in kubeconfig not valid PEM format")
}
if !certPool.AppendCertsFromPEM(routerCrtBytes) {
return errors.New("ca-bundle.crt from router-ca configmap not valid PEM format")
}

newCA := append(routerCrtBytes, clusterCABytes...)
c.CertificateAuthorityData = newCA
}
if err := clientcmd.WriteToFile(*kconfig, kubeconfig); err != nil {
return errors.Wrap(err, "writing kubeconfig")
}
return nil
}

// FIXME: pulling the kubeconfig and metadata out of the root
// directory is a bit cludgy when we already have them in memory.
func destroyBootstrap(ctx context.Context, config *rest.Config, directory string) (err error) {
Expand Down