OCPBUGS-17022: further reduce frequency of proxy config checks where necessary #630

liouk · 2023-08-25T12:45:47Z

This PR brings the following improvements:

increase the interval of the check of the OAuthRouteCheckController from 1 to 5 minutes
modify ProxyConfigChecker to ignore events caused by informer cache resyncs that don't yield changes in the respective config maps
filter only relevant ConfigMaps and Secrets to the OAuthRouteCheckController
if the connection parameters haven't changed, don't check an endpoint more frequently than the desired interval

openshift-ci-robot · 2023-08-25T12:45:52Z

@liouk: This pull request references Jira Issue OCPBUGS-17022, which is invalid:

expected the bug to target the "4.14.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

This PR brings the following improvements:

increase the interval of the check of the OAuthRouteCheckController from 1 to 5 minutes

modify ProxyConfigChecker to ignore events caused by informer cache resyncs that don't yield changes in the respective config maps

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.

liouk · 2023-09-12T08:19:26Z

/hold cancel
the automation should be capable of merging this when the critical-fixes-only gate is lifted

openshift-ci-robot · 2023-09-28T13:23:25Z

@liouk: This pull request references Jira Issue OCPBUGS-17022, which is invalid:

expected the bug to target the "4.15.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

This PR brings the following improvements:

increase the interval of the check of the OAuthRouteCheckController from 1 to 5 minutes

modify ProxyConfigChecker to ignore events caused by informer cache resyncs that don't yield changes in the respective config maps

skip config checks triggered by configmap changes whenever an HTTP proxy has been configured in the httpsProxy field

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-robot · 2023-09-28T13:25:04Z

@liouk: This pull request references Jira Issue OCPBUGS-17022, which is invalid:

expected the bug to target the "4.15.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

This PR brings the following improvements:

increase the interval of the check of the OAuthRouteCheckController from 1 to 5 minutes

modify ProxyConfigChecker to ignore events caused by informer cache resyncs that don't yield changes in the respective config maps

skip config checks triggered by CA configmap changes whenever an HTTP proxy has been configured in the httpsProxy field (OCPBUGS-17130)

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-merge-robot · 2023-10-03T13:10:48Z

@liouk: 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/unit	`5cdbfd5`	link	true	`/test unit`

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.

liouk · 2023-10-05T12:17:02Z

/retest-required

openshift-ci-robot · 2023-10-05T14:59:57Z

@liouk: This pull request references Jira Issue OCPBUGS-17022, which is invalid:

expected the bug to target the "4.15.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

This PR brings the following improvements:

increase the interval of the check of the OAuthRouteCheckController from 1 to 5 minutes

modify ProxyConfigChecker to ignore events caused by informer cache resyncs that don't yield changes in the respective config maps

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.

liouk · 2023-10-06T15:11:23Z

/jira refresh

openshift-ci-robot · 2023-10-06T15:11:26Z

@liouk: This pull request references Jira Issue OCPBUGS-17022, which is invalid:

expected the bug to target the "4.15.0" version, but it targets "4.14.0" instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

/jira refresh

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.

liouk · 2023-10-06T15:46:02Z

/jira refresh

pkg/libs/endpointaccessible/endpoint_accessible_controller.go

ibihim · 2023-12-21T17:49:26Z

pkg/libs/endpointaccessible/endpoint_accessible_controller.go

+	endpointsChanged := !c.lastEndpoints.Equal(newEndpoints)
+
+	tlsChanged := false
+	var tlsConfig *tls.Config


nit:

would look slightly better:

var tlsChanged bool var tlsConfig *tls.Config

ibihim

/lgtm

openshift-ci · 2023-12-21T17:52:33Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ibihim, liouk
Once this PR has been reviewed and has the lgtm label, please assign deads2k for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

stlaz · 2024-01-12T08:31:42Z

pkg/controllers/proxyconfig/proxyconfig_controller.go

+	if syncCtx == nil {
+		return
+	}


remove, panic instead

stlaz · 2024-01-12T08:35:35Z

pkg/controllers/oauthendpoints/oauth_endpoints_controller.go

@@ -57,6 +57,19 @@ func NewOAuthRouteCheckController(
 		return getOAuthRouteTLSConfig(cmLister, secretLister, ingressLister, systemCABundle)
 	}

+	filterFunc := factory.NamesFilter(


non-generic, self-explanatory name, please

stlaz · 2024-01-12T08:55:49Z

pkg/libs/endpointaccessible/endpoint_accessible_controller.go

 		}
 		transport.TLSClientConfig = tlsConfig
+
+		// these are the fields that are set by our getTLSConfigFn funcs
+		tlsChanged = c.lastServerName != tlsConfig.ServerName || !tlsConfig.RootCAs.Equal(c.lastCA)


tlsConfig is a generic construct, is comparison of just these two attributes good enough?

These are the only fields that are set by the oauth endpoints controller:

cluster-authentication-operator/pkg/controllers/oauthendpoints/oauth_endpoints_controller.go

Lines 274 to 276 in 91360bd

return &tls.Config{

RootCAs: rootCAs,

}, nil

cluster-authentication-operator/pkg/controllers/oauthendpoints/oauth_endpoints_controller.go

Lines 294 to 302 in 91360bd

return &tls.Config{

RootCAs: rootCAs,

// Specify a host name allowed by the serving cert of the

// endpoints to ensure that TLS validates successfully. The

// serving cert the endpoint uses does not include IP SANs

// so accessing the endpoint via IP would otherwise result

// in validation failure.

ServerName: "oauth-openshift.openshift-authentication.svc",

}, nil

stlaz · 2024-01-12T08:56:53Z

pkg/libs/endpointaccessible/endpoint_accessible_controller.go

-		return err
+	tlsChanged := false
+	var tlsConfig *tls.Config
+	if c.getTLSConfigFn != nil {


do we allow this to be nil?

This was like this before -- I've removed the check now, it is now used as endpointListFn() is, without any nil check.

that ignores updates that do not modify the actual object; this can occur when an informer updates its cache, as the cache update will trigger a sync even if there's no change in the object

…ibleController

…outeCheckController

openshift-ci · 2024-01-25T11:03:20Z

New changes are detected. LGTM label has been removed.

If there are no changes in the endpoint parameters, skip the check.

openshift-ci · 2024-01-25T15:45:19Z

@liouk: 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/e2e-agnostic-ipv6	`91360bd`	link	false	`/test e2e-agnostic-ipv6`

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.

liouk · 2024-01-30T13:05:47Z

/retest-required

openshift-bot · 2024-05-09T01:00:58Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

liouk · 2024-05-13T07:58:58Z

/remove-lifecycle stale

openshift-bot · 2024-08-11T09:01:03Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

liouk · 2024-08-13T07:36:20Z

/lifecycle frozen

openshift-ci · 2024-08-13T07:36:21Z

@liouk: The lifecycle/frozen label cannot be applied to Pull Requests.

In response to this:

/lifecycle frozen

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-sigs/prow repository.

liouk · 2024-08-13T07:36:40Z

/remove-lifecycle stale

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 25, 2023

openshift-ci bot requested review from deads2k and stlaz August 25, 2023 12:47

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

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

liouk force-pushed the fix-proxy-checks branch from a9c19e5 to a332db4 Compare September 28, 2023 13:21

liouk force-pushed the fix-proxy-checks branch 3 times, most recently from 5cdbfd5 to e3dfe2b Compare October 3, 2023 13:10

liouk force-pushed the fix-proxy-checks branch 2 times, most recently from b56b962 to e7672ff Compare October 5, 2023 09:18

liouk force-pushed the fix-proxy-checks branch 2 times, most recently from 7942d3a to 99e7947 Compare October 5, 2023 14:14

liouk changed the title ~~WIP: OCPBUGS-17022: further reduce frequency of proxy config checks where necessary~~ OCPBUGS-17022: further reduce frequency of proxy config checks where necessary Oct 5, 2023

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 5, 2023

openshift-ci-robot added jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. and removed jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Oct 6, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 18, 2023

ibihim reviewed Dec 19, 2023

View reviewed changes

pkg/libs/endpointaccessible/endpoint_accessible_controller.go Outdated Show resolved Hide resolved

ibihim reviewed Dec 19, 2023

View reviewed changes

pkg/libs/endpointaccessible/endpoint_accessible_controller.go Outdated Show resolved Hide resolved

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 21, 2023

ibihim reviewed Dec 21, 2023

View reviewed changes

ibihim approved these changes Dec 21, 2023

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 21, 2023

stlaz reviewed Jan 12, 2024

View reviewed changes

liouk added 4 commits January 25, 2024 12:03

proxyconfig: use ConfigMap informers with custom event handlers

24c80ca

that ignores updates that do not modify the actual object; this can occur when an informer updates its cache, as the cache update will trigger a sync even if there's no change in the object

proxyconfig: improve error messages

ec0252d

oauthendpoints: use separate resync intervals for each EndpointAccess…

d11d24e

…ibleController

oauthendpoints: filter only relevant informer resources to the OAuthR…

594a160

…outeCheckController

liouk force-pushed the fix-proxy-checks branch from 256229d to d12fe44 Compare January 25, 2024 11:03

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 25, 2024

endpointaccessible: check if endpoint parameters changed at every sync

91360bd

If there are no changes in the endpoint parameters, skip the check.

liouk force-pushed the fix-proxy-checks branch from d12fe44 to 91360bd Compare January 25, 2024 12:23

openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 9, 2024

openshift-ci bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 13, 2024

openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 11, 2024

openshift-ci bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCPBUGS-17022: further reduce frequency of proxy config checks where necessary #630

OCPBUGS-17022: further reduce frequency of proxy config checks where necessary #630

liouk commented Aug 25, 2023 •

edited

Loading

openshift-ci-robot commented Aug 25, 2023

liouk commented Sep 12, 2023

openshift-ci-robot commented Sep 28, 2023

openshift-ci-robot commented Sep 28, 2023

openshift-merge-robot commented Oct 3, 2023

liouk commented Oct 5, 2023

openshift-ci-robot commented Oct 5, 2023

liouk commented Oct 6, 2023

openshift-ci-robot commented Oct 6, 2023

liouk commented Oct 6, 2023

ibihim Dec 21, 2023

ibihim left a comment

openshift-ci bot commented Dec 21, 2023

stlaz Jan 12, 2024

stlaz Jan 12, 2024

stlaz Jan 12, 2024

liouk Feb 8, 2024

stlaz Jan 12, 2024

liouk Jan 25, 2024

openshift-ci bot commented Jan 25, 2024

openshift-ci bot commented Jan 25, 2024 •

edited

Loading

liouk commented Jan 30, 2024

openshift-bot commented May 9, 2024

liouk commented May 13, 2024

openshift-bot commented Aug 11, 2024

liouk commented Aug 13, 2024

openshift-ci bot commented Aug 13, 2024

liouk commented Aug 13, 2024

	return &tls.Config{
	RootCAs: rootCAs,
	// Specify a host name allowed by the serving cert of the
	// endpoints to ensure that TLS validates successfully. The
	// serving cert the endpoint uses does not include IP SANs
	// so accessing the endpoint via IP would otherwise result
	// in validation failure.
	ServerName: "oauth-openshift.openshift-authentication.svc",
	}, nil

OCPBUGS-17022: further reduce frequency of proxy config checks where necessary #630

Are you sure you want to change the base?

OCPBUGS-17022: further reduce frequency of proxy config checks where necessary #630

Conversation

liouk commented Aug 25, 2023 • edited Loading

openshift-ci-robot commented Aug 25, 2023

liouk commented Sep 12, 2023

openshift-ci-robot commented Sep 28, 2023

openshift-ci-robot commented Sep 28, 2023

openshift-merge-robot commented Oct 3, 2023

liouk commented Oct 5, 2023

openshift-ci-robot commented Oct 5, 2023

liouk commented Oct 6, 2023

openshift-ci-robot commented Oct 6, 2023

liouk commented Oct 6, 2023

ibihim Dec 21, 2023

Choose a reason for hiding this comment

ibihim left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Dec 21, 2023

stlaz Jan 12, 2024

Choose a reason for hiding this comment

stlaz Jan 12, 2024

Choose a reason for hiding this comment

stlaz Jan 12, 2024

Choose a reason for hiding this comment

liouk Feb 8, 2024

Choose a reason for hiding this comment

stlaz Jan 12, 2024

Choose a reason for hiding this comment

liouk Jan 25, 2024

Choose a reason for hiding this comment

openshift-ci bot commented Jan 25, 2024

openshift-ci bot commented Jan 25, 2024 • edited Loading

liouk commented Jan 30, 2024

openshift-bot commented May 9, 2024

liouk commented May 13, 2024

openshift-bot commented Aug 11, 2024

liouk commented Aug 13, 2024

openshift-ci bot commented Aug 13, 2024

liouk commented Aug 13, 2024

liouk commented Aug 25, 2023 •

edited

Loading

openshift-ci bot commented Jan 25, 2024 •

edited

Loading