feat: Update CustomCloudConfig proto file

[Devinwong]

What type of PR is this?
/kind feature

What this PR does / why we need it:
Update CustomCloudConfig proto file to use enable_custom_cloud_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:

• Rename enums for simplicity
• Update LoadBalancerConfig

Release note:

none

[tallaxes]

I am not at all sure shortening the prefixes works; here especially one has to get creative with NetworkPolicyType (NPOT) since NPT is already taken. Not pretty.

Other notes here:

• Can we drop TYPE in value names? And Type in enum names? (Or is any of this part of best practices for some good reason?)
• I am checking if NETWORK_MODE is still needed (suspect we could just assume "transparent")
• Surprised there is no NETWORK_POLICY=Cilium. Actually, it looks like NETWORK_POLICY is only used to determine bridge/transparent, so maybe we can get rid of it too (checking)

[Devinwong]

@tallaxes @lilypan26 The reason I was trying to shorten it is because of simplicity.
For example, if I use an enum in the client, it looked like nbcontractv1.NetworkPolicyType_NETWORK_POLICY_TYPE_CALICO. After the simplicity, it looks like this now nbcontractv1.NetworkPolicyType_NPOT_CALICO, though still very long.

• We can drop TYPE. As the enum name becomes longer, dropping TYPE makes sense to me.
• The reason why we still need the prefix (e.g, NETWORK_MODE) is because protbuf compilation. For example, if I have the following definitions

enum NetworkPluginType {
  NPT_UNSPECIFIED = 0;
  NPT_NONE = 1;
  AZURE = 2;
  NPT_KUBENET = 3;
}

enum NetworkPolicyType {
  NPOT_UNSPECIFIED = 0;
  NPOT_NONE = 1;
  AZURE = 2;
  NPOT_CALICO = 3;
}

message NetworkConfig {
  NetworkPluginType network_plugin = 1;
  NetworkPolicyType network_policy = 2;
  NetworkModeType network_mode = 3;
  string vnet_cni_plugins_url = 4;
  string cni_plugins_url = 5;
}

Note I removed the prefix from AZURE which exists in both NetworkPluginType and NetworkPolicyType.
Protobuf will complain symbol "nbcontract.v1.AZURE" already defined at pkg/proto/nbcontract/v1/networkconfig.proto:21:3; protobuf uses C++ scoping rules for enum values, so they exist in the scope enclosing the enum. That's why I put a shortened prefix. But if we prefer the longer form over shortened form, I am also fine. I don't have strong preference against either.

• NETWORK_POLICY and other network variables are still used in the bootstrap scripts for other computations (e.g., configureCNIIPTables) . So we still need to keep them😂. Cilium was found in the const.go in AgentBaker but seems not used in the bootstrap scripts. Will add it back if it's found useful.

[tallaxes]

NETWORK_MODE/NETWORK_POLICY are only involved in bridge/transparent decision (in configureCNIIPTables), so if we confirm transparent is always used (and as far as I can tell, the recent Azure CNI tarballs already have 10-azure.conflist with mode: transparent, so these sed commands don't do anything), we don't need them in contract. We could still set them to something on the VHD side, if we don't want to touch the scripts for now. The main point is that VHD right now does not really depend on, and does not need to know NETWORK_MODE/NETWORK_POLICY, so they don't need to be in the contract, and bootstrappers don't need to supply them (to be confirmed).

[lilypan26]

Should we just get rid of the abbreviations in front then? For example, I dont think NPOT_ adds anything useful.

[Devinwong]

But NETWORK_POLICY is also written to /etc/default/kubelet here https://github.com/Azure/AgentBaker/blob/a5e1c918a7c8dd0cf6a7e9fd8c8d8cfa1540eb41/parts/linux/cloud-init/artifacts/cse_config.sh#L382C11-L382C25. I don't know how kubelet is consuming it though.

[tallaxes]

But NETWORK_POLICY is also written to /etc/default/kubelet here https://github.com/Azure/AgentBaker/blob/a5e1c918a7c8dd0cf6a7e9fd8c8d8cfa1540eb41/parts/linux/cloud-init/artifacts/cse_config.sh#L382C11-L382C25. I don't know how kubelet is consuming it though.

I cannot think of a reason, but there may be one; would be great to track it down. This was introduced in #3349 (moved from original placement by #2788?) @anujmaheshwari1, @cameronmeissner, any insight of what this could possibly be for?

[anujmaheshwari1]

historically, not 100% sure why we need it, what values could it take? azure, calico?, from GPT:

The value for NETWORK_POLICY should indeed be set to the name of the network plugin you're using for network policy enforcement. Common values include calico, azure, cilium, flannel, kube-router, etc., depending on the network solution you're using in your Kubernetes cluster.

This configuration tells the kubelet to use the specified network plugin for implementing network policies within the cluster.

[tallaxes]

kubelet should have nothing to do with network policy, that's the confusing part.

[tallaxes]

Mystery solved, looks like it was used by a script that is no longer there, #351 (Thank you @wedaly for tracking this down!)

[Devinwong]

@tallaxes Updated the comments and removed TYPE from enum names. I still keep the NP_ and NPO unless there is better approach.

[coveralls]

Pull Request Test Coverage Report for Build 8607217012

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 82.077%

---

Totals
Change from base Build 8605193868:	0.0%
Covered Lines:	2450
Relevant Lines:	2985

---

💛 - Coveralls

[lilypan26]

I believe the conclusion was that we can remove network_mode. It was always be transparent so we can just directly pass that to cse_cmd for now until we update the scripts, but it can be removed from contract.

[Devinwong]

Totally! Missed that conclusion. Removed network_mode from contract now