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

Customizing secret event sources #88

Merged
merged 1 commit into from Oct 14, 2021
Merged

Customizing secret event sources #88

merged 1 commit into from Oct 14, 2021

Conversation

skeeey
Copy link
Contributor

@skeeey skeeey commented Sep 29, 2021

Signed-off-by: liuwei liuweixa@redhat.com

@skeeey
Copy link
Contributor Author

skeeey commented Sep 29, 2021

@skeeey
Copy link
Contributor Author

skeeey commented Oct 13, 2021

/assign @qiujian16

@skeeey
Copy link
Contributor Author

skeeey commented Oct 13, 2021

/assign @zhiweiyin318

@skeeey
Copy link
Contributor Author

skeeey commented Oct 13, 2021

/assign @zhujian7

@skeeey
Copy link
Contributor Author

skeeey commented Oct 13, 2021

Namespace: managedClusterName,
Name: constants.AutoImportSecretName,
}, autoImportSecret)
autoImportSecret, err := r.kubeClient.CoreV1().Secrets(managedClusterName).Get(ctx, constants.AutoImportSecretName, metav1.GetOptions{})
Copy link
Contributor

Choose a reason for hiding this comment

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

should we use lister here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, we should consider it, I will add a TODO firstly

}

// NewAutoImportSecretSource return a SecretSource only for auto import secrets
func NewAutoImportSecretSource(secretInformer cache.SharedIndexInformer) *SecretSource {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems duplicate with NewImportSecretSource

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just use different name to distinguish different informer

@zhujian7
Copy link
Contributor

Can we hold this PR temporarily?
I find it seems there is a simpler way to implement this. I will try it and discuss it with @skeeey later.

Signed-off-by: liuwei <liuweixa@redhat.com>
@sonarcloud
Copy link

sonarcloud bot commented Oct 13, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@skeeey skeeey changed the title WIP: Customizing secret and manifestwork event sources Customizing secret and manifestwork event sources Oct 14, 2021
@zhiweiyin318
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Oct 14, 2021
@openshift-ci
Copy link

openshift-ci bot commented Oct 14, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: skeeey, 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:
  • OWNERS [skeeey,zhiweiyin318]

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

@openshift-merge-robot openshift-merge-robot merged commit 2fb6f7e into stolostron:main Oct 14, 2021
@zhujian7
Copy link
Contributor

zhujian7 commented Oct 14, 2021

Can we hold this PR temporarily? I find it seems there is a simpler way to implement this. I will try it and discuss it with @skeeey later.

Yesterday I found the SelectorsByObject option to cache feature of the controller-runtime, I think we can set the label/field selector for Secret by overriding the NewCache function while creating controller runtime manager.

...
	// Create controller-runtime manager
	mgr, err := ctrl.NewManager(cfg, manager.Options{
		Scheme:             scheme,
		MetricsBindAddress: fmt.Sprintf(":%d", metricsPort),
		LeaderElection:     true,
		LeaderElectionID:   "managedcluster-import-controller.open-cluster-management.io",
		NewCache:           newCache,
	})
...
// newCache initializes and returns a new Cache.
func newCache(config *rest.Config, opts cache.Options) (cache.Cache, error) {
	labelSelector := labels.NewSelector()
	requirement, err := labels.NewRequirement(constants.ClusterImportSecretLabel, selection.Exists, nil)
	if err != nil {
		return nil, err
	}
	labelSelector = labelSelector.Add(*requirement)

	fieldSelector := fields.OneTermEqualSelector("metadata.name", constants.AutoImportSecretName)

	opts.SelectorsByObject = cache.SelectorsByObject{
		&corev1.Secret{}: {
			// Label and Field selector should be OR relationship instead of AND.
			Label: labelSelector,
			Field: fieldSelector,
		},
	}
	return cache.New(config, opts)
}

But we need the auto import secret(field selector, result A) and the import secret(label selector, result B), union of A and B. But SelectorsByObject feature gets the intersection of A and B.

It is close to code freeze I think we can merge this PR now. And in the future, we can

  • add a specific label to the auto import secret and make all secrets we are interested in could be selected by labels, then we can use SelectorsByObject to filter the Secret cache.

OR

  • push the upstream controller-runtime to support union label selector and field selector of SelectorsByObject option to cache

/hold cancel

@skeeey skeeey changed the title Customizing secret and manifestwork event sources Customizing secret event sources Oct 14, 2021
@skeeey skeeey deleted the performance branch October 29, 2021 04:45
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.

None yet

5 participants