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.

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

OpenAPI generic_mount_paths follow-up #18663

Merged
merged 4 commits into from
Jan 18, 2023

Conversation

maxb
Copy link
Contributor

@maxb maxb commented Jan 11, 2023

An incremental improvement within larger context discussed in #18560.

  • Following the revert in Revert "Add mount path into the default generated openapi.json spec (#17926)" #18617, re-introduce the change from {mountPath} to {<path-of-mount>_mount_path}; this is needed, as otherwise paths from multiple plugins would clash - e.g. almost every auth method would provide a conflicting definition for auth/{mountPath}/login, and the last one written into the map would win.

  • Move the half of the functionality that was in sdk/framework/ to vault/logical_system.go with the rest; this is needed, as sdk/framework/ gets compiled in to externally built plugins, and therefore there may be version skew between it and the Vault main code. Implementing the generic_mount_paths feature entirely on one side of this boundary frees us from problems caused by this.

  • Update the special exception that recognizes system and identity as singleton mounts to also include the other two singleton mounts, cubbyhole and auth/token.

  • Include a comment that documents to restricted circumstances in which the generic_mount_paths option makes sense to use:

      // Note that for this to actually be useful, you have to be using it with
      // a Vault instance in which you have mounted one of each secrets engine
      // and auth method of types you are interested in, at paths which identify
      // their type, and for the KV secrets engine you will probably want to
      // mount separate kv-v1 and kv-v2 mounts to include the documentation for
      // each of those APIs.
    

An incremental improvement within larger context discussed in hashicorp#18560.

* Following the revert in hashicorp#18617, re-introduce the change from
  `{mountPath}` to `{<path-of-mount>_mount_path}`; this is needed, as
  otherwise paths from multiple plugins would clash - e.g. almost every
  auth method would provide a conflicting definition for
  `auth/{mountPath}/login`, and the last one written into the map would
  win.

* Move the half of the functionality that was in `sdk/framework/` to
  `vault/logical_system.go` with the rest; this is needed, as
  `sdk/framework/` gets compiled in to externally built plugins, and
  therefore there may be version skew between it and the Vault main
  code. Implementing the `generic_mount_paths` feature entirely on one
  side of this boundary frees us from problems caused by this.

* Update the special exception that recognizes `system` and `identity`
  as singleton mounts to also include the other two singleton mounts,
  `cubbyhole` and `auth/token`.

* Include a comment that documents to restricted circumstances in which
  the `generic_mount_paths` option makes sense to use:

	    // Note that for this to actually be useful, you have to be using it with
	    // a Vault instance in which you have mounted one of each secrets engine
	    // and auth method of types you are interested in, at paths which identify
	    // their type, and for the KV secrets engine you will probably want to
	    // mount separate kv-v1 and kv-v2 mounts to include the documentation for
	    // each of those APIs.
// hardcoded default paths. For example /auth/approle/login would be replaced
// with /auth/{mountPath}/login. This will be replaced for all secrets
// engines and auth methods that are enabled.
genericMountPaths, _ := req.Get("genericMountPaths").(bool)
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for removing this! 🎉

Comment on lines +4514 to +4515
// mount separate kv-v1 and kv-v2 mounts to include the documentation for
// each of those APIs.
Copy link
Contributor

Choose a reason for hiding this comment

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

"documentation" might be a bit confusing especially with "will primarily be used for code generation purposes" mentioned above.

@averche
Copy link
Contributor

averche commented Jan 12, 2023

Thank you very much for implementing this!

I really like the idea of moving the mount-path-specific code out of sdk, it will definitely address a number of the problems you mentioned in #18560.

  • Update the special exception that recognizes system and identity as singleton mounts to also include the other two singleton mounts, cubbyhole and auth/token.

This is a great fix too!

I tried running gen_openapi.sh with generic_mount_paths=true and it works! There are still a few issues that need to be fixed, but I'd be happy to continue on this in future PR's.

    "/auth/{alicloud_mount_path}/login": {
      "description": "Authenticates an RAM entity with Vault.",
      "parameters": [
        {
          "name": "alicloud_mount_path",
          "description": "Path that the backend was mounted at",
          "in": "path",
          "schema": {
            "type": "string"            // needs a default value
          },
          "required": true              // needs to be false for generators to work properly
        }
      ],
      "x-vault-unauthenticated": true,
      "post": {
        "summary": "Authenticates an RAM entity with Vault.",
        "operationId": "postAuthAlicloud_mount_pathLogin",  // will need to fix this, possibly as part of #18321
        "tags": [
          "auth"
        ],

@averche
Copy link
Contributor

averche commented Jan 12, 2023

It appears some mount-path-related tests are currently failing, probably need to be updated.

Also remove comment "// TODO update after kv repo update" which was
added 4 years ago in hashicorp#5687 - the implied update has not happened.
@maxb
Copy link
Contributor Author

maxb commented Jan 13, 2023

Ah, yes - test expectations now updated accordingly.

@averche
Copy link
Contributor

averche commented Jan 16, 2023

Thanks! Could you please add a changelog file and we can merge this one.

@maxb
Copy link
Contributor Author

maxb commented Jan 17, 2023

Changelog added ... it's a bit long, but there's quite a lot going on here!

@averche averche merged commit 04b2461 into hashicorp:main Jan 18, 2023
@maxb maxb deleted the openapi-generic-mount-paths-followup branch January 18, 2023 07:37
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.

None yet

2 participants