-
Notifications
You must be signed in to change notification settings - Fork 296
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
Add tests for HTTPS #671
Add tests for HTTPS #671
Conversation
ff23bfc
to
3f6832d
Compare
3f6832d
to
e010448
Compare
e010448
to
6df01dd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry that I missed this. LGTM overall
cmd/e2e-test/basic_https_test.go
Outdated
name := fmt.Sprintf("cert%d--%s", i, s.Namespace) | ||
cert, err := e2e.NewCert(name, h, tc.certType) | ||
if err != nil { | ||
t.Fatalf("error initializing cert: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit (and a couple below): error -> Error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -153,3 +156,68 @@ func EnsureIngress(s *Sandbox, ing *v1beta1.Ingress) (*v1beta1.Ingress, error) { | |||
|
|||
return currentIng, nil | |||
} | |||
|
|||
// DeleteSecret deletes a secret. | |||
func DeleteSecret(s *Sandbox, name string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe putting these under cert.go
as well?
func (c *Cert) Delete() {}
func (c *Cert) Create() {}
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh..seems like this func is forgotten?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I purposefully left just that one since there may be a use case later to delete a secret not related to a cert.
} | ||
t.Logf("Ingress created (%s/%s)", s.Namespace, ing.Name) | ||
|
||
ing, err = e2e.WaitForIngress(s, ing, nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't recall we have logic implemented to check if server responses with the correct cert based on request hostname. Similar to this https://github.com/kubernetes/kubernetes/blob/118e33dfcd8bf052f34d9a922145ab5f97e6a354/test/e2e/framework/ingress/ingress_utils.go#L638-L670.
Is that still necessary? (Maybe as followup if needed.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm missing something but I don't know if that check is worth it. Is there a case when we could potentially configure GCLB to serve the wrong cert? My thinking was that this could only happen if the user made a config error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One case that comes to my mind is in case of using multiple certs, if there is a bug that ingress controller attaches only one cert to GCLB, it will still pass this test. Should we at least check that all certs are attached?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we put a TODO / issue to fix this? We do need to check this behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I think checking that all certs are attached may be more worthwhile than checking if the "correct" cert is served. Will tackle that in the followup.
3a358cb
to
8c1147f
Compare
8c1147f
to
237ddbc
Compare
/approve |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bowei, MrHohn, 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 |
Porting from k/k. Note that the PR to remove the tests in k/k is kubernetes/kubernetes#75840
This PR is already pretty big so I'll add transition tests in a followup.
Ref: #667