Support for setting init container options and per-pod security groups #617

alex-hunt-materialize · 2021-09-02T23:48:00Z

Proposed changes

Supports setting the init container image and DISABLE_TCP_EARLY_DEMUX
env var.

This allows users to upgrade their version of the CNI, and to support
per-pod security groups.

Related issues

Fixes #619
Fixes #620

Relates to, but doesn't solve: #622

Supports setting the init container image and DISABLE_TCP_EARLY_DEMUX env var. This allows users to upgrade their version of the CNI, and to support per-pod security groups.

alex-hunt-materialize · 2021-09-02T23:55:20Z

~~I unfortunately am unable to test this change, since I can't figure out how to install it and use it with a python virtualenv (all our pulumi uses python).~~ Any advice on how to do that would be greatly appreciated.

EDIT: I got this working with the python module by a hacky/half-implementation of #595 and copying the binary and some remaining files into my PATH. This PR works in our environment.

~~The tests seem to run locally, but all get marked as failed due to being unable to write the results to S3.~~ EDIT: that's a red herring, it's actually because I hit AWS limits for VPCs and EIPs. Running without parallelism seems to fix all failures except TestAccAwsProfile which requires a second AWS profile that I haven't configured.

I have no idea why local tests should write their output to S3. That seems like an unrelated bug, which I've filed at #621

lukehoban

Nice - thanks!

On test coverage - it would be really nice to add a simple test that uses a newer CNI than the default - just to lock in that that works. Perhaps alongside examples/cluster but configured with the new input parameter supported here?

lukehoban · 2021-09-07T23:15:59Z

provider/cmd/pulumi-gen-eks/main.go

+		},
+		"disableTcpEarlyDemux": {
+			TypeSpec:    schema.TypeSpec{Type: "boolean"},
+			Description: "Allows the kubelet's liveness and readiness probes to connect via TCP when pod ENI is enabled. This will slightly increase local TCP connection latency.",


It's a little unfortunate, but for consistency, lets line wrap this similar to other descriptions in this file. (A little surprised we aren't getting a int failure on this).

lukehoban · 2021-09-07T23:21:44Z

provider/cmd/pulumi-gen-eks/main.go

-				"of secondary ENI IP addresses. If set to true, the SNAT iptables rule and off-VPC " +
-				"IP rule are not applied, and these rules are removed if they have already been " +
-				"applied.\n\nDefaults to false.",
-		},


Should we deprecate this instead of removing it in case users are depending on it?

Probably not related to this PR - but the inconsistency that some of these are cni... and others aren't is a little odd. It actually feels like we most likely want to standardize on not including the cni prefix (since these are all properties of the VpcCni anyway. In which case, it might make sense to deprecate the other one instead? (And file an issue to track aligning the remaining too cni... property names with the naming scheme?

+1 on deprecating instead. If you want to back this change out and open an issue about the duplication instead, we can address that separately and in the process standardize the properties as Luke suggested.

I've backed it out, and am filing a new issue for this.

viveklak

Thanks for taking this on! Looks great. Could you take a look at a couple of the suggestions add a test as Luke mentioned?

viveklak · 2021-09-08T17:27:01Z

provider/cmd/pulumi-gen-eks/main.go

-				"of secondary ENI IP addresses. If set to true, the SNAT iptables rule and off-VPC " +
-				"IP rule are not applied, and these rules are removed if they have already been " +
-				"applied.\n\nDefaults to false.",
-		},


+1 on deprecating instead. If you want to back this change out and open an issue about the duplication instead, we can address that separately and in the process standardize the properties as Luke suggested.

add custom images to cluster example, and reduce line length in disableTcpEarlyDemux description.

alex-hunt-materialize · 2021-09-08T20:30:56Z

I've backed out the externalSnat change, added image and initImage VpcCniOptions to examples/cluster, and filed #624 to later fix the externalSnat issue.

viveklak · 2021-09-08T21:04:59Z

examples/cluster/index.ts

+    vpcCniOptions: {
+        image: "602401143452.dkr.ecr.us-west-2.amazonaws.com/amazon-k8s-cni:v1.9.0",
+        initImage: "602401143452.dkr.ecr.us-west-2.amazonaws.com/amazon-k8s-cni-init:v1.9.0",
+    },


Would be nice to ensure the VPC CNI options were actually honored as well. The validation could be added here.

Would be nice to ensure the VPC CNI options were actually honored as well. The validation could be added here.

You might have to export some additional information, e.g. the spec from index.ts.

My typescript/javascript skills are pretty much non-existent. I'm unsure how to export these.
I've tried:

export const image2 = cluster2.core.vpcCni.image; export const initImage2 = cluster2.core.vpcCni.initImage;

but I get:

index.ts(36,45): error TS2339: Property 'image' does not exist on type 'VpcCni'. index.ts(37,49): error TS2339: Property 'initImage' does not exist on type 'VpcCni'.

viveklak · 2021-09-08T21:07:01Z

This also fixes a duplicate argument, where externalSnat and cniExternalSnat would potentially fight over setting the AWS_VPC_K8S_CNI_EXTERNALSNAT environment variable. I can find no reason for both to exist, and cniExternalSnat seemed more complete, so I kept that one.

Could you remove this from the PR body since it was backed out?

alex-hunt-materialize · 2021-09-08T22:57:05Z

This also fixes a duplicate argument, where externalSnat and cniExternalSnat would potentially fight over setting the AWS_VPC_K8S_CNI_EXTERNALSNAT environment variable. I can find no reason for both to exist, and cniExternalSnat seemed more complete, so I kept that one.

Could you remove this from the PR body since it was backed out?

Done

benesch · 2021-09-10T04:08:17Z

So, @viveklak, I'm afraid we don't have the expertise in house at @MaterializeInc to get the test suite properly amended for this change. This package in particular is really tricky to develop on locally!

The good news is that I managed to ship a forked of pulumi-eks: https://github.com/MaterializeInc/pulumi-eks/commit/d4b05f9370462c1a64a0e582e36ef2c1c0a04e77#diff-5d43b9a26b3f3c15075a30aed416693a0ccd1d793cce198952bc4cf0693ca63c. That relieves the time pressure on our end. We'd still love to see this merged, because we don't want to be maintaining a fork long term, but I think we've probably pushed this PR as far as we can.

viveklak · 2021-09-10T05:28:53Z

So, @viveklak, I'm afraid we don't have the expertise in house at @MaterializeInc to get the test suite properly amended for this change. This package in particular is really tricky to develop on locally!

The good news is that I managed to ship a forked of pulumi-eks: MaterializeInc@d4b05f9#diff-5d43b9a26b3f3c15075a30aed416693a0ccd1d793cce198952bc4cf0693ca63c. That relieves the time pressure on our end. We'd still love to see this merged, because we don't want to be maintaining a fork long term, but I think we've probably pushed this PR as far as we can.

No worries and glad this worked for you. I will verify this on my end tomorrow and merge, then follow up with any test related updates. Thanks for taking the time to make the change!

TonyRippy · 2021-10-08T16:11:39Z

Hi all, looks like there are some merge conflicts that need to be resolved before this can be merged. Does anyone with write access have some time to resolve the conflict?

github-actions · 2021-10-08T22:54:05Z

PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

alex-hunt-materialize · 2021-11-01T20:19:36Z

@viveklak Any update on this? It's been working great for us on our fork for close to two months now. We'd love to point back at the upstream soon, but we can't until this is merged.

viveklak · 2021-11-09T01:57:59Z

/run-acceptance-tests

github-actions · 2021-11-09T01:58:23Z

Please view the PR build - https://github.com/pulumi/pulumi-eks/actions/runs/1437579302

viveklak · 2021-11-09T06:36:28Z

I have confirmed the changes work. However, the command dispatch workflow used to validate external PRs seems to be currently broken due to schema change detection not being able to get access to the originating workflow. We are looking to fix this but I would like to get this change in, so I opened #631 which pulls in these changes and adds a test to validate. I'd like to close this in favor of #631. Huge thanks for the contribution @alex-hunt-materialize and team!

lukehoban · 2021-11-09T20:44:38Z

Thanks @alex-hunt-materialize! 🚀

alex-hunt-materialize · 2021-11-09T20:55:26Z

Thank you all for merging it :)

viveklak · 2021-11-09T20:56:18Z

🙌🏾 - I will cut a release shortly.

Support for setting init container options

34221ce

Supports setting the init container image and DISABLE_TCP_EARLY_DEMUX env var. This allows users to upgrade their version of the CNI, and to support per-pod security groups.

alex-hunt-materialize mentioned this pull request Sep 7, 2021

Use the latest VPC_CNI Version 1.9 #610

Closed

lukehoban reviewed Sep 7, 2021

View reviewed changes

viveklak self-requested a review September 7, 2021 23:30

viveklak reviewed Sep 8, 2021

View reviewed changes

back out externalSnat removal

0f3751f

add custom images to cluster example, and reduce line length in disableTcpEarlyDemux description.

viveklak approved these changes Sep 8, 2021

View reviewed changes

viveklak reviewed Sep 8, 2021

View reviewed changes

Merge branch 'master' into init_container_support

bd2bd47

viveklak mentioned this pull request Nov 9, 2021

Support for setting init container options and per-pod security groups #631

Merged

viveklak merged commit bd2bd47 into pulumi:master Nov 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for setting init container options and per-pod security groups #617

Support for setting init container options and per-pod security groups #617

alex-hunt-materialize commented Sep 2, 2021 •

edited

Loading

alex-hunt-materialize commented Sep 2, 2021 •

edited

Loading

lukehoban left a comment

lukehoban Sep 7, 2021

alex-hunt-materialize Sep 8, 2021

lukehoban Sep 7, 2021

viveklak Sep 8, 2021

alex-hunt-materialize Sep 8, 2021

viveklak left a comment

viveklak Sep 8, 2021

alex-hunt-materialize commented Sep 8, 2021

viveklak Sep 8, 2021

viveklak Sep 8, 2021 •

edited

Loading

alex-hunt-materialize Sep 8, 2021

viveklak commented Sep 8, 2021

alex-hunt-materialize commented Sep 8, 2021

benesch commented Sep 10, 2021

viveklak commented Sep 10, 2021

TonyRippy commented Oct 8, 2021

github-actions bot commented Oct 8, 2021

alex-hunt-materialize commented Nov 1, 2021 •

edited

Loading

viveklak commented Nov 9, 2021

github-actions bot commented Nov 9, 2021

viveklak commented Nov 9, 2021

lukehoban commented Nov 9, 2021

alex-hunt-materialize commented Nov 9, 2021 •

edited

Loading

viveklak commented Nov 9, 2021

Support for setting init container options and per-pod security groups #617

Support for setting init container options and per-pod security groups #617

Conversation

alex-hunt-materialize commented Sep 2, 2021 • edited Loading

Proposed changes

Related issues

alex-hunt-materialize commented Sep 2, 2021 • edited Loading

lukehoban left a 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

viveklak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-hunt-materialize commented Sep 8, 2021

Choose a reason for hiding this comment

viveklak Sep 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viveklak commented Sep 8, 2021

alex-hunt-materialize commented Sep 8, 2021

benesch commented Sep 10, 2021

viveklak commented Sep 10, 2021

TonyRippy commented Oct 8, 2021

github-actions bot commented Oct 8, 2021

alex-hunt-materialize commented Nov 1, 2021 • edited Loading

viveklak commented Nov 9, 2021

github-actions bot commented Nov 9, 2021

viveklak commented Nov 9, 2021

lukehoban commented Nov 9, 2021

alex-hunt-materialize commented Nov 9, 2021 • edited Loading

viveklak commented Nov 9, 2021

alex-hunt-materialize commented Sep 2, 2021 •

edited

Loading

alex-hunt-materialize commented Sep 2, 2021 •

edited

Loading

viveklak Sep 8, 2021 •

edited

Loading

alex-hunt-materialize commented Nov 1, 2021 •

edited

Loading

alex-hunt-materialize commented Nov 9, 2021 •

edited

Loading