CORS-3855: Remove ARO build flag from installer #9124
Conversation
|
@rna-afk: This pull request references CORS-3493 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set. 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/valid-reference
openshift-ci
bot
added
the
do-not-merge/work-in-progress
rna-afk
force-pushed
the
remove_aro_build_flag
branch
2 times, most recently
from
67fb5b2 to
808e68c
Compare
rna-afk
force-pushed
the
remove_aro_build_flag
branch
2 times, most recently
from
6154c08 to
aba5982
Compare
rna-afk
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/retest-required |
rna-afk
force-pushed
the
remove_aro_build_flag
branch
2 times, most recently
from
f2939ae to
d322415
Compare
rna-afk
changed the title
|
@rna-afk: This pull request references CORS-3855 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set. 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. |
rna-afk
force-pushed
the
remove_aro_build_flag
branch
from
d322415 to
b59c000
Compare
rna-afk
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
rna-afk
force-pushed
the
remove_aro_build_flag
branch
from
b59c000 to
52eaa93
Compare
|
@patrickdillon I'm working on fixing this validation but there's no reason to have a client check at this part. |
rna-afk
force-pushed
the
remove_aro_build_flag
branch
from
5a709db to
b9f4fb4
Compare
rna-afk
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
rna-afk
force-pushed
the
remove_aro_build_flag
branch
2 times, most recently
from
b2a7a39 to
c85e6fc
Compare
|
/retest |
There was a problem hiding this comment.
I would like to see if we can actually remove all ARO flags--not just the build flag. So far I haven't seen any flags we could not remove...
The only check I think we were struggling with is the one we have discussed at length, about skipping validation if the resource group is not actually empty.
It looks like the current code is checking the ManagedBy field, but IIRC our conversation with @zaneb we were going to inspect tags on the resource group and skip the validation (i.e. allow non-empty resource group) if there was an ARO-specific tag (which we would need to identify) and/or a new installer specific tag.
pkg/asset/manifests/openshift.go
Outdated
| if aro && installConfig.Config.CredentialsMode != types.ManualCredentialsMode { | ||
| // config is used to created compatible secret to trigger azure cloud | ||
| // controller config merge behaviour | ||
| // https://github.com/openshift/origin/blob/90c050f5afb4c52ace82b15e126efe98fa798d88/vendor/k8s.io/legacy-cloud-providers/azure/azure_config.go#L83 | ||
| session, err := installConfig.Azure.Session() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| config := struct { | ||
| AADClientID string `json:"aadClientId" yaml:"aadClientId"` | ||
| AADClientSecret string `json:"aadClientSecret" yaml:"aadClientSecret"` | ||
| }{ | ||
| AADClientID: session.Credentials.ClientID, | ||
| AADClientSecret: session.Credentials.ClientSecret, | ||
| } |
There was a problem hiding this comment.
TIL (or remembered) ARO is generating its own secret for the cloud provider... Does this even work? These are the credentials passed to the installer, but IIUC ARO does not use a secret--they use a certificate. So I would think the value for ClientSecret would be empty???
The reason this would not fail during install is because the CCCMO is actually authenticating the cloud provider through its credentials request: https://github.com/openshift/cluster-cloud-controller-manager-operator/blob/master/manifests/0000_26_cloud-controller-manager-operator_14_credentialsrequest-azure.yaml
So I'm wondering if we can actually just delete this code?
There was a problem hiding this comment.
@hlipsig Do you know if there's any other specific reason we would need this or should we remove it?
There was a problem hiding this comment.
I don't think we should resolve this until we get a better sense of whether we can actually remove the secret. https://issues.redhat.com/browse/CORS-3883 is removing the VM identity which is how the installer used to authenticate the cloud provider, but testing has shown that it is no longer needed and the cloud provider seems to be authenticated through cred requests.
There was a problem hiding this comment.
Per slack conversations we are updating this secret for ARO clusters. I don't think we have any code to create it if it's not already present so I expect it would be breaking for us.
There was a problem hiding this comment.
I don't think we have any code to create it if it's not already present so I expect it would be breaking for us.
The CCMO creates a credentials request which it uses to authenticate the cloud provider (see link in my first comment). We have already successfully tested removing the credentials which the installer creates for standalone openshift, so it stands to reason we can do the same for ARO. I think we should test it out so that we can drop this code.
There was a problem hiding this comment.
Finally decided to YOLO it and delete this in a separate commit
|
/retest-required |
|
/retest |
rna-afk
force-pushed
the
remove_aro_build_flag
branch
from
346975e to
0385863
Compare
Removing the ARO build flag from the installer to achieve the goal of moving ARO installs closer to the installer. Ideally, these ARO specific changes to the installs/installer need to be done either in the wrapper or hive. Adding a check for the aro tag and if exists, avoid certain checks ARO does not need. Also removing pieces of code that are no longer needed like the check if ARO for resources in existing RG. Also, as of [1], ARO adds a bogus base domain resource group to the install config for installation and will not have an empty field for that so removing the check if ARO code for validating empty base domain [1] - openshift/installer-aro-wrapper@86b8bf5
Removing CCO creds from the installer. Keeping a commit to revert to in case there's trouble on ARO front.
|
/lgtm |
|
aws failure is due to a quota issue and not due to these changes in the PR. /override ci/prow/e2e-aws-ovn |
|
@rna-afk: Overrode contexts on behalf of rna-afk: ci/prow/e2e-aws-ovn 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 kubernetes-sigs/prow repository. |
|
/skip |
|
/override ci/prow/e2e-azure-ovn test has already passed before. stuck in restest hell. let's see if this gets it merged. |
|
@patrickdillon: Overrode contexts on behalf of patrickdillon: ci/prow/e2e-azure-ovn 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 kubernetes-sigs/prow repository. |
|
/override ci/prow/e2e-aws-ovn |
|
@patrickdillon: Overrode contexts on behalf of patrickdillon: ci/prow/e2e-aws-ovn 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 kubernetes-sigs/prow repository. |
|
@rna-afk: 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-sigs/prow repository. I understand the commands that are listed here. |
|
/override ci/prow/e2e-aws-ovn |
|
@patrickdillon: Overrode contexts on behalf of patrickdillon: ci/prow/e2e-aws-ovn 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 kubernetes-sigs/prow repository. |
|
[ART PR BUILD NOTIFIER] Distgit: ose-installer-terraform-providers |
|
[ART PR BUILD NOTIFIER] Distgit: ose-installer-altinfra |
|
[ART PR BUILD NOTIFIER] Distgit: ose-baremetal-installer |
|
[ART PR BUILD NOTIFIER] Distgit: ose-installer-artifacts |
Removing the ARO build flag from the installer to achieve the goal of moving ARO installs closer to the installer. Ideally, these ARO specific changes to the installs/installer need to be done either in the wrapper or hive.