Skip to content
This repository was archived by the owner on Nov 6, 2020. It is now read-only.

Conversation

@JulianKahnert
Copy link
Contributor

Hi,
I wanted to improve the readability of the Pods/Services etc. and skip the lookups of hashes when debugging or looking at the monitoring system.

I did not changed the getPVCName method, because that might break the StatefulSets, right!?

Do you think that we can use this as a drop in replacement or do the changes break a deployment?

@fredlahde
Copy link
Contributor

Good work, but k8s is a bit bitchy about names (and oddly enough contrary to RFC-1123). So here are a few considerations:

  • As per this error message, everything that gets resolved via k8s's internal DNS (service names, pod names for example) has to be lower case alphanumeric. toSlug() does not currently account for this
  • Concatenating autoCD.getServiceName() to a name is IMO unsafe, since we do not validate getServiceName for DNS conformity. We should crash with a meaningful error message if we find characters which are not allowed
  • IIRC there is a limit of 64 characters for DNS names inside of k8s. I can't find a source for this currently, but I'm quite sure that I've seen corresponding error messages in the wild

I can't recall why and if we need to hash() in getPVCName. Frankly, I have yet to see a use case where we needed to track down a specific PVC, so I'd argue that we cant just leave the hashed names there.

@fredlahde
Copy link
Contributor

More considerations:

As written here, names are obligated to:

  • be no longer than 63 characters (which sums up to 64 bytes with the null byte at the end)
  • have to start and end with an alphanumeric character

This has to be somehow guaranteed by toSlug()

@JulianKahnert
Copy link
Contributor Author

Thanks for your feedback!
I addded it to: 3c5d5a8

Any other considerations?

@larsgrah
Copy link
Contributor

In order to merge this we need a way to redeploy and clean all namespaces that are managed by autocd, also this is quite a big change in a fundamental way autocd works, I'd only be comfortable merging if we can test this extensively with some edgecases @JulianKahnert

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants