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

Installing node from images.json #203

Merged
merged 19 commits into from
Sep 13, 2024
Merged

Conversation

pacostas
Copy link
Contributor

Summary

This PR selects which node-engine to install based on the node versions that exist on images.json file.

Use Cases

Checklist

  • I have viewed, signed, and submitted the Contributor License Agreement.
  • I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
  • I have added an integration test, if necessary.
  • I have reviewed the styleguide for guidance on my code quality.
  • I'm happy with the commit history on this PR (I have rebased/squashed as needed).

generate_test.go Outdated Show resolved Hide resolved
Comment on lines -26 to -44
type DuringBuildPermissions struct {
CNB_USER_ID, CNB_GROUP_ID int
}

//go:embed templates/build.Dockerfile
var buildDockerfileTemplate string

type BuildDockerfileProps struct {
NODEJS_VERSION uint64
CNB_USER_ID, CNB_GROUP_ID int
CNB_STACK_ID, PACKAGES string
}

//go:embed templates/run.Dockerfile
var runDockerfileTemplate string

type RunDockerfileProps struct {
Source string
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has been moved to utils

Comment on lines -127 to -180
func FillPropsToTemplate(properties interface{}, templateString string) (result string, Error error) {

templ, err := template.New("template").Parse(templateString)
if err != nil {
return "", err
}

var buf bytes.Buffer
err = templ.Execute(&buf, properties)
if err != nil {
panic(err)
}

return buf.String(), nil
}

func GetDuringBuildPermissions(filepath string) DuringBuildPermissions {

defaultPermissions := DuringBuildPermissions{
CNB_USER_ID: DEFAULT_USER_ID,
CNB_GROUP_ID: DEFAULT_GROUP_ID,
}
re := regexp.MustCompile(`cnb:x:(\d+):(\d+)::`)

etcPasswdFile, err := os.ReadFile(filepath)

if err != nil {
return defaultPermissions
}
etcPasswdContent := string(etcPasswdFile)

matches := re.FindStringSubmatch(etcPasswdContent)

if len(matches) != 3 {
return defaultPermissions
}

CNB_USER_ID, err := strconv.Atoi(matches[1])

if err != nil {
return defaultPermissions
}

CNB_GROUP_ID, err := strconv.Atoi(matches[2])

if err != nil {
return defaultPermissions
}

return DuringBuildPermissions{
CNB_USER_ID: CNB_USER_ID,
CNB_GROUP_ID: CNB_GROUP_ID,
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has been moved untouched to utils

Comment on lines -25 to -39
type RunDockerfileProps struct {
Source string
}

//go:embed templates/run.Dockerfile
var runDockerfileTemplate string

type BuildDockerfileProps struct {
NODEJS_VERSION uint64
CNB_USER_ID, CNB_GROUP_ID int
CNB_STACK_ID, PACKAGES string
}

//go:embed templates/build.Dockerfile
var buildDockerfileTemplate string
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has been moved to utils untouched

Comment on lines -93 to -185
func testFetchingPermissionsFromEtchPasswdFile(t *testing.T, context spec.G, it spec.S) {

var (
Expect = NewWithT(t).Expect
tmpDir string
path string
err error
)

context("/etc/passwd exists and has the cnb user", func() {

it("It should return the permissions specified for the cnb user", func() {
tmpDir, err = os.MkdirTemp("", "")
Expect(err).NotTo(HaveOccurred())

path = filepath.Join(tmpDir, "/passwd")

Expect(os.WriteFile(path, []byte(`root:x:0:0:root:/root:/bin/bash
bin:x:1:1:bin:/bin:/sbin/nologin
daemon:x:2:2:daemon:/sbin:/sbin/nologin
adm:x:3:4:adm:/var/adm:/sbin/nologin
lp:x:4:7:lp:/var/spool/lpd:/sbin/nologin
sync:x:5:0:sync:/sbin:/bin/sync
shutdown:x:6:0:shutdown:/sbin:/sbin/shutdown
halt:x:7:0:halt:/sbin:/sbin/halt
mail:x:8:12:mail:/var/spool/mail:/sbin/nologin
operator:x:11:0:operator:/root:/sbin/nologin
games:x:12:100:games:/usr/games:/sbin/nologin
ftp:x:14:50:FTP User:/var/ftp:/sbin/nologin
cnb:x:1234:2345::/home/cnb:/bin/bash
nobody:x:65534:65534:Kernel Overflow User:/:/sbin/nologin
`), 0600)).To(Succeed())

duringBuilderPermissions := ubinodejsextension.GetDuringBuildPermissions(path)

Expect(duringBuilderPermissions).To(Equal(
ubinodejsextension.DuringBuildPermissions{
CNB_USER_ID: 1234,
CNB_GROUP_ID: 2345},
))
})
})

context("/etc/passwd exists and does NOT have the cnb user", func() {

it("It should return the default permissions", func() {
tmpDir, err = os.MkdirTemp("", "")
Expect(err).NotTo(HaveOccurred())

path = filepath.Join(tmpDir, "/passwd")

Expect(os.WriteFile(path, []byte(`root:x:0:0:root:/root:/bin/bash
bin:x:1:1:bin:/bin:/sbin/nologin
daemon:x:2:2:daemon:/sbin:/sbin/nologin
adm:x:3:4:adm:/var/adm:/sbin/nologin
lp:x:4:7:lp:/var/spool/lpd:/sbin/nologin
sync:x:5:0:sync:/sbin:/bin/sync
shutdown:x:6:0:shutdown:/sbin:/sbin/shutdown
halt:x:7:0:halt:/sbin:/sbin/halt
mail:x:8:12:mail:/var/spool/mail:/sbin/nologin
operator:x:11:0:operator:/root:/sbin/nologin
games:x:12:100:games:/usr/games:/sbin/nologin
ftp:x:14:50:FTP User:/var/ftp:/sbin/nologin
nobody:x:65534:65534:Kernel Overflow User:/:/sbin/nologin
`), 0600)).To(Succeed())

duringBuilderPermissions := ubinodejsextension.GetDuringBuildPermissions(path)

Expect(duringBuilderPermissions).To(Equal(
ubinodejsextension.DuringBuildPermissions{
CNB_USER_ID: ubinodejsextension.DEFAULT_USER_ID,
CNB_GROUP_ID: ubinodejsextension.DEFAULT_GROUP_ID},
))
})
})

context("/etc/passwd does NOT exist", func() {

it("It should return the default permissions", func() {
tmpDir, err = os.MkdirTemp("", "")
Expect(err).NotTo(HaveOccurred())

duringBuilderPermissions := ubinodejsextension.GetDuringBuildPermissions(tmpDir)

Expect(duringBuilderPermissions).To(Equal(
ubinodejsextension.DuringBuildPermissions{
CNB_USER_ID: ubinodejsextension.DEFAULT_USER_ID,
CNB_GROUP_ID: ubinodejsextension.DEFAULT_GROUP_ID},
))
})
})
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has been moved to utils_test untouched

Comment on lines -815 to -846
it("Should error in case there are no entries in the buildpack plan.", func() {

extensionToml, _ := readExtensionTomlTemplateFile()

cnbDir, err = os.MkdirTemp("", "cnb")
Expect(err).NotTo(HaveOccurred())
Expect(os.WriteFile(cnbDir+"/extension.toml", []byte(extensionToml), 0600)).To(Succeed())

entriesTests := []struct {
Entries []packit.BuildpackPlanEntry
}{
{
Entries: []packit.BuildpackPlanEntry{},
},
}

for _, tt := range entriesTests {

generateResult, err = generate(packit.GenerateContext{
WorkingDir: workingDir,
CNBPath: cnbDir,
Plan: packit.BuildpackPlan{
Entries: tt.Entries,
},
Stack: "io.buildpacks.stacks.ubi8",
})

Expect(err).To(HaveOccurred())

}

})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed as is exactly the same with this test

it("Node no longer requested in buildplan", func() {
generateResult, err = generate(packit.GenerateContext{
WorkingDir: workingDir,
Plan: packit.BuildpackPlan{
Entries: []packit.BuildpackPlanEntry{},
},
})
Expect(err).To(HaveOccurred())
Expect(generateResult.BuildDockerfile).To(BeNil())
})
}, spec.Sequential())

Comment on lines +193 to +207
func fillPropsToTemplate(properties interface{}, templateString string) (result string, Error error) {

templ, err := template.New("template").Parse(templateString)
if err != nil {
return "", err
}

var buf bytes.Buffer
err = templ.Execute(&buf, properties)
if err != nil {
panic(err)
}

return buf.String(), nil
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved to utils from generate untouched

Comment on lines +1 to +15
package structs

type DuringBuildPermissions struct {
CNB_USER_ID, CNB_GROUP_ID int
}

type BuildDockerfileProps struct {
NODEJS_VERSION uint64
CNB_USER_ID, CNB_GROUP_ID int
CNB_STACK_ID, PACKAGES string
}

type RunDockerfileProps struct {
Source string
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved to structs from generate, untouched

Comment on lines 397 to 489
func testGetDuringBuildPermissions(t *testing.T, context spec.G, it spec.S) {

var (
Expect = NewWithT(t).Expect
tmpDir string
path string
err error
)

context("/etc/passwd exists and has the cnb user", func() {

it("It should return the permissions specified for the cnb user", func() {
tmpDir, err = os.MkdirTemp("", "")
Expect(err).NotTo(HaveOccurred())

path = filepath.Join(tmpDir, "/passwd")

Expect(os.WriteFile(path, []byte(`root:x:0:0:root:/root:/bin/bash
bin:x:1:1:bin:/bin:/sbin/nologin
daemon:x:2:2:daemon:/sbin:/sbin/nologin
adm:x:3:4:adm:/var/adm:/sbin/nologin
lp:x:4:7:lp:/var/spool/lpd:/sbin/nologin
sync:x:5:0:sync:/sbin:/bin/sync
shutdown:x:6:0:shutdown:/sbin:/sbin/shutdown
halt:x:7:0:halt:/sbin:/sbin/halt
mail:x:8:12:mail:/var/spool/mail:/sbin/nologin
operator:x:11:0:operator:/root:/sbin/nologin
games:x:12:100:games:/usr/games:/sbin/nologin
ftp:x:14:50:FTP User:/var/ftp:/sbin/nologin
cnb:x:1234:2345::/home/cnb:/bin/bash
nobody:x:65534:65534:Kernel Overflow User:/:/sbin/nologin
`), 0600)).To(Succeed())

duringBuilderPermissions := utils.GetDuringBuildPermissions(path)

Expect(duringBuilderPermissions).To(Equal(
structs.DuringBuildPermissions{
CNB_USER_ID: 1234,
CNB_GROUP_ID: 2345,
},
))
})
})

context("/etc/passwd exists and does NOT have the cnb user", func() {

it("It should return the default permissions", func() {
tmpDir, err = os.MkdirTemp("", "")
Expect(err).NotTo(HaveOccurred())

path = filepath.Join(tmpDir, "/passwd")

Expect(os.WriteFile(path, []byte(`root:x:0:0:root:/root:/bin/bash
bin:x:1:1:bin:/bin:/sbin/nologin
daemon:x:2:2:daemon:/sbin:/sbin/nologin
adm:x:3:4:adm:/var/adm:/sbin/nologin
lp:x:4:7:lp:/var/spool/lpd:/sbin/nologin
sync:x:5:0:sync:/sbin:/bin/sync
shutdown:x:6:0:shutdown:/sbin:/sbin/shutdown
halt:x:7:0:halt:/sbin:/sbin/halt
mail:x:8:12:mail:/var/spool/mail:/sbin/nologin
operator:x:11:0:operator:/root:/sbin/nologin
games:x:12:100:games:/usr/games:/sbin/nologin
ftp:x:14:50:FTP User:/var/ftp:/sbin/nologin
nobody:x:65534:65534:Kernel Overflow User:/:/sbin/nologin
`), 0600)).To(Succeed())

duringBuildPermissions := utils.GetDuringBuildPermissions(path)

Expect(duringBuildPermissions).To(Equal(
structs.DuringBuildPermissions{
CNB_USER_ID: ubinodejsextension.DEFAULT_USER_ID,
CNB_GROUP_ID: ubinodejsextension.DEFAULT_GROUP_ID},
))
})
})

context("/etc/passwd does NOT exist", func() {

it("It should return the default permissions", func() {
tmpDir, err = os.MkdirTemp("", "")
Expect(err).NotTo(HaveOccurred())

duringBuilderPermissions := utils.GetDuringBuildPermissions(tmpDir)

Expect(duringBuilderPermissions).To(Equal(
structs.DuringBuildPermissions{
CNB_USER_ID: ubinodejsextension.DEFAULT_USER_ID,
CNB_GROUP_ID: ubinodejsextension.DEFAULT_GROUP_ID},
))
})
})
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to utils_test from generate_test untouched.

Comment on lines +334 to +396
func testGenerateBuildDockerfile(t *testing.T, context spec.G, it spec.S) {

var (
Expect = NewWithT(t).Expect
)

context("Adding props on build.dockerfile template", func() {

it("Should fill with properties the template/build.Dockerfile", func() {

output, err := utils.GenerateBuildDockerfile(structs.BuildDockerfileProps{
NODEJS_VERSION: 16,
CNB_USER_ID: 1000,
CNB_GROUP_ID: 1000,
CNB_STACK_ID: "io.buildpacks.stacks.ubi8",
PACKAGES: ubinodejsextension.PACKAGES,
})

Expect(err).NotTo(HaveOccurred())
Expect(output).To(Equal(fmt.Sprintf(`ARG base_image
FROM ${base_image}

USER root

ARG build_id=0
RUN echo ${build_id}

RUN microdnf -y module enable nodejs:16
RUN microdnf --setopt=install_weak_deps=0 --setopt=tsflags=nodocs install -y %s && microdnf clean all

RUN echo uid:gid "1000:1000"
USER 1000:1000

RUN echo "CNB_STACK_ID: io.buildpacks.stacks.ubi8"`, ubinodejsextension.PACKAGES)))

})

})
}

func testGenerateRunDockerfile(t *testing.T, context spec.G, it spec.S) {

var (
Expect = NewWithT(t).Expect
)

context("Adding props on build.dockerfile template", func() {

it("Should fill with properties the template/run.Dockerfile", func() {

RunDockerfileProps := structs.RunDockerfileProps{
Source: "paketocommunity/run-nodejs-18-ubi-base",
}

output, err := utils.GenerateRunDockerfile(RunDockerfileProps)

Expect(err).NotTo(HaveOccurred())
Expect(output).To(Equal(`FROM paketocommunity/run-nodejs-18-ubi-base`))

})
})
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code is the same with the old testfillPropstotemplate

func testFillPropsToTemplate(t *testing.T, context spec.G, it spec.S) {
var (
Expect = NewWithT(t).Expect
)
context("Adding props on templates with FillPropsToTemplate", func() {
it("Should fill with properties the template/build.Dockerfile", func() {
output, err := ubinodejsextension.FillPropsToTemplate(BuildDockerfileProps{
NODEJS_VERSION: 16,
CNB_USER_ID: 1000,
CNB_GROUP_ID: 1000,
CNB_STACK_ID: "",
PACKAGES: ubinodejsextension.PACKAGES,
}, buildDockerfileTemplate)
Expect(err).NotTo(HaveOccurred())
Expect(output).To(Equal(fmt.Sprintf(`ARG base_image
FROM ${base_image}
USER root
ARG build_id=0
RUN echo ${build_id}
RUN microdnf -y module enable nodejs:16
RUN microdnf --setopt=install_weak_deps=0 --setopt=tsflags=nodocs install -y %s && microdnf clean all
RUN echo uid:gid "1000:1000"
USER 1000:1000
RUN echo "CNB_STACK_ID: "`, ubinodejsextension.PACKAGES)))
})
it("Should fill with properties the template/run.Dockerfile", func() {
RunDockerfileProps := RunDockerfileProps{
Source: "paketocommunity/run-nodejs-18-ubi-base",
}
output, err := ubinodejsextension.FillPropsToTemplate(RunDockerfileProps, runDockerfileTemplate)
Expect(err).NotTo(HaveOccurred())
Expect(output).To(Equal(`FROM paketocommunity/run-nodejs-18-ubi-base`))
})
})
}
but it has been splitted into two sections to wrap this function one for the run dockerfile and one for the build dockerfile

Comment on lines -218 to -220
workingDir = t.TempDir()
Expect(err).NotTo(HaveOccurred())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The error check here was wrong, the t.TempDir() does not return any err object, therefore has been deleted. The WorkingDir variable has been moved a little bit down, next to the buildplan creation.

Comment on lines +40 to +63
func GenerateConfigTomlContentFromImagesJson(imagesJsonPath string, stackId string) ([]byte, error) {
imagesJsonData, err := ParseImagesJsonFile(imagesJsonPath)
if err != nil {
return []byte{}, err
}

nodejsStacks, err := GetNodejsStackImages(imagesJsonData)
if err != nil {
return []byte{}, err
}

defaultNodeVersion, err := GetDefaultNodeVersion(nodejsStacks)
if err != nil {
return []byte{}, err
}

configTomlContent, err := CreateConfigTomlFileContent(defaultNodeVersion, nodejsStacks, stackId)
if err != nil {
return []byte{}, err
}

configTomlContentString := configTomlContent.Bytes()
return configTomlContentString, nil
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a wrapper, and for that wrapper on the utils tests we only test that it manages to return the result and that it manages to propagate an error. The type of the error, the type of the result and the edge cases are covered from the tests of the functions that are wrapped.

WithPullPolicy("always").
Execute(name, source)

Expect(err).NotTo(HaveOccurred(), logs.String())
Expect(err).To(HaveOccurred())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The function errors after the extension has run due to it tries to download the fake image nodeRunImage := "this-is-a-run-image". Therefore the build fails, so I changed it to only check the logs and that the build failed instead of checking the logs and checking and that the build that succeeded. If we can find an image that will always be available, we can have also the build to succeed. In any case the extension is tested properly so to me seems like there is no issue of test coverage on this scenario.

@pacostas pacostas marked this pull request as ready for review September 12, 2024 11:54
@pacostas pacostas requested a review from a team as a code owner September 12, 2024 11:54
@pacostas
Copy link
Contributor Author

@mhdawson The tests do not pass due to there is no new ubi-base image with the images.json file in it. If you force the release on the ubi-base-stack, it will generate one, which will update the builder and the tests of this PR will pass.

@mhdawson mhdawson added the semver:major A change requiring a major version bump label Sep 12, 2024
)

var DEFAULT_USER_ID = 1002
var DEFAULT_GROUP_ID = 1000
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if these could be in a common place as we define them in multiple files?

@mhdawson
Copy link
Member

Just one tiny question, otherwise looks good.

Pushed through a release of the stack, will re-run the tests.

@pacostas
Copy link
Contributor Author

pacostas commented Sep 13, 2024

Yes, you are right. On the generate.go these constants are not being used anymore as the getDuringBuildPermissions function has been moved to utils package. Although I moved these constants to a new package called constants as are a bit hidden under the utils package. I can easily revert these changes by moving these constants under the utils package if you think is not on the context of this PR.
Also I added this commit 15c9400 which removes the tmp directory by using the builtin function for making tmp directories which also removes them automatically.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@mhdawson mhdawson merged commit 37c1106 into paketo-community:main Sep 13, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:major A change requiring a major version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants