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

update serverURL and CABundle if ManagedCluster is created before reg… #270

Merged
merged 4 commits into from
Sep 7, 2022
Merged

update serverURL and CABundle if ManagedCluster is created before reg… #270

merged 4 commits into from
Sep 7, 2022

Conversation

ivan-cai
Copy link
Contributor

@ivan-cai ivan-cai commented Sep 2, 2022

…istration-agent running

Sometimes, ManagedCluster is created before deploy klusterlet, we want registration-agent can also update serverURL and CABundle from Klusterlet CR while it is running.

cluster-gateway will use serverURL and CABundle in ManagedCluster if ClusterEndpointType is Const not ClusterProxy.

Signed-off-by: ivan-cai caijing.cai@alibaba-inc.com

@ivan-cai ivan-cai force-pushed the update_clientconfigs_for_cluster branch from 5ac4072 to c387d7d Compare September 2, 2022 11:13
@ivan-cai
Copy link
Contributor Author

ivan-cai commented Sep 2, 2022

@qiujian16 plz review.

qiujian16
qiujian16 reviewed Sep 2, 2022
View reviewed changes
pkg/spoke/managedcluster/creating_controller.go Outdated
if len(c.spokeExternalServerURLs) != 0 {
for _, serverURL := range c.spokeExternalServerURLs {
managedClusterClientConfigs = append(managedClusterClientConfigs, clusterv1.ClientConfig{
URL: serverURL,
Copy link
Member

Choose a reason for hiding this comment

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

we need to consider the case that user update this, we should not update this back from agent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we need to consider the case that user update this, we should not update this back from agent

@qiujian16 ok, I have change the condition for updating this in https://github.com/open-cluster-management-io/registration/pull/270/files#diff-0b36934623fbbd233f2529b6c4e12a550c7a7537f8d1223070b2c2748140cdb9R74
Plz review again, thx.

@ivan-cai
update serverURL and CABundle if ManagedCluster is created before reg…
d2bcaff 
…istration-agent running

Signed-off-by: ivan-cai <caijing.cai@alibaba-inc.com>
@ivan-cai ivan-cai force-pushed the update_clientconfigs_for_cluster branch from c387d7d to d2bcaff Compare September 5, 2022 02:33
qiujian16
qiujian16 reviewed Sep 6, 2022
View reviewed changes
pkg/spoke/managedcluster/creating_controller.go Outdated
clusterCopy := existingCluster.DeepCopy()
clusterCopy.Spec.ManagedClusterClientConfigs = managedClusterClientConfigs
if _, err := c.hubClusterClient.ClusterV1().ManagedClusters().Update(ctx, clusterCopy, metav1.UpdateOptions{}); err != nil {
return fmt.Errorf("unable to update ManagedClusterClientConfigs of managed cluster %q on hub: %w", c.clusterName, err)
Copy link
Member

Choose a reason for hiding this comment

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

we should also ignore Unauthorized error here

pkg/spoke/managedcluster/creating_controller.go Outdated
switch {
case errors.IsUnauthorized(err),
errors.IsForbidden(err) && strings.Contains(err.Error(), anonymous):
klog.V(4).Infof("unable to get the managed cluster %q from hub: %v", c.clusterName, err)
return nil
case errors.IsNotFound(err):
case err == nil:
if len(existingCluster.Spec.ManagedClusterClientConfigs) == 0 && len(managedClusterClientConfigs) > 0 {
Copy link
Member

@qiujian16 qiujian16 Sep 6, 2022 •
edited

Choose a reason for hiding this comment

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

do you think it is makes more sense if we check the ManagedClusterClientConfigs in clusters include the managedClusterClientConfigs, if not, merge it?

pkg/spoke/managedcluster/creating_controller.go Show resolved Hide resolved
@ivan-cai
merge spokeExternalServerURLs to ManagedClusterClientConfigs, and ing…
07a81f0 
…nore Unauthorized error

Signed-off-by: ivan-cai <caijing.cai@alibaba-inc.com>
@ivan-cai
Copy link
Contributor Author

ivan-cai commented Sep 6, 2022

@qiujian16 I have refactored the codes, and merged serverUrl. I have also ingored the Unauthorized error.
Plz review again, thx!

qiujian16
qiujian16 reviewed Sep 6, 2022
View reviewed changes
pkg/spoke/managedcluster/creating_controller.go Outdated
Name: c.clusterName,
},
// create ManagedCluster if not found
if err != nil && errors.IsNotFound(err) {
Copy link
Member

Choose a reason for hiding this comment

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

nit, err!=nil is not necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@qiujian16
err != nil is necessary, because I am not check err == nil before this.

Copy link
Member

Choose a reason for hiding this comment

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

i think

if errors.IsNotFound(err)

indicate err!=nil already

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think

if errors.IsNotFound(err)

indicate err!=nil already

done

pkg/spoke/managedcluster/creating_controller.go Outdated
case errors.IsUnauthorized(err),
errors.IsForbidden(err) && strings.Contains(err.Error(), anonymous):
existingCluster, err := c.hubClusterClient.ClusterV1().ManagedClusters().Get(ctx, c.clusterName, metav1.GetOptions{})
if err != nil && (errors.IsUnauthorized(err) || errors.IsForbidden(err) && strings.Contains(err.Error(), anonymous)) {
Copy link
Member

Choose a reason for hiding this comment

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

if errors.IsUnauthorized(err) || errors.IsForbidden(err)

do we really need to check anonymous?

Also pls add a comment here on why we return when unauthorized.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

Copy link
Member

Choose a reason for hiding this comment

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

also use skipUnauthorizedError 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.

also use skipUnauthorizedError here
@qiujian16 if use skipUnauthorizedError, here will be if err != nil && skipUnauthorizedError(err) == nil && strings.Contains(err.Error(), anonymous) , is it ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also use skipUnauthorizedError here

done @qiujian16

pkg/spoke/managedcluster/creating_controller.go Outdated
}

_, err = c.hubClusterClient.ClusterV1().ManagedClusters().Create(ctx, managedCluster, metav1.CreateOptions{})
if 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.

I think you would want to skip unauthorized here also?

It is better if we have a func skipUnauthorizedError(err error) error. So we can use it when run get/update/create

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@qiujian16 done!

@ivan-cai
add comment for skipping unauthorized error, and skip unauthorized er…
b84a212 
…ror when creating ManagedCluster

Signed-off-by: ivan-cai <caijing.cai@alibaba-inc.com>
@ivan-cai
use skipUnauthorizedError at creating controller
e877aa2 
Signed-off-by: ivan-cai <caijing.cai@alibaba-inc.com>
qiujian16
qiujian16 reviewed Sep 7, 2022
View reviewed changes
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 Sep 7, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 7, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ivan-cai, qiujian16

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-ci openshift-ci bot added the approved label Sep 7, 2022
@openshift-merge-robot openshift-merge-robot merged commit d6081f9 into open-cluster-management-io:main Sep 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qiujian16 qiujian16 qiujian16 left review comments

@deads2k deads2k Awaiting requested review from deads2k

@mdelder mdelder Awaiting requested review from mdelder

Assignees

@qiujian16 qiujian16

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ivan-cai @qiujian16 @openshift-merge-robot