[bitnami/contour] Sync upstream changes and chart standardization #3381

mkilchhofer · 2020-08-10T20:00:14Z

Description of the change
The most important change is the refactoring of the shutdown sidecar:
projectcontour/contour@7cd9f4a

I manually diff'ed the example manifests here: https://github.com/projectcontour/contour/tree/master/examples/contour

Benefits
By using this configuration in combination with contour version 1.7.0 the envoys now properly entering into the DRAINING state when the pods needs to be terminated.

Additionally this change fixes the failing pipeline issue.

Possible drawbacks
-

Applicable issues
-

Additional information
-

Checklist

Chart version bumped in Chart.yaml according to semver.
Variables are documented in the README.md
Title of the PR starts with chart name (e.g. [bitnami/chart])
If the chart contains a values-production.yaml apart from values.yaml, ensure that you implement the changes in both files

⚠️ Keep in mind that if you want to make changes to the kubeapps chart, please implement them in the kubeapps repository. This is only a synchronized mirror.

The most important change is the refactoring of the shutdown sidecar: projectcontour/contour@7cd9f4a Signed-off-by: Marco Kilchhofer <marco@kilchhofer.info>

dani8art

Hi @mkilchhofer thank you so much for the PR, please take a look at my comments

bitnami/contour/Chart.yaml

bitnami/contour/README.md

bitnami/contour/templates/daemonset.yaml

bitnami/contour/values-production.yaml

bitnami/contour/values.yaml

dani8art

Please, could you fix the errors when deploying the chart? it seems it cannot execute the readiness probe well or doesn't get the proper response.

mkilchhofer · 2020-08-12T11:56:18Z

Please, could you fix the errors when deploying the chart? it seems it cannot execute the readiness probe well or doesn't get the proper response.

Hi @dani8art, I would love to fix them, but it is not possible. Please read what your colleague @marcosbc wrote here: #3367

As for the tests, we recently enabled PR chart testing so some failures like these are expected until we polish things up. Note that we run more reliable tests in our internal pipeline, and in the case of Contour they're currently succeeding.

> 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>

Signed-off-by: Marco Kilchhofer <marco@kilchhofer.info>

API group contour.heptio.com is no longer supported since 1.6.x

Signed-off-by: Marco Kilchhofer <marco@kilchhofer.info>

I found a hint in PR bitnami#2721 that it should be possible to override values used in GH actions.

.github/workflows/lint-test.yaml

mkilchhofer · 2020-08-13T12:16:53Z

Finally the tests are green 🟢 💯 🥳

…tions" This was requested during the review process by dani8art. This reverts commit 253a8ec.

mkilchhofer · 2020-08-19T10:41:43Z

Hi @dani8art
Are my changes ok now? :)

dani8art · 2020-08-19T16:29:10Z

Hi @mkilchhofer we went deeper on these changes and we saw you are introducing and modify CRDs. We would like to consider these changes as a major version and take this opportunity to include some of our standardization and recent common features.

Even though a helm upgrade works fine, it will remove a CRD and will update the rest, so instances that are deployed using previous CRDs will not work properly after an upgrade.

We know that adding those standardization and common features may be out of scope for you but I can help you to manage this.

We would like to include the next list of features.

add bitnami/common as a dependency, please find it here you can also find an example for all the helpers in the common chart here
extraVolumes, extraVolumeMounts both contour and envoy, please find an example here
extraEnvVar both contour and envoy, please find an example here
extraEnvVarsConfigMap both contour and envoy, please find an example here
extraEnvVarsSecret both contour and envoy, please find an example here
initContainers both contour and envoy, please find an example here
service.extraPorts here
rolling tags helpers here
~~passwords/secrets upgrade helper here~~

mkilchhofer · 2020-08-19T16:53:35Z

Hi @dani8art
Sure, I understand your point and I can work on it. The changeset would be even bigger then.

I only want to mention that the guys behind projectcontour (mainly the heptio.com people which are now part of VMware) talks about using the bitnami chart and already prepared their changes:
projectcontour/contour#2050

Do we still need to support helm2 for all the bitnami charts?

Signed-off-by: Marco Kilchhofer <marco@kilchhofer.info>

dani8art

Thank you so much for these amazing changes, please take a look at my latest comments.

Could we use these other helpers as well? https://github.com/bitnami/charts/blob/master/bitnami/common/templates/_capabilities.tpl

bitnami/contour/README.md

bitnami/contour/templates/rbac.yaml

mkilchhofer · 2020-09-05T10:26:47Z

Could we use these other helpers as well? https://github.com/bitnami/charts/blob/master/bitnami/common/templates/_capabilities.tpl

We could use the one for the Deployment apiVersion. But batch/v1 for the Job and apps/v1 for the DaemonSet is not available in the common chart. I suggest to leave this as-is for all 3 types of workload definitions used and do the implentations inside the common at the time the apiVersion changes again (eg. when the workload definitions changes to v2 or so).

@dani8art

This was requested during review by @dani8art

bitnami/contour/templates/crd-ingressroutes.yaml

bitnami/contour/templates/service.yaml

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.

Cellebyte · 2020-09-06T15:15:12Z

@mkilchhofer thank you for improving my Chart. :)

dani8art · 2020-09-07T10:31:18Z

Could we use these other helpers as well? https://github.com/bitnami/charts/blob/master/bitnami/common/templates/_capabilities.tpl

We could use the one for the Deployment apiVersion. But batch/v1 for the Job and apps/v1 for the DaemonSet is not available in the common chart. I suggest to leave this as-is for all 3 types of workload definitions used and do the implentations inside the common at the time the apiVersion changes again (eg. when the workload definitions changes to v2 or so).

I would use it for the one that we have implemented in the common, please.

mkilchhofer · 2020-09-07T10:43:41Z

Thanks for the review again @dani8art. I will implement the changes 👍 But before I go on, I made a proposal here to refactor the whole chart into subfolders based on components mkilchhofer#4. WDYT? Is it a good idea? If so, I would merge this in before I make changes to not do my changes twice :)

Update: changes are outside of the restructured files and therefore I updated both this PR and PR mkilchhofer#4.

Services for ServiceMonitor do not need to allocate an IP address. We therefore do not waste IPs from the services CIDR pool for this.

dani8art · 2020-09-09T08:03:54Z

Thanks for the review again @dani8art. I will implement the changes 👍 But before I go on, I made a proposal here to refactor the whole chart into subfolders based on components mkilchhofer#4. WDYT? Is it a good idea? If so, I would merge this in before I make changes to not do my changes twice :)

Update: changes are outside of the restructured files and therefore I updated both this PR and PR mkilchhofer#4.

Yes I think it would be a good approach LGTM!

mkilchhofer · 2020-09-09T09:14:45Z

Yes I think it would be a good approach LGTM!

Superb :) I now merged these changes also on this PR here. So from my point of view, we now should be done with all the requested changes, right?

dani8art · 2020-09-09T09:29:09Z

Yes I think it would be a good approach LGTM!

Superb :) I now merged these changes also on this PR here. So from my point of view, we now should be done with all the requested changes, right?

Yes, regarding changes suggested by bitnami team it is all ok. If you are ok with your changes/refactors we could proceed to test it internally in our CI/CD system. Do you agree?

mkilchhofer · 2020-09-09T09:39:43Z

Yes I think it would be a good approach LGTM!

Superb :) I now merged these changes also on this PR here. So from my point of view, we now should be done with all the requested changes, right?

Yes, regarding changes suggested by bitnami team it is all ok. If you are ok with your changes/refactors we could proceed to test it internally in our CI/CD system. Do you agree?

Sure. I hope we get green light from your CI/CD system :)

dani8art

We have got the green light!

@mkilchhofer Thank you so much for this amazing job!

Sync back most significant upstream projectcontour changes

254da50

The most important change is the refactoring of the shutdown sidecar: projectcontour/contour@7cd9f4a Signed-off-by: Marco Kilchhofer <marco@kilchhofer.info>

dani8art requested changes Aug 11, 2020

View reviewed changes

mkilchhofer requested a review from dani8art August 11, 2020 14:58

mkilchhofer mentioned this pull request Aug 11, 2020

Create a helm chart for Contour projectcontour/contour#2050

Open

dani8art reviewed Aug 12, 2020

View reviewed changes

mkilchhofer force-pushed the sync_projectcontour_changes branch from 62175fb to 7a006fb Compare August 12, 2020 18:46

mkilchhofer changed the title ~~[bitnami/contour] Sync back most significant upstream projectcontour changes~~ WIP: [bitnami/contour] Sync back most significant upstream projectcontour changes Aug 12, 2020

mkilchhofer marked this pull request as draft August 12, 2020 19:07

mkilchhofer added 3 commits August 12, 2020 21:09

Sync HTTPProxy CRD

a8fe6f1

Signed-off-by: Marco Kilchhofer <marco@kilchhofer.info>

Sync TLSCertificateDelegation CRD

613c4b7

Drop old TLSCertificateDelegation CRD on API group contour.heptio.com

5f0166c

API group contour.heptio.com is no longer supported since 1.6.x

mkilchhofer changed the title ~~WIP: [bitnami/contour] Sync back most significant upstream projectcontour changes~~ [bitnami/contour] Sync back most significant upstream projectcontour changes Aug 13, 2020

mkilchhofer marked this pull request as ready for review August 13, 2020 10:05

Bump minor chart version as the change is no longer a patch

effa8ed

Signed-off-by: Marco Kilchhofer <marco@kilchhofer.info>

mkilchhofer requested a review from dani8art August 13, 2020 10:09

Attempt to fix contour tests by upgrading to latest github actions

253a8ec

mkilchhofer force-pushed the sync_projectcontour_changes branch 2 times, most recently from 8a8c196 to 222f848 Compare August 13, 2020 12:00

Override envoy service type to ClusterIP to make tests happy

8bab0b3

I found a hint in PR bitnami#2721 that it should be possible to override values used in GH actions.

mkilchhofer force-pushed the sync_projectcontour_changes branch from 222f848 to 8bab0b3 Compare August 13, 2020 12:05

mkilchhofer commented Aug 13, 2020

View reviewed changes

.github/workflows/lint-test.yaml Outdated Show resolved Hide resolved

Revert "Attempt to fix contour tests by upgrading to latest github ac…

ca41277

…tions" This was requested during the review process by dani8art. This reverts commit 253a8ec.

Add bitnami/common as a dependency

09d94d3

Signed-off-by: Marco Kilchhofer <marco@kilchhofer.info>

mkilchhofer force-pushed the sync_projectcontour_changes branch from 9e31042 to 09d94d3 Compare August 19, 2020 18:17

mkilchhofer marked this pull request as ready for review September 2, 2020 11:02

mkilchhofer requested review from juan131 and dani8art September 2, 2020 11:03

dani8art requested changes Sep 3, 2020

View reviewed changes

bitnami/contour/README.md Show resolved Hide resolved

bitnami/contour/templates/rbac.yaml Outdated Show resolved Hide resolved

Replace colons with dashes (standardize even more)

e6e1ca1

This was requested during review by @dani8art

mkilchhofer commented Sep 5, 2020

View reviewed changes

bitnami/contour/templates/crd-ingressroutes.yaml Show resolved Hide resolved

Clearify README regarding CRDs and helm v3

1f3456c

mkilchhofer force-pushed the sync_projectcontour_changes branch from a9a6f67 to 1f3456c Compare September 5, 2020 19:53

mkilchhofer and others added 2 commits September 5, 2020 22:30

Add small upgrading notes to README

d468dc8

Merge branch 'master' into sync_projectcontour_changes

ccd9267

mkilchhofer mentioned this pull request Sep 6, 2020

[bitnami/chart] Add NLB load balancer support, run envoy container as root #2961

Merged

4 tasks

mkilchhofer commented Sep 6, 2020

View reviewed changes

bitnami/contour/templates/service.yaml Outdated Show resolved Hide resolved

Fix helm2 incompatibility due to PR bitnami#2961

d29dcd1

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.

mkilchhofer mentioned this pull request Sep 6, 2020

Split into subfolders mkilchhofer/bitnami-charts#4

Merged

Use consistent component labels on certgen resources

cbc5fe7

mkilchhofer added 4 commits September 7, 2020 13:02

Use bitnami/common for apiVersion of kind Deployment

6a113a5

Rename parameter contour.createCustomResource to contour.installCRDs

db8af4c

Do not allocate an IP address on metrics services

e03ebda

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

e567750

dani8art approved these changes Sep 9, 2020

View reviewed changes

dani8art merged commit 91fa2c0 into bitnami:master Sep 9, 2020

mkilchhofer deleted the sync_projectcontour_changes branch October 19, 2020 12:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[bitnami/contour] Sync upstream changes and chart standardization #3381

[bitnami/contour] Sync upstream changes and chart standardization #3381

mkilchhofer commented Aug 10, 2020 •

edited

Loading

dani8art left a comment

dani8art left a comment

mkilchhofer commented Aug 12, 2020

mkilchhofer commented Aug 13, 2020

mkilchhofer commented Aug 19, 2020

dani8art commented Aug 19, 2020 •

edited

Loading

mkilchhofer commented Aug 19, 2020

dani8art left a comment

mkilchhofer commented Sep 5, 2020

Cellebyte commented Sep 6, 2020

dani8art commented Sep 7, 2020

mkilchhofer commented Sep 7, 2020 •

edited

Loading

dani8art commented Sep 9, 2020

mkilchhofer commented Sep 9, 2020 •

edited

Loading

dani8art commented Sep 9, 2020

mkilchhofer commented Sep 9, 2020

dani8art left a comment •

edited

Loading

[bitnami/contour] Sync upstream changes and chart standardization #3381

[bitnami/contour] Sync upstream changes and chart standardization #3381

Conversation

mkilchhofer commented Aug 10, 2020 • edited Loading

dani8art left a comment

Choose a reason for hiding this comment

dani8art left a comment

Choose a reason for hiding this comment

mkilchhofer commented Aug 12, 2020

mkilchhofer commented Aug 13, 2020

mkilchhofer commented Aug 19, 2020

dani8art commented Aug 19, 2020 • edited Loading

mkilchhofer commented Aug 19, 2020

dani8art left a comment

Choose a reason for hiding this comment

mkilchhofer commented Sep 5, 2020

Cellebyte commented Sep 6, 2020

dani8art commented Sep 7, 2020

mkilchhofer commented Sep 7, 2020 • edited Loading

dani8art commented Sep 9, 2020

mkilchhofer commented Sep 9, 2020 • edited Loading

dani8art commented Sep 9, 2020

mkilchhofer commented Sep 9, 2020

dani8art left a comment • edited Loading

Choose a reason for hiding this comment

mkilchhofer commented Aug 10, 2020 •

edited

Loading

dani8art commented Aug 19, 2020 •

edited

Loading

mkilchhofer commented Sep 7, 2020 •

edited

Loading

mkilchhofer commented Sep 9, 2020 •

edited

Loading

dani8art left a comment •

edited

Loading