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

OCPBUGS-30162: Introduce --issuer-url flag in oc login #1694

Merged
merged 2 commits into from Mar 5, 2024
Merged

OCPBUGS-30162: Introduce --issuer-url flag in oc login #1694

merged 2 commits into from Mar 5, 2024

Conversation

ardaguclu
Copy link
Member

Normally, oc login extracts the issuer url from API server but in cases where multiple oidc providers exist, user may want to explicitly specify issuer url to authenticate. That's why, this PR introduces --issuer-url flag in oc login to be passed to oc get-token oidc cred exec plugin of oc.

@openshift-ci-robot openshift-ci-robot added jira/severity-moderate Referenced Jira bug's severity is moderate for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. labels Mar 4, 2024
@openshift-ci-robot
Copy link

@ardaguclu: This pull request references Jira Issue OCPBUGS-30162, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (4.16.0) matches configured target version for branch (4.16.0)
  • bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @xingxingxia

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Normally, oc login extracts the issuer url from API server but in cases where multiple oidc providers exist, user may want to explicitly specify issuer url to authenticate. That's why, this PR introduces --issuer-url flag in oc login to be passed to oc get-token oidc cred exec plugin of oc.

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.

@ardaguclu
Copy link
Member Author

/cc @stlaz

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 4, 2024
@ardaguclu
Copy link
Member Author

/cherry-pick release-4.15

@openshift-cherrypick-robot
Copy link

@ardaguclu: once the present PR merges, I will cherry-pick it on top of release-4.15 in a new PR and assign it to you.

In response to this:

/cherry-pick release-4.15

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.

@ardaguclu
Copy link
Member Author

Failures are not related;
/retest

@ardaguclu
Copy link
Member Author

/retest

stlaz
stlaz suggested changes Mar 4, 2024
View reviewed changes
pkg/cli/login/login.go Outdated Show resolved Hide resolved
pkg/cli/login/login.go Outdated Show resolved Hide resolved
pkg/cli/login/loginoptions.go Outdated
if err != nil {
return nil, err
var err error
if o.OIDCIssuerURL == "" {
Copy link
Member

Choose a reason for hiding this comment

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

IMO this must always be set

Copy link
Member Author

@ardaguclu ardaguclu Mar 4, 2024 •
edited

Choose a reason for hiding this comment

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

If this is always be set, why/what are we extracting from oauth-authorization-server endpoint?

Copy link
Member Author

Choose a reason for hiding this comment

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

From the oc point of view, this doesn't matter too much and if in the future, it is decided that users always have to set issuer-url, it is easy to add a validation. I think, we can move on as is.

Copy link
Member

@stlaz stlaz Mar 4, 2024 •
edited

Choose a reason for hiding this comment

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

If this is always be set, why/what are we extracting from oauth-authorization-server endpoint?

ideally we shouldn't extract anything from there

@ardaguclu
Introduce --issuer-url flag in oc login
854344d 
Normally, oc login extracts the issuer url from API server but in cases
where multiple oidc providers exist, user may want to explicitly specify
issuer url to authenticate. That's why, this PR introduces `--issuer-url`
flag in oc login to be passed to `oc get-token` oidc cred exec plugin of oc.
@stlaz
Copy link
Member

stlaz commented Mar 4, 2024

/hold
until we're clear on the path we're taking
/lgtm

@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 Mar 4, 2024
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 4, 2024
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 4, 2024
@stlaz
Copy link
Member

stlaz commented Mar 4, 2024

/lgtm

@ardaguclu
Copy link
Member Author

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 4, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Mar 4, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ardaguclu, stlaz

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-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 4, 2024
@stlaz
Copy link
Member

stlaz commented Mar 4, 2024

/cherry-pick release-4.15

@openshift-cherrypick-robot
Copy link

@stlaz: once the present PR merges, I will cherry-pick it on top of release-4.15 in a new PR and assign it to you.

In response to this:

/cherry-pick release-4.15

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.

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 91ba77a and 2 for PR HEAD 68e0597 in total

@ardaguclu
Copy link
Member Author

/retest

@xingxingxia
Copy link
Contributor

Failed e2e jobs were due to one MCO bug being fixed by openshift/os#1455 .
Given https://github.com/openshift/os/pull/1455 is emerged hours ago, let me issue retest:
/retest

@ardaguclu
Copy link
Member Author

/retest

3 similar comments
@ardaguclu
Copy link
Member Author

/retest

@ardaguclu
Copy link
Member Author

/retest

@ardaguclu
Copy link
Member Author

/retest

@ardaguclu
Copy link
Member Author

I don't think there is anything we can do apart from brute-force retesting;
/retest

@xingxingxia
Copy link
Contributor

I have marked https://issues.redhat.com/browse/OCPBUGS-30149 as Release Blocker to reminder related owners resolve (including verify) it faster to unblock our PR tests.

@xingxingxia
Copy link
Contributor

I manage to launch cluster successfully with latest build 4.16.0-0.nightly-2024-03-05-105513 without https://issues.redhat.com/browse/OCPBUGS-30149 error now Cluster operator machine-config Degraded is True with RequiredPoolsFailed: Unable to apply 4.16.0-0.nightly-2024-03-04-155853: error during syncRequiredMachineConfigPools: [context deadline exceeded, failed to update clusteroperator: [client rate limiter Wait returned an error: context deadline exceeded, error MachineConfigPool master is not ready, retrying. Status: (pool degraded: true total: 3, ready 0, updated: 0, unavailable: 3)]]. Congrats! Retest again:

/retest

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Mar 5, 2024

@ardaguclu: all tests passed!

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.

@openshift-merge-bot openshift-merge-bot bot merged commit 92ede80 into openshift:master Mar 5, 2024
13 checks passed
@openshift-ci-robot
Copy link

@ardaguclu: Jira Issue OCPBUGS-30162: All pull requests linked via external trackers have merged:

Jira Issue OCPBUGS-30162 has been moved to the MODIFIED state.

In response to this:

Normally, oc login extracts the issuer url from API server but in cases where multiple oidc providers exist, user may want to explicitly specify issuer url to authenticate. That's why, this PR introduces --issuer-url flag in oc login to be passed to oc get-token oidc cred exec plugin of oc.

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-cherrypick-robot
Copy link

@ardaguclu: new pull request created: #1696

In response to this:

/cherry-pick release-4.15

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.

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Mar 5, 2024
Merged
@ardaguclu ardaguclu deleted the ocpbugs-30162 branch March 5, 2024 17:20
@openshift-bot
Copy link
Contributor

[ART PR BUILD NOTIFIER]

This PR has been included in build openshift-enterprise-cli-container-v4.16.0-202403052341.p0.g92ede80.assembly.stream.el8 for distgit openshift-enterprise-cli.
All builds following this will include this PR.

@ardaguclu ardaguclu mentioned this pull request Mar 14, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stlaz stlaz stlaz requested changes

@xingxingxia xingxingxia Awaiting requested review from xingxingxia

@deads2k deads2k Awaiting requested review from deads2k

@soltysh soltysh Awaiting requested review from soltysh

Assignees

@stlaz stlaz

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/severity-moderate Referenced Jira bug's severity is moderate for the branch this PR is targeting. jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. 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
6 participants
@ardaguclu @openshift-ci-robot @openshift-cherrypick-robot @stlaz @xingxingxia @openshift-bot