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 kubeconfig secret ref in hypershift deployment status #33

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

Conversation

zhujian7
Copy link
Contributor

@zhujian7 zhujian7 commented Mar 7, 2022

Signed-off-by: zhujian [email protected]

Now we copied the hosted cluster kubeconfig to the hub cluster in a form of fmt.Sprintf("%s-admin-kubeconfig", hydName), but if the hypershiftDeployment name is too long, I think creating the secret will fail, And if there are two hypershift deployment with the same name but different spec.TargetName, the secret will conflict.

Learn from the way hypershift handles hostedcluster, we can record the secret ref into the hypershift deployment status.

@openshift-ci
Copy link

openshift-ci bot commented Mar 7, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zhujian7

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 Mar 7, 2022
@sonarcloud
Copy link

sonarcloud bot commented Mar 7, 2022

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
0.0% 0.0% Duplication

@zhujian7
Copy link
Contributor Author

zhujian7 commented Mar 7, 2022

@jnpacker @ianzhang366

@jnpacker
Copy link
Member

jnpacker commented Mar 9, 2022

Is the plan to copy this back to the same namespace as the HypershiftDeployment? In which case there shouldn't be a name collision, or is it copied to the managedCluster Namespace for the ManagementCluster?

@zhujian7
Copy link
Contributor Author

Is the plan to copy this back to the same namespace as the HypershiftDeployment? In which case there shouldn't be a name collision, or is it copied to the managedCluster Namespace for the ManagementCluster?

@jnpacker It was copied to the managedCluster Namespace for the ManagementCluster since the addon-agent only has permission to create secret in the managementCluster namespace on the hub. If we want to copy this to the HypershiftDeployment namespace, I think we need to grant the addon-agent permission to create/delete secret in all namespaces on the hub, which is not really safe.

@jnpacker
Copy link
Member

Are you looking for a review yet?

@zhujian7
Copy link
Contributor Author

Are you looking for a review yet?

This PR is not in a hurry to merge, just wanted to discuss how to deal with these two secrets would be better.

@jnpacker
Copy link
Member

jnpacker commented Mar 22, 2022

I like the reference idea then.
fmt.Sprintf("%s-admin-kubeconfig", hyd.infraID) Maybe put a 63 character check and drop the suffix (though I don't love that), and maybe a label identifier.

@sonarcloud
Copy link

sonarcloud bot commented Jun 7, 2022

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
0.0% 0.0% Duplication

@jnpacker
Copy link
Member

jnpacker commented Jun 8, 2022

Can we close this out, as its from Mar 7?

@zhujian7
Copy link
Contributor Author

zhujian7 commented Jun 9, 2022

Can we close this out, as its from Mar 7?

@philipwu08 For https://issues.redhat.com/browse/CMCS-102, do we still need this, if it is not necessary, I will close this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants