Spaces: add support for public load balancer #360
Conversation
| func detectKubernetes(kClient kubernetes.Interface) (CloudType, error) { | ||
| // EKS and Kind are _harder_ to detect based on version, so look at node labels. | ||
| ctx := context.Background() | ||
| if nodes, err := kClient.CoreV1().Nodes().List(ctx, metav1.ListOptions{}); err == nil { |
There was a problem hiding this comment.
Do we want to return the error if it's non-nil?
There was a problem hiding this comment.
Maybe? I didn't error since nodes is not a required aspect of Spaces. If the detection fails, I didn't think that this worthy to abort the installation since the cloud type could be set via --set clusterType=
There was a problem hiding this comment.
OK. If we don't think it's worthy, maybe we could add a comment for why we don't care and then handle the lint err 👍
cmd/up/space/init.go
Outdated
| if c.Set == nil { | ||
| c.Set = make(map[string]string) | ||
| } | ||
| for k, v := range defs.SpacesValues { | ||
| skip := false | ||
| for cs, _ := range c.Set { | ||
| if k == cs { | ||
| skip = true | ||
| } | ||
| } | ||
| if !skip { | ||
| c.Set[k] = v | ||
| } | ||
| } | ||
| if c.PrivateIngress { | ||
| defs.PublicIngress = false | ||
| } |
There was a problem hiding this comment.
nit - could we pull this logic into its own method? It'll make it easier to test this logic in isolation 👍
cmd/up/space/init.go
Outdated
| Yes bool `name:"yes" type:"bool" help:"Answer yes to all questions"` | ||
| Version string `arg:"" help:"Upbound Spaces version to install."` | ||
| Yes bool `name:"yes" type:"bool" help:"Answer yes to all questions"` | ||
| PrivateIngress bool `name:"private-ingress" type:"bool" help:"For AKS,EKS,GKE do not expose ingress publically"` |
There was a problem hiding this comment.
I think this will default to false, do we want to default it to true?
There was a problem hiding this comment.
I'd argue from a customer perspective, if you are in a public cloud that the ingress should be public by default (otherwise you have manually change that). The cloud logic sets it to false for Generic/undetected and Kind.
There was a problem hiding this comment.
The few customers I've talked to have generally been surprised when we have defaulted to setting up a public loadbalancer. Maybe the sampleset is skewed, but it seems like we should default to the common case and allow for opting out in the uncommon case (allowing a public loadbalancer).
There was a problem hiding this comment.
Fair enough....the last push is now:
ben@minerva:~/src/up $ _output/bin/darwin_arm64/up space init --token-file=~/scratch/pull-key.json v0.15.1 --set clusterType=eks --public-ingress
INFO Applying settings for Managed Kubernetes on EKS
INFO Public ingress will be exposed
INFO Required prerequisites met!
INFO Proceeding with Upbound Spaces installation...
√ [1/3]: Creating pull secret upbound-pull-secret
▀ [2/3]: Initializing Space components (0s)up: error: space.initCmd.Run(): chart already installed with version 0.15.1
ben@minerva:~/src/up $ _output/bin/darwin_arm64/up space init --token-file=~/scratch/pull-key.json v0.15.1 --set clusterType=eks
INFO Applying settings for Managed Kubernetes on EKS
INFO Required prerequisites met!
INFO Proceeding with Upbound Spaces installation...
√ [1/3]: Creating pull secret upbound-pull-secret
▀ [2/3]: Initializing Space components (0s)up: error: space.initCmd.Run(): chart already installed with version 0.15.1
ben@minerva:~/src/up $ _output/bin/darwin_arm64/up space init --token-file=~/scratch/pull-key.json v0.15.1
INFO Setting defaults for vanilla Kubernetes (type kind)
INFO Required prerequisites met!
INFO Proceeding with Upbound Spaces installation...
√ [1/3]: Creating pull secret upbound-pull-secret
▀ [2/3]: Initializing Space components (0s)up: error: space.initCmd.Run(): chart already installed with version 0.15.1
The detection based on Kubernetes GitVersion doesn't work for kind clusters and is not guaranteed for some managed Kubernetes. By using the provider, the vendor detection of more correct.
|
Tested and confirmed to work with GKE, AKS, EKS and kind -- all work beautifully |
For managed kubernetes, provision a public ingress unless the user opts out. Signed-off-by: Ben Howard <ben@upbound.io>
Co-authored-by: Taylor Thornton <2375126+tnthornton@users.noreply.github.com>
Description of your changes
I have:
make reviewableto ensure this PR is ready for review.backport release-x.ylabels to auto-backport this PR, as appropriate.