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

Kubernetes Secret Kubeconfig integration #66

Merged
merged 1 commit into from
Sep 6, 2023

Conversation

stevendborrelli
Copy link
Contributor

@stevendborrelli stevendborrelli commented May 26, 2023

Description of your changes

Ready for review.

  • Add ability to use a kubeconfig secret generated by Crossplane.
  • Converts ClusterParameters Name, Server, and Config.TLSClientConfig to pointers as they are optional.

Fixes #13

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

  • I ran this for several hours against a cluster with some debugging enabled to monitor the secret versions and ensure every time the referenced secret was updated our controller requested an update.
  • Deployed an application against the cluster.

@janwillies
Copy link
Collaborator

Thanks for taking this on @stevendborrelli! This is our most wanted feature request. Is there anything you want to discuss before doing further work here?

@stevendborrelli
Copy link
Contributor Author

I am still doing some testing and refactoring.

There are a couple of things I'd like some clarity on:

  1. if meta.WasDeleted(cr) && meta.GetExternalName(cr) == observedCluster.Name {

is causing cluster deletes to be skipped:

	if meta.WasDeleted(cr) && meta.GetExternalName(cr) == observedCluster.Name {
		// ArgoCD Cluster resource ignores the name field. This detects the deletion of the default cluster resource.
		return managed.ExternalObservation{}, nil
	}

@stevendborrelli stevendborrelli changed the title Do not merge: WIP for kubeconfig integration Kubernetes Secret Kubeconfig integration Jun 5, 2023
@stevendborrelli
Copy link
Contributor Author

@janwillies this PR is ready for review. My main concern is with the deletion code. Without the change my clusters would not delete.

@stevendborrelli
Copy link
Contributor Author

To test this PR, you can use the EKS example for the AWS official providers at : https://github.com/upbound/provider-aws/blob/main/examples/eks/cluster.yaml.

The secret will be written to cluster-conn in upbound-system.

The example in examples/cluster/cluster-kubeconfig.yaml will use this secret to register the cluster to ArgoCD.

apis/cluster/v1alpha1/types.go Outdated Show resolved Hide resolved
apis/cluster/v1alpha1/types.go Outdated Show resolved Hide resolved
apis/cluster/v1alpha1/types.go Outdated Show resolved Hide resolved
examples/cluster/cluster-kubeconfig.yaml Outdated Show resolved Hide resolved
pkg/controller/cluster/controller.go Outdated Show resolved Hide resolved
pkg/controller/cluster/controller.go Outdated Show resolved Hide resolved
pkg/controller/cluster/controller.go Outdated Show resolved Hide resolved
pkg/controller/cluster/controller.go Outdated Show resolved Hide resolved
@MisterMX
Copy link
Collaborator

MisterMX commented Jun 6, 2023

Thanks for your PR @stevendborrelli! Can you take a look at the remarks and also solve the conflicts?

We would be happy to see this feature in v0.3.0. Do you think you can make it in the next couple of days?

@stevendborrelli
Copy link
Contributor Author

@MisterMX I made an initial pass changing fields to optional but there are some issues in the Mock function causing tests to fail. I'll try to get something by Friday.

@deggja
Copy link

deggja commented Aug 10, 2023

@stevendborrelli @MisterMX
Is there any plans to still implement this? I can see that the work stopped a couple of months ago. If not, what is missing to get this implemented?

Sorry for barging in here like this.

@stevendborrelli
Copy link
Contributor Author

Sorry for the delay everyone, I needed to add the ability to track changes to the kubeconfig, as cloud providers rotate the credentials. I've pushed a new commit that includes this tracking.

@stevendborrelli
Copy link
Contributor Author

The PR has been updated and is ready for review.

Copy link
Collaborator

@MisterMX MisterMX left a comment

Choose a reason for hiding this comment

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

Thank you very much for your contribition @stevendborrelli. I added two remarks to your code. Can you take a look at them?

Also, can you squash your changes into a single commit and sign it?

pkg/controller/cluster/controller.go Outdated Show resolved Hide resolved
pkg/controller/cluster/controller.go Outdated Show resolved Hide resolved
Signed-off-by: Steven Borrelli <steve@borrelli.org>
@stevendborrelli
Copy link
Contributor Author

stevendborrelli commented Sep 6, 2023

@MisterMX I've squashed the changes for the PR, and addressed your feedback.

Copy link
Collaborator

@MisterMX MisterMX left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you very much for your contribution @stevendborrelli!

@MisterMX MisterMX merged commit d456a7b into crossplane-contrib:main Sep 6, 2023
7 checks passed
@MisterMX MisterMX mentioned this pull request Feb 15, 2024
2 tasks
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.

server reference to a crossplane k8s cluster
4 participants