Skip to content
This repository has been archived by the owner on Dec 16, 2022. It is now read-only.

Fix bug in vtgate when running with keyspace filtering #199

Merged
merged 5 commits into from
Mar 15, 2021

Conversation

setassociative
Copy link

@setassociative setassociative commented Mar 10, 2021

Description

This checks if a vtgate is currently filtering keyspaces before requesting the TopoServer. This is necessary because a TopoServer can't be accessed in those cases as the filtered Topo in those cases could make it unsafe to make writes since all reads would be returning a subset of the actual topo data.

The only use of the requested topoServer that I found was in the DDL handling path and was introduced in vitessio#6547.

This is deployed on dev but should get testing (endtoend or unit, unclear on best path atm) before going upstream.

Signed-off-by: Richard Bailey <rbailey@slack-corp.com>
Signed-off-by: Richard Bailey <rbailey@slack-corp.com>
Signed-off-by: Richard Bailey <rbailey@slack-corp.com>
Signed-off-by: Richard Bailey <rbailey@slack-corp.com>
@setassociative setassociative changed the title [DRAFT] Fix bug in vtgate when running with keyspace filtering Mar 15, 2021
@setassociative setassociative marked this pull request as ready for review March 15, 2021 17:27
@rafael
Copy link

rafael commented Mar 15, 2021

This makes sense to me. In other words, if I'm reading this correctly: if you have filters on, you can't safely access the topo because it doesn't have the full view.

For the context of online DDL's, you need the full topo, so you need to make sure that you run them from a vtgate that is not filtering.

@@ -477,6 +490,9 @@ func (vc *vcursorImpl) TabletType() topodatapb.TabletType {

// SubmitOnlineDDL implements the VCursor interface
func (vc *vcursorImpl) SubmitOnlineDDL(onlineDDl *schema.OnlineDDL) error {
if vc.topoServer == nil {
return vterrors.New(vtrpcpb.Code_INTERNAL, "Unable to apply DDL, missing toposerver in vcursor")
Copy link

Choose a reason for hiding this comment

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

one nit - this error might be too cryptic for an end user. I wonder if we should mention that probably they have a keyspace filter and that is incompatible with online DDLs

Copy link
Author

Choose a reason for hiding this comment

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

Fair enough -- I referenced filtered keyspaces in the error but since it's a bit indirect i'm going to leave the general commentary around why the failure is happening as well.

@setassociative
Copy link
Author

setassociative commented Mar 15, 2021

For the context of online DDL's, you need the full topo, so you need to make sure that you run them from a vtgate that is not filtering.

More or less. Technically I don't actually think it needs the full topo, the impl for writing the request into topo looks like:

func (onlineDDL *OnlineDDL) WriteTopo(ctx context.Context, conn topo.Conn, basePath string) error {
	if onlineDDL.UUID == "" {
		return fmt.Errorf("onlineDDL UUID not found; keyspace=%s, sql=%s", onlineDDL.Keyspace, onlineDDL.SQL)
	}
	bytes, err := onlineDDL.ToJSON()
	if err != nil {
		return fmt.Errorf("onlineDDL marshall error:%s, keyspace=%s, sql=%s", err.Error(), onlineDDL.Keyspace, onlineDDL.SQL)
	}
	_, err = conn.Create(ctx, fmt.Sprintf("%s/%s", basePath, onlineDDL.UUID), bytes)
	if err != nil {
		return fmt.Errorf("onlineDDL topo create error:%s, keyspace=%s, sql=%s", err.Error(), onlineDDL.Keyspace, onlineDDL.SQL)
	}
	return nil
}

which has a limited impact -- it's writing only to the schema-migration/requests/<uuid> path which shouldn't be filtered... but the actual topo.Server interface doesn't have any kind of guard rails around what's being written when you request a writeable interface so the keyspace filtering stuff blocks access to it.

@setassociative
Copy link
Author

setassociative commented Mar 15, 2021

Taking your 👍 as a Ship it to merge this branch down, feel free to claw it back if we don't want to roll this out

@setassociative setassociative merged commit 845715b into slack-vitess-8-2021.03.03r0.wip Mar 15, 2021
setassociative added a commit that referenced this pull request Mar 17, 2021
This checks if a vtgate is currently filtering keyspaces before requesting the TopoServer. This is necessary because a TopoServer can't be accessed in those cases as the filtered Topo in those cases could make it unsafe to make writes since all reads would be returning a subset of the actual topo data.

The only use of the requested topoServer that I found was in the DDL handling path and was introduced in vitessio#6547.

This is deployed on dev but should get testing (endtoend or unit, unclear on best path atm) before going upstream.
setassociative added a commit that referenced this pull request Apr 15, 2021
This checks if a vtgate is currently filtering keyspaces before requesting the TopoServer. This is necessary because a TopoServer can't be accessed in those cases as the filtered Topo in those cases could make it unsafe to make writes since all reads would be returning a subset of the actual topo data.

The only use of the requested topoServer that I found was in the DDL handling path and was introduced in vitessio#6547.

This is deployed on dev but should get testing (endtoend or unit, unclear on best path atm) before going upstream.
# Conflicts:
#	go/vt/vtgate/vcursor_impl.go

Signed-off-by: Richard Bailey <rbailey@slack-corp.com>
ajm188 pushed a commit that referenced this pull request Jun 16, 2021
This checks if a vtgate is currently filtering keyspaces before requesting the TopoServer. This is necessary because a TopoServer can't be accessed in those cases as the filtered Topo in those cases could make it unsafe to make writes since all reads would be returning a subset of the actual topo data.

The only use of the requested topoServer that I found was in the DDL handling path and was introduced in vitessio#6547.

This is deployed on dev but should get testing (endtoend or unit, unclear on best path atm) before going upstream.
# Conflicts:
#	go/vt/vtgate/vcursor_impl.go

Signed-off-by: Richard Bailey <rbailey@slack-corp.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants