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

Update ObjectStore HealthCheck to use StatObject #265

Merged

Conversation

gmfrasca
Copy link
Member

  • Provided Credentials may not necessarily have ListBucket permissions, so update the Object Store Health Check to use StatObject instead

The issue resolved by this Pull Request:

Resolves #262

Description of your changes:

Updates DSPA ObjectStore prereq Health Check to use StatObject instead of ListBuckets. We cannot assume that the provided credentials have ListBuckets permissions granted, so updating the verification command to StatObject (which is used as part of GetObject/ReadObject and required for the underlying DataSciencePipelines functionality)

Testing instructions

Attempt to create a DSPA with externalStorage, and use credentials that only have PutObject, GetObject, and DeleteObject permissions. Ensure that the DSPA eventually passes the health check (ObjectStorageAvailable CR condition = true). Additional test cases include using default objectStorage and externally hosted, but non-AWS storage options such as minio, ceph, etc.

Checklist

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

- Provided Credentials may not necessarily have ListBucket
  permissions, so update the Object Store Health Check
  to use StatObject instead
@dsp-developers
Copy link
Contributor

A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-265

@HumairAK HumairAK added the qe/verify Labels to inform qe on issues to verify. label Aug 14, 2023
- Bucket could be created by APIServer if missing, so allow the
  ObjStore HC to pass if the errCode is NoSuchBucket
@dsp-developers
Copy link
Contributor

A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-265

@rimolive
Copy link
Contributor

/lgtm

@HumairAK
Copy link
Contributor

With odh-dashboard and bad creds I see:

    - lastTransitionTime: '2023-08-23T16:51:33Z'
      message: Could not connect to Object Store
      observedGeneration: 2
      reason: ObjectStoreAvailable
      status: 'False'
      type: ObjectStoreAvailable

With bad endpoint I see the same thing, logs show:

2023-08-23T17:04:47Z INFO Could not connect to (s3.a321mazonaws.com), Error: Get "https://s3.a321mazonaws.com/rhods-dsp-dev/?location=": dial tcp: lookup s3.a321mazonaws.com on 172.30.0.10:53: no such host {"namespace": "my-ds-project", "dspa_name": "pipelines-definition"}

/lgtm
/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 23, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: HumairAK

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

@openshift-merge-robot openshift-merge-robot merged commit 37935d8 into opendatahub-io:main Aug 23, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm qe/verify Labels to inform qe on issues to verify.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update ObjectStore health check to use less permissive action
5 participants