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

feat: add EnableSecureTLSBootstrapping to bootstrap config #3653

Merged
merged 3 commits into from
Sep 28, 2023

Conversation

cameronmeissner
Copy link
Collaborator

What type of PR is this?

/kind feature

What this PR does / why we need it:
This is the initial PR for secure TLS bootstrap integration work. It simply adds a new property to the node bootstrapping configuration datamodel which specifies whether or not secure TLS bootstrapping is enabled by RP. Subsequent PRs will use this value to implement the secure TLS bootstrap-specific logic.

Which issue(s) this PR fixes:

Fixes #

Requirements:

Special notes for your reviewer:

Release note:

none

@cameronmeissner cameronmeissner temporarily deployed to test September 28, 2023 18:53 — with GitHub Actions Inactive
// EnableSecureTLSBootstraping - when this feature is enabled we don't hard-code TLS bootstrap tokens at all,
// instead we create a modified bootstrap kubeconfig which points towards the STLS bootstrap client-go
// credential plugin installed on the VHD, which will be responsible for generating TLS bootstrap tokens on the fly
EnableSecureTLSBootstrapping bool
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this will be set to false until enabled on the RP-side

@coveralls
Copy link

Pull Request Test Coverage Report for Build 6343069414

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 79.142%

Totals Coverage Status
Change from base Build 6330510332: 0.02%
Covered Lines: 2231
Relevant Lines: 2819

💛 - Coveralls

@@ -379,7 +379,10 @@ func getContainerServiceFuncMap(config *datamodel.NodeBootstrappingConfiguration
return IsKubeletConfigFileEnabled(cs, profile, config.EnableKubeletConfigFile)
},
"IsKubeletClientTLSBootstrappingEnabled": func() bool {
return IsKubeletClientTLSBootstrappingEnabled(config.KubeletClientTLSBootstrapToken)
return config.EnableSecureTLSBootstrapping || IsKubeletClientTLSBootstrappingEnabled(config.KubeletClientTLSBootstrapToken)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

since we're OR'ing, EnableSecureTLSBootstrapping being false won't affect the overall value

Copy link
Contributor

Choose a reason for hiding this comment

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

clueless question - whats the difference between config having EnableSecureTLSBootstrapping set to true and IsKubeletClientTLSBootstrappingEnabled returning true?

Copy link
Collaborator Author

@cameronmeissner cameronmeissner Sep 28, 2023

Choose a reason for hiding this comment

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

basically, EnableSecureTLSBootstrap will be true when we are doing TLS bootstrapping but we're NOT using a hard-coded token, IsKubeletClientTLSBootstrappingEnabled() will be true when the node bootstrapping config we get from RP includes a hard-coded token, also implying that we're doing TLS bootstrapping.

so IsKubeletClientTLSBootstrappingEnabled overall will return true when we're doing some form of TLS bootstrapping, whether that be the secure way or with a hard-coded token

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, makes sense - might be a good idea to add this as a comment somewhere maybe for the future reference?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

did some renaming to make this a bit more clear, also added comments :)

@cameronmeissner cameronmeissner temporarily deployed to test September 28, 2023 19:23 — with GitHub Actions Inactive
@cameronmeissner cameronmeissner temporarily deployed to test September 28, 2023 19:26 — with GitHub Actions Inactive
@cameronmeissner cameronmeissner merged commit 1ad396c into master Sep 28, 2023
@cameronmeissner cameronmeissner deleted the cameissner/stlsbootstrap-datamodel branch September 28, 2023 21:12
@maxwolffe
Copy link
Contributor

@alexeldeib , @cameronmeissner - Hey friends - when are y'all planning on releasing this?

DB is planning on moving off of our custom AKS image (described in some detail here - #2815) - but we don't plan on being able to be fully off of it until the end of December.

I'm wondering if there's a way to explicitly exclude our clusters or subscriptions (within the MSFT tenant) from this feature until we've completed that migration?

@maxwolffe
Copy link
Contributor

cc @juan-lee , @yangl900

wanlonghenry pushed a commit that referenced this pull request Oct 6, 2023
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.

5 participants