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.

Sign up for GitHub

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

Jump to bottom

Fix catsrc pod hash logic #3102

Merged
merged 3 commits into from Nov 9, 2023
Merged

Fix catsrc pod hash logic #3102

merged 3 commits into from Nov 9, 2023

Conversation

awgreene
Copy link
Member

@awgreene awgreene commented Nov 9, 2023

No description provided.

@awgreene
Revert "Fix Image Update Test Cleanup"
db0210e 
This reverts commit 2abfb3c.

Signed-off-by: Alexander Greene <greene.al1991@gmail.com>
@awgreene
Revert "Fix Image Update test"
f85ed4e 
This reverts commit f7b970e.

Signed-off-by: Alexander Greene <greene.al1991@gmail.com>
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 9, 2023
@awgreene
Copy link
Member Author

awgreene commented Nov 9, 2023 •
edited

/hold

I'll remove this when I'm ready to merge.

@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 Nov 9, 2023
@awgreene awgreene force-pushed the fix-catsrc-pod-hash-logic branch 2 times, most recently from 1529781 to df14862 Compare November 9, 2023 15:00
@stevekuznetsov
Copy link
Member

Er sorry - TestPodExtractContent is a specific test of Pod() for extracting content. I think you want to add a test case to a general test for Pod() or add a new TestPodServiceAccountImagePullSecrets()

@awgreene
Copy link
Member Author

awgreene commented Nov 9, 2023

Er sorry - TestPodExtractContent is a specific test of Pod() for extracting content. I think you want to add a test case to a general test for Pod() or add a new TestPodServiceAccountImagePullSecrets()

@stevekuznetsov I can move it to its own test suite, but functionally the existing implementation guarantees that the pod is constructed with the proper imagePullSecrets defined by the serviceAccount, is the reason for the change request just to avoid muddying the purpose of a test?

@stevekuznetsov
Copy link
Member

Yes, that's all.

stevekuznetsov
stevekuznetsov previously approved these changes Nov 9, 2023
View reviewed changes
@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Nov 9, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awgreene

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

@awgreene
Copy link
Member Author

awgreene commented Nov 9, 2023

/unhold

Good to go!

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 9, 2023
@awgreene
Update CatalogSource Pod Hash Logic
8a52c14 
Problem: Commit 95405d81e4c87c8113ccd7a95ba4d088b200a42ai updated the
catalog operator's logic so it does not delete the pod associated with a
catalogSource while it is in a Pending state. Unfortunately, there is a
race condition in which the pod may be admitted to the cluster without
the imagePullSecrets specified for it's serviceAccount by the admission
controller, preventing the pod from pulling its image from registries
that require authentication and causing the pod to never reach a
successful state.

Solution: Update the catalog operator to detect when a pod is missing
the imagePullSecrets granted to its serviceAccount.

Signed-off-by: Alexander Greene <greene.al1991@gmail.com>
awgreene
awgreene commented Nov 9, 2023
View reviewed changes
pkg/controller/registry/reconciler/reconciler.go
@@ -192,6 +201,9 @@ func Pod(source *operatorsv1alpha1.CatalogSource, name, opmImg, utilImage, img,
"kubernetes.io/os": "linux",
},
ServiceAccountName: saName,
// If this field is not set, there that the is a chance that pod will be created without the imagePullSecret
// defined by the serviceAccount
ImagePullSecrets: saImagePullSecrets,
Copy link
Member Author

Choose a reason for hiding this comment

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

This line change is more or less the purpose for the entire PR.

@stevekuznetsov stevekuznetsov added this pull request to the merge queue Nov 9, 2023
Merged via the queue into operator-framework:master with commit 0e1e089 Nov 9, 2023
15 of 16 checks passed
@awgreene awgreene mentioned this pull request Nov 10, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stevekuznetsov stevekuznetsov stevekuznetsov left review comments

@gallettilance gallettilance Awaiting requested review from gallettilance

@grokspawn grokspawn Awaiting requested review from grokspawn

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@awgreene @stevekuznetsov