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

Edns certs stage2 w initramfs r9 #6094

Open
wants to merge 16 commits into
base: rhel-9
Choose a base branch
from

Conversation

rvykydal
Copy link
Contributor

@rvykydal rvykydal commented Jan 14, 2025

Port of #6045 to rhel9.
Port to rhel10 is used as it does not contain updates for module import ordering fixes.
The (minor) modifications of rhel10 port patch:

  • SubmoduleManager is not in rhel-9 yet (minor modification) - rvykydal@c257b4a
  • join_paths utility function in a different module, make_directories not available yet - rvykydal@5567034

TODO

  • update required pykickstart version

@pep8speaks
Copy link

pep8speaks commented Jan 14, 2025

Hello @rvykydal! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2025-01-22 08:56:05 UTC

@rvykydal rvykydal force-pushed the edns-certs-stage2-w-initramfs-r9 branch from b356d48 to 5bd3098 Compare January 14, 2025 14:13
Kickstart %certificate section is used.
Submodule, data structures, parsing.

Resolves: RHEL-61430

Patch modified by rvykydal.
Unlike the %packages section where the data of all sections are merged
into single data object the %certificates section holds the per
instance section data in a list.

Related: INSTALLER-61430
The certificates imported in initramfs are already imported earlier by a
service.

Resolves: RHEL-61430
In case they are needed or processed by package scriptlets

Resolves: RHEL-61430
@rvykydal rvykydal force-pushed the edns-certs-stage2-w-initramfs-r9 branch 2 times, most recently from f77f727 to 1c58b68 Compare January 15, 2025 09:15
@rvykydal rvykydal changed the title [WIP] Edns certs stage2 w initramfs r9 Edns certs stage2 w initramfs r9 Jan 15, 2025
@rvykydal
Copy link
Contributor Author

All the current unit-test failures are due to missing updated pykickstart.

@rvykydal rvykydal marked this pull request as ready for review January 15, 2025 12:04
@rvykydal
Copy link
Contributor Author

Today pykickstart should go into nigthtly compose and we can run kickstart tests then.
Unit tests with pykickstart should be runnable soonish also (pykickstart needs to get into c9s devel repo).

Copy link
Contributor

@M4rtinK M4rtinK 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 to me - just some minor improvement suggestions about how we write out the cert files. :)

@@ -60,9 +67,17 @@ def __init__(self):
self.realm_changed = Signal()
self._realm = RealmData()

def _add_module(self, security_module):
"""Add a base kickstart module."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this correct ? Looks like a copy-paste leftover on the first glance. ;-)

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 just followed the existing code in storage module:

"""Add a base kickstart module."""
.
Since rhel 10 we have SubmoduleManager which provides the method so this patch is specific to rhel-9 branch. That said I'd leave it as it is.

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 the patch

mkdirChain(dst_dir)

dst = join_paths(dst_dir, cert.filename)
with open(dst, 'w') as f:
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should specify the text mode explicitly ? Eq. wt to make it apparent that we are writing text data right now.

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'd suggest this as upstream patch first.

dst_dir, cert.filename)
mkdirChain(dst_dir)

dst = join_paths(dst_dir, cert.filename)
Copy link
Contributor

Choose a reason for hiding this comment

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

What if the file already exists ? While it will most likely write over it, it might be a good idea to log that we are overwriting an existing file.

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 catch, we even have this upstream, I must have somehow missed the patch:
649c6bb

if os.path.exists(dst):
log.warning("Certificate file %s already exists, replacing.", dst)

with open(dst, 'w') as f:
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here - we might want to use 'wt' & log if we overwrite something.

Copy link
Contributor Author

@rvykydal rvykydal Jan 22, 2025

Choose a reason for hiding this comment

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

The logging is there.

Copy link
Member

@jkonecny12 jkonecny12 left a comment

Choose a reason for hiding this comment

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

LGTM

Don't require it on parsing level, don't own a default.

Resolves: RHEL-61430
Also dump for transfer durig switchroot so that the certificates
can be potentially imported early after switchroot by a service.

Resolves: RHEL-61430
For example when --dir is pointing to read-only filesystem.

Resolves: RHEL-61430
@rvykydal rvykydal force-pushed the edns-certs-stage2-w-initramfs-r9 branch from fdaae76 to 86e7913 Compare January 22, 2025 08:55
@rvykydal
Copy link
Contributor Author

I've just added missing upstream patch based on @M4rtinK's review.

@rvykydal
Copy link
Contributor Author

/kickstart-test --testtype smoke

@rvykydal
Copy link
Contributor Author

/kickstart-test --kstest-pr 1358 certificate

1 similar comment
@rvykydal
Copy link
Contributor Author

/kickstart-test --kstest-pr 1358 certificate

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

4 participants