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

untar fdi-image as proxy user/group #774

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jhoblitt
Copy link
Contributor

@jhoblitt jhoblitt commented Aug 9, 2022

To prevent fdi-image files from being chown'd on subsequent puppet agent
runs.

@jhoblitt
Copy link
Contributor Author

jhoblitt commented Aug 9, 2022

@ekohl Is it possible to introduce puppet/archive as a dependency? It would be nice to be able to specify a checksum for the fdi-image tarball.

@jhoblitt jhoblitt marked this pull request as draft August 9, 2022 23:02
@jhoblitt jhoblitt force-pushed the feature/fdi-ownership branch 2 times, most recently from ee13da7 to b1e101f Compare August 10, 2022 03:06
@jhoblitt jhoblitt marked this pull request as ready for review August 10, 2022 03:06
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I'm unsure about adding a dependency since I like to keep the dependencies light. On the other hand, I do see the benefit of using checksums.

https://puppet.com/docs/puppet/7/type.html#file-attribute-checksum doesn't appear to be what you want and I can't determine if https://puppet.com/docs/puppet/7/type.html#file-attribute-checksum_value is either.

spec/acceptance/discovery_spec.rb Outdated Show resolved Hide resolved
spec/acceptance/discovery_spec.rb Outdated Show resolved Hide resolved
As this lookup may have useful for other/future acceptance tests.
To prevent fdi-image files from being chown'd on subsequent puppet agent
runs.
@jhoblitt
Copy link
Contributor Author

https://puppet.com/docs/puppet/7/type.html#file-attribute-checksum doesn't appear to be what you want and I can't determine if https://puppet.com/docs/puppet/7/type.html#file-attribute-checksum_value is either.

I'm not sure how to use those params to achieve a digest validation of a downloaded file. I suppose it might be possible to use the path of downloaded file as the source param to a file resource, depending on how the provider is implemented... but it would be a lot easier to use puppet/archive.

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I forgot this for a while. Is there any reason this should be owned by foreman-proxy rather than root?

@jhoblitt
Copy link
Contributor Author

I forgot this for a while. Is there any reason this should be owned by foreman-proxy rather than root?

That is a good question. I don't think foreman tries to download and replace the image file so it could be root.

@jhoblitt
Copy link
Contributor Author

Do you want me to change the ownership?

@ekohl
Copy link
Member

ekohl commented Oct 28, 2022

I'd prefer root. The only downside is that untar runs as root too, but it already does that today so I'd say it's not a step back.

@jhoblitt
Copy link
Contributor Author

I just ran into the current logic not unpackaging the tarball if the fdi-image directory already exists... Any chance we can introduce a dep on the archive mod?

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Something else to consider: in theory it's also possible to build a proper package instead of having to retrieve a tarball. It's actually what we do in downstream and would align much better.

To open that discussion I started theforeman/foreman-packaging#8929.

Comment on lines +27 to +28
).without(
).that_requires('Exec[mkdir -p /tmp]')
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
).without(
).that_requires('Exec[mkdir -p /tmp]')
).that_requires('Exec[mkdir -p /tmp]')

@jhoblitt
Copy link
Contributor Author

I general prefer using the package manager, when possible, but that is a bit of a heavy weight solution... I think we need to the ability to download images, at least for testing.

@kenyon
Copy link

kenyon commented Nov 30, 2023

I just ran into the current logic not unpackaging the tarball if the fdi-image directory already exists... Any chance we can introduce a dep on the archive mod?

I just ran into this as well when trying to update the FDI tar file. It's because of the creates:

creates => "${tftp_root_clean}/boot/fdi-image/initrd0.img",

Seems like the creates should be removed, and the exec should be refreshonly. Did this in #822.

kenyon added a commit to kenyon/puppet-foreman_proxy that referenced this pull request Dec 1, 2023
Previously, if you change the image_name (such as for updating the
Foreman Discovery Image version), the new tar file would be downloaded
but not untarred because the content of the previous tar file would be
there and pass the `creates` check.

If we use `refreshonly` instead of `creates`, the untar exec will be
executed when the tar file is updated, as expected.

See also: theforeman#774 (comment)
kenyon added a commit to kenyon/puppet-foreman_proxy that referenced this pull request Dec 1, 2023
Previously, if you change the image_name (such as for updating the
Foreman Discovery Image version), the new tar file would be downloaded
but not untarred because the content of the previous tar file would be
there and pass the `creates` check.

If we use `refreshonly` instead of `creates`, the untar exec will be
executed when the tar file is updated, as expected.

See also: theforeman#774 (comment)
kenyon added a commit to kenyon/puppet-foreman_proxy that referenced this pull request Apr 4, 2024
Previously, if you change the image_name (such as for updating the
Foreman Discovery Image version), the new tar file would be downloaded
but not untarred because the content of the previous tar file would be
there and pass the `creates` check.

If we use `refreshonly` instead of `creates`, the untar exec will be
executed when the tar file is updated, as expected.

See also: theforeman#774 (comment)
kenyon added a commit to kenyon/puppet-foreman_proxy that referenced this pull request Jun 7, 2024
Previously, if you change the image_name (such as for updating the
Foreman Discovery Image version), the new tar file would be downloaded
but not untarred because the content of the previous tar file would be
there and pass the `creates` check.

If we use `refreshonly` instead of `creates`, the untar exec will be
executed when the tar file is updated, as expected.

See also: theforeman#774 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants