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 #301

Merged
merged 1 commit into from
Jun 12, 2018
Merged

IAP + CDN #301

merged 1 commit into from
Jun 12, 2018

Conversation

rramkumar1
Copy link
Contributor

@rramkumar1 rramkumar1 commented Jun 4, 2018 •
edited
Loading

This is the last PR for IAP + CDN integration.

TODO's in separate PR's:
1. Need to write an e2e test using new test framework.
2. Need to add more unit tests to verify behavior.

This change is Reviewable

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 4, 2018
@ConradIrwin
Copy link

Thanks @rramkumar1 looking forward to this :D.

@rramkumar1 rramkumar1 force-pushed the iap-cdn-work branch 2 times, most recently from eb877ac to 8c60a7b Compare June 6, 2018 20:22
@rramkumar1 rramkumar1 changed the title IAP + CDN [WIP] IAP + CDN Jun 6, 2018
@rramkumar1
Copy link
Contributor Author

/assign @MrHohn

MrHohn
MrHohn reviewed Jun 6, 2018
View reviewed changes
pkg/annotations/service.go Outdated
@@ -120,7 +122,9 @@ func (svc Service) GetBackendConfigs() (*BackendConfigs, error) {
return nil, err
}
if configs.Default == "" && len(configs.Ports) == 0 {
return nil, fmt.Errorf("no backendConfigs found in annotation: %v", val)
// We return an error here because if the annotation exists
// but has nothing in it, the user probably wanted to use it.
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the comments :)

pkg/backends/features/cdn.go Outdated
@@ -0,0 +1,71 @@
/*
Copyright 2015 The Kubernetes Authors.
Copy link
Member

Choose a reason for hiding this comment

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

2018

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/backends/features/cdn.go
be.EnableCDN = beConfig.Spec.Cdn.Enabled
// Apply the cache key policies
cacheKeyPolicy := beConfig.Spec.Cdn.CachePolicy
be.CdnPolicy = &composite.BackendServiceCdnPolicy{CacheKeyPolicy: &composite.CacheKeyPolicy{}}
Copy link
Member

Choose a reason for hiding this comment

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

Curious is it valid to have be.EnableCDN=false but be.CdnPolicy=non-nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. If the user disabled CDN but provided cache policies, then we will act on it accordingly.

pkg/backends/features/cdn.go
// made to actually persist the changes.
func applyCDNSettings(sp utils.ServicePort, be *composite.BackendService) {
beConfig := sp.BackendConfig
setCDNDefaults(beConfig)
Copy link
Member

Choose a reason for hiding this comment

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

So this is applying defaults to backendConfig but not backendService, I was confused :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, we apply defaults to the BackendConfig so that in the apply[IAP,CDN]Settings functions we don't have to have ugly nil checks.

pkg/composite/composite.go Outdated
@@ -240,6 +240,13 @@ func (be *BackendService) toAlpha() (*computealpha.BackendService, error) {
if err != nil {
return nil, fmt.Errorf("error unmarshalling BackendService JSON to compute alpha type: %v", err)
}
// Set force send fields. This is a temporary hack.
if alpha.CdnPolicy != nil && alpha.CdnPolicy.CacheKeyPolicy != nil {
alpha.CdnPolicy.CacheKeyPolicy.ForceSendFields = []string{"IncludeHost", "IncludeProtocol", "IncludeQueryString", "QueryStringBlaclist", "QueryStringWhitelist"}
Copy link
Member

Choose a reason for hiding this comment

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

QueryStringBlaclist -> QueryStringBlacklist, same for below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch :)

pkg/backends/features/iap.go Outdated
if be.Iap == nil || beTemp.Iap.Enabled != be.Iap.Enabled || beTemp.Iap.Oauth2ClientId != be.Iap.Oauth2ClientId || beTemp.Iap.Oauth2ClientSecretSha256 != be.Iap.Oauth2ClientSecretSha256 {
glog.V(3).Infof("BackendService is %+v", be)
applyIAPSettings(sp, be)
glog.V(3).Infof("Updated IAP settings for service %v/%v.", sp.ID.Service.Namespace, sp.ID.Service.Name)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe V(2)? Seems worth to log it if an update is needed.

Also Updated might not be accurate? As the actual update will happen after this.

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/backends/features/iap.go Outdated
// since that field is redacted when getting a BackendService.
beTemp.Iap.Oauth2ClientSecretSha256 = fmt.Sprintf("%x", sha256.Sum256([]byte(beTemp.Iap.Oauth2ClientSecret)))
if be.Iap == nil || beTemp.Iap.Enabled != be.Iap.Enabled || beTemp.Iap.Oauth2ClientId != be.Iap.Oauth2ClientId || beTemp.Iap.Oauth2ClientSecretSha256 != be.Iap.Oauth2ClientSecretSha256 {
glog.V(3).Infof("BackendService is %+v", be)
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, was debugging something and forgot to delete.

pkg/backends/features/iap.go Outdated
// and applies it to the BackendService if it is stale. It returns true
// if there were existing settings on the BackendService that were overwritten.
func EnsureIAP(sp utils.ServicePort, be *composite.BackendService) bool {
if sp.BackendConfig == nil {
Copy link
Member

Choose a reason for hiding this comment

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

I might be missing something...How is the disable case handled? Similar question for CDN as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed this offline but we should probably document that user should remove the annotation from the service first?

@rramkumar1 rramkumar1 force-pushed the iap-cdn-work branch 2 times, most recently from 6ef255a to 84f8b94 Compare June 7, 2018 20:52
@rramkumar1 rramkumar1 mentioned this pull request Jun 7, 2018
Merged
@rramkumar1 rramkumar1 force-pushed the iap-cdn-work branch 4 times, most recently from c93320f to 0dc0865 Compare June 11, 2018 18:16
MrHohn
MrHohn approved these changes Jun 12, 2018
View reviewed changes
Copy link
Member

@MrHohn MrHohn left a comment

Choose a reason for hiding this comment

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

/lgtm

pkg/backends/backends.go
nodePool: nodePool,
healthChecker: healthChecker,
namer: namer,
backendConfigEnabled: backendConfigEnabled,
Copy link
Member

Choose a reason for hiding this comment

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

Might be worth to separate this out to another PR if we can't get this in soon..

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 12, 2018
@nicksardo nicksardo merged commit e9dd9fe into kubernetes:master Jun 12, 2018
@ConradIrwin
Copy link

w00w00! thanks all :D

@rramkumar1
Copy link
Contributor Author

@ConradIrwin Note that this is not officially available yet. It will only be released when we cut a new ingress-gce release, which will be 1.2.0

@ConradIrwin
Copy link

Noted, thank you. Do you have an estimate on that timeline?

I'm just excited to see you're working through the backlog of feature requests :). (and I'd cheekily put another nudge in for #33, but don't block shipping this on implementing that :D)

@rramkumar1
Copy link
Contributor Author

@ConradIrwin A very conservative estimate would be early July.

@mikedanese
Copy link
Member

mikedanese commented Jul 17, 2018 •
edited
Loading

We shouldn't use abbreviations in names in APIs unless they are extremly common:

https://github.com/kubernetes/community/blob/master/contributors/devel/api-conventions.md#naming-conventions

CDN might fit the bill but IAP does not. If we do use abbreviations, they should be all caps in the go types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MrHohn MrHohn MrHohn approved these changes

Assignees

@MrHohn MrHohn

Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@rramkumar1 @ConradIrwin @mikedanese @MrHohn @nicksardo @k8s-ci-robot