Use controllerruntime to discover CABundle #636

JacobGabrielson · 2021-08-24T20:11:17Z

Issue, if available:

N/A

Description of changes:

Apologies for the churn folks, after talking to @ellistarn offline, it seems like the best path forward, given some other changes coming soon, is to explicitly call GetCABundle() from the cloudprovider, rather than dynamically default it in.

Also, this change now uses client-go to discover the CABundle contents (effectively it's still doing the same thing
as before, just using their library code now).

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

netlify · 2021-08-24T20:11:22Z

✔️ Deploy Preview for karpenter-docs-prod canceled.

🔨 Explore the source changes: 85a721d

🔍 Inspect the deploy log: https://app.netlify.com/sites/karpenter-docs-prod/deploys/61303df7a6caed0007f5addf

pkg/cloudprovider/aws/suite_test.go

bwagner5 · 2021-08-24T21:03:50Z

looks like the toolchain install is broken because this link does not work to download kubebuilder 2.3.1 anymore:

https://go.kubebuilder.io/dl/2.3.1/linux/amd64

Not sure why that one doesn't work, but https://go.kubebuilder.io/releases/latest/linux/amd64 does work.

The github assets link does work though, so maybe we just change to that (the go.kubebuilder.io/releases just redirects to github):
https://github.com/kubernetes-sigs/kubebuilder/releases/download/v2.3.2/kubebuilder_2.3.2_linux_amd64.tar.gz

pkg/cloudprovider/aws/cloudprovider.go

pkg/cloudprovider/aws/suite_test.go

pkg/utils/filesys/filesys.go

pkg/cloudprovider/aws/suite_test.go

ellistarn

Feel free to merge. Comments are optional and nonblocking.

rustrial · 2021-08-30T06:23:26Z

pkg/cloudprovider/aws/cloudprovider.go

 		errs = errs.Also(apis.ErrMissingField("name")).ViaField("cluster")
 	}
+	if ptr.StringValue(spec.Cluster.CABundle) == "" {


FYI: I deliberately skipped this check in my initial version of CABundle handling, to support the use-case where there is no CABundle (e.g. if the k8s API endpoint is served in plain-text without TLS). I know this might not be possible with EKS (which is always TLS), but it can still happen with non EKS clusters hosted on EC2.

Personally, I don't care to much, as I only use TLS protected k8s API endpoints and plain-text endpoints are a bad practice anyway. However, I still prefer to have secure defaults while still allowing others to opt-out (for whatever reasons they might have).

Maybe, we can make this check conditional, based on the protocol (scheme) part for the cluster endpoint.

https -> apply this check

http -> ignore this check

@rustrial thanks! That makes sense to me, I'll make the change and see what everyone thinks.

Ok, for now I've changed the code back to the way it was before - it won't complain if CABundle is missing. I'll add the http/https nuance back in in a future change.

pkg/cloudprovider/aws/fake/filesystem.go

pkg/apis/provisioning/v1alpha3/provisioner_validation.go

pkg/cloudprovider/aws/suite_test.go

absolute paths

... instead of rest.InClusterConfig() - this will work in more scenarios, such as when Karpenter is not running inside of the cluster

bwagner5

lgtm

ellistarn

Curious why we move away from gotemplates for userdata. I find it a bit more difficult to parse.

JacobGabrielson · 2021-09-02T16:11:55Z

Curious why we move away from gotemplates for userdata. I find it a bit more difficult to parse.

It started getting complicated trying to call a function from the templates, so it seemed easier overall to switch to the more clunky old school method.

This reverts commit 7012c92.

Re-do of #636

… keys (aws#636)

JacobGabrielson requested a review from ellistarn August 24, 2021 20:11

bwagner5 reviewed Aug 24, 2021

View reviewed changes

pkg/cloudprovider/aws/suite_test.go Outdated Show resolved Hide resolved

JacobGabrielson force-pushed the fstest branch from 81f6683 to 6f17e85 Compare August 26, 2021 20:39

JacobGabrielson changed the title ~~[WIP][EXPERIMENTAL] support fake filesystems in unit tests~~ Support fake filesystems in unit tests Aug 29, 2021

JacobGabrielson changed the title ~~Support fake filesystems in unit tests~~ Add additional spec validations for AWS cloudprovider Aug 29, 2021

JacobGabrielson requested a review from bwagner5 August 29, 2021 00:29

ellistarn reviewed Aug 29, 2021

View reviewed changes

pkg/cloudprovider/aws/cloudprovider.go Outdated Show resolved Hide resolved

ellistarn reviewed Aug 29, 2021

View reviewed changes

pkg/cloudprovider/aws/suite_test.go Outdated Show resolved Hide resolved

ellistarn reviewed Aug 29, 2021

View reviewed changes

pkg/cloudprovider/aws/suite_test.go Outdated Show resolved Hide resolved

ellistarn reviewed Aug 29, 2021

View reviewed changes

pkg/utils/filesys/filesys.go Outdated Show resolved Hide resolved

ellistarn reviewed Aug 29, 2021

View reviewed changes

pkg/cloudprovider/aws/suite_test.go Outdated Show resolved Hide resolved

JacobGabrielson requested a review from ellistarn August 29, 2021 22:31

ellistarn previously approved these changes Aug 30, 2021

View reviewed changes

JacobGabrielson dismissed ellistarn’s stale review via 8ce467d August 30, 2021 00:57

JacobGabrielson force-pushed the fstest branch from bf9e964 to 8ce467d Compare August 30, 2021 00:57

JacobGabrielson requested a review from ellistarn August 30, 2021 01:01

rustrial reviewed Aug 30, 2021

View reviewed changes

ellistarn reviewed Aug 30, 2021

View reviewed changes

pkg/cloudprovider/aws/fake/filesystem.go Outdated Show resolved Hide resolved

JacobGabrielson requested a review from ellistarn August 30, 2021 18:12

JacobGabrielson force-pushed the fstest branch 2 times, most recently from ea33ba6 to 05a211e Compare August 30, 2021 21:37

ellistarn reviewed Aug 30, 2021

View reviewed changes

pkg/apis/provisioning/v1alpha3/provisioner_validation.go Outdated Show resolved Hide resolved

JacobGabrielson requested a review from ellistarn August 30, 2021 23:00

ellistarn reviewed Aug 30, 2021

View reviewed changes

pkg/cloudprovider/aws/suite_test.go Outdated Show resolved Hide resolved

ellistarn previously approved these changes Aug 30, 2021

View reviewed changes

JacobGabrielson changed the title ~~Add additional spec validations for AWS cloudprovider~~ [WIP] Add additional spec validations for AWS cloudprovider Aug 31, 2021

JacobGabrielson marked this pull request as draft August 31, 2021 00:30

JacobGabrielson dismissed ellistarn’s stale review via faf44e4 August 31, 2021 00:38

jacob added 16 commits September 1, 2021 15:52

address pr fback

b9228a7

switch to afero everywhere because io/fs just doesn't like

982ebd1

absolute paths

minor cleanup

139931e

Address PR feedback from rustrial@

08e4884

address etarn pr fback

dacdbf5

update docs

2e4500c

remove no longer valid test

541a2df

remove stray line

b610d1d

pr fback

b6825c8

revert changes

36a393d

undo my changes hrere

2fc7224

start to migrate clusterCA default code to cloudprovider logic

2f7cc91

move CABundle dynamic loading to launch template generation time

c16ae01

switch to using client-go to get CABundle

4b9f612

revert

1acd986

remove stray line

9e7ef29

JacobGabrielson force-pushed the fstest branch from e0abdba to 9e7ef29 Compare September 1, 2021 22:53

JacobGabrielson changed the title ~~Use client-go to discover CABundle~~ [WIP] Use client-go to discover CABundle Sep 1, 2021

use GetConfigOrDie() from controllerruntime ...

85a721d

... instead of rest.InClusterConfig() - this will work in more scenarios, such as when Karpenter is not running inside of the cluster

JacobGabrielson changed the title ~~[WIP] Use client-go to discover CABundle~~ [WIP] Use controllerruntime to discover CABundle Sep 2, 2021

JacobGabrielson changed the title ~~[WIP] Use controllerruntime to discover CABundle~~ Use controllerruntime to discover CABundle Sep 2, 2021

bwagner5 approved these changes Sep 2, 2021

View reviewed changes

ellistarn approved these changes Sep 2, 2021

View reviewed changes

JacobGabrielson merged commit 7012c92 into aws:main Sep 2, 2021

JacobGabrielson added a commit that referenced this pull request Sep 2, 2021

Revert "Use controllerruntime to discover CABundle (#636)"

9c38e67

This reverts commit 7012c92.

JacobGabrielson mentioned this pull request Sep 2, 2021

Revert "Use controllerruntime to discover CABundle" #653

Merged

JacobGabrielson added a commit that referenced this pull request Sep 2, 2021

Revert "Use controllerruntime to discover CABundle (#636)" (#653)

9f56936

This reverts commit 7012c92.

JacobGabrielson pushed a commit that referenced this pull request Sep 3, 2021

Auto-discover caBundle when missing (take 2) (#655)

31c6082

Re-do of #636

gfcroft pushed a commit to gfcroft/karpenter-provider-aws that referenced this pull request Nov 25, 2023

test: Add a test function that allows node requirement replacement on…

9827cae

… keys (aws#636)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use controllerruntime to discover CABundle #636

Use controllerruntime to discover CABundle #636

JacobGabrielson commented Aug 24, 2021 •

edited

Loading

netlify bot commented Aug 24, 2021 •

edited

Loading

bwagner5 commented Aug 24, 2021 •

edited

Loading

ellistarn left a comment

rustrial Aug 30, 2021 •

edited

Loading

JacobGabrielson Aug 30, 2021

JacobGabrielson Sep 1, 2021

bwagner5 left a comment

ellistarn left a comment

JacobGabrielson commented Sep 2, 2021

Use controllerruntime to discover CABundle #636

Use controllerruntime to discover CABundle #636

Conversation

JacobGabrielson commented Aug 24, 2021 • edited Loading

netlify bot commented Aug 24, 2021 • edited Loading

bwagner5 commented Aug 24, 2021 • edited Loading

ellistarn left a comment

Choose a reason for hiding this comment

rustrial Aug 30, 2021 • edited Loading

Choose a reason for hiding this comment

JacobGabrielson Aug 30, 2021

Choose a reason for hiding this comment

JacobGabrielson Sep 1, 2021

Choose a reason for hiding this comment

bwagner5 left a comment

Choose a reason for hiding this comment

ellistarn left a comment

Choose a reason for hiding this comment

JacobGabrielson commented Sep 2, 2021

JacobGabrielson commented Aug 24, 2021 •

edited

Loading

netlify bot commented Aug 24, 2021 •

edited

Loading

bwagner5 commented Aug 24, 2021 •

edited

Loading

rustrial Aug 30, 2021 •

edited

Loading