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

Do not use default as namespace for KAR templates if not supplied #1361

Merged
merged 1 commit into from
Mar 23, 2020

Conversation

hasheddan
Copy link
Member

Signed-off-by: hasheddan georgedanielmangum@gmail.com

Description of your changes

This changes the behavior of the KubernetesApplicationResource controller to not automatically assume the resources that do not have a namespace defined should be created in the default namespace. This was causing issues because we updated the patch logic in this controller to actually update the remote resources, but the change caused errors when patching Namespace resources that had no namespace supplied (as is customary).

A few summarizing notes:

  • If a Namespace is included in a template without having the namespace field defined, it will be able to be created and patched successfully
  • If a Namespace is included in a template and does have the namespace field defined, it will be created but will fail to be patched (this is probably the most surprising behavior)
  • If a cluster-scoped resource that is not a Namespace (like a CustomResourceDefinition) is included in a template with or without having the namespace field defined, it will be able to be created and patched successfully
  • If a namespace-scoped resource (like a Service) is included in a template without having the namespace field defined, it will fail to be created
  • If a namespace-scoped resource (like a Service) is included in a template without having the namespace field defined, it will fail to be created

How has this code been tested?

I created various renditions of the scenarios listed above. One such config, labeled with success / error for each template is included below:

Config:
apiVersion: workload.crossplane.io/v1alpha1
kind: KubernetesApplication
metadata:
  name: helloworld
  namespace: cp-quickstart
  labels:
    app: helloworld
spec:
  resourceSelector:
    matchLabels:
      app: helloworld
  targetSelector:
    matchLabels:
      cluster: helloworld
  resourceTemplates:
# SUCCESSFUL CREATION AND PATCH
    - metadata:
        name: helloworld-deployment
        labels:
          app: helloworld
      spec:
        secrets:
        - name: k8scluster
        template:
          apiVersion: apps/v1
          kind: Deployment
          metadata:
            name: helloworld-deployment
            namespace: helloworld
          spec:
            selector:
              matchLabels:
                app: helloworld
            replicas: 2
            template:
              metadata:
                labels:
                  app: helloworld
              spec:
                containers:
                - name: hello-world
                  image: gcr.io/google-samples/hello-app:1.0
                  ports:
                  - containerPort: 8080
                    protocol: TCP
# FAILED CREATION
    - metadata:
        name: helloworld-service
        labels:
          app: helloworld
      spec:
        template:
          kind: Service
          metadata:
            name: helloworld-service
            # namespace: helloworld <-- this missing causes failure (we do not insert `default` any more)
          spec:
            selector:
              app: helloworld
            ports:
            - port: 80
              targetPort: 8080
            type: LoadBalancer
# SUCCESSFUL CREATION, FAILED PATCH
    - metadata:
        name: helloworld-namespace
        labels:
          app: helloworld
      spec:
        template:
          apiVersion: v1
          kind: Namespace
          metadata:
            name: helloworld
            namespace: hmmm # this causes patch fail
            labels:
              app: helloworld
# SUCCESSFUL CREATION AND PATCH
    - metadata:
        name: helloworld-random
        labels:
          app: helloworld
      spec:
        template:
          apiVersion: apiextensions.k8s.io/v1beta1
          kind: CustomResourceDefinition
          metadata:
            creationTimestamp: null
            name: wordpressinstances.wordpress.apps.crossplane.io
            namespace: hi
          spec:
            preserveUnknownFields: true
            group: wordpress.apps.crossplane.io
            names:
              kind: WordpressInstance
              listKind: WordpressInstanceList
              plural: wordpressinstances
              singular: wordpressinstance
            scope: Namespaced
            subresources:
              status: {}
            validation:
              openAPIV3Schema:
                description: WordpressInstance is the Schema for the wordpressinstances API
                properties:
                  apiVersion:
                    description: 'APIVersion defines the versioned schema of this representation
                      of an object. Servers should convert recognized schemas to the latest
                      internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources'
                    type: string
                  kind:
                    description: 'Kind is a string value representing the REST resource this
                      object represents. Servers may infer this from the endpoint the client
                      submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds'
                    type: string
                  metadata:
                    type: object
                  spec:
                    description: WordpressInstanceSpec defines the desired state of WordpressInstance
                    properties:
                      image:
                        description: Image will be used as image of the container that Wordpress
                          runs in. If not specified, the default will be used.
                        type: string
                      provisionPolicy:
                        description: ProvisionPolicy indicates whether Wordpress should be deployed
                          to an existing Kubernetes cluster or to provision a new cluster.
                        enum:
                        - ProvisionNewCluster
                        - UseExistingTarget
                        type: string
                    type: object
                type: object
            version: v1alpha1
            versions:
            - name: v1alpha1
              served: true
              storage: true
          status:
            acceptedNames:
              kind: ""
              plural: ""
            conditions: []
            storedVersions: []

Checklist

I have:

  • Run make reviewable to ensure this PR is ready for review.
  • Ensured this PR contains a neat, self documenting set of commits.
  • Updated any relevant documentation and examples.
  • Reported all new error conditions into the log or as an event, as
    appropriate.

For more about what we believe makes a pull request complete, see our
definition of done.

Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
@hasheddan hasheddan requested a review from negz March 23, 2020 22:26
@negz negz merged commit 3adb2d5 into crossplane:master Mar 23, 2020
@prasek prasek added this to the v0.9 milestone Mar 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants