Make aws-cni config more flexible and generalized #11816

moshevayner · 2021-06-19T14:01:29Z

johngmyers · 2021-06-20T00:40:20Z

pkg/apis/kops/networking.go

+	// Specifies whether ipamd should configure rp filter for primary interface.
+	// Setting this to false will require rp filter to be configured through init container.
+	AwsVpcK8sCniConfigureRpFilter string   `json:"awsVpcK8sCniConfigureRpFilter,omitempty"`
+	Env                           []EnvVar `json:"env,omitempty"`


Should not remove the documentation comment

johngmyers · 2021-06-20T00:41:00Z

pkg/apis/kops/networking.go

@@ -248,12 +248,19 @@ type RomanaNetworkingSpec struct {

 // AmazonVPCNetworkingSpec declares that we want Amazon VPC CNI networking
 type AmazonVPCNetworkingSpec struct {
+	// Specifies whether ipamd should configure rp filter for primary interface.


Doc comment needs to start with the name of the field.

johngmyers · 2021-06-20T00:42:21Z

pkg/apis/kops/networking.go

+	// Setting this to false will require rp filter to be configured through init container.
+	AwsVpcK8sCniConfigureRpFilter string   `json:"awsVpcK8sCniConfigureRpFilter,omitempty"`
+	Env                           []EnvVar `json:"env,omitempty"`
+	// If ENABLE_POD_ENI is set to true, in order for the kubelet to connect via TCP (for liveness or readiness probes) to pods that are using per pod security groups,


Doc comment needs to start with the name of the field.

upup/models/cloudup/resources/addons/networking.amazon-vpc-routed-eni/k8s-1.16.yaml.template

moshevayner · 2021-06-20T09:56:20Z

Thanks for the review, @johngmyers !
I've addressed all your comments, please have a look when you get a chance and let me know if you had any other concerns 😄 🙏🏻

johngmyers · 2021-06-21T03:28:42Z

pkg/apis/kops/networking.go

-	InitImageName string `json:"initImageName,omitempty"`
+	// AwsVpcK8sCniConfigureRpFilter specifies whether ipamd should configure rp filter for primary interface.
+	// Setting this to false will require rp filter to be configured through init container.
+	AwsVpcK8sCniConfigureRpFilter string `json:"awsVpcK8sCniConfigureRpFilter,omitempty"`


All of this refers to the AWS VPC CNI, so that part of the name is redundant.

"RP" is an acronym for "Reverse Path".

The possible values are "true" and "false", so it's a boolean.

Suggested change

AwsVpcK8sCniConfigureRpFilter string `json:"awsVpcK8sCniConfigureRpFilter,omitempty"`

ConfigureRPFilter *bool `json:"configureRPFilter,omitempty"`

johngmyers · 2021-06-21T03:30:02Z

pkg/apis/kops/networking.go

-	// The init container image name to use
-	InitImageName string `json:"initImageName,omitempty"`
+	// AwsVpcK8sCniConfigureRpFilter specifies whether ipamd should configure rp filter for primary interface.
+	// Setting this to false will require rp filter to be configured through init container.


Should mention the default is true.

johngmyers · 2021-06-21T03:31:34Z

pkg/apis/kops/networking.go

+	// in order for the kubelet to connect via TCP (for liveness or readiness probes) to pods that are using per pod security groups.
+	// This will increase the local TCP connection latency slightly.
+	// To use this setting, a Linux kernel version of at least 4.6 is needed on the worker node.
+	DisableTCPEarlyDemux string `json:"disableTCPEarlyDemux,omitempty"`


Also a boolean.

We try to avoid negative booleans.

Suggested change

DisableTCPEarlyDemux string `json:"disableTCPEarlyDemux,omitempty"`

TCPEarlyDemux *bool `json:"enableTCPEarlyDemux,omitempty"`

pkg/apis/kops/networking.go

upup/pkg/fi/cloudup/template_functions.go

hakman

I still think that initializing the Env vars with the options kOps overrides by default in template functions is better and also allows users to change those too in a generic way, without adding extra options.

pkg/apis/kops/networking.go

moshevayner · 2021-06-21T09:38:45Z

I still think that initializing the Env vars with the options kOps overrides by default in template functions is better and also allows users to change those too in a generic way, without adding extra options.

So I did that with everything else, however it seems like these two are actually different from the default values according to the docs, so I needed to make an override on them. Is there a better way to handle that, than the one I used?

moshevayner · 2021-06-21T09:57:36Z

/retest

hakman · 2021-06-22T04:23:31Z

Hey @MoShitrit, sorry for the delay, but was traveling for the past few days. Will take another look at this today.

hakman · 2021-06-22T06:04:04Z

I would add a template function like this:

	if cluster.Spec.Networking != nil && cluster.Spec.Networking.AmazonVPC != nil {
		c := cluster.Spec.Networking.AmazonVPC
		dest["AmazonVpcEnvVars"] = func() map[string]string {
			envVars := map[string]string{
				"AWS_VPC_K8S_CNI_CONFIGURE_RPFILTER": "false",
			}
			for _, e := range c.Env {
				envVars[e.Name] = e.Value
			}
			return envVars
		}
	}

and change the template to use it instead of directly accessing the env vars:

        {{- range $name, $value := AmazonVpcEnvVars }}
        - "name": "{{ $name }}"
          "value": "{{ $value }}"
        {{- end }}

This way we can change the defaults, but still allow overwriting them.

hakman

These are just some remaining nits, otherwise looks good from my point of view. Maybe also squash the commits.
@rifelpet Want to take a look too?

upup/models/cloudup/resources/addons/networking.amazon-vpc-routed-eni/k8s-1.16.yaml.template

…mplate functions for ease of customization Update auto-generated files

rifelpet · 2021-06-22T12:20:00Z

/lgtm

Will let @hakman do the final approval

hakman

Thanks @MoShitrit!

k8s-ci-robot · 2021-06-23T12:57:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hakman

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [hakman]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

moshevayner · 2021-06-23T13:25:17Z

/retest

moshevayner · 2021-06-23T14:33:43Z

/retest

k8s-ci-robot requested a review from hakman June 19, 2021 14:01

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/addons area/api size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 19, 2021

moshevayner force-pushed the fix-11144-aws-cni-config branch from 625161b to b8c401d Compare June 19, 2021 14:11

hakman requested a review from rifelpet June 19, 2021 15:38

johngmyers requested changes Jun 20, 2021

View reviewed changes

k8s-ci-robot assigned johngmyers Jun 20, 2021

moshevayner requested a review from johngmyers June 20, 2021 09:51

johngmyers requested changes Jun 21, 2021

View reviewed changes

moshevayner force-pushed the fix-11144-aws-cni-config branch from 8a05ab4 to 5f9f480 Compare June 21, 2021 09:23

hakman requested changes Jun 21, 2021

View reviewed changes

pkg/apis/kops/networking.go Show resolved Hide resolved

pkg/apis/kops/networking.go Show resolved Hide resolved

k8s-ci-robot assigned hakman Jun 21, 2021

moshevayner force-pushed the fix-11144-aws-cni-config branch from 5f9f480 to e239838 Compare June 21, 2021 09:29

moshevayner force-pushed the fix-11144-aws-cni-config branch 3 times, most recently from 6ffaba3 to dd26563 Compare June 21, 2021 21:56

moshevayner requested review from johngmyers and hakman June 21, 2021 21:57

moshevayner force-pushed the fix-11144-aws-cni-config branch from dd26563 to 0bdfabc Compare June 22, 2021 08:04

hakman requested changes Jun 22, 2021

View reviewed changes

upup/models/cloudup/resources/addons/networking.amazon-vpc-routed-eni/k8s-1.16.yaml.template Show resolved Hide resolved

upup/models/cloudup/resources/addons/networking.amazon-vpc-routed-eni/k8s-1.16.yaml.template Show resolved Hide resolved

moshevayner force-pushed the fix-11144-aws-cni-config branch from 0bdfabc to c4ab20a Compare June 22, 2021 08:42

Comment-out hardcoded default values and add the overriden ones as te…

6dee0ad

…mplate functions for ease of customization Update auto-generated files

moshevayner force-pushed the fix-11144-aws-cni-config branch from c4ab20a to 6dee0ad Compare June 22, 2021 09:29

moshevayner requested a review from hakman June 22, 2021 09:30

k8s-ci-robot assigned rifelpet Jun 22, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 22, 2021

hakman approved these changes Jun 23, 2021

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 23, 2021

k8s-ci-robot merged commit 82c050c into kubernetes:master Jun 23, 2021

k8s-ci-robot added this to the v1.22 milestone Jun 23, 2021

moshevayner deleted the fix-11144-aws-cni-config branch June 23, 2021 15:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make aws-cni config more flexible and generalized #11816

Make aws-cni config more flexible and generalized #11816

moshevayner commented Jun 19, 2021

johngmyers Jun 20, 2021

johngmyers Jun 20, 2021

johngmyers Jun 20, 2021

moshevayner commented Jun 20, 2021

johngmyers Jun 21, 2021

johngmyers Jun 21, 2021

johngmyers Jun 21, 2021

hakman left a comment

moshevayner commented Jun 21, 2021

moshevayner commented Jun 21, 2021

hakman commented Jun 22, 2021

hakman commented Jun 22, 2021

hakman left a comment •

edited

Loading

rifelpet commented Jun 22, 2021

hakman left a comment

k8s-ci-robot commented Jun 23, 2021

moshevayner commented Jun 23, 2021

moshevayner commented Jun 23, 2021

	AwsVpcK8sCniConfigureRpFilter string `json:"awsVpcK8sCniConfigureRpFilter,omitempty"`
	ConfigureRPFilter *bool `json:"configureRPFilter,omitempty"`

	DisableTCPEarlyDemux string `json:"disableTCPEarlyDemux,omitempty"`
	TCPEarlyDemux *bool `json:"enableTCPEarlyDemux,omitempty"`

Make aws-cni config more flexible and generalized #11816

Make aws-cni config more flexible and generalized #11816

Conversation

moshevayner commented Jun 19, 2021

johngmyers Jun 20, 2021

Choose a reason for hiding this comment

johngmyers Jun 20, 2021

Choose a reason for hiding this comment

johngmyers Jun 20, 2021

Choose a reason for hiding this comment

moshevayner commented Jun 20, 2021

johngmyers Jun 21, 2021

Choose a reason for hiding this comment

johngmyers Jun 21, 2021

Choose a reason for hiding this comment

johngmyers Jun 21, 2021

Choose a reason for hiding this comment

hakman left a comment

Choose a reason for hiding this comment

moshevayner commented Jun 21, 2021

moshevayner commented Jun 21, 2021

hakman commented Jun 22, 2021

hakman commented Jun 22, 2021

hakman left a comment • edited Loading

Choose a reason for hiding this comment

rifelpet commented Jun 22, 2021

hakman left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 23, 2021

moshevayner commented Jun 23, 2021

moshevayner commented Jun 23, 2021

hakman left a comment •

edited

Loading