Skip to content

Improve JWK messages#807

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

Improve JWK messages#807
1000TurquoisePogs wants to merge 4 commits intov3.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>
Copy link
Copy Markdown
Contributor

@JoeNemo JoeNemo left a comment

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.

Copy link
Copy Markdown
Contributor

@ifakhrutdinov ifakhrutdinov left a comment

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

4 participants