Skip to content
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.

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

MCO-552: implement the ability for the MCO to handle image registry certificates #3770

Merged
merged 1 commit into from Aug 2, 2023

Conversation

cdoern
Copy link
Contributor

@cdoern cdoern commented Jun 29, 2023

- What I did

The MCO can now manage image registry certs

  1. the MCO reads from image.config.openshift.io/cluster and looks for additionalTrustedCA, which is a user specified additional registry

  2. the MCO reads from openshift-config-managed/image-registry-ca configmap which is to be created and managed by the image registry operator

  3. the MCO creates and writes to an additional openshift-config-managed/merged-trusted-image-registry-cas configmap with the combined data

  4. we write this all to disk in the daemon as we now do with other certificates. These paths follow: /etc/docker/certs.d/<CONFIGMAP_KEY>/ca.crt format

- How to verify it

oc describe controllerconfig to see the data from the CMs

ssh into the node and check out /etc/docker/certs.d to see if the files propagate.

- Description for the changelog

The MCO now supports managing image registry certificates and writes them to disk.

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 29, 2023
@cdoern
Copy link
Contributor Author

cdoern commented Jun 29, 2023

I have tested this locally, so I am not making this as a draft PR as it works well in its current state!

@cdoern cdoern changed the title implement the ability for the MCO to handle image registry certificates MCO-552: implement the ability for the MCO to handle image registry certificates Jun 29, 2023
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jun 29, 2023
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jun 29, 2023

@cdoern: This pull request references MCO-552 which is a valid jira issue.

In response to this:

- What I did

The MCO can now manage image registry certs

  1. the MCO reads from image.config.openshift.io/cluster and looks for additionalTrustedCA, which is a user specified additional registry

  2. the MCO reads from openshift-config-managed/image-registry-ca configmap which is to be created and managed by the image registry operator

  3. the MCO creates and writes to an additional openshift-config-managed/merged-trusted-image-registry-cas configmap with the combined data

  4. we write this all to disk in the daemon as we now do with other certificates. These paths follow: /etc/docker/certs.d/<CONFIGMAP_KEY>/ca.crt format

- How to verify it

oc describe controllerconfig to see the data from the CMs

ssh into the node and check out /etc/docker/certs.d to see if the files propagate.

- Description for the changelog

The MCO now supports managing image registry certificates and writes them to disk.

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.

@cdoern
Copy link
Contributor Author

cdoern commented Jun 29, 2023

/jira-refresh

Copy link
Contributor

@yuqi-zhang yuqi-zhang left a comment

Choose a reason for hiding this comment

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

Generally makes sense! some questions:

manifests/controllerconfig.crd.yaml Show resolved Hide resolved
pkg/daemon/certificate_writer.go Outdated Show resolved Hide resolved
pkg/operator/sync.go Outdated Show resolved Hide resolved
pkg/operator/sync.go Show resolved Hide resolved
pkg/operator/sync.go Show resolved Hide resolved
pkg/operator/sync.go Outdated Show resolved Hide resolved
pkg/operator/sync.go Outdated Show resolved Hide resolved
pkg/operator/sync.go Show resolved Hide resolved
pkg/operator/sync.go Outdated Show resolved Hide resolved
@cdoern
Copy link
Contributor Author

cdoern commented Jun 30, 2023

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 30, 2023
@cdoern cdoern force-pushed the CAservice branch 6 times, most recently from 040e7c3 to 9a16cf6 Compare July 3, 2023 13:18
Copy link
Contributor

@yuqi-zhang yuqi-zhang left a comment

Choose a reason for hiding this comment

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

Some additional comments/nits below, mostly makes sense functionality wise (pending conversation with IR team)

manifests/controllerconfig.crd.yaml Show resolved Hide resolved
pkg/operator/sync.go Show resolved Hide resolved
pkg/operator/sync.go Outdated Show resolved Hide resolved
pkg/operator/sync.go Show resolved Hide resolved
pkg/operator/sync.go Outdated Show resolved Hide resolved
@cdoern
Copy link
Contributor Author

cdoern commented Jul 14, 2023

/retest-required

1 similar comment
@qJkee
Copy link

qJkee commented Jul 17, 2023

/retest-required

@cdoern
Copy link
Contributor Author

cdoern commented Jul 17, 2023

/retest-required

1 similar comment
@cdoern
Copy link
Contributor Author

cdoern commented Jul 19, 2023

/retest-required

@cdoern
Copy link
Contributor Author

cdoern commented Jul 19, 2023

/retest-required

Signed-off-by: Charlie Doern <cdoern@redhat.com>
@cdoern
Copy link
Contributor Author

cdoern commented Jul 31, 2023

/retest-required

1 similar comment
@cdoern
Copy link
Contributor Author

cdoern commented Jul 31, 2023

/retest-required

@cdoern
Copy link
Contributor Author

cdoern commented Jul 31, 2023

/payload-job periodic-ci-openshift-hypershift-release-4.14-periodics-e2e-aws-ovn-conformance

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 31, 2023

@cdoern: trigger 1 job(s) for the /payload-(job|aggregate) command

  • periodic-ci-openshift-hypershift-release-4.14-periodics-e2e-aws-ovn-conformance

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/65544ba0-2fea-11ee-80f1-4de9f10a5544-0

@rioliu-rh
Copy link

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 1, 2023
@rioliu-rh
Copy link

rioliu-rh commented Aug 1, 2023

verification for bundle delete

  • create new user bundle
openssl x509 -in mco_test_ca.pem -noout -subject -issuer -dates
subject=C = US, ST = CA, O = Redhat, CN = MCO, emailAddress = mco-qe@redhat.com
issuer=C = US, ST = CA, O = Redhat, CN = MCO, emailAddress = mco-qe@redhat.com
notBefore=Aug  1 02:29:47 2023 GMT
notAfter=Jun  9 02:29:47 2033 GMT

oc create cm mco-crt-test -n openshift-config --from-file=mco_test_ca.pem
configmap/mco-crt-test created

oc patch image.config.openshift.io/cluster --type merge -p '{"spec": {"additionalTrustedCA": {"name": "mco-crt-test"}}}'
image.config.openshift.io/cluster patched

oc get controllerconfig -o jsonpath='{.items[*].status.controllerCertificates[?(@.bundleFile=="mco_test_ca.pem")]}' | jq
{
  "bundleFile": "mco_test_ca.pem",
  "notAfter": "2033-06-09 02:29:47 +0000 UTC",
  "notBefore": "2023-08-01 02:29:47 +0000 UTC",
  "signer": "CN=MCO,O=Redhat,ST=CA,C=US,1.2.840.113549.1.9.1=#0c116d636f2d7165407265646861742e636f6d",
  "subject": "CN=MCO,O=Redhat,ST=CA,C=US,1.2.840.113549.1.9.1=#0c116d636f2d7165407265646861742e636f6d"
}

debug node/ci-ln-mfzgvs2-72292-s5vrz-worker-a-5vcfz -- chroot /host stat /etc/docker/certs.d/mco_test_ca.pem/ca.crt
  File: /etc/docker/certs.d/mco_test_ca.pem/ca.crt
  Size: 2000      	Blocks: 8          IO Block: 4096   regular file
Device: 804h/2052d	Inode: 72169537    Links: 1
Access: (0644/-rw-r--r--)  Uid: (    0/    root)   Gid: (    0/    root)
Context: system_u:object_r:etc_t:s0
Access: 2023-08-01 02:44:36.473825173 +0000
Modify: 2023-08-01 02:44:30.297836938 +0000
Change: 2023-08-01 02:44:30.299836934 +0000
 Birth: 2023-08-01 02:44:30.297836938 +0000
  • delete this user bundle
oc delete cm/mco-crt-test -n openshift-config
configmap "mco-crt-test" deleted

oc patch image.config.openshift.io/cluster --type merge -p '{"spec": {"additionalTrustedCA": {"name": ""}}}'
image.config.openshift.io/cluster patched

oc get controllerconfig -o jsonpath='{.items[*].status.controllerCertificates[?(@.bundleFile=="mco_test_ca.pem")]}' | jq
>> empty

oc get image.config.openshift.io/cluster -o jsonpath='{.spec}'
{"additionalTrustedCA":{"name":""}}%
  • check bundle file exists on cluster node
for node in $(node -o name);do echo;echo $node;debug $node -- chroot /host stat /etc/docker/certs.d/mco_test_ca.pem;done

node/ci-ln-mfzgvs2-72292-s5vrz-master-0
stat: cannot statx '/etc/docker/certs.d/mco_test_ca.pem': No such file or directory
error: non-zero exit code from debug container

node/ci-ln-mfzgvs2-72292-s5vrz-master-1
stat: cannot statx '/etc/docker/certs.d/mco_test_ca.pem': No such file or directory
error: non-zero exit code from debug container

node/ci-ln-mfzgvs2-72292-s5vrz-master-2
stat: cannot statx '/etc/docker/certs.d/mco_test_ca.pem': No such file or directory
error: non-zero exit code from debug container

node/ci-ln-mfzgvs2-72292-s5vrz-worker-a-5vcfz
stat: cannot statx '/etc/docker/certs.d/mco_test_ca.pem': No such file or directory
error: non-zero exit code from debug container

node/ci-ln-mfzgvs2-72292-s5vrz-worker-b-zln7h
stat: cannot statx '/etc/docker/certs.d/mco_test_ca.pem': No such file or directory
error: non-zero exit code from debug container

node/ci-ln-mfzgvs2-72292-s5vrz-worker-c-dgxgd
stat: cannot statx '/etc/docker/certs.d/mco_test_ca.pem': No such file or directory
error: non-zero exit code from debug container

@cdoern please confirm thanks

@cdoern
Copy link
Contributor Author

cdoern commented Aug 1, 2023

@rioliu-rh looks good! Thank you

@rioliu-rh
Copy link

/unhold
/label qe-approved

@openshift-ci openshift-ci bot added qe-approved Signifies that QE has signed off on this PR and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Aug 1, 2023
@rioliu-rh
Copy link

/retest-required

@yuqi-zhang
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 1, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 1, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cdoern, yuqi-zhang

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 9c50df6 and 2 for PR HEAD 81136ed in total

@cdoern
Copy link
Contributor Author

cdoern commented Aug 1, 2023

/test e2e-hypershift

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 1, 2023

@cdoern: 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/okd-scos-e2e-aws-ovn 81136ed link false /test okd-scos-e2e-aws-ovn

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.

@yuqi-zhang
Copy link
Contributor

/override ci/prow/e2e-hypershift

Overriding the hypershift test since:

  1. it has passed on the current PR
  2. it has been failing consistently today
  3. this PR is important for the IR team as well for 4.14

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 1, 2023

@yuqi-zhang: Overrode contexts on behalf of yuqi-zhang: ci/prow/e2e-hypershift

In response to this:

/override ci/prow/e2e-hypershift

Overriding the hypershift test since:

  1. it has passed on the current PR
  2. it has been failing consistently today
  3. this PR is important for the IR team as well for 4.14

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.

@yuqi-zhang
Copy link
Contributor

/test e2e-aws-ovn-upgrade

@openshift-merge-robot openshift-merge-robot merged commit a341b03 into openshift:master Aug 2, 2023
12 of 13 checks passed
wking added a commit to wking/machine-config-operator that referenced this pull request Jan 2, 2024
These had snuck in back in 80e7b4d (keep track of certs in
ControllerConfigStatus, 2023-06-20, openshift#3756), 81136ed (implement the
ability for the MCO to handle image registry certificates, 2023-06-27, openshift#3770),
and similar.  Having the same package imported under multiple names
doesn't have any functional impact, but it's less confusing to read if
the package is refered to with a consistent prefix.  This commit
addresses all the duplicates turned up with:

  $ git grep -c '"github.com/openshift/api/machineconfiguration/v1"'  | grep '[.]go:' | grep -v 'vendor/\|:1$'
wking added a commit to wking/machine-config-operator that referenced this pull request Jan 2, 2024
These had snuck in back in 80e7b4d (keep track of certs in
ControllerConfigStatus, 2023-06-20, openshift#3756), 81136ed (implement the
ability for the MCO to handle image registry certificates, 2023-06-27, openshift#3770),
and similar.  Having the same package imported under multiple names
doesn't have any functional impact, but it's less confusing to read if
the package is refered to with a consistent prefix.  This commit
addresses all the duplicates turned up with:

  $ git grep -c '"github.com/openshift/api/machineconfiguration/v1"'  | grep '[.]go:' | grep -v 'vendor/\|:1$'
wking added a commit to wking/machine-config-operator that referenced this pull request Jan 2, 2024
These had snuck in back in 80e7b4d (keep track of certs in
ControllerConfigStatus, 2023-06-20, openshift#3756), 81136ed (implement the
ability for the MCO to handle image registry certificates, 2023-06-27, openshift#3770),
and similar.  Having the same package imported under multiple names
doesn't have any functional impact, but it's less confusing to read if
the package is refered to with a consistent prefix.  This commit
addresses all the duplicates turned up with:

  $ git grep -c '"github.com/openshift/api/machineconfiguration/v1"'  | grep '[.]go:' | grep -v 'vendor/\|:1$'
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. qe-approved Signifies that QE has signed off on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet