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.

Sign up for GitHub

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

Jump to bottom

[BZ1365986] new-app: validate that EXPOSEd ports are numbers #10677

Merged
merged 1 commit into from
Oct 28, 2016
Merged

[BZ1365986] new-app: validate that EXPOSEd ports are numbers #10677

merged 1 commit into from
Oct 28, 2016

Conversation

php-coder
Copy link
Contributor

@php-coder php-coder commented Aug 26, 2016 •
edited
Loading

Add validation to oc new-app to ensure that exposed ports from the Dockerfile in a user's repository are numbers.

Example:

$ oc new-app https://github.com/php-coder/s2i-test#use_arg_directive 
error: the Dockerfile has an invalid EXPOSE instruction: could not parse "$JETTY_PORT": must be numerical

At the same time oc new-build works as before:

$ oc new-build https://github.com/php-coder/s2i-test#use_arg_directive 
--> Found Docker image 34a035d (2 days old) from Docker Hub for "java:8"

    * An image stream will be created as "java:8" that will track the source image
    * A Docker build using source code from https://github.com/php-coder/s2i-test#use_arg_directive will be created
      * The resulting image will be pushed to image stream "s2i-test:latest"
      * Every time "java:8" changes a new build will be triggered

--> Creating resources with label build=s2i-test ...
    imagestream "java" created
    imagestream "s2i-test" created
    buildconfig "s2i-test" created
--> Success
    Build configuration "s2i-test" created and build triggered.
    Run 'new-build logs -f bc/s2i-test' to stream the build progress.

PTAL @bparees

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1365986

bparees
bparees reviewed Aug 26, 2016
View reviewed changes
pkg/generate/app/cmd/resolve.go
@@ -502,6 +503,11 @@ func AddMissingComponentsToRefBuilder(
errs = append(errs, fmt.Errorf("the Dockerfile in the repository %q has no FROM instruction", info.Path))
continue
}
err := exposedPortsAreNumeric(dockerfileutil.LastExposedPorts(node))
if err != nil {
errs = append(errs, fmt.Errorf("the Dockerfile in the repository has invalid EXPOSE instruction: %v", err))
Copy link
Contributor

Choose a reason for hiding this comment

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

s/the Dockerfile in the repository/the Dockerfile/
s/has invalid/has an invalid/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@bparees bparees added this to the 1.3.1 milestone Aug 26, 2016
@php-coder php-coder force-pushed the bz1365986_new_app_and_arg_instruction branch from 5a0c070 to ff9fc69 Compare August 26, 2016 17:58
bparees
bparees reviewed Aug 26, 2016
View reviewed changes
pkg/generate/app/cmd/resolve.go
if err != nil {
errs = append(errs, fmt.Errorf("the Dockerfile has an invalid EXPOSE instruction: %v", err))
continue
}
Copy link
Contributor

Choose a reason for hiding this comment

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

i'm wondering if this is the right place for this check. is this also going to cause "oc new-build" to fail w/ these dockerfiles? because it probably shouldn't.

it should only be an error in cases where we're trying to generate a service(which is why we need to know what ports the dockerfile exposes).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is this also going to cause "oc new-build" to fail w/ these dockerfiles?

Yes, it will :-(

(oc new-app and oc new-build looks very similar. Do they duplicate each other?)

Copy link
Contributor

Choose a reason for hiding this comment

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

they use the same code.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you just need to move the check to where we're creating the service.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, will do. (In September.)

@php-coder php-coder changed the title [BZ1365986] new-app: validate that EXPOSEd ports are numbers [WIP][BZ1365986] new-app: validate that EXPOSEd ports are numbers Oct 13, 2016
@php-coder php-coder force-pushed the bz1365986_new_app_and_arg_instruction branch from ff9fc69 to 3443ea6 Compare October 24, 2016 17:41
@php-coder php-coder changed the title [WIP][BZ1365986] new-app: validate that EXPOSEd ports are numbers [BZ1365986] new-app: validate that EXPOSEd ports are numbers Oct 24, 2016
@php-coder
Copy link
Contributor Author

@bparees PTAL again. Because new-app and new-build both use AppConfig.Run() method, I have to add a special flag.

@php-coder php-coder force-pushed the bz1365986_new_app_and_arg_instruction branch from 3443ea6 to dcc2955 Compare October 24, 2016 17:46
bparees
bparees requested changes Oct 24, 2016
View reviewed changes
Copy link
Contributor

@bparees bparees left a comment

Choose a reason for hiding this comment

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

one nit and i'd like to see tests for both the new-app and new-build codepaths.

pkg/generate/app/cmd/newapp.go
func exposedPortsAreNumeric(node *dockerfileparser.Node) error {
for _, port := range dockerfileutil.LastExposedPorts(node) {
if _, err := strconv.ParseInt(port, 10, 32); err != nil {
return fmt.Errorf("could not parse %q: must be numerical", port)
Copy link
Contributor

Choose a reason for hiding this comment

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

s/numerical/numeric/

@bparees bparees self-assigned this Oct 24, 2016
@php-coder php-coder force-pushed the bz1365986_new_app_and_arg_instruction branch from dcc2955 to 80e16ec Compare October 27, 2016 12:30
@php-coder
Copy link
Contributor Author

@bparees PTAL again.

@bparees
Copy link
Contributor

bparees commented Oct 27, 2016

lgtm [merge]

@openshift-bot
Copy link
Contributor

openshift-bot commented Oct 27, 2016 •
edited
Loading

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10739/) (Image: devenv-rhel7_5263)

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 80e16ec

@openshift-bot
Copy link
Contributor

[Test]ing while waiting on the merge queue

@php-coder
Copy link
Contributor Author

php-coder commented Oct 27, 2016 •
edited
Loading

[test] flake #9548

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 80e16ec

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10739/) (Base Commit: 9f85202)

@openshift-bot openshift-bot merged commit 2a68b18 into openshift:master Oct 28, 2016
@php-coder php-coder deleted the bz1365986_new_app_and_arg_instruction branch October 31, 2016 11:52
@php-coder php-coder mentioned this pull request Nov 1, 2016
Merged
@dashford dashford mentioned this pull request May 19, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bparees bparees bparees requested changes

Assignees

@bparees bparees

Labels
None yet
Projects
None yet
Milestone
1.3.1
Development

Successfully merging this pull request may close these issues.
3 participants
@php-coder @bparees @openshift-bot