Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better documentation for "MINIMUM_IP_TARGET" and related variables #3161

Open
guessi opened this issue Dec 26, 2024 · 0 comments
Open

Better documentation for "MINIMUM_IP_TARGET" and related variables #3161

guessi opened this issue Dec 26, 2024 · 0 comments

Comments

@guessi
Copy link
Contributor

guessi commented Dec 26, 2024

What would you like to be added:

Although MINIMUM_IP_TARGET have been documented, but there have no explanation what's the relationship between MINIMUM_IP_TARGET and --minimum-ip-target / --windows-minimum-ip-target? besides, it may need to explain why minimum-ip-target would pointed to minimumWindowsIPTarget variable as well.

Why is this needed:

Tried to study where the minimum-ip-target inside configmaps/amazon-vpc-cni came from, and I can see it's defined by minimumWindowsIPTarget under values.yaml

data:
enable-windows-ipam: {{ .Values.enableWindowsIpam | quote }}
enable-network-policy-controller: {{ .Values.enableNetworkPolicy | quote }}
enable-windows-prefix-delegation: {{ .Values.enableWindowsPrefixDelegation | quote }}
warm-prefix-target: {{ .Values.warmWindowsPrefixTarget | quote }}
warm-ip-target: {{ .Values.warmWindowsIPTarget | quote }}
minimum-ip-target: {{ .Values.minimumWindowsIPTarget | quote }}
branch-eni-cooldown: {{ .Values.branchENICooldown | quote }}

# - Windows Prefix Delegation settings
enableWindowsPrefixDelegation: "false"
warmWindowsPrefixTarget: 0
warmWindowsIPTarget: 1
minimumWindowsIPTarget: 3

After few minutes code tracing, I found it was initially introduced by PR#2715, but no explanation for why minimum-ip-target would initially pointed to minimumWindowsIPTarget ? I though it should be windows-minimum-ip-target?

Later on, I can see PR#2716 changed its default to 3, but still no explanation for why the default value changed?

I'm aware of the variables renamed at aws/amazon-vpc-resource-controller-k8s,

but it looks like the variable names does not reflect to what it should be.

Proposed change

templates/configmap.yaml

# https://github.com/aws/amazon-vpc-cni-k8s/tree/master?tab=readme-ov-file#warm_prefix_target-v190
warm-prefix-target: {{ .Values.warmPrefixTarget | quote }} 
windows-warm-prefix-target: {{ .Values.warmWindowsPrefixTarget | quote }} 

# https://github.com/aws/amazon-vpc-cni-k8s/tree/master?tab=readme-ov-file#warm_ip_target
warm-ip-target: {{ .Values.warmIPTarget | quote }} 
windows-warm-ip-target: {{ .Values.warmWindowsIPTarget | quote }} 

# https://github.com/aws/amazon-vpc-cni-k8s/tree/master?tab=readme-ov-file#minimum_ip_target-v160
minimum-ip-target: {{ .Values.minimumIPTarget | quote }} 
windows-minimum-ip-target: {{ .Values.minimumWindowsIPTarget | quote }} 

values.yaml

# https://github.com/aws/amazon-vpc-cni-k8s/tree/master?tab=readme-ov-file#warm_prefix_target-v190
warmPrefixTarget: xxx
warmWindowsPrefixTarget: xxx

# https://github.com/aws/amazon-vpc-cni-k8s/tree/master?tab=readme-ov-file#warm_ip_target
warmIPTarget: xxx
warmWindowsIPTarget: xxx

# https://github.com/aws/amazon-vpc-cni-k8s/tree/master?tab=readme-ov-file#minimum_ip_target-v160
minimumIPTarget: xxx
minimumWindowsIPTarget: xxx
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant