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

Static Manifests Explicitly Target a K8s Version (for all supported versions) #8000

Closed

Conversation

afirth
Copy link
Member

@afirth afirth commented Nov 30, 2021

What this PR does / why we need it:

Currently, the static manifests are only compatible with K8s 1.20 APIs. This change generates static manifests for each supported version by calling helm template with the recently re-added --kube-version flag

Summary of changes

  • besides the default K8s version (currently 1.20), static manifests for other supported versions are generated, fixing cannot install 1.0.4 on K8s v1.19 #7810
    • the various semverCompare statements are respected e.g. this one
  • add_namespace.py (which also deleted spec.replicas) is removed
    • the manual steps in RELEASE are not required and are removed because the generated yaml retains the "standard" K8s format
    • helm --namespace is used to set the namespace
    • helm --kube-version is used to set the target version
    • kustomize is used to delete spec.replicas and add the namespace resource
    • the inline heredocs in generate-deploy-scripts.sh and add-namespace.py move to yaml files whose directory layout matches that of the generated manifests

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • [?] New feature (non-breaking change which adds functionality)
  • Documentation

Which issue/s this PR fixes

fixes #7810

How Has This Been Tested?

# check out a new branch from PR HEAD
gcb make-diffs

# checkout the manifests from main
git reset main -- ./deploy/static
git checkout main -- ./deploy/static

# deal with the one I moved
mkdir deploy/static/provider/aws/nlb-with-tls-termination
mv deploy/static/provider/aws/deploy-tls-termination.yaml deploy/static/provider/aws/nlb-with-tls-termination/deploy.yaml
cp deploy/static/provider/aws/kustomization.yaml deploy/static/provider/aws/nlb-with-tls-termination/

# run a hacky shell script which `kustomize builds` each directory using the existing kustomization.yaml
cat <<'EOF' > diffmaker.sh
#!/usr/bin/env bash
set -eux -o pipefail

#find . -type f -name deploy.yaml | xargs dirname
MANIFESTS=("./deploy/static/provider/do" "./deploy/static/provider/baremetal" "./deploy/static/provider/kind" "./deploy/static/provider/exoscale" "./deploy/static/provider/cloud" "./deploy/static/provider/scw" "./deploy/static/provider/aws" "./deploy/static/provider/aws/nlb-with-tls-termination/")

mkdir -p diffs
for dir in "${MANIFESTS[@]}"
do
  kustomize build $dir/ > diffs/$(basename $dir).yaml
done
EOF

chmod +x diffmaker.sh
./diffmaker.sh

ga diffmaker.sh
ga diffs
gcm 'original manifests'

# checkout the HEAD of the PR and generate those manifests
git reset 62e00e076 -- deploy/static
git checkout 62e00e076 -- deploy/static

ga diffs
gcm 'PR #8000 manifests'
gpsup

the diff is now visible between 7f3b490d1 and cfeda686a on my fork and shows that only the helm chart version annotation changed

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • n/a I have added tests to cover my changes.
  • All new and existing tests passed.

Detailed changes

dc420e6

  • the manifests are regenerated at 4.0.12 (HEAD) using the existing script

85c4873

  • the inline helm values in hack/generate-deploy-scripts.sh are replaced with files
  • hack/generate-deploy-scripts.sh is DRYd with a loop over the values files
  • the deploy/static/provider/ directory is cleaned when hack/generate-deploy-scripts.sh is run, in case providers, variants, or K8s target versions are removed in the future

60c0f1e

  • .tool-versions is added to ./hack for anyone using asdf (and generally to indicate to releasers or CI what versions to run)

1035cb9

  • kustomize is used to mixin the namespace resource and python is removed
    • this results in a massive reordering, but no change besides the spec.replicas coming back for one commit
  • the manual fix for aws/deploy-tls-termination.yaml specified in RELEASE.md is no longer required and is removed
  • the RELEASE_NAME and NAMESPACE variables are removed from the shell script as they are only used once. Users wishing to customize the namespace can do so with kustomize by modifiying namespace.yaml and setting namespace in their overlay.
  • the manifest aws/deploy-tls-termination.yaml is renamed to aws/nlb-with-tls-termination/deploy.yaml. This is the only provider with a variant manifest at this time. This change is necessary as multiple files are now used to generate the manifest (plus it smelled a bit)
  • the kustomization.yaml present in each static manifest directory is generated from a template (previously created by hand)

33e5bff

  • spec.replicas is removed where found (as previously done in add_namespace.py)

adcfd8b

  • static manifests are generated for all supported versions
  • manifests targeting 1.20 (the helm default) which was used to generate the existing manifests is retained at the root, so docs links will still work.

62e00e0

  • the docs referencing the aws/deploy-tls-termination.yaml are updated with the new path
  • RELEASE.md is updated to remove the manual steps and update dependencies
  • trailing whitespace is cleaned in RELEASE.md
  • quickstart docs updated with note about api version deprecations and that specific manifests are updated

Notes to reviewers

Because this introduces reordering and formatting changes to the yaml files (because they are no longer loaded and dumped by python), I've left the intermediate commits and regenerated the manifests at each one, and recommend to review individually.

Additionally, I've loaded the manifests from main into kustomize and generated diffs, so you can easily see what materially changed (just the chart version annotations)

Further work

If desired I can make a github action to ensure that the static manifests are up to date for each PR

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 30, 2021
@k8s-ci-robot
Copy link
Contributor

@afirth: This issue is currently awaiting triage.

If Ingress contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 30, 2021
@afirth afirth changed the title 2021 11 30 static manifest versioned Versioned Static Manifests Nov 30, 2021
@afirth afirth changed the title Versioned Static Manifests Static Manifests Explicitly Target a K8s Version (for all supported versions) Nov 30, 2021
afirth added a commit to afirth/ingress-nginx that referenced this pull request Nov 30, 2021
@afirth afirth marked this pull request as ready for review November 30, 2021 21:22
@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Nov 30, 2021
@afirth
Copy link
Member Author

afirth commented Nov 30, 2021

/kind bug
ping @aledbf as the primary contributor of `generate-deploy-scripts.sh
/test all

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Nov 30, 2021
@afirth afirth changed the title Static Manifests Explicitly Target a K8s Version (for all supported versions) WIP Static Manifests Explicitly Target a K8s Version (for all supported versions) Nov 30, 2021
@afirth afirth changed the title WIP Static Manifests Explicitly Target a K8s Version (for all supported versions) Static Manifests Explicitly Target a K8s Version (for all supported versions) Nov 30, 2021
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 30, 2021
@tao12345666333
Copy link
Member

Thanks! I will finish the review today.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 4, 2021
@afirth
Copy link
Member Author

afirth commented Dec 6, 2021

Thanks! I will finish the review today.

@tao12345666333 is there anything I can do to help this along?

@aslafy-z
Copy link
Contributor

Any chance this get rebased, reviewed and merged anytime soon @afirth? Thanks

@afirth afirth force-pushed the 2021-11-30-static-manifest-versioned branch from 62e00e0 to b7d25a7 Compare December 16, 2021 14:41
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 16, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: afirth
To complete the pull request process, please assign rikatz after the PR has been reviewed.
You can assign the PR to them by writing /assign @rikatz in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@afirth
Copy link
Member Author

afirth commented Dec 16, 2021

Any chance this get rebased, reviewed and merged anytime soon @afirth? Thanks

Rebased the docs (and fixed some more typos). Maybe reach out on slack to find reviewers? Or perhaps @caseydavenport can help? I'm happy to have a call with an OWNER and step through it if need be (@afirth on cncf slack)

@tao12345666333
Copy link
Member

@tao12345666333 is there anything I can do to help this along?

@afirth This is indeed a huge PR.I didn't spare so much time to finish it.

@rikatz
Copy link
Contributor

rikatz commented Dec 23, 2021

/assign

@rikatz rikatz added this to the v1.2.0 milestone Dec 29, 2021
RELEASE.md Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 12, 2022
@k8s-ci-robot
Copy link
Contributor

@afirth: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@afirth
Copy link
Member Author

afirth commented Jan 13, 2022

/close
will bring in #8099 first, then trivial to modify the script to generate all versions as this one does.
cannot keep up with rebasing both

@afirth afirth closed this Jan 13, 2022
@k8s-ci-robot
Copy link
Contributor

@afirth: Closed this PR.

In response to this:

/close
will bring in #8099 first, then trivial to modify the script to generate all versions as this one does.
cannot keep up with rebasing both

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@afirth afirth mentioned this pull request Jan 18, 2022
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/docs cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. needs-priority needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cannot install 1.0.4 on K8s v1.19
5 participants