Bug 1990826: routes without TLS are rejected for missing HSTS annotation #240

candita · 2021-08-10T19:26:38Z

Routes without TLS should just be admitted if they are missing HSTS annotation in a domain that requires HSTS annotation.
Error messages should have more information.

openshift-ci · 2021-08-10T19:26:46Z

@candita: This pull request references Bugzilla bug 1990826, which is valid. The bug has been moved to the POST state. 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 (4.9.0) matches configured target release for branch (4.9.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @quarterpin

In response to this:

Bug 1990826: routes without TLS are rejected for missing HSTS annotation

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.

Miciah · 2021-08-11T21:49:01Z

pkg/route/apiserver/admission/requiredrouteannotations/admission.go

+	// Cannot apply HSTS if route is not TLS, but the route is still valid.  Just log a warning.
+	tls := newRoute.Spec.TLS
+	if tls == nil || (tls.Termination != routeapi.TLSTerminationEdge && tls.Termination != routeapi.TLSTerminationReencrypt) {
+		klog.Warningf("HSTS Policy not added for %s, wrong termination type: %v", newRoute.Name, tls)


Does this log a warning on every passthrough or non-TLS route that the plugin observes? That seems a bit noisy, especially as a warning.

It only tries to validate if there is HSTS added or changed:

openshift-apiserver/pkg/route/apiserver/admission/requiredrouteannotations/admission.go

Line 106 in f5941a5

if wants == has && newHSTS == oldHSTS {

That means it logs a warning for each passthrough or non-TLS route that is created, right?

The openshift-apiserver logs for the most recent e2e-aws CI job run for this PR shows these warnings:

W0810 20:18:34.581431 1 admission.go:189] HSTS Policy not added for route-2, wrong termination type: <nil> W0810 20:18:52.278999 1 admission.go:189] HSTS Policy not added for test-oauth-route, wrong termination type: &{passthrough Redirect} W0810 20:19:46.207319 1 admission.go:189] HSTS Policy not added for weightedroute, wrong termination type: <nil> W0810 20:24:01.136944 1 admission.go:189] HSTS Policy not added for test-oauth-route, wrong termination type: &{passthrough Redirect} W0810 20:24:38.956535 1 admission.go:189] HSTS Policy not added for route-1, wrong termination type: <nil> W0810 20:24:39.032518 1 admission.go:189] HSTS Policy not added for route-2, wrong termination type: <nil> W0810 20:25:04.799717 1 admission.go:189] HSTS Policy not added for test-oauth-route, wrong termination type: &{passthrough Redirect} W0810 20:26:22.782223 1 admission.go:189] HSTS Policy not added for test-oauth-route, wrong termination type: &{passthrough Redirect} W0810 20:26:38.298478 1 admission.go:189] HSTS Policy not added for test-oauth-route, wrong termination type: &{passthrough Redirect} W0810 20:28:18.489976 1 admission.go:189] HSTS Policy not added for passthrough-route, wrong termination type: &{passthrough } W0810 20:29:26.733012 1 admission.go:189] HSTS Policy not added for test-oauth-route, wrong termination type: &{passthrough Redirect} W0810 20:29:31.775479 1 admission.go:189] HSTS Policy not added for test-oauth-route, wrong termination type: &{passthrough Redirect} W0810 20:29:58.628117 1 admission.go:189] HSTS Policy not added for test-vjbft, wrong termination type: <nil> W0810 20:30:38.974223 1 admission.go:189] HSTS Policy not added for 0, wrong termination type: <nil> W0810 20:30:39.226531 1 admission.go:189] HSTS Policy not added for 4, wrong termination type: <nil> W0810 20:30:39.541531 1 admission.go:189] HSTS Policy not added for 11, wrong termination type: <nil> W0810 20:30:39.595524 1 admission.go:189] HSTS Policy not added for 12, wrong termination type: <nil> W0810 20:30:45.563139 1 admission.go:189] HSTS Policy not added for test-oauth-route, wrong termination type: &{passthrough Redirect} W0810 20:31:06.028072 1 admission.go:189] HSTS Policy not added for route-1, wrong termination type: <nil> W0810 20:31:06.090396 1 admission.go:189] HSTS Policy not added for route-2, wrong termination type: <nil> W0810 20:31:06.167867 1 admission.go:189] HSTS Policy not added for route-override-domain-1, wrong termination type: <nil> W0810 20:31:06.189040 1 admission.go:189] HSTS Policy not added for 8, wrong termination type: <nil> W0810 20:32:09.658686 1 admission.go:189] HSTS Policy not added for route-override-domain-1, wrong termination type: <nil> W0810 20:33:34.032948 1 admission.go:189] HSTS Policy not added for route, wrong termination type: <nil>

That could be a lot of noise on a cluster with thousands of routes. Would it make sense to skip logging for these routes, or at least reduce it from a warning to info?

That's strange. I wouldn't think there'd be any HSTS annotations in CI yet.

Oh yes, it checks for any newly created route and reports these warnings. It should report those warnings only if HSTS is needed, so I'll look into that.

Per team chat, we will just ignore silently for now, and address missing annotations on routes as route status in https://issues.redhat.com/browse/NE-678

candita · 2021-08-12T22:43:51Z

FAIL: API "openshift-api-available-new-connections" was unreachable during disruption (AWS has a known issue: https://bugzilla.redhat.com/show_bug.cgi?id=1943804) for at least 15s of 1h27m30s (0%):
/test e2e-aws-upgrade

Miciah · 2021-08-16T14:52:56Z

/lgtm

sttts · 2021-08-16T19:05:20Z

pkg/route/apiserver/admission/requiredrouteannotations/admission.go

@@ -108,6 +108,15 @@ func (o *requiredRouteAnnotations) Validate(ctx context.Context, a admission.Att
 		}
 	}

+	newRoute := a.GetObject().(*routeapi.Route)


why not the one 20 lines higher?

openshift-ci · 2021-08-16T23:29:12Z

@candita: This pull request references Bugzilla bug 1990826, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

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

Requesting review from QA contact:
/cc @quarterpin

In response to this:

Bug 1990826: routes without TLS are rejected for missing HSTS annotation

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.

candita · 2021-08-17T02:56:05Z

STEP: ensuring all cluster operators are stable
Aug 17 00:16:54.270: FAIL: Some cluster operators are not ready: image-registry (Degraded=True ImagePrunerJobFailed: ImagePrunerDegraded: Job has reached the specified backoff limit)

/test e2e-aws

quarterpin · 2021-08-17T10:42:18Z

/bugzilla cc-qa

openshift-ci · 2021-08-17T10:42:22Z

@quarterpin: This pull request references Bugzilla bug 1990826, which is valid.

3 validation(s) were run on this bug

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

Requesting review from QA contact:
/cc @quarterpin

In response to this:

/bugzilla cc-qa

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.

quarterpin · 2021-08-17T10:45:28Z

/lgtm
/label qe-approved

Verified via pre-merge verification workflow, more references related to the test can be found in:
https://bugzilla.redhat.com/show_bug.cgi?id=1990826#c4

Miciah · 2021-08-17T14:21:30Z

[sig-arch] events should not repeat pathologically
3 events happened too frequently

event happened 23 times, something is wrong: ns/cmd-env deploymentconfig/testdc - reason/DeploymentAwaitingCancellation Deployment of version 2 awaiting cancellation of older running deployments
event happened 23 times, something is wrong: ns/cmd-volumes deploymentconfig/test-deployment-config - reason/DeploymentAwaitingCancellation Deployment of version 2 awaiting cancellation of older running deployments
event happened 22 times, something is wrong: ns/cmd-basicresources deploymentconfig/test-deployment-config - reason/DeploymentAwaitingCancellation Deployment of version 2 awaiting cancellation of older running deployments

/test e2e-cmd

Miciah · 2021-08-17T17:14:02Z

/test e2e-cmd

Miciah · 2021-08-17T17:37:54Z

error: error creating buildah builder: Error reading signatures: Error downloading signatures for sha256:7e0a26a6957250636be130cc454b15e13a5ec545650fb5698ffb0b66c02c02d5 in registry.build02.ci.openshift.org/ci/managed-clonerefs: received unexpected HTTP status: 504 Gateway Time-out

/test e2e-cmd

Miciah · 2021-08-18T02:01:18Z

Looks like e2e-cmd keeps hitting BZ#1990041.
/test e2e-cmd

openshift-ci · 2021-08-18T03:20:45Z

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

Test name	Commit	Details	Rerun command
ci/prow/e2e-cmd	`a8c6e73`	link	`/test e2e-cmd`

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.

deads2k · 2021-08-18T11:29:40Z

/approve

openshift-ci · 2021-08-18T11:32:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: candita, deads2k, Miciah, quarterpin

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:

~~pkg/route/OWNERS~~ [deads2k]

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

openshift-ci · 2021-08-18T11:32:51Z

@candita: All pull requests linked via external trackers have merged:

openshift/openshift-apiserver#240

Bugzilla bug 1990826 has been moved to the MODIFIED state.

In response to this:

Bug 1990826: routes without TLS are rejected for missing HSTS annotation

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-high Referenced Bugzilla bug's severity is high 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 Aug 10, 2021

openshift-ci bot requested review from quarterpin, ironcladlou and knobunc August 10, 2021 19:26

Miciah reviewed Aug 11, 2021

View reviewed changes

candita force-pushed the BZ-1990826-TLS branch from f5941a5 to f3bef83 Compare August 12, 2021 17:44

openshift-ci bot assigned Miciah Aug 16, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 16, 2021

sttts reviewed Aug 16, 2021

View reviewed changes

Bug 1990826: routes without TLS are rejected for missing HSTS annotation

a8c6e73

candita force-pushed the BZ-1990826-TLS branch from f3bef83 to a8c6e73 Compare August 16, 2021 23:28

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 16, 2021

openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Aug 17, 2021

openshift-ci bot assigned quarterpin Aug 17, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 17, 2021

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 18, 2021

openshift-ci bot merged commit 04b49d3 into openshift:master Aug 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1990826: routes without TLS are rejected for missing HSTS annotation #240

Bug 1990826: routes without TLS are rejected for missing HSTS annotation #240

candita commented Aug 10, 2021 •

edited

openshift-ci bot commented Aug 10, 2021

Miciah Aug 11, 2021

candita Aug 11, 2021

Miciah Aug 11, 2021

Miciah Aug 11, 2021

candita Aug 11, 2021

candita Aug 11, 2021

candita Aug 12, 2021

candita commented Aug 12, 2021

Miciah commented Aug 16, 2021

sttts Aug 16, 2021

candita Aug 16, 2021

openshift-ci bot commented Aug 16, 2021

candita commented Aug 17, 2021

quarterpin commented Aug 17, 2021

openshift-ci bot commented Aug 17, 2021

quarterpin commented Aug 17, 2021

Miciah commented Aug 17, 2021

Miciah commented Aug 17, 2021

Miciah commented Aug 17, 2021

Miciah commented Aug 18, 2021

openshift-ci bot commented Aug 18, 2021

deads2k commented Aug 18, 2021

openshift-ci bot commented Aug 18, 2021

openshift-ci bot commented Aug 18, 2021

Bug 1990826: routes without TLS are rejected for missing HSTS annotation #240

Bug 1990826: routes without TLS are rejected for missing HSTS annotation #240

Conversation

candita commented Aug 10, 2021 • edited

openshift-ci bot commented Aug 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

candita commented Aug 12, 2021

Miciah commented Aug 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Aug 16, 2021

candita commented Aug 17, 2021

quarterpin commented Aug 17, 2021

openshift-ci bot commented Aug 17, 2021

quarterpin commented Aug 17, 2021

Miciah commented Aug 17, 2021

Miciah commented Aug 17, 2021

Miciah commented Aug 17, 2021

Miciah commented Aug 18, 2021

openshift-ci bot commented Aug 18, 2021

deads2k commented Aug 18, 2021

openshift-ci bot commented Aug 18, 2021

openshift-ci bot commented Aug 18, 2021

candita commented Aug 10, 2021 •

edited