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

Pin kustomize version in cockroachdb example #5534

Closed

Conversation

karlkfi
Copy link
Contributor

@karlkfi karlkfi commented Feb 8, 2024

  • Use tools/go.mod to pin the kustomize version to v5.3.0.
    For most modules, we would want to use the tools pinned in
    hack/go.mod, but for examples we want to show users how to pin
    kustomize within the scope of their function repository.
    As an example, this example's pinned version of kustomize doesn't
    really need to stay current, unless the install process changes.
  • Increment example version to v0.1.1

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 8, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: karlkfi
Once this PR has been reviewed and has the lgtm label, please assign knverey for approval. For more information see the Kubernetes Code Review Process.

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

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 8, 2024
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 20, 2024
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 2, 2024
@karlkfi karlkfi force-pushed the karl-cockroach-example branch 2 times, most recently from a9bb2e0 to a357ecf Compare April 3, 2024 00:42
@karlkfi
Copy link
Contributor Author

karlkfi commented Apr 3, 2024

@koba1t I've rebased this and changed it to use the same type of go.mod tool version pinning as hack/go.mod, but separate to serve as an example of how to pin and install kustomize in a Dockerfile.

Blocked by #5637

@karlkfi karlkfi force-pushed the karl-cockroach-example branch 2 times, most recently from 0afc689 to 7bd3f4a Compare April 4, 2024 16:55
- Use tools/go.mod to pin the kustomize version to v5.3.0.
  For most modules, we would want to use the tools pinned in
  hack/go.mod, but for examples we want to show users how to pin
  kustomize within the scope of their function repository.
  As an example, this example's pinned version of kustomize doesn't
  really need to stay current, unless the install process changes.
- Increment example version to v0.1.1
@karlkfi
Copy link
Contributor Author

karlkfi commented Apr 9, 2024

Rebased and ready to go

@karlkfi karlkfi requested a review from koba1t April 9, 2024 17:20
@koba1t
Copy link
Member

koba1t commented Apr 11, 2024

We release a container image to contain the kustomize binary when creating a new kustomize release.
kubernetes/k8s.io#6646

I think using those containers is enough to pin the kustomize version when running the container.
But if you have a different opinion from mine, let me know!

for example

docker pull registry.k8s.io/kustomize/kustomize:v5.4.1

@karlkfi
Copy link
Contributor Author

karlkfi commented Apr 11, 2024

I'm dealing with a bit of security theater here. I've got a scanner and policy that wants not just a pinned version but also some kind of checksum to confirm that the version is exactly the content you expected.

If we could use a package manager, like apt or apk we could let that handle the checksum and verification, but kustomize isn't published there. If we use go.mod we can let go install handle verifying the checksum in go.sum. But if we use a docker image, we also need the image checksum, not just the tag.

Since this is just an example, the risk is minimal, but the scanner still picks it up unless I configure an exception, and I don't really want to carry forward patches on a fork indefinitely.

So I'm just trying to find a way to validate a checksum that isn't a huge pain to maintain.

@koba1t
Copy link
Member

koba1t commented Apr 11, 2024

I think we also released SHA256. Does this not work for your use case?

https://github.com/kubernetes/k8s.io/pull/6646/files#diff-d9af49f58d305caa5e08487d5217b8c9c0eb972394301545ba4258d7c4ec9350R43

@karlkfi
Copy link
Contributor Author

karlkfi commented Apr 11, 2024

My first draft of this PR used the image sha and you told me not to. Thats why I switched to using go.mod.

Would you prefer the image sha instead?

@koba1t
Copy link
Member

koba1t commented Apr 12, 2024

you told me not to

I didn't read your comment below, and I didn't know why you are using digest for the pull container.
#5534 (comment)

I thought this was just an application example.
I care about some people who just ran this example but didn't care about container image architecture, and they will create a bug report issue because they got an error message from the docker...

Is the digest really different for different architectures? I thought you could build multi-arch images.

I think so because I checked the below page, but maybe you are right.
for example https://hub.docker.com/_/golang/tags

Would you prefer the image sha instead?

I think you have a special reason. That is a good way!
However, if you add an example for your new use case, I think it is better to create a new example with a readme containing a description.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 26, 2024
@k8s-ci-robot
Copy link
Contributor

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.

@karlkfi
Copy link
Contributor Author

karlkfi commented May 6, 2024

Replaced by #5691

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants