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

Node.of(construct).uniqueId is not strictly compatible with the K8s labels #323

Closed
2 tasks done
foriequal0 opened this issue Sep 22, 2020 · 0 comments · Fixed by #326
Closed
2 tasks done

Node.of(construct).uniqueId is not strictly compatible with the K8s labels #323

foriequal0 opened this issue Sep 22, 2020 · 0 comments · Fixed by #326
Assignees
Labels
bug Something isn't working effort/medium 1 week tops

Comments

@foriequal0
Copy link
Contributor

foriequal0 commented Sep 22, 2020

Description of the feature or enhancement:

I can find some usages of Node.of(construct).uniqueId across the codebase and examples. However, it is bound to CloudFormation's resource name rules and it isn't strictly compatable with Kubernetes' name/labels rules.
It might be compatible if the resource name is short enough to be within k8s' length limitations, but you still bear the fact that Node.of(construct).uniqueId gives you a uniqueidbutwithbarelyreadablebecauseitlacksofseparatorcharactorsA8322FCE when you use a snake-case for resources names.

There is already Names.toDnsLabel and Chart.generateObjectName using it. However, labels are superset of dns labels.

Use Case:

I can find uniqueId in 8 files:

Potentially invalid

https://github.com/awslabs/cdk8s/blob/d863232e4bf8a6dfcdd11e32b18b03586a4b766d/packages/cdk8s-plus/src/deployment.ts#L208
They are examples, but it should be changed considering the fact that people are blindfoldingly conpy-paste them.
https://github.com/awslabs/cdk8s/blob/85cf6313c20771b718c19ee6085afc23cd787311/examples/typescript/podinfo/lib/deployment.ts#L92-L95
https://github.com/awslabs/cdk8s/blob/85cf6313c20771b718c19ee6085afc23cd787311/examples/typescript/web-service/web-service.ts#L33
https://github.com/awslabs/cdk8s/blob/0f70a4fa0a471b27b3c75e25d0609935060b44ed/docs/getting-started/typescript.md#L285

It would be better to be changed for the readibility

https://github.com/awslabs/cdk8s/blob/2ae890a92cf67be1e1a5478102dd6003e446440f/packages/cdk8s/src/app.ts#L51
https://github.com/awslabs/cdk8s/blob/2ae890a92cf67be1e1a5478102dd6003e446440f/packages/cdk8s/src/dependency.ts#L160
Okay-ish for internal use
https://github.com/awslabs/cdk8s/blob/2ae890a92cf67be1e1a5478102dd6003e446440f/packages/cdk8s/src/dependency.ts#L26-L32

Proposed Solution:

It would be better to have a separate function for the label.

...
const MAX_LABEL_LEN = 63;
const INVALIDATE_LABEL = /[^0-9a-zA-Z-_.]/
...
export class Names {
...
    public static toLabel(path: string, delim: string = '-', maxLen: number = MAX_LABEL_LEN) {
        if (maxLen < HASH_LEN) {
          throw new Error(`minimum max length for label is ${HASH_LEN} (required for hash)`);
        }

        if (INVALIDATE_LABEL.test(delim)) {
          throw new Error(`delim should not contain '[^0-9a-zA-Z-_.]'`);
        }

        let components = path.split('/');

        // special case: if we only have one component in our path and it adheres to DNS_NAME, we don't decorate it
        if (components.length === 1 && !INVALIDATE_LABEL.test(components[0]) && components[0].length <= maxLen) {
          return components[0];
        }

        components = components.map(normalizeToLabel);

        components.push(calcHash(path, HASH_LEN));

        return components
          .reverse()
          .filter(omitDuplicates)
          .join('/')
          .slice(0, 63)
          .split('/')
          .reverse()
          .filter(x => x)
          .join(delim);
    }
...
}

function normalizeToLabel(c: string) {
    return c
        .replace(/[^0-9a-zA-Z-_.]/g, ''); // remove non-allowed characters
}
...

And I think it is cumbersome to use

Names.toLabel(Node.of(construct).path);

Names.toDnsLabel(Node.of(construct).path)
// which is
Chart.of(construct).generateObjectName(construct);

Overloading them would be helpful?

...
    public static toDnsLabel(pathOrConstruct: string | IConstruct, maxLen?: number): string {
        ...
        const path = typeof pathOrConstruct === "string" ? pathOrConstruct : Node.of(pathOrConstruct).path;
        let components = path.split(".");
...

Other:

Relative issue: aws/constructs#250

Short question: Why normalizeToDnsName limits components to maxLen? https://github.com/awslabs/cdk8s/blob/473b5ef7f442b932e9c4c4356945b91e8e1bc4e4/packages/cdk8s/src/names.ts#L79

  • 👋 I may be able to implement this feature request
  • ⚠️ This feature might incur a breaking change

This is a 🚀 Feature Request

@foriequal0 foriequal0 added feature-request New/Enhanced functionality wanted needs-triage Priority and effort undetermined yet labels Sep 22, 2020
@foriequal0 foriequal0 changed the title Node.of(construct).uniqueId nor Names.toDnsLabel(Node.of(construct).path) is not strictly compatible with the K8s labels Node.of(construct).uniqueId and Names.toDnsLabel(Node.of(construct).path) are not strictly compatible with the K8s labels Sep 22, 2020
@foriequal0 foriequal0 changed the title Node.of(construct).uniqueId and Names.toDnsLabel(Node.of(construct).path) are not strictly compatible with the K8s labels Node.of(construct).uniqueId is not strictly compatible with the K8s labels Sep 23, 2020
@iliapolo iliapolo added effort/medium 1 week tops p1 bug Something isn't working and removed feature-request New/Enhanced functionality wanted needs-triage Priority and effort undetermined yet labels Sep 27, 2020
@mergify mergify bot closed this as completed in #326 Oct 14, 2020
mergify bot pushed a commit that referenced this issue Oct 14, 2020
`Node.of(construct).uniqueId` is bound to CloudFormation's resource logical ID rule, which is not strictly compatible with the K8s label rule. It might lead to invalid K8s manifests.
I've changed all `Node.of(construct).uniqueId` with `Node.of(construct).path`, or `Names.toDnsLabel(Node.of(construct).path)` if needed.
Since K8s DNS label rule are subset of label key name segment/value rule, I've added `Names.toLabelValue` and used it for selector labels.

fixes: #323

BREAKING CHANGE: cdk8s-plus's value of a label `cdk8s.deployment` of Pods are changed

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working effort/medium 1 week tops
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants