Skip to content

Commit 7764ea6

Browse files
wkingwking
committed
pkg/risk/adminack: Extract the first valid https:// URI from message
Conditional update risks require a URL [1]. I'd previously just used a placeholder FIXME while building out the broader logic. But now that the broader logic is setting in, I'm coming back to extract a useful URL from the admin-gate message. These admin-gate messages were already bubbling up into Upgradeable messages, and the web-console linkifies them using react-linkify [2], which in turn uses linkify-it [3], which has some fairly complex regular-expression handling [4]. Rather than port that logic to Go, I'm just assuming that admin-gate authors are considerate enough to leave whitespace after the URI, and that when they don't, users will be savvy enough to find the URI in the message themselves. The real-world gate I'm using to seed the new test-cases is from [5]. [1]: https://github.com/openshift/api/blob/73d7ca93df6d0a1b02f5533ce20f68d27869a1fe/config/v1/types_cluster_version.go#L909-L913 [2]: https://github.com/openshift/console/blob/c94391787806efd0ca67206cdf5cdb92f6f0e8a5/frontend/public/components/app.tsx#L16 [3]: https://github.com/tasti/react-linkify/blob/325cb5e43300d7ada5bdf8d2849783d98fcd3c2c/src/decorators/defaultMatchDecorator.js#L3 [4]: https://github.com/markdown-it/linkify-it/blob/8e887d5bace3f5b09b1d1f70492fa0364ef1793d/lib/re.mjs [5]: openshift/machine-config-operator@f249bea#diff-436a15d11f20b12603ca30029771a45151c594aa2860dc68ac1f7c79c7a5d123R16-R17
1 parent a684bff commit 7764ea6

2 files changed

Lines changed: 90 additions & 10 deletions

File tree

‎pkg/risk/adminack/adminack.go‎

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"context"
77
"errors"
88
"fmt"
9+
"net/url"
910
"regexp"
1011
"slices"
1112
"strings"
@@ -32,6 +33,7 @@ const (
3233
)
3334

3435
var adminAckGateRegexp = regexp.MustCompile(adminAckGateFmt)
36+
var uriRegexp = regexp.MustCompile("https://[^[:space:]]*")
3537

3638
type adminAck struct {
3739
name string
@@ -169,6 +171,7 @@ func gateApplicableToCurrentVersion(gateName string, currentVersion semver.Versi
169171
}
170172

171173
func checkAdminGate(gateName string, gateValue string, riskNamePrefix string, currentVersion semver.Version, ackConfigmap *corev1.ConfigMap) *configv1.ConditionalUpdateRisk {
174+
uri := "https://docs.redhat.com/en/documentation/openshift_container_platform/"
172175
riskName := fmt.Sprintf("%s%s", riskNamePrefix, gateName)
173176
if applies, err := gateApplicableToCurrentVersion(gateName, currentVersion); err == nil {
174177
if !applies {
@@ -178,7 +181,7 @@ func checkAdminGate(gateName string, gateValue string, riskNamePrefix string, cu
178181
return &configv1.ConditionalUpdateRisk{
179182
Name: riskName,
180183
Message: fmt.Sprintf("%s configmap gate %q is invalid: %v", internal.AdminGatesConfigMap, gateName, err),
181-
URL: "https://docs.redhat.com/en/documentation/openshift_container_platform/",
184+
URL: uri,
182185
MatchingRules: []configv1.ClusterCondition{{Type: "Always"}},
183186
}
184187

@@ -187,15 +190,32 @@ func checkAdminGate(gateName string, gateValue string, riskNamePrefix string, cu
187190
return &configv1.ConditionalUpdateRisk{
188191
Name: riskName,
189192
Message: fmt.Sprintf("%s configmap gate %s must contain a non-empty value.", internal.AdminGatesConfigMap, gateName),
190-
URL: "https://docs.redhat.com/en/documentation/openshift_container_platform/",
193+
URL: uri,
191194
MatchingRules: []configv1.ClusterCondition{{Type: "Always"}},
192195
}
193196
}
197+
198+
for _, match := range uriRegexp.FindAllString(gateValue, -1) {
199+
if _, err := url.Parse(match); err == nil {
200+
uri = match
201+
break
202+
}
203+
}
204+
205+
if ackConfigmap == nil {
206+
return &configv1.ConditionalUpdateRisk{
207+
Name: riskName,
208+
Message: gateValue,
209+
URL: uri,
210+
MatchingRules: []configv1.ClusterCondition{{Type: "Always"}},
211+
}
212+
}
213+
194214
if val, ok := ackConfigmap.Data[gateName]; !ok || val != "true" {
195215
return &configv1.ConditionalUpdateRisk{
196216
Name: riskName,
197217
Message: gateValue,
198-
URL: "https://example.com/FIXME-look-for-a-URI-in-the-message",
218+
URL: uri,
199219
MatchingRules: []configv1.ClusterCondition{{Type: "Always"}},
200220
}
201221
}

‎pkg/risk/adminack/adminack_test.go‎

Lines changed: 67 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ func Test_New(t *testing.T) {
6969
expectedRisks: []configv1.ConditionalUpdateRisk{{
7070
Name: "AdminAck-ack-4.21-test",
7171
Message: "Description.",
72-
URL: "https://example.com/FIXME-look-for-a-URI-in-the-message",
72+
URL: "https://docs.redhat.com/en/documentation/openshift_container_platform/",
7373
MatchingRules: []configv1.ClusterCondition{{Type: "Always"}},
7474
}},
7575
},
@@ -86,7 +86,7 @@ func Test_New(t *testing.T) {
8686
expectedRisks: []configv1.ConditionalUpdateRisk{{
8787
Name: "AdminAck-ack-4.21-test",
8888
Message: "Description A.",
89-
URL: "https://example.com/FIXME-look-for-a-URI-in-the-message",
89+
URL: "https://docs.redhat.com/en/documentation/openshift_container_platform/",
9090
MatchingRules: []configv1.ClusterCondition{{Type: "Always"}},
9191
}},
9292
},
@@ -104,7 +104,7 @@ func Test_New(t *testing.T) {
104104
{
105105
Name: "AdminAck-ack-4.21-with-description",
106106
Message: "Description.",
107-
URL: "https://example.com/FIXME-look-for-a-URI-in-the-message",
107+
URL: "https://docs.redhat.com/en/documentation/openshift_container_platform/",
108108
MatchingRules: []configv1.ClusterCondition{{Type: "Always"}},
109109
},
110110
{
@@ -130,20 +130,20 @@ func Test_New(t *testing.T) {
130130
{
131131
Name: "AdminAck-ack-4.21-a",
132132
Message: "Description 1.",
133-
URL: "https://example.com/FIXME-look-for-a-URI-in-the-message",
133+
URL: "https://docs.redhat.com/en/documentation/openshift_container_platform/",
134134
MatchingRules: []configv1.ClusterCondition{{Type: "Always"}},
135135
},
136136
{
137137
Name: "AdminAck-ack-4.21-b",
138138
Message: "Description 2.",
139-
URL: "https://example.com/FIXME-look-for-a-URI-in-the-message",
139+
URL: "https://docs.redhat.com/en/documentation/openshift_container_platform/",
140140
MatchingRules: []configv1.ClusterCondition{{Type: "Always"}},
141141
},
142142

143143
{
144144
Name: "AdminAck-ack-4.21-c",
145145
Message: "Description 3.",
146-
URL: "https://example.com/FIXME-look-for-a-URI-in-the-message",
146+
URL: "https://docs.redhat.com/en/documentation/openshift_container_platform/",
147147
MatchingRules: []configv1.ClusterCondition{{Type: "Always"}},
148148
},
149149
},
@@ -176,7 +176,7 @@ func Test_New(t *testing.T) {
176176
expectedRisks: []configv1.ConditionalUpdateRisk{{
177177
Name: "AdminAck-ack-4.21-b",
178178
Message: "Description 2.",
179-
URL: "https://example.com/FIXME-look-for-a-URI-in-the-message",
179+
URL: "https://docs.redhat.com/en/documentation/openshift_container_platform/",
180180
MatchingRules: []configv1.ClusterCondition{{Type: "Always"}},
181181
}},
182182
},
@@ -452,6 +452,66 @@ func Test_gateApplicableToCurrentVersion(t *testing.T) {
452452
}
453453
}
454454

455+
func Test_checkAdminGate(t *testing.T) {
456+
riskNamePrefix := "AdminAck-"
457+
tests := []struct {
458+
name string
459+
gateName string
460+
gateValue string
461+
currentVersion string
462+
ackConfigmap *corev1.ConfigMap
463+
expectedRisk *configv1.ConditionalUpdateRisk
464+
}{
465+
{
466+
name: "valid gate not acknowledged",
467+
gateName: "ack-4.20-sigstore-in-4.21",
468+
gateValue: "This cluster has mirrors configured. 4.21 will require Sigstore signatures for quay.io/openshift-release-dev/ocp-release release verification. Please ensure that any registries configured as mirrors of quay.io/openshift-release-dev/ocp-release images contain the Sigstore signature images associated with any release images before updating to 4.21. See https://docs.redhat.com/en/documentation/openshift_container_platform/4.20/html/nodes/nodes-sigstore-using.html#nodes-sigstore-prepare-for-4.21_nodes-sigstore-using for more details.",
469+
currentVersion: "4.20.15",
470+
expectedRisk: &configv1.ConditionalUpdateRisk{
471+
Name: "AdminAck-ack-4.20-sigstore-in-4.21",
472+
Message: "This cluster has mirrors configured. 4.21 will require Sigstore signatures for quay.io/openshift-release-dev/ocp-release release verification. Please ensure that any registries configured as mirrors of quay.io/openshift-release-dev/ocp-release images contain the Sigstore signature images associated with any release images before updating to 4.21. See https://docs.redhat.com/en/documentation/openshift_container_platform/4.20/html/nodes/nodes-sigstore-using.html#nodes-sigstore-prepare-for-4.21_nodes-sigstore-using for more details.",
473+
URL: "https://docs.redhat.com/en/documentation/openshift_container_platform/4.20/html/nodes/nodes-sigstore-using.html#nodes-sigstore-prepare-for-4.21_nodes-sigstore-using",
474+
MatchingRules: []configv1.ClusterCondition{{Type: "Always"}},
475+
},
476+
},
477+
{
478+
name: "valid gate not acknowledged, newline at end of URI",
479+
gateName: "ack-4.20-sigstore-in-4.21",
480+
gateValue: "This cluster has mirrors configured. See https://example.com/\nfor details.",
481+
currentVersion: "4.20.15",
482+
expectedRisk: &configv1.ConditionalUpdateRisk{
483+
Name: "AdminAck-ack-4.20-sigstore-in-4.21",
484+
Message: "This cluster has mirrors configured. See https://example.com/\nfor details.",
485+
URL: "https://example.com/",
486+
MatchingRules: []configv1.ClusterCondition{{Type: "Always"}},
487+
},
488+
},
489+
{
490+
name: "valid gate acknowledged",
491+
gateName: "ack-4.20-sigstore-in-4.21",
492+
gateValue: "This cluster has mirrors configured. 4.21 will require Sigstore signatures for quay.io/openshift-release-dev/ocp-release release verification. Please ensure that any registries configured as mirrors of quay.io/openshift-release-dev/ocp-release images contain the Sigstore signature images associated with any release images before updating to 4.21. See https://docs.redhat.com/en/documentation/openshift_container_platform/4.20/html/nodes/nodes-sigstore-using.html#nodes-sigstore-prepare-for-4.21_nodes-sigstore-using for more details.",
493+
currentVersion: "4.20.15",
494+
ackConfigmap: &corev1.ConfigMap{
495+
Data: map[string]string{"ack-4.20-sigstore-in-4.21": "true"},
496+
},
497+
},
498+
}
499+
for _, tt := range tests {
500+
{
501+
t.Run(tt.name, func(t *testing.T) {
502+
version, err := semver.Parse(tt.currentVersion)
503+
if err != nil {
504+
t.Fatalf("unable to parse version %q: %v", tt.currentVersion, err)
505+
}
506+
risk := checkAdminGate(tt.gateName, tt.gateValue, riskNamePrefix, version, tt.ackConfigmap)
507+
if diff := cmp.Diff(tt.expectedRisk, risk); diff != "" {
508+
t.Errorf("risk mismatch (-want +got):\n%s", diff)
509+
}
510+
})
511+
}
512+
}
513+
}
514+
455515
// waits for admin ack configmap changes
456516
func waitForCm(t *testing.T, cmChan chan *corev1.ConfigMap) {
457517
select {

0 commit comments

Comments
 (0)