cmd/openshift-install/create: Drop addRouterCAToClusterCA #1541
Conversation
This was added in 4033577 (Append router CA to cluster CA in kubeconfig, 2019-02-12, openshift#1242), where the motivation was [1]: With users created with an identity-provider, OAuth flow goes through router and router-ca is not trusted. This prohibits the user from using oc login from command line, without manually appending the router ca to the cluster ca. The openshift-ingress-operator creates the router-CA and this is not available until the very end of an install. This PR adds the router CA from configmap router-ca -n openshift-config-managed to the kubeconfig certificate-authority-data. This allows an identity-provided user to oc login from the terminal. But the admin kubeconfig is already authenticated, so folks shouldn't be using 'oc login' with that kubeconfig. For example, see dee6929 (Modify kubeadmin usage message, admins should not use kubeadmin via CLI, 2019-04-01, openshift#1513). That leaves us with no use case for this modification, and making the finish code watch-only sets us up to rename away from the user-provided-infrastructure subcommand. [1]: openshift#1242 (comment)
openshift-ci-robot
added
the
size/M
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wking 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:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
|
If this passes CI. i'll :lgtm: :) |
So we'll need to tweak the test suite to drop that check (and add another replacement check?). I'll take a look at the test suite (originally from openshift/origin#21733) and see if we can save it. |
|
Talking this over with @enj and @ericavonb, there is concern that users will continue to attempt: $ export KUBECONFIG=auth/kubeconfig
$ oc login ...despite there being no point (you're already an admin) and us removing the docs around that in #1513. Ideally, somebody (the admin, an ACME request from the cluster, etc.) would be provisioning a router CA that was trusted by the cluster's expected user base (because that helps with web-console access, other users running |
The real reason for this is to get around some crazy http2 errors: https://bugzilla.redhat.com/show_bug.cgi?id=1686476#c16 |
|
I find the motivation to add the router CA to the kubeconfig dubious. It's an admin only kubeconfig that's already got a certificate granting system:admin rights. There's no need to My suggestion would be that we clean up the |
|
After discussing this with @enj he's clarified that the routerCA needs to be added to the kubeconfig only for purposes of making @derekwaynecarr can I get your thoughts on what we should be doing here as we try to clean up these commands so that they're purely waiting for a condition? Do we drop adding the routerCA to the admin kubeconfig or do we sacrifice the purity of the command and just silently add it? If we continue adding it silently I think we should make sure that it's also added to assets/tls/ca.crt as well as this is a file that I would reasonably expect an admin to provide to end users needing to log into the cluster. |
|
This is primarily a matter of UX. Users understand usernames and password. Users understand the concept of a |
There is no hard line between installer- and user-provided infrastructure. Rename these commands to focus on what they'll do instead of the work-flow into which we expect them to fit. We're still working out how we can drop the router-CA injection to avoid 'wait-for cluster-ready' surprising users my editing their on-disk kubeconfig [1]. But that's mitigated somewhat by the fact that addRouterCAToClusterCA is idempotent, because AppendCertsFromPEM wraps AddCert [2] and AddCert checks to avoid duplicate certificates [3]. [1]: openshift#1541 [2]: https://github.com/golang/go/blob/go1.12/src/crypto/x509/cert_pool.go#L144 [3]: https://github.com/golang/go/blob/go1.12/src/crypto/x509/cert_pool.go#L106-L109
And also to
|
There is no hard line between installer- and user-provided infrastructure. Rename these commands to focus on what they'll do instead of the work-flow into which we expect them to fit. We're still working out how we can drop the router-CA injection to avoid 'wait-for cluster-ready' surprising users my editing their on-disk kubeconfig [1]. But that's mitigated somewhat by the fact that addRouterCAToClusterCA is idempotent, because AppendCertsFromPEM wraps AddCert [2] and AddCert checks to avoid duplicate certificates [3]. [1]: openshift#1541 [2]: https://github.com/golang/go/blob/go1.12/src/crypto/x509/cert_pool.go#L144 [3]: https://github.com/golang/go/blob/go1.12/src/crypto/x509/cert_pool.go#L106-L109
|
/hold So we're now ok waiting on more easily configurable router CAs and having |
openshift-ci-robot
added
the
do-not-merge/hold
|
@wking: PR needs rebase. 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
added
the
needs-rebase
|
@wking: The following tests failed, say
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. |
|
/close closing without prejudice due to age. Please consider opening an issue or enhancement to discuss and bring consensus on the fix, or if you think this is still important in current state feel free to reopen. |
|
@abhinavdahiya: Closed this PR. In response to this:
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. |
The console may become optional [1], so teach the installer to handle its absence gracefully. We've waited on the console since way back in ff53523 (add logs at end of install for kubeadmin, consoleURL, 2018-12-06, openshift#806). Back then, install-complete timing was much less organized, and since e17ba3c (cmd: wait for the cluster to be initialized, 2019-01-25, 1. Console route admission blocks console operator from going Available=True in its ClusterOperator. 2. Console ClusterOperator blocks cluster-version operator from going Available=True in ClusterVersion. 3. ClusterVersion blocks installer's waitForInitializedCluster. So we no longer need to wait for the route to show up, and can fail fast if we get a clear IsNotFound. I'm keeping a bit of polling so we don't fail an install on a temporary network hiccup. We don't want to drop the console check entirely, because when it is found, we want: * To continue to log that access pathway on install-complete. * To continue to append the router CA to the kubeconfig. That latter point has been done since 4033577 (append router CA to cluster CA in kubeconfig, 2019-02-12, openshift#1242). The motication in that commit message is not explicit, but the idea is to support folks who naively run 'oc login' with the kubeadmin kubeconfig [2] (despite that kubeconfig already having cluster-root access) when the console route's cert's CA happens to be something that the user's local trust store doesn't include by default. [1]: openshift/enhancements#922 [2]: openshift#1541 (comment)
The console may become optional [1], so teach the installer to handle its absence gracefully. We've waited on the console since way back in ff53523 (add logs at end of install for kubeadmin, consoleURL, 2018-12-06, openshift#806). Back then, install-complete timing was much less organized, and since e17ba3c (cmd: wait for the cluster to be initialized, 2019-01-25, openshift#1132) we've blocked on ClusterVersion going Available=True. So the current dependency chain is: 1. Console route admission blocks console operator from going Available=True in its ClusterOperator. 2. Console ClusterOperator blocks cluster-version operator from going Available=True in ClusterVersion. 3. ClusterVersion blocks installer's waitForInitializedCluster. So we no longer need to wait for the route to show up, and can fail fast if we get a clear IsNotFound. I'm keeping a bit of polling so we don't fail an install on a temporary network hiccup. We don't want to drop the console check entirely, because when it is found, we want: * To continue to log that access pathway on install-complete. * To continue to append the router CA to the kubeconfig. That latter point has been done since 4033577 (append router CA to cluster CA in kubeconfig, 2019-02-12, openshift#1242). The motication in that commit message is not explicit, but the idea is to support folks who naively run 'oc login' with the kubeadmin kubeconfig [2] (despite that kubeconfig already having cluster-root access) when the console route's cert's CA happens to be something that the user's local trust store doesn't include by default. [1]: openshift/enhancements#922 [2]: openshift#1541 (comment)
The console may become optional [1], so teach the installer to handle its absence gracefully. We've waited on the console since way back in ff53523 (add logs at end of install for kubeadmin, consoleURL, 2018-12-06, openshift#806). Back then, install-complete timing was much less organized, and since e17ba3c (cmd: wait for the cluster to be initialized, 2019-01-25, openshift#1132) we've blocked on ClusterVersion going Available=True. So the current dependency chain is: 1. Console route admission blocks console operator from going Available=True in its ClusterOperator. 2. Console ClusterOperator blocks cluster-version operator from going Available=True in ClusterVersion. 3. ClusterVersion blocks installer's waitForInitializedCluster. So we no longer need to wait for the route to show up, and can fail fast if we get a clear IsNotFound. I'm keeping a bit of polling so we don't fail an install on a temporary network hiccup. We don't want to drop the console check entirely, because when it is found, we want: * To continue to log that access pathway on install-complete. * To continue to append the router CA to the kubeconfig. That latter point has been done since 4033577 (append router CA to cluster CA in kubeconfig, 2019-02-12, openshift#1242). The motivation in that commit message is not explicit, but the idea is to support folks who naively run 'oc login' with the kubeadmin kubeconfig [2] (despite that kubeconfig already having cluster-root access) when the console route's cert's CA happens to be something that the user's local trust store doesn't include by default. [1]: openshift/enhancements#922 [2]: openshift#1541 (comment)
The console may become optional [1], so teach the installer to handle its absence gracefully. We've waited on the console since way back in ff53523 (add logs at end of install for kubeadmin, consoleURL, 2018-12-06, openshift#806). Back then, install-complete timing was much less organized, and since e17ba3c (cmd: wait for the cluster to be initialized, 2019-01-25, openshift#1132) we've blocked on ClusterVersion going Available=True. So the current dependency chain is: 1. Console route admission blocks console operator from going Available=True in its ClusterOperator. 2. Console ClusterOperator blocks cluster-version operator from going Available=True in ClusterVersion. 3. ClusterVersion blocks installer's waitForInitializedCluster. So we no longer need to wait for the route to show up, and can fail fast if we get a clear IsNotFound. I'm keeping a bit of polling so we don't fail an install on a temporary network hiccup. We don't want to drop the console check entirely, because when it is found, we want: * To continue to log that access pathway on install-complete. * To continue to append the router CA to the kubeconfig. That latter point has been done since 4033577 (append router CA to cluster CA in kubeconfig, 2019-02-12, openshift#1242). The motivation in that commit message is not explicit, but the idea is to support folks who naively run 'oc login' with the kubeadmin kubeconfig [2] (despite that kubeconfig already having cluster-root access) when the console route's cert's CA happens to be something that the user's local trust store doesn't include by default. [1]: openshift/enhancements#922 [2]: openshift#1541 (comment)
This was added in 4033577 (#1242), where the motivation was:
But the admin kubeconfig is already authenticated, so folks shouldn't be using
oc loginwith that kubeconfig (more in dee6929, #1513). That leaves us with no use case for this modification, and making the finish code watch-only sets us up to rename away from the user-provided-infrastructure subcommand.CC @abhinavdahiya, @sallyom