Bug 2078531: add an option expose all endpoints on plain HTTP port (#71) #74

vrutkovs · 2022-05-02T06:56:16Z

This re-introduces #71 with a few changes:

moved internal/services to pkg/services so that this could be reused in assisted-service
renamed Init in New
start http handler on httpsPort if no certs are passed and httpPort is not set
adds a middleware to make sure we serve just pxe-initrd via http in http+https mode

vrutkovs · 2022-05-02T06:56:22Z

/test all

openshift-ci · 2022-05-02T06:56:26Z

@vrutkovs: This pull request references Bugzilla bug 2078531, which is valid. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (rhacm-2.6) matches configured target release for branch (rhacm-2.6)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 2078531: add an option expose all endpoints on plain HTTP port (#71)

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

vrutkovs · 2022-05-02T07:16:09Z

This passes locally :/

/test test

codecov · 2022-05-02T07:18:59Z

Codecov Report

Merging #74 (8444886) into main (95f275c) will increase coverage by 1.07%.
The diff coverage is 84.31%.

❗ Current head 8444886 differs from pull request most recent head 6ea164a. Consider uploading reports for the commit 6ea164a to get more accurate results

@@            Coverage Diff             @@
##             main      #74      +/-   ##
==========================================
+ Coverage   66.92%   68.00%   +1.07%     
==========================================
  Files          16       17       +1     
  Lines         774      825      +51     
==========================================
+ Hits          518      561      +43     
- Misses        160      165       +5     
- Partials       96       99       +3

Impacted Files	Coverage Δ
pkg/servers/servers.go	`80.48% <80.48%> (ø)`
internal/handlers/middleware.go	`100.00% <100.00%> (ø)`

vrutkovs · 2022-05-02T09:41:38Z

/hold

until openshift/release#28257 adds subsystem-aws test

openshift-ci · 2022-05-02T09:42:54Z

@vrutkovs: This pull request references Bugzilla bug 2078531, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (rhacm-2.6) matches configured target release for branch (rhacm-2.6)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 2078531: add an option expose all endpoints on plain HTTP port (#71)

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

carbonin

Can you also add a test case in servers_test.go for the behavior that prompted the revert? So "https" port provided, but no certs should start just the http server on the https port.

internal/handlers/middleware.go

pkg/servers/servers.go

main.go

vrutkovs · 2022-05-03T14:43:48Z

/refresh
/skip
/retest

carbonin · 2022-05-04T18:26:06Z

Since we're not setting the certs, would it be more correct to also change LISTEN_PORT to HTTP_LISTEN_PORT in the template that deploys to the SaaS?

Ideally we'll make this change everywhere we're not explicitly using https and we should then be able to simplify the logic when we start the server, right?

carbonin · 2022-05-04T18:28:05Z

Can you also update the configuration section of the README?

…penshift#71) * main: add an option expose all endpoints on plain HTTP port IPXE flow requires artifacts to be available via HTTP. This change would ensure that if `LISTEN_HTTP_PORT` env var is set and HTTPS cert/key are set, the same endpoints are exposed on plain HTTP port as well * main: shutdown http/https servers gracefully * main: move server handling into internal/servers

openshift-ci · 2022-05-05T11:22:01Z

@vrutkovs: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/test	`d0dbfe3`	link	true	`/test test`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

This lets users inject additional middleware conditionally

When both HTTPS and HTTP handlers are running only pxe-initrd can be fetched via HTTP

vrutkovs · 2022-05-06T06:29:15Z

/test e2e-metal-assisted

openshift-ci · 2022-05-06T06:29:18Z

@vrutkovs: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test edge-e2e-metal-assisted-deploy-nodes
/test edge-images
/test edge-lint
/test edge-test
/test images

The following commands are available to trigger optional jobs:

/test edge-e2e-metal-assisted

Use /test all to run the following jobs that were automatically triggered:

pull-ci-openshift-assisted-image-service-main-edge-e2e-metal-assisted-deploy-nodes
pull-ci-openshift-assisted-image-service-main-edge-images
pull-ci-openshift-assisted-image-service-main-edge-lint
pull-ci-openshift-assisted-image-service-main-edge-test
pull-ci-openshift-assisted-image-service-main-images

In response to this:

/test e2e-metal-assisted

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

vrutkovs · 2022-05-06T06:29:35Z

/test edge-e2e-metal-assisted

carbonin · 2022-05-06T12:20:41Z

/unhold

openshift-ci · 2022-05-06T12:21:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: carbonin, vrutkovs

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [carbonin]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2022-05-06T12:23:20Z

@vrutkovs: Some pull requests linked via external trackers have merged:

The following pull requests linked via external trackers have not merged:

openshift/assisted-service#3705 is open

These pull request must merge or be unlinked from the Bugzilla bug in order for it to move to the next state. Once unlinked, request a bug refresh with /bugzilla refresh.

Bugzilla bug 2078531 has not been moved to the MODIFIED state.

In response to this:

Bug 2078531: add an option expose all endpoints on plain HTTP port (#71)

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci bot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels May 2, 2022

openshift-ci bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 2, 2022

openshift-ci bot requested review from eliorerz and jakub-dzon May 2, 2022 06:57

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 2, 2022

vrutkovs force-pushed the ipxe-server-fixups branch 2 times, most recently from 8e71b17 to 1caf5ee Compare May 2, 2022 11:13

carbonin reviewed May 2, 2022

View reviewed changes

internal/handlers/middleware.go Show resolved Hide resolved

internal/handlers/middleware.go Outdated Show resolved Hide resolved

pkg/servers/servers.go Outdated Show resolved Hide resolved

main.go Show resolved Hide resolved

vrutkovs force-pushed the ipxe-server-fixups branch 5 times, most recently from 76a38f4 to d0dbfe3 Compare May 2, 2022 13:39

carbonin reviewed May 2, 2022

View reviewed changes

main.go Show resolved Hide resolved

vrutkovs force-pushed the ipxe-server-fixups branch from d0dbfe3 to 5292763 Compare May 3, 2022 15:12

carbonin mentioned this pull request May 4, 2022

Bug 2078531: Expose ipxe boot artifacts via HTTP openshift/assisted-service#3705

Merged

20 tasks

vrutkovs force-pushed the ipxe-server-fixups branch from 2f4b240 to 8444886 Compare May 5, 2022 11:06

pkg/servers: Split ListenAndServe from New

7e5ff1d

This lets users inject additional middleware conditionally

internal/handlers: allow only pxe-initrd to be served via HTTP

6ea164a

When both HTTPS and HTTP handlers are running only pxe-initrd can be fetched via HTTP

vrutkovs force-pushed the ipxe-server-fixups branch from 8444886 to 6ea164a Compare May 5, 2022 11:30

carbonin approved these changes May 6, 2022

View reviewed changes

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 6, 2022

openshift-ci bot assigned carbonin May 6, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 6, 2022

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 6, 2022

openshift-merge-robot merged commit 2f689a1 into openshift:main May 6, 2022

This was referenced May 23, 2022

Bug 2089195: [release-ocm-2.5] add an option expose all endpoints on plain HTTP port #77

Merged

Bug 2089195: [release-ocm-2.5] add an option expose all endpoints on plain HTTP port" #79

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 2078531: add an option expose all endpoints on plain HTTP port (#71) #74

Bug 2078531: add an option expose all endpoints on plain HTTP port (#71) #74

vrutkovs commented May 2, 2022 •

edited

vrutkovs commented May 2, 2022

openshift-ci bot commented May 2, 2022

vrutkovs commented May 2, 2022

codecov bot commented May 2, 2022 •

edited

vrutkovs commented May 2, 2022

openshift-ci bot commented May 2, 2022

carbonin left a comment

vrutkovs commented May 3, 2022

carbonin commented May 4, 2022 •

edited

carbonin commented May 4, 2022

openshift-ci bot commented May 5, 2022 •

edited

vrutkovs commented May 6, 2022

openshift-ci bot commented May 6, 2022

vrutkovs commented May 6, 2022

carbonin commented May 6, 2022

openshift-ci bot commented May 6, 2022

openshift-ci bot commented May 6, 2022

Bug 2078531: add an option expose all endpoints on plain HTTP port (#71) #74

Bug 2078531: add an option expose all endpoints on plain HTTP port (#71) #74

Conversation

vrutkovs commented May 2, 2022 • edited

vrutkovs commented May 2, 2022

openshift-ci bot commented May 2, 2022

vrutkovs commented May 2, 2022

codecov bot commented May 2, 2022 • edited

Codecov Report

vrutkovs commented May 2, 2022

openshift-ci bot commented May 2, 2022

carbonin left a comment

Choose a reason for hiding this comment

vrutkovs commented May 3, 2022

carbonin commented May 4, 2022 • edited

carbonin commented May 4, 2022

openshift-ci bot commented May 5, 2022 • edited

vrutkovs commented May 6, 2022

openshift-ci bot commented May 6, 2022

vrutkovs commented May 6, 2022

carbonin commented May 6, 2022

openshift-ci bot commented May 6, 2022

openshift-ci bot commented May 6, 2022

vrutkovs commented May 2, 2022 •

edited

codecov bot commented May 2, 2022 •

edited

carbonin commented May 4, 2022 •

edited

openshift-ci bot commented May 5, 2022 •

edited