[bitnami/chart] Add NLB load balancer support, run envoy container as root #2961
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
geota
changed the title
geota
force-pushed
the
nlb_better_security_context_handling
branch
from
4b075be to
b2d51de
Compare
…y context to run as root update readme, update values-production.yaml default envoy container security context to runAsRoot, implement nlb support without requiring a new loadBalancerType variable
geota
force-pushed
the
nlb_better_security_context_handling
branch
from
bb7d419 to
ba6cc60
Compare
geota
changed the title
juan131
requested changes
Jul 1, 2020
|
@juan131 sorry for the delay, ive addressed all the PR feedback. |
…onfig blocks bump minor instead of patch add values.yaml changes to values-production.yaml file as well
geota
force-pushed
the
nlb_better_security_context_handling
branch
from
539713f to
9a528ba
Compare
mkilchhofer
reviewed
Sep 6, 2020
| @@ -28,13 +28,16 @@ metadata: | |||
| {{- include "contour.tplValue" (dict "value" .Values.envoy.service.labels "context" $) | nindent 4 }} | |||
| {{- end }} | |||
| annotations: | |||
| {{- if (ne (get .Values.envoy.service.annotations "service.beta.kubernetes.io/aws-load-balancer-type") "nlb") }} | |||
There was a problem hiding this comment.
While working on #3381 and testing my changes with helm 2.14 and 2.16 I get this error:
helm2 lint .
==> Linting .
[ERROR] templates/: parse error in "contour/templates/service.yaml": template: contour/templates/service.yaml:34: function "get" not defined
Error: 1 chart(s) linted, 1 chart(s) failedI realized that this is a function introduced in Sprig v3: Masterminds/sprig#197.
Since bitnami needs helm v2 compat, I think we should try to solve this using an available function. I'll propose to fix it like this inside #3381
{{- if (ne (index .Values.envoy.service.annotations "service.beta.kubernetes.io/aws-load-balancer-type" | toString ) "nlb") }}There was a problem hiding this comment.
I agree with you @mkilchhofer !! Would you open a PR addressing this? thanks in advance
mkilchhofer
added a commit
to mkilchhofer/bitnami-charts
that referenced
this pull request
Sep 6, 2020
Helm2 uses Sprig v2 and therefore the funtion "get" is not available there. Since we need to guarantee helm v2 support, we need to workaround this.
dani8art
pushed a commit
that referenced
this pull request
Sep 9, 2020
) * Sync back most significant upstream projectcontour changes The most important change is the refactoring of the shutdown sidecar: projectcontour/contour@7cd9f4a Signed-off-by: Marco Kilchhofer <marco@kilchhofer.info> * Delete removed CRD ingressroutes.contour.heptio.com > As a reminder, support for IngressRoute was officially dropped in v1.6. > If you haven’t already migrated to HTTPProxy, see the IngressRoute to > HTTPProxy migration guide for instructions on how to do so. Once you have > migrated, delete the IngressRoute and related CRDs. Ref: https://projectcontour.io/resources/upgrading/ Signed-off-by: Marco Kilchhofer <marco@kilchhofer.info> * Sync HTTPProxy CRD Signed-off-by: Marco Kilchhofer <marco@kilchhofer.info> * Sync TLSCertificateDelegation CRD * Drop old TLSCertificateDelegation CRD on API group contour.heptio.com API group contour.heptio.com is no longer supported since 1.6.x * Bump minor chart version as the change is no longer a patch Signed-off-by: Marco Kilchhofer <marco@kilchhofer.info> * Attempt to fix contour tests by upgrading to latest github actions * Override envoy service type to ClusterIP to make tests happy I found a hint in PR #2721 that it should be possible to override values used in GH actions. * Revert "Attempt to fix contour tests by upgrading to latest github actions" This was requested during the review process by dani8art. This reverts commit 253a8ec. * Add bitnami/common as a dependency Signed-off-by: Marco Kilchhofer <marco@kilchhofer.info> * Also use bitnami/common subchart for image * Implement extraVolumes and extraVolumeMounts on contour and envoy Signed-off-by: Marco Kilchhofer <marco@kilchhofer.info> * Implement extraEnvVars on contour and envoy Signed-off-by: Marco Kilchhofer <marco@kilchhofer.info> * Implement extraEnvVarsConfigMap and extraEnvVarsSecret * Implement initContainers for contour and envoy * Implement service.extraPorts on contour and envoy * Implement rolling tags helpers * Sync rbac with upstream As George Goh (@georgegoh) mentioned in issue projectcontour/contour#2050 the leaderelection role is not needed anymore: > 1. I remove the `contour-leaderelection` role as it's no longer needed > in `templates/rbac.yaml`. > 2. General matching of the rbac.yaml to the rbacs defined in this project. Signed-off-by: Marco Kilchhofer <marco@kilchhofer.info> * Update README.md * Fix default values for image repositories * Add quotes (`) * Fix whitespaces in table * Also use bitnami/common subchart for pullSecrets The subchart bitnami/common now supports this (version 0.6.2 and newer). See PR #3566 * Use same tpl functions for job Inside the Deployment we can use templating with 'tpl'. Since we use the same structure (affinity, nodeSelector and tolerations) for the Deployment and the Job, we should also use the same template functions. * Fix: affinity defined twice * Use more specific keyword antiAffinity -> antiAffinityPolicy * Bump chart major: 2.0.0 * Variant2: Implement certgen by using hooks (#2) * Implement certgen via hooks * Delete resources with helm hooks only if needed * Revert "Delete resources with helm hooks only if needed" This reverts commit 76449252d06b7bda0f16c490316478c3fb1004f1. As documented inside the helm docs, we should remove unneeded resources: ~~~ The resources that a hook creates are currently not tracked or managed as part of the release. Once Helm verifies that the hook has reached its ready state, it will leave the hook resource alone. Garbage collection of hook resources when the corresponding release is deleted may be added to Helm 3 in the future, so any hook resources that must never be deleted should be annotated with `helm.sh/resource-policy: keep`. Practically speaking, this means that if you create resources in a hook, you cannot rely upon helm uninstall to remove the resources. To destroy such resources, you need to either add a custom `helm.sh/hook-delete-policy` annotation to the hook template file, or set the time to live (TTL) field of a Job resource. ~~~ * Replace colons with dashes (standardize even more) This was requested during review by @dani8art * Clearify README regarding CRDs and helm v3 * Add small upgrading notes to README * Fix helm2 incompatibility due to PR #2961 Helm2 uses Sprig v2 and therefore the funtion "get" is not available there. Since we need to guarantee helm v2 support, we need to workaround this. * Use consistent component labels on certgen resources * Use bitnami/common for apiVersion of kind Deployment * Rename parameter `contour.createCustomResource` to `contour.installCRDs` * Do not allocate an IP address on metrics services Services for ServiceMonitor do not need to allocate an IP address. We therefore do not waste IPs from the services CIDR pool for this. * Split into subfolders
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of the change
Support NLB load balancers for contour and fix crash loop in envoy container due to insufficient permissions.
Benefits
NLB have a number of benefits over ELBs. Including support service endpoints, no need to pre-warm, higher throughput etc.
https://docs.aws.amazon.com/elasticloadbalancing/latest/network/introduction.html
Possible drawbacks
NLB support is implemented in a backward-compatible way and defaults to the existing behavior of using a Classic ELB.
Defaulting the envoy container to run as root maybe is unacceptable. If so, I am happy to remove defaulting that and just support passing it in.
Applicable issues
Additional information
Relevant docs:
https://projectcontour.io/guides/deploy-aws-nlb/
https://github.com/projectcontour/contour/tree/master/examples/contour#deploying-with-host-networking-enabled-for-envoy
Chart.yamlaccording to semver.[bitnami/chart])values-production.yamlapart fromvalues.yaml, ensure that you implement the changes in both files