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

Auto detect cluster name on eks, aks, gke #131

Merged
merged 2 commits into from
Jul 28, 2021
Merged

Auto detect cluster name on eks, aks, gke #131

merged 2 commits into from
Jul 28, 2021

Conversation

jmorganca
Copy link
Contributor

@jmorganca jmorganca commented Jul 24, 2021

  • Uses the metadata APIs to detect cluster name
  • Defaults to a cluster-<short hash of cluster CA> (e.g. cluster-79b0c378), which should be unique per cluster (is there a better alternative here?)
  • User can still specify infra engine --name to force a name. This is probably what will be done when spinning up clusters via infrastructure-as-code.

Before merging (or shortly after) it may be important to support docker-desktop and perhaps minikube for users who are trying out Infra locally.

Fixes #42

@jmorganca jmorganca requested a review from BruceMacD July 26, 2021 13:33
Copy link
Collaborator

@BruceMacD BruceMacD left a comment

Choose a reason for hiding this comment

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

Looks good.

I can't come up with a better solution than the CA has naming either. Im assuming we can always rely on a unique CA cert being present?

return "", err
}

req.Header.Add("Metadata-Flavor", "Google")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer my metadata to be chocolate flavor 🍨

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was stuck on this for an hour. Ends up no flavor = no answer from the metadata api

🙃


func (k *Kubernetes) Name() (string, error) {
name, err := eksClusterName()
if err == nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should log the response errors at the debug level rather than completely ignoring them, just in case we are ever trying to figure out why a name isn't getting assigned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great catch. Will get some logging in here before returning an error if we can't automatically determine the cluster name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now will log a generic error, but I've created an issue to use zap logging for the engine as well: #134

@jmorganca
Copy link
Contributor Author

@BruceMacD yes, having a CA is a must-have for in any kubernetes cluster to my knowledge.

However, I do wonder about the case where a user creates two clusters with the same private CA. I don't think this is possible with GKE/EKS/AKS (or almost any other kubernetes setup) but that is something to watch out for.

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.

Automatically detect cluster name
2 participants