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

Add kustomizations and tooling to create 2.x manifests #1563

Merged
merged 12 commits into from
Jul 22, 2021

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Jul 16, 2021

What this PR does / why we need it:

  • Adds Kustomize patches to change the image in deploy manifest bases to 2.x and use its readiness endpoint
  • Adds a script (and make target) to copy the 2.x kubebuilder-generated manifests and existing 1.x deploy manifests into tempdir and kustomize them into single-file manifests under deploy/single-v2
  • Move existing manifests to deploy/single-v1 and link deploy/single to deploy/single-v1. We should flip this to v2 once v2 is GA.
  • Update manifest verify script to use new locations and check v2 manifests.

Which issue this PR fixes: Fixes #1256

Special notes for your reviewer:

  • This PR includes the generated manifests, and you probably don't want to review them line by line. kubectl apply -f deploy/single-v2/all-in-one-dbless.yaml and so on in a test cluster is a more effective review strategy.
  • Main files to review are hack/verify-manifests.sh, railgun/config/base/kong-ingress-2x.yaml, railgun/config/base/kustomization.yaml , and railgun/hack/deploy/build-single-manifests.sh. The rest are largely repetitive and/or generated manifests composed from existing content (e.g. there's nothing new about the CRDs or RBAC resources, this just composes them into new files).
  • There are two open TODOs:
    • Fix the role bindings. Does anyone (probably @shaneutt) recall if the binding was written by hand or generated? I think it was written by hand and I can just modify it directly and remove tmp-bindings.yaml. If it was generated, however, I need to fix the source, and I don't know what that is.
    • This doesn't deal with the auth proxy role. Are we okay taking that as a followup item?

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

  • the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

Travis Raines added 3 commits July 16, 2021 14:35
Add Kustomize manifests for 2.x. Many of these are links to existing 1.x
manifests that require no changes for 2.x. Uses a new location so that
the original unchanged 1.x manifests can live alongside new
2.x-only content.
Add a script to compose 2.x base manifests into single/all-in-one
manifests.
@rainest rainest temporarily deployed to Configure ci July 16, 2021 22:10 Inactive
@codecov
Copy link

codecov bot commented Jul 16, 2021

Codecov Report

Merging #1563 (bfa46b0) into next (f76b19b) will increase coverage by 0.19%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #1563      +/-   ##
==========================================
+ Coverage   51.32%   51.51%   +0.19%     
==========================================
  Files          93       93              
  Lines        8737     8737              
==========================================
+ Hits         4484     4501      +17     
+ Misses       3939     3926      -13     
+ Partials      314      310       -4     
Flag Coverage Δ
integration-test 47.48% <ø> (+0.58%) ⬆️
unit-test 39.85% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
railgun/internal/ctrlutils/ingress-status.go 64.64% <0.00%> (+1.01%) ⬆️
...n/internal/proxy/clientgo_cached_proxy_resolver.go 81.11% <0.00%> (+1.39%) ⬆️
...trollers/configuration/zz_generated_controllers.go 34.63% <0.00%> (+2.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f76b19b...bfa46b0. Read the comment docs.

@rainest
Copy link
Contributor Author

rainest commented Jul 16, 2021

Draft version of this attempts to more or less mimic the 1.x hack script, using the existing 1.x kustomize bases with some additional patches for 2.x. 2.x bases symlink to the 1.x bases when they don't need changes.

Open questions to refine the draft into a ready PR:

  • Does the file organization proposed here make sense? This is sort of an arbitrary organic layout I threw together to make it work that sorta tries to follow the existing base layout. I don't know how much we like/don't like the existing structure under deploy/ and railgun/config/ and/or whether it makes intuitive sense, and don't have much opinion about it myself: it's there and I think I understand it, but I am not a particularly organized person and will thrive in whatever chaotic mess I find 🙃
  • Does this approach to maintaining a 1.x/2.x split seem sound? From initial basic testing (run make allinones and deploy the resulting 2.x single manifests to KIND, see if they come up without errors), the existing 1.x deployment files are fine with minimal edits to change the version, and the main 2.x stuff is to compose the new generated CRDs and RBAC resources alongside the deployments.
  • What's a better name for the make target? make allinones is my extremely eNgLiSh "make is a transitive verb whose object must desribe its output, and its output is 'allinones', a neonoun for 'all in ones', the combined YAML manifests including Deployment, CRD, RBAC, etc. There's probably a more sensible name for this, but my brain is stuck on "well, that's what it outputs" at the moment. After thinking about this, manifests.single seems appropriate and aligned with our naming scheme for other targets.

Copy link
Contributor

@ccfishk ccfishk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kindly help to remove duplicates

Travis Raines added 3 commits July 21, 2021 10:40
- Move deploy/single to deploy/single-v1
- Move railgun/config/single to deploy/single-v2
- Update manifest generator scripts with new locations
- Add v2 manifests to manifest verifier
- Link deploy/single to deploy/single-v1
@rainest rainest temporarily deployed to Configure ci July 21, 2021 18:35 Inactive
@rainest rainest temporarily deployed to Configure ci July 21, 2021 18:37 Inactive
@rainest rainest temporarily deployed to Configure ci July 21, 2021 18:37 Inactive
@rainest rainest marked this pull request as ready for review July 21, 2021 18:53
@rainest rainest requested a review from a team as a code owner July 21, 2021 18:53
Copy link
Contributor

@ccfishk ccfishk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a few comments

@rainest rainest requested a review from ccfishk July 21, 2021 21:06
deploy/single Show resolved Hide resolved
@ccfishk
Copy link
Contributor

ccfishk commented Jul 21, 2021

Kindly split the files and remove duplicates.

@ccfishk ccfishk self-requested a review July 21, 2021 22:45
Copy link
Contributor

@ccfishk ccfishk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added comments

Copy link
Contributor

@ccfishk ccfishk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commented.

Use the default ServiceAccount in 2.x role bindings. Remove the patch
that previously overwrote the ServiceAccount for the 2.x all-in-one
manifests.
@rainest rainest temporarily deployed to Configure ci July 22, 2021 00:25 Inactive
@rainest rainest temporarily deployed to Configure ci July 22, 2021 00:29 Inactive
@rainest rainest requested a review from a team July 22, 2021 00:30
Copy link
Contributor

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I have no hard blocks 👍

I do however have a few comments about follow ups for some of these hardcoded versions, and to make sure we're following up on the TODO item from the script, once those are complete and other people's comments are complete ✔️

Copy link
Contributor

@ccfishk ccfishk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after removing the tmp-binding.yaml. Thanks for working hard removing duplicates, I know it is challenge, but we'll get there.

@rainest rainest temporarily deployed to Configure ci July 22, 2021 16:34 Inactive
@rainest rainest temporarily deployed to Configure ci July 22, 2021 16:38 Inactive
@rainest rainest merged commit 208733f into next Jul 22, 2021
@rainest rainest deleted the feat/rg-deploy-tooling branch July 22, 2021 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants