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.

Sign up for GitHub

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

Jump to bottom

reorganize the internal structure #428

Merged
merged 1 commit into from
Aug 15, 2023
Merged

reorganize the internal structure #428

merged 1 commit into from
Aug 15, 2023

Conversation

Rozzii
Copy link
Member

@Rozzii Rozzii commented Aug 4, 2023 •
edited
Loading

This commit:

  • provides a more strict access controll for the httpd ditectories
  • blocks access to the ftp directories via http paths
  • removes unused legacy code (mainly related to ignition and coreOS)
  • where possible 'sed' commands have been removed and replaced with jinja templates
  • httpd configuration has been modifed to better utilize the modular nature of apache configuration files
  • environment variable controlled switch has been introduced to the allow/block serving /shared/html/images when "runhttpd" is in use

This PR is also a preparatory work before image-server basic auth and iPXE hardening is introduced.

@metal3-io metal3-io deleted a comment from metal3-io-bot Aug 4, 2023
@metal3-io-bot metal3-io-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 4, 2023
@Rozzii
Copy link
Member Author

Rozzii commented Aug 4, 2023

/test-ubuntu-integration-main

@Rozzii Rozzii mentioned this pull request Aug 4, 2023
Closed
@elfosardo
Copy link
Member

/test-centos-integration-main

elfosardo
elfosardo reviewed Aug 4, 2023
View reviewed changes
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
@metal3-io metal3-io deleted a comment from metal3-io-bot Aug 4, 2023
@metal3-io metal3-io deleted a comment from metal3-io-bot Aug 4, 2023
@Rozzii
Copy link
Member Author

Rozzii commented Aug 4, 2023

/test-ubuntu-integration-main
/test-centos-integration-main

@Rozzii Rozzii requested a review from elfosardo August 4, 2023 16:18
@Rozzii
Copy link
Member Author

Rozzii commented Aug 7, 2023

/test-ubuntu-integration-main

@Rozzii
Copy link
Member Author

Rozzii commented Aug 7, 2023

/cc @dtantsur @iurygregory @lentzi90

@Rozzii
Copy link
Member Author

Rozzii commented Aug 7, 2023

/hold

@metal3-io-bot metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 7, 2023
@Rozzii
Copy link
Member Author

Rozzii commented Aug 7, 2023

/test-ubuntu-integration-main
/test-centos-integration-main

@Rozzii
Copy link
Member Author

Rozzii commented Aug 7, 2023

/hold cancel

@metal3-io-bot metal3-io-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 7, 2023
@metal3-io metal3-io deleted a comment from metal3-io-bot Aug 7, 2023
@elfosardo
Copy link
Member

/approve

@metal3-io-bot metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 7, 2023
@metal3-io metal3-io deleted a comment from metal3-io-bot Aug 8, 2023
@Rozzii Rozzii mentioned this pull request Aug 8, 2023
Merged
dtantsur
dtantsur reviewed Aug 9, 2023
View reviewed changes
README.md Outdated Show resolved Hide resolved
dtantsur
dtantsur reviewed Aug 9, 2023
View reviewed changes
ironic-config/httpd.conf.j2

LogLevel warn

<IfModule log_config_module>
Copy link
Member

Choose a reason for hiding this comment

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

Everything below this line seems to be repeating the defaults or at least stuff that is not metal3-specific. Do we really need it?

Copy link
Member Author

@Rozzii Rozzii Aug 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I have no strong option about that, I am happy to change it if it is useless, I just copied everything over from the previous config that were not modified by the "sed"s

Copy link
Member Author

Choose a reason for hiding this comment

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

I was looking into these some of these are not hard coded defaults, I think neither the logging nor the mime types are the hard coded defaults.

When I have more time I could go through this file and remove options that match the hard coded defaults. I will also turn EnableSendfile on configurable in a followup anyways.

But for now I would like to merge this and not delay the merger further by introducing test failures that might result from deleting something here.

kashifest
kashifest reviewed Aug 11, 2023
View reviewed changes
Copy link
Member

@kashifest kashifest left a comment

Choose a reason for hiding this comment

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

lgtm
One question inline.

Dockerfile
COPY ironic-config/httpd-modules.conf /etc/httpd/conf.modules.d/
COPY ironic-config/apache2-ironic-api.conf.j2 /etc/httpd-ironic-api.conf.j2
Copy link
Member

Choose a reason for hiding this comment

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

so this configuration file is not needed anymore?

Copy link
Member 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 up a bit to L49 https://github.com/metal3-io/ironic-image/pull/428/files#diff-dd2c0eb6ea5cfc6c4bd4eac30934e2d5746747af48fef6da689e85b752f39557R49

tuminoid
tuminoid reviewed Aug 11, 2023
View reviewed changes
Copy link
Member

@tuminoid tuminoid left a comment

Choose a reason for hiding this comment

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

One typo, plus the servername pointed out earlier is still to be fixed.

README.md Outdated Show resolved Hide resolved
@Rozzii
Copy link
Member Author

Rozzii commented Aug 11, 2023

/test-ubuntu-integration-main

@Rozzii
reorganize the internal structure
4eb43fa 
This commit:
  - provides a more strict access controll for the httpd ditectories
  - blocks access to the ftp directories via http paths
  - removes unused legacy code (mainly related to ignition and coreOS)
  - where possible 'sed' commands have been removed and replaced with
    jinja templates
  - httpd configuration has been modifed to better utilize the
    modular nature of apache configuration files
  - environment variable controlled switch has been introduced to
    the allow/block serving /shared/html/images when "runhttpd" is
    in use
@Rozzii
Copy link
Member Author

Rozzii commented Aug 11, 2023

/test-ubuntu-integration-main

@metal3-io metal3-io deleted a comment from metal3-io-bot Aug 11, 2023
lentzi90
lentzi90 reviewed Aug 15, 2023
View reviewed changes
Copy link
Member

@lentzi90 lentzi90 left a comment

Choose a reason for hiding this comment

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

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 15, 2023
@metal3-io-bot metal3-io-bot merged commit ce6cce1 into metal3-io:main Aug 15, 2023
@Rozzii Rozzii deleted the reorganize_adam branch October 5, 2023 06:44
@hardys hardys mentioned this pull request Oct 6, 2023
Merged
iurygregory pushed a commit to iurygregory/ironic-image that referenced this pull request Nov 28, 2023
@openshift-merge-bot
Merge pull request metal3-io#428 from iurygregory/firmware-interface
a41fa42 
OCPBUGS-23939: Firmware interface
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dtantsur dtantsur dtantsur left review comments

@tuminoid tuminoid tuminoid left review comments

@lentzi90 lentzi90 lentzi90 left review comments

@kashifest kashifest kashifest left review comments

@elfosardo elfosardo Awaiting requested review from elfosardo

@iurygregory iurygregory Awaiting requested review from iurygregory

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
Metal3 - Roadmap
Status: Done / Closed
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Rozzii @elfosardo @dtantsur @tuminoid @lentzi90 @kashifest @metal3-io-bot