feat: add configuration support for secure TLS bootstrap client RPC timeouts#8261
Open
cameronmeissner wants to merge 6 commits intomainfrom
Open
feat: add configuration support for secure TLS bootstrap client RPC timeouts#8261cameronmeissner wants to merge 6 commits intomainfrom
cameronmeissner wants to merge 6 commits intomainfrom
Conversation
cameronmeissner
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Adds support for configuring secure TLS bootstrapping client timeouts via CSE variables, updating the Linux bootstrap scripts and Go template plumbing to pass new per-operation timeout flags (and treating the existing --deadline as deprecated/optional).
Changes:
- Extend
SecureTLSBootstrappingConfigwith per-step timeout fields and add corresponding template funcs inbaker.go. - Update Linux CSE templates/scripts to emit and consume new timeout env vars and append the new flags to
BOOTSTRAP_FLAGS. - Update unit/spec tests to validate the new env vars/flags and adjust expectations around
--deadline.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
spec/parts/linux/cloud-init/artifacts/cse_config_spec.sh |
Updates ShellSpec expectations for secure-tls-bootstrap drop-in content and adds new env vars for test coverage. |
pkg/agent/datamodel/types.go |
Adds new secure TLS bootstrapping timeout fields + getters; marks Deadline deprecated. |
pkg/agent/baker.go |
Plumbs new timeout getters into the CSE template function map. |
pkg/agent/baker_test.go |
Adds a test asserting secure TLS bootstrapping vars are correctly emitted into the CSE command. |
parts/linux/cloud-init/artifacts/cse_config.sh |
Appends new timeout flags when corresponding env vars are set; changes defaulting behavior for --deadline. |
parts/linux/cloud-init/artifacts/cse_cmd.sh |
Adds new timeout variables to the generated CSE environment. |
Contributor
|
The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).
|
aks-node-controller/proto/aksnodeconfig/v1/bootstrapping_config.proto
Outdated
Show resolved
Hide resolved
aks-node-controller/pkg/gen/aksnodeconfig/v1/bootstrapping_config.pb.go
Outdated
Show resolved
Hide resolved
cameronmeissner
requested review from
Devinwong,
lilypan26 and
r2k1
as code owners
cameronmeissner
requested review from
AbelHu,
djsly,
ganeshkumarashok,
junjiezhang1997,
phealy and
timmy-wright
as code owners
cameronmeissner
requested review from
SriHarsha001,
awesomenix,
calvin197,
mxj220,
pdamianov-dev,
sulixu,
surajssd and
zachary-bailey
as code owners
Comment on lines
+1929
to
+1943
| func (c *SecureTLSBootstrappingConfig) GetGetAccessTokenTimeout() string { | ||
| if c == nil { | ||
| return "" | ||
| } | ||
| return c.GetAccessTokenTimeout | ||
| } | ||
|
|
||
| func (c *SecureTLSBootstrappingConfig) GetGetInstanceDataTimeout() string { | ||
| if c == nil { | ||
| return "" | ||
| } | ||
| return c.GetInstanceDataTimeout | ||
| } | ||
|
|
||
| func (c *SecureTLSBootstrappingConfig) GetGetNonceTimeout() string { |
There was a problem hiding this comment.
The getter names GetGetAccessTokenTimeout, GetGetInstanceDataTimeout, etc. have a duplicated Get prefix and are the only GetGet* methods in this package. Renaming these to GetAccessTokenTimeout, GetInstanceDataTimeout, etc. would improve API clarity and avoid spreading this naming pattern to templates/callers.
Suggested change
| func (c *SecureTLSBootstrappingConfig) GetGetAccessTokenTimeout() string { | |
| if c == nil { | |
| return "" | |
| } | |
| return c.GetAccessTokenTimeout | |
| } | |
| func (c *SecureTLSBootstrappingConfig) GetGetInstanceDataTimeout() string { | |
| if c == nil { | |
| return "" | |
| } | |
| return c.GetInstanceDataTimeout | |
| } | |
| func (c *SecureTLSBootstrappingConfig) GetGetNonceTimeout() string { | |
| func (c *SecureTLSBootstrappingConfig) GetAccessTokenTimeout() string { | |
| if c == nil { | |
| return "" | |
| } | |
| return c.GetAccessTokenTimeout | |
| } | |
| func (c *SecureTLSBootstrappingConfig) GetInstanceDataTimeout() string { | |
| if c == nil { | |
| return "" | |
| } | |
| return c.GetInstanceDataTimeout | |
| } | |
| func (c *SecureTLSBootstrappingConfig) GetNonceTimeout() string { |
| "SECURE_TLS_BOOTSTRAPPING_GET_ATTESTED_DATA_TIMEOUT": config.GetBootstrappingConfig().GetSecureTlsBootstrappingGetAttestedDataTimeout(), | ||
| "SECURE_TLS_BOOTSTRAPPING_GET_CREDENTIAL_TIMEOUT": config.GetBootstrappingConfig().GetSecureTlsBootstrappingGetCredentialTimeout(), | ||
| "SECURE_TLS_BOOTSTRAPPING_DEADLINE": config.GetBootstrappingConfig().GetSecureTlsBootstrappingDeadline(), | ||
| "CUSTOM_SECURE_TLS_BOOTSTRAPPING_CLIENT_URL": config.GetBootstrappingConfig().GetSecureTlsBootstrappingCustomClientDownloadUrl(), |
There was a problem hiding this comment.
The env var key CUSTOM_SECURE_TLS_BOOTSTRAPPING_CLIENT_URL does not match the variable name used by the provisioning scripts (CUSTOM_SECURE_TLS_BOOTSTRAPPING_CLIENT_DOWNLOAD_URL in parts/linux/cloud-init/artifacts/cse_install.sh / cse_cmd.sh). As a result, when bootstrapping via aks-node-controller, a configured custom secure TLS bootstrap client download URL will not be picked up by the scripts. Align the env var name with what the scripts actually consume.
Suggested change
| "CUSTOM_SECURE_TLS_BOOTSTRAPPING_CLIENT_URL": config.GetBootstrappingConfig().GetSecureTlsBootstrappingCustomClientDownloadUrl(), | |
| "CUSTOM_SECURE_TLS_BOOTSTRAPPING_CLIENT_DOWNLOAD_URL": config.GetBootstrappingConfig().GetSecureTlsBootstrappingCustomClientDownloadUrl(), |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #