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 manifest generation uses kustomize instead of python #8099

Merged

Conversation

afirth
Copy link
Member

@afirth afirth commented Jan 4, 2022

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. In this PR, only 1.20 is targeted. #8000 adds the other versions, however based on feedback that this was too big to review, I split the PR.

Summary of changes

  • static manifests are generated at 1.20 explicitly - this mimics the current unspecified behaviour
    • 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

building blocks to fix #7810
relates #8000

How Has This Been Tested?

See #8000 (comment) - Basically by calling kustomize build > file on both the existing and new manifests and confirming there is no diff (besides the AWS NLB example changing name + location to a subfolder)

gco 2022-01-04-static-manifest-generation
gcb make-diffs

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

# 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 # save the original manifests
gcm 'original manifests'

# checkout new manifests and generate
git reset 2022-01-04-static-manifest-generation -- deploy/static
git checkout 2022-01-04-static-manifest-generation -- deploy/static
./hack/generate-deploy-scripts.sh

# save the new manifests and push to my fork
ga diffs
gcm 'PR #8099 manifests'
gpsup

git rev-parse --short HEAD

resulting diff - there isn't one

ga diffs
gcm 'PR #8099 manifests'
>> nothing added to commit but untracked files present (use "git add" to track)

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.

@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Jan 4, 2022
@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-kind Indicates a PR lacks a `kind/foo` label and requires one. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 4, 2022
@afirth
Copy link
Member Author

afirth commented Jan 4, 2022

/hold either this or #8000 should be merged but not both

@rikatz this contains only the existing reordered manifests and the script changes. There are two comments in hack/generate-deploy-scripts.sh which when reverted will generate all the manifests as in #8000. As indicated in "How has this been tested" I think a script is the best way to verify the lack of manifest diffs besides the reordering. I've run it again for this manifest

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 4, 2022
@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
@afirth afirth force-pushed the 2022-01-04-static-manifest-generation branch from 9500a3d to 39159d6 Compare January 13, 2022 09:49
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 13, 2022
@afirth
Copy link
Member Author

afirth commented Jan 13, 2022

rebased again. I will close #8000 as it doesn't make sense to keep both of them up to date with the rebases. After this is in it's trivial to generate for the other versions
/assign @rikatz

@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 13, 2022
@rikatz
Copy link
Contributor

rikatz commented Jan 16, 2022

/approve
/lgtm
/hold

Also @afirth please rebase and let me know (as the lgtm will be removed)
@strongjz @ElvinEfendi one last look here please :)

Thanks!

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 16, 2022
@afirth afirth force-pushed the 2022-01-04-static-manifest-generation branch from 39159d6 to 19c6855 Compare January 17, 2022 09:45
@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 17, 2022
@afirth
Copy link
Member Author

afirth commented Jan 17, 2022

@rikatz rebased
(just release.md)
will teach me to clean up formatting in the future :/

Copy link
Contributor

@rikatz rikatz left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve
Thanks!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 17, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afirth, rikatz

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

The pull request process is described 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

@rikatz
Copy link
Contributor

rikatz commented Jan 17, 2022

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 17, 2022
@k8s-ci-robot k8s-ci-robot merged commit d16e0de into kubernetes:main Jan 17, 2022
@tao12345666333 tao12345666333 mentioned this pull request Feb 27, 2022
rchshld pushed a commit to joomcode/ingress-nginx that referenced this pull request May 19, 2023
…es#8099)

* regenerate at 4.0.12

* bash for loop and static values files

* add .tool-versions

* fixup static manifests with kustomize instead of python

* remove spec.replicas where set

* generate manifests for all supported versions

* update docs

* remove all versions except default (1.20) for now

* update to 1.1.1/4.0.15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/docs cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority 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
3 participants