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

✨ Add image pullSecret to hub controllers #397

Conversation

zhiweiyin318
Copy link
Member

Summary

Support to sync the imagePullSecret open-cluster-management-image-pull-credentials in the operator ns to the hub namespace.

Related issue(s)

Fixes #

@openshift-ci openshift-ci bot requested review from xuezhaojun and zhujian7 April 8, 2024 01:36
@zhiweiyin318 zhiweiyin318 force-pushed the add-imagepullsecret branch 4 times, most recently from c35ab59 to 520c29c Compare April 8, 2024 06:55
@zhiweiyin318
Copy link
Member Author

/assign @qiujian16

@qiujian16
Copy link
Member

qiujian16 commented Apr 8, 2024

we need to consider this together with #381, we basically want to sync secrets from one ns to another. I think we should handle them at the same time. cc @morvencao

@morvencao
Copy link
Member

are you referring to #381 for syncing work-driver-config secret?

Copy link

codecov bot commented Apr 15, 2024

Codecov Report

Attention: Patch coverage is 65.21739% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 62.51%. Comparing base (3f304e8) to head (61ae0d4).

Files Patch % Lines
...stermanagercontroller/clustermanager_controller.go 60.00% 2 Missing and 2 partials ⚠️
...nagercontroller/clustermanager_secret_reconcile.go 60.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #397      +/-   ##
==========================================
- Coverage   62.51%   62.51%   -0.01%     
==========================================
  Files         134      134              
  Lines       11430    11438       +8     
==========================================
+ Hits         7146     7151       +5     
- Misses       3521     3523       +2     
- Partials      763      764       +1     
Flag Coverage Δ
unit 62.51% <65.21%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zhiweiyin318 zhiweiyin318 force-pushed the add-imagepullsecret branch 2 times, most recently from 185e4bc to 2d3b895 Compare April 25, 2024 12:19
@@ -379,3 +383,15 @@ func cleanResources(ctx context.Context, kubeClient kubernetes.Interface, cm *op
}
return cm, reconcileContinue, nil
}

func (n *clusterManagerController) getImagePullSecret(ctx context.Context) (string, error) {
Copy link
Member

Choose a reason for hiding this comment

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

do you still need this?

@@ -259,6 +259,10 @@ func (n *clusterManagerController) sync(ctx context.Context, controllerContext f
config.RegistrationAPIServiceCABundle = encodedCaBundle
config.WorkAPIServiceCABundle = encodedCaBundle

if config.ImagePullSecret, err = n.getImagePullSecret(ctx); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

why we need this step?

Copy link
Member Author

@zhiweiyin318 zhiweiyin318 Apr 25, 2024

Choose a reason for hiding this comment

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

will not add the pullsecretName to the sa if the imagepullSecret does not exist.
https://github.com/open-cluster-management-io/ocm/pull/397/files#diff-9f7acaff5c7d5c32d15173a975e8bed87d52c8add8e4bf8b4783854741c4cf39R6

if the imagepullSecret is not created but the name is fixed in the sa, a warning event will be output in the ns. The warning event can cause test case failure in openshift-conformance test suite.
There are 2 options here to avoid the warning event:

  1. create an empty pullsecret in the operator namespace.
  2. remove the pullsecret name from the sa if the pullsecret does not exist.

I chose option 2 in this PR.

the ApplyDirectly in libary-go does not update the imagepullsecret filed in the sa, I also create a new func ApplyServiceAccount to apply sa in the PR.

Copy link
Member Author

@zhiweiyin318 zhiweiyin318 Apr 28, 2024

Choose a reason for hiding this comment

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

moved the imagepullsecret to deployment and added a comment here

@zhiweiyin318 zhiweiyin318 force-pushed the add-imagepullsecret branch 2 times, most recently from 8319256 to 1237c43 Compare April 28, 2024 03:01
Signed-off-by: Zhiwei Yin <zyin@redhat.com>
@zhiweiyin318
Copy link
Member Author

@qiujian16 @morvencao please take another look.

Copy link
Member

@qiujian16 qiujian16 left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm

@openshift-ci openshift-ci bot added the lgtm label Apr 28, 2024
Copy link
Contributor

openshift-ci bot commented Apr 28, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qiujian16, zhiweiyin318

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-merge-bot openshift-merge-bot bot merged commit 2636009 into open-cluster-management-io:main Apr 28, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants