Skip to content

Improve JWK messages - #807

Open
1000TurquoisePogs wants to merge 4 commits into
v3.x/stagingfrom
bugfix/v3/better-jwk-troubleshooting
Open

Improve JWK messages#807
1000TurquoisePogs wants to merge 4 commits into
v3.x/stagingfrom
bugfix/v3/better-jwk-troubleshooting

Conversation

@1000TurquoisePogs

Copy link
Copy Markdown
Member

This is a PR that takes a pragmatic approach about that we know what follow-up troubleshooting certain JWK error messages involve, so we should just print more about what happened and what the user can do about it.

Signed-off-by: 1000TurquoisePogs <sgrady@rocketsoftware.com>
Signed-off-by: 1000TurquoisePogs <sgrady@rocketsoftware.com>
Signed-off-by: 1000TurquoisePogs <sgrady@rocketsoftware.com>

@JoeNemo JoeNemo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approved.

Comment thread c/jwk.c
zowelog(NULL, LOG_COMP_ID_JWK, ZOWE_LOG_WARNING, "JWK doesn't contain key\n");
*statusOut = JWK_STATUS_UNRECOGNIZED_FMT_ERROR;

zowelog(NULL, LOG_COMP_ID_JWK, ZOWE_LOG_WARNING, "JWK response:\n");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Probably needs fflush(stdout); after the zowelog. Otherwise I can see message header after the text:

"possible error message"JWK response:

instead of

JWK response:
"possible error message"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

On top of that, the message might get split by some other stray message since they're not printed as a single message.

@ifakhrutdinov ifakhrutdinov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

See my recommendation (I also incorporated them in a commit, which has only been build-tested).

Comment thread c/jwk.c
Comment thread h/zssLogging.h
#define ZSS_LOG_JWK_FAILED_MSG_ID ZSS_LOG_MSG_PRFX"1605W"
#endif
#define ZSS_LOG_JWK_FAILED_MSG_TEXT "Server will not accept JWT\n"
#define ZSS_LOG_JWK_FAILED_MSG_TEXT "Server will not accept JWT\nCheck URL https://%s:%d%s for errors.\n"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I suggest we have a single messages: Server will not accept JWT\nCheck URL %s for errors.\n and format the path in a separate function. The reason for that is for a simple task (print an error message) we're spilling so much unnecessary details here and in jwkTaskMain; it's better to isolate that.

Comment thread c/jwk.c
}
if (success) {
zowelog(NULL, LOG_COMP_ID_JWK, ZOWE_LOG_INFO, ZSS_LOG_JWK_READY_MSG, settings->fallback ? "with" : "without");
} else {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd add a helper function and simplify this code:

static char *printJWKURL(const JwkSettings *settings, char *buffer, size_t bufferLen) {
  int charsPrinted;
  if (indexOf(settings->host, strlen(settings->host), ':', 0) != -1) {
    charsPrinted = snprintf(buffer, bufferLen, "https://[%s]:%d%s", settings->host, settings->port, settings->path);
  } else {
    charsPrinted = snprintf(buffer, bufferLen, "https://%s:%d%s", settings->host, settings->port, settings->path);
  }
  if (charsPrinted < 0) {
    memset(buffer, 0, bufferLen);
  }
  return buffer;
}

static int jwkTaskMain(RLETask *task) {
...
  if (success) {
    zowelog(NULL, LOG_COMP_ID_JWK, ZOWE_LOG_INFO, ZSS_LOG_JWK_READY_MSG, settings->fallback ? "with" : "without");
  } else {
    char url[256];
    zowelog(NULL, LOG_COMP_ID_JWK, ZOWE_LOG_WARNING, ZSS_LOG_JWK_FAILED_MSG, printJWKURL(settings, url, sizeof(url)));
  }
...
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This looks to make the code more complicated. Why do this?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do this?

Because it's not more complicated?

In the current implementation, you mix lower and higher level logic, and then you have to introduce additional message templates just to work around the peculiarities of IPv6; this all basically unnecessarily spreads lower level details over a larger area than needed.

So, why not just hide this in a specialised function which can be easily reviewed and verified?

Comment thread c/jwk.c
zowelog(NULL, LOG_COMP_ID_JWK, ZOWE_LOG_WARNING, "JWK doesn't contain key\n");
*statusOut = JWK_STATUS_UNRECOGNIZED_FMT_ERROR;

zowelog(NULL, LOG_COMP_ID_JWK, ZOWE_LOG_WARNING, "JWK response:\n");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

On top of that, the message might get split by some other stray message since they're not printed as a single message.

@github-actions github-actions Bot added the stale-reopen-if-needed An issue closed due to inactivity. No indication of completion or validity. label Jul 21, 2026
@JoeNemo JoeNemo self-assigned this Jul 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale-reopen-if-needed An issue closed due to inactivity. No indication of completion or validity.

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

4 participants