Skip to content

Use oidc during component registration#196

Merged
brosenberg42 merged 6 commits into
developfrom
feat/oidc-support
Nov 30, 2023
Merged

Use oidc during component registration#196
brosenberg42 merged 6 commits into
developfrom
feat/oidc-support

Conversation

@brosenberg42
Copy link
Copy Markdown
Member

@brosenberg42 brosenberg42 commented Sep 12, 2023

Copy link
Copy Markdown
Member

@jrobble jrobble left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @brosenberg42)


components/component_registration.py line 196 at r1 (raw file):

    def _request_auth_token(self) -> None:
        headers = create_basic_auth_header(self._wfm_user, self._wfm_password)

After I did some live testing and modified the docker compose file a few times, I think it would clearer for components to use OIDC_CLIENT_ID and OIDC_CLIENT_SECRET env. vars instead of WFM_USER and WFM_PASSWORD. They have to be configured to use OIDC_ISSUER_URI, so using OIDC_* variables for all of the OIDC values, like the WFM, keeps things consistent.


components/component_registration.py line 211 at r1 (raw file):

        self._reuse_token_until = time.time() + expires_in
        if expires_in > 60:
            self._reuse_token_until -= 60

Why subtract a minute? Just to give you more of a buffer (that is, more time to get a new token before it expires)?

A comment here would help.


components/java_executor/Dockerfile line 59 at r1 (raw file):

COPY --from=openmpf_build /build-artifacts/java-executor/*.jar $MPF_HOME/jars/

COPY component-executor.py component_registration.py /scripts/

Do you have a preference for hyphens or underscores? component-executor.py uses one and component_registration.py uses the other.

Copy link
Copy Markdown
Member

@jrobble jrobble left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @brosenberg42)

a discussion (no related file):
I sent you a message about this log line in private chat:

The request will be re-attempted in 10 seconds.

That is logged even when we don't reattempt.


Copy link
Copy Markdown
Member

@jrobble jrobble left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r2.
Reviewable status: 4 of 5 files reviewed, 2 unresolved discussions (waiting on @brosenberg42)


components/component_registration.py line 87 at r2 (raw file):

        server_message = response_content

    error_msg = f'The following error occurred while sending HTTP request to {url}: {error}'

I now see this:

RuntimeError: The following error occurred while sending HTTP request to http://workflow-manager:8080/workflow-manager/rest/components/registerUnmanaged: HTTP Error 401:
The WFM_USER and WFM_PASSWORD environment variables need to be changed.

I'm just showing the second line for reference to let you know what I did to generate this error.

Just pointing out that HTTP Error 401: ends in a colon for some strange reason, but it seems we don't have any control over that.

@brosenberg42
Copy link
Copy Markdown
Member Author

components/java_executor/Dockerfile line 59 at r1 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Do you have a preference for hyphens or underscores? component-executor.py uses one and component_registration.py uses the other.

It needs underscores to be importable

Copy link
Copy Markdown
Member Author

@brosenberg42 brosenberg42 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 4 of 5 files reviewed, 1 unresolved discussion (waiting on @jrobble)


components/component_registration.py line 196 at r1 (raw file):

Previously, jrobble (Jeff Robble) wrote…

After I did some live testing and modified the docker compose file a few times, I think it would clearer for components to use OIDC_CLIENT_ID and OIDC_CLIENT_SECRET env. vars instead of WFM_USER and WFM_PASSWORD. They have to be configured to use OIDC_ISSUER_URI, so using OIDC_* variables for all of the OIDC values, like the WFM, keeps things consistent.

Done.


components/component_registration.py line 211 at r1 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Why subtract a minute? Just to give you more of a buffer (that is, more time to get a new token before it expires)?

A comment here would help.

Done.


components/component_registration.py line 87 at r2 (raw file):

Previously, jrobble (Jeff Robble) wrote…

I now see this:

RuntimeError: The following error occurred while sending HTTP request to http://workflow-manager:8080/workflow-manager/rest/components/registerUnmanaged: HTTP Error 401:
The WFM_USER and WFM_PASSWORD environment variables need to be changed.

I'm just showing the second line for reference to let you know what I did to generate this error.

Just pointing out that HTTP Error 401: ends in a colon for some strange reason, but it seems we don't have any control over that.

It is because the server didn't include text on the status line. In some cases the server will include it.

Copy link
Copy Markdown
Member

@jrobble jrobble left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @brosenberg42)


components/component_registration.py line 87 at r2 (raw file):

Previously, brosenberg42 wrote…

It is because the server didn't include text on the status line. In some cases the server will include it.

Error handling looks good. Thanks for thinking about the various failure conditions.

@brosenberg42 brosenberg42 merged commit fe8b674 into develop Nov 30, 2023
@brosenberg42 brosenberg42 deleted the feat/oidc-support branch November 30, 2023 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants