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

IAP + CDN e2e testing implementation #319

Merged
merged 2 commits into from
Jun 19, 2018
Merged

IAP + CDN e2e testing implementation #319

merged 2 commits into from
Jun 19, 2018

Conversation

rramkumar1
Copy link
Contributor

@rramkumar1 rramkumar1 commented Jun 7, 2018 •
edited
Loading

Note that this PR also contains some small changes to the validator:
1. In order to prevent the http client in the validator from following redirects, I implemented the
CheckRedirect hook.
2. Move the pathToDefaultBackend variable up higher so that other files can use it.

Ref: #301

This change is Reviewable

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 7, 2018
@rramkumar1 rramkumar1 force-pushed the iap-cdn-e2e-tests branch 3 times, most recently from 4fbdc3a to ba89130 Compare June 11, 2018 16:46
@rramkumar1
Copy link
Contributor Author

/assign @bowei

@rramkumar1 rramkumar1 changed the title IAP + CDN e2e testing implementation [WIP] IAP + CDN e2e testing implementation Jun 11, 2018
@rramkumar1 rramkumar1 force-pushed the iap-cdn-e2e-tests branch 3 times, most recently from c113c28 to 85d1b07 Compare June 11, 2018 19:49
bowei
bowei reviewed Jun 13, 2018
View reviewed changes
pkg/fuzz/features/cdn.go Outdated
)

// Cdn is a feature in BackendConfig that supports using GCP CDN.
var Cdn = &CDNFeature{}
Copy link
Member

Choose a reason for hiding this comment

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

cdn? (no need to export?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since I made this fully capitalized now, it will technically be exported.

pkg/fuzz/features/cdn.go

// ModifyRequest implements fuzz.FeatureValidator.
func (v *cdnValidator) ModifyRequest(host, path string, req *http.Request) {
// Warm up the cache, regardless of whether this host + path has CDN enabled.
Copy link
Member

Choose a reason for hiding this comment

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

should probably comment that // cache=true makes the echo server add caching headers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done..

pkg/fuzz/features/features.go Outdated
@@ -25,4 +25,6 @@ import "k8s.io/ingress-gce/pkg/fuzz"
var All = []fuzz.Feature{
AllowHTTP,
PresharedCert,
Cdn,
Copy link
Member

Choose a reason for hiding this comment

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

CDN

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

pkg/fuzz/features/features.go Outdated
@@ -25,4 +25,6 @@ import "k8s.io/ingress-gce/pkg/fuzz"
var All = []fuzz.Feature{
AllowHTTP,
PresharedCert,
Cdn,
Iap,
Copy link
Member

Choose a reason for hiding this comment

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

IAP (golang code style)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

pkg/fuzz/features/iap.go
var iapEnabled bool
if backendConfig.Spec.Iap != nil && backendConfig.Spec.Iap.Enabled == true {
iapEnabled = true
}
Copy link
Member

Choose a reason for hiding this comment

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

do you need iapEnabled? why not return if false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See comment below.

pkg/fuzz/features/iap.go
return fuzz.CheckResponseSkip, nil
}
// If IAP is turned off, verify that the response code was not 302.
if !iapEnabled && resp.StatusCode == http.StatusFound {
Copy link
Member

Choose a reason for hiding this comment

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

don't need this?, standard check will be for 200

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main reason was to return a custom error.

@bowei
Copy link
Member

bowei commented Jun 14, 2018

We need to tag the image used for guitar tests. Right now as it targets master, the tests will start failing as it is testing the old ingress controller. Let's talk tomorrow.

/lgtm

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

New changes are detected. LGTM label has been removed.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 14, 2018
bowei
bowei reviewed Jun 18, 2018
View reviewed changes
pkg/fuzz/helpers.go
)

// BackendConfigForPath returns the BackendConfig associated with the given path.
// Note: This function returns an empty object (not nil pointer) if a BackendConfig
Copy link
Member

Choose a reason for hiding this comment

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

we probably want to distinguish these two cases for testing BackendConfig does not exist vs BackendConfig exists but is empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean have separate function for each case? I can take care of it in a separate PR.

bowei
bowei reviewed Jun 18, 2018
View reviewed changes
Copy link
Member

@bowei bowei left a comment

Choose a reason for hiding this comment

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

lgtm except for the helper function

@bowei bowei merged commit c848efe into kubernetes:master Jun 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bowei bowei bowei left review comments

Assignees

@bowei bowei

Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rramkumar1 @bowei @k8s-ci-robot