CFE-1020: feature:route external certificate reference by chiragkyal · Pull Request #565 · openshift/router

chiragkyal · 2024-02-28T19:33:08Z

Description

Introduces new RouteSecretManager plugin for the management and validation of routes with externalCertificate.
Integration with secretManager to register, unregister, and read referenced secret.
Handling of the routes when referenced secret is updated or deleted.
ValidateTLSExternalCertificate function to test preconditions for using externalCertificate.
Everything is behind AllowExternalCertificates flag which is true when cluster-ingress-operator sets it, if RouteExternalCertificate feature-gate is enabled.

Implements: openshift/enhancements#1307
Related to https://issues.redhat.com//browse/CFE-1020
Depends on: CFE-866: Add secret monitor for Routes
Feature-gate wiring: openshift/cluster-ingress-operator#1017

openshift-ci · 2024-02-28T19:33:13Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

chiragkyal · 2024-02-28T19:34:28Z

/jira refresh

openshift-ci-robot · 2024-02-28T19:34:31Z

@chiragkyal: No Jira issue is referenced in the title of this pull request.
To reference a jira issue, add 'XYZ-NNN:' to the title of this pull request and request another refresh with /jira refresh.

Details

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 openshift-eng/jira-lifecycle-plugin repository.

chiragkyal · 2024-02-28T19:36:16Z

/jira refresh

openshift-ci-robot · 2024-02-28T19:36:19Z

@chiragkyal: No Jira issue is referenced in the title of this pull request.
To reference a jira issue, add 'XYZ-NNN:' to the title of this pull request and request another refresh with /jira refresh.

Details

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 openshift-eng/jira-lifecycle-plugin repository.

chiragkyal · 2024-04-26T11:00:56Z

/test images

openshift-ci · 2024-04-26T11:00:59Z

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

/test e2e-agnostic
/test e2e-aws-serial
/test e2e-upgrade
/test images
/test unit
/test verify

The following commands are available to trigger optional jobs:

/test e2e-metal-ipi-ovn-dualstack
/test e2e-metal-ipi-ovn-ipv6
/test e2e-metal-ipi-ovn-router
/test okd-scos-images
/test perfscale-aws-fips-ingress-perf
/test perfscale-aws-ingress-perf

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

pull-ci-openshift-router-master-e2e-agnostic
pull-ci-openshift-router-master-e2e-aws-serial
pull-ci-openshift-router-master-e2e-metal-ipi-ovn-ipv6
pull-ci-openshift-router-master-e2e-upgrade
pull-ci-openshift-router-master-images
pull-ci-openshift-router-master-unit
pull-ci-openshift-router-master-verify

Details

In response to this:

/test image

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.

chiragkyal · 2024-04-26T11:02:25Z

/test all

chiragkyal · 2024-04-26T15:42:46Z

/test images

chiragkyal · 2024-04-26T15:44:31Z

/hold
WIP

alebedev87 · 2024-04-30T15:03:56Z

/assign

Miciah · 2024-04-30T18:16:47Z

pkg/router/controller/route_secret_manager.go

+// For Deleted events, it unregisters the route if it's registered.
+// Additionally, it delegates the handling of the event to the next plugin in the chain after performing the necessary actions.
+func (p *RouteSecretManager) HandleRoute(eventType watch.EventType, route *routev1.Route) error {
+	klog.Infof("Executing RouteSecretManager plugin with eventType %s...", eventType)


Other plugins use the log variable; is there some reason to use klog here?

Suggested change

klog.Infof("Executing RouteSecretManager plugin with eventType %s...", eventType)

log.V(10).Infof("HandleRoute: RouteSecretManager, eventType %s", eventType)

Log-level 10 would be consistent with this recent change: 66eb223.

Thanks, I've removed klog from the plugin and now using log with required log-levels.

Miciah · 2024-04-30T18:17:01Z

pkg/router/controller/route_secret_manager.go

+		// unregister associated secret monitor, if registered
+		if p.secretManager.IsRouteRegistered(route.Namespace, route.Name) {
+			if err := p.secretManager.UnregisterRoute(route.Namespace, route.Name); err != nil {
+				klog.Error("failed to unregister route", err)


Suggested change

klog.Error("failed to unregister route", err)

log.Error(err, "failed to unregister route")

Thanks, updated to log

Miciah · 2024-04-30T18:22:44Z

pkg/router/controller/route_secret_manager.go

+		klog.Error(err, "skipping route due to invalid externalCertificate configuration", " route ", route.Name)
+		p.recorder.RecordRouteRejection(route, "ExternalCertificateValidationFailed", err.Error())
+		p.plugin.HandleRoute(watch.Deleted, route)
+		return fmt.Errorf("invalid route configuration for externalCertificate")


Should the return error include the actual validation error message? For example, the UniqueHost plugin does this:

router/pkg/router/controller/unique_host.go

Lines 106 to 119 in b6f7c63

// Validate that the route host name conforms to DNS requirements.

// Defends against routes created before validation rules were added for host names.

if errs := ValidateHostName(route); len(errs) > 0 {

log.V(4).Info("invalid host name", "routeName", routeName, "host", host)

errMessages := make([]string, len(errs))

for i := 0; i < len(errs); i++ {

errMessages[i] = errs[i].Error()

}

err := fmt.Errorf("host name validation errors: %s", strings.Join(errMessages, ", "))

p.recorder.RecordRouteRejection(route, "InvalidHost", err.Error())

p.plugin.HandleRoute(watch.Deleted, route)

return err

}

Sure, updated to return err so that it includes the actual validation error message.

Miciah · 2024-04-30T18:27:36Z

pkg/router/controller/route_secret_manager.go

+	secreth := cache.ResourceEventHandlerFuncs{
+		AddFunc: func(obj interface{}) {
+			secret := obj.(*kapi.Secret)
+			klog.Infof("secret %s added for %s/%s", secret.Name, route.Namespace, route.Name)


Suggested change

klog.Infof("secret %s added for %s/%s", secret.Name, route.Namespace, route.Name)

log.V(4).Info("secret added for route", "namespace", route.Namespace, "secret", secret.Name, "route", route.Name)

Thanks, updated to log with correct log-level

Miciah · 2024-04-30T18:32:05Z

pkg/router/controller/route_secret_manager.go

+// must be called after hasExternalCertificate
+func getReferencedSecret(route *routev1.Route) string {
+	secretName := route.Spec.TLS.ExternalCertificate.Name
+	klog.Info("Referenced secretName: ", secretName)


It seems odd that getReferencedSecret logs the secret name. If you keep this log statement, it should have a high log level (I'd say at least 6, maybe 10).

I'll keep this for now at the highest log-level (10), incase of any debugging need.

Miciah · 2024-04-30T18:33:19Z

pkg/router/controller/route_secret_manager.go

+	// update tls.Certificate and tls.Key
+	// NOTE: this will be in-memory change and won't update actual route resource
+	route.Spec.TLS.Certificate = string(secret.Data["tls.crt"])
+	route.Spec.TLS.Key = string(secret.Data["tls.key"])


Should this also clear route.Spec.TLS.CACertificate? If not, please add a comment explaining why.

since externalCertificate will not contain CACertificate, tls.CACertificate won't be updated. I've added a comment explaining this.

chiragkyal · 2024-05-08T10:35:45Z

/retest-required

openshift-ci-robot · 2024-05-09T12:53:14Z

@chiragkyal: This pull request references CFE-1020 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

Details

In response to this:

Description

Introduces new RouteSecretManager plugin for the management and validation of routes with externalCertificate.

Integration with secretManager to register, unregister, and read referenced secret.

Handling of the routes when referenced secret is updated or deleted.

ValidateTLSExternalCertificate function to test preconditions for using externalCertificate.

Everything is behind AllowExternalCertificates flag which is true when cluster-ingress-operator sets it, if RouteExternalCertificate feature-gate is enabled.

Implements: openshift/enhancements#1307
Related to https://issues.redhat.com//browse/CFE-1020
Depends on: CFE-866: Add secret monitor for Routes

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 openshift-eng/jira-lifecycle-plugin repository.

openshift-ci-robot · 2024-05-09T13:47:43Z

@chiragkyal: This pull request references CFE-1020 which is a valid jira issue.

Details

In response to this:

Description

Introduces new RouteSecretManager plugin for the management and validation of routes with externalCertificate.

Integration with secretManager to register, unregister, and read referenced secret.

Handling of the routes when referenced secret is updated or deleted.

ValidateTLSExternalCertificate function to test preconditions for using externalCertificate.

Everything is behind AllowExternalCertificates flag which is true when cluster-ingress-operator sets it, if RouteExternalCertificate feature-gate is enabled.

Implements: openshift/enhancements#1307
Related to https://issues.redhat.com//browse/CFE-1020
Depends on: CFE-866: Add secret monitor for Routes
Feature-gate wiring: openshift/cluster-ingress-operator#1017

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 openshift-eng/jira-lifecycle-plugin repository.

Miciah · 2024-05-10T03:16:02Z

pkg/router/controller/route_secret_manager.go

+	case watch.Modified:
+		// remove old watch
+		if p.secretManager.IsRouteRegistered(route.Namespace, route.Name) {
+			if err := p.secretManager.UnregisterRoute(route.Namespace, route.Name); err != nil {
+				klog.Error("failed to unregister route", err)
+				return err
+			}
+		}
+		// create new watch
+		if err := p.validateAndRegister(route); err != nil {
+			return err
+		}


It does seem odd that the plugin always unregisters and reregisters the route, even if the TLS configuration did not change. In particular, I'm concerned about the overhead of validating and reregistering the route for updates that don't touch the TLS configuration. If that is necessary to do, can you add a comment explaining why? If it isn't necessary, please either add a check to skip the unnecessary deregister-reregister or add a to-do comment to improve the logic before GA.

I got the validation and reregistration overhead concern here. The plugin always needs to unregister and reregister the route even if the TLS configuration did not change, for two reasons

The HandleRoute() method does not carry the old route spec; it's only the new route that propagates in the plugin chain. So, there isn't a definite way to compare the old TLS configuration and the new TLS configuration, and decide whether there is any change happened or not. Hence, we assume that it's always updated, so reregistration is required.

Another reason for always doing reregistration is to have an updated secretHandler. If we don't always create a new handler (with every route update event), there may be changes of serving stale route spec in the next plugin chain, when the referenced secret is updated or deleted. Sending outdated route in the next plugin, may impact their expected functionality.

router/pkg/router/controller/route_secret_manager.go

Lines 163 to 207 in c9ce887

UpdateFunc: func(old interface{}, new interface{}) {

secretOld := old.(*kapi.Secret)

secretNew := new.(*kapi.Secret)

log.V(4).Info("secret updated for route", "namespace", route.Namespace, "secret", secretNew.Name, "old-version", secretOld.ResourceVersion, "new-version", secretNew.ResourceVersion, "route", route.Name)

// re-validate

fldPath := field.NewPath("spec").Child("tls").Child("externalCertificate")

if err := routeapihelpers.ValidateTLSExternalCertificate(route, fldPath, p.sarClient, p.secretsGetter).ToAggregate(); err != nil {

log.Error(err, "skipping route due to invalid externalCertificate configuration", "namespace", route.Namespace, "route", route.Name)

p.recorder.RecordRouteRejection(route, "ExternalCertificateValidationFailed", err.Error())

p.plugin.HandleRoute(watch.Deleted, route)

return

}

// read referenced secret (updated data)

secret, err := p.secretManager.GetSecret(context.TODO(), route.Namespace, route.Name)

if err != nil {

log.Error(err, "failed to get referenced secret")

p.recorder.RecordRouteRejection(route, "ExternalCertificateGetFailed", err.Error())

p.plugin.HandleRoute(watch.Deleted, route)

return

}

// update tls.Certificate and tls.Key

route.Spec.TLS.Certificate = string(secret.Data["tls.crt"])

route.Spec.TLS.Key = string(secret.Data["tls.key"])

// call the next plugin with watch.Modified

p.plugin.HandleRoute(watch.Modified, route)

// commit the changes

p.plugin.Commit()

},

DeleteFunc: func(obj interface{}) {

secret := obj.(*kapi.Secret)

msg := fmt.Sprintf("secret %s deleted for route %s/%s", secret.Name, route.Namespace, route.Name)

log.V(4).Info(msg)

// unregister associated secret monitor

if err := p.secretManager.UnregisterRoute(route.Namespace, route.Name); err != nil {

log.Error(err, "failed to unregister route")

}

p.recorder.RecordRouteRejection(route, "ExternalCertificateSecretDeleted", msg)

p.plugin.HandleRoute(watch.Deleted, route)

If we want to improve the logic for GA, then I think we could rely on secretManager to give the previously referenced secret name. It could have a new method called GetSecretName() to return the previously registered secret name. And we will only unregister and re-register if it's updated.

This could look something like

case watch.Modified: newHasExt := hasExternalCertificate(route) oldHadExt := p.secretManager.IsRouteRegistered(route.Namespace, route.Name) if newHasExt { // new route has externalCertificate if oldHadExt { // old route was also having externalCertificate oldSecret := p.secretManager.GetSecretName(route.Namespace, route.Name) if oldSecret != route.Spec.TLS.ExternalCertificate.Name { // externalCertificate is updated // unregister and re-register p.secretManager.UnregisterRoute(route.Namespace, route.Name) p.validateAndRegister(route) } else { // do nothing if externalCertificate is not updated } } else { // old route was not having externalCertificate p.validateAndRegister(route) } } else { // new route doesn't have externalCertificate if oldHadExt { // old route was having externalCertificate p.secretManager.UnregisterRoute(route.Namespace, route.Name) } else { // do nothing : no update to externalCertificate } }

This could solve the issue of always doing unregister and re-register.

Along with that, we also need to ensure that the secretHandler always processes the updated route object whenever there is a secret update or delete event.

I think this can be archived by getting the updated route object from the client. So the UpdateFunc and DeleteFunc of secretHandler will first query the apiserver for the latest route, and then will continue the next steps.

Let me know if this approach would improve the overhead issue. We can update it for GA.
If there are any other suggestions, we can discuss them too.

I've added a comment in the code for now, with a to-do to improve the logic while going to GA.

Placeholder to track : https://issues.redhat.com/browse/CFE-1057

pkg/router/controller/route_secret_manager.go

chiragkyal · 2024-05-10T10:53:52Z

/retest-required

pkg/router/controller/route_secret_manager_test.go

alebedev87 · 2024-05-13T21:18:42Z

pkg/router/controller/route_secret_manager_test.go

+			recorder := routeStatusRecorder{rejections: make(map[string]string)}
+
+			// assign default value to expectedRejections
+			if s.expectedRejections == nil {
+				s.expectedRejections = map[string]string{}
+			}


If you keep rejections and expectedRejections unset reflect.DeepEqual() should give true:

Suggested change

recorder := routeStatusRecorder{rejections: make(map[string]string)}

// assign default value to expectedRejections

if s.expectedRejections == nil {

s.expectedRejections = map[string]string{}

}

recorder := routeStatusRecorder{}

If we keep rejections unset, it will cause panic

panic: assignment to entry in nil map

So, need to create the map.

chiragkyal · 2024-05-14T07:23:54Z

/retest-required

Vendoring openshift/library-go#1626 Signed-off-by: chiragkyal <ckyal@redhat.com>

Introduces new RouteSecretManager plugin for the management and validation of routes with externalCertificate. Integration with secretManager to register, unregister, and read referenced secret. Handling of the routes when referenced secret is updated or deleted. ValidateTLSExternalCertificate function to test preconditions for using externalCertificate. Everything is behind AllowExternalCertificates flag which is true when cluster-ingress-operator sets it, if RouteExternalCertificate feature-gate is enabled. Signed-off-by: chiragkyal <ckyal@redhat.com>

openshift-ci · 2024-05-14T13:58:56Z

@chiragkyal: all tests passed!

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

alebedev87 · 2024-05-14T17:42:23Z

/lgtm
/approve
/hold

Holding for @rfredette to have the final look.

openshift-ci · 2024-05-14T17:43:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alebedev87

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [alebedev87]

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

rfredette · 2024-05-14T18:33:36Z

/lgtm
/unhold

chiragkyal · 2024-05-14T18:35:40Z

/payload 4.16 nightly informing

openshift-ci · 2024-05-14T18:38:54Z

@chiragkyal: trigger 68 job(s) of type informing for the nightly release of OCP 4.16

periodic-ci-openshift-release-master-nightly-4.16-e2e-agent-compact-ipv4-conformance
periodic-ci-openshift-release-master-nightly-4.16-e2e-agent-ha-dualstack-conformance
periodic-ci-openshift-release-master-nightly-4.16-e2e-agent-sno-ipv6
periodic-ci-openshift-release-master-nightly-4.16-console-aws
periodic-ci-openshift-cluster-control-plane-machine-set-operator-release-4.16-periodics-e2e-aws
periodic-ci-openshift-release-master-nightly-4.16-e2e-aws-csi
periodic-ci-openshift-release-master-ci-4.16-e2e-aws-ovn
periodic-ci-openshift-release-master-nightly-4.16-e2e-aws-ovn-fips
periodic-ci-openshift-release-master-nightly-4.16-e2e-aws-ovn-single-node
periodic-ci-openshift-release-master-nightly-4.16-e2e-aws-ovn-single-node-serial
periodic-ci-openshift-release-master-ci-4.16-e2e-aws-ovn-techpreview
periodic-ci-openshift-release-master-ci-4.16-e2e-aws-ovn-techpreview-serial
periodic-ci-openshift-release-master-ci-4.16-e2e-aws-ovn-upgrade-out-of-change
periodic-ci-openshift-release-master-nightly-4.16-e2e-aws-sdn
periodic-ci-openshift-release-master-nightly-4.16-e2e-aws-sdn-cgroupsv2
periodic-ci-openshift-release-master-ci-4.16-e2e-aws-sdn-upgrade-out-of-change
periodic-ci-openshift-release-master-nightly-4.16-e2e-aws-ovn-upi
periodic-ci-openshift-cluster-control-plane-machine-set-operator-release-4.16-periodics-e2e-azure
periodic-ci-openshift-release-master-nightly-4.16-e2e-azure-csi
periodic-ci-openshift-release-master-ci-4.16-e2e-azure-ovn
periodic-ci-openshift-release-master-ci-4.16-e2e-azure-ovn-serial
periodic-ci-openshift-release-master-ci-4.16-e2e-azure-ovn-techpreview
periodic-ci-openshift-release-master-ci-4.16-e2e-azure-ovn-techpreview-serial
periodic-ci-openshift-release-master-ci-4.16-e2e-azure-ovn-upgrade-out-of-change
periodic-ci-openshift-release-master-nightly-4.16-e2e-azure-sdn
periodic-ci-openshift-release-master-ci-4.16-e2e-azure-sdn-upgrade
periodic-ci-openshift-release-master-ci-4.16-e2e-azure-sdn-upgrade-out-of-change
periodic-ci-openshift-release-master-cnv-nightly-4.16-e2e-azure-deploy-cnv
periodic-ci-openshift-release-master-cnv-nightly-4.16-e2e-azure-upgrade-cnv
periodic-ci-openshift-release-master-nightly-4.16-e2e-aws-driver-toolkit
periodic-ci-openshift-cluster-control-plane-machine-set-operator-release-4.16-periodics-e2e-gcp
periodic-ci-openshift-release-master-ci-4.16-e2e-gcp-ovn
periodic-ci-openshift-release-master-nightly-4.16-e2e-gcp-ovn-csi
periodic-ci-openshift-release-master-nightly-4.16-e2e-gcp-ovn-rt
periodic-ci-openshift-release-master-ci-4.16-e2e-gcp-ovn-techpreview
periodic-ci-openshift-release-master-ci-4.16-e2e-gcp-ovn-techpreview-serial
periodic-ci-openshift-release-master-ci-4.16-upgrade-from-stable-4.15-e2e-gcp-ovn-upgrade
periodic-ci-openshift-release-master-nightly-4.16-e2e-gcp-sdn
periodic-ci-openshift-release-master-nightly-4.16-e2e-gcp-sdn-serial
periodic-ci-openshift-release-master-nightly-4.16-e2e-gcp-sdn-upgrade
periodic-ci-openshift-release-master-ci-4.16-upgrade-from-stable-4.15-e2e-gcp-sdn-upgrade
periodic-ci-openshift-release-master-nightly-4.16-e2e-metal-ipi-ovn-bm-upgrade
periodic-ci-openshift-release-master-nightly-4.16-e2e-metal-ipi-ovn-dualstack
periodic-ci-openshift-release-master-nightly-4.16-e2e-metal-ipi-ovn-serial-ipv4
periodic-ci-openshift-release-master-nightly-4.16-e2e-metal-ipi-ovn-serial-virtualmedia
periodic-ci-openshift-release-master-nightly-4.16-upgrade-from-stable-4.15-e2e-metal-ipi-sdn-bm-upgrade
periodic-ci-openshift-release-master-nightly-4.16-e2e-metal-ipi-serial-ovn-ipv6
periodic-ci-openshift-release-master-nightly-4.16-e2e-metal-ipi-serial-ovn-dualstack
periodic-ci-openshift-release-master-nightly-4.16-e2e-metal-ipi-upgrade-ovn-ipv6
periodic-ci-openshift-release-master-nightly-4.16-upgrade-from-stable-4.15-e2e-metal-ipi-upgrade-ovn-ipv6
periodic-ci-openshift-release-master-nightly-4.16-e2e-metal-ovn-assisted
periodic-ci-openshift-release-master-nightly-4.16-metal-ovn-single-node-recert-cluster-rename
periodic-ci-openshift-osde2e-main-nightly-4.16-osd-aws
periodic-ci-openshift-osde2e-main-nightly-4.16-osd-gcp
periodic-ci-openshift-release-master-nightly-4.16-e2e-aws-ovn-proxy
periodic-ci-openshift-release-master-nightly-4.16-e2e-metal-ovn-single-node-live-iso
periodic-ci-openshift-osde2e-main-nightly-4.16-rosa-classic-sts
periodic-ci-openshift-release-master-nightly-4.16-e2e-aws-sdn-upgrade
periodic-ci-openshift-release-master-nightly-4.16-e2e-telco5g
periodic-ci-openshift-release-master-nightly-4.16-upgrade-from-stable-4.15-e2e-aws-sdn-upgrade
periodic-ci-openshift-release-master-nightly-4.16-e2e-vsphere-ovn-csi
periodic-ci-openshift-release-master-nightly-4.16-e2e-vsphere-ovn-serial
periodic-ci-openshift-release-master-nightly-4.16-e2e-vsphere-ovn-techpreview
periodic-ci-openshift-release-master-nightly-4.16-e2e-vsphere-ovn-techpreview-serial
periodic-ci-openshift-release-master-nightly-4.16-e2e-vsphere-ovn-upi
periodic-ci-openshift-release-master-nightly-4.16-e2e-vsphere-ovn-upi-serial
periodic-ci-openshift-release-master-nightly-4.16-e2e-vsphere-sdn
periodic-ci-openshift-release-master-nightly-4.16-e2e-vsphere-static-ovn

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/c433d780-1220-11ef-91a9-e5af5c26871f-0

chiragkyal · 2024-05-15T07:36:04Z

/payload 4.16 ci blocking
/payload 4.16 nightly blocking

openshift-ci · 2024-05-15T07:36:39Z

@chiragkyal: trigger 5 job(s) of type blocking for the ci release of OCP 4.16

periodic-ci-openshift-release-master-ci-4.16-upgrade-from-stable-4.15-e2e-aws-ovn-upgrade
periodic-ci-openshift-release-master-ci-4.16-upgrade-from-stable-4.15-e2e-azure-sdn-upgrade
periodic-ci-openshift-release-master-ci-4.16-e2e-gcp-ovn-upgrade
periodic-ci-openshift-release-master-ci-4.16-e2e-aws-sdn-serial
periodic-ci-openshift-hypershift-release-4.16-periodics-e2e-aws-ovn

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/c9be8610-128d-11ef-87bb-2bb6fe4e6d3b-0

trigger 8 job(s) of type blocking for the nightly release of OCP 4.16

periodic-ci-openshift-release-master-nightly-4.16-e2e-aws-sdn-upgrade
periodic-ci-openshift-release-master-ci-4.16-e2e-azure-ovn-upgrade
periodic-ci-openshift-release-master-ci-4.16-upgrade-from-stable-4.15-e2e-gcp-ovn-rt-upgrade
periodic-ci-openshift-hypershift-release-4.16-periodics-e2e-aws-ovn-conformance
periodic-ci-openshift-release-master-nightly-4.16-e2e-aws-ovn-serial
periodic-ci-openshift-release-master-ci-4.16-e2e-aws-ovn-upgrade
periodic-ci-openshift-release-master-nightly-4.16-e2e-metal-ipi-ovn-bm
periodic-ci-openshift-release-master-nightly-4.16-e2e-metal-ipi-ovn-ipv6

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/c9be8610-128d-11ef-87bb-2bb6fe4e6d3b-1

chiragkyal · 2024-05-15T12:45:52Z

/label acknowledge-critical-fixes-only

The payload jobs look okay to me. Additionally, this is a TechPreview feature behind feature-gate. I think we are good to add acknowledge-critical-fixes-only label

openshift-bot · 2024-05-15T17:05:18Z

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-haproxy-router-base-container-v4.17.0-202405151441.p0.g4d9b8c4.assembly.stream.el9 for distgit ose-haproxy-router-base.
All builds following this will include this PR.

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 Feb 28, 2024

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 25, 2024

chiragkyal force-pushed the feature/external-certificate-reference branch from c21ee84 to 2c6940e Compare April 26, 2024 08:49

chiragkyal force-pushed the feature/external-certificate-reference branch from dace218 to cc6eb11 Compare April 26, 2024 15:41

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 26, 2024

chiragkyal marked this pull request as ready for review April 26, 2024 15:43

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 Apr 26, 2024

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

openshift-ci bot requested review from alebedev87 and candita April 26, 2024 15:45

chiragkyal force-pushed the feature/external-certificate-reference branch from 2a44e68 to 1f3ad90 Compare April 29, 2024 06:33

chiragkyal changed the title ~~CFE:1020 - [WIP] feature:route external certificate reference~~ CFE:1020 - feature:route external certificate reference Apr 30, 2024

openshift-ci bot assigned alebedev87 Apr 30, 2024

Miciah reviewed Apr 30, 2024

View reviewed changes

Miciah reviewed May 10, 2024

View reviewed changes

pkg/router/controller/route_secret_manager.go Outdated Show resolved Hide resolved

rfredette reviewed May 13, 2024

View reviewed changes

pkg/router/controller/route_secret_manager_test.go Outdated Show resolved Hide resolved

alebedev87 reviewed May 13, 2024

View reviewed changes

chiragkyal added 2 commits May 14, 2024 16:13

CFE-1020: Bump library-go and update deps

c1d3e84

Vendoring openshift/library-go#1626 Signed-off-by: chiragkyal <ckyal@redhat.com>

chiragkyal force-pushed the feature/external-certificate-reference branch from 9c0b2e7 to 2f06f7c Compare May 14, 2024 10:46

openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels May 14, 2024

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

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 14, 2024

openshift-ci bot added the acknowledge-critical-fixes-only Indicates if the issuer of the label is OK with the policy. label May 15, 2024

openshift-merge-bot bot merged commit 4d9b8c4 into openshift:master May 15, 2024

chiragkyal deleted the feature/external-certificate-reference branch May 15, 2024 12:53

git001 mentioned this pull request May 16, 2024

Add documentation for externalCertificate option in secure routes openshift/openshift-docs#76046

Closed

	klog.Infof("Executing RouteSecretManager plugin with eventType %s...", eventType)
	log.V(10).Infof("HandleRoute: RouteSecretManager, eventType %s", eventType)

	klog.Error("failed to unregister route", err)
	log.Error(err, "failed to unregister route")

	// Validate that the route host name conforms to DNS requirements.
	// Defends against routes created before validation rules were added for host names.
	if errs := ValidateHostName(route); len(errs) > 0 {
	log.V(4).Info("invalid host name", "routeName", routeName, "host", host)
	errMessages := make([]string, len(errs))
	for i := 0; i < len(errs); i++ {
	errMessages[i] = errs[i].Error()
	}

	err := fmt.Errorf("host name validation errors: %s", strings.Join(errMessages, ", "))
	p.recorder.RecordRouteRejection(route, "InvalidHost", err.Error())
	p.plugin.HandleRoute(watch.Deleted, route)
	return err
	}

	klog.Infof("secret %s added for %s/%s", secret.Name, route.Namespace, route.Name)
	log.V(4).Info("secret added for route", "namespace", route.Namespace, "secret", secret.Name, "route", route.Name)

	UpdateFunc: func(old interface{}, new interface{}) {
	secretOld := old.(*kapi.Secret)
	secretNew := new.(*kapi.Secret)
	log.V(4).Info("secret updated for route", "namespace", route.Namespace, "secret", secretNew.Name, "old-version", secretOld.ResourceVersion, "new-version", secretNew.ResourceVersion, "route", route.Name)

	// re-validate
	fldPath := field.NewPath("spec").Child("tls").Child("externalCertificate")
	if err := routeapihelpers.ValidateTLSExternalCertificate(route, fldPath, p.sarClient, p.secretsGetter).ToAggregate(); err != nil {
	log.Error(err, "skipping route due to invalid externalCertificate configuration", "namespace", route.Namespace, "route", route.Name)
	p.recorder.RecordRouteRejection(route, "ExternalCertificateValidationFailed", err.Error())
	p.plugin.HandleRoute(watch.Deleted, route)
	return
	}

	// read referenced secret (updated data)
	secret, err := p.secretManager.GetSecret(context.TODO(), route.Namespace, route.Name)
	if err != nil {
	log.Error(err, "failed to get referenced secret")
	p.recorder.RecordRouteRejection(route, "ExternalCertificateGetFailed", err.Error())
	p.plugin.HandleRoute(watch.Deleted, route)
	return
	}

	// update tls.Certificate and tls.Key
	route.Spec.TLS.Certificate = string(secret.Data["tls.crt"])
	route.Spec.TLS.Key = string(secret.Data["tls.key"])

	// call the next plugin with watch.Modified
	p.plugin.HandleRoute(watch.Modified, route)
	// commit the changes
	p.plugin.Commit()
	},

	DeleteFunc: func(obj interface{}) {
	secret := obj.(*kapi.Secret)
	msg := fmt.Sprintf("secret %s deleted for route %s/%s", secret.Name, route.Namespace, route.Name)
	log.V(4).Info(msg)

	// unregister associated secret monitor
	if err := p.secretManager.UnregisterRoute(route.Namespace, route.Name); err != nil {
	log.Error(err, "failed to unregister route")
	}

	p.recorder.RecordRouteRejection(route, "ExternalCertificateSecretDeleted", msg)
	p.plugin.HandleRoute(watch.Deleted, route)

Comments

Conversation

chiragkyal commented Feb 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

openshift-ci bot commented Feb 28, 2024

Uh oh!

chiragkyal commented Feb 28, 2024

Uh oh!

openshift-ci-robot commented Feb 28, 2024

Uh oh!

chiragkyal commented Feb 28, 2024

Uh oh!

openshift-ci-robot commented Feb 28, 2024

Uh oh!

chiragkyal commented Apr 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Apr 26, 2024

Uh oh!

chiragkyal commented Apr 26, 2024

Uh oh!

chiragkyal commented Apr 26, 2024

Uh oh!

chiragkyal commented Apr 26, 2024

Uh oh!

alebedev87 commented Apr 30, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chiragkyal commented May 8, 2024

Uh oh!

openshift-ci-robot commented May 9, 2024 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

openshift-ci-robot commented May 9, 2024 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chiragkyal commented May 10, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chiragkyal commented May 14, 2024

chiragkyal commented Feb 28, 2024 •

edited

Loading

chiragkyal commented Apr 26, 2024 •

edited

Loading

openshift-ci-robot commented May 9, 2024 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented May 9, 2024 •

edited by openshift-ci bot

Loading