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 ManagedCertificate CRD #508

Merged
merged 4 commits into from Oct 30, 2018

Conversation

@krzykwas
Copy link
Contributor

krzykwas commented Oct 2, 2018

This change adds to Ingress support for ManagedCertificate CRD to integrate with https://github.com/GoogleCloudPlatform/gke-managed-certs

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Oct 2, 2018

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@krzykwas

This comment has been minimized.

Copy link
Contributor

krzykwas commented Oct 2, 2018

I signed it

1 similar comment
@krzykwas

This comment has been minimized.

Copy link
Contributor

krzykwas commented Oct 2, 2018

I signed it

@krzykwas

This comment has been minimized.

Copy link
Contributor

krzykwas commented Oct 2, 2018

/assign @bowei

@rramkumar1

This comment has been minimized.

Copy link
Member

rramkumar1 commented Oct 2, 2018

/ok-to-test

@krzykwas

This comment has been minimized.

Copy link
Contributor

krzykwas commented Oct 10, 2018

/assign @rramkumar1

Show resolved Hide resolved cmd/glbc/main.go
@@ -43,6 +44,13 @@ var (
clusterUID = "aaaaa"
)

type recorderProducerMock struct {
}

This comment has been minimized.

@rramkumar1

rramkumar1 Oct 10, 2018

Member

Can you move this mock to pkg/events.

This comment has been minimized.

@rramkumar1

rramkumar1 Oct 10, 2018

Member

In the same file as the interface definition is fine.

This comment has been minimized.

@krzykwas

krzykwas Oct 11, 2018

Contributor

Done.

separator = ","
)

// splitAnnotation splits annotation by separator and trims whitespace

This comment has been minimized.

@rramkumar1

rramkumar1 Oct 10, 2018

Member

Can you put this in pkg/utils instead?

This comment has been minimized.

@krzykwas

krzykwas Oct 11, 2018

Contributor

Done. I also added a unit test. There was one special case I didn't expect: if annotation was empty, splitAnnotation produced []string{""} instead of nil or []string{}, I fixed it.

Show resolved Hide resolved pkg/loadbalancers/certificates.go Outdated
@bowei

This comment has been minimized.

Copy link
Member

bowei commented Oct 15, 2018

can you rebase and squash the commits, then split into:

  • vendor/ only changes
  • files changed for vendoring
  • implementation
  • unit tests

@krzykwas krzykwas force-pushed the krzykwas:mcrt branch 2 times, most recently from 00571a9 to 934d510 Oct 15, 2018

@krzykwas

This comment has been minimized.

Copy link
Contributor

krzykwas commented Oct 15, 2018

I rebased, squashed and split the commits into vendor/+Gopkg.lock changes, implementation and unit tests. I didn't get what files changed for vendoring means.

@rramkumar1

This comment has been minimized.

Copy link
Member

rramkumar1 commented Oct 18, 2018

/cc @prameshj

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Oct 18, 2018

@rramkumar1: GitHub didn't allow me to request PR reviews from the following users: prameshj.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @prameshj

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Show resolved Hide resolved pkg/utils/annotation.go
func (l *L7) checkSSLCert() error {
// Handle Pre-Shared cert and early return if used
if used, err := l.usePreSharedCert(); used {
// Handle annotation managed-certificates

This comment has been minimized.

@rramkumar1

rramkumar1 Oct 19, 2018

Member

So the previous behavior was that if the user specified both pre-shared certs and secrets, we would only take the pre-shared certs and ignore the secrets. Correct me if I am wrong, but here it seems like you accept all three (managed, pre-shared and secret).

I would prefer to stick with the existing semantics. Specifically, Managed certs should take precedence over everything else.

This comment has been minimized.

@krzykwas

krzykwas Oct 22, 2018

Contributor

I think that the current code either uses managed certificates and pre-shared-cert or managed certificates and k8s secrets. I think it does not support all 3 types at once. This is because of return at line 51.:

if used {
	l.sslCerts = append(managedSslCerts, preSharedSslCerts...)
	return err
}

Also I added a test case for that to loadbalancer_test.go (one of last two test cases).

This comment has been minimized.

@prameshj

prameshj Oct 23, 2018

Contributor

Any reason we want to mix the modes? If we support both managed certs and preshared together, user might be confused why preshared and secrets don't work together or why all 3 are not picked up. Supporting just one mode with a priority in case more than one is specified might be clearer?

This comment has been minimized.

@prameshj

prameshj Oct 23, 2018

Contributor

I guess one reason to combine preshared and managed certs is because neither of them is ingress created and hence do not need to be garbage-collected. I am curious to see if you had any other reasons, and also motivation for supporting managed certs and secrets .

This comment has been minimized.

@krzykwas

krzykwas Oct 23, 2018

Contributor

Only with modes combined it is possible to support no-downtime migration scenarios. GCLB has its own logic for selecting a certificate and Ingress really should just pass all the certificates down to GCLB. But pre-shared-cert and secrets are already implemented differently and I just wanted to keep this behavior unchanged.

This comment has been minimized.

@rramkumar1

rramkumar1 Oct 25, 2018

Member

As discussed offline, I think it would make sense to keep consistency with the existing behavior. Namely, only take one form of cert. In this case, the precedence would be managed cert > pre-shared > secret.

@@ -0,0 +1,32 @@
/*
Copyright 2015 The Kubernetes Authors.

This comment has been minimized.

@prameshj

prameshj Oct 23, 2018

Contributor

nit : Change to 2018?

This comment has been minimized.

@krzykwas

krzykwas Oct 23, 2018

Contributor

ACK - please leave it unresolved and once we agree what changes are necessary I'll take a look to fix it.

func (l *L7) checkSSLCert() error {
// Handle Pre-Shared cert and early return if used
if used, err := l.usePreSharedCert(); used {
// Handle annotation managed-certificates

This comment has been minimized.

@prameshj

prameshj Oct 23, 2018

Contributor

Any reason we want to mix the modes? If we support both managed certs and preshared together, user might be confused why preshared and secrets don't work together or why all 3 are not picked up. Supporting just one mode with a priority in case more than one is specified might be clearer?

func (l *L7) checkSSLCert() error {
// Handle Pre-Shared cert and early return if used
if used, err := l.usePreSharedCert(); used {
// Handle annotation managed-certificates

This comment has been minimized.

@prameshj

prameshj Oct 23, 2018

Contributor

I guess one reason to combine preshared and managed certs is because neither of them is ingress created and hence do not need to be garbage-collected. I am curious to see if you had any other reasons, and also motivation for supporting managed certs and secrets .

// getExistingSecretsSslCerts fetches SslCertificate resources created and managed by this load balancer
// instance. These SslCertificate resources were created based on kubernetes secrets in Ingress
// configuration.
func (l *L7) getExistingSecretsSslCerts() ([]*compute.SslCertificate, error) {

This comment has been minimized.

@prameshj

prameshj Oct 23, 2018

Contributor

Maybe call this "getIngressManagedSslCerts()" ?

This comment has been minimized.

@krzykwas

krzykwas Oct 23, 2018

Contributor

ACK, I can rename later when introducing all required changes.

var result []string
for _, token := range strings.Split(annotation, separator) {
if token != "" {
result = append(result, strings.TrimSpace(token))

This comment has been minimized.

@prameshj

prameshj Oct 23, 2018

Contributor

Is it possible for token to be " ", in which case we will still append an empty string to result since trimspace will make it "" ?
Maybe just check the result of TrimSpace for empty string?

This comment has been minimized.

@krzykwas

krzykwas Oct 23, 2018

Contributor

Good catch, thanks. I'll also fix it once we agree what changes are necessary.

Show resolved Hide resolved pkg/utils/annotation_test.go
Show resolved Hide resolved pkg/loadbalancers/l7.go

@krzykwas krzykwas force-pushed the krzykwas:mcrt branch from 934d510 to 35be9b2 Oct 26, 2018

@rramkumar1

This comment has been minimized.

Copy link
Member

rramkumar1 commented Oct 29, 2018

/lgtm
/hold

@prameshj if this looks good to you, we can merge.

@prameshj
Copy link
Contributor

prameshj left a comment

Some small nits, looks good to me otherwise.
Thanks for making the changes.

Show resolved Hide resolved pkg/loadbalancers/certificates.go Outdated
}

sel := labels.NewSelector()
sel.Add(*req)

This comment has been minimized.

@prameshj

prameshj Oct 29, 2018

Contributor

Can req be nil if there were no errors in line 156?

This comment has been minimized.

@krzykwas

krzykwas Oct 30, 2018

Contributor

I think it can't based on the implementation of apimachinery labels, and also based on the Go convention for constructors I observed: err != nil or err == nil and an object is instantiated.

This comment has been minimized.

@prameshj

prameshj Oct 30, 2018

Contributor

ok, thanks for verifying.

for _, mcrt := range mcrts {
if mcrt.Status.CertificateName != "" {
names = append(names, mcrt.Status.CertificateName)
}

This comment has been minimized.

@prameshj

prameshj Oct 29, 2018

Contributor

Is it valid to have managed cert status with empty Certificate name? Should we log an error in this case?

This comment has been minimized.

@krzykwas

krzykwas Oct 30, 2018

Contributor

When a user creates a ManagedCertificate, he wouldn't usually fill in the status field, so the CertificateName field can be empty and it is not an error.

@k8s-ci-robot k8s-ci-robot removed the lgtm label Oct 30, 2018

@rramkumar1

This comment has been minimized.

Copy link
Member

rramkumar1 commented Oct 30, 2018

/lgtm
/hold cancel

@k8s-ci-robot k8s-ci-robot added lgtm and removed do-not-merge/hold labels Oct 30, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Oct 30, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: krzykwas, rramkumar1

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:

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

@k8s-ci-robot k8s-ci-robot merged commit 5a27ec5 into kubernetes:master Oct 30, 2018

3 checks passed

cla/linuxfoundation krzykwas authorized
Details
pull-ingress-gce-test Job succeeded.
Details
tide In merge pool.
Details
@fproulx-eoscanada

This comment has been minimized.

Copy link

fproulx-eoscanada commented Nov 19, 2018

@krzykwas when can we expect this to be available in prod ?
Also, how and when are those releases made available to Kubernetes Engine, do I need to wait for an update of my Master for it to be available ?

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