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

✨ Registration-agent supports multiple bootstrapkubeconfigs. #443

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

xuezhaojun
Copy link
Member

@xuezhaojun xuezhaojun commented Apr 25, 2024

Summary

Klusterlet supports configure multiple bootstrapkubeconfigs and switch when hub doesn't accpet it or failed to connect to a hub.

Special Notes:

  • The feature is depends on the auto-approve enabled and configured.

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.

can we split this to two PRs. the first one only update the registration part.

@xuezhaojun xuezhaojun force-pushed the feature-switch-hub branch 2 times, most recently from 7115d11 to 51b4c3b Compare April 25, 2024 10:02
@xuezhaojun
Copy link
Member Author

can we split this to two PRs. the first one only update the registration part.

Yes, I have split the PR and now it only contains registration part.

@xuezhaojun xuezhaojun force-pushed the feature-switch-hub branch 3 times, most recently from 649f468 to 55d1b74 Compare April 25, 2024 10:16
@xuezhaojun xuezhaojun changed the title ✨ Klusterlet support multiple bootstrapkubeconfigs. ✨ Klusterlet support multiple bootstrapkubeconfigs. [WIP] Apr 25, 2024
Copy link

codecov bot commented Apr 25, 2024

Codecov Report

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

Project coverage is 62.09%. Comparing base (bcbe4d2) to head (983b8da).
Report is 49 commits behind head on main.

Files Patch % Lines
...pkubeconfigsmanager/bootstrapkubeconfigsmanager.go 38.53% 54 Missing and 13 partials ⚠️
pkg/registration/spoke/spokeagent.go 0.00% 37 Missing ⚠️
...istration/bootstrapkubeconfig_secret_controller.go 0.00% 18 Missing ⚠️
...ation/spoke/registration/hub_timeout_controller.go 40.74% 13 Missing and 3 partials ⚠️
...ration/spoke/registration/hub_accept_controller.go 26.31% 12 Missing and 2 partials ⚠️
...bootstrapkubeconfigsmanager/bootstrapkubeconfig.go 69.69% 5 Missing and 5 partials ⚠️
...trapkubeconfigsmanager/bootstrapkubeconfiginuse.go 62.50% 3 Missing and 3 partials ⚠️
pkg/registration/spoke/options.go 20.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #443      +/-   ##
==========================================
+ Coverage   61.54%   62.09%   +0.54%     
==========================================
  Files         133      142       +9     
  Lines       14078    11823    -2255     
==========================================
- Hits         8665     7342    -1323     
+ Misses       4664     3687     -977     
- Partials      749      794      +45     
Flag Coverage Δ
unit 62.09% <34.84%> (+0.54%) ⬆️

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.

@xuezhaojun xuezhaojun changed the title ✨ Klusterlet support multiple bootstrapkubeconfigs. [WIP] ✨ Klusterlet support multiple bootstrapkubeconfigs. Apr 28, 2024
@xuezhaojun xuezhaojun force-pushed the feature-switch-hub branch 2 times, most recently from eeeee2a to 73f58c6 Compare April 28, 2024 15:20
@xuezhaojun xuezhaojun force-pushed the feature-switch-hub branch 3 times, most recently from 2baf7de to fa7196e Compare April 29, 2024 05:59
@@ -411,6 +424,16 @@ func (o *SpokeAgentConfig) RunSpokeAgentWithSpokeInformers(ctx context.Context,
)
}

hubAcceptController := registration.NewHubAcceptController(
o.agentOptions.SpokeClusterName,
Copy link
Member

Choose a reason for hiding this comment

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

the agent will be unauthorized when accept is false. So this controller does not always work.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the feature depends on the auto-approve enabled and configured, I will note this point in the doc in the future.

pkg/registration/spoke/lease/lease_controller.go Outdated Show resolved Hide resolved
)

// bootstrapKubeConfigInUse is the registration spoke's current in used bootstrap kubeconfig.
type bootstrapKubeConfigInUse interface {
Copy link
Member

Choose a reason for hiding this comment

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

why interface, we always use the bootstrap kubeconfig from file system, isn't it?

Copy link
Member Author

Choose a reason for hiding this comment

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

ack.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, the current bootstrap kubeconfig used to trigger rebootstrap is based on the secret, and if in the future we want to change to another type such as file, then the interface is a good practice to prevent that change impact other parts of code.


// ReSelect is the only public method of a bootstrapkubeconfigManager
func (m *bootstrapkubeconfigsManager) ReSelect(ctx context.Context) error {
if m.reselected {
Copy link
Member

Choose a reason for hiding this comment

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

when reslect will be set false again?

Copy link
Member Author

Choose a reason for hiding this comment

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

Never in the same lifecycle, the pod will restart and in the next lifecycle of container, the reselect will be set as false initially.

Comment on lines 39 to 50
if cluster.Spec.HubAcceptsClient {
return nil
}
Copy link
Member Author

Choose a reason for hiding this comment

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

If hubClientAccpet set to false, is there a controller to remove the clusterrole and prevent agent to get ManagedCluster?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Comment added: the hubAccpetController can only handle the case that "true" -> "false".

Copy link
Member Author

Choose a reason for hiding this comment

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

PS: from this line of code we can see that an event is send to all watchers at the same time. So if the informer on the hub side and informer on the agent side will both get the update event.


type bootstrapKubeConfigInUseImpl struct {
secretName string
secretNamespace string
Copy link
Member Author

Choose a reason for hiding this comment

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

remove secretName and use from constant pkg.

Copy link
Member Author

Choose a reason for hiding this comment

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

@qiujian16

From the code we know, the spoke compoennt namespace is not always the defaultSpokeComponentNamespace, it depends on which ns the agent is installed, which means it's neccessary to keep the secretNamespace.

// NewAgentOptions returns the flags with default value set
func NewAgentOptions() *AgentOptions {
	opts := &AgentOptions{
		HubKubeconfigDir:   "/spoke/hub-kubeconfig",
		ComponentNamespace: defaultSpokeComponentNamespace,
		CommoOpts:          NewOptions(),
	}
	// get component namespace of spoke agent
	nsBytes, err := os.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/namespace")
	if err == nil {
		opts.ComponentNamespace = string(nsBytes)
	}
	return opts
}

Copy link
Contributor

openshift-ci bot commented May 4, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: xuezhaojun
Once this PR has been reviewed and has the lgtm label, please assign skeeey for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@xuezhaojun xuezhaojun force-pushed the feature-switch-hub branch 4 times, most recently from 9719311 to 8ccb0b3 Compare May 12, 2024 12:39
@xuezhaojun xuezhaojun requested a review from qiujian16 May 12, 2024 12:46
@xuezhaojun xuezhaojun changed the title ✨ Klusterlet support multiple bootstrapkubeconfigs. ✨ Klusterlet agent support multiple bootstrapkubeconfigs. May 12, 2024
@xuezhaojun xuezhaojun changed the title ✨ Klusterlet agent support multiple bootstrapkubeconfigs. ✨ Registration-agent supports multiple bootstrapkubeconfigs. May 12, 2024
…hub.

Signed-off-by: xuezhaojun <zxue@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants