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

Associate primary network interface SG with the trunk-eni when SG is not specified in ENIConfig #221

Merged
merged 2 commits into from
May 12, 2023

Conversation

sushrk
Copy link
Contributor

@sushrk sushrk commented May 11, 2023

Issue #, if available:
Fixes #189

Description of changes:
Currently, when security group is not specified in ENIConfig, the trunk ENI is associated with the default security group of the VPC. The expected behavior when using vpc-cni >= v1.8.0 is to use the primary network interface security groups with secondary network interfaces.

Changes in this PR is to associate the trunk ENI with the primary network interface security groups when ENIConfig is missing security groups.

Testing:

  1. Created two eniconfigs as follows-
[10/05/23 1:34:11] ➜  ~ kubectl get eniconfigs.crd.k8s.amazonaws.com -o yaml
apiVersion: v1
items:
- apiVersion: crd.k8s.amazonaws.com/v1alpha1
  kind: ENIConfig
  metadata:
    annotations:
      kubectl.kubernetes.io/last-applied-configuration: |
        {"apiVersion":"crd.k8s.amazonaws.com/v1alpha1","kind":"ENIConfig","metadata":{"annotations":{},"name":"us-west-2a"},"spec":{"securityGroups":["sg-1111111111"],"subnet":"subnet-1111111111"}}
    creationTimestamp: "2023-04-24T23:07:30Z"
    generation: 4
    name: us-west-2a
    resourceVersion: "11479510"
    uid: a261d5a8-8aea-4df4-a1b5-05e2525ea395
  spec:
    securityGroups:
    - sg-1111111111
    subnet: subnet-1111111111
- apiVersion: crd.k8s.amazonaws.com/v1alpha1
  kind: ENIConfig
  metadata:
    annotations:
      kubectl.kubernetes.io/last-applied-configuration: |
        {"apiVersion":"crd.k8s.amazonaws.com/v1alpha1","kind":"ENIConfig","metadata":{"annotations":{},"name":"us-west-2b"},"spec":{"subnet":"subnet-2222222222"}}
    creationTimestamp: "2023-04-27T16:05:58Z"
    generation: 3
    name: us-west-2b
    resourceVersion: "6314138"
    uid: f80f995b-a1fc-4d56-a93d-b4dab5b60546
  spec:
    subnet: subnet-2222222222
kind: List
metadata:
  resourceVersion: ""

In v1.1.5, the trunk ENI of the instance in us-west-2b is associated with the default security group of the VPC. Verified that with the changes in this PR the trunk ENI is associated with the primary network interface security groups.
Also verified with correct ENIConfigs that the trunk ENI is associated with the SG specified in the ENIConfig

  1. Ran all SGP tests in non-custom networking cluster to verify no regression issues- (skipping LOCAL tests)
Running Suite: Per Pod Security Group Suite - /home/ravsushm/go/src/github.com/aws/amazon-vpc-resource-controller-k8s/test/integration/perpodsg
===============================================================================================================================================
Random Seed: 1683852685

Will run 18 of 22 specs
-- redacted--

[AfterSuite] 
/home/ravsushm/go/src/github.com/aws/amazon-vpc-resource-controller-k8s/test/integration/perpodsg/perpodsg_suite_test.go:58
  STEP: waiting for all the ENIs to be deleted after being cooled down @ 05/12/23 01:12:50.433
[AfterSuite] PASSED [90.552 seconds]
------------------------------

Ran 18 of 22 Specs in 1372.557 seconds
SUCCESS! -- 18 Passed | 0 Failed | 0 Pending | 4 Skipped
PASS

Release Notes:
This PR introduces a breaking change. Starting in v1.1.6+, when security groups are not specified in ENIConfig with custom networking enabled, the trunk ENI will be associated with the security group of the primary network interface rather than the default security group of the VPC.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@sushrk sushrk requested a review from a team as a code owner May 11, 2023 02:32
Copy link
Contributor

@haouc haouc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good to me. Just some nit comments and one question. Thanks

pkg/aws/ec2/instance.go Outdated Show resolved Hide resolved
pkg/aws/ec2/instance.go Outdated Show resolved Hide resolved
pkg/aws/ec2/instance.go Outdated Show resolved Hide resolved
pkg/node/manager/manager_test.go Show resolved Hide resolved
scripts/gen_mocks.sh Show resolved Hide resolved
Copy link
Contributor

@haouc haouc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@sushrk sushrk merged commit 7971b0c into aws:master May 12, 2023
3 checks passed
@sushrk sushrk deleted the eniconfig-sg-fix branch May 12, 2023 01:45
haouc added a commit that referenced this pull request May 29, 2023
* add healthz subpathes for all controllers (#201)

* support arch arg in dockerfile (#207)

* updated vpc limits to include fields for hypervisor type and bare metal status (#217)

* enable node events when instance type is not supported (#218)

* Associate primary network interface SG with the trunk ENI when SG is not specified in ENIConfig (#221)

* Associate primary network interface SG with the trunk ENI when SG is not specified in ENIConfig

* add a new CRD to delegate vpc resource requests (#210)

* upgrade controller runtime version (#227)

* rebased onto master branch

* fixed merge conflict

---------

Co-authored-by: Hao Zhou <haouc@users.noreply.github.com>
Co-authored-by: Sushmitha Ravikumar <58063229+sushrk@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants