Skip to content

Conversation

@ekohl
Copy link
Member

@ekohl ekohl commented May 11, 2025

What changes are you introducing?

Why are you introducing these changes? (Explanation, links to references, issues, etc.)

In both the "Enabling connections from a client to {ProjectServer}" and "Enabling connections from {ProjectServer} and clients to a {SmartProxyServer}" procedures there is a --add-service=tftp line so users are already told to open up port 69 from clients.

Then the specific part of allowing it through NAT is not something we support anywhere.

Anything else to add? (Considerations, potential downsides, alternative solutions you have explored, etc.)

This came up because of https://github.com/theforeman/foreman-documentation/pull/3851/files#r2083388803, but already in #3530 and #3839 I looked at the content and had my questions about it.

Checklists

  • I am okay with my commits getting squashed when you merge this PR.
  • I am familiar with the contributing guidelines.

Please cherry-pick my commits into:

  • Foreman 3.14/Katello 4.16
  • Foreman 3.13/Katello 4.15 (EL9 only)
  • Foreman 3.12/Katello 4.14 (Satellite 6.16; orcharhino 7.2 on EL9 only)
  • Foreman 3.11/Katello 4.13 (orcharhino 6.11 on EL8 only; orcharhino 7.0 on EL8+EL9; orcharhino 7.1 with Leapp)
  • Foreman 3.10/Katello 4.12
  • Foreman 3.9/Katello 4.11 (Satellite 6.15; orcharhino 6.8/6.9/6.10)
  • Foreman 3.8/Katello 4.10
  • Foreman 3.7/Katello 4.9 (Satellite 6.14)
  • We do not accept PRs for Foreman older than 3.7.

In both the "Enabling connections from a client to {ProjectServer}" and
"Enabling connections from {ProjectServer} and clients to a
{SmartProxyServer}" procedures there is a --add-service=tftp line so
users are already told to open up port 69 from clients.

Then the specific part of allowing it through NAT is not something we
support anywhere.
@github-actions github-actions bot added Needs tech review Requires a review from the technical perspective Needs style review Requires a review from docs style/grammar perspective Needs testing Requires functional testing labels May 11, 2025
@ekohl ekohl mentioned this pull request May 11, 2025
10 tasks
@aneta-petrova aneta-petrova added style review done No issues from docs style/grammar perspective and removed Needs style review Requires a review from docs style/grammar perspective Needs testing Requires functional testing labels May 12, 2025
@aneta-petrova
Copy link
Member

Style-wise no issues and there is nothing to test.

I'm still not sure what to do for tech review in these cases. On one hand, it would be good to get a second opinion to confirm that the procedure is indeed undesirable to keep, on the other hand, when it's a writer submitting a PR, we only require one engineer's opinion for tech review, not two.

@ekohl
Copy link
Member Author

ekohl commented May 12, 2025

@evgeni could you have a look?

@evgeni
Copy link
Member

evgeni commented May 12, 2025

I like diffs with negative line counts, so am happy to provide my ACK here :)

@maximiliankolb maximiliankolb added tech review done No issues from the technical perspective and removed Needs tech review Requires a review from the technical perspective labels May 12, 2025
@Lennonka Lennonka merged commit 9e7fa3f into theforeman:master May 15, 2025
10 of 11 checks passed
@ekohl ekohl deleted the drop-tftp-through-nat-procedure branch May 15, 2025 15:15
@Lennonka
Copy link
Contributor

@ekohl Since #3851 has been cherry picked down to 3.12, do you think it would be okay to cherry pick this one as well?

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

Labels

style review done No issues from docs style/grammar perspective tech review done No issues from the technical perspective

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants