feat: cse support credential provider and vhd cache binary for Linux #4258

mainred · 2024-04-09T07:32:14Z

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Requirements:

uses conventional commit messages
includes documentation
adds unit tests
tested upgrade from previous version

Special notes for your reviewer:

Release note:

none

mainred · 2024-04-09T07:43:13Z

parts/linux/cloud-init/artifacts/cse_config.sh

+EOF
+}
+# remove blank lines from the config file.
+sed -i '/^$/d' ${CREDENTIAL_PROVIDER_CONFIG_FILE}


if L721 condition does not meet, there'll be a blank line left over.

mainred · 2024-04-09T07:45:15Z

parts/linux/cloud-init/artifacts/cse_config.sh

@@ -503,6 +503,14 @@ EOF
 # for DNS.
 iptables -I FORWARD -d 168.63.129.16 -p tcp --dport 80 -j DROP
 EOF
+
+    # check if kubelet flags contain image-credential-provider-config and image-credential-provider-bin-dir
+    if [[ $KUBELET_FLAGS == *"image-credential-provider-config"* && $KUBELET_FLAGS == *"image-credential-provider-bin-dir"* ]]; then


before 1.30 these two flags are optional

mainred · 2024-04-09T07:47:47Z

vhdbuilder/packer/install-dependencies.sh

@@ -408,6 +408,15 @@ fi
 stop_watch $capture_time "GPU Device plugin" false
 start_watch

+# Kubelet credential provider plugins
+CREDENTIAL_PROVIDER_VERSIONS="
+"


There's no release published yet.
Same case for Windows VHD packer script.

where is windows packer script?

I guess we'd need something similar here: https://github.com/Azure/AgentBaker/blob/master/vhdbuilder/packer/generate-windows-vhd-configuration.ps1

mainred · 2024-04-10T09:19:05Z

parts/windows/kuberneteswindowssetup.ps1

+# Kubelet credential provider
+$global:CredentialProviderURL = "{{GetParameter "windowsCredentialProviderURL"}}"
+$global:CredentialProviderBinDir = "c:\var\lib\kubelet\credential-provider"
+$global:CredentialProviderConfDir = "c:\var\lib\kubelet"


Should use c:\k instead because c:\k is the working folder of kuberentes for Windows node, and the other folder will bring error like access failure.
In my test, "c:\var\lib\kubelet" lead to access failure when writing the configure file.
|Access to the path 'C:\var\lib\kubelet\credential-provider-config.yaml' is denied.

parts/windows/kuberneteswindowssetup.ps1

AbelHu

Please follow staging/cse/windows/README to add the logic in aks windows cse scripts.

parts/windows/windowscsehelper.ps1

parts/windows/kuberneteswindowssetup.ps1

mainred · 2024-04-12T08:18:51Z

@AbelHu This change is basically for Linux side work, but include a few data model change, could you please take another look?

ping @feiskyer for another look.

Thank you.

cameronmeissner · 2024-04-12T15:35:35Z

pkg/agent/datamodel/types.go

+
+	// Full path to the Windows credential provider (tar.gz) to use.
+	// For example: https://acs-mirror.azureedge.net/cloud-provider-azure/v1.29.4/binaries/azure-acr-credential-provider-windows-amd64-v1.29.4.tar.gz
+	WindowsCredentialProviderURL string


are these specifically for ACR? if so can we reflect that in their names?

e.g. WindowsACRCredentialProviderURL

CredentialProviderURL is enough, credentialprovider is an official indicating container registry credential provider and in our case acr.

cameronmeissner · 2024-04-12T15:37:13Z

parts/linux/cloud-init/artifacts/cse_config.sh

@@ -503,6 +503,14 @@ EOF
 # for DNS.
 iptables -I FORWARD -d 168.63.129.16 -p tcp --dport 80 -j DROP
 EOF
+
+    # check if kubelet flags contain image-credential-provider-config and image-credential-provider-bin-dir
+    if [[ $KUBELET_FLAGS == *"image-credential-provider-config"* && $KUBELET_FLAGS == *"image-credential-provider-bin-dir"* ]]; then


nit: separate clauses with 2 separate [[ ]]'s

if [[ $KUBELET_FLAGS == *"image-credential-provider-config"* ]] && [[ $KUBELET_FLAGS == *"image-credential-provider-bin-dir"* ]]; then

I think it makes no difference between these two if logic.

cameronmeissner · 2024-04-12T15:38:11Z

parts/linux/cloud-init/artifacts/cse_config.sh

+    CREDENTIAL_PROVIDER_CONFIG_FILE=/var/lib/kubelet/credential-provider-config.yaml
+    mkdir -p "$(dirname "${CREDENTIAL_PROVIDER_CONFIG_FILE}")"
+    touch "${CREDENTIAL_PROVIDER_CONFIG_FILE}"
+    tee "${CREDENTIAL_PROVIDER_CONFIG_FILE}" > /dev/null <<EOF


nit: just use cat instead of tee'ing and piping output to the null device

tee is the common tool used in case to write files, so I it's good to keep it as it is.

cameronmeissner · 2024-04-12T15:39:22Z

parts/linux/cloud-init/artifacts/cse_config.sh

+    CREDENTIAL_PROVIDER_CONFIG_FILE=/var/lib/kubelet/credential-provider-config.yaml
+    mkdir -p "$(dirname "${CREDENTIAL_PROVIDER_CONFIG_FILE}")"
+    touch "${CREDENTIAL_PROVIDER_CONFIG_FILE}"
+    tee "${CREDENTIAL_PROVIDER_CONFIG_FILE}" > /dev/null <<EOF


Suggested change

tee "${CREDENTIAL_PROVIDER_CONFIG_FILE}" > /dev/null <<EOF

cat > "${CREDENTIAL_PROVIDER_CONFIG_FILE}" <<EOF

parts/linux/cloud-init/artifacts/cse_config.sh

parts/linux/cloud-init/artifacts/cse_cmd.sh

use a separate PR for windows work

mainred requested review from juan-lee, cameronmeissner, UtheMan, ganeshkumarashok, anujmaheshwari1, AlisonB319, Devinwong, lilypan26, ShiqianTao, AbelHu, junjiezhang1997 and jason1028kr as code owners April 9, 2024 07:32

mainred temporarily deployed to test April 9, 2024 07:32 — with GitHub Actions Inactive

mainred force-pushed the qinhao/credential-provider branch from 74af154 to 019f1f7 Compare April 9, 2024 07:40

mainred temporarily deployed to test April 9, 2024 07:40 — with GitHub Actions Inactive

mainred commented Apr 9, 2024

View reviewed changes

mainred force-pushed the qinhao/credential-provider branch from 019f1f7 to 57f9fef Compare April 9, 2024 08:03

mainred temporarily deployed to test April 9, 2024 08:03 — with GitHub Actions Inactive

mainred force-pushed the qinhao/credential-provider branch from 57f9fef to e213418 Compare April 10, 2024 04:06

mainred commented Apr 10, 2024

View reviewed changes

AbelHu reviewed Apr 10, 2024

View reviewed changes

parts/windows/kuberneteswindowssetup.ps1 Outdated Show resolved Hide resolved

AbelHu previously requested changes Apr 10, 2024

View reviewed changes

AbelHu reviewed Apr 10, 2024

View reviewed changes

parts/windows/windowscsehelper.ps1 Outdated Show resolved Hide resolved

AbelHu reviewed Apr 10, 2024

View reviewed changes

parts/windows/kuberneteswindowssetup.ps1 Outdated Show resolved Hide resolved

mainred force-pushed the qinhao/credential-provider branch 2 times, most recently from 7123bbd to ebbdb5d Compare April 11, 2024 08:51

mainred temporarily deployed to test April 11, 2024 08:51 — with GitHub Actions Inactive

feat: cse support credential provider and vhd cache binary for linux

f1bcfe6

mainred force-pushed the qinhao/credential-provider branch from ebbdb5d to f1bcfe6 Compare April 11, 2024 08:52

mainred temporarily deployed to test April 12, 2024 06:14 — with GitHub Actions Inactive

mainred temporarily deployed to test April 12, 2024 06:26 — with GitHub Actions Inactive

fix url link

1646a1c

mainred force-pushed the qinhao/credential-provider branch from dfd1c8c to 1646a1c Compare April 12, 2024 06:29

mainred temporarily deployed to test April 12, 2024 06:31 — with GitHub Actions Inactive

mainred enabled auto-merge (squash) April 12, 2024 08:20

feiskyer approved these changes Apr 12, 2024

View reviewed changes

no gocognit lint for getBakerFuncMap

aaffa7f

mainred temporarily deployed to test April 12, 2024 09:04 — with GitHub Actions Inactive

remove windows code

2faa9b4

mainred temporarily deployed to test April 12, 2024 09:18 — with GitHub Actions Inactive

AbelHu disabled auto-merge April 12, 2024 10:44

AbelHu self-requested a review April 12, 2024 10:44

cameronmeissner reviewed Apr 12, 2024

View reviewed changes

parts/linux/cloud-init/artifacts/cse_config.sh Outdated Show resolved Hide resolved

cameronmeissner reviewed Apr 12, 2024

View reviewed changes

parts/linux/cloud-init/artifacts/cse_config.sh Outdated Show resolved Hide resolved

cameronmeissner reviewed Apr 12, 2024

View reviewed changes

parts/linux/cloud-init/artifacts/cse_cmd.sh Outdated Show resolved Hide resolved

address comments

40d44d0

mainred temporarily deployed to test April 12, 2024 23:41 — with GitHub Actions Inactive

Merge branch 'master' into qinhao/credential-provider

8267ad9

mainred temporarily deployed to test April 12, 2024 23:43 — with GitHub Actions Inactive

cameronmeissner approved these changes Apr 12, 2024

View reviewed changes

mainred enabled auto-merge (squash) April 13, 2024 00:03

mainred merged commit f433ead into master Apr 13, 2024
20 checks passed

mainred deleted the qinhao/credential-provider branch April 13, 2024 00:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: cse support credential provider and vhd cache binary for Linux #4258

feat: cse support credential provider and vhd cache binary for Linux #4258

mainred commented Apr 9, 2024 •

edited

Loading

mainred Apr 9, 2024

mainred Apr 9, 2024

mainred Apr 9, 2024

feiskyer Apr 12, 2024

cameronmeissner Apr 12, 2024

mainred Apr 10, 2024

AbelHu left a comment

mainred commented Apr 12, 2024

cameronmeissner Apr 12, 2024

mainred Apr 12, 2024

cameronmeissner Apr 12, 2024

mainred Apr 12, 2024

cameronmeissner Apr 12, 2024

mainred Apr 12, 2024

cameronmeissner Apr 12, 2024

	tee "${CREDENTIAL_PROVIDER_CONFIG_FILE}" > /dev/null <<EOF
	cat > "${CREDENTIAL_PROVIDER_CONFIG_FILE}" <<EOF

feat: cse support credential provider and vhd cache binary for Linux #4258

feat: cse support credential provider and vhd cache binary for Linux #4258

Conversation

mainred commented Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AbelHu left a comment

Choose a reason for hiding this comment

mainred commented Apr 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mainred commented Apr 9, 2024 •

edited

Loading