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

pkg/aws/eni: new subnet-ids parameter #16119

Merged
merged 1 commit into from May 28, 2021
Merged

Conversation

mvisonneau
Copy link
Contributor

This change permits the configuration of specific SubnetIDs []string
as part of the pkg/aws/eni/types.ENISpec. Essentially, it follows
the same principle as what was already in place with SecurityGroups /
SecurityGroupTags.

If SubnetIDs is set, it will be considered instead of SubnetTags. Here
is an example configuration:

{
  "cniVersion": "0.3.1",
  "name": "cilium",
  "type": "cilium-cni",
  "enable-debug": false,
  "eni": {
    "first-interface-index": 0,
    "pre-allocate": 10,
    "subnet-ids": ["subnet-0661a8d7e35b56e00","subnet-0661a8d7e35b56e01"],
    "subnet-tags": {},
    "security-group-tags": {},
    "security-groups": []
  }
}

This will result in the operator-aws looking up for subnets matching the
configured list in an exhaustive fashion.

pkg/aws/eni: new subnet-ids parameter

@mvisonneau mvisonneau requested review from a team as code owners May 12, 2021 15:14
@mvisonneau mvisonneau requested review from a team and tgraf May 12, 2021 15:14
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label May 12, 2021
@mvisonneau mvisonneau force-pushed the eni_subnet_ids branch 2 times, most recently from a0bd785 to 988de67 Compare May 12, 2021 15:26
@ungureanuvladvictor ungureanuvladvictor added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label May 12, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label May 12, 2021
@ungureanuvladvictor ungureanuvladvictor added area/eni Impacts ENI based IPAM. area/operator Impacts the cilium-operator component labels May 12, 2021
@ungureanuvladvictor
Copy link
Member

test-me-please

Copy link
Member

@ungureanuvladvictor ungureanuvladvictor left a comment

Choose a reason for hiding this comment

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

Overall LGTM but can you update the docs at https://github.com/cilium/cilium/blob/master/Documentation/concepts/networking/ipam/eni.rst#L349 to reflect the new behavior as well?

@mvisonneau
Copy link
Contributor Author

Oh indeed @ungureanuvladvictor my bad, I updated the docs 👍

Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Looks good, thanks! A few comments below.

Documentation/concepts/networking/ipam/eni.rst Outdated Show resolved Hide resolved
pkg/aws/eni/instances.go Outdated Show resolved Hide resolved
This change permits the configuration of specific `SubnetIDs []string`
as part of the pkg/aws/eni/types.ENISpec. Essentially, it follows
the same principle as what was already in place with SecurityGroups /
SecurityGroupTags.

If `SubnetIDs` is set, it will be considered instead of `SubnetTags`. Here
is an example configuration:

```json
{
  "cniVersion": "0.3.1",
  "name": "cilium",
  "type": "cilium-cni",
  "enable-debug": false,
  "eni": {
    "first-interface-index": 0,
    "pre-allocate": 10,
    "subnet-ids": ["subnet-0661a8d7e35b56e00","subnet-0661a8d7e35b56e01"],
    "subnet-tags": {},
    "security-group-tags": {},
    "security-groups": []
  }
}
```

This will result in the operator-aws looking up for subnets matching the
configured list in an exhaustive fashion.

Signed-off-by: Maxime VISONNEAU <maxime.visonneau@gmail.com>
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you!

@ungureanuvladvictor
Copy link
Member

test-me-please

@twpayne
Copy link
Contributor

twpayne commented May 25, 2021

ci-aks

@twpayne
Copy link
Contributor

twpayne commented May 25, 2021

test-1.20-4.19

@twpayne
Copy link
Contributor

twpayne commented May 25, 2021

test-1.21-4.9

@twpayne
Copy link
Contributor

twpayne commented May 25, 2021

test-runtime

@aanm aanm merged commit 2946fa2 into cilium:master May 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/eni Impacts ENI based IPAM. area/operator Impacts the cilium-operator component release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants