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.

Sign up for GitHub

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

Jump to bottom

KATA-2159: add cloud-api-adaptor images as relatedImage #313

Merged
merged 1 commit into from May 24, 2023
Merged

KATA-2159: add cloud-api-adaptor images as relatedImage #313

merged 1 commit into from May 24, 2023

Conversation

jensfr
Copy link
Contributor

@jensfr jensfr commented May 16, 2023 •
edited by openshift-ci bot

Add the cloud-api-adaptor(-webhook) image as a relatedImage to the CSV.

Fixes: rhjira#KATA-2159

- Description of the problem which is fixed/What is the use case

add cloud-api-adaptor images as related images. By adding them to config/manager/manager.yaml they will be automatically added to the generated CSV when make bundle is run.

- What I did
Add the caa and caa-webhook image to the manager.yaml file. This is related to and dependent on confidential-containers/cloud-api-adaptor#963 which changes the env variable name

- How to verify it
the generated CSV includes the env vars RELATED_IMAGE_CAA(_WEBHOOK). When built with OSBS and digest pinning is enabled it should replace the tag in the pull spec with the digest and by that enable the operator to work in disconnected clusters.

- Description for the changelog
set CAA images (caa itself and the webhook) as relatedImages in the CSV

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label May 16, 2023
@openshift-ci-robot
Copy link

openshift-ci-robot commented May 16, 2023 •
edited by openshift-ci bot

@jensfr: This pull request references KATA-2159 which is a valid jira issue.

In response to this:

Add the cloud-api-adaptor(-webhook) image as a relatedImage to the CSV.

Fixes: rhjira#KATA-2159

- Description of the problem which is fixed/What is the use case

add cloud-api-adaptor images as related images. By adding them to config/manager/manager.yaml they will be automatically added to the generated CSV when make bundle is run.

- What I did
Add the caa and caa-webhook image to the manager.yaml file. This is related to and dependent on confidential-containers/cloud-api-adaptor#963 which changes the env variable name

- How to verify it
the generated CSV includes the env vars RELATED_IMAGE_CAA(_WEBHOOK). When built with OSBS and digest pinning is enabled it should replace the tag in the pull spec with the digest and by that enable the operator to work in disconnected clusters.

- Description for the changelog
set CAA images (caa itself and the webhook) as relatedImages in the CSV

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/test-infra repository.

1 similar comment
@openshift-ci-robot
Copy link

openshift-ci-robot commented May 16, 2023 •
edited by openshift-ci bot

@jensfr: This pull request references KATA-2159 which is a valid jira issue.

In response to this:

Add the cloud-api-adaptor(-webhook) image as a relatedImage to the CSV.

Fixes: rhjira#KATA-2159

- Description of the problem which is fixed/What is the use case

add cloud-api-adaptor images as related images. By adding them to config/manager/manager.yaml they will be automatically added to the generated CSV when make bundle is run.

- What I did
Add the caa and caa-webhook image to the manager.yaml file. This is related to and dependent on confidential-containers/cloud-api-adaptor#963 which changes the env variable name

- How to verify it
the generated CSV includes the env vars RELATED_IMAGE_CAA(_WEBHOOK). When built with OSBS and digest pinning is enabled it should replace the tag in the pull spec with the digest and by that enable the operator to work in disconnected clusters.

- Description for the changelog
set CAA images (caa itself and the webhook) as relatedImages in the CSV

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/test-infra repository.

bpradipt
bpradipt reviewed May 17, 2023
View reviewed changes
Copy link
Contributor

@bpradipt bpradipt left a comment

Choose a reason for hiding this comment

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

Need bumping up the peerpodconfig-ctrl deps ?

@jensfr jensfr changed the title (draft) KATA-2159: add cloud-api-adaptor images as relatedImage KATA-2159: add cloud-api-adaptor images as relatedImage May 17, 2023
@jensfr
Copy link
Contributor Author

jensfr commented May 17, 2023

Need bumping up the peerpodconfig-ctrl deps ?

Any idea why I get
go get github.com/confidential-containers/cloud-api-adaptor/peerpodconfig-ctrl@v0.0.0-20230517153549-cb0274212c72 go: downloading github.com/confidential-containers/cloud-api-adaptor v0.0.0-20230517153549-cb0274212c72 go: upgraded github.com/confidential-containers/cloud-api-adaptor/peerpodconfig-ctrl v0.0.0-20230512144533-a9941bba4692 => v0.0.0-20230517153549-cb0274212c72 go: downgraded k8s.io/kubernetes v1.26.0 => v1.15.0-alpha.0

when I try this?

@jensfr
Copy link
Contributor Author

jensfr commented May 17, 2023

updated the PR to also change KATA_MONITOR_IMAGE to RELATED_IMAGE_KATA_MONITOR

gkurz
gkurz reviewed May 18, 2023
View reviewed changes
config/manager/manager.yaml Outdated
- name: RELATED_IMAGE_CAA
value: registry.redhat.io/openshift-sandboxed-containers/osc-rhel9-cloud-api-adaptor
- name: RELATED_IMAGE_CAA_WEBHOOK
value: registry.redhat.io/openshift-sandboxed-containers/osc-rhel9-cloud-api-adaptor-webhook
Copy link
Member

@gkurz gkurz May 18, 2023 •
edited

Choose a reason for hiding this comment

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

It is a bit unclear how these two are consumed. I cannot find users for them in the repo.

Also, are the default values appropriate for upstream ? I would have expected them to point to quay :

e.g. quay.io/confidential-containers/cloud-api-adaptor as in https://github.com/confidential-containers/cloud-api-adaptor/blob/staging/peerpodconfig-ctrl/config/manager/manager.yaml#L65

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are right, updated those values to the public images on quay

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RELATED_IMAGE_CAA is used by cloud-api-adaptor, see https://github.com/confidential-containers/cloud-api-adaptor/pull/963/files which we import.

I removed RELATED_IMAGE_CAA_WEBHOOK now as it will be merged with the KATA-2172 PR

cpmeadors
cpmeadors approved these changes May 23, 2023
View reviewed changes
Copy link
Contributor

@cpmeadors cpmeadors left a comment

Choose a reason for hiding this comment

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

Looks good. Will have to merge midstream and create a build to see if it actually works.

@jensfr jensfr force-pushed the KATA-2159 branch 3 times, most recently from bb347ac to cbf369d Compare May 23, 2023 15:13
@jensfr
Copy link
Contributor Author

jensfr commented May 23, 2023

Need bumping up the peerpodconfig-ctrl deps ?

Any idea why I get go get github.com/confidential-containers/cloud-api-adaptor/peerpodconfig-ctrl@v0.0.0-20230517153549-cb0274212c72 go: downloading github.com/confidential-containers/cloud-api-adaptor v0.0.0-20230517153549-cb0274212c72 go: upgraded github.com/confidential-containers/cloud-api-adaptor/peerpodconfig-ctrl v0.0.0-20230512144533-a9941bba4692 => v0.0.0-20230517153549-cb0274212c72 go: downgraded k8s.io/kubernetes v1.26.0 => v1.15.0-alpha.0

when I try this?

I still have a problem with this and can't seem to fix it. Does anyone get the same when they do go get -u github.com/confidential-containers/cloud-api-adaptor/peerpodconfig-ctrl

@jensfr
KATA-2159: add cloud-api-adaptor images as relatedImage
38f4cce 
Add the cloud-api-adaptor(-webhook) image as a relatedImage to
the CSV.

Fixes: rhjira#KATA-2159

Signed-off-by: Jens Freimann <jfreimann@redhat.com>
@cpmeadors
Copy link
Contributor

I was just rereading the OSBS docs and in https://osbs.readthedocs.io/en/latest/users.html?highlight=related_image#creating-the-relatedimages-section it indicates we need a spec.relatedImages section (not exactly sure which file) that would specific the image to replace for "kata_monitor" name as referred to by RELATED_IMAGE_KATA_MONITOR. Is this an alternative solution or a necessary step to use related images?

@cpmeadors cpmeadors self-requested a review May 23, 2023 17:57
@jensfr
Copy link
Contributor Author

jensfr commented May 24, 2023

I was just rereading the OSBS docs and in https://osbs.readthedocs.io/en/latest/users.html?highlight=related_image#creating-the-relatedimages-section it indicates we need a spec.relatedImages section (not exactly sure which file) that would specific the image to replace for "kata_monitor" name as referred to by RELATED_IMAGE_KATA_MONITOR. Is this an alternative solution or a necessary step to use related images?

You're right it does require a spec.relatedImages section. It is added to the CSV. By adding the env variable to manager yaml, when I run 'make manifests' it adds the section to bundle/manifests/sandboxedcontainersoperator.csv

relatedImages:
   - image: quay.io/openshift_sandboxed_containers/openshift-sandboxed-containers-monitor:latest
     name: kata-monitor
   - image: quay.io/confidential-containers/cloud-api-adaptor
     name: caa
>

bpradipt
bpradipt approved these changes May 24, 2023
View reviewed changes
Copy link
Contributor

@bpradipt bpradipt left a comment

Choose a reason for hiding this comment

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

/lgtm
Thanks @jensfr.

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 24, 2023
@jensfr jensfr merged commit de66314 into openshift:peer-pods-tech-preview May 24, 2023
@jensfr jensfr mentioned this pull request May 24, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gkurz gkurz gkurz left review comments

@bpradipt bpradipt bpradipt approved these changes

@cpmeadors cpmeadors Awaiting requested review from cpmeadors

Assignees

@bpradipt bpradipt

Labels
jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@jensfr @openshift-ci-robot @cpmeadors @gkurz @bpradipt