Add validation for build.spec.source.url #512

xiujuan95 · 2020-12-08T07:51:30Z

Fix issue: #464

Add a new package git to verify if remote repository exists. Here, leverage go-git package and call List func to do the same thing with git ls-remote -p ${source_url} command to check it;

If a valid remote public repository exists, nothing will be returned when run this command. Otherwise, it will return an error like below:

xiangxiulis-mbp:build xiangxiuli$ kubectl get build
NAME                                        REGISTERED   REASON                 BUILDSTRATEGYKIND      BUILDSTRATEGYNAME   CREATIONTIME
buildpack-nodejs-build           False        repository not found   ClusterBuildStrategy   buildpacks-v3       2m27s

Introduce a new annotation build.build.dev/verify.repository. If the value of it is true, then build controller will go to check sourceUrl, otherwise, it won't check it;
Introduce two new build failed reasons repository not found and invalid source url:

xiangxiulis-mbp:build xiangxiuli$ k get build
NAME                                        REGISTERED   REASON                 BUILDSTRATEGYKIND      BUILDSTRATEGYNAME   CREATIONTIME
buildpack-nodejs-build         False        repository not found   ClusterBuildStrategy   buildpacks-v3       2m27s
buildpack-nodejs-build-invalidURL   False        invalid source url     ClusterBuildStrategy   buildpacks-v3       116s

Add two unit tests to verify valid remote public repositories exist or not;
Add several integration tests, mainly cover below scenario:
- Check a invalid remote public repo;
- Check source url is invalid, such as foobar;
- Skip check private git repo;

Note: this PR mainly focus on verifying a public repo.

xiujuan95 · 2020-12-08T08:03:06Z

/test 4.4-unit

HeavyWombat

Thanks for the validation. I think this and possible other validation can be really helpful for end-user happiness. I would like to ask you to look into an alternative solution for the actual Git interaction.

pkg/git/git.go

pkg/controller/build/build_controller.go

pkg/apis/build/v1alpha1/build_types.go

pkg/git/git_test.go

pkg/apis/build/v1alpha1/build_types.go

pkg/controller/build/build_controller.go

pkg/git/git.go

pkg/git/git_test.go

test/integration/build_to_git_test.go

pkg/git/git.go

coreydaley · 2020-12-10T19:01:08Z

It makes a pull request much easier to review if you split it into two commits, one for the code that you are writing, and one for vendoring dependencies. Otherwise we find ourselves combing through 400+ files.

xiujuan95 · 2020-12-11T04:57:12Z

It makes a pull request much easier to review if you split it into two commits, one for the code that you are writing, and one for vendoring dependencies. Otherwise we find ourselves combing through 400+ files.

@coreydaley Thanks for your suggestions! Now, I split it into two. Pls take a look again, thanks!

coreydaley · 2020-12-11T13:09:25Z

It makes a pull request much easier to review if you split it into two commits, one for the code that you are writing, and one for vendoring dependencies. Otherwise we find ourselves combing through 400+ files.

@coreydaley Thanks for your suggestions! Now, I split it into two. Pls take a look again, thanks!

Awesome, thank you!

qu1queee

@xiujuan95 nice work. There is still some pieces of this PR to improve, but we are close. Sorry for my late reply. I didn´t check the integration-tests yet, but lets address the rest of my comments first.

pkg/controller/build/build_controller.go

pkg/git/git.go

pkg/git/git_test.go

test/catalog.go

test/integration/build_to_git_test.go

xiujuan95 · 2020-12-15T06:18:10Z

/test 4.4-unit

pkg/controller/build/build_controller.go

pkg/git/git.go

test/build_samples.go

xiujuan95 · 2020-12-16T04:09:26Z

@coreydaley @qu1queee There are many many comments. I think I have refined code according to your all comments. Pls take a look again, thanks! If I am missing anything, pls let me know!

coreydaley · 2020-12-16T13:31:15Z

pkg/apis/build/v1alpha1/build_types.go

+
+	// AnnotationBuildRefRepository is a label key that tells the Build Controller to check whether remote repository exists or not.
+	// If its value is true, controller checks remote repository, otherwise, it won't be checked
+	AnnotationBuildRefRepository = "build.build.dev/verify.repository"


This constant name doesn't really signify what it is being used for, how about something like "AnnotationBuildVerifyRepository" ?

coreydaley · 2020-12-16T13:35:03Z

pkg/git/git.go

+}
+
+// ValidateGitURLFormat validates the given URL format is valid and public or not
+func ValidateGitURLFormat(url string) string {


From what I can see, this function is not really validating anything, it is parsing the url and returning whether it is a public, private, or invalid repository, so the function name seems a bit off. How about something like "IdentifyGitURLType" or "ParseGitURLType" ?

pkg/git/git_test.go

coreydaley · 2020-12-16T13:37:07Z

test/integration/build_to_git_test.go

+//
+// SPDX-License-Identifier: Apache-2.0
+
+package integration_test


Shouldn't this just be "package integration"?

Keep use integration_test package name.

pkg/git/git_suite_test.go

qu1queee

@xiujuan95 thanks, we should be close to finish. There is one more concern here. The current PR does not enforce the GIT validation by default. If we want the Build controller to validate a Build spec.source.url, users need to define the annotation.

I believe we need to deal with the GIT validation in the same way as we do with the secrets and strategies validation which is by default. But, because of @gabemontero concerns, we should be flexible, therefore the annotation should be a way to allow users to disable the validation if desired. I know this contradicts my statement in #464 (comment) , but now that I see the PR I think we should opt-in by default, and opt-out if desired via the annotation. @gabemontero fyi.

xiujuan95 · 2020-12-17T03:58:08Z

The current PR does not enforce the GIT validation by default. If we want the Build controller to validate a Build spec.source.url, users need to define the annotation.

Just for your info, I realized this annotation will be alway added and set to be true by UI and CLI by default, so our users don't need to define this annotation by themselves. But for shipwright build users, yes, they indeed need to add this annotation by themselves.

Besides, if we validate sourceURL by default, then most of our test cases will be fail unless we updates all sample cases to add this annotation and set it to be true.

So @qu1queee , I think current way is fine for us. Only the value of this annotation is true, Build will go to verify sourceURL.

xiujuan95 · 2020-12-17T06:54:46Z

@coreydaley @qu1queee I add a new commit to update build document, pls take a look, thanks!

qu1queee · 2020-12-17T09:47:27Z

@xiujuan95 im not sure I fully understand your concerns.

Besides, if we validate sourceURL by default, then most of our test cases will be fail unless we updates all sample cases to add this annotation and set it to be true.

why the above? if we validate by default, the annotation does not need to be set it at all, we would use the annotation only to disable the validation, not to enable.

Just for your info, I realized this annotation will be alway added and set to be true by UI and CLI by default, so our users don't need to define this annotation by themselves. But for shipwright build users, yes, they indeed need to add this annotation by themselves.

I dont think is true. I think in general there is a thinking that UI/CLI can abstract complexity from users, e.g. adding an annotation in Builds or Secrets during creation , thinking which we should use only if needed. For example, for the build.build.dev/referenced.secret we really needed users to add this on their secrets(we cannot do this because of performance implications in our controllers), and there we acknowledge that UI/CLI can remove this complexity by adding the annotation on the secrets creation in behalf of users. Another example is build.build.dev/build-run-deletion annotation in Build, where we cannot enable this annotation by default, because then we will defining for users a behaviour that is only the user responsibility to define. But for build.build.dev/verify.repository, I´m not sure why we need to delegate this responsibility to UI/CLI, specially when there is no real argument to do this. We can enable this validation by default, so why we wouldn´t do this. Not doing this will force UI/CLI users to abstract this complexity(annotation definition), but as I said, there is no real argument to delegate this to them. The only argument I see is around avoiding this validation, e.g. concerns on doing a remote call or similar, therefore the flexibility to disable this validation via an annotation.

xiujuan95 · 2020-12-21T03:11:34Z

why the above? if we validate by default, the annotation does not need to be set it at all, we would use the annotation only to disable the validation, not to enable.

@qu1queee @zhangtbj @SaschaSchwarze0 I am fine about validating sourceURL by default. But I still want to say if we validate it by default, we need to update all our test samples and add this annotation and set it to be false.

This is because we don't set the annotation by default, that means b.GetAnnotations()[build.AnnotationBuildVerifyRepository] is an empty string, however, Build also will validate sourceURL by default.

Under this situation, because for all our test samples, sourceURLs only are fake, then Build will validate these failed because sourceURL is invalid or not found.

For example, when we create a build with an invalid URL: https://github.com/shipwright-io/build/blob/master/test/catalog.go#L55-L78 , Build will validate URL by default. Then build will be fail because of invalidate sourceURL. However, this sample is created for validating secret. So ideally, we expect Build will skip validate URL for this test case, so we need to update this sample and build.build.dev/verify.repository : "false" annotation to it. Otherwise, this build will be fail because of an unexpected reason. For other test cases, we need to do the same action.

Not sure if I express my concerns clearly or not?

qu1queee · 2021-01-14T13:16:33Z

@bigkevmcd @SaschaSchwarze0 @gabemontero asking for re-review on this PR, we already addressed all the required changes, thanks for this, very valuable feedback.

One comment is that we will be working on a second PR where we are enhancing the overall Status of the Build, there we will improve on the current new error messages we are putting now in Build Status.Reason field.

SaschaSchwarze0

/lgtm

Nice work.

gabemontero

I have what I think is a minor addition request .... we've certainly seen use of http for the git protocol in some cases with openshift build v1

gabemontero · 2021-01-18T14:08:34Z

docs/build.md

+    revision: master
+```
+
+_Note_: The Build controller only validates two scenarios. The first one where the endpoint uses an `https` protocol, the second one when a `ssh` protocol (_e.g. `git@`_) is defined and none referenced secret was provided(_e.g. source.credentials.name_).


technically speaking, users can still use http for accessing git repositories ... especially if they are private/local ones vs. actual github.com

I think we should allow for that as well

I also do not get why we would rule out http at this point.

We miss this one. I added it, please take a look and resolve this conversation if possible.

gabemontero · 2021-01-18T14:11:17Z

pkg/git/git.go

+	}
+
+	switch endpoint.Protocol {
+	case httpsProtocol:


yeah correlating with my comment in the doc section, http is a valid, if not primary, use case, that should be easy to add

and this would be the spot, correct?

I'd suggest that it should log out that it's insecurely accessing the repository when using plain HTTP.

It could be a typo.

I added it. Thanks for the feedback. @bigkevmcd somehow I cannot re-request a review to you, not sure why.

HeavyWombat

I think we should consider allowing http if possible.

docs/build.md

HeavyWombat · 2021-01-18T15:57:47Z

docs/build.md

+    revision: master
+```
+
+_Note_: The Build controller only validates two scenarios. The first one where the endpoint uses an `https` protocol, the second one when a `ssh` protocol (_e.g. `git@`_) is defined and none referenced secret was provided(_e.g. source.credentials.name_).


I also do not get why we would rule out http at this point.

pkg/controller/build/build_controller.go

pkg/controller/build/build_controller_test.go

test/integration/build_to_git_test.go

gabemontero · 2021-01-18T17:09:17Z

pkg/git/git.go

+	}
+
+	switch endpoint.Protocol {
+	case httpsProtocol, httpProtocol:


I think @bigkevmcd 's suggestion at #512 (comment) is a good idea

So have a case httpProtocol: that prints that message, and then fallsthrough to the httpsProtocol block.

Othwerise I'm good @qu1queee

@bigkevmcd this can only be logged out if the validation fails. Just to be clear, you say that if an http protocol is used, we should enhance remote repository unreachable to be something like insecurely accessing the repository failed, repository unreachable ?

I think any time we parse an httpProtocol case, we should log this out as a possibly insecure case.

@bigkevmcd thanks. I see. Do you mind if I take this enhancement into the other PR we are gonna make available soon for review, which is #536 . At the moment Build have only a Status.Reason which is a message, what we are gonna do is to make Status.Reason a one-word camelCase and introduce Status.Message, see field changes .With Status.Message, we can then embed there the as a possibly insecure case whenever we validate an httpProtocol. I think we have more flexibility on that pr for this suggested change.

I like the idea of incorporating this into the #536 work @qu1queee - good idea

That said, I don't see why that would preclude us from still providing a simple informational log here. Catch the user's attention both in real time (via the log) and post-mortem (via the Build status).

That would be my preference.

…alidates spec.source.url refine the main logic of build validates sourceURL return validate error and update error only return updateError when validate sourceURL failed rename verify sourceURL annotation and make verifySourceURL by default update test cases again revert verifySourceURL verify sourceURL by default add unit test for validateSourceURL return a same error when list remote repo failed output different errors and update tests Signed-off-by: Enrique Encalada <encalada@de.ibm.com>

Signed-off-by: Enrique Encalada <encalada@de.ibm.com>

Change verify sourceURL by default Signed-off-by: Enrique Encalada <encalada@de.ibm.com>

gabemontero · 2021-01-19T13:09:53Z

/approve

still have some discussion between @qu1queee @bigkevmcd and myself on the http vs. https validation

sbose78 · 2021-01-19T15:21:30Z

Thanks for your kind reminder! As far as I know, until now, we don't have any users are using Azure. If have in the future, we can modify it again I think.

@xiujuan95 I feel this may not be a fair assumption 🤔

xiujuan95 · 2021-01-20T07:12:09Z

I feel this may not be a fair assumption 🤔

@sbose78 Aha, maybe I am missing something, please correct me about this and put your thoughts, thanks a lot!

SaschaSchwarze0 · 2021-01-20T08:13:22Z

/approve

Not sure why there is no approved tag for this comment from @gabemontero.

/lgtm
/approve

openshift-ci-robot · 2021-01-20T08:13:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gabemontero, SaschaSchwarze0

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~~ [SaschaSchwarze0,gabemontero]

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

openshift-ci-robot requested review from HeavyWombat and qu1queee December 8, 2020 07:51

HeavyWombat requested changes Dec 8, 2020

View reviewed changes

pkg/git/git.go Outdated Show resolved Hide resolved

pkg/controller/build/build_controller.go Outdated Show resolved Hide resolved

pkg/controller/build/build_controller.go Outdated Show resolved Hide resolved

pkg/controller/build/build_controller.go Outdated Show resolved Hide resolved

coreydaley reviewed Dec 8, 2020

View reviewed changes

pkg/apis/build/v1alpha1/build_types.go Outdated Show resolved Hide resolved

coreydaley reviewed Dec 8, 2020

View reviewed changes

pkg/git/git_test.go Show resolved Hide resolved

qu1queee requested a review from HeavyWombat December 10, 2020 14:33

HeavyWombat requested changes Dec 10, 2020

View reviewed changes

qu1queee requested changes Dec 14, 2020

View reviewed changes

qu1queee requested a review from coreydaley December 15, 2020 16:41

coreydaley reviewed Dec 15, 2020

View reviewed changes

xiujuan95 requested review from qu1queee and coreydaley December 16, 2020 13:17

coreydaley reviewed Dec 16, 2020

View reviewed changes

pkg/git/git_test.go Show resolved Hide resolved

coreydaley reviewed Dec 16, 2020

View reviewed changes

pkg/git/git_suite_test.go Show resolved Hide resolved

qu1queee requested changes Dec 16, 2020

View reviewed changes

xiujuan95 requested review from coreydaley and qu1queee December 17, 2020 06:54

zhangtbj removed their request for review January 9, 2021 05:37

qu1queee removed their request for review January 14, 2021 10:47

openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 14, 2021

qu1queee requested review from gabemontero, qu1queee and SaschaSchwarze0 January 14, 2021 13:07

SaschaSchwarze0 approved these changes Jan 14, 2021

View reviewed changes

openshift-ci-robot assigned SaschaSchwarze0 Jan 14, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 14, 2021

qu1queee requested a review from HeavyWombat January 15, 2021 08:25

gabemontero requested changes Jan 18, 2021

View reviewed changes

HeavyWombat requested changes Jan 18, 2021

View reviewed changes

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 18, 2021

qu1queee requested review from HeavyWombat and gabemontero January 18, 2021 17:02

gabemontero reviewed Jan 18, 2021

View reviewed changes

xiujuan95 added 3 commits January 19, 2021 10:33

update vendor dependencies

b073f77

Signed-off-by: Enrique Encalada <encalada@de.ibm.com>

update document

9a2d98b

Change verify sourceURL by default Signed-off-by: Enrique Encalada <encalada@de.ibm.com>

qu1queee requested a review from gabemontero January 19, 2021 09:39

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 20, 2021

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 20, 2021

openshift-merge-robot merged commit a46cddf into shipwright-io:master Jan 20, 2021

Add validation for build.spec.source.url #512

Add validation for build.spec.source.url #512

Conversation

xiujuan95 commented Dec 8, 2020 • edited

xiujuan95 commented Dec 8, 2020

HeavyWombat left a comment

Choose a reason for hiding this comment

coreydaley commented Dec 10, 2020

xiujuan95 commented Dec 11, 2020

coreydaley commented Dec 11, 2020

qu1queee left a comment

Choose a reason for hiding this comment

xiujuan95 commented Dec 15, 2020

xiujuan95 commented Dec 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qu1queee left a comment

Choose a reason for hiding this comment

xiujuan95 commented Dec 17, 2020 • edited

xiujuan95 commented Dec 17, 2020

qu1queee commented Dec 17, 2020

xiujuan95 commented Dec 21, 2020

qu1queee commented Jan 14, 2021

SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

gabemontero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HeavyWombat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qu1queee Jan 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabemontero commented Jan 19, 2021

sbose78 commented Jan 19, 2021

xiujuan95 commented Jan 20, 2021

SaschaSchwarze0 commented Jan 20, 2021

openshift-ci-robot commented Jan 20, 2021

xiujuan95 commented Dec 8, 2020 •

edited

xiujuan95 commented Dec 17, 2020 •

edited

qu1queee Jan 18, 2021 •

edited