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

Implement support for HTTPS Redirects #1206

Merged
merged 1 commit into from
Aug 6, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions pkg/annotations/ingress.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,8 @@ const (

// UrlMapKey is the annotation key used by controller to record GCP URL map.
UrlMapKey = StatusPrefix + "/url-map"
// UrlMapKey is the annotation key used by controller to record GCP URL map used for Https Redirects only.
RedirectUrlMapKey = StatusPrefix + "/redirect-url-map"
// HttpForwardingRuleKey is the annotation key used by controller to record
// GCP http forwarding rule.
HttpForwardingRuleKey = StatusPrefix + "/forwarding-rule"
Expand Down
12 changes: 11 additions & 1 deletion pkg/apis/frontendconfig/v1beta1/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,17 @@ type FrontendConfig struct {
// FrontendConfigSpec is the spec for a FrontendConfig resource
// +k8s:openapi-gen=true
type FrontendConfigSpec struct {
SslPolicy *string `json:"sslPolicy,omitempty"`
SslPolicy *string `json:"sslPolicy,omitempty"`
RedirectToHttps *HttpsRedirectConfig `json:"redirectToHttps,omitempty"`
}

// HttpsRedirectConfig representing the configuration of Https redirects
// +k8s:openapi-gen=true
type HttpsRedirectConfig struct {
Enabled bool `json:"enabled"`
// String representing the HTTP response code
// Options are MOVED_PERMANENTLY_DEFAULT, FOUND, TEMPORARY_REDIRECT, or PERMANENT_REDIRECT
ResponseCodeName string `json:"responseCodeName,omitempty"`
}

// FrontendConfigStatus is the status for a FrontendConfig resource
Expand Down
21 changes: 21 additions & 0 deletions pkg/apis/frontendconfig/v1beta1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

43 changes: 39 additions & 4 deletions pkg/apis/frontendconfig/v1beta1/zz_generated.openapi.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

36 changes: 36 additions & 0 deletions pkg/loadbalancers/l7.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,8 @@ type L7 struct {
cloud *gce.Cloud
// um is the UrlMap associated with this L7.
um *composite.UrlMap
// rum is the Http Redirect only UrlMap associated with this L7.
redirectUm *composite.UrlMap
// tp is the TargetHTTPProxy associated with this L7.
tp *composite.TargetHttpProxy
// tps is the TargetHTTPSProxy associated with this L7.
Expand Down Expand Up @@ -150,6 +152,13 @@ func (l *L7) edgeHop() error {
if err := l.ensureComputeURLMap(); err != nil {
return err
}

if flags.F.EnableFrontendConfig {
if err := l.ensureRedirectURLMap(); err != nil {
return fmt.Errorf("ensureRedirectUrlMap() = %v", err)
}
}

if l.runtimeInfo.AllowHTTP {
if err := l.edgeHopHttp(); err != nil {
return err
Expand Down Expand Up @@ -371,6 +380,23 @@ func (l *L7) Cleanup(versions *features.ResourceVersions) error {
if err := utils.IgnoreHTTPNotFound(composite.DeleteUrlMap(l.cloud, key, versions.UrlMap)); err != nil {
return err
}

// Delete RedirectUrlMap if exists
if flags.F.EnableFrontendConfig {
umName, supported := l.namer.RedirectUrlMap()
if !supported {
// Skip deletion
return nil
}
klog.V(2).Infof("Deleting Redirect URL Map %v", umName)
key, err := l.CreateKey(umName)
if err != nil {
return err
}
if err := utils.IgnoreHTTPNotFound(composite.DeleteUrlMap(l.cloud, key, versions.UrlMap)); err != nil {
return err
}
}
return nil
}

Expand Down Expand Up @@ -407,6 +433,16 @@ func (l *L7) getFrontendAnnotations(existing map[string]string) map[string]strin
} else {
delete(existing, annotations.TargetHttpsProxyKey)
}

// Handle Https Redirect Map
if flags.F.EnableFrontendConfig {
if l.redirectUm != nil {
existing[annotations.RedirectUrlMapKey] = l.redirectUm.Name
} else {
delete(existing, annotations.RedirectUrlMapKey)
}
}

// Note that ingress IP annotation is not deleted when user disables one of http/https.
// This is because the promoted static IP is retained for use and will be deleted only
// when load-balancer is deleted or user specifies a different IP.
Expand Down
52 changes: 52 additions & 0 deletions pkg/loadbalancers/loadbalancers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1098,6 +1098,58 @@ func TestFrontendConfigSslPolicy(t *testing.T) {
}
}

func TestFrontendConfigRedirects(t *testing.T) {
flags.F.EnableFrontendConfig = true
defer func() { flags.F.EnableFrontendConfig = false }()

j := newTestJig(t)
ing := newIngress()

// Use v2 naming scheme since v1 is not supported
ing.ObjectMeta.Finalizers = []string{common.FinalizerKeyV2}

gceUrlMap := utils.NewGCEURLMap()
gceUrlMap.DefaultBackend = &utils.ServicePort{NodePort: 31234, BackendNamer: j.namer}
gceUrlMap.PutPathRulesForHost("bar.example.com", []utils.PathRule{{Path: "/bar", Backend: utils.ServicePort{NodePort: 30000, BackendNamer: j.namer}}})
lbInfo := &L7RuntimeInfo{
AllowHTTP: true,
TLS: []*translator.TLSCerts{createCert("key", "cert", "name")},
UrlMap: gceUrlMap,
Ingress: ing,
FrontendConfig: &frontendconfigv1beta1.FrontendConfig{Spec: frontendconfigv1beta1.FrontendConfigSpec{RedirectToHttps: &frontendconfigv1beta1.HttpsRedirectConfig{Enabled: true}}},
}

l7, err := j.pool.Ensure(lbInfo)
if err != nil {
t.Fatalf("j.pool.Ensure(%v) = %v, want nil", lbInfo, err)
}

// Only verify HTTPS since the HTTP Target Proxy points to the redirect url map
verifyHTTPSForwardingRuleAndProxyLinks(t, j, l7)

if l7.redirectUm == nil {
t.Errorf("l7.redirectUm is nil")
}

tpName := l7.tp.Name
tp, err := composite.GetTargetHttpProxy(j.fakeGCE, meta.GlobalKey(tpName), meta.VersionGA)
if err != nil {
t.Error(err)
}

resourceID, err := cloud.ParseResourceURL(tp.UrlMap)
if err != nil {
t.Errorf("ParseResourceURL(%+v) = %v, want nil", tp.UrlMap, err)
}

path := resourceID.ResourcePath()
want := "global/urlMaps/" + l7.redirectUm.Name

if path != want {
t.Errorf("tps ssl policy = %q, want %q", path, want)
}
}

func TestEnsureSslPolicy(t *testing.T) {
t.Parallel()
j := newTestJig(t)
Expand Down
23 changes: 19 additions & 4 deletions pkg/loadbalancers/target_proxies.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,29 @@ const (

// checkProxy ensures the correct TargetHttpProxy for a loadbalancer
func (l *L7) checkProxy() (err error) {
isL7ILB := flags.F.EnableL7Ilb && utils.IsGCEL7ILBIngress(l.runtimeInfo.Ingress)
tr := translator.NewTranslator(isL7ILB, l.namer)
// Get UrlMap Name, could be the url map or the redirect url map
// TODO(shance): move to translator
var umName string
Copy link

Choose a reason for hiding this comment

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

Why didn't this code written as follow with less branches?

umName := l.um.Name
if flags.F.EnableFrontendConfig {
  // TODO(shance): check for empty name?
    if l.redirectUm != nil && l.runtimeInfo.FrontendConfig.Spec.RedirectToHttps != nil && l.runtimeInfo.FrontendConfig.Spec.RedirectToHttps.Enabled {
      umName = l.redirectUm.Name
    }
}

if flags.F.EnableFrontendConfig {
// TODO(shance): check for empty name?
if l.redirectUm != nil && l.runtimeInfo.FrontendConfig.Spec.RedirectToHttps != nil && l.runtimeInfo.FrontendConfig.Spec.RedirectToHttps.Enabled {
umName = l.redirectUm.Name
} else {
umName = l.um.Name
}
} else {
umName = l.um.Name
}

description, err := l.description()
urlMapKey, err := l.CreateKey(umName)
if err != nil {
return err
}
urlMapKey, err := l.CreateKey(l.um.Name)

isL7ILB := flags.F.EnableL7Ilb && utils.IsGCEL7ILBIngress(l.runtimeInfo.Ingress)
tr := translator.NewTranslator(isL7ILB, l.namer)

description, err := l.description()
if err != nil {
return err
}
Expand Down
69 changes: 69 additions & 0 deletions pkg/loadbalancers/url_maps.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/ingress-gce/pkg/composite"
"k8s.io/ingress-gce/pkg/events"
"k8s.io/ingress-gce/pkg/flags"
"k8s.io/ingress-gce/pkg/translator"
"k8s.io/ingress-gce/pkg/utils"
"k8s.io/klog"
Expand Down Expand Up @@ -80,6 +81,74 @@ func (l *L7) ensureComputeURLMap() error {
return nil
}

func (l *L7) ensureRedirectURLMap() error {
feConfig := l.runtimeInfo.FrontendConfig
isL7ILB := flags.F.EnableL7Ilb && utils.IsGCEL7ILBIngress(&l.ingress)

t := translator.NewTranslator(isL7ILB, l.namer)
env := &translator.Env{FrontendConfig: feConfig, Ing: &l.ingress}

name, namerSupported := l.namer.RedirectUrlMap()
expectedMap := t.ToRedirectUrlMap(env, l.Versions().UrlMap)

key, err := l.CreateKey(name)
if err != nil {
return err
}

// TODO(shance): Remove this get unless the ingress status has the redirectUrlMap
currentMap, err := composite.GetUrlMap(l.cloud, key, l.Versions().UrlMap)
Copy link
Contributor

Choose a reason for hiding this comment

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

Might worth avoiding this GET call every time an irrelevant ingress was synced.

One idea is to check if the the ingress status annotation has the redirectUrlMap

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

if utils.IgnoreHTTPNotFound(err) != nil {
return err
}

// Do not expect to have a RedirectUrlMap
if expectedMap == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

add a comment here to say "do not expect to have a RedirectUrlMap"

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

if currentMap == nil {
return nil
} else {
if err := composite.DeleteUrlMap(l.cloud, key, l.Versions().UrlMap); err != nil {
return err
}
}
return nil
}

// Cannot enable for internal ingress
if isL7ILB {
return fmt.Errorf("error: cannot enable HTTPS Redirects with L7 ILB")
}

if !namerSupported {
return fmt.Errorf("error: cannot enable HTTPS Redirects with the V1 Ingress naming scheme. Please recreate your ingress to use the newest naming scheme.")
}

if currentMap == nil {
if err := composite.CreateUrlMap(l.cloud, key, expectedMap); err != nil {
return err
}
} else if compareRedirectUrlMaps(expectedMap, currentMap) {
expectedMap.Fingerprint = currentMap.Fingerprint
if err := composite.UpdateUrlMap(l.cloud, key, expectedMap); err != nil {
return err
}
}

l.redirectUm = expectedMap

return nil
}

// compareRedirectUrlMaps() compares the fields specified on the url map by the frontendconfig and returns true
// if there's a diff, false otherwise
func compareRedirectUrlMaps(a, b *composite.UrlMap) bool {
if a.DefaultUrlRedirect.HttpsRedirect != b.DefaultUrlRedirect.HttpsRedirect ||
a.DefaultUrlRedirect.RedirectResponseCode != b.DefaultUrlRedirect.RedirectResponseCode {
return true
}
return false
}

// getBackendNames returns the names of backends in this L7 urlmap.
func getBackendNames(computeURLMap *composite.UrlMap) ([]string, error) {
beNames := sets.NewString()
Expand Down
22 changes: 22 additions & 0 deletions pkg/translator/translator.go
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,28 @@ func ToCompositeURLMap(g *utils.GCEURLMap, namer namer.IngressFrontendNamer, key
return m
}

// ToRedirectUrlMap returns the UrlMap used for HTTPS Redirects on a L7 ELB
// This function returns nil if no url map needs to be created
func (t *Translator) ToRedirectUrlMap(env *Env, version meta.Version) *composite.UrlMap {
if env.FrontendConfig == nil || env.FrontendConfig.Spec.RedirectToHttps == nil {
return nil
}

if !env.FrontendConfig.Spec.RedirectToHttps.Enabled {
return nil
}

// Second arg is handled upstream
name, _ := t.FrontendNamer.RedirectUrlMap()
redirectConfig := env.FrontendConfig.Spec.RedirectToHttps
expectedMap := &composite.UrlMap{
Name: name,
DefaultUrlRedirect: &composite.HttpRedirectAction{HttpsRedirect: redirectConfig.Enabled, RedirectResponseCode: redirectConfig.ResponseCodeName},
Version: version,
}
return expectedMap
}

// getNameForPathMatcher returns a name for a pathMatcher based on the given host rule.
// The host rule can be a regex, the path matcher name used to associate the 2 cannot.
func getNameForPathMatcher(hostRule string) string {
Expand Down
Loading