Skip to content

ipam: Support for static IP allocation in AWS#34622

Merged
julianwiedmann merged 1 commit intocilium:mainfrom
DataDog:ai/static-ip-aws
Oct 2, 2024
Merged

ipam: Support for static IP allocation in AWS#34622
julianwiedmann merged 1 commit intocilium:mainfrom
DataDog:ai/static-ip-aws

Conversation

@antonipp
Copy link
Copy Markdown
Contributor

Description

This PR is a proposal for supporting static IP allocation as described in #34094.
The proposed abstraction is Cloud Provider agnostic. This PR implements the code path for AWS which is fully functional. The code for other Cloud Providers can be implemented later separately.

In this version of the implementation, we assume that users manually manage pools of static IP addresses (in this particular example, pools of AWS Elastic IP addresses) and assign different tags to differentiate the pools.
The main idea of this proposal is to add a new field (static-ip-tags) to the ipam section of the CNI spec. This will allow users to indicate that they want a static IP with given tags to be assigned to the node.
Example:

"ipam": {
  "static-ip-tags": {
    "pool": "pool-a",
    "kubernetes_cluster": "cluster-1"
  }
}

When the Operator sees that static-ip-tags is set on the CiliumNode object, it makes the Cloud Provider API calls to retrieve the IPs with the given tags and associates the first found IP with the instance. It then updates the status of the CiliumNode object.

Testing

I added some unit tests but also tried out the PR in a real AWS environment.
At first I manually allocated an EIP with tags "anton": "test" and "kubernetes_cluster": "<MY_CLUSTER_NAME>" in our AWS account.
I then provisioned a node with the following CNI spec:

$ sudo cat /etc/cni/net.d/10-generic-veth.conflist
{
  "cniVersion": "0.3.1",
  "name": "cilium",
  "plugins": [
    {
      "name": "cilium",
      "type": "cilium-cni",
      "eni": {
        "delete-on-termination": true,
        "first-interface-index": 1,
        "use-primary-address": false,
        "pre-allocate": 1,
        "min-allocate": 3
      },
      "ipam": {
        "pre-allocate": 1,
        "min-allocate": 3,
        "static-ip-tags": {
          "anton": "test",
          "kubernetes_cluster": ""
        }
      }
    }
  ]
} 

I then confirmed that the IPAM settings were successfully passed on to the CiliumNode CRD object:

$ kubectl get ciliumnode $MY_NODE_NAME -o=jsonpath='{.spec.ipam}' | jq
{
  "min-allocate": 3,
  "pool": {
    "100.xxx.xxx.xxx": {
      "resource": "eni-xxx"
    },
    "100.xxx.xxx.xxx": {
      "resource": "eni-xxx"
    }
  },
  "pools": {},
  "pre-allocate": 1,
  "static-ip-tags": {
    "anton": "test",
    "kubernetes_cluster": "<MY_CLUSTER_NAME>"
  }
}

The Operator saw that IPAM field and associated the EIP I created. Logs:

Found 1 EIPs corresponding to tags map[anton:test kubernetes_cluster:<MY_CLUSTER_NAME>]

Associated EIP 18.213.xxx.xxx with Instance i-xxxxf7517 (association ID: eipassoc-xxx)

It also updated the CRD resource status with the right IP address:

$ k get ciliumnode $MY_NODE_NAME -o=jsonpath='{.status.ipam}' | jq
{
  "assigned-static-ip": "18.213.xxx.xxx",
  "operator-status": {},
  "used": [...]
}

The EIP is successfully associated with the EC2 instance as well:
image

@antonipp antonipp requested review from a team as code owners August 30, 2024 13:28
@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 Aug 30, 2024
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Aug 30, 2024
Copy link
Copy Markdown
Contributor

@doniacld doniacld left a comment

Choose a reason for hiding this comment

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

One minor comment otherwise the PR seems good to me but note that I do not have a extended knowledge of this code.

Copy link
Copy Markdown
Contributor

@ldelossa ldelossa left a comment

Choose a reason for hiding this comment

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

LGTM

@ldelossa ldelossa added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Sep 17, 2024
@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 Sep 17, 2024
@ldelossa
Copy link
Copy Markdown
Contributor

Hey @antonipp

When making k8s related changes, there maybe some extra steps to do in our k8s infrastructure. Please view and handle the failing test here: https://github.com/cilium/cilium/actions/runs/10775087980/job/29878708227?pr=34622

Signed-off-by: Anton Ippolitov <anton.ippolitov@datadoghq.com>
@antonipp
Copy link
Copy Markdown
Contributor Author

Thanks, I completely missed this error!

I did run the required steps before but looks like since #34463 the format for YAML multi-line strings changed so I re-generated the manifests once again from latest main.

@julianwiedmann
Copy link
Copy Markdown
Member

/test

@julianwiedmann julianwiedmann added the area/ipam IP address management, including cloud IPAM label Oct 1, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 1, 2024
@julianwiedmann julianwiedmann added this pull request to the merge queue Oct 2, 2024
Merged via the queue into cilium:main with commit c097c2d Oct 2, 2024
@mvisonneau mvisonneau deleted the ai/static-ip-aws branch October 3, 2024 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/ipam IP address management, including cloud IPAM kind/community-contribution This was a contribution made by a community member. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. 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.

5 participants