deis / workflow-cli

The CLI for Deis Workflow
http://deis.com
MIT License
31 stars 43 forks source link

Custom HTTP-Headers for Readiness-Checks ignored #279

Closed felixbuenemann closed 7 years ago

felixbuenemann commented 7 years ago

Custom HTTP-Headers are ignored when trying to create a readiness check on Workflow v2.9.0 on K8s 1.4.6 with CLI v2.9.0-eb21ef2 (current master).

Example:

deis healthchecks:set readiness httpGet 5000 \
  --path /healthz \
  --type web \
  --initial-delay-timeout=20 \
  --timeout-seconds=5 \
  --header="X-Forwarded-Proto:https"

This is using the format listed in the specs, I also tried with --header="X-Forwarded-Proto=https", which is the format listed in the docs.

The generated readiness probe in the app's deployment looks like:

readinessProbe: 
  failureThreshold: 3
  httpGet: 
    path: /healthz
    port: 5000
    scheme: HTTP
  initialDelaySeconds: 20
  periodSeconds: 10
  successThreshold: 1
  timeoutSeconds: 5

Note that httpHeaders is missing.

The expected readiness probe would look like:

readinessProbe:
  failureThreshold: 3
  httpGet:
    httpHeaders:
    - name: X-Forwarded-Proto
      value: https
    path: /healthz
    port: 5000
    scheme: HTTP
  initialDelaySeconds: 20
  periodSeconds: 10
  successThreshold: 1
  timeoutSeconds: 5

Prettified dump of the JSON I captured from the POST /v2/apps/:name/config/ request using tcpdump:

{
  "healthcheck": {
    "web": {
      "readinessProbe": {
        "initialDelaySeconds": 20,
        "timeoutSeconds": 5,
        "periodSeconds": 10,
        "successThreshold": 1,
        "failureThreshold": 3,
        "httpGet": {
          "path": "/healthz",
          "port": 5000
        }
      }
    }
  }
}

There's no header info here either.

Btw. it would be really helpful if the correct format for specifying headers would be shown when running deis healthchecks:set --help.

felixbuenemann commented 7 years ago

@kmala If I understand your changes correctly, the option must now be named --headers (plural) instead of --header (singular)?

bacongobbler commented 7 years ago

that is correct. Simultaneously there was a docopt bug which cannot parse multiple --header flags. We might need to update the docs as well