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

Populate the storage version hash #72942

Open
wants to merge 10 commits into
base: master
from

Conversation

@caesarxuchao
Copy link
Member

caesarxuchao commented Jan 15, 2019

Implementing https://github.com/kubernetes/enhancements/blob/master/keps/sig-api-machinery/35-storage-version-hash.md.

Populating the storage version hash added in #73191, for built-in APIs, custom resources. For a custom apiserver built with the generic apiserver library, to serve the storage version hash, it needs to set k8s.io/apiserver/pkg/server/options#RecommendedOptions.Etcd.StorageConfig.DiscoverableStorageVersion.

/sig api-machinery
/kind feature
/release-note-none

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 15, 2019

@caesarxuchao: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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.

@caesarxuchao caesarxuchao force-pushed the caesarxuchao:expose-storage-version-hash branch 2 times, most recently from 06589df to 1e6e8cb Jan 20, 2019

authorizer authorizer.Authorizer

ruleResolver rbacregistryvalidation.AuthorizationRuleResolver
}

func NewStorage(s rest.StandardStorage, authorizer authorizer.Authorizer, ruleResolver rbacregistryvalidation.AuthorizationRuleResolver) *Storage {
return &Storage{s, authorizer, ruleResolver}
func NewStorage(s rest.StandardStorage, svp rest.StorageVersionProvider, authorizer authorizer.Authorizer, ruleResolver rbacregistryvalidation.AuthorizationRuleResolver) *Storage {

This comment has been minimized.

@caesarxuchao

caesarxuchao Jan 22, 2019

Author Member

I could have simply added the StorageVersionProvider method to the StandardStorage interface, but I'm concerned about changing a published interface.

This comment has been minimized.

@lavalamp

lavalamp Feb 21, 2019

Member

You can test whether s implements the provider interface rather than make the callers all change, no?

This comment has been minimized.

@lavalamp

lavalamp Feb 21, 2019

Member

plus I'd rather have a nil svp here than an svp that panics if we call the Version() method :)

@caesarxuchao caesarxuchao force-pushed the caesarxuchao:expose-storage-version-hash branch from 1e6e8cb to 6515572 Jan 22, 2019

// TODO: cleanup?
})

It("[Feature: StorageVersionHash] Custom resource should have storage version hash", func() {

This comment has been minimized.

@caesarxuchao

caesarxuchao Jan 22, 2019

Author Member

I sent kubernetes/test-infra#10881 to run this test in the alpha feature test suite.

@caesarxuchao caesarxuchao force-pushed the caesarxuchao:expose-storage-version-hash branch 2 times, most recently from 7fd4066 to c6b498c Jan 22, 2019

@caesarxuchao caesarxuchao force-pushed the caesarxuchao:expose-storage-version-hash branch from c6b498c to 8ce9257 Jan 22, 2019

@k8s-ci-robot k8s-ci-robot removed the size/XL label Jan 22, 2019

@caesarxuchao

This comment has been minimized.

Copy link
Member Author

caesarxuchao commented Feb 13, 2019

@liggitt PTAL, thanks.

@deads2k @lavalamp you might want to take a look as well.

@liggitt liggitt added this to the v1.14 milestone Feb 14, 2019

func (s *serviceStorage) StorageVersion() schema.GroupVersion {
panic("not implemented")
}

This comment has been minimized.

@yliaog

yliaog Feb 14, 2019

Contributor

what about a test case for service? it is different from other resources.

This comment has been minimized.

@caesarxuchao

caesarxuchao Feb 15, 2019

Author Member

There are a few resources that do not use the vanilla generic registry. The TestStorageVersionHashExist verifies that they have hashes. I don't want to check their values, that makes the test too brittle.

This comment has been minimized.

@lavalamp

lavalamp Feb 21, 2019

Member

What happens if we omit this function? Why should this type implement interfaces that will make it panic? (I see you're not making the problem worse but still...)

Show resolved Hide resolved test/e2e/apimachinery/discovery.go Outdated
Show resolved Hide resolved test/e2e/apimachinery/discovery.go Outdated
Show resolved Hide resolved test/e2e/apimachinery/discovery.go Outdated
var _ = SIGDescribe("Discovery", func() {
var context *certContext
_ = context
f := framework.NewDefaultFramework("discovery")

This comment has been minimized.

@yliaog

yliaog Feb 14, 2019

Contributor

'discovery' is more generic, is it better to make it more specific? it is just testing the CRD storage version hash, right?

This comment has been minimized.

@caesarxuchao

caesarxuchao Feb 14, 2019

Author Member

I intentionally made the file name and the framework name more general. I expected to add more tests to cover other aspects of the discovery doc, e.g., ResourceID.

Show resolved Hide resolved test/e2e/apimachinery/aggregator.go Outdated
Show resolved Hide resolved test/e2e/apimachinery/aggregator.go Outdated
It("Should be able to support the 1.10 Sample API Server using the current Aggregator", func() {
// Make sure the relevant provider supports Aggregator
// We have to put the version skew test and the storage version hash
// test in the same "It" block, because

This comment has been minimized.

@yliaog

yliaog Feb 14, 2019

Contributor

i don't follow, it looks to me that two tests are independent.

This comment has been minimized.

@caesarxuchao

caesarxuchao Feb 14, 2019

Author Member

Reworded.

Show resolved Hide resolved test/e2e/apimachinery/aggregator.go Outdated

@caesarxuchao caesarxuchao force-pushed the caesarxuchao:expose-storage-version-hash branch from 10ab7db to 8188e58 Feb 14, 2019

@yliaog

This comment has been minimized.

Copy link
Contributor

yliaog commented Feb 15, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 15, 2019

@yliaog

This comment has been minimized.

Copy link
Contributor

yliaog commented Feb 15, 2019

better to squash the commits before merge

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 20, 2019

New changes are detected. LGTM label has been removed.

@k8s-ci-robot k8s-ci-robot added needs-rebase and removed lgtm labels Feb 20, 2019

caesarxuchao added some commits Jan 15, 2019

generated BUILD
generated proto

@caesarxuchao caesarxuchao force-pushed the caesarxuchao:expose-storage-version-hash branch from 3602b4c to 31d0888 Feb 20, 2019

@caesarxuchao

This comment has been minimized.

Copy link
Member Author

caesarxuchao commented Feb 20, 2019

Rebased. PTAL. Thanks.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 20, 2019

@caesarxuchao: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-kops-aws c43479f link /test pull-kubernetes-e2e-kops-aws
pull-kubernetes-e2e-gce 31d0888 link /test pull-kubernetes-e2e-gce

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

@liggitt liggitt added this to In progress in API Reviews Feb 21, 2019

@liggitt liggitt added the api-review label Feb 21, 2019


server := httptest.NewServer(master.GenericAPIServer.Handler.GoRestfulContainer.ServeMux)

// Test 1: extensions/v1beta1/replicasets and apps/v1/replicasets have

This comment has been minimized.

@lavalamp

lavalamp Feb 21, 2019

Member

This test seems valuable but not future proof-- maybe the prior test could be changed to make a map[hash][]GVK and then verify that every hash has just 1 entry except for these specific ones?

This comment has been minimized.

@lavalamp

lavalamp Feb 21, 2019

Member

It also seems important that these hashes don't change except when we change something about the storage; perhaps it would be good to check in a golden data file (somewhere owned by api reviewers) so that it's difficult to accidentally change this?

func (s *serviceStorage) StorageVersion() schema.GroupVersion {
panic("not implemented")
}

This comment has been minimized.

@lavalamp

lavalamp Feb 21, 2019

Member

What happens if we omit this function? Why should this type implement interfaces that will make it panic? (I see you're not making the problem worse but still...)

authorizer authorizer.Authorizer

ruleResolver rbacregistryvalidation.AuthorizationRuleResolver
}

func NewStorage(s rest.StandardStorage, authorizer authorizer.Authorizer, ruleResolver rbacregistryvalidation.AuthorizationRuleResolver) *Storage {
return &Storage{s, authorizer, ruleResolver}
func NewStorage(s rest.StandardStorage, svp rest.StorageVersionProvider, authorizer authorizer.Authorizer, ruleResolver rbacregistryvalidation.AuthorizationRuleResolver) *Storage {

This comment has been minimized.

@lavalamp

lavalamp Feb 21, 2019

Member

You can test whether s implements the provider interface rather than make the callers all change, no?

authorizer authorizer.Authorizer

ruleResolver rbacregistryvalidation.AuthorizationRuleResolver
}

func NewStorage(s rest.StandardStorage, authorizer authorizer.Authorizer, ruleResolver rbacregistryvalidation.AuthorizationRuleResolver) *Storage {
return &Storage{s, authorizer, ruleResolver}
func NewStorage(s rest.StandardStorage, svp rest.StorageVersionProvider, authorizer authorizer.Authorizer, ruleResolver rbacregistryvalidation.AuthorizationRuleResolver) *Storage {

This comment has been minimized.

@lavalamp

lavalamp Feb 21, 2019

Member

plus I'd rather have a nil svp here than an svp that panics if we call the Version() method :)


// A basic test if the sample-apiserver code from 1.10 and compiled against 1.10
// will work on the current Aggregator/API-Server.

This comment has been minimized.

@lavalamp

lavalamp Feb 21, 2019

Member

This is now older than we promise to support, isn't it? I guess we can fix that in another PR.

}
found := false
// The calculation of the hash value is for test purpose only. Clients
// shouldn't rely on the exact hash value, which is an implementation

This comment has been minimized.

@lavalamp

lavalamp Feb 21, 2019

Member

But they should rely on it only changing at particular times, no?

// shouldn't rely on the exact hash value, which is an implementation
// detail.
b := sha256.Sum256([]byte("wardle.k8s.io/v1alpha1"))
expected := base64.StdEncoding.EncodeToString(b[:])

This comment has been minimized.

@lavalamp

lavalamp Feb 21, 2019

Member

I think it'd be better to just hard code the value this computes.

// shouldn't rely on the exact hash value, which is an implementation
// detail.
b := sha256.Sum256([]byte(storageVersion))
expected := base64.StdEncoding.EncodeToString(b[:])

This comment has been minimized.

@lavalamp

lavalamp Feb 21, 2019

Member

here too, then you don't need the disclaimer, too... (or at least you need a different one)

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