Update nbc contract: identityconfig, vmtype, nodepoolprofile #4208

lilypan26 · 2024-03-25T22:38:13Z

What type of PR is this?

What this PR does / why we need it:
Updating node bootstrap contract as needed. Updated IdentityConfig and added vmType enum. Removed nodepoolprofile config.

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

lilypan26 · 2024-03-25T22:39:21Z

pkg/proto/nbcontract/v1/nodepoolprofile.proto

-
-  // vm_size represents the size of this node pool
-  // Let's keep it for now but continue to review if this can be removed from contract as this can be retrieved from IMDS
-  string vm_size = 1;


Not seeing this used anywhere for populating cse_cmd, not sure why it was initially added?

Added back as it can be used to compute many other contract variables which can then be removed.

coveralls · 2024-03-25T22:40:27Z

Pull Request Test Coverage Report for Build 8483202629

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 79.321%

Totals
Change from base Build 8478658043:	0.0%
Covered Lines:	2290
Relevant Lines:	2887

💛 - Coveralls

lilypan26 · 2024-03-25T22:41:48Z

pkg/proto/nbcontract/v1/vmtype.proto

+package nbcontract.v1;
+
+enum VmType {
+  VM_TYPE_STANDARD = 0;


Not sure if defaulting to standard is the best move here, but this is what AB currently does unless vmss is explicity specified.

Agreed. I think it should be good as long as AB is doing this too.

BTW, protobuff official doc suggests to set the first enum=0 as unspecified to handle evolution scenario.
You may want to consider unspecified = 0, standard = 1, etc. And in our go binary parser, if it's unspecified, we will still set it to standard. Does that sound make sense to you?
https://protobuf.dev/programming-guides/dos-donts/#unspecified-enum

Do we know where (and what for) this is used?

the only place I found is that is it is logged into the azure json file: https://github.com/Azure/AgentBaker/blob/master/parts/linux/cloud-init/artifacts/cse_config.sh#L192C5-L192C28

i'm not sure how this file is used however

This file is used by cloud-provder-azure clients, the structure is documented here: https://cloud-provider-azure.sigs.k8s.io/install/configs/. But:

Cataloguing all of the uses of this on the node would be great. Currently it is likely used by kubelet (to authenticate to ACRs, via in-tree auth helpers which use cloud-provider-azure clients) and probably CSI drivers. There may be other uses I am not aware of.

I am not sure why any of these need vmType ... (kubelet ACR auth definitely should not need it?)

From the contract perspective, it may still be a good idea to collect everything that goes to azure.json in one place (unless we have a clear indication some settings are used elsewhere as well)

Sure I can start noting down where azure.json is used and what variables in the contract are stored inside. Not sure how best to confirm what variables are need or not though.

lilypan26 · 2024-03-25T22:42:34Z

pkg/proto/nbcontract/v1/identityconfig.proto

@@ -2,16 +2,8 @@ syntax = "proto3";
 package nbcontract.v1;

 message IdentityConfig {
-  IdentityType identity_type = 1;


Removing, since not seeing this used anywhere

Makes sense to me too. If later on we have other identity type to support, we can add it back.

….com/Azure/AgentBaker into lily/update-contract-identity-config

tallaxes · 2024-03-25T23:59:50Z

pkg/proto/nbcontract/v1/identityconfig.proto

+  string service_principal_id = 1;
+  string service_principal_secret = 2;
+  string assigned_identity_id = 3;  //could be user or system assigned, depending on the type
+  string use_managed_identity_extension = 4;


This should be bool, I think

good catch, should be a bool

tallaxes · 2024-03-26T00:00:52Z

pkg/proto/nbcontract/v1/identityconfig.proto

+  string service_principal_id = 1;
+  string service_principal_secret = 2;
+  string assigned_identity_id = 3;  //could be user or system assigned, depending on the type
+  string use_managed_identity_extension = 4;


Wondering if this is only used for generating azure config file? If yes - it might make sense to collect all the settings that affect it in one place ...

Grouped all the auth config fields and cluster config fields used to generate azure config file as listed here: https://cloud-provider-azure.sigs.k8s.io/install/configs/ into AuthConfig and ClusterConfig.

pkg/proto/nbcontract/v1/clusterconfig.proto

Devinwong · 2024-03-28T17:01:01Z

One unit test failed. It should be because the contract is changed. In case you didn't know yet, you can run it locally for convenience with go test go list ./... | grep -v e2e -coverprofile coverage_raw.out -covermode count

….com/Azure/AgentBaker into lily/update-contract-identity-config

tallaxes · 2024-03-28T21:53:55Z

pkg/proto/nbcontract/v1/authconfig.proto

+  string service_principal_id = 4; // set to aadClientId
+  string service_principal_secret = 5; // set to aadClientSecret


At least these (and possibly others) can be unspecified. What pattern did we settle on for handling optional strings?

I think we decided that unless we need to change the default value of just an empty string or need to handle the nil case specially, then there is no need to specify optional. In this case, not specifying it should have an empty string value so i excluded the optional.

….com/Azure/AgentBaker into lily/update-contract-identity-config

Lily Pan added 2 commits March 25, 2024 15:22

update nbc contract: identityconfig, vmtype, and nodepoolprofile

39b32ba

change default vmtype to standard

82559ff

lilypan26 requested review from juan-lee, cameronmeissner, UtheMan, ganeshkumarashok, anujmaheshwari1, AlisonB319, Devinwong, ShiqianTao, AbelHu, junjiezhang1997 and jason1028kr as code owners March 25, 2024 22:38

lilypan26 temporarily deployed to test March 25, 2024 22:38 — with GitHub Actions Inactive

lilypan26 commented Mar 25, 2024

View reviewed changes

Merge branch 'master' into lily/update-contract-identity-config

c6e9fcb

lilypan26 temporarily deployed to test March 25, 2024 23:27 — with GitHub Actions Inactive

Lily Pan added 2 commits March 25, 2024 16:58

add vmtype unspecified

8ab66b0

Merge branch 'lily/update-contract-identity-config' of https://github…

3fcc491

….com/Azure/AgentBaker into lily/update-contract-identity-config

lilypan26 temporarily deployed to test March 25, 2024 23:58 — with GitHub Actions Inactive

tallaxes reviewed Mar 25, 2024

View reviewed changes

tallaxes reviewed Mar 26, 2024

View reviewed changes

add back vm_size as top level field

2f59233

lilypan26 temporarily deployed to test March 26, 2024 17:48 — with GitHub Actions Inactive

Lily Pan added 2 commits March 26, 2024 16:50

replace identityconfig with authconfig

aa05cb4

group cluster config fields

8bf7226

lilypan26 temporarily deployed to test March 27, 2024 19:50 — with GitHub Actions Inactive

Devinwong reviewed Mar 27, 2024

View reviewed changes

pkg/proto/nbcontract/v1/clusterconfig.proto Outdated Show resolved Hide resolved

Devinwong reviewed Mar 27, 2024

View reviewed changes

pkg/proto/nbcontract/v1/clusterconfig.proto Show resolved Hide resolved

Devinwong reviewed Mar 27, 2024

View reviewed changes

pkg/proto/nbcontract/v1/clusterconfig.proto Show resolved Hide resolved

move vmtype and lbconfig to clusterconfig

96b373c

lilypan26 temporarily deployed to test March 27, 2024 23:01 — with GitHub Actions Inactive

Devinwong approved these changes Mar 28, 2024

View reviewed changes

fix parser

eaced64

lilypan26 temporarily deployed to test March 28, 2024 17:13 — with GitHub Actions Inactive

Merge branch 'master' into lily/update-contract-identity-config

f8006cc

lilypan26 temporarily deployed to test March 28, 2024 17:13 — with GitHub Actions Inactive

Lily Pan added 2 commits March 28, 2024 12:16

add comments

59bde8a

Merge branch 'lily/update-contract-identity-config' of https://github…

6f79fc8

….com/Azure/AgentBaker into lily/update-contract-identity-config

lilypan26 temporarily deployed to test March 28, 2024 19:17 — with GitHub Actions Inactive

Merge branch 'master' into lily/update-contract-identity-config

dc587ca

lilypan26 temporarily deployed to test March 28, 2024 19:19 — with GitHub Actions Inactive

tallaxes reviewed Mar 28, 2024

View reviewed changes

remove isSgxNode

ebdec88

tallaxes approved these changes Mar 29, 2024

View reviewed changes

Merge branch 'lily/update-contract-identity-config' of https://github…

2f4a709

….com/Azure/AgentBaker into lily/update-contract-identity-config

lilypan26 temporarily deployed to test March 29, 2024 00:42 — with GitHub Actions Inactive

Merge branch 'master' into lily/update-contract-identity-config

b724971

lilypan26 temporarily deployed to test March 29, 2024 16:31 — with GitHub Actions Inactive

lilypan26 enabled auto-merge (squash) March 29, 2024 16:31

lilypan26 merged commit d9ad8ea into master Mar 29, 2024
16 checks passed

lilypan26 deleted the lily/update-contract-identity-config branch March 29, 2024 16:32

tallaxes mentioned this pull request Mar 30, 2024

feat: support custom subnet ID (--vnet-subnet-id) Azure/karpenter-provider-azure#238

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update nbc contract: identityconfig, vmtype, nodepoolprofile #4208

Update nbc contract: identityconfig, vmtype, nodepoolprofile #4208

lilypan26 commented Mar 25, 2024

lilypan26 Mar 25, 2024

lilypan26 Mar 26, 2024

coveralls commented Mar 25, 2024 •

edited

Loading

lilypan26 Mar 25, 2024

Devinwong Mar 25, 2024

Devinwong Mar 25, 2024

tallaxes Mar 25, 2024

lilypan26 Mar 26, 2024

tallaxes Mar 26, 2024 •

edited

Loading

lilypan26 Mar 26, 2024 •

edited

Loading

lilypan26 Mar 25, 2024

Devinwong Mar 25, 2024

tallaxes Mar 25, 2024

lilypan26 Mar 26, 2024

tallaxes Mar 26, 2024

lilypan26 Mar 27, 2024

Devinwong commented Mar 28, 2024

tallaxes Mar 28, 2024

lilypan26 Mar 28, 2024

		string service_principal_id = 4; // set to aadClientId
		string service_principal_secret = 5; // set to aadClientSecret

Update nbc contract: identityconfig, vmtype, nodepoolprofile #4208

Update nbc contract: identityconfig, vmtype, nodepoolprofile #4208

Conversation

lilypan26 commented Mar 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 25, 2024 • edited Loading

Pull Request Test Coverage Report for Build 8483202629

Details

💛 - Coveralls

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

tallaxes Mar 26, 2024 • edited Loading

Choose a reason for hiding this comment

lilypan26 Mar 26, 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

Choose a reason for hiding this comment

Devinwong commented Mar 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 25, 2024 •

edited

Loading

tallaxes Mar 26, 2024 •

edited

Loading

lilypan26 Mar 26, 2024 •

edited

Loading