-
Notifications
You must be signed in to change notification settings - Fork 472
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
feat: add useragent to conformance test run #3211
feat: add useragent to conformance test run #3211
Conversation
8820474
to
21f890c
Compare
It would be really helpful if once this is merged, it can be backported to v1.0 and v1.1 test suites for historic data generation. |
5bc013e
to
b8afde8
Compare
testSuiteUserAgentPrefix, | ||
suite.apiVersion, | ||
test.ShortName, | ||
strings.Join(featureNames, ","), |
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.
Seems like this list could get pretty long, is it okay to have all the feature names present in every useragent string? How long can a useragent string be?
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.
@youngnick, good question!
Looking at the latest Kubernetes test results, filtering by conformance tests:
$ curl -sSL https://storage.googleapis.com/kubernetes-jenkins/logs/ci-kubernetes-e2e-gci-gce/1810176656531263488/artifacts/junit_01.xml \
| yq -p=xml -o=yaml \
| yq '.testsuites.testsuite.testcase[] | select(.+@name == "*[Conformance]*") | .+@name' \
| sed 's/.*\]\(.*\)\[.*/\1/g' \
| awk '{ print length($0) " " $0; }' | sort -r -n | cut -d ' ' -f 2- | head -n 1 | wc -c
The longest useragent is 131 characters.
The current longest useragent with the changes from this PR is
apisnoop=# select distinct(useragent) as ua, length(useragent) as ual from testing.audit_event where useragent ilike 'gateway-api-conformance.test%' order by ual desc limit 1;
ua |
ual
-----------------------------------------------------------------------------------------------------------------------------------------------------+
-----
gateway-api-conformance.test::v1.2.0-dev::HTTPRouteRequestMultipleMirrors::Gateway,HTTPRoute,HTTPRouteRequestMirror,HTTPRouteRequestMultipleMirrors |
147
(1 row)
is 147.
Kubernetes Audit Logging appears to only store 1024 characters for useragents
https://github.com/kubernetes/kubernetes/blob/2b03f04/staging/src/k8s.io/apiserver/pkg/audit/request.go#L39
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.
Thanks for the detailed analysis @BobyMCbobs! If I'm understanding this correctly, it seems like this would be easy to change in the future if/when we run into an issue, and the only limit that really matters is k8s API Server. Given that, I think I'm ok with the current path. In the future we could add a numerical id for each feature 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.
@robscott, absolutely!
Yes and the main thing is that we are able to map tests to features. Using this as a framework, we can pump any extra data through as usefulness and need arises.
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.
In the future we could add a numerical id for each feature if needed.
I think this is a great idea, instead of putting all the features' names here.
Thanks @BobyMCbobs! Will defer LGTM to someone else. Unclear if we'll ever get to a v1.1.1 release, but this should at least create a cherry pick PR. /cherry-pick release-1.1 |
@robscott: once the present PR merges, I will cherry-pick it on top of release-1.1 in a new PR and assign it to you. In response to this:
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-sigs/prow repository. |
LGTM but I think that one of the conformance reviewers should check this before we merge. @mlavacca @LiorLieberman @sunjayBhatia maybe? |
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.
LGTM in that this does not affect tests running as normal
(I did not verify the user agent was set in the api server audit logs myself however)
One question i have is if we would be missing any api server calls (or if it matters) since where setClientsetForTest
is called happens when tests are run, after the base manifests are already applied in suite Setup
:
gateway-api/conformance/utils/suite/suite.go
Lines 341 to 352 in 86c06a0
tlog.Logf(t, "Test Setup: Applying base manifests") | |
suite.Applier.MustApplyWithCleanup(t, suite.Client, suite.TimeoutConfig, suite.BaseManifests, suite.Cleanup) | |
tlog.Logf(t, "Test Setup: Applying programmatic resources") | |
secret := kubernetes.MustCreateSelfSignedCertSecret(t, "gateway-conformance-web-backend", "certificate", []string{"*"}) | |
suite.Applier.MustApplyObjectsWithCleanup(t, suite.Client, suite.TimeoutConfig, []client.Object{secret}, suite.Cleanup) | |
secret = kubernetes.MustCreateSelfSignedCertSecret(t, "gateway-conformance-infra", "tls-validity-checks-certificate", []string{"*", "*.org"}) | |
suite.Applier.MustApplyObjectsWithCleanup(t, suite.Client, suite.TimeoutConfig, []client.Object{secret}, suite.Cleanup) | |
secret = kubernetes.MustCreateSelfSignedCertSecret(t, "gateway-conformance-infra", "tls-passthrough-checks-certificate", []string{"abc.example.com"}) | |
suite.Applier.MustApplyObjectsWithCleanup(t, suite.Client, suite.TimeoutConfig, []client.Object{secret}, suite.Cleanup) | |
secret = kubernetes.MustCreateSelfSignedCertSecret(t, "gateway-conformance-app-backend", "tls-passthrough-checks-certificate", []string{"abc.example.com"}) | |
suite.Applier.MustApplyObjectsWithCleanup(t, suite.Client, suite.TimeoutConfig, []client.Object{secret}, suite.Cleanup) |
conformance/utils/suite/suite.go
Outdated
cfg, err := clicfg.GetConfig() | ||
if err != nil { | ||
return 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.
We already have the config stored in the suite. Can we use that instead of re-creating it?
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.
@BobyMCbobs, looks like this needs a rebase and has an outstanding comment, but is pretty close aside from those things. |
5c3382f
to
a7fab63
Compare
adds a per-test useragent containing gateway-api version and test features
a7fab63
to
7a5e8df
Compare
LGTM, but I think @mlavacca should be the last sign-off here. |
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.
Thanks, @BobyMCbobs!
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: BobyMCbobs, mlavacca, robscott, sunjayBhatia 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 |
@robscott: #3211 failed to apply on top of branch "release-1.1":
In response to this:
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-sigs/prow repository. |
adds a per-test useragent containing gateway-api version and test features
/kind test
/area conformance
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #3193