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

Deviation changing leafref to another type still triggers leafref validation. #771

Open
robshakir opened this issue Nov 23, 2021 · 5 comments · May be fixed by #772
Open

Deviation changing leafref to another type still triggers leafref validation. #771

robshakir opened this issue Nov 23, 2021 · 5 comments · May be fixed by #772

Comments

@robshakir
Copy link

Considering this module:

module a {
	prefix "a";
	namespace "urn:a";

	container a {
		leaf b {
			type leafref {
				path "/d"; // does not exist
			}
		}
	}

	deviation "/a/b" {
		deviate replace {
			type string;
		}
	}
}

the deviate replace changes the type for /a/b to string, but pyang will still fail reporting that path /d doesn't exist. The replace is successful, but since i_leafref remains set, then the validation continues to check the leafref characteristics.

Opening this issue to get an issue number to create the test demonstrating the fix here.

Note, this fix appears to need a new validation phase to be added, since reference_1 runs over statements with children, unless I've misunderstood something. Happy to take alternate suggestions as to the fix. I'll open a PR.

robshakir added a commit to robshakir/pyang that referenced this issue Nov 23, 2021
 * (M) pyang/statements.py
   - Add a validation phase where changes to types with references
     by deviations can be made ahead of validating any reference.
   - Remove the `i_leafref` attribute when a deviation changes the
     type of a leaf to a type that isn't a leafref.
 * (A) test/test_issues/test_i771
   - Add a test case validating the fix to issue mbj4668#771.
@robshakir robshakir linked a pull request Nov 23, 2021 that will close this issue
@mbj4668
Copy link
Owner

mbj4668 commented Nov 24, 2021

Hmm, I don't think is an accurate description on what's going on. pyang validates the leafref before applying deviations.

Consider this version of the module:

module a {
  namespace "urn:a";
  prefix a;

  container a {
    leaf b {
      type leafref {
        path "/d"; // does not exist
      }
    }
  }
  leaf d {
    type int8;
  }
  deviation "/d" {
    deviate not-supported;
  }
  deviation "/a/b" {
    deviate replace {
      type string;
    }
  }
}

If the i_leafref property was not deleted, this module would fail to validate.

I don't think that this is in fact a bug. RFC 7950 says about deviate:

   The "deviate" statement defines how the server's implementation of
   the target node deviates from its original definition.

So this means that you have an original model, which then an implementation can choose to deviate from. Hence, the original model must be valid in itself. This is why pyang first validates the references, and then applies the deviations.

Do you have a use case for this in mind?

@robshakir
Copy link
Author

robshakir commented Nov 24, 2021

That is indeed an interesting case. But I think it is valid using the same logic. The module is valid once all its statements have been considered. 'd' is removed and then never referenced. I'm not clear we should expect a half parsed module to be valid in and of itself. Of course this - the module deviating itself - is not the common case. We expect that the server publishes deviations authored the implementor of the server to a module that is not written by the same author.

The scenario that this is used for is more nuanced than the test module. We have a structure that is used within a wider context - namely "openconfig-aft" that is used within "openconfig-network-instance". The AFT module has an absolute leafref to '/network-instances/network-instance/config/name' within a grouping - let's call it 'aft-top'.

We want to create a module which is just the subtree defined in 'aft-top' for code generation purposes. To do this we have a 'uses aft-top' and then a number of 'deviate replace' statements to replace the absolute leafrefs with the type that they target.

In this case, I argue that the original module is valid - since the original module is the sum of the 'uses' and 'deviate' statements.

@robshakir
Copy link
Author

Sorry. I hit send too early here - the gap in the specification here is really around existence of 'deviate' statements that modify the contents of data nodes defined within the same module. I can't find anything in 7950 that says this construct is invalid.

How do we solve this otherwise - one needs to apply a patch to the module. You can see this in https://github.com/openconfig/gribi where we apply a patch before generating protobuf - this solution seems significantly cleaner since it keeps the changes within the source YANG itself.

@robshakir
Copy link
Author

Friendly ping here -- this issue is causing us some upstream issues. I'd like to discuss the above further if there's disagreement here. :-) Thanks & Happy New Year!

@mbj4668
Copy link
Owner

mbj4668 commented Dec 30, 2021

It was never the intention that deviations would be used in this way. But setting that aside for a moment, the proposed fix is not correct, since it rejects the module in my example above. Also, it rejects the following variant of your original module:

module a {
  namespace "urn:a";
  prefix a;

  typedef dref {
    type leafref {
      path "/d"; // does not exist
    }
  }   

  container a {
    leaf b {
      type dref;
    }
  }
  deviation "/a/b" {
    deviate replace {
      type string;
    }
  }
}

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 a pull request may close this issue.

2 participants