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

Fix RPMs using a too-new version of glibc #11008

Merged
merged 3 commits into from
Mar 10, 2022
Merged

Conversation

fheinecke
Copy link
Contributor

Fixed an issue where RPMs would contain artifacts built against a newer version of glibc than CentOS and RHEL support. This is a fix for #10686. This is primarily an update to dronegen that formalizes the v8 version of this fix (c0a1e07).

@github-actions github-actions bot requested a review from zmb3 March 9, 2022 21:24
@@ -175,7 +176,11 @@ func tagPipelines() []pipeline {

// RPM/DEB package builds
for _, packageType := range []string{rpmPackage, debPackage} {
ps = append(ps, tagPackagePipeline(packageType, buildType{os: "linux", arch: arch, fips: fips}))
bt := buildType{os: "linux", arch: arch, fips: fips}
if packageType == "rpm" && arch == "amd64" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add a comment here explaining why we're doing this. Similar to the one in build-package.sh.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I add one here or at #R291 ? This simply sets the field value (which could be used anywhere) while R291-R295 does something with it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think here is appropriate since we want to explain why we're setting the centos7 flag when building x8664 rpms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Come to think of it I don't know that we should be checking the architecture here. The original fix only covered amd64, but I don't see any reason why the issue wouldn't affect i386 and ARM as well. That being said, I don't have an easy way to test this. Do you have any thoughts here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good question. I imagine those are less commonly used than amd64 but we'd probably fix them too. Let's do amd64 first so we can fix the most common use case and then follow up with ARM/32-bit (don't know if anyone's actually using it TBH). Can you use AWS account to spin up proper CentOS 7 boxes for testing? They should have ARM boxes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, looking at our downloads page, it doesn't look like we actually provide CentOS 7 compatible ARM binaries at all currently.

Copy link
Contributor Author

@fheinecke fheinecke Mar 10, 2022

Choose a reason for hiding this comment

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

I can test 64 bit ARM on AWS pretty easily, but as far as I am aware there are not any 32 bit CentOS 7 or RHEL AMIs available on AWS. This makes it non-trivial to test for 32 bit issues. That being said, if the issue persists with 64 bit ARM it probably affects i386 and ARMv7 as well.

@r0mant r0mant mentioned this pull request Mar 9, 2022
38 tasks
@@ -175,7 +176,11 @@ func tagPipelines() []pipeline {

// RPM/DEB package builds
for _, packageType := range []string{rpmPackage, debPackage} {
ps = append(ps, tagPackagePipeline(packageType, buildType{os: "linux", arch: arch, fips: fips}))
bt := buildType{os: "linux", arch: arch, fips: fips}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
bt := buildType{os: "linux", arch: arch, fips: fips}
bt := buildType{os: "linux", arch: arch, fips: fips, centos7: packageType == "rpm"}
  1. You can inline this without the need for a conditional.
  2. Do we need to check arch? If package type is RPM I think it's safe to assume we should use the CentOS builds.

Copy link
Contributor

@wadells wadells left a comment

Choose a reason for hiding this comment

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

LGTM.

Aside from the 32 bit/arm concerns discussed above, only question is: How did you test the changes?

@r0mant r0mant enabled auto-merge (squash) March 10, 2022 01:45
@r0mant r0mant merged commit 24626ac into branch/v9 Mar 10, 2022
@r0mant r0mant deleted the fred/rpm-centos7-fix branch March 10, 2022 02:08
@fheinecke
Copy link
Contributor Author

Aside from the 32 bit/arm concerns discussed above, only question is: How did you test the changes?

I stood up an amd64 CentOS 7 box in EC2 (ami-00f8e2c955f7ffa9b) and verified that the commands in the package would run. Previously they would fail to execute because the dynamic linker could not find a matching library for glibc. I did not test any specific functionality outside of teleport/tsh/tctl version.

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.

6 participants