feat: add ServiceAccountImagePullProfile for identity-binding-based image pull #7596
Conversation
qweeah
changed the title
qweeah
requested review from
AbelHu,
Devinwong,
awesomenix,
cameronmeissner,
djsly,
ganeshkumarashok,
juan-lee,
junjiezhang1997,
lilypan26,
phealy,
r2k1,
timmy-wright and
zachary-bailey
as code owners
|
This PR's logic LGTM. But I don't quiet understand what is step 2 "RP integration and configuration flow". Agentbaker won't rely on anything from AKS RP, I think you could make all changes in the same PR and add E2E for it? Here is an example PR: #7059 . |
|
The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).
|
| type SecurityProfile struct { | ||
| PrivateEgress *PrivateEgress `json:"privateEgress,omitempty"` | ||
| PrivateEgress *PrivateEgress `json:"privateEgress,omitempty"` | ||
| ServiceAccountImagePullProfile *ServiceAccountImagePullProfile `json:"serviceAccountImagePullProfile,omitempty"` |
There was a problem hiding this comment.
I'm not very sure, but this profile belongs to ContainerService.Properties, it's not very common(?) to directly edit this struct, we typically append NodeBootstrappingConfiguration for feature implementation. Node sig reviewers may have more insight into this.
There was a problem hiding this comment.
I saw the comment here mentioning that Properties stands for the AKS cluster definition. Therefore, I thought that AKS-related settings ought to be placed in properties?
qweeah
force-pushed
the
qweeah/ibip
branch
2 times, most recently
from
c1bd134 to
5a21e71
Compare
Signed-off-by: Billy Zha <[email protected]>
Signed-off-by: Billy Zha <[email protected]>
norshtein
left a comment
norshtein
left a comment
There was a problem hiding this comment.
LGTM. Kindly request review from node sig folks.
| IB_ARGS="" | ||
| if [ "${SERVICE_ACCOUNT_IMAGE_PULL_ENABLED}" = "true" ]; then | ||
| IB_TOKEN_ATTRIBUTES=" | ||
| tokenAttributes: |
There was a problem hiding this comment.
nit: is it possible to remove \n in the line above So it will look like
IB_TOKEN_ATTRIBUTES="tokenAttributes:
serviceAccountTokenAudience: api://AKSIdentityBinding
...
and put ${IB_TOKEN_ATTRIBUTES} to the next line (line1080)? Seems more readable
| optionalServiceAccountAnnotationKeys: | ||
| - kubernetes.azure.com/acr-client-id" | ||
| IB_ARGS=" | ||
| - --ib-sni-name=${IDENTITY_BINDINGS_LOCAL_AUTHORITY_SNI}" |
There was a problem hiding this comment.
Same here. Remove the \n here?
| }) | ||
| } | ||
|
|
||
| func Test_Ubuntu2204Gen2_ImagePullIdentityBinding_Enabled(t *testing.T) { |
There was a problem hiding this comment.
Could you add some tests for Scriptless? Not necessarily all 4 tests, or 2 most significant tests are also good.
Scriptless is a new way to provision a node. We start to release it to production recently and should be available in all regions by the end of this month.
Check tests_ with suffix _scriptless for reference.
| type SecurityProfile struct { | ||
| PrivateEgress *PrivateEgress `json:"privateEgress,omitempty"` | ||
| PrivateEgress *PrivateEgress `json:"privateEgress,omitempty"` | ||
| ServiceAccountImagePullProfile *ServiceAccountImagePullProfile `json:"serviceAccountImagePullProfile,omitempty"` |
There was a problem hiding this comment.
ServiceAccountImagePullProfile doesn't sound like a SecurityProfile to me. Maybe just put it at the same level of SecurityProfile? (Move it one level up). But this is just my opinion. Will ask other teammates for inputs.
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR adds support for service-account-based image pull authentication (aka Identity Binding) from Azure Container Registry (ACR), implementing KEP-4412 projected service account tokens for kubelet image credential providers.
Changes
Data Model
Added
ServiceAccountImagePullProfiletoSecurityProfilewith fields:Enabled: Enable/disable service account-based image pullDefaultClientID: Default managed identity client IDDefaultTenantID: Default managed identity tenant IDLocalAuthoritySNI: SNI endpoint for Identity Bindings Local AuthorityAdded getter methods to
SecurityProfilefor null-safe access.Implementation Paths
1. Legacy CSE (Template-based)
pkg/agent/variables.go: Template variables for CSE scripts (using the naming conventionserviceAccountImagePull...)parts/linux/cloud-init/artifacts/cse_cmd.sh: Environment variable declarations (e.g.,SERVICE_ACCOUNT_IMAGE_PULL_ENABLED,SERVICE_ACCOUNT_IMAGE_PULL_DEFAULT_CLIENT_ID)parts/linux/cloud-init/artifacts/cse_config.sh: Credential provider config generation2. AKSNodeConfig (Proto-based)
aks-node-controller/proto/aksnodeconfig/v1/security_profile.proto: Proto definitions (ServiceAccountImagePullProfile)aks-node-controller/parser/parser.go: Environment variable generationaks-node-controller/parser/helper.go: Null-safe helper functionsBoth paths converge at
cse_config.shto generate/var/lib/kubelet/credential-provider-config.yamlwith identity binding arguments (--ib-default-client-id,--ib-default-tenant-id,--ib-sni-name) and token attributes.Testing
variables_test.go,datamodel/types_test.go, andparser/helper_test.goto reflect the new naming convention.All tests validate that the credential provider config file contains the correct identity binding configuration.
Cluster Support
Which issue(s) this PR fixes:
Fixes #
Requirements:
Special notes for your reviewer:
Dual implementation approach (legacy CSE + modern AKSNodeConfig) for backward compatibility. Both paths are fully tested and generate identical credential provider configuration. The renaming from
ImagePullIdentitytoServiceAccountImagePullensures consistency with the upstream feature name.Release note: