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

More nodeup golden tests #9248

Merged
merged 2 commits into from
Jun 8, 2020
Merged

More nodeup golden tests #9248

merged 2 commits into from
Jun 8, 2020

Conversation

justinsb
Copy link
Member

@justinsb justinsb commented Jun 2, 2020

Add tests for kube-controller-manager, kube-proxy, kube-scheduler

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 2, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb

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 requested review from hakman and rdrgmnzs June 2, 2020 12:26
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 2, 2020
johngmyers
johngmyers reviewed Jun 2, 2020
View reviewed changes
nodeup/pkg/model/kubelet_test.go Outdated
"apiserver-aggregator-ca": mustParsePrivateKey(dummy_key),
"kube-controller-manager": mustParsePrivateKey(dummy_key),
"kube-proxy": mustParsePrivateKey(dummy_key),
"kube-scheduler": mustParsePrivateKey(dummy_key),
Copy link
Member

Choose a reason for hiding this comment

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

Would be slightly better to use different certs/keys to catch cases where the code used the wrong CA, but then it's not likely a human would catch the wrong key when reviewing the expected output.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't disagree; I debated actually removing the keys entirely before comparison, but I figured that would be more confusing.

It's the flags and other pod settings (e.g. resources) that I think are particularly valuable in these tests.

Copy link
Member

@johngmyers johngmyers Jun 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

That's a minor point. The more concerning thing would be what to do in my planned refactor that causes the kube-scheduler client cert to be generated out of the nodeup task. It would generate different output on every run.

Copy link
Member

Choose a reason for hiding this comment

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

I suppose I'd need to access pki.IssueCert through an interface that the tests would mock out.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah - I have two ideas on that front then.

  • If you've put cert generation into a task (which isn't a bad idea, you might be doing that already) then I think we'll be testing the right thing - the task invocation arguments, not the filesystem write calls.

  • Otherwise we can post-process the serialized yaml and either remove the keys or replace them with their key characteristics (e.g. "CN=foo, Validity=Client")

Are you planning on putting cert generation into a task?

Copy link
Member

Choose a reason for hiding this comment

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

#9172

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll take a look at #9130, to get closer to #9172. Your approach looks good. I don't know exactly how the TaskDependentResource will get serialized, but I imagine we can make it work nicely!

We can always defer this until #9172 has landed ... we've gotten by without it so far!

@justinsb
Copy link
Member Author

justinsb commented Jun 2, 2020

/retest

@hakman
Copy link
Member

hakman commented Jun 2, 2020

kubernetes/test-infra#17805 should fix e2e.
/retest

@hakman
Copy link
Member

hakman commented Jun 2, 2020

/retest

johngmyers
johngmyers reviewed Jun 2, 2020
View reviewed changes
nodeup/pkg/model/kube_apiserver_test.go
nodeUpModelContext.KeyStore = &fakeKeyStore{T: t}
keystore := &fakeCAStore{}
keystore.T = t
nodeUpModelContext.KeyStore = keystore
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for the more verbose style?

Copy link
Member Author

Choose a reason for hiding this comment

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

Edit: because of the embedded type, it would be &fakeCAStore{fakeKeyStore: fakeKeyStore{t}}

Copy link
Member

Choose a reason for hiding this comment

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

I suggest creating a NewFakeCAStore(*testing.T). You can push default test data into it as well.

nodeup/pkg/model/kube_apiserver_test.go Outdated
func (k fakeKeyStore) FindCertificatePool(name string) (*fi.CertificatePool, error) {
panic("implement me")
type fakeCAStore struct {
fakeKeyStore
Copy link
Member

Choose a reason for hiding this comment

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

Since you're changing all existing references to fakeKeyStore to fakeCAStore, why keep fakeKeyStore separate?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not entirely happy with the dual CAStore / KeyStore structure, and I'm hoping we can clean it up over time. I didn't want to add another thing that would need to be untangled later.

Copy link
Member

@johngmyers johngmyers Jun 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The only things that take just a KeyStore are BuildKubecfg and fi.Context. One thing using the field in fi.Context upcasts it to a VFSCAStore. (The other is fitasks.Keypair.) I don't think the interface split is well-designed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I do agree with that - it has resisted my attempts at making them cleaner (thus far)

Copy link
Member

Choose a reason for hiding this comment

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

It comes down to sharing fi.Context between nodeup and cloudup. Two of the three methods in KeyStore are only needed by specific cloudup tasks.

johngmyers
johngmyers reviewed Jun 2, 2020
View reviewed changes
nodeup/pkg/model/kube_apiserver_test.go Outdated
@@ -70,53 +71,63 @@ type fakeKeyStore struct {
T *testing.T
}

var _ fi.Keystore = &fakeKeyStore{}
Copy link
Member

Choose a reason for hiding this comment

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

Not actually necessary, as this file is assigning it to a field of type CAStore and a CAStore embeds Keystore.

But perhaps this type should be pulled out into a separate file, since it is referenced from tests other than kube_apiserver?

Copy link
Member Author

@justinsb justinsb Jun 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I like to leave the assignment because it documents that the struct should implement the interface (i.e. it is for reading). That could reveal my language history though ;-)

Agree that it can be pulled out into a separate file - did that, and added some comments, including on the split between CAStore and KeyStore.

johngmyers
johngmyers approved these changes Jun 5, 2020
View reviewed changes
Copy link
Member

@johngmyers johngmyers left a comment

Choose a reason for hiding this comment

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

/lgtm

nodeup/pkg/model/kube_apiserver_test.go
nodeUpModelContext.KeyStore = &fakeKeyStore{T: t}
keystore := &fakeCAStore{}
keystore.T = t
nodeUpModelContext.KeyStore = keystore
Copy link
Member

Choose a reason for hiding this comment

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

I suggest creating a NewFakeCAStore(*testing.T). You can push default test data into it as well.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 5, 2020
@johngmyers
Copy link
Member

/hold
in case you want another round. Feel free to cancel.

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 5, 2020
@johngmyers
Copy link
Member

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 5, 2020
justinsb added 2 commits June 7, 2020 10:39
@justinsb
Richer mocking of keystore in nodeup tests
ce577b0
@justinsb
More nodeup golden tests
64273a6 
kube-controller-manager, kube-proxy, kube-scheduler
@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 7, 2020
@johngmyers johngmyers mentioned this pull request Jun 7, 2020
Merged
@johngmyers
Copy link
Member

/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 7, 2020
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

4 similar comments
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@hakman
Copy link
Member

hakman commented Jun 8, 2020

/test pull-kops-e2e-kubernetes-aws

@k8s-ci-robot k8s-ci-robot merged commit d6dfff2 into kubernetes:master Jun 8, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.19 milestone Jun 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johngmyers johngmyers johngmyers approved these changes

@hakman hakman Awaiting requested review from hakman

@rdrgmnzs rdrgmnzs Awaiting requested review from rdrgmnzs

Assignees

@johngmyers johngmyers

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/nodeup 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
v1.19
Development

Successfully merging this pull request may close these issues.
5 participants
@justinsb @k8s-ci-robot @hakman @johngmyers @fejta-bot