Skip to content

[iproute2]: Add macsec-xpn-support iproute2 in syncd #8702

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

Merged
merged 6 commits into from
Nov 25, 2021

Conversation

Pterosaur
Copy link
Contributor

@Pterosaur Pterosaur commented Sep 8, 2021

Signed-off-by: Ze Gan [email protected]

Wait for PR: sonic-net/sonic-swss#1970

Why I did it

The iproute2 from apt source is out-of-date to support 256 bits cipher of MACsec and the iproute2 isn't support XPN of MACsec in current version.

How I did it

Install the iproute2 from debian source code and patch the XPN support change.

How to verify it

Run ip link add link eth0 name macsec0 type macsec sci 1 encrypt on cipher gcm-aes-xpn-256 in syncd container without error

Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106

Description for the changelog

A picture of a cute animal (not mandatory but encouraged)

@@ -9,7 +9,7 @@ ENV DEBIAN_FRONTEND=noninteractive

RUN apt-get update

RUN apt-get install -f -y iproute2 libcap2-bin
RUN apt-get install -f -y libbsd0 libcap2-bin
Copy link
Collaborator

@qiluo-msft qiluo-msft Sep 9, 2021

Choose a reason for hiding this comment

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

Is this build dependency? If yes, add some comment.
Alternatively, use apt-get build-dep iproute2 ? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think libbsd0 is a run-time dependency. And I want the original iproute2 from apt source is replaced by this PR. So I remove the original one.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If it is a runtime dependency, is it better to install by apt-get install -f without specifying them one-by-one. In future, the dependency is free to change, and we are still good.

wget -O iproute2_$(IPROUTE2_VERSION_FULL).dsc -N "https://sonicstorage.blob.core.windows.net/packages/iproute2_4.9.0-1.dsc?sv=2015-04-05&sr=b&sig=m6FcMH9dOh8ggipBgOsONiXvDxoi6bfUO%2BxvidsMNMQ%3D&se=2154-10-23T11%3A59%3A53Z&sp=r"
wget -O iproute2_$(IPROUTE2_VERSION_FULL).debian.tar.xz -N "https://sonicstorage.blob.core.windows.net/packages/iproute2_4.9.0-1.debian.tar.xz?sv=2015-04-05&sr=b&sig=U5NFuwG5C3vZXlUUNvoPMnKDtMKk66zbweA9rQYbEVY%3D&se=2154-10-23T12%3A00%3A15Z&sp=r"
dpkg-source -x iproute2_$(IPROUTE2_VERSION_FULL).dsc
git clone https://salsa.debian.org/debian/iproute2.git iproute2-$(IPROUTE2_VERSION)
Copy link
Collaborator

@qiluo-msft qiluo-msft Sep 9, 2021

Choose a reason for hiding this comment

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

Did you ever try source code from debian website, like https://packages.debian.org/buster-backports/iproute2 ? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there any difference? I saw some other components use the source from salsa.debian.org. Which one is recommended?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Debian source code is more similiar to the debian vanilla installation package.

From f07b3b162f23c7159146b4098fb25994e3b55a9d Mon Sep 17 00:00:00 2001
From: Ze Gan <[email protected]>
Date: Mon, 30 Aug 2021 06:45:28 +0000
Subject: [PATCH] MACsec XPN support
Copy link
Collaborator

@qiluo-msft qiluo-msft Sep 9, 2021

Choose a reason for hiding this comment

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

Did you submit this patch to upstream? #Pending

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I just generated this patch from my private repo, is it a right way?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a recommendation. If the feature is useful in general, you may collect more feedback there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I will do that, but I think it will take a long cycle to merge my change to the upstream and backport it to the debian.

@Pterosaur
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@lguohan
Copy link
Collaborator

lguohan commented Nov 11, 2021

/azp run

@azure-pipelines
Copy link

You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list.

@lguohan
Copy link
Collaborator

lguohan commented Nov 11, 2021

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

pkg-config
pkg-config \
# For iproute2
libbpf-dev=1:0.3-2~bpo10+1 \
Copy link
Collaborator

@qiluo-msft qiluo-msft Nov 12, 2021

Choose a reason for hiding this comment

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

libbpf-dev=1:0.3-2~bpo10+1 \

Can we use apt-get install -t buster-backports libbpf-dev?
The benefit is it could follow the backport sources and catch any security update. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion, let me try it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, please review it.

@Pterosaur Pterosaur requested a review from qiluo-msft November 12, 2021 06:05
qiluo-msft
qiluo-msft previously approved these changes Nov 12, 2021
@Pterosaur
Copy link
Contributor Author

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Pterosaur
Copy link
Contributor Author

/azp run Azure.sonic-buildimage (Build vs),Azure.sonic-buildimage (Build mellanox),Azure.sonic-buildimage (Build broadcom),Azure.docker-slave-bullseye (Build Build_bullseye_arm64),Azure.docker-slave-bullsey

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

wget -O iproute2_$(IPROUTE2_VERSION_FULL).debian.tar.xz -N "https://sonicstorage.blob.core.windows.net/packages/iproute2_4.9.0-1.debian.tar.xz?sv=2015-04-05&sr=b&sig=U5NFuwG5C3vZXlUUNvoPMnKDtMKk66zbweA9rQYbEVY%3D&se=2154-10-23T12%3A00%3A15Z&sp=r"
wget -O iproute2_$(IPROUTE2_VERSION).orig.tar.xz http://deb.debian.org/debian/pool/main/i/iproute2/iproute2_$(IPROUTE2_VERSION).orig.tar.xz
wget -O iproute2_$(IPROUTE2_VERSION_FULL).dsc http://deb.debian.org/debian/pool/main/i/iproute2/iproute2_$(IPROUTE2_VERSION_FULL).dsc
wget -O iproute2_$(IPROUTE2_VERSION_FULL).debian.tar.xz http://deb.debian.org/debian/pool/main/i/iproute2/iproute2_$(IPROUTE2_VERSION_FULL).debian.tar.xz
Copy link
Collaborator

Choose a reason for hiding this comment

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

http://deb.debian.org

@xumia, for reproducible build, do we still need to manually backup the downloaded files to sonicstorage?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not necessary, it will upload the package automatically.

@Pterosaur Pterosaur merged commit ada0e50 into sonic-net:master Nov 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants