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

[WIP] Predefined Dynamic namespaces #23

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

MChorfa
Copy link
Collaborator

@MChorfa MChorfa commented Oct 24, 2020

This feature will allow to pre create namespaces that are required within the bundle . So we don't have to run a separate step to create a namespace when performing an action

Closes #22

Signed-off-by: Mohamed Chorfa <[email protected]>
Signed-off-by: Mohamed Chorfa <[email protected]>
@MChorfa MChorfa force-pushed the dynamic-namespaces branch from a4fceeb to 4a44565 Compare October 24, 2020 02:26
@MChorfa MChorfa changed the title WIP: Predefined Dynamic namespaces [WIP] Predefined Dynamic namespaces Oct 24, 2020
@carolynvs
Copy link
Member

I don't believe doing the namespace creation during build using mixin configuration is the way to go here. A few things:

  • it can't be templated because porter templates don't (yet) support templating outside of actions
  • build is the wrong time to create the namespaces, they should be created during at least install and also probably upgrade and invoke, but not uninstall.
  • Perhaps namespace creation should be a manifest that users provide?
  • Or if we must, let's help people out so that it's not an explicit listing of the namespaces in the porter.yaml, which is guaranteed to get out-of-date vs the manifests.

The most straightforward solution would be for users of the mixin to include a namespace definition in the manifests directory. kubectl will then handle it just fine, creating the namespace if needed. It moves the namespace definition/creation back to the source of truth, the manifests instead of having it shared in the porter.yaml. I'd like to understand first if that is an acceptable solution. @squillace? If not what scenarios are not covered adequately by this?

Otherwise if we must create the namespaces, they should be created during the relevant actions. One thing that we could do is parse their manifests, identify namespaces used and execute the command only if they don't exist. Then in the mixin config, have a flag for controlling that behavior, off by default.

I'd really want to make sure before we implement anything further that we understand the implications of having this feature vs. the user defining the namespace manifest themselves. From a security standpoint (it requires additional privileges, people may not have access or we are relying upon undesired rbac because we created the namespace) and also from the limited configuration (you can't define annotations or labels, not sure what else is on a namespace).

@carolynvs carolynvs self-assigned this Oct 26, 2020
@carolynvs carolynvs self-requested a review October 26, 2020 17:36
@MChorfa
Copy link
Collaborator Author

MChorfa commented Oct 26, 2020

We do actually have namespace property defined in schema


The namespace property is additive to the kubectl command.
We can do:

  • option 1:
    Augment the current actions logic, where If the namespace property is defined. check the namespace exists. If not, we run the namespace creation prior to building kubectl command.

  • Option 2:
    Introduce a property that allows pre-create the namespaces if defined within a given action

install:
  - kubernetes:
      description: "Install Hello World App"
      namespaces:
          - my-namespace-01
          - my-namespace-02
...

@squillace
Copy link

Following helm's experience, in particular, I do NOT want to "make creation of a namespace automatic"; instead, I want a specific command that creates a namespace if it's not there.

install:
  kubernetes:
    namespace-upsert: <namespace string>

a single command. If namespace-upsert isn't there, it's a regular apply command.

We know the user problem of K8s here. I do not wish to modify the default experience, however. It must remain leashed to the default k8s experience. I think this is in line with what @carolynvs is suggesting -- but I like explicit commands rather than files that must be dropped in a specific folder. What if a folder contains lots of manifests? but hey, what carolyn is suggesting as the UX is right: a special way that is obvious and does NOT "add features" to kubectl is the way to go.

@carolynvs
Copy link
Member

@squillace We use manifests to create everything else: services, deployments, etc. Can you help me understand why we wouldn't also use a manifest to ensure the namespace exists? I am guessing that there is a slightly different desired behavior, or team permission situation that I am missing context on that makes it undesirable?

@squillace
Copy link

As always, these questions are subjective, so really it's just an argument here. namespaces are not normal k8s resources; they are not upsert actions; and the manifest is really the api payload, right?

image

Now, the user problem is that namespaces aren't upsertable imperatively, though creating the ns imperatively is by far the easiest way to do it for non-experts. So the question is, how can most creators of the package who aren't experts in each element of the app most easily obtain upsert behavior?

Requiring them to understand the manifest and modify it as they were just creating the bundle with manifests and/or any number of other assets likely given by developers will retard usage.

NOTE: most helm charts are a mess and wrong. But they're incredibly useful to help people get moving with something and before they dive in, pull the charts offline, vet them, understand them, and rebuild their internal workings with supportable confidence. So it's really important to keep the "I didn't create it but need to bundle it" users separate from the "this bundle I shall vet because I built the app in the first place and it will be production ready" users. They're very different, and "ease of use" is not necessarily the highest requirement of the latter group. But it is critical to the former group.

So the objective is the following: I'm a bundle builder from READMEs given by the dev. Turns out the readme always says "create me a namespace" and never adds the namespace to the manifest (because that's not templatable easily) and just assumes the README user could modify the manifest.

As a result, the use case I'm looking to solve with this is, "I have been told to create a new namespace and it may or may not already exist and I do not feel comfy diving into manifests in my directory -- I don't really know K8s deeply and I do not want those files to vary from what the dev gave me. Please just upsert the namespace."

Now, this is the same issue that Helm dealt with moving from 2 to 3, precisely. Precisely. And the choice was, "create the namespace" and the examples given to others is kubectl create ns foo, so that's what people do. That's what they'll find. So the UX for this in our "simplification system" called porter should be something like a simple switch that does, in fact, inject the manifest for you (to obtain upsert behavior):

image

Whilst I CAN do that, above, to obtain the behavior I want, the mixin should have this for people in whatever fashion is easiest. This is my argument. Is it persuasive at all?

@squillace
Copy link

Here's the workaround, and if it's sufficient for now, we can let this idea fester a bit more.

  - exec:
      command: bash
      description: "Creating the mysql namespace."
      flags:
        c: '" kubectl create ns mysql --dry-run=client -o yaml | kubectl apply -f -"' 

This can be run and re-run, and if it's already there, it'll merely no-op rather than throw.

@carolynvs
Copy link
Member

The workaround is exactly the same as including a namespace manifest in the bundle. 😀

Now, the user problem is that namespaces aren't upsertable imperatively

I am still not seeing the problem with defining the namespace in a yaml file, and applying it every time along with the other manifests. What is the unwanted side-effect of doing that?

I just tested out creating a namespace, setting labels on it, then applying a blank definition of that namespace on top of it. It didn't wipe out my original definition; the labels were still there. Seems like an upsert to me?


Or is "upsert" not really the problem and what you are really focusing on as a root problem is a bundle author experience?

I have been told to create a new namespace and it may or may not already exist and I do not feel comfy diving into manifests in my directory -- I don't really know K8s deeply and I do not want those files to vary from what the dev gave me. Please just upsert the namespace.

Someone writing the bundle has full control over what happens. Make a file that defines a namespace, put it in the same directory as the porter.yaml and then use the kubernetes mixin to apply the manifest

kubernetes:
  description: "Run an off-the-shelf set of manifests and also mine"
  manifests:
   - namespace.yaml
   - some-app/manifests

Mixins are intended to adapt using a tool inside a bundle with Porter. I do not agree that their purpose is to make it so that you don't have to understand the tools that you are using in the bundle which I think(?) is what you are suggesting. Mixins use their knowledge of how the tool is being used in a bundle to make common tasks less cumbersome (less yaml) or more reliable (error handling). In this case in particular, it appears that there is a straightforward way of addressing creating a namespace if it doesn't exist, in a way consistent with both the wrapped tool (kubectl) and how the mixin treats everything else.

@carolynvs
Copy link
Member

One idea is flipping the mixin to more directly wrap kubectl and support more kubectl commands. Right now it's a bit different from how we have done other mixins, and switching that may work better with the patterns we have set.

Apply manifests today

kubernetes:
  description: "Install stuff"
  manifests:
   - app1/manifests/
   - app2/manifests/

Proposed apply manifests

kubectl:
  description: "Install stuff"
  apply:
    files:
      - app1/manifests/
      - app2/manifests/

Create Namespace today
Either create a manifest and use the commands above or use the exec mixin and call kubectl directly.

Proposed create namespace
Wrap the kubectl create command but detect and handle the "AlreadyExists" error gracefully to be idempotent. If we get Error from server (AlreadyExists): namespaces "mysql" already exists after calling create, the mixin doesn't report that as an error and continues.

kubectl:
  description: "Create app namespace"
  create:
    namespace: mysql

Proposed run rando kubectl command
Support the same flag and argument/output options that the exec mixin has.

kubectl:
  description: "Run an arbitrary kubectl command with using exec"
  command: cluster-info

@squillace
Copy link

Right. I think as a user here, building bundles, I see my expertise as being the ability to faithfully exercise the apps that I myself did NOT build. I do not want to touch that pile of artifacts myself (I might make a mistake), and if my bundle were going to go to production, I'd be going back to the engineers and confirming each and every step.

This is not the same person as the developer of the application, and that will matter when we weigh a UX feature or not. For me, then, I do not see a mixin as having a constraint to not optimize the ux of a tool in yaml. In fact, the schemas are optimizations on the CLIs for these things already, right? They eliminate "install" if you're in the install action because "implicit", the conversion of --set foo=param to foo: "param", and so on. Easier.

What I don't believe is that any mixin should obscure the underlying tool. A professional should be able to do precisely what they know how to do and never be confused, for sure. But I think clear optimizations that make it easier to various users are clearly in play. Whether YOU agree is precisely the point here. As I now have my workaround, I have an opinion but it's not tightly held here. :-)

As I strongly feel that there's the developer of a complex app, and they may or may NOT be the bundler, I try to keep focus on the experience of both groups, hence the issues.

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.

kubernetes mixin should have a namespace-create command, that's an upsert
3 participants