OCPBUGS-33671, OCPBUGS-34079: fix secret canonicalization #4366

cheesesashimi · 2024-05-14T21:13:52Z

- What I did

If multiple MachineOSConfigs are created simultaneously (e.g., from multiple
YAML documents in the same file) and each of these MachineOSConfigs references
a legacy pull secret (that is, a pull secret that does not have a top-level
{"auths": {}} key), only one of the MachineOSBuilds that gets created will
actually get a status and begin building.

This is because multiple Goroutines will attempt to create a canonicalized
secret and only one will be able to. In the event that this occurs, the best
thing to do is to just fetch the secret and continue.

This PR also includes fixes for updating the canonicalized pull secrets. Basically, if a legacy-style pull secret is supplied and then changed, it would not be kept up-to-date. Instead, we now set a label which contains the name of the original pull secret and read the original pull secret to determine if an update is required.

- How to verify it

Bring up a 4.16+ cluster.
Enable the tech preview no upgrade featuregate.
Apply the following MachineConfigs from the same file:

---
apiVersion: machineconfiguration.openshift.io/v1
kind: MachineConfigPool
metadata:
  name: pool-1
spec:
  machineConfigSelector:
    matchExpressions:
      - key: machineconfiguration.openshift.io/role
        operator: In
        values:
        - worker
        - pool-1
  nodeSelector:
    matchLabels:
      node-role.kubernetes.io/pool-1: ""
---
apiVersion: machineconfiguration.openshift.io/v1
kind: MachineConfigPool
metadata:
  name: pool-2
spec:
  machineConfigSelector:
    matchExpressions:
      - key: machineconfiguration.openshift.io/role
        operator: In
        values:
        - worker
        - pool-2
  nodeSelector:
    matchLabels:
      node-role.kubernetes.io/pool-2: ""

Wait for them to get a rendered MachineConfig.
Apply the following MachineOSConfigs from the same file:

---
apiVersion: machineconfiguration.openshift.io/v1alpha1
kind: MachineOSConfig
metadata:
  name: pool-1
spec:
  buildInputs:
    baseImagePullSecret:
      name: global-pull-secret-copy
    containerFile:
    - containerfileArch: noarch
      content: |
        FROM configs AS final
        RUN rpm-ostree install cowsay && \
            ostree container commit
    imageBuilder:
      imageBuilderType: PodImageBuilder
    renderedImagePushSecret:
      name: builder-dockercfg-hknx6-canonical
    renderedImagePushspec: image-registry.openshift-image-registry.svc:5000/openshift-machine-config-operator/pool-1:latest
  buildOutputs:
    currentImagePullSecret:
      name: global-pull-secret-copy
  machineConfigPool:
    name: pool-1
---
apiVersion: machineconfiguration.openshift.io/v1alpha1
kind: MachineOSConfig
metadata:
  name: pool-2
spec:
  buildInputs:
    baseImagePullSecret:
      name: global-pull-secret-copy
    containerFile:
    - containerfileArch: noarch
      content: |
        FROM configs AS final
        RUN rpm-ostree install cowsay && \
            ostree container commit
    imageBuilder:
      imageBuilderType: PodImageBuilder
    renderedImagePushSecret:
      name: builder-dockercfg-hknx6-canonical
    renderedImagePushspec: image-registry.openshift-image-registry.svc:5000/openshift-machine-config-operator/pool-2:latest
  buildOutputs:
    currentImagePullSecret:
      name: global-pull-secret-copy
  machineConfigPool:
    name: pool-2

New MachineOSBuilds should be created for each MachineOSConfig and they should begin building.

- Description for the changelog
Fix secret canonicalization in on-cluster layering

openshift-ci-robot · 2024-05-14T21:13:57Z

@cheesesashimi: This pull request references Jira Issue OCPBUGS-33671, which is invalid:

expected the bug to target the "4.16.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

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

In response to this:

- What I did

If multiple MachineOSConfigs are created simultaneously (e.g., from multiple
YAML documents in the same file) and each of these MachineOSConfigs references
a legacy pull secret (that is, a pull secret that does not have a top-level
{"auths": {}} key), only one of the MachineOSBuilds that gets created will
actually get a status and begin building.

This is because multiple Goroutines will attempt to create a canonicalized
secret and only one will be able to. In the event that this occurs, the best
thing to do is to just fetch the secret and continue.

- How to verify it

Bring up a 4.16+ cluster.
Enable the tech preview no upgrade featuregate.
Apply the following MachineConfigs from the same file:

---
apiVersion: machineconfiguration.openshift.io/v1
kind: MachineConfigPool
metadata:
 name: pool-1
spec:
 machineConfigSelector:
   matchExpressions:
     - key: machineconfiguration.openshift.io/role
       operator: In
       values:
       - worker
       - pool-1
 nodeSelector:
   matchLabels:
     node-role.kubernetes.io/pool-1: ""
---
apiVersion: machineconfiguration.openshift.io/v1
kind: MachineConfigPool
metadata:
 name: pool-2
spec:
 machineConfigSelector:
   matchExpressions:
     - key: machineconfiguration.openshift.io/role
       operator: In
       values:
       - worker
       - pool-2
 nodeSelector:
   matchLabels:
     node-role.kubernetes.io/pool-2: ""

Wait for them to get a rendered MachineConfig.
Apply the following MachineOSConfigs from the same file:

---
apiVersion: machineconfiguration.openshift.io/v1alpha1
kind: MachineOSConfig
metadata:
 name: pool-1
spec:
 buildInputs:
   baseImagePullSecret:
     name: global-pull-secret-copy
   containerFile:
   - containerfileArch: noarch
     content: |
       FROM configs AS final
       RUN rpm-ostree install cowsay && \
           ostree container commit
   imageBuilder:
     imageBuilderType: PodImageBuilder
   renderedImagePushSecret:
     name: builder-dockercfg-hknx6-canonical
   renderedImagePushspec: image-registry.openshift-image-registry.svc:5000/openshift-machine-config-operator/pool-1:latest
 buildOutputs:
   currentImagePullSecret:
     name: global-pull-secret-copy
 machineConfigPool:
   name: pool-1
---
apiVersion: machineconfiguration.openshift.io/v1alpha1
kind: MachineOSConfig
metadata:
 name: pool-2
spec:
 buildInputs:
   baseImagePullSecret:
     name: global-pull-secret-copy
   containerFile:
   - containerfileArch: noarch
     content: |
       FROM configs AS final
       RUN rpm-ostree install cowsay && \
           ostree container commit
   imageBuilder:
     imageBuilderType: PodImageBuilder
   renderedImagePushSecret:
     name: builder-dockercfg-hknx6-canonical
   renderedImagePushspec: image-registry.openshift-image-registry.svc:5000/openshift-machine-config-operator/pool-2:latest
 buildOutputs:
   currentImagePullSecret:
     name: global-pull-secret-copy
 machineConfigPool:
   name: pool-2

New MachineOSBuilds should be created for each MachineOSConfig and they should begin building.

- Description for the changelog
Fix secret canonicalization in on-cluster layering

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-05-21T17:11:39Z

Interesting corner case 🤔 Makes sense to me
/lgtm
/approve

/hold for QE review if needed

djoshy · 2024-05-21T17:27:11Z

/jira refresh

openshift-ci-robot · 2024-05-21T17:27:18Z

@djoshy: This pull request references Jira Issue OCPBUGS-33671, 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.17.0) matches configured target version for branch (4.17.0)
bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @sergiordlr

In response to this:

/jira refresh

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-ci-robot · 2024-05-21T20:49:38Z

@cheesesashimi: This pull request references Jira Issue OCPBUGS-34079, which is invalid:

expected the bug to target the "4.17.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

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

In response to this:

- What I did

If multiple MachineOSConfigs are created simultaneously (e.g., from multiple
YAML documents in the same file) and each of these MachineOSConfigs references
a legacy pull secret (that is, a pull secret that does not have a top-level
{"auths": {}} key), only one of the MachineOSBuilds that gets created will
actually get a status and begin building.

This is because multiple Goroutines will attempt to create a canonicalized
secret and only one will be able to. In the event that this occurs, the best
thing to do is to just fetch the secret and continue.

- How to verify it

Bring up a 4.16+ cluster.
Enable the tech preview no upgrade featuregate.
Apply the following MachineConfigs from the same file:

---
apiVersion: machineconfiguration.openshift.io/v1
kind: MachineConfigPool
metadata:
 name: pool-1
spec:
 machineConfigSelector:
   matchExpressions:
     - key: machineconfiguration.openshift.io/role
       operator: In
       values:
       - worker
       - pool-1
 nodeSelector:
   matchLabels:
     node-role.kubernetes.io/pool-1: ""
---
apiVersion: machineconfiguration.openshift.io/v1
kind: MachineConfigPool
metadata:
 name: pool-2
spec:
 machineConfigSelector:
   matchExpressions:
     - key: machineconfiguration.openshift.io/role
       operator: In
       values:
       - worker
       - pool-2
 nodeSelector:
   matchLabels:
     node-role.kubernetes.io/pool-2: ""

Wait for them to get a rendered MachineConfig.
Apply the following MachineOSConfigs from the same file:

---
apiVersion: machineconfiguration.openshift.io/v1alpha1
kind: MachineOSConfig
metadata:
 name: pool-1
spec:
 buildInputs:
   baseImagePullSecret:
     name: global-pull-secret-copy
   containerFile:
   - containerfileArch: noarch
     content: |
       FROM configs AS final
       RUN rpm-ostree install cowsay && \
           ostree container commit
   imageBuilder:
     imageBuilderType: PodImageBuilder
   renderedImagePushSecret:
     name: builder-dockercfg-hknx6-canonical
   renderedImagePushspec: image-registry.openshift-image-registry.svc:5000/openshift-machine-config-operator/pool-1:latest
 buildOutputs:
   currentImagePullSecret:
     name: global-pull-secret-copy
 machineConfigPool:
   name: pool-1
---
apiVersion: machineconfiguration.openshift.io/v1alpha1
kind: MachineOSConfig
metadata:
 name: pool-2
spec:
 buildInputs:
   baseImagePullSecret:
     name: global-pull-secret-copy
   containerFile:
   - containerfileArch: noarch
     content: |
       FROM configs AS final
       RUN rpm-ostree install cowsay && \
           ostree container commit
   imageBuilder:
     imageBuilderType: PodImageBuilder
   renderedImagePushSecret:
     name: builder-dockercfg-hknx6-canonical
   renderedImagePushspec: image-registry.openshift-image-registry.svc:5000/openshift-machine-config-operator/pool-2:latest
 buildOutputs:
   currentImagePullSecret:
     name: global-pull-secret-copy
 machineConfigPool:
   name: pool-2

New MachineOSBuilds should be created for each MachineOSConfig and they should begin building.

- Description for the changelog
Fix secret canonicalization in on-cluster layering

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.

cheesesashimi · 2024-05-22T19:19:59Z

/test e2e-gcp-op-techpreview
/test e2e-gcp-op-single-node

These test failures look transient in nature, retrying.

cheesesashimi · 2024-05-22T19:42:50Z

/test e2e-hypershift

openshift-ci · 2024-05-22T21:50:05Z

@cheesesashimi: 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-images	`e7d82d5`	link	false	`/test okd-images`

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

cheesesashimi · 2024-05-30T15:47:21Z

/test e2e-gcp-op-techpreview

djoshy

Overall makes sense to me! Couple of non-blocking questions

djoshy · 2024-05-31T18:18:56Z

pkg/controller/build/build_controller.go

+
+// Creates a canonicalized secret.
+func (ctrl *Controller) createCanonicalizedSecret(secret *corev1.Secret) (*corev1.Secret, error) {
+	if err := validateCanonicalizedSecret(secret); err != nil {


This call seems redundant as the calling function would have run validateCanonicalizedSecret()

It may be redundant, though I added it purely as a guard clause because there is no way to distinguish a non-canonicalized corev1.Secret and a canonicalized one. In the future, introducing another type that represents canonicalized secrets might be a better way forward.

fair enough, that makes sense to me!

djoshy · 2024-05-31T18:19:54Z

pkg/controller/build/helpers_test.go

@@ -160,6 +160,8 @@ func TestCanonicalizePullSecret(t *testing.T) {

 			if testCase.expectCanonical {
 				assert.Contains(t, out.Name, "canonical")
+				assert.True(t, isCanonicalizedSecret(out))
+				assert.True(t, hasCanonicalizedSecretLabels(out))


Similar to earlier question, wouldn't isCanonicalizedSecret(out) check for labels too?

It would. Although in this case, since its a test, the added verbosity is nice as it explicitly validates this particular case.

djoshy · 2024-05-31T18:23:04Z

pkg/controller/build/build_controller_test.go

+		// secret.
+		assert.Contains(t, s.Name, canonicalSecretSuffix)
+		assert.True(t, isCanonicalizedSecret(s))
+		assert.True(t, hasCanonicalizedSecretLabels(s))


same question as https://github.com/openshift/machine-config-operator/pull/4366/files#r1622799274

djoshy · 2024-06-03T14:00:02Z

Happy with Zack's answers, this sounds good to me. Thanks for working on this!

/lgtm
/approve

Holding for QE review:
/hold

openshift-ci · 2024-06-03T14:02:02Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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~~ [cheesesashimi,djoshy]

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

djoshy · 2024-06-03T14:04:26Z

/jira refresh

openshift-ci-robot · 2024-06-03T14:04:30Z

@djoshy: This pull request references Jira Issue OCPBUGS-34079, which is invalid:

expected the bug to target the "4.17.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

/jira refresh

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-06-03T14:05:24Z

/jira refresh

openshift-ci-robot · 2024-06-03T14:05:32Z

@djoshy: This pull request references Jira Issue OCPBUGS-34079, 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.17.0) matches configured target version for branch (4.17.0)
bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @sergiordlr

In response to this:

/jira refresh

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-ci-robot added jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels May 14, 2024

openshift-ci bot requested review from djoshy and dkhater-redhat May 14, 2024 21:14

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 14, 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 May 21, 2024

openshift-ci bot assigned djoshy May 21, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 21, 2024

openshift-ci-robot added jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. and removed jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels May 21, 2024

openshift-ci bot requested a review from sergiordlr May 21, 2024 17:27

cheesesashimi force-pushed the zzlotnik/fix-ocp-33671 branch from 36eddc7 to 67429c5 Compare May 21, 2024 20:47

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label May 21, 2024

cheesesashimi changed the title ~~OCPBUGS-33671: fix secret canonicalization~~ OCPBUGS-33671, OCPBUGS-34079: fix secret canonicalization May 21, 2024

openshift-ci-robot added jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. and removed jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. labels May 21, 2024

fixes canonicalized secret creation and updates

e7d82d5

cheesesashimi force-pushed the zzlotnik/fix-ocp-33671 branch from 67429c5 to e7d82d5 Compare May 22, 2024 15:10

djoshy reviewed May 31, 2024

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 3, 2024

openshift-ci-robot added jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. and removed jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Jun 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCPBUGS-33671, OCPBUGS-34079: fix secret canonicalization #4366

OCPBUGS-33671, OCPBUGS-34079: fix secret canonicalization #4366

cheesesashimi commented May 14, 2024 •

edited

openshift-ci-robot commented May 14, 2024

djoshy commented May 21, 2024

djoshy commented May 21, 2024

openshift-ci-robot commented May 21, 2024

openshift-ci-robot commented May 21, 2024

cheesesashimi commented May 22, 2024

cheesesashimi commented May 22, 2024

openshift-ci bot commented May 22, 2024 •

edited

cheesesashimi commented May 30, 2024

djoshy left a comment

djoshy May 31, 2024

cheesesashimi Jun 3, 2024

djoshy Jun 3, 2024

djoshy May 31, 2024

cheesesashimi Jun 3, 2024

djoshy May 31, 2024

djoshy commented Jun 3, 2024

openshift-ci bot commented Jun 3, 2024

djoshy commented Jun 3, 2024

openshift-ci-robot commented Jun 3, 2024

djoshy commented Jun 3, 2024

openshift-ci-robot commented Jun 3, 2024

OCPBUGS-33671, OCPBUGS-34079: fix secret canonicalization #4366

Are you sure you want to change the base?

OCPBUGS-33671, OCPBUGS-34079: fix secret canonicalization #4366

Conversation

cheesesashimi commented May 14, 2024 • edited

openshift-ci-robot commented May 14, 2024

djoshy commented May 21, 2024

djoshy commented May 21, 2024

openshift-ci-robot commented May 21, 2024

openshift-ci-robot commented May 21, 2024

cheesesashimi commented May 22, 2024

cheesesashimi commented May 22, 2024

openshift-ci bot commented May 22, 2024 • edited

cheesesashimi commented May 30, 2024

djoshy left a comment

Choose a reason for hiding this comment

djoshy May 31, 2024

Choose a reason for hiding this comment

cheesesashimi Jun 3, 2024

Choose a reason for hiding this comment

djoshy Jun 3, 2024

Choose a reason for hiding this comment

djoshy May 31, 2024

Choose a reason for hiding this comment

cheesesashimi Jun 3, 2024

Choose a reason for hiding this comment

djoshy May 31, 2024

Choose a reason for hiding this comment

djoshy commented Jun 3, 2024

openshift-ci bot commented Jun 3, 2024

djoshy commented Jun 3, 2024

openshift-ci-robot commented Jun 3, 2024

djoshy commented Jun 3, 2024

openshift-ci-robot commented Jun 3, 2024

cheesesashimi commented May 14, 2024 •

edited

openshift-ci bot commented May 22, 2024 •

edited