OCPBUGS-43309: The "oc adm ocp-certificates regenerate-machine-config-server-serving-cert" command is failing by djoshy · Pull Request #1900 · openshift/oc

djoshy · 2024-10-17T09:35:05Z

This ensures that the machine-config-server-tls secret is of the kubernetes.io/tls type before attempting to rotate them.

This is only an issue in 4.18+, as the vendored cert controller package was updated during the 1.31.1 rebase. (#1877, diff)

In the current master of library-go, the cert controller only permits secrets of the type kubernetes.io/tls to be rotated, and therefore this "preflight check" is necessary before running the controller sync.

openshift-ci-robot · 2024-10-17T09:35:12Z

@djoshy: This pull request references Jira Issue OCPBUGS-43309, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.18.0) matches configured target version for branch (4.18.0)
bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @sergiordlr

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

Closes: OCPBUGS-43309

This ensures that the machine-config-server-tls secret is of the kubernetes.io/tls type before attempting to rotate them.

This is only an issue in 4.18+, as the vendored cert controller package was updated during the 1.31.1 rebase. (#1877, diff)

In the current master of library-go, the cert controller only permits secrets of the type kubernetes.io/tls to be rotated, and therefore this "preflight check" is necessary before running the controller sync.

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 openshift-eng/jira-lifecycle-plugin repository.

djoshy · 2024-10-23T07:34:53Z

/retest-required

ardaguclu

Dropped a few comments. I haven't checked the CI failures related or not
/retest

ardaguclu · 2024-10-29T06:14:50Z

+func (o *RegenerateMCOOptions) ensureMCSSecretType(c *kubernetes.Clientset, ctx context.Context) error {
+	// Retrieve the machine-config-server-tls secret
+	mcsTLSSecret, err := c.CoreV1().Secrets(mcoNamespace).Get(ctx, mcsTlsSecretName, metav1.GetOptions{})
+	if err != nil {


I assume that notfound error for this secret is not expected?

It is not expected(nodes cannot join the cluster if this secret doesn't exist in the MCO namespace), but good callout - I think it might be good if the command accounted for this case

ardaguclu · 2024-10-29T06:16:44Z

+			Data: mcsTLSSecret.Data,
+			Type: corev1.SecretTypeTLS,
+		}
+		if _, err := c.CoreV1().Secrets(mcoNamespace).Create(ctx, newSecret, metav1.CreateOptions{}); err != nil {


What is the consequence of deletion succeeds but creation fails. Because in that case we won't have this secret anymore?. Besides, we are not handling not found errors. Thus, this command will always start failing?.

Nothing catastrophic. This secret will be created or updated by the cert controller's sync which is manually called later in this command:

oc/pkg/cli/admin/ocpcertificates/regeneratemco/rotatecerts.go

Line 99 in ee93eb5

if err := cont.Sync(ctx, syncCtx); err != nil {

What seems to be problematic is, if the secret fed into the controller is not of the kubernetes.io/tls type, it causes the cert controller sync to fail. This wasn't the case before the 1.31 rebase, but it seems like they've made it more strict now.

I have added handling for the not found error, but if we are missing this secret, the cluster may have bigger problems. Also, just for some additional context, this command is used to manually rotate a cert that has a 10 year lifecycle. We are hoping to automate this rotation in the future, but our use rate for this command is quite low at the moment.

djoshy · 2024-10-29T17:35:54Z

/retest-required

ardaguclu · 2024-10-30T04:39:23Z

Thank you
/lgtm
/retest

openshift-ci · 2024-10-30T04:39:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ardaguclu, djoshy

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [ardaguclu]

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

djoshy · 2024-10-30T09:08:39Z

Holding for QE review

/hold

/retest-required

sergiordlr · 2024-10-30T13:01:33Z

Verified using IPI on AWS

$ oc -n openshift-machine-config-operator get secret  machine-config-server-tls
NAME                        TYPE     DATA   AGE
machine-config-server-tls   Opaque   2      44m

$ oc version
Client Version: 4.18.0-0.test-2024-10-30-093806-ci-ln-rmyszn2-latest
Kustomize Version: v5.4.2
Server Version: 4.18.0-0.test-2024-10-30-093806-ci-ln-rmyszn2-latest
Kubernetes Version: v1.31.2

$ oc  adm ocp-certificates regenerate-machine-config-server-serving-cert
Migration to kubernetes.io/tls for machine-config-server-tls required
Migration to kubernetes.io/tls for machine-config-server-tls successful
I1030 10:46:04.858487 4043545 recorder_logging.go:44] &Event{ObjectMeta:{dummy.18033530a38366a2  dummy    0 0001-01-01 00:00:00 +0000 UTC <nil> <nil> map[] map[] [] [] []},InvolvedObject:ObjectReference{Kind:Pod,Namespace:dummy,Name:dummy,UID:,APIVersion:v1,ResourceVersion:,FieldPath:,},Reason:TargetUpdateRequired,Message:"machine-config-server-tls" in "openshift-machine-config-operator" requires a new target cert/key pair: missing notAfter,Source:EventSource{Component:,Host:,},FirstTimestamp:2024-10-30 10:46:04.858402466 +0000 UTC m=+0.504114621,LastTimestamp:2024-10-30 10:46:04.858402466 +0000 UTC m=+0.504114621,Count:1,Type:Normal,EventTime:0001-01-01 00:00:00 +0000 UTC,Series:nil,Action:,Related:nil,ReportingController:,ReportingInstance:,}
I1030 10:46:04.971233 4043545 recorder_logging.go:44] &Event{ObjectMeta:{dummy.18033530aa3bf370  dummy    0 0001-01-01 00:00:00 +0000 UTC <nil> <nil> map[] map[] [] [] []},InvolvedObject:ObjectReference{Kind:Pod,Namespace:dummy,Name:dummy,UID:,APIVersion:v1,ResourceVersion:,FieldPath:,},Reason:SecretUpdated,Message:Updated Secret/machine-config-server-tls -n openshift-machine-config-operator because it changed,Source:EventSource{Component:,Host:,},FirstTimestamp:2024-10-30 10:46:04.971160432 +0000 UTC m=+0.616872588,LastTimestamp:2024-10-30 10:46:04.971160432 +0000 UTC m=+0.616872588,Count:1,Type:Normal,EventTime:0001-01-01 00:00:00 +0000 UTC,Series:nil,Action:,Related:nil,ReportingController:,ReportingInstance:,}
Successfully rotated MCS CA + certs. Redeploying MCS and updating references.
Successfully modified user-data secret master-user-data
Successfully modified user-data secret worker-user-data


$ oc get secret  machine-config-server-tls
NAME                        TYPE                DATA   AGE
machine-config-server-tls   kubernetes.io/tls   2      22s

$ oc scale machineset sregidor-vrot-2jfgc-worker-us-east-2c --replicas 2
machineset.machine.openshift.io/sregidor-vrot-2jfgc-worker-us-east-2c scaled

$ oc get nodes
NAME                                        STATUS   ROLES                  AGE    VERSION
ip-10-0-23-49.us-east-2.compute.internal    Ready    control-plane,master   57m    v1.31.2
ip-10-0-26-215.us-east-2.compute.internal   Ready    worker                 52m    v1.31.2
ip-10-0-52-91.us-east-2.compute.internal    Ready    worker                 50m    v1.31.2
ip-10-0-53-225.us-east-2.compute.internal   Ready    control-plane,master   57m    v1.31.2
ip-10-0-70-5.us-east-2.compute.internal     Ready    control-plane,master   57m    v1.31.2
ip-10-0-80-176.us-east-2.compute.internal   Ready    worker                 50m    v1.31.2
ip-10-0-90-115.us-east-2.compute.internal   Ready    worker                 106s   v1.31.2   <--- new node

/label cherry-pick-approved

openshift-ci · 2024-10-30T13:01:36Z

@sergiordlr: Can not set label cherry-pick-approved: Must be member in one of these teams: []

Details

In response to this:

Verified using IPI on AWS

$ oc -n openshift-machine-config-operator get secret  machine-config-server-tls
NAME                        TYPE     DATA   AGE
machine-config-server-tls   Opaque   2      44m

$ oc version
Client Version: 4.18.0-0.test-2024-10-30-093806-ci-ln-rmyszn2-latest
Kustomize Version: v5.4.2
Server Version: 4.18.0-0.test-2024-10-30-093806-ci-ln-rmyszn2-latest
Kubernetes Version: v1.31.2

$ oc  adm ocp-certificates regenerate-machine-config-server-serving-cert
Migration to kubernetes.io/tls for machine-config-server-tls required
Migration to kubernetes.io/tls for machine-config-server-tls successful
I1030 10:46:04.858487 4043545 recorder_logging.go:44] &Event{ObjectMeta:{dummy.18033530a38366a2  dummy    0 0001-01-01 00:00:00 +0000 UTC <nil> <nil> map[] map[] [] [] []},InvolvedObject:ObjectReference{Kind:Pod,Namespace:dummy,Name:dummy,UID:,APIVersion:v1,ResourceVersion:,FieldPath:,},Reason:TargetUpdateRequired,Message:"machine-config-server-tls" in "openshift-machine-config-operator" requires a new target cert/key pair: missing notAfter,Source:EventSource{Component:,Host:,},FirstTimestamp:2024-10-30 10:46:04.858402466 +0000 UTC m=+0.504114621,LastTimestamp:2024-10-30 10:46:04.858402466 +0000 UTC m=+0.504114621,Count:1,Type:Normal,EventTime:0001-01-01 00:00:00 +0000 UTC,Series:nil,Action:,Related:nil,ReportingController:,ReportingInstance:,}
I1030 10:46:04.971233 4043545 recorder_logging.go:44] &Event{ObjectMeta:{dummy.18033530aa3bf370  dummy    0 0001-01-01 00:00:00 +0000 UTC <nil> <nil> map[] map[] [] [] []},InvolvedObject:ObjectReference{Kind:Pod,Namespace:dummy,Name:dummy,UID:,APIVersion:v1,ResourceVersion:,FieldPath:,},Reason:SecretUpdated,Message:Updated Secret/machine-config-server-tls -n openshift-machine-config-operator because it changed,Source:EventSource{Component:,Host:,},FirstTimestamp:2024-10-30 10:46:04.971160432 +0000 UTC m=+0.616872588,LastTimestamp:2024-10-30 10:46:04.971160432 +0000 UTC m=+0.616872588,Count:1,Type:Normal,EventTime:0001-01-01 00:00:00 +0000 UTC,Series:nil,Action:,Related:nil,ReportingController:,ReportingInstance:,}
Successfully rotated MCS CA + certs. Redeploying MCS and updating references.
Successfully modified user-data secret master-user-data
Successfully modified user-data secret worker-user-data


$ oc get secret  machine-config-server-tls
NAME                        TYPE                DATA   AGE
machine-config-server-tls   kubernetes.io/tls   2      22s

$ oc scale machineset sregidor-vrot-2jfgc-worker-us-east-2c --replicas 2
machineset.machine.openshift.io/sregidor-vrot-2jfgc-worker-us-east-2c scaled

$ oc get nodes
NAME                                        STATUS   ROLES                  AGE    VERSION
ip-10-0-23-49.us-east-2.compute.internal    Ready    control-plane,master   57m    v1.31.2
ip-10-0-26-215.us-east-2.compute.internal   Ready    worker                 52m    v1.31.2
ip-10-0-52-91.us-east-2.compute.internal    Ready    worker                 50m    v1.31.2
ip-10-0-53-225.us-east-2.compute.internal   Ready    control-plane,master   57m    v1.31.2
ip-10-0-70-5.us-east-2.compute.internal     Ready    control-plane,master   57m    v1.31.2
ip-10-0-80-176.us-east-2.compute.internal   Ready    worker                 50m    v1.31.2
ip-10-0-90-115.us-east-2.compute.internal   Ready    worker                 106s   v1.31.2   <--- new node

/label cherry-pick-approved

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-sigs/prow repository.

djoshy · 2024-10-30T13:04:13Z

/unhold

openshift-ci-robot · 2024-10-30T16:20:05Z

/retest-required

Remaining retests: 0 against base HEAD 62471c2 and 2 for PR HEAD 833cc92 in total

openshift-ci · 2024-10-30T19:28:42Z

@djoshy: all tests passed!

Full PR test history. Your PR dashboard.

Details

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-sigs/prow repository. I understand the commands that are listed here.

openshift-ci-robot · 2024-10-30T19:33:28Z

@djoshy: Jira Issue OCPBUGS-43309: All pull requests linked via external trackers have merged:

openshift/oc#1900

Jira Issue OCPBUGS-43309 has been moved to the MODIFIED state.

Details

In response to this:

Closes: OCPBUGS-43309

This ensures that the machine-config-server-tls secret is of the kubernetes.io/tls type before attempting to rotate them.

This is only an issue in 4.18+, as the vendored cert controller package was updated during the 1.31.1 rebase. (#1877, diff)

In the current master of library-go, the cert controller only permits secrets of the type kubernetes.io/tls to be rotated, and therefore this "preflight check" is necessary before running the controller sync.

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 openshift-eng/jira-lifecycle-plugin repository.

openshift-bot · 2024-10-30T21:28:00Z

[ART PR BUILD NOTIFIER]

Distgit: openshift-enterprise-cli
This PR has been included in build openshift-enterprise-cli-container-v4.18.0-202410302041.p0.g8e9a2be.assembly.stream.el9.
All builds following this will include this PR.

openshift-bot · 2024-10-30T21:34:01Z

[ART PR BUILD NOTIFIER]

Distgit: ose-tools
This PR has been included in build ose-tools-container-v4.18.0-202410302041.p0.g8e9a2be.assembly.stream.el9.
All builds following this will include this PR.

openshift-bot · 2024-10-30T21:40:11Z

[ART PR BUILD NOTIFIER]

Distgit: openshift-enterprise-deployer
This PR has been included in build openshift-enterprise-deployer-container-v4.18.0-202410302041.p0.g8e9a2be.assembly.stream.el9.
All builds following this will include this PR.

openshift-bot · 2024-10-30T22:10:14Z

[ART PR BUILD NOTIFIER]

Distgit: ose-cli-artifacts
This PR has been included in build ose-cli-artifacts-container-v4.18.0-202410302041.p0.g8e9a2be.assembly.stream.el9.
All builds following this will include this PR.

openshift-ci Bot requested review from deads2k, ingvagabund and sergiordlr October 17, 2024 09:35

djoshy force-pushed the mcs-tls-fix branch from 9f95cc6 to d600087 Compare October 28, 2024 14:20

ardaguclu reviewed Oct 29, 2024

View reviewed changes

djoshy added 2 commits October 29, 2024 06:47

ensure mcs secret type before rotation

4e5ca2b

add MCO owners file

833cc92

djoshy force-pushed the mcs-tls-fix branch from baddad2 to 833cc92 Compare October 29, 2024 10:47

openshift-ci Bot assigned ardaguclu Oct 30, 2024

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Oct 30, 2024

openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 30, 2024

openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 30, 2024

openshift-ci Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 30, 2024

openshift-merge-bot Bot merged commit 8e9a2be into openshift:master Oct 30, 2024

djoshy deleted the mcs-tls-fix branch November 8, 2024 16:30

Conversation

djoshy commented Oct 17, 2024

Uh oh!

openshift-ci-robot commented Oct 17, 2024

Uh oh!

djoshy commented Oct 23, 2024

Uh oh!

ardaguclu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ardaguclu Oct 29, 2024

Choose a reason for hiding this comment

Uh oh!

djoshy Oct 29, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ardaguclu Oct 29, 2024

Choose a reason for hiding this comment

Uh oh!

djoshy Oct 29, 2024

Choose a reason for hiding this comment

Uh oh!

djoshy commented Oct 29, 2024

Uh oh!

ardaguclu commented Oct 30, 2024

Uh oh!

openshift-ci Bot commented Oct 30, 2024

Uh oh!

djoshy commented Oct 30, 2024

Uh oh!

sergiordlr commented Oct 30, 2024

Uh oh!

openshift-ci Bot commented Oct 30, 2024

Uh oh!

djoshy commented Oct 30, 2024

Uh oh!

openshift-ci-robot commented Oct 30, 2024

Uh oh!

openshift-ci Bot commented Oct 30, 2024

Uh oh!

openshift-ci-robot commented Oct 30, 2024

Uh oh!

openshift-bot commented Oct 30, 2024

Uh oh!

openshift-bot commented Oct 30, 2024

Uh oh!

openshift-bot commented Oct 30, 2024

Uh oh!

openshift-bot commented Oct 30, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants