fix!: Make fields on S3 structs mandatory, add S3 helpers

[sbernauer]

Description

Reviving #679 (replacing it)

Example usage is in https://github.com/stackabletech/trino-operator/tree/chore/new-s3-structs

Definition of Done Checklist

• Not all of these items are applicable to all PRs, the author should update this template to only leave the boxes in that are relevant
• Please make sure all these things are done and tick the boxes

# Author
- [ ] Changes are OpenShift compatible
- [ ] CRD changes approved
- [ ] Integration tests passed (for non trivial changes)

# Reviewer
- [ ] Code contains useful comments
- [ ] (Integration-)Test cases added
- [ ] Documentation added or updated
- [ ] Changelog updated
- [ ] Cargo.toml only contains references to git tags (not specific commits or branches)

# Acceptance
- [ ] Feature Tracker has been updated
- [ ] Proper release label has been added

[NickLarsenNZ]

Partial review (as I see changes are still coming in)

[NickLarsenNZ]

Partial review again :)

[Techassi]

I didn't look at the S3 stuff super closely (I can if you want @sbernauer). The TLS code looks fine (as it is copy+pasted from the previous location).

All other comments are minor notes / suggestions.

[Techassi]

LGTM, only minor changes.

[Techassi]

LGTM overall, just three small comments.

[nightkr]

This is effectively dropping the validation regex from the composite type.

[sbernauer]

You're right. So far we don't have any regex on HostName, but instead let IpAddr::parse do it's thing and try to parse it as domain name in case it#s not an IP.
We should be able to come up with an regex that accepts a DomainName, IPv4 or IPv6 address. But do you think it's worth the effort?

[nightkr]

That's fair.

[nightkr]

Do we actually care about distinguishing these cases?

[sbernauer]

I personally modeled it as String, but @Techassi suggested using an enum instead, which sounded fine to me. No preference from me.

[Techassi]

The RFC calls for these two distinct variants of a hostname.

Why should we remove all the value we get from using proper types by just using a plain string? We could simplify the DomainName variant to use String, but I would definitely keep the enum.

[nightkr]

Ultimately the question here is the purpose of the type. Detailed types makes sense for inspecting and manipulating the hostname. The original intent for Hostname was the much smaller scope of "make sure this opaque looks close enough to a hostname that it won't fuck up the syntax of the config file it's string-templated into".

[Techassi]

looks close enough

If we want to achieve this, sure. But then why reference RFC 1123 and only vaguely conform to it. If we want to conform to various RFCs, we should do so as closely as possible.

Another region where we try to conform to an RFC (1035), we also completely miss the mark when validating domain labels. So I'm just trying to avoid the same thing happening here (even tho the domain name is not 100% valid).

[nightkr]

This is dropping the context for why validation failed.

[sbernauer]

At this code point we have access to an IpAdd::parse error and and regex validation error.

        if let Ok(ip) = value.parse::<IpAddr>() {
            return Ok(HostName::IpAddress(ip));
        }

        if let Ok(domain_name) = value.parse() {
            return Ok(HostName::DomainName(domain_name));
        };

        InvalidHostnameSnafu {
            hostname: value.to_owned(),
        }
        .fail()

Should we capture both errors and put them both in? E.g. "needs to be an ip, which it does'nt because of {error1}, and also is no domain name because of [error2}"?

[nightkr]

That's one way, but it loses the error chain. Another alternative would be to guess based on a cursory inspection.. something like:

if value.starts_with(char::is_ascii_digit) || value.contains(':') {
    Ok(HostName::IpAddress(value.parse().context(InvalidIpAddressSnafu { hostname: value })?))
} else {
    Ok(HostName::DomainName(value.parse().context(InvalidDomainNameSnafu { hostname: value })?))
}

[Techassi]

We could also do something like:

match value.parse::<IpAddr>() {
    Ok(ip_addr) => return Ok(Hostname::IpAddr(ip_addr)),
    Err(ip_err) => match value.parse::<DomainName>() {
        Ok(dn) => return Ok(Hostname::DomainName(dn)),
        Err(dn_err) => // return both errors
    }
}

[nightkr]

Yes that's effectively what @sbernauer suggested. There problem is that there's no way for an Error to have multiple source()s, so we'd have to pick one primary and render the other one as unawkwardly as we can.

[Techassi]

there's no way for an Error to have multiple source()s, so we'd have to pick one primary and render the other one as unawkwardly as we can.

That is indeed very true (and unfortunate). The cursed inspection is also kinda meh tho. I don't know if it helps (I assume not), RFC 1123 states that the host name should first tried to be parsed as an IP address. So at least we do the correct thing RFC-wise.