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.

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

test/e2e/prometheusadapter: reenable CA rotation test #576

Conversation

s-urbaniak
Copy link
Contributor

This reenables the requestheader CA rotation e2e test
by using a different method.

It deletes the CSR signer secrets causing the extension-apiserver-authentication
configmap to be reissued.

/cc @openshift/openshift-team-monitoring

@openshift-ci-robot openshift-ci-robot requested a review from a team December 3, 2019 14:20
@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 3, 2019
@s-urbaniak
Copy link
Contributor Author

/test e2e-aws-operator

@s-urbaniak
Copy link
Contributor Author

I have to look into this one 🤔

2019/12/03 14:58:05 wait for prometheus-k8s: invalid character '<' looking for beginning of value: timed out waiting for the condition
FAIL	github.com/openshift/cluster-monitoring-operator/test/e2e	61.985s

@s-urbaniak
Copy link
Contributor Author

I found at least the root cause for the above message: it is the OAuth proxy telling us we're not allowed to execute the request (403). As we don't check the response status code, we try to parse the OAuth login html which causes the above error to be generated.

This is unrelated to this PR as it happens in the framework setup but I'll try to fix it here as we're here.

Copy link
Contributor

@lilic lilic left a comment

Choose a reason for hiding this comment

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

/approve

Guessing this is good to go?

This reenables the requestheader CA rotation e2e test
by using a different method.

It deletes the CSR signer secrets causing the extension-apiserver-authentication
configmap to be reissued.
@s-urbaniak s-urbaniak force-pushed the reenable-prom-adapter-rotation-test branch from 59220c0 to c5a802d Compare December 4, 2019 14:28
@s-urbaniak
Copy link
Contributor Author

@lilic now this is good to go, PTAL. I hopefully fixed the outstanding issues with our e2e test client in new commit.

Copy link
Contributor

@lilic lilic left a comment

Choose a reason for hiding this comment

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

One comment otherwise lgtm

_, dockerToken := secret.Annotations["openshift.io/create-dockercfg-secrets"]
e2eToken := strings.Contains(secret.Name, "cluster-monitoring-operator-e2e-token-")

if !dockerToken && e2eToken {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just add a comment that if its dockerToken that means this token is an invalid one, to avoid future confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good

We have a couple of potential e2e flake sources:

1. We don't check the response code from the prometheus query response.

That causes the vanilla http response in case of error to be parsed as JSON which fails.
We should bail out with the http status code instead.

2. It happens that we sometimes don't pick the correct token secret.

With every service account there are two token secrets.
One is the internal Kubernetes service account token,
the other one is associated with the internal OpenShift Docker registry.
We have to pick the right one otherwise non-deterministically the wrong token will be chosen.

This fixes the above two issues.
@s-urbaniak s-urbaniak force-pushed the reenable-prom-adapter-rotation-test branch from c5a802d to c9e05d6 Compare December 4, 2019 17:21
@s-urbaniak
Copy link
Contributor Author

s-urbaniak commented Dec 5, 2019

@lilic ptal

Copy link
Contributor

@lilic lilic left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 5, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: LiliC, s-urbaniak

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

@lilic
Copy link
Contributor

lilic commented Dec 5, 2019

Seems like a flake
/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 9383705 into openshift:master Dec 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants