Add support for AdditionalTrustBundle #972

hardys · 2022-02-04T17:43:09Z

This adds initial support for a new HostedCluster API additionalTrustBundle which allows user provided CA certs to be provided, similar to the standalone installer

This is useful when you want to use a local container registry that uses a self-signed cert - this is a common scenario for developing on-prem scenarios, particularly disconnected and ipv6 where access to an upstream registry is not possible.

The expected usage is to reference a local object reference, e.g ConfigMap in the clusters namespace:

spec:
  additionalTrustBundle:
    name: user-ca-bundle

If the management cluster already contains such a ConfigMap, it may be copied from the openshift-config namespace, e.g:

oc patch cm user-ca-bundle -p '{"metadata":{ "namespace":"clusters"}}' -n openshift-config --dry-run=client -o yaml | oc apply -f -

Partially-Fixes https://issues.redhat.com/browse/HOSTEDCP-291

Checklist

Subject and description added to both, commit and PR.
Relevant issues have been referenced.
This change includes docs.
This change includes unit tests.

netlify · 2022-02-04T17:44:01Z

✔️ Deploy Preview for hypershift-docs ready!

🔨 Explore the source changes: cbd329c

🔍 Inspect the deploy log: https://app.netlify.com/sites/hypershift-docs/deploys/62349e17297c970009572250

😎 Browse the preview: https://deploy-preview-972--hypershift-docs.netlify.app/reference/api

hardys · 2022-02-04T18:00:10Z

/cc @csrwng @enxebre

This implements the changes described in HOSTEDCP-291 - I went with the local object reference option, since it seems more convenient (particularly in the case where you want to copy the ConfigMap from the management cluster) than inlining the cert contents.

Left as WIP because:

I've not yet tested the MCS part, or e2e with a local registry
I need to add some unit tests
It doesn't yet handle the delete path (when AdditionalTrustBundle is removed from HostedCluster/HostedControlPlane, although this seems a fairly unlikely corner-case)
I suspect we need to wire the user CA into some hosted controlplane pods

I'm also not sure if we need to replicate any of the CNO ConfigMapInjector logic - since we do see several CMs with that label in the guest cluster:

$ oc get cm --selector=config.openshift.io/inject-trusted-cabundle=true -A
NAMESPACE                                NAME                DATA   AGE
openshift-apiserver-operator             trusted-ca-bundle   0      5h39m
openshift-authentication-operator        trusted-ca-bundle   0      5h39m
openshift-cloud-credential-operator      cco-trusted-ca      0      5h39m
openshift-cluster-node-tuning-operator   trusted-ca          0      5h39m
openshift-image-registry                 trusted-ca          0      5h39m
openshift-ingress-operator               trusted-ca          0      5h39m
openshift-insights                       trusted-ca-bundle   0      5h39m
openshift-machine-api                    cbo-trusted-ca      0      5h40m
openshift-machine-api                    mao-trusted-ca      0      5h40m

Any thoughts on that, and review of the changes so far would be welcome, thanks!

sjenning · 2022-02-04T21:04:31Z

untracked files detected: M hack/app-sre/saas_template.yaml

API changes require make verify to update generated files. This isn't the best. I opened a PR for an update make target #976.

hardys · 2022-03-04T17:32:25Z

This is closer, the hypershift operator can now start consuming a release image from a local self-signed mirror, but I need to also wire in the trusted-ca-bundle to the CPO

hardys · 2022-03-14T17:51:44Z

This is now working - tested with this dev-scripts branch (None platform)

Hypershift was installed using a local release image, now that the user CA is wired into the pods that use the registryclient all pods start up OK

hypershift install --hypershift-image virthost.ostest.test.metalkube.org:5000/hypershift:latest --additional-trust-bundle user-ca-bundle

The cluster was then deployed like:

hypershift create cluster none --etcd-storage-class local-storage --ssh-key /home/shardy/.ssh/id_rsa.pub --name shtest --node-pool-replicas 2 --render --additional-trust-bundle user-ca-bundle --control-plane-operator-image virthost.ostest.test.metalkube.org:5000/hypershift:latest --release-image virthost.ostest.test.metalkube.org:5000/localimages/local-release-image:latest --pull-secret /home/shardy/dev-scripts/ocp/ostest/hypershift/pull_secret.json

Note --render was used to enable addition of the ICSP config as follows (in future adding a CLI option for this would be nice):

  imageContentSources:
  - mirrors:
    - virthost.ostest.test.metalkube.org:5000/localimages/local-release-image
    source: registry.ci.openshift.org/ocp/release
  - mirrors:
    - virthost.ostest.test.metalkube.org:5000/localimages/local-release-image
    source: registry.ci.openshift.org/ocp/4.11-2022-03-04-042425

This results in a working cluster, with the expected ICSP and two nodes were joined and pulled their images correctly from the mirror:

$ oc get imageContentSourcePolicy -o yaml
apiVersion: v1
items:
- apiVersion: operator.openshift.io/v1alpha1
  kind: ImageContentSourcePolicy
  metadata:
    creationTimestamp: "2022-03-14T16:26:07Z"
    generation: 1
    labels:
      hypershift.io/managed: "true"
      machineconfiguration.openshift.io/role: worker
    name: cluster
    resourceVersion: "1037"
    uid: c12eeccd-0d07-4686-bcfb-c0254d563632
  spec:
    repositoryDigestMirrors:
    - mirrors:
      - virthost.ostest.test.metalkube.org:5000/localimages/local-release-image
      source: registry.ci.openshift.org/ocp/release
    - mirrors:
      - virthost.ostest.test.metalkube.org:5000/localimages/local-release-image
      source: registry.ci.openshift.org/ocp/4.11-2022-03-04-042425
kind: List
metadata:
  resourceVersion: ""
  selfLink: ""

$ oc version
Client Version: 4.11.0-0.ci-2022-03-04-042425
Server Version: 4.11.0-0.ci-2022-03-04-042425
Kubernetes Version: v1.23.3-2001+7478cf2c86c567-dirty

$ oc get nodes
NAME                 STATUS   ROLES    AGE     VERSION
hypershiftworker-0   Ready    worker   9m18s   v1.23.3+7478cf2
hypershiftworker-1   Ready    worker   9m18s   v1.23.3+7478cf2

This still needs some tests/docs but otherwise ready for review :)

hardys · 2022-03-15T15:39:10Z

/retitle Add support for AdditionalTrustBundle

csrwng

@hardys thank you so much for working on this. It looks good to me. Just a couple of nits.

cmd/cluster/cluster.go

hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go

hardys · 2022-03-16T15:28:44Z

/hold the CI failures are real

{"level":"error","ts":"2022-03-15T16:54:22Z","logger":"controller.hostedcluster","msg":"Reconciler error","reconciler group":"hypershift.openshift.io","reconciler kind":"HostedCluster","name":"example-lswql","namespace":"e2e-clusters-jqm87","error":"failed to get hostedcluster AdditionalTrustBundle ConfigMap : ConfigMap \"\" not found","stacktrace":"sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem\n\t/hypershift/vendor/sigs.k8s.io

hardys · 2022-03-24T17:42:26Z

/hold cancel

hardys · 2022-03-24T17:43:01Z

@csrwng this is ready for another review pass when you get some time, thanks!

HostedCluster can optionally reference a configmap, in which case we copy the configmap to the HostedControlPlane namespace (similar to SSHKey and other fields).

When AdditionalTrustBundle is defined we create this ConfigMap to align with the behavior of regular OCP clusters and enable consumption of user-defined CA certs by the guest cluster.

When AdditionalTrustBundle is specified, we serialize the configmap and pass to the MCO bootstrap command via the default user-ca-bundle-config.yaml location - this means the MCO bootstrap will read the file when included, (the code already ignores the case where the file doesn't exist, since openshift/installer only conditionally creates the manifest)

This can be used to reference a ConfigMap that contains a user CA bundle.

The CPO and ignition server need the user CA so the registryclient can access a local registry with a self-signed cert

Adds a CLI option and corresponding volume to the operator pod, this is needed so the operator can look up release image metadata when the release image specified is locally mirrored. Note the mount path/filename were chosen to align with the expected defaults ref https://go.dev/src/crypto/x509/root_linux.go (and also current OCP docs for cert injection using operators)

csrwng · 2022-03-25T13:34:33Z

@hardys looks like the verify is complaining about:

cmd/install/assets/hypershift_operator.go:418:40: k8s.io/api/core/v1.LocalObjectReference composite literal uses unkeyed fields

Other than that lgtm

hardys · 2022-03-25T13:52:45Z

@hardys looks like the verify is complaining about:
cmd/install/assets/hypershift_operator.go:418:40: k8s.io/api/core/v1.LocalObjectReference composite literal uses unkeyed fields
Other than that lgtm

Thanks I thought I fixed that in my latest push (previously I missed the Name in the LocalObjectReference) but will try to figure out why it's still failing as it's working fine locally now

hardys · 2022-03-25T13:55:49Z

/retest

[36mINFO�[0m[2022-03-25T11:37:25Z] Resolved source https://github.com/openshift/hypershift to main@339bea98, merging: #972 5b3d5162 @hardys

Looks like it may be from before the push as the head of the branch is now 8fd5d43

csrwng · 2022-03-25T14:05:45Z

/lgtm

csrwng · 2022-03-25T14:05:56Z

/approve

csrwng · 2022-03-25T18:11:06Z

/retest-required

openshift-ci · 2022-03-25T19:50:09Z

@hardys: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-aws-nested	`8fd5d43`	link	false	`/test e2e-aws-nested`

Full PR test history. Your PR dashboard.

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.

hardys · 2022-03-28T12:38:20Z

/test capi-provider-agent-sanity

csrwng · 2022-03-28T14:29:43Z

/approve
/lgtm

openshift-ci · 2022-03-28T14:30:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: csrwng, hardys

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~~ [csrwng]

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

In openshift#972 I relied on the fact that MCSIgnitionProvider mounts the MCO ConfigMap under /assets/manifests - this means that the user-ca-bundle-config.yaml file path matches the default location from the MCO bootstrap code, thus no explicit --additional-trust-bundle-config-file argument was required. In openshift#1214 we introduced a new LocalIgnitionProvider, which unpacks the MCO config to a different path e.g /payloads/get-payload123/config hence we need to explicitly pass --additional-trust-bundle-config-file or the additionalTrustBundle data is ignored.

hardys requested a review from csrwng February 4, 2022 17:43

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 4, 2022

openshift-ci bot requested a review from enxebre February 4, 2022 18:00

hardys force-pushed the additional_trust_bundle branch from ef9395b to e591110 Compare February 7, 2022 16:48

hardys force-pushed the additional_trust_bundle branch from e591110 to 2c0cb60 Compare March 4, 2022 17:31

hardys force-pushed the additional_trust_bundle branch 2 times, most recently from 8db40d6 to 050eb1f Compare March 14, 2022 16:44

hardys force-pushed the additional_trust_bundle branch from 050eb1f to ed76598 Compare March 15, 2022 15:38

openshift-ci bot changed the title ~~[WIP] Add support for AdditionalTrustBundle~~ Add support for AdditionalTrustBundle Mar 15, 2022

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 15, 2022

csrwng reviewed Mar 15, 2022

View reviewed changes

cmd/cluster/cluster.go Outdated Show resolved Hide resolved

hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go Show resolved Hide resolved

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 16, 2022

hardys force-pushed the additional_trust_bundle branch 3 times, most recently from cbd329c to 0801be1 Compare March 18, 2022 15:28

hardys force-pushed the additional_trust_bundle branch from 0801be1 to 5b3d516 Compare March 24, 2022 17:39

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 24, 2022

Steven Hardy added 4 commits March 25, 2022 11:36

Add additionalTrustBundle to HostedCluster API

09dfcbb

Copy HostedCluster additionalTrustBundle to HostedControlPlane

40d4dbc

HostedCluster can optionally reference a configmap, in which case we copy the configmap to the HostedControlPlane namespace (similar to SSHKey and other fields).

Create HostedCluster user-ca-bundle configmap

12773d4

When AdditionalTrustBundle is defined we create this ConfigMap to align with the behavior of regular OCP clusters and enable consumption of user-defined CA certs by the guest cluster.

Steven Hardy added 4 commits March 25, 2022 11:36

Add create cli option for additional-trust-bundle

0dd4049

This can be used to reference a ConfigMap that contains a user CA bundle.

Add trust bundle volumes to hostedcluster_controller

cdfc64b

The CPO and ignition server need the user CA so the registryclient can access a local registry with a self-signed cert

Add trust bundle to hosted-cluster-config-operator

5626d38

hardys force-pushed the additional_trust_bundle branch from 5b3d516 to 8fd5d43 Compare March 25, 2022 11:36

openshift-ci bot assigned csrwng Mar 28, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 28, 2022

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 28, 2022

openshift-merge-robot merged commit b9418cb into openshift:main Mar 28, 2022

hardys mentioned this pull request Jul 20, 2022

Use non-default path for user CA certs #1593

Merged

hardys mentioned this pull request Jul 28, 2022

Fix additionalTrustBundle with local ignition provider #1626

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for AdditionalTrustBundle #972

Add support for AdditionalTrustBundle #972

hardys commented Feb 4, 2022 •

edited

netlify bot commented Feb 4, 2022 •

edited

hardys commented Feb 4, 2022 •

edited

sjenning commented Feb 4, 2022

hardys commented Mar 4, 2022

hardys commented Mar 14, 2022

hardys commented Mar 15, 2022

csrwng left a comment

hardys commented Mar 16, 2022

hardys commented Mar 24, 2022

hardys commented Mar 24, 2022

csrwng commented Mar 25, 2022

hardys commented Mar 25, 2022

hardys commented Mar 25, 2022

csrwng commented Mar 25, 2022

csrwng commented Mar 25, 2022

csrwng commented Mar 25, 2022

openshift-ci bot commented Mar 25, 2022 •

edited

hardys commented Mar 28, 2022

csrwng commented Mar 28, 2022

openshift-ci bot commented Mar 28, 2022

Add support for AdditionalTrustBundle #972

Add support for AdditionalTrustBundle #972

Conversation

hardys commented Feb 4, 2022 • edited

netlify bot commented Feb 4, 2022 • edited

hardys commented Feb 4, 2022 • edited

sjenning commented Feb 4, 2022

hardys commented Mar 4, 2022

hardys commented Mar 14, 2022

hardys commented Mar 15, 2022

csrwng left a comment

Choose a reason for hiding this comment

hardys commented Mar 16, 2022

hardys commented Mar 24, 2022

hardys commented Mar 24, 2022

csrwng commented Mar 25, 2022

hardys commented Mar 25, 2022

hardys commented Mar 25, 2022

csrwng commented Mar 25, 2022

csrwng commented Mar 25, 2022

csrwng commented Mar 25, 2022

openshift-ci bot commented Mar 25, 2022 • edited

hardys commented Mar 28, 2022

csrwng commented Mar 28, 2022

openshift-ci bot commented Mar 28, 2022

hardys commented Feb 4, 2022 •

edited

netlify bot commented Feb 4, 2022 •

edited

hardys commented Feb 4, 2022 •

edited

openshift-ci bot commented Mar 25, 2022 •

edited