feat: add outbound check variable #4531
Conversation
Pull Request Test Coverage Report for Build 9556178509Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9556627598Details
💛 - Coveralls |
AlisonB319
requested review from
juan-lee,
cameronmeissner,
UtheMan,
ganeshkumarashok,
anujmaheshwari1,
Devinwong,
lilypan26,
ShiqianTao,
AbelHu,
junjiezhang1997,
jason1028kr and
djsly
as code owners
| @@ -906,6 +906,12 @@ func getContainerServiceFuncMap(config *datamodel.NodeBootstrappingConfiguration | |||
| "GetOutboundCommand": func() string { | |||
| return getOutBoundCmd(config, config.CloudSpecConfig) | |||
| }, | |||
| "BlockOutboundNetwork": func() bool { | |||
| if config.OutboundType == datamodel.OutboundTypeBlock || config.OutboundType == datamodel.OutboundTypeNone { | |||
There was a problem hiding this comment.
are we sure we want OutboundTypeNone to indicate the same behavior as OutboundTypeBlock?
There was a problem hiding this comment.
Yes, they both mean the same to us - I've double checked this a handful of times since I think its a bit convoluted but it is what I've been told
There was a problem hiding this comment.
can we add comments to the respective definitions of these outbound types to indicate they mean the same thing?
maybe worth following up and seeing if we can eliminate one or the other altogether
There was a problem hiding this comment.
Here's a reply from Coco in one of the various NIC chats. I'll leave some commends as well but I'll drop the response in the comment for legacy sake
What’s the difference between outboundtype=none and outboundtype=block?
–outbound-type=none: AKS doesn't set up an outbound for users, but users can set the outbound rules. If there is an Azure Route Server announcing a default route via an NVA for egress to the internet. In this scenario, outboundType==none would still allow internet egress via the NVA
–outbound-type=block: AKS doesn't set up an outbound for users and AKS actively adds an NSG rule to all cluster nodes to prevent egress to the internet. So users cannot set the outbound rules. With –outbound-type=block and ArtifactSource=Cache, it’s the ‘zero egree cluster’. There are no outbound rules allowed on the cluster.
There was a problem hiding this comment.
the finny part is that the datamodel only contains those two attribute, so it's unclear what other value there could be
| logs_to_events "AKS.CSE.installNetworkPlugin" installNetworkPlugin | ||
| # Network plugin should not install if outbound is not allowed. | ||
| if [ "${BLOCK_OUTBOUND_NETWORK}" == "false" ]; then | ||
| logs_to_events "AKS.CSE.installNetworkPlugin" installNetworkPlugin |
There was a problem hiding this comment.
have we re-done things to make sure we still get a valid CNI plugin even if RP wants a different version than what's cached, and we want to block outbound network?
There was a problem hiding this comment.
I'm going to take this portion out of the PR for now, since I would like to get the BLOCK_OUTBOUND_NETWORK var in. But I think I made a mistake here, I'll need to double check, but we should just be getting an alternative URL to download CNI
Pull Request Test Coverage Report for Build 9898232093Details
💛 - Coveralls |
AlisonB319
changed the title
| @@ -88,6 +88,7 @@ GPU_NEEDS_FABRIC_MANAGER="{{GPUNeedsFabricManager}}" | |||
| NEEDS_DOCKER_LOGIN="{{and IsDockerContainerRuntime HasPrivateAzureRegistryServer}}" | |||
| IPV6_DUAL_STACK_ENABLED="{{IsIPv6DualStackFeatureEnabled}}" | |||
| OUTBOUND_COMMAND="{{GetOutboundCommand}}" | |||
| BLOCK_OUTBOUND_NETWORK="{{BlockOutboundNetwork}}" | |||
There was a problem hiding this comment.
not critical yet since we aren't actually using it, though we will want to let the karpenter folks know this is a new parameter they'll also need to set on their side
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Requirements:
Special notes for your reviewer:
Release note: