Skip to content

edns: allow dnsconfd name resolution for kickstart fetching - #6258

Merged
rvykydal merged 2 commits into
rhinstaller:mainfrom
rvykydal:restart-bind-after-fetching-certs
Apr 14, 2025
Merged

edns: allow dnsconfd name resolution for kickstart fetching#6258
rvykydal merged 2 commits into
rhinstaller:mainfrom
rvykydal:restart-bind-after-fetching-certs

Conversation

@rvykydal

@rvykydal rvykydal commented Mar 12, 2025

Copy link
Copy Markdown
Contributor

@rvykydal

Copy link
Copy Markdown
Contributor Author

/build-image

@github-actions github-actions Bot added the f43 label Mar 12, 2025
@github-actions

Copy link
Copy Markdown

Images built based on commit eada098:

  • boot.iso: success

Download the images from the bottom of the job status page.

@rvykydal rvykydal changed the title DBG: mark the installer edns: allow kickstart location resolution by known certificate Mar 13, 2025
Comment thread dracut/anaconda-lib.sh Fixed
@rvykydal
rvykydal force-pushed the restart-bind-after-fetching-certs branch from ec4789b to 11341a5 Compare March 13, 2025 07:30
@rvykydal

Copy link
Copy Markdown
Contributor Author

/build-image

@rvykydal

Copy link
Copy Markdown
Contributor Author

/kickstart-test dns-global-bootopts dns-global-exclusive-tls-2 dns-global-exclusive-tls-httpks-2 dns-global-exclusive-tls-httpks dns-global-exclusive-tls-initramfs dns-global-exclusive-tls-ksnet dns-global-exclusive-tls dns network-dns-search

@github-actions

Copy link
Copy Markdown

Images built based on commit 11341a5:

  • boot.iso: success

Download the images from the bottom of the job status page.

@rvykydal

Copy link
Copy Markdown
Contributor Author

/kickstart-test --force dns-global-bootopts dns-global-exclusive-tls-2 dns-global-exclusive-tls-httpks-2 dns-global-exclusive-tls-httpks dns-global-exclusive-tls-initramfs dns-global-exclusive-tls-ksnet dns-global-exclusive-tls dns network-dns-search

@rvykydal

Copy link
Copy Markdown
Contributor Author

/build-image

@github-actions

Copy link
Copy Markdown

Images built based on commit b08f034:

  • boot.iso: success

Download the images from the bottom of the job status page.

@rvykydal
rvykydal force-pushed the restart-bind-after-fetching-certs branch from b08f034 to f1b567e Compare March 19, 2025 09:33
@rvykydal

Copy link
Copy Markdown
Contributor Author

/kickstart-test --testtype dns

@rvykydal
rvykydal force-pushed the restart-bind-after-fetching-certs branch from f1b567e to e4e0bdd Compare March 19, 2025 09:48
@rvykydal

Copy link
Copy Markdown
Contributor Author

/kickstart-test --testtype dns

@rvykydal
rvykydal force-pushed the restart-bind-after-fetching-certs branch from e4e0bdd to dd71040 Compare April 10, 2025 09:39
@rvykydal

Copy link
Copy Markdown
Contributor Author

/build-image

@github-actions

Copy link
Copy Markdown

Images built based on commit dd71040:

  • boot.iso: success

Download the images from the bottom of the job status page.

@rvykydal
rvykydal force-pushed the restart-bind-after-fetching-certs branch from dd71040 to c7249d1 Compare April 10, 2025 11:55
@rvykydal

Copy link
Copy Markdown
Contributor Author

/kickstart-test --testtype smoke

@rvykydal

Copy link
Copy Markdown
Contributor Author

/kickstart-test --testtype dns

@rvykydal
rvykydal force-pushed the restart-bind-after-fetching-certs branch from c7249d1 to 5e56c15 Compare April 11, 2025 08:48
@rvykydal
rvykydal force-pushed the restart-bind-after-fetching-certs branch from 5e56c15 to 4e70724 Compare April 11, 2025 08:49
@rvykydal rvykydal changed the title edns: allow kickstart location resolution by known certificate edns: allow dnsconfd name resolution for kickstart fetching Apr 11, 2025
Start dnsconfd already before kickstart fetching.

We used to start dnsconfd only after the kickstart was fetched if
kickstart usage was detected so that the potential certificates from
kikcstart are applied. But this mechanism ruled out use case when
dnsconfd (name resolution) is needed for the kickstart fetching. So
start the dnsconfd early and if certificates were fetched restart it (it
is done by restarting unbound service as recommended by dnsconfd).

Resolves: RHEL-82694
We need this guard because kickstart may be fetched over multiple
network devices and it is parsed after each fetching. Parsing itself
should be idempotent (multiple parsing harmless) as it only dumps files
with configuration / options for further actions.

Resolves: RHEL-82694
@rvykydal
rvykydal force-pushed the restart-bind-after-fetching-certs branch from 4e70724 to d98e411 Compare April 11, 2025 08:56
@rvykydal

Copy link
Copy Markdown
Contributor Author

/kickstart-test --testtype dns

@rvykydal

Copy link
Copy Markdown
Contributor Author

/kickstart-test --testtype smoke

@rvykydal
rvykydal marked this pull request as ready for review April 11, 2025 11:20

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hey @rvykydal - I've reviewed your changes - here's some feedback:

Overall Comments:

  • Consider adding more comments to explain the logic behind the different stages in start_dnsconfd.
  • It might be helpful to define the possible values of the stage variable as constants to improve readability.
Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@jkonecny12 jkonecny12 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me.

I just wonder if these string constants can't be translated to something more robust. Something like start_dnsconfd <should import certificates yes/no> <reason>

@rvykydal

Copy link
Copy Markdown
Contributor Author

Looks good to me.

I just wonder if these string constants can't be translated to something more robust. Something like start_dnsconfd <should import certificates yes/no> <reason>

The approach here is to keep all the logic intentionally at single place - in the function. The callers are only passing information where the call comes from.

@jstodola jstodola left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It seems to work fine, thanks.

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