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

v0.0.5 fixes #5

Merged
merged 2 commits into from
Jun 5, 2019
Merged

v0.0.5 fixes #5

merged 2 commits into from
Jun 5, 2019

Conversation

desimone
Copy link
Contributor

@desimone desimone commented May 30, 2019

Fixes #3
Fixes #4 (thx @travisgroth ! desimone#1)

  • confirm everything works on GKE
  • ready for review

@desimone desimone changed the title idp-service-account should be optional [wip] v0.0.5 fixes [wip] May 30, 2019
@desimone
Copy link
Contributor Author

desimone commented May 31, 2019

/cc @travisgroth two issues I'm stuck on and welcome any ideas.

  1. Any ideas how to pass the GKE service annotation cloud.google.com/app-protocols: '{"https":"HTTPS"}' with the proper escapes so can pass those details in via the command line instead of with values?
  2. I was hitting an error with the resources key. You can see it is removed above. I couldn't get it working. Refactor chart for new config methods #2

@travisgroth
Copy link
Contributor

travisgroth commented May 31, 2019

  1. I think this will do it.
helm template . \
--set ingress.enabled=true \
--set ingress.annotations."cloud\.google\.com/app-protocols"='\{"https":"HTTPS"\}
# Source: pomerium/templates/ingress.yaml
apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: release-name-pomerium
  labels:
    app.kubernetes.io/name: pomerium
    app.kubernetes.io/instance: release-name
    app.kubernetes.io/managed-by: Tiller
    helm.sh/chart: pomerium-1.0.0
  annotations:
    cloud.google.com/app-protocols: '{"https":"HTTPS"}'
    kubernetes.io/ingress.allow-http: "false"

Funny thing here. The quotes are only needed for one of the 4 ingresses:

helm template . --set ingress.enabled=true --set ingress.annotations.cloud\.google\.com/app-protocols='\{"https":"HTTPS"\}'  | grep cloud
    cloud.google.com/app-protocols: '{"https":"HTTPS"}'
    cloud.google.com/app-protocols: '{"https":"HTTPS"}'
    cloud.google.com/app-protocols: '{"https":"HTTPS"}'
    cloud:

Edit: Ah there's only one ingress obviously. the toYaml is messing it up without the quotes.

@travisgroth
Copy link
Contributor

  1. The resource block needs to move over two spaces. Fix resource indentation desimone/pomerium-helm#1

  2. It looks like there will also be a conflict between the config volume and extravolumes if you specify both right now. I don't have time to reason through the change right now, but it is unlikely to be hit in the near term.

@desimone
Copy link
Contributor Author

desimone commented Jun 1, 2019

\cc @victornoel @travisgroth if you don't mind giving this another look. This is what I'll merged (see updated tag) after v0.0.5 drops.

@victornoel
Copy link
Contributor

@desimone if you can wait I will test this version of the chart with the latest build on Monday

@desimone
Copy link
Contributor Author

desimone commented Jun 1, 2019 via email

@victornoel
Copy link
Contributor

@desimone there is something strange with .Values.config.existingPolicy: it seems it is referenced in some place but it is never really used. I think maybe it should be completely removed?

@travisgroth
Copy link
Contributor

@desimone there is something strange with .Values.config.existingPolicy: it seems it is referenced in some place but it is never really used. I think maybe it should be completely removed?

I must've introduced that while reworking the chart. I agree - I don't believe it makes sense anymore in the current state of things. An existing CM for just policy wouldn't make much sense. (1) config.existingConfig covers that and (2) config.policy is inlined to the in-chart CM.

@desimone desimone changed the title v0.0.5 fixes [wip] v0.0.5 fixes Jun 3, 2019
@victornoel
Copy link
Contributor

@desimone I think when you force-pushed yesterday you undid some of the change you had done: https://github.com/pomerium/pomerium-helm/compare/cb1b10c89fa9c49cfa2d0de7a6d608a587cf0ee2..95d2757b184bc0a8af3c0b597beaed890df78b3f

@desimone
Copy link
Contributor Author

desimone commented Jun 5, 2019

@victornoel Thank you. I totally did.

- standardized example domain to corp.beyondperimeter.com
- make GKE ingress annotations default (ick)
- change idp.serviceAccount to be optional
- fix ingress to support wilcard (individual routes cannot be hot-reloaded)
- fix authorize_url to use internal location (this is a point of confusion)
- fix the way internal certificates are generated
@desimone desimone merged commit 480b6d0 into pomerium:master Jun 5, 2019
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.

improper indentation for resources Couldn't find key idp-service-account
3 participants