Drop deprecated v1 prefixed routes

[azasypkin]

Dropping previously deprecated Security related API routes.

Tasks:

• Update Elasticsearch docs/references (Switch to the most recent Kibana configuration format and SAML/OIDC endpoints. elasticsearch#50652)
• Update Cloud docs/reference (Cloud team has been notified)
• Try to incorporate notes about these breaking changes into Upgrade Assistant for 8.0 (tracked in [Upgrade Assistant] Announce Kibana Plugin Deprecations #54702, we'll leverage this functionality as soon as it's implemented)

---

"Release note: /api/security/v1/me endpoint is no longer public API and completely replaced with internal-use-only endpoint."

"Release note: /api/security/v1/logout endpoint is removed in favor of /api/security/logout. Please make sure to switch any Single sign-on providers you use with Kibana to /api/security/logout instead."

"Release note: /api/security/v1/oidc endpoint is removed in favor of /api/security/oidc/callback. Please make sure to switch Elasticsearch and any OpenID Connect providers you use with Kibana to /api/security/oidc/callback instead."

"Release note: /api/security/v1/oidc/implicit endpoint is removed in favor of /api/security/oidc/implicit. Please make sure to switch Elasticsearch and any OpenID Connect providers you use with Kibana to /api/security/oidc/implicit instead."

"Release note: /api/security/v1/oidc endpoint is removed and should no longer be used for Third-Party Initiated login. Please make sure to switch OpenID Connect providers you use with Kibana for Third-Party Initiated login to /api/security/oidc/initiate_login instead."

Blocked by: #53886

[elasticmachine]

Pinging @elastic/kibana-security (Team:Security)

[azasypkin]

Hey @elastic/es-ui! Is there any guide that explains how one can add something to the upgrade assistant for 8.0 and what type of stuff is supposed to be there?

In our case we're changing Kibana URLs that are used by the external 3rd parties (e.g. by Okta if Kibana SAML authentication is used). There is no way for us to figure out if user/admin still relies on the deprecated URLs (we only log deprecation message if such deprecated endpoint eventually receives request).

Is there any section in Upgrade Assistant where we could ask user to make sure that they use correct/new Kibana URLs in 3rd-party/ES configuration in case we detect SAML authentication is enabled?

[cjcenizal]

@azasypkin I don't believe there is a guide, but that sounds like it would be super-useful!

In terms of what you're trying to accomplish: are you only asking about where to add logic for asking the user this question, or are you also asking about where to add logic for detecting whether SAML authentication is enabled?

[azasypkin]

are you only asking about where to add logic for asking the user this question

Yes! And also whether it makes sense to display this kind of stuff in the Upgrade Assistant at all? I had an impression that we only display the things that we can automatically verify and be 100% sure if there is any action required from the user or not. And in this case we're guessing and asking user to go to the 3rd-party and double check (it's certainly better than leaving users on their own though).

or are you also asking about where to add logic for detecting whether SAML authentication is enabled?

My initial thought was to expose this information/API from New Platform Security plugin and make it as an optional dependency of Upgrade Assistant plugin, but I'm open to any other ideas.

[jloleysens]

@cjcenizal @azasypkin

My understanding of what is displayed in the Upgrade Assistant (UA) at the moment is that it's very Elasticsearch focused (@joshdover can correct me here). The only mention of Kibana in Upgrade Assistant is how it relates to ES (ES versions and ES re-indexing tasks respectively):

• x-pack/legacy/plugins/upgrade_assistant/public/np_ready/application/components/tabs.tsx
• x-pack/legacy/plugins/upgrade_assistant/public/np_ready/application/components/tabs/checkup/deprecations/reindex/flyout/checklist_step.tsx

However, I think announcing Kibana endpoint deprecations/changes like this could still be appropriate.

An alternative implementation (to what @azasypkin mentioned) is upgrade assistant exposes a function (is this possible if UA is in legacy?) to other plugins for appending PluginDeprecationNotice (fictional) objects that are rendered somewhere†. It may be over-generalising if only Security will want to use something like this, but my feeling is it will be a similar-ish amount of work if we need to pull the security plugin into UA specifically. Either way I think we would want a plugin to maintain its list of changes for a given version. Not sure what we can pull off given time constraints @cjcenizal ?

† to my mind somewhere could be a specific tab (Overview, Cluster, Indices, Plugins) or it could be a (collapsible) section under Overview. We would need to be clear that we're taking about Kibana not ES.

[azasypkin]

An alternative implementation (to what @azasypkin mentioned) is upgrade assistant exposes a function (is this possible if UA is in legacy?) to other plugins for appending PluginDeprecationNotice (fictional) objects that are rendered somewhere†.

Something like this would be ideal if it can fit into your roadmap 🙂

It may be over-generalising if only Security will want to use something like this

I don't see this as over-generalizing, I believe just few know what UA is capable of and how to actually leverage it, but with easy to use API Upgrade Assistant may get much wider adoption among the plugins that would make Kibana upgrades smoother overall.

[cjcenizal]

@jloleysens @azasypkin I'm really excited by the discussion going on here, and I do strongly believe we should surface the information Alex has mentioned within UA.

@jloleysens Would you mind creating an issue to capture requirements, and loop in Alex to make sure what we're spec'ing will meet his needs?

Here are a couple initial user requirements I can think of.

• Filtering. In the event that there are a ton of notices, users will want to be able to focus on specific notices based on plugin or urgency.
• Checklist functionality. Ability to "check-off" or otherwise dismiss a notice, so the user can work through the list of notices. There should be a way to view the list of dismissed notices and un-dismiss them. This state can be persisted in local storage.

[azasypkin]

@elasticmachine merge upstream

[kobelb]

ACK: reviewing

[kobelb]

I've been thinking about the manner in which we've been deprecating and removing functionality in major version upgrades. Particularly about the manner in which changes like these make backporting code between master and 7.x complicated, and has caused us to accidentally ship bugs like #51337.

Removing these routes as soon as possible from master is great, as it makes downstream consumers aware of the forthcoming breaking changes. However, just deleting the code in master then makes the backports complicated. I've noticed that the Elasticsearch team will sometimes leave the code behind, but guarded with a version check. For example, https://github.com/elastic/elasticsearch/pull/51122/files#diff-3687136733f7753464b916686d798768R780-R782.

@elastic/kibana-security how do you all feel about adopting this approach for deprecations? We'd likely need to work though some of the details, but it seems to strike a middle-ground of propagating the breaking changes to downstream consumers ASAP but without introducing the backporting complications?

[jportner]

how do you all feel about adopting this approach for deprecations?

I like this a lot, I've wrestled with this myself and it seems like the best of both worlds.

[azasypkin]

I completely agree with the sentiment, but I feel there are two related but separate problems here:

Regressions related to the deprecated functionality

Higher complexity of the backports may become a reason for regression for sure, but usually the main reason for regressions like that (incl. #51337) was the lack of proper tests for the deprecated functionality and not the complexity of backports by itself. Usually when something is deprecated tests are rewritten to test new and recommended setup (well, at least I still do this frequently 🙈 ) while ideally we should keep tests for the deprecated functionality and add more for the new. We can enforce that during review and make sure we have enough API integration/functional tests (not unit tests) that specifically test both deprecated and new functionality. Risk of breaking something still remains, but theoretically it should be significantly lower.

I'll go ahead and see if all affected by this PR places have enough tests for both deprecated and respective new functionality.

Complex backports

When we completely remove deprecated functionality we gradually decrease technical debt and become more efficient while working with master at the cost of more complex backports. Even though it sounds like a reasonable deal to me I agree that it's worth spending time to find and implement a better solution.

But ES approach seems to be just reversed solution comparing to what we have now, e.g. deprecated code still needs to "compile" properly even for Kibana version in which it's not supposed to be used, that can become a serious hurdle for us. By the way, I see ES folks use this kind of check only in a couple of QA related places unless I'm missing something?

I'm sorry that I don't have a better proposal yet, just wanted to brain dump and separate two issues since they can be tackled in parallel.

[kobelb]

While I do agree that proper tests can catch these regressions, this will only happen after a PR has been merged to master and the backport has been created. Best case scenario, this will require minimal changes to get the backport working with the deprecated functionality. Worst case scenario, the functionality which has been deprecated will invalidate a core tenant which made the approach possible and we potentially have to revert the merge to master and figure out an alternate approach. We've generally been able to prevent the worst case scenario from occurring because the person writing the code is generally aware of the divergence between master and the branch we'll be backporting the code. However, as the team grows it becomes harder to rely upon this as people who are new are less likely to be familiar with the code outside of master.

By the way, I see ES folks use this kind of check only in a couple of QA related places unless I'm missing something?

The examples I provided were bad... It's not always done this way, but it is sometimes. Here are some better ones:

• https://github.com/elastic/elasticsearch/pull/50594/files#diff-e908d79e9ece3c13014a36edd2b89e4fR90-R91
• https://github.com/elastic/elasticsearch/pull/48781/files#diff-c3c568ac3ad92f651177006a51068b8bR27-R30
• https://github.com/elastic/elasticsearch/pull/48781/files#diff-747014bbbd1762b6195abdc68dea7f2dR78-R83

[azasypkin]

While I do agree that proper tests can catch these regressions, this will only happen after a PR has been merged to master and the backport has been created.

Yeah, that's true. But what is the expected lifetime of these version checks? Won't we have to remove them at some point anyway?

Also do you happen to know if these checks are optimized out when compiling ES for a particular version somehow (I suspect no, since I see these checks are also used for deprecation warnings as well)?

The examples I provided were bad... It's not always done this way, but it is sometimes. Here are some better ones:

Ah, that's much better, thanks! It will take some time before we get used to code like this in Kibana if go this route though 😛

if (in.getVersion().onOrAfter(Version.V_6_5_0)) {
    builder.mappingVersion(in.readVLong());
} else {
    builder.mappingVersion(1);
}
if (in.getVersion().onOrAfter(Version.V_6_5_0)) {
    builder.settingsVersion(in.readVLong());
} else {
    builder.settingsVersion(1);
}
if (in.getVersion().onOrAfter(Version.V_7_2_0)) {
    builder.aliasesVersion(in.readVLong());
} else {
    builder.aliasesVersion(1);
}
builder.setRoutingNumShards(in.readInt());

[kobelb]

Yeah, that's true. But what is the expected lifetime of these version checks? Won't we have to remove them at some point anyway?

Yeah, they'll have to be removed at some point. I think we'd be safe removing them after we release a major version. An argument could be made to keep them around for longer, as we still support 7.last.x during the entirely of 8.x for "critical bugfixes", but since backports are a lot less common now, I don't know if we need to go that far.

Also do you happen to know if these checks are optimized out when compiling ES for a particular version somehow (I suspect no, since I see these checks are also used for deprecation warnings as well)?

AFAIK, they aren't compiled out as Java doesn't conditional compilation.

Ah, that's much better, thanks! It will take some time before we get used to code like this in Kibana if go this route though 😛

It's super simplistic, but it doesn't seem that bad to provide the facility to do version checking. I think the real complexity comes with the use of the version checking: 00724de

[cjcenizal]

The ES UI team recently got bit by this branch-divergence problem (#60508). We got lucky and caught it in time but it could have easily shipped.

I see a lot of promise in Brandon's approach. Clearly, tests would catch the problem (especially functional/e2e tests in the case I linked to), but I think Brandon's suggestion nails the task of communicating this divergence, which I think is the underlying problem. What I like about this approach is:

• Anyone skimming the code will be able to spot the version-dependent branching logic easily. Even if they're unfamiliar with the code they'll see something weird is going on and will know what to look for to make sure the backport is correct. This will also make it easier for people to know when is the right time to remove this branching logic.
• The consistent pattern also makes it easy to grep for the codebase and find other places where 7.x and master diverge. It becomes easy to answer the question "Where are the version-divergence landmines?"

[legrego]

@azasypkin are we able to close this given our recent decision to support these deprecated SSO routes in 8.x?

[kibanamachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request
• Commit: 14b86f2

History

• 💚 Build #21540 succeeded 14b86f2
• 💚 Build #17710 succeeded 73128d7
• 💚 Build #17548 succeeded d77efc128f3edf1188161ff9f5c30e2acd9145f5
• 💚 Build #17217 succeeded 592857e5cb0685b848fdc7c1df45f519cdc6209e
• 💚 Build #15069 succeeded 475408c4104ae3f262fc1f47da95dfec13b2c5a2

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

[azasypkin]

@azasypkin are we able to close this given our recent decision to support these deprecated SSO routes in 8.x?

Yes, thanks for the reminder.