Update default credential and rename method #1214

grac3gao-zz · 2020-06-04T16:48:39Z

Proposed Changes

Update default credential setup. Currently, the default setup will automatically setup the default secret if secret is empty, regardless whether KSA is there or not. If the default auth is Secret, and we specify KSA in sources, it will return error as secret and KSA can't exist as the same time.
rename method (DefaultAuthorization -> DefaultGCPAuth)

Release Note

If either `spec.serviceAccountName` or `spec.secret` is specified, then no additional credential defaulting will be applied to the Source or Channel.

knative-prow-robot · 2020-06-04T16:48:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grac3gao

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:

~~OWNERS~~ [grac3gao]

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

grac3gao-zz · 2020-06-04T16:57:10Z

/test pull-google-knative-gcp-unit-tests

Harwayne

/lgtm

I think we will eventually want users to be able to set both the KSA and the secret via the defaulting mechanism. But, it is easier to add that feature later rather than remove it. So let's do this for now and if a user asks to default both, we can add the capability then.

grac3gao-zz · 2020-06-04T18:34:23Z

@Harwayne Thank you for mentioning this! I created a issue to track that #1216

grac3gao-zz · 2020-06-04T20:16:35Z

/retest

capri-xiyue · 2020-06-04T20:50:58Z

It looks like we encountered this error kind of often
{"level":"error","ts":1591303337.6070914,"logger":"fallback","caller":"v1alpha1/pubsub_defaults.go:33","msg":"Failed to get the GCPAuthDefaults","stacktrace":"github.com/google/knative-gcp/pkg/apis/duck/v1alpha1.(*PubSubSpec).SetPubSubDefaults\n\t/home/prow/go/src/github.com/google/knative-gcp/pkg/apis/duck/v1alpha1/pubsub_defaults.go:33\ngithub.com/google/knative-gcp/pkg/apis/intevents/v1alpha1.(*PullSubscriptionSpec).SetDefaults\n\t/home/prow/go/src/github.com/google/knative-gcp/pkg/apis/intevents/v1alpha1/pullsubscription_defaults.go:54\ngithub.com/google/knative-gcp/pkg/apis/intevents/v1alpha1.(*PullSubscription).SetDefaults\n\t/home/prow/go/src/github.com/google/knative-gcp/pkg/apis/intevents/v1alpha1/pullsubscription_defaults.go:38\ngithub.com/google/knative-gcp/pkg/reconciler/testing.NewPullSubscription\n\t/home/prow/go/src/github.com/google/knative-gcp/pkg/reconciler/testing/pullsubscription.go:53\ngithub.com/google/knative-gcp/test/e2e.PullSubscriptionWithTargetTestImpl\n\t/home/prow/go/src/github.com/google/knative-gcp/test/e2e/test_pullsubscription.go:84\ngithub.com/google/knative-gcp/test/e2e.TestPullSubscriptionWithTarget\n\t/home/prow/go/src/github.com/google/knative-gcp/test/e2e/e2e_test.go:174\ntesting.tRunner\n\t/root/.gvm/gos/go1.14.2/src/testing/testing.go:991"} which cause the flakiness of e2e tests

grac3gao-zz · 2020-06-04T21:08:26Z

@capri-xiyue If you are referring to the flakiness only for this PR, Failed to get the GCPAuthDefaults is not the root cause for it. We added a new ConfigMap for GCP Auth to set credential in webhook. This Error will not affect the proper credential setup in the wi e2e test (will add it after changes in wi reconciler), as we specified a GSA, and are not relying the default webhook. Other PRs also shows Failed to get the GCPAuthDefaults, but it doesn't affect the test result.

However, it is true that the test is more flaky, I'll investigate it....

Harwayne · 2020-06-04T21:12:27Z

It looks like we encountered this error kind of often
{"level":"error","ts":1591303337.6070914,"logger":"fallback","caller":"v1alpha1/pubsub_defaults.go:33","msg":"Failed to get the GCPAuthDefaults","stacktrace":"github.com/google/knative-gcp/pkg/apis/duck/v1alpha1.(*PubSubSpec).SetPubSubDefaults\n\t/home/prow/go/src/github.com/google/knative-gcp/pkg/apis/duck/v1alpha1/pubsub_defaults.go:33\ngithub.com/google/knative-gcp/pkg/apis/intevents/v1alpha1.(*PullSubscriptionSpec).SetDefaults\n\t/home/prow/go/src/github.com/google/knative-gcp/pkg/apis/intevents/v1alpha1/pullsubscription_defaults.go:54\ngithub.com/google/knative-gcp/pkg/apis/intevents/v1alpha1.(*PullSubscription).SetDefaults\n\t/home/prow/go/src/github.com/google/knative-gcp/pkg/apis/intevents/v1alpha1/pullsubscription_defaults.go:38\ngithub.com/google/knative-gcp/pkg/reconciler/testing.NewPullSubscription\n\t/home/prow/go/src/github.com/google/knative-gcp/pkg/reconciler/testing/pullsubscription.go:53\ngithub.com/google/knative-gcp/test/e2e.PullSubscriptionWithTargetTestImpl\n\t/home/prow/go/src/github.com/google/knative-gcp/test/e2e/test_pullsubscription.go:84\ngithub.com/google/knative-gcp/test/e2e.TestPullSubscriptionWithTarget\n\t/home/prow/go/src/github.com/google/knative-gcp/test/e2e/e2e_test.go:174\ntesting.tRunner\n\t/root/.gvm/gos/go1.14.2/src/testing/testing.go:991"} which cause the flakiness of e2e tests

This is related to #1183, not this PR. #1220 should remove the error message from the test logs.

Harwayne

We should make the same changes in pkg/apis/intevents/{v1alpha1,v1beta1}/topic_defaults.go too.

grac3gao-zz · 2020-06-04T23:13:13Z

@Harwayne Thank you for pointing out this! Topic default changed, with some UTs updated.

Harwayne

/lgtm

Harwayne

/lgtm

knative-test-reporter-robot · 2020-06-05T17:46:39Z

The following jobs failed:

Test name	Triggers	Retries
pull-google-knative-gcp-wi-tests		0/3

Failed non-flaky tests preventing automatic retry of pull-google-knative-gcp-wi-tests:

github.com/google/knative-gcp/test/e2e.TestSmokeChannel
github.com/google/knative-gcp/test/e2e.TestSmokePullSubscription
github.com/google/knative-gcp/test/e2e.TestPullSubscriptionWithTarget
github.com/google/knative-gcp/test/e2e.TestSmokeCloudPubSubSource
github.com/google/knative-gcp/test/e2e.TestCloudPubSubSourceWithTarget
github.com/google/knative-gcp/test/e2e.TestSmokeCloudSchedulerSource
github.com/google/knative-gcp/test/e2e.TestCloudSchedulerSourceWithTargetTestImpl
github.com/google/knative-gcp/test/e2e.TestSmokeGCPBroker

and 4 more.

grac3gao-zz · 2020-06-05T17:48:25Z

/test pull-google-knative-gcp-wi-tests

# Conflicts: # pkg/apis/intevents/v1alpha1/topic_defaults_test.go # pkg/apis/intevents/v1beta1/topic_defaults_test.go

Harwayne

/lgtm

knative-metrics-robot · 2020-06-05T18:31:51Z

The following is the coverage report on the affected files.
Say /test pull-google-knative-gcp-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/duck/v1alpha1/pubsub_defaults.go	75.0%	100.0%	25.0
pkg/apis/duck/v1beta1/pubsub_defaults.go	75.0%	100.0%	25.0
pkg/apis/intevents/v1alpha1/topic_defaults.go	86.7%	100.0%	13.3
pkg/apis/intevents/v1beta1/topic_defaults.go	85.7%	100.0%	14.3

update default and rename

2512ffc

grac3gao-zz requested review from Harwayne and nachocano June 4, 2020 16:48

knative-prow-robot requested review from capri-xiyue and grantr June 4, 2020 16:48

knative-prow-robot added the approved label Jun 4, 2020

googlebot added the cla: yes (override cla status due to multiple authors bug) label Jun 4, 2020

knative-prow-robot added the size/M label Jun 4, 2020

grac3gao-zz removed request for grantr and capri-xiyue June 4, 2020 16:57

grac3gao-zz changed the title ~~Update default credential and rename method~~ [WIP]Update default credential and rename method Jun 4, 2020

knative-prow-robot added the do-not-merge/work-in-progress label Jun 4, 2020

Harwayne reviewed Jun 4, 2020

View reviewed changes

knative-prow-robot assigned Harwayne Jun 4, 2020

knative-prow-robot added the lgtm label Jun 4, 2020

grac3gao-zz mentioned this pull request Jun 4, 2020

Allow to set both the KSA and the secret via the defaulting mechanism #1216

Closed

add UT

8c5c36f

knative-prow-robot removed the lgtm label Jun 4, 2020

grac3gao-zz changed the title ~~[WIP]Update default credential and rename method~~ Update default credential and rename method Jun 4, 2020

knative-prow-robot removed the do-not-merge/work-in-progress label Jun 4, 2020

Harwayne reviewed Jun 4, 2020

View reviewed changes

update topic default

e0d9efc

knative-prow-robot removed the size/M label Jun 4, 2020

knative-prow-robot added the size/L label Jun 4, 2020

Harwayne reviewed Jun 4, 2020

View reviewed changes

knative-prow-robot added the lgtm label Jun 4, 2020

update UT

1ca42f3

knative-prow-robot removed the lgtm label Jun 5, 2020

Harwayne reviewed Jun 5, 2020

View reviewed changes

knative-prow-robot added the lgtm label Jun 5, 2020

knative-prow-robot added the needs-rebase label Jun 5, 2020

Merge remote-tracking branch 'upstream/master' into default

c53b549

# Conflicts: # pkg/apis/intevents/v1alpha1/topic_defaults_test.go # pkg/apis/intevents/v1beta1/topic_defaults_test.go

knative-prow-robot removed lgtm needs-rebase labels Jun 5, 2020

Harwayne reviewed Jun 5, 2020

View reviewed changes

knative-prow-robot added the lgtm label Jun 5, 2020

knative-prow-robot merged commit c68c1a4 into google:master Jun 5, 2020

grac3gao-zz deleted the default branch June 10, 2020 00:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update default credential and rename method #1214

Update default credential and rename method #1214

grac3gao-zz commented Jun 4, 2020 •

edited by Harwayne

Loading

knative-prow-robot commented Jun 4, 2020

grac3gao-zz commented Jun 4, 2020

Harwayne left a comment

grac3gao-zz commented Jun 4, 2020

grac3gao-zz commented Jun 4, 2020

capri-xiyue commented Jun 4, 2020

grac3gao-zz commented Jun 4, 2020 •

edited

Loading

Harwayne commented Jun 4, 2020

Harwayne left a comment

grac3gao-zz commented Jun 4, 2020 •

edited

Loading

Harwayne left a comment

Harwayne left a comment

knative-test-reporter-robot commented Jun 5, 2020

grac3gao-zz commented Jun 5, 2020

Harwayne left a comment

knative-metrics-robot commented Jun 5, 2020

Update default credential and rename method #1214

Update default credential and rename method #1214

Conversation

grac3gao-zz commented Jun 4, 2020 • edited by Harwayne Loading

Proposed Changes

knative-prow-robot commented Jun 4, 2020

grac3gao-zz commented Jun 4, 2020

Harwayne left a comment

Choose a reason for hiding this comment

grac3gao-zz commented Jun 4, 2020

grac3gao-zz commented Jun 4, 2020

capri-xiyue commented Jun 4, 2020

grac3gao-zz commented Jun 4, 2020 • edited Loading

Harwayne commented Jun 4, 2020

Harwayne left a comment

Choose a reason for hiding this comment

grac3gao-zz commented Jun 4, 2020 • edited Loading

Harwayne left a comment

Choose a reason for hiding this comment

Harwayne left a comment

Choose a reason for hiding this comment

knative-test-reporter-robot commented Jun 5, 2020

grac3gao-zz commented Jun 5, 2020

Harwayne left a comment

Choose a reason for hiding this comment

knative-metrics-robot commented Jun 5, 2020

grac3gao-zz commented Jun 4, 2020 •

edited by Harwayne

Loading

grac3gao-zz commented Jun 4, 2020 •

edited

Loading

grac3gao-zz commented Jun 4, 2020 •

edited

Loading