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

Creating an AKS cluster or properties with uppercase letters causes failure #4699

Closed
dtzar opened this issue Apr 2, 2024 · 12 comments · Fixed by #4725
Closed

Creating an AKS cluster or properties with uppercase letters causes failure #4699

dtzar opened this issue Apr 2, 2024 · 12 comments · Fixed by #4725
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.

Comments

@dtzar
Copy link
Contributor

dtzar commented Apr 2, 2024

/kind bug

What steps did you take and what happened:
Created an extension with an uppercase name. It failed to create with this message:

    Message:               extension failed to create or update. err: failed to create resource default/CiliumEnterprise (service: extension): Extension.kubernetesconfiguration.azure.com "CiliumEnterprise" is invalid: metadata.name: Invalid value: "CiliumEnterprise": a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')
    Reason:                Failed
    Severity:              Error
    Status:                False
    Type:                  Ready
    Last Transition Time:  2024-04-02T19:15:40Z

Created a cluster and failed with this:

Error from server (Invalid): error when creating "argocluster.yaml": Cluster.cluster.x-k8s.io "Yargoaks" is invalid: metadata.name: Invalid value: "Yargoaks": a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')

What did you expect to happen:
minimum - documentation stating uppercase letters are not supported at all
best - Auto-converting uppercase letters to lowercase and just working with informational message

Anything else you would like to add:
[Miscellaneous information that will assist in solving the issue.]

Environment:

  • cluster-api-provider-azure version: 1.14.1
  • Kubernetes version: (use kubectl version): 1.29.2
  • OS (e.g. from /etc/os-release): WSL / docker desktop
@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Apr 2, 2024
@mboersma mboersma added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Apr 4, 2024
@nojnhuh
Copy link
Contributor

nojnhuh commented Apr 4, 2024

@willie-yao can you PTAL?

@willie-yao
Copy link
Contributor

/assign

@willie-yao
Copy link
Contributor

Did some testing and it looks like not allowing uppercase letters isn't caused by ASO. Same errors show up on release v1.10 which didn't include ASO. Is this still something we want to add support for?

@nojnhuh
Copy link
Contributor

nojnhuh commented Apr 11, 2024

@willie-yao Can you share the template you were testing with?

@willie-yao
Copy link
Contributor

@nojnhuh I just used to default AKS template and replaced ${CLUSTER_NAME} with capital letters

@willie-yao
Copy link
Contributor

Seems like this validation is done by kubernetes itself from the metadata.name field which is something CAPZ can't control. Should we just settle for documenting this for now? https://github.com/kubernetes/kubernetes/blob/9791f0d1f39f3f1e0796add7833c1059325d5098/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go#L205

@nojnhuh
Copy link
Contributor

nojnhuh commented Apr 12, 2024

@willie-yao I think the original case here is with an extension name that contains capital letters. It looks like CAPZ isn't validating the name of the extension in the AzureManagedControlPlane, but when CAPZ goes to create the ASO extension resource, it uses the invalid name. I think CAPZ should be able to create the ASO resource with a valid kubernetes name in that case.

@jackfrancis
Copy link
Contributor

A capital letter in a name property violates a core Kubernetes contract. For example:

Francisbook:cluster-api-provider-azure jackfrancis$ cat spec-with-capitalized-names.yaml 
apiVersion: apps/v1
kind: Deployment
metadata:
  name: PHP-apache
spec:
  selector:
    matchLabels:
      run: php-apache
  template:
    metadata:
      labels:
        run: php-apache
    spec:
      containers:
      - name: php-apache
        image: registry.k8s.io/hpa-example
        ports:
        - containerPort: 80
        resources:
          limits:
            cpu: 500m
          requests:
            cpu: 200m
---
apiVersion: v1
kind: Service
metadata:
  name: PHP-apache
  labels:
    run: php-apache
spec:
  ports:
  - port: 80
  selector:
    run: php-apache

Francisbook:cluster-api-provider-azure jackfrancis$ k apply -f spec-with-capitalized-names.yaml 
Error from server (Invalid): error when creating "spec-with-capitalized-names.yaml": Deployment.apps "PHP-apache" is invalid: metadata.name: Invalid value: "PHP-apache": a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')
Error from server (Invalid): error when creating "spec-with-capitalized-names.yaml": Service "PHP-apache" is invalid: metadata.name: Invalid value: "PHP-apache": a DNS-1035 label must consist of lower case alphanumeric characters or '-', start with an alphabetic character, and end with an alphanumeric character (e.g. 'my-name',  or 'abc-123', regex used for validation is '[a-z]([-a-z0-9]*[a-z0-9])?')

It wouldn't be a terrible idea to document this, although strictly speaking this is not a CAPZ error.

It doesn't seem like mutating the name during resource creation time is a great idea (maybe I'm missing something?). The idiomatic thing would be to inherit the Kubernetes data contract here and let folks know right away (via admission webhook) that their spec is invalid.

@dtzar
Copy link
Contributor Author

dtzar commented Apr 16, 2024

Yes, I think that if we document this somewhere on the book and don't allow people to provision something immediately returning a useful message as such would be a good user experience. Allowing you to provision it and then having to look in debug logs is not the best.

@willie-yao
Copy link
Contributor

We already don't allow people to provision clusters with upper case characters as seen in the AzureCluster webhook:

fmt.Sprintf("Cluster Name doesn't match regex %s, can contain only lowercase alphanumeric characters and '-', must start/end with an alphanumeric character",

I've also added a note in the notes in my PR. @jackfrancis just following up from our discussions, should we still allow users to use capital letters specifically for AKS Extensions, or just strictly enforce lower-case characters?

@SimonKienzler
Copy link

I encountered a similar error while working through the ClusterAPI quick start for Azure: I'm not in control of the Azure ResourceGroup name (I do not manage the subscription myself). The Azure Provider let's me set a custom name using the AZURE_RESOURCE_GROUP environment variable. So far, so good.

Unfortunately, the name of the ResourceGroup provided to me contains underscores. The Azure Provider tries to use this name as the metadata.name for the ResourceGroup.resources.azure.com CR created during cluster creation. This fails for the reasons mentioned above (it's not an RFC 1123 compliant name).

Is this something that the Azure Provider should be able to support, e.g. by converting ResourceGroup names to all lowercase and replace _ with - before using them as resource names within Kubernetes? Or would you just consider it a requirement that the Azure ResourceGroup needs to be RFC 1123 compliant to work with CAPI?

@willie-yao
Copy link
Contributor

We discussed in CAPZ office hours that the transition to ASO may have introduced a regression with naming as now resources like resource groups, vnets, etc are backed by Kubernetes CRDs, which adds the RFC 1123 restriction. We will make sure to fix this soon!

@jackfrancis jackfrancis modified the milestones: v1.15, next May 2, 2024
@willie-yao willie-yao added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 6, 2024
@dtzar dtzar removed this from the next milestone Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

7 participants