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

Add Optional Restricted Network Check #858

Merged

Conversation

komish
Copy link
Contributor

@komish komish commented Dec 16, 2022

This PR is built off of the lib-->main rebase because that rebase has moved a majority of the library code into an internal package. This PR does the same for new code to avoid rework. If you want to review this before that merge happens, just click on the last commit of this commit stack, which should show you only the changes actually associated with this PR (e.g. b05a562 but this will change as I fix bugs)


This PR adds a check that attempts to review a CSV for hints that the developer has implemented guidelines to support clusters that have restricted networks. This boils down to:

  • Does the operator have related images defined in their CSV spec. There's already a related image check, so we do not replicate that work, but instead just do a soft check to make sure there are values in that section.
  • Does this operator have the "disconnected" infrastructure-feature annotation.
  • Does this operator include environment variables in any of its deployment containers that are prefixed with RELATED_IMAGE_, indicating that the developer intends to pass through (to the controller) what images it should use for its managed workloads.

There's no technical way to validate that the third bullet results in a workload that respects this value. With that being said, OperatorSDK and operator-manifest-tools will automatically pin tagged images to their digest values if they're prefixed with this string, and so it makes sense to at least look for them to try to increase the confidence that things will work as expected.

This check is marked as optional. It may fail, but that failure does not block overall results, or reflect as a failed check.


Note that a majority of this check's logic was thrown into a library (internal/csv) just to make it easier to test.

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 16, 2022
@komish
Copy link
Contributor Author

komish commented Dec 16, 2022

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 16, 2022
@acornett21
Copy link
Contributor

acornett21 commented Dec 16, 2022

My comments ended up on the commit and not this PR. :(

Summary:

  • A few nonblocking nits.
  • My main issue is that how validate has to be implemented to conform to our validate style (and go's coding style), we are going to be catering to the 1% of operators that pass through validate completely. Then the 99% of operator owners are going to see a bunch of warnings in their logs that are completely confusing. I feel this is going to lead to a far number of support cases.

@komish
Copy link
Contributor Author

komish commented Dec 16, 2022

357e60c addresses golangci-lint complaints

@coveralls
Copy link

coveralls commented Dec 16, 2022

Coverage Status

coverage: 74.378% (-7.4%) from 81.77%
when pulling 9dd58dd on komish:add-restr-net-check
into 8d6f8de on redhat-openshift-ecosystem:main.

@komish
Copy link
Contributor Author

komish commented Dec 16, 2022

The 2.5% coverage decrease is also coming from the original commits related to the rebase - not this PR.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 5, 2023
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 6, 2023
@komish komish removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 6, 2023
@komish
Copy link
Contributor Author

komish commented Jan 6, 2023

Removing the hold on this as lib-->main has now merged, and this should no longer reflect those changes in this PR's commit history. Just the relevant changes.

Copy link
Contributor

@bcrochet bcrochet left a comment

Choose a reason for hiding this comment

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

A nit, a question, and a bug.

internal/csv/csv.go Show resolved Hide resolved
internal/policy/operator/restricted_network_aware.go Outdated Show resolved Hide resolved
internal/policy/operator/restricted_network_aware.go Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 13, 2023
Copy link
Contributor

@bcrochet bcrochet left a comment

Choose a reason for hiding this comment

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

Just a small nit. I'll drop an approve in case you want to ignore it.

internal/csv/csv.go Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 17, 2023
@acornett21
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 17, 2023
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 17, 2023
…eadiness guidelines have been applied

Signed-off-by: Jose R. Gonzalez <jose@flutes.dev>
Copy link
Contributor

@bcrochet bcrochet 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 openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 18, 2023
@openshift-ci
Copy link

openshift-ci bot commented Jan 18, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: acornett21, bcrochet, komish

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 [acornett21,bcrochet,komish]

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

@bcrochet bcrochet merged commit 9e2b88b into redhat-openshift-ecosystem:main Jan 18, 2023
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants