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

Expose eks cluster certificate authority data in status #966

Merged

Conversation

hanlins
Copy link
Contributor

@hanlins hanlins commented Nov 19, 2021

Description of your changes

Fixes #955

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

Added unit test for the new field introduced.

Signed-off-by: Hanlin Shi <shihanlin9@gmail.com>
Signed-off-by: Hanlin Shi <shihanlin9@gmail.com>
Copy link
Member

@hasheddan hasheddan left a comment

Choose a reason for hiding this comment

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

@hanlins the CA data is currently exposed in the connection secret that gets created for the cluster -- is that suitable for your use case?

@hanlins
Copy link
Contributor Author

hanlins commented Nov 22, 2021

@hanlins the CA data is currently exposed in the connection secret that gets created for the cluster -- is that suitable for your use case?

Hi @hasheddan, thanks a lot! Btw, is there a way to figure out what's inside that secret without digging into code? Currently, the secret reference is in the cluster spec, and the contents in that secret are actually controlled by the implementation of the controllers, not part of the CRD API contract. I'm fine relying on this implicit contract, but is there a better way to document this and make it more explicit? For now I think user has to either really provision a Cluster object and check what's in that referenced secret, or go through the code as you pointed above to learn what's in that secret, it would be nice to make this information explicit since it's the interface to users. If there're some documents on this (or did I miss any existing docs?) that make this API clear to the users, if so then I'm ok to close this PR :)

@haarchri
Copy link
Member

@hasheddan did we have in crossplane compositions an patch like fromConnectionSecretKey then it is easier to get informations from ConnectionSecretKey to other resources

@ONordander
Copy link
Contributor

Having the clusterCA exposed in the status would solve my use-case of automatically adding the cluster to ArgoCD by using the provider-kubernetes.
So +1 for this

@muvaf
Copy link
Member

muvaf commented Jan 29, 2022

Is certificate authority data sensitive? If so, we can't have it under status.

@janwillies
Copy link
Contributor

This is basically the question in crossplane/crossplane#2772 (comment)

@chlunde
Copy link
Collaborator

chlunde commented Feb 14, 2022

I don't think it should be considered sensitive. It's the public key for validating the control plane certificate, but kubectl does omit it for brevity: https://github.com/kubernetes/client-go/blob/f6ce18ae578c8cca64d14ab9687824d9e1305a67/tools/clientcmd/api/helpers.go#L83-L112

@haarchri
Copy link
Member

I am fine to merge the Open PR to add this ca in status

@hanlins
Copy link
Contributor Author

hanlins commented Feb 22, 2022

Any further thoughts/concerns? Should we merge this change? @haarchri :)

@ONordander
Copy link
Contributor

Any updates on this? Would be nice to have in the next release.

Copy link
Member

@haarchri haarchri left a comment

Choose a reason for hiding this comment

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

@hanlins thanks for implementation
@chlunde thanks for add clarification for CAdata

@haarchri haarchri merged commit 8ab5582 into crossplane-contrib:master Mar 7, 2022
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.

Expose CAData in EKS CRD
7 participants