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

ROX-23263: move from OSCI cluster pool to infra clusters for e2e #1940

Merged
merged 9 commits into from
Jul 10, 2024

Conversation

johannes94
Copy link
Contributor

@johannes94 johannes94 commented Jul 9, 2024

Description

This PR prepares the required changes in this repo for e2e tests to run against cluster create through stackrox infra and respective workflows in the openshift/release repository.

This PR adds:

  • Wrapper scripts to be able to properly configure, create and destroy stackrox infra cluster via the workflow
  • Changes to FM's in cluster oidc implementation, so that it is compatible with the OIDC discovery document returned by infra clusters

To test the changes, I created a PR with a rehearsal job in the openshift/release repository:
openshift/release#54055

As a follow up once this is merged, instead of having a job that runs against the branch in this PR, I will modify our current e2e ci-operator configuration to run a job like above PR but against the main branch.

Checklist (Definition of Done)

  • Unit and integration tests added
  • Added test description under Test manual
  • Documentation added if necessary (i.e. changes to dev setup, test execution, ...)
  • CI and all relevant tests are passing
  • Add the ticket number to the PR title if available, i.e. ROX-12345: ...
  • Discussed security and business related topics privately. Will move any security and business related topics that arise to private communication channel.
  • Add secret to app-interface Vault or Secrets Manager if necessary
  • RDS changes were e2e tested manually
  • Check AWS limits are reasonable for changes provisioning new resources
  • (If applicable) Changes to the dp-terraform Helm values have been reflected in the addon on integration environment

Test manual

Successful rehearsal job in openshift/release PR linked above.

# To run tests locally run:
make db/teardown db/setup db/migrate
make ocm/setup
make verify lint binary test test/integration

scripts/ci/lib.sh Show resolved Hide resolved
scripts/ci/lib.sh Outdated Show resolved Hide resolved
pkg/client/iam/config.go Outdated Show resolved Hide resolved
// is a private IP address of the pod running the oidc server. This breaks tls validation.
// This override makes sure that in those cases kubernetes.default.svc is used instead of the IP
glog.V(5).Infof("Configured issuer is: %s and jwks_uri contains IP, replacing host with internal kubernetes svc", i.IssuerURI)
jwksURI = i.overrideJwksURIForInternalCluster(jwksURL)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just thinking out loud: Maybe we need to fallback to this scenario in all cases where we fail to fetch the jwks file?

  1. IP check looks too heuristic, in theory it can be public;
  2. It might be necessary in other edge cases;
  3. We could expand this fallback to the local cluster edge-case that also looks like a workaround.

It doesn't have to be done right now, just food for thought for future refactoring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sound like it would be good to do some refactoring here. Lets discuss what you have in mind in more detail in slack and do the refactoring in a follow up PR.

local env_value="$2"

if command -v cci-export >/dev/null; then
cci-export "$env_name" "$env_value"
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we going to use cci-export in CI? Couldn't find any relevant information about it. If not, maybe we could simplify this function..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function is copied from stackrox/stackrox repo. I'm not sure if we're using it but left it there to make sure we don't accidentally break something in the images used by the predefined workflow steps.

@openshift-ci openshift-ci bot added the lgtm label Jul 9, 2024
Copy link
Contributor

openshift-ci bot commented Jul 9, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johannes94, kovayur

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

@johannes94 johannes94 merged commit a0fec88 into main Jul 10, 2024
6 checks passed
@johannes94 johannes94 deleted the osci-move-to-infra branch July 10, 2024 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants