Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: align kubernetes cluster name validation with EKS #64

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 15 additions & 10 deletions bottlerocket-settings-models/modeled-types/src/kubernetes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -311,14 +311,20 @@ mod test_kubernetes_taint_value {

/// KubernetesClusterName represents a string that contains a valid Kubernetes cluster name. It
/// stores the original string and makes it accessible through standard traits.
// Note: I was unable to find the rules for cluster naming. We know they have to fit into label
// values, because of the common cluster-name label, but they also can't be empty. This combines
// those two characteristics into a new type, until we find an explicit syntax.
// Note: this uses the EKS rules for cluster naming.
#[derive(Debug, Clone, Eq, PartialEq, Hash)]
pub struct KubernetesClusterName {
inner: String,
}

lazy_static! {
pub(crate) static ref KUBERNETES_CLUSTER_NAME: Regex = Regex::new(
// follow EKS cluster name requirements: https://docs.aws.amazon.com/eks/latest/APIReference/API_CreateCluster.html#API_CreateCluster_RequestSyntax
r"^[0-9A-Za-z][A-Za-z0-9\-_]{0,99}$"
)
.unwrap();
}

impl TryFrom<&str> for KubernetesClusterName {
type Error = error::Error;

Expand All @@ -331,13 +337,12 @@ impl TryFrom<&str> for KubernetesClusterName {
}
);
ensure!(
KubernetesLabelValue::try_from(input).is_ok(),
error::InvalidClusterNameSnafu {
name: input,
msg: "cluster names must be valid Kubernetes label values"
KUBERNETES_CLUSTER_NAME.is_match(input),
error::BigPatternSnafu {
thing: "Kubernetes cluster name",
input
}
);

Ok(KubernetesClusterName {
inner: input.to_string(),
})
Expand All @@ -353,14 +358,14 @@ mod test_kubernetes_cluster_name {

#[test]
fn good_cluster_names() {
for ok in &["more-chars_here.now", &"a".repeat(63)] {
for ok in &["more-chars_here-123", &"a".repeat(100)] {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

. no longer being an allowed character is the only notable change in this PR, I think

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe a couple more test cases since we now allow trailing dashes and underscores:

Suggested change
for ok in &["more-chars_here-123", &"a".repeat(100)] {
for ok in &["more-chars_here-123", "trailing-dash-", "under_score_", &"a".repeat(100)] {

KubernetesClusterName::try_from(*ok).unwrap();
}
}

#[test]
fn bad_values() {
for err in &["", ".bad", "bad.", &"a".repeat(64)] {
for err in &["", "bad.", "-bad", "_bad", "very bad", &"a".repeat(101)] {
KubernetesClusterName::try_from(*err).unwrap_err();
}
}
Expand Down
19 changes: 19 additions & 0 deletions deny.toml
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,25 @@ allow = [
exceptions = [
{ name = "generational-arena", allow = ["MPL-2.0"] },
{ name = "unicode-ident", allow = ["MIT", "Apache-2.0", "Unicode-DFS-2016"] },
{ name = "icu_collections", allow = ["Unicode-3.0"] },
{ name = "icu_locid", allow = ["Unicode-3.0"] },
{ name = "icu_locid_transform", allow = ["Unicode-3.0"] },
{ name = "icu_locid_transform_data", allow = ["Unicode-3.0"] },
{ name = "icu_normalizer", allow = ["Unicode-3.0"] },
{ name = "icu_normalizer_data", allow = ["Unicode-3.0"] },
{ name = "icu_properties", allow = ["Unicode-3.0"] },
{ name = "icu_properties_data", allow = ["Unicode-3.0"] },
{ name = "icu_provider", allow = ["Unicode-3.0"] },
{ name = "icu_provider_macros", allow = ["Unicode-3.0"] },
{ name = "litemap", allow = ["Unicode-3.0"] },
{ name = "tinystr", allow = ["Unicode-3.0"] },
{ name = "writeable", allow = ["Unicode-3.0"] },
{ name = "yoke", allow = ["Unicode-3.0"] },
{ name = "yoke-derive", allow = ["Unicode-3.0"] },
{ name = "zerofrom", allow = ["Unicode-3.0"] },
{ name = "zerofrom-derive", allow = ["Unicode-3.0"] },
{ name = "zerovec", allow = ["Unicode-3.0"] },
{ name = "zerovec-derive", allow = ["Unicode-3.0"] },
]

[bans]
Expand Down