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

feat: scaffolding ca certs #1823

Merged
merged 1 commit into from
Jun 26, 2023
Merged

Conversation

lkingland
Copy link
Member

@lkingland lkingland commented Jun 21, 2023

  • 🎁 Scaffolded functions include root CA certs
  • 🎁 adds make target for updating embedded certs

All functions which utilize scaffolding (Go for now, but will work for all) will now include root CA certs, as provided by the curl project.

@knative-prow knative-prow bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 21, 2023
@knative-prow knative-prow bot requested review from maximilien and nainaz June 21, 2023 08:01
@codecov
Copy link

codecov bot commented Jun 21, 2023

Codecov Report

Patch coverage: 56.17% and project coverage change: +0.98 🎉

Comparison is base (8a078ce) 62.59% compared to head (fb63f3e) 63.58%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1823      +/-   ##
==========================================
+ Coverage   62.59%   63.58%   +0.98%     
==========================================
  Files         107      107              
  Lines       13645    13729      +84     
==========================================
+ Hits         8541     8729     +188     
+ Misses       4298     4159     -139     
- Partials      806      841      +35     
Flag Coverage Δ
e2e-test 35.25% <2.24%> (-0.21%) ⬇️
e2e-test-oncluster 31.22% <2.24%> (-0.21%) ⬇️
e2e-test-oncluster-runtime 26.67% <2.24%> (?)
e2e-test-runtime-go 25.62% <2.24%> (?)
e2e-test-runtime-node 26.57% <2.24%> (?)
e2e-test-runtime-python 26.57% <2.24%> (?)
e2e-test-runtime-quarkus 26.60% <2.24%> (?)
e2e-test-runtime-springboot 25.75% <2.24%> (?)
e2e-test-runtime-typescript 26.69% <2.24%> (?)
integration-tests 51.69% <56.17%> (+3.54%) ⬆️
unit-tests-macos-latest 49.56% <56.17%> (+0.05%) ⬆️
unit-tests-ubuntu-latest 50.32% <56.17%> (+0.02%) ⬆️
unit-tests-windows-latest 49.57% <56.17%> (+0.02%) ⬆️

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

Impacted Files Coverage Δ
pkg/scaffolding/scaffold.go 69.38% <0.00%> (-9.69%) ⬇️
pkg/filesystem/filesystem.go 63.38% <40.00%> (-0.86%) ⬇️
pkg/oci/containerize.go 60.74% <60.52%> (-0.63%) ⬇️
pkg/functions/repository.go 70.42% <100.00%> (+0.18%) ⬆️

... and 6 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@lkingland lkingland mentioned this pull request Jun 22, 2023
14 tasks
@lkingland lkingland force-pushed the scaffolding-certs branch 4 times, most recently from 19edbdf to 7c65640 Compare June 22, 2023 04:53
@lkingland lkingland marked this pull request as ready for review June 22, 2023 06:17
@knative-prow knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 22, 2023
@knative-prow knative-prow bot requested a review from jrangelramos June 22, 2023 06:17
@lkingland lkingland requested review from lance, salaboy, zroubalik, aslom and matejvasek and removed request for maximilien, jrangelramos and nainaz June 22, 2023 06:17
@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 22, 2023
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@matejvasek
Copy link
Contributor

matejvasek commented Jun 22, 2023

On similar note like #1823 (comment).
I really do not like using $(CODE) as prerequisites -- it cannot detect file deletion (only addtition/modification).
Moreover go build, go test, golangci-lint are smart enough to know when/what re-compilation/re-test is required.
Similarly I don't see much value in these stamps, what I see is possible risk of not running recipe when is should be run.
I rather run recipe that takes 500ms all the time than risking that the recipe is not done when it really needs to be.

@lkingland lkingland force-pushed the scaffolding-certs branch 3 times, most recently from c13bbc8 to 89225d7 Compare June 23, 2023 05:30
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 23, 2023
@lkingland lkingland force-pushed the scaffolding-certs branch 2 times, most recently from 1795986 to 79fa677 Compare June 23, 2023 11:04
@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 23, 2023
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 23, 2023
@lkingland
Copy link
Member Author

lkingland commented Jun 23, 2023

Thanks for the review @matejvasek , I have added filepath.ToSlash() to the filesystem symlink test in place of the conditionals, and removed all but the minimum makefile updates for discussion in a separate PR.

@matejvasek
Copy link
Contributor

/approve
/lgtm

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jun 23, 2023
@matejvasek
Copy link
Contributor

The windows test are failing on the symlink despite filepath.ToSlash(), it makes no sense.

@knative-prow knative-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 24, 2023
@lkingland
Copy link
Member Author

The windows test are failing on the symlink despite filepath.ToSlash(), it makes no sense.

Shucks. I was hoping the ToSlash would do the trick. I really don't mind if they are a few bytes different, so long as they are both symlinks.

@matejvasek
Copy link
Contributor

matejvasek commented Jun 24, 2023

The windows test are failing on the symlink despite filepath.ToSlash(), it makes no sense.

Shucks. I was hoping the ToSlash would do the trick.

@lkingland I think I know where the issue might be.
The unit test in GH Action also regenerates zz file on Windows.
That is IMO not good because actual app do use zz file generated on Linux (because of exec perms).
This means the GH Action test is testing something that is not really happening with actual app.

Anyway... the generate/templates/main.go scrip should also fixup links here (the same way it fixes up filepaths already here ).

I really don't mind if they are a few bytes different, so long as they are both symlinks.

But these bytes are significant.

@matejvasek
Copy link
Contributor

matejvasek commented Jun 24, 2023

actual app do use zz file generated on Linux

Actually I am not 100% positive about this (but if we are not doing it we should be). I am only sure that we version file generated on Linux.

@matejvasek
Copy link
Contributor

matejvasek commented Jun 24, 2023

@lkingland tl;dr we should not suppress the tests, it really indicates that something is wrong

We need filepath.ToSlash() in:

After these 3 changes it should work. I tested it.
I know this is bug in existing code, not it this PR, sill we should fix it.

diff --git a/generate/templates/main.go b/generate/templates/main.go
index 45ec4848..f29ceb4f 100644
--- a/generate/templates/main.go
+++ b/generate/templates/main.go
@@ -80,7 +80,7 @@ func main() {
 			if err != nil {
 				return err
 			}
-			_, err = w.Write([]byte(symlinkTarget))
+			_, err = w.Write([]byte(filepath.ToSlash(symlinkTarget)))
 			return err
 		case info.Mode()&fs.ModeType == 0: // regular file
 			f, err := os.Open(path)
diff --git a/pkg/filesystem/filesystem.go b/pkg/filesystem/filesystem.go
index d43aad75..c72cf5ee 100644
--- a/pkg/filesystem/filesystem.go
+++ b/pkg/filesystem/filesystem.go
@@ -173,7 +173,11 @@ func (o osFilesystem) Stat(name string) (fs.FileInfo, error) {
 
 func (o osFilesystem) Readlink(link string) (string, error) {
 	link = filepath.FromSlash(link)
-	return os.Readlink(filepath.Join(o.root, link))
+	t, err := os.Readlink(filepath.Join(o.root, link))
+	if err != nil {
+		return "", err
+	}
+	return filepath.ToSlash(t), nil
 }
 
 // subFS exposes subdirectory of underlying FS, this is similar to `chroot`.

@matejvasek
Copy link
Contributor

/approve
/lgtm

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jun 26, 2023
@knative-prow
Copy link

knative-prow bot commented Jun 26, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lkingland, matejvasek

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:
  • OWNERS [lkingland,matejvasek]

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

@knative-prow knative-prow bot merged commit 19509e5 into knative:main Jun 26, 2023
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. lgtm Indicates that a PR is ready to be merged. 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.

3 participants