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.

Sign up for GitHub

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

Jump to bottom

Restrict the number of IPs an ExternalWorkload can have #12026

Merged
merged 1 commit into from
Feb 2, 2024
Merged

Restrict the number of IPs an ExternalWorkload can have #12026

merged 1 commit into from
Feb 2, 2024

Conversation

mateiidavid
Copy link
Member

@mateiidavid mateiidavid commented Feb 1, 2024 •
edited
Loading

A Kubernetes pod may be assigned at most one IP address for each supported protocol (i.e. IPv6 and IPv4), without the use of specialised CNIs or network configurations. When processing addresses in an endpoint, we will only ever use one address.

ExternalWorkload resources have a generic workloadIPs field that allow any number of addresses to be added. We want the behaviour to be similar to a pod -- only one address (of each protcol) should be used for routing.

We restrict the CRD server-side validation to allow only one IP address. Since we do not yet support IPv6, this will ensure that two IPv4 addresses will not be declared by the same workload. Once IPv6 support lands, or once we have a dedicated validator, we can relax the CRD validation.

How to test

  • Install Linkerd after building the branch (just the crds will do, linkerd install --crds).
  • Try to apply the following CRD:
apiVersion: workload.linkerd.io/v1alpha1
kind: ExternalWorkload
metadata:
  labels:
    app: legacy
  name: external-workload-invalid
  namespace: mixed-env
spec:
  meshTls:
    identity: spiffe://root.linkerd.cluster.local/external-workload-invalid
    serverName: external-workload-invalid.cluster.local
  ports:
  - name: http
    port: 80
    protocol: TCP
  workloadIPs:
  - ip: 172.22.0.5
  - ip: 172.22.0.6
status:
  conditions:
  - lastTransitionTime: "2024-01-24T11:53:43Z"
    message: This workload is alive
    reason: Alive
    status: "True"
    type: Ready
  • Expect the creation to fail

The ExternalWorkload "external-workload-invalid" is invalid: spec.workloadIPs: Too many: 2: must have at most 1 items

Edit: going to open up a separate PR for the refactor.

@mateiidavid mateiidavid requested a review from a team as a code owner February 1, 2024 15:25
zaharidichev
zaharidichev reviewed Feb 1, 2024
View reviewed changes
Copy link
Member

@zaharidichev zaharidichev 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 overall, left some suggestions. Would be useful to head what @adleong thinks about all that

controller/api/destination/watcher/endpoints_watcher.go Outdated Show resolved Hide resolved
controller/api/destination/watcher/endpoints_watcher_test.go Outdated Show resolved Hide resolved
@mateiidavid
Restrict the number of IPs an ExternalWorkload can have
33cd970 
A Kubernetes pod may be assigned at [most one IP address][pod-docs]
for each supported protocol (i.e. IPv6 and IPv4), without the use of
specialised CNIs or network configurations. When processing
addresses in an endpoint, we will only ever use one address.

ExternalWorkload resources have a generic workloadIPs field that
allow any number of addresses to be added. We want the behaviour to
be similar to a pod -- only one address (of each protcol) should be
used for routing.

We restrict the CRD server-side validation to allow only one IP
address. Since we do not yet support IPv6, this will ensure that two
IPv4 addresses will not be declared by the same workload. Once IPv6
support lands, or once we have a dedicated validator, we can relax
the CRD validation.

[pod-docs]:  https://pkg.go.dev/k8s.io/[email protected]/pkg/apis/core#PodStatus

Signed-off-by: Matei David <[email protected]>
@zaharidichev
Copy link
Member

As discussed I think we should decouple these two changes. Lets get the CRD change in and worry about simplifying the destinations service code in a follow-up PR.

@mateiidavid mateiidavid force-pushed the matei/crd-ip-validation branch from 8ff0766 to 33cd970 Compare February 1, 2024 16:56
@alpeb alpeb merged commit d820a23 into main Feb 2, 2024
31 checks passed
@alpeb alpeb deleted the matei/crd-ip-validation branch February 2, 2024 13:20
alpeb added a commit that referenced this pull request Feb 2, 2024
@alpeb
Change notes for edge-24.2.1
6748153 
This edge release contains performance and stability improvements to the
Destination controller, and continues stabilizing support for ExternalWorkloads.

* Reduced the load on the Destination controller by only processing Server
  updates on workloads affected by the Server ([#12017])
* Changed how the Destination controller reacts to target clusters (in
  multicluster pod-to-pod mode) whose Server CRD is outdated: skip them and log
  an error instead of panicking ([#12008])
* Improved the leader election of the ExternalWorkloads Endpoints controller to
  avoid missing events ([#12021])
* Improved naming of EndpointSlices generated by ExternWorkloads ([#12016])
* Restriced the number of IPs an ExternalWorkload can have ([#12026])
@alpeb alpeb mentioned this pull request Feb 2, 2024
Merged
alpeb added a commit that referenced this pull request Feb 2, 2024
@alpeb
Change notes for edge-24.2.1 (#12029)
bcbcf43 
This edge release contains performance and stability improvements to the
Destination controller, and continues stabilizing support for ExternalWorkloads.

* Reduced the load on the Destination controller by only processing Server
  updates on workloads affected by the Server ([#12017])
* Changed how the Destination controller reacts to target clusters (in
  multicluster pod-to-pod mode) whose Server CRD is outdated: skip them and log
  an error instead of panicking ([#12008])
* Improved the leader election of the ExternalWorkloads Endpoints controller to
  avoid missing events ([#12021])
* Improved naming of EndpointSlices generated by ExternWorkloads ([#12016])
* Restriced the number of IPs an ExternalWorkload can have ([#12026])
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zaharidichev zaharidichev zaharidichev approved these changes

@adleong adleong adleong approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mateiidavid @zaharidichev @adleong @alpeb