Skip to content

Commit bb5e4af

Browse files
siegfriedwebersiegfriedweber
committed
Fix regexes for attributed string types; Add unit tests for validation
1 parent d492e1e commit bb5e4af

5 files changed

Lines changed: 125 additions & 29 deletions

File tree

‎rust/operator-binary/src/controller/build/role_builder.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -612,6 +612,7 @@ mod tests {
612612
"spec": {
613613
"className": "external-stable",
614614
"extraPodSelectorLabels": {},
615+
"objectOverrides": [],
615616
"ports": [
616617
{
617618
"name": "http",

‎rust/operator-binary/src/controller/build/role_group_builder.rs‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -453,9 +453,6 @@ impl<'a> RoleGroupBuilder<'a> {
453453
}
454454

455455
/// Returns the labels of OpenSearch nodes with the `cluster_manager` role.
456-
///
457-
/// As described in [`super::role_builder::RoleBuilder::build_seed_nodes_service`], this
458-
/// function will be changed or deleted.
459456
pub fn cluster_manager_labels(
460457
cluster: &ValidatedCluster,
461458
context_names: &ContextNames,

‎rust/operator-binary/src/controller/validate.rs‎

Lines changed: 113 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -341,6 +341,7 @@ mod tests {
341341
product_image_selection::ResolvedProductImage,
342342
resources::{CpuLimits, MemoryLimits, PvcConfig, Resources},
343343
},
344+
crd::listener::{self},
344345
deep_merger::ObjectOverrides,
345346
k8s_openapi::{
346347
api::core::v1::{
@@ -364,8 +365,8 @@ mod tests {
364365
use crate::{
365366
built_info,
366367
controller::{
367-
ContextNames, DereferencedObjects, ValidatedCluster, ValidatedLogging,
368-
ValidatedOpenSearchConfig,
368+
ContextNames, DereferencedObjects, ValidatedCluster, ValidatedDiscoveryEndpoint,
369+
ValidatedLogging, ValidatedOpenSearchConfig,
369370
},
370371
crd::{NodeRoles, OpenSearchKeystoreKey, v1alpha1},
371372
framework::{
@@ -375,9 +376,10 @@ mod tests {
375376
},
376377
role_utils::{GenericProductSpecificCommonConfig, RoleGroupConfig},
377378
types::{
379+
common::Port,
378380
kubernetes::{
379-
ConfigMapName, ListenerClassName, NamespaceName, SecretClassName, SecretKey,
380-
SecretName,
381+
ConfigMapName, Hostname, ListenerClassName, NamespaceName, SecretClassName,
382+
SecretKey, SecretName,
381383
},
382384
operator::{
383385
ClusterName, ControllerName, OperatorName, ProductName, ProductVersion,
@@ -389,11 +391,7 @@ mod tests {
389391

390392
#[test]
391393
fn test_validate_ok() {
392-
let dereferenced_objects = DereferencedObjects {
393-
maybe_discovery_service_listener: None,
394-
};
395-
396-
let result = validate(&context_names(), &cluster(), &dereferenced_objects);
394+
let result = validate(&context_names(), &cluster(), &dereferenced_objects());
397395

398396
assert_eq!(
399397
Some(ValidatedCluster::new(
@@ -610,7 +608,10 @@ mod tests {
610608
key: SecretKey::from_str_unsafe("my-keystore-file")
611609
}
612610
}],
613-
None
611+
Some(ValidatedDiscoveryEndpoint {
612+
hostname: Hostname::from_str_unsafe("my-opensearch.default.svc.cluster.local"),
613+
port: Port(9200),
614+
})
614615
)),
615616
result.ok()
616617
);
@@ -620,6 +621,7 @@ mod tests {
620621
fn test_validate_err_get_cluster_name() {
621622
test_validate_err(
622623
|cluster| cluster.metadata.name = None,
624+
|_| (),
623625
ErrorDiscriminants::GetClusterName,
624626
);
625627
}
@@ -628,6 +630,7 @@ mod tests {
628630
fn test_validate_err_get_cluster_namespace() {
629631
test_validate_err(
630632
|cluster| cluster.metadata.namespace = None,
633+
|_| (),
631634
ErrorDiscriminants::GetClusterNamespace,
632635
);
633636
}
@@ -636,6 +639,7 @@ mod tests {
636639
fn test_validate_err_get_cluster_uid() {
637640
test_validate_err(
638641
|cluster| cluster.metadata.uid = None,
642+
|_| (),
639643
ErrorDiscriminants::GetClusterUid,
640644
);
641645
}
@@ -648,6 +652,7 @@ mod tests {
648652
serde_json::from_str(r#"{"productVersion": "invalid product version"}"#)
649653
.expect("should be a valid ProductImage structure")
650654
},
655+
|_| (),
651656
ErrorDiscriminants::ResolveProductImage,
652657
);
653658
}
@@ -668,6 +673,7 @@ mod tests {
668673
.role_groups
669674
.insert("invalid role-group name".to_owned(), role_group);
670675
},
676+
|_| (),
671677
ErrorDiscriminants::ParseRoleGroupName,
672678
);
673679
}
@@ -690,6 +696,7 @@ mod tests {
690696
)]
691697
.into()
692698
},
699+
|_| (),
693700
ErrorDiscriminants::ValidateLoggingConfig,
694701
);
695702
}
@@ -703,6 +710,7 @@ mod tests {
703710
.cluster_config
704711
.vector_aggregator_config_map_name = None
705712
},
713+
|_| (),
706714
ErrorDiscriminants::GetVectorAggregatorConfigMapName,
707715
);
708716
}
@@ -714,6 +722,7 @@ mod tests {
714722
cluster.spec.nodes.config.config.graceful_shutdown_timeout =
715723
Some(Duration::from_secs(u64::MAX))
716724
},
725+
|_| (),
717726
ErrorDiscriminants::TerminationGracePeriodTooLong,
718727
);
719728
}
@@ -728,20 +737,91 @@ mod tests {
728737
)]
729738
.into()
730739
},
740+
|_| (),
731741
ErrorDiscriminants::ParseEnvironmentVariable,
732742
);
733743
}
734744

745+
#[test]
746+
fn test_validate_err_parse_listener_status_hostname() {
747+
test_validate_err(
748+
|_| (),
749+
|dereferenced_objects| {
750+
dereferenced_objects.maybe_discovery_service_listener =
751+
Some(listener::v1alpha1::Listener {
752+
metadata: ObjectMeta::default(),
753+
spec: listener::v1alpha1::ListenerSpec::default(),
754+
status: Some(listener::v1alpha1::ListenerStatus {
755+
ingress_addresses: Some(vec![listener::v1alpha1::ListenerIngress {
756+
address: "invalid hostname".to_owned(),
757+
address_type: listener::v1alpha1::AddressType::Hostname,
758+
ports: [("http".to_owned(), 9200)].into(),
759+
}]),
760+
..listener::v1alpha1::ListenerStatus::default()
761+
}),
762+
});
763+
},
764+
ErrorDiscriminants::ParseListenerStatusHostname,
765+
);
766+
}
767+
768+
#[test]
769+
fn test_validate_err_get_listener_status_port() {
770+
test_validate_err(
771+
|_| (),
772+
|dereferenced_objects| {
773+
dereferenced_objects.maybe_discovery_service_listener =
774+
Some(listener::v1alpha1::Listener {
775+
metadata: ObjectMeta::default(),
776+
spec: listener::v1alpha1::ListenerSpec::default(),
777+
status: Some(listener::v1alpha1::ListenerStatus {
778+
ingress_addresses: Some(vec![listener::v1alpha1::ListenerIngress {
779+
address: "my-opensearch.default.svc.cluster.local".to_owned(),
780+
address_type: listener::v1alpha1::AddressType::Hostname,
781+
// Validation should fail because the http port is expected.
782+
ports: [("transport".to_owned(), 9300)].into(),
783+
}]),
784+
..listener::v1alpha1::ListenerStatus::default()
785+
}),
786+
});
787+
},
788+
ErrorDiscriminants::GetListenerStatusPort,
789+
);
790+
}
791+
792+
#[test]
793+
fn test_validate_err_parse_listener_status_port() {
794+
test_validate_err(
795+
|_| (),
796+
|dereferenced_objects| {
797+
dereferenced_objects.maybe_discovery_service_listener =
798+
Some(listener::v1alpha1::Listener {
799+
metadata: ObjectMeta::default(),
800+
spec: listener::v1alpha1::ListenerSpec::default(),
801+
status: Some(listener::v1alpha1::ListenerStatus {
802+
ingress_addresses: Some(vec![listener::v1alpha1::ListenerIngress {
803+
address: "my-opensearch.default.svc.cluster.local".to_owned(),
804+
address_type: listener::v1alpha1::AddressType::Hostname,
805+
ports: [("http".to_owned(), -1)].into(),
806+
}]),
807+
..listener::v1alpha1::ListenerStatus::default()
808+
}),
809+
});
810+
},
811+
ErrorDiscriminants::ParseListenerStatusPort,
812+
);
813+
}
814+
735815
fn test_validate_err(
736-
f: fn(&mut v1alpha1::OpenSearchCluster) -> (),
816+
change_cluster: fn(&mut v1alpha1::OpenSearchCluster) -> (),
817+
change_dereferenced_objects: fn(&mut DereferencedObjects) -> (),
737818
expected_err: ErrorDiscriminants,
738819
) {
739820
let mut cluster = cluster();
740-
f(&mut cluster);
821+
change_cluster(&mut cluster);
741822

742-
let dereferenced_objects = DereferencedObjects {
743-
maybe_discovery_service_listener: None,
744-
};
823+
let mut dereferenced_objects = dereferenced_objects();
824+
change_dereferenced_objects(&mut dereferenced_objects);
745825

746826
let result = validate(&context_names(), &cluster, &dereferenced_objects);
747827

@@ -905,4 +985,22 @@ mod tests {
905985
status: None,
906986
}
907987
}
988+
989+
fn dereferenced_objects() -> DereferencedObjects {
990+
DereferencedObjects {
991+
maybe_discovery_service_listener: Some(listener::v1alpha1::Listener {
992+
metadata: ObjectMeta::default(),
993+
spec: listener::v1alpha1::ListenerSpec::default(),
994+
status: Some(listener::v1alpha1::ListenerStatus {
995+
ingress_addresses: Some(vec![listener::v1alpha1::ListenerIngress {
996+
// TODO Check if it is the FQDN
997+
address: "my-opensearch.default.svc.cluster.local".to_owned(),
998+
address_type: listener::v1alpha1::AddressType::Hostname,
999+
ports: [("http".to_owned(), 9200)].into(),
1000+
}]),
1001+
..listener::v1alpha1::ListenerStatus::default()
1002+
}),
1003+
}),
1004+
}
1005+
}
9081006
}

‎rust/operator-binary/src/framework/macros/attributed_string_type.rs‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -185,7 +185,7 @@ macro_rules! attributed_string_type {
185185
None
186186
},
187187
"pattern": match $name::REGEX {
188-
$crate::framework::macros::attributed_string_type::Regex::Expression(regex) => Some(std::format!("^{regex}$")),
188+
$crate::framework::macros::attributed_string_type::Regex::Expression(regex) => Some(regex),
189189
_ => None
190190
}
191191
})
@@ -513,7 +513,7 @@ mod tests {
513513
"test",
514514
(min_length = 2), // should set the minimum length to 2
515515
(max_length = 8), // should not affect the minimum length
516-
(regex = ".{4}"), // should not affect the minimum length
516+
(regex = "^.{4}$"), // should not affect the minimum length
517517
is_rfc_1035_label_name, // should be overruled by the greater min_length
518518
is_valid_label_value // should be overruled by the greater min_length
519519
}
@@ -550,7 +550,7 @@ mod tests {
550550
"test",
551551
(min_length = 2), // should not affect the maximum length
552552
(max_length = 8), // should set the maximum length to 8
553-
(regex = ".{4}"), // should not affect the maximum length
553+
(regex = "^.{4}$"), // should not affect the maximum length
554554
is_rfc_1035_label_name, // should be overruled by the lower max_length
555555
is_valid_label_value // should be overruled by the lower max_length
556556
}
@@ -587,15 +587,15 @@ mod tests {
587587
"test",
588588
(min_length = 2), // should not affect the regular expression
589589
(max_length = 8), // should not affect the regular expression
590-
(regex = "[est]{4}") // should set the regular expression to "[est]{4}"
590+
(regex = "^[est]{4}$") // should set the regular expression to "[est]{4}"
591591
}
592592

593593
#[test]
594594
fn test_attributed_string_type_regex_with_one_constraint() {
595595
type T = RegexWithOneConstraintTest;
596596

597597
T::test_example();
598-
assert_eq!(Regex::Expression("[est]{4}"), T::REGEX);
598+
assert_eq!(Regex::Expression("^[est]{4}$"), T::REGEX);
599599
assert_eq!(
600600
Err(ErrorDiscriminants::RegexNotMatched),
601601
T::from_str("t-st").map_err(ErrorDiscriminants::from)
@@ -608,7 +608,7 @@ mod tests {
608608
"test",
609609
(min_length = 2), // should not affect the regular expression
610610
(max_length = 8), // should not affect the regular expression
611-
(regex = "[est]{4}"), // should not be combinable with is_rfc_1123_dns_subdomain_name
611+
(regex = "^[est]{4}$"), // should not be combinable with is_rfc_1123_dns_subdomain_name
612612
is_rfc_1123_dns_subdomain_name // should not be combinable with regex
613613
}
614614

@@ -679,7 +679,7 @@ mod tests {
679679
"test",
680680
(min_length = 2),
681681
(max_length = 4),
682-
(regex = "[est-]+"),
682+
(regex = "^[est-]+$"),
683683
is_rfc_1035_label_name
684684
}
685685

@@ -785,7 +785,7 @@ mod tests {
785785
"test",
786786
(min_length = 4),
787787
(max_length = 8),
788-
(regex = "[est]+")
788+
(regex = "^[est]+$")
789789
}
790790

791791
#[test]

‎rust/operator-binary/src/framework/types/kubernetes.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ attributed_string_type! {
1919
(min_length = 1),
2020
// see https://github.com/kubernetes/kubernetes/blob/v1.34.1/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go#L435-L451
2121
(max_length = RFC_1123_SUBDOMAIN_MAX_LENGTH),
22-
(regex = "[-._a-zA-Z0-9]+")
22+
(regex = "^[-._a-zA-Z0-9]+$")
2323
}
2424

2525
attributed_string_type! {
@@ -47,7 +47,7 @@ attributed_string_type! {
4747
(min_length = 1),
4848
(max_length = 253),
4949
// see https://en.wikipedia.org/wiki/Hostname#Syntax
50-
(regex = "[a-zA-Z0-9]([-a-zA-Z0-9]{0,60}[a-zA-Z0-9])?(\\.[a-zA-Z0-9]([-a-zA-Z0-9]{0,60}[a-zA-Z0-9])?)*\\.?")
50+
(regex = "^[a-zA-Z0-9]([-a-zA-Z0-9]{0,60}[a-zA-Z0-9])?(\\.[a-zA-Z0-9]([-a-zA-Z0-9]{0,60}[a-zA-Z0-9])?)*\\.?$")
5151
}
5252

5353
attributed_string_type! {
@@ -105,7 +105,7 @@ attributed_string_type! {
105105
(min_length = 1),
106106
// see https://github.com/kubernetes/kubernetes/blob/v1.34.1/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go#L435-L451
107107
(max_length = RFC_1123_SUBDOMAIN_MAX_LENGTH),
108-
(regex = "[-._a-zA-Z0-9]+")
108+
(regex = "^[-._a-zA-Z0-9]+$")
109109
}
110110

111111
attributed_string_type! {

0 commit comments

Comments
 (0)