OCPBUGS-12890: Create bucket, signed url and use proxy info for installs #8056
Conversation
openshift-ci-robot
added
jira/severity-critical
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
@barbacbd: This pull request references Jira Issue OCPBUGS-12890, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In response to this: 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/invalid-bug
barbacbd
force-pushed
the
OCPBUGS-12890
branch
3 times, most recently
from
32e9f84
to
3f2082d
Compare
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@barbacbd: This pull request references Jira Issue OCPBUGS-12890, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
barbacbd
force-pushed
the
OCPBUGS-12890
branch
2 times, most recently
from
0b44f22
to
e9262e1
Compare
barbacbd
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
barbacbd
force-pushed
the
OCPBUGS-12890
branch
2 times, most recently
from
3fbbd76
to
05f7550
Compare
|
Depends on #8027 |
openshift-merge-robot
added
the
needs-rebase
** Create an ignition shim and use this for metadata. ** Add proxy information to the shim.
openshift-merge-robot
removed
the
needs-rebase
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
@barbacbd: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
/cc @r4f4 |
| if err := gcpbootstrap.FillBucket(ctx, bucketHandle, bootstrapIgn); err != nil { | ||
| return fmt.Errorf("failed to fill bootstrap ignition bucket: %w", err) | ||
| } |
There was a problem hiding this comment.
if we upload the bootstrap.ign file here, won't that break the custom dns, as the ignition needs to be updated with the LB data from the terraform run?
It looks like it might be possible to still handle the bootstrap ignition upload in terraform?
There was a problem hiding this comment.
This shouldn't break the custom DNS runs. The data that is stored in the bucket can be edited.
There was a problem hiding this comment.
Ok, it would be possible to upload it here and do another upload later with the updated bootstrap ignition. But this PR is still removing the terraform config where we upload the bootstrap.ign with the lb config in TF, so as it stands I think it will break.
I would try skipping this call to gcpbootstrap.FillBucket here (it seems like we can generate the shim without it) and restore the terraform resource resource "google_storage_bucket_object" "ignition" {
| if err := gcpbootstrap.FillBucket(context.Background(), bucketHandle, string(ignitionOutput)); err != nil { | ||
| return "", fmt.Errorf("failed to fill gcp bucket with updated boostrap ignition contents: %w", err) |
There was a problem hiding this comment.
I still think it's confusing to fill the GCP bucket here as this function is intended for another purpose (custom DNS). I'm a little confused how this seems to be working, because earlier in the function there is:
if !userConfiguredDNSRaw.(bool) {
return "", nil
}
which should bail out if we're not using custom DNS.
In any case, it still seems better to me to keep the uploading of ignition in Terraform and not combine it with this custom DNS flow.
There was a problem hiding this comment.
@patrickdillon The bucket is only re-filled in this case. The bucket is originally filled in tfvars with the original data. Do we still want to use terraform as shown below?
There was a problem hiding this comment.
Got it! That works well. Sorry i missed it
| resource "google_storage_bucket_object" "ignition" { | ||
| bucket = google_storage_bucket.ignition.name | ||
| name = "bootstrap.ign" | ||
| content = var.ignition_bootstrap | ||
| } |
There was a problem hiding this comment.
I think you can keep the ignition upload in terraform by not deleting this
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: patrickdillon 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 |
openshift-ci
bot
added
the
approved
openshift-merge-bot
bot
merged commit 4eb0837
into
openshift:master
|
@barbacbd: Jira Issue OCPBUGS-12890: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-12890 has been moved to the MODIFIED state. In response to this: 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@barbacbd As I mentioned in #8151 this is causing issues with the CAPG install path because the bucket is being created twice. I also realized that due to this PR we are not deleting ignition during bootstrap destroy. I should have caught this when reviewing. Looking at a 4.15 periodic it has lines like: where Terraform is destroying the ignition objects. So we need some changes to fix this, and we should consider reverting this PR. But I think we can handle this in a way that makes the changes more straightforward, and fix both problems (CAPG & bootstrap destroy) by keeping the provisioning of the storage objects within terraform. It seems that we are able to create signedURLs without the storage objects existing. This is what we do in AWS, and here's a similar example for GCP: https://cloud.google.com/storage/docs/samples/storage-generate-signed-url-v4#storage_generate_signed_url_v4-go If we confirm that works, we should be able to keep resource provisioning in terraform, and the only changes needed to satisfy these requirements are within the ignition stub--and the resource provisioning should continue to work as it did prior to this PR. |
No description provided.