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

Including Service Bindings for nginx.conf and mime.types #281

Open
3 tasks
voor opened this issue Aug 10, 2021 · 22 comments
Open
3 tasks

Including Service Bindings for nginx.conf and mime.types #281

voor opened this issue Aug 10, 2021 · 22 comments
Assignees
Labels
enhancement A new feature or request

Comments

@voor
Copy link

voor commented Aug 10, 2021

What happened?

I would like to inject a nginx.conf file and mime.types from a common location using https://github.com/k8s-service-bindings/spec so that I do not need developers to commit this into source code.

Build Configuration

gcr.io/paketo-buildpacks/nginx:0.3.1

pack and Tanzu Build Service (kpack)

  • Can you provide a sample app or relevant configuration (buildpack.yml,
    nginx.conf, etc.)?

nginx.conf: https://github.com/voor/static-build-service-template/blob/5cfaac6b3b4dec9e5238ea290a866bed52b5c369/nginx.conf
mime.types: https://github.com/voor/static-build-service-template/blob/5cfaac6b3b4dec9e5238ea290a866bed52b5c369/mime.types

Checklist

  • I have included log output.
  • The log output includes an error message.
  • I have included steps for reproduction.
@arjun024
Copy link
Member

@voor It's possible to add such a feature but I'm wondering about the value of such a feature. Bindings are typically used for secrets that users do not/cannot check-in with the their app. Is there a compelling reason to avoid checking in configuration files?

@voor
Copy link
Author

voor commented Aug 10, 2021

Yes, different teams are responsible for the hand-off, and we do not want the consumers of the buildpack to change them, if the files are present in the users repository they might accidentally modify or delete it, causing significant issues in the build process.

Additionally, if we need to change the files we would need to go to all of the consumers and make sure the file is updated accordingly.

@dmikusa
Copy link

dmikusa commented Aug 10, 2021

+1 for this. I haven't tried it, but I think you can use a ConfigMap instead of a secret with a binding. The binding just has to be mapped into the file system in the expected location. I'm pretty sure this is common practice with Nginx config, to pass it in through volume mounts.

@voor
Copy link
Author

voor commented Aug 10, 2021

I'm using kpack syntax for this, but I'd be looking for something like this:

apiVersion: kpack.io/v1alpha1
kind: Image
metadata:
  name: simple-static-app
  namespace: default
spec:
  build:
    bindings:
    - name: nginx
      secretRef:
        name: nginx-config
      metadataRef:
        name: settings-binding-metadata
---
apiVersion: v1
kind: ConfigMap
metadata:
  name: settings-binding-metadata
data:
  kind: nginx
  provider: nginx
---
apiVersion: v1
kind: Secret
metadata:
  name: nginx-config
type: Opaque
stringData:
  mime.types: |
    types {
        ...
    }
  nginx.confg: |
    worker_processes  5;  ## Default: 1
    daemon off;
    worker_rlimit_nofile 8192;
    ...

@arjun024 arjun024 added the enhancement A new feature or request label Aug 11, 2021
@arjun024
Copy link
Member

We'll take it as a feature request and look into it.

@menehune23 menehune23 self-assigned this Sep 9, 2021
@menehune23
Copy link

Note, we'll follow this spec instead, which supersedes the originally posted link: https://github.com/k8s-service-bindings/spec

@voor
Copy link
Author

voor commented Sep 13, 2021

Awesome, we've been using https://github.com/vmware-labs/service-bindings as a possible implementation of this spec.

@menehune23
Copy link

Also note, that we'll play paketo-buildpacks/packit#107 first, as this buildpack is packit-based. Marking this story is blocked on that for now.

@menehune23 menehune23 added the status/blocked This issue has been triaged and resolving it is blocked on some other issue label Sep 13, 2021
@menehune23
Copy link

menehune23 commented Sep 28, 2021

@voor @arjun024 @dmikusa-pivotal Now that paketo-buildpacks/packit#107 is done, I'm coming back around to this. However, there's some things to consider with implementation.

Service bindings should not become embedded in the final app image, by design. This means that the need here is really a launch-time concern and not a build-time concern (note that this eliminates the need for the kpack-based workflow @voor posted).

If what you want is to avoid having to check in these files, then it seems that the launch command just needs to be altered to load the config from a location given by the binding spec (like this). I can't remember if there's a way to alter the launch command manually or by some other buildpack, or if we'll need to supply a custom means for specifying the config location via this buildpack, like via an env var (perhaps @arjun024 you can enlighten me).

@menehune23
Copy link

Of course, I need to look further to see how this buildpack implements the templating for the config, and understand how that would be affected.

@voor
Copy link
Author

voor commented Sep 28, 2021

Service bindings should not become embedded in the final app image, by design. This means that the need here is really a launch-time concern and not a build-time concern (note that this eliminates the need for the kpack-based workflow @voor posted).

While that does make a lot of sense, my concern in this particular instance is that the image becomes significantly less portable as a result, you need to now have knowledge that this particular nginx backed container doesn't actually have any nginx config inside of it, and you need to account for mounting that in at runtime.

@menehune23
Copy link

@voor I see what you mean. I'm wondering about whether or not this is a buildpack concern, since what you're referring to is to inject a file into the app dir prior to build. My gut says that this should be taken care of by something else before the buildpack kicks in, but I also understand that it may not be easy to do. Let me chat with the maintainers of this buildpack and see what they think.

@arjun024
Copy link
Member

arjun024 commented Sep 28, 2021

Service bindings should not become embedded in the final app image, by design

Why shouldn't the nginx.conf be part of the final image? I believe the use-case here is
for user to provide from outside version-control and wants the conf to end up in the
final image

This means that the need here is really a launch-time concern and not a build-time concern

Nginx buildpack currently "detects" based on the presence of an nginx.conf file
in the application. If nginx.conf is not going to mounted during detect/build time,
then there has to be another robust mechanism to run detection for this buildpack.

I can't remember if there's a way to alter the launch command manually or by some other buildpack

Launch-time alteration of start command shouldn't even be a concern. I think a user should be able to use
Procfile buildpack to change the start command and consume an nginx.conf from a mounted directory.

I think this work constitutes a "substantial change" as described here and would like to
see an RFC proposed with the design/implementation & achieve consensus before we implement
this.

@menehune23
Copy link

menehune23 commented Sep 28, 2021

@voor I spoke with @arjun024 and it turns out that this nginx case is really a special case of a more generic need -- the need to copy volume-mounted files into the app dir. If we had a utility buildpack that could do this, it could solve for your use case without the need to make this buildpack more complex. It would simply need to be placed before this nginx buildpack.

I'll write up an RFC for the buildpack and will provide a link here when done.

@menehune23
Copy link

@menehune23
Copy link

menehune23 commented Sep 30, 2021

@voor Based on the conversation in the RFC, could you try volume mounting to /workspace/nginx.conf, etc during a build (and also at launch if that's a concern), and see if that works for your use case (whether pack/kpack/etc)?

Give it a try with a templated nginx config, as that apparently works for both build-time and at launch (though I suspect that launch-time is really what you want).

@menehune23
Copy link

@voor any update? Looking to see if this issue (and the RFC) can be closed.

@voor
Copy link
Author

voor commented Oct 6, 2021

@voor any update? Looking to see if this issue (and the RFC) can be closed.

No, sorry, haven't had a chance to revisit this, which is unfortunate because this is something that we really need.

@fg-j fg-j removed the status/blocked This issue has been triaged and resolving it is blocked on some other issue label May 23, 2022
@fg-j
Copy link

fg-j commented May 23, 2022

@paketo-buildpacks/web-servers-maintainers Is this still a relevant feature request?

@voor
Copy link
Author

voor commented May 23, 2022

I am seeing BP_NGINX_CONF_LOCATION now allows a configurable location for the nginx conf, does that mean BP_NGINX_CONF_LOCATION could be enhanced with a service binding location?

@ForestEckhardt
Copy link
Contributor

@voor Have you been able to test using BP_NGINX_CONF_LOCATION as a configuration option? If so does it do everything you would expect?

@voor
Copy link
Author

voor commented Jul 7, 2022

I have not unfortunately 😞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A new feature or request
Projects
None yet
Development

No branches or pull requests

6 participants