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

Ignored namespaces aren't skipped for Deployment pods #52

Open
ribbybibby opened this issue Sep 25, 2020 · 3 comments
Open

Ignored namespaces aren't skipped for Deployment pods #52

ribbybibby opened this issue Sep 25, 2020 · 3 comments

Comments

@ribbybibby
Copy link

What's going on?

The metadata of the CREATE request object doesn't always contain the namespace or the name of the pod. This seems to be the case when the pod is launched on behalf of a Deployment. It doesn't seem to be the case with StatefulSets or a bare Pod. I haven't tested Jobs or CronJobs or any other controllers.

The check for ignored namespaces uses metadata.namespace to perform the comparison, so pods in kube-system and kube-public aren't skipped for Deployment pods.

Additionally, some logging statements are missing the namespace and name:

I0925 13:20:51.062652       1 webhook.go:165] Pod / annotation injector.tumblr.com/request is missing, skipping injection

Expected Behavior

List of ignored namespaces should be respected for all pod admission requests, regardless of the source.

Reproducer

This is the Deployment I've been using to test:

apiVersion: apps/v1
kind: Deployment
metadata:
  name: test 
  namespace: kube-system
spec:
  replicas: 1
  selector:
    matchLabels:
      app: test
  template:
    metadata:
      annotations:
        injector.tumblr.com/request: my-sidecar
      labels:
        app: test
    spec:
      containers:
        - name: test
          image: alpine
          command:
            - ash
            - -c
            - |
              while true; do sleep 86400; done

Version Deets

  • Kubernetes Version: 1.18.5, 1.19.1
  • k8s-sidecar-injector Version: release-v0.5.0
@ribbybibby
Copy link
Author

I've logged this PR to address the issue: #53.

I'm also interested in #44 though. Is there any utility in having the ignored namespaces in the code at all? Could this not be achieved more flexibly with a namespaceSelector on the webhook itself? That would unblock people (like myself) who would like to use the injector in kube-system.

@komapa
Copy link
Contributor

komapa commented Sep 26, 2020

Could this not be achieved more flexibly with a namespaceSelector on the webhook itself?

I personally think that will be a great improvement.

@ribbybibby
Copy link
Author

Updated #53 to remove the ignored namespaces feature from the code altogether.

gadotroee pushed a commit to up9inc/k8s-sidecar-injector that referenced this issue Oct 12, 2020
gadotroee added a commit to up9inc/k8s-sidecar-injector that referenced this issue Oct 15, 2020
* no message
* service is array of service namespace and selector (no need for service name right now)
* fix type
* fix mistake
* simple comment
* better and fix tumblr#52
* fix review comment
* disable the inject all option
* fix comment
* not sure why does this files changed but I see we need to check it them
* delete travis yaml
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

No branches or pull requests

2 participants